[Issue 74] Create var for Cloud-Init Disk Type #256

xrayj11 · 2024-03-24T02:07:07Z

This change will allow users to specify a cloud_init_disk_type, which determines which type (ide, sata, scsi) of drive is created for Cloud-Init. If no value is given or the variable is not provided, the Cloud-Init drive will be created on the first available ide drive, which was how it operated before.
This change should not break any existing configurations that use Cloud-Init, and only provides additional customization to specify a different drive type than the default ide

These changes passed the make test, as well as packer validation after installing the newly packaged plugin and declaring the cloud_init_disk_type var in my hcl file.
I have tested this change with the proxmox-iso builder on each disk type (ide, sata, scsi) as well as not providing the var to ensure it will not break existing configurations. Each option resulted in the expected outcome, where the Cloud-Init drive was created with the type specified by the var.

Closes #74 - By giving the option of SCSI for Cloud-Init

hashicorp-cla · 2024-03-24T02:07:11Z

All committers have signed the CLA.

lbajolet-hashicorp

Hi @xrayj11,

Thanks for the PR, and sorry I took a long time to review it.

This looks good to me overall, I have left a couple comments/suggestions, feel free to address them, and I can do another review when you've had time to respond.

Thanks again!

lbajolet-hashicorp · 2024-05-22T15:12:31Z

builder/proxmox/common/step_finalize_template_config.go

+					scsiController := fmt.Sprintf("scsi%d", i)
+					diskControllers = append(diskControllers, scsiController)
+				}
+			default:


I would suggest defaulting to ide only if it's empty or explicitly ide, otherwise this allows for anything and I can see this being a surprise potentially.

Suggested change

default:

"", "ide":

EDIT: nevermind I notice we're in Run here, I'd suggest then having that check/error in the Prepare function then, so we can set ide if unset for example, and we can error if the option has an unsupported value.

I have added a default, check, and error in the Prepare function. Does this look good?
I tested it with a CloudInitDiskType value that was not supported (not scsi,sata, or ide) and it stopped packer with the error message I added. I also confirmed that blanks still default to ide.
e03831f

I would still suggest expliciting ide for this case, just in case something else manages to go through the Prepare, so that it doesn't go into ide mode when reaching this step.
Adding a default with an error is probably for the best for later developments, as this will error in case we update Prepare but forget to do the same here.

Suggested change

default:

"ide":

// Unspecified disk type defaults to "ide"

diskControllers = []string{"ide0", "ide1", "ide2", "ide3"}

default:

state.Put("error", fmt.Errorf("unsupported disk type %q", c.CloudInitDiskType))

return multistep.ActionHalt

lbajolet-hashicorp · 2024-05-22T15:17:03Z

builder/proxmox/common/config.go

@@ -183,6 +183,9 @@ type Config struct {
 	// Name of the Proxmox storage pool
 	// to store the Cloud-Init CDROM on. If not given, the storage pool of the boot device will be used.
 	CloudInitStoragePool string `mapstructure:"cloud_init_storage_pool"`
+	// The type of Cloud-Init disk. Can be `scsi`, `sata`, or `ide`
+	// If not given, defaults to `ide`.


Nit to make it more concise.

Suggested change

// If not given, defaults to `ide`.

// Defaults to `ide`.

lbajolet-hashicorp

Hi @xrayj11,

Thanks for the update, I left a few more comments on the code for simplification and hardening purposes, please feel free to take a look and address them when you can.

I'll do another review when you've had time to do this.

Thanks!

lbajolet-hashicorp · 2024-05-27T14:28:34Z

builder/proxmox/common/config.go

@@ -652,6 +652,16 @@ func (c *Config) Prepare(upper interface{}, raws ...interface{}) ([]string, []st
 		log.Printf("SCSI controller not set, using default 'lsi'")
 		c.SCSIController = "lsi"
 	}
+	if c.CloudInit == true {


Suggested change

if c.CloudInit == true {

if c.CloudInit {

lbajolet-hashicorp · 2024-05-27T14:31:27Z

builder/proxmox/common/config.go

@@ -652,6 +652,16 @@ func (c *Config) Prepare(upper interface{}, raws ...interface{}) ([]string, []st
 		log.Printf("SCSI controller not set, using default 'lsi'")
 		c.SCSIController = "lsi"
 	}
+	if c.CloudInit == true {
+		if c.CloudInitDiskType == "" {


I believe the whole if c.CloudInit block can be simplified with a switch?

switch c.CloudInitDiskType { case "ide", "scsi", "sata": case "": c.CloudInitDiskType = "ide" default: errs = packersdk.MultiErrorAppend(errs, fmt.Errorf("invalid value for `cloud_init_disk_type` %q: only one of 'ide', 'scsi', and 'sata' is valid", c.CloudInitDiskType)) }

lbajolet-hashicorp · 2024-05-27T14:35:45Z

builder/proxmox/common/step_finalize_template_config.go

+					scsiController := fmt.Sprintf("scsi%d", i)
+					diskControllers = append(diskControllers, scsiController)
+				}
+			default:


I would still suggest expliciting ide for this case, just in case something else manages to go through the Prepare, so that it doesn't go into ide mode when reaching this step.
Adding a default with an error is probably for the best for later developments, as this will error in case we update Prepare but forget to do the same here.

Suggested change

default:

"ide":

// Unspecified disk type defaults to "ide"

diskControllers = []string{"ide0", "ide1", "ide2", "ide3"}

default:

state.Put("error", fmt.Errorf("unsupported disk type %q", c.CloudInitDiskType))

return multistep.ActionHalt

lbajolet-hashicorp · 2024-05-27T14:38:21Z

.web-docs/components/builder/iso/README.md

@@ -197,6 +197,9 @@ in the image's Cloud-Init settings for provisioning.
 - `cloud_init_storage_pool` (string) - Name of the Proxmox storage pool
  to store the Cloud-Init CDROM on. If not given, the storage pool of the boot device will be used.

+- `cloud_init_disk_type` (string) - The type of Cloud-Init disk. Can be `scsi`, `sata`, or `ide`
+  If not given, defaults to `ide`.


I believe this should update itself based on the comment from config.go, have you run make generate to update that?

lbajolet-hashicorp · 2024-05-27T21:43:21Z

Hi @xrayj11,

The rework looks good to me, however it seems there are failures during test, also the file should be linted/goimported so the CI is happy, may I ask you to fix those before we can merge?

xrayj11 · 2024-05-28T02:14:16Z

Hey @lbajolet-hashicorp, I've been trying to figure out the issue with the failed all_options_with_cloud-init test, and I think I've narrowed it down. Although we have a case to default to ide when CloudInitDiskType is blank in the Prepare function of config.go, the test is not using it. This means it's going into the Run function with a blank/undefined value and triggering the default case, and erroring out.
When I define a valid CloudInitDiskType here the test passes:
https://github.com/hashicorp/packer-plugin-proxmox/blob/main/builder/proxmox/common/step_finalize_template_config_test.go#L106-L112
Could we do that, or is there another way we should be testing this since the value can be changed/defaulted by config.go?
Also, I've fixed the formatting/goimport issues, they'll be added once we figure out the test.

lbajolet-hashicorp · 2024-05-28T20:46:32Z

Ah I see @xrayj11, in this case I would suggest either of the two alternatives:

Add a call to Prepare before calling Run here: this way only valid configs will reach Run. This is more in line with what we expect from normal plugin usage, as such it cannot hurt imho. This could conflict with other tests though, so I would advise not doing this if this becomes a problem for other test cases.
Add a CloudInitDiskType to the config, so it is not empty when executing Run in this case. Given this option is supposed to be vetted/set by Prepare, if the aim of the test is only to test Run in a valid case, this may be a valid change.

I'll let you pick which approach seems more appropriate to you, feel free to ping me when you've settled on either!

…n Prepare. config.go formatting

xrayj11 · 2024-05-29T00:47:27Z

I looked into trying to call Prepare in the test(s), but it looks like it's going to require a bit of a rewrite of how all the tests are done. I do agree it would be a better implementation of testing since it is more in line with how it actually runs. Since that's out of the scope of this issue, I've just defined CloudInitDiskType: "ide" on any test that uses CloudInit: true.
All tests ran good on my side, and in actual packer runs in my environment the plugin still works without defining a CloudInitDiskType, so existing configs will not be affected. Let me know if you see anything else that needs changed, or if this is can be merged.
Thanks for the help @lbajolet-hashicorp

lbajolet-hashicorp · 2024-05-29T13:30:49Z

Looks good to me @xrayj11, thanks for the reroll!

Will merge if/when tests go green

xrayj11 added 2 commits March 23, 2024 11:45

Add cloud_init_disk_type argument

816abb5

fix controller var for cloud-init disk

d08938f

xrayj11 requested a review from a team as a code owner March 24, 2024 02:07

Use make generate for docs

f32babd

lbajolet-hashicorp reviewed May 22, 2024

View reviewed changes

Add CloudInitDiskType checks and default to Prepare function

e03831f

xrayj11 requested a review from lbajolet-hashicorp May 27, 2024 01:46

lbajolet-hashicorp reviewed May 27, 2024

View reviewed changes

Use switch for CloudInit Prepare, add IDE case in Run, update docs

3ad50a6

Define CloudInitDiskType in tests since checking/defaulting is done i…

362560e

…n Prepare. config.go formatting

lbajolet-hashicorp approved these changes May 29, 2024

View reviewed changes

lbajolet-hashicorp merged commit f75bdf4 into hashicorp:main May 29, 2024
12 checks passed

lbajolet-hashicorp added the enhancement label Sep 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Issue 74] Create var for Cloud-Init Disk Type #256

[Issue 74] Create var for Cloud-Init Disk Type #256

xrayj11 commented Mar 24, 2024

hashicorp-cla commented Mar 24, 2024 •

edited

Loading

lbajolet-hashicorp left a comment

lbajolet-hashicorp May 22, 2024

lbajolet-hashicorp May 22, 2024

xrayj11 May 26, 2024

lbajolet-hashicorp May 27, 2024

lbajolet-hashicorp May 22, 2024

lbajolet-hashicorp left a comment

lbajolet-hashicorp May 27, 2024

lbajolet-hashicorp May 27, 2024

lbajolet-hashicorp May 27, 2024

lbajolet-hashicorp May 27, 2024

lbajolet-hashicorp commented May 27, 2024

xrayj11 commented May 28, 2024

lbajolet-hashicorp commented May 28, 2024

xrayj11 commented May 29, 2024

lbajolet-hashicorp commented May 29, 2024

-			default:
+			"ide":
+				// Unspecified disk type defaults to "ide"
+				diskControllers = []string{"ide0", "ide1", "ide2", "ide3"}
+			default:
+				state.Put("error", fmt.Errorf("unsupported disk type %q", c.CloudInitDiskType))
+				return multistep.ActionHalt

[Issue 74] Create var for Cloud-Init Disk Type #256

[Issue 74] Create var for Cloud-Init Disk Type #256

Conversation

xrayj11 commented Mar 24, 2024

hashicorp-cla commented Mar 24, 2024 • edited Loading

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

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

lbajolet-hashicorp commented May 27, 2024

xrayj11 commented May 28, 2024

lbajolet-hashicorp commented May 28, 2024

xrayj11 commented May 29, 2024

lbajolet-hashicorp commented May 29, 2024

hashicorp-cla commented Mar 24, 2024 •

edited

Loading