From 41ec1d1dfa7425d4ea5cdf8c200f7036a5489a9c Mon Sep 17 00:00:00 2001 From: Richard Case Date: Sat, 7 Jan 2023 13:42:27 +0000 Subject: [PATCH] chore: changes after first review Signed-off-by: Richard Case --- README.md | 3 +- .../microvm/cloudhypervisor/cloudinit.go | 9 ++- .../microvm/cloudhypervisor/create.go | 29 -------- .../microvm/cloudhypervisor/delete.go | 23 +++--- .../microvm/cloudhypervisor/provider.go | 5 +- .../microvm/cloudhypervisor/state.go | 71 +++++-------------- infrastructure/microvm/firecracker/state.go | 42 +---------- infrastructure/microvm/providers.go | 2 - infrastructure/microvm/shared/pid.go | 47 ++++++++++++ internal/command/flags/flags.go | 2 +- pkg/defaults/defaults.go | 4 ++ 11 files changed, 99 insertions(+), 138 deletions(-) create mode 100644 infrastructure/microvm/shared/pid.go diff --git a/README.md b/README.md index abd9622b..28db9e95 100644 --- a/README.md +++ b/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** | diff --git a/infrastructure/microvm/cloudhypervisor/cloudinit.go b/infrastructure/microvm/cloudhypervisor/cloudinit.go index 86551271..5c491732 100644 --- a/infrastructure/microvm/cloudhypervisor/cloudinit.go +++ b/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, diff --git a/infrastructure/microvm/cloudhypervisor/create.go b/infrastructure/microvm/cloudhypervisor/create.go index 431b2d0f..7acee8e4 100644 --- a/infrastructure/microvm/cloudhypervisor/create.go +++ b/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 } diff --git a/infrastructure/microvm/cloudhypervisor/delete.go b/infrastructure/microvm/cloudhypervisor/delete.go index 65ac4ff7..fe615418 100644 --- a/infrastructure/microvm/cloudhypervisor/delete.go +++ b/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) diff --git a/infrastructure/microvm/cloudhypervisor/provider.go b/infrastructure/microvm/cloudhypervisor/provider.go index b2bfd6c9..c958642c 100644 --- a/infrastructure/microvm/cloudhypervisor/provider.go +++ b/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) } diff --git a/infrastructure/microvm/cloudhypervisor/state.go b/infrastructure/microvm/cloudhypervisor/state.go index ffdf79e8..f3df2164 100644 --- a/infrastructure/microvm/cloudhypervisor/state.go +++ b/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) } diff --git a/infrastructure/microvm/firecracker/state.go b/infrastructure/microvm/firecracker/state.go index a0cfe056..05d84ef9 100644 --- a/infrastructure/microvm/firecracker/state.go +++ b/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 { diff --git a/infrastructure/microvm/providers.go b/infrastructure/microvm/providers.go index 6709d50f..dbe13b57 100644 --- a/infrastructure/microvm/providers.go +++ b/infrastructure/microvm/providers.go @@ -25,7 +25,6 @@ func New(name string, cfg *config.Config, networkSvc ports.NetworkService, diskS return cloudhypervisor.New(cloudHypervisorConfig(cfg), networkSvc, diskSvc, fs), nil default: return nil, errUnknownProvider - } } @@ -33,7 +32,6 @@ func New(name string, cfg *config.Config, networkSvc ports.NetworkService, diskS 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) } diff --git a/infrastructure/microvm/shared/pid.go b/infrastructure/microvm/shared/pid.go new file mode 100644 index 00000000..491eec38 --- /dev/null +++ b/infrastructure/microvm/shared/pid.go @@ -0,0 +1,47 @@ +package shared + +import ( + "bytes" + "fmt" + "io/ioutil" + "os" + "strconv" + + "github.com/spf13/afero" + "github.com/weaveworks-liquidmetal/flintlock/pkg/defaults" +) + +func PIDReadFromFile(pidFile string, fs afero.Fs) (int, error) { + file, err := 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 PIDWriteToFile(pid int, pidFile string, fs afero.Fs) error { + file, err := 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 +} diff --git a/internal/command/flags/flags.go b/internal/command/flags/flags.go index 5f4359a3..2fa4c3a6 100644 --- a/internal/command/flags/flags.go +++ b/internal/command/flags/flags.go @@ -205,7 +205,7 @@ func addCloudHypervisorFlagsToCommand(cmd *cobra.Command, cfg *config.Config) er "The path to the cloud hypervisor binary to use.") cmd.Flags().BoolVar(&cfg.CloudHypervisorDetatch, cloudHypervisorDetachFlag, - true, + defaults.CloudHypervisorDetach, "If true the child cloud hypervisor processes will be detached from the parent flintlock process.") return nil diff --git a/pkg/defaults/defaults.go b/pkg/defaults/defaults.go index adc3f15a..a9b50760 100644 --- a/pkg/defaults/defaults.go +++ b/pkg/defaults/defaults.go @@ -30,6 +30,10 @@ const ( // CloudHypervisorBin is the name of the Cloud Hypervisor binary. CloudHypervisorBin = "cloud-hypervisor-static" + // CloudHypervisorDetach is the default for the flag to indicates with the child cloud-hypervisor + // processes should be run detached. + CloudHypervisorDetach = true + // ConfigurationDir is the default configuration directory. ConfigurationDir = "/etc/opt/flintlockd"