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

[ipintutil]Handle exception in show ip interfaces command #3182

Merged
merged 5 commits into from
Mar 18, 2024

Conversation

dgsudharsan
Copy link
Collaborator

What I did

Handle exception in show ip interfaces command when executed during config reload. Sometimes during config reload the interfaces are removed and if show ip interfaces was executed during this time we may get the below traceback. The interface would exist when the call multi_asic_get_ip_intf_from_ns was made but would have been removed in the subsequent for loop which tries to get ip interface data for each interface

show ip interfaces

Traceback (most recent call last):
  File "/usr/local/bin/ipintutil", line 276, in

     main()
  File "/usr/local/bin/ipintutil", line 269, in main
    ip_intfs = get_ip_intfs(af, namespace, display)
  File "/usr/local/bin/ipintutil", line 232, in get_ip_intfs
    ip_intfs_in_ns = get_ip_intfs_in_namespace(af, namespace, display)
  File "/usr/local/bin/ipintutil", line 153, in get_ip_intfs_in_namespace
    ipaddresses = multi_asic_util.multi_asic_get_ip_intf_addr_from_ns(namespace, iface)
  File "/usr/local/lib/python3.9/dist-packages/utilities_common/multi_asic.py", line 186, in multi_asic_get_ip_intf_addr_from_ns

     ipaddresses = netifaces.ifaddresses(iface)
ValueError: You must specify a valid interface name.

How I did it

Adding try exception block so that if an interface is not present, it would be skipped.

How to verify it

Running show ip interface command and performing config reload in parallel

Previous command output (if the output of a command-line utility has changed)

New command output (if the output of a command-line utility has changed)

@dgsudharsan
Copy link
Collaborator Author

@qiluo-msft The code change is simple and just adding a try exception block. This scenario is hard to replicate through UT and I request exception for coverage for this PR

try:
ipaddresses = multi_asic_util.multi_asic_get_ip_intf_addr_from_ns(namespace, iface)
except ValueError:
continue
Copy link
Contributor

@qiluo-msft qiluo-msft Feb 28, 2024

Choose a reason for hiding this comment

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

Is it better to print error message and exit? If you continue, ipaddresses is not initialized and used. #Closed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think its correct. This will avoid other interfaces detail to not be shown. This is a very rare scenario during config reload.
Here is the scenario - While getting the interfaces there are 4 ports (docker0, eth0, lo0 and Loopback0). But while it enters the for loop in the parallel config reload flow Loopback0 gets deleted. However other interfaces exist and those information should be displayed. We can skip Loopback0 as we get valueerror meaning the interface does not exist now

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, there is no uninitialized variable in use.

@qiluo-msft
Copy link
Contributor

qiluo-msft commented Feb 28, 2024

@qiluo-msft The code change is simple and just adding a try exception block. This scenario is hard to replicate through UT and I request exception for coverage for this PR

Recommend still add UT to cover the lines. It is possible to mock some functions to reach the code branch. #Closed

@dgsudharsan
Copy link
Collaborator Author

@qiluo-msft The code change is simple and just adding a try exception block. This scenario is hard to replicate through UT and I request exception for coverage for this PR

Recommend still add UT to cover the lines. It is possible to mock some functions to reach the code branch.

I am trying to add. Will update if I can add

@dgsudharsan
Copy link
Collaborator Author

@qiluo-msft The code change is simple and just adding a try exception block. This scenario is hard to replicate through UT and I request exception for coverage for this PR

Recommend still add UT to cover the lines. It is possible to mock some functions to reach the code branch.

I am trying to add. Will update if I can add

Added UT and coverage is passing now.

@qiluo-msft qiluo-msft merged commit 125f36f into sonic-net:master Mar 18, 2024
5 checks passed
mssonicbld pushed a commit to mssonicbld/sonic-utilities that referenced this pull request Mar 21, 2024
…3182)

### What I did
Handle exception in show ip interfaces command when executed during config reload. Sometimes during config reload the interfaces are removed and if show ip interfaces was executed during this time we may get the below traceback. The interface would exist when the call multi_asic_get_ip_intf_from_ns was made but would have been removed in the subsequent for loop which tries to get ip interface data for each interface

```
show ip interfaces

Traceback (most recent call last):
  File "/usr/local/bin/ipintutil", line 276, in

     main()
  File "/usr/local/bin/ipintutil", line 269, in main
    ip_intfs = get_ip_intfs(af, namespace, display)
  File "/usr/local/bin/ipintutil", line 232, in get_ip_intfs
    ip_intfs_in_ns = get_ip_intfs_in_namespace(af, namespace, display)
  File "/usr/local/bin/ipintutil", line 153, in get_ip_intfs_in_namespace
    ipaddresses = multi_asic_util.multi_asic_get_ip_intf_addr_from_ns(namespace, iface)
  File "/usr/local/lib/python3.9/dist-packages/utilities_common/multi_asic.py", line 186, in multi_asic_get_ip_intf_addr_from_ns

     ipaddresses = netifaces.ifaddresses(iface)
ValueError: You must specify a valid interface name.
```

#### How I did it
Adding try exception block so that if an interface is not present, it would be skipped.

#### How to verify it
Running show ip interface command and performing config reload in parallel
@mssonicbld
Copy link
Collaborator

Cherry-pick PR to 202311: #3226

mssonicbld pushed a commit that referenced this pull request Mar 21, 2024
### What I did
Handle exception in show ip interfaces command when executed during config reload. Sometimes during config reload the interfaces are removed and if show ip interfaces was executed during this time we may get the below traceback. The interface would exist when the call multi_asic_get_ip_intf_from_ns was made but would have been removed in the subsequent for loop which tries to get ip interface data for each interface

```
show ip interfaces

Traceback (most recent call last):
  File "/usr/local/bin/ipintutil", line 276, in

     main()
  File "/usr/local/bin/ipintutil", line 269, in main
    ip_intfs = get_ip_intfs(af, namespace, display)
  File "/usr/local/bin/ipintutil", line 232, in get_ip_intfs
    ip_intfs_in_ns = get_ip_intfs_in_namespace(af, namespace, display)
  File "/usr/local/bin/ipintutil", line 153, in get_ip_intfs_in_namespace
    ipaddresses = multi_asic_util.multi_asic_get_ip_intf_addr_from_ns(namespace, iface)
  File "/usr/local/lib/python3.9/dist-packages/utilities_common/multi_asic.py", line 186, in multi_asic_get_ip_intf_addr_from_ns

     ipaddresses = netifaces.ifaddresses(iface)
ValueError: You must specify a valid interface name.
```

#### How I did it
Adding try exception block so that if an interface is not present, it would be skipped.

#### How to verify it
Running show ip interface command and performing config reload in parallel
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.

5 participants