Add CLI configuration options for teamd retry count feature #2642

saiarcot895 · 2023-02-01T02:20:42Z

Add a SONiC CLI to more easily configure the retry count for port channels. This effectively acts like a wrapper around the underlying teamdctl command.

Also add a python script that'll be installed into /usr/local/bin/teamd_increase_retry_count.py that will detect if the peer device likely supports this teamd feature (based on LLDP neighbor info) and increases the teamd retry count to 5 in preparation for warm upgrade. This script requires sudo to run.

This is tied to sonic-net/sonic-buildimage#13453.

Signed-off-by: Saikrishna Arcot [email protected]

What I did

How I did it

How to verify it

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)

admin@vlab-02:~$ sudo config portchannel retry-count get PortChannel101
3
admin@vlab-02:~$ sudo config portchannel retry-count get PortChannel999
Usage: config portchannel retry-count get [OPTIONS] <portchannel_name>
Try "config portchannel retry-count get -h" for help.

Error: PortChannel999 is not present.
admin@vlab-02:~$ sudo config portchannel retry-count set PortChannel101 3
admin@vlab-02:~$ sudo config portchannel retry-count set PortChannel101 5

Add a SONiC CLI to more easily configure the retry count for port channels. This effectively acts like a wrapper around the underlying teamdctl command. Also add a python script that'll be installed into /usr/local/bin/teamd_increase_retry_count.py that will detect if the peer device likely supports this teamd feature (based on LLDP neighbor info) and increases the teamd retry count to 5 in preparation for warm upgrade. This script requires sudo to run. Signed-off-by: Saikrishna Arcot <[email protected]>

Signed-off-by: Saikrishna Arcot <[email protected]>

…try-count-cli

Signed-off-by: Saikrishna Arcot <[email protected]>

… or set Signed-off-by: Saikrishna Arcot <[email protected]>

Signed-off-by: Saikrishna Arcot <[email protected]>

Scapy's IPv6 support appears to have caused some issues with older versions of scapy, which may be present on older SONiC images. Signed-off-by: Saikrishna Arcot <[email protected]>

Don't fail warm reboot if teamd retry count support doesn't happen to be present. Also use fastfast-reboot for Mellanox devices. Signed-off-by: Saikrishna Arcot <[email protected]>

config/main.py

scripts/fast-reboot

scripts/teamd_increase_retry_count.py

scripts/fast-reboot

Signed-off-by: Saikrishna Arcot <[email protected]>

Also update a failure message in the warm-reboot script if the retry count script fails. Signed-off-by: Saikrishna Arcot <[email protected]>

Signed-off-by: Saikrishna Arcot <[email protected]>

yxieca · 2023-05-20T21:41:09Z

scripts/fast-reboot

+    /usr/local/bin/teamd_increase_retry_count.py --probe-only || TEAMD_RETRY_COUNT_PROBE_RC=$?
+    if [[ TEAMD_RETRY_COUNT_PROBE_RC -ne 0 ]]; then
+        debug "Warning: Retry count feature support unknown for one or more neighbor devices; assuming that it's not available"
+        TEAMD_INCREASE_RETRY_COUNT=0


Are we supposed to bail out from here?

For the images we deploy to our internal environment, this will be changed to bail out and fail the warm-reboot if SONiC neighbors are not seen. For the public version, since we can't require using SONiC-only neighbors for warm reboot, this will only be a warning.

Signed-off-by: Saikrishna Arcot <[email protected]>

…equired or not Signed-off-by: Saikrishna Arcot <[email protected]>

This is so that in the case of the teamd retry count check failing, there's fewer changes that happen on the system (it'll fail faster). Signed-off-by: Saikrishna Arcot <[email protected]>

This doesn't need to be after the point-of-no-return, since this will detach and be sending LACPDUs on its own. Signed-off-by: Saikrishna Arcot <[email protected]>

Signed-off-by: Saikrishna Arcot <[email protected]>

vaibhavhd

LGTM overall. Posted minor concerns.

scripts/fast-reboot

vaibhavhd · 2023-05-27T05:52:00Z

scripts/fast-reboot

+if [[ "${REBOOT_TYPE}" = "warm-reboot" || "${REBOOT_TYPE}" = "fastfast-reboot" ]]; then
+    TEAMD_RETRY_COUNT_PROBE_RC=0
+    /usr/local/bin/teamd_increase_retry_count.py --probe-only || TEAMD_RETRY_COUNT_PROBE_RC=$?
+    if [[ $TEAMD_RETRY_COUNT_PROBE_RC -ne 0 ]]; then


Minor: use ${} syntax

