From 8ad18bf2ecd76c9a50f990bf6ae6b4b179f22d3d Mon Sep 17 00:00:00 2001 From: Kenneth Owens Date: Thu, 24 Aug 2017 09:51:28 -0700 Subject: [PATCH] Ensures that the DaemonSet controller does not launch a Pod on a Node while waiting for a Pod that it has previously created to terminate. --- pkg/controller/daemon/daemon_controller.go | 12 ++++---- .../daemon/daemon_controller_test.go | 27 +++++++++++------ pkg/controller/daemon/update.go | 3 +- pkg/controller/daemon/update_test.go | 29 +++++++++++++++++++ 4 files changed, 55 insertions(+), 16 deletions(-) diff --git a/pkg/controller/daemon/daemon_controller.go b/pkg/controller/daemon/daemon_controller.go index d933eea01a0..c931df8b589 100644 --- a/pkg/controller/daemon/daemon_controller.go +++ b/pkg/controller/daemon/daemon_controller.go @@ -743,7 +743,7 @@ func (dsc *DaemonSetsController) getDaemonPods(ds *extensions.DaemonSet) ([]*v1. } // If any adoptions are attempted, we should first recheck for deletion with // an uncached quorum read sometime after listing Pods (see #42639). - canAdoptFunc := controller.RecheckDeletionTimestamp(func() (metav1.Object, error) { + dsNotDeleted := controller.RecheckDeletionTimestamp(func() (metav1.Object, error) { fresh, err := dsc.kubeClient.ExtensionsV1beta1().DaemonSets(ds.Namespace).Get(ds.Name, metav1.GetOptions{}) if err != nil { return nil, err @@ -753,8 +753,9 @@ func (dsc *DaemonSetsController) getDaemonPods(ds *extensions.DaemonSet) ([]*v1. } return fresh, nil }) + // Use ControllerRefManager to adopt/orphan as needed. - cm := controller.NewPodControllerRefManager(dsc.podControl, ds, selector, controllerKind, canAdoptFunc) + cm := controller.NewPodControllerRefManager(dsc.podControl, ds, selector, controllerKind, dsNotDeleted) return cm.ClaimPods(pods) } @@ -770,10 +771,6 @@ func (dsc *DaemonSetsController) getNodesToDaemonPods(ds *extensions.DaemonSet) // Group Pods by Node name. nodeToDaemonPods := make(map[string][]*v1.Pod) for _, pod := range claimedPods { - // Skip terminating pods - if pod.DeletionTimestamp != nil { - continue - } nodeName := pod.Spec.NodeName nodeToDaemonPods[nodeName] = append(nodeToDaemonPods[nodeName], pod) } @@ -838,6 +835,9 @@ func (dsc *DaemonSetsController) manage(ds *extensions.DaemonSet, hash string) e // If there's non-daemon pods left on this node, we will create it in the next sync loop var daemonPodsRunning []*v1.Pod for _, pod := range daemonPods { + if pod.DeletionTimestamp != nil { + continue + } if pod.Status.Phase == v1.PodFailed { msg := fmt.Sprintf("Found failed daemon pod %s/%s on node %s, will try to kill it", pod.Namespace, node.Name, pod.Name) glog.V(2).Infof(msg) diff --git a/pkg/controller/daemon/daemon_controller_test.go b/pkg/controller/daemon/daemon_controller_test.go index a708a4b9979..4deb81aff4c 100644 --- a/pkg/controller/daemon/daemon_controller_test.go +++ b/pkg/controller/daemon/daemon_controller_test.go @@ -1213,15 +1213,30 @@ func TestNodeDaemonLaunchesToleratePod(t *testing.T) { ds.Spec.UpdateStrategy = *strategy setDaemonSetToleration(ds, noScheduleTolerations) manager, podControl, _ := newTestController(ds) - - node := newNode("untainted", nil) - manager.nodeStore.Add(node) + addNodes(manager.nodeStore, 0, 1, nil) manager.dsStore.Add(ds) syncAndValidateDaemonSets(t, manager, ds, podControl, 1, 0, 0) } } +// DaemonSet should launch a pod on a not ready node with taint notReady:NoExecute. +func TestDaemonSetRespectsTermination(t *testing.T) { + for _, strategy := range updateStrategies() { + ds := newDaemonSet("foo") + ds.Spec.UpdateStrategy = *strategy + manager, podControl, _ := newTestController(ds) + + addNodes(manager.nodeStore, 0, 1, simpleNodeLabel) + pod := newPod(fmt.Sprintf("%s-", "node-0"), "node-0", simpleDaemonSetLabel, ds) + dt := metav1.Now() + pod.DeletionTimestamp = &dt + manager.podStore.Add(pod) + manager.dsStore.Add(ds) + syncAndValidateDaemonSets(t, manager, ds, podControl, 0, 0, 0) + } +} + func setNodeTaint(node *v1.Node, taints []v1.Taint) { node.Spec.Taints = taints } @@ -1837,12 +1852,6 @@ func TestGetNodesToDaemonPods(t *testing.T) { newPod("non-matching-owned-0-", "node-0", simpleDaemonSetLabel2, ds), newPod("non-matching-orphan-1-", "node-1", simpleDaemonSetLabel2, nil), newPod("matching-owned-by-other-0-", "node-0", simpleDaemonSetLabel, ds2), - func() *v1.Pod { - pod := newPod("matching-owned-2-but-set-for-deletion", "node-2", simpleDaemonSetLabel, ds) - now := metav1.Now() - pod.DeletionTimestamp = &now - return pod - }(), } for _, pod := range ignoredPods { manager.podStore.Add(pod) diff --git a/pkg/controller/daemon/update.go b/pkg/controller/daemon/update.go index d67cf336847..ea3f973ae44 100644 --- a/pkg/controller/daemon/update.go +++ b/pkg/controller/daemon/update.go @@ -408,7 +408,8 @@ func (dsc *DaemonSetsController) getUnavailableNumbers(ds *extensions.DaemonSet, } available := false for _, pod := range daemonPods { - if podutil.IsPodAvailable(pod, ds.Spec.MinReadySeconds, metav1.Now()) { + //for the purposes of update we ensure that the Pod is both available and not terminating + if podutil.IsPodAvailable(pod, ds.Spec.MinReadySeconds, metav1.Now()) && pod.DeletionTimestamp == nil { available = true break } diff --git a/pkg/controller/daemon/update_test.go b/pkg/controller/daemon/update_test.go index 452b6459699..1585f8e4e9c 100644 --- a/pkg/controller/daemon/update_test.go +++ b/pkg/controller/daemon/update_test.go @@ -21,6 +21,7 @@ import ( "k8s.io/api/core/v1" extensions "k8s.io/api/extensions/v1beta1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/intstr" ) @@ -236,6 +237,34 @@ func TestGetUnavailableNumbers(t *testing.T) { maxUnavailable: 1, numUnavailable: 0, }, + { + name: "Two nodes with pods, MaxUnavailable in percents, pod terminating", + Manager: func() *daemonSetsController { + manager, _, _ := newTestController() + addNodes(manager.nodeStore, 0, 2, nil) + return manager + }(), + ds: func() *extensions.DaemonSet { + ds := newDaemonSet("x") + intStr := intstr.FromString("50%") + ds.Spec.UpdateStrategy.RollingUpdate = &extensions.RollingUpdateDaemonSet{MaxUnavailable: &intStr} + return ds + }(), + nodeToPods: func() map[string][]*v1.Pod { + mapping := make(map[string][]*v1.Pod) + pod0 := newPod("pod-0", "node-0", simpleDaemonSetLabel, nil) + pod1 := newPod("pod-1", "node-1", simpleDaemonSetLabel, nil) + now := metav1.Now() + markPodReady(pod0) + markPodReady(pod1) + pod1.DeletionTimestamp = &now + mapping["node-0"] = []*v1.Pod{pod0} + mapping["node-1"] = []*v1.Pod{pod1} + return mapping + }(), + maxUnavailable: 1, + numUnavailable: 1, + }, } for _, c := range cases {