Skip to content

NP-764: Enable no uplink OVN-K gateway bridge mode#1692

Merged
openshift-merge-robot merged 2 commits intoopenshift:mainfrom
pliurh:dummy_gw
Jun 28, 2023
Merged

NP-764: Enable no uplink OVN-K gateway bridge mode#1692
openshift-merge-robot merged 2 commits intoopenshift:mainfrom
pliurh:dummy_gw

Conversation

@pliurh
Copy link
Copy Markdown
Contributor

@pliurh pliurh commented Apr 20, 2023

Use no uplink OVN-K gateway bridge to decouple br-ex from host physical interface.
Use ovs-vsctl to create br-ex bridge instead of NM, as we don't need to run DHCP client for br-ex interface anymore.

@pliurh pliurh changed the title WIP WIP: Use dummy interface in br-ex bridge Apr 20, 2023
@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 20, 2023
@openshift-ci openshift-ci Bot requested review from ggiguash and pacevedom April 20, 2023 13:38
@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 20, 2023
@dhellmann
Copy link
Copy Markdown
Contributor

@pliurh I think that if you rebase this PR on the HEAD of the main branch we will get better results from the CI jobs.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 25, 2023
@pliurh pliurh force-pushed the dummy_gw branch 2 times, most recently from 3745816 to 181e9fe Compare April 27, 2023 04:02
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 27, 2023
@pliurh
Copy link
Copy Markdown
Contributor Author

pliurh commented May 4, 2023

/retest

@pliurh pliurh changed the title WIP: Use dummy interface in br-ex bridge WIP: Enable no uplink OVN-K gateway bridge mode Jun 9, 2023
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 9, 2023
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 9, 2023
@pliurh
Copy link
Copy Markdown
Contributor Author

pliurh commented Jun 9, 2023

/retest

1 similar comment
@pliurh
Copy link
Copy Markdown
Contributor Author

pliurh commented Jun 12, 2023

/retest

@anuragthehatter
Copy link
Copy Markdown

anuragthehatter commented Jun 13, 2023

/hold

This is failing on QE Ushift clusters causing node to go into NOTREADY state with error
container runtime network not ready: NetworkReady=false reason:NetworkPluginNotReady message:Network plugin returns error: No CNI configuration file in /etc/cni/net.d/.

10-ovn-kubernetes.conf is absent under /etc/cni/net.d/

@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 Jun 13, 2023
@pliurh
Copy link
Copy Markdown
Contributor Author

pliurh commented Jun 19, 2023

/retest

1 similar comment
@pmtk
Copy link
Copy Markdown
Member

pmtk commented Jun 19, 2023

/retest

@pmtk
Copy link
Copy Markdown
Member

pmtk commented Jun 19, 2023

Today's rebase brought in changes for amd64 (it seems that we already had them for arm - introduced in 2023-06-15-173508, we had 2023-06-16-015443)

- ovn-kubernetes image-amd64 23c67c62ed1d912387791a1d8eb422b367c422dc to 5217351470aaa77bcfca04a1b6ee1ebf2bfbe7d9
  - eacdaa7 2023-06-13T09:44:38+02:00 Validate port before deleting conntrack flow
  - d829d7d 2023-06-13T09:44:21+02:00 Use no-uplink gateway bridge in compact-mode e2e test
  - f764a79 2023-06-13T09:44:16+02:00 Allow external gateway bridge without uplink port In local gateway mode

@pmtk
Copy link
Copy Markdown
Member

pmtk commented Jun 19, 2023

some infra problems
/retest

@pmtk
Copy link
Copy Markdown
Member

pmtk commented Jun 19, 2023

/test e2e-openshift-conformance-reduced

@pliurh pliurh changed the title WIP: Enable no uplink OVN-K gateway bridge mode Enable no uplink OVN-K gateway bridge mode Jun 19, 2023
@pliurh
Copy link
Copy Markdown
Contributor Author

pliurh commented Jun 25, 2023

Several other comments besides the inline ones:

  1. shall we deprecate the "gatewayInterface" in this PR?

Yes.

  1. we should no longer need to listen on the "br-ex" interface in the mdns controller.

Agree.

  1. do we "need to" consider upgrade from default ovnk to no-uplink ovnk?

The configure-ovs.sh will handle the upgrade scenario. It will remove the NM config files and the OVS bridges and ports, then create br-ex with ovs-vsctl command.

@pliurh pliurh force-pushed the dummy_gw branch 2 times, most recently from 0f181ab to baefef1 Compare June 25, 2023 06:05
Comment thread pkg/config/ovn/ovn.go Outdated
Comment thread pkg/node/netconfig.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In a fresh deployment, the service address will always be assigned to loopback device since this node network configuration runs before the InfrastructureServices controller (where CNI is started). Is it expected?

@pliurh
Copy link
Copy Markdown
Contributor Author

pliurh commented Jun 26, 2023

/retest

@zshi-redhat
Copy link
Copy Markdown
Contributor

@jcaamano Could you also take a look?
This is about integrating no-uplink ovnk gateway change in microshift, specifically the configure-ovs.sh script is updated to handle upgrade from uplink NM settings to no-uplink NM settings.

@pliurh pliurh changed the title Enable no uplink OVN-K gateway bridge mode NP-764: Enable no uplink OVN-K gateway bridge mode Jun 26, 2023
@openshift-ci-robot
Copy link
Copy Markdown

openshift-ci-robot commented Jun 26, 2023

@pliurh: This pull request references NP-764 which is a valid jira issue.

Details

In response to this:

Use no uplink OVN-K gateway bridge to decouple br-ex from host physical interface.
Use ovs-vsctl to create br-ex bridge instead of NM, as we don't need to run DHCP client for br-ex interface anymore.

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-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jun 26, 2023
@pliurh
Copy link
Copy Markdown
Contributor Author

pliurh commented Jun 26, 2023

/unhold

@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 Jun 26, 2023
Comment thread packaging/systemd/configure-ovs.sh Outdated
@jcaamano
Copy link
Copy Markdown

@jcaamano Could you also take a look? This is about integrating no-uplink ovnk gateway change in microshift, specifically the configure-ovs.sh script is updated to handle upgrade from uplink NM settings to no-uplink NM settings.

That part LGTM, just that small comment

Copy link
Copy Markdown
Member

@pmtk pmtk left a comment

Choose a reason for hiding this comment

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

LGTM

I also verified upgrade from 4.13 to (this+#1957) and worked perfectly

@pliurh
Copy link
Copy Markdown
Contributor Author

pliurh commented Jun 27, 2023

/retest

Comment thread assets/components/ovn/single-node/master/daemonset.yaml Outdated
pliurh added 2 commits June 27, 2023 11:27
Use no uplink OVN-K gateway bridge to decouple br-ex from host
physical interface.
Use ovs-vsctl to create br-ex bridge instead of NM, as we don't
need run DHCP client for br-ex interface anymore.

Signed-off-by: Peng Liu <pliu@redhat.com>
Signed-off-by: Peng Liu <pliu@redhat.com>
@zshi-redhat
Copy link
Copy Markdown
Contributor

/retest

1 similar comment
@pmtk
Copy link
Copy Markdown
Member

pmtk commented Jun 27, 2023

/retest

@zshi-redhat
Copy link
Copy Markdown
Contributor

/test e2e-openshift-conformance-reduced-arm

Copy link
Copy Markdown
Contributor

@dhellmann dhellmann left a comment

Choose a reason for hiding this comment

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

This version looks good and seems to be passing the tests now that the topolvm image issue is resolved. I have one comment about some documentation, but that can (and probably should) be addressed in a simple follow-up PR, rather than requiring another iteration on this one.

The ARM tests are still a bit flakey for reasons outside of our control, so if those fail we can still merge this, unless you have reason to think the failure is related to the code.

@zshi-redhat do you want to apply the LGTM when you're happy?

Comment thread pkg/node/netconfig.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The details in this comment thread are an important part of the design. Are they written down somewhere? If not, could you add them in a comment in this function, please? You can use a follow-up PR, since this one looks like it is almost ready to be merged.

@zshi-redhat
Copy link
Copy Markdown
Contributor

This version looks good and seems to be passing the tests now that the topolvm image issue is resolved. I have one comment about some documentation, but that can (and probably should) be addressed in a simple follow-up PR, rather than requiring another iteration on this one.

@pliurh please file a followup PR for the documentation change.

The ARM tests are still a bit flakey for reasons outside of our control, so if those fail we can still merge this, unless you have reason to think the failure is related to the code.

agree, looking at the job history, it has 4 pass in May and zero pass in June. And the failed tests are different in each run. I will go ahead and lgtm the PR.

@zshi-redhat do you want to apply the LGTM when you're happy?

/lgtm

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Jun 28, 2023
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Jun 28, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pliurh, zshi-redhat

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
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Jun 28, 2023

@pliurh: The following test 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-openshift-conformance-reduced-arm e3fc6b9 link false /test e2e-openshift-conformance-reduced-arm

Full PR test history. Your PR dashboard.

Details

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.

@openshift-merge-robot openshift-merge-robot merged commit 70cda68 into openshift:main Jun 28, 2023
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. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. qe-approved Signifies that QE has signed off on this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants