Allows for the mounting of ISOs when a Proxmox VM s created. Same as … #9653

paginabianca · 2020-07-27T09:36:41Z

…in PR #9055

Allows the mounting of additional ISOs when the VM is created. The config option was taken from PR #9055 and slightly changed. Users can specify an array of bus names, bus numbers and filenames.

"cd_drive":[
{
"bus": "ide",
"bus_number": 3,
"filename": "isos:iso/virtio-win-0.1.187.iso"
},
{
"bus": "sata",
"bus_number": 3,
"filename": "isos:iso/someother.iso"
}
]

Closes: #7950

hashicorp-cla · 2020-07-27T09:36:43Z

All committers have signed the CLA.

paginabianca · 2020-07-27T15:48:59Z

Sorry that I created that many commits for PR. I was trying to get all tests to pass but somehow make generate does not generate the definitions for FlatstorageConfig as it does with FlatnicConfig, FlatdiskConfig and FlatvgaConfig.

I did add the definition of FlatstorageConfig manually but that is not allowed in autogenerated files.
I'm probably missing something here but don't really know what exactly. Can someone point me in the right direction?

I have tested the build after manually adding the definitions of FlatstorageConfig on my Proxmox host and it works.

codecov · 2020-07-28T07:51:38Z

Codecov Report

Merging #9653 into master will decrease coverage by 0.05%.
The diff coverage is 0.00%.

Impacted Files	Coverage Δ
builder/proxmox/builder.go	`2.58% <0.00%> (-0.36%)`	⬇️
builder/proxmox/config.go	`56.52% <0.00%> (-18.48%)`	⬇️
builder/proxmox/step_finalize_template_config.go	`68.53% <0.00%> (-21.17%)`	⬇️
builder/proxmox/step_start_vm.go	`16.33% <0.00%> (-1.27%)`	⬇️
builder/proxmox/step_upload_additional_isos.go	`0.00% <0.00%> (ø)`
builder/proxmox/step_upload_iso.go	`100.00% <ø> (ø)`
helper/multistep/basic_runner.go	`88.57% <0.00%> (-5.72%)`	⬇️
packer/communicator.go	`75.53% <0.00%> (-1.07%)`	⬇️
command/build.go	`71.74% <0.00%> (-0.96%)`	⬇️

SwampDragons · 2020-07-31T17:37:24Z

I'm a big fan of squashing if you have a ton of little tweak commits.

…in PR 9055 but working Fixed check-lint errors Fixed bus_number check Fixed check-genereate error Revert "Fixed bus_number check" This reverts commit ac8dc15. fmt Naming Fixed hcl2 autogenerated code issue fmt less than is not greater than

Fixed Template Finalization Error fmt Documentation and fixes

carlpett

Thanks @paginabianca!
I've left a couple of comments below. Additionally, as was discussed in the previous PR, we probably need to tackle uploads of ISOs as well.

carlpett · 2020-08-09T07:33:48Z

builder/proxmox/step_finalize_template_config.go

+			} else {
+				changes[cdrom] = c.AdditionalISOFiles[idx].Filename + ",media=cdrom"
+			}


This wouldn't be a change, right? It is what you set already in the StartVM step.

carlpett · 2020-08-09T07:34:10Z

website/pages/docs/builders/proxmox.mdx

@@ -213,6 +213,29 @@ builder.
 - `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.

+- `additional_iso_files` (array of objects) - Additional ISO filess attached to the virtual machine.


Typo

Suggested change

- `additional_iso_files` (array of objects) - Additional ISO filess attached to the virtual machine.

- `additional_iso_files` (array of objects) - Additional ISO files attached to the virtual machine.

carlpett · 2020-08-09T07:34:33Z

website/pages/docs/builders/proxmox.mdx

+    }
+  ]
+  ```
+  - `device` (string) - Bus type that the ISO will be mountet on. Can be `ide`,


Typo

Suggested change

- `device` (string) - Bus type that the ISO will be mountet on. Can be `ide`,

- `device` (string) - Bus type that the ISO will be mounted on. Can be `ide`,

Also, wouldn't it semantically be a device_type or similar, since a device would include the number as well?

carlpett · 2020-08-09T07:51:54Z

builder/proxmox/config.go

+		if c.AdditionalISOFiles[idx].Device == "" {
+			log.Printf("AdditionalISOFile %d Device not set, using default 'ide'", idx)
+			c.AdditionalISOFiles[idx].Device = "ide"
+		}
+		if !contains([]string{"ide", "sata", "scsi"}, c.AdditionalISOFiles[idx].Device) {
+			errs = packer.MultiErrorAppend(errs, fmt.Errorf("%q is not a valid AdditionalISOFile Device", c.AdditionalISOFiles[idx].Device))
+		}
+		if c.AdditionalISOFiles[idx].BusNumber == 0 {
+			log.Printf("AdditionalISOFile %d number not set, using default: '3'", idx)
+			c.AdditionalISOFiles[idx].BusNumber = 3
+		}


If I leave multiple additional isos without a bus number, they will overwrite each other this way. I can see three options:

Make bus_number mandatory (and in that case, maybe merge it with device_type so you'd specify the entire device like device="ide3")

Keep track of what device numbers we've used and use the next unused one. This will probably get tricky.

Error out if there's more than one element without a set number.

I'd lean towards option 1 because it will more obvious, although it does sacrifice a small amount of convenience when just adding a single additional iso.

carlpett · 2020-08-09T07:52:39Z

website/pages/docs/builders/proxmox.mdx

+    {
+      "device": "scsi",
+      "bus_number": "3",
+      "filemane": "local:iso/virtio-win-0.1.185.iso",


Typo

Suggested change

"filemane": "local:iso/virtio-win-0.1.185.iso",

"filename": "local:iso/virtio-win-0.1.185.iso",

carlpett · 2020-08-09T07:53:03Z

website/pages/docs/builders/proxmox.mdx

+
+  - `bus_number` (string) - Number of the device. Can be 0 to 3 for `ide`, 0 to 5
+    for `sata` and 0 to 30 for `scsi`.
+    Defaults to 3 sice `ide` bus 2 is generally the boot drive.


Typo

Suggested change

Defaults to 3 sice `ide` bus 2 is generally the boot drive.

Defaults to 3 since `ide` bus 2 is generally the boot drive.

paginabianca · 2020-08-09T16:04:25Z

Thanks for the feedback and sorry for the typos. I'll fix those and the naming eather today or tomorrow.

I've looked into uploads of ISOs but I'm not sure where and how it would be best to implement that.
Do you have anything specific in mind? Otherwise I'll try to figure out a solution.

Fixed Template Finalization Error fmt Documentation and fixes Finalize Fix Description fix Typos Finalize Fix Prepared ISO upload feature Preparations for ISO upload More preparations Even more preparations Hopefully Final preparation removed

carlpett

This notification apparently drowned in a bunch of other email, sorry for taking so long! Nice work on the upload part, looks good 🎉 Ideally we could merge the main/additional iso uploads, but this works.

Other than that, one final naming nit and I think a minor bug, otherwise 👍

carlpett · 2020-08-26T19:18:04Z

builder/proxmox/step_upload_additional_isos.go

+	for idx := range c.AdditionalISOFiles {
+		if !c.AdditionalISOFiles[idx].shouldUploadISO {
+			state.Put("additional_iso_files", c.AdditionalISOFiles)
+			return multistep.ActionContinue


This should be a continue, right? Otherwise we'll skip uploading any iso:s later in the list

Yeah you're right. Thanks for the heads up 👍

carlpett · 2020-08-26T19:24:35Z

builder/proxmox/config.go

@@ -87,6 +90,15 @@ type vgaConfig struct {
 	Type   string `mapstructure:"type"`
 	Memory int    `mapstructure:"memory"`
 }
+type storageConfig struct {
+	common.ISOConfig `mapstructure:",squash"`
+	DeviceType       string `mapstructure:"device_type"`


I think I had a couple of comments around this last time round, which addressed the naming here in different ways and we got a bit of a mix of all now 😄 I think it's great that we no longer have the number as a separate field, but now it should probably be called device rather than device_type.

That makes sense. Will change it shortly.

carlpett

Nice! Just one small typo, apart from that LGTM!

builder/proxmox/config.go

fix typo Co-authored-by: Calle Pettersson <carlpett@users.noreply.github.com>

azr

Code LGTM, nice work @paginabianca. I applied your last suggestion @carlpett awesome reviewing 😄 , I'm also trusting your review as the owner of this builder on this and I think this can be merged. ( just waiting for the tests to be green )

🎉

paginabianca · 2020-08-31T09:46:22Z

Awesome 😄 thanks @carlpett for providing all the feedback.

rgevaert · 2020-08-31T10:37:55Z

Yes thank you 🥰🥰

carlpett · 2020-08-31T12:14:46Z

🎉 Nice work!

ghost · 2020-10-13T01:52:15Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

paginabianca requested a review from carlpett as a code owner July 27, 2020 09:36

paginabianca changed the title ~~[WIP] Allows for the mounting of ISOs when a Proxmox VM s created. Same as …~~ Allows for the mounting of ISOs when a Proxmox VM s created. Same as … Jul 28, 2020

paginabianca force-pushed the proxmox-add-devices branch from 6043c9d to 4fd76f3 Compare August 3, 2020 05:58

paginabianca changed the title ~~Allows for the mounting of ISOs when a Proxmox VM s created. Same as …~~ [WIP]Allows for the mounting of ISOs when a Proxmox VM s created. Same as … Aug 3, 2020

Added ability to unmount ISOs

6830648

Fixed Template Finalization Error fmt Documentation and fixes

carlpett reviewed Aug 9, 2020

View reviewed changes

paginabianca added 3 commits August 10, 2020 15:29

Added ability to unmount ISOs

ff5a77a

Fixed Template Finalization Error fmt Documentation and fixes Finalize Fix Description fix Typos Finalize Fix Prepared ISO upload feature Preparations for ISO upload More preparations Even more preparations Hopefully Final preparation removed

Added ability to upload ISO files

25de12b

Fmt and removed unneeded depencencies

478f7a8

paginabianca changed the title ~~[WIP]Allows for the mounting of ISOs when a Proxmox VM s created. Same as …~~ Allows for the mounting of ISOs when a Proxmox VM s created. Same as … Aug 13, 2020

carlpett reviewed Aug 26, 2020

View reviewed changes

Bugfix and Renaming

cb4224a

carlpett reviewed Aug 29, 2020

View reviewed changes

builder/proxmox/config.go Outdated Show resolved Hide resolved

Update builder/proxmox/config.go

3624746

fix typo Co-authored-by: Calle Pettersson <carlpett@users.noreply.github.com>

azr approved these changes Aug 31, 2020

View reviewed changes

azr merged commit e4f975f into hashicorp:master Aug 31, 2020

paginabianca deleted the proxmox-add-devices branch August 31, 2020 14:30

SwampDragons added builder/proxmox enhancement labels Sep 9, 2020

SwampDragons added this to the 1.6.3 milestone Sep 9, 2020

ghost locked as resolved and limited conversation to collaborators Oct 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allows for the mounting of ISOs when a Proxmox VM s created. Same as … #9653

Allows for the mounting of ISOs when a Proxmox VM s created. Same as … #9653

paginabianca commented Jul 27, 2020

hashicorp-cla commented Jul 27, 2020 •

edited

Loading

paginabianca commented Jul 27, 2020

codecov bot commented Jul 28, 2020 •

edited

Loading

SwampDragons commented Jul 31, 2020

carlpett left a comment

carlpett Aug 9, 2020

carlpett Aug 9, 2020

carlpett Aug 9, 2020

carlpett Aug 9, 2020

carlpett Aug 9, 2020

carlpett Aug 9, 2020

carlpett Aug 9, 2020

paginabianca commented Aug 9, 2020

carlpett left a comment

carlpett Aug 26, 2020

paginabianca Aug 27, 2020

carlpett Aug 26, 2020

paginabianca Aug 27, 2020

carlpett left a comment

azr left a comment •

edited

Loading

paginabianca commented Aug 31, 2020

rgevaert commented Aug 31, 2020 via email

carlpett commented Aug 31, 2020

ghost commented Oct 13, 2020

	- `additional_iso_files` (array of objects) - Additional ISO filess attached to the virtual machine.
	- `additional_iso_files` (array of objects) - Additional ISO files attached to the virtual machine.

	- `device` (string) - Bus type that the ISO will be mountet on. Can be `ide`,
	- `device` (string) - Bus type that the ISO will be mounted on. Can be `ide`,

	"filemane": "local:iso/virtio-win-0.1.185.iso",
	"filename": "local:iso/virtio-win-0.1.185.iso",

	Defaults to 3 sice `ide` bus 2 is generally the boot drive.
	Defaults to 3 since `ide` bus 2 is generally the boot drive.

Allows for the mounting of ISOs when a Proxmox VM s created. Same as … #9653

Allows for the mounting of ISOs when a Proxmox VM s created. Same as … #9653

Conversation

paginabianca commented Jul 27, 2020

hashicorp-cla commented Jul 27, 2020 • edited Loading

paginabianca commented Jul 27, 2020

codecov bot commented Jul 28, 2020 • edited Loading

Codecov Report

SwampDragons commented Jul 31, 2020

carlpett 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

paginabianca commented Aug 9, 2020

carlpett 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

carlpett left a comment

Choose a reason for hiding this comment

azr left a comment • edited Loading

Choose a reason for hiding this comment

paginabianca commented Aug 31, 2020

rgevaert commented Aug 31, 2020 via email

carlpett commented Aug 31, 2020

ghost commented Oct 13, 2020

hashicorp-cla commented Jul 27, 2020 •

edited

Loading

codecov bot commented Jul 28, 2020 •

edited

Loading

azr left a comment •

edited

Loading