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

Add backup module for proxmox #9197

Open
wants to merge 36 commits into
base: main
Choose a base branch
from

Conversation

IamLunchbox
Copy link
Contributor

@IamLunchbox IamLunchbox commented Nov 26, 2024

SUMMARY

This module adds a task to create backup tasks in a proxmox virtual environment cluster.

ISSUE TYPE
  • New Module/Plugin Pull Request
COMPONENT NAME

proxmox_backup

ADDITIONAL INFORMATION

Currently, not all api options are covered, since they are either difficult to implement or only available for the root-user of the instance used.

For example, the api offers an exclude-mode next to the include, pool and all-mode. But this would would require other dependent settings to be set at as well. It could be added though, but I thought this will probably not be used, when include-mode and node-restrictions are available.

As to root-limited options (e.g. tmpdir api-parameter): I opted reproducing the options available in the GUI of a proxmox cluster, the options which I not implemented are not visible there either. And since I suspect most organizations would rather use a dedicated user or api-key to perform backups instead of the master-root user, I opted for simplicity and left these options out entirely. Users, which rely soley on the GUI to schedule and perform backups won't even know, that these options exist.

Compared to other proxmox modules, this module adds several checks regarding available permissions, but I value backups as mission critical and wanted to ensure a reasonable user experience by failing gracefully, if basic conditions are not met.
But on the other hand, this adds some complexity to the module, so if this is overburdening for an ansible module, i can delete them as well.

Lastly, I have seen PR #8360, but I have two issues with it:

  1. It does not implement what is states to do. It uses the cluster/backup-api endpoint, which is only used to schedule backups, not start them. Since it does not do what i need (actually starting backups, not just planning them), I opted for an implementation using the correct api endpoint (nodes/{node}/vzdump).
  2. The aforementioned endpoint needs high privileges to begin with: (Check: ["perm","/",["Sys.Modify"]]). Any user who would want to perform backups using feat/add proxmox backup module  #8360 would essentially need root-privileges, while proxmox has a quite detailed permission system, which allows user to have a dedicated VM.Backup permission. This fine grained control is quite interesting for CI/CD setups, where an api-key with low level permissions can be used, to create a backup before for example a patch is deployed.

If #8360 is merged at some point, I suggest it to be renamed to proxmox_backup_schedule, since that is what it does. I know that the proxmox api documentation is kind of misleading regarding the relationship of those two endpoints. If need be, I can post images of the GUI usage and the corresponding api-calls to the proxmox backend.

@ansibullbot
Copy link
Collaborator

@IamLunchbox this PR contains the following merge commits:

Please rebase your branch to remove these commits.

click here for bot help

@ansibullbot

This comment was marked as outdated.

@ansibullbot ansibullbot added merge_commit This PR contains at least one merge commit. Please resolve! module module needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR new_plugin New plugin labels Nov 26, 2024
@ansibullbot
Copy link
Collaborator

@ansibullbot

This comment was marked as outdated.

@ansibullbot ansibullbot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Nov 26, 2024
@felixfontein felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-10 Automatically create a backport for the stable-10 branch labels Nov 26, 2024
Copy link
Collaborator

@felixfontein felixfontein left a 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 on the documentation.

plugins/modules/proxmox_backup.py Outdated Show resolved Hide resolved
plugins/modules/proxmox_backup.py Outdated Show resolved Hide resolved
plugins/modules/proxmox_backup.py Outdated Show resolved Hide resolved
plugins/modules/proxmox_backup.py Outdated Show resolved Hide resolved
plugins/modules/proxmox_backup.py Outdated Show resolved Hide resolved
plugins/modules/proxmox_backup.py Outdated Show resolved Hide resolved
plugins/modules/proxmox_backup.py Outdated Show resolved Hide resolved
plugins/modules/proxmox_backup.py Outdated Show resolved Hide resolved
plugins/modules/proxmox_backup.py Outdated Show resolved Hide resolved
plugins/modules/proxmox_backup.py Outdated Show resolved Hide resolved
@IamLunchbox
Copy link
Contributor Author

Thanks for your contribution! I've added some first comments on the documentation.

Thank you for the review! I implemented your suggestions and added an additional option, which was recently released.

Does action_group_added need to be 10.0.1 as well?

@felixfontein
Copy link
Collaborator

version_added for attributes is only needed if they are added in later versions. So you can remove it.

Also note that you did not add the module to the action group. Only the modules listed for the proxmox action group in meta/runtime.yml are actually part of it.

@IamLunchbox
Copy link
Contributor Author

Ok, thanks. I will implement those two fixes today.

@ansibullbot ansibullbot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Nov 30, 2024
@ansibullbot ansibullbot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Nov 30, 2024
@felixfontein
Copy link
Collaborator

If nobody objects, I'll merge this PR for the next release.

@ansibullbot ansibullbot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Nov 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-10 Automatically create a backport for the stable-10 branch check-before-release PR will be looked at again shortly before release and merged if possible. module module needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI new_plugin New plugin plugins plugin (any type) tests tests unit tests/unit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants