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

ci: replace static comamnds with grpc calls #103

Merged
merged 8 commits into from
Aug 31, 2023
Merged

Conversation

glimchb
Copy link
Member

@glimchb glimchb commented Aug 9, 2023

Signed-off-by: Boris Glimcher [email protected]

@codecov
Copy link

codecov bot commented Aug 9, 2023

Codecov Report

Merging #103 (4ff5865) into main (652a65a) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #103   +/-   ##
=======================================
  Coverage   48.00%   48.00%           
=======================================
  Files           5        5           
  Lines         852      852           
=======================================
  Hits          409      409           
  Misses        424      424           
  Partials       19       19           

@glimchb glimchb force-pushed the main branch 4 times, most recently from 2d18bd4 to 8555203 Compare August 9, 2023 20:34
docker-compose.yml Outdated Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
@glimchb glimchb force-pushed the main branch 8 times, most recently from 5444bb0 to f555b63 Compare August 11, 2023 17:18
@glimchb glimchb force-pushed the main branch 2 times, most recently from f38c8d2 to 78b0fa9 Compare August 26, 2023 02:56
@glimchb glimchb marked this pull request as ready for review August 26, 2023 03:00
@glimchb glimchb requested a review from a team as a code owner August 26, 2023 03:00
@glimchb glimchb requested a review from a team August 26, 2023 03:12
@glimchb glimchb force-pushed the main branch 2 times, most recently from 0ae4f7f to 1ca25c4 Compare August 28, 2023 20:48
docker-compose.yml Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
@glimchb glimchb added the Merge Candidate in the open merge window, next candidate for merge label Aug 30, 2023
/entrypoint.sh call --json_input --json_output localhost:50151 CreateLogicalBridge "{\"logical_bridge_id\" : \"vlan30\", \"logical_bridge\" : {\"spec\" : {\"vni\" : 30, \"vlan_id\": 30 }} }" && \
/entrypoint.sh call --json_input --json_output localhost:50151 CreateLogicalBridge "{\"logical_bridge_id\" : \"vlan40\", \"logical_bridge\" : {\"spec\" : {\"vni\" : 40, \"vlan_id\": 40 }} }" && \
/entrypoint.sh call --json_input --json_output localhost:50151 CreateLogicalBridge "{\"logical_bridge_id\" : \"vlan50\", \"logical_bridge\" : {\"spec\" : { \"vlan_id\": 50 }} }" && \
/entrypoint.sh call --json_input --json_output localhost:50151 CreateSvi "{\"svi_id\" : \"blue-vlan20\", \"svi\" : {\"spec\" : {\"vrf\": \"//network.opiproject.org/vrfs/blue\", \"logical_bridge\": \"//network.opiproject.org/bridges/vlan20\", \"mac_address\" : \"qrvMAAAh\", \"gw_ip_prefix\": [{\"addr\": {\"af\": \"IP_AF_INET\", \"v4_addr\": 336860161}, \"len\": 24}] }} }" && \
Copy link
Member

@venkatmahalingam venkatmahalingam Aug 30, 2023

Choose a reason for hiding this comment

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

Can we use vlan20 instead of blue-vlan20 for SVI name? and logical_bridge_id as br"vlan-id"? ,e.g br50

Choose a reason for hiding this comment

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

I specifically want the SVI name to reflect the binding between VRF and logical bridge.
And I don't see any reason to encode the object type as prefix into the logical bridge objects. We are not doing that for other object types either. How we call the underlying Linux realization entities is a different matter.

Copy link
Member

@venkatmahalingam venkatmahalingam Aug 30, 2023

Choose a reason for hiding this comment

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

How we call the underlying Linux realization entities is a different matter.
-> This is the problem, unnecessary lengthy names lead to have additional mapping in the backend as Linux interface names length restricted to 15 characters. IMO, it is easy to get and know the SVI to VRF mappings, if we want to rebind the SVI from one VRF to another VRF on the fly, I would need to delete the SVI and create it with new VRF name part of SVI name.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree with @venkatmahalingam on this problem

if we want to rebind the SVI from one VRF to another VRF on the fly, I would need to delete the SVI and create it with new VRF name part of SVI name.

But naming of objects is left to the user, there is no code or rules to mandate to use any names... so we as OPI should not care...
user will pick a name or (since resource ID) is optional, grpc server will generate UUID as a name (or more accurately resource ID) instead...

Choose a reason for hiding this comment

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

How realistic and important a use case is it to move a subnet on a logical bridge to another VRF without any traffic impact? To me that is a change in network planning, which typically has wider implications in the network.
I know it is possible in Linux to shift an existing SVI netdev to another VRF just by enslaving it to the new vrf netdev, but is this something the EVPN-GW API must cover? In fact, I think it currently does cover it, as the VRF reference in the SVI spec is not marked as immutable. Same goes for the LogicalBridge reference, by the way, but the VLAN ID of an SVI netdev can't be changed in Linux on the fly.
At any rate, the object naming arbitrary, and we can apply any convention we like in this example configuration.

ip link add vni40 type vxlan id 40 local 10.0.0.2 dstport 4789 nolearning && \
ip link set vni40 master br-tenant up && \
bridge vlan add dev vni40 vid 40 pvid untagged && \
bridge link set dev vni40 neigh_suppress on learning off && \
Copy link
Member

Choose a reason for hiding this comment

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

How the fields "neigh_suppress" and "learning off" are mapped to the API?

Choose a reason for hiding this comment

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

They don't have to be reflected in the API. They are hard-coded in the EVPN-bridge implementation (until the point where someone shows that it is necessary to be able to control them. Then we can discuss how to enhance the logical bridge API with an optional attribute).

/entrypoint.sh call --json_input --json_output localhost:50151 CreateVrf "{\"vrf_id\" : \"blue\", \"vrf\" : {\"spec\" : { \"loopback_ip_prefix\": {\"addr\": {\"af\": \"IP_AF_INET\", \"v4_addr\": 167772418}, \"len\": 24}, \"vtep_ip_prefix\": {\"addr\": {\"af\": \"IP_AF_INET\", \"v4_addr\": 167772162}, \"len\": 24} }} }" && \
/entrypoint.sh call --json_input --json_output localhost:50151 CreateVrf "{\"vrf_id\" : \"green\", \"vrf\" : {\"spec\" : {\"vni\" : 100, \"loopback_ip_prefix\": {\"addr\": {\"af\": \"IP_AF_INET\", \"v4_addr\": 167772674}, \"len\": 24}, \"vtep_ip_prefix\": {\"addr\": {\"af\": \"IP_AF_INET\", \"v4_addr\": 167772162}, \"len\": 24} }} }" && \
/entrypoint.sh call --json_input --json_output localhost:50151 CreateVrf "{\"vrf_id\" : \"yellow\", \"vrf\" : {\"spec\" : {\"vni\" : 101, \"loopback_ip_prefix\": {\"addr\": {\"af\": \"IP_AF_INET\", \"v4_addr\": 167772930}, \"len\": 24}, \"vtep_ip_prefix\": {\"addr\": {\"af\": \"IP_AF_INET\", \"v4_addr\": 167772162}, \"len\": 24} }} }" && \
/entrypoint.sh call --json_input --json_output localhost:50151 CreateLogicalBridge "{\"logical_bridge_id\" : \"vlan10\", \"logical_bridge\" : {\"spec\" : {\"vni\" : 10, \"vlan_id\": 10 }} }" && \
Copy link
Member

@venkatmahalingam venkatmahalingam Aug 30, 2023

Choose a reason for hiding this comment

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

ip link add vni10 type vxlan id 10 local 10.0.0.2 dstport 4789 nolearning

How's the above command is mapped in the API incase of L2VXLAN (I dont expect VRF and SVI objects in L2-VXLAN)? where to provide VTEP local IP in the API?

Choose a reason for hiding this comment

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

The VRF spec has a field vtep_ip_prefix for this purpose.

Copy link
Member

Choose a reason for hiding this comment

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

Why do we need a VRF for L2VXLAN case?

Choose a reason for hiding this comment

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

Good catch, the LogicalBridgeSpec in the API should also have a vtep_ip_prefix. See issue #83

Copy link
Member Author

@glimchb glimchb Aug 31, 2023

Choose a reason for hiding this comment

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

opened #147 for VTEP IP

Copy link

@JanScheurich JanScheurich left a comment

Choose a reason for hiding this comment

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

LGTM

@glimchb glimchb merged commit 1a2b0e5 into opiproject:main Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Merge Candidate in the open merge window, next candidate for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants