Merge pull request #1117 from weaveworks/feature/careful-sync-order

Sort resources using dependency order before applying

Makefile

@@ -48,7 +48,7 @@ build/.%.done: docker/Dockerfile.%
 		-f build/docker/$*/Dockerfile.$* ./build/docker/$*
 	touch $@
 
-build/.flux.done: build/fluxd build/kubectl docker/ssh_config
+build/.flux.done: build/fluxd build/kubectl docker/ssh_config docker/kubeconfig
 build/.helm-operator.done: build/helm-operator build/kubectl docker/ssh_config
 
 build/fluxd: $(FLUXD_DEPS)

cluster/kubernetes/kubernetes.go

@@ -45,13 +45,15 @@ type extendedClient struct {
 
 // --- internal types for keeping track of syncing
 
+type metadata struct {
+	Name      string `yaml:"name"`
+	Namespace string `yaml:"namespace"`
+}
+
 type apiObject struct {
 	resource.Resource
-	Kind     string `yaml:"kind"`
-	Metadata struct {
-		Name      string `yaml:"name"`
-		Namespace string `yaml:"namespace"`
-	} `yaml:"metadata"`
+	Kind     string   `yaml:"kind"`
+	Metadata metadata `yaml:"metadata"`
 }
 
 // A convenience for getting an minimal object from some bytes.
@@ -99,30 +101,6 @@ func isAddon(obj k8sObject) bool {
 
 // --- /add ons
 
-type changeSet struct {
-	nsObjs   map[string][]*apiObject
-	noNsObjs map[string][]*apiObject
-}
-
-func makeChangeSet() changeSet {
-	return changeSet{
-		nsObjs:   make(map[string][]*apiObject),
-		noNsObjs: make(map[string][]*apiObject),
-	}
-}
-
-func (c *changeSet) stage(cmd string, o *apiObject) {
-	if o.hasNamespace() {
-		c.nsObjs[cmd] = append(c.nsObjs[cmd], o)
-	} else {
-		c.noNsObjs[cmd] = append(c.noNsObjs[cmd], o)
-	}
-}
-
-type Applier interface {
-	apply(log.Logger, changeSet) cluster.SyncError
-}
-
 // Cluster is a handle to a Kubernetes API server.
 // (Typically, this code is deployed into the same cluster.)
 type Cluster struct {

cluster/kubernetes/release.go → cluster/kubernetes/sync.go

@@ -5,15 +5,34 @@ import (
 	"fmt"
 	"io"
 	"os/exec"
+	"sort"
 	"strings"
 	"time"
 
+	rest "k8s.io/client-go/rest"
+
 	"github.com/go-kit/kit/log"
 	"github.com/pkg/errors"
 	"github.com/weaveworks/flux/cluster"
-	rest "k8s.io/client-go/rest"
 )
 
+type changeSet struct {
+	objs map[string][]*apiObject
+}
+
+func makeChangeSet() changeSet {
+	return changeSet{objs: make(map[string][]*apiObject)}
+}
+
+func (c *changeSet) stage(cmd string, o *apiObject) {
+	c.objs[cmd] = append(c.objs[cmd], o)
+}
+
+// Applier is something that will apply a changeset to the cluster.
+type Applier interface {
+	apply(log.Logger, changeSet) cluster.SyncError
+}
+
 type Kubectl struct {
 	exe    string
 	config *rest.Config
@@ -52,9 +71,50 @@ func (c *Kubectl) connectArgs() []string {
 	return args
 }
 
+// rankOfKind returns an int denoting the position of the given kind
+// in the partial ordering of Kubernetes resources, according to which
+// kinds depend on which (derived by hand).
+func rankOfKind(kind string) int {
+	switch kind {
+	// Namespaces answer to NOONE
+	case "Namespace":
+		return 0
+	// These don't go in namespaces; or do, but don't depend on anything else
+	case "ServiceAccount", "ClusterRole", "Role", "PersistentVolume", "Service":
+		return 1
+	// These depend on something above, but not each other
+	case "ResourceQuota", "LimitRange", "Secret", "ConfigMap", "RoleBinding", "ClusterRoleBinding", "PersistentVolumeClaim", "Ingress":
+		return 2
+	// Same deal, next layer
+	case "DaemonSet", "Deployment", "ReplicationController", "ReplicaSet", "Job", "CronJob", "StatefulSet":
+		return 3
+	// Assumption: anything not mentioned isn't depended _upon_, so
+	// can come last.
+	default:
+		return 4
+	}
+}
+
+type applyOrder []*apiObject
+
+func (objs applyOrder) Len() int {
+	return len(objs)
+}
+
+func (objs applyOrder) Swap(i, j int) {
+	objs[i], objs[j] = objs[j], objs[i]
+}
+
+func (objs applyOrder) Less(i, j int) bool {
+	ranki, rankj := rankOfKind(objs[i].Kind), rankOfKind(objs[j].Kind)
+	if ranki == rankj {
+		return objs[i].Metadata.Name < objs[j].Metadata.Name
+	}
+	return ranki < rankj
+}
+
 func (c *Kubectl) apply(logger log.Logger, cs changeSet) (errs cluster.SyncError) {
-	f := func(m map[string][]*apiObject, cmd string, args ...string) {
-		objs := m[cmd]
+	f := func(objs []*apiObject, cmd string, args ...string) {
 		if len(objs) == 0 {
 			return
 		}
@@ -70,15 +130,19 @@ func (c *Kubectl) apply(logger log.Logger, cs changeSet) (errs cluster.SyncError
 		}
 	}
 
-	// When deleting resources we must ensure any resource in a non-default
-	// namespace is deleted before the namespace that it is in. Since namespace
-	// resources don't specify a namespace, this ordering guarantees that.
-	f(cs.nsObjs, "delete")
-	f(cs.noNsObjs, "delete", "--namespace", "default")
-	// Likewise, when applying resources we must ensure the namespace is applied
-	// first, so we run the commands the other way round.
-	f(cs.noNsObjs, "apply", "--namespace", "default")
-	f(cs.nsObjs, "apply")
+	// When deleting objects, the only real concern is that we don't
+	// try to delete things that have already been deleted by
+	// Kubernete's GC -- most notably, resources in a namespace which
+	// is also being deleted. GC does not have the dependency ranking,
+	// but we can use it as a shortcut to avoid the above problem at
+	// least.
+	objs := cs.objs["delete"]
+	sort.Sort(sort.Reverse(applyOrder(objs)))
+	f(objs, "delete")
+
+	objs = cs.objs["apply"]
+	sort.Sort(applyOrder(objs))
+	f(objs, "apply")
 	return errs
 }
 

cluster/kubernetes/kubernetes_test.go → cluster/kubernetes/sync_test.go

@@ -1,6 +1,7 @@
 package kubernetes
 
 import (
+	"sort"
 	"testing"
 
 	"github.com/go-kit/kit/log"
@@ -15,7 +16,7 @@ type mockApplier struct {
 }
 
 func (m *mockApplier) apply(_ log.Logger, c changeSet) cluster.SyncError {
-	if len(c.nsObjs) != 0 || len(c.noNsObjs) != 0 {
+	if len(c.objs) != 0 {
 		m.commandRun = true
 	}
 	return nil
@@ -79,3 +80,33 @@ func TestSyncMalformed(t *testing.T) {
 		t.Error("expected no commands run")
 	}
 }
+
+// TestApplyOrder checks that applyOrder works as expected.
+func TestApplyOrder(t *testing.T) {
+	objs := []*apiObject{
+		{
+			Kind: "Deployment",
+			Metadata: metadata{
+				Name: "deploy",
+			},
+		},
+		{
+			Kind: "Secret",
+			Metadata: metadata{
+				Name: "secret",
+			},
+		},
+		{
+			Kind: "Namespace",
+			Metadata: metadata{
+				Name: "namespace",
+			},
+		},
+	}
+	sort.Sort(applyOrder(objs))
+	for i, name := range []string{"namespace", "secret", "deploy"} {
+		if objs[i].Metadata.Name != name {
+			t.Errorf("Expected %q at position %d, got %q", name, i, objs[i].Metadata.Name)
+		}
+	}
+}

docker/Dockerfile.flux

@@ -28,6 +28,7 @@ RUN ssh-keyscan github.com gitlab.com bitbucket.org >> /etc/ssh/ssh_known_hosts
 # Add default SSH config, which points at the private key we'll mount
 COPY ./ssh_config /etc/ssh/ssh_config
 
+COPY ./kubeconfig /root/.kube/config
 COPY ./kubectl /usr/local/bin/
 COPY ./fluxd /usr/local/bin/
 

docker/kubeconfig

@@ -0,0 +1,12 @@
+apiVersion: v1
+clusters: []
+contexts:
+- context:
+    cluster: ""
+    namespace: default
+    user: ""
+  name: default
+current-context: default
+kind: Config
+preferences: {}
+users: []