Skip to content

Conversation

@domendobnikar
Copy link
Collaborator

  • filter and delete based on special properties
  • filter and delete based on timestamps

domendobnikar and others added 3 commits May 3, 2023 13:08
@domendobnikar domendobnikar self-assigned this May 3, 2023
@domendobnikar
Copy link
Collaborator Author

domendobnikar commented May 3, 2023

@justinc1
vm_snapshot PR needs to be merged first. #217
Sanity issues are related to missing module.

@domendobnikar domendobnikar requested a review from justinc1 May 3, 2023 13:46
Copy link
Collaborator

@justinc1 justinc1 left a comment

Choose a reason for hiding this comment

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

Minor changes.

The timezone is problematic, but if it is always UTC, than we just document this.

@domendobnikar domendobnikar changed the title Adding vm_snapshot examples to collection: Adding vm_snapshot examples to collection May 15, 2023
@domendobnikar
Copy link
Collaborator Author

We talked about adding a practical example of using uuid with vm_snapshot. @justinc1
I added examples with absent (using uuid) into both of these example playbooks.
Would something like this be enough? Or should I add a separate playbook that shows both preset and absent?

Copy link
Collaborator

@justinc1 justinc1 left a comment

Choose a reason for hiding this comment

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

I'm worried get_snapshot() has small bug.

from ..module_utils.vm import VM


def get_snapshot(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any chance to get a snapshot with correct label, but from a different VM?

It seems to me: filter to domainUUID=vm_object.uuid part is lost. And integration tests should be extended too.

Copy link
Collaborator Author

@domendobnikar domendobnikar May 16, 2023

Choose a reason for hiding this comment

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

We pass VMobject as one of the parameters in this function.
It is then used with label for snapshot query if snapshot_uuid isn't passed as parameter to playbook.
The only added part was the if module.params["uuid"]: , everything else is the same. (code was taken from ensure_present function)
UUID is unique throughout the platform and use a different endpoint, it does not require any additional filtering by VM or label.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will extend integration tests to include the filtering by uuid part.
Good point 👍

@domendobnikar
Copy link
Collaborator Author

Copy link
Collaborator

@justinc1 justinc1 left a comment

Choose a reason for hiding this comment

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

Added integration test - OK, thank you.

I think I didn't manage to describe what was potential problem. Say we have:

  • vm_a, with snapshot:
    • snap_label_X, snap_uuid_a
  • vm_b, with snapshot:
    • snap_label_X, snap_uuid_b

Then I try to task:

vm_snaptshot:
  vm_name: vm_a
  snapshot_uuid: snap_uuid_b

I think this case is not covered by current integration tests. In this case, module might continue to work with snap_uuid_b, but it should fail, vm_a has no snap_uuid_b.

@domendobnikar
Copy link
Collaborator Author

Oh okey, I misunderstood the problem.
I will extend tests to also cover that potential bug.

@domendobnikar
Copy link
Collaborator Author

Copy link
Collaborator

@justinc1 justinc1 left a comment

Choose a reason for hiding this comment

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

ty

@justinc1 justinc1 merged commit 03eb1b7 into main May 18, 2023
@justinc1 justinc1 deleted the support-label branch May 18, 2023 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants