From fac372d0905defdd32e25da1bdeb446dce15bf9c Mon Sep 17 00:00:00 2001 From: Anthony Yeh Date: Tue, 7 Mar 2017 15:01:11 -0800 Subject: [PATCH] DaemonSet: Relist Pods before each phase of sync. The design of DaemonSet requires a relist before each phase (manage, update, status) because it does not short-circuit and requeue for each action triggered. --- pkg/controller/daemon/daemoncontroller.go | 40 +++++++++++-------- .../daemon/daemoncontroller_test.go | 26 ++++++++++++ pkg/controller/daemon/update.go | 7 +++- 3 files changed, 55 insertions(+), 18 deletions(-) diff --git a/pkg/controller/daemon/daemoncontroller.go b/pkg/controller/daemon/daemoncontroller.go index 419ecdbcd6f..8d86376593c 100644 --- a/pkg/controller/daemon/daemoncontroller.go +++ b/pkg/controller/daemon/daemoncontroller.go @@ -504,7 +504,13 @@ func (dsc *DaemonSetsController) resolveControllerRef(namespace string, controll return ds } -func (dsc *DaemonSetsController) manage(ds *extensions.DaemonSet, nodeToDaemonPods map[string][]*v1.Pod) error { +func (dsc *DaemonSetsController) manage(ds *extensions.DaemonSet) error { + // Find out which nodes are running the daemon pods controlled by ds. + nodeToDaemonPods, err := dsc.getNodesToDaemonPods(ds) + if err != nil { + return fmt.Errorf("couldn't get node to daemon pod mapping for daemon set %q: %v", ds.Name, err) + } + // For each node, if the node is running the daemon pod but isn't supposed to, kill the daemon // pod. If the node is supposed to run the daemon pod, but isn't, create the daemon pod on the node. nodeList, err := dsc.nodeLister.List(labels.Everything()) @@ -682,8 +688,12 @@ func storeDaemonSetStatus(dsClient unversionedextensions.DaemonSetInterface, ds return updateErr } -func (dsc *DaemonSetsController) updateDaemonSetStatus(ds *extensions.DaemonSet, nodeToDaemonPods map[string][]*v1.Pod) error { +func (dsc *DaemonSetsController) updateDaemonSetStatus(ds *extensions.DaemonSet) error { glog.V(4).Infof("Updating daemon set status") + nodeToDaemonPods, err := dsc.getNodesToDaemonPods(ds) + if err != nil { + return fmt.Errorf("couldn't get node to daemon pod mapping for daemon set %q: %v", ds.Name, err) + } nodeList, err := dsc.nodeLister.List(labels.Everything()) if err != nil { @@ -760,12 +770,6 @@ func (dsc *DaemonSetsController) syncDaemonSet(key string) error { return nil } - // Find out which nodes are running the daemon pods controlled by ds. - nodeToDaemonPods, err := dsc.getNodesToDaemonPods(ds) - if err != nil { - return fmt.Errorf("couldn't get node to daemon pod mapping for daemon set %q: %v", ds.Name, err) - } - // Don't process a daemon set until all its creations and deletions have been processed. // For example if daemon set foo asked for 3 new daemon pods in the previous call to manage, // then we do not want to call manage on foo until the daemon pods have been created. @@ -773,25 +777,27 @@ func (dsc *DaemonSetsController) syncDaemonSet(key string) error { if err != nil { return fmt.Errorf("couldn't get key for object %#v: %v", ds, err) } - dsNeedsSync := dsc.expectations.SatisfiedExpectations(dsKey) - if dsNeedsSync && ds.DeletionTimestamp == nil { - if err := dsc.manage(ds, nodeToDaemonPods); err != nil { - return err - } + if ds.DeletionTimestamp != nil || !dsc.expectations.SatisfiedExpectations(dsKey) { + // Only update status. + return dsc.updateDaemonSetStatus(ds) } - dsNeedsSync = dsc.expectations.SatisfiedExpectations(dsKey) - if dsNeedsSync && ds.DeletionTimestamp == nil { + if err := dsc.manage(ds); err != nil { + return err + } + + // Process rolling updates if we're ready. + if dsc.expectations.SatisfiedExpectations(dsKey) { switch ds.Spec.UpdateStrategy.Type { case extensions.RollingUpdateDaemonSetStrategyType: - err = dsc.rollingUpdate(ds, nodeToDaemonPods) + err = dsc.rollingUpdate(ds) } if err != nil { return err } } - return dsc.updateDaemonSetStatus(ds, nodeToDaemonPods) + return dsc.updateDaemonSetStatus(ds) } // nodeShouldRunDaemonPod checks a set of preconditions against a (node,daemonset) and returns a diff --git a/pkg/controller/daemon/daemoncontroller_test.go b/pkg/controller/daemon/daemoncontroller_test.go index b41ba545840..120a6aa0de9 100644 --- a/pkg/controller/daemon/daemoncontroller_test.go +++ b/pkg/controller/daemon/daemoncontroller_test.go @@ -346,6 +346,32 @@ func TestSimpleDaemonSetLaunchesPods(t *testing.T) { syncAndValidateDaemonSets(t, manager, ds, podControl, 5, 0) } +func TestSimpleDaemonSetUpdatesStatusAfterLaunchingPods(t *testing.T) { + manager, podControl, clientset := newTestController() + + var updated *extensions.DaemonSet + clientset.PrependReactor("update", "daemonsets", func(action core.Action) (handled bool, ret runtime.Object, err error) { + if action.GetSubresource() != "status" { + return false, nil, nil + } + if u, ok := action.(core.UpdateAction); ok { + updated = u.GetObject().(*extensions.DaemonSet) + } + return false, nil, nil + }) + + ds := newDaemonSet("foo") + manager.dsStore.Add(ds) + addNodes(manager.nodeStore, 0, 5, nil) + syncAndValidateDaemonSets(t, manager, ds, podControl, 5, 0) + + // Make sure the single sync() updated Status already for the change made + // during the manage() phase. + if got, want := updated.Status.CurrentNumberScheduled, int32(5); got != want { + t.Errorf("Status.CurrentNumberScheduled = %v, want %v", got, want) + } +} + // DaemonSets should do nothing if there aren't any nodes func TestNoNodesDoesNothing(t *testing.T) { manager, podControl, _ := newTestController() diff --git a/pkg/controller/daemon/update.go b/pkg/controller/daemon/update.go index 44895a54a08..56645b4eb14 100644 --- a/pkg/controller/daemon/update.go +++ b/pkg/controller/daemon/update.go @@ -31,7 +31,12 @@ import ( // rollingUpdate deletes old daemon set pods making sure that no more than // ds.Spec.UpdateStrategy.RollingUpdate.MaxUnavailable pods are unavailable -func (dsc *DaemonSetsController) rollingUpdate(ds *extensions.DaemonSet, nodeToDaemonPods map[string][]*v1.Pod) error { +func (dsc *DaemonSetsController) rollingUpdate(ds *extensions.DaemonSet) error { + nodeToDaemonPods, err := dsc.getNodesToDaemonPods(ds) + if err != nil { + return fmt.Errorf("couldn't get node to daemon pod mapping for daemon set %q: %v", ds.Name, err) + } + _, oldPods, err := dsc.getAllDaemonSetPods(ds, nodeToDaemonPods) maxUnavailable, numUnavailable, err := dsc.getUnavailableNumbers(ds, nodeToDaemonPods) if err != nil {