Remove existing Cloud-Init drive before creation #139
Conversation
There was a problem hiding this comment.
Hi @sebastian-de,
Just left a few comments on the code and tests, I'm puzzled by the test more than the code.
Once you addressed the comments I left, I'll do another pass and if we're good, we'll merge that.
Thanks!
| return multistep.ActionHalt | ||
| } | ||
| // The cloud-init drive is usually attached to ide0, but check the other controllers as well | ||
| ideControllers := []string{"ide0", "ide1", "ide2", "ide3"} |
There was a problem hiding this comment.
I'm not super knowledgeable when it comes to Proxmox, but is this list finite? Is there any chance the cloud-init drive might not be one of those?
Judging how you're getting the controllers, would it be relevant to iterate on the vmParams to gather all the existing controllers (ide or other possible values) and remove the attribute in those that define it?
There was a problem hiding this comment.
Unfortunately it's not possible to link to the config entries directly, so I'm quoting https://pve.proxmox.com/pve-docs/api-viewer/index.html#/nodes/{node}/qemu/{vmid}/config here:
ide[n] string [...] Use volume as IDE hard disk or CD-ROM (n is 0 to 3).
Proxmox always defaults to IDE for Cloud-Init drives, but yes, in theory you can attach it to other controllers as well. I added them to the code.
| if !c.expectCallSetConfig { | ||
| t.Error("Did not expect SetVmConfig to be called") | ||
| } | ||
| for key, val := range c.expectedVMConfig { |
There was a problem hiding this comment.
I'm confused by this test, what is it working with? Looking at the code the payload should essentially contain only a delete key/val, therefore the other values are nil by default, since no value exists for each key in the expectedVMConfig, unless I'm missing something?
There was a problem hiding this comment.
Oh dear, sorry for the headaches...
I rewrote the tests, don't know what I was thinking, when I wrote them initially.
There was a problem hiding this comment.
Hi @sebastian-de,
Thanks for the reroll, the test makes more sense to me with the rewrite, I'm approving this PR.
I left a couple more nits to address, once you've had a chance to address them, we can merge.
Thanks again for the contribution!
If the existing drive is not deleted it won't get updated in stepFinalizeTemplateConfig and Proxmox gives an error: "TASK ERROR: zfs error: cannot create 'rpool/data/vm-106-cloudinit': dataset already exists" To produce a clean template, also remove Cloud-Init configuration parameters set during build.
If a template is created from a VM with a Cloud-Init drive, the existing drive is neither deleted nor updated in stepFinalizeTemplateConfig and Proxmox throws
TASK ERROR: zfs error: cannot create 'rpool/data/vm-106-cloudinit': dataset already existsand the build fails.This PR adds
stepRemoveCloudInitDrive, which will remove the Cloud-Init drive before finalizing the template.To produce a clean template, also remove Cloud-Init configuration parameters set during build.
If
cloud_initis set to true, the resulting drive will be empty.Tested on PVE 7.3
Closes #112