From 568c55f29f90c208c28a822ae68b0b46db703770 Mon Sep 17 00:00:00 2001 From: Martin Pywell Date: Sat, 28 Sep 2024 12:24:11 +0000 Subject: [PATCH 1/4] common: add skip_convert_to_template option --- .web-docs/components/builder/clone/README.md | 3 ++ .web-docs/components/builder/iso/README.md | 3 ++ builder/proxmox/clone/config.hcl2spec.go | 2 + builder/proxmox/common/artifact.go | 11 +++-- builder/proxmox/common/builder.go | 17 ++++--- builder/proxmox/common/config.go | 3 ++ builder/proxmox/common/config.hcl2spec.go | 2 + .../common/step_convert_to_template.go | 28 +++++++---- .../common/step_convert_to_template_test.go | 46 +++++++++++++++---- ...late_config.go => step_finalize_config.go} | 13 +++--- ...g_test.go => step_finalize_config_test.go} | 9 +++- builder/proxmox/iso/config.hcl2spec.go | 2 + .../proxmox/common/Config-not-required.mdx | 3 ++ 13 files changed, 104 insertions(+), 38 deletions(-) rename builder/proxmox/common/{step_finalize_template_config.go => step_finalize_config.go} (92%) rename builder/proxmox/common/{step_finalize_template_config_test.go => step_finalize_config_test.go} (97%) diff --git a/.web-docs/components/builder/clone/README.md b/.web-docs/components/builder/clone/README.md index d4552350..69d21256 100644 --- a/.web-docs/components/builder/clone/README.md +++ b/.web-docs/components/builder/clone/README.md @@ -262,6 +262,9 @@ boot time. - `template_description` (string) - Description of the template, visible in the Proxmox interface. +- `skip_convert_to_template` (bool) - Skip converting the VM to a template on completion of build. + Defaults to `false` + - `cloud_init` (bool) - If true, add an empty Cloud-Init CDROM drive after the virtual machine has been converted to a template. Defaults to `false`. diff --git a/.web-docs/components/builder/iso/README.md b/.web-docs/components/builder/iso/README.md index f9750c7b..d7c9361c 100644 --- a/.web-docs/components/builder/iso/README.md +++ b/.web-docs/components/builder/iso/README.md @@ -197,6 +197,9 @@ in the image's Cloud-Init settings for provisioning. - `template_description` (string) - Description of the template, visible in the Proxmox interface. +- `skip_convert_to_template` (bool) - Skip converting the VM to a template on completion of build. + Defaults to `false` + - `cloud_init` (bool) - If true, add an empty Cloud-Init CDROM drive after the virtual machine has been converted to a template. Defaults to `false`. diff --git a/builder/proxmox/clone/config.hcl2spec.go b/builder/proxmox/clone/config.hcl2spec.go index 0de5951f..743b1245 100644 --- a/builder/proxmox/clone/config.hcl2spec.go +++ b/builder/proxmox/clone/config.hcl2spec.go @@ -114,6 +114,7 @@ type FlatConfig struct { DisableKVM *bool `mapstructure:"disable_kvm" cty:"disable_kvm" hcl:"disable_kvm"` TemplateName *string `mapstructure:"template_name" cty:"template_name" hcl:"template_name"` TemplateDescription *string `mapstructure:"template_description" cty:"template_description" hcl:"template_description"` + SkipConvertToTemplate *bool `mapstructure:"skip_convert_to_template" cty:"skip_convert_to_template" hcl:"skip_convert_to_template"` CloudInit *bool `mapstructure:"cloud_init" cty:"cloud_init" hcl:"cloud_init"` CloudInitStoragePool *string `mapstructure:"cloud_init_storage_pool" cty:"cloud_init_storage_pool" hcl:"cloud_init_storage_pool"` CloudInitDiskType *string `mapstructure:"cloud_init_disk_type" cty:"cloud_init_disk_type" hcl:"cloud_init_disk_type"` @@ -243,6 +244,7 @@ func (*FlatConfig) HCL2Spec() map[string]hcldec.Spec { "disable_kvm": &hcldec.AttrSpec{Name: "disable_kvm", Type: cty.Bool, Required: false}, "template_name": &hcldec.AttrSpec{Name: "template_name", Type: cty.String, Required: false}, "template_description": &hcldec.AttrSpec{Name: "template_description", Type: cty.String, Required: false}, + "skip_convert_to_template": &hcldec.AttrSpec{Name: "skip_convert_to_template", Type: cty.Bool, Required: false}, "cloud_init": &hcldec.AttrSpec{Name: "cloud_init", Type: cty.Bool, Required: false}, "cloud_init_storage_pool": &hcldec.AttrSpec{Name: "cloud_init_storage_pool", Type: cty.String, Required: false}, "cloud_init_disk_type": &hcldec.AttrSpec{Name: "cloud_init_disk_type", Type: cty.String, Required: false}, diff --git a/builder/proxmox/common/artifact.go b/builder/proxmox/common/artifact.go index f9565f31..fcb5c704 100644 --- a/builder/proxmox/common/artifact.go +++ b/builder/proxmox/common/artifact.go @@ -14,7 +14,8 @@ import ( type Artifact struct { builderID string - templateID int + artifactID int + artifactType string proxmoxClient *proxmox.Client // StateData should store data such as GeneratedData @@ -34,11 +35,11 @@ func (*Artifact) Files() []string { } func (a *Artifact) Id() string { - return strconv.Itoa(a.templateID) + return strconv.Itoa(a.artifactID) } func (a *Artifact) String() string { - return fmt.Sprintf("A template was created: %d", a.templateID) + return fmt.Sprintf("A %s was created: %d", a.artifactType, a.artifactID) } func (a *Artifact) State(name string) interface{} { @@ -46,7 +47,7 @@ func (a *Artifact) State(name string) interface{} { } func (a *Artifact) Destroy() error { - log.Printf("Destroying template: %d", a.templateID) - _, err := a.proxmoxClient.DeleteVm(proxmox.NewVmRef(a.templateID)) + log.Printf("Destroying %s: %d", a.artifactType, a.artifactID) + _, err := a.proxmoxClient.DeleteVm(proxmox.NewVmRef(a.artifactID)) return err } diff --git a/builder/proxmox/common/builder.go b/builder/proxmox/common/builder.go index a10fd691..88a5e3e3 100644 --- a/builder/proxmox/common/builder.go +++ b/builder/proxmox/common/builder.go @@ -71,7 +71,7 @@ func (b *Builder) Run(ctx context.Context, ui packersdk.Ui, hook packersdk.Hook, }, &stepRemoveCloudInitDrive{}, &stepConvertToTemplate{}, - &stepFinalizeTemplateConfig{}, + &stepFinalizeConfig{}, &stepSuccess{}, } preSteps := b.preSteps @@ -118,19 +118,24 @@ func (b *Builder) Run(ctx context.Context, ui packersdk.Ui, hook packersdk.Hook, return nil, errors.New("build was cancelled") } - // Verify that the template_id was set properly, otherwise we didn't progress through the last step - tplID, ok := state.Get("template_id").(int) + // Verify that the artifact_id and artifact_type was set properly, otherwise we didn't progress through the last step + artifactID, ok := state.Get("artifact_id").(int) if !ok { - return nil, fmt.Errorf("template ID could not be determined") + return nil, fmt.Errorf("artifact ID could not be determined") + } + + artifactType, ok := state.Get("artifact_type").(string) + if !ok { + return nil, fmt.Errorf("artifact type could not be determined") } artifact := &Artifact{ builderID: b.id, - templateID: tplID, + artifactID: artifactID, + artifactType: artifactType, proxmoxClient: b.proxmoxClient, StateData: map[string]interface{}{"generated_data": state.Get("generated_data")}, } - return artifact, nil } diff --git a/builder/proxmox/common/config.go b/builder/proxmox/common/config.go index fe4eb33f..3a7dab08 100644 --- a/builder/proxmox/common/config.go +++ b/builder/proxmox/common/config.go @@ -178,6 +178,9 @@ type Config struct { // Description of the template, visible in // the Proxmox interface. TemplateDescription string `mapstructure:"template_description"` + // Skip converting the VM to a template on completion of build. + // Defaults to `false` + SkipConvertToTemplate bool `mapstructure:"skip_convert_to_template"` // If true, add an empty Cloud-Init CDROM drive after the virtual // machine has been converted to a template. Defaults to `false`. diff --git a/builder/proxmox/common/config.hcl2spec.go b/builder/proxmox/common/config.hcl2spec.go index de4bae7c..843fe44c 100644 --- a/builder/proxmox/common/config.hcl2spec.go +++ b/builder/proxmox/common/config.hcl2spec.go @@ -113,6 +113,7 @@ type FlatConfig struct { DisableKVM *bool `mapstructure:"disable_kvm" cty:"disable_kvm" hcl:"disable_kvm"` TemplateName *string `mapstructure:"template_name" cty:"template_name" hcl:"template_name"` TemplateDescription *string `mapstructure:"template_description" cty:"template_description" hcl:"template_description"` + SkipConvertToTemplate *bool `mapstructure:"skip_convert_to_template" cty:"skip_convert_to_template" hcl:"skip_convert_to_template"` CloudInit *bool `mapstructure:"cloud_init" cty:"cloud_init" hcl:"cloud_init"` CloudInitStoragePool *string `mapstructure:"cloud_init_storage_pool" cty:"cloud_init_storage_pool" hcl:"cloud_init_storage_pool"` CloudInitDiskType *string `mapstructure:"cloud_init_disk_type" cty:"cloud_init_disk_type" hcl:"cloud_init_disk_type"` @@ -236,6 +237,7 @@ func (*FlatConfig) HCL2Spec() map[string]hcldec.Spec { "disable_kvm": &hcldec.AttrSpec{Name: "disable_kvm", Type: cty.Bool, Required: false}, "template_name": &hcldec.AttrSpec{Name: "template_name", Type: cty.String, Required: false}, "template_description": &hcldec.AttrSpec{Name: "template_description", Type: cty.String, Required: false}, + "skip_convert_to_template": &hcldec.AttrSpec{Name: "skip_convert_to_template", Type: cty.Bool, Required: false}, "cloud_init": &hcldec.AttrSpec{Name: "cloud_init", Type: cty.Bool, Required: false}, "cloud_init_storage_pool": &hcldec.AttrSpec{Name: "cloud_init_storage_pool", Type: cty.String, Required: false}, "cloud_init_disk_type": &hcldec.AttrSpec{Name: "cloud_init_disk_type", Type: cty.String, Required: false}, diff --git a/builder/proxmox/common/step_convert_to_template.go b/builder/proxmox/common/step_convert_to_template.go index ac663341..43550820 100644 --- a/builder/proxmox/common/step_convert_to_template.go +++ b/builder/proxmox/common/step_convert_to_template.go @@ -16,7 +16,7 @@ import ( // stepConvertToTemplate takes the running VM configured in earlier steps, stops it, and // converts it into a Proxmox template. // -// It sets the template_id state which is used for Artifact lookup. +// It sets the artifact_id state which is used for Artifact lookup. type stepConvertToTemplate struct{} type templateConverter interface { @@ -30,6 +30,7 @@ func (s *stepConvertToTemplate) Run(ctx context.Context, state multistep.StateBa ui := state.Get("ui").(packersdk.Ui) client := state.Get("proxmoxClient").(templateConverter) vmRef := state.Get("vmRef").(*proxmox.VmRef) + c := state.Get("config").(*Config) ui.Say("Stopping VM") _, err := client.ShutdownVm(vmRef) @@ -40,16 +41,23 @@ func (s *stepConvertToTemplate) Run(ctx context.Context, state multistep.StateBa return multistep.ActionHalt } - ui.Say("Converting VM to template") - err = client.CreateTemplate(vmRef) - if err != nil { - err := fmt.Errorf("Error converting VM to template: %s", err) - state.Put("error", err) - ui.Error(err.Error()) - return multistep.ActionHalt + if c.SkipConvertToTemplate { + ui.Say("skip_convert_to_template set, skipping conversion to template") + state.Put("artifact_type", "VM") + } else { + ui.Say("Converting VM to template") + err = client.CreateTemplate(vmRef) + if err != nil { + err := fmt.Errorf("Error converting VM to template: %s", err) + state.Put("error", err) + ui.Error(err.Error()) + return multistep.ActionHalt + } + state.Put("artifact_type", "template") } - log.Printf("template_id: %d", vmRef.VmId()) - state.Put("template_id", vmRef.VmId()) + + log.Printf("artifact_id: %d", vmRef.VmId()) + state.Put("artifact_id", vmRef.VmId()) return multistep.ActionContinue } diff --git a/builder/proxmox/common/step_convert_to_template_test.go b/builder/proxmox/common/step_convert_to_template_test.go index 01835fc2..34a009e0 100644 --- a/builder/proxmox/common/step_convert_to_template_test.go +++ b/builder/proxmox/common/step_convert_to_template_test.go @@ -34,27 +34,45 @@ func TestConvertToTemplate(t *testing.T) { expectCallCreateTemplate bool createTemplateErr error expectedAction multistep.StepAction - expectTemplateIdSet bool + expectArtifactIdSet bool + expectArtifactType string + builderConfig *Config }{ { name: "no errors returns continue and sets template id", expectCallCreateTemplate: true, expectedAction: multistep.ActionContinue, - expectTemplateIdSet: true, + expectArtifactIdSet: true, + expectArtifactType: "template", + builderConfig: &Config{}, + }, + { + name: "no errors returns continue and sets vm id", + expectCallCreateTemplate: true, + expectedAction: multistep.ActionContinue, + expectArtifactIdSet: true, + expectArtifactType: "VM", + builderConfig: &Config{ + SkipConvertToTemplate: true, + }, }, { name: "when shutdown fails, don't try to create template and halt", shutdownErr: fmt.Errorf("failed to stop vm"), expectCallCreateTemplate: false, expectedAction: multistep.ActionHalt, - expectTemplateIdSet: false, + expectArtifactIdSet: false, + expectArtifactType: "", + builderConfig: &Config{}, }, { name: "when create template fails, halt", expectCallCreateTemplate: true, createTemplateErr: fmt.Errorf("failed to stop vm"), expectedAction: multistep.ActionHalt, - expectTemplateIdSet: false, + expectArtifactIdSet: false, + expectArtifactType: "", + builderConfig: &Config{}, }, } @@ -85,6 +103,7 @@ func TestConvertToTemplate(t *testing.T) { state.Put("ui", packersdk.TestUi(t)) state.Put("vmRef", proxmox.NewVmRef(vmid)) state.Put("proxmoxClient", converter) + state.Put("config", c.builderConfig) step := stepConvertToTemplate{} action := step.Run(context.TODO(), state) @@ -92,14 +111,23 @@ func TestConvertToTemplate(t *testing.T) { t.Errorf("Expected action to be %v, got %v", c.expectedAction, action) } - id, wasSet := state.GetOk("template_id") + artifactId, artifactIdWasSet := state.GetOk("artifact_id") + artifactType, artifactTypeWasSet := state.GetOk("artifact_type") + + if c.expectArtifactIdSet != artifactIdWasSet { + t.Errorf("Expected artifact_id state present=%v was present=%v", c.expectArtifactIdSet, artifactIdWasSet) + } + + if c.expectArtifactIdSet && artifactId != vmid { + t.Errorf("Expected artifact_id state to be set to %d, got %v", vmid, artifactId) + } - if c.expectTemplateIdSet != wasSet { - t.Errorf("Expected template_id state present=%v was present=%v", c.expectTemplateIdSet, wasSet) + if c.expectArtifactType == "" && artifactTypeWasSet { + t.Errorf("Expected artifact_type state present=%v was present=%v", c.expectArtifactType, artifactTypeWasSet) } - if c.expectTemplateIdSet && id != vmid { - t.Errorf("Expected template_id state to be set to %d, got %v", vmid, id) + if artifactTypeWasSet && c.expectArtifactType != artifactType { + t.Errorf("Expected artifact_type state to be set to %s, got %s", c.expectArtifactType, artifactType) } }) } diff --git a/builder/proxmox/common/step_finalize_template_config.go b/builder/proxmox/common/step_finalize_config.go similarity index 92% rename from builder/proxmox/common/step_finalize_template_config.go rename to builder/proxmox/common/step_finalize_config.go index ff739043..ec44befe 100644 --- a/builder/proxmox/common/step_finalize_template_config.go +++ b/builder/proxmox/common/step_finalize_config.go @@ -17,18 +17,19 @@ import ( // stepFinalizeTemplateConfig does any required modifications to the configuration _after_ // the VM has been converted into a template, such as updating name and description, or // unmounting the installation ISO. -type stepFinalizeTemplateConfig struct{} +type stepFinalizeConfig struct{} -type templateFinalizer interface { +type finalizer interface { GetVmConfig(*proxmox.VmRef) (map[string]interface{}, error) SetVmConfig(*proxmox.VmRef, map[string]interface{}) (interface{}, error) + StartVm(*proxmox.VmRef) (string, error) } -var _ templateFinalizer = &proxmox.Client{} +var _ finalizer = &proxmox.Client{} -func (s *stepFinalizeTemplateConfig) Run(ctx context.Context, state multistep.StateBag) multistep.StepAction { +func (s *stepFinalizeConfig) Run(ctx context.Context, state multistep.StateBag) multistep.StepAction { ui := state.Get("ui").(packersdk.Ui) - client := state.Get("proxmoxClient").(templateFinalizer) + client := state.Get("proxmoxClient").(finalizer) c := state.Get("config").(*Config) vmRef := state.Get("vmRef").(*proxmox.VmRef) @@ -151,4 +152,4 @@ func (s *stepFinalizeTemplateConfig) Run(ctx context.Context, state multistep.St return multistep.ActionContinue } -func (s *stepFinalizeTemplateConfig) Cleanup(state multistep.StateBag) {} +func (s *stepFinalizeConfig) Cleanup(state multistep.StateBag) {} diff --git a/builder/proxmox/common/step_finalize_template_config_test.go b/builder/proxmox/common/step_finalize_config_test.go similarity index 97% rename from builder/proxmox/common/step_finalize_template_config_test.go rename to builder/proxmox/common/step_finalize_config_test.go index 72223813..2dd241cf 100644 --- a/builder/proxmox/common/step_finalize_template_config_test.go +++ b/builder/proxmox/common/step_finalize_config_test.go @@ -18,6 +18,7 @@ import ( type finalizerMock struct { getConfig func() (map[string]interface{}, error) setConfig func(map[string]interface{}) (string, error) + startVm func() (string, error) } func (m finalizerMock) GetVmConfig(*proxmox.VmRef) (map[string]interface{}, error) { @@ -27,7 +28,11 @@ func (m finalizerMock) SetVmConfig(vmref *proxmox.VmRef, c map[string]interface{ return m.setConfig(c) } -var _ templateFinalizer = finalizerMock{} +func (m finalizerMock) StartVm(*proxmox.VmRef) (string, error) { + return m.startVm() +} + +var _ finalizer = finalizerMock{} func TestTemplateFinalize(t *testing.T) { cs := []struct { @@ -218,7 +223,7 @@ func TestTemplateFinalize(t *testing.T) { state.Put("vmRef", proxmox.NewVmRef(1)) state.Put("proxmoxClient", finalizer) - step := stepFinalizeTemplateConfig{} + step := stepFinalizeConfig{} action := step.Run(context.TODO(), state) if action != c.expectedAction { t.Errorf("Expected action to be %v, got %v", c.expectedAction, action) diff --git a/builder/proxmox/iso/config.hcl2spec.go b/builder/proxmox/iso/config.hcl2spec.go index 9dcc1618..8f22b2e9 100644 --- a/builder/proxmox/iso/config.hcl2spec.go +++ b/builder/proxmox/iso/config.hcl2spec.go @@ -114,6 +114,7 @@ type FlatConfig struct { DisableKVM *bool `mapstructure:"disable_kvm" cty:"disable_kvm" hcl:"disable_kvm"` TemplateName *string `mapstructure:"template_name" cty:"template_name" hcl:"template_name"` TemplateDescription *string `mapstructure:"template_description" cty:"template_description" hcl:"template_description"` + SkipConvertToTemplate *bool `mapstructure:"skip_convert_to_template" cty:"skip_convert_to_template" hcl:"skip_convert_to_template"` CloudInit *bool `mapstructure:"cloud_init" cty:"cloud_init" hcl:"cloud_init"` CloudInitStoragePool *string `mapstructure:"cloud_init_storage_pool" cty:"cloud_init_storage_pool" hcl:"cloud_init_storage_pool"` CloudInitDiskType *string `mapstructure:"cloud_init_disk_type" cty:"cloud_init_disk_type" hcl:"cloud_init_disk_type"` @@ -247,6 +248,7 @@ func (*FlatConfig) HCL2Spec() map[string]hcldec.Spec { "disable_kvm": &hcldec.AttrSpec{Name: "disable_kvm", Type: cty.Bool, Required: false}, "template_name": &hcldec.AttrSpec{Name: "template_name", Type: cty.String, Required: false}, "template_description": &hcldec.AttrSpec{Name: "template_description", Type: cty.String, Required: false}, + "skip_convert_to_template": &hcldec.AttrSpec{Name: "skip_convert_to_template", Type: cty.Bool, Required: false}, "cloud_init": &hcldec.AttrSpec{Name: "cloud_init", Type: cty.Bool, Required: false}, "cloud_init_storage_pool": &hcldec.AttrSpec{Name: "cloud_init_storage_pool", Type: cty.String, Required: false}, "cloud_init_disk_type": &hcldec.AttrSpec{Name: "cloud_init_disk_type", Type: cty.String, Required: false}, diff --git a/docs-partials/builder/proxmox/common/Config-not-required.mdx b/docs-partials/builder/proxmox/common/Config-not-required.mdx index 83de1cde..7602cf2f 100644 --- a/docs-partials/builder/proxmox/common/Config-not-required.mdx +++ b/docs-partials/builder/proxmox/common/Config-not-required.mdx @@ -125,6 +125,9 @@ - `template_description` (string) - Description of the template, visible in the Proxmox interface. +- `skip_convert_to_template` (bool) - Skip converting the VM to a template on completion of build. + Defaults to `false` + - `cloud_init` (bool) - If true, add an empty Cloud-Init CDROM drive after the virtual machine has been converted to a template. Defaults to `false`. From f2a0326643c3a3c9e1a6036937a0dc74976556ba Mon Sep 17 00:00:00 2001 From: Martin Pywell Date: Tue, 29 Oct 2024 12:39:37 +1100 Subject: [PATCH 2/4] skip_convert_to_template: return a running VM, update force flag to work with VM builds --- .../proxmox/common/step_finalize_config.go | 14 +++++- builder/proxmox/common/step_start_vm.go | 48 ++++++++++++++----- builder/proxmox/common/step_start_vm_test.go | 27 +++++++++++ 3 files changed, 75 insertions(+), 14 deletions(-) diff --git a/builder/proxmox/common/step_finalize_config.go b/builder/proxmox/common/step_finalize_config.go index ec44befe..9ac850fb 100644 --- a/builder/proxmox/common/step_finalize_config.go +++ b/builder/proxmox/common/step_finalize_config.go @@ -14,7 +14,7 @@ import ( packersdk "github.com/hashicorp/packer-plugin-sdk/packer" ) -// stepFinalizeTemplateConfig does any required modifications to the configuration _after_ +// stepFinalizeConfig does any required modifications to the configuration _after_ // the VM has been converted into a template, such as updating name and description, or // unmounting the installation ISO. type stepFinalizeConfig struct{} @@ -149,6 +149,18 @@ func (s *stepFinalizeConfig) Run(ctx context.Context, state multistep.StateBag) } } + // When build artifact is to be a VM, return a running VM + if c.SkipConvertToTemplate { + ui.Say("skip_convert_to_template set, resuming VM") + _, err := client.StartVm(vmRef) + if err != nil { + err := fmt.Errorf("Error starting VM: %s", err) + state.Put("error", err) + ui.Error(err.Error()) + return multistep.ActionHalt + } + } + return multistep.ActionContinue } diff --git a/builder/proxmox/common/step_start_vm.go b/builder/proxmox/common/step_start_vm.go index 3781c6b1..8f3b11d3 100644 --- a/builder/proxmox/common/step_start_vm.go +++ b/builder/proxmox/common/step_start_vm.go @@ -36,7 +36,9 @@ type vmStarter interface { GetVmConfig(vmr *proxmox.VmRef) (vmConfig map[string]interface{}, err error) GetVmRefsByName(vmName string) (vmrs []*proxmox.VmRef, err error) SetVmConfig(*proxmox.VmRef, map[string]interface{}) (interface{}, error) + GetVmState(vmr *proxmox.VmRef) (vmState map[string]interface{}, err error) StartVm(*proxmox.VmRef) (string, error) + StopVm(*proxmox.VmRef) (string, error) } var ( @@ -45,7 +47,7 @@ var ( // Check if the given builder configuration maps to an existing VM template on the Proxmox cluster. // Returns an empty *proxmox.VmRef when no matching ID or name is found. -func getExistingTemplate(c *Config, client vmStarter) (*proxmox.VmRef, error) { +func getExistingTemplate(c *Config, client vmStarter) (*proxmox.VmRef, string, error) { vmRef := &proxmox.VmRef{} if c.VMID > 0 { log.Printf("looking up VM with ID %d", c.VMID) @@ -57,9 +59,9 @@ func getExistingTemplate(c *Config, client vmStarter) (*proxmox.VmRef, error) { notFoundError := fmt.Sprintf("vm '%d' not found", c.VMID) if err.Error() == notFoundError { log.Println(err.Error()) - return &proxmox.VmRef{}, nil + return &proxmox.VmRef{}, "", nil } - return &proxmox.VmRef{}, err + return &proxmox.VmRef{}, "", err } log.Printf("found VM with ID %d", vmRef.VmId()) } else { @@ -71,30 +73,33 @@ func getExistingTemplate(c *Config, client vmStarter) (*proxmox.VmRef, error) { notFoundError := fmt.Sprintf("vm '%s' not found", c.TemplateName) if err.Error() == notFoundError { log.Println(err.Error()) - return &proxmox.VmRef{}, nil + return &proxmox.VmRef{}, "", nil } - return &proxmox.VmRef{}, err + return &proxmox.VmRef{}, "", err } if len(vmRefs) > 1 { vmIDs := []int{} for _, vmr := range vmRefs { vmIDs = append(vmIDs, vmr.VmId()) } - return &proxmox.VmRef{}, fmt.Errorf("found multiple VMs with name '%s', IDs: %v", c.TemplateName, vmIDs) + return &proxmox.VmRef{}, "", fmt.Errorf("found multiple VMs with name '%s', IDs: %v", c.TemplateName, vmIDs) } vmRef = vmRefs[0] log.Printf("found VM with name '%s' (ID: %d)", c.TemplateName, vmRef.VmId()) } + if c.SkipConvertToTemplate { + return vmRef, "VM", nil + } log.Printf("check if VM %d is a template", vmRef.VmId()) vmConfig, err := client.GetVmConfig(vmRef) if err != nil { - return &proxmox.VmRef{}, err + return &proxmox.VmRef{}, "", err } log.Printf("VM %d template: %d", vmRef.VmId(), vmConfig["template"]) if vmConfig["template"] == nil { - return &proxmox.VmRef{}, fmt.Errorf("found matching VM (ID: %d, name: %s), but it is not a template", vmRef.VmId(), vmConfig["name"]) + return &proxmox.VmRef{}, "", fmt.Errorf("found matching VM (ID: %d, name: %s), but it is not a template", vmRef.VmId(), vmConfig["name"]) } - return vmRef, nil + return vmRef, "VM template", nil } func (s *stepStartVM) Run(ctx context.Context, state multistep.StateBag) multistep.StepAction { @@ -162,21 +167,38 @@ func (s *stepStartVM) Run(ctx context.Context, state multistep.StateBag) multist if c.PackerForce { ui.Say("Force set, checking for existing artifact on PVE cluster") - vmRef, err := getExistingTemplate(c, client) + vmRef, vmType, err := getExistingTemplate(c, client) if err != nil { state.Put("error", err) ui.Error(err.Error()) return multistep.ActionHalt } if vmRef.VmId() != 0 { - ui.Say(fmt.Sprintf("found existing VM template with ID %d on PVE node %s, deleting it", vmRef.VmId(), vmRef.Node())) + ui.Say(fmt.Sprintf("found existing %s with ID %d on PVE node %s, deleting it", vmType, vmRef.VmId(), vmRef.Node())) + // If building a VM artifact and c.PackerForce is true, + // running VMs can't be deleted. Stop before deleting. + vmState, err := client.GetVmState(vmRef) + if err != nil { + state.Put("error", err) + ui.Error(fmt.Sprintf("error getting VM state: %s", err.Error())) + return multistep.ActionHalt + } + if vmState["status"] == "running" { + log.Printf("VM %d running, stopping for deletion", vmRef.VmId()) + _, err = client.StopVm(vmRef) + if err != nil { + state.Put("error", err) + ui.Error(fmt.Sprintf("error stopping %s: %s", vmType, err.Error())) + return multistep.ActionHalt + } + } _, err = client.DeleteVm(vmRef) if err != nil { state.Put("error", err) - ui.Error(fmt.Sprintf("error deleting VM template: %s", err.Error())) + ui.Error(fmt.Sprintf("error deleting %s: %s", vmType, err.Error())) return multistep.ActionHalt } - ui.Say(fmt.Sprintf("Successfully deleted VM template %d", vmRef.VmId())) + ui.Say(fmt.Sprintf("Successfully deleted %s %d", vmType, vmRef.VmId())) } else { ui.Say("No existing artifact found") } diff --git a/builder/proxmox/common/step_start_vm_test.go b/builder/proxmox/common/step_start_vm_test.go index a228778b..25280834 100644 --- a/builder/proxmox/common/step_start_vm_test.go +++ b/builder/proxmox/common/step_start_vm_test.go @@ -116,9 +116,11 @@ func TestCleanupStartVM(t *testing.T) { type startVMMock struct { create func(*proxmox.VmRef, proxmox.ConfigQemu, multistep.StateBag) error startVm func(*proxmox.VmRef) (string, error) + stopVm func(*proxmox.VmRef) (string, error) setVmConfig func(*proxmox.VmRef, map[string]interface{}) (interface{}, error) getNextID func(id int) (int, error) getVmConfig func(vmr *proxmox.VmRef) (vmConfig map[string]interface{}, err error) + getVmState func(vmr *proxmox.VmRef) (vmState map[string]interface{}, err error) checkVmRef func(vmr *proxmox.VmRef) (err error) getVmByName func(vmName string) (vmrs []*proxmox.VmRef, err error) deleteVm func(vmr *proxmox.VmRef) (exitStatus string, err error) @@ -130,6 +132,9 @@ func (m *startVMMock) Create(vmRef *proxmox.VmRef, config proxmox.ConfigQemu, st func (m *startVMMock) StartVm(vmRef *proxmox.VmRef) (string, error) { return m.startVm(vmRef) } +func (m *startVMMock) StopVm(vmRef *proxmox.VmRef) (string, error) { + return m.stopVm(vmRef) +} func (m *startVMMock) SetVmConfig(vmRef *proxmox.VmRef, config map[string]interface{}) (interface{}, error) { return m.setVmConfig(vmRef, config) } @@ -139,6 +144,9 @@ func (m *startVMMock) GetNextID(id int) (int, error) { func (m *startVMMock) GetVmConfig(vmr *proxmox.VmRef) (map[string]interface{}, error) { return m.getVmConfig(vmr) } +func (m *startVMMock) GetVmState(vmr *proxmox.VmRef) (map[string]interface{}, error) { + return m.getVmState(vmr) +} func (m *startVMMock) CheckVmRef(vmr *proxmox.VmRef) (err error) { return m.checkVmRef(vmr) } @@ -189,6 +197,9 @@ func TestStartVM(t *testing.T) { startVm: func(*proxmox.VmRef) (string, error) { return "", nil }, + stopVm: func(*proxmox.VmRef) (string, error) { + return "", nil + }, setVmConfig: func(*proxmox.VmRef, map[string]interface{}) (interface{}, error) { return nil, nil }, @@ -308,6 +319,7 @@ func TestStartVMWithForce(t *testing.T) { expectedAction multistep.StepAction mockGetVmRefsByName func(vmName string) (vmrs []*proxmox.VmRef, err error) mockGetVmConfig func(vmr *proxmox.VmRef) (map[string]interface{}, error) + mockGetVmState func(vmr *proxmox.VmRef) (map[string]interface{}, error) }{ { name: "Delete existing VM when it's a template and force is enabled", @@ -323,6 +335,9 @@ func TestStartVMWithForce(t *testing.T) { // proxmox-api-go returns a float for "template" return map[string]interface{}{"template": 1.0}, nil }, + mockGetVmState: func(vmr *proxmox.VmRef) (map[string]interface{}, error) { + return map[string]interface{}{"status": "stopped"}, nil + }, }, { name: "Don't delete VM when it's not a template", @@ -337,6 +352,9 @@ func TestStartVMWithForce(t *testing.T) { mockGetVmConfig: func(vmr *proxmox.VmRef) (map[string]interface{}, error) { return map[string]interface{}{}, nil }, + mockGetVmState: func(vmr *proxmox.VmRef) (map[string]interface{}, error) { + return map[string]interface{}{"status": "stopped"}, nil + }, }, { name: "Don't delete VM when force disabled", @@ -390,6 +408,9 @@ func TestStartVMWithForce(t *testing.T) { mockGetVmConfig: func(vmr *proxmox.VmRef) (map[string]interface{}, error) { return map[string]interface{}{"template": 1.0}, nil }, + mockGetVmState: func(vmr *proxmox.VmRef) (map[string]interface{}, error) { + return map[string]interface{}{"status": "running"}, nil + }, }, } @@ -403,6 +424,9 @@ func TestStartVMWithForce(t *testing.T) { startVm: func(*proxmox.VmRef) (string, error) { return "", nil }, + stopVm: func(*proxmox.VmRef) (string, error) { + return "", nil + }, setVmConfig: func(*proxmox.VmRef, map[string]interface{}) (interface{}, error) { return nil, nil }, @@ -418,6 +442,9 @@ func TestStartVMWithForce(t *testing.T) { getVmConfig: func(vmr *proxmox.VmRef) (config map[string]interface{}, err error) { return c.mockGetVmConfig(vmr) }, + getVmState: func(vmr *proxmox.VmRef) (config map[string]interface{}, err error) { + return c.mockGetVmState(vmr) + }, deleteVm: func(vmr *proxmox.VmRef) (exitStatus string, err error) { deleteWasCalled = true return "", nil From a1d970c407170bd008c6e1d41e427dc864f0e381 Mon Sep 17 00:00:00 2001 From: Martin Pywell Date: Wed, 30 Oct 2024 23:11:14 +1100 Subject: [PATCH 3/4] common: skip_convert_to_template updates - updated VM stop/start logic for hardware changes to a VM build - reverted getExistingTemplate signature changes --- .../common/step_convert_to_template.go | 18 +++++------ .../proxmox/common/step_finalize_config.go | 16 ++++++++-- .../common/step_finalize_config_test.go | 11 +++++-- builder/proxmox/common/step_start_vm.go | 30 +++++++++---------- 4 files changed, 46 insertions(+), 29 deletions(-) diff --git a/builder/proxmox/common/step_convert_to_template.go b/builder/proxmox/common/step_convert_to_template.go index 43550820..2915f21c 100644 --- a/builder/proxmox/common/step_convert_to_template.go +++ b/builder/proxmox/common/step_convert_to_template.go @@ -32,19 +32,19 @@ func (s *stepConvertToTemplate) Run(ctx context.Context, state multistep.StateBa vmRef := state.Get("vmRef").(*proxmox.VmRef) c := state.Get("config").(*Config) - ui.Say("Stopping VM") - _, err := client.ShutdownVm(vmRef) - if err != nil { - err := fmt.Errorf("Error converting VM to template, could not stop: %s", err) - state.Put("error", err) - ui.Error(err.Error()) - return multistep.ActionHalt - } - if c.SkipConvertToTemplate { ui.Say("skip_convert_to_template set, skipping conversion to template") state.Put("artifact_type", "VM") } else { + ui.Say("Stopping VM") + _, err := client.ShutdownVm(vmRef) + if err != nil { + err := fmt.Errorf("Error converting VM to template, could not stop: %s", err) + state.Put("error", err) + ui.Error(err.Error()) + return multistep.ActionHalt + } + ui.Say("Converting VM to template") err = client.CreateTemplate(vmRef) if err != nil { diff --git a/builder/proxmox/common/step_finalize_config.go b/builder/proxmox/common/step_finalize_config.go index 9ac850fb..58eee906 100644 --- a/builder/proxmox/common/step_finalize_config.go +++ b/builder/proxmox/common/step_finalize_config.go @@ -23,6 +23,7 @@ type finalizer interface { GetVmConfig(*proxmox.VmRef) (map[string]interface{}, error) SetVmConfig(*proxmox.VmRef, map[string]interface{}) (interface{}, error) StartVm(*proxmox.VmRef) (string, error) + ShutdownVm(*proxmox.VmRef) (string, error) } var _ finalizer = &proxmox.Client{} @@ -46,7 +47,7 @@ func (s *stepFinalizeConfig) Run(ctx context.Context, state multistep.StateBag) vmParams, err := client.GetVmConfig(vmRef) if err != nil { - err := fmt.Errorf("error fetching template config: %s", err) + err := fmt.Errorf("error fetching config: %s", err) state.Put("error", err) ui.Error(err.Error()) return multistep.ActionHalt @@ -140,6 +141,17 @@ func (s *stepFinalizeConfig) Run(ctx context.Context, state multistep.StateBag) changes["delete"] = strings.Join(deleteItems, ",") if len(changes) > 0 { + // Adding a Cloud-Init drive or removing CD-ROM devices won't take effect without a power off and on of the QEMU VM + if c.SkipConvertToTemplate { + ui.Say("Hardware changes pending for VM, stopping VM") + _, err := client.ShutdownVm(vmRef) + if err != nil { + err := fmt.Errorf("Error converting VM to template, could not stop: %s", err) + state.Put("error", err) + ui.Error(err.Error()) + return multistep.ActionHalt + } + } _, err := client.SetVmConfig(vmRef, changes) if err != nil { err := fmt.Errorf("Error updating template: %s", err) @@ -151,7 +163,7 @@ func (s *stepFinalizeConfig) Run(ctx context.Context, state multistep.StateBag) // When build artifact is to be a VM, return a running VM if c.SkipConvertToTemplate { - ui.Say("skip_convert_to_template set, resuming VM") + ui.Say("Resuming VM") _, err := client.StartVm(vmRef) if err != nil { err := fmt.Errorf("Error starting VM: %s", err) diff --git a/builder/proxmox/common/step_finalize_config_test.go b/builder/proxmox/common/step_finalize_config_test.go index 2dd241cf..31c696d5 100644 --- a/builder/proxmox/common/step_finalize_config_test.go +++ b/builder/proxmox/common/step_finalize_config_test.go @@ -16,9 +16,10 @@ import ( ) type finalizerMock struct { - getConfig func() (map[string]interface{}, error) - setConfig func(map[string]interface{}) (string, error) - startVm func() (string, error) + getConfig func() (map[string]interface{}, error) + setConfig func(map[string]interface{}) (string, error) + startVm func() (string, error) + shutdownVm func() (string, error) } func (m finalizerMock) GetVmConfig(*proxmox.VmRef) (map[string]interface{}, error) { @@ -32,6 +33,10 @@ func (m finalizerMock) StartVm(*proxmox.VmRef) (string, error) { return m.startVm() } +func (m finalizerMock) ShutdownVm(*proxmox.VmRef) (string, error) { + return m.shutdownVm() +} + var _ finalizer = finalizerMock{} func TestTemplateFinalize(t *testing.T) { diff --git a/builder/proxmox/common/step_start_vm.go b/builder/proxmox/common/step_start_vm.go index 8f3b11d3..5d1fdac3 100644 --- a/builder/proxmox/common/step_start_vm.go +++ b/builder/proxmox/common/step_start_vm.go @@ -47,7 +47,7 @@ var ( // Check if the given builder configuration maps to an existing VM template on the Proxmox cluster. // Returns an empty *proxmox.VmRef when no matching ID or name is found. -func getExistingTemplate(c *Config, client vmStarter) (*proxmox.VmRef, string, error) { +func getExistingTemplate(c *Config, client vmStarter) (*proxmox.VmRef, error) { vmRef := &proxmox.VmRef{} if c.VMID > 0 { log.Printf("looking up VM with ID %d", c.VMID) @@ -59,9 +59,9 @@ func getExistingTemplate(c *Config, client vmStarter) (*proxmox.VmRef, string, e notFoundError := fmt.Sprintf("vm '%d' not found", c.VMID) if err.Error() == notFoundError { log.Println(err.Error()) - return &proxmox.VmRef{}, "", nil + return &proxmox.VmRef{}, nil } - return &proxmox.VmRef{}, "", err + return &proxmox.VmRef{}, err } log.Printf("found VM with ID %d", vmRef.VmId()) } else { @@ -73,33 +73,33 @@ func getExistingTemplate(c *Config, client vmStarter) (*proxmox.VmRef, string, e notFoundError := fmt.Sprintf("vm '%s' not found", c.TemplateName) if err.Error() == notFoundError { log.Println(err.Error()) - return &proxmox.VmRef{}, "", nil + return &proxmox.VmRef{}, nil } - return &proxmox.VmRef{}, "", err + return &proxmox.VmRef{}, err } if len(vmRefs) > 1 { vmIDs := []int{} for _, vmr := range vmRefs { vmIDs = append(vmIDs, vmr.VmId()) } - return &proxmox.VmRef{}, "", fmt.Errorf("found multiple VMs with name '%s', IDs: %v", c.TemplateName, vmIDs) + return &proxmox.VmRef{}, fmt.Errorf("found multiple VMs with name '%s', IDs: %v", c.TemplateName, vmIDs) } vmRef = vmRefs[0] log.Printf("found VM with name '%s' (ID: %d)", c.TemplateName, vmRef.VmId()) } if c.SkipConvertToTemplate { - return vmRef, "VM", nil + return vmRef, nil } log.Printf("check if VM %d is a template", vmRef.VmId()) vmConfig, err := client.GetVmConfig(vmRef) if err != nil { - return &proxmox.VmRef{}, "", err + return &proxmox.VmRef{}, err } log.Printf("VM %d template: %d", vmRef.VmId(), vmConfig["template"]) if vmConfig["template"] == nil { - return &proxmox.VmRef{}, "", fmt.Errorf("found matching VM (ID: %d, name: %s), but it is not a template", vmRef.VmId(), vmConfig["name"]) + return &proxmox.VmRef{}, fmt.Errorf("found matching VM (ID: %d, name: %s), but it is not a template", vmRef.VmId(), vmConfig["name"]) } - return vmRef, "VM template", nil + return vmRef, nil } func (s *stepStartVM) Run(ctx context.Context, state multistep.StateBag) multistep.StepAction { @@ -167,14 +167,14 @@ func (s *stepStartVM) Run(ctx context.Context, state multistep.StateBag) multist if c.PackerForce { ui.Say("Force set, checking for existing artifact on PVE cluster") - vmRef, vmType, err := getExistingTemplate(c, client) + vmRef, err := getExistingTemplate(c, client) if err != nil { state.Put("error", err) ui.Error(err.Error()) return multistep.ActionHalt } if vmRef.VmId() != 0 { - ui.Say(fmt.Sprintf("found existing %s with ID %d on PVE node %s, deleting it", vmType, vmRef.VmId(), vmRef.Node())) + ui.Say(fmt.Sprintf("found existing resource with ID %d on PVE node %s, deleting it", vmRef.VmId(), vmRef.Node())) // If building a VM artifact and c.PackerForce is true, // running VMs can't be deleted. Stop before deleting. vmState, err := client.GetVmState(vmRef) @@ -188,17 +188,17 @@ func (s *stepStartVM) Run(ctx context.Context, state multistep.StateBag) multist _, err = client.StopVm(vmRef) if err != nil { state.Put("error", err) - ui.Error(fmt.Sprintf("error stopping %s: %s", vmType, err.Error())) + ui.Error(fmt.Sprintf("error stopping resource: %s", err.Error())) return multistep.ActionHalt } } _, err = client.DeleteVm(vmRef) if err != nil { state.Put("error", err) - ui.Error(fmt.Sprintf("error deleting %s: %s", vmType, err.Error())) + ui.Error(fmt.Sprintf("error deleting resource: %s", err.Error())) return multistep.ActionHalt } - ui.Say(fmt.Sprintf("Successfully deleted %s %d", vmType, vmRef.VmId())) + ui.Say(fmt.Sprintf("Successfully deleted %d", vmRef.VmId())) } else { ui.Say("No existing artifact found") } From 2d794d1d543aa68a4b5eef7781a01ae5e25cb15f Mon Sep 17 00:00:00 2001 From: Martin Pywell Date: Wed, 30 Oct 2024 23:17:52 +1100 Subject: [PATCH 4/4] fix error message --- builder/proxmox/common/step_finalize_config.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builder/proxmox/common/step_finalize_config.go b/builder/proxmox/common/step_finalize_config.go index 58eee906..23fea603 100644 --- a/builder/proxmox/common/step_finalize_config.go +++ b/builder/proxmox/common/step_finalize_config.go @@ -146,7 +146,7 @@ func (s *stepFinalizeConfig) Run(ctx context.Context, state multistep.StateBag) ui.Say("Hardware changes pending for VM, stopping VM") _, err := client.ShutdownVm(vmRef) if err != nil { - err := fmt.Errorf("Error converting VM to template, could not stop: %s", err) + err := fmt.Errorf("Error stopping VM: %s", err) state.Put("error", err) ui.Error(err.Error()) return multistep.ActionHalt