[sonic_installer] Change sonic_installer check ASIC mismatch by platforms list #1836

wen587 · 2021-09-22T05:46:09Z

What I did

Handle wrong image type installation by checking platform. Before the verification was done by comparing image's ASIC and running platform's ASIC. But the running's ASIC will change if wrong image was installed.
In this patch, the installation process will also check asic but based on platform info which will stay the same even with wrong image installed.
Add the same support for Aboot image verification.

How I did it

Add a devices list file for the same ASIC while build the image and Check by devices list instead of machine.conf

How to verify it

Install a bin file which differs the running platform's ASIC. For example;
install sonic-broadcom.bin to mellanox ASIC platform
install sonic-vs.bin to broadcom ASIC platform
install sonic-aboot-barefoot.swi to broadcom ASIC platform

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)

wen587 · 2021-09-23T09:49:30Z

To make this PR functional, it requires sonic-net/sonic-buildimage#8814 to work.
But it can also be merged alone. Because it works backward compatible.

qiluo-msft · 2021-09-27T11:23:21Z

sonic_installer/main.py

@@ -533,14 +533,14 @@ def install(url, force, skip_platform_check=False, skip_migration=False, skip_pa
            raise click.Abort()
    else:
        # Verify not installing non-secure image in a secure running image
-        if not bootloader.verify_secureboot_image(image_path) and not force:
+        if not force and not bootloader.verify_secureboot_image(image_path):
            echo_and_log("Image file '{}' is of a different type than running image.\n".format(url) +
                "If you are sure you want to install this image, use -f|--force|--skip-secure-check.\n" +
                "Aborting...", LOG_ERR)
            raise click.Abort()

        # Verify that the binary image is of the same platform type as running platform


platform

Platform has multiple meanings in SONiC context. Suggest platform -> asic
running platform -> running platform's asic type #Closed

will change

qiluo-msft · 2021-09-27T11:28:42Z

sonic_installer/bootloader/grub.py

-            click.echo("Caught an exception: " + str(e))
+            with open(os.devnull, 'w') as fnull:
+                p = subprocess.Popen(["sed", "-e", "1,/^exit_marker$/d", image_path], stdout=subprocess.PIPE, preexec_fn=default_sigpipe)
+                output = subprocess.check_output(["tar", "xf", "-", PLATFORMS_ASIC, -O], stdin=p.stdout, stderr=fnull, preexec_fn=default_sigpipe, text=True)


-O

Did you test this line? #Closed

Yes. It works as expected.

My bad. Will change later.

qiluo-msft · 2021-09-27T11:30:18Z

sonic_installer/bootloader/grub.py

-            click.echo("Caught an exception: " + str(e))
+            with open(os.devnull, 'w') as fnull:
+                p = subprocess.Popen(["sed", "-e", "1,/^exit_marker$/d", image_path], stdout=subprocess.PIPE, preexec_fn=default_sigpipe)
+                output = subprocess.check_output(["tar", "xf", "-", PLATFORMS_ASIC, -O], stdin=p.stdout, stderr=fnull, preexec_fn=default_sigpipe, text=True)


stderr=fnull

Is it the same as the default value None? #Closed

No. I tried locally. None still prompt error msg.

Then the with scope is too small, you need to cover p2.wait(), to be safer, cover until function exit

qiluo-msft · 2021-09-27T11:34:19Z

sonic_installer/bootloader/grub.py

+            with open(os.devnull, 'w') as fnull:
+                p = subprocess.Popen(["sed", "-e", "1,/^exit_marker$/d", image_path], stdout=subprocess.PIPE, preexec_fn=default_sigpipe)
+                output = subprocess.check_output(["tar", "xf", "-", PLATFORMS_ASIC, -O], stdin=p.stdout, stderr=fnull, preexec_fn=default_sigpipe, text=True)
+                return platform in output


output

output could be a long list. You can reduce the memory consuption by continuing pipe and grep, or use line-by-line file manipulation. #Closed

I was thinking using pipe and grep. But seems Popen dont have better ways to handle the case of unexisted file.
Because 'platform_asic nonexistence' and 'platform unmatch' will both produce the empty string if using pipe and grep.
I am thinking of Popen(['sed'...) first then check_call(['tar'...) first to see if there is any CalledProcessError to confirm platforms_asic exist. Then do the above sed & tar then pipe and grep.
But it will cost more time as you can see. Do you prefer the pipe and grep over check_output?

qiluo-msft · 2021-09-27T11:39:05Z

sonic_installer/bootloader/aboot.py

+        try:
+            output = subprocess.check_output(['/usr/bin/unzip', '-qop', image_path, '.platforms_asic'], text=True)
+            return platform in output
+        except subprocess.CalledProcessError:


CalledProcessError

This section is good consideration for downgrade use case.

However, CalledProcessError is larger than the the case ".platforms_asic is not existed". Could you check it in straighforward way? #Closed

I am refering here. Seems the image_path is guaranteed to exist. So it only raise exeption if platform_asic is not existed. Maybe it is enough to just for checking existence?
image_version

qiluo-msft · 2021-09-27T11:39:40Z

sonic_installer/bootloader/aboot.py

+            output = subprocess.check_output(['/usr/bin/unzip', '-qop', image_path, '.platforms_asic'], text=True)
+            return platform in output
+        except subprocess.CalledProcessError:
+            pass


pass

Your comment said "simply return True" #Closed

will change

qiluo-msft · 2021-09-27T11:40:05Z

sonic_installer/bootloader/grub.py

+                output = subprocess.check_output(["tar", "xf", "-", PLATFORMS_ASIC, -O], stdin=p.stdout, stderr=fnull, preexec_fn=default_sigpipe, text=True)
+                return platform in output
+        except subprocess.CalledProcessError:
+            pass


pass

simply return True #Closed

will change

qiluo-msft · 2021-09-27T11:41:31Z

Regarding the PR description, you can move "Check by devices list instead of machine.conf" into the section "How I did it".

Please give more explanation on the question "What I did". This PR actually changes behavior.

In reply to: 927791388

qiluo-msft · 2021-09-30T07:16:31Z

sonic_installer/common.py

+
+# Needed to prevent "broken pipe" error messages when piping
+# output of multiple commands using subprocess.Popen()
+def default_sigpipe():


default_sigpipe

what is the impact of ""broken pipe" error messages when piping" ? #Closed

wen587 · 2021-10-11T08:16:11Z

/azp run

azure-pipelines · 2021-10-11T08:16:22Z

Azure Pipelines successfully started running 1 pipeline(s).

wen587 · 2021-10-15T05:44:11Z

/azp run

azure-pipelines · 2021-10-15T05:44:22Z

Azure Pipelines successfully started running 1 pipeline(s).

wen587 added 2 commits September 22, 2021 05:40

Use devicelist to check instead of machine.conf

c3cd2e7

1. Change platforms list into one file; 2. Add Aboot support

653fdb2

wen587 changed the title ~~Use devicelist to check instead of machine.conf~~ [sonic_installer]Change sonic_installer check ASIC mismatch by platforms list Sep 23, 2021

Use pass instead of return True directly

830ab38

Change naming of platforms list

bc483c0

wen587 requested a review from qiluo-msft September 26, 2021 02:27

wen587 marked this pull request as ready for review September 26, 2021 02:27

qiluo-msft reviewed Sep 27, 2021

View reviewed changes

qiluo-msft changed the title ~~[sonic_installer]Change sonic_installer check ASIC mismatch by platforms list~~ [sonic_installer] Change sonic_installer check ASIC mismatch by platforms list Sep 29, 2021

Use Popen instead of check_output to save memory

5fbb34c

wen587 requested a review from qiluo-msft September 30, 2021 02:38

qiluo-msft reviewed Sep 30, 2021

View reviewed changes

Cover wait() to be safer

908c742

qiluo-msft previously approved these changes Oct 9, 2021

View reviewed changes

platfroms dir change

383f826

wen587 dismissed qiluo-msft’s stale review via 383f826 October 14, 2021 06:02

qiluo-msft approved these changes Oct 19, 2021

View reviewed changes

wen587 merged commit 8ea834b into sonic-net:master Oct 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[sonic_installer] Change sonic_installer check ASIC mismatch by platforms list #1836

[sonic_installer] Change sonic_installer check ASIC mismatch by platforms list #1836

wen587 commented Sep 22, 2021 •

edited

Loading

wen587 commented Sep 23, 2021

qiluo-msft Sep 27, 2021 •

edited

Loading

wen587 Sep 28, 2021

qiluo-msft Sep 27, 2021 •

edited

Loading

wen587 Sep 28, 2021

wen587 Sep 28, 2021

qiluo-msft Sep 27, 2021 •

edited

Loading

wen587 Sep 28, 2021

qiluo-msft Sep 30, 2021

qiluo-msft Sep 27, 2021 •

edited

Loading

wen587 Sep 28, 2021

qiluo-msft Sep 27, 2021 •

edited

Loading

wen587 Sep 28, 2021

qiluo-msft Sep 27, 2021 •

edited

Loading

wen587 Sep 28, 2021

qiluo-msft Sep 27, 2021 •

edited

Loading

wen587 Sep 28, 2021

qiluo-msft commented Sep 27, 2021 •

edited

Loading

qiluo-msft Sep 30, 2021 •

edited

Loading

wen587 commented Oct 11, 2021

azure-pipelines bot commented Oct 11, 2021

wen587 commented Oct 15, 2021

azure-pipelines bot commented Oct 15, 2021

[sonic_installer] Change sonic_installer check ASIC mismatch by platforms list #1836

[sonic_installer] Change sonic_installer check ASIC mismatch by platforms list #1836

Conversation

wen587 commented Sep 22, 2021 • edited Loading

What I did

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)

wen587 commented Sep 23, 2021

qiluo-msft Sep 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Sep 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Sep 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Sep 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Sep 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Sep 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Sep 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft commented Sep 27, 2021 • edited Loading

qiluo-msft Sep 30, 2021 • edited Loading

Choose a reason for hiding this comment

wen587 commented Oct 11, 2021

azure-pipelines bot commented Oct 11, 2021

wen587 commented Oct 15, 2021

azure-pipelines bot commented Oct 15, 2021

wen587 commented Sep 22, 2021 •

edited

Loading

qiluo-msft Sep 27, 2021 •

edited

Loading

qiluo-msft Sep 27, 2021 •

edited

Loading

qiluo-msft Sep 27, 2021 •

edited

Loading

qiluo-msft Sep 27, 2021 •

edited

Loading

qiluo-msft Sep 27, 2021 •

edited

Loading

qiluo-msft Sep 27, 2021 •

edited

Loading

qiluo-msft Sep 27, 2021 •

edited

Loading

qiluo-msft commented Sep 27, 2021 •

edited

Loading

qiluo-msft Sep 30, 2021 •

edited

Loading