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

[Issue 71] Adds convert_to_template option to proxmox builders #72

Closed
wants to merge 6 commits into from
Closed

[Issue 71] Adds convert_to_template option to proxmox builders #72

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cdd14b5
adds convert_to_template option to config and updates docs
LethalServant Feb 28, 2022
2dfa394
adds convert_to_template to iso config tests and ran go generate on c…
LethalServant Feb 28, 2022
fc68e7d
updates proxmox builder steps to follow convert_to_template config op…
LethalServant Mar 6, 2022
fc876b4
updates step convert to template and stop vm tests
LethalServant Mar 7, 2022
3ca1ef1
moves convert to template config check to within step
LethalServant Jul 31, 2022
fcb4b79
Changes ConvertToTemplate option to SkipConvertToTemplate
LethalServant Aug 1, 2022
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
2 changes: 2 additions & 0 deletions builder/proxmox/clone/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.

20 changes: 15 additions & 5 deletions builder/proxmox/common/artifact.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ import (

type Artifact struct {
builderID string
templateID int
vmID int
isTemplate bool
Copy link
Contributor

Choose a reason for hiding this comment

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

While I don't think there's other possible types than VM and template, I would maybe suggest changing this to a string, and have an enumeration to the side.

Something like

type ArtifactType string

const (
	TemplateArtifact ArtifactType = "Template"
	VMArtifact       ArtifactType = "VM"
)
[...]
type Artifact struct {
	builderID string
	artifactID int
	artifactType ArtifactType
[...]
}

Doing it this way also makes it possible to inject this in the String() and Destroy() methods without needing conditionals.

proxmoxClient *proxmox.Client

// StateData should store data such as GeneratedData
Expand All @@ -31,19 +32,28 @@ func (*Artifact) Files() []string {
}

func (a *Artifact) Id() string {
return strconv.Itoa(a.templateID)
return strconv.Itoa(a.vmID)
}

func (a *Artifact) String() string {
return fmt.Sprintf("A template was created: %d", a.templateID)

if a.isTemplate {
return fmt.Sprintf("A Template was created: %d", a.vmID)
}
return fmt.Sprintf("A VM was created: %d", a.vmID)
}

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))

if a.isTemplate {
log.Printf("Destroying Template: %d", a.vmID)
} else {
log.Printf("Destroying VM: %d", a.vmID)
}
_, err := a.proxmoxClient.DeleteVm(proxmox.NewVmRef(a.vmID))
return err
}
57 changes: 31 additions & 26 deletions builder/proxmox/common/builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,29 +47,33 @@ func (b *Builder) Run(ctx context.Context, ui packersdk.Ui, hook packersdk.Hook,

comm := &b.config.Comm

// Build the steps
coreSteps := []multistep.Step{
&stepStartVM{
vmCreator: b.vmCreator,
},
commonsteps.HTTPServerFromHTTPConfig(&b.config.HTTPConfig),
&stepTypeBootCommand{
BootConfig: b.config.BootConfig,
Ctx: b.config.Ctx,
},
&communicator.StepConnect{
Config: comm,
Host: commHost((*comm).Host()),
SSHConfig: (*comm).SSHConfigFunc(),
},
&commonsteps.StepProvision{},
&commonsteps.StepCleanupTempKeys{
Comm: &b.config.Comm,
},
&stepConvertToTemplate{},
&stepFinalizeTemplateConfig{},
&stepSuccess{},
// Build the steps (Order matters)
coreSteps := []multistep.Step{}

coreSteps = append(coreSteps, &stepStartVM{vmCreator: b.vmCreator})
coreSteps = append(coreSteps, commonsteps.HTTPServerFromHTTPConfig(&b.config.HTTPConfig))
coreSteps = append(coreSteps, &stepTypeBootCommand{
BootConfig: b.config.BootConfig,
Ctx: b.config.Ctx,
})
coreSteps = append(coreSteps, &communicator.StepConnect{
Config: comm,
Host: commHost((*comm).Host()),
SSHConfig: (*comm).SSHConfigFunc(),
})
coreSteps = append(coreSteps, &commonsteps.StepProvision{})
coreSteps = append(coreSteps, &commonsteps.StepCleanupTempKeys{
Comm: &b.config.Comm,
})
coreSteps = append(coreSteps, &stepStopVM{})

if b.config.ConvertToTemplate {
coreSteps = append(coreSteps, &stepConvertToTemplate{})
Copy link
Contributor

Choose a reason for hiding this comment

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

I like better when we pass the configuration to the step and within the step is determined if it should be skipped or not based on the configuration. It makes it easier to log "Skipping ..." and keeps this step slice clean.

}

coreSteps = append(coreSteps, &stepFinalizeVMConfig{})
coreSteps = append(coreSteps, &stepSuccess{})
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 for this change? This might be me only but I don't find this more readable than the original way of defining the steps, and it complexifies the diff so I would consider leaving this change out of the current patchset.


preSteps := b.preSteps
for idx := range b.config.AdditionalISOFiles {
preSteps = append(preSteps, &commonsteps.StepDownload{
Expand Down Expand Up @@ -97,15 +101,16 @@ 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)
// Get vmRef for vmid in artifact storage
vmRef, ok := state.Get("vmRef").(*proxmox.VmRef)
Copy link
Contributor

Choose a reason for hiding this comment

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

To be sure, if the artifact is a template, do we still get the appropriate ID from the vmRef in state?

Copy link
Contributor

Choose a reason for hiding this comment

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

Note: yes we do, the current way this is implemented is by writing vmRef.VmId() to the state as template_id, which is not necessary since the vmRef is already part of the state.

if !ok {
return nil, fmt.Errorf("template ID could not be determined")
return nil, fmt.Errorf("vmRef could not be determined")
}

artifact := &Artifact{
builderID: b.id,
templateID: tplID,
vmID: vmRef.VmId(),
isTemplate: b.config.ConvertToTemplate,
proxmoxClient: b.proxmoxClient,
StateData: map[string]interface{}{"generated_data": state.Get("generated_data")},
}
Expand Down
4 changes: 4 additions & 0 deletions builder/proxmox/common/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ type Config struct {

TemplateName string `mapstructure:"template_name"`
TemplateDescription string `mapstructure:"template_description"`
ConvertToTemplate bool `mapstructure:"convert_to_template"`
Copy link
Contributor

Choose a reason for hiding this comment

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

The default of this configuration will always be false which will indeed change the default behavior of the builder. Also, in the prepare method you're explicitly setting it to false as default.

If the intention is to not change the default behavior, the default should be true. In this case, you can:

  • Use Trilian, a custom type available in the SDK for this type of configuration
  • Rename it to skip_convert_to_template that will skip converting to template when set to true. The default will be automatically false.

Copy link
Author

Choose a reason for hiding this comment

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

@sylviamoss Thanks for the feedback, this should be resolved now.


CloudInit bool `mapstructure:"cloud_init"`
CloudInitStoragePool string `mapstructure:"cloud_init_storage_pool"`
Expand Down Expand Up @@ -109,6 +110,9 @@ type vgaConfig struct {
func (c *Config) Prepare(upper interface{}, raws ...interface{}) ([]string, []string, error) {
// Do not add a cloud-init cdrom by default
c.CloudInit = false
// Do not covert vm to a template by default
c.ConvertToTemplate = false

var md mapstructure.Metadata
err := config.Decode(upper, &config.DecodeOpts{
Metadata: &md,
Expand Down
2 changes: 2 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.

21 changes: 3 additions & 18 deletions builder/proxmox/common/step_convert_to_template.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,21 +3,17 @@ package proxmox
import (
"context"
"fmt"
"log"

"github.com/Telmate/proxmox-api-go/proxmox"
"github.com/hashicorp/packer-plugin-sdk/multistep"
packersdk "github.com/hashicorp/packer-plugin-sdk/packer"
)

// 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.
// stepConvertToTemplate takes the stopped VM configured in earlier steps,
// and converts it into a Proxmox VM template.
type stepConvertToTemplate struct{}

type templateConverter interface {
ShutdownVm(*proxmox.VmRef) (string, error)
CreateTemplate(*proxmox.VmRef) error
}

Expand All @@ -28,25 +24,14 @@ func (s *stepConvertToTemplate) Run(ctx context.Context, state multistep.StateBa
client := state.Get("proxmoxClient").(templateConverter)
vmRef := state.Get("vmRef").(*proxmox.VmRef)

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)
var 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
}
log.Printf("template_id: %d", vmRef.VmId())
state.Put("template_id", vmRef.VmId())

return multistep.ActionContinue
}
Expand Down
37 changes: 4 additions & 33 deletions builder/proxmox/common/step_convert_to_template_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,9 @@ import (
)

type converterMock struct {
shutdownVm func(*proxmox.VmRef) (string, error)
createTemplate func(*proxmox.VmRef) error
}

func (m converterMock) ShutdownVm(r *proxmox.VmRef) (string, error) {
return m.shutdownVm(r)
}
func (m converterMock) CreateTemplate(r *proxmox.VmRef) error {
return m.createTemplate(r)
}
Expand All @@ -27,31 +23,21 @@ var _ templateConverter = converterMock{}
func TestConvertToTemplate(t *testing.T) {
cs := []struct {
name string
shutdownErr error
expectCallCreateTemplate bool
createTemplateErr error
expectedAction multistep.StepAction
expectTemplateIdSet bool
}{
{
name: "no errors returns continue and sets template id",
name: "NoErrors",
expectCallCreateTemplate: true,
createTemplateErr: nil,
expectedAction: multistep.ActionContinue,
expectTemplateIdSet: 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,
},
{
name: "when create template fails, halt",
name: "RaiseConvertTemplateError",
expectCallCreateTemplate: true,
createTemplateErr: fmt.Errorf("failed to stop vm"),
createTemplateErr: fmt.Errorf("failed to convert vm to template"),
expectedAction: multistep.ActionHalt,
expectTemplateIdSet: false,
},
}

Expand All @@ -60,12 +46,6 @@ func TestConvertToTemplate(t *testing.T) {
for _, c := range cs {
t.Run(c.name, func(t *testing.T) {
converter := converterMock{
shutdownVm: func(r *proxmox.VmRef) (string, error) {
if r.VmId() != vmid {
t.Errorf("ShutdownVm called with unexpected id, expected %d, got %d", vmid, r.VmId())
}
return "", c.shutdownErr
},
createTemplate: func(r *proxmox.VmRef) error {
if r.VmId() != vmid {
t.Errorf("CreateTemplate called with unexpected id, expected %d, got %d", vmid, r.VmId())
Expand All @@ -89,15 +69,6 @@ func TestConvertToTemplate(t *testing.T) {
t.Errorf("Expected action to be %v, got %v", c.expectedAction, action)
}

id, wasSet := state.GetOk("template_id")

if c.expectTemplateIdSet != wasSet {
t.Errorf("Expected template_id state present=%v was present=%v", c.expectTemplateIdSet, wasSet)
}

if c.expectTemplateIdSet && id != vmid {
t.Errorf("Expected template_id state to be set to %d, got %v", vmid, id)
}
})
}
}
8 changes: 4 additions & 4 deletions ...x/common/step_finalize_template_config.go → ...proxmox/common/step_finalize_vm_config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,10 @@ import (
packersdk "github.com/hashicorp/packer-plugin-sdk/packer"
)

// stepFinalizeTemplateConfig does any required modifications to the configuration _after_
// stepFinalizeVMConfig 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 stepFinalizeVMConfig struct{}
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we can either produce a template or a VM, might I suggest renaming this step as stepFinalize instead? Just to avoid confusion


type templateFinalizer interface {
GetVmConfig(*proxmox.VmRef) (map[string]interface{}, error)
Expand All @@ -22,7 +22,7 @@ type templateFinalizer interface {

var _ templateFinalizer = &proxmox.Client{}

func (s *stepFinalizeTemplateConfig) Run(ctx context.Context, state multistep.StateBag) multistep.StepAction {
func (s *stepFinalizeVMConfig) Run(ctx context.Context, state multistep.StateBag) multistep.StepAction {
ui := state.Get("ui").(packersdk.Ui)
client := state.Get("proxmoxClient").(templateFinalizer)
c := state.Get("config").(*Config)
Expand Down Expand Up @@ -118,4 +118,4 @@ func (s *stepFinalizeTemplateConfig) Run(ctx context.Context, state multistep.St
return multistep.ActionContinue
}

func (s *stepFinalizeTemplateConfig) Cleanup(state multistep.StateBag) {}
func (s *stepFinalizeVMConfig) Cleanup(state multistep.StateBag) {}
2 changes: 1 addition & 1 deletion ...mon/step_finalize_template_config_test.go → ...ox/common/step_finalize_vm_config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ func TestTemplateFinalize(t *testing.T) {
state.Put("vmRef", proxmox.NewVmRef(1))
state.Put("proxmoxClient", finalizer)

step := stepFinalizeTemplateConfig{}
step := stepFinalizeVMConfig{}
action := step.Run(context.TODO(), state)
if action != c.expectedAction {
t.Errorf("Expected action to be %v, got %v", c.expectedAction, action)
Expand Down
39 changes: 39 additions & 0 deletions builder/proxmox/common/step_stop_vm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
package proxmox

import (
"context"
"fmt"

"github.com/Telmate/proxmox-api-go/proxmox"
"github.com/hashicorp/packer-plugin-sdk/multistep"
packersdk "github.com/hashicorp/packer-plugin-sdk/packer"
)

// stepStopVM takes the running VM configured in earlier steps and stops it.

type stepStopVM struct{}

type vmTerminator interface {
ShutdownVm(*proxmox.VmRef) (string, error)
}

var _ templateConverter = &proxmox.Client{}

func (s *stepStopVM) Run(ctx context.Context, state multistep.StateBag) multistep.StepAction {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder why this becomes a step on its own here, it seems that the step_convert_to_template does this already, why not move the check after the VM is shutdown so we don't have to introduce another step?
For the record, I don't hate this change by itself, it's just that this brings more changes to this PR which makes the diff larger and harder to understand.

ui := state.Get("ui").(packersdk.Ui)
client := state.Get("proxmoxClient").(vmTerminator)
vmRef := state.Get("vmRef").(*proxmox.VmRef)

ui.Say("Stopping VM")
_, err := client.ShutdownVm(vmRef)
if err != nil {
err := fmt.Errorf("Error converting VM to template, could not stop: %s", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

If we keep this step, the error message should be reworked to specify that this is not converting the VM to a template that failed, but the "stop VM" step.

state.Put("error", err)
ui.Error(err.Error())
return multistep.ActionHalt
}

return multistep.ActionContinue
}

func (s *stepStopVM) Cleanup(state multistep.StateBag) {}
Loading