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

[show] Fix 'show mac' output, when FDB entry with Vlan 1 is present #1368

Merged
merged 1 commit into from
Mar 4, 2021

Conversation

ghost
Copy link

@ghost ghost commented Jan 19, 2021

  • Skip records of FDB entries, which are linked to default Vlan 1,
    to prevent exception throwing while performing
    command 'show mac' or 'fdbshow'.

Signed-off-by: Maksym Belei Maksym_Belei@jabil.com

- What I did
Resolves #894
Fixed "show mac" command execution failure in case, when the system has an FDB entry, which is linked to default Vlan 1.
The failure is caused by throwing exception, while trying to get int from None type object.
- How I did it
The condition has added to src/sonic-utilities/scripts/fdbshow script to handle and skip FDB entries, for which the system can not get Vlan ID.

- How to verify it
Configure your system to receive both tagged and untagged traffic. For example, you could use the next steps:

  1. Do configuration on DUT
    sudo config portchannel add PortChannel0002
    sudo config portchannel member add PortChannel0002 Ethernet68
    sudo config vlan add 40
    sudo config vlan member add 40 PortChannel0002
    sudo config interface ip add Vlan40 40.0.0.1/24
  2. Do configuration on Linux host
    sudo ip link add bond0 type bond
    sudo ip link set dev bond0 type bond mode 4
    sudo ip link set enp5s0f1 down
    sudo ip link set enp5s0f1 master bond0
    sudo ip link set enp5s0f1 up
    sudo ip link set bond0 up
    sudo ip link add link bond0 name bond0.40 type vlan id 40
    sudo ip link set bond0.40 up
    sudo ip addr add 40.0.0.3/24 dev bond0.40
  3. Do ping from linux host to DUT IP 40.0.0.1
  4. Do command "show mac" on DUT
    "show mac" command should not be finished with the next message: int() argument must be a string, a bytes-like object or a number, not 'NoneType'. Instead, the normal output of the command should be shown.

@ghost ghost changed the title [show] Fix 'show mac' execution error for case, when an FDB entry pre… [show] Fix 'show mac' output, when FDB entry with Vlan 1 is present Jan 19, 2021
@ghost ghost marked this pull request as ready for review January 19, 2021 14:16
@ghost
Copy link
Author

ghost commented Jan 19, 2021

Dear @prsunny,

The issue sonic-net/sonic-buildimage#894 is pretty similar to sonic-net/sonic-buildimage#6367. Could I ask you to check could we solve the issue with this changes?

@prsunny
Copy link
Contributor

prsunny commented Jan 19, 2021

Based on your configuration, IMO, the FDB should be on the 'untagged' vlan, i.e Vlan40 as a port can be untagged in only one Vlan. Displaying as "untagged" looks to be incomplete.

@ghost
Copy link
Author

ghost commented Jan 19, 2021

Dear @prsunny,

Actually, the system has two different FDB entries for the same MAC address in case with tagged Vlan: entry, linked to Vlan40 and entry with link to default Vlan(I have updated PR overview comment. Now, there are 3 FDB entries, 2 of which belongs to the same MAC address).

Displaying as "untagged" looks to be incomplete.
I am sorry, could you clarify what did you mean with this comment? Are you proposing to mark FDB entries with default Vlan somehow like this:

  No.  Vlan          MacAddress         Port             Type
-----  ------------  -----------------  ---------------  -------
    1  (40)Untagged  24:8A:07:EE:4D:E1  PortChannel0002  Dynamic

@prsunny
Copy link
Contributor

prsunny commented Jan 22, 2021

In the first place, how is fdb learnt on a non-configured Vlan? During initialization all ports are removed from default vlan. Secondly, providing the info as "untagged" can have implications. For. e.g, how can user flush this entry as there is no Vlan associated.

@ghost
Copy link
Author

ghost commented Jan 22, 2021

@prsunny, I was wrong in my understanding of the situation. In fact, when you link Ethernet port with tagged Vlan, as in my case, the port could either receive tagged and untagged traffic. In tagged case, the traffic routes to the Vlan, but in untagged case the traffic will be automatically assigned to Native Vlan and routed there. For this traffic, the FDB entry with default Vlan is being created(and marked as Untagged in show mac with my solution). I could not see any configurational mechanisms for native Vlans in the system and I presume that the untagged traffic in this case is currently being dropped by the system. Could you correct me if it not so?
Maybe, it would be better not to show FDB entries with default Vlan assigned, as the system behave with the traffic as with trash?

Answering on your questions:

how is fdb learnt on a non-configured Vlan? During initialization all ports are removed from default vlan.:

The system does not create FDB entries till you link the port with a Vlan.

providing the info as "untagged" can have implications. For. e.g, how can user flush this entry as there is no Vlan associated.

I agree with you. According to my proposition above, maybe, we could skip such FDB entries and not to show them with show mac, or, we could mark them not as Untagged, but, for example, as Native or something similar? But, IMO, skipping would be better.

@ghost
Copy link
Author

ghost commented Feb 15, 2021

@prsunny, could I ask you to pay some attention on my last comment?

@gechiang
Copy link
Contributor

@maksymbelei95 When You add a port to a vlan as untagged, It means that any untagged packet packets entering that port should be part of that VLAN (Bridge domain). Any interface can be an "untagged" member of only 1 Bridging domain (VLAN).
For example, you configure a new VLAN 50 and then add port1 to it as "untagged" and also add port2 as tagged.
If now you send an untagged packet with unknown destination MAC through port1, it will be flooded to port2 only. If you add port3 to VLAN 50 and send the same packet, it will now be flooded to both port2 and port3. This is because port1, 2, and 3 are all part of the same "bridging domain" that was configured under VLAN 50.
If you send an unknown tagged packet through port2, it will be flooded through port1 and port3. When the packet exit out of port1, it should be untagged.
So it does not make sense to have a "show mac" to display "untagged" under the VLAN column. As @prsunny mentioned, it would not help the user to understand which VLAN (bridging domain) that mac belongs to. The purpose of show mac is to list out MACs learnt for each of the VLAN. In my opinion "show mac" output should not care whether it was learnt over an "untagged" or "tagged" interface. This is because we already have the interface displayed as part of the "show mac" and we can find out if that interface was part of the "tagged"/"untagged" membership of that VLAN already...
Also, if a VLAN is not configured and thus no port can be part of it, there should not be any MAC learnt as there are no "bridging domain" to classify it to. In fact, SAI Shall not be announcing such MAC learning event. If it does, then it is a SAI issue that needs to be fixed.

* Skip records of FDB entries, which are linked to default Vlan 1,
  to prevent exception throwing while performing
  command 'show mac' or 'fdbshow'.

Signed-off-by: Maksym Belei <Maksym_Belei@jabil.com>
@ghost
Copy link
Author

ghost commented Mar 3, 2021

@gechiang, @prsunny, sorry for delayed response. Thank you for explanation. I have updated the PR. Now, FDB entries, linked to default Vlan, just will not be displayed for user.

@prsunny prsunny requested a review from gechiang March 4, 2021 07:32
@gechiang
Copy link
Contributor

@maksymbelei95
Looks like I overlooked one thing... All changes require unit test code to have test coverage over the new changes.
Can you raise a separate PR to include the missing unit test for this?
Thanks!

@qiluo-msft
Copy link
Contributor

@maksymbelei95 Could you also fix the 201911 branch? I tried cherry-pick and it does not work, since there are some other code changes happen on master.

daall pushed a commit that referenced this pull request Mar 16, 2021
…1368)

* Skip records of FDB entries, which are linked to default Vlan 1,
  to prevent exception throwing while performing
  command 'show mac' or 'fdbshow'.

Signed-off-by: Maksym Belei <Maksym_Belei@jabil.com>
qiluo-msft pushed a commit that referenced this pull request Mar 16, 2021
…1507)

* Skip records of FDB entries, which are linked to default Vlan 1,
  to prevent exception throwing while performing
  command 'show mac' or 'fdbshow'.

#### What I did
Resolves #894
Fixed "show mac" command execution failure in case, when the system has an FDB entry, which is linked to default Vlan 1.
The failure is caused by throwing exception, while trying to get int from None type object.
#### How I did it
The condition has added to src/sonic-utilities/scripts/fdbshow script to handle and skip FDB entries, for which the system can not get Vlan ID.

#### How to verify it
Configure your system to receive both tagged and untagged traffic. For example, you could use the next steps:

Do configuration on DUT
sudo config portchannel add PortChannel0002
sudo config portchannel member add PortChannel0002 Ethernet68
sudo config vlan add 40
sudo config vlan member add 40 PortChannel0002
sudo config interface ip add Vlan40 40.0.0.1/24
Do configuration on Linux host
sudo ip link add bond0 type bond
sudo ip link set dev bond0 type bond mode 4
sudo ip link set enp5s0f1 down
sudo ip link set enp5s0f1 master bond0
sudo ip link set enp5s0f1 up
sudo ip link set bond0 up
sudo ip link add link bond0 name bond0.40 type vlan id 40
sudo ip link set bond0.40 up
sudo ip addr add 40.0.0.3/24 dev bond0.40
Do ping from linux host to DUT IP 40.0.0.1
Do command "show mac" on DUT
"show mac" command should not be finished with the next message: int() argument must be a string, a bytes-like object or a number, not 'NoneType'. Instead, the normal output of the command should be shown.

#### Additional information
Cherry-pick of #1368.

Pay attention, additional change is required. HEAD of https://github.com/Azure/sonic-py-swsssdk submodule of buildimage should be updated and be pointed to the top of 201911 branch, because current head of the submodule causes KeyError exception raising inside function `get_vlan_id_from_bvid` of `port_util.py` module:
```
SAI_VLAN_ATTR_VLAN_ID ('SAI_VLAN_ATTR_VLAN_ID',)
Failed to get Vlan id for bvid oid:0x26000000000013
```
The raising has already fixed on the top of 201911 branch of the submodule.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

show mac display wrong information for tagged vlan
4 participants