Added SmartSwitch support in chassisd and enabling chassisd #467

rameshraghupathy · 2024-04-15T03:38:50Z

Added SmartSwitch support in chassisd and enabling chassisd for fixed SmartSwitches

Description

chassisd is enabled only for modular chassis. Smartswitch is a fixed chassis. However it has been treated like a modular chassis to manage the DPU cards just like the line-cards of a modular chassis. chassisd will be enabled only on the smartswitch NPU and hence the scope of these changes are limited only to NPU and not applicable to DPU.

Motivation and Context

chassisd is enabled only for modular chassis. Smartswitch is a fixed chassis. However it has been treated like a modular chassis to manage the DPU cards just like the line-cards of a modular chassis. Hence, chassid is needed for SmartSwitch and enabled here. Also, some of the table updates and clean up are not required for SmartSwitch platform and hence using the is_smartswitch API to selectively enable it. the text "fixes #xxxx", "closes #xxxx" or "resolves #xxxx" here

How Has This Been Tested?

Enabled and built and image and checked if the chassisd is running all the time without crashing
Verify if the config change handler is working as expected by issuing config CLIs to startup and shutdown DPUs

Additional Information (Optional)

sonic-chassisd/scripts/chassisd

SmartSwitches

platform identifier.

temp fix to get the build passing.

sonic-chassisd/scripts/chassisd

oleksandrivantsiv · 2024-06-11T01:57:45Z

What are the states supported by the DPUs in the Smart Switch?

rameshraghupathy · 2024-06-11T02:22:09Z

What are the states supported by the DPUs in the Smart Switch?

”dpu_midplane_link_state”
”dpu_control_plane_state"
"dpu_data_plane_state"

sonic-chassisd/scripts/chassisd

accidental removal of a few lines from the original chassisd file

sonic-chassisd/tests/test_chassisd.py

oleksandrivantsiv · 2024-07-16T21:08:27Z

sonic-chassisd/scripts/chassisd

@@ -184,7 +188,7 @@ class ModuleUpdater(logger.Logger):
        self.chassis = chassis


I think that instead of modifying the existing ModuleUpdater class, we should implement SmartSwitchModuleUpdater(ModuleUpdater). SmartSwitchModuleUpdater should be derived from ModuleUpdater and overwrite the methods that should behave differently for the Smart Switch. This approach allows us to keep the original implementation untouched and guarantees full backward compatibility with the chassisd.

Will consider this.

Implemented this change.

@oleksandrivantsiv

sonic-chassisd/tests/test_chassisd.py

oleksandrivantsiv

as commented

prgeor · 2024-10-08T14:21:27Z

sonic-chassisd/scripts/chassisd

+
+            if op == 'SET':
+                admin_state = MODULE_ADMIN_DOWN
+            elif op == 'DEL':


@rameshraghupathy on DEL of the admin state why do module ADMIN UP?

@rameshraghupathy on DEL of the admin state why do module ADMIN UP?

@prgeor The original behavior is: The default is no config - meaning admin up when the user wants to shutdown a module adds a config "admins_status: down". So, the SET operation has been used for "admins_status: down" and DEL has been used for setting the module up. If you want we can change this for Smartswitch. Please let me know.

@rameshraghupathy didn't we agree to keep the default behavior on missing of the config DB admin status to have dark mode enabled as default? @vvolam is that not captured in the HLD?

@rameshraghupathy didn't we agree to keep the default behavior on missing of the config DB admin status to have dark mode enabled as default? @vvolam is that not captured in the HLD?

@prgeor Yes, that is taken care by "def set_initial_dpu_admin_state(self):"

vvolam · 2024-10-09T20:31:39Z

sonic-chassisd/scripts/chassisd

@@ -73,6 +73,7 @@ CHASSIS_MODULE_REBOOT_INFO_TABLE = 'CHASSIS_MODULE_REBOOT_INFO_TABLE'
 CHASSIS_MODULE_REBOOT_TIMESTAMP_FIELD = 'timestamp'
 CHASSIS_MODULE_REBOOT_REBOOT_FIELD = 'reboot'
 DEFAULT_LINECARD_REBOOT_TIMEOUT = 180
