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

Only show disk space warning when files are actually being uploaded #552

Merged
merged 2 commits into from
Apr 25, 2024

Conversation

kajes
Copy link
Contributor

@kajes kajes commented Apr 25, 2024

This PR contains changes for handling the preparation of upgrades.

Currently, there's is a disk space warning that is triggered if sdpctl detects that an appliance has low disk space. This is to make sure that the appliances has enough disk space to handle the uploads.

The disk space warning, however, does not take into consideration that uploads may be skipped if the file already exists on the appliance, making it obsolete if no files are actually being uploaded.

This PR fixes this by moving the disk space warning check until after we know that files need to be uploaded.

Copy link
Contributor

@thomascellerier thomascellerier left a comment

Choose a reason for hiding this comment

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

Note that we still might need disk space on Controllers for several reasons:

  • We usually take a backup of the primary controller, which requires some temporary storage.
  • We take a copy of the database to be able to rollback a failed upgrade. Note that usually this only overwrites an old copy of the database from a previous upgrade to I don't think its much of an issue in practice.

I still think this change makes sense in general though as we have too many false positives before it.

@kajes kajes merged commit a2f351f into main Apr 25, 2024
4 checks passed
@kajes kajes deleted the more-accurate-disk-space-warning branch April 25, 2024 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants