Support to verify reboot for secure boot by xumia · Pull Request #979 · sonic-net/sonic-utilities

xumia · 2020-07-09T11:56:49Z

- What I did
Add a command "sonic_installer verify_reboot" to verify the image before reboot.
The command will be called when reboot/fast-reboot/warm-reboot.

- How I did it

- How to verify it

- 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)

qiluo-msft · 2020-07-09T20:09:00Z

@Staphylo Could you please help review ? #Closed

sonic_installer/bootloader/bootloader.py

jleveque · 2020-07-09T20:15:39Z

sonic_installer/main.py

+@cli.command('verify_reboot')
+def verify_reboot():


Along with Qi's comment above, this command should also be named form clearly, e.g., verify-next-image.

qiluo-msft · 2020-07-09T20:20:47Z

    subprocess.call(['rm','-rf', os.path.join(HOST_PATH, image_dir)])

Seem you function get_image_path is useful for many use cases. Could you refactor thoroughly?

Refers to: sonic_installer/bootloader/aboot.py:115 in 3a374f3. [](commit_id = 3a374f3, deletion_comment = False)

qiluo-msft · 2020-07-09T20:21:06Z

sonic_installer/bootloader/aboot.py

+        if not super(AbootBootloader, self).verify_reboot():
+            return False
+        image = self.get_next_image()
+        image_path = self.get_image_path(image) + '/sonic.swi'


sonic.swi [](start = 52, length = 9)

Magic string in aboot.py #Closed

and os.path.join

qiluo-msft · 2020-07-09T20:22:11Z

scripts/fast-reboot

-        debug "Next image ${NEXT_SONIC_IMAGE} doesn't exist ..."
-        exit ${EXIT_NEXT_IMAGE_NOT_EXISTS}
+    # Verify reboot by sonic_installer
+    local message=$(sonic_installer verify-reboot)


message [](start = 10, length = 7)

Did you only capture stdout but stderr? #Closed

Added the stderr message.

qiluo-msft

As comments

Staphylo · 2020-07-09T20:31:06Z

sonic_installer/bootloader/aboot.py

+        if not super(AbootBootloader, self).verify_reboot():
+            return False
+        image = self.get_next_image()
+        image_path = self.get_image_path(image) + '/sonic.swi'


and os.path.join

sonic_installer/bootloader/bootloader.py

lgtm-com · 2020-07-10T01:00:54Z

This pull request introduces 2 alerts when merging b10af37 into 1b992b3 - view on LGTM.com

new alerts:

2 for Syntax error

sonic_installer/bootloader/aboot.py

lgtm-com · 2020-07-10T01:36:57Z

This pull request introduces 1 alert when merging db0a9d2 into 1b992b3 - view on LGTM.com

new alerts:

1 for Unused import

scripts/fast-reboot

scripts/reboot

sonic_installer/main.py

qiluo-msft

Add new comment

lgtm-com · 2020-07-10T07:52:44Z

This pull request introduces 1 alert when merging 7914998 into 1b992b3 - view on LGTM.com

new alerts:

1 for Unused import

xumia · 2020-07-10T13:50:57Z

retest default please

yxieca · 2020-07-10T17:29:07Z

scripts/fast-reboot

-        exit ${EXIT_NEXT_IMAGE_NOT_EXISTS}
+    # Verify the next image by sonic_installer
+    local message=$(sonic_installer verify_next_image 2>&1)
+    if [ $? -ne 0 ]; then


This part of the reboot script is executed under -e option, meaning that the failure will trigger script to exit. Please take examples in the script to catch error code.

Thanks, the command can be simplified, fixed. #Closed

If you simplified this way, the sonic_installer verify-next-image will only output Failed or Done, which has no context in fast-reboot output.

As Ying suggested: Please take examples in the script to catch error code.

Your original debug command is still useful.

In reply to: 453135626 [](ancestors = 453135626)

Thanks Qi, fixed.

Thanks @yxieca actually #Closed

sonic_installer/main.py

#995) Root group name was changed from `cli` to `sonic_installer` in #983. However, `verify-next-image` subcommand was being added in an already-open PR #979 at the time. It did not get updated before merge. This aligns it with the new group name.

#### Why I did it To pick up fixes from submodule sonic-sairedis which include the following fixes: ``` commit 1027eef3a331e84560827c7584ee8009baf434d5 (HEAD -> 202012, origin/202012) Author: gechiang <62408185+gechiang@users.noreply.github.com> Date: Wed Dec 8 03:13:34 2021 -0800 [202012] Prevent other notification event storms to keep enqueue unchecked and drained all memory that leads to crashing the switch router (sonic-net#976) commit 94455e50d3444dcd60093b7a39c7f427337a94d2 Author: VenkatCisco <77468614+VenkatCisco@users.noreply.github.com> Date: Tue Jun 15 03:23:20 2021 -0700 Add cisco-8000 checks to syncd_init_common (sonic-net#839) commit 2df539483ed68519c3c9c6df958d3ed2f31dd629 Author: Kamil Cudnik <kcudnik@gmail.com> Date: Mon Dec 6 20:50:23 2021 +0100 [lgtm] Add gmock libs to lgtm (sonic-net#979) ```

…d (#995) Root group name was changed from `cli` to `sonic_installer` in sonic-net/sonic-utilities#983. However, `verify-next-image` subcommand was being added in an already-open PR sonic-net/sonic-utilities#979 at the time. It did not get updated before merge. This aligns it with the new group name.

xumia added 3 commits July 9, 2020 07:59

Support to verify reboot for secure boot

85a3ee1

Support to verify reboot for secure boot

bb9e287

Fix type issue

3a374f3

xumia requested a review from qiluo-msft July 9, 2020 11:57

qiluo-msft requested review from jleveque, lguohan, marian-pritsak, yxieca and zhenggen-xu July 9, 2020 20:05

qiluo-msft reviewed Jul 9, 2020

View reviewed changes

sonic_installer/bootloader/bootloader.py Outdated Show resolved Hide resolved

qiluo-msft reviewed Jul 9, 2020

View reviewed changes

sonic_installer/bootloader/bootloader.py Outdated Show resolved Hide resolved

jleveque suggested changes Jul 9, 2020

View reviewed changes

qiluo-msft reviewed Jul 9, 2020

View reviewed changes

qiluo-msft requested changes Jul 9, 2020

View reviewed changes

Staphylo reviewed Jul 9, 2020

View reviewed changes

jleveque suggested changes Jul 10, 2020

View reviewed changes

sonic_installer/bootloader/bootloader.py Outdated Show resolved Hide resolved

xumia added 2 commits July 10, 2020 00:42

Merge remote-tracking branch 'origin/master' into verify_reboot

73cfbaf

Change the verification command to verify-next-image

b10af37

qiluo-msft reviewed Jul 10, 2020

View reviewed changes

sonic_installer/bootloader/aboot.py Outdated Show resolved Hide resolved

Fix python function error

db0a9d2

Fix Host_PATH case issue

17118af

qiluo-msft previously approved these changes Jul 10, 2020

View reviewed changes

jleveque suggested changes Jul 10, 2020

View reviewed changes

scripts/fast-reboot Outdated Show resolved Hide resolved

scripts/reboot Outdated Show resolved Hide resolved

sonic_installer/main.py Outdated Show resolved Hide resolved

Fix some messages issue

7da32e3

xumia dismissed qiluo-msft’s stale review via 7da32e3 July 10, 2020 05:22

qiluo-msft requested changes Jul 10, 2020

View reviewed changes

Remove not neccessary message

7914998

not to import no use module

7e50016

qiluo-msft previously approved these changes Jul 10, 2020

View reviewed changes

yxieca reviewed Jul 10, 2020

View reviewed changes

Change command to use hyphens

40d0222

xumia dismissed qiluo-msft’s stale review via 40d0222 July 11, 2020 00:17

Simplify the command verify-next-image

9f86eb0

jleveque added Enhancement sonic-installer fast-reboot reboot labels Jul 11, 2020

Add fast-reboot exit code

fbce19a

qiluo-msft previously approved these changes Jul 12, 2020

View reviewed changes

jleveque suggested changes Jul 12, 2020

View reviewed changes

sonic_installer/main.py Outdated Show resolved Hide resolved

sonic_installer/main.py Outdated Show resolved Hide resolved

Improve the verification message

3b94802

xumia dismissed qiluo-msft’s stale review via 3b94802 July 13, 2020 02:32

jleveque approved these changes Jul 13, 2020

View reviewed changes

xumia merged commit b4b5fd3 into sonic-net:master Jul 13, 2020

jleveque mentioned this pull request Jul 17, 2020

[sonic-installer] Update group name for 'verify-next-image' subcommand #995

Merged

		@cli.command('verify_reboot')
		def verify_reboot():

Conversation

xumia commented Jul 9, 2020

Uh oh!

qiluo-msft commented Jul 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft commented Jul 9, 2020

Uh oh!

qiluo-msft Jul 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Jul 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lgtm-com bot commented Jul 10, 2020

Uh oh!

Uh oh!

lgtm-com bot commented Jul 10, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

qiluo-msft left a comment

Choose a reason for hiding this comment

Uh oh!

lgtm-com bot commented Jul 10, 2020

Uh oh!

xumia commented Jul 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xumia Jul 11, 2020 • edited by qiluo-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Jul 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

qiluo-msft commented Jul 9, 2020 •

edited

Loading

qiluo-msft Jul 9, 2020 •

edited

Loading

qiluo-msft Jul 9, 2020 •

edited

Loading

xumia Jul 11, 2020 •

edited by qiluo-msft

Loading

qiluo-msft Jul 12, 2020 •

edited

Loading