vaibhavhd · 2023-05-27T05:59:23Z

scripts/fast-reboot

+    /usr/local/bin/teamd_increase_retry_count.py --probe-only || TEAMD_RETRY_COUNT_PROBE_RC=$?
+    if [[ $TEAMD_RETRY_COUNT_PROBE_RC -ne 0 ]]; then
+        if [[ "${REQUIRE_TEAMD_RETRY_COUNT}" = "yes" ]]; then
+            error "Could not confirm that all neighbor devices are running SONiC with the retry count feature"


Looks like you are missing an exit w/ error code here?
Wouldn't the logic detect the error state and still continue to warm-reboot?

I think error does the exit as well?

Looks like it doesn't, not sure why I didn't see this not failing before. Added an exit with a new error code here.

vaibhavhd · 2023-05-27T06:00:27Z

scripts/fast-reboot

+            error "Could not confirm that all neighbor devices are running SONiC with the retry count feature"
+        else
+            debug "Warning: Retry count feature support unknown for one or more neighbor devices; assuming that it's not available"
+            TEAMD_INCREASE_RETRY_COUNT=0


Is this needed to be set to 0 again? The variable is initially set to 0 anyway, right?

That's true, removed this line.

vaibhavhd · 2023-05-27T06:02:53Z

scripts/fast-reboot

@@ -663,6 +688,10 @@ if [[ "$REBOOT_TYPE" = "fast-reboot" ]]; then
    fi
 fi

+if [[ ( "${REBOOT_TYPE}" = "warm-reboot" || "${REBOOT_TYPE}" = "fastfast-reboot" ) && "${TEAMD_INCREASE_RETRY_COUNT}" -eq 1 ]]; then


I think this feature is needed even for fast-reboot?
For fast-reboot it will help avoid LAG flaps too - if the boot up is slow enough that 90s is reached before even syncd does ASIC reset call. So essentially, preventing LAG flap other than where it should really happen.

We can make decision on this later too.

vaibhavhd · 2023-05-27T06:06:45Z

scripts/teamd_increase_retry_count.py

+    if os.geteuid() != 0:
+        log.log_error("Root privileges required for this operation", also_print_to_console=True)
+        sys.exit(1)
+        return False


This return will never be reached after sys.exit

Removed the extra returns.

vaibhavhd · 2023-05-27T06:07:33Z

scripts/teamd_increase_retry_count.py

+        return False
+    portChannels = getPortChannels()
+    if not portChannels:
+        return True


Why is this main returning True/False? What is checking on these return values?

I think at one point of time, I had some logic that would indicate success/failure via the True/False returns, but this isn't used at all. Removed the True/False returns.

vaibhavhd · 2023-05-27T06:17:41Z

scripts/teamd_increase_retry_count.py

+    activePortChannels = []
+    for portChannel in portChannels:
+        state = portChannelTable.get(portChannel)
+        if not state[0]:


Risk of index error here?

I think libswsscommon returns whether the call is a success or failure in the first index of the tuple that's returned, but regardless, I added a check to see if an empty tuple/list was returned.

vaibhavhd · 2023-05-27T06:20:31Z

scripts/teamd_increase_retry_count.py

+                    log.log_warning("WARNING: No peer description available via LLDP for {}; skipping".format(portName))
+                    continue
+                portChannelChecked = True
+                if "SONiC" not in peerInfo["descr"]:


Minor: Check w/ ignore case here might be better?

Signed-off-by: Saikrishna Arcot <[email protected]>

Update sonic-utilities submodule pointer to include the following: * 5c9b2177 Fix issue: out of range sflow polling interval is accepted and stored in config_db ([sonic-net#2847](sonic-net/sonic-utilities#2847)) * 72ca4848 Add CLI configuration options for teamd retry count feature ([sonic-net#2642](sonic-net/sonic-utilities#2642)) * 359dfc0c [Clock] Implement clock CLI ([sonic-net#2793](sonic-net/sonic-utilities#2793)) * b316fc27 Add transceiver status CLI to show output from TRANSCEIVER_STATUS table ([sonic-net#2772](sonic-net/sonic-utilities#2772)) * dc59dbd2 Replace pickle by json ([sonic-net#2849](sonic-net/sonic-utilities#2849)) * a66f41c4 [show] replace shell=True, replace xml by lxml, replace exit by sys.exit ([sonic-net#2666](sonic-net/sonic-utilities#2666)) * 57500572 [utilities_common] replace shell=True ([sonic-net#2718](sonic-net/sonic-utilities#2718)) * 6e0ee3e7 [CRM][DASH] Extend CRM utility to support DASH resources. ([sonic-net#2800](sonic-net/sonic-utilities#2800)) * b2c29b0b [config] Generate sysinfo in single asic ([sonic-net#2856](sonic-net/sonic-utilities#2856)) Signed-off-by: dprital <[email protected]>

…nic-utilities submodule on master (#15193) Dependency: sonic-net/sonic-utilities#2718 Why I did it This PR sonic-net/sonic-utilities#2718 reduce shell=True usage in utilities_common.cli.run_command() function. Work item tracking Microsoft ADO (number only): 15022050 How I did it Replace strings commands using utilities_common.cli.run_command() function to list of strings due to circular dependency, advance sonic-utilities submodule 72ca4848 (HEAD -> master, upstream/master, upstream/HEAD) Add CLI configuration options for teamd retry count feature (sonic-net/sonic-utilities#2642) 359dfc0c [Clock] Implement clock CLI (sonic-net/sonic-utilities#2793) b316fc27 Add transceiver status CLI to show output from TRANSCEIVER_STATUS table (sonic-net/sonic-utilities#2772) dc59dbd2 Replace pickle by json (sonic-net/sonic-utilities#2849) a66f41c4 [show] replace shell=True, replace xml by lxml, replace exit by sys.exit (sonic-net/sonic-utilities#2666) 57500572 [utilities_common] replace shell=True (sonic-net/sonic-utilities#2718) 6e0ee3e7 [CRM][DASH] Extend CRM utility to support DASH resources. (sonic-net/sonic-utilities#2800) b2c29b0b [config] Generate sysinfo in single asic (sonic-net/sonic-utilities#2856)

…t#2642) * Add CLI configuration options for teamd retry count feature Add a SONiC CLI to more easily configure the retry count for port channels. This effectively acts like a wrapper around the underlying teamdctl command. Also add a python script that'll be installed into /usr/local/bin/teamd_increase_retry_count.py that will detect if the peer device likely supports this teamd feature (based on LLDP neighbor info) and increases the teamd retry count to 5 in preparation for warm upgrade. This script requires sudo to run. This is tied to sonic-net/sonic-buildimage#13453. Signed-off-by: Saikrishna Arcot <[email protected]> * Add test for error case from teamd when it's not running Signed-off-by: Saikrishna Arcot <[email protected]> * Fix up test cases Signed-off-by: Saikrishna Arcot <[email protected]> * Add some error handling if teamdctl doesn't exist Signed-off-by: Saikrishna Arcot <[email protected]> * Add probe functionality and sending current LACPDU packet functionality Signed-off-by: Saikrishna Arcot <[email protected]> * Check to see if the retry count feature is enabled before doing a get or set Signed-off-by: Saikrishna Arcot <[email protected]> * Add option to only send probe packets or only change retry count Signed-off-by: Saikrishna Arcot <[email protected]> * Call the teamd retry count script if doing a warm-reboot Signed-off-by: Saikrishna Arcot <[email protected]> * Fix pycheck errors, and disable scapy's IPv6 and verbose mode Scapy's IPv6 support appears to have caused some issues with older versions of scapy, which may be present on older SONiC images. Signed-off-by: Saikrishna Arcot <[email protected]> * Make teamd retry count support optional Don't fail warm reboot if teamd retry count support doesn't happen to be present. Also use fastfast-reboot for Mellanox devices. Signed-off-by: Saikrishna Arcot <[email protected]> * Address review comments, and restructure code to increase code coverage Signed-off-by: Saikrishna Arcot <[email protected]> * Address some review comments Signed-off-by: Saikrishna Arcot <[email protected]> * Replace tabs with spaces Signed-off-by: Saikrishna Arcot <[email protected]> * Verify that expected keys are present in the data returned from teamdctl Also update a failure message in the warm-reboot script if the retry count script fails. Signed-off-by: Saikrishna Arcot <[email protected]> * Fix TimeoutExpired undefined error Signed-off-by: Saikrishna Arcot <[email protected]> * Add ability to mock subprocess calls (at a limited level) Signed-off-by: Saikrishna Arcot <[email protected]> * Return an actual subprocess object, and add a test for checking timeout Signed-off-by: Saikrishna Arcot <[email protected]> * Change variable syntax Signed-off-by: Saikrishna Arcot <[email protected]> * Fix set being accessed with an index Signed-off-by: Saikrishna Arcot <[email protected]> * Add option to warm-reboot script to control if teamd retry count is required or not Signed-off-by: Saikrishna Arcot <[email protected]> * Move the teamd retry count check to before orchagent This is so that in the case of the teamd retry count check failing, there's fewer changes that happen on the system (it'll fail faster). Signed-off-by: Saikrishna Arcot <[email protected]> * Move retry count script start to be prior to point-of-no-return This doesn't need to be after the point-of-no-return, since this will detach and be sending LACPDUs on its own. Signed-off-by: Saikrishna Arcot <[email protected]> * Set executable bit Signed-off-by: Saikrishna Arcot <[email protected]> * Address PR comments Signed-off-by: Saikrishna Arcot <[email protected]> * Change to case-insensitive string contains check Signed-off-by: Saikrishna Arcot <[email protected]> * Make sure the global abort variable is used Signed-off-by: Saikrishna Arcot <[email protected]> --------- Signed-off-by: Saikrishna Arcot <[email protected]>

…nic-utilities submodule on master (sonic-net#15193) Dependency: sonic-net/sonic-utilities#2718 Why I did it This PR sonic-net/sonic-utilities#2718 reduce shell=True usage in utilities_common.cli.run_command() function. Work item tracking Microsoft ADO (number only): 15022050 How I did it Replace strings commands using utilities_common.cli.run_command() function to list of strings due to circular dependency, advance sonic-utilities submodule 72ca4848 (HEAD -> master, upstream/master, upstream/HEAD) Add CLI configuration options for teamd retry count feature (sonic-net/sonic-utilities#2642) 359dfc0c [Clock] Implement clock CLI (sonic-net/sonic-utilities#2793) b316fc27 Add transceiver status CLI to show output from TRANSCEIVER_STATUS table (sonic-net/sonic-utilities#2772) dc59dbd2 Replace pickle by json (sonic-net/sonic-utilities#2849) a66f41c4 [show] replace shell=True, replace xml by lxml, replace exit by sys.exit (sonic-net/sonic-utilities#2666) 57500572 [utilities_common] replace shell=True (sonic-net/sonic-utilities#2718) 6e0ee3e7 [CRM][DASH] Extend CRM utility to support DASH resources. (sonic-net/sonic-utilities#2800) b2c29b0b [config] Generate sysinfo in single asic (sonic-net/sonic-utilities#2856)

saiarcot895 added 3 commits January 31, 2023 18:06

Add test for error case from teamd when it's not running

b42b635

Signed-off-by: Saikrishna Arcot <[email protected]>

Fix up test cases

e9e9af0

Signed-off-by: Saikrishna Arcot <[email protected]>

saiarcot895 requested review from yxieca and vaibhavhd February 2, 2023 17:02

Add some error handling if teamdctl doesn't exist

f834b8a

Signed-off-by: Saikrishna Arcot <[email protected]>

yxieca previously approved these changes Feb 10, 2023

View reviewed changes

saiarcot895 added 2 commits April 19, 2023 18:05

Merge commit 'd433b2f954e446db7a655e882a7274cd5bce3a50' into teamd-re…

bd40c1b

…try-count-cli

Add probe functionality and sending current LACPDU packet functionality

7fc5ebd

Signed-off-by: Saikrishna Arcot <[email protected]>

saiarcot895 dismissed yxieca’s stale review via 7fc5ebd April 26, 2023 00:32

saiarcot895 added 5 commits May 4, 2023 14:19

Check to see if the retry count feature is enabled before doing a get…

b5b372b

… or set Signed-off-by: Saikrishna Arcot <[email protected]>

Add option to only send probe packets or only change retry count

c3c6b2e

Signed-off-by: Saikrishna Arcot <[email protected]>

Call the teamd retry count script if doing a warm-reboot

ad54c4c

Signed-off-by: Saikrishna Arcot <[email protected]>

Fix pycheck errors, and disable scapy's IPv6 and verbose mode

fc9195f

Scapy's IPv6 support appears to have caused some issues with older versions of scapy, which may be present on older SONiC images. Signed-off-by: Saikrishna Arcot <[email protected]>

Make teamd retry count support optional

44a6712

Don't fail warm reboot if teamd retry count support doesn't happen to be present. Also use fastfast-reboot for Mellanox devices. Signed-off-by: Saikrishna Arcot <[email protected]>

vaibhavhd reviewed May 16, 2023

View reviewed changes

saiarcot895 added 8 commits May 17, 2023 14:21

Address review comments, and restructure code to increase code coverage

5aa89b5

Signed-off-by: Saikrishna Arcot <[email protected]>

Address some review comments

1a4e17f

Signed-off-by: Saikrishna Arcot <[email protected]>

Replace tabs with spaces

bbad1e3

Signed-off-by: Saikrishna Arcot <[email protected]>

Verify that expected keys are present in the data returned from teamdctl

5acd304

Also update a failure message in the warm-reboot script if the retry count script fails. Signed-off-by: Saikrishna Arcot <[email protected]>

Merge commit '7d2ca0b' into teamd-retry-count-cli

0f4f822

Fix TimeoutExpired undefined error

e6acfe0

Signed-off-by: Saikrishna Arcot <[email protected]>

Add ability to mock subprocess calls (at a limited level)

9e2d7a3

Signed-off-by: Saikrishna Arcot <[email protected]>

Return an actual subprocess object, and add a test for checking timeout

65c1bdb

Signed-off-by: Saikrishna Arcot <[email protected]>

saiarcot895 requested review from vaibhavhd and yxieca May 19, 2023 18:34

yxieca reviewed May 20, 2023

View reviewed changes

saiarcot895 added 3 commits May 22, 2023 11:29

Change variable syntax

edea4ce

Signed-off-by: Saikrishna Arcot <[email protected]>

Fix set being accessed with an index

f76e1e9

Signed-off-by: Saikrishna Arcot <[email protected]>

Add option to warm-reboot script to control if teamd retry count is r…

e547f50

…equired or not Signed-off-by: Saikrishna Arcot <[email protected]>

saiarcot895 added 3 commits May 23, 2023 12:50

Move the teamd retry count check to before orchagent

a5966f2

This is so that in the case of the teamd retry count check failing, there's fewer changes that happen on the system (it'll fail faster). Signed-off-by: Saikrishna Arcot <[email protected]>

Move retry count script start to be prior to point-of-no-return

1670cb7

This doesn't need to be after the point-of-no-return, since this will detach and be sending LACPDUs on its own. Signed-off-by: Saikrishna Arcot <[email protected]>

Set executable bit

ee72908

Signed-off-by: Saikrishna Arcot <[email protected]>

saiarcot895 requested a review from yxieca May 26, 2023 17:11

yxieca previously approved these changes May 26, 2023

View reviewed changes

vaibhavhd reviewed May 27, 2023

View reviewed changes

saiarcot895 added 2 commits May 30, 2023 09:31

Address PR comments

c719f32

Signed-off-by: Saikrishna Arcot <[email protected]>

Change to case-insensitive string contains check

01852c6

Signed-off-by: Saikrishna Arcot <[email protected]>

saiarcot895 dismissed yxieca’s stale review via 01852c6 May 30, 2023 16:33

vaibhavhd previously approved these changes May 30, 2023

View reviewed changes

Make sure the global abort variable is used

18fba7c

Signed-off-by: Saikrishna Arcot <[email protected]>

saiarcot895 dismissed vaibhavhd’s stale review via 18fba7c June 2, 2023 18:55

vaibhavhd approved these changes Jun 2, 2023

View reviewed changes

saiarcot895 merged commit 72ca484 into sonic-net:master Jun 2, 2023

saiarcot895 deleted the teamd-retry-count-cli branch June 2, 2023 20:34

maipbui mentioned this pull request Jun 4, 2023

modify commands using utilities_common.cli.run_command and advance sonic-utilities submodule on master sonic-net/sonic-buildimage#15193

Merged

10 tasks

dprital mentioned this pull request Jun 5, 2023

[submodule] Advance sonic-utilities pointer sonic-net/sonic-buildimage#15336

Closed

10 tasks

saiarcot895 mentioned this pull request Jun 29, 2023

HLD for changing teamd expiry timer sonic-net/SONiC#1073

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CLI configuration options for teamd retry count feature #2642

Add CLI configuration options for teamd retry count feature #2642

saiarcot895 commented Feb 1, 2023 •

edited

Loading

yxieca May 20, 2023

saiarcot895 May 22, 2023

vaibhavhd left a comment

vaibhavhd May 27, 2023

saiarcot895 May 30, 2023

vaibhavhd May 27, 2023

saiarcot895 May 30, 2023

saiarcot895 May 30, 2023

vaibhavhd May 27, 2023

saiarcot895 May 30, 2023

vaibhavhd May 27, 2023

vaibhavhd May 27, 2023

vaibhavhd May 27, 2023

saiarcot895 May 30, 2023

vaibhavhd May 27, 2023

saiarcot895 May 30, 2023

vaibhavhd May 27, 2023

saiarcot895 May 30, 2023

vaibhavhd May 27, 2023

saiarcot895 May 30, 2023

Add CLI configuration options for teamd retry count feature #2642

Add CLI configuration options for teamd retry count feature #2642

Conversation

saiarcot895 commented Feb 1, 2023 • edited Loading

What I did

How I did it

How to verify it

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)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vaibhavhd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saiarcot895 commented Feb 1, 2023 •

edited

Loading