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

Proxmox Inventory: added new statuses for qemu #4723

Merged

Conversation

ilijamt
Copy link
Contributor

@ilijamt ilijamt commented May 24, 2022

SUMMARY

Added support for missing statuses for QEMU

  • prelaunch
  • paused
ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

proxmox.inventory

@ansibullbot
Copy link
Collaborator

@ansibullbot ansibullbot added WIP Work in progress feature This issue/PR relates to a feature request inventory inventory plugin plugins plugin (any type) labels May 24, 2022
@ilijamt ilijamt changed the title WIP: Proxmox Inventory: added new statuses for qemu Proxmox Inventory: added new statuses for qemu May 24, 2022
@ilijamt
Copy link
Contributor Author

ilijamt commented May 24, 2022

/rebuild_failed

@ansibullbot

This comment was marked as outdated.

@ansibullbot ansibullbot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR and removed WIP Work in progress labels May 24, 2022
@ansibullbot ansibullbot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label May 24, 2022
@felixfontein felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-5 labels May 24, 2022
# get more details about the status of the qemu VM if want_facts == True
if want_facts:
item_status = properties.get(self._fact('qmpstatus'), item_status)
self.inventory.add_child(self._group('all_%s' % (item_status)), name)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, isn't this a breaking change, since some hosts are now in different groups than before?

Copy link
Contributor Author

@ilijamt ilijamt May 24, 2022

Choose a reason for hiding this comment

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

Now they would be correct. Any VM that has a Paused state cannot be used and cannot be connected to. The reason for this was that Proxmox says that the VM is running, but in actuality is not. Without this, you have a wrong status for your machine.

The only other thing I can think of is introduce a new group prefixed with qemu like proxmox_qemu_paused and leave it in running. That way it will always be added to running even though it's in a paused/preflight state.

I'm fine with adding the two new groups and add the new status there. What do you think @felixfontein ?

Copy link
Contributor Author

@ilijamt ilijamt May 24, 2022

Choose a reason for hiding this comment

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

We are gonna end up with the following groups. And we can do intersection between proxmox_all_running and proxmox_all_qemu_running to figure out which ones are currently actually running.

  |--@proxmox_all_lxc:
  |--@proxmox_all_qemu:
  |--@proxmox_all_qemu_paused:
  |--@proxmox_all_qemu_prelaunch:
  |--@proxmox_all_qemu_running:
  |--@proxmox_all_qemu_stopped:
  |--@proxmox_all_running:
  |--@proxmox_all_stopped:

Copy link
Collaborator

Choose a reason for hiding this comment

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

Adding the new groups (and not modifying the old ones) definitely works; the chance that it breaks an existing setup should be negligible.

What do the other proxmox maintainers think? Do you prefer the old solution, or tend more to this one (which is definitely backwards compatible)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed it, so we still have the old behavior, and we just add them to the new group.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm inclined to still move ahead with it. @felixfontein absolutely right it's a breaking change on how it behaves, however the current behavior should more be considered a bug then that this is a new feature. As @ilijamt pointed out, in his current setup, which might impact other users as well, the module breaks play executions when a KVM is currently moving somewhere else, because it's reporting as running, while it technically not is.

Copy link
Contributor Author

@ilijamt ilijamt May 30, 2022

Choose a reason for hiding this comment

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

It's not the only case, you can also manually suspend a VM, and it will report as running when it's suspended.

The only issue is that we need want_facts set to true to be able to tell the difference

Copy link
Contributor Author

@ilijamt ilijamt May 31, 2022

Choose a reason for hiding this comment

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

So what is the consensus? cc @Thulium-Drake @felixfontein

  • Add a new flag to allow for the new statuses (empty when flag not set), and deprecate it on the new release.
  • Fix the issue now, and the statuses will be extended with the additional statuses prelaunch and paused

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm good with either, but I think option A is easier for @felixfontein to release right now :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

If the next release would be a major release I would be OK with option B, but since we just had one it's probably better to go with option A. This feels too close to a breaking change to me, even though I can understand very well why the current behavior sucks :)

@ansibullbot ansibullbot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI and removed needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI labels May 25, 2022
@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 and removed 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 May 26, 2022
plugins/inventory/proxmox.py Outdated Show resolved Hide resolved
plugins/inventory/proxmox.py Outdated Show resolved Hide resolved
plugins/inventory/proxmox.py Outdated Show resolved Hide resolved
changelogs/fragments/4724-proxmox-qemu-extend.yaml Outdated Show resolved Hide resolved
This was referenced Nov 20, 2022
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 inventory inventory plugin plugins plugin (any type) tests tests unit tests/unit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants