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

Remove partially installer image when image install failed. #3712

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

liuh-80
Copy link
Contributor

@liuh-80 liuh-80 commented Jan 13, 2025

Remove partially installer image when image install failed.

What I did

When install image failed, partially installed image not removed, which may cause disk full issue on small disk device.

How I did it

Handle install image SystemExit exception and uninstall image.

How to verify it

Manually verify.
Pass all test case

Work item tracking
  • Microsoft ADO: 30611724

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)

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@liuh-80 liuh-80 marked this pull request as ready for review January 15, 2025 13:09
@liuh-80 liuh-80 requested a review from qiluo-msft January 15, 2025 13:09
echo_and_log('Delete partial installed image: {}'.format(binary_image_version))
new_image_dir = bootloader.get_image_path(binary_image_version)
run_command(["rm", "-rf", new_image_dir])
raise e
Copy link
Contributor

@qiluo-msft qiluo-msft Jan 16, 2025

Choose a reason for hiding this comment

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

raise e

just raise is better, it keep original stack info. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

echo_and_log('Image install failed with exception: {}'.format(e))
echo_and_log('Delete partial installed image: {}'.format(binary_image_version))
new_image_dir = bootloader.get_image_path(binary_image_version)
run_command(["rm", "-rf", new_image_dir])
Copy link
Contributor

Choose a reason for hiding this comment

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

run_command

possible to use python function? rm-rf is always dangerous.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we use bootloader.remove_image ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't because remove_image need check bootloader info, but install failed at here doesn't update bootloader.

The bootloader will be update by line 612:
update_sonic_environment(bootloader, binary_image_version)

A partial installed image failed here only extract files to /host/image-version folder, so we only need to delete that folder.

Copy link
Contributor

Choose a reason for hiding this comment

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

how about shutil.rmtree ?

bootloader.install_image(image_path)
try:
bootloader.install_image(image_path)
except BaseException as e:
Copy link
Contributor

@qiluo-msft qiluo-msft Jan 16, 2025

Choose a reason for hiding this comment

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

BaseException

Can we use more specific exception type relating disk issue? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, install image failed with throw SystemExit exception.

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

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