refactor: only add / remove minimul number of cdroms #355

Hi-Angel · 2024-01-15T14:17:58Z

In PR #344 a feature to reuse an existing VM is being added. As part of that feature I had to factor out from AddCdrom the logic of creating a CD-rom as opposed to just mounting an image. Recently merged feature reattach_cdroms turned out to be a new user of the older logic, which made rebasing #344 non-trivial.

In this PR I refactor the reattach_cdroms logic to avoid unnecessarily calling EjectCdroms, RemoveCdroms and AddCdrom by calculating amount of CDroms that we want to have added/removed, and only calling the mentioned functions for that amount (see the last commit).

This is a useful change in itself, disregarding whether #344 exists, so I'm sending it separately from the other PR. And it should simplify rebasing #344 later.

nywilken · 2024-01-16T22:27:25Z

@Hi-Angel thanks for opening up this quick refactor PR. I was planning to release the vSphere plugin with the latest changes.

Should I wait until this is merged? Or is the refactor for supporting #344

Hi-Angel · 2024-01-16T22:34:16Z

I don't have any preference here. On one hand, I presume the code that is already in the repo has probably seen more testing than the new one, so regressions are less likely. On the other hand, this refactor reduces a bit calls which (per my understanding) communicate to ESXi via network, which is slow. So if you decide to wait for it to land, the new functional might be a tiny bit faster.

nywilken · 2024-01-17T19:01:13Z

I don't have any preference here. On one hand, I presume the code that is already in the repo has probably seen more testing than the new one, so regressions are less likely. On the other hand, this refactor reduces a bit calls which (per my understanding) communicate to ESXi via network, which is slow. So if you decide to wait for it to land, the new functional might be a tiny bit faster.

@Hi-Angel that's a good call out on the testing part. We release every week, or shoot for it when we have pending changes. I think we can focus on getting this PR reviewed and merged for the next release to give the new feature time to setting.

Hi-Angel · 2024-02-01T18:29:29Z

Any news here?

tenthirtyam · 2024-02-01T19:33:30Z

Any news here?

I've rebased from main and have squashed the commits. I'll review this w/in the next week.

Hi-Angel · 2024-02-01T19:35:41Z

Any news here?

I've rebased from main and have squashed the commits. I'll review this w/in the next week.

Why have you squashed the commits before review? 😅 I mean, I know the project squashes them on merge for some reason, but don't you want for review purposes to see each functional change separated?

tenthirtyam · 2024-02-01T19:36:48Z

It's all good - I've read through them already.

nywilken

Hi @Hi-Angel I finally got a chance to get back to you on this. I was heads down on a few other Packer related changes. I left a few nits and a suggestion to your RemoveCdroms function change. The code looks good overall but I found a few things hard to wrap my head around.

Give the suggestions a look over and let me know if you have any questions.

builder/vsphere/common/step_reattach_cdrom.go

builder/vsphere/driver/vm_cdrom.go

The `range` body is skipped anyway when len(ISOPaths) == 0.

This simplifies the code a bit, and it will also be useful later when we add support for reusing a VM

Hi-Angel · 2024-02-13T07:35:30Z

Everything seems addressed

nywilken

@Hi-Angel then updated changes look good to me. After your explanation dropping Virtual from the function name makes senses. Thanks for responding so quickly to the review.

@tenthirtyam would you like to give this another set of eyes before I merge?

tenthirtyam · 2024-02-13T16:36:44Z

@tenthirtyam would you like to give this another set of eyes before I merge?

Will review and test as well and update you.

tenthirtyam · 2024-02-13T16:39:37Z

builder/vsphere/common/step_add_cdrom.go

+	// Bug in govmomi: can't batch-create cdroms and then mount ISO files, that
+	// resuls in wrong UnitNumber. So do that one-by-one.


If calling out a bug (or is it actually a limitation) in vmware/govmomi, it would be ideal to include a reference so that this can be reviewed in the future.

Well… I currently don't have resources for creating a minimal testcase for this, so let's just rename this to a "limitation" if that's okay with you

Renamed per comment above

tenthirtyam · 2024-02-13T16:40:01Z

builder/vsphere/common/step_add_cdrom.go

+	// resuls in wrong UnitNumber. So do that one-by-one.
+	for _, path := range s.Config.ISOPaths {
+		if path == "" {
+			state.Put("error", fmt.Errorf("Invalid path: empty string"))


Suggested change

state.Put("error", fmt.Errorf("Invalid path: empty string"))

state.Put("error", fmt.Errorf("invalid path: empty string"))

tenthirtyam · 2024-02-13T16:41:23Z

builder/vsphere/common/step_reattach_cdrom.go

-				state.Put("error", fmt.Errorf("error adding sata controller: %v", err))
-				return multistep.ActionHalt
-			}
+	nActableCdroms := ReattachCDRom - len(s.CDRomConfig.ISOPaths)


nAttachableCdroms

tenthirtyam · 2024-02-13T16:41:30Z

builder/vsphere/common/step_reattach_cdrom.go

-				return multistep.ActionHalt
-			}
+	nActableCdroms := ReattachCDRom - len(s.CDRomConfig.ISOPaths)
+	if nActableCdroms < 0 {


nAttachableCdroms

tenthirtyam · 2024-02-13T16:42:39Z

builder/vsphere/common/step_reattach_cdrom.go

-	// Add the CD-ROM devices to the image based on the value of `reattach_cdroms`.
-	// A valid ISO path is required for this step. The media will subsequently be ejected.


Isn't this still useful information?

Hmm, well, I suppose I can put it before nAttachableCdroms calculation… I initially removed it because the whole point of the class and its method is to process reattach_cdroms option. I.e. if you are inside this function, then you already know what this code supposed to do.

But I suppose the comment wouldn't hurt either, so I put it back.

tenthirtyam · 2024-02-13T16:43:05Z

builder/vsphere/common/step_reattach_cdrom.go

+			return multistep.ActionHalt
+		}
+	} else {
+		// Eject media from pre-existing CD-ROM devices.


Suggested change

// Eject media from pre-existing CD-ROM devices.

// Eject media from the existing CD-ROM devices.

tenthirtyam · 2024-02-13T16:43:52Z

builder/vsphere/common/step_reattach_cdrom.go

-	if err != nil {
-		state.Put("error", fmt.Errorf("error ejecting cdrom media: %v", err))
-		return multistep.ActionHalt
+	// create more CD-ROMs if required


Suggested change

// create more CD-ROMs if required

// Add CD-ROMs, if required.

tenthirtyam · 2024-02-13T16:44:12Z

builder/vsphere/common/step_reattach_cdrom.go

-		state.Put("error", fmt.Errorf("error ejecting cdrom media: %v", err))
-		return multistep.ActionHalt
+	// create more CD-ROMs if required
+	if nActableCdroms > 0 {


nAttachableCdroms

tenthirtyam · 2024-02-13T16:44:32Z

builder/vsphere/common/step_reattach_cdrom.go

+		}
+
+		ui.Say("Adding CD-ROM devices...")
+		for i := 0; i < nActableCdroms; i++ {


nAttachableCdroms

builder/vsphere/common/step_reattach_cdrom_test.go

builder/vsphere/driver/vm_cdrom.go

tenthirtyam

Minor changes requested.

This commit changes the "reattach" logic so that instead of removing all CDroms and recreating everything anew, we only remove the number of CDroms that are excess, and only add new CDroms when there was not enough pre-existing ones.

I'm dismissing in order to merge. All requested changes have been made

nywilken · 2024-02-21T18:26:16Z

Hi Folks, apologies I've been out sick. Given that @Hi-Angel has addressed all of your request @tenthirtyam I dismissed your last review in order to unblock the merge and pending rebase. We can revisit anything if needed but this looks good to go.

This fixes regression introduced by the commit "common: deduplicate AddCdrom call by appending path to ISOPaths" that was squashed into: "refactor: only add / remove minimul number of cdroms (hashicorp#355)" It assumed that the order is not guaranteed and does not matter, however the documentation says: > If the "iso_url" is defined in addition to the "iso_paths", the > "iso_url" is added to the VM first. This keeps the "iso_url" first > in the boot order by default allowing the boot iso being defined by > the iso_url and the vmware tools iso added from the datastore. So fix the regression. Fixes: hashicorp#386

This fixes regression introduced by the commit "common: deduplicate AddCdrom call by appending path to ISOPaths" that was squashed into: "refactor: only add / remove minimul number of cdroms (hashicorp#355)" It assumed that the order is not guaranteed and does not matter, however the documentation says: > If the "iso_url" is defined in addition to the "iso_paths", the > "iso_url" is added to the VM first. This keeps the "iso_url" first > in the boot order by default allowing the boot iso being defined by > the iso_url and the vmware tools iso added from the datastore. So fix the regression. Fixes: hashicorp#386 Signed-off-by: Konstantin Kharlamov <Hi-Angel@yandex.ru>

This fixes regression introduced by the commit "common: deduplicate AddCdrom call by appending path to ISOPaths" that was squashed into: "refactor: only add / remove minimul number of cdroms (#355)" It assumed that the order is not guaranteed and does not matter, however the documentation says: > If the "iso_url" is defined in addition to the "iso_paths", the > "iso_url" is added to the VM first. This keeps the "iso_url" first > in the boot order by default allowing the boot iso being defined by > the iso_url and the vmware tools iso added from the datastore. So fix the regression. Fixes: #386 Signed-off-by: Konstantin Kharlamov <Hi-Angel@yandex.ru>

Hi-Angel requested a review from a team as a code owner January 15, 2024 14:17

Hi-Angel force-pushed the separate-cdrom-creation-n-mount-have-old-AddCdrom branch 2 times, most recently from dedd2d5 to 017f49a Compare January 15, 2024 14:24

tenthirtyam force-pushed the separate-cdrom-creation-n-mount-have-old-AddCdrom branch 2 times, most recently from 8eff8a9 to 19cfa45 Compare February 1, 2024 19:32

tenthirtyam self-requested a review February 1, 2024 19:33

tenthirtyam assigned Hi-Angel Feb 1, 2024

tenthirtyam added the chore Chore label Feb 1, 2024

tenthirtyam changed the title ~~Refactor reattach_cdroms to only remove/add amount of CDroms that is necessary~~ refactor: only add / remove minimul number of cdroms Feb 1, 2024

nywilken requested changes Feb 12, 2024

View reviewed changes

nywilken mentioned this pull request Feb 12, 2024

🚧 feat: add support for reusing a vm #344

Draft

2 tasks

tenthirtyam marked this pull request as draft February 12, 2024 21:56

Hi-Angel added 3 commits February 13, 2024 09:06

common: remove excess check for ISOPaths greater than 0

b053fd2

The `range` body is skipped anyway when len(ISOPaths) == 0.

common: deduplicate AddCdrom call by appending path to ISOPaths

27d5a49

This simplifies the code a bit, and it will also be useful later when we add support for reusing a VM

common: improve explanation on finding SATA controller

97ed309

Hi-Angel force-pushed the separate-cdrom-creation-n-mount-have-old-AddCdrom branch from 19cfa45 to 05e1a98 Compare February 13, 2024 07:32

Hi-Angel marked this pull request as ready for review February 13, 2024 08:09

nywilken approved these changes Feb 13, 2024

View reviewed changes

tenthirtyam reviewed Feb 13, 2024

View reviewed changes

builder/vsphere/common/step_reattach_cdrom_test.go Outdated Show resolved Hide resolved

tenthirtyam reviewed Feb 13, 2024

View reviewed changes

builder/vsphere/common/step_reattach_cdrom_test.go Outdated Show resolved Hide resolved

tenthirtyam reviewed Feb 13, 2024

View reviewed changes

builder/vsphere/common/step_reattach_cdrom_test.go Outdated Show resolved Hide resolved

tenthirtyam reviewed Feb 13, 2024

View reviewed changes

builder/vsphere/driver/vm_cdrom.go Outdated Show resolved Hide resolved

tenthirtyam previously requested changes Feb 13, 2024

View reviewed changes

iso: do not create CDroms with ISOs unless necessary

860f710

This commit changes the "reattach" logic so that instead of removing all CDroms and recreating everything anew, we only remove the number of CDroms that are excess, and only add new CDroms when there was not enough pre-existing ones.

Hi-Angel force-pushed the separate-cdrom-creation-n-mount-have-old-AddCdrom branch from 05e1a98 to 860f710 Compare February 14, 2024 06:48

nywilken requested a review from tenthirtyam February 21, 2024 18:23

nywilken merged commit 6c6667c into hashicorp:main Feb 21, 2024
12 checks passed

tenthirtyam mentioned this pull request Mar 18, 2024

1.2.5 No longer builds Windows 2019 & 2022 #386

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: only add / remove minimul number of cdroms #355

refactor: only add / remove minimul number of cdroms #355

Hi-Angel commented Jan 15, 2024

nywilken commented Jan 16, 2024

Hi-Angel commented Jan 16, 2024

nywilken commented Jan 17, 2024

Hi-Angel commented Feb 1, 2024

tenthirtyam commented Feb 1, 2024

Hi-Angel commented Feb 1, 2024 •

edited

tenthirtyam commented Feb 1, 2024

nywilken left a comment

Hi-Angel commented Feb 13, 2024

nywilken left a comment

tenthirtyam commented Feb 13, 2024

tenthirtyam Feb 13, 2024

Hi-Angel Feb 14, 2024

Hi-Angel Feb 14, 2024

tenthirtyam Feb 13, 2024

Hi-Angel Feb 14, 2024

tenthirtyam Feb 13, 2024

Hi-Angel Feb 14, 2024

tenthirtyam Feb 13, 2024

Hi-Angel Feb 14, 2024

tenthirtyam Feb 13, 2024

Hi-Angel Feb 14, 2024 •

edited

tenthirtyam Feb 13, 2024

Hi-Angel Feb 14, 2024

tenthirtyam Feb 13, 2024

Hi-Angel Feb 14, 2024

tenthirtyam Feb 13, 2024

Hi-Angel Feb 14, 2024

tenthirtyam Feb 13, 2024

Hi-Angel Feb 14, 2024

tenthirtyam left a comment

nywilken commented Feb 21, 2024

		// Bug in govmomi: can't batch-create cdroms and then mount ISO files, that
		// resuls in wrong UnitNumber. So do that one-by-one.

	state.Put("error", fmt.Errorf("Invalid path: empty string"))
	state.Put("error", fmt.Errorf("invalid path: empty string"))

		// Add the CD-ROM devices to the image based on the value of `reattach_cdroms`.
		// A valid ISO path is required for this step. The media will subsequently be ejected.

	// Eject media from pre-existing CD-ROM devices.
	// Eject media from the existing CD-ROM devices.

	// create more CD-ROMs if required
	// Add CD-ROMs, if required.

refactor: only add / remove minimul number of cdroms #355

refactor: only add / remove minimul number of cdroms #355

Conversation

Hi-Angel commented Jan 15, 2024

nywilken commented Jan 16, 2024

Hi-Angel commented Jan 16, 2024

nywilken commented Jan 17, 2024

Hi-Angel commented Feb 1, 2024

tenthirtyam commented Feb 1, 2024

Hi-Angel commented Feb 1, 2024 • edited

tenthirtyam commented Feb 1, 2024

nywilken left a comment

Choose a reason for hiding this comment

Hi-Angel commented Feb 13, 2024

nywilken left a comment

Choose a reason for hiding this comment

tenthirtyam commented Feb 13, 2024

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

Choose a reason for hiding this comment

Hi-Angel Feb 14, 2024 • edited

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

tenthirtyam left a comment

Choose a reason for hiding this comment

nywilken commented Feb 21, 2024

Hi-Angel commented Feb 1, 2024 •

edited

Hi-Angel Feb 14, 2024 •

edited