From 041e5b55609daf2de49a224a13b7b3857048be53 Mon Sep 17 00:00:00 2001 From: Abdullah Gharaibeh Date: Sat, 4 Jan 2020 00:55:11 -0500 Subject: [PATCH] addressed comments --- pkg/controller/daemon/daemon_controller.go | 34 +++++++------- .../daemon/daemon_controller_test.go | 45 +++++++++---------- test/e2e/apps/daemon_set.go | 2 +- 3 files changed, 40 insertions(+), 41 deletions(-) diff --git a/pkg/controller/daemon/daemon_controller.go b/pkg/controller/daemon/daemon_controller.go index a3e8c4a751e..3ef22ba55b9 100644 --- a/pkg/controller/daemon/daemon_controller.go +++ b/pkg/controller/daemon/daemon_controller.go @@ -623,11 +623,11 @@ func (dsc *DaemonSetsController) addNode(obj interface{}) { } node := obj.(*v1.Node) for _, ds := range dsList { - shouldSchedule, _, err := dsc.nodeShouldRunDaemonPod(node, ds) + shouldRun, _, err := dsc.nodeShouldRunDaemonPod(node, ds) if err != nil { continue } - if shouldSchedule { + if shouldRun { dsc.enqueueDaemonSet(ds) } } @@ -685,15 +685,15 @@ 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 { - oldShouldSchedule, oldShouldContinueRunning, err := dsc.nodeShouldRunDaemonPod(oldNode, ds) + oldShouldRun, oldShouldContinueRunning, err := dsc.nodeShouldRunDaemonPod(oldNode, ds) if err != nil { continue } - currentShouldSchedule, currentShouldContinueRunning, err := dsc.nodeShouldRunDaemonPod(curNode, ds) + currentShouldRun, currentShouldContinueRunning, err := dsc.nodeShouldRunDaemonPod(curNode, ds) if err != nil { continue } - if (oldShouldSchedule != currentShouldSchedule) || (oldShouldContinueRunning != currentShouldContinueRunning) { + if (oldShouldRun != currentShouldRun) || (oldShouldContinueRunning != currentShouldContinueRunning) { dsc.enqueueDaemonSet(ds) } } @@ -789,7 +789,7 @@ func (dsc *DaemonSetsController) podsShouldBeOnNode( ds *apps.DaemonSet, ) (nodesNeedingDaemonPods, podsToDelete []string, err error) { - shouldSchedule, shouldContinueRunning, err := dsc.nodeShouldRunDaemonPod(node, ds) + shouldRun, shouldContinueRunning, err := dsc.nodeShouldRunDaemonPod(node, ds) if err != nil { return } @@ -797,7 +797,7 @@ func (dsc *DaemonSetsController) podsShouldBeOnNode( daemonPods, exists := nodeToDaemonPods[node.Name] switch { - case shouldSchedule && !exists: + case shouldRun && !exists: // If daemon pod is supposed to be running on node, but isn't, create daemon pod. nodesNeedingDaemonPods = append(nodesNeedingDaemonPods, node.Name) case shouldContinueRunning: @@ -1054,14 +1054,14 @@ func (dsc *DaemonSetsController) updateDaemonSetStatus(ds *apps.DaemonSet, nodeL var desiredNumberScheduled, currentNumberScheduled, numberMisscheduled, numberReady, updatedNumberScheduled, numberAvailable int for _, node := range nodeList { - wantToRun, _, err := dsc.nodeShouldRunDaemonPod(node, ds) + shouldRun, _, err := dsc.nodeShouldRunDaemonPod(node, ds) if err != nil { return err } scheduled := len(nodeToDaemonPods[node.Name]) > 0 - if wantToRun { + if shouldRun { desiredNumberScheduled++ if scheduled { currentNumberScheduled++ @@ -1195,8 +1195,8 @@ func (dsc *DaemonSetsController) syncDaemonSet(key string) error { // nodeShouldRunDaemonPod checks a set of preconditions against a (node,daemonset) and returns a // summary. Returned booleans are: -// * shouldSchedule: -// Returns true when a daemonset should be scheduled to a node if a daemonset pod is not already +// * shouldRun: +// Returns true when a daemonset should run on the node if a daemonset pod is not already // running on that node. // * shouldContinueRunning: // Returns true when a daemonset should continue running on a node if a daemonset pod is already @@ -1217,24 +1217,24 @@ func (dsc *DaemonSetsController) nodeShouldRunDaemonPod(node *v1.Node, ds *apps. return false, false, err } - fitsNodeName, fitsNodeAffinity, fitsTaints := PodFitsNode(pod, node, taints) + fitsNodeName, fitsNodeAffinity, fitsTaints := Predicates(pod, node, taints) if !fitsNodeName || !fitsNodeAffinity { return false, false, nil } if !fitsTaints { - fitsNoExecuteTaint := v1helper.TolerationsTolerateTaintsWithFilter(pod.Spec.Tolerations, taints, func(t *v1.Taint) bool { + // Scheduled daemon pods should continue running if they tolerate NoExecute taint. + shouldContinueRunning := v1helper.TolerationsTolerateTaintsWithFilter(pod.Spec.Tolerations, taints, func(t *v1.Taint) bool { return t.Effect == v1.TaintEffectNoExecute }) - - return false, fitsNoExecuteTaint, nil + return false, shouldContinueRunning, nil } return true, true, nil } -// PodFitsNode Checks if a DaemonSet's pod can be scheduled on a node. -func PodFitsNode(pod *v1.Pod, node *v1.Node, taints []v1.Taint) (fitsNodeName, fitsNodeAffinity, fitsTaints bool) { +// 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) fitsTaints = v1helper.TolerationsTolerateTaintsWithFilter(pod.Spec.Tolerations, taints, func(t *v1.Taint) bool { diff --git a/pkg/controller/daemon/daemon_controller_test.go b/pkg/controller/daemon/daemon_controller_test.go index 6d9fc2ccda1..83d2edc1795 100644 --- a/pkg/controller/daemon/daemon_controller_test.go +++ b/pkg/controller/daemon/daemon_controller_test.go @@ -1539,17 +1539,16 @@ func setDaemonSetCritical(ds *apps.DaemonSet) { } func TestNodeShouldRunDaemonPod(t *testing.T) { - var shouldCreate, shouldContinueRunning bool - shouldCreate = true - shouldContinueRunning = true + shouldRun := true + shouldContinueRunning := true cases := []struct { - predicateName string - podsOnNode []*v1.Pod - nodeCondition []v1.NodeCondition - nodeUnschedulable bool - ds *apps.DaemonSet - wantToRun, shouldCreate, shouldContinueRunning bool - err error + predicateName string + podsOnNode []*v1.Pod + nodeCondition []v1.NodeCondition + nodeUnschedulable bool + ds *apps.DaemonSet + shouldRun, shouldContinueRunning bool + err error }{ { predicateName: "ShouldRunDaemonPod", @@ -1564,7 +1563,7 @@ func TestNodeShouldRunDaemonPod(t *testing.T) { }, }, }, - shouldCreate: true, + shouldRun: true, shouldContinueRunning: true, }, { @@ -1580,7 +1579,7 @@ func TestNodeShouldRunDaemonPod(t *testing.T) { }, }, }, - shouldCreate: shouldCreate, + shouldRun: shouldRun, shouldContinueRunning: true, }, { @@ -1596,7 +1595,7 @@ func TestNodeShouldRunDaemonPod(t *testing.T) { }, }, }, - shouldCreate: false, + shouldRun: false, shouldContinueRunning: false, }, { @@ -1629,7 +1628,7 @@ func TestNodeShouldRunDaemonPod(t *testing.T) { }, }, }, - shouldCreate: shouldCreate, + shouldRun: shouldRun, shouldContinueRunning: shouldContinueRunning, }, { @@ -1657,7 +1656,7 @@ func TestNodeShouldRunDaemonPod(t *testing.T) { }, }, }, - shouldCreate: shouldCreate, // This is because we don't care about the resource constraints any more and let default scheduler handle it. + shouldRun: shouldRun, // This is because we don't care about the resource constraints any more and let default scheduler handle it. shouldContinueRunning: true, }, { @@ -1685,7 +1684,7 @@ func TestNodeShouldRunDaemonPod(t *testing.T) { }, }, }, - shouldCreate: true, + shouldRun: true, shouldContinueRunning: true, }, { @@ -1703,7 +1702,7 @@ func TestNodeShouldRunDaemonPod(t *testing.T) { }, }, }, - shouldCreate: false, + shouldRun: false, shouldContinueRunning: false, }, { @@ -1721,7 +1720,7 @@ func TestNodeShouldRunDaemonPod(t *testing.T) { }, }, }, - shouldCreate: true, + shouldRun: true, shouldContinueRunning: true, }, { @@ -1755,7 +1754,7 @@ func TestNodeShouldRunDaemonPod(t *testing.T) { }, }, }, - shouldCreate: false, + shouldRun: false, shouldContinueRunning: false, }, { @@ -1789,7 +1788,7 @@ func TestNodeShouldRunDaemonPod(t *testing.T) { }, }, }, - shouldCreate: true, + shouldRun: true, shouldContinueRunning: true, }, { @@ -1806,7 +1805,7 @@ func TestNodeShouldRunDaemonPod(t *testing.T) { }, }, nodeUnschedulable: true, - shouldCreate: true, + shouldRun: true, shouldContinueRunning: true, }, } @@ -1830,8 +1829,8 @@ func TestNodeShouldRunDaemonPod(t *testing.T) { c.ds.Spec.UpdateStrategy = *strategy shouldRun, shouldContinueRunning, err := manager.nodeShouldRunDaemonPod(node, c.ds) - if shouldRun != c.shouldCreate { - t.Errorf("[%v] strategy: %v, predicateName: %v expected shouldRun: %v, got: %v", i, c.ds.Spec.UpdateStrategy.Type, c.predicateName, c.shouldCreate, shouldRun) + 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) } 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) diff --git a/test/e2e/apps/daemon_set.go b/test/e2e/apps/daemon_set.go index 330391f197a..afd3fac1554 100644 --- a/test/e2e/apps/daemon_set.go +++ b/test/e2e/apps/daemon_set.go @@ -693,7 +693,7 @@ func canScheduleOnNode(node v1.Node, ds *appsv1.DaemonSet) bool { framework.Failf("Can't test DaemonSet predicates for node %s: %v", node.Name, err) return false } - fitsNodeName, fitsNodeAffinity, fitsTaints := daemon.PodFitsNode(newPod, &node, taints) + fitsNodeName, fitsNodeAffinity, fitsTaints := daemon.Predicates(newPod, &node, taints) return fitsNodeName && fitsNodeAffinity && fitsTaints }