Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix handling of temporary cdrom devices #31

Merged
merged 3 commits into from
Aug 18, 2021
Merged

Conversation

puetzk
Copy link
Contributor

@puetzk puetzk commented Aug 9, 2021

Fix two more issues with vmware and cd_{files,content}

  1. if a temporary cdPath is set up, during step_configure_vmx, it should be claned up by step_clean_vmx
    There is already code to do this cleanup, but it wasn't triggering because it was only applied to vmware-iso (and had the wrong device name there). Only floppies were added to temporaryDevices when the setup was actually being done.

  2. Fix updating/overwriting existing cdrom device properties (case-sensitivity)
    packer lowercases all the vmx keys when reading a file in:

    key := strings.ToLower(matches[1])

    so using mixed-case keys to update the .vmx introduces semantic duplicates (rather than replacing old values)

    vmxData[cdromPrefix+".fileName"] = cdPath.(string)
    vmxData[cdromPrefix+".deviceType"] = "cdrom-image"

puetzk added 2 commits August 9, 2021 18:12
There was a previous attempt to do this in step_configure_vmx,
but that only covers vmware-iso (missing potential cases when using
cd_files/cd_content in vmware-vmx) and it had the wrong device name
(would mark <type>0:<primary> as temporary when the device name used
for cd_path is actually <type>1:<primary>
ParseVMX/EncodeVMX force the keys to lowercase, so inserting with a
mixed-case key fails to replace existing values, resulting in duplicate
lines and vmware potentially still using the previous filename
@puetzk puetzk requested a review from a team as a code owner August 9, 2021 23:16
@puetzk
Copy link
Contributor Author

puetzk commented Aug 17, 2021

Hmm, just noticed one flaw in this PR - StepConfigureVMX actually runs twice:

&vmwcommon.StepConfigureVMX{
CustomData: b.config.VMXData,
VMName: b.config.VMName,
DisplayName: b.config.VMXDisplayName,
DiskAdapterType: b.config.DiskAdapterType,
CDROMAdapterType: "",
},

&vmwcommon.StepConfigureVMX{
CustomData: b.config.VMXDataPost,
SkipFloppy: true,
VMName: b.config.VMName,
DisplayName: b.config.VMXDisplayName,

And the CD data is set both times, so it actually attempts to overwrite itself (harmless, if a little weird), but that now means it adds
the cdrom twice: https://github.com/hashicorp/packer-plugin-vmware/pull/31/files#diff-9a390b1c72b8076046aeb82247ce79248acca5ac5dd9bf7f6c161d378649514fR101

and that's confirmed in the log:

vmware: Cleaning VMX prior to finishing up...
vmware: Detaching ISO from CD-ROM device ide1:0...
vmware: Detaching ISO from CD-ROM device ide1:0...

It looks like floppy avoids this because of SkipFloppy - I wonder if we should just generalize that to SkipDevices, since it really doesn't make sense create temp devices right before (with nothing in between) StepCleanVmx. The later StepConfigureVMX is just setting the VMXDataPost (and, for some reason, resetting the name and DisplayName)

// Add it to our list of build devices to later remove
tmpBuildDevices = append(tmpBuildDevices, "floppy0")

Cleaning up is idempotent, so doing it twice is better than zero times like before, but that's a little untidy.

It's not just floppies, it doesn't make sense to create any new
temporaryDevices in between StepCleanFiles and StepCleanVMX.
Copy link
Contributor

@azr azr left a comment

Choose a reason for hiding this comment

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

Hey @puetzk, super nice, this LGTM & worked on my machine ! Thanks for your time again 🙂

@azr azr merged commit 048e31a into hashicorp:main Aug 18, 2021
Comment on lines +68 to +70
// StepConfigureVMX runs both before and after provisioning (for VmxDataPost),
// the latter time shouldn't create temporary devices
if !s.SkipDevices {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍🏼

@arizvisa
Copy link
Contributor

arizvisa commented Sep 25, 2021

Ftr, there was some further discussion in #31 that might affect this PR. So I'm noting it in this PR's history despite it already being merged.

@azr
Copy link
Contributor

azr commented Sep 27, 2021

👋🏼 @arizvisa, thanks. Is #31 the PR you meant ? I think probably not, since it's this one.

Edit: I think you mean #33

@arizvisa
Copy link
Contributor

@azr: Ah, yes. My bad.

@hashicorp hashicorp locked and limited conversation to collaborators Jun 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants