refactor: PingSSH -> WaitVmReady

We want to know when a VM can be safely accessed. This might happen
only long after sshd was configured. Since we no longer wait just
for SSH, we rename the wrapping function, so we can safely change
implementation details without confusing function consumers.

To this end, renamed:
* virter.PingSSH -> virter.WaitVmReady
* virter.SSHPingConfig -> virter.VmReadyConfig

We leave the config values related to VM readiness unchanged, since
there does not seem to be an easily implemented and backwards compatible
way of renaming those.

cmd/config.go

@@ -14,6 +14,7 @@ import (
 	log "github.com/sirupsen/logrus"
 	"github.com/spf13/viper"
 
+	"github.com/LINBIT/virter/internal/virter"
 	"github.com/LINBIT/virter/pkg/sshkeys"
 )
 
@@ -209,3 +210,12 @@ func initSSHFromConfig() {
 		log.Fatal(err)
 	}
 }
+
+func getReadyConfig() virter.VmReadyConfig {
+	// The config keys are kept for compatibility.
+	// Note that viper.RegisterAlias sounds like it could be used, but I couldn't make it work.
+	return virter.VmReadyConfig{
+		Retries:      viper.GetInt("time.ssh_ping_count"),
+		CheckTimeout: viper.GetDuration("time.ssh_ping_period"),
+	}
+}

cmd/image_build.go

@@ -184,11 +184,6 @@ func imageBuildCommand() *cobra.Command {
 				Mounts:             mounts,
 			}
 
-			sshPingConfig := virter.SSHPingConfig{
-				SSHPingCount:  viper.GetInt("time.ssh_ping_count"),
-				SSHPingPeriod: viper.GetDuration("time.ssh_ping_period"),
-			}
-
 			containerName := "virter-build-" + newImageName
 
 			if provisionConfig.NeedsContainers() {
@@ -210,7 +205,7 @@ func imageBuildCommand() *cobra.Command {
 
 			p = mpb.NewWithContext(ctx, DefaultContainerOpt())
 
-			err = v.ImageBuild(ctx, tools, vmConfig, sshPingConfig, buildConfig, virter.WithProgress(DefaultProgressFormat(p)))
+			err = v.ImageBuild(ctx, tools, vmConfig, getReadyConfig(), buildConfig, virter.WithProgress(DefaultProgressFormat(p)))
 			if err != nil {
 				log.Fatalf("Failed to build image: %v", err)
 			}

cmd/vm_run.go

@@ -226,12 +226,7 @@ func vmRunCommand() *cobra.Command {
 					}
 
 					if waitSSH {
-						sshPingConfig := virter.SSHPingConfig{
-							SSHPingCount:  viper.GetInt("time.ssh_ping_count"),
-							SSHPingPeriod: viper.GetDuration("time.ssh_ping_period"),
-						}
-
-						err = v.PingSSH(ctx, SSHClientBuilder{}, thisVMName, sshPingConfig)
+						err = v.WaitVmReady(ctx, SSHClientBuilder{}, thisVMName, getReadyConfig())
 						if err != nil {
 							return fmt.Errorf("Failed to connect to VM %d over SSH: %w", id, err)
 						}

internal/virter/image.go

@@ -537,11 +537,11 @@ type ImageBuildConfig struct {
 	ResetMachineID  bool
 }
 
-func (v *Virter) imageBuildProvisionCommit(ctx context.Context, tools ImageBuildTools, vmConfig VMConfig, pingConfig SSHPingConfig, buildConfig ImageBuildConfig, opts ...LayerOperationOption) error {
+func (v *Virter) imageBuildProvisionCommit(ctx context.Context, tools ImageBuildTools, vmConfig VMConfig, readyConfig VmReadyConfig, buildConfig ImageBuildConfig, opts ...LayerOperationOption) error {
 	vmNames := []string{vmConfig.Name}
 	var err error
 
-	err = v.PingSSH(ctx, tools.ShellClientBuilder, vmConfig.Name, pingConfig)
+	err = v.WaitVmReady(ctx, tools.ShellClientBuilder, vmConfig.Name, readyConfig)
 	if err != nil {
 		return err
 	}
@@ -588,7 +588,7 @@ func (v *Virter) imageBuildProvisionCommit(ctx context.Context, tools ImageBuild
 }
 
 // ImageBuild builds an image by running a VM and provisioning it.
-func (v *Virter) ImageBuild(ctx context.Context, tools ImageBuildTools, vmConfig VMConfig, pingConfig SSHPingConfig, buildConfig ImageBuildConfig, opts ...LayerOperationOption) error {
+func (v *Virter) ImageBuild(ctx context.Context, tools ImageBuildTools, vmConfig VMConfig, readyConfig VmReadyConfig, buildConfig ImageBuildConfig, opts ...LayerOperationOption) error {
 	// VMRun is responsible to call CheckVMConfig here!
 	// TODO(): currently we can not know why VM run failed, so we don't clean up in this stage,
 	//         it could have been an existing VM, we don't want to delete it.
@@ -598,7 +598,7 @@ func (v *Virter) ImageBuild(ctx context.Context, tools ImageBuildTools, vmConfig
 	}
 
 	// from here on it is safe to rm the VM if something fails
-	err = v.imageBuildProvisionCommit(ctx, tools, vmConfig, pingConfig, buildConfig, opts...)
+	err = v.imageBuildProvisionCommit(ctx, tools, vmConfig, readyConfig, buildConfig, opts...)
 	if err != nil {
 		log.Warn("could not build image, deleting VM")
 		if rmErr := v.VMRm(vmConfig.Name, !vmConfig.StaticDHCP, true); rmErr != nil {

internal/virter/virter.go

@@ -152,10 +152,10 @@ type VMMeta struct {
 	HostKey string `xml:"hostkey"`
 }
 
-// SSHPingConfig contains the configuration for pinging a VM via SSH
-type SSHPingConfig struct {
-	SSHPingCount  int
-	SSHPingPeriod time.Duration
+// VmReadyConfig contains the configuration for waiting for a VM to be ready.
+type VmReadyConfig struct {
+	Retries      int
+	CheckTimeout time.Duration
 }
 
 func checkDisks(vmConfig VMConfig) error {

internal/virter/vm.go

@@ -169,8 +169,8 @@ func diskVolumeName(vmName, diskName string) string {
 	return vmName + "-" + diskName
 }
 
-// PingSSH repeatedly tries to connect to a VM via SSH until it succeeds
-func (v *Virter) PingSSH(ctx context.Context, shellClientBuilder ShellClientBuilder, vmName string, pingConfig SSHPingConfig) error {
+// WaitVmReady repeatedly tries to connect to a VM and checks if it's ready to be used.
+func (v *Virter) WaitVmReady(ctx context.Context, shellClientBuilder ShellClientBuilder, vmName string, readyConfig VmReadyConfig) error {
 	ips, err := v.getIPs([]string{vmName})
 	if err != nil {
 		return err
@@ -190,41 +190,37 @@ func (v *Virter) PingSSH(ctx context.Context, shellClientBuilder ShellClientBuil
 
 	sshConfig := ssh.ClientConfig{
 		Auth:              v.sshkeys.Auth(),
-		Timeout:           pingConfig.SSHPingPeriod,
+		Timeout:           readyConfig.CheckTimeout,
 		User:              "root",
 		HostKeyCallback:   hostkeyCheck,
 		HostKeyAlgorithms: supportedAlgos,
 	}
 
-	sshTry := func() error {
-		return tryDialSSH(ctx, shellClientBuilder, hostPort, sshConfig)
+	readyFunc := func() error {
+		sshClient := shellClientBuilder.NewShellClient(hostPort, sshConfig)
+		if err := sshClient.DialContext(ctx); err != nil {
+			log.Debugf("SSH dial attempt failed: %v", err)
+			return err
+		}
+		defer sshClient.Close()
+		if err := sshClient.ExecScript("test -f /run/cloud-init/result.json"); err != nil {
+			log.Debugf("cloud-init not done: %v", err)
+			return err
+		}
+
+		return nil
 	}
 
-	log.Print("Wait for SSH port to open")
+	log.Print("Wait for VM to get ready")
 
 	// Using ActualTime breaks the expectation of the unit tests
 	// that this code does not sleep, but we work around that by
 	// always making the first ping successful in tests
-	if err := (actualtime.ActualTime{}.Ping(ctx, pingConfig.SSHPingCount, pingConfig.SSHPingPeriod, sshTry)); err != nil {
-		return fmt.Errorf("unable to connect to SSH port: %w", err)
-	}
-
-	log.Print("Successfully connected to SSH port")
-	return nil
-}
-
-func tryDialSSH(ctx context.Context, shellClientBuilder ShellClientBuilder, hostPort string, sshConfig ssh.ClientConfig) error {
-	sshClient := shellClientBuilder.NewShellClient(hostPort, sshConfig)
-	if err := sshClient.DialContext(ctx); err != nil {
-		log.Debugf("SSH dial attempt failed: %v", err)
-		return err
-	}
-	defer sshClient.Close()
-	if err := sshClient.ExecScript("test -f /run/cloud-init/result.json"); err != nil {
-		log.Debugf("cloud-init not done: %v", err)
-		return err
+	if err := (actualtime.ActualTime{}.Ping(ctx, readyConfig.Retries, readyConfig.CheckTimeout, readyFunc)); err != nil {
+		return fmt.Errorf("VM not ready: %w", err)
 	}
 
+	log.Print("Successfully connected to ready VM")
 	return nil
 }
 

internal/virter/vm_test.go

@@ -76,15 +76,15 @@ func TestVMRun(t *testing.T) {
 	assert.True(t, domain.active)
 }
 
-func TestSSHPing(t *testing.T) {
+func TestWaitVmReady(t *testing.T) {
 	shell := new(mocks.ShellClient)
 	shell.On("DialContext", mock.Anything).Return(nil)
 	shell.On("Close").Return(nil)
 	shell.On("ExecScript", mock.Anything).Return(nil)
 
-	sshPingConfig := virter.SSHPingConfig{
-		SSHPingCount:  1,
-		SSHPingPeriod: time.Second, // ignored
+	readyConfig := virter.VmReadyConfig{
+		Retries:      1,
+		CheckTimeout: time.Second, // ignored
 	}
 
 	l := newFakeLibvirtConnection()
@@ -96,7 +96,7 @@ func TestSSHPing(t *testing.T) {
 
 	v := virter.New(l, poolName, networkName, newMockKeystore())
 
-	err := v.PingSSH(context.Background(), MockShellClientBuilder{shell}, vmName, sshPingConfig)
+	err := v.WaitVmReady(context.Background(), MockShellClientBuilder{shell}, vmName, readyConfig)
 	assert.NoError(t, err)
 
 	shell.AssertExpectations(t)