+DEFAULT_DPU_REBOOT_TIMEOUT = 180


Is 180 secs sufficient for entire DPU reboot time?

Also, should we add this to platform.json as vendor configurable timeout?

This may not be sufficient for a reboot as I told you. This is good for shutdown.
Here is what may work:
shutdown: pre_shutdown_time + graceful shutdown time + 180s for proper HW shutdown (240s is safe)
reboot: shutdown (240s) + hw off-on requirement if any (say 30s) + 180s for power on

A vendor configurable option is a good thing.

We can pick a number now and optimize it as we move.

@rameshraghupathy during the HDL review sessions we agreed that this value is vendor specific and should be taken from platform.json. Please change the implementation based on our agreement

@oleksandrivantsiv Yes, agree, but the existing modular-chassis LC use "platform_env.conf". Shall we be consistent with that?
PLATFORM_ENV_CONF_FILE = "/usr/share/sonic/platform/platform_env.conf"
self.linecard_reboot_timeout = DEFAULT_LINECARD_REBOOT_TIMEOUT
if os.path.isfile(PLATFORM_ENV_CONF_FILE):
with open(PLATFORM_ENV_CONF_FILE, 'r') as file:
for line in file:
field = line.split('=')[0].strip()
if field == "linecard_reboot_timeout":
self.linecard_reboot_timeout = int(line.split('=')[1].strip())

vvolam · 2024-10-09T20:46:15Z

sonic-chassisd/scripts/chassisd

+        :return:
+        """
+
+    def module_config_update(self, key, admin_state):


@rameshraghupathy could you please add a quick summary of function here and also remaining functions for future clarity?

prgeor · 2024-10-10T21:45:57Z

sonic-chassisd/scripts/chassisd

+        self.chassis = chassis
+        # The threading.Lock is required for concurrent setting of admin_state
+        # on multiple DPUs for faster bring-up and shutdown.
+        self.lock = threading.Lock()
+


@rameshraghupathy which data is be protected against multiple thread access?

@prgeor It is hard to predict and say what vendor API would do under the "submit_callback". When multiple threads are forked chassis/modules resources can be corrupted. One thread can increase the chassis.number_of_modules and the other can decrease it causing data corruption and unpredictable states and behavior. In fact it is good have another one in submit_callback as well. But I haven't tested putting a lock in submit_callback.

prgeor · 2024-10-10T22:03:52Z

sonic-chassisd/scripts/chassisd

+        """Send admin_state trigger once to modules those are powered up"""
+        lock = threading.Lock()
+        threads = []
+        for module_index in range(0, self.module_updater.num_modules):


@rameshraghupathy I am not sure why we cannot handle all DPUs in one single thread.

@rameshraghupathy I am not sure why we cannot handle all DPUs in one single thread.
@prgeor It is not that we can't. We can but the call goes sequentially to each DPU and power cycling is a time consuming process and each DPU requires roughly 3 mins. So to power cycle a chassis we need 24+ mins. When we fork all 8 finishes in 3 mins

oleksandrivantsiv · 2024-10-10T23:24:55Z

sonic-chassisd/scripts/chassisd

+            with self.lock:
+                # Submit the callback function as a separate thread. It is the responsibility
+                # of the module to handle multiple b2b requests to the same module
+                t = threading.Thread(target=self.submit_callback, args=(module_index, admin_state))


Duplicating the comment since it was marked as resolved:

The lock is valid until the start of the thread. It is safe to have that as multiple threads may access common resources in chassis and modules causing data corruption. Once the thread is created it has its own stack. In fact it is good to have a lock inside the submit_callback. It is hard to find what common resources are accessed inside each vendor API. So, it is better to have a lock even in submit_callback.

This lock doesn’t protect anything. It is created and acquired in the main thread, but none of the child threads use it.

gpunathilell · 2024-10-15T02:00:49Z

sonic-chassisd/scripts/chassisd

+                        def handle_dpu_logic():
+                            try:
+                                with lock:
+                                    module = self.module_updater.chassis.get_module(module_index)


The module_index and op variables here are module specific, so these variables should be function arguments for handle_dpu_logic. With the current implementation there is a good probability that the module_index and op are already updated by the time a thread has started for the previous module, Example scenario:
DPU1 : op = MODULE_ADMIN_UP and module_index = 1 thread.start() is called (but thread has still not started yet or lock is not yet obtained - > this is thread1)
We move to the next element in the for loop in for module_index in range(0, self.module_updater.num_modules):
DPU2 : op = MODULE_ADMIN_DOWN and module_index = 2 thread.start() is called -> this is thread2.
At this point if thread1 starts executing the DPU2 related admin down operation is handled while the DPU1 admin up operation is skipped (Since op and module_index are already updated)

gpunathilell · 2024-10-15T16:45:30Z

sonic-chassisd/scripts/chassisd

+                if module_info_dict is not None:
+                    # Get admin state of DPU
+                    key = module_info_dict[CHASSIS_MODULE_INFO_NAME_FIELD]
+                    admin_state = self.module_updater.get_module_admin_status(key)


As per this comment and this comment the set_initial_dpu_admin_state is supposed to handle the dark mode requirement (If there is no CONFIG_DB CHASSIS_MODULE entries for the DPU then the admin state for the DPU should be down) but in this function, the get_module_admin_status would return 'up' by default if there is no CONFIG_DB entry, meaning op = 'up' which would power on the dpu, but by default the DPU should be powered off if there is no CONFIG_DB entry

chassisd db clean up on admin_down state, removed the thread locks

oleksandrivantsiv · 2024-10-24T19:01:20Z

sonic-chassisd/tests/mock_platform.py

@@ -75,13 +75,25 @@ def set_midplane_reachable(self, up):
    def get_all_asics(self):
        return self.asic_list

+    def get_reboot_cause(self):
+        reboot_cause = {


Where does this schema come from? This not what is written in the HLD:

Retrieves the cause of the previous reboot of the DPU module Returns: A tuple (string, string) where the first element is a string containing the cause of the previous reboot. This string must be one of the predefined strings in this class. If the first string is "REBOOT_CAUSE_HARDWARE_OTHER", the second string can be used to pass a description of the reboot cause.

https://github.com/sonic-net/SONiC/blob/master/doc/smart-switch/pmon/smartswitch-pmon.md#get-dpu-reboot-cause

oleksandrivantsiv · 2024-10-24T19:03:38Z

sonic-chassisd/scripts/chassisd

+                    if current_status != ModuleBase.MODULE_STATUS_OFFLINE:
+                        # DPU module has been turned on, update the reboot-cause
+                        # If the reboot-cause file not already present save it
+                        # If the STATE_DB REBOOT_CAUSE table already doesn't have


Why STATE_DB? Should it be CHASSIS_STATE_DB?

oleksandrivantsiv · 2024-10-24T19:07:59Z

sonic-chassisd/scripts/chassisd

+                        # If the reboot-cause file not already present save it
+                        # If the STATE_DB REBOOT_CAUSE table already doesn't have
+                        # an entry for the recent reboot update the DB
+                        reboot_cause = try_get(self.chassis.get_module(module_index).get_reboot_cause)


Where is the reboot cause populated in the CHASSIS_DB? I don't see the implementation for this. The reboot cause is read into a local variable and that is it. Nothing else is done with the variable

oleksandrivantsiv · 2024-10-24T19:11:24Z

Please implement a unit test for reboot_cause implementation

oleksandrivantsiv · 2024-10-24T19:32:21Z

@rameshraghupathy, @prgeor please extend the HLD with the flows for reboot_case and reboot_case_history. From this implementation, it is not clear how it works for the platforms that support the get_reboot_causes API without information in the CHASSIS_STATE_DB

seamless migration to lightup mode, dpu_state db update when midplane fails, persisting dpu reboot cause etc

converting it to string format

oleksandrivantsiv suggested changes Apr 25, 2024

View reviewed changes

sonic-chassisd/scripts/chassisd Outdated Show resolved Hide resolved

oleksandrivantsiv reviewed Apr 26, 2024

View reviewed changes

sonic-chassisd/scripts/chassisd Outdated Show resolved Hide resolved

Added SmartSwitch support in chassisd and enabling chassisd for fixed

0785bb1

SmartSwitches

rameshraghupathy force-pushed the master branch from d5ed9fc to 0785bb1 Compare April 26, 2024 22:03

rameshraghupathy added 2 commits April 28, 2024 13:28

Made temp fix to avoid chassisd tests passing. Need to handle it using

c4725c4

platform identifier.

The test_chassisd needs to updated for smartswitch. The change is a

d1a6b2a

temp fix to get the build passing.

rameshraghupathy mentioned this pull request Apr 29, 2024

PMON High Level Design for SmartSwitch (Replaces the PR #1538) sonic-net/SONiC#1584

Merged

rameshraghupathy added 4 commits May 31, 2024 12:09

Addressing review comments

af4ff55

Merge remote-tracking branch 'upstream/master'

356fa8b

Fixed the merge conflict breakage in "asic_table"

97b6518

checking test_chassisd.py is ok

60cb8ed

rameshraghupathy force-pushed the master branch from 4f9d602 to 60cb8ed Compare June 5, 2024 20:00

rameshraghupathy added 3 commits June 5, 2024 13:13

Trying to resolve test failure

cd6db9b

Using is_smartswitch in MockChassis

8e958ba

Disabling the test until the dependency is committed

fb7b4ac

oleksandrivantsiv reviewed Jun 11, 2024

View reviewed changes

sonic-chassisd/scripts/chassisd Outdated Show resolved Hide resolved

oleksandrivantsiv reviewed Jun 11, 2024

View reviewed changes

sonic-chassisd/scripts/chassisd Outdated Show resolved Hide resolved

oleksandrivantsiv reviewed Jun 11, 2024

View reviewed changes

sonic-chassisd/scripts/chassisd Outdated Show resolved Hide resolved

Addressing review comments

312129f

vvolam reviewed Jun 19, 2024

View reviewed changes

sonic-chassisd/scripts/chassisd Outdated Show resolved Hide resolved

sonic-chassisd/scripts/chassisd Show resolved Hide resolved

Addressed review comments: 1. removed SWITCH module 2. Restored

ce3bc30

accidental removal of a few lines from the original chassisd file

oleksandrivantsiv reviewed Jul 16, 2024

View reviewed changes

sonic-chassisd/tests/test_chassisd.py Outdated Show resolved Hide resolved

oleksandrivantsiv reviewed Jul 16, 2024

View reviewed changes

sonic-chassisd/tests/test_chassisd.py Outdated Show resolved Hide resolved

oleksandrivantsiv reviewed Jul 16, 2024

View reviewed changes

sonic-chassisd/tests/test_chassisd.py Outdated Show resolved Hide resolved

oleksandrivantsiv suggested changes Jul 16, 2024

View reviewed changes

rameshraghupathy added 2 commits August 5, 2024 15:58

As per one of the review comments created derived class for SmartSwitch

ca1fe7c

Uncommented the previously commented blocks in test_chassisd

b2ac82c

prgeor reviewed Oct 8, 2024

View reviewed changes

vvolam reviewed Oct 9, 2024

View reviewed changes

prgeor reviewed Oct 10, 2024

View reviewed changes

oleksandrivantsiv reviewed Oct 10, 2024

View reviewed changes

Added a docstring and updated a comment

2ae658d

gpunathilell reviewed Oct 15, 2024

View reviewed changes

rameshraghupathy added 3 commits October 18, 2024 17:47

Fixed a thread issue and removed all locks

a55ecac

Added support to persist reboot-cause, user defined reboot timeout,

54accfe

chassisd db clean up on admin_down state, removed the thread locks

Added get_reboot_cause() to mock_platform.py

466f6d3

oleksandrivantsiv reviewed Oct 24, 2024

View reviewed changes

Added the necessary changes for dark-mode dpu initial admin status,

43e6b61

seamless migration to lightup mode, dpu_state db update when midplane fails, persisting dpu reboot cause etc

rameshraghupathy force-pushed the master branch from 91d0a47 to 43e6b61 Compare October 29, 2024 22:38

rameshraghupathy added 11 commits October 29, 2024 16:23

Fixing test issues

5a81fc7

Added a mock for file open

53ce3a6

working on test coverage

094c0ad

Initialized previous reboot time

55fd678

Fixed time format

c424271

For some reason the test thinks that "reboot_cause" is a dict. So

90e9825

converting it to string format

checking if the file_path exists before creating a new symlink

ee11414

Adding some error handling

153962b

Adding error handling

465bf31

Adding error handling

1de545e

Adding error handling

d2969bd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added SmartSwitch support in chassisd and enabling chassisd #467

Added SmartSwitch support in chassisd and enabling chassisd #467

rameshraghupathy commented Apr 15, 2024 •

edited

Loading

oleksandrivantsiv commented Jun 11, 2024

rameshraghupathy commented Jun 11, 2024

oleksandrivantsiv Jul 16, 2024

rameshraghupathy Jul 19, 2024 •

edited

Loading

rameshraghupathy Aug 5, 2024

rameshraghupathy Sep 3, 2024

oleksandrivantsiv left a comment

prgeor Oct 8, 2024

rameshraghupathy Oct 9, 2024

prgeor Oct 10, 2024

rameshraghupathy Oct 10, 2024 •

edited

Loading

vvolam Oct 9, 2024

vvolam Oct 9, 2024

rameshraghupathy Oct 11, 2024

rameshraghupathy Oct 11, 2024

rameshraghupathy Oct 11, 2024

oleksandrivantsiv Oct 18, 2024

rameshraghupathy Oct 22, 2024 •

edited

Loading

vvolam Oct 9, 2024

prgeor Oct 10, 2024

rameshraghupathy Oct 11, 2024

prgeor Oct 10, 2024

rameshraghupathy Oct 10, 2024

oleksandrivantsiv Oct 10, 2024

rameshraghupathy Oct 11, 2024

oleksandrivantsiv Oct 11, 2024

gpunathilell Oct 15, 2024 •

edited

Loading

gpunathilell Oct 15, 2024 •

edited

Loading

oleksandrivantsiv Oct 24, 2024

oleksandrivantsiv Oct 24, 2024

oleksandrivantsiv Oct 24, 2024

oleksandrivantsiv Oct 24, 2024

oleksandrivantsiv commented Oct 24, 2024

oleksandrivantsiv commented Oct 24, 2024

		@@ -184,7 +188,7 @@ class ModuleUpdater(logger.Logger):
		self.chassis = chassis

Added SmartSwitch support in chassisd and enabling chassisd #467

Are you sure you want to change the base?

Added SmartSwitch support in chassisd and enabling chassisd #467

Conversation

rameshraghupathy commented Apr 15, 2024 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Additional Information (Optional)

oleksandrivantsiv commented Jun 11, 2024

rameshraghupathy commented Jun 11, 2024

Choose a reason for hiding this comment

rameshraghupathy Jul 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleksandrivantsiv 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

rameshraghupathy Oct 10, 2024 • edited Loading

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

rameshraghupathy Oct 22, 2024 • edited Loading

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

gpunathilell Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

gpunathilell Oct 15, 2024 • edited Loading

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

oleksandrivantsiv commented Oct 24, 2024

oleksandrivantsiv commented Oct 24, 2024

rameshraghupathy commented Apr 15, 2024 •

edited

Loading

rameshraghupathy Jul 19, 2024 •

edited

Loading

rameshraghupathy Oct 10, 2024 •

edited

Loading

rameshraghupathy Oct 22, 2024 •

edited

Loading

gpunathilell Oct 15, 2024 •

edited

Loading

gpunathilell Oct 15, 2024 •

edited

Loading