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 sarcot@microsoft.com

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 <sarcot@microsoft.com>

Signed-off-by: Saikrishna Arcot <sarcot@microsoft.com>

…try-count-cli

Signed-off-by: Saikrishna Arcot <sarcot@microsoft.com>

… or set Signed-off-by: Saikrishna Arcot <sarcot@microsoft.com>

Signed-off-by: Saikrishna Arcot <sarcot@microsoft.com>

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 <sarcot@microsoft.com>

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 <sarcot@microsoft.com>

config/main.py

scripts/fast-reboot

scripts/teamd_increase_retry_count.py

scripts/fast-reboot

Signed-off-by: Saikrishna Arcot <sarcot@microsoft.com>

Also update a failure message in the warm-reboot script if the retry count script fails. Signed-off-by: Saikrishna Arcot <sarcot@microsoft.com>

Signed-off-by: Saikrishna Arcot <sarcot@microsoft.com>

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 <sarcot@microsoft.com>

…equired or not Signed-off-by: Saikrishna Arcot <sarcot@microsoft.com>

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 <sarcot@microsoft.com>

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 <sarcot@microsoft.com>

Signed-off-by: Saikrishna Arcot <sarcot@microsoft.com>

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 <sarcot@microsoft.com>

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 <drorp@nvidia.com>

…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 <sarcot@microsoft.com> * Add test for error case from teamd when it's not running Signed-off-by: Saikrishna Arcot <sarcot@microsoft.com> * Fix up test cases Signed-off-by: Saikrishna Arcot <sarcot@microsoft.com> * Add some error handling if teamdctl doesn't exist Signed-off-by: Saikrishna Arcot <sarcot@microsoft.com> * Add probe functionality and sending current LACPDU packet functionality Signed-off-by: Saikrishna Arcot <sarcot@microsoft.com> * Check to see if the retry count feature is enabled before doing a get or set Signed-off-by: Saikrishna Arcot <sarcot@microsoft.com> * Add option to only send probe packets or only change retry count Signed-off-by: Saikrishna Arcot <sarcot@microsoft.com> * Call the teamd retry count script if doing a warm-reboot Signed-off-by: Saikrishna Arcot <sarcot@microsoft.com> * 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 <sarcot@microsoft.com> * 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 <sarcot@microsoft.com> * Address review comments, and restructure code to increase code coverage Signed-off-by: Saikrishna Arcot <sarcot@microsoft.com> * Address some review comments Signed-off-by: Saikrishna Arcot <sarcot@microsoft.com> * Replace tabs with spaces Signed-off-by: Saikrishna Arcot <sarcot@microsoft.com> * 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 <sarcot@microsoft.com> * Fix TimeoutExpired undefined error Signed-off-by: Saikrishna Arcot <sarcot@microsoft.com> * Add ability to mock subprocess calls (at a limited level) Signed-off-by: Saikrishna Arcot <sarcot@microsoft.com> * Return an actual subprocess object, and add a test for checking timeout Signed-off-by: Saikrishna Arcot <sarcot@microsoft.com> * Change variable syntax Signed-off-by: Saikrishna Arcot <sarcot@microsoft.com> * Fix set being accessed with an index Signed-off-by: Saikrishna Arcot <sarcot@microsoft.com> * Add option to warm-reboot script to control if teamd retry count is required or not Signed-off-by: Saikrishna Arcot <sarcot@microsoft.com> * 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 <sarcot@microsoft.com> * 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 <sarcot@microsoft.com> * Set executable bit Signed-off-by: Saikrishna Arcot <sarcot@microsoft.com> * Address PR comments Signed-off-by: Saikrishna Arcot <sarcot@microsoft.com> * Change to case-insensitive string contains check Signed-off-by: Saikrishna Arcot <sarcot@microsoft.com> * Make sure the global abort variable is used Signed-off-by: Saikrishna Arcot <sarcot@microsoft.com> --------- Signed-off-by: Saikrishna Arcot <sarcot@microsoft.com>

…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 <sarcot@microsoft.com>

Fix up test cases

e9e9af0

Signed-off-by: Saikrishna Arcot <sarcot@microsoft.com>

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 <sarcot@microsoft.com>

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 <sarcot@microsoft.com>

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 <sarcot@microsoft.com>

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

c3c6b2e

Signed-off-by: Saikrishna Arcot <sarcot@microsoft.com>

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

ad54c4c

Signed-off-by: Saikrishna Arcot <sarcot@microsoft.com>

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 <sarcot@microsoft.com>

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 <sarcot@microsoft.com>

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 <sarcot@microsoft.com>

Address some review comments

1a4e17f

Signed-off-by: Saikrishna Arcot <sarcot@microsoft.com>

Replace tabs with spaces

bbad1e3

Signed-off-by: Saikrishna Arcot <sarcot@microsoft.com>

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 <sarcot@microsoft.com>

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

0f4f822

Fix TimeoutExpired undefined error

e6acfe0

Signed-off-by: Saikrishna Arcot <sarcot@microsoft.com>

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

9e2d7a3

Signed-off-by: Saikrishna Arcot <sarcot@microsoft.com>

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

65c1bdb

Signed-off-by: Saikrishna Arcot <sarcot@microsoft.com>

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 <sarcot@microsoft.com>

Fix set being accessed with an index

f76e1e9

Signed-off-by: Saikrishna Arcot <sarcot@microsoft.com>

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

e547f50

…equired or not Signed-off-by: Saikrishna Arcot <sarcot@microsoft.com>

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 <sarcot@microsoft.com>

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 <sarcot@microsoft.com>

Set executable bit

ee72908

Signed-off-by: Saikrishna Arcot <sarcot@microsoft.com>

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 <sarcot@microsoft.com>

Change to case-insensitive string contains check

01852c6

Signed-off-by: Saikrishna Arcot <sarcot@microsoft.com>

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 <sarcot@microsoft.com>

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
5 checks passed

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