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

[DELLEMC][S6000] Platform 2.0 Reboot Reason and fixed issue in process-reboot-cause #3156

Merged
merged 4 commits into from
Jul 24, 2019

Conversation

sridhar-ravindran
Copy link
Contributor

- What I did

  1. Added Reboot Reason for S6000 in platform 2.0
  2. Fixed issue in process-reboot-cause
  3. Added package uninstall code in platform de-init code for z9100, s6100
    - How I did it
    -> Added support for S6000 Reboot Reason
    -> Added platform.py for all platforms
    -> Verified show reboot-cause command with the code changes. Added UT logs with show reboot-cause
    -> Modified process-reboot-cause service to start after pmon.service. In S6000, we have to wait for nvram to be loaded.
    -> If reboot-cause service starts before pmon.service, show reboot-cause is showing incorrect reason.
    -> Bug fix in process-reboot-cause file
    - import sonic_platform
    + import sonic_platform.platform

Changes to be committed:
(use "git reset HEAD ..." to unstage)

    modified:   files/image_config/process-reboot-cause/process-reboot-cause
    modified:   files/image_config/process-reboot-cause/process-reboot-cause.service
    modified:   platform/broadcom/sonic-platform-modules-dell/debian/platform-modules-s6000.init
    modified:   platform/broadcom/sonic-platform-modules-dell/debian/platform-modules-s6000.install
    modified:   platform/broadcom/sonic-platform-modules-dell/debian/rules
    modified:   platform/broadcom/sonic-platform-modules-dell/s6000/modules/dell_s6000_platform.c
    copied:     platform/broadcom/sonic-platform-modules-dell/debian/platform-modules-s6000.init -> platform/broadcom/sonic-platform-modules-dell/s6000/scripts/s6000_platform.sh
    new file:   platform/broadcom/sonic-platform-modules-dell/s6000/setup.py
    new file:   platform/broadcom/sonic-platform-modules-dell/s6000/sonic_platform/__init__.py
    new file:   platform/broadcom/sonic-platform-modules-dell/s6000/sonic_platform/chassis.py
    new file:   platform/broadcom/sonic-platform-modules-dell/s6000/sonic_platform/platform.py
    modified:   platform/broadcom/sonic-platform-modules-dell/s6000/systemd/platform-modules-s6000.service
    modified:   platform/broadcom/sonic-platform-modules-dell/s6100/scripts/s6100_platform.sh
    new file:   platform/broadcom/sonic-platform-modules-dell/s6100/sonic_platform/platform.py
    modified:   platform/broadcom/sonic-platform-modules-dell/z9100/scripts/z9100_platform.sh
    new file:   platform/broadcom/sonic-platform-modules-dell/z9100/sonic_platform/platform.py

- How to verify it
Verified show reboot-cause command with the code changes. Added UT logs with show reboot-cause

- Description for the changelog

Added Reboot reason for S6000 and fix issue in process-reboot-cause

- A picture of a cute animal (not mandatory but encouraged)

@sridhar-ravindran
Copy link
Contributor Author

Show_Reboot_Reason_UT_logs.txt

@@ -70,7 +70,7 @@ def main():
# if there is no sonic_platform package installed, we only provide
# software-related reboot causes.
try:
import sonic_platform
import sonic_platform.platform
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this necessary?

Copy link
Contributor Author

@sridhar-ravindran sridhar-ravindran Jul 16, 2019

Choose a reason for hiding this comment

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

Hi Joe,

sonic_platform is directory. platform is the file.

When we import directory alone We get the following error.
From File :- /usr/bin/process-reboot-cause
import sonic_platform
# Check if the previous reboot was caused by hardware
platform = sonic_platform.platform.Platform()
chassis = platform.get_chassis()
hardware_reboot_cause, optional_details = chassis.get_reboot_cause()

root@sonic:/home/admin# /usr/bin/process-reboot-cause
Traceback (most recent call last):
File "/usr/bin/process-reboot-cause", line 121, in
main()
File "/usr/bin/process-reboot-cause", line 76, in main

platform = sonic_platform.platform.Platform()
AttributeError: 'module' object has no attribute 'platform'

When we import the file "platform" the script is working
import sonic_platform.platform
# Check if the previous reboot was caused by hardware
platform = sonic_platform.platform.Platform()
chassis = platform.get_chassis()
hardware_reboot_cause, optional_details = chassis.get_reboot_cause()

root@sonic:/home/admin# /usr/bin/process-reboot-cause
root@sonic:/home/admin# <<<<<<<<<<<<< No Error

Please let me know if my understanding is fine.

Thanks
Sridhar.R

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm. I don't believe this should be necessary. You should be able to import the entire package by specifying the package (directory) name. Let me look into this further.

Copy link
Contributor

@jleveque jleveque Jul 16, 2019

Choose a reason for hiding this comment

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

It appears for this to work, you need to modify the init.py file of your sonic_platform package to either import the files and/or list the module names in __all__. See: https://docs.python.org/3/tutorial/modules.html#importing-from-a-package

I should probably also do this for sonic_platform_base, as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Joe,
After some literature study, i was able to successfully make it work. Updated init.py with
all = ["platform", "chassis"]
from sonic_platform import *

On adding these 2 lines, "import sonic_platform" works fine.

On adding new files like sfp.py , psu.py, device,py, we have to update the init.py file as well.

Thanks for your help and support Joe.

Thanks
Sridhar.R

@@ -1,6 +1,6 @@
[Unit]
Description=Reboot cause determination service
After=rc-local.service
After=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.

Why change this?

Copy link
Contributor Author

@sridhar-ravindran sridhar-ravindran Jul 16, 2019

Choose a reason for hiding this comment

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

Hi Joe,
The hardware drivers required to provide reboot reason are installed only inside platform-modules-XXXX service.

When process-reboot-cause service starts before the pmon service, loading of drivers are not complete and it leads to race condition where the show reboot-cause CLI return incorrect answers.

If process-reboot-cause loads after pmon.service, the platform drivers would have successfully loaded by then and show reboot-cause cli will return correct value

Thanks
Sridhar.R

Copy link
Contributor

@jleveque jleveque Jul 16, 2019

Choose a reason for hiding this comment

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

Hi Sridhar,

SONiC requires that hardware reboot reason and watchdog functionality need to be available in the host OS. The sonic_platform package should be installed in both the host OS and PMon container. Host OS services should not have any dependencies on the containers. We want reboot cause and watchdog to be as reliable as possible. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Joe,
Thanks for the input. I have removed the pmon dependency from process-restart-cause service.
Thanks
Sridhar.R

@jleveque
Copy link
Contributor

Retest vs please

@lguohan lguohan merged commit 95558ad into sonic-net:master Jul 24, 2019
@sridhar-ravindran sridhar-ravindran deleted the s6000_reboot_reason branch July 25, 2019 02:49
@sridhar-ravindran
Copy link
Contributor Author

Hi,

Could you please port this changes to november branch and 201904 branch

@jleveque
Copy link
Contributor

@sridhar-ravindran: will this PR cherry-pick cleanly into those branches?

@sridhar-ravindran
Copy link
Contributor Author

@sridhar-ravindran: will this PR cherry-pick cleanly into those branches?

Hi @jleveque
I will check and get back on this
Thanks
Sridhar.R

@yxieca
Copy link
Contributor

yxieca commented Jul 30, 2019

This PR cannot be cherry-picked into 201811 branch directly. Can you make an PR 201811?

sridhar-ravindran added a commit to sridhar-ravindran/sonic-buildimage that referenced this pull request Aug 5, 2019
…net#3156)

Added Reboot Reason for S6000 in platform 2.0
Fixed issue in process-reboot-cause
Added package uninstall code in platform de-init code for z9100, s6100
- How I did it
-> Added support for S6000 Reboot Reason
-> Added platform.py for all platforms
-> Verified show reboot-cause command with the code changes. Added UT logs with show reboot-cause
-> Modified process-reboot-cause service to start after pmon.service. In S6000, we have to wait for nvram to be loaded.
-> If reboot-cause service starts before pmon.service, show reboot-cause is showing incorrect reason.
-> Bug fix in process-reboot-cause file
- import sonic_platform
+ import sonic_platform.platform
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