🚧 feat: add support for reusing a vm

[Hi-Angel]

This PR resolves the problem where users could not install a system from ISO to a pre-created Virtual Machine.

This mostly ready and works, except the two TODO points below. I wanted to submit this at its current 90% ready state to get comments on whether the approach is okay, design, etc. I am barely familiar both with Packer, its plugin system, ESXi, and it's also my first ever experience with Go. So would be good to get feedback at this point to make sure I won't have to rewrite everything from scratch 😊

Closes #334

TODO:

• Handle the case of iso_paths having more than one element. I presume the correct way of solving that for reuseVM case would be pre-building a list of all CD-roms, then asserting that amount of ISOs ≤ amount of CD-roms, then mounting ISO to each CD-rom in the list.
• Error out if HCL/JSON config has reuse_vm = true and also hardware configuration (because we don't want to modify hw on a pre-created VM).

[hashicorp-cla]

All committers have signed the CLA.

[tenthirtyam]

Marking as Draft whilst WIP.

[Hi-Angel]

Oh, PRs with wip prefix aren't drafts in Github… In Gitlab they are

[Hi-Angel]

Oh, thank you! For the record, it's still WIP, so the CI not passing is expected.

[tenthirtyam]

I pushed 489a517 to apply the formatting to the edited .go files and generated the docs to ensure the CI passes.

packer-plugin-vsphere on  pr/344 
➜ go fmt ./... 
builder/vsphere/common/step_add_cdrom.go
builder/vsphere/iso/builder.go
builder/vsphere/iso/config.hcl2spec.go
builder/vsphere/iso/step_create.go

packer-plugin-vsphere on  pr/344
➜ make generate
2023/12/19 16:09:16 Copying "docs" to ".docs/"
2023/12/19 16:09:16 Replacing @include '...' calls in .docs/
Compiling MDX docs in '.docs' to Markdown in '.web-docs'...

Please pull the latest.

[Hi-Angel]

Okay. Do you want it be a separate commit? It fixes whatever was added by the "WIP" commit prior to it, so it looks like the changes should belong to it

[tenthirtyam]

Okay. Do you want it be a separate commit? It fixes whatever was added by the "WIP" commit prior to it, so it looks like the changes should belong to it

It's fine to be a seperate commit. Commits are squashed when merged.

[Hi-Angel]

Commits are squashed when merged.

Oh, wow… Alright… I was just working on separating the changes to distinct commits to make sure the git-log is clean and every functional change is in a separate commit. But I guess if this project squashes all changes, there's no point in doing that…

[Hi-Angel]

Okay, I have a question that probably requires having some intricate knowledge of go build system.

When I execute a make test, I get an output like:

[…]
ok      github.com/hashicorp/packer-plugin-vsphere/builder/vsphere/clone        1.071s
ok      github.com/hashicorp/packer-plugin-vsphere/builder/vsphere/common       1.787s
FAIL    github.com/hashicorp/packer-plugin-vsphere/builder/vsphere/driver [build failed]
ok      github.com/hashicorp/packer-plugin-vsphere/builder/vsphere/iso  1.049s
ok      github.com/hashicorp/packer-plugin-vsphere/builder/vsphere/supervisor   3.144s
[…]

…note the build failed. There's no details, prints, nothing, it just says "it failed". And when I execute either a go build or a go build github.com/hashicorp/packer-plugin-vsphere/builder/vsphere/driver that finishes just fine! Adding a -v doesn't help.

Something odd is happening with the build system. I have found this unanswered but highly upvoted question on Stackoverflow so it's not clear how to debug that. Still, figured I might ask that right away in case someone has ideas.

[Hi-Angel]

Aha, I found: it turns out, if you execute a go test github.com/hashicorp/packer-plugin-vsphere/builder/vsphere/driver manually, only then it will print the compilation error!

UPD: posted an answer to the SO as well! So hopefully future contributors will know what to look for

[Hi-Angel]

@tenthirtyam now that the PR has your commit, after being squashed who is gonna show up as the author of the commit in Github?

[tenthirtyam]

Credit is given to both.

[Hi-Angel]

Alright, so, the functional works, I have a question whether adding the check that hardware isn't defined in HCL/JSON file is required for that to get merged.

I spent a bunch of time trying to figure out where the necessary checks that I need to suppress happen, but still to no avail. Packer-plugins are super hard to debug because α) attaching with gdb means packer is gonna throw on timeout (and apparently it's not possible to disable, I asked such question a month ago, and now the packer google group is even banned) β) there's some weirdness, such as that pressing next in debugger works as continue γ) prints in the plugin don't show up in the output.

With that said, I added tests for the new functional, so everything works per se.

[Hi-Angel]

upd: fixed linting complaints (hopefully)

[nywilken]

Alright, so, the functional works, I have a question whether adding the check that hardware isn't defined in HCL/JSON file is required for that to get merged.

Can you point me to the code you are talking about here. I'm not sure I understand the questions.

I asked such question a month ago, and now the packer google group is even banned

Thanks for bubbling this up. I looked into this yesterday and working internally to get this back up and running.

As for debugging, its hard. I don't have much to add there but I can help figure out what to do next. Attaching a debugger won't really work to my knowledge given how Packer communicates with the plugin. But this gets easier if we have a unit test and use devel to debug. Maybe...

[Hi-Angel]

Alright, so, the functional works, I have a question whether adding the check that hardware isn't defined in HCL/JSON file is required for that to get merged.

Can you point me to the code you are talking about here. I'm not sure I understand the questions.

Thank you for reply!

Basically, I am interested whether more changes are required for this to get this merged or if it's okay as is.

I'm already using this code and it works for me.

[tenthirtyam]

@Hi-Angel - can you rebase your branch from main to resolve the conflicts?

[Hi-Angel]

Ugh, sorry, yeah, it will take some time… I did rebase locally, the conflicts were trivial, however it does not compile anymore because in one of the files an vm.AddCdrom was added. I will need to figure out what this does and how to make it work with the functional the MR adds…

[nywilken]

I believe this one is stuck on rebasing, which will get resolved by #355. Lets get 355 updated and merged. I can follow up quickly here to get this merged as well.

[nywilken]

Please rebase onto the latest main with your recent changes.

[Hi-Angel]

Thanks, sure! I'll probably get to it the next week

[nywilken]

Thanks, sure! I'll probably get to it the next week

Sounds good!

[Hi-Angel]

Okay, it turned out to be somewhat harder, but done now.

I have a question though: I'd like reuse_vm to conflict with reattach_cdroms option (because the option implies we reuse a VM unmodified), how do I do that? So e.g. I could in CreateConfig::Prepare step error out if reattach_cdroms is set, but the problem is that I don't seem to have access to StepReattachCDRom at that point.

Other than that, it's ready for review.

---

I'd like to point out a paragraph of code that may raise eyebrows which is this. In this paragraph depending on the ReuseVM value I either execute "find a CDrom and mount an image" or a "create a CDrom and mount an image". That seems like overcomplication: why not just "create or find a CDrom", and then always "mount an image"? Well, as a matter of fact this is exactly what I did in the older version of code. But turned out it doesn't work and Idk if it's a bug in govmomi or the plugin API. What happens is that when you create a cdrom, commit it to vCenter, then you pass it to our MountCdrom(), the latter triggers a vague vCenter error Invalid configuration for device '0'.

I tried reducing that to a minimal testcase, which turned out to be pretty hard because examples of creating a CDrom with govmomi do not exist. At some point of trying of write such code by copying lines from the plugin I stumbled upon that some API in the plugin for getting devices uses local configuration instead of calling to govmomi. At that point I stopped because I figured I'll have to spend too much time on this. So… anyway, just know that creating a CDrom and mounting an image in separate steps doesn't work due to a bug in either govmomi or in the plugin.

It may be for the better, because "create a cdrom" is an operation that requires sending data over the network, so breaking it to two operations "create a cdrom" and "mount an image" would increase amount of data sent. As it is done currently the amount is smaller.

[nywilken]

I have a question though: I'd like reuse_vm to conflict with reattach_cdroms option (because the option implies we reuse a VM unmodified), how do I do that? So e.g. I could in CreateConfig::Prepare step error out if reattach_cdroms is set, but the problem is that I don't seem to have access to StepReattachCDRom at that point.

I have to revisit the code but I think you can take a different approach here. Instead of erroring and adding a check inside of CreateConfig you could take the following approach.

1. Add documentation to the reuse_vm attribute and reattach_cdrom attributes indicating that any settings for reattach_cdrom will be ignored when reuse_vm is true because of xyz reasons.
2. Add a boolean field to StepReattachCDRom called SkipStep or something to denote the step should not be executed.
3. In StepReattachCDRom.Run have a conditional that checks if SkipStep is set. If set display a message to the user "that the reattachment of n cdroms is being ignored because a conflicting setting such as reuse_vm is set" and return multistep.ActionContinue to return immediately.
4. In the builder.go file update the step initialization to include the value of ReuseVM

&common.StepReattachCDRom{
 Config:      &b.config.ReattachCDRomConfig,
 CDRomConfig: &b.config.CDRomConfig,
 SkipStep:   b.config.CreateConfig.ReuseVM,
},

[Hi-Angel]

• Add documentation to the reuse_vm attribute and reattach_cdrom attributes indicating that any settings for reattach_cdrom will be ignored when reuse_vm is true because of xyz reasons.

Well, it would seem to me that erroring out is better from the POV of a user, because otherwise a user who haven't read documentation too carefully would wonder "why reattach_cdrom is not working".

In my experience, the "ignore X if Y is set" design typically leads to hard to find bugs on the user side.

[Hi-Angel]

Any news here?