[vbox-clean-snapshots.py] Fix several bugs & improve code #645
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
Remove print just before exception as the same information is already printed by the exception.
The root snapshot can be protected, so that its children get deleted but not the root snapshot. Example (remove `EMPTY` children: `Snapshot 1` and `Snapshot 2`): ``` Fresh ├─ EMPTY │ ├─ Snapshot 1 │ └─ Snapshot 2 └─ Snapshot 3 vbox-clean-snapshots.py FLARE-VM --protected EMPTY --root_snapshot "EMPTY" ```
Use `snapshot` command (with the `list` option) instead of `showvminfo` as it returns only the snapshot information in `showinfo` and that is the only information we use.
The current code includes the current snapshot twice, which causes that a confusing output is rendered and that the snapshot is being removed twice (failing the second time). Example of `VBoxManage snapshot VM_NAME list --machinereadable` output: ``` SnapshotName="ROOT" SnapshotUUID="86b38fc9-9d68-4e4b-a033-4075002ab570" SnapshotName-1="Snapshot 1" SnapshotUUID-1="e383e702-fee3-4e0b-b1e0-f3b869dbcaea" CurrentSnapshotName="Snapshot 1" CurrentSnapshotUUID="e383e702-fee3-4e0b-b1e0-f3b869dbcaea" CurrentSnapshotNode="SnapshotName-1" SnapshotName-1-1="Snapshot 2" SnapshotUUID-1-1="8cc12787-99df-466e-8a51-80e373d3447a" SnapshotName-2="Snapshot 3" SnapshotUUID-2="f42533a8-7c14-4855-aa66-7169fe8187fe" ROOT ├─ Snapshot 1 │ └─ Snapshot 2 └─ Snapshot 3 ```
The current code excludes the root snapshot from deletion if a child is protected. But it is possible to delete a root snapshot with a single protected child. It is not possible to delete the root snapshot if it has more than one protected child. But this issue happens at every level, not only at the root snapshot. For example (fail to delete 'Snapshot 1'): ``` ROOT ├─ Snapshot 1 │ ├─ PROTECTED 1 │ └─ PROTECTED 2 └─ Snapshot 2 vbox-clean-snapshots.py FLARE-VM --protected "ROOT,PROTECTED" --root_snapshot ROOT ``` This inconsistent behavior is confusing and complicates the code unnecessarily. Instead, try to always delete non protected snapshots (including the root snapshot) and improve the rendered error output. In the error output, include the command as a string (instead of a list) as this allows to copy-paste the command to manually execute it. Simplify also the long stderr from VBoxManage, by using only the first `VBoxManage: error:` line to prevent using the long VBoxManage help message or noisy information like the details and context. Remove the use of `check` in `subprocess.run` (triggering an exception on error) as it can be handle without exceptions.
Use the snapshot ID instead of the name to avoid removing an snapshot
outside of the root snapshot.
Example (wrong `IMPORTANT` is deleted):
```
ROOT
├─ IMPORTANT
└─ Snapshot 1
└─ IMPORTANT
vbox-clean-snapshots.py FLARE-VM --root_snapshot
```
The current code breaks if no root snapshot is provided. The root snapshot argument should be optional and case sensitive. Make clear that this is the case and use all snapshots if no root snapshot is provided.
Do not catch unexpected exceptions so that the original exception reaches to the user.
williballenthin
approved these changes
Jan 23, 2025
|
+1 the commit messages are :chefs_kiss: I can do a quick once over tomorrow |
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.
The following PR fixes the following bugs:
a confusing output is rendered and that the snapshot is being removed twice (failing the second time).
In addition, the PR includes other improvements, such as better exception handling and using more specific VBoxManage commands. See the detailed commit messages for more details on every of the changes.