Skip to content

[config] Add vlan member only after checking RIF is not present#1513

Closed
ghost wants to merge 3 commits intosonic-net:masterfrom
d-dashkov:fix_vlan_member_add
Closed

[config] Add vlan member only after checking RIF is not present#1513
ghost wants to merge 3 commits intosonic-net:masterfrom
d-dashkov:fix_vlan_member_add

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Mar 17, 2021

What I did

Resolves sonic-net/sonic-buildimage#7028
Updated mechanism how to check is the port has RIF or not before adding it the port to the Vlan.
The related unit tests updated.

Copied implementation of port_util from swsssdk to utilities_common of sonic_utilities. Replaced using of swsssdk to utilities_common.

How I did it

By using functionality of port_util.py to check whether a RIF is present for the port in the system.
Now, the user can add a member to the Vlan only if there are no router records(ASIC_STATE:SAI_OBJECT_TYPE_ROUTER_INTERFACE:) in ASIC_DB, linked to OID of the interface.

How to verify it

  1. sudo config interface ip add Ethernet0 30.0.0.1/24
  2. sudo config route add prefix 20.0.0.1/32 nexthop Ethernet0
  3. sudo config interface ip remove Ethernet0 30.0.0.1/24
  4. sudo config vlan add 30
  5. sudo config vlan member add --untagged 30 Ethernet0
    The last command will return the next error: Error: Ethernet0 is a router interface! Check assigned L3 addresses, a related static routes etc.
  6. sudo config route del prefix 20.0.0.1/32 nexthop Ethernet0
    This will delete the route and RIF for the port.
  7. sudo config vlan member add --untagged 30 Ethernet0
    The port will be added successfully,

Previous command output

admin@sonic:~$ sudo config vlan member add 30 Ethernet0
Usage: config vlan member add [OPTIONS] <vid> port
Try "config vlan member add -h" for help.

Error: Ethernet0 is a router interface!

New command output

Output of command config vlan member add for the case is little bit clarified to help user to understand that is going on.

admin@sonic:~$ sudo config vlan member add 30 Ethernet0
Usage: config vlan member add [OPTIONS] <vid> port
Try "config vlan member add -h" for help.

Error: Ethernet0 is a router interface! Check assigned L3 addresses, a related static routes etc.

Maksym Belei added 2 commits March 17, 2021 16:46
* Checking whether the interface port has RIF or not before
  setting it as a member of the vlan. Only ports and LAGs,
  which have not RIFs, assigned to them, can be a vlan members.
  Even if there is no L3 address assigned to the interface,
  the RIF could be still present, e.g, if static route had
  created before L3 address deleted. Due to this, data
  from ASIC_DB and COUNTERS_DB is used for checking.

Signed-off-by: Maksym Belei <[email protected]>
* Updating unit tests according to the last changes in
  the source code. Mocking COUNTERS_DB and ASIC_DB for
  vlan_test.py from separated input to avoid dependencies
  between vlan test class and the other classes.

Signed-off-by: Maksym Belei <[email protected]>
config/vlan.py Outdated

from time import sleep
from .utils import log
from swsssdk import port_util
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

no more swsssdk, it is deprecating.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Got it. I am sorry, but I cannot find an alternate for port_util in swsscommon or any other places. Are there plans to implement something similar to port_util to swsscommon or somewhere else? I see the some scripts are still using port_util. I think the alternative will be useful.
If not, I am going to implement all the necessary directly inside vlan.py.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

As I said before, port_util is still can be useful, so, I decided to copy its implementation to utilities_common of sonic_utilities.
I have found all places in sonic_utilities, where port_util is imported from swsssdk and replaced them to import from utilities_common.

@anshuv-mfst
Copy link
Copy Markdown

@prsunny - FYI

* As swsssdk in deprecating process, copy port_util from swsssdk
  to utilities_common and replace all usages of swsssdk.port_util
  to its copy from utilities_common. Remove redundant parts of code
  from new port_util.

Signed-off-by: Maksym Belei <[email protected]>
@ghost ghost requested a review from lguohan March 18, 2021 14:34
@ghost ghost marked this pull request as ready for review March 18, 2021 14:34
@ghost
Copy link
Copy Markdown
Author

ghost commented Mar 19, 2021

retest this please

@ghost
Copy link
Copy Markdown
Author

ghost commented Mar 23, 2021

@lguohan, @prsunny, could I ask you to review the PR?

@prsunny
Copy link
Copy Markdown
Contributor

prsunny commented Mar 23, 2021

IMO, the existing check is sufficient. I think the reported issue must be handled differently. I've provided my comments in the issue - sonic-net/sonic-buildimage#7028

@ghost
Copy link
Copy Markdown
Author

ghost commented Mar 24, 2021

@prsunny, I think my changes will be a more robust solution to check whether the port is RIF of not, because it checks the result of deleting of the RIF(absence of the related record in ASIC_DB), rather then checking of records in CONFIG_DB, which represent desirable state of RIFs, but, not guarantee that the RIF is completely deleted, like in case with static route.
Maybe, it is not a full fix for the issue, but, I think, it is a good enhancement for the RIF checking mechanism.
Regarding your proposal to check whether a static route present or not before deleting ip entry. I thought about such solution. It may be more complicated, then only checking presence of the RIF, because, in my understanding, we should read the all routes and check whether the related routes presents or not. Also. we should check are there any other IP entries, assigned to the port or not, because the RIF will be deleted only after deleting of all the IP entries and before that, a static routes could be valid. Because of that, I decided to stay on checking of RIF on the port.
If checking of static routes on deleting of IP entry is desirable check, I could try to implement something such thing and add to this PR.

@prsunny
Copy link
Copy Markdown
Contributor

prsunny commented Mar 25, 2021

Adding static route is not a typical use-case, as we already know that static route is not persisted across reboots. Also the fix cannot be considered complete, as adding such a port to portchannel should also be then returning failure. IMO, it is adding more overhead than simple checking for IP address in config_db.

@ghost ghost mentioned this pull request Mar 30, 2021
@ghost ghost closed this Oct 20, 2021
This pull request was closed.
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.

Impossible to set port to be member of VLAN(specific scenario with static route)

3 participants