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

multi-vtep: set ovn-encap-ip on OVS Port when adding it to br-int #4357

Merged
merged 3 commits into from
Jun 6, 2024

Conversation

l8huang
Copy link

@l8huang l8huang commented May 15, 2024

What this PR does and why is it needed

To utilize multi-VTEP, the external_ids:ovn-encap-ip of the VF's OVS Port
must be set. The VF's PCI device ID is then used to identify its corresponding
PF, which in turn is used to look up the external_ids:ovn-pf-encap-ip-mapping
to determine the encap-ip."

Special notes for reviewers

doc is in progress

How to verify it

Unit test cases are added to verify ``external_ids:ovn-encap-ip` is set on OVS Port.

@l8huang l8huang requested a review from a team as a code owner May 15, 2024 01:49
@l8huang l8huang requested a review from ricky-rav May 15, 2024 01:49
@coveralls
Copy link

coveralls commented May 15, 2024

Coverage Status

coverage: 52.882% (+0.2%) from 52.671%
when pulling 174c7ea on l8huang:multi-vteps
into f5d4dfb on ovn-org:master.

@l8huang
Copy link
Author

l8huang commented May 15, 2024

#4177 is required for this feature.

Copy link
Contributor

@cathy-zhou cathy-zhou left a comment

Choose a reason for hiding this comment

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

looks good to me, just a minor nit.

go-controller/pkg/cni/helper_linux.go Outdated Show resolved Hide resolved
@tssurya tssurya added feature/multi-rails Issues, PRs related to multi-rails kind/feature All issues/PRs that are new features labels May 17, 2024
@l8huang
Copy link
Author

l8huang commented May 20, 2024

/help

@ovn-robot
Copy link
Collaborator

Supported operations are /retest, /retest-failed, /cancel

@l8huang
Copy link
Author

l8huang commented May 20, 2024

/retest-failed

@ovn-robot
Copy link
Collaborator

Oops, something went wrong:

Must have admin rights to Repository.

@cathy-zhou
Copy link
Contributor

/retest-failed

@ovn-robot
Copy link
Collaborator

Oops, something went wrong:

Must have admin rights to Repository.

@tssurya
Copy link
Member

tssurya commented May 27, 2024

@l8huang / @girishmg : Is there an ETA for this, will it make it before Friday this week that's when we agreed for the 1.0.0 release cut, I hope we don't push it out further (but happy to get a reasonable ETA here else let's remove it from the roadmap milestone)

@l8huang
Copy link
Author

l8huang commented May 29, 2024

@girishmg did you get a chance to review this PR?

@github-actions github-actions bot added kind/documentation All issues related to documentation area/unit-testing Issues related to adding/updating unit tests labels Jun 6, 2024
docs/features/multiple-networks/multi-vtep.md Outdated Show resolved Hide resolved
docs/features/multiple-networks/multi-vtep.md Outdated Show resolved Hide resolved
docs/features/multiple-networks/multi-vtep.md Outdated Show resolved Hide resolved
On OVN Kubernetes side, no addtional configuration required to enable this feature.

This feature depends on a specific underlay network setup; it cannot be turned off
Copy link
Member

Choose a reason for hiding this comment

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

should this be on.

this feature cannot be turned on without an adequate underlay network configuration?

Copy link
Author

Choose a reason for hiding this comment

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

you are right.
done

docs/features/multiple-networks/multi-vtep.md Outdated Show resolved Hide resolved
docs/features/multiple-networks/multi-vtep.md Show resolved Hide resolved
Copy link
Author

@l8huang l8huang left a comment

Choose a reason for hiding this comment

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

Thanks for your comments. I need to install spelling check in my editor.

docs/features/multiple-networks/multi-vtep.md Outdated Show resolved Hide resolved
docs/features/multiple-networks/multi-vtep.md Outdated Show resolved Hide resolved
docs/features/multiple-networks/multi-vtep.md Outdated Show resolved Hide resolved
On OVN Kubernetes side, no addtional configuration required to enable this feature.

This feature depends on a specific underlay network setup; it cannot be turned off
Copy link
Author

Choose a reason for hiding this comment

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

you are right.
done

docs/features/multiple-networks/multi-vtep.md Outdated Show resolved Hide resolved
docs/features/multiple-networks/multi-vtep.md Show resolved Hide resolved
Lei Huang added 3 commits June 6, 2024 10:32
To utilize multi-VTEP, the `external_ids:ovn-encap-ip` of the VF's OVS Port
must be set. The VF's PCI device ID is then used to identify its corresponding
PF, which in turn is used to look up the `external_ids:ovn-pf-encap-ip-mapping`
to determine the encap-ip."

Signed-off-by: Lei Huang <[email protected]>
Signed-off-by: Lei Huang <[email protected]>
…-encap-ip

external_ids:ovn-encap-ip can be a list of IP address separted by
comma, ovnkube-node should be able to startup without issue.

Signed-off-by: Lei Huang <[email protected]>
Copy link
Member

@girishmg girishmg left a comment

Choose a reason for hiding this comment

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

Thanks for addressing all my comments @l8huang !

LGTM

@girishmg girishmg merged commit 16da3ce into ovn-org:master Jun 6, 2024
35 of 36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/unit-testing Issues related to adding/updating unit tests feature/multi-rails Issues, PRs related to multi-rails kind/documentation All issues related to documentation kind/feature All issues/PRs that are new features
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

6 participants