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

[DELL] S6100 Support PowerCycle in Last Reboot Reason #3403

Merged
merged 5 commits into from
Sep 17, 2019

Conversation

sridhar-ravindran
Copy link
Contributor

- What I did
Added support to fix power-cycle last reboot cause in S6100
Known Caveat :- If we upgrade old_image (old_image without this fix) to the new_image(with this fix)
If we do a power-cycle instead of fast/warm/cold reset, show reboot-cause will return "Unknown" instead of "power-cycle"
- How I did it
Changes Done
modified: files/image_config/process-reboot-cause/process-reboot-cause
modified: files/image_config/platform/rc.local
modified: platform/broadcom/sonic-platform-modules-dell/common/dell_pmc.c
modified: platform/broadcom/sonic-platform-modules-dell/s6000/sonic_platform/chassis.py
modified: platform/broadcom/sonic-platform-modules-dell/s6100/scripts/s6100_platform.sh
modified: platform/broadcom/sonic-platform-modules-dell/s6100/sonic_platform/chassis.py

rc.local will inform the platform about first boot. Platform Init Script will use the info and update the
logical to return non-hardware in case of first time onie boot/image upgrade.

process-reboot-cause will use the first_boot notification and use it to return Valid last-reset-reason
Currently it will show unkwown which is not correct.
- How to verify it
Unit Test Results attached
a) power-cycle the box and issue show reboot-cause
b) Onie install the box and check return value of get_reboot_cause api
c) Upgrade the image and check return value of get_reboot_Cause api

- Description for the changelog

UnitTesting_Final_With_PowerCycle_and_FirstBoot (1).txt

[DELL] S6100 Support PowerCycle in Last Reboot Reason
- A picture of a cute animal (not mandatory but encouraged)

@sujinmkang
Copy link
Collaborator

sujinmkang commented Sep 3, 2019

I tested this with the build on Jenkins using S6000 (which should be supported the reboot-cause at this point, I think) but the reboot reason never got properly updated after once hardware reboot happened with PDU control.
It's stuck in the following response whenever I issue the "show reboot-cause" even after I issued "sudo reboot".
+++++++++++++++++++++++++++
admin@str-s6000-acs-7:~$ show reboot-cause
Unable to determine cause of previous reboot
+++++++++++++++++++++++++++
Please check this again.
This is only regression on s6000. It seems working o S6100.

@sridhar-ravindran
Copy link
Contributor Author

S6000_UT_Logs.txt
Hi sujinmkang,
Fixed it and uploaded the UT results.
S6000 does not support powerCycle. Hence it will be Unknown for that.
Subsequent reloads will show proper results.

@lguohan
Copy link
Collaborator

lguohan commented Sep 5, 2019

what about S6100?

@sujinmkang
Copy link
Collaborator

retest vs please

1 similar comment
@sujinmkang
Copy link
Collaborator

retest vs please

@sujinmkang
Copy link
Collaborator

sujinmkang commented Sep 10, 2019

@sridhar-ravindran, I just checked the latest Jenkins build on this PR. It seems S6100 has a problem. "show reboot-cause" always returns "Unable to determine cause of previous reboot".
What could cause this message? And can you please check this PR again on S6000 and S6100 for reboot/warm-reboot/fast-reboot and power loss and share the results?

@sridhar-ravindran
Copy link
Contributor Author

@sridhar-ravindran, I just checked the latest Jenkins build on this PR. It seems S6100 has a problem. "show reboot-cause" always returns "Unable to determine cause of previous reboot".
What could cause this message? And can you please check this PR again on S6000 and S6100 for reboot/warm-reboot/fast-reboot and power loss and share the results?

Hi @sujinmkang
sonic-net/sonic-swss#1006
As a side effect of above check-in, Syncd process in crashing in S6100.
We are in process of raising a pull request for that.

For internal unit testing, i reverted that change and tested.
Things were working fine.

Thanks
Sridhar.R

# Update the reboot cause as required
if os.path.isfile(FIRST_BOOT_PLATFORM_FILE):
if (previous_reboot_cause == UNKNOWN_REBOOT_CAUSE):
previous_reboot_cause = UNKNOWN_REBOOT_CAUSE
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do we need to set previous_reboot_cause = UNKNOWN_REBOOT_CAUSE in this case?

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 @sujinmkang

Ideally the code can be

   # Update the reboot cause as required
   if os.path.isfile(FIRST_BOOT_PLATFORM_FILE):
          if (previous_reboot_cause == UNKNOWN_REBOOT_CAUSE):
                   previous_reboot_cause = "First Boot"

When the device is booted with sonic first time, the first boot file will be present.

Now when we install Sonic the first time (lets say FTOS to sonic conversion),
the show reboot-cause will return "Unable to determine the previous reboot cause".
Instead it will be good if we return clear data like "First Time Boot" or "FirstDeployment"
However this has to be decided by MSFT. Hence i set it as previous value itself.
MSFT, if needed can change to relevant return string.

Thanks
Sridhar.R

Copy link
Collaborator

Choose a reason for hiding this comment

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

@sridhar-ravindran I got it. Let's keep it as is for now. We can revisit if needed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please resolve the merge conflicts ?

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 @sujinmkang
Synced to latest and resolved conflicts

@sujinmkang
Copy link
Collaborator

retest vs please

@sujinmkang sujinmkang merged commit 3c0b56a into sonic-net:master Sep 17, 2019
@sridhar-ravindran sridhar-ravindran deleted the LRR_S6100_PowerCycle branch September 18, 2019 04:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants