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.

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

[Issue 71] add skip_convert_to_template option #283

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -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`.

Expand Down
3 changes: 3 additions & 0 deletions .web-docs/components/builder/iso/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`.

Expand Down
2 changes: 2 additions & 0 deletions builder/proxmox/clone/config.hcl2spec.go

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

11 changes: 6 additions & 5 deletions builder/proxmox/common/artifact.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -34,19 +35,19 @@ 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{} {
return a.StateData[name]
}

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
}
17 changes: 11 additions & 6 deletions builder/proxmox/common/builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
}

Expand Down
3 changes: 3 additions & 0 deletions builder/proxmox/common/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
Expand Down
2 changes: 2 additions & 0 deletions builder/proxmox/common/config.hcl2spec.go

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

44 changes: 26 additions & 18 deletions builder/proxmox/common/step_convert_to_template.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -30,26 +30,34 @@ 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)
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 {
err := fmt.Errorf("Error converting VM to template: %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 {
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
}
Expand Down
46 changes: 37 additions & 9 deletions builder/proxmox/common/step_convert_to_template_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{},
},
}

Expand Down Expand Up @@ -85,21 +103,31 @@ 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)
if action != c.expectedAction {
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)
}
})
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,21 +14,23 @@ 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 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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason to add StartVm to that contract? It seems that the function is not invoked in the step, and AFAICT this is not used elsewhere, so I wonder if that might be a leftover from a WIP state of the PR?
Feel free to correct me if I'm wrong though, I might've missed something

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was left over from a WIP version of the PR where the returned artifact was a running VM. I noted in #72 (comment) a desire to have a running VM returned, so I've rerolled with those changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yeah understood, I missed the resume VM part.

I wonder though, the current workflow appears to work (can't test it as I don't have a proxmox machine on hand), but I would think we can keep the VM running throughout the process if we're not exporting to a template, don't we?
In this case we can change a bit the Cleanup method of the step_start_vm so it doesn't kill and delete the VM, unless it cannot?
Also to be sure, since the cleanup for the VM will run after step_finalize_config, will the VM still exist? I'd assume this will be stopped and removed, won't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A stop and start of the VM is required if there are VM hardware changes for the VM's Cloud-Init storage and CD-ROM devices (step step_finalize_config).

These changes aren't picked up by the VM unless it's completely stopped then started again, they can't be hot-swapped or mounted on an ACPI reboot. This is a limitation on the Proxmox backend.

I've reworked the logic to only stop the VM artifact if there are changes to be done in step_finalize_config (if len(changes) > 0) that way if Cloud-Init isn't being configured or CD-ROM devices are being kept, the VM won't restart.

ShutdownVm(*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)

Expand All @@ -45,7 +47,7 @@ func (s *stepFinalizeTemplateConfig) Run(ctx context.Context, state multistep.St

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
Expand Down Expand Up @@ -139,6 +141,17 @@ func (s *stepFinalizeTemplateConfig) Run(ctx context.Context, state multistep.St
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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, why is the shutdown only done when there are changes? I would've thought this should be coupled with the call to StartVM in every case should you decide not to convert the VM to a template.
One scenario that I have in mind (no clue if that is realistic or not though), if we don't have changes the VM won't be shutdown, and we'll try to start it again afterwards, could the call fail if the VM is still running?

ui.Say("Hardware changes pending for VM, stopping VM")
_, err := client.ShutdownVm(vmRef)
if err != nil {
err := fmt.Errorf("Error stopping VM: %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)
Expand All @@ -148,7 +161,19 @@ func (s *stepFinalizeTemplateConfig) Run(ctx context.Context, state multistep.St
}
}

// When build artifact is to be a VM, return a running VM
if c.SkipConvertToTemplate {
ui.Say("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
}

func (s *stepFinalizeTemplateConfig) Cleanup(state multistep.StateBag) {}
func (s *stepFinalizeConfig) Cleanup(state multistep.StateBag) {}
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,10 @@ import (
)

type finalizerMock struct {
getConfig func() (map[string]interface{}, error)
setConfig func(map[string]interface{}) (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) {
Expand All @@ -27,7 +29,15 @@ 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()
}

func (m finalizerMock) ShutdownVm(*proxmox.VmRef) (string, error) {
return m.shutdownVm()
}

var _ finalizer = finalizerMock{}

func TestTemplateFinalize(t *testing.T) {
cs := []struct {
Expand Down Expand Up @@ -218,7 +228,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)
Expand Down
Loading