Merge pull request #661 from innobead/bug-660-fail-restart-vm-stopped…

…-by-signal

cmd/ignite/run/stop.go

@@ -1,8 +1,6 @@
 package run
 
 import (
-	"fmt"
-
 	api "github.com/weaveworks/ignite/pkg/apis/ignite"
 	"github.com/weaveworks/ignite/pkg/operations"
 )
@@ -24,11 +22,6 @@ func (sf *StopFlags) NewStopOptions(vmMatches []string) (so *stopOptions, err er
 
 func Stop(so *stopOptions) error {
 	for _, vm := range so.vms {
-		// Check if the VM is running
-		if !vm.Running() {
-			return fmt.Errorf("VM %q is not running", vm.GetUID())
-		}
-
 		// Stop the VM, and optionally kill it
 		if err := operations.StopVM(vm, so.Kill, false); err != nil {
 			return err

go.mod

@@ -59,6 +59,6 @@ require (
 	golang.org/x/sys v0.0.0-20200610111108-226ff32320da
 	gotest.tools v2.2.0+incompatible
 	k8s.io/apimachinery v0.18.3
-	k8s.io/kube-openapi v0.0.0-20200727223308-4c7aaf529f79
+	k8s.io/kube-openapi v0.0.0-20200811211545-daf3cbb84823
 	sigs.k8s.io/yaml v1.2.0
 )

pkg/network/cni/cni.go

@@ -189,7 +189,7 @@ func cniToIgniteResult(r *gocni.CNIResult) *network.Result {
 	return result
 }
 
-func (plugin *cniNetworkPlugin) RemoveContainerNetwork(containerID string, isRunning bool) (err error) {
+func (plugin *cniNetworkPlugin) RemoveContainerNetwork(containerID string) (err error) {
 	if err = plugin.initialize(); err != nil {
 		return err
 	}
@@ -208,13 +208,10 @@ func (plugin *cniNetworkPlugin) RemoveContainerNetwork(containerID string, isRun
 		return nil
 	}
 
-	netnsPath := ""
-	if isRunning {
-		netnsPath = fmt.Sprintf(netNSPathFmt, c.PID)
-		if c.PID == 0 {
-			log.Info("CNI failed to retrieve network namespace path, PID was 0")
-			return nil
-		}
+	netnsPath := fmt.Sprintf(netNSPathFmt, c.PID)
+	if c.PID == 0 {
+		log.Info("CNI failed to retrieve network namespace path, PID was 0")
+		return nil
 	}
 
 	return plugin.cni.Remove(context.Background(), containerID, netnsPath)
@@ -239,7 +236,7 @@ func (plugin *cniNetworkPlugin) cleanupBridges(containerID string) error {
 		}
 
 		if hasBridge {
-			log.Debugf("TeardownIPMasq for container %q on CNI network %q which contains a bridge", containerID, net.Config.Name)
+			log.Debugf("Teardown IPMasq for container %q on CNI network %q which contains a bridge", containerID, net.Config.Name)
 			comment := utils.FormatComment(net.Config.Name, containerID)
 			for _, t := range result {
 				if err = ip.TeardownIPMasq(t.ip, t.chain, comment); err != nil {

pkg/network/docker/docker.go

@@ -44,7 +44,7 @@ func (plugin *dockerNetworkPlugin) SetupContainerNetwork(containerID string, _ .
 	}, nil
 }
 
-func (*dockerNetworkPlugin) RemoveContainerNetwork(_ string, _ bool) error {
+func (*dockerNetworkPlugin) RemoveContainerNetwork(_ string) error {
 	// no-op for docker, this is handled automatically
 	return nil
 }

pkg/network/types.go

@@ -21,7 +21,7 @@ type Plugin interface {
 	SetupContainerNetwork(containerID string, portmappings ...meta.PortMapping) (*Result, error)
 
 	// RemoveContainerNetwork is the method called before a container using the network plugin can be deleted
-	RemoveContainerNetwork(containerID string, isRunning bool) error
+	RemoveContainerNetwork(containerID string) error
 }
 
 type Result struct {

pkg/operations/remove.go

@@ -32,16 +32,11 @@ func CleanupVM(vm *api.VM) error {
 	// Inspect the container before trying to stop it and it gets auto-removed
 	inspectResult, _ := providers.Runtime.InspectContainer(util.NewPrefixer().Prefix(vm.GetUID()))
 
-	// If the VM is running, try to kill it first so we don't leave dangling containers
-	if vm.Running() {
-		if err := StopVM(vm, true, true); err != nil {
+	// If the VM is running, try to kill it first so we don't leave dangling containers. Otherwise, try to cleanup VM networking.
+	if err := StopVM(vm, true, true); err != nil {
+		if vm.Running() {
 			return err
 		}
-	} else {
-		// Try to cleanup VM networking
-		if err := removeNetworking(util.NewPrefixer().Prefix(vm.GetUID()), false); err != nil {
-			log.Warnf("Failed to cleanup networking for stopped container %s %q: %v", vm.GetKind(), vm.GetUID(), err)
-		}
 	}
 
 	// Remove the VM container if it exists
@@ -77,43 +72,51 @@ func RemoveVMContainer(result *runtime.ContainerInspectResult) {
 	_ = providers.Runtime.RemoveContainer(result.ID)
 }
 
-// StopVM stops or kills a VM
+// StopVM removes networking of the given VM and stops or kills it
 func StopVM(vm *api.VM, kill, silent bool) error {
 	var err error
 	container := util.NewPrefixer().Prefix(vm.GetUID())
 	action := "stop"
 
+	if !vm.Running() && !logs.Quiet {
+		log.Warnf("VM %q is not running but trying to cleanup networking for stopped container\n", vm.GetUID())
+	}
+
 	// Remove VM networking
-	if err = removeNetworking(util.NewPrefixer().Prefix(vm.GetUID()), true); err != nil {
+	if err = removeNetworking(util.NewPrefixer().Prefix(vm.GetUID())); err != nil {
+		log.Warnf("Failed to cleanup networking for stopped container %s %q: %v", vm.GetKind(), vm.GetUID(), err)
+
 		return err
 	}
 
-	// Stop or kill the VM container
-	if kill {
-		action = "kill"
-		err = providers.Runtime.KillContainer(container, signalSIGQUIT) // TODO: common constant for SIGQUIT
-	} else {
-		err = providers.Runtime.StopContainer(container, nil)
-	}
+	if vm.Running() {
+		// Stop or kill the VM container
+		if kill {
+			action = "kill"
+			err = providers.Runtime.KillContainer(container, signalSIGQUIT) // TODO: common constant for SIGQUIT
+		} else {
+			err = providers.Runtime.StopContainer(container, nil)
+		}
 
-	if err != nil {
-		return fmt.Errorf("failed to %s container for %s %q: %v", action, vm.GetKind(), vm.GetUID(), err)
-	}
+		if err != nil {
+			return fmt.Errorf("failed to %s container for %s %q: %v", action, vm.GetKind(), vm.GetUID(), err)
+		}
 
-	if silent {
-		return nil
-	}
+		if silent {
+			return nil
+		}
 
-	if logs.Quiet {
-		fmt.Println(vm.GetUID())
-	} else {
-		log.Infof("Stopped %s with name %q and ID %q", vm.GetKind(), vm.GetName(), vm.GetUID())
+		if logs.Quiet {
+			fmt.Println(vm.GetUID())
+		} else {
+			log.Infof("Stopped %s with name %q and ID %q", vm.GetKind(), vm.GetName(), vm.GetUID())
+		}
 	}
 
 	return nil
 }
 
-func removeNetworking(containerID string, isRunning bool) error {
+func removeNetworking(containerID string) error {
 	log.Infof("Removing the container with ID %q from the %q network", containerID, providers.NetworkPlugin.Name())
-	return providers.NetworkPlugin.RemoveContainerNetwork(containerID, isRunning)
+	return providers.NetworkPlugin.RemoveContainerNetwork(containerID)
 }

vendor/modules.txt

@@ -615,7 +615,7 @@ k8s.io/apimachinery/third_party/forked/golang/json
 k8s.io/apimachinery/third_party/forked/golang/reflect
 # k8s.io/klog v1.0.0
 k8s.io/klog
-# k8s.io/kube-openapi v0.0.0-20200727223308-4c7aaf529f79 => k8s.io/kube-openapi v0.0.0-20200121204235-bf4fb3bd569c
+# k8s.io/kube-openapi v0.0.0-20200811211545-daf3cbb84823 => k8s.io/kube-openapi v0.0.0-20200121204235-bf4fb3bd569c
 ## explicit
 k8s.io/kube-openapi/pkg/common
 k8s.io/kube-openapi/pkg/util/proto