From 8d8884a907a0d2948bd47ab0a61e7ee91416b939 Mon Sep 17 00:00:00 2001 From: Clayton Coleman Date: Wed, 27 Jan 2021 21:47:41 -0500 Subject: [PATCH] daemonset: Remove unnecessary error returns in strategy code The nodeShouldRunDaemonPod method does not need to return an error because there are no scenarios under which it fails. Remove the error return path for its direct calls as well. --- pkg/controller/daemon/daemon_controller.go | 55 ++++++------------- .../daemon/daemon_controller_test.go | 6 +- pkg/controller/daemon/update.go | 5 +- 3 files changed, 18 insertions(+), 48 deletions(-) diff --git a/pkg/controller/daemon/daemon_controller.go b/pkg/controller/daemon/daemon_controller.go index c6664a4eaa9..a383bb382b0 100644 --- a/pkg/controller/daemon/daemon_controller.go +++ b/pkg/controller/daemon/daemon_controller.go @@ -642,11 +642,7 @@ func (dsc *DaemonSetsController) addNode(obj interface{}) { } node := obj.(*v1.Node) for _, ds := range dsList { - shouldRun, _, err := dsc.nodeShouldRunDaemonPod(node, ds) - if err != nil { - continue - } - if shouldRun { + if shouldRun, _ := dsc.nodeShouldRunDaemonPod(node, ds); shouldRun { dsc.enqueueDaemonSet(ds) } } @@ -704,14 +700,8 @@ func (dsc *DaemonSetsController) updateNode(old, cur interface{}) { } // TODO: it'd be nice to pass a hint with these enqueues, so that each ds would only examine the added node (unless it has other work to do, too). for _, ds := range dsList { - oldShouldRun, oldShouldContinueRunning, err := dsc.nodeShouldRunDaemonPod(oldNode, ds) - if err != nil { - continue - } - currentShouldRun, currentShouldContinueRunning, err := dsc.nodeShouldRunDaemonPod(curNode, ds) - if err != nil { - continue - } + oldShouldRun, oldShouldContinueRunning := dsc.nodeShouldRunDaemonPod(oldNode, ds) + currentShouldRun, currentShouldContinueRunning := dsc.nodeShouldRunDaemonPod(curNode, ds) if (oldShouldRun != currentShouldRun) || (oldShouldContinueRunning != currentShouldContinueRunning) { dsc.enqueueDaemonSet(ds) } @@ -807,13 +797,9 @@ func (dsc *DaemonSetsController) podsShouldBeOnNode( nodeToDaemonPods map[string][]*v1.Pod, ds *apps.DaemonSet, hash string, -) (nodesNeedingDaemonPods, podsToDelete []string, err error) { - - shouldRun, shouldContinueRunning, err := dsc.nodeShouldRunDaemonPod(node, ds) - if err != nil { - return - } +) (nodesNeedingDaemonPods, podsToDelete []string) { + shouldRun, shouldContinueRunning := dsc.nodeShouldRunDaemonPod(node, ds) daemonPods, exists := nodeToDaemonPods[node.Name] switch { @@ -918,7 +904,7 @@ func (dsc *DaemonSetsController) podsShouldBeOnNode( } } - return nodesNeedingDaemonPods, podsToDelete, nil + return nodesNeedingDaemonPods, podsToDelete } // manage manages the scheduling and running of Pods of ds on nodes. @@ -936,14 +922,9 @@ func (dsc *DaemonSetsController) manage(ds *apps.DaemonSet, nodeList []*v1.Node, // pod. If the node is supposed to run the daemon pod, but isn't, create the daemon pod on the node. var nodesNeedingDaemonPods, podsToDelete []string for _, node := range nodeList { - nodesNeedingDaemonPodsOnNode, podsToDeleteOnNode, err := dsc.podsShouldBeOnNode( + nodesNeedingDaemonPodsOnNode, podsToDeleteOnNode := dsc.podsShouldBeOnNode( node, nodeToDaemonPods, ds, hash) - if err != nil { - klog.V(0).Infof("DEBUG: sync of node %s for ds %s failed: %v", node.Name, ds.Name, err) - continue - } - nodesNeedingDaemonPods = append(nodesNeedingDaemonPods, nodesNeedingDaemonPodsOnNode...) podsToDelete = append(podsToDelete, podsToDeleteOnNode...) } @@ -1124,11 +1105,7 @@ func (dsc *DaemonSetsController) updateDaemonSetStatus(ds *apps.DaemonSet, nodeL var desiredNumberScheduled, currentNumberScheduled, numberMisscheduled, numberReady, updatedNumberScheduled, numberAvailable int now := dsc.failedPodsBackoff.Clock.Now() for _, node := range nodeList { - shouldRun, _, err := dsc.nodeShouldRunDaemonPod(node, ds) - if err != nil { - return err - } - + shouldRun, _ := dsc.nodeShouldRunDaemonPod(node, ds) scheduled := len(nodeToDaemonPods[node.Name]) > 0 if shouldRun { @@ -1272,39 +1249,39 @@ func (dsc *DaemonSetsController) syncDaemonSet(key string) error { // * shouldContinueRunning: // Returns true when a daemonset should continue running on a node if a daemonset pod is already // running on that node. -func (dsc *DaemonSetsController) nodeShouldRunDaemonPod(node *v1.Node, ds *apps.DaemonSet) (bool, bool, error) { +func (dsc *DaemonSetsController) nodeShouldRunDaemonPod(node *v1.Node, ds *apps.DaemonSet) (bool, bool) { pod := NewPod(ds, node.Name) // If the daemon set specifies a node name, check that it matches with node.Name. if !(ds.Spec.Template.Spec.NodeName == "" || ds.Spec.Template.Spec.NodeName == node.Name) { - return false, false, nil + return false, false } taints := node.Spec.Taints fitsNodeName, fitsNodeAffinity, fitsTaints := Predicates(pod, node, taints) if !fitsNodeName || !fitsNodeAffinity { - return false, false, nil + return false, false } if !fitsTaints { // Scheduled daemon pods should continue running if they tolerate NoExecute taint. - _, untolerated := v1helper.FindMatchingUntoleratedTaint(taints, pod.Spec.Tolerations, func(t *v1.Taint) bool { + _, hasUntoleratedTaint := v1helper.FindMatchingUntoleratedTaint(taints, pod.Spec.Tolerations, func(t *v1.Taint) bool { return t.Effect == v1.TaintEffectNoExecute }) - return false, !untolerated, nil + return false, !hasUntoleratedTaint } - return true, true, nil + return true, true } // Predicates checks if a DaemonSet's pod can run on a node. func Predicates(pod *v1.Pod, node *v1.Node, taints []v1.Taint) (fitsNodeName, fitsNodeAffinity, fitsTaints bool) { fitsNodeName = len(pod.Spec.NodeName) == 0 || pod.Spec.NodeName == node.Name fitsNodeAffinity = pluginhelper.PodMatchesNodeSelectorAndAffinityTerms(pod, node) - _, untolerated := v1helper.FindMatchingUntoleratedTaint(taints, pod.Spec.Tolerations, func(t *v1.Taint) bool { + _, hasUntoleratedTaint := v1helper.FindMatchingUntoleratedTaint(taints, pod.Spec.Tolerations, func(t *v1.Taint) bool { return t.Effect == v1.TaintEffectNoExecute || t.Effect == v1.TaintEffectNoSchedule }) - fitsTaints = !untolerated + fitsTaints = !hasUntoleratedTaint return } diff --git a/pkg/controller/daemon/daemon_controller_test.go b/pkg/controller/daemon/daemon_controller_test.go index d480b5d82b4..1b5c27207bc 100644 --- a/pkg/controller/daemon/daemon_controller_test.go +++ b/pkg/controller/daemon/daemon_controller_test.go @@ -1983,7 +1983,6 @@ func TestNodeShouldRunDaemonPod(t *testing.T) { nodeUnschedulable bool ds *apps.DaemonSet shouldRun, shouldContinueRunning bool - err error }{ { predicateName: "ShouldRunDaemonPod", @@ -2262,7 +2261,7 @@ func TestNodeShouldRunDaemonPod(t *testing.T) { manager.podNodeIndex.Add(p) } c.ds.Spec.UpdateStrategy = *strategy - shouldRun, shouldContinueRunning, err := manager.nodeShouldRunDaemonPod(node, c.ds) + shouldRun, shouldContinueRunning := manager.nodeShouldRunDaemonPod(node, c.ds) if shouldRun != c.shouldRun { t.Errorf("[%v] strategy: %v, predicateName: %v expected shouldRun: %v, got: %v", i, c.ds.Spec.UpdateStrategy.Type, c.predicateName, c.shouldRun, shouldRun) @@ -2270,9 +2269,6 @@ func TestNodeShouldRunDaemonPod(t *testing.T) { if shouldContinueRunning != c.shouldContinueRunning { t.Errorf("[%v] strategy: %v, predicateName: %v expected shouldContinueRunning: %v, got: %v", i, c.ds.Spec.UpdateStrategy.Type, c.predicateName, c.shouldContinueRunning, shouldContinueRunning) } - if err != c.err { - t.Errorf("[%v] strategy: %v, predicateName: %v expected err: %v, got: %v", i, c.predicateName, c.ds.Spec.UpdateStrategy.Type, c.err, err) - } } } } diff --git a/pkg/controller/daemon/update.go b/pkg/controller/daemon/update.go index 0e3792ed34e..cd2ee0c72f1 100644 --- a/pkg/controller/daemon/update.go +++ b/pkg/controller/daemon/update.go @@ -521,10 +521,7 @@ func (dsc *DaemonSetsController) updatedDesiredNodeCounts(ds *apps.DaemonSet, no var desiredNumberScheduled int for i := range nodeList { node := nodeList[i] - wantToRun, _, err := dsc.nodeShouldRunDaemonPod(node, ds) - if err != nil { - return -1, -1, err - } + wantToRun, _ := dsc.nodeShouldRunDaemonPod(node, ds) if !wantToRun { continue }