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

ovnkube-node: add support for comma-separated IPs in external_ids:ovn-encap-ip #4177

Closed
wants to merge 1 commit into from

Conversation

l8huang
Copy link

@l8huang l8huang commented Feb 22, 2024

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

…-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 <leih@nvidia.com>
@coveralls
Copy link

Coverage Status

coverage: 51.949% (-0.003%) from 51.952%
when pulling 486a1ea on l8huang:encaps
into d3726b0 on ovn-org:master.

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.

Does having multiple encap-ips actually do anything with how OVNK configures OVN? @dceara maybe you can comment here. I'm not sure with OVN IC if we would even setup multiple tunnels if there were multiple encap ips provided.

@l8huang
Copy link
Author

l8huang commented Mar 1, 2024

Thanks for looking at this PR @trozet.

Does having multiple encap-ips actually do anything with how OVNK configures OVN?

For current upstream, it doesn't. This is just for making ovn-k8s works with legit external_ids:ovn-encap-ip value.

A feature added to OVN to support multi encap ips(see ovn-org/ovn@41eefcb, it depends on the change in this PR), the corresponding feature on ovn-k8s will be pushed later.

@dceara
Copy link
Contributor

dceara commented Mar 5, 2024

Thanks for looking at this PR @trozet.

Does having multiple encap-ips actually do anything with how OVNK configures OVN?

For current upstream, it doesn't. This is just for making ovn-k8s works with legit external_ids:ovn-encap-ip value.

A feature added to OVN to support multi encap ips(see ovn-org/ovn@41eefcb, it depends on the change in this PR), the corresponding feature on ovn-k8s will be pushed later.

Just a note, this requires bumping the OVN version used in CI to the freshly released OVN 24.03.0.

@l8huang
Copy link
Author

l8huang commented Mar 5, 2024

Just a note, this requires bumping the OVN version used in CI to the freshly released OVN 24.03.0.

Nope, this change doesn't require OVN version change. Supporting comma-separated IPs in external_ids:ovn-encap-ip is a old feature.

@dceara
Copy link
Contributor

dceara commented Mar 5, 2024

Just a note, this requires bumping the OVN version used in CI to the freshly released OVN 24.03.0.

Nope, this change doesn't require OVN version change. Supporting comma-separated IPs in external_ids:ovn-encap-ip is a old feature.

I was under the impression that without ovn-org/ovn@41eefcb only the first IP will be used.

@l8huang
Copy link
Author

l8huang commented Mar 5, 2024

yes, now only the firstly IP is used, this PR just allow it to be a list. Another PR will be pushed later for corresponding changes in ovn-k8s for ovn-org/ovn@41eefcb.

@trozet
Copy link
Contributor

trozet commented Mar 7, 2024

I think it would be better to hold off on merging this PR, until the full functionality is introduced. Otherwise we are just adding a config option that isn't being used.

@l8huang
Copy link
Author

l8huang commented Mar 7, 2024

I think it would be better to hold off on merging this PR, until the full functionality is introduced. Otherwise we are just adding a config option that isn't being used.

This change is not bound to the new multi encap ips feature added by ovn-org/ovn@41eefcb -- external_ids:ovn-encap-ip supports comma-separated IPs before that. That's the reason this change is submitted separately.

If there is any concern regarding this PR, it is okay to hold off on this change.

@tssurya tssurya added the area/gateway Issues related to node gateway code label Mar 14, 2024
@trozet
Copy link
Contributor

trozet commented Apr 30, 2024

Please reopen when the full feature/use case is identified that we want to enable in ovnk.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/gateway Issues related to node gateway code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants