chore: changes after first review

Signed-off-by: Richard Case <richard.case@outlook.com>

README.md

@@ -55,7 +55,8 @@ The table below shows you which versions of Firecracker are compatible with Flin
 
 | Flintlock         | Firecracker                      | Cloud Hypervisor  |
 | ----------------- | -------------------------------- | ----------------- |
-|    v0.4.0         | Official v1.0+ or v1.0.0-macvtap | v26.0             |
+|    v0.5.0         | Official v1.0+ or v1.0.0-macvtap | v26.0             |
+|    v0.4.0         | Official v1.0+ or v1.0.0-macvtap | **Not Supported** |
 |    v0.3.0         | Official v1.0+ or v1.0.0-macvtap | **Not Supported** |
 | <= v0.2.0         | <= v0.25.2-macvtap               | **Not Supported** |
 | <= v0.1.0-alpha.6 | <= v0.25.2-macvtap               | **Not Supported** |

infrastructure/microvm/cloudhypervisor/cloudinit.go

@@ -10,6 +10,10 @@ import (
 	"github.com/weaveworks-liquidmetal/flintlock/infrastructure/microvm/shared"
 )
 
+const (
+	cloudInitDiskSize = "8Mb"
+)
+
 func (p *provider) createCloudInitImage(ctx context.Context, vm *models.MicroVM, state State) error {
 	imagePath := state.CloudInitImage()
 
@@ -23,8 +27,7 @@ func (p *provider) createCloudInitImage(ctx context.Context, vm *models.MicroVM,
 
 	files := []ports.DiskFile{}
 	for k, v := range vm.Spec.Metadata {
-		cloudInitKey := isCloudInitKey(k)
-		if !cloudInitKey {
+		if !isCloudInitKey(k) {
 			continue
 		}
 
@@ -37,7 +40,7 @@ func (p *provider) createCloudInitImage(ctx context.Context, vm *models.MicroVM,
 
 	input := ports.DiskCreateInput{
 		Path:       imagePath,
-		Size:       "8Mb",
+		Size:       cloudInitDiskSize,
 		VolumeName: cloudinit.VolumeName,
 		Type:       ports.DiskTypeFat32,
 		Overwrite:  true,

infrastructure/microvm/cloudhypervisor/create.go

@@ -70,8 +70,6 @@ func (p *provider) startCloudHypervisor(ctx context.Context, vm *models.MicroVM,
 	cmd.Stdout = stdOutFile
 	cmd.Stdin = &bytes.Buffer{}
 
-	fmt.Printf("%s %s\n", cmd.Path, cmd.Args)
-
 	var startErr error
 	if detached {
 		startErr = process.DetachedStart(cmd)
@@ -87,27 +85,12 @@ func (p *provider) startCloudHypervisor(ctx context.Context, vm *models.MicroVM,
 }
 
 func (p *provider) buildArgs(vm *models.MicroVM, state State) ([]string, error) {
-	// This works:
-	// --api-socket /var/lib/flintlock/vm/default/hardcore_carver6/01G16F6FJMMZ96E7760F5SH9AM/cloudhypervisor.sock
-	// --kernel /home/richard/code/ww/image-builder/flintlock/kernel/linux-cloud-hypervisor/arch/x86/boot/compressed/vmlinux.bin
-	// --disk path=/dev/mapper/flintlock-thinpool-snap-659
-	// --cpus boot=4
-	// --memory size=2048M
-	// --net "tap=,mac=,ip=,mask="
-	// --cmdline  "console=hvc0 root=/dev/vda rw" -v
 	args := []string{
 		"--api-socket",
 		state.SockPath(),
 		"--log-file",
 		state.LogPath(),
 		"-v",
-		// Testing tty
-		//"--console",
-		//"pty",
-		//"--serial",
-		//"tty",
-		//"--platform",
-		//fmt.Sprintf("serial_number=%s", vm.ID.UID()),
 	}
 
 	// Kernel and cmdline args
@@ -117,7 +100,6 @@ func (p *provider) buildArgs(vm *models.MicroVM, state State) ([]string, error)
 		kernelCmdLine.Set(key, value)
 	}
 
-	//args = append(args, "--cmdline", fmt.Sprintf("\"%s\"", kernelCmdLine.String()))
 	args = append(args, "--cmdline", kernelCmdLine.String())
 	args = append(args, "--kernel", fmt.Sprintf("%s/%s", vm.Status.KernelMount.Source, vm.Spec.Kernel.Filename))
 
@@ -210,16 +192,5 @@ func (p *provider) ensureState(vmState State) error {
 
 	logFile.Close()
 
-	// metadataDirExists, err := afero.DirExists(p.fs, vmState.MetadataDir())
-	// if err != nil {
-	// 	return fmt.Errorf("checking if metadata dir exists: %w", err)
-	// }
-
-	// if !metadataDirExists {
-	// 	if err = p.fs.MkdirAll(vmState.MetadataDir(), defaults.DataDirPerm); err != nil {
-	// 		return fmt.Errorf("creating metadata directory %s: %w", vmState.MetadataDir(), err)
-	// 	}
-	// }
-
 	return nil
 }

infrastructure/microvm/cloudhypervisor/delete.go

@@ -16,6 +16,11 @@ import (
 	"github.com/weaveworks-liquidmetal/flintlock/pkg/process"
 )
 
+const (
+	shutdownTimeOutSeconds  = 30
+	shutdownCheckIntervalMS = 500
+)
+
 // Stop will stop a running microvm.
 func (p *provider) Delete(ctx context.Context, id string) error {
 	logger := log.GetLogger(ctx).WithFields(logrus.Fields{
@@ -45,28 +50,30 @@ func (p *provider) Delete(ctx context.Context, id string) error {
 	}
 
 	chClient := cloudhypervisor.New(vmState.SockPath())
-	err = chClient.Shutdown(ctx)
-	if err != nil {
-		return fmt.Errorf("shutting down cloud-hypervisor vm: %w", err)
+
+	if shutdownErr := chClient.Shutdown(ctx); shutdownErr != nil {
+		return fmt.Errorf("shutting down cloud-hypervisor vm: %w", shutdownErr)
 	}
 
 	shutdownFunc := func() (bool, error) {
 		info, infoErr := chClient.Info(ctx)
 		if infoErr != nil {
-			return false, err
+			return false, infoErr
 		}
 
 		return info.State == cloudhypervisor.VmStateShutdown || info.State == cloudhypervisor.VmStateCreated, nil
 	}
-	shutDownTimeout := 30 * time.Second
-	checkInterval := 500 * time.Millisecond
+
+	shutDownTimeout := shutdownTimeOutSeconds * time.Second
+	checkInterval := shutdownCheckIntervalMS * time.Millisecond
+
 	if waitErr := wait.ForCondition(shutdownFunc, shutDownTimeout, checkInterval); waitErr != nil {
 		if !errors.Is(waitErr, wait.ErrWaitTimeout) {
-			return fmt.Errorf("waiting for vm shutdown: %w", err)
+			return fmt.Errorf("waiting for vm shutdown: %w", waitErr)
 		}
 	}
 
-	logger.Infof("sending SIGHUP to %d", pid)
+	logger.Debugf("sending SIGHUP to %d", pid)
 
 	if sigErr := process.SendSignal(pid, syscall.SIGHUP); sigErr != nil {
 		return fmt.Errorf("failed to terminate with SIGHUP: %w", sigErr)

infrastructure/microvm/cloudhypervisor/provider.go

@@ -112,17 +112,18 @@ func (p *provider) State(ctx context.Context, id string) (ports.MicroVMState, er
 	}
 
 	chClient := cloudhypervisor.New(vmState.SockPath())
+
 	vmInfo, err := chClient.Info(ctx)
 	if err != nil {
 		return ports.MicroVMStateUnknown, fmt.Errorf("querying cloud-hypervisor for info: %w", err)
 	}
 
-	//TODO: support paused in the future
+	//NOTE: we can support paused/unpause in the future
 	switch vmInfo.State {
 	case cloudhypervisor.VmStateRunning:
 		return ports.MicroVMStateRunning, nil
 	case cloudhypervisor.VmStateCreated:
-		return ports.MicroVMStatePending, nil //TODO: or should this be created???
+		return ports.MicroVMStatePending, nil
 	default:
 		return ports.MicroVMStateUnknown, fmt.Errorf("cloud-hypervisor in an unsupported state: %s", vmInfo.State)
 	}

infrastructure/microvm/cloudhypervisor/state.go

@@ -1,15 +1,20 @@
 package cloudhypervisor
 
 import (
-	"bytes"
 	"fmt"
-	"io/ioutil"
-	"os"
-	"strconv"
 
 	"github.com/spf13/afero"
 	"github.com/weaveworks-liquidmetal/flintlock/core/models"
-	"github.com/weaveworks-liquidmetal/flintlock/pkg/defaults"
+	"github.com/weaveworks-liquidmetal/flintlock/infrastructure/microvm/shared"
+)
+
+const (
+	pidFileName       = "cloudhypervisor.pid"
+	logFileName       = "cloudhypervisor.log"
+	stdOutFileName    = "cloudhypervisor.stdout"
+	stdErrFileName    = "cloudhypervisor.stderr"
+	socketFileName    = "cloudhypervisor.sock"
+	cloudInitFileName = "cloud-init.img"
 )
 
 type State interface {
@@ -24,7 +29,6 @@ type State interface {
 	StderrPath() string
 	SockPath() string
 
-	//MetadataDir() string
 	CloudInitImage() string
 }
 
@@ -45,72 +49,33 @@ func (s *fsState) Root() string {
 }
 
 func (s *fsState) PIDPath() string {
-	return fmt.Sprintf("%s/cloudhypervisor.pid", s.stateRoot)
+	return fmt.Sprintf("%s/%s", s.stateRoot, pidFileName)
 }
 
 func (s *fsState) PID() (int, error) {
-	return s.pidReadFromFile(s.PIDPath())
+	return shared.PIDReadFromFile(s.PIDPath(), s.fs)
 }
 
 func (s *fsState) LogPath() string {
-	return fmt.Sprintf("%s/cloudhypervisor.log", s.stateRoot)
+	return fmt.Sprintf("%s/%s", s.stateRoot, logFileName)
 }
 
 func (s *fsState) StdoutPath() string {
-	return fmt.Sprintf("%s/cloudhypervisor.stdout", s.stateRoot)
+	return fmt.Sprintf("%s/%s", s.stateRoot, stdOutFileName)
 }
 
 func (s *fsState) StderrPath() string {
-	return fmt.Sprintf("%s/cloudhypervisor.stderr", s.stateRoot)
+	return fmt.Sprintf("%s/%s", s.stateRoot, stdErrFileName)
 }
 
 func (s *fsState) SockPath() string {
-	return fmt.Sprintf("%s/cloudhypervisor.sock", s.stateRoot)
+	return fmt.Sprintf("%s/%s", s.stateRoot, socketFileName)
 }
 
-// func (s *fsState) MetadataDir() string {
-// 	return fmt.Sprintf("%s/metadata", s.stateRoot)
-// }
-
 func (s *fsState) CloudInitImage() string {
-	return fmt.Sprintf("%s/cloud-init.img", s.stateRoot)
+	return fmt.Sprintf("%s/%s", s.stateRoot, cloudInitFileName)
 }
 
 func (s *fsState) SetPid(pid int) error {
-	return s.pidWriteToFile(pid, s.PIDPath())
-}
-
-func (s *fsState) pidReadFromFile(pidFile string) (int, error) {
-	file, err := s.fs.Open(pidFile)
-	if err != nil {
-		return -1, fmt.Errorf("opening pid file %s: %w", pidFile, err)
-	}
-
-	data, err := ioutil.ReadAll(file)
-	if err != nil {
-		return -1, fmt.Errorf("reading pid file %s: %w", pidFile, err)
-	}
-
-	pid, err := strconv.Atoi(string(bytes.TrimSpace(data)))
-	if err != nil {
-		return -1, fmt.Errorf("converting data to int: %w", err)
-	}
-
-	return pid, nil
-}
-
-func (s *fsState) pidWriteToFile(pid int, pidFile string) error {
-	file, err := s.fs.OpenFile(pidFile, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, defaults.DataFilePerm)
-	if err != nil {
-		return fmt.Errorf("opening pid file %s: %w", pidFile, err)
-	}
-
-	defer file.Close()
-
-	_, err = fmt.Fprintf(file, "%d", pid)
-	if err != nil {
-		return fmt.Errorf("writing pid %d to file %s: %w", pid, pidFile, err)
-	}
-
-	return nil
+	return shared.PIDWriteToFile(pid, s.PIDPath(), s.fs)
 }

infrastructure/microvm/firecracker/state.go

@@ -1,16 +1,15 @@
 package firecracker
 
 import (
-	"bytes"
 	"encoding/base64"
 	"encoding/json"
 	"fmt"
 	"io/ioutil"
 	"os"
-	"strconv"
 
 	"github.com/spf13/afero"
 	"github.com/weaveworks-liquidmetal/flintlock/core/models"
+	"github.com/weaveworks-liquidmetal/flintlock/infrastructure/microvm/shared"
 	"github.com/weaveworks-liquidmetal/flintlock/pkg/defaults"
 )
 
@@ -56,7 +55,7 @@ func (s *fsState) PIDPath() string {
 }
 
 func (s *fsState) PID() (int, error) {
-	return s.pidReadFromFile(s.PIDPath())
+	return shared.PIDReadFromFile(s.PIDPath(), s.fs)
 }
 
 func (s *fsState) LogPath() string {
@@ -76,7 +75,7 @@ func (s *fsState) StderrPath() string {
 }
 
 func (s *fsState) SetPid(pid int) error {
-	return s.pidWriteToFile(pid, s.PIDPath())
+	return shared.PIDWriteToFile(pid, s.PIDPath(), s.fs)
 }
 
 func (s *fsState) ConfigPath() string {
@@ -142,41 +141,6 @@ func (s *fsState) MetadataPath() string {
 	return fmt.Sprintf("%s/metadata.json", s.stateRoot)
 }
 
-func (s *fsState) pidReadFromFile(pidFile string) (int, error) {
-	file, err := s.fs.Open(pidFile)
-	if err != nil {
-		return -1, fmt.Errorf("opening pid file %s: %w", pidFile, err)
-	}
-
-	data, err := ioutil.ReadAll(file)
-	if err != nil {
-		return -1, fmt.Errorf("reading pid file %s: %w", pidFile, err)
-	}
-
-	pid, err := strconv.Atoi(string(bytes.TrimSpace(data)))
-	if err != nil {
-		return -1, fmt.Errorf("converting data to int: %w", err)
-	}
-
-	return pid, nil
-}
-
-func (s *fsState) pidWriteToFile(pid int, pidFile string) error {
-	file, err := s.fs.OpenFile(pidFile, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, defaults.DataFilePerm)
-	if err != nil {
-		return fmt.Errorf("opening pid file %s: %w", pidFile, err)
-	}
-
-	defer file.Close()
-
-	_, err = fmt.Fprintf(file, "%d", pid)
-	if err != nil {
-		return fmt.Errorf("writing pid %d to file %s: %w", pid, pidFile, err)
-	}
-
-	return nil
-}
-
 func (s *fsState) readJSONFile(cfg interface{}, inputFile string) error {
 	file, err := s.fs.Open(inputFile)
 	if err != nil {

infrastructure/microvm/providers.go

@@ -25,15 +25,13 @@ func New(name string, cfg *config.Config, networkSvc ports.NetworkService, diskS
 		return cloudhypervisor.New(cloudHypervisorConfig(cfg), networkSvc, diskSvc, fs), nil
 	default:
 		return nil, errUnknownProvider
-
 	}
 }
 
 // NewFromConfig will create instances of the vm providers based on the config.
 func NewFromConfig(cfg *config.Config, networkSvc ports.NetworkService, diskSvc ports.DiskService, fs afero.Fs) (map[string]ports.MicroVMService, error) {
 	providers := map[string]ports.MicroVMService{}
 
-	//TODO: handle this better, disable flags????
 	if cfg.CloudHypervisorBin != "" {
 		providers[cloudhypervisor.ProviderName] = cloudhypervisor.New(cloudHypervisorConfig(cfg), networkSvc, diskSvc, fs)
 	}