[vm_topology.py]: Vlan interface addition delay handling by praveen-li · Pull Request #645 · sonic-net/sonic-mgmt

praveen-li · 2018-06-22T22:45:38Z

Vlan interface addition may take few secs to reflect in OVS Command, which causes entire add-topo task to fail. It is good to have more concrete fail check. As of now added a code to Retry and Exception.

Signed-off-by: Praveen Chaudhary pchaudhary@linkedin.com

Description of PR

Vlan interface addition may take few secs to reflect in OVS Command, which causes entire add-topo task to fail. It is good to have more concrete fail check. As of now added a code to Retry and Exception.

Fixes # (issue)

Type of change

Bug fix
[-] Testbed and Framework(new/improvement)
Test case(new/improvement)

Approach

How did you do it?
Added retry part in case of delay. Retries count is 3.
How did you verify/test it?
Part of pipeline:
http://172.25.11.20:8080/user/falco/my-views/view/Sonic_Build_Deploy_And_Test_PipeLine/
Any platform specific information?
Supported testbed topology if it's a new test case?

Documentation

Vlan interface addition may take few secs to reflect in OVS Command, which causes entire add-topo task to fail. We should have more concrete fail check. As of now added a code to Retry and Exception. Signed-off-by: Praveen Chaudhary <pchaudhary@linkedin.com>

pavel-shirshov

As requested

pavel-shirshov · 2018-06-22T23:01:26Z

ansible/roles/vm_set/library/vm_topology.py

+        if vlan_iface_id is None:
+            raise Exception("Can't find vlan_iface_id")
+
        injected_iface_id = bindings[injected_iface]


Can you please hide retry logic inside of get_ovs_port_bindings()?

1.) I can do that, but I will need to add an extra argument [=vlan_iface] to function get_ovs_port_bindings().
i.e get_ovs_port_bindings(br_name) ==> get_ovs_port_bindings(br_name, vlan_iface).

2.) Or I can write an wrapper on top on get_ovs_port_bindings(br_name) to check vlan_iface.

Kindly let me know which sounds better. Thanks a lot for review.

Vlan interface addition may take few secs to reflect in OVS Command, which causes entire add-topo task to fail. We should have more concrete fail check. As of now added a code to Retry and Exception. Move Retry handing in get_ovs_port_bindings Signed-off-by: Praveen Chaudhary <pchaudhary@linkedin.com>

praveen-li

Moved Retry mechanism in get_ovs_port_bindings. Kindly Review new changes.

praveen-li · 2018-06-22T23:15:31Z

ansible/roles/vm_set/library/vm_topology.py

+        if vlan_iface_id is None:
+            raise Exception("Can't find vlan_iface_id")
+
        injected_iface_id = bindings[injected_iface]


1.) I can do that, but I will need to add an extra argument [=vlan_iface] to function get_ovs_port_bindings().
i.e get_ovs_port_bindings(br_name) ==> get_ovs_port_bindings(br_name, vlan_iface).

2.) Or I can write an wrapper on top on get_ovs_port_bindings(br_name) to check vlan_iface.

Kindly let me know which sounds better. Thanks a lot for review.

praveen-li · 2018-07-18T17:40:15Z

ansible/roles/vm_set/library/vm_topology.py

            raise Exception("Can't find vlan_iface_id")
-
+        vlan_iface_id = bindings[vlan_iface]
        injected_iface_id = bindings[injected_iface]


Gentle Reminder for review.

pavel-shirshov

as comments

pavel-shirshov · 2018-07-18T18:34:19Z

ansible/roles/vm_set/library/vm_topology.py

-        bindings = VMTopology.get_ovs_port_bindings(br_name)
+        bindings, error = VMTopology.get_ovs_port_bindings(br_name, vlan_iface)
+        if error:
+            raise Exception("Can't find vlan_iface_id")


Can you please raise an Exception inside of the get_ovs_port_bindings method? I don't see any reason you want to have it here.

Sure I will change that part.

Sure will raise in function get_ovs_port_bindings.

pavel-shirshov · 2018-07-18T18:34:42Z

ansible/roles/vm_set/library/vm_topology.py

+        # Let`s retry few times in that case.
+        retries = 0
+        vlan_iface_id = None
+        while retries < RETRIES:


I'd better to use for retries in xrange(RETRIES):

I will change this to range. range takes more space than xrange but it is supported in python3 as well.

pavel-shirshov · 2018-07-18T18:36:37Z

ansible/roles/vm_set/library/vm_topology.py

+            if vlan_iface is None:
+                return result, False
+            # Check if we have vlan_iface populated
+            vlan_iface_id = result[vlan_iface]


how can you be sure you have vlan_iface key in the result? I think you need to check that you have vlan_iface key in the result first

Sure let me add a check for this.

pavel-shirshov · 2018-07-18T18:38:01Z

ansible/roles/vm_set/library/vm_topology.py

+            vlan_iface_id = result[vlan_iface]
+            if vlan_iface_id:
+                return result, False
+            time.sleep(1)


make the sleep time increasing in some way?
1,2,3 seconds
or
1, 3, 5 seconds

i can use 2*retries +1.

Vlan interface addition may take few secs to reflect in OVS Command, which causes entire add-topo task to fail. We should have more concrete fail check. As of now added a code to Retry and Exception. Review Comments Signed-off-by: Praveen Chaudhary <pchaudhary@linkedin.com>

praveen-li · 2018-07-18T21:10:17Z

Incorporated Review comments, kindly Review. Thanks a lot.

Incorporated review comments from PR sonic-net#645, sonic-net#645 Changes: 1.) Move vlan retry logic in get_ovs_port_bindings. 2.) Raise Exception only in get_ovs_port_bindings. RB=1366591 G=lnos-reviewers R=pchaudhary,pmao,rmolina,samaity,sfardeen,zxu A=

cherry-pick sonic-net#20169 to 202412 branch.  ### Description of PR  Summary: warm reboot is not supported in isolated topology. change the skip condition to cover both 202412 and 202505 branch. ### Type of change  - [ ] Bug fix - [ ] Testbed and Framework(new/improvement) - [ ] New Test case - [ ] Skipped for non-supported platforms - [ ] Test case improvement ### Back port request - [ ] 202205 - [ ] 202305 - [ ] 202311 - [ ] 202405 - [ ] 202411 - [ ] 202505 ### Approach #### What is the motivation for this PR? skip warm reboot test cases for pfcwd #### How did you do it? skip it with conditional mark #### How did you verify/test it? #### Any platform specific information? #### Supported testbed topology if it's a new test case? ### Documentation

… advance submodule head (sonic-net#11518) sairedis: * 38c0bb1 2022-07-21 | [sairedis] Fix reopen recoding file (sonic-net#1087) (HEAD -> 202205, github/202205) [Kamil Cudnik] platform-daemon: * 17587b6 2022-07-22 | [ycabled] add secure channel support for grpc dualtor active-active connectivity (sonic-net#275) (HEAD -> 202205, github/202205) [vdahiya12] linkmgrd: * c911ec7 2022-07-21 | Avoid unnecessary error logs from `handleGetServerMacAddressNotification` (sonic-net#96) (HEAD -> 202205) [Jing Zhang] * bbae81d 2022-07-18 | Add support for reconciliation after warm restart (sonic-net#76) [Jing Zhang] utilities: * bcc1206 2022-07-20 | Change db_migrator major version on master branch from version 2 to 3 (sonic-net#2272) (HEAD -> 202205) [Vaibhav Hemant Dixit] * ad40697 2022-07-21 | Fix test for pfcwd_sw_enable in db_migrator_test (sonic-net#2253) [bingwang-ms] * 886f612 2022-07-22 | Revert "show commands for SYSTEM READY (sonic-net#1851) (sonic-net#2261)" (sonic-net#2274) (github/202205) [Ying Xie] * a6404b7 2022-07-17 | show commands for SYSTEM READY (sonic-net#1851) (sonic-net#2261) [Senthil Kumar Guruswamy] swss-common: * 509b265 2022-07-06 | Add device global table definition (sonic-net#645) (HEAD -> 202205) [tjchadaga] Signed-off-by: Ying Xie <ying.xie@microsoft.com>

pavel-shirshov suggested changes Jun 22, 2018

View reviewed changes

praveen-li commented Jul 18, 2018

View reviewed changes

pavel-shirshov suggested changes Jul 18, 2018

View reviewed changes

pavel-shirshov approved these changes Jul 19, 2018

View reviewed changes

maggiemsft merged commit 3e97f6b into sonic-net:master Aug 1, 2018

Conversation

praveen-li commented Jun 22, 2018

Description of PR

Type of change

Approach

Documentation

Uh oh!

pavel-shirshov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

praveen-li left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pavel-shirshov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

praveen-li commented Jul 18, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants