Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SDN 2316: Use GatewayConfig in OVN-K to set gateway modes #1209

Merged
merged 2 commits into from
Dec 14, 2021

Conversation

tssurya
Copy link
Contributor

@tssurya tssurya commented Oct 25, 2021

Depends On openshift/api#1033 and #1242

In this PR, we are doing two things:

  • Removing the exclusiveness between EgressIPs and ICNI
  • Removing the hidden config-map and using the new api routingViaHost to configure gateway modes.

We also handle the upgrade logic of populating the new API field in the cluster CRD based on the gateway-mode-config map values. Also note that this enables users to migrate between the two gateway modes at runtime.

@tssurya tssurya force-pushed the add-gm-config-ovnk branch from b9996f3 to 7c7f406 Compare October 25, 2021 07:56
@tssurya tssurya force-pushed the add-gm-config-ovnk branch 2 times, most recently from 81be268 to b009be0 Compare November 25, 2021 06:25
@tssurya
Copy link
Contributor Author

tssurya commented Nov 25, 2021

I have tested this PR against a cluster and it works as expected.

spec:
  clusterNetwork:
  - cidr: 10.128.0.0/14
    hostPrefix: 23
  defaultNetwork:
    ovnKubernetesConfig:
      gatewayConfig:
        routingViaHost: true
      genevePort: 6081
      mtu: 8901
      policyAuditConfig:
        destination: "null"
        maxFileSize: 50
        rateLimit: 20
        syslogFacility: local0
    type: OVNKubernetes

It rolls out local gateway as soon as we change the gatewayConfig and add the routingViaHost config parameter.

@tssurya
Copy link
Contributor Author

tssurya commented Nov 26, 2021

/test e2e-agnostic-upgrade
/test e2e-gcp-ovn-upgrade

@tssurya tssurya changed the title [DNM] [SDN 2316] Add gm config ovnk [SDN 2316] Add gm config ovnk Nov 29, 2021
@tssurya tssurya changed the title [SDN 2316] Add gm config ovnk SDN 2316 Add gm config ovnk Dec 6, 2021
@tssurya tssurya changed the title SDN 2316 Add gm config ovnk SDN 2316: Add gm config ovnk Dec 6, 2021
@tssurya tssurya changed the title SDN 2316: Add gm config ovnk SDN 2316: Use GatewayConfig in OVN-K to set gateway modes Dec 6, 2021
@tssurya tssurya force-pushed the add-gm-config-ovnk branch 3 times, most recently from cfe1bf8 to 8ada60b Compare December 6, 2021 18:14
@tssurya
Copy link
Contributor Author

tssurya commented Dec 6, 2021

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 6, 2021
@tssurya tssurya force-pushed the add-gm-config-ovnk branch 6 times, most recently from 2c91d1f to b081fb0 Compare December 9, 2021 19:32
@tssurya
Copy link
Contributor Author

tssurya commented Dec 9, 2021

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 9, 2021
@tssurya tssurya force-pushed the add-gm-config-ovnk branch from b081fb0 to 2c21678 Compare December 9, 2021 19:51
@tssurya
Copy link
Contributor Author

tssurya commented Dec 10, 2021

/retest

Let's remove the support for hidden config-map
that was used to set gatewaymode values and
instead use the new API `routingViaHost`. This
PR also handles the upgrade logic for existing
clusters and populates the routingViaHost field
based on previous cluster state.
@tssurya
Copy link
Contributor Author

tssurya commented Dec 10, 2021

/assign @trozet @astoycos : PTAL

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 10, 2021

@tssurya: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-vsphere-ovn 5ee9f36 link false /test e2e-vsphere-ovn
ci/prow/e2e-ovn-hybrid-step-registry 5ee9f36 link false /test e2e-ovn-hybrid-step-registry

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@tssurya
Copy link
Contributor Author

tssurya commented Dec 13, 2021

/retest-required

if modeOverride == OVN_LOCAL_GW_MODE {
routeViaHost = true
}
conf.Spec.DefaultNetwork.OVNKubernetesConfig.GatewayConfig = &operv1.GatewayConfig{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't actually update the config map right? Do we need to update the config map from here so that when we remove the ability to use the hidden configmap we dont break users? Or should we print some warning or event to tell the user they need to update the gatewayConfig in their configmap?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do update the configmap when this function returns. Let me explain:

  1. case operv1.NetworkTypeOVNKubernetes:
  2. func bootstrapOVN(conf *operv1.Network, kubeClient client.Client) (*bootstrap.BootstrapResult, error) {
  3. func bootstrapOVNConfig(kubeClient client.Client) (*bootstrap.OVNConfigBoostrapResult, error) {
  4. Within bootstrapOVNConfig we do our thing.
  5. Then we return all the way back to the bootstrap call
    bootstrapResult, err := network.Bootstrap(newOperConfig, r.client)
  6. Aaaand this is where we update the CRD:
    if !reflect.DeepEqual(operConfig, newOperConfig) {
    if err := r.UpdateOperConfig(newOperConfig); err != nil {

Hence any changes done to the CRD during bootstrap gets written into the CRD. After all that the rendering starts and it regards the updated CRD the source of truth.

I tested it using the steps mentioned in https://docs.google.com/document/d/1SYaNQNW4sWVTE9XgHG6gKsS4ZActK8_Z7wILXU5YuoU/edit#heading=h.d5s838mpuww0 in a cluster-bot cluster and it works well. The CRD does get updated to something like:

Gateway Config:{
    Routing Via Host: true
}

@tssurya
Copy link
Contributor Author

tssurya commented Dec 14, 2021

/test e2e-gcp-ovn-upgrade

Copy link
Contributor

@trozet trozet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 14, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 14, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: trozet, tssurya

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 14, 2021
@openshift-merge-robot openshift-merge-robot merged commit ee498c6 into openshift:master Dec 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants