Fix Aboot breakage sonic-installer due to package migration #1625
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This pull request introduces 2 alerts when merging 29e4061c675480aba9571e973579a525b6820636 into 00bd0ce - view on LGTM.com new alerts:
|
> Failure cause The `get_rootfs_path` contextmanager was repurposed to implement `get_file_in_image` and later used as a function by leveraging some python complexity to bypass the restrictions coming with the contextmanager which were added purposefuly. It was then called multiple times to compute paths though a simple path join using `new_image_dir` could have been performed. The `get_rootfs_path` implementation for Aboot behaved differently when a rootfs was extracted or kept within the SWI image. It also behaved differently on secureboot systems. The updated method was then called on non-existing files for which the method was never meant to process. > Context around the failure Over time, the installation and boot process has slightly diverged from the ONIE one. There are 3 scenarios to consider. 1) Regular boot similar to ONIE This one should just work the same as the filesystem layout is unchanged. 2) docker_inram where dockerfs.tar.gz is extracted in tmpfs at boot In this scenario the boot is similar to the regular one beside that dockerfs.tar.gz is preserved intact on the flash and not extracted. By design this does not fit the sonic-package-manager requierements and the migration should be skipped which is what I did in this review. In the coming month this boot mode will look closer to 3) below. 3) Secureboot on Arista devices In this scenario the SWI image is kept intact and nothing extracted from it. By ensuring the integrity of the SWI we can guarantee that no code/data has been tampered with. This mode also relies on `docker_inram` at the moment. It could be enhanced when sonic-package-manager can guarantee the integrity of code and data that is both installed and migrated. > Solution provided The method `get_file_in_image` was reverted to its original meaning `get_rootfs_path` as there is no point in keeping the new one. It doesn't have the necessary logic to handle more than the rootfs and the logic can be easily be achieved by the following line. `os.path.join(bootloader.get_image_path(binary_image_name), 'something')` A new Bootloader method called `support_package_migration` is introduced to allow the bootloader to skip the package migration based on the image (docker_inram) or its own configuration (secureboot). By default all bootloaders enable the package migration. That change leads to 1) above running package migration while 2) and 3) skip it.
Staphylo
force-pushed
the
master-aboot-spm-fix
branch
from
29e4061
to
763eaab
Compare
|
@stepanblyschak: Please review. |
stepanblyschak
approved these changes
May 20, 2021
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
yxieca
approved these changes
May 28, 2021
gitsabari
pushed a commit
to gitsabari/sonic-utilities
that referenced
this pull request
Jun 15, 2021
…-net#1625) > Failure cause The `get_rootfs_path` contextmanager was repurposed to implement `get_file_in_image` and later used as a function by leveraging some python complexity to bypass the restrictions coming with the contextmanager which were added purposefuly. It was then called multiple times to compute paths though a simple path join using `new_image_dir` could have been performed. The `get_rootfs_path` implementation for Aboot behaved differently when a rootfs was extracted or kept within the SWI image. It also behaved differently on secureboot systems. The updated method was then called on non-existing files for which the method was never meant to process. > Context around the failure Over time, the installation and boot process has slightly diverged from the ONIE one. There are 3 scenarios to consider. 1) Regular boot similar to ONIE This one should just work the same as the filesystem layout is unchanged. 2) docker_inram where dockerfs.tar.gz is extracted in tmpfs at boot In this scenario the boot is similar to the regular one beside that dockerfs.tar.gz is preserved intact on the flash and not extracted. By design this does not fit the sonic-package-manager requierements and the migration should be skipped which is what I did in this review. In the coming month this boot mode will look closer to 3) below. 3) Secureboot on Arista devices In this scenario the SWI image is kept intact and nothing extracted from it. By ensuring the integrity of the SWI we can guarantee that no code/data has been tampered with. This mode also relies on `docker_inram` at the moment. It could be enhanced when sonic-package-manager can guarantee the integrity of code and data that is both installed and migrated. > Solution provided The method `get_file_in_image` was reverted to its original meaning `get_rootfs_path` as there is no point in keeping the new one. It doesn't have the necessary logic to handle more than the rootfs and the logic can be easily be achieved by the following line. `os.path.join(bootloader.get_image_path(binary_image_name), 'something')` A new Bootloader method called `support_package_migration` is introduced to allow the bootloader to skip the package migration based on the image (docker_inram) or its own configuration (secureboot). By default all bootloaders enable the package migration. That change leads to 1) above running package migration while 2) and 3) skip it.
raphaelt-nvidia
pushed a commit
to raphaelt-nvidia/sonic-utilities
that referenced
this pull request
Aug 10, 2021
…-net#1625) > Failure cause The `get_rootfs_path` contextmanager was repurposed to implement `get_file_in_image` and later used as a function by leveraging some python complexity to bypass the restrictions coming with the contextmanager which were added purposefuly. It was then called multiple times to compute paths though a simple path join using `new_image_dir` could have been performed. The `get_rootfs_path` implementation for Aboot behaved differently when a rootfs was extracted or kept within the SWI image. It also behaved differently on secureboot systems. The updated method was then called on non-existing files for which the method was never meant to process. > Context around the failure Over time, the installation and boot process has slightly diverged from the ONIE one. There are 3 scenarios to consider. 1) Regular boot similar to ONIE This one should just work the same as the filesystem layout is unchanged. 2) docker_inram where dockerfs.tar.gz is extracted in tmpfs at boot In this scenario the boot is similar to the regular one beside that dockerfs.tar.gz is preserved intact on the flash and not extracted. By design this does not fit the sonic-package-manager requierements and the migration should be skipped which is what I did in this review. In the coming month this boot mode will look closer to 3) below. 3) Secureboot on Arista devices In this scenario the SWI image is kept intact and nothing extracted from it. By ensuring the integrity of the SWI we can guarantee that no code/data has been tampered with. This mode also relies on `docker_inram` at the moment. It could be enhanced when sonic-package-manager can guarantee the integrity of code and data that is both installed and migrated. > Solution provided The method `get_file_in_image` was reverted to its original meaning `get_rootfs_path` as there is no point in keeping the new one. It doesn't have the necessary logic to handle more than the rootfs and the logic can be easily be achieved by the following line. `os.path.join(bootloader.get_image_path(binary_image_name), 'something')` A new Bootloader method called `support_package_migration` is introduced to allow the bootloader to skip the package migration based on the image (docker_inram) or its own configuration (secureboot). By default all bootloaders enable the package migration. That change leads to 1) above running package migration while 2) and 3) skip it.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Failure cause
Fixes sonic-net/sonic-buildimage#7566
The
get_rootfs_pathcontextmanager was recently repurposed to implementget_file_in_image. It then got used as a function by leveraging somepython complexity to bypass the restrictions coming with the contextmanager construct.
This was a conscious decision to prevent exactly what happened.
The method was then called multiple times to compute relative paths to the image directory.
Simple path joins using
new_image_dircould have been performed instead.The
get_rootfs_pathimplementation for Aboot bootloaders behaves differentlywhen a rootfs is extracted or kept within the SWI image. It also behaves differently on secureboot systems.
The updated method was used by providing parameters it was never meant to process.
Context around the failure
Over time, the installation and boot process on Aboot has slightly diverged from
the ONIE one. There are 3 scenarios to consider.
Regular boot similar to ONIE
This one should just work the same as the filesystem layout is unchanged.
docker_inram where dockerfs.tar.gz is extracted in tmpfs at boot
In this scenario the boot is similar to the regular one beside that
dockerfs.tar.gzis preserved intact on the flash and not extracted.By design this does not fit the sonic-package-manager requierements and
the migration should be skipped which is what I did in this review.
In the coming months this boot mode will look closer to
3)below.Secureboot on Arista devices
In this scenario the SWI image is kept intact and nothing extracted
from it. By ensuring the integrity of the SWI we can guarantee that no
code/data has been tampered with. This mode also relies on
docker_inramat the moment.It could be enhanced when sonic-package-manager can guarantee the
integrity of code and data that is both installed and migrated.
Solution provided
The method
get_file_in_imagewas reverted to its original meaningget_rootfs_pathas there is no point in keeping the new one.It doesn't have the necessary logic to handle more than the rootfs and
the logic for which it was repurposed can easily be achieved by the following line.
os.path.join(bootloader.get_image_path(binary_image_name), 'something')If this is a desirable helper it should become a method of its own.
A new
Bootloadermethod calledsupport_package_migrationisintroduced to allow the bootloader to skip the package migration based
on the image (e.g docker_inram) or its own configuration (e.g secureboot).
By default all bootloaders enable the package migration.
That change leads to
1)above running package migration while2)and3)skip it.
New command output (if the output of a command-line utility has changed)
The temporary error message now becomes a warning message for platforms that do not
support package migration.