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

modules/proxmox_kvm: initial support for online migrations #6448

Merged

Conversation

tyxieblub
Copy link
Contributor

SUMMARY

I found myself wanting to use the proxmox_kvmmodule to migrate VM on different hosts, so I propose this change to do that by adding the migrate option, which is Falseby default (to prevent any breaking of existing playbooks).

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

proxmox_kvm

@ansibullbot
Copy link
Collaborator

@ansibullbot ansibullbot added feature This issue/PR relates to a feature request module module new_contributor Help guide this first time contributor plugins plugin (any type) labels Apr 28, 2023
@felixfontein felixfontein added the check-before-release PR will be looked at again shortly before release and merged if possible. label Apr 28, 2023
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! Without knowing proxmox_kvm the change looks good on a first glance.

@@ -1256,6 +1283,18 @@ def main():
except Exception as e:
module.fail_json(vmid=vmid, msg='Unable to revert settings on VM {0} with vmid {1}: Maybe is not a pending task... '.format(name, vmid) + str(e))

if migrate:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I find it a bit strange that this overrides state=absent, but then the other options above do it the same way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well as it is, migrate would fail on the proxmox.get_vm(vmid) if it does not exist in the cluster, but indeed it will never ensure states when migrate (or previous actions) is True. The way I see it, users need to define several tasks: one for managing the state and one or more for specific actions (clone, delete, revert and migrate).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nevermind, I just re-read this part and clone does not prevent state management. We could do the same for migrate, but having state and migrate as mutually exclusive options seems easier to implement.

Copy link
Collaborator

@russoz russoz left a comment

Choose a reason for hiding this comment

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

Also lacking familiarity with proxmox, but also agreeing it looks good.

plugins/modules/proxmox_kvm.py Outdated Show resolved Hide resolved
Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com>
Co-authored-by: Felix Fontein <felix@fontein.de>
Co-authored-by: Felix Fontein <felix@fontein.de>
@felixfontein felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label May 6, 2023
@felixfontein felixfontein merged commit fe224a6 into ansible-collections:main May 6, 2023
142 checks passed
@felixfontein
Copy link
Collaborator

@tyxieblub thanks for your contribution!
@russoz thanks for reviewing!

@tyxieblub tyxieblub deleted the proxmox-kvm-migrate branch May 9, 2023 06:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature This issue/PR relates to a feature request module module new_contributor Help guide this first time contributor plugins plugin (any type)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants