Merge pull request #12645 from sdminonne/labels_selector_overlaps

to add tests for RC selector overlapping and fix few comments

pkg/controller/replication/replication_controller.go

@@ -202,10 +202,10 @@ func (rm *ReplicationManager) getPodController(pod *api.Pod) *api.ReplicationCon
 		return nil
 	}
 	// In theory, overlapping controllers is user error. This sorting will not prevent
-	// osciallation of replicas in all cases, eg:
-	// rc1 (older rc): [(k1:v1)], replicas=1 rc2: [(k2:v2), (k1:v1)], replicas=2
-	// pod: [(k1:v1)] will wake both rc1 and rc2, and we will sync rc1.
-	// pod: [(k2:v2), (k1:v1)] will wake rc2 which creates a new replica.
+	// oscillation of replicas in all cases, eg:
+	// rc1 (older rc): [(k1=v1)], replicas=1 rc2: [(k2=v2)], replicas=2
+	// pod: [(k1:v1), (k2:v2)] will wake both rc1 and rc2, and we will sync rc1.
+	// pod: [(k2:v2)] will wake rc2 which creates a new replica.
 	sort.Sort(overlappingControllers(controllers))
 	return &controllers[0]
 }

pkg/kubectl/stop.go

@@ -117,19 +117,20 @@ func (reaper *ReplicationControllerReaper) Stop(namespace, name string, timeout
 
 	// The rc manager will try and detect all matching rcs for a pod's labels,
 	// and only sync the oldest one. This means if we have a pod with labels
-	// [(k1, v1)] and rcs with selectors [(k1, v2)] and [(k1, v1), (k2, v2)],
+	// [(k1: v1), (k2: v2)] and two rcs: rc1 with selector [(k1=v1)], and rc2 with selector [(k1=v1),(k2=v2)],
 	// the rc manager will sync the older of the two rcs.
 	//
 	// If there are rcs with a superset of labels, eg:
-	// deleting: (k1:v1), superset: (k2:v2, k1:v1)
+	// deleting: (k1=v1), superset: (k2=v2, k1=v1)
 	//	- It isn't safe to delete the rc because there could be a pod with labels
-	//	  (k1:v1) that isn't managed by the superset rc. We can't scale it down
-	//	  either, because there could be a pod (k2:v2, k1:v1) that it deletes
+	//	  (k1=v1) that isn't managed by the superset rc. We can't scale it down
+	//	  either, because there could be a pod (k2=v2, k1=v1) that it deletes
 	//	  causing a fight with the superset rc.
 	// If there are rcs with a subset of labels, eg:
-	// deleting: (k2:v2, k1:v1), subset: (k1: v1), superset: (k2:v2, k1:v1, k3:v3)
-	//  - It's safe to delete this rc without a scale down because all it's pods
-	//	  are being controlled by the subset rc.
+	// deleting: (k2=v2, k1=v1), subset: (k1=v1), superset: (k2=v2, k1=v1, k3=v3)
+	//  - Even if it's safe to delete this rc without a scale down because all it's pods
+	//	  are being controlled by the subset rc the code returns an error.
+
 	// In theory, creating overlapping controllers is user error, so the loop below
 	// tries to account for this logic only in the common case, where we end up
 	// with multiple rcs that have an exact match on selectors.

pkg/kubectl/stop_test.go

@@ -18,46 +18,255 @@ package kubectl
 
 import (
 	"fmt"
+	"reflect"
 	"testing"
 	"time"
 
 	"k8s.io/kubernetes/pkg/api"
 	client "k8s.io/kubernetes/pkg/client/unversioned"
 	"k8s.io/kubernetes/pkg/client/unversioned/testclient"
+	"k8s.io/kubernetes/pkg/runtime"
 )
 
 func TestReplicationControllerStop(t *testing.T) {
 	name := "foo"
 	ns := "default"
-	fake := testclient.NewSimpleFake(&api.ReplicationController{
-		ObjectMeta: api.ObjectMeta{
-			Name:      name,
-			Namespace: ns,
+	tests := []struct {
+		Name            string
+		Objs            []runtime.Object
+		StopError       error
+		StopMessage     string
+		ExpectedActions []string
+	}{
+		{
+			Name: "OnlyOneRC",
+			Objs: []runtime.Object{
+				&api.ReplicationController{ // GET
+					ObjectMeta: api.ObjectMeta{
+						Name:      name,
+						Namespace: ns,
+					},
+					Spec: api.ReplicationControllerSpec{
+						Replicas: 0,
+						Selector: map[string]string{"k1": "v1"}},
+				},
+				&api.ReplicationControllerList{ // LIST
+					Items: []api.ReplicationController{
+						{
+							ObjectMeta: api.ObjectMeta{
+								Name:      name,
+								Namespace: ns,
+							},
+							Spec: api.ReplicationControllerSpec{
+								Replicas: 0,
+								Selector: map[string]string{"k1": "v1"}},
+						},
+					},
+				},
+			},
+			StopError:       nil,
+			StopMessage:     "foo stopped",
+			ExpectedActions: []string{"get", "list", "get", "update", "get", "get", "delete"},
 		},
-		Spec: api.ReplicationControllerSpec{
-			Replicas: 0,
+		{
+			Name: "NoOverlapping",
+			Objs: []runtime.Object{
+				&api.ReplicationController{ // GET
+					ObjectMeta: api.ObjectMeta{
+						Name:      name,
+						Namespace: ns,
+					},
+					Spec: api.ReplicationControllerSpec{
+						Replicas: 0,
+						Selector: map[string]string{"k1": "v1"}},
+				},
+				&api.ReplicationControllerList{ // LIST
+					Items: []api.ReplicationController{
+						{
+							ObjectMeta: api.ObjectMeta{
+								Name:      "baz",
+								Namespace: ns,
+							},
+							Spec: api.ReplicationControllerSpec{
+								Replicas: 0,
+								Selector: map[string]string{"k3": "v3"}},
+						},
+						{
+							ObjectMeta: api.ObjectMeta{
+								Name:      name,
+								Namespace: ns,
+							},
+							Spec: api.ReplicationControllerSpec{
+								Replicas: 0,
+								Selector: map[string]string{"k1": "v1"}},
+						},
+					},
+				},
+			},
+			StopError:       nil,
+			StopMessage:     "foo stopped",
+			ExpectedActions: []string{"get", "list", "get", "update", "get", "get", "delete"},
+		},
+		{
+			Name: "OverlappingError",
+			Objs: []runtime.Object{
+
+				&api.ReplicationController{ // GET
+					ObjectMeta: api.ObjectMeta{
+						Name:      name,
+						Namespace: ns,
+					},
+					Spec: api.ReplicationControllerSpec{
+						Replicas: 0,
+						Selector: map[string]string{"k1": "v1"}},
+				},
+				&api.ReplicationControllerList{ // LIST
+					Items: []api.ReplicationController{
+						{
+							ObjectMeta: api.ObjectMeta{
+								Name:      "baz",
+								Namespace: ns,
+							},
+							Spec: api.ReplicationControllerSpec{
+								Replicas: 0,
+								Selector: map[string]string{"k1": "v1", "k2": "v2"}},
+						},
+						{
+							ObjectMeta: api.ObjectMeta{
+								Name:      name,
+								Namespace: ns,
+							},
+							Spec: api.ReplicationControllerSpec{
+								Replicas: 0,
+								Selector: map[string]string{"k1": "v1"}},
+						},
+					},
+				},
+			},
+			StopError:       fmt.Errorf("Detected overlapping controllers for rc foo: baz, please manage deletion individually with --cascade=false."),
+			StopMessage:     "",
+			ExpectedActions: []string{"get", "list"},
+		},
+
+		{
+			Name: "OverlappingButSafeDelete",
+			Objs: []runtime.Object{
+
+				&api.ReplicationController{ // GET
+					ObjectMeta: api.ObjectMeta{
+						Name:      name,
+						Namespace: ns,
+					},
+					Spec: api.ReplicationControllerSpec{
+						Replicas: 0,
+						Selector: map[string]string{"k1": "v1", "k2": "v2"}},
+				},
+				&api.ReplicationControllerList{ // LIST
+					Items: []api.ReplicationController{
+						{
+							ObjectMeta: api.ObjectMeta{
+								Name:      "baz",
+								Namespace: ns,
+							},
+							Spec: api.ReplicationControllerSpec{
+								Replicas: 0,
+								Selector: map[string]string{"k1": "v1", "k2": "v2", "k3": "v3"}},
+						},
+						{
+							ObjectMeta: api.ObjectMeta{
+								Name:      "zaz",
+								Namespace: ns,
+							},
+							Spec: api.ReplicationControllerSpec{
+								Replicas: 0,
+								Selector: map[string]string{"k1": "v1"}},
+						},
+						{
+							ObjectMeta: api.ObjectMeta{
+								Name:      name,
+								Namespace: ns,
+							},
+							Spec: api.ReplicationControllerSpec{
+								Replicas: 0,
+								Selector: map[string]string{"k1": "v1", "k2": "v2"}},
+						},
+					},
+				},
+			},
+
+			StopError:       fmt.Errorf("Detected overlapping controllers for rc foo: baz,zaz, please manage deletion individually with --cascade=false."),
+			StopMessage:     "",
+			ExpectedActions: []string{"get", "list"},
+		},
+
+		{
+			Name: "TwoExactMatchRCs",
+			Objs: []runtime.Object{
+
+				&api.ReplicationController{ // GET
+					ObjectMeta: api.ObjectMeta{
+						Name:      name,
+						Namespace: ns,
+					},
+					Spec: api.ReplicationControllerSpec{
+						Replicas: 0,
+						Selector: map[string]string{"k1": "v1"}},
+				},
+				&api.ReplicationControllerList{ // LIST
+					Items: []api.ReplicationController{
+						{
+							ObjectMeta: api.ObjectMeta{
+								Name:      "zaz",
+								Namespace: ns,
+							},
+							Spec: api.ReplicationControllerSpec{
+								Replicas: 0,
+								Selector: map[string]string{"k1": "v1"}},
+						},
+						{
+							ObjectMeta: api.ObjectMeta{
+								Name:      name,
+								Namespace: ns,
+							},
+							Spec: api.ReplicationControllerSpec{
+								Replicas: 0,
+								Selector: map[string]string{"k1": "v1"}},
+						},
+					},
+				},
+			},
+
+			StopError:       nil,
+			StopMessage:     "foo stopped",
+			ExpectedActions: []string{"get", "list", "delete"},
 		},
-	})
-	reaper := ReplicationControllerReaper{fake, time.Millisecond, time.Millisecond}
-	s, err := reaper.Stop(ns, name, 0, nil)
-	if err != nil {
-		t.Errorf("unexpected error: %v", err)
 	}
-	expected := "foo stopped"
-	if s != expected {
-		t.Errorf("expected %s, got %s", expected, s)
-	}
-	actions := fake.Actions()
-	if len(actions) != 7 {
-		t.Errorf("unexpected actions: %v, expected 6 actions (get, list, get, update, get, get, delete)", fake.Actions)
-	}
-	for i, verb := range []string{"get", "list", "get", "update", "get", "get", "delete"} {
-		if actions[i].GetResource() != "replicationcontrollers" {
-			t.Errorf("unexpected action: %+v, expected %s-replicationController", actions[i], verb)
+
+	for _, test := range tests {
+		fake := testclient.NewSimpleFake(test.Objs...)
+		reaper := ReplicationControllerReaper{fake, time.Millisecond, time.Millisecond}
+		s, err := reaper.Stop(ns, name, 0, nil)
+		if !reflect.DeepEqual(err, test.StopError) {
+			t.Errorf("%s unexpected error: %v", test.Name, err)
 			continue
 		}
-		if actions[i].GetVerb() != verb {
-			t.Errorf("unexpected action: %+v, expected %s-replicationController", actions[i], verb)
+
+		if s != test.StopMessage {
+			t.Errorf("%s expected '%s', got '%s'", test.Name, test.StopMessage, s)
+			continue
+		}
+		actions := fake.Actions()
+		if len(actions) != len(test.ExpectedActions) {
+			t.Errorf("%s unexpected actions: %v, expected %d actions got %d", test.Name, actions, len(test.ExpectedActions), len(actions))
+			continue
+		}
+		for i, verb := range test.ExpectedActions {
+			if actions[i].GetResource() != "replicationcontrollers" {
+				t.Errorf("%s unexpected action: %+v, expected %s-replicationController", test.Name, actions[i], verb)
+			}
+			if actions[i].GetVerb() != verb {
+				t.Errorf("%s unexpected action: %+v, expected %s-replicationController", test.Name, actions[i], verb)
+			}
 		}
 	}
 }