vm commit: reset machine ID before committing image

Author: JoelColledge

cmd/image_build.go

@@ -212,11 +212,14 @@ func imageBuildCommand() *cobra.Command {
 			}
 
 			buildConfig := virter.ImageBuildConfig{
-				ImageName:       newImageName,
 				ContainerName:   containerName,
-				ShutdownTimeout: shutdownTimeout,
 				ProvisionConfig: provisionConfig,
-				ResetMachineID:  resetMachineID,
+				CommitConfig: virter.CommitConfig{
+					ImageName:       newImageName,
+					Shutdown:        true,
+					ShutdownTimeout: shutdownTimeout,
+					ResetMachineID:  resetMachineID,
+				},
 			}
 
 			p = mpb.NewWithContext(ctx, DefaultContainerOpt())

cmd/vm_commit.go

@@ -12,6 +12,7 @@ import (
 )
 
 func vmCommitCommand() *cobra.Command {
+	var resetMachineID bool
 	var shutdown bool
 
 	var commitCmd = &cobra.Command{
@@ -39,7 +40,14 @@ the virtual machine name if no other value is given.`,
 
 			p := mpb.NewWithContext(ctx, DefaultContainerOpt())
 
-			err = v.VMCommit(ctx, actualtime.ActualTime{}, vmName, imageName, shutdown, shutdownTimeout, viper.GetBool("libvirt.static_dhcp"), virter.WithProgress(DefaultProgressFormat(p)))
+			commitConfig := virter.CommitConfig{
+				ImageName:       imageName,
+				Shutdown:        shutdown,
+				ShutdownTimeout: shutdownTimeout,
+				ResetMachineID:  resetMachineID,
+			}
+
+			err = v.VMCommit(ctx, actualtime.ActualTime{}, vmName, commitConfig, viper.GetBool("libvirt.static_dhcp"), virter.WithProgress(DefaultProgressFormat(p)))
 			if err != nil {
 				log.Fatal(err)
 			}
@@ -57,7 +65,8 @@ the virtual machine name if no other value is given.`,
 		},
 	}
 
-	commitCmd.Flags().BoolVarP(&shutdown, "shutdown", "s", false, "whether to shut the VM down and wait until it stops (default false)")
+	commitCmd.Flags().BoolVar(&resetMachineID, "reset-machine-id", true, "Whether or not to clear the /etc/machine-id file after provisioning")
+	commitCmd.Flags().BoolVarP(&shutdown, "shutdown", "s", true, "Whether to shut the VM down and wait until it stops")
 
 	return commitCmd
 }

internal/virter/image.go

