From 4cfa8ccb27ba642fc78b37fa733fcd0d6271ad0f Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Thu, 7 Jul 2022 14:06:05 +0200 Subject: machine test: fix endless loop in test The problem is that this could loop forever as long as podman start doe snot exit (which could happen due bugs). Also since there no timeout between the machine list calls the test is using the full cpu and this causes the system to slow down making the machine start command even slower. IMO it is enough to only check the status every three seconds. Signed-off-by: Paul Holzinger --- pkg/machine/e2e/list_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'pkg/machine') diff --git a/pkg/machine/e2e/list_test.go b/pkg/machine/e2e/list_test.go index e2121e7bf..f12fcbd11 100644 --- a/pkg/machine/e2e/list_test.go +++ b/pkg/machine/e2e/list_test.go @@ -2,6 +2,7 @@ package e2e import ( "strings" + "time" "github.com/containers/common/pkg/util" "github.com/containers/podman/v4/cmd/podman/machine" @@ -87,7 +88,7 @@ var _ = Describe("podman machine list", func() { startSession, err := mb.setCmd(s).runWithoutWait() Expect(err).To(BeNil()) l := new(listMachine) - for { // needs to be infinite because we need to check if running when inspect returns to avoid race conditions. + for i := 0; i < 30; i++ { listSession, err := mb.setCmd(l).run() Expect(listSession).To(Exit(0)) Expect(err).To(BeNil()) @@ -96,6 +97,7 @@ var _ = Describe("podman machine list", func() { } else { break } + time.Sleep(3 * time.Second) } Expect(startSession).To(Exit(0)) listSession, err := mb.setCmd(l).run() -- cgit v1.2.3-54-g00ecf From 4374038cc67405e3f5555b1870d5bb7f6570fa5d Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Thu, 7 Jul 2022 14:13:26 +0200 Subject: pkg/machine/e2e: do not import from cmd/podman It should be avoided to import cmd/podman/... packages from outside of cmd/podman. This can lead in weird hard to debug import paths but also can have negative consequences when imported in unit tests. In this case it will set XDG_CONFIG_HOME and thus the machine tests this dir over the tmp HOME env variable which is set at a later point. This caused machine files to be leaked into the actual users home dir. Signed-off-by: Paul Holzinger --- cmd/podman/machine/list.go | 36 ++++++++++-------------------------- pkg/domain/entities/machine.go | 18 ++++++++++++++++++ pkg/machine/e2e/list_test.go | 4 ++-- 3 files changed, 30 insertions(+), 28 deletions(-) create mode 100644 pkg/domain/entities/machine.go (limited to 'pkg/machine') diff --git a/cmd/podman/machine/list.go b/cmd/podman/machine/list.go index b1e31566f..dd4a86697 100644 --- a/cmd/podman/machine/list.go +++ b/cmd/podman/machine/list.go @@ -16,6 +16,7 @@ import ( "github.com/containers/podman/v4/cmd/podman/common" "github.com/containers/podman/v4/cmd/podman/registry" "github.com/containers/podman/v4/cmd/podman/validate" + "github.com/containers/podman/v4/pkg/domain/entities" "github.com/containers/podman/v4/pkg/machine" "github.com/docker/go-units" "github.com/spf13/cobra" @@ -44,23 +45,6 @@ type listFlagType struct { quiet bool } -type ListReporter struct { - Name string - Default bool - Created string - Running bool - Starting bool - LastUp string - Stream string - VMType string - CPUs uint64 - Memory string - DiskSize string - Port int - RemoteUsername string - IdentityPath string -} - func init() { registry.Commands = append(registry.Commands, registry.CliCommand{ Command: lsCmd, @@ -70,7 +54,7 @@ func init() { flags := lsCmd.Flags() formatFlagName := "format" flags.StringVar(&listFlag.format, formatFlagName, "{{.Name}}\t{{.VMType}}\t{{.Created}}\t{{.LastUp}}\t{{.CPUs}}\t{{.Memory}}\t{{.DiskSize}}\n", "Format volume output using JSON or a Go template") - _ = lsCmd.RegisterFlagCompletionFunc(formatFlagName, common.AutocompleteFormat(&ListReporter{})) + _ = lsCmd.RegisterFlagCompletionFunc(formatFlagName, common.AutocompleteFormat(&entities.ListReporter{})) flags.BoolVar(&listFlag.noHeading, "noheading", false, "Do not print headers") flags.BoolVarP(&listFlag.quiet, "quiet", "q", false, "Show only machine names") } @@ -123,8 +107,8 @@ func list(cmd *cobra.Command, args []string) error { return outputTemplate(cmd, machineReporter) } -func outputTemplate(cmd *cobra.Command, responses []*ListReporter) error { - headers := report.Headers(ListReporter{}, map[string]string{ +func outputTemplate(cmd *cobra.Command, responses []*entities.ListReporter) error { + headers := report.Headers(entities.ListReporter{}, map[string]string{ "LastUp": "LAST UP", "VmType": "VM TYPE", "CPUs": "CPUS", @@ -183,15 +167,15 @@ func streamName(imageStream string) string { return imageStream } -func toMachineFormat(vms []*machine.ListResponse) ([]*ListReporter, error) { +func toMachineFormat(vms []*machine.ListResponse) ([]*entities.ListReporter, error) { cfg, err := config.ReadCustomConfig() if err != nil { return nil, err } - machineResponses := make([]*ListReporter, 0, len(vms)) + machineResponses := make([]*entities.ListReporter, 0, len(vms)) for _, vm := range vms { - response := new(ListReporter) + response := new(entities.ListReporter) response.Default = vm.Name == cfg.Engine.ActiveService response.Name = vm.Name response.Running = vm.Running @@ -211,15 +195,15 @@ func toMachineFormat(vms []*machine.ListResponse) ([]*ListReporter, error) { return machineResponses, nil } -func toHumanFormat(vms []*machine.ListResponse) ([]*ListReporter, error) { +func toHumanFormat(vms []*machine.ListResponse) ([]*entities.ListReporter, error) { cfg, err := config.ReadCustomConfig() if err != nil { return nil, err } - humanResponses := make([]*ListReporter, 0, len(vms)) + humanResponses := make([]*entities.ListReporter, 0, len(vms)) for _, vm := range vms { - response := new(ListReporter) + response := new(entities.ListReporter) if vm.Name == cfg.Engine.ActiveService { response.Name = vm.Name + "*" response.Default = true diff --git a/pkg/domain/entities/machine.go b/pkg/domain/entities/machine.go new file mode 100644 index 000000000..6ba53dbd1 --- /dev/null +++ b/pkg/domain/entities/machine.go @@ -0,0 +1,18 @@ +package entities + +type ListReporter struct { + Name string + Default bool + Created string + Running bool + Starting bool + LastUp string + Stream string + VMType string + CPUs uint64 + Memory string + DiskSize string + Port int + RemoteUsername string + IdentityPath string +} diff --git a/pkg/machine/e2e/list_test.go b/pkg/machine/e2e/list_test.go index f12fcbd11..9e3e9956c 100644 --- a/pkg/machine/e2e/list_test.go +++ b/pkg/machine/e2e/list_test.go @@ -5,7 +5,7 @@ import ( "time" "github.com/containers/common/pkg/util" - "github.com/containers/podman/v4/cmd/podman/machine" + "github.com/containers/podman/v4/pkg/domain/entities" jsoniter "github.com/json-iterator/go" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" @@ -134,7 +134,7 @@ var _ = Describe("podman machine list", func() { Expect(err).To(BeNil()) Expect(listSession2).To(Exit(0)) - var listResponse []*machine.ListReporter + var listResponse []*entities.ListReporter err = jsoniter.Unmarshal(listSession.Bytes(), &listResponse) Expect(err).To(BeNil()) -- cgit v1.2.3-54-g00ecf From a26cf638e070321ec529fce058f1cf1935f36708 Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Thu, 7 Jul 2022 19:00:13 +0200 Subject: machine: qemu fix chardev id starting with letter qemu need the id to start with a letter for some reason. If this is not the case qemu will fail: ``` qemu-system-x86_64: -device virtserialport,chardev=ad053e0bb519f_ready,name=org.fedoraproject.port.0: Property 'virtserialport.chardev' can't find value 'ad053e0bb519f_ready' er Identifiers consist of letters, digits, '-', '.', '_', starting with a letter. ``` To fix this we just add an "a" in front of it. Signed-off-by: Paul Holzinger --- pkg/machine/qemu/machine.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'pkg/machine') diff --git a/pkg/machine/qemu/machine.go b/pkg/machine/qemu/machine.go index 322aa3a15..054960e18 100644 --- a/pkg/machine/qemu/machine.go +++ b/pkg/machine/qemu/machine.go @@ -138,8 +138,10 @@ func (p *Provider) NewMachine(opts machine.InitOptions) (machine.VM, error) { cmd = append(cmd, []string{ "-device", "virtio-serial", // qemu needs to establish the long name; other connections can use the symlink'd - "-chardev", "socket,path=" + vm.ReadySocket.Path + ",server=on,wait=off,id=" + vm.Name + "_ready", - "-device", "virtserialport,chardev=" + vm.Name + "_ready" + ",name=org.fedoraproject.port.0", + // Note both id and chardev start with an extra "a" becuase qemu requires that it + // starts with an letter but users can also use numbers + "-chardev", "socket,path=" + vm.ReadySocket.Path + ",server=on,wait=off,id=a" + vm.Name + "_ready", + "-device", "virtserialport,chardev=a" + vm.Name + "_ready" + ",name=org.fedoraproject.port.0", "-pidfile", vm.VMPidFilePath.GetPath()}...) vm.CmdLine = cmd return vm, nil -- cgit v1.2.3-54-g00ecf From 61a67a07b1f6d7ab0a358ee19da90b9392900f49 Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Thu, 7 Jul 2022 19:30:51 +0200 Subject: pkg/machine/qemu: start VM check if qemu is alive When trying to connect to the qemu ready socket we should check if the qemu process is still running, if it is not we can just error out. There is no point in retrying. To do so we have to directly call wait with WNOHANG. Also change StartProcess to os/exec package which is higher level and allows us to use a buffer as qemu stderr fd. Signed-off-by: Paul Holzinger --- pkg/machine/qemu/machine.go | 35 +++++++++++++++++++++++++++++------ 1 file changed, 29 insertions(+), 6 deletions(-) (limited to 'pkg/machine') diff --git a/pkg/machine/qemu/machine.go b/pkg/machine/qemu/machine.go index 054960e18..6134e69e1 100644 --- a/pkg/machine/qemu/machine.go +++ b/pkg/machine/qemu/machine.go @@ -5,6 +5,7 @@ package qemu import ( "bufio" + "bytes" "context" "encoding/base64" "encoding/json" @@ -138,7 +139,7 @@ func (p *Provider) NewMachine(opts machine.InitOptions) (machine.VM, error) { cmd = append(cmd, []string{ "-device", "virtio-serial", // qemu needs to establish the long name; other connections can use the symlink'd - // Note both id and chardev start with an extra "a" becuase qemu requires that it + // Note both id and chardev start with an extra "a" because qemu requires that it // starts with an letter but users can also use numbers "-chardev", "socket,path=" + vm.ReadySocket.Path + ",server=on,wait=off,id=a" + vm.Name + "_ready", "-device", "virtserialport,chardev=a" + vm.Name + "_ready" + ",name=org.fedoraproject.port.0", @@ -573,15 +574,25 @@ func (v *MachineVM) Start(name string, _ machine.StartOptions) error { files := []*os.File{dnr, dnw, dnw, fd} attr.Files = files logrus.Debug(v.CmdLine) - cmd := v.CmdLine + cmdLine := v.CmdLine // Disable graphic window when not in debug mode // Done in start, so we're not suck with the debug level we used on init if !logrus.IsLevelEnabled(logrus.DebugLevel) { - cmd = append(cmd, "-display", "none") + cmdLine = append(cmdLine, "-display", "none") } - _, err = os.StartProcess(v.CmdLine[0], cmd, attr) + stderrBuf := &bytes.Buffer{} + + cmd := &exec.Cmd{ + Args: cmdLine, + Path: cmdLine[0], + Stdin: dnr, + Stdout: dnw, + Stderr: stderrBuf, + ExtraFiles: []*os.File{fd}, + } + err = cmd.Start() if err != nil { // check if qemu was not found if !errors.Is(err, os.ErrNotExist) { @@ -592,15 +603,17 @@ func (v *MachineVM) Start(name string, _ machine.StartOptions) error { if err != nil { return err } - cmd[0], err = cfg.FindHelperBinary(QemuCommand, true) + cmdLine[0], err = cfg.FindHelperBinary(QemuCommand, true) if err != nil { return err } - _, err = os.StartProcess(cmd[0], cmd, attr) + cmd.Path = cmdLine[0] + err = cmd.Start() if err != nil { return fmt.Errorf("unable to execute %q: %w", cmd, err) } } + defer cmd.Process.Release() //nolint:errcheck fmt.Println("Waiting for VM ...") socketPath, err := getRuntimeDir() if err != nil { @@ -615,6 +628,16 @@ func (v *MachineVM) Start(name string, _ machine.StartOptions) error { if err == nil { break } + // check if qemu is still alive + var status syscall.WaitStatus + pid, err := syscall.Wait4(cmd.Process.Pid, &status, syscall.WNOHANG, nil) + if err != nil { + return fmt.Errorf("failed to read qemu process status: %w", err) + } + if pid > 0 { + // child exited + return fmt.Errorf("qemu exited unexpectedly with exit code %d, stderr: %s", status.ExitStatus(), stderrBuf.String()) + } time.Sleep(wait) wait++ } -- cgit v1.2.3-54-g00ecf