Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 13 commits into from
Sep 26, 2024
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
13a0b51
common: add disk config asyncio setting
mpywell May 29, 2024
6fdbe8a
common: change iso unmount device default behaviour
mpywell May 29, 2024
a78726c
iso: add iso_device config option
mpywell May 31, 2024
064a8b5
fix: replace ide2 static references in finalize_iso
mpywell Jun 1, 2024
7b4fd64
fix: code quality updates
mpywell Jun 4, 2024
99efad8
consolidate iso device configuration
mpywell Jun 12, 2024
a52fd9c
iso: update basic example documentation with iso block
mpywell Jun 13, 2024
6b51d75
iso: reintroduce iso config struct for boot iso
mpywell Jun 20, 2024
e635f0e
common: update documentation of ISOsConfig Type field
mpywell Jun 20, 2024
8370873
common: restore default disk backup behaviour
mpywell Jul 15, 2024
80edb15
add backwards compatibility support for ISOs
mpywell Sep 18, 2024
b51baca
Improve tests, update doc
mpywell Sep 24, 2024
bfa19d5
test all buses for overallocation, cleanup test bool conditions
mpywell Sep 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .web-docs/components/builder/clone/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -676,6 +676,9 @@ In HCL2:

- `unmount` (bool) - If true, remove the mounted ISO from the template after finishing. Defaults to `false`.

- `unmount_keep_device` (bool) - Keep CDRom device attached to template if unmounting ISO. Defaults to `false`.
Has no effect if unmount is `false`

<!-- End of code generated from the comments of the additionalISOsConfig struct in builder/proxmox/common/config.go; -->


Expand Down
6 changes: 6 additions & 0 deletions .web-docs/components/builder/iso/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,9 @@ in the image's Cloud-Init settings for provisioning.
- `unmount_iso` (bool) - If true, remove the mounted ISO from the template
after finishing. Defaults to `false`.

- `unmount_keep_device` (bool) - Keep CDRom device attached to template if unmounting ISO. Defaults to `false`.
Has no effect if unmount is `false`

<!-- End of code generated from the comments of the Config struct in builder/proxmox/iso/config.go; -->


Expand Down Expand Up @@ -430,6 +433,9 @@ In HCL2:

- `unmount` (bool) - If true, remove the mounted ISO from the template after finishing. Defaults to `false`.

- `unmount_keep_device` (bool) - Keep CDRom device attached to template if unmounting ISO. Defaults to `false`.
Has no effect if unmount is `false`

<!-- End of code generated from the comments of the additionalISOsConfig struct in builder/proxmox/common/config.go; -->


