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

mpywell · 2024-05-31T14:28:57Z

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

This commit changes the default behaviour for proxmox-iso with unmount_iso=true and additional_iso_files blocks with unmount=true to also remove the cdrom device from provisioned VM templates. A new configuration option unmount_keep_device allows templates to retain an empty cdrom device.

Enabled device type and index assignment to the default ISO required by proxmox-iso. In testing it was observed that when statically assigning a device type and slot to an Additional ISO block it would overwrite iterative Disk index assignments. This would have been existing behaviour in >1.8. Logic is added in this commit to reserve the default ISO and additional ISO file block's configured bus and index then assign disks around these. Validation added to ensure default ISO and an Additional ISO cannot be assigned the same bus index. In testing it was observed there was no validation to prevent overassignment of devices to a bus, eg. IDE has a max capacity of 4 devices, SATA 6 devices. This would have been existing behaviour in >1.8. Logic added in this commit to proxmox-iso to validate total assignments to each bus don't exceed their limits.

lbajolet-hashicorp

Hey @mpywell,

Thanks for opening this one!

There's a lot happening in this PR, I've reviewed each commit independently to make it clearer to me what's happening, I left a bunch of comments/questions on the code, please feel free to address them and I'll be back for another round of reviews.

builder/proxmox/common/step_start_vm.go

lbajolet-hashicorp · 2024-05-31T15:54:36Z

builder/proxmox/common/config.go

@@ -664,6 +667,9 @@ func (c *Config) Prepare(upper interface{}, raws ...interface{}) ([]string, []st
 				}
 			}
 		}
+		if disk.AsyncIO != "" && !(disk.AsyncIO == "native" || disk.AsyncIO == "threads" || disk.AsyncIO == "io_uring") {


To refer to what I commented below, here's in essence what I have in mind, let me know what you think!

Suggested change

if disk.AsyncIO != "" && !(disk.AsyncIO == "native" || disk.AsyncIO == "threads" || disk.AsyncIO == "io_uring") {

if disk.AsyncIO == "" {

disk.AsyncIO = "io_uring"

}

switch disk.AsyncIO{

case "native", "threads", "io_uring":

default:

errs = packersdk.MultiErrorAppend(errs, fmt.Errorf("AsyncIO must be native, threads or io_uring"))

}

Note: you may want to gofmt this, not sure what I wrote is fully compliant :)

Looks good! I've added this to the reroll.

lbajolet-hashicorp · 2024-05-31T15:59:53Z

builder/proxmox/common/config.go

+	Unmount bool `mapstructure:"unmount"`
+	// Keep CDRom device attached to template if unmounting ISO. Defaults to `false`.
+	// Has no effect if unmount is `false`
+	UnmountKeepDevice    bool   `mapstructure:"unmount_keep_device"`


I would maybe suggest renaming this so it's clearer that the cdrom device is kept alive.

Suggested change

UnmountKeepDevice bool `mapstructure:"unmount_keep_device"`

KeepCDRomDevice bool `mapstructure:"keep_cdrom_device"`

Though I guess there's a point to be made for cohesion with the unmount argument there, so feel free to keep the current naming scheme if you feel this is more relevant!

I was going for cohesion, but I do think KeepCDRomDevice is better so I've rerolled with this naming.

builder/proxmox/iso/config.go

builder/proxmox/common/config.go

lbajolet-hashicorp · 2024-06-03T15:43:57Z

builder/proxmox/iso/config.go

+	// For `ide` the bus index ranges from 0 to 3, for `sata` from 0 to 5 and for
+	// `scsi` from 0 to 30.
+	// Defaults to `ide2`
+	ISODevice string `mapstructure:"iso_device"`


I wonder if this should just be an ISODeviceType, so they are all appended to the list of the appropriate type without needing to give it an index?

I was on the fence as to whether to treat this like Disks and just accept sata, ide or scsi then leave it to generateProxmoxDisks to assign a free bus index, I ended up deciding to make the behaviour consistent with the existing Device setting for the additional_iso_files block.

Yeah regarding disks I can see a reason why we'd want to keep the order in which we enumerate them (even when modern OSes don't access their devices by the order they're discovered but through UUID), regarding cdrom devices, I believe they can fill-in the remaining slots, that would be likely be simpler and probably won't break existing configs.
What do you think?

I tracked down the original additional_iso_files issue hashicorp/packer#7950 and PR hashicorp/packer#9653 that brought the logic into the builder to understand why a device and index are specified. It looks like the builder at the time only accepted one set of iso device config (what the proxmox-iso builder now handles) and additional_iso_files was added in.

It seems the device and index weren't explicit requirements in the feature request, so we could look at removing the index attribute for all ISOs and just enable the assignment of a device type. Looking at hashicorp/packer#9653 (comment) we've ended up implementing the second proposed option for disks to meet the new API struct format anyway so we would just be folding in ISO devices.

It would certainly simplify aspects of this PR 😉 however this would be a breaking change for existing packer configs where an additional_iso_files block has a device field value.

I'm happy to have a go at updating everything to use device type assignments without indexes.. with the above information do you think this is a viable way forward?

lbajolet-hashicorp · 2024-06-03T15:47:00Z

builder/proxmox/iso/config.go

+			}
+		}
+		if !(device == "ide" || device == "sata" || device == "scsi") {
+			errs = packersdk.MultiErrorAppend(errs, errors.New("iso_device must be of type ide, sata or scsi. VirtIO not supported for ISO devices"))


This could be the default for the switch device above I think?

I've updated this to be the default for switch device in the reroll commit

lbajolet-hashicorp · 2024-06-03T15:49:29Z

builder/proxmox/iso/config.go

+			scsiCount++
+		}
+		for idx, iso := range c.AdditionalISOFiles {
+			if iso.Device == c.ISODevice {


Does that iso.Device come from proxmox, or is it coming from the SDK? Wondering if this should also be updated to only specify the device type so the plugin can handle the numbering automatically
Of course if this comes from the SDK and we already support it this way, we cannot remove that capability on a minor/patch version, please let me know if this is the case, and we can ignore/revisit my suggestion then.

iso.Device here is the Device field for an additional_iso_files block (from common's Config struct). This was added to ensure the ISO device in the iso builder's Config struct doesn't conflict with a device assignment in the common builder's Config struct, an example where this could happen from a user's perspective would be having iso_device = sata0 and additional_iso_files{ device = sata0 } configured at the same time.

Gotcha, so this is indeed part of the proxmox plugin, so we can change this for this plugin. I would maybe revisit the way we add those iso devices to the VM config, using a single function from common, this way we can use it from both the Clone and ISO builders, and not have this logic scattered around.
Maybe we can add this as a step is relevant? Otherwise a function should do the trick possibly.
Let me know what you think about this idea, and if you need some help with the code, feel free to ask and I can take a jab at that!

With the background obtained answering the above comment on device indexes, I think iso and additional_iso_files could potentially be merged together and maybe the packer config adapted to just have 1 or more iso blocks rather than iso_* fields/values outside of a block for the first ISO then multiple additional_iso_files blocks for each after? They could be enumerated in the order they're defined just as the disks blocks are, that way you could consistently define the first ISO as an Operating System installer media, second is an autounattend.xml for Windows builds (just examples here), third VirtIO drivers and so on.
I do then wonder what the purpose of the iso builder would be if all the ISO logic moves to common.

builder/proxmox/iso/config.go

builder/proxmox/common/step_start_vm.go

- Rename UnmountKeepDevice to KeepCDRomDevice - Added for loop limit for proxmoxGenerateDisks - Added disk validation for clone builder - Code quality improvements to iso builder storage validation

- this reroll merges iso builder's iso device and common's additional_iso_files into a single common iso struct. - disk and iso validation is now handled centrally in common - generateProxmoxDisks handles overassignment of device types with returned ui and state errors - new prestep added to clone builder to map source vm storage before packer disk and iso enumeration. Enables appending of new disks to a cloned vm.

mpywell · 2024-06-12T13:39:20Z

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 · 2024-06-18T18:47:46Z

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 · 2024-06-19T06:38:48Z

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 · 2024-06-19T14:58:18Z

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!

- to reduce impact on packer configs of previous versions, revert to additional_iso_files map naming - the boot iso is now a config item under the iso builder and prepended to common's ISOs slice for enumeration

mpywell · 2024-06-20T09:07:31Z

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

In testing it was observed that the upstream disk API changes are excluding disks from Proxmox backup jobs by default (setting backup=0). This commit reinstates the behaviour of previous versions of this packer-plugin-proxmox where disks created by Packer are enabled for backup by default. A new ExcludeFromBackup disk field has been added to opt out of this behaviour.

mpywell · 2024-07-31T05:41:13Z

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

lbajolet-hashicorp · 2024-09-06T14:27:56Z

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!

lbajolet-hashicorp

Hi @mpywell,

Thanks for your persistence, and apologies for not reviewing this earlier.

Overall the code looks good to me, I am however worried about this change being breaking for existing configurations. We could make this part of a major release if there's no way around it, as the code does fix several issues, and IMHO makes the management of extra disks/ISOs much clearer, but I would like to explore if this needs to be a breaking change.

My understanding is that ISOsConfig replaces the current additionalISOsConfig, but I wonder if we can make the transition easier for the future.
Typically is there a way to convert entries of the current structure into the new ones. My rationale being that if an existing template defines additional_iso_files in their configuration, we can maybe convert those into the new structure, even if it doesn't map 1:1; typically device contains both how the media is attached and an ID that is now computed on the fly, we could maybe extract the device type from additional_iso_files and use the new numbering strategy as a replacement. This would allow existing templates to conform to the new model, even if there's a slight change. Or we could try to fit the numbering at the same time if this is important for some reason.
This would be accompanied by a warning if using the old structure, so users know they should update their template to match the new model, making the transition between the old world and the new one smoother.

I'll rely on you to tell me if this is possible or not, and if you want to meet to discuss that, please let me know and we can probably schedule something; I suspect this'll be easier for us to reach an understanding, but we can also continue discussing this on the PR.

Thanks again for the effort, we really appreciate this.

mpywell · 2024-09-11T02:51:05Z

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.

- Add conversion handling, tests, UI deprecation warnings for deprecated iso builder config options - Add conversion handling, tests, UI deprecation warnings for `device` field in ISOsConfig struct - Add logic to handle statically assigned ISO devices from `device` field - Add default static mapping of boot ISO to ide2 if boot_iso `type` undefined in packer config - Update documentation

mpywell · 2024-09-18T11:51:19Z

Hi @lbajolet-hashicorp,

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

lbajolet-hashicorp

Hey @mpywell,

Thanks for the reroll!
From the looks of it, I would think we are good in terms of backwards-compatibility, and the new features, overall LGTM!

I left a few last nits/questions on the code, feel free to address those when you can, but we're nearing a merge, kudos to you!

builder/proxmox/iso/config.go

builder/proxmox/clone/step_map_source_disks.go

lbajolet-hashicorp · 2024-09-23T15:03:40Z

builder/proxmox/common/config.go

-	scsiCount := 0
-	virtIOCount := 0
+	// validate iso devices
+	for idx := range c.ISOs {


Doesn't need to happen in this PR, but I wonder if we can make each ISOs here have its own Prepare function with a similar signature as the global Prepare, if only to split the logic into something more bite-sized.

I think maybe will leave it as is for now as this section would become a bit tidier when the device field for ISOs is eventually removed, lines 690-734 handle its validation and conversion.

builder/proxmox/common/config_test.go

lbajolet-hashicorp · 2024-09-23T15:09:01Z

builder/proxmox/common/config_test.go

+			// Ensure deprecated device field is converted
+			name: "device should be converted to type and index",
+			ISOs: map[string]interface{}{
+				"device":   "ide3",


Might be superfluous, but I wonder if we should add cases with a device index too high? These are probably caught by the blob above though, I'll leave it up to you to decide

The handling of bus overallocation moved to generateProxmoxDisks in builder/proxmox/common/step_start_vm.go because there's also the potential for overallocation when preserving clone builder disks.

I've pushed an update for TestGenerateProxmoxDisks in builder/proxmox/common/step_start_vm_test.go to test overallocating sata storage with a mix of defined disks, ISOs and clone source vms mappings, do you think one is needed for each storage type?

Needed is maybe a strong word, the logic is simple so I would think we can proceed without urgently needing them, but it would be better if there were here just to make sure we don't break them in the future.

- Added test for overallocation of storage - Added additional tests for ISO `device` field conversion - Added missing default bus detail to `device` field documentation

mpywell

Hey @lbajolet-hashicorp, I've pushed a reroll that I think addresses some of the points raised, let me know if you have any further questions

mpywell · 2024-09-24T00:08:26Z

builder/proxmox/common/config.go

-	scsiCount := 0
-	virtIOCount := 0
+	// validate iso devices
+	for idx := range c.ISOs {


I think maybe will leave it as is for now as this section would become a bit tidier when the device field for ISOs is eventually removed, lines 690-734 handle its validation and conversion.

builder/proxmox/clone/step_map_source_disks.go

builder/proxmox/common/config_test.go

mpywell · 2024-09-24T01:32:08Z

builder/proxmox/common/config_test.go

+			// Ensure deprecated device field is converted
+			name: "device should be converted to type and index",
+			ISOs: map[string]interface{}{
+				"device":   "ide3",


The handling of bus overallocation moved to generateProxmoxDisks in builder/proxmox/common/step_start_vm.go because there's also the potential for overallocation when preserving clone builder disks.

I've pushed an update for TestGenerateProxmoxDisks in builder/proxmox/common/step_start_vm_test.go to test overallocating sata storage with a mix of defined disks, ISOs and clone source vms mappings, do you think one is needed for each storage type?

lbajolet-hashicorp

Left a final couple of nits, but overall LGTM!

Thanks for pushing that one @mpywell, much appreciated.
I'll let you address those as you see fit, but imho we should be ready to merge after that.

I'm pre-approving this PR so it's not a blocker later

lbajolet-hashicorp · 2024-09-24T15:00:48Z

builder/proxmox/common/config_test.go

+			// Ensure deprecated device field is converted
+			name: "device should be converted to type and index",
+			ISOs: map[string]interface{}{
+				"device":   "ide3",


Needed is maybe a strong word, the logic is simple so I would think we can proceed without urgently needing them, but it would be better if there were here just to make sure we don't break them in the future.

builder/proxmox/common/config_test.go

mpywell · 2024-09-25T23:35:37Z

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

lbajolet-hashicorp · 2024-09-26T13:27:27Z

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 :)

mpywell added 3 commits May 31, 2024 14:11

common: add disk config asyncio setting

13a0b51

mpywell requested a review from a team as a code owner May 31, 2024 14:28

fix: replace ide2 static references in finalize_iso

064a8b5

lbajolet-hashicorp reviewed Jun 3, 2024

View reviewed changes

mpywell added 2 commits June 4, 2024 14:09

fix: code quality updates

7b4fd64

- Rename UnmountKeepDevice to KeepCDRomDevice - Added for loop limit for proxmoxGenerateDisks - Added disk validation for clone builder - Code quality improvements to iso builder storage validation

iso: update basic example documentation with iso block

a52fd9c

iso: reintroduce iso config struct for boot iso

6b51d75

- to reduce impact on packer configs of previous versions, revert to additional_iso_files map naming - the boot iso is now a config item under the iso builder and prepended to common's ISOs slice for enumeration

mpywell added 2 commits June 20, 2024 09:24

common: update documentation of ISOsConfig Type field

e635f0e

lbajolet-hashicorp reviewed Sep 9, 2024

View reviewed changes

lbajolet-hashicorp reviewed Sep 23, 2024

View reviewed changes

Improve tests, update doc

b51baca

- Added test for overallocation of storage - Added additional tests for ISO `device` field conversion - Added missing default bus detail to `device` field documentation

mpywell commented Sep 24, 2024

View reviewed changes

lbajolet-hashicorp approved these changes Sep 24, 2024

View reviewed changes

test all buses for overallocation, cleanup test bool conditions

bfa19d5

lbajolet-hashicorp merged commit c0a6e24 into hashicorp:main Sep 26, 2024
12 checks passed

lbajolet-hashicorp added the enhancement label Sep 26, 2024

-		if disk.AsyncIO != "" && !(disk.AsyncIO == "native" || disk.AsyncIO == "threads" || disk.AsyncIO == "io_uring") {
+		if disk.AsyncIO == "" {
+			disk.AsyncIO = "io_uring"
+		}
+		switch disk.AsyncIO{
+		case "native", "threads", "io_uring":
+		default:
+			errs = packersdk.MultiErrorAppend(errs, fmt.Errorf("AsyncIO must be native, threads or io_uring"))
+		}

	UnmountKeepDevice bool `mapstructure:"unmount_keep_device"`
	KeepCDRomDevice bool `mapstructure:"keep_cdrom_device"`

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

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

Conversation

mpywell commented May 31, 2024

lbajolet-hashicorp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpywell Jun 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpywell Jun 5, 2024 • edited Loading

Choose a reason for hiding this comment

mpywell commented Jun 12, 2024 • edited Loading

lbajolet-hashicorp commented Jun 18, 2024 • edited Loading

mpywell commented Jun 19, 2024 • edited Loading

lbajolet-hashicorp commented Jun 19, 2024 • edited Loading

mpywell commented Jun 20, 2024

mpywell commented Jul 31, 2024

lbajolet-hashicorp commented Sep 6, 2024

lbajolet-hashicorp left a comment

Choose a reason for hiding this comment

mpywell commented Sep 11, 2024

mpywell commented Sep 18, 2024

lbajolet-hashicorp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpywell Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpywell left a comment

Choose a reason for hiding this comment

mpywell Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbajolet-hashicorp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpywell commented Sep 25, 2024

lbajolet-hashicorp commented Sep 26, 2024

mpywell Jun 5, 2024 •

edited

Loading

mpywell Jun 5, 2024 •

edited

Loading

mpywell commented Jun 12, 2024 •

edited

Loading

lbajolet-hashicorp commented Jun 18, 2024 •

edited

Loading

mpywell commented Jun 19, 2024 •

edited

Loading

lbajolet-hashicorp commented Jun 19, 2024 •

edited

Loading

mpywell Sep 24, 2024 •

edited

Loading

mpywell Sep 24, 2024 •

edited

Loading