[Issues 257, 263, 187, 80] - AsyncIO disk config support, ISO Storage uplift

[mpywell]

Hey @lbajolet-hashicorp!

As mentioned in #260, here is my follow up to close out some related storage open issues.

To resolve #257 I've added AsyncIO as an option to the storage uplift from #260.

To resolve #263 and #187 I've added a config option to the proxmox-iso builder's ISO device to specify a device type and bus index in the same style as the Additional ISO Files block device attribute.

To address #80 I've changed the default behaviour of the unmount option for ISOs to remove the empty CDROM device. A new config option unmount_keep_device is available if there's a desire to keep empty CDROM devices in the produced VM template.

The commits for #263, #187 and #80 have a description that further explains their changes.

Closes #257, #263, #187, #80

[mpywell]

Hey @lbajolet-hashicorp,

I've pushed a reroll that consolidates the iso builder's iso and common's additional_iso_files into a single common iso struct.

  ## HCL v1.8 iso builder example
  # Define boot iso device
  iso_url          = "https://cdimage.debian.org/debian-cd/current/amd64/iso-cd/debian-12.5.0-amd64-netinst.iso"
  iso_checksum     = "sha512:33c08e56c83d13007e4a5511b9bf2c4926c4aa12fd5dd56d493c0653aecbab380988c5bf1671dbaea75c582827797d98c4a611f7fb2b131fbde2c677d5258ec9"
  iso_storage_pool = var.iso_storage_pool
  unmount_iso      = true

  # define additional iso device
  additional_iso_files {
    device           = "ide0"
    iso_storage_pool = var.iso_storage_pool
    unmount          = true
    iso_url          = "https://fedorapeople.org/groups/virt/virtio-win/direct-downloads/stable-virtio/virtio-win.iso"
    iso_checksum     = "none"
  }

  ## HCL for this commit
  isos {
    type         = "ide"
    storage_pool = var.iso_storage_pool
    unmount      = true
    iso_url      = "https://cdimage.debian.org/debian-cd/current/amd64/iso-cd/debian-12.5.0-amd64-netinst.iso"
    iso_checksum = "sha512:33c08e56c83d13007e4a5511b9bf2c4926c4aa12fd5dd56d493c0653aecbab380988c5bf1671dbaea75c582827797d98c4a611f7fb2b131fbde2c677d5258ec9"
  }
  isos {
    type         = "ide"
    storage_pool = var.iso_storage_pool
    unmount      = true
    iso_url      = "https://fedorapeople.org/groups/virt/virtio-win/direct-downloads/stable-virtio/virtio-win.iso"
    iso_checksum = "none"
  }

The isos are enumerated in the order they are defined just as disks are, after disks have been enumerated.

This enables all ISOs to have uniform configuration and makes common the central point of storage validation for both the iso and clone builders.

I've also added a prestep to the clone builder to map source vm disks to their device type and index before common enumerates packer defined disks and isos to enable the appending of new disks and isos to a cloned vm.

generateProxmoxDisks now returns ui and state errors if device types are overallocated

[lbajolet-hashicorp]

Hey @mpywell,

Just to let you know, I haven't forgotten about this PR, I plan on reviewing it this week (FYI I'll be OOO for a couple weeks after this so this might take a while before getting folded into a release, sorry about this).

I like the new look for the template, reifying everything as a list of blocks seems on point to me.

I'll take a look at the code to make sure this makes sense.

Question: this changes the supported grammar, are we expecting a breaking change in the configs?

Edit: looking at the template once more, it doesn't seem that there's a difference between the main ISO for the builder and the extra ISOs to mount; I wonder if we could make the main ISO config an attribute that accepts this block, and the additional ISOs being a list of iso blocks (as in a slice attribute, not a BlockList)?

[mpywell]

Hey @lbajolet-hashicorp,

Yeah at a functional level there are no differences between the main ISO and the additional ISOs, ie. both are processed by the proxmox-api-go SDK in the same way and accept the same configuration options in the Proxmox backend. The Proxmox backend also only has the one 'cdrom' VM device for attaching ISOs.

I pivoted to making the ISO definition the same for both based on this and because of the earlier challenges around validating two types of ISO config, but it does also mean that packer configs for the next release would need adjusting to this new iso block format, the 1.8 values wouldn't be accepted anymore.

I didn't quite understand your comment about making the main ISO config an attribute that accepts the block, could you show me in a code snippet example? Or feel free to add a commit 😄

[lbajolet-hashicorp]

Hey @mpywell,

Thanks for the quick response, much appreciated!

