Skip to content

Allow predefined VM UUIDs#83

Open
dahoat-sprecher wants to merge 1 commit intoseapath:mainfrom
dahoat-sprecher:daho-allow-predefined-vm-uuids
Open

Allow predefined VM UUIDs#83
dahoat-sprecher wants to merge 1 commit intoseapath:mainfrom
dahoat-sprecher:daho-allow-predefined-vm-uuids

Conversation

@dahoat-sprecher
Copy link

Hello to All,

Although guest.xml.j2 read the variable vm.uuid, this value was later discarded by vm_manager. This behavior is fixed, permitting redeployment of applications binding their licenses to this UUID.

Best regards,
Daniel

Although guest.xml.j2 read the variable vm.uuid, this value was later discarded by vm_manager.
This behavior is fixed, permitting redeployment of applications binding their licenses to this UUID.

Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Daniel Hofer <daniel.hofer@sprecher-automation.com>
@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 6, 2026

dupremathieu
dupremathieu previously approved these changes Mar 9, 2026
@dupremathieu
Copy link
Member

dupremathieu commented Mar 9, 2026

We regenerate the UUID to avoid conflicts with existing VM.

If we want to be sure there is no conflict we have to list all VM UUID in all machines to check if the UUID is available. We could do it easily with SEAPATH VM but for VM deploy with libvirt directly we cannot detect it unless to have a libvirt connection to each machine which is for the moment optional.

I know that latest version of libvirt allow overriding the UUID which is accessible inside the VM without changing the VM UUID but as far as know it is not supported in our libvirt version.

@eroussy @insatomcat is it ok for you to only check for SEAPATH UUID? I've made a POC of this solution: 0001-Add-UUID-collision-detection-for-predefined-VM-UUIDs.patch

@dupremathieu dupremathieu self-requested a review March 9, 2026 15:03
@dupremathieu dupremathieu dismissed their stale review March 9, 2026 15:04

After testing this can lead to unclear error if there is already a VM with the UUID deployed in the cluster.

@eroussy
Copy link
Member

eroussy commented Mar 9, 2026

@eroussy @insatomcat is it ok for you to only which for SEAPATH UUID? I've made a POC of this solution: 0001-Add-UUID-collision-detection-for-predefined-VM-UUIDs.patch

I'm not sure to understand your question but direct Libvirt deployment should not be a problem because

  • We should use vm-manager even in standalone (still to test)
  • Libvirt would fail with a meaningful message

IMO, the real use case/problem is vm-manager deployement in cluster mode

@dupremathieu
Copy link
Member

@eroussy @insatomcat is it ok for you to only which for SEAPATH UUID? I've made a POC of this solution: 0001-Add-UUID-collision-detection-for-predefined-VM-UUIDs.patch

I'm not sure to understand your question but direct Libvirt deployment should not be a problem because

* We should use vm-manager even in standalone (still to test)

* Libvirt would fail with a meaningful message

IMO, the real use case/problem is vm-manager deployement in cluster mode

@eroussy The only use case that cause trouble is in cluster if you have deployed outside SEAPATH a libvirt VM with the case UUID that the VM we want to deploy.

If you have use vm-manager we could detect it (cf my patch).

My question is do we accept this limitation?

@insatomcat
Copy link
Member

Thanks for the discussion and the POC patch @dupremathieu.

I think we should keep this simple: if a user explicitly sets a vm.uuid in their configuration, they are opting into manual UUID management and therefore take full responsibility for ensuring that UUID is unique across the cluster. This is a deliberate, advanced use case — the whole point is to preserve a specific UUID for license-binding purposes, which implies the user already knows exactly what UUID they are working with.

Adding collision detection adds complexity and, as noted, cannot be made fully reliable anyway (e.g. VMs deployed directly via libvirt outside of SEAPATH). A partial check could even give users a false sense of safety.

I'd suggest we document this clearly — something like: "When a predefined UUID is specified, the user is responsible for guaranteeing its uniqueness in the target environment. No collision detection is performed." — and proceed with merging the current PR as-is.

@eroussy
Copy link
Member

eroussy commented Mar 10, 2026

I'd suggest we document this clearly — something like: "When a predefined UUID is specified, the user is responsible for guaranteeing its uniqueness in the target environment. No collision detection is performed." — and proceed with merging the current PR as-is.

I can agree with that. But we should then also document that in the deploy_vms_cluster ansible role.
@dupremathieu is it ok for you ?

@eroussy
Copy link
Member

eroussy commented Mar 10, 2026

@dahoat-sprecher You have one test failing in CI about uuid replacement https://github.com/seapath/vm_manager/blob/main/tests/test_vm_manager_libvirt.py#L22

Can you replace this test with two tests:

  • One verifying that uuid is correct when the user specifically provides one
  • Another to verify that no uuid is present when the user doesn't define any

@dahoat-sprecher
Copy link
Author

@eroussy The current test verifies that a UUID is set which is different to the old one. Should my second test verify the same behavior if no UUID is provided by the user (UUID by vm_manager) or do we actually want no UUID so libvirt generates a new one?

@eroussy
Copy link
Member

eroussy commented Mar 10, 2026

@eroussy The current test verifies that a UUID is set which is different to the old one. Should my second test verify the same behavior if no UUID is provided by the user (UUID by vm_manager) or do we actually want no UUID so libvirt generates a new one?

For me, the default would be for libvirt to generate a uuid when no one is provided by the user (that necessitate one test)
And then we need another test for your new feature about providing the uuid manually

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.

4 participants