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 all 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: 30 additions & 27 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,31 @@ 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{})

coreSteps = append(coreSteps, &stepConvertToTemplate{})

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 +99,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.SkipConvertToTemplate.True(),
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 decide to keep it as a Trilean, this could be changed to

Suggested change
isTemplate: !b.config.SkipConvertToTemplate.True(),
isTemplate: b.config.SkipConvertToTemplate.False(),

proxmoxClient: b.proxmoxClient,
StateData: map[string]interface{}{"generated_data": state.Get("generated_data")},
}
Expand Down
6 changes: 4 additions & 2 deletions builder/proxmox/common/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,9 @@ type Config struct {
Onboot bool `mapstructure:"onboot"`
DisableKVM bool `mapstructure:"disable_kvm"`

TemplateName string `mapstructure:"template_name"`
TemplateDescription string `mapstructure:"template_description"`
TemplateName string `mapstructure:"template_name"`
TemplateDescription string `mapstructure:"template_description"`
SkipConvertToTemplate config.Trilean `mapstructure:"skip_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.

If there a reason why this is a Trilean? If the intent is that this is false by default, bool would work this way, and this would simplify using the option


CloudInit bool `mapstructure:"cloud_init"`
CloudInitStoragePool string `mapstructure:"cloud_init_storage_pool"`
Expand Down Expand Up @@ -109,6 +110,7 @@ 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

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.

38 changes: 15 additions & 23 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 @@ -27,26 +23,22 @@ 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.True() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can be inverted here since this is the last step here

Suggested change
if !c.SkipConvertToTemplate.True() {
if c.SkipConvertToTemplate {
ui.Say("Skipping VM template conversion")
return multistep.ActionContinue
}

The rest of the code can be left unchanged if we do it this way


ui.Say("Converting VM to template")
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
}

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
} else {
ui.Say("Skipping VM template conversion")
}
log.Printf("template_id: %d", vmRef.VmId())
state.Put("template_id", vmRef.VmId())

return multistep.ActionContinue
}
Expand Down
61 changes: 29 additions & 32 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 @@ -8,16 +8,13 @@ import (
"github.com/Telmate/proxmox-api-go/proxmox"
"github.com/hashicorp/packer-plugin-sdk/multistep"
packersdk "github.com/hashicorp/packer-plugin-sdk/packer"
"github.com/hashicorp/packer-plugin-sdk/template/config"
)

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,46 +24,54 @@ var _ templateConverter = converterMock{}
func TestConvertToTemplate(t *testing.T) {
cs := []struct {
name string
shutdownErr error
expectCallCreateTemplate bool
createTemplateErr error
expectedAction multistep.StepAction
expectTemplateIdSet bool
builderConfig *Config
}{
{
name: "no errors returns continue and sets template id",
name: "NoErrorsUnset",
expectCallCreateTemplate: true,
createTemplateErr: nil,
expectedAction: multistep.ActionContinue,
expectTemplateIdSet: true,
builderConfig: &Config{},
},
{
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: "NoErrors",
expectCallCreateTemplate: true,
createTemplateErr: nil,
expectedAction: multistep.ActionContinue,
builderConfig: &Config{
SkipConvertToTemplate: config.TriFalse,
},
},
{
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,
builderConfig: &Config{
SkipConvertToTemplate: config.TriFalse,
},
},
{
name: "SkipConvertToTemplate",
expectCallCreateTemplate: false,
createTemplateErr: nil,
expectedAction: multistep.ActionContinue,
builderConfig: &Config{
SkipConvertToTemplate: config.TriTrue,
},
},
}

const vmid = 123
const vmid = 1

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 @@ -82,22 +87,14 @@ 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")

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
Loading