-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
[feat] proxmox_snap: snapshot containers with configured mountpoints #5274
[feat] proxmox_snap: snapshot containers with configured mountpoints #5274
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution! I've added some first comments. Can you please add a changelog fragment? Thanks.
This comment was marked as outdated.
This comment was marked as outdated.
The last two commits address the remarks above, specifically:
|
changelogs/fragments/5274-proxmox-snap-container-with-mountpoints.yml
Outdated
Show resolved
Hide resolved
changelogs/fragments/5274-proxmox-snap-container-with-mountpoints.yml
Outdated
Show resolved
Hide resolved
Co-authored-by: Felix Fontein <[email protected]>
This comment was marked as outdated.
This comment was marked as outdated.
Great to see all tests are passing, looking forward to the release! The commit history obviously requires a rebase, will you take care of it when merging? Just wanted to add that I'm super happy to be contributing to Ansible! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides the following point, this looks good to me. I cannot really test this, but the code changes look good to me (i.e. they don't seem to break something that already worked) and since none of the maintainers reacted so far, I guess nobody objects. I'll merge this in a couple of days (assuming the below modification has been done).
…(#5274) * module_utils.proxmox: new `api_task_ok` helper + integrated with existing modules * proxmox_snap: add `unbind` param to snapshot containers with mountpoints * [fix] errors reported by 'test sanity pep8' at ansible-collections/community.general#5274 (comment) * module_utils.proxmox.api_task_ok: small improvement * proxmox_snap.unbind: version_added, formatting errors, changelog fragment * Apply suggestions from code review Co-authored-by: Felix Fontein <[email protected]> * proxmox_snap.unbind: update version_added tag Co-authored-by: Felix Fontein <[email protected]> Co-authored-by: Felix Fontein <[email protected]>
…(#5274) * module_utils.proxmox: new `api_task_ok` helper + integrated with existing modules * proxmox_snap: add `unbind` param to snapshot containers with mountpoints * [fix] errors reported by 'test sanity pep8' at ansible-collections/community.general#5274 (comment) * module_utils.proxmox.api_task_ok: small improvement * proxmox_snap.unbind: version_added, formatting errors, changelog fragment * Apply suggestions from code review Co-authored-by: Felix Fontein <[email protected]> * proxmox_snap.unbind: update version_added tag Co-authored-by: Felix Fontein <[email protected]> Co-authored-by: Felix Fontein <[email protected]>
SUMMARY
This PR adds a new parameter
unbind
to theproxmox_snap
module, which allows to take snapshots of containers with configured mount points, bypassing the Proxmox limit by temporarily disabling the configuration before the snapshot.There is also a small improvement in how the API tasks are checked.
A new
api_task_ok
helper is provided in themodule_utils.proxmox.ProxmoxAnsible
class, and used across the proxmox modules to standardize how pending tasks are checked.ISSUE TYPE
COMPONENT NAME
community.general.proxmox_snap
ADDITIONAL INFORMATION
proxmox_snap
The mountpoint-specific config API endpoint
mp[n]
is unfortunately (from an implementation pov, of course it's only logical) restricted toroot@pam
authenticating with password. Tokens will not work, at least I couldn't get them to work. More info here, PUT tab.This limit is not enforced when deleting the mounts config though, which means bad credentials would allow to remove the mount points BUT fail at reconfig, leaving the container in a misconfigured state. This behavior is properly commented in-code, documented, and the implementation checks if the user provided the correct authentication parameters, eventually failing before running any task.
Mainly to improve readability, two new helpers
start_instance
andshutdown_instance
have been introduced, as well as internal helpers to get and manipulate the mountpoints configuration (_container_mp_{get,disable,restore}
).Changes to the container's mountpoints config require shutting down the machine. The state of the container (
running
or not) is stored before applying any changes, and restored after the snapshot if necessary. This behavior is also properly documented.Finally the new logic tries to gracefully handle timeouts during the snapshot, by restoring the original config and state regardless of the successful execution of the API task.
api_task_ok
There could be some room for improvement, providing a
wait_api_task
helper to also abstract thewhile timeout:
blocks across the modules, but it would have been out of scope for this PR.Test Playbook