From f60aa6971b80373e93b547532123b2106750b394 Mon Sep 17 00:00:00 2001 From: Sebastian Fleer Date: Wed, 25 Jan 2023 23:32:10 +0100 Subject: [PATCH 1/2] clone: Add support to clone VM by ID --- builder/proxmox/clone/builder.go | 29 ++++++++---- builder/proxmox/clone/config.go | 14 +++++- builder/proxmox/clone/config.hcl2spec.go | 2 + builder/proxmox/clone/config_test.go | 59 +++++++++++++++++++++++- docs/builders/clone.mdx | 5 ++ 5 files changed, 96 insertions(+), 13 deletions(-) diff --git a/builder/proxmox/clone/builder.go b/builder/proxmox/clone/builder.go index 0a937ebc..16214cd8 100644 --- a/builder/proxmox/clone/builder.go +++ b/builder/proxmox/clone/builder.go @@ -69,20 +69,29 @@ func (*cloneVMCreator) Create(vmRef *proxmoxapi.VmRef, config proxmoxapi.ConfigQ } config.Ipconfig = IpconfigMap - sourceVmrs, err := client.GetVmRefsByName(c.CloneVM) - if err != nil { - return err - } + var sourceVmr *proxmoxapi.VmRef + if c.CloneVM != "" { + sourceVmrs, err := client.GetVmRefsByName(c.CloneVM) + if err != nil { + return err + } - // prefer source Vm located on same node - sourceVmr := sourceVmrs[0] - for _, candVmr := range sourceVmrs { - if candVmr.Node() == vmRef.Node() { - sourceVmr = candVmr + // prefer source Vm located on same node + sourceVmr = sourceVmrs[0] + for _, candVmr := range sourceVmrs { + if candVmr.Node() == vmRef.Node() { + sourceVmr = candVmr + } + } + } else if c.CloneVMID != 0 { + sourceVmr = proxmoxapi.NewVmRef(c.CloneVMID) + err := client.CheckVmRef(sourceVmr) + if err != nil { + return err } } - err = config.CloneVm(sourceVmr, vmRef, client) + err := config.CloneVm(sourceVmr, vmRef, client) if err != nil { return err } diff --git a/builder/proxmox/clone/config.go b/builder/proxmox/clone/config.go index 0bc1d695..2fcd52fb 100644 --- a/builder/proxmox/clone/config.go +++ b/builder/proxmox/clone/config.go @@ -18,6 +18,7 @@ type Config struct { proxmoxcommon.Config `mapstructure:",squash"` CloneVM string `mapstructure:"clone_vm" required:"true"` + CloneVMID int `mapstructure:"clone_vm_id" required:"true"` FullClone config.Trilean `mapstructure:"full_clone" required:"false"` Nameserver string `mapstructure:"nameserver" required:"false"` @@ -39,8 +40,17 @@ func (c *Config) Prepare(raws ...interface{}) ([]string, []string, error) { errs = packersdk.MultiErrorAppend(errs, merrs) } - if c.CloneVM == "" { - errs = packersdk.MultiErrorAppend(errs, errors.New("clone_vm must be specified")) + if c.CloneVM == "" && c.CloneVMID == 0 { + errs = packersdk.MultiErrorAppend(errs, errors.New("one of clone_vm or clone_vm_id must be specified")) + } + if c.CloneVM != "" && c.CloneVMID != 0 { + errs = packersdk.MultiErrorAppend(errs, errors.New("clone_vm and clone_vm_id cannot both be specified")) + } + // Technically Proxmox VMIDs are unsigned 32bit integers, but are limited to + // the range 100-999999999. Source: + // https://pve-devel.pve.proxmox.narkive.com/Pa6mH1OP/avoiding-vmid-reuse#post8 + if c.CloneVMID != 0 && (c.CloneVMID < 100 || c.CloneVMID > 999999999) { + errs = packersdk.MultiErrorAppend(errs, errors.New("clone_vm_id must be in range 100-999999999")) } // Check validity of given IP addresses diff --git a/builder/proxmox/clone/config.hcl2spec.go b/builder/proxmox/clone/config.hcl2spec.go index 194720ce..9c8c8c16 100644 --- a/builder/proxmox/clone/config.hcl2spec.go +++ b/builder/proxmox/clone/config.hcl2spec.go @@ -113,6 +113,7 @@ type FlatConfig struct { AdditionalISOFiles []proxmox.FlatadditionalISOsConfig `mapstructure:"additional_iso_files" cty:"additional_iso_files" hcl:"additional_iso_files"` VMInterface *string `mapstructure:"vm_interface" cty:"vm_interface" hcl:"vm_interface"` CloneVM *string `mapstructure:"clone_vm" required:"true" cty:"clone_vm" hcl:"clone_vm"` + CloneVMID *int `mapstructure:"clone_vm_id" required:"true" cty:"clone_vm_id" hcl:"clone_vm_id"` FullClone *bool `mapstructure:"full_clone" required:"false" cty:"full_clone" hcl:"full_clone"` Nameserver *string `mapstructure:"nameserver" required:"false" cty:"nameserver" hcl:"nameserver"` Searchdomain *string `mapstructure:"searchdomain" required:"false" cty:"searchdomain" hcl:"searchdomain"` @@ -233,6 +234,7 @@ func (*FlatConfig) HCL2Spec() map[string]hcldec.Spec { "additional_iso_files": &hcldec.BlockListSpec{TypeName: "additional_iso_files", Nested: hcldec.ObjectSpec((*proxmox.FlatadditionalISOsConfig)(nil).HCL2Spec())}, "vm_interface": &hcldec.AttrSpec{Name: "vm_interface", Type: cty.String, Required: false}, "clone_vm": &hcldec.AttrSpec{Name: "clone_vm", Type: cty.String, Required: false}, + "clone_vm_id": &hcldec.AttrSpec{Name: "clone_vm_id", Type: cty.Number, Required: false}, "full_clone": &hcldec.AttrSpec{Name: "full_clone", Type: cty.Bool, Required: false}, "nameserver": &hcldec.AttrSpec{Name: "nameserver", Type: cty.String, Required: false}, "searchdomain": &hcldec.AttrSpec{Name: "searchdomain", Type: cty.String, Required: false}, diff --git a/builder/proxmox/clone/config_test.go b/builder/proxmox/clone/config_test.go index d0b086df..e4037840 100644 --- a/builder/proxmox/clone/config_test.go +++ b/builder/proxmox/clone/config_test.go @@ -30,7 +30,7 @@ func TestRequiredParameters(t *testing.T) { t.Fatal("Expected errors to be packersdk.MultiError") } - required := []string{"username", "token", "proxmox_url", "node", "ssh_username", "clone_vm"} + required := []string{"username", "token", "proxmox_url", "node", "ssh_username", "clone_vm", "clone_vm_id"} for _, param := range required { found := false for _, err := range errs.Errors { @@ -45,6 +45,63 @@ func TestRequiredParameters(t *testing.T) { } } +func TestVMNameOrID(t *testing.T) { + ipconfigTest := []struct { + name string + cloneVM string + cloneVMID int + expectFailure bool + }{ + { + name: "clone_vm given, no error", + cloneVM: "myVM", + cloneVMID: 0, + expectFailure: false, + }, + { + name: "valid clone_vm_id given, no error", + cloneVM: "", + cloneVMID: 100, + expectFailure: false, + }, + { + name: "clone_vm_id out of range, error", + cloneVM: "", + cloneVMID: 50, + expectFailure: true, + }, + { + name: "clone_vm and clone_vm_id given, error", + cloneVM: "myVM", + cloneVMID: 100, + expectFailure: true, + }, + { + name: "neither clone_vm nor clone_vm_id given, error", + cloneVM: "", + cloneVMID: 0, + expectFailure: true, + }, + } + + for _, tt := range ipconfigTest { + t.Run(tt.name, func(t *testing.T) { + cfg := mandatoryConfig(t) + cfg["clone_vm"] = tt.cloneVM + cfg["clone_vm_id"] = tt.cloneVMID + + var c Config + _, _, err := c.Prepare(&c, cfg) + if err != nil && !tt.expectFailure { + t.Fatalf("unexpected failure: %s", err) + } + if err == nil && tt.expectFailure { + t.Errorf("expected failure, but prepare succeeded") + } + }) + } +} + func TestNameserver(t *testing.T) { ipconfigTest := []struct { name string diff --git a/docs/builders/clone.mdx b/docs/builders/clone.mdx index 46080017..8f269653 100644 --- a/docs/builders/clone.mdx +++ b/docs/builders/clone.mdx @@ -68,6 +68,11 @@ in the image's Cloud-Init settings for provisioning. machine on during creation. - `clone_vm` (string) - The name of the VM Packer should clone and build from. + Either `clone_vm` or `clone_vm_id` must be specifed. + +- `clone_vm_id` (int) - The ID of the VM Packer should clone and build from. + Proxmox VMIDs are limited to the range 100-999999999. + Either `clone_vm` or `clone_vm_id` must be specifed. ### Optional: From 7dd1189d51e9e7b9e817362d19ab3f5938dc5c45 Mon Sep 17 00:00:00 2001 From: Sebastian Fleer Date: Sun, 12 Feb 2023 11:36:58 +0100 Subject: [PATCH 2/2] Check if vm_id is in valid range. Technically Proxmox VMIDs are unsigned 32bit integers, but are limited to the range 100-999999999. Source: https://pve-devel.pve.proxmox.narkive.com/Pa6mH1OP/avoiding-vmid-reuse#post8 Check if vm_id is in that range. --- builder/proxmox/common/config.go | 6 ++++ builder/proxmox/common/config_test.go | 49 +++++++++++++++++++++++++++ docs/builders/clone.mdx | 5 +-- docs/builders/iso.mdx | 5 +-- 4 files changed, 61 insertions(+), 4 deletions(-) diff --git a/builder/proxmox/common/config.go b/builder/proxmox/common/config.go index a7c0fe5b..f5abbbc6 100644 --- a/builder/proxmox/common/config.go +++ b/builder/proxmox/common/config.go @@ -169,6 +169,12 @@ func (c *Config) Prepare(upper interface{}, raws ...interface{}) ([]string, []st c.BootKeyInterval = 5 * time.Millisecond } + // Technically Proxmox VMIDs are unsigned 32bit integers, but are limited to + // the range 100-999999999. Source: + // https://pve-devel.pve.proxmox.narkive.com/Pa6mH1OP/avoiding-vmid-reuse#post8 + if c.VMID != 0 && (c.VMID < 100 || c.VMID > 999999999) { + errs = packersdk.MultiErrorAppend(errs, errors.New("vm_id must be in range 100-999999999")) + } if c.VMName == "" { // Default to packer-[time-ordered-uuid] c.VMName = fmt.Sprintf("packer-%s", uuid.TimeOrderedUUID()) diff --git a/builder/proxmox/common/config_test.go b/builder/proxmox/common/config_test.go index d88c09ee..5fad9213 100644 --- a/builder/proxmox/common/config_test.go +++ b/builder/proxmox/common/config_test.go @@ -286,3 +286,52 @@ func TestSerials(t *testing.T) { }) } } + +func TestVMID(t *testing.T) { + serialsTest := []struct { + name string + VMID int + expectFailure bool + }{ + { + name: "VMID zero, no error", + expectFailure: false, + VMID: 0, + }, + { + name: "VMID in range, no error", + expectFailure: false, + VMID: 1000, + }, + { + name: "VMID above range, fail", + expectFailure: true, + VMID: 1000000000, + }, + { + name: "VMID below range, fail", + expectFailure: true, + VMID: 50, + }, + } + + for _, tt := range serialsTest { + t.Run(tt.name, func(t *testing.T) { + cfg := mandatoryConfig(t) + cfg["vm_id"] = tt.VMID + + var c Config + _, _, err := c.Prepare(&c, cfg) + if err != nil { + if !tt.expectFailure { + t.Fatalf("unexpected failure to prepare config: %s", err) + } + t.Logf("got expected failure: %s", err) + } + + if err == nil && tt.expectFailure { + t.Errorf("expected failure, but prepare succeeded") + } + }) + } +} diff --git a/docs/builders/clone.mdx b/docs/builders/clone.mdx index 8f269653..0aa1bf5c 100644 --- a/docs/builders/clone.mdx +++ b/docs/builders/clone.mdx @@ -87,8 +87,9 @@ in the image's Cloud-Init settings for provisioning. given, a random uuid will be used. - `vm_id` (int) - The ID used to reference the virtual machine. This will - also be the ID of the final template. If not given, the next free ID on - the node will be used. + also be the ID of the final template. Proxmox VMIDs are unique cluster-wide + and are limited to the range 100-999999999. + If not given, the next free ID on the cluster will be used. - `memory` (int) - How much memory, in megabytes, to give the virtual machine. Defaults to `512`. diff --git a/docs/builders/iso.mdx b/docs/builders/iso.mdx index 688992aa..dafbf52c 100644 --- a/docs/builders/iso.mdx +++ b/docs/builders/iso.mdx @@ -98,8 +98,9 @@ in the image's Cloud-Init settings for provisioning. given, a random uuid will be used. - `vm_id` (int) - The ID used to reference the virtual machine. This will - also be the ID of the final template. If not given, the next free ID on - the node will be used. + also be the ID of the final template. Proxmox VMIDs are unique cluster-wide + and are limited to the range 100-999999999. + If not given, the next free ID on the cluster will be used. - `memory` (int) - How much memory, in megabytes, to give the virtual machine. Defaults to `512`.