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

subnet can have an associated tunnel interface for evpn #310

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jainvipin
Copy link
Contributor

@glimchb @venkatmahalingam - pls review one field change in subnet.proto file

@jainvipin jainvipin requested a review from a team as a code owner July 10, 2023 23:02
Copy link
Member

@glimchb glimchb left a comment

Choose a reason for hiding this comment

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

Vipin, I thought you adding subnet reference to the tunnel object. Not the other way around… if tunnel is vxlan, and subnet is bridge we may have one to many…

@jainvipin
Copy link
Contributor Author

jainvipin commented Jul 11, 2023

Vipin, I thought you adding subnet reference to the tunnel object. Not the other way around… if tunnel is vxlan, and subnet is bridge we may have one to many…

Yes, a bridge would/could have many tunnel interfaces like it can have many local interfaces. Would it be better to make this field repeated instead of having subnet reference in the tunnel? This way we can keep all interfaces within a subnet together (local and tunnel). e.g. field number 15 is for the local interfaces today. Given that we are not worried about backward compatibility, we can put these two fields together for clarity.

 // when operating in DEVICE_OPER_MODE_HOST mode with multiple host PFs/VFs
  // present, subnet needs to be attached to a PF/VF (aka. host interface),
  // HostIf is list of such VF/PFs
  repeated string host_interface_name_ref = 15;

@jainvipin
Copy link
Contributor Author

jainvipin commented Sep 7, 2023

@glimchb @sandersms - should we merge this? this way cloud APIs can be a bit more flexible. Having said that we are addressing evpn use case as a first class APIs and so the use case for this enhancement becomes somewhat unclear.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants