diff --git a/pkg/controller/controller_ref_manager.go b/pkg/controller/controller_ref_manager.go index 3fc94626805..1e95848f5cd 100644 --- a/pkg/controller/controller_ref_manager.go +++ b/pkg/controller/controller_ref_manager.go @@ -107,6 +107,12 @@ func (m *BaseControllerRefManager) ClaimObject(ctx context.Context, obj metav1.O // Ignore if the object is being deleted return false, nil } + + if m.Controller.GetNamespace() != obj.GetNamespace() { + // Ignore if namespace not match + return false, nil + } + // Selector matches. Try to adopt. if err := adopt(ctx, obj); err != nil { // If the pod no longer exists, ignore the error. diff --git a/pkg/controller/controller_ref_manager_test.go b/pkg/controller/controller_ref_manager_test.go index e06ec35bc8f..6af261b1764 100644 --- a/pkg/controller/controller_ref_manager_test.go +++ b/pkg/controller/controller_ref_manager_test.go @@ -68,19 +68,24 @@ func TestClaimPods(t *testing.T) { patches int } var tests = []test{ - { - name: "Claim pods with correct label", - manager: NewPodControllerRefManager(&FakePodControl{}, - &v1.ReplicationController{}, - productionLabelSelector, - controllerKind, - func(ctx context.Context) error { return nil }), - pods: []*v1.Pod{newPod("pod1", productionLabel, nil), newPod("pod2", testLabel, nil)}, - claimed: []*v1.Pod{newPod("pod1", productionLabel, nil)}, - patches: 1, - }, func() test { controller := v1.ReplicationController{} + controller.Namespace = metav1.NamespaceDefault + return test{ + name: "Claim pods with correct label", + manager: NewPodControllerRefManager(&FakePodControl{}, + &controller, + productionLabelSelector, + controllerKind, + func(ctx context.Context) error { return nil }), + pods: []*v1.Pod{newPod("pod1", productionLabel, nil), newPod("pod2", testLabel, nil)}, + claimed: []*v1.Pod{newPod("pod1", productionLabel, nil)}, + patches: 1, + } + }(), + func() test { + controller := v1.ReplicationController{} + controller.Namespace = metav1.NamespaceDefault controller.UID = types.UID(controllerUID) now := metav1.Now() controller.DeletionTimestamp = &now @@ -97,6 +102,7 @@ func TestClaimPods(t *testing.T) { }(), func() test { controller := v1.ReplicationController{} + controller.Namespace = metav1.NamespaceDefault controller.UID = types.UID(controllerUID) now := metav1.Now() controller.DeletionTimestamp = &now @@ -115,7 +121,9 @@ func TestClaimPods(t *testing.T) { controller := v1.ReplicationController{} controller2 := v1.ReplicationController{} controller.UID = types.UID(controllerUID) + controller.Namespace = metav1.NamespaceDefault controller2.UID = types.UID("AAAAA") + controller2.Namespace = metav1.NamespaceDefault return test{ name: "Controller can not claim pods owned by another controller", manager: NewPodControllerRefManager(&FakePodControl{}, @@ -129,6 +137,7 @@ func TestClaimPods(t *testing.T) { }(), func() test { controller := v1.ReplicationController{} + controller.Namespace = metav1.NamespaceDefault controller.UID = types.UID(controllerUID) return test{ name: "Controller releases claimed pods when selector doesn't match", @@ -144,6 +153,7 @@ func TestClaimPods(t *testing.T) { }(), func() test { controller := v1.ReplicationController{} + controller.Namespace = metav1.NamespaceDefault controller.UID = types.UID(controllerUID) podToDelete1 := newPod("pod1", productionLabel, &controller) podToDelete2 := newPod("pod2", productionLabel, nil) @@ -164,6 +174,7 @@ func TestClaimPods(t *testing.T) { }(), func() test { controller := v1.ReplicationController{} + controller.Namespace = metav1.NamespaceDefault controller.UID = types.UID(controllerUID) return test{ name: "Controller claims or release pods according to selector with finalizers", @@ -178,6 +189,25 @@ func TestClaimPods(t *testing.T) { patches: 2, } }(), + func() test { + controller := v1.ReplicationController{} + controller.Namespace = metav1.NamespaceDefault + controller.UID = types.UID(controllerUID) + pod1 := newPod("pod1", productionLabel, nil) + pod2 := newPod("pod2", productionLabel, nil) + pod2.Namespace = "fakens" + return test{ + name: "Controller does not claim pods of different namespace", + manager: NewPodControllerRefManager(&FakePodControl{}, + &controller, + productionLabelSelector, + controllerKind, + func(ctx context.Context) error { return nil }), + pods: []*v1.Pod{pod1, pod2}, + claimed: []*v1.Pod{pod1}, + patches: 1, + } + }(), } for _, test := range tests { t.Run(test.name, func(t *testing.T) { diff --git a/pkg/controller/daemon/daemon_controller_test.go b/pkg/controller/daemon/daemon_controller_test.go index bafc0cedd0d..cb7c73d6573 100644 --- a/pkg/controller/daemon/daemon_controller_test.go +++ b/pkg/controller/daemon/daemon_controller_test.go @@ -225,6 +225,19 @@ func addFailedPods(podStore cache.Store, nodeName string, label map[string]strin } } +func newControllerRevision(name string, namespace string, label map[string]string, + ownerReferences []metav1.OwnerReference) *apps.ControllerRevision { + return &apps.ControllerRevision{ + TypeMeta: metav1.TypeMeta{APIVersion: "apps/v1"}, + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Labels: label, + Namespace: namespace, + OwnerReferences: ownerReferences, + }, + } +} + type fakePodControl struct { sync.Mutex *controller.FakePodControl diff --git a/pkg/controller/daemon/update.go b/pkg/controller/daemon/update.go index cf66cdaebc5..dd487d99bbb 100644 --- a/pkg/controller/daemon/update.go +++ b/pkg/controller/daemon/update.go @@ -406,7 +406,7 @@ func (dsc *DaemonSetsController) controlledHistories(ctx context.Context, ds *ap // List all histories to include those that don't match the selector anymore // but have a ControllerRef pointing to the controller. - histories, err := dsc.historyLister.List(labels.Everything()) + histories, err := dsc.historyLister.ControllerRevisions(ds.Namespace).List(labels.Everything()) if err != nil { return nil, err } diff --git a/pkg/controller/daemon/update_test.go b/pkg/controller/daemon/update_test.go index 57a1baaf9fc..278a08a8db5 100644 --- a/pkg/controller/daemon/update_test.go +++ b/pkg/controller/daemon/update_test.go @@ -18,6 +18,7 @@ package daemon import ( "context" + "reflect" "testing" "time" @@ -635,3 +636,73 @@ func TestGetUnavailableNumbers(t *testing.T) { }) } } + +func TestControlledHistories(t *testing.T) { + ds1 := newDaemonSet("ds1") + crOfDs1 := newControllerRevision(ds1.GetName()+"-x1", ds1.GetNamespace(), ds1.Spec.Template.Labels, + []metav1.OwnerReference{*metav1.NewControllerRef(ds1, controllerKind)}) + orphanCrInSameNsWithDs1 := newControllerRevision(ds1.GetName()+"-x2", ds1.GetNamespace(), ds1.Spec.Template.Labels, nil) + orphanCrNotInSameNsWithDs1 := newControllerRevision(ds1.GetName()+"-x3", ds1.GetNamespace()+"-other", ds1.Spec.Template.Labels, nil) + cases := []struct { + name string + manager *daemonSetsController + historyCRAll []*apps.ControllerRevision + expectControllerRevisions []*apps.ControllerRevision + }{ + { + name: "controller revision in the same namespace", + manager: func() *daemonSetsController { + manager, _, _, err := newTestController(ds1, crOfDs1, orphanCrInSameNsWithDs1) + if err != nil { + t.Fatalf("error creating DaemonSets controller: %v", err) + } + manager.dsStore.Add(ds1) + return manager + }(), + historyCRAll: []*apps.ControllerRevision{crOfDs1, orphanCrInSameNsWithDs1}, + expectControllerRevisions: []*apps.ControllerRevision{crOfDs1, orphanCrInSameNsWithDs1}, + }, + { + name: "Skip adopting the controller revision in namespace other than the one in which DS lives", + manager: func() *daemonSetsController { + manager, _, _, err := newTestController(ds1, orphanCrNotInSameNsWithDs1) + if err != nil { + t.Fatalf("error creating DaemonSets controller: %v", err) + } + manager.dsStore.Add(ds1) + return manager + }(), + historyCRAll: []*apps.ControllerRevision{orphanCrNotInSameNsWithDs1}, + expectControllerRevisions: []*apps.ControllerRevision{}, + }, + } + for _, c := range cases { + for _, h := range c.historyCRAll { + c.manager.historyStore.Add(h) + } + crList, err := c.manager.controlledHistories(context.TODO(), ds1) + if err != nil { + t.Fatalf("Test case: %s. Unexpected error: %v", c.name, err) + } + if len(crList) != len(c.expectControllerRevisions) { + t.Errorf("Test case: %s, expect controllerrevision count %d but got:%d", + c.name, len(c.expectControllerRevisions), len(crList)) + } else { + // check controller revisions match + for _, cr := range crList { + found := false + for _, expectCr := range c.expectControllerRevisions { + if reflect.DeepEqual(cr, expectCr) { + found = true + break + } + } + if !found { + t.Errorf("Test case: %s, controllerrevision %v not expected", + c.name, cr) + } + } + t.Logf("Test case: %s done", c.name) + } + } +}