@@ -15,7 +15,6 @@ import (
 	"net/http"
 	"strings"
 	"sync"
-	"time"
 
 	"github.com/LINBIT/containerapi"
 	regv1 "github.com/google/go-containerregistry/pkg/v1"
@@ -531,11 +530,9 @@ type ImageBuildTools struct {
 
 // ImageBuildConfig contains the configuration for building an image
 type ImageBuildConfig struct {
-	ImageName       string
 	ContainerName   string
-	ShutdownTimeout time.Duration
 	ProvisionConfig ProvisionConfig
-	ResetMachineID  bool
+	CommitConfig    CommitConfig
 }
 
 func (v *Virter) imageBuildProvisionCommit(ctx context.Context, tools ImageBuildTools, vmConfig VMConfig, readyConfig VmReadyConfig, buildConfig ImageBuildConfig, opts ...LayerOperationOption) error {
@@ -547,17 +544,6 @@ func (v *Virter) imageBuildProvisionCommit(ctx context.Context, tools ImageBuild
 		return err
 	}
 
-	if buildConfig.ResetMachineID {
-		// starting the VM creates a machine-id
-		// we want these IDs to be unique, so reset to empty
-		resetMachineID := ProvisionStep{
-			Shell: &ProvisionShellStep{
-				Script: "truncate -c -s 0 /etc/machine-id",
-			},
-		}
-		buildConfig.ProvisionConfig.Steps = append(buildConfig.ProvisionConfig.Steps, resetMachineID)
-	}
-
 	for _, s := range buildConfig.ProvisionConfig.Steps {
 		if s.Container != nil {
 			containerCfg := containerapi.NewContainerConfig(
@@ -580,7 +566,7 @@ func (v *Virter) imageBuildProvisionCommit(ctx context.Context, tools ImageBuild
 		}
 	}
 
-	err = v.VMCommit(ctx, tools.AfterNotifier, vmConfig.Name, buildConfig.ImageName, true, buildConfig.ShutdownTimeout, vmConfig.StaticDHCP, opts...)
+	err = v.VMCommit(ctx, tools.AfterNotifier, vmConfig.Name, buildConfig.CommitConfig, vmConfig.StaticDHCP, opts...)
 	if err != nil {
 		return err
 	}

internal/virter/vm.go

@@ -380,28 +380,55 @@ func (v *Virter) rmSnapshots(domain libvirt.Domain) error {
 	return nil
 }
 
+// CommitConfig contains the configuration for committing a VM
+type CommitConfig struct {
+	ImageName       string
+	Shutdown        bool
+	ShutdownTimeout time.Duration
+	ResetMachineID  bool
+}
+
 // VMCommit commits a VM to an image. If shutdown is true, the VM is shut down
 // before committing. If shutdown is false, the caller is responsible for
 // ensuring that the VM is not running.
-func (v *Virter) VMCommit(ctx context.Context, afterNotifier AfterNotifier, vmName, imageName string, shutdown bool, shutdownTimeout time.Duration, staticDHCP bool, opts ...LayerOperationOption) error {
+func (v *Virter) VMCommit(ctx context.Context, afterNotifier AfterNotifier, vmName string, commitConfig CommitConfig, staticDHCP bool, opts ...LayerOperationOption) error {
 	domain, err := v.libvirt.DomainLookupByName(vmName)
 	if err != nil {
 		return fmt.Errorf("could not get domain: %w", err)
 	}
 
-	if shutdown {
-		err = v.vmShutdown(ctx, afterNotifier, shutdownTimeout, domain)
-		if err != nil {
-			return err
+	active, err := v.libvirt.DomainIsActive(domain)
+	if err != nil {
+		return fmt.Errorf("could not check if domain is active: %w", err)
+	}
+
+	running := active != 0
+
+	// Check if shutdown is allowed before resetting machine ID.
+	if running && !commitConfig.Shutdown {
+		return fmt.Errorf("must allow shutdown to commit a running VM")
+	}
+
+	if commitConfig.ResetMachineID {
+		if !running {
+			return fmt.Errorf("cannot reset machine ID of VM '%s' that is not running", vmName)
 		}
-	} else {
-		active, err := v.libvirt.DomainIsActive(domain)
+
+		// Starting the VM creates a machine ID.
+		// We want these IDs to be unique, so reset to empty.
+		err = v.VMExecShell(
+			ctx, []string{vmName},
+			&ProvisionShellStep{Script: "truncate -c -s 0 /etc/machine-id"})
+
 		if err != nil {
-			return fmt.Errorf("could not check if domain is active: %w", err)
+			return err
 		}
+	}
 
-		if active != 0 {
-			return fmt.Errorf("cannot commit a running VM")
+	if running && commitConfig.Shutdown {
+		err = v.vmShutdown(ctx, afterNotifier, commitConfig.ShutdownTimeout, domain)
+		if err != nil {
+			return err
 		}
 	}
 
@@ -424,7 +451,7 @@ func (v *Virter) VMCommit(ctx context.Context, afterNotifier AfterNotifier, vmNa
 		return err
 	}
 
-	_, err = v.MakeImage(imageName, volumeLayer, opts...)
+	_, err = v.MakeImage(commitConfig.ImageName, volumeLayer, opts...)
 	if err != nil {
 		return err
 	}

internal/virter/vm_test.go

@@ -267,7 +267,7 @@ func TestVMCommit(t *testing.T) {
 
 		an := new(mocks.AfterNotifier)
 
-		if r[commitShutdown] {
+		if r[commitShutdown] && r[commitDomainActive] {
 			if r[commitShutdownTimeout] {
 				timeout := make(chan time.Time, 1)
 				timeout <- time.Unix(0, 0)
@@ -281,7 +281,14 @@ func TestVMCommit(t *testing.T) {
 		pool, err := l.StoragePoolLookupByName(poolName)
 		assert.NoError(t, err)
 
-		err = v.VMCommit(context.Background(), an, vmName, vmName, r[commitShutdown], shutdownTimeout, false)
+		commitConfig := virter.CommitConfig{
+			ImageName:       vmName,
+			Shutdown:        r[commitShutdown],
+			ShutdownTimeout: shutdownTimeout,
+			ResetMachineID:  false,
+		}
+
+		err = v.VMCommit(context.Background(), an, vmName, commitConfig, false)
 		if expectOk {
 			assert.NoError(t, err)