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
14 changes: 9 additions & 5 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 @@ -671,12 +671,16 @@ In HCL2:

<!-- Code generated from the comments of the ISOsConfig struct in builder/proxmox/common/config.go; DO NOT EDIT MANUALLY -->

- `device` (string) - DEPRECATED. Assign bus type with `type`. Optionally assign a bus index with `index`.
Bus type and bus index that the ISO will be mounted on. Can be `ideX`,
`sataX` or `scsiX`.
For `ide` the bus index ranges from 0 to 3, for `sata` from 0 to 5 and for
`scsi` from 0 to 30.
Defaulted to `ide3` in versions up to v1.8, now defaults to dynamic assignment (next available bus index after hard disks are allocated)

- `type` (string) - Bus type that the ISO will be mounted on. Can be `ide`, `sata` or `scsi`. Defaults to `ide`.

In v1.9 bus indexes are no longer accepted for ISOs. ISOs are now attached to VMs in the order
they are configured, using free bus indexes after disks are attached.
Example: if two Disks and one ISO are defined as type `sata`, the disks will be attached to the VM
as `sata0`, `sata1`, and the ISO will be mapped to `sata2` (the next free device index)

- `index` (string) - Optional: Used in combination with `type` to statically assign an ISO to a bus index.

- `iso_file` (string) - Path to the ISO file to boot from, expressed as a
proxmox datastore path, for example
Expand Down
38 changes: 30 additions & 8 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 @@ -37,13 +37,13 @@ in the image's Cloud-Init settings for provisioning.

<!-- Code generated from the comments of the Config struct in builder/proxmox/iso/config.go; DO NOT EDIT MANUALLY -->

- `iso` (common.ISOsConfig) - Boot ISO attached to the virtual machine.
- `boot_iso` (common.ISOsConfig) - Boot ISO attached to the virtual machine.

JSON Example:

```json

"iso": {
"boot_iso": {
"type": "scsi",
"iso_file": "local:iso/debian-12.5.0-amd64-netinst.iso",
"unmount": true,
Expand All @@ -55,7 +55,7 @@ in the image's Cloud-Init settings for provisioning.

```hcl

