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.

Sign up for GitHub

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

Jump to bottom

ovs: add internal interface #830

Merged
merged 1 commit into from
Jan 13, 2025
Merged

ovs: add internal interface #830

merged 1 commit into from
Jan 13, 2025

Conversation

rollandf
Copy link
Contributor

When creating a bridge with ovs-vsctl, an internal interface is added by default.
The same behavior is added in this commit

ovs-vsctl code ref:
https://github.com/openvswitch/ovs/blob/main/utilities/ovs-vsctl.c#L1597

@github-actions GitHub Actions
Copy link

Thanks for your PR,
To run vendors CIs, Maintainers can use one of:

  • /test-all: To run all tests for all vendors.
  • /test-e2e-all: To run all E2E tests for all vendors.
  • /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs, Maintainers can use one of:

  • /skip-all: To skip all tests for all vendors.
  • /skip-e2e-all: To skip all E2E tests for all vendors.
  • /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
    Best regards.

@rollandf
Copy link
Contributor Author

@ykulazhenkov PTAL

@coveralls
Copy link

coveralls commented Jan 12, 2025 •
edited
Loading

Pull Request Test Coverage Report for Build 12744720588

Details

  • 6 of 9 (66.67%) changed or added relevant lines in 1 file are covered.
  • 5 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.002%) to 47.404%
Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/host/internal/bridge/ovs/ovs.go 6 9 66.67%
Files with Coverage Reduction New Missed Lines %
controllers/generic_network_controller.go 5 74.38%
Totals Coverage Status
Change from base Build 12743499022: -0.002%
Covered Lines: 7267
Relevant Lines: 15330
💛 - Coveralls

adrianchiris
adrianchiris reviewed Jan 13, 2025
View reviewed changes
Copy link
Collaborator

@adrianchiris adrianchiris left a comment

Choose a reason for hiding this comment

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

overall LGTM.

im OK with not having api change for it and let that be the default.
later on we can change if the use-case rises (e.g need to enable/disable creation of internal port or adding some external ids to it)

@rollandf what would happen to existing bridges created that dont have internal port set ? will they be reconciled ? i think no with current logic.
do we want them to be reconciled ?

pkg/host/internal/bridge/ovs/ovs_test.go Outdated
ovs := dbContent.OpenVSwitch[0]
br := dbContent.Bridge[0]
port := dbContent.Port[0]
iface := dbContent.Interface[0]
var internalPort, port *PortEntry
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: i would create a port and interface map keyed by name.
that way if we have more later, there would be no need to do additional if conditions as below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@ykulazhenkov
Copy link
Collaborator

do we want them to be reconciled ?

I think it is ok not to reconcile internal ports for now. If we will add such reconciliation, then all existing bridges will need to be reconfigured, meaning all nodes will be drained without any chance to opt out reconfiguration.

@adrianchiris
Copy link
Collaborator

I think it is ok not to reconcile internal ports for now.

agree, since adding internal port will not hinder current use-cases.

ykulazhenkov
ykulazhenkov reviewed Jan 13, 2025
View reviewed changes
Copy link
Collaborator

@ykulazhenkov ykulazhenkov left a comment

Choose a reason for hiding this comment

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

@rollandf
looks good to me.

The PR currently has a merge conflict

@rollandf
ovs: add internal interface
84d0a6d 
When creating a bridge with ovs-vsctl, an internal
interface is added by default.
The same behavior is added in this commit

ovs-vsctl code ref:
https://github.com/openvswitch/ovs/blob/main/utilities/ovs-vsctl.c#L1597

Signed-off-by: Fred Rolland <[email protected]>
@rollandf
Copy link
Contributor Author

@rollandf looks good to me.

The PR currently has a merge conflict

Fixed conflict

adrianchiris
adrianchiris approved these changes Jan 13, 2025
View reviewed changes
Copy link
Collaborator

@adrianchiris adrianchiris left a comment

Choose a reason for hiding this comment

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

LGTM

@rollandf
Copy link
Contributor Author

@zeeke @SchSeba PTAL

zeeke
zeeke approved these changes Jan 13, 2025
View reviewed changes
Copy link
Member

@zeeke zeeke left a comment

Choose a reason for hiding this comment

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

LGTM

@zeeke zeeke merged commit f891498 into k8snetworkplumbingwg:master Jan 13, 2025
13 of 14 checks passed
@rollandf rollandf deleted the ovs-internal branch January 21, 2025 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ykulazhenkov ykulazhenkov ykulazhenkov approved these changes

@adrianchiris adrianchiris adrianchiris approved these changes

@zeeke zeeke zeeke approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@rollandf @coveralls @ykulazhenkov @adrianchiris @zeeke