Skip to content

Bug 1881979: Fixes gateway mode parameters for OVN#801

Merged
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
trozet:fix_ovn_gateway_modes
Sep 23, 2020
Merged

Bug 1881979: Fixes gateway mode parameters for OVN#801
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
trozet:fix_ovn_gateway_modes

Conversation

@trozet
Copy link
Contributor

@trozet trozet commented Sep 22, 2020

We have now switched back to using local gateway mode in ovn-kubernetes.
This is currently forced in ovn-k8s with a hack. This patch sets it
correctly in CNO and we will revert the hack in ovn-k8s.

Additionally, this passes the gateway-interface of "none" when
ovs-configuration is not available. This allows us to do upgrade via

openshift/ovn-kubernetes#281

Signed-off-by: Tim Rozet trozet@redhat.com

@trozet
Fixes gateway mode parameters for OVN
2fb40b2 
We have now switched back to using local gateway mode in ovn-kubernetes.
This is currently forced in ovn-k8s with a hack. This patch sets it
correctly in CNO and we will revert the hack in ovn-k8s.

Additionally, this passes the gateway-interface of "none" when
ovs-configuration is not available. This allows us to do upgrade via

openshift/ovn-kubernetes#281

Signed-off-by: Tim Rozet <trozet@redhat.com>
@trozet trozet mentioned this pull request Sep 22, 2020
Merged
@abhat
Copy link
Contributor

abhat commented Sep 22, 2020

/lgtm
/approve

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Sep 22, 2020

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Details 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-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 22, 2020
@openshift-bot
Copy link
Contributor

openshift-bot commented Sep 22, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@trozet trozet mentioned this pull request Sep 22, 2020
Merged
@openshift-bot
Copy link
Contributor

openshift-bot commented Sep 22, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

10 similar comments
@openshift-bot
Copy link
Contributor

openshift-bot commented Sep 22, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Sep 22, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Sep 22, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Sep 23, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Sep 23, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Sep 23, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Sep 23, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Sep 23, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Sep 23, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Sep 23, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

danwinship
danwinship reviewed Sep 23, 2020
View reviewed changes
Copy link
Contributor

@danwinship danwinship left a comment •
edited
Loading

Choose a reason for hiding this comment

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

So the idea is that in the 4.5 node case, this would make ovn-kubernetes move eth0 into br-ex itself rather than us doing it in ovs-configuration.service, but we expect that to work fine for this limited case for the duration of the upgrade?

Considering all the cases:

  • 4.5 node, 4.5 CNO, 4.5 OVS - obviously that works
  • 4.5 node, 4.6 CNO, 4.5 OVS - CNO will specify --gateway-mode local (because the node is 4.5), OVS will use old-style local gateway mode (because it's 4.5), it works
  • 4.5 node, 4.5 CNO, 4.6 OVS - CNO will specify --gateway-mode local (because it's 4.5), OVS will use new-style local gateway mode and manually move eth0 into br-ex, it works
  • 4.5 node, 4.6 CNO, 4.6 OVS - CNO will specify --gateway-mode local (because the node is 4.5), OVS will use new-style local gateway mode and manually move eth0 into br-ex, it works
  • 4.6 node, either CNO or OVS is 4.5 - this wouldn't work but we don't believe that can happen
  • 4.6 node, 4.6 CNO, 4.6 OVS - obviously that works

bindata/network/ovn-kubernetes/ovnkube-master.yaml
gateway_mode_flags="--gateway-mode local --gateway-interface br-ex"
else
gateway_mode_flags="--gateway-mode local"
gateway_mode_flags="--gateway-mode local --gateway-interface none"
Copy link
Contributor

@danwinship danwinship Sep 23, 2020

Choose a reason for hiding this comment

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

When the docs talk about "If none specified", it means "if you don't specify it", not "if you specify none". This will tell it to look for an interface named none and fail if it's not found.

Copy link
Contributor Author

@trozet trozet Sep 23, 2020

Choose a reason for hiding this comment

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

which docs?

@danwinship
Copy link
Contributor

danwinship commented Sep 23, 2020

  • 4.5 node, 4.6 CNO, 4.5 OVS - CNO will specify --gateway-mode local (because the node is 4.5), OVS will use old-style local gateway mode (because it's 4.5), it works
  • 4.5 node, 4.5 CNO, 4.6 OVS - CNO will specify --gateway-mode local (because it's 4.5), OVS will use new-style local gateway mode and manually move eth0 into br-ex, it works

oh... also, those can't happen. I mean, we could have a cluster with a 4.6 CNO and 4.5 OVN, but the OVN pods in that case would be using the configuration that had been generated by the 4.5 CNO still. And there's no way to have 4.5 CNO and 4.6 OVN at all

@trozet
Copy link
Contributor Author

trozet commented Sep 23, 2020

So the idea is that in the 4.5 node case, this would make ovn-kubernetes move eth0 into br-ex itself rather than us doing it in ovs-configuration.service, but we expect that to work fine for this limited case for the duration of the upgrade?

No, the idea here is we don't want ovn-kubernetes to move any interface into OVS. in a 4.5-> 4.6 upgrade case, CNO gets upgraded before MCO. Meaning that this will happen:

  1. 4.5 Cluster starts upgrade
  2. CNO gets upgraded, which rolls out 4.6 ovn-kubernetes pods
  3. Normally ovnkube-node starts (with new local gw) will attempt to move the interface cause there is no br-ex. However because "none" was specified as the gateway-interface, it knows not to do that part. So no interface is moved, but local gw will still function.
  4. MCO rolls out upgrades per node asynchronously. Eventually the node gets rebooted with machine content change including ovs-configuration service.
  5. ovs-configuration service runs on the reboot, before kubelet ever comes up and migrates the interface into br-ex
  6. kubelet starts with ovnkube-node, local gw continues to function with "none"
  7. CNO refreshes and changes the args to ovnkube-node to be gateway-interface "br-ex" (because ovs-configuration is now present on the machine)
  8. ovnkube-node restarts with gateway-interface br-ex, and then sets up the OVN GR and other "shared" topology components

@danwinship
Copy link
Contributor

danwinship commented Sep 23, 2020

oh, sorry, I missed that there was a linked ovn-kubernetes PR

@trozet trozet changed the title Fixes gateway mode parameters for OVN Bug 1881979: Fixes gateway mode parameters for OVN Sep 23, 2020
@openshift-ci-robot openshift-ci-robot added bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Sep 23, 2020
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Sep 23, 2020

@trozet: This pull request references Bugzilla bug 1881979, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.6.0) matches configured target release for branch (4.6.0)
  • bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)
Details

In response to this:

Bug 1881979: Fixes gateway mode parameters for OVN

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.

@openshift-merge-robot openshift-merge-robot merged commit fb76143 into openshift:master Sep 23, 2020
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Sep 23, 2020

@trozet: All pull requests linked via external trackers have merged:

Bugzilla bug 1881979 has been moved to the MODIFIED state.

Details

In response to this:

Bug 1881979: Fixes gateway mode parameters for OVN

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@danwinship danwinship danwinship left review comments

@dcbw dcbw Awaiting requested review from dcbw

@juanluisvaladas juanluisvaladas Awaiting requested review from juanluisvaladas

Assignees

@abhat abhat

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@trozet @abhat @openshift-ci-robot @openshift-bot @danwinship @openshift-merge-robot