CNI networking cleanup support, Docker client robustness improvements

pkg/operations/remove.go

@@ -2,8 +2,8 @@ package operations
 
 import (
 	"fmt"
-	"log"
 
+	log "github.com/sirupsen/logrus"
 	api "github.com/weaveworks/ignite/pkg/apis/ignite"
 	meta "github.com/weaveworks/ignite/pkg/apis/meta/v1alpha1"
 	"github.com/weaveworks/ignite/pkg/client"
@@ -29,8 +29,10 @@ func RemoveVM(c *client.Client, vm *api.VM) error {
 		return err
 	}
 
-	// Force-remove the VM container. Don't care about the error.
-	_ = RemoveVMContainer(vm)
+	// Remove the VM container if it exists
+	if err := RemoveVMContainer(vm); err != nil {
+		return err
+	}
 
 	if logs.Quiet {
 		fmt.Println(vm.GetUID())
@@ -42,12 +44,19 @@ func RemoveVM(c *client.Client, vm *api.VM) error {
 }
 
 func RemoveVMContainer(vm meta.Object) error {
+	containerName := util.NewPrefixer().Prefix(vm.GetUID())
+	result, err := providers.Runtime.InspectContainer(containerName)
+	if err != nil {
+		return nil // The container doesn't exist, bail out
+	}
+
 	// Remove the VM container
-	if err := providers.Runtime.RemoveContainer(util.NewPrefixer().Prefix(vm.GetUID())); err != nil {
+	if err := providers.Runtime.RemoveContainer(result.ID); err != nil {
 		return fmt.Errorf("failed to remove container for VM %q: %v", vm.GetUID(), err)
 	}
 
-	return nil
+	// Remove the CNI networking of the VM
+	return removeCNINetworking(vm.(*api.VM), result.ID)
 }
 
 // StopVM stops or kills a VM
@@ -80,3 +89,14 @@ func StopVM(vm *api.VM, kill, silent bool) error {
 
 	return nil
 }
+
+func removeCNINetworking(vm *api.VM, containerID string) error {
+	// Skip all other network modes
+	if vm.Spec.Network.Mode != api.NetworkModeCNI {
+		return nil
+	}
+
+	// Perform the removal
+	log.Printf("Trying to remove the container with ID %q from the CNI network", containerID)
+	return providers.NetworkPlugin.RemoveContainerNetwork(containerID)
+}

pkg/operations/start.go

@@ -22,8 +22,10 @@ import (
 )
 
 func StartVM(vm *api.VM, debug bool) error {
-	// Make sure the VM container does not exist. Don't care about the error.
-	_ = RemoveVMContainer(vm)
+	// Remove the VM container if it exists
+	if err := RemoveVMContainer(vm); err != nil {
+		return err
+	}
 
 	// Setup the snapshot overlay filesystem
 	if err := dmlegacy.ActivateSnapshot(vm); err != nil {

pkg/runtime/docker/client.go

@@ -8,6 +8,7 @@ import (
 
 	"github.com/docker/docker/api/types"
 	"github.com/docker/docker/api/types/container"
+	cont "github.com/docker/docker/api/types/container"
 	"github.com/docker/docker/client"
 	"github.com/docker/go-connections/nat"
 	"github.com/weaveworks/ignite/pkg/runtime"
@@ -73,6 +74,19 @@ func (dc *dockerClient) ExportImage(image string) (io.ReadCloser, string, error)
 	return rc, config.ID, err
 }
 
+func (dc *dockerClient) InspectContainer(container string) (*runtime.ContainerInspectResult, error) {
+	res, _, err := dc.client.ContainerInspectWithRaw(context.Background(), container, false)
+	if err != nil {
+		return nil, err
+	}
+
+	return &runtime.ContainerInspectResult{
+		ID:     res.ID,
+		Image:  res.Image,
+		Status: res.State.Status,
+	}, nil
+}
+
 func (dc *dockerClient) AttachContainer(container string) (err error) {
 	// TODO: Rework to perform the attach via the Docker client,
 	// this will require manual TTY and signal emulation/handling.
@@ -140,15 +154,40 @@ func (dc *dockerClient) RunContainer(image string, config *runtime.ContainerConf
 }
 
 func (dc *dockerClient) StopContainer(container string, timeout *time.Duration) error {
-	return dc.client.ContainerStop(context.Background(), container, timeout)
+	if err := dc.client.ContainerStop(context.Background(), container, timeout); err != nil {
+		return err
+	}
+
+	// Wait for the container to be stopped
+	return dc.waitForContainer(container, cont.WaitConditionNotRunning, nil)
 }
 
 func (dc *dockerClient) KillContainer(container, signal string) error {
-	return dc.client.ContainerKill(context.Background(), container, signal)
+	if err := dc.client.ContainerKill(context.Background(), container, signal); err != nil {
+		return err
+	}
+
+	// Wait for the container to be killed
+	return dc.waitForContainer(container, cont.WaitConditionNotRunning, nil)
 }
 
 func (dc *dockerClient) RemoveContainer(container string) error {
-	return dc.client.ContainerRemove(context.Background(), container, types.ContainerRemoveOptions{})
+	// Container waiting can fail if the container gets removed before
+	// we reach the waiting fence. Start the waiter in a goroutine before
+	// performing the removal to ensure proper removal detection.
+	errC := make(chan error)
+	readyC := make(chan bool)
+	go func() {
+		errC <- dc.waitForContainer(container, cont.WaitConditionRemoved, &readyC)
+	}()
+
+	<-readyC // The ready channel is used to wait until removal detection has started
+	if err := dc.client.ContainerRemove(context.Background(), container, types.ContainerRemoveOptions{}); err != nil {
+		return err
+	}
+
+	// Wait for the container to be removed
+	return <-errC
 }
 
 func (dc *dockerClient) ContainerLogs(container string) (io.ReadCloser, error) {
@@ -167,6 +206,28 @@ func (dc *dockerClient) ContainerNetNS(container string) (string, error) {
 	return getNetworkNamespace(&r)
 }
 
+func (dc *dockerClient) waitForContainer(container string, condition cont.WaitCondition, readyC *chan bool) error {
+	resultC, errC := dc.client.ContainerWait(context.Background(), container, condition)
+
+	// The ready channel can be used to wait until
+	// the container wait request has been sent to
+	// Docker to ensure proper ordering
+	if readyC != nil {
+		*readyC <- true
+	}
+
+	select {
+	case result := <-resultC:
+		if result.Error != nil {
+			return fmt.Errorf("failed to wait for container %q: %s", container, result.Error.Message)
+		}
+	case err := <-errC:
+		return err
+	}
+
+	return nil
+}
+
 func getNetworkNamespace(c *types.ContainerJSON) (string, error) {
 	if c.State.Pid == 0 {
 		// Docker reports pid 0 for an exited container.

pkg/runtime/types.go

@@ -13,6 +13,12 @@ type ImageInspectResult struct {
 	Size        int64
 }
 
+type ContainerInspectResult struct {
+	ID     string
+	Image  string
+	Status string
+}
+
 type Bind struct {
 	HostPath      string
 	ContainerPath string
@@ -36,6 +42,7 @@ type Interface interface {
 	PullImage(image string) (io.ReadCloser, error)
 	ExportImage(image string) (io.ReadCloser, string, error)
 
+	InspectContainer(container string) (*ContainerInspectResult, error)
 	AttachContainer(container string) error
 	RunContainer(image string, config *ContainerConfig, name string) (string, error)
 	StopContainer(container string, timeout *time.Duration) error

pkg/storage/cache/cache.go

@@ -37,16 +37,12 @@ func NewCache(backingStorage storage.Storage) Cache {
 	return c
 }
 
-func (c *cache) New(gvk schema.GroupVersionKind) (obj meta.Object, err error) {
-	// Request the storage to create the Object
-	obj, err = c.storage.New(gvk)
-
-	// If no errors occurred, cache it
-	if err == nil {
-		err = c.index.store(obj)
-	}
-
-	return
+func (c *cache) New(gvk schema.GroupVersionKind) (meta.Object, error) {
+	// Request the storage to create the Object. The
+	// newly generated Object has not got an UID which
+	// is required for indexing, so just return it
+	// without storing it into the cache
+	return c.storage.New(gvk)
 }
 
 func (c *cache) Get(gvk schema.GroupVersionKind, uid meta.UID) (obj meta.Object, err error) {

pkg/storage/cache/index.go

@@ -56,6 +56,12 @@ func (i *index) loadAll() ([]meta.Object, error) {
 }
 
 func store(i *index, obj meta.Object, apiType bool) error {
+	// If store is called for an invalid Object lacking an UID,
+	// panic and print the stack trace. This should never happen.
+	if obj.GetUID() == "" {
+		panic("Attempt to cache invalid Object: missing UID")
+	}
+
 	co, err := newCacheObject(i.storage, obj, apiType)
 	if err != nil {
 		return err