Regarding the main ISO config being an attribute instead of an entry to the ISO block list I meant something like this:

  ## HCL for current HEAD
  isos {
    type         = "ide"
    storage_pool = var.iso_storage_pool
    unmount      = true
    iso_url      = "https://cdimage.debian.org/debian-cd/current/amd64/iso-cd/debian-12.5.0-amd64-netinst.iso"
    iso_checksum = "sha512:33c08e56c83d13007e4a5511b9bf2c4926c4aa12fd5dd56d493c0653aecbab380988c5bf1671dbaea75c582827797d98c4a611f7fb2b131fbde2c677d5258ec9"
  }
  isos {
    type         = "ide"
    storage_pool = var.iso_storage_pool
    unmount      = true
    iso_url      = "https://fedorapeople.org/groups/virt/virtio-win/direct-downloads/stable-virtio/virtio-win.iso"
    iso_checksum = "none"
  }

  ## HCL example with ISO being an attribute
  iso = {
    type         = "ide"
    storage_pool = var.iso_storage_pool
    unmount      = true
    iso_url      = "https://cdimage.debian.org/debian-cd/current/amd64/iso-cd/debian-12.5.0-amd64-netinst.iso"
    iso_checksum = "sha512:33c08e56c83d13007e4a5511b9bf2c4926c4aa12fd5dd56d493c0653aecbab380988c5bf1671dbaea75c582827797d98c4a611f7fb2b131fbde2c677d5258ec9"
  }
  additional_isos = [{
    type         = "ide"
    storage_pool = var.iso_storage_pool
    unmount      = true
    iso_url      = "https://fedorapeople.org/groups/virt/virtio-win/direct-downloads/stable-virtio/virtio-win.iso"
    iso_checksum = "none"
  }]

This can be done by playing with mapstructure tags in the config, typically for the ISO builder this could be something like this:

type Config struct {
	[...]
	ISO ISOsConfig `mapstructure:"iso" required:"true"`
	// ISO files attached to the virtual machine.
	// See [ISO Files](#iso-files).
	ExtraISOs []ISOsConfig `mapstructure:"additional_iso_files"`
}

Note: I renamed your isos to additional_iso_files here in order to maintain compatibility, I think we should strive to avoid breaking existing configs, unless when calculated.

The ISO specified as iso could be prepended to the ISO list, and then passed to the common logic that adds ISOs to the VMConfig on proxmox.

This has the benefit of explicitly stating that this ISO is a special case that is only available for the ISO builder (clone only supports additional_iso_files anyway I think?), and is clearly marked as such. To be fair, your approach works great as well, I think it maybe should be documented that the first ISO is meant to be the boot one? Not sure if the order changes something (my guess is that'd depend on the EFI/BIOS config/revision, and the OS after that), but if we keep the current approach we should still document it at least I think.

I would think we can also preserve backwards compatibility (albeit maybe with less configuration possibilities) by keeping the original main ISO config schema alive (with deprecation warnings), and build an ISOsConfig from those, so we can still keep the updated logic in one spot. If we go with this, we should also make sure the two alternatives are mutually exclusive, so that means updating Prepare to do that check.

What do you think? Let me know if something's unclear, happy to elaborate more if needed. And please feel free also to tell me if you want me to delve into the code, I've got bandwidth for that a bit, so happy to tinker if you think that'd help.

Thanks again!

[mpywell]

Hey @lbajolet-hashicorp,

I've pushed another reroll with your suggestion, the boot iso is prepended to common's ISO slice and enumerated through common as before.

 iso = {
   type         = "ide"
   iso_storage_pool = var.iso_storage_pool
   unmount      = true
   iso_url      = "https://cdimage.debian.org/debian-cd/current/amd64/iso-cd/debian-12.5.0-amd64-netinst.iso"
   iso_checksum = "sha512:33c08e56c83d13007e4a5511b9bf2c4926c4aa12fd5dd56d493c0653aecbab380988c5bf1671dbaea75c582827797d98c4a611f7fb2b131fbde2c677d5258ec9"
 }
 additional_iso_files = [{
   type         = "ide"
   iso_storage_pool = var.iso_storage_pool
   unmount      = true
   iso_url      = "https://fedorapeople.org/groups/virt/virtio-win/direct-downloads/stable-virtio/virtio-win.iso"
   iso_checksum = "none"
 }]

Note: As we're moving to enumeration for ISO devices the additional_iso_files block's type field doesn't accept device indexes anymore, so there will still be changes required in packer configs. I might try to clarify this in docs through another commit

[mpywell]

Hey @lbajolet-hashicorp just following up to see when we might be able to pick this one up again?

[lbajolet-hashicorp]

Hi @mpywell,

Sorry was a bit scattered these last few weeks, apologies for leaving you hanging. I'll do another pass of review ASAP on this!

[mpywell]

Hi @lbajolet-hashicorp, thanks for your feedback.

I think it should be possible to implement a conversion for existing additionalISOsConfig configs to ISOsConfig, leave it with me and I'll see what I can do, it shouldn't take too long.

If I have any difficulty I'll reach out and take you up on organising a meet.

[mpywell]

Hi @lbajolet-hashicorp,

I've added backwards compatibility support for all of the ISO changes, let me know your thoughts.

[mpywell]

@lbajolet-hashicorp I've resolved the last few items, should be right to merge

[lbajolet-hashicorp]

Awesome, thanks for the update and the ping @mpywell, much appreciated.

We are good to go, merging this one now, and I'll schedule a release for the plugin ASAP, we've got a few items to cram into this one :)