[chassisd] Address the chassisd crash issue and add UT for it #573
Conversation
Signed-off-by: mlok <[email protected]>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@judyjoseph and @arlakshm This PR address the chassisd crash issue. I also add UT check module_db_update support slot number or slot string. Please review |
There was a problem hiding this comment.
LGTM.
@mlok-nokia can you also confirm it will not conflict with these two platform API's module.get_slot(), chassis.get_my_slot() which as per commens will return int.
|
@kenneth-arista @anamehra could you check in your platforms too if this change is ok ? |
|
@kenneth-arista, please review this change |
| if not self._is_supervisor(): | ||
| module_info_dict = self._get_module_info(my_index) | ||
| hostname_key = "{}{}".format(ModuleBase.MODULE_TYPE_LINE, int(self.my_slot) - 1) |
There was a problem hiding this comment.
What is the expectation here? This makes an assumption that line card slots must be numbers, but should that be a requirement? Should this be using my_index instead? Or should there be some logic to handle if the LC slot was non-numeric?
There was a problem hiding this comment.
Move the self._get_module_info() call inside if condition since my_slot is only use inside. Not need to call self._get_module_info(0 if it not required.
There was a problem hiding this comment.
But this is still requiring that line cards must be named as numbers, only supervisors are allowed to be alphanumeric. Is that expected and required? Why can't alphanumeric line card names also be supported since this was already found to be a limitation of supervisor naming?
There was a problem hiding this comment.
Yes. Linecard slot must be number string or number. It is required. Otherwise, we will not able construct the linecard name. With the existing implementation, Linecard slot is a number or a number string, it will work. BTW, this change just moved the _get_module_info() call into "if" section since we only use it inside the "if".
There was a problem hiding this comment.
The line card name could be constructed if the slot wasn't numeric (like the supervisor) as well, the name is still just a string. I'm not sure what else relies on line card slots being numeric though, so this should be fine for now. It may be worth looking into what changes would be required to support alphanumeric slots for line cards as well, but not required for this change.
| @@ -393,7 +393,7 @@ class ModuleUpdater(logger.Logger): | |||
|
|
|||
| module_info_dict[CHASSIS_MODULE_INFO_NAME_FIELD] = name | |||
| module_info_dict[CHASSIS_MODULE_INFO_DESC_FIELD] = str(desc) | |||
| module_info_dict[CHASSIS_MODULE_INFO_SLOT_FIELD] = str(slot) | |||
| module_info_dict[CHASSIS_MODULE_INFO_SLOT_FIELD] = slot | |||
There was a problem hiding this comment.
This may be an unexpected behavior change. slot can still be an integer, so module_info_dict[CHASSIS_MODULE_INFO_SLOT_FIELD] can now have varying types. Previously when slot was always int, all logic was expecting this field to be str` since it was converted here (though I'm not sure any logic relies on that type). It may be best to revert this change to maintain a consistent (and existing) type requirement.
I do see there is a conversion for one of the usages of this dict entry to str to account for this, but it may just be best to maintain the expectation that this dict entry is always str from this point, as it was previously.
There was a problem hiding this comment.
The fundamental issue is that a slot value on a chassis may not be a number, it could be a string -- such as Nokia IXRE7250 Supervisor card which is slot "A".
It is not necessary to convert slot value to a string in the module_info_dict[] here and then convert it back to a number when use it later. We can directly use it as what function get_slot() returned. For consistency, the only place is needed to be converted to a string when it is stored into a STATE_DB. This change simplfy it and no need to convert it back and forth (StringToNumber or NumberToString) when use the slot info in module_info_dict[]. This change will address this issue forever. Otherwise, another solution could be a big change.
There was a problem hiding this comment.
Sounds good. My concern was mostly that it may be unclear to developers that the type stored in this dict field can vary. Functionally it seems fine. The documentation of APIs such as get_slot and get_my_slot should be updated as @judyjoseph previously mentioned; they not need not return only an int, they may also return str.
Yes. Based on the design, both functions should return the same type and same value. Otherwise, the existing code doesn't work. |
Description
On Nokia platform, slot name of Supervisor is string "A" instead of a number. Using "int" to convert it could cause issue backtrace. We should use slot value to any checking without any conversion. This will fixes sonic-net/sonic-buildimage#21131
Motivation and Context
Modify the _get_module_info not to convert "slot" to a string value. And also modify the code not to convert slot value to an to do any checking. Just directly use the returned value of get_slot(). Also add UT test_moduleupdater_check_slot_string() to valid it.
How Has This Been Tested?
Tested on 202405 branch
Which release branch to backport (provide reason below if selected)
Additional Information (Optional)