From 9b97a85a8fd82f1c7e1d4b9a8cacc97a80d15ce8 Mon Sep 17 00:00:00 2001 From: balopat Date: Fri, 25 Jan 2019 16:26:28 -0800 Subject: [PATCH 1/5] fix stale hyperkit.pid making minikube start hang --- pkg/drivers/hyperkit/driver.go | 74 +++++++++++++++++++++++++++------- 1 file changed, 60 insertions(+), 14 deletions(-) diff --git a/pkg/drivers/hyperkit/driver.go b/pkg/drivers/hyperkit/driver.go index f814c7cb2965..5c5b2c42fe4e 100644 --- a/pkg/drivers/hyperkit/driver.go +++ b/pkg/drivers/hyperkit/driver.go @@ -33,12 +33,13 @@ import ( "github.com/docker/machine/libmachine/drivers" "github.com/docker/machine/libmachine/log" "github.com/docker/machine/libmachine/state" - nfsexports "github.com/johanneswuerbach/nfsexports" - hyperkit "github.com/moby/hyperkit/go" + "github.com/johanneswuerbach/nfsexports" + "github.com/moby/hyperkit/go" "github.com/pkg/errors" pkgdrivers "k8s.io/minikube/pkg/drivers" "k8s.io/minikube/pkg/minikube/constants" commonutil "k8s.io/minikube/pkg/util" + "io/ioutil" ) const ( @@ -126,6 +127,7 @@ func (d *Driver) GetURL() (string, error) { // GetState returns the state that the host is in (running, stopped, etc) func (d *Driver) GetState() (state.State, error) { pid := d.getPid() + log.Infof("Found hyperkit pid: %d", pid) if pid == 0 { return state.Stopped, nil } @@ -169,12 +171,14 @@ func (d *Driver) Restart() error { // Start a host func (d *Driver) Start() error { - h, err := hyperkit.New("", d.VpnKitSock, filepath.Join(d.StorePath, "machines", d.MachineName)) + stateDir := filepath.Join(d.StorePath, "machines", d.MachineName) + if err := d.checkForPidFile(); err != nil { + return err + } + h, err := hyperkit.New("", d.VpnKitSock, stateDir) if err != nil { return errors.Wrap(err, "new-ing Hyperkit") } - - // TODO: handle the rest of our settings. h.Kernel = d.ResolveStorePath("bzimage") h.Initrd = d.ResolveStorePath("initrd") h.VMNet = true @@ -183,13 +187,20 @@ func (d *Driver) Start() error { h.CPUs = d.CPU h.Memory = d.Memory h.UUID = d.UUID - if vsockPorts, err := d.extractVSockPorts(); err != nil { return err } else if len(vsockPorts) >= 1 { h.VSock = true h.VSockPorts = vsockPorts } + h.Disks = []hyperkit.DiskConfig{ + { + Path: pkgdrivers.GetDiskPath(d.BaseDriver), + Size: d.DiskSize, + Driver: "virtio-blk", + }, + } + //} log.Infof("Using UUID %s", h.UUID) mac, err := GetMACAddressFromUUID(h.UUID) @@ -200,13 +211,7 @@ func (d *Driver) Start() error { // Need to strip 0's mac = trimMacAddress(mac) log.Infof("Generated MAC %s", mac) - h.Disks = []hyperkit.DiskConfig{ - { - Path: pkgdrivers.GetDiskPath(d.BaseDriver), - Size: d.DiskSize, - Driver: "virtio-blk", - }, - } + log.Infof("Starting with cmdline: %s", d.Cmdline) if err := h.Start(d.Cmdline); err != nil { return errors.Wrapf(err, "starting with cmd line: %s", d.Cmdline) @@ -240,6 +245,48 @@ func (d *Driver) Start() error { return nil } +func (d *Driver) checkForPidFile() error { + stateDir := filepath.Join(d.StorePath, "machines", d.MachineName) + pidFile := filepath.Join(stateDir, pidFileName) + + if _, err := os.Stat(pidFile); err != nil { + log.Infof("clean start, hyperkit pid file doesn't exist: %s", pidFile) + return nil + } + + log.Warnf("hyperkit pid file exists: %s", pidFile) + // we have a pid file, hyperkit might be running! + // Make sure the pid written by hyperkit is the same as in the json + content, err := ioutil.ReadFile(pidFile) + if err != nil { + return errors.Wrapf(err, "reading pidfile %s", pidFile) + } + pid, err := strconv.Atoi(string(content[:])) + if err != nil { + return errors.Wrapf(err, "parsing pidfile %s", pidFile) + } + + p, _ := os.FindProcess(pid) //noop on linux + // Sending a signal of 0 can be used to check the existence of a process. + if err := p.Signal(syscall.Signal(0)); err == nil { + return fmt.Errorf("something is not right...please stop all minikube instances, seemingly a hyperkit server is already running with pid %d", pid) + } + + machinePID := d.getPid() + + if machinePID != pid { + return fmt.Errorf("something is not right...the PID reported in %s (%d) is different from machine PID (%d from %s)", pidFile, pid, machinePID, machineFileName) + } + + log.Warnf("No running process found with PID %d, removing %s...", machinePID, pidFile) + if err := os.Remove(pidFile); err != nil { + return errors.Wrap(err, fmt.Sprintf("removing pidFile %s", pidFile)) + } + log.Infof("Removed PID file.") + + return nil +} + // Stop a host gracefully func (d *Driver) Stop() error { d.cleanupNfsExports() @@ -350,7 +397,6 @@ func (d *Driver) sendSignal(s os.Signal) error { func (d *Driver) getPid() int { pidPath := d.ResolveStorePath(machineFileName) - f, err := os.Open(pidPath) if err != nil { log.Warnf("Error reading pid file: %v", err) From 933356e4776f18a508a1c9416d1569438f4e7983 Mon Sep 17 00:00:00 2001 From: balopat Date: Fri, 25 Jan 2019 17:20:57 -0800 Subject: [PATCH 2/5] lint --- pkg/drivers/hyperkit/driver.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/drivers/hyperkit/driver.go b/pkg/drivers/hyperkit/driver.go index 5c5b2c42fe4e..f253d70362f2 100644 --- a/pkg/drivers/hyperkit/driver.go +++ b/pkg/drivers/hyperkit/driver.go @@ -36,10 +36,10 @@ import ( "github.com/johanneswuerbach/nfsexports" "github.com/moby/hyperkit/go" "github.com/pkg/errors" + "io/ioutil" pkgdrivers "k8s.io/minikube/pkg/drivers" "k8s.io/minikube/pkg/minikube/constants" commonutil "k8s.io/minikube/pkg/util" - "io/ioutil" ) const ( From f70e6a03858ac33a7e4168680f8c38fdb1ad8b92 Mon Sep 17 00:00:00 2001 From: balopat Date: Mon, 28 Jan 2019 13:42:43 -0800 Subject: [PATCH 3/5] fixing review remarks --- pkg/drivers/hyperkit/driver.go | 39 +++++++++++++++++++++------------- 1 file changed, 24 insertions(+), 15 deletions(-) diff --git a/pkg/drivers/hyperkit/driver.go b/pkg/drivers/hyperkit/driver.go index f253d70362f2..53498bad17f3 100644 --- a/pkg/drivers/hyperkit/driver.go +++ b/pkg/drivers/hyperkit/driver.go @@ -21,6 +21,7 @@ package hyperkit import ( "encoding/json" "fmt" + "github.com/mitchellh/go-ps" "os" "os/user" "path" @@ -127,7 +128,6 @@ func (d *Driver) GetURL() (string, error) { // GetState returns the state that the host is in (running, stopped, etc) func (d *Driver) GetState() (state.State, error) { pid := d.getPid() - log.Infof("Found hyperkit pid: %d", pid) if pid == 0 { return state.Stopped, nil } @@ -172,7 +172,7 @@ func (d *Driver) Restart() error { // Start a host func (d *Driver) Start() error { stateDir := filepath.Join(d.StorePath, "machines", d.MachineName) - if err := d.checkForPidFile(); err != nil { + if err := d.recoverFromUncleanShutdown(); err != nil { return err } h, err := hyperkit.New("", d.VpnKitSock, stateDir) @@ -245,16 +245,29 @@ func (d *Driver) Start() error { return nil } -func (d *Driver) checkForPidFile() error { +//recoverFromUncleanShutdown searches for an existing hyperkit.pid file in +//the machine directory. If it can't find it, a clean shutdown is assumed. +//If it finds the pid file, it checks for a running hyperkit process with that pid +//as the existence of a file might not indicate an unclean shutdown but an actual running +//hyperkit server. This is an error situation - we shouldn't start minikube as there is likely +//an instance running already. If the PID in the pidfile does not belong to a running hyperkit +//process, we can safely delete it, and there is a good chance the machine will recover when restarted. +func (d *Driver) recoverFromUncleanShutdown() error { stateDir := filepath.Join(d.StorePath, "machines", d.MachineName) pidFile := filepath.Join(stateDir, pidFileName) - if _, err := os.Stat(pidFile); err != nil { + _, err := os.Stat(pidFile) + + if os.IsNotExist(err) { log.Infof("clean start, hyperkit pid file doesn't exist: %s", pidFile) return nil } - log.Warnf("hyperkit pid file exists: %s", pidFile) + if err != nil { + return errors.Wrap(err, "checking hyperkit pid file existence") + } + + log.Warnf("minikube might have been shutdown in an unclean way, the hyperkit pid file still exists: %s", pidFile) // we have a pid file, hyperkit might be running! // Make sure the pid written by hyperkit is the same as in the json content, err := ioutil.ReadFile(pidFile) @@ -266,23 +279,19 @@ func (d *Driver) checkForPidFile() error { return errors.Wrapf(err, "parsing pidfile %s", pidFile) } - p, _ := os.FindProcess(pid) //noop on linux - // Sending a signal of 0 can be used to check the existence of a process. - if err := p.Signal(syscall.Signal(0)); err == nil { - return fmt.Errorf("something is not right...please stop all minikube instances, seemingly a hyperkit server is already running with pid %d", pid) + p, err := ps.FindProcess(pid) + if err != nil { + return errors.Wrapf(err, "trying to find process for PID %s", pid) } - machinePID := d.getPid() - - if machinePID != pid { - return fmt.Errorf("something is not right...the PID reported in %s (%d) is different from machine PID (%d from %s)", pidFile, pid, machinePID, machineFileName) + if p != nil && !strings.Contains(p.Executable(), "hyperkit") { + return fmt.Errorf("something is not right...please stop all minikube instances, seemingly a hyperkit server is already running with pid %d, executable: %s", pid, p.Executable()) } - log.Warnf("No running process found with PID %d, removing %s...", machinePID, pidFile) + log.Infof("No running hyperkit process found with PID %d, removing %s...", pid, pidFile) if err := os.Remove(pidFile); err != nil { return errors.Wrap(err, fmt.Sprintf("removing pidFile %s", pidFile)) } - log.Infof("Removed PID file.") return nil } From 5af2f1eafc7a5d00238d05637682bf4264343497 Mon Sep 17 00:00:00 2001 From: balopat Date: Mon, 28 Jan 2019 13:47:24 -0800 Subject: [PATCH 4/5] formatting / removed extra comment --- pkg/drivers/hyperkit/driver.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/pkg/drivers/hyperkit/driver.go b/pkg/drivers/hyperkit/driver.go index 53498bad17f3..60067f75ccb9 100644 --- a/pkg/drivers/hyperkit/driver.go +++ b/pkg/drivers/hyperkit/driver.go @@ -21,7 +21,6 @@ package hyperkit import ( "encoding/json" "fmt" - "github.com/mitchellh/go-ps" "os" "os/user" "path" @@ -31,13 +30,16 @@ import ( "syscall" "time" + "github.com/mitchellh/go-ps" + + "io/ioutil" + "github.com/docker/machine/libmachine/drivers" "github.com/docker/machine/libmachine/log" "github.com/docker/machine/libmachine/state" "github.com/johanneswuerbach/nfsexports" "github.com/moby/hyperkit/go" "github.com/pkg/errors" - "io/ioutil" pkgdrivers "k8s.io/minikube/pkg/drivers" "k8s.io/minikube/pkg/minikube/constants" commonutil "k8s.io/minikube/pkg/util" @@ -268,13 +270,12 @@ func (d *Driver) recoverFromUncleanShutdown() error { } log.Warnf("minikube might have been shutdown in an unclean way, the hyperkit pid file still exists: %s", pidFile) - // we have a pid file, hyperkit might be running! - // Make sure the pid written by hyperkit is the same as in the json + content, err := ioutil.ReadFile(pidFile) if err != nil { return errors.Wrapf(err, "reading pidfile %s", pidFile) } - pid, err := strconv.Atoi(string(content[:])) + pid, err := strconv.Atoi(string(content)) if err != nil { return errors.Wrapf(err, "parsing pidfile %s", pidFile) } From 17c0244cf7846c47413b4603a3b39ec6c311dbff Mon Sep 17 00:00:00 2001 From: balopat Date: Mon, 28 Jan 2019 17:28:57 -0800 Subject: [PATCH 5/5] reverting unnecessary change --- pkg/drivers/hyperkit/driver.go | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/pkg/drivers/hyperkit/driver.go b/pkg/drivers/hyperkit/driver.go index 60067f75ccb9..996a6289db04 100644 --- a/pkg/drivers/hyperkit/driver.go +++ b/pkg/drivers/hyperkit/driver.go @@ -181,6 +181,8 @@ func (d *Driver) Start() error { if err != nil { return errors.Wrap(err, "new-ing Hyperkit") } + + // TODO: handle the rest of our settings. h.Kernel = d.ResolveStorePath("bzimage") h.Initrd = d.ResolveStorePath("initrd") h.VMNet = true @@ -189,20 +191,13 @@ func (d *Driver) Start() error { h.CPUs = d.CPU h.Memory = d.Memory h.UUID = d.UUID + if vsockPorts, err := d.extractVSockPorts(); err != nil { return err } else if len(vsockPorts) >= 1 { h.VSock = true h.VSockPorts = vsockPorts } - h.Disks = []hyperkit.DiskConfig{ - { - Path: pkgdrivers.GetDiskPath(d.BaseDriver), - Size: d.DiskSize, - Driver: "virtio-blk", - }, - } - //} log.Infof("Using UUID %s", h.UUID) mac, err := GetMACAddressFromUUID(h.UUID) @@ -213,7 +208,13 @@ func (d *Driver) Start() error { // Need to strip 0's mac = trimMacAddress(mac) log.Infof("Generated MAC %s", mac) - + h.Disks = []hyperkit.DiskConfig{ + { + Path: pkgdrivers.GetDiskPath(d.BaseDriver), + Size: d.DiskSize, + Driver: "virtio-blk", + }, + } log.Infof("Starting with cmdline: %s", d.Cmdline) if err := h.Start(d.Cmdline); err != nil { return errors.Wrapf(err, "starting with cmd line: %s", d.Cmdline) @@ -407,6 +408,7 @@ func (d *Driver) sendSignal(s os.Signal) error { func (d *Driver) getPid() int { pidPath := d.ResolveStorePath(machineFileName) + f, err := os.Open(pidPath) if err != nil { log.Warnf("Error reading pid file: %v", err)