Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't panic on podman4 machine configs #21573

Merged
merged 1 commit into from
Feb 12, 2024
Merged

Don't panic on podman4 machine configs #21573

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions pkg/machine/define/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,3 +40,12 @@ type ErrNewDiskSizeTooSmall struct {
func (err *ErrNewDiskSizeTooSmall) Error() string {
return fmt.Sprintf("invalid disk size %d: new disk must be larger than %dGB", err.OldSize, err.NewSize)
}

type ErrIncompatibleMachineConfig struct {
Name string
Path string
}

func (err *ErrIncompatibleMachineConfig) Error() string {
return fmt.Sprintf("incompatible machine config %q (%s) for this version of Podman", err.Path, err.Name)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

registry.SetExitCode(os.ENOTSUP) ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nope, we we also use this to emit a soft error ... the use case is like podman machine ls; as it is walking the dir for JSON files, it tries to read one; if we detect that this is an old config (version 0), we emit a soft error but continue on ... assuming we list another machine, it seems reasonable that the error code is 0 (meaning the ls worked).

In all other cases, where the VM name is provided (we are not walking a dir looking for files), then we do hard error and yes, in that case we could set the return code.

i think we have options to explore, for example, if the code above worked, we could make the soft error just a fmt.errorf instead of calling this actual error (the message could be the same). That way, again if the above worked, it would set the code.

thoughts?

}
15 changes: 0 additions & 15 deletions pkg/machine/e2e/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,18 +235,3 @@ func isVmtype(vmType define.VMType) bool {
func isWSL() bool {
return isVmtype(define.WSLVirt)
}

// TODO temporarily suspended
// func getFCOSDownloadLocation(p vmconfigs.VMStubber) string {
// dd, err := p.NewDownload("")
// if err != nil {
// Fail("unable to create new download")
// }
//
// fcd, err := dd.GetFCOSDownload(defaultStream)
// if err != nil {
// Fail("unable to get virtual machine image")
// }
//
// return fcd.Location
// }
132 changes: 132 additions & 0 deletions pkg/machine/e2e/init_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,4 +315,136 @@ var _ = Describe("podman machine init", func() {
Expect(err).To(HaveOccurred())
}
})

It("verify a podman 4 config does not break podman 5", func() {
vmName := "foobar-machine"
configDir := filepath.Join(testDir, ".config", "containers", "podman", "machine", testProvider.VMType().String())
if err := os.MkdirAll(configDir, 0755); err != nil {
Expect(err).ToNot(HaveOccurred())
}
f, err := os.Create(filepath.Join(configDir, fmt.Sprintf("%s.json", vmName)))
Expect(err).ToNot(HaveOccurred())
if _, err := f.Write(p4Config); err != nil {
Expect(err).ToNot(HaveOccurred())
}
err = f.Close()
Expect(err).ToNot(HaveOccurred())

// At this point we have a p4 config in the config dir
// podman machine list should emit a "soft" error but complete
list := new(listMachine)
firstList, err := mb.setCmd(list).run()
Expect(err).NotTo(HaveOccurred())
Expect(firstList).Should(Exit(0))
Expect(firstList.errorToString()).To(ContainSubstring("incompatible machine config"))

// podman machine inspect should fail because we are
// trying to work with the incompatible machine json
ins := inspectMachine{}
inspectShouldFail, err := mb.setName(vmName).setCmd(&ins).run()
Expect(err).ToNot(HaveOccurred())
Expect(inspectShouldFail).To(Exit(125))
Expect(inspectShouldFail.errorToString()).To(ContainSubstring("incompatible machine config"))

// We should be able to init with a bad config present
i := new(initMachine)
name := randomString()
session, err := mb.setName(name).setCmd(i.withImagePath(mb.imagePath)).run()
Expect(err).ToNot(HaveOccurred())
Expect(session).To(Exit(0))

// We should still be able to ls
secondList, err := mb.setCmd(list).run()
Expect(err).NotTo(HaveOccurred())
Expect(secondList).Should(Exit(0))

// And inspecting the valid machine should not error
inspectShouldPass, err := mb.setName(name).setCmd(&ins).run()
Expect(err).ToNot(HaveOccurred())
Expect(inspectShouldPass).To(Exit(0))
})
})

var p4Config = []byte(`{
"ConfigPath": {
"Path": "/home/baude/.config/containers/podman/machine/qemu/podman-machine-default.json"
},
"CmdLine": [
"/usr/bin/qemu-system-x86_64",
"-accel",
"kvm",
"-cpu",
"host",
"-m",
"2048",
"-smp",
"12",
"-fw_cfg",
"name=opt/com.coreos/config,file=/home/baude/.config/containers/podman/machine/qemu/podman-machine-default.ign",
"-qmp",
"unix:/run/user/1000/podman/qmp_podman-machine-default.sock,server=on,wait=off",
"-netdev",
"socket,id=vlan,fd=3",
"-device",
"virtio-net-pci,netdev=vlan,mac=5a:94:ef:e4:0c:ee",
"-device",
"virtio-serial",
"-chardev",
"socket,path=/run/user/1000/podman/podman-machine-default_ready.sock,server=on,wait=off,id=apodman-machine-default_ready",
"-device",
"virtserialport,chardev=apodman-machine-default_ready,name=org.fedoraproject.port.0",
"-pidfile",
"/run/user/1000/podman/podman-machine-default_vm.pid",
"-virtfs",
"local,path=/home/baude,mount_tag=vol0,security_model=none",
"-drive",
"if=virtio,file=/home/baude/.local/share/containers/podman/machine/qemu/podman-machine-default_fedora-coreos-39.20240128.2.2-qemu.x86_64.qcow2"
],
"Rootful": false,
"UID": 1000,
"HostUserModified": false,
"IgnitionFilePath": {
"Path": "/home/baude/.config/containers/podman/machine/qemu/podman-machine-default.ign"
},
"ImageStream": "testing",
"ImagePath": {
"Path": "/home/baude/.local/share/containers/podman/machine/qemu/podman-machine-default_fedora-coreos-39.20240128.2.2-qemu.x86_64.qcow2"
},
"Mounts": [
{
"ReadOnly": false,
"Source": "/home/baude",
"Tag": "vol0",
"Target": "/home/baude",
"Type": "9p"
}
],
"Name": "podman-machine-default",
"PidFilePath": {
"Path": "/run/user/1000/podman/podman-machine-default_proxy.pid"
},
"VMPidFilePath": {
"Path": "/run/user/1000/podman/podman-machine-default_vm.pid"
},
"QMPMonitor": {
"Address": {
"Path": "/run/user/1000/podman/qmp_podman-machine-default.sock"
},
"Network": "unix",
"Timeout": 2000000000
},
"ReadySocket": {
"Path": "/run/user/1000/podman/podman-machine-default_ready.sock"
},
"CPUs": 12,
"DiskSize": 100,
"Memory": 2048,
"USBs": [],
"IdentityPath": "/home/baude/.local/share/containers/podman/machine/machine",
"Port": 38419,
"RemoteUsername": "core",
"Starting": false,
"Created": "2024-02-08T10:34:14.067604999-06:00",
"LastUp": "0001-01-01T00:00:00Z"
}
`)
2 changes: 2 additions & 0 deletions pkg/machine/shim/host.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,8 @@ func Init(opts machineDefine.InitOptions, mp vmconfigs.VMProvider) (*vmconfigs.M
return nil, err
}

mc.Version = vmconfigs.MachineConfigVersion

createOpts := machineDefine.CreateVMOpts{
Name: opts.Name,
Dirs: dirs,
Expand Down
2 changes: 2 additions & 0 deletions pkg/machine/vmconfigs/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ import (
"github.com/containers/storage/pkg/lockfile"
)

const MachineConfigVersion = 1

type MachineConfig struct {
// Common stuff
Created time.Time
Expand Down
20 changes: 20 additions & 0 deletions pkg/machine/vmconfigs/machine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,16 @@ func LoadMachineByName(name string, dirs *define.MachineDirs) (*MachineConfig, e
}
mc.dirs = dirs
mc.configPath = fullPath

// If we find an incompatible configuration, we return a hard
// error because the user wants to deal directly with this
// machine
if mc.Version == 0 {
return mc, &define.ErrIncompatibleMachineConfig{
Name: name,
Path: fullPath.GetPath(),
}
}
return mc, nil
}

Expand Down Expand Up @@ -345,6 +355,16 @@ func LoadMachinesInDir(dirs *define.MachineDirs) (map[string]*MachineConfig, err
if err != nil {
return err
}
// if we find an incompatible machine configuration file, we emit and error
//
if mc.Version == 0 {
tmpErr := &define.ErrIncompatibleMachineConfig{
Name: mc.Name,
Path: fullPath.GetPath(),
}
logrus.Error(tmpErr)
return nil
}
mc.configPath = fullPath
mc.dirs = dirs
mcs[mc.Name] = mc
Expand Down
Loading