iso {
boot_iso {
type = "scsi"
iso_file = "local:iso/debian-12.5.0-amd64-netinst.iso"
unmount = true
Expand Down Expand Up @@ -221,6 +221,24 @@ in the image's Cloud-Init settings for provisioning.

<!-- Code generated from the comments of the Config struct in builder/proxmox/iso/config.go; DO NOT EDIT MANUALLY -->

- `iso_file` (string) - DEPRECATED. Define Boot ISO config with the `boot_iso` block instead.
Path to the ISO file to boot from, expressed as a
proxmox datastore path, for example
`local:iso/Fedora-Server-dvd-x86_64-29-1.2.iso`.
Either `iso_file` OR `iso_url` must be specifed.

- `iso_storage_pool` (string) - DEPRECATED. Define Boot ISO config with the `boot_iso` block instead.
Proxmox storage pool onto which to upload
the ISO file.

- `iso_download_pve` (bool) - DEPRECATED. Define Boot ISO config with the `boot_iso` block instead.
Download the ISO directly from the PVE node rather than through Packer.

Defaults to `false`

- `unmount_iso` (bool) - DEPRECATED. Define Boot ISO config with the `boot_iso` block instead.
If true, remove the mounted ISO from the template
after finishing. Defaults to `false`.

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

Expand Down Expand Up @@ -331,12 +349,16 @@ HCL2 example:

<!-- Code generated from the comments of the ISOsConfig struct in builder/proxmox/common/config.go; DO NOT EDIT MANUALLY -->

- `device` (string) - DEPRECATED. Assign bus type with `type`. Optionally assign a bus index with `index`.
Bus type and bus index that the ISO will be mounted on. Can be `ideX`,
`sataX` or `scsiX`.
For `ide` the bus index ranges from 0 to 3, for `sata` from 0 to 5 and for
`scsi` from 0 to 30.
Defaulted to `ide3` in versions up to v1.8, now defaults to dynamic assignment (next available bus index after hard disks are allocated)

- `type` (string) - Bus type that the ISO will be mounted on. Can be `ide`, `sata` or `scsi`. Defaults to `ide`.

In v1.9 bus indexes are no longer accepted for ISOs. ISOs are now attached to VMs in the order
they are configured, using free bus indexes after disks are attached.
Example: if two Disks and one ISO are defined as type `sata`, the disks will be attached to the VM
as `sata0`, `sata1`, and the ISO will be mapped to `sata2` (the next free device index)

- `index` (string) - Optional: Used in combination with `type` to statically assign an ISO to a bus index.

- `iso_file` (string) - Path to the ISO file to boot from, expressed as a
proxmox datastore path, for example
Expand Down
60 changes: 55 additions & 5 deletions builder/proxmox/common/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,13 +237,17 @@ type Config struct {
// ```
type ISOsConfig struct {
commonsteps.ISOConfig `mapstructure:",squash"`
// DEPRECATED. Assign bus type with `type`. Optionally assign a bus index with `index`.
// Bus type and bus index that the ISO will be mounted on. Can be `ideX`,
// `sataX` or `scsiX`.
// For `ide` the bus index ranges from 0 to 3, for `sata` from 0 to 5 and for
// `scsi` from 0 to 30.
// Defaulted to `ide3` in versions up to v1.8, now defaults to dynamic assignment (next available bus index after hard disks are allocated)
Device string `mapstructure:"device"`
// Bus type that the ISO will be mounted on. Can be `ide`, `sata` or `scsi`. Defaults to `ide`.
//
// In v1.9 bus indexes are no longer accepted for ISOs. ISOs are now attached to VMs in the order
// they are configured, using free bus indexes after disks are attached.
// Example: if two Disks and one ISO are defined as type `sata`, the disks will be attached to the VM
// as `sata0`, `sata1`, and the ISO will be mapped to `sata2` (the next free device index)
Type string `mapstructure:"type"`
// Optional: Used in combination with `type` to statically assign an ISO to a bus index.
Index string `mapstructure:"index"`
// Path to the ISO file to boot from, expressed as a
// proxmox datastore path, for example
// `local:iso/Fedora-Server-dvd-x86_64-29-1.2.iso`.
Expand Down Expand Up @@ -682,6 +686,52 @@ func (c *Config) Prepare(upper interface{}, raws ...interface{}) ([]string, []st
}
c.ISOs[idx].ShouldUploadISO = true
}
// validate device field
if c.ISOs[idx].Device != "" {
warnings = append(warnings, "additional_iso_files field 'device' is deprecated and will be removed in a future release, assign bus type with 'type'. Optionally assign a bus index with 'index'")
if strings.HasPrefix(c.ISOs[idx].Device, "ide") {
busnumber, err := strconv.Atoi(c.ISOs[idx].Device[3:])
if err != nil {
errs = packersdk.MultiErrorAppend(errs, fmt.Errorf("%s is not a valid bus index", c.ISOs[idx].Device[3:]))
}
if busnumber > 3 {
errs = packersdk.MultiErrorAppend(errs, fmt.Errorf("IDE bus index can't be higher than 3"))
} else {
// convert device field to type and index fields
log.Printf("converting deprecated field 'device' value %s to 'type' %s and 'index' %d", c.ISOs[idx].Device, "ide", busnumber)
c.ISOs[idx].Type = "ide"
c.ISOs[idx].Index = strconv.Itoa(busnumber)
}
}
if strings.HasPrefix(c.ISOs[idx].Device, "sata") {
busnumber, err := strconv.Atoi(c.ISOs[idx].Device[4:])
if err != nil {
errs = packersdk.MultiErrorAppend(errs, fmt.Errorf("%s is not a valid bus index", c.ISOs[idx].Device[4:]))
}
if busnumber > 5 {
errs = packersdk.MultiErrorAppend(errs, fmt.Errorf("SATA bus index can't be higher than 5"))
} else {
// convert device field to type and index fields
log.Printf("converting deprecated field 'device' value %s to 'type' %s and 'index' %d", c.ISOs[idx].Device, "sata", busnumber)
c.ISOs[idx].Type = "sata"
c.ISOs[idx].Index = strconv.Itoa(busnumber)
}
}
if strings.HasPrefix(c.ISOs[idx].Device, "scsi") {
busnumber, err := strconv.Atoi(c.ISOs[idx].Device[4:])
if err != nil {
errs = packersdk.MultiErrorAppend(errs, fmt.Errorf("%s is not a valid bus index", c.ISOs[idx].Device[4:]))
}
if busnumber > 30 {
errs = packersdk.MultiErrorAppend(errs, fmt.Errorf("SCSI bus index can't be higher than 30"))
} else {
// convert device field to type and index fields
log.Printf("converting deprecated field 'device' value %s to 'type' %s and 'index' %d", c.ISOs[idx].Device, "scsi", busnumber)
c.ISOs[idx].Type = "scsi"
c.ISOs[idx].Index = strconv.Itoa(busnumber)
}
}
}
// validate device type, assign if unset
switch c.ISOs[idx].Type {
case "ide", "sata", "scsi":
Expand Down
4 changes: 4 additions & 0 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.

41 changes: 41 additions & 0 deletions builder/proxmox/common/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package proxmox

import (
"fmt"
"regexp"
"strings"
"testing"

Expand Down Expand Up @@ -244,7 +245,47 @@ func TestISOs(t *testing.T) {
}
})
}
}

func TestDeprecatedISOOptionsAreConverted(t *testing.T) {
isotests := []struct {
name string
ISOs map[string]interface{}
}{
{
// Ensure deprecated device field is converted
name: "device should be converted to type and index",
ISOs: map[string]interface{}{
"device": "ide3",
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

"iso_file": "local:iso/test.iso",
},
},
}
for _, c := range isotests {
t.Run(c.name, func(t *testing.T) {
cfg := mandatoryConfig(t)
cfg["additional_iso_files"] = c.ISOs

var config Config
_, _, err := config.Prepare(&config, cfg)
if err != nil {
t.Fatal(err)
}

rd := regexp.MustCompile(`\D+`)
bus := rd.FindString(config.ISOs[0].Device)

rb := regexp.MustCompile(`\d+`)
index := rb.FindString(config.ISOs[0].Device)

if config.ISOs[0].Type != bus {
t.Errorf("Expected device to be converted to type %s", bus)
}
if config.ISOs[0].Index != index {
t.Errorf("Expected device to be converted to index %s", index)
}
})
}
}

func TestRng0(t *testing.T) {
Expand Down
Loading