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

Add option to destroy vifs #43

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 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
2 changes: 2 additions & 0 deletions builder/xenserver/common/common_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ type CommonConfig struct {
RawSSHWaitTimeout string `mapstructure:"ssh_wait_timeout"`
SSHWaitTimeout time.Duration

DestroyVIFs bool `mapstructure:"destroy_vifs"`

OutputDir string `mapstructure:"output_directory"`
Format string `mapstructure:"format"`
KeepVM string `mapstructure:"keep_vm"`
Expand Down
2 changes: 2 additions & 0 deletions builder/xenserver/common/config.hcl2spec.go

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

50 changes: 50 additions & 0 deletions builder/xenserver/common/step_destroy_vifs.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
package common

import (
"context"
"fmt"
"log"

"github.com/hashicorp/packer-plugin-sdk/multistep"
"github.com/hashicorp/packer-plugin-sdk/packer"
)

type StepDestroyVIFs struct{}

func (self *StepDestroyVIFs) Run(ctx context.Context, state multistep.StateBag) multistep.StepAction {
c := state.Get("client").(*Connection)
config := state.Get("config").(Config)
ui := state.Get("ui").(packer.Ui)

if !config.DestroyVIFs {
log.Printf("Not destroying VIFs")
return multistep.ActionContinue
}

ui.Say("Step: Destroy VIFs")

uuid := state.Get("instance_uuid").(string)
instance, err := c.client.VM.GetByUUID(c.session, uuid)
if err != nil {
ui.Error(fmt.Sprintf("Unable to get VM from UUID '%s': %s", uuid, err.Error()))
return multistep.ActionHalt
}

vifs, err := c.client.VM.GetVIFs(c.session, instance)
if err != nil {
ui.Error(fmt.Sprintf("Error getting VIFs: %s", err.Error()))
return multistep.ActionHalt
}

for _, vif := range vifs {
err = c.client.VIF.Destroy(c.session, vif)
if err != nil {
ui.Error(fmt.Sprintf("Error destroying VIF: %s", err.Error()))
return multistep.ActionHalt
}
}

return multistep.ActionContinue
}

func (self *StepDestroyVIFs) Cleanup(state multistep.StateBag) {}
1 change: 1 addition & 0 deletions builder/xenserver/iso/builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -297,6 +297,7 @@ func (self *Builder) Run(ctx context.Context, ui packer.Ui, hook packer.Hook) (p
&xscommon.StepDetachVdi{
VdiUuidKey: "floppy_vdi_uuid",
},
new(xscommon.StepDestroyVIFs),
new(xscommon.StepExport),
}

Expand Down
1 change: 1 addition & 0 deletions builder/xenserver/xva/builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@ func (self *Builder) Run(ctx context.Context, ui packer.Ui, hook packer.Hook) (p
&xscommon.StepDetachVdi{
VdiUuidKey: "tools_vdi_uuid",
},
new(xscommon.StepDestroyVIFs),
new(xscommon.StepExport),
}

Expand Down
4 changes: 4 additions & 0 deletions docs/builders/iso/xenserver-iso.html.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,10 @@ each category, the available options are alphabetized and described.
run `xe template-list`. Setting the correct value hints to XenServer how to
optimize the virtual hardware to work best with that operating system.

* `destroy_vifs` (boolean) - Whether to destroy VIFs on the VM prior to
exporting. Removing them may make the image more generic and reusable.
Default is `false`.

Comment on lines +89 to +92
Copy link
Owner

@ddelnano ddelnano Apr 14, 2023

Choose a reason for hiding this comment

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

@heindsight can you explain a scenario where having the VIFs remain hurts the template's reusability? I understand the general sentiment that it is unnecessary to include extra configuration, but I don't quite see where this could manifest to a concrete problem.

Copy link
Author

Choose a reason for hiding this comment

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

@ddelnano to be honest, I have not encountered such a situation myself. I cherry picked this from another fork that we were previously using at Sohonet because we have a number of existing templates that use this option.

The main reason for originally using this option in our templates really is just to avoid including unnecessary extra configuration.

Choose a reason for hiding this comment

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

Hello - I couldn't resist chiming in.

On the surface, it seems sloppy to leave a 10MB container that will never be used again. Or is there an example where it could be?

What will happen if it is left in place and ends up inside a template. Will we get TWO of these things when a VM is deployed from the template, and if so - will the "current" or new one be used correctly for deployment?

Or instead will a new deployment just overwrite the existing one?

How will a Windows VM react to a snapshot request with a drive like this hanging around?

Additionally, it seems wasteful - I know we live in an age where we can be sloppy with diskspace because you can get 1TB drive for $40 or so, so... who cares about soaking up 10MB on these things. But not all VMs run on cheap harddrives

In principal, it would seem to be best if temporary drives were really temporary and cleaned up after the fact. Garbage collection seems to be a good practice, whether in terms of RAM or disk space.

Copy link
Author

Choose a reason for hiding this comment

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

@Roche-Mike this PR adds an option to remove virtual network interfaces from the the VM template. It has nothing to do with any 10MB temporary drive.

Copy link
Owner

Choose a reason for hiding this comment

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

On the surface, it seems sloppy to leave a 10MB container that will never be used again. Or is there an example where it could be?

@Roche-Mike as Heinrich mentioned, this should be unrelated to what you described. However, I am interested in what you are describing and it's something we should consider if the plugin is having the correct behavior. Can you please open a new issue with the relevant context so we can have a focused discussion on that behavior?

* `disk_size` (integer) - The size, in megabytes, of the hard disk to create
for the VM. By default, this is 40000 (about 40 GB).

Expand Down