Expand Down
5 changes: 4 additions & 1 deletion builder/proxmox/common/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,10 @@ type additionalISOsConfig struct {
// Defaults to `false`
ISODownloadPVE bool `mapstructure:"iso_download_pve"`
// If true, remove the mounted ISO from the template after finishing. Defaults to `false`.
Unmount bool `mapstructure:"unmount"`
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"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"`

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

ShouldUploadISO bool `mapstructure-to-hcl2:",skip"`
DownloadPathKey string `mapstructure-to-hcl2:",skip"`
commonsteps.CDConfig `mapstructure:",squash"`
Expand Down
28 changes: 15 additions & 13 deletions builder/proxmox/common/config.hcl2spec.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 9 additions & 4 deletions builder/proxmox/common/step_finalize_template_config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ func (s *stepFinalizeTemplateConfig) Run(ctx context.Context, state multistep.St
}
}

deleteItems := []string{}
if len(c.AdditionalISOFiles) > 0 {
for idx := range c.AdditionalISOFiles {
cdrom := c.AdditionalISOFiles[idx].Device
Expand All @@ -115,7 +116,11 @@ func (s *stepFinalizeTemplateConfig) Run(ctx context.Context, state multistep.St
ui.Error(err.Error())
return multistep.ActionHalt
}
changes[cdrom] = "none,media=cdrom"
if c.AdditionalISOFiles[idx].UnmountKeepDevice {
changes[cdrom] = "none,media=cdrom"
} else {
deleteItems = append(deleteItems, cdrom)
}
} else {
changes[cdrom] = c.AdditionalISOFiles[idx].ISOFile + ",media=cdrom"
}
Expand All @@ -125,13 +130,13 @@ func (s *stepFinalizeTemplateConfig) Run(ctx context.Context, state multistep.St
// Disks that get replaced by the builder end up as unused disks -
// find and remove them.
rxUnused := regexp.MustCompile(`^unused\d+`)
unusedDisks := []string{}
for key := range vmParams {
if unusedDisk := rxUnused.FindString(key); unusedDisk != "" {
unusedDisks = append(unusedDisks, unusedDisk)
deleteItems = append(deleteItems, unusedDisk)
}
}
changes["delete"] = strings.Join(unusedDisks, ",")

changes["delete"] = strings.Join(deleteItems, ",")

if len(changes) > 0 {
_, err := client.SetVmConfig(vmRef, changes)
Expand Down
7 changes: 5 additions & 2 deletions builder/proxmox/iso/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,11 @@ type Config struct {
ISODownloadPVE bool `mapstructure:"iso_download_pve"`
// If true, remove the mounted ISO from the template
// after finishing. Defaults to `false`.
UnmountISO bool `mapstructure:"unmount_iso"`
shouldUploadISO bool
UnmountISO bool `mapstructure:"unmount_iso"`
// 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"`
lbajolet-hashicorp marked this conversation as resolved.
Show resolved Hide resolved
shouldUploadISO bool
}

func (c *Config) Prepare(raws ...interface{}) ([]string, []string, error) {
Expand Down
2 changes: 2 additions & 0 deletions builder/proxmox/iso/config.hcl2spec.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 5 additions & 1 deletion builder/proxmox/iso/step_finalize_iso.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,11 @@ func (s *stepFinalizeISOTemplate) Run(ctx context.Context, state multistep.State
ui.Error(err.Error())
return multistep.ActionHalt
}
changes["ide2"] = "none,media=cdrom"
if c.UnmountKeepDevice {
changes["ide2"] = "none,media=cdrom"
} else {
changes["delete"] = "ide2"
}
}

if len(changes) > 0 {
Expand Down
15 changes: 15 additions & 0 deletions builder/proxmox/iso/step_finalize_iso_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,21 @@ func TestISOTemplateFinalize(t *testing.T) {
"ide2": "local:iso/Fedora-Server-dvd-x86_64-29-1.2.iso,media=cdrom",
},
expectCallSetConfig: true,
expectedVMConfig: map[string]interface{}{
"ide2": nil,
},
expectedAction: multistep.ActionContinue,
},
{
name: "should keep iso device and unmount ISO when configured",
builderConfig: &Config{
UnmountISO: true,
UnmountKeepDevice: true,
},
initialVMConfig: map[string]interface{}{
"ide2": "local:iso/Fedora-Server-dvd-x86_64-29-1.2.iso,media=cdrom",
},
expectCallSetConfig: true,
expectedVMConfig: map[string]interface{}{
"ide2": "none,media=cdrom",
},
Expand Down
3 changes: 3 additions & 0 deletions docs-partials/builder/proxmox/common/additionalISOsConfig-not-required.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,7 @@

- `unmount` (bool) - If true, remove the mounted ISO from the template after finishing. Defaults to `false`.

- `unmount_keep_device` (bool) - Keep CDRom device attached to template if unmounting ISO. Defaults to `false`.
Has no effect if unmount is `false`

<!-- End of code generated from the comments of the additionalISOsConfig struct in builder/proxmox/common/config.go; -->
3 changes: 3 additions & 0 deletions docs-partials/builder/proxmox/iso/Config-not-required.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,7 @@
- `unmount_iso` (bool) - If true, remove the mounted ISO from the template
after finishing. Defaults to `false`.

- `unmount_keep_device` (bool) - Keep CDRom device attached to template if unmounting ISO. Defaults to `false`.
Has no effect if unmount is `false`

<!-- End of code generated from the comments of the Config struct in builder/proxmox/iso/config.go; -->