Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chassisd: Fix crash on exit on linecard #347

Merged

Conversation

patrickmacarthur
Copy link
Contributor

Description

Set the config_manager variable to None if we are running on a linecard and thus don't need to set up the config manager.

Motivation and Context

During cleanup, the chassid service tries to clean up the config_manager, but the config_manager variable is only ever initialized if we are on the supervisor. Thus, checking if it is None is insufficient because this results in an UnboundLocalError that prevents the cleanup from succeeding on a linecard.

How Has This Been Tested?

This was tested via the sonic-mgmt platform_tests/daemon/test_chassisd.py test. Without this change, the test_pmon_chassisd_stop_and_start_status test would fail and a traceback would be present in syslog. The test passes with this patch applied.

Additional Information (Optional)

Copy link

@wenyiz2021 wenyiz2021 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@patrickmacarthur thanks for the fix, can you please add a UT to cover this? PR is blocked by coverage check

@wenyiz2021 wenyiz2021 requested a review from abdosi March 16, 2023 22:14
@abdosi
Copy link
Contributor

abdosi commented Mar 17, 2023

@gechiang for viz.

abdosi
abdosi previously approved these changes Mar 17, 2023
@gechiang
Copy link
Contributor

@patrickmacarthur can you also include a test to cover the one line change you fixed? This is required before the change can be merged.
You can reference this original PR to see how the testcases were originally added and add one test to cover your new change for non-supervisor case.
#97

@abdosi
Copy link
Contributor

abdosi commented Mar 20, 2023

@bmridul

We try to access config_manager, even if it wasn't set. Unconditionally
set it to resolve the issue.
@patrickmacarthur
Copy link
Contributor Author

/azpw run Azure.sonic-platform-daemons

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-platform-daemons

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@gechiang gechiang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@gechiang gechiang merged commit 11d438a into sonic-net:master Mar 23, 2023
yxieca pushed a commit that referenced this pull request Mar 24, 2023
We try to access config_manager, even if it wasn't set. Unconditionally
set it to resolve the issue.
@StormLiangMS
Copy link

@patrickmacarthur cherry pick to 202211 with conflict, could you submit PR separately for 202211?
@gechiang @abdosi for vis.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants