[Issue 71] add skip_convert_to_template option #283
Conversation
There was a problem hiding this comment.
Overall LGTM, thanks @mpywell
Just left a very small nit and a question about a mock signature changing, but overall the code looks good to me, once you've had a chance to address my comments we should be good to go!
Also apologies for leaving this one aside for so long, I'll do a full review of the open PRs on Proxmox today, with the aim of scheduling a release for later this week if possible.
Thanks again!
| type finalizer interface { | ||
| GetVmConfig(*proxmox.VmRef) (map[string]interface{}, error) | ||
| SetVmConfig(*proxmox.VmRef, map[string]interface{}) (interface{}, error) | ||
| StartVm(*proxmox.VmRef) (string, error) |
There was a problem hiding this comment.
Is there a reason to add StartVm to that contract? It seems that the function is not invoked in the step, and AFAICT this is not used elsewhere, so I wonder if that might be a leftover from a WIP state of the PR?
Feel free to correct me if I'm wrong though, I might've missed something
There was a problem hiding this comment.
It was left over from a WIP version of the PR where the returned artifact was a running VM. I noted in #72 (comment) a desire to have a running VM returned, so I've rerolled with those changes.
There was a problem hiding this comment.
Ah yeah understood, I missed the resume VM part.
I wonder though, the current workflow appears to work (can't test it as I don't have a proxmox machine on hand), but I would think we can keep the VM running throughout the process if we're not exporting to a template, don't we?
In this case we can change a bit the Cleanup method of the step_start_vm so it doesn't kill and delete the VM, unless it cannot?
Also to be sure, since the cleanup for the VM will run after step_finalize_config, will the VM still exist? I'd assume this will be stopped and removed, won't it?
There was a problem hiding this comment.
A stop and start of the VM is required if there are VM hardware changes for the VM's Cloud-Init storage and CD-ROM devices (step step_finalize_config).
These changes aren't picked up by the VM unless it's completely stopped then started again, they can't be hot-swapped or mounted on an ACPI reboot. This is a limitation on the Proxmox backend.
I've reworked the logic to only stop the VM artifact if there are changes to be done in step_finalize_config (if len(changes) > 0) that way if Cloud-Init isn't being configured or CD-ROM devices are being kept, the VM won't restart.
mpywell
force-pushed
the
feat/skip_convert_to_template
branch
from
b8c44e9 to
f2a0326
Compare
|
Hi @lbajolet-hashicorp, I've rerolled and addressed comments |
| type finalizer interface { | ||
| GetVmConfig(*proxmox.VmRef) (map[string]interface{}, error) | ||
| SetVmConfig(*proxmox.VmRef, map[string]interface{}) (interface{}, error) | ||
| StartVm(*proxmox.VmRef) (string, error) |
There was a problem hiding this comment.
Ah yeah understood, I missed the resume VM part.
I wonder though, the current workflow appears to work (can't test it as I don't have a proxmox machine on hand), but I would think we can keep the VM running throughout the process if we're not exporting to a template, don't we?
In this case we can change a bit the Cleanup method of the step_start_vm so it doesn't kill and delete the VM, unless it cannot?
Also to be sure, since the cleanup for the VM will run after step_finalize_config, will the VM still exist? I'd assume this will be stopped and removed, won't it?
- updated VM stop/start logic for hardware changes to a VM build - reverted getExistingTemplate signature changes
|
Hi @lbajolet-hashicorp pushed another reroll and comments addressed |
|
Hey @lbajolet-hashicorp just checking in to see if anything further is needed for this one? |
There was a problem hiding this comment.
Hey @mpywell,
Apologies I took so long to revisit this PR; I left one last comment on the stop/start VM code, I understand that you don't want to restart the VM unnecessarily, but it seems to me that trying to start a VM if it wasn't stopped might fail, but please feel free to correct me if that's not the case.
Besides this nit/comment, the PR looks good to me, we should be ready to merge soon, thanks!
|
|
||
| if len(changes) > 0 { | ||
| // Adding a Cloud-Init drive or removing CD-ROM devices won't take effect without a power off and on of the QEMU VM | ||
| if c.SkipConvertToTemplate { |
There was a problem hiding this comment.
Out of curiosity, why is the shutdown only done when there are changes? I would've thought this should be coupled with the call to StartVM in every case should you decide not to convert the VM to a template.
One scenario that I have in mind (no clue if that is realistic or not though), if we don't have changes the VM won't be shutdown, and we'll try to start it again afterwards, could the call fail if the VM is still running?
|
Can this be merged now or not ? |
|
I would also love to this merged. |
4 participants
Rewrite of #72 on current codebase.
Implements config option skip_convert_to_template which skips converting the build to a Template and leaves the build artifact as a VM.
Closes #71