[process-reboot-cause]Address the issue: Incorrect reboot cause returned when warm reboot follows a hardware caused reboot #3880

stephenxs · 2019-12-12T03:30:16Z

- What I did
Fix issue reboot cause test failed if warm-reboot follows any kind of hardware-caused reboot #3871

- How I did it

check whether /proc/cmdline indicates warm/fast reboot.
if yes the software reboot cause file will be treated as the reboot cause.
finish
check whether platform api returns a reboot cause.
if yes it is treated as the reboot cause.
finish.
check whether /hosts/reboot-cause contains a cause.
if yes it is treated as the cause otherwise return unknown.

Detailed information can be found in the issue description.

- How to verify it
reboot the DUT in the following sequence

reboot
warm-reboot
fast-reboot
power cycle reboot
warm-reboot
fast-reboot
issue a warm-reboot and then cut off the power supply after kernel reloaded and ahead of process-reboot-cause handling the reboot cause, the reboot cause should be power off.

reboot-cause-test.txt

- Description for the changelog

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

…ned when warm reboot follows a hardware caused reboot 1. check whether /proc/cmdline indicates warm/fast reboot. if yes the software reboot cause file will be treated as the reboot cause. finish 2. check whether platform api returns a reboot cause. if yes it is treated as the reboot cause. finish. 3. check whether /hosts/reboot-cause contains a cause. if yes it is treated as the cause otherwise return unknown.

stepanblyschak

As commented

stepanblyschak · 2019-12-12T10:45:06Z

files/image_config/process-reboot-cause/process-reboot-cause

@@ -22,6 +23,9 @@ REBOOT_CAUSE_DIR = "/host/reboot-cause/"
 REBOOT_CAUSE_FILE = REBOOT_CAUSE_DIR + "reboot-cause.txt"
 PREVIOUS_REBOOT_CAUSE_FILE = REBOOT_CAUSE_DIR + "previous-reboot-cause.txt"
 FIRST_BOOT_PLATFORM_FILE = "/tmp/notify_firstboot_to_platform"
+REBOOT_CAUSE_KEXEC = "/proc/cmdline"
+# The following SONIC_BOOT_TYPEs come from the warm/fast reboot script which is in sonic-utilities
+REBOOT_CAUSE_KEXEC_PATTERN = ".*SONIC_BOOT_TYPE=(fast-reboot|warm|fastfast)$"


According to new design for fast reboot - "SONIC_BOOT_TYPE=fast", but also need to check for "fast-reboot" in case reboot was done from 201803

Regular expression will match if SONIC_BOOT_TYPE is the last argument, lets have '.*' instead of '$'

@stephenxs IMHO:

REBOOT_CAUSE_KEXEC_PATTERN = ".*SONIC_BOOT_TYPE=(fast-reboot|warm|fastfast)$" -> REBOOT_CAUSE_KEXEC_PATTERN = "SONIC_BOOT_TYPE=(fast-reboot|warm|fastfast)" re.match(REBOOT_CAUSE_KEXEC_PATTERN, cause_file_kexec) -> re.search(REBOOT_CAUSE_KEXEC_PATTERN, cause_file_kexec)

stepanblyschak · 2019-12-12T10:48:40Z

files/image_config/process-reboot-cause/process-reboot-cause

@@ -47,6 +51,16 @@ def log_error(msg):


 # ============================= Functions =============================
+def is_warmfast_reboot_from_proc_cmdline():
+    if os.path.isfile(REBOOT_CAUSE_KEXEC):
+        cause_file = open(REBOOT_CAUSE_KEXEC, "r")


files was not closed, try to use with open(REBOOT_CASE_KEXEC, "r"):

fixed. use "with" for all open file cases.

nazariig · 2019-12-12T12:13:54Z

files/image_config/process-reboot-cause/process-reboot-cause

+                # The reboot was not caused by hardware. If there is a REBOOT_CAUSE_FILE, it will
+                # contain any software-related reboot info. We will use it as the previous cause.
+                if os.path.isfile(REBOOT_CAUSE_FILE):
+                    cause_file = open(REBOOT_CAUSE_FILE, "r")


@stephenxs please use with statement

1. use "with" statement 2. update fast/warm reboot BOOT_ARG

stepanblyschak · 2019-12-12T15:09:05Z

files/image_config/process-reboot-cause/process-reboot-cause

@@ -22,6 +23,9 @@ REBOOT_CAUSE_DIR = "/host/reboot-cause/"
 REBOOT_CAUSE_FILE = REBOOT_CAUSE_DIR + "reboot-cause.txt"
 PREVIOUS_REBOOT_CAUSE_FILE = REBOOT_CAUSE_DIR + "previous-reboot-cause.txt"
 FIRST_BOOT_PLATFORM_FILE = "/tmp/notify_firstboot_to_platform"
+REBOOT_CAUSE_KEXEC = "/proc/cmdline"
+# The following SONIC_BOOT_TYPEs come from the warm/fast reboot script which is in sonic-utilities
+REBOOT_CAUSE_KEXEC_PATTERN = ".*SONIC_BOOT_TYPE=(fast-reboot|warm|fastfast|fast).*"


Will not work for 201803->master upgrade. Please take a look at https://github.com/Azure/sonic-utilities/blob/201803/scripts/fast-reboot#L30

thank you for pointing out. will fix it.
Btw, we don't need to consider versions earlier than 201803, right?
@jleveque as Stepan isn't in working hours, can you help ensure that

@stephenxs: You are correct; we don't need to consider versions earlier than 201803.

yxieca · 2019-12-12T15:44:46Z

files/image_config/process-reboot-cause/process-reboot-cause

-    prev_cause_file.write(previous_reboot_cause)
-    prev_cause_file.close()
+    with open(PREVIOUS_REBOOT_CAUSE_FILE, "w") as prev_cause_file:
+        prev_cause_file.write(previous_reboot_cause)


This is not an issue for this PR or change request for this PR. Just want to start the discussion here.

This change is the result of a team brainstorm of various combinations of hard/soft reboot reason. We fell that we've covered all the cases for now. But in case we didn't. We could consider change the previous reboot cause file to 3 lines:

Reboot cause: (calculation result)
Detected hardware cause: (hardware cause)
Detected software cause: (software cause)

Not sure if any other part of our code has dependency on this file being single line?

personally I agree your point.
in addition, if reboot causes from different sources are conflict, it's better to have it in a detailed way.
meanwhile, the drawback is it can confuse user sometimes. for example, if the following info displayed:

calculated reboot cause: warm reboot hardware cause: power off software cause: warm reboot

probably it's better to only display in a 3-line way in case of we known for sure there is conflicts among causes from different sources?

You got a good point. However, if we know there is a conflict, then we would have figured it out and fixed it in code. We cannot predict what we don't know yet. :-)

yxieca

Thanks for working on this change!

files/image_config/process-reboot-cause/process-reboot-cause

lguohan · 2019-12-13T14:51:47Z

files/image_config/process-reboot-cause/process-reboot-cause

        else:
-            previous_reboot_cause = hardware_reboot_cause
+            # 2. Check if the previous reboot was caused by hardware
+            #    If yes, the hardware reboot cause will be treated as teh reboot cause


teh [](start = 70, length = 3)

-> the

lguohan · 2019-12-13T14:58:30Z

files/image_config/process-reboot-cause/process-reboot-cause

+                if os.path.isfile(FIRST_BOOT_PLATFORM_FILE):
+                    if (previous_reboot_cause == UNKNOWN_REBOOT_CAUSE):
+                        previous_reboot_cause = UNKNOWN_REBOOT_CAUSE
+                    os.remove(FIRST_BOOT_PLATFORM_FILE)


line 121 to line 132 are same as line 140 to line 151.

suggest to consolidate them as a single function.

previous_reboot_cause = find_software_reboot_cause()

lguohan · 2019-12-13T15:01:25Z

files/image_config/process-reboot-cause/process-reboot-cause

+            else:
+                # /proc/cmdline says it's a warm/fast reboot but /host/reboot-cause.txt doesn't exist.
+                # report an error.
+                log_error("/proc/cmdline indicates a fast/warm reboot but {} doesn't exist".format(REBOOT_CAUSE_DIR))


other than report error,

I think we should still assign the

proc_cmdline_reboot_cause = warm or fast

we should treat proc cmdline as truth.

yxieca · 2019-12-14T03:38:05Z

retest vs please

…ned when warm reboot follows a hardware caused reboot (#3880) * [process-reboot-cause]Address the issue: Incorrect reboot cause returned when warm reboot follows a hardware caused reboot 1. check whether /proc/cmdline indicates warm/fast reboot. if yes the software reboot cause file will be treated as the reboot cause. finish 2. check whether platform api returns a reboot cause. if yes it is treated as the reboot cause. finish. 3. check whether /hosts/reboot-cause contains a cause. if yes it is treated as the cause otherwise return unknown. * [process-reboot-cause]Fix review comments * [process-reboot-cause]address comments 1. use "with" statement 2. update fast/warm reboot BOOT_ARG * [process-reboot-cause]address comments * refactor the code flow * Remove escape * Remove extra ':'

Stephen Sun added 2 commits December 12, 2019 09:55

[process-reboot-cause]Fix review comments

757d82b

stephenxs mentioned this pull request Dec 12, 2019

[process-reboot-cause]Address the issue: Incorrect reboot cause returned when warm reboot follows a hardware caused reboot stephenxs/sonic-buildimage#21

Closed

keboliu approved these changes Dec 12, 2019

View reviewed changes

lguohan added Request for 201811 Branch Request for 201911 Branch labels Dec 12, 2019

stepanblyschak suggested changes Dec 12, 2019

View reviewed changes

nazariig reviewed Dec 12, 2019

View reviewed changes

[process-reboot-cause]address comments

70a7df1

1. use "with" statement 2. update fast/warm reboot BOOT_ARG

stepanblyschak suggested changes Dec 12, 2019

View reviewed changes

yxieca reviewed Dec 12, 2019

View reviewed changes

yxieca approved these changes Dec 12, 2019

View reviewed changes

jleveque suggested changes Dec 12, 2019

View reviewed changes

files/image_config/process-reboot-cause/process-reboot-cause Outdated Show resolved Hide resolved

[process-reboot-cause]address comments

0ea7089

jleveque approved these changes Dec 13, 2019

View reviewed changes

yxieca reviewed Dec 13, 2019

View reviewed changes

files/image_config/process-reboot-cause/process-reboot-cause Outdated Show resolved Hide resolved

lguohan reviewed Dec 13, 2019

View reviewed changes

yxieca added 3 commits December 13, 2019 09:11

refactor the code flow

8fb11e3

Remove escape

f37d08d

Remove extra ':'

00603a9

stepanblyschak approved these changes Dec 14, 2019

View reviewed changes

yxieca merged commit 80bb7fd into sonic-net:master Dec 14, 2019

yxieca added the Included in 201811 Branch label Dec 14, 2019

stephenxs deleted the reboot-cause-warm-follows-hw branch December 16, 2019 22:29

stephenxs mentioned this pull request Dec 18, 2019

reboot cause test failed if warm-reboot follows any kind of hardware-caused reboot #3871

Closed

yxieca mentioned this pull request Dec 18, 2019

Propose a further code clean up on top of PR #3880 #3927

Open

abdosi added the Included in 201911 Branch label Jan 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[process-reboot-cause]Address the issue: Incorrect reboot cause returned when warm reboot follows a hardware caused reboot #3880

[process-reboot-cause]Address the issue: Incorrect reboot cause returned when warm reboot follows a hardware caused reboot #3880

stephenxs commented Dec 12, 2019

stepanblyschak left a comment

stepanblyschak Dec 12, 2019

stepanblyschak Dec 12, 2019

nazariig Dec 12, 2019 •

edited

Loading

stepanblyschak Dec 12, 2019

stephenxs Dec 12, 2019

nazariig Dec 12, 2019 •

edited

Loading

stepanblyschak Dec 12, 2019

stephenxs Dec 12, 2019

jleveque Dec 12, 2019

stephenxs Dec 13, 2019

yxieca Dec 12, 2019 •

edited

Loading

stephenxs Dec 12, 2019

yxieca Dec 13, 2019

yxieca left a comment

lguohan Dec 13, 2019

yxieca Dec 13, 2019

lguohan Dec 13, 2019

yxieca Dec 13, 2019

lguohan Dec 13, 2019

yxieca Dec 13, 2019

yxieca commented Dec 14, 2019

[process-reboot-cause]Address the issue: Incorrect reboot cause returned when warm reboot follows a hardware caused reboot #3880

[process-reboot-cause]Address the issue: Incorrect reboot cause returned when warm reboot follows a hardware caused reboot #3880

Conversation

stephenxs commented Dec 12, 2019

stepanblyschak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nazariig Dec 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nazariig Dec 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yxieca Dec 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yxieca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yxieca commented Dec 14, 2019

nazariig Dec 12, 2019 •

edited

Loading

nazariig Dec 12, 2019 •

edited

Loading

yxieca Dec 12, 2019 •

edited

Loading