Merge pull request #13674 from baude/refactor1

Machine refactor - part 1

5 files changed, 311 insertions, 12 deletions

diff --git a/pkg/machine/config.go b/pkg/machine/config.go
index aaf8da872..7e1561506 100644
--- a/pkg/machine/config.go
+++ b/pkg/machine/config.go
@@ -29,7 +29,7 @@ type InitOptions struct {
 	Username     string
 	ReExec       bool
 	Rootful      bool
-	// The numberical userid of the user that called machine
+	// The numerical userid of the user that called machine
 	UID string
 }
 
@@ -128,6 +128,7 @@ type DistributionDownload interface {
 }
 
 func (rc RemoteConnectionType) MakeSSHURL(host, path, port, userName string) url.URL {
+	//TODO Should this function have input verification?
 	userInfo := url.User(userName)
 	uri := url.URL{
 		Scheme:     "ssh",
diff --git a/pkg/machine/config_test.go b/pkg/machine/config_test.go
new file mode 100644
index 000000000..d9fc5425e
--- /dev/null
+++ b/pkg/machine/config_test.go
@@ -0,0 +1,71 @@
+package machine
+
+import (
+	"net"
+	"net/url"
+	"reflect"
+	"testing"
+)
+
+func TestRemoteConnectionType_MakeSSHURL(t *testing.T) {
+	var (
+		host     = "foobar"
+		path     = "/path/to/socket"
+		rc       = "ssh"
+		username = "core"
+	)
+	type args struct {
+		host     string
+		path     string
+		port     string
+		userName string
+	}
+	tests := []struct {
+		name string
+		rc   RemoteConnectionType
+		args args
+		want url.URL
+	}{
+		{
+			name: "Good no port",
+			rc:   "ssh",
+			args: args{
+				host:     host,
+				path:     path,
+				port:     "",
+				userName: username,
+			},
+			want: url.URL{
+				Scheme:     rc,
+				User:       url.User(username),
+				Host:       host,
+				Path:       path,
+				ForceQuery: false,
+			},
+		},
+		{
+			name: "Good with port",
+			rc:   "ssh",
+			args: args{
+				host:     host,
+				path:     path,
+				port:     "222",
+				userName: username,
+			},
+			want: url.URL{
+				Scheme:     rc,
+				User:       url.User(username),
+				Host:       net.JoinHostPort(host, "222"),
+				Path:       path,
+				ForceQuery: false,
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := tt.rc.MakeSSHURL(tt.args.host, tt.args.path, tt.args.port, tt.args.userName); !reflect.DeepEqual(got, tt.want) { //nolint: scopelint
+				t.Errorf("MakeSSHURL() = %v, want %v", got, tt.want) //nolint: scopelint
+			}
+		})
+	}
+}
diff --git a/pkg/machine/qemu/config.go b/pkg/machine/qemu/config.go
index 211d96ccb..408b33a33 100644
--- a/pkg/machine/qemu/config.go
+++ b/pkg/machine/qemu/config.go
@@ -4,12 +4,28 @@
 package qemu
 
 import (
+	"errors"
+	"os"
 	"time"
+
+	"github.com/sirupsen/logrus"
+)
+
+const (
+	// FCOS streams
+	// Testing FCOS stream
+	Testing string = "testing"
+	// Next FCOS stream
+	Next string = "next"
+	// Stable FCOS stream
+	Stable string = "stable"
 )
 
 type Provider struct{}
 
-type MachineVM struct {
+// Deprecated: MachineVMV1 is being deprecated in favor a more flexible and informative
+// structure
+type MachineVMV1 struct {
 	// CPUs to be assigned to the VM
 	CPUs uint64
 	// The command line representation of the qemu command
@@ -42,6 +58,74 @@ type MachineVM struct {
 	UID int
 }
 
+type MachineVM struct {
+	// The command line representation of the qemu command
+	CmdLine []string
+	// HostUser contains info about host user
+	HostUser
+	// ImageConfig describes the bootable image
+	ImageConfig
+	// Mounts is the list of remote filesystems to mount
+	Mounts []Mount
+	// Name of VM
+	Name string
+	// PidFilePath is the where the PID file lives
+	PidFilePath MachineFile
+	// QMPMonitor is the qemu monitor object for sending commands
+	QMPMonitor Monitor
+	// ReadySocket tells host when vm is booted
+	ReadySocket MachineFile
+	// ResourceConfig is physical attrs of the VM
+	ResourceConfig
+	// SSHConfig for accessing the remote vm
+	SSHConfig
+}
+
+// ImageConfig describes the bootable image for the VM
+type ImageConfig struct {
+	IgnitionFilePath string
+	// ImageStream is the update stream for the image
+	ImageStream string
+	// ImagePath is the fq path to
+	ImagePath string
+}
+
+// HostUser describes the host user
+type HostUser struct {
+	// Whether this machine should run in a rootful or rootless manner
+	Rootful bool
+	// UID is the numerical id of the user that called machine
+	UID int
+}
+
+// SSHConfig contains remote access information for SSH
+type SSHConfig struct {
+	// IdentityPath is the fq path to the ssh priv key
+	IdentityPath string
+	// SSH port for user networking
+	Port int
+	// RemoteUsername of the vm user
+	RemoteUsername string
+}
+
+// ResourceConfig describes physical attributes of the machine
+type ResourceConfig struct {
+	// CPUs to be assigned to the VM
+	CPUs uint64
+	// Memory in megabytes assigned to the vm
+	Memory uint64
+	// Disk size in gigabytes assigned to the vm
+	DiskSize uint64
+}
+
+type MachineFile struct {
+	// Path is the fully qualified path to a file
+	Path string
+	// Symlink is a shortened version of Path by using
+	// a symlink
+	Symlink *string
+}
+
 type Mount struct {
 	Type     string
 	Tag      string
@@ -52,7 +136,7 @@ type Mount struct {
 
 type Monitor struct {
 	//	Address portion of the qmp monitor (/tmp/tmp.sock)
-	Address string
+	Address MachineFile
 	// Network portion of the qmp monitor (unix)
 	Network string
 	// Timeout in seconds for qmp monitor transactions
@@ -64,3 +148,37 @@ var (
 	// qmp monitor interactions.
 	defaultQMPTimeout time.Duration = 2 * time.Second
 )
+
+// GetPath returns the working path for a machinefile.  it returns
+// the symlink unless one does not exist
+func (m *MachineFile) GetPath() string {
+	if m.Symlink == nil {
+		return m.Path
+	}
+	return *m.Symlink
+}
+
+// Delete removes the machinefile symlink (if it exists) and
+// the actual path
+func (m *MachineFile) Delete() error {
+	if m.Symlink != nil {
+		if err := os.Remove(*m.Symlink); err != nil {
+			logrus.Errorf("unable to remove symlink %q", *m.Symlink)
+		}
+	}
+	return os.Remove(m.Path)
+}
+
+// NewMachineFile is a constructor for MachineFile
+func NewMachineFile(path string, symlink *string) (*MachineFile, error) {
+	if len(path) < 1 {
+		return nil, errors.New("invalid machine file path")
+	}
+	if symlink != nil && len(*symlink) < 1 {
+		return nil, errors.New("invalid symlink path")
+	}
+	return &MachineFile{
+		Path:    path,
+		Symlink: symlink,
+	}, nil
+}
diff --git a/pkg/machine/qemu/config_test.go b/pkg/machine/qemu/config_test.go
new file mode 100644
index 000000000..e3e7437b5
--- /dev/null
+++ b/pkg/machine/qemu/config_test.go
@@ -0,0 +1,103 @@
+package qemu
+
+import (
+	"reflect"
+	"testing"
+)
+
+func TestMachineFile_GetPath(t *testing.T) {
+	path := "/var/tmp/podman/my.sock"
+	sym := "/tmp/podman/my.sock"
+	type fields struct {
+		Path    string
+		Symlink *string
+	}
+	tests := []struct {
+		name   string
+		fields fields
+		want   string
+	}{
+		{
+			name:   "Original path",
+			fields: fields{path, nil},
+			want:   path,
+		},
+		{
+			name: "Symlink over path",
+			fields: fields{
+				Path:    path,
+				Symlink: &sym,
+			},
+			want: sym,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			m := &MachineFile{
+				Path:    tt.fields.Path,    //nolint: scopelint
+				Symlink: tt.fields.Symlink, //nolint: scopelint
+			}
+			if got := m.GetPath(); got != tt.want { //nolint: scopelint
+				t.Errorf("GetPath() = %v, want %v", got, tt.want) //nolint: scopelint
+			}
+		})
+	}
+}
+
+func TestNewMachineFile(t *testing.T) {
+	p := "/var/tmp/podman/my.sock"
+	sym := "/tmp/podman/my.sock"
+	empty := ""
+
+	m := MachineFile{
+		Path:    p,
+		Symlink: nil,
+	}
+	type args struct {
+		path    string
+		symlink *string
+	}
+	tests := []struct {
+		name    string
+		args    args
+		want    *MachineFile
+		wantErr bool
+	}{
+		{
+			name:    "Good",
+			args:    args{path: p},
+			want:    &m,
+			wantErr: false,
+		},
+		{
+			name:    "Good with Symlink",
+			args:    args{p, &sym},
+			want:    &MachineFile{p, &sym},
+			wantErr: false,
+		},
+		{
+			name:    "Bad path name",
+			args:    args{empty, nil},
+			want:    nil,
+			wantErr: true,
+		},
+		{
+			name:    "Bad symlink name",
+			args:    args{p, &empty},
+			want:    nil,
+			wantErr: true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, err := NewMachineFile(tt.args.path, tt.args.symlink) //nolint: scopelint
+			if (err != nil) != tt.wantErr {                           //nolint: scopelint
+				t.Errorf("NewMachineFile() error = %v, wantErr %v", err, tt.wantErr) //nolint: scopelint
+				return
+			}
+			if !reflect.DeepEqual(got, tt.want) { //nolint: scopelint
+				t.Errorf("NewMachineFile() got = %v, want %v", got, tt.want) //nolint: scopelint
+			}
+		})
+	}
+}
diff --git a/pkg/machine/qemu/machine.go b/pkg/machine/qemu/machine.go
index f04625781..ac8e7d75c 100644
--- a/pkg/machine/qemu/machine.go
+++ b/pkg/machine/qemu/machine.go
@@ -111,7 +111,7 @@ func (p *Provider) NewMachine(opts machine.InitOptions) (machine.VM, error) {
 		return nil, err
 	}
 	vm.QMPMonitor = monitor
-	cmd = append(cmd, []string{"-qmp", monitor.Network + ":/" + monitor.Address + ",server=on,wait=off"}...)
+	cmd = append(cmd, []string{"-qmp", monitor.Network + ":/" + monitor.Address.GetPath() + ",server=on,wait=off"}...)
 
 	// Add network
 	// Right now the mac address is hardcoded so that the host networking gives it a specific IP address.  This is
@@ -134,7 +134,8 @@ func (p *Provider) NewMachine(opts machine.InitOptions) (machine.VM, error) {
 // LoadByName reads a json file that describes a known qemu vm
 // and returns a vm instance
 func (p *Provider) LoadVMByName(name string) (machine.VM, error) {
-	vm := &MachineVM{UID: -1} // posix reserves -1, so use it to signify undefined
+	vm := &MachineVM{Name: name}
+	vm.HostUser = HostUser{UID: -1} // posix reserves -1, so use it to signify undefined
 	vmConfigDir, err := machine.GetConfDir(vmtype)
 	if err != nil {
 		return nil, err
@@ -176,7 +177,7 @@ func (v *MachineVM) Init(opts machine.InitOptions) (bool, error) {
 	v.Rootful = opts.Rootful
 
 	switch opts.ImagePath {
-	case "testing", "next", "stable", "":
+	case Testing, Next, Stable, "":
 		// Get image as usual
 		v.ImageStream = opts.ImagePath
 		dd, err := machine.NewFcosDownloader(vmtype, v.Name, opts.ImagePath)
@@ -576,12 +577,12 @@ func (v *MachineVM) checkStatus(monitor *qmp.SocketMonitor) (machine.QemuMachine
 func (v *MachineVM) Stop(name string, _ machine.StopOptions) error {
 	var disconnected bool
 	// check if the qmp socket is there. if not, qemu instance is gone
-	if _, err := os.Stat(v.QMPMonitor.Address); os.IsNotExist(err) {
+	if _, err := os.Stat(v.QMPMonitor.Address.GetPath()); os.IsNotExist(err) {
 		// Right now it is NOT an error to stop a stopped machine
 		logrus.Debugf("QMP monitor socket %v does not exist", v.QMPMonitor.Address)
 		return nil
 	}
-	qmpMonitor, err := qmp.NewSocketMonitor(v.QMPMonitor.Network, v.QMPMonitor.Address, v.QMPMonitor.Timeout)
+	qmpMonitor, err := qmp.NewSocketMonitor(v.QMPMonitor.Network, v.QMPMonitor.Address.GetPath(), v.QMPMonitor.Timeout)
 	if err != nil {
 		return err
 	}
@@ -684,20 +685,25 @@ func NewQMPMonitor(network, name string, timeout time.Duration) (Monitor, error)
 	if timeout == 0 {
 		timeout = defaultQMPTimeout
 	}
+	address, err := NewMachineFile(filepath.Join(rtDir, "qmp+"+name+".sock"), nil)
+	if err != nil {
+		return Monitor{}, err
+	}
 	monitor := Monitor{
 		Network: network,
-		Address: filepath.Join(rtDir, "qmp_"+name+".sock"),
+		Address: *address,
 		Timeout: timeout,
 	}
 	return monitor, nil
 }
 
+// Remove deletes all the files associated with a machine including ssh keys, the image itself
 func (v *MachineVM) Remove(name string, opts machine.RemoveOptions) (string, func() error, error) {
 	var (
 		files []string
 	)
 
-	// cannot remove a running vm
+	// cannot remove a running vm unless --force is used
 	running, err := v.isRunning()
 	if err != nil {
 		return "", nil, err
@@ -768,11 +774,11 @@ func (v *MachineVM) Remove(name string, opts machine.RemoveOptions) (string, fun
 
 func (v *MachineVM) isRunning() (bool, error) {
 	// Check if qmp socket path exists
-	if _, err := os.Stat(v.QMPMonitor.Address); os.IsNotExist(err) {
+	if _, err := os.Stat(v.QMPMonitor.Address.GetPath()); os.IsNotExist(err) {
 		return false, nil
 	}
 	// Check if we can dial it
-	monitor, err := qmp.NewSocketMonitor(v.QMPMonitor.Network, v.QMPMonitor.Address, v.QMPMonitor.Timeout)
+	monitor, err := qmp.NewSocketMonitor(v.QMPMonitor.Network, v.QMPMonitor.Address.GetPath(), v.QMPMonitor.Timeout)
 	if err != nil {
 		// FIXME: this error should probably be returned
 		return false, nil // nolint: nilerr