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

fix(port): change TRUNK to ACCESS #156

Merged
merged 3 commits into from
Sep 13, 2023
Merged

fix(port): change TRUNK to ACCESS #156

merged 3 commits into from
Sep 13, 2023

Conversation

glimchb
Copy link
Member

@glimchb glimchb commented Sep 6, 2023

Fixes #104

Signed-off-by: Boris Glimcher Boris.Glimcher@emc.com

@glimchb glimchb added the bug Something isn't working label Sep 6, 2023
@codecov
Copy link

codecov bot commented Sep 6, 2023

Codecov Report

Merging #156 (ef000a4) into main (297ebb8) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #156   +/-   ##
=======================================
  Coverage   75.00%   75.00%           
=======================================
  Files           5        5           
  Lines        1012     1012           
=======================================
  Hits          759      759           
  Misses        218      218           
  Partials       35       35           

@venkatmahalingam
Copy link
Member

venkatmahalingam commented Sep 12, 2023

Please take the below change, the build should pass.

diff --git a/docker-compose.yml b/docker-compose.yml
index 1dd8df2..678da15 100644
--- a/docker-compose.yml
+++ b/docker-compose.yml
@@ -260,7 +260,7 @@ services:
       /entrypoint.sh call --json_input --json_output localhost:50151 CreateSvi            "{\"svi_id\"            : \"green-vlan40\",  \"svi\" :             {\"spec\" : {\"vrf\": \"//network.opiproject.org/vrfs/green\",  \"logical_bridge\": \"//network.opiproject.org/bridges/vlan40\", \"mac_address\" : \"qrvMAABB\", \"gw_ip_prefix\": [{\"addr\": {\"af\": \"IP_AF_INET\", \"v4_addr\": 673720321}, \"len\": 24}] }} }" && \
       /entrypoint.sh call --json_input --json_output localhost:50151 CreateSvi            "{\"svi_id\"            : \"yellow-vlan50\", \"svi\" :             {\"spec\" : {\"vrf\": \"//network.opiproject.org/vrfs/yellow\", \"logical_bridge\": \"//network.opiproject.org/bridges/vlan50\", \"mac_address\" : \"qrvMAABR\", \"gw_ip_prefix\": [{\"addr\": {\"af\": \"IP_AF_INET\", \"v4_addr\": 842150401}, \"len\": 24}] }} }" && \
       /entrypoint.sh call --json_input --json_output localhost:50151 CreateBridgePort     "{\"bridge_port_id\"    : \"eth1\",          \"bridge_port\" :     {\"spec\" : {\"mac_address\" : \"qrvMAAAB\", \"ptype\": \"TRUNK\",  \"logical_bridges\": [\"//network.opiproject.org/bridges/vlan10\", \"//network.opiproject.org/bridges/vlan20\", \"//network.opiproject.org/bridges/vlan40\"] }} }" && \
-      /entrypoint.sh call --json_input --json_output localhost:50151 CreateBridgePort     "{\"bridge_port_id\"    : \"eth2\",          \"bridge_port\" :     {\"spec\" : {\"mac_address\" : \"qrvMAAAB\", \"ptype\": \"TRUNK\",  \"logical_bridges\": [\"//network.opiproject.org/bridges/vlan50\"] }} }" && \
+      /entrypoint.sh call --json_input --json_output localhost:50151 CreateBridgePort     "{\"bridge_port_id\"    : \"eth2\",          \"bridge_port\" :     {\"spec\" : {\"mac_address\" : \"qrvMAAAB\", \"ptype\": \"ACCESS\",  \"logical_bridges\": [\"//network.opiproject.org/bridges/vlan50\"] }} }" && \
       echo get && \
       /entrypoint.sh call --json_input --json_output localhost:50151 GetVrf               "{\"name\" : \"//network.opiproject.org/vrfs/blue\" }" && \
       /entrypoint.sh call --json_input --json_output localhost:50151 GetVrf               "{\"name\" : \"//network.opiproject.org/vrfs/green\" }" && \
@@ -366,11 +366,10 @@ services:
                 ip route add default via 40.40.40.1 dev eth0.40 table 1001
                 sleep 5 && \
                 ping -c 3 6.6.6.6 -I 40.40.40.40 && ping -c 3 40.40.40.41 && \
-                ip link add link eth1 name eth1.50 type vlan id 50 && \
-                ip link set eth1.50 up && \
-                ip addr add 50.50.50.50/24 dev eth1.50 && \
+                ip addr add 50.50.50.50/24 dev eth1 && \
                 ip rule add from 50.50.50.50 lookup 1002
-                ip route add default via 50.50.50.1 dev eth1.50 table 1002
+                ip route add default via 50.50.50.1 dev eth1 table 1002
+                arp -s 50.50.50.1 aa:bb:cc:00:00:51
                 sleep 5 && \
                 ping -c 3 7.7.7.7 -I 50.50.50.50'

@@ -392,12 +391,6 @@ networks:
       config:
         - subnet: 10.168.3.0/24

-  internet:
-    ipam:
-      driver: default
-      config:
-        - subnet: 5.5.5.0/24
-
   internet1:
     ipam:
       driver: default
@@ -422,24 +415,12 @@ networks:
       config:
         - subnet: 10.10.10.32/28

-  n3htoleafbn1:
-    ipam:
-      driver: default
-      config:
-        - subnet: 20.20.20.0/24
-
   n4htoleafbn2:
     ipam:
       driver: default
       config:
         - subnet: 30.30.30.0/24

-  n5h1tol1r:
-    ipam:
-      driver: default
-      config:
-        - subnet: 40.40.40.16/28
-
   n6h1tol1y:
     ipam:
       driver: default

Fixes opiproject#104

Signed-off-by: Boris Glimcher <Boris.Glimcher@emc.com>
Signed-off-by: Boris Glimcher <Boris.Glimcher@emc.com>
@glimchb glimchb removed the bug Something isn't working label Sep 12, 2023
@glimchb glimchb marked this pull request as ready for review September 12, 2023 14:02
@glimchb glimchb requested a review from a team as a code owner September 12, 2023 14:02
@glimchb glimchb added the Merge Candidate in the open merge window, next candidate for merge label Sep 12, 2023
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.

The actual changes from TRUNK to ACCESS look OK.
Please consider my detailed comments.

docker-compose.yml Outdated Show resolved Hide resolved
docker-compose.yml Show resolved Hide resolved
Copy link
Contributor

@mardim91 mardim91 left a comment

Choose a reason for hiding this comment

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

ip rule add from 50.50.50.50 lookup 1002
ip route add default via 50.50.50.1 dev eth1.50 table 1002
ip route add default via 50.50.50.1 dev eth1 table 1002
arp -s 50.50.50.1 aa:bb:cc:00:00:51
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this arp entry on the arp table of host1 related to the mac addr of the GW ip ?

If yes why this is necessary ? Static arp entries should not be needed the host 1 should arp for the GW mac address automatically

Copy link
Member

Choose a reason for hiding this comment

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

We dont need this anymore, please refer my above reply.

Copy link
Contributor

@mardim91 mardim91 Sep 13, 2023

Choose a reason for hiding this comment

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

I am curious why this is not happening with the Trunk case and only access case. I mean how the bridges on the host are reacting when they see packets with vlan tags on them taking into account that those bridges have no vlan configuration ? Do they drop the packets or they forward the packet based on the MAC dest ignoring the vlan tag ? Because I am curious how the virtual env works coreclty for trunk cases and only have problems for access case.
@venkatmahalingam ^

Copy link
Member Author

Choose a reason for hiding this comment

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

removed static ARP ef000a4

Copy link
Member

Choose a reason for hiding this comment

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

@mardim91 I'll check and get back for your questions.

Copy link
Contributor

Choose a reason for hiding this comment

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

@venkatmahalingam
Copy link
Member

Do we need also to change the diagramm here @venkatmahalingam ?

https://github.com/opiproject/opi-evpn-bridge/blob/main/docs/OPI-EVPN-Leaf1-Detailed-View.png

Yes, @glimchb Please create issues for this and Jan's comment "It's a bit hard to correlate the bridge ports named "eth1" and "eth2" with the interfaces "eth0" and "eth1" of the opi-test container. Could you align those to make it easier to understand?" comment?

Signed-off-by: Boris Glimcher <Boris.Glimcher@emc.com>
Copy link
Contributor

@mardim91 mardim91 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
Copy link
Member Author

glimchb commented Sep 13, 2023

Yes, @glimchb Please create issues for this and Jan's comment

opened #179 and #180

@glimchb glimchb merged commit 25ccd49 into opiproject:main Sep 13, 2023
19 checks passed
@glimchb glimchb deleted the access branch September 13, 2023 16:50
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.

port: eth2 is supposed to be ACCESS but pvid untagged didn't work
4 participants