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.

Sign up for GitHub

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

Jump to bottom

proxmox_kvm: enable 'force' restart of vm (as documented) #6914

Merged
merged 3 commits into from
Jul 23, 2023
Merged

proxmox_kvm: enable 'force' restart of vm (as documented) #6914

merged 3 commits into from
Jul 23, 2023

Conversation

J4RF
Copy link
Contributor

@J4RF J4RF commented Jul 12, 2023

SUMMARY

The documentation states the follow for the the force parameter:

Can be used with states stopped, restarted, and absent.

Despite that, force is not actually used when state=restarted, this PR aims to fix that by executing a VM "reset" when force is True, and a "restart" otherwise.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

proxmox_kvm

ADDITIONAL INFORMATION

N/A

@ansibullbot
Copy link
Collaborator

cc @Ajpantuso @Thulium-Drake @UnderGreen @helldorado @joshainglis @karmab @tleguern
click here for bot help

@ansibullbot ansibullbot added bug This issue/PR relates to a bug module module plugins plugin (any type) small_patch Hopefully easy to review labels Jul 12, 2023
@ansibullbot ansibullbot removed the small_patch Hopefully easy to review label Jul 12, 2023
@felixfontein felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-6 labels Jul 12, 2023
felixfontein
felixfontein reviewed Jul 12, 2023
View reviewed changes
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'm not 100% sure whether this is a good idea. Right now, the documentation does not say what should happen if force=true is used with state=restarted. Changing the current behavior from reboot to reset could break quite a lot of user.

Maybe it's better to fix this by updating the docs to no longer claim that force is used for restarts as well, and by adding (in a separate PR) a new state resetted that does a reset instead of a reboot?

@felixfontein
Copy link
Collaborator

recheck

@J4RF
Copy link
Contributor Author

J4RF commented Jul 20, 2023

I would argue that this implementation follows the same design pattern as stop_vm() (with regard to forced/non-forced option) whereas your suggestion introduces a new pattern. While I do understand not wanting to introduce a potentially breaking change, I also think anyone blindly using the "force" option deserves to suffer :D. I really do feel this is the way it was originally intended, per the docs.

That said, you have authority, and I don't :P. If you'd like it done as you suggested, then I will do it that way.

@felixfontein
Copy link
Collaborator

This is something for the module maintainers to decide... I don't know proxmox and the proxmox_kvm module good enough for that.

@ansibullbot ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label Jul 21, 2023
@UnderGreen
Copy link
Contributor

I agree with @J4RF in this case. If the documentation for the whole time states the force param is applied to restarted state then we are doing a bug fix here and making the docs match the code. Whoever used force before and will run into some problems, shouldn't be using force in the first place.

@felixfontein
Copy link
Collaborator

Ok, sounds good to me then.

@J4RF can you update the changelog fragment to use double backticks instead of single quotes? (See my suggestion above.) Thanks.

@J4RF @felixfontein
Update changelogs/fragments/6914-proxmox_kvm-enable-force-restart.yml
3128b97 
Co-authored-by: Felix Fontein <felix@fontein.de>
@ansibullbot ansibullbot removed the stale_ci CI is older than 7 days, rerun before merging label Jul 23, 2023
@felixfontein felixfontein added backport-7 and removed check-before-release PR will be looked at again shortly before release and merged if possible. labels Jul 23, 2023
@felixfontein felixfontein merged commit 17b4219 into ansible-collections:main Jul 23, 2023
@patchback
Copy link

patchback bot commented Jul 23, 2023 •
edited
Loading

Backport to stable-7: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-7/17b4219b8b360dac8e4dc58880a4368e4adc94d3/pr-6914

Backported as #6997

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request Jul 23, 2023
@J4RF @patchback
proxmox_kvm: enable 'force' restart of vm (as documented) (#6914)
2980956 
* enable 'force' restart of vm

* added changelog fragment

* Update changelogs/fragments/6914-proxmox_kvm-enable-force-restart.yml

Co-authored-by: Felix Fontein <felix@fontein.de>

---------

Co-authored-by: Felix Fontein <felix@fontein.de>
(cherry picked from commit 17b4219)
@patchback patchback bot mentioned this pull request Jul 23, 2023
Merged
@felixfontein
Copy link
Collaborator

@J4RF thanks for fixing this!
@UnderGreen thanks for reviewing!

felixfontein pushed a commit that referenced this pull request Jul 23, 2023
@patchback @J4RF
[PR #6914/17b4219b backport][stable-7] proxmox_kvm: enable 'force' re…
7d68af5 
…start of vm (as documented) (#6997)

proxmox_kvm: enable 'force' restart of vm (as documented) (#6914)

* enable 'force' restart of vm

* added changelog fragment

* Update changelogs/fragments/6914-proxmox_kvm-enable-force-restart.yml

Co-authored-by: Felix Fontein <felix@fontein.de>

---------

Co-authored-by: Felix Fontein <felix@fontein.de>
(cherry picked from commit 17b4219)

Co-authored-by: Jeff Turner <jeff@torusoft.com>
felixfontein added a commit to felixfontein/community.general that referenced this pull request Jul 31, 2023
@felixfontein
Revert "[PR ansible-collections#6914/17b4219b backport][stable-7] pro…
e19a280 
…xmox_kvm: enable 'force' restart of vm (as documented) (ansible-collections#6997)"

This reverts commit 7d68af5.
felixfontein added a commit that referenced this pull request Jul 31, 2023
@felixfontein
[stable-7] Revert new features to be able to do 7.2.1 release (#7042)
7cf834f 
* Revert "[PR #7020/b46d5d81 backport][stable-7] redfish_utils: Add support for "nextLink" property tag pagination (#7026)"

This reverts commit 1dad953.

* Revert "[PR #6914/17b4219b backport][stable-7] proxmox_kvm: enable 'force' restart of vm (as documented) (#6997)"

This reverts commit 7d68af5.

* Revert "[PR #6976/d7c1a814 backport][stable-7] [proxmox_vm_info] Re-use cluster resources API to use module without requiring node param (#6993)"

This reverts commit fb3768a.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@felixfontein felixfontein felixfontein left review comments

Assignees
No one assigned
Labels
bug This issue/PR relates to a bug module module plugins plugin (any type)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@J4RF @ansibullbot @felixfontein @UnderGreen