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

Stop PMON docker before cold and soft reboots #1514

Merged
merged 7 commits into from
Apr 15, 2021

Conversation

aravindmani-1
Copy link
Contributor

@aravindmani-1 aravindmani-1 commented Mar 18, 2021

What I did

Prevent potential kernel oops if drivers are removed/devices are deinitialized while PMon daemons are still trying to access those devices.

How I did it

Edit reboot script and stop pmon docker and services

How to verify it

Check docker ps output just before cold boot is being executed.

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)

UT:
cold_boot_UT.txt

@aravindmani-1
Copy link
Contributor Author

@jleveque Please review this PR.

scripts/reboot Outdated
@@ -178,6 +190,8 @@ if [ -x ${DEVPATH}/${PLATFORM}/${PLATFORM_UPDATE_REBOOT_CAUSE} ]; then
${DEVPATH}/${PLATFORM}/${PLATFORM_UPDATE_REBOOT_CAUSE}
fi

stop_pmon_service
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand you want to stop this at the last possible moment, but does it just make things simpler to add the logic to the existing stop_sonic_services() function?

Copy link
Contributor Author

@aravindmani-1 aravindmani-1 Mar 18, 2021

Choose a reason for hiding this comment

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

If there is any platform API is introduced/invoked in between, it will fail as PMON is shutdown early.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see a problem moving stop_sonic_service to here.

Copy link
Contributor

Choose a reason for hiding this comment

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

@aravindthoram can we consolidate the change in stop_sonic_services and move the call to there?

Copy link
Contributor Author

@aravindmani-1 aravindmani-1 Mar 27, 2021

Choose a reason for hiding this comment

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

@yxieca I'll commit the new changes soon.

Copy link
Contributor

@jleveque jleveque left a comment

Choose a reason for hiding this comment

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

Once all comments are addressed, please also add the same logic to the new soft-reboot script as part of this PR: scripts/soft-reboot

@aravindmani-1
Copy link
Contributor Author

@yxieca , @jleveque can you please review the latest changes?.

@jleveque
Copy link
Contributor

jleveque commented Apr 9, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

scripts/reboot Outdated Show resolved Hide resolved
@aravindmani-1
Copy link
Contributor Author

@yxieca Addressed the comments.

@jleveque jleveque changed the title Stop PMON docker before cold boot Stop PMON docker before cold and soft reboots Apr 12, 2021
@jleveque jleveque merged commit 0e84418 into sonic-net:master Apr 15, 2021
@aravindmani-1 aravindmani-1 deleted the Stop_PMon_in_coldboot branch April 19, 2021 04:47
daall pushed a commit that referenced this pull request Apr 21, 2021
Prevent potential kernel oops if drivers are removed/devices are deinitialized while PMon daemons are still trying to access those devices.
gitsabari pushed a commit to gitsabari/sonic-utilities that referenced this pull request Jun 15, 2021
Prevent potential kernel oops if drivers are removed/devices are deinitialized while PMon daemons are still trying to access those devices.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants