diff --git a/pkg/controller/replication/replication_controller.go b/pkg/controller/replication/replication_controller.go index 517feb9af5e..b303c71233e 100644 --- a/pkg/controller/replication/replication_controller.go +++ b/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] } diff --git a/pkg/kubectl/stop.go b/pkg/kubectl/stop.go index 345926d8402..7cf92bf20d4 100644 --- a/pkg/kubectl/stop.go +++ b/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. diff --git a/pkg/kubectl/stop_test.go b/pkg/kubectl/stop_test.go index 6b2e34cde80..e4acce66140 100644 --- a/pkg/kubectl/stop_test.go +++ b/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) + } } } }