From 7208d41194daee3c27af951afea055ef038d950f Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Wed, 1 Apr 2020 14:08:47 +0000 Subject: [PATCH] mantle: Rework testiso/metal to use new qemu channel API First, add an API for creating channels that give a pipe to the Go side, so we don't need to write to temp files that we then read. Now, I'm still trying to fine-tune the interface between the "metal" API and the more hardcoded "testiso". Tweak things so that the install path supports being provided a builder object, but add an API that creates a default one that mostly just sets up the default max memory. (Also, I stopped trying to special case a lower memory for the legacy install because that's going to go away anyways) --- mantle/cmd/kola/testiso.go | 145 ++++++++++++++++++++++--------------- mantle/platform/metal.go | 68 +++++------------ mantle/platform/qemu.go | 26 ++++++- 3 files changed, 132 insertions(+), 107 deletions(-) diff --git a/mantle/cmd/kola/testiso.go b/mantle/cmd/kola/testiso.go index b864d5f496..5455ea873d 100644 --- a/mantle/cmd/kola/testiso.go +++ b/mantle/cmd/kola/testiso.go @@ -20,13 +20,17 @@ package main import ( + "bufio" "encoding/json" "fmt" "io/ioutil" + "math" "os" - "os/exec" "path/filepath" + "strings" + "time" + "github.com/coreos/mantle/system" "github.com/coreos/mantle/util" "github.com/pkg/errors" @@ -60,16 +64,18 @@ var ( console bool ) -var signalCompletionUnit = `[Unit] -Requires=dev-virtio\\x2dports-completion.device +var signalCompleteString = "coreos-installer-test-OK" +var signalCompletionUnit = fmt.Sprintf(`[Unit] +Requires=dev-virtio\\x2dports-testisocompletion.device OnFailure=emergency.target OnFailureJobMode=isolate [Service] Type=oneshot -ExecStart=/bin/sh -c '/usr/bin/echo coreos-installer-test-OK >/dev/virtio-ports/completion && systemctl poweroff' +RemainAfterExit=yes +ExecStart=/bin/sh -c '/usr/bin/echo %s >/dev/virtio-ports/testisocompletion && systemctl poweroff' [Install] RequiredBy=multi-user.target -` +`, signalCompleteString) func init() { cmdTestIso.Flags().BoolVarP(&instInsecure, "inst-insecure", "S", false, "Do not verify signature on metal image") @@ -82,6 +88,32 @@ func init() { root.AddCommand(cmdTestIso) } +func newQemuBuilder() *platform.QemuBuilder { + builder := platform.NewMetalQemuBuilderDefault() + builder.Firmware = kola.QEMUOptions.Firmware + sectorSize := 0 + if kola.QEMUOptions.Native4k { + sectorSize = 4096 + } + builder.AddPrimaryDisk(&platform.Disk{ + Size: "12G", // Arbitrary + + SectorSize: sectorSize, + }) + + // https://github.com/coreos/fedora-coreos-tracker/issues/388 + // https://github.com/coreos/fedora-coreos-docs/pull/46 + builder.Memory = 4096 + if system.RpmArch() == "s390x" { + // FIXME - determine why this is + builder.Memory = int(math.Max(float64(builder.Memory), 16384)) + } + + builder.InheritConsole = console + + return builder +} + func runTestIso(cmd *cobra.Command, args []string) error { if kola.CosaBuild == nil { return fmt.Errorf("Must provide --cosa-build") @@ -89,10 +121,7 @@ func runTestIso(cmd *cobra.Command, args []string) error { baseInst := platform.Install{ CosaBuild: kola.CosaBuild, - - Console: console, - Firmware: kola.QEMUOptions.Firmware, - Native4k: kola.QEMUOptions.Native4k, + Native4k: kola.QEMUOptions.Native4k, } if instInsecure { @@ -107,10 +136,6 @@ func runTestIso(cmd *cobra.Command, args []string) error { completionfile := filepath.Join(tmpd, "completion.txt") - baseInst.QemuArgs = []string{ - "-device", "virtio-serial", "-device", "virtserialport,chardev=completion,name=completion", - "-chardev", "file,id=completion,path=" + completionfile} - if kola.CosaBuild.Meta.BuildArtifacts.Metal == nil { return fmt.Errorf("Build %s must have a `metal` artifact", kola.CosaBuild.Meta.OstreeVersion) } @@ -124,7 +149,7 @@ func runTestIso(cmd *cobra.Command, args []string) error { inst := baseInst // Pretend this is Rust and I wrote .copy() inst.LegacyInstaller = true - if err := testPXE(inst, completionfile); err != nil { + if err := testPXE(inst); err != nil { return err } fmt.Printf("Successfully tested legacy installer for %s\n", kola.CosaBuild.Meta.OstreeVersion) @@ -142,7 +167,7 @@ func runTestIso(cmd *cobra.Command, args []string) error { ranTest = true instPxe := baseInst // Pretend this is Rust and I wrote .copy() - if err := testPXE(instPxe, completionfile); err != nil { + if err := testPXE(instPxe); err != nil { return err } printSuccess("PXE") @@ -165,6 +190,35 @@ func runTestIso(cmd *cobra.Command, args []string) error { return nil } +func awaitCompletion(inst *platform.QemuInstance, qchan *os.File, expected []string) error { + errchan := make(chan error) + go func() { + if err := inst.Wait(); err != nil { + errchan <- err + } + time.Sleep(1 * time.Minute) + errchan <- fmt.Errorf("QEMU exited; timed out waiting for completion") + }() + go func() { + r := bufio.NewReader(qchan) + for _, exp := range expected { + l, err := r.ReadString('\n') + if err != nil { + errchan <- errors.Wrapf(err, "reading from completion channel") + return + } + line := strings.TrimSpace(l) + if line != exp { + errchan <- fmt.Errorf("Unexpected string from completion channel: %s expected: %s", line, exp) + return + } + } + // OK! + errchan <- nil + }() + return <-errchan +} + func printSuccess(mode string) { metaltype := "metal" if kola.QEMUOptions.Native4k { @@ -173,9 +227,7 @@ func printSuccess(mode string) { fmt.Printf("Successfully tested %s live installer for %s on %s (%s)\n", mode, kola.CosaBuild.Meta.OstreeVersion, kola.QEMUOptions.Firmware, metaltype) } -func testPXE(inst platform.Install, completionfile string) error { - completionstamp := "coreos-installer-test-OK" - +func testPXE(inst platform.Install) error { config := ignv3types.Config{ Ignition: ignv3types.Ignition{ Version: "3.0.0", @@ -209,47 +261,43 @@ func testPXE(inst platform.Install, completionfile string) error { configStr = string(buf) } - mach, err := inst.PXE(nil, configStr) - if err != nil { - return errors.Wrapf(err, "running PXE") - } - defer mach.Destroy() - - err = mach.QemuInst.Wait() + inst.Builder = newQemuBuilder() + completionChannel, err := inst.Builder.VirtioChannelRead("testisocompletion") if err != nil { return err } - err = exec.Command("grep", "-q", "-e", completionstamp, completionfile).Run() - if err != nil { - return fmt.Errorf("Failed to find %s in %s: %s", completionstamp, completionfile, err) - } - - err = os.Remove(completionfile) + mach, err := inst.PXE(nil, configStr) if err != nil { - return errors.Wrapf(err, "removing %s", completionfile) + return errors.Wrapf(err, "running PXE") } + defer mach.Destroy() - return nil + return awaitCompletion(mach.QemuInst, completionChannel, []string{signalCompleteString}) } func testLiveIso(inst platform.Install, completionfile string) error { - completionstamp := "coreos-installer-test-OK" + inst.Builder = newQemuBuilder() + completionChannel, err := inst.Builder.VirtioChannelRead("testisocompletion") + if err != nil { + return err + } // We're just testing that executing our custom Ignition in the live // path worked ok. liveOKSignal := "live-test-OK" - var liveSignalOKUnit = `[Unit] - Requires=dev-virtio\\x2dports-completion.device + var liveSignalOKUnit = fmt.Sprintf(`[Unit] + Requires=dev-virtio\\x2dports-testisocompletion.device OnFailure=emergency.target OnFailureJobMode=isolate Before=coreos-installer.service [Service] Type=oneshot - ExecStart=/bin/sh -c '/usr/bin/echo live-test-OK >/dev/virtio-ports/completion' + RemainAfterExit=yes + ExecStart=/bin/sh -c '/usr/bin/echo %s >/dev/virtio-ports/testisocompletion' [Install] RequiredBy=multi-user.target - ` + `, liveOKSignal) liveConfig := ignv3types.Config{ Ignition: ignv3types.Ignition{ @@ -295,24 +343,5 @@ func testLiveIso(inst platform.Install, completionfile string) error { } defer mach.Destroy() - err = mach.QemuInst.Wait() - if err != nil { - return err - } - - err = exec.Command("grep", "-q", "-e", liveOKSignal, completionfile).Run() - if err != nil { - return fmt.Errorf("Failed to find %s in %s: %s", liveOKSignal, completionfile, err) - } - err = exec.Command("grep", "-q", "-e", completionstamp, completionfile).Run() - if err != nil { - return fmt.Errorf("Failed to find %s in %s: %s", completionstamp, completionfile, err) - } - - err = os.Remove(completionfile) - if err != nil { - return errors.Wrapf(err, "removing %s", completionfile) - } - - return nil + return awaitCompletion(mach.QemuInst, completionChannel, []string{liveOKSignal, signalCompleteString}) } diff --git a/mantle/platform/metal.go b/mantle/platform/metal.go index 30f68399e5..b54171412e 100644 --- a/mantle/platform/metal.go +++ b/mantle/platform/metal.go @@ -76,16 +76,26 @@ var ( } ) -type Install struct { - CosaBuild *sdk.LocalBuild - - Firmware string - Native4k bool - Console bool - Insecure bool - QemuArgs []string +// NewMetalQemuBuilderDefault returns a QEMU builder instance with some +// defaults set up for bare metal. +func NewMetalQemuBuilderDefault() *QemuBuilder { + builder := NewBuilder() + // https://github.com/coreos/fedora-coreos-tracker/issues/388 + // https://github.com/coreos/fedora-coreos-docs/pull/46 + builder.Memory = 4096 + if system.RpmArch() == "s390x" { + // FIXME - determine why this is + builder.Memory = int(math.Max(float64(builder.Memory), 16384)) + } + return builder +} +type Install struct { + CosaBuild *sdk.LocalBuild + Builder *QemuBuilder + Insecure bool LegacyInstaller bool + Native4k bool // These are set by the install path kargs []string @@ -216,34 +226,6 @@ func setupMetalImage(builddir, metalimg, destdir string) (string, error) { } } -func newQemuBuilder(firmware string, console bool, native4k bool) *QemuBuilder { - builder := NewBuilder() - builder.Firmware = firmware - - sectorSize := 0 - if native4k { - sectorSize = 4096 - } - - builder.AddPrimaryDisk(&Disk{ - Size: "12G", // Arbitrary - - SectorSize: sectorSize, - }) - - // This applies just in the legacy case - builder.Memory = 1536 - if system.RpmArch() == "s390x" { - // FIXME - determine why this is - builder.Memory = int(math.Max(float64(builder.Memory), 16384)) - } - - // For now, but in the future we should rely on log capture - builder.InheritConsole = console - - return builder -} - func (inst *Install) setup(kern *kernelSetup) (*installerRun, error) { if kern.kernel == "" { return nil, fmt.Errorf("Missing kernel artifact") @@ -252,7 +234,7 @@ func (inst *Install) setup(kern *kernelSetup) (*installerRun, error) { return nil, fmt.Errorf("Missing initramfs artifact") } - builder := newQemuBuilder(inst.Firmware, inst.Console, inst.Native4k) + builder := inst.Builder tempdir, err := ioutil.TempDir("", "kola-testiso") if err != nil { @@ -452,7 +434,6 @@ func (t *installerRun) run() (*QemuInstance, error) { usernetdev += ",net=192.168.76.0/24,dhcpstart=192.168.76.9" } builder.Append("-netdev", usernetdev) - builder.Append(t.inst.QemuArgs...) inst, err := builder.Exec() if err != nil { @@ -461,12 +442,6 @@ func (t *installerRun) run() (*QemuInstance, error) { return inst, nil } -func setBuilderLiveMemory(builder *QemuBuilder) { - // https://github.com/coreos/fedora-coreos-tracker/issues/388 - // https://github.com/coreos/fedora-coreos-docs/pull/46 - builder.Memory = int(math.Max(float64(builder.Memory), 4096)) -} - func (inst *Install) runPXE(kern *kernelSetup, legacy bool) (*InstalledMachine, error) { t, err := inst.setup(kern) if err != nil { @@ -476,7 +451,6 @@ func (inst *Install) runPXE(kern *kernelSetup, legacy bool) (*InstalledMachine, kargs := renderBaseKargs() if !legacy { - setBuilderLiveMemory(t.builder) kargs = append(kargs, liveKargs...) } @@ -666,10 +640,8 @@ WantedBy=multi-user.target return nil, errors.Wrapf(err, "running coreos-installer iso embed") } - qemubuilder := newQemuBuilder(inst.Firmware, inst.Console, inst.Native4k) - setBuilderLiveMemory(qemubuilder) + qemubuilder := inst.Builder qemubuilder.AddInstallIso(isoEmbeddedPath) - qemubuilder.Append(inst.QemuArgs...) qinst, err := qemubuilder.Exec() if err != nil { diff --git a/mantle/platform/qemu.go b/mantle/platform/qemu.go index 5d999a49c4..0848c764ec 100644 --- a/mantle/platform/qemu.go +++ b/mantle/platform/qemu.go @@ -178,7 +178,10 @@ type QemuBuilder struct { finalized bool diskId uint fs9pId uint - fds []*os.File + // virtioSerialId is incremented for each device + virtioSerialId uint + // fds is file descriptors we own to pass to qemu + fds []*os.File } func NewBuilder() *QemuBuilder { @@ -686,6 +689,27 @@ func (builder *QemuBuilder) setupUefi(secureBoot bool) error { return nil } +// VirtioChannelRead allocates a virtio-serial channel that will appear in +// the guest as /dev/virtio-ports/. The guest can write to it, and +// the host can read. +func (builder *QemuBuilder) VirtioChannelRead(name string) (*os.File, error) { + // Set up the virtio channel to get Ignition failures by default + r, w, err := os.Pipe() + if err != nil { + return nil, errors.Wrapf(err, "virtioChannelRead creating pipe") + } + if builder.virtioSerialId == 0 { + builder.Append("-device", "virtio-serial") + } + builder.virtioSerialId += 1 + id := fmt.Sprintf("virtioserial%d", builder.virtioSerialId) + // https://www.redhat.com/archives/libvir-list/2015-December/msg00305.html + builder.Append("-chardev", fmt.Sprintf("file,id=%s,path=%s,append=on", id, builder.AddFd(w))) + builder.Append("-device", fmt.Sprintf("virtserialport,chardev=%s,name=%s", id, name)) + + return r, nil +} + func (builder *QemuBuilder) Exec() (*QemuInstance, error) { builder.finalize() var err error