⚡ Quick Guide: [orchestrator/updater] Do not reschedule tasks if only placement constraints change and are satisfied by the assigned node by anshulpundir · Pull Request #2496 · moby/swarmkit · GitHub

anshulpundir · 2018-01-31T01:47:31Z

No description provided.

codecov · 2018-01-31T04:38:32Z

Codecov Report

Merging #2496 into master will decrease coverage by 0.14%.
The diff coverage is 79.16%.

@@            Coverage Diff             @@
##           master    #2496      +/-   ##
==========================================
- Coverage   61.46%   61.32%   -0.15%     
==========================================
  Files          49      131      +82     
  Lines        6898    21449   +14551     
==========================================
+ Hits         4240    13153    +8913     
- Misses       2220     6889    +4669     
- Partials      438     1407     +969

dperny · 2018-01-31T22:52:47Z


+// IsTaskPlacementConstraintsOnlyDirty checks if the Placement field alone
+// in the spec has changed.
+func IsTaskPlacementConstraintsOnlyDirty(s *api.Service, t *api.Task) bool {


Needs a unit test IMHOIRL.

Also IMO IsTaskDirtyPlacementConstraintsOnly would be a better name.

Agreed on both.

areOnlyTaskPlacementConstraintsDirty?

dperny · 2018-01-31T22:54:30Z

+		return true
+	}
+
+	constraints, _ := constraint.Parse(u.newService.Spec.Task.Placement.Constraints)


Why toss the error? Is it guaranteed that the constraint will parse correctly? If so, can you add a comment to that effect?

FWIW I believe there are other places where we ignore the error. The idea is that it's validated elsewhere, and if it's somehow invalid, there's nothing that can be done here except spamming the logs, which we probably don't want to do. If parsing fails, constraints will be empty, which is the fallback behavior we would want anyway.

dperny · 2018-01-31T22:56:47Z

+	constraints, _ := constraint.Parse(u.newService.Spec.Task.Placement.Constraints)
+
+	match := false
+	u.store.Update(func(tx store.Tx) error {


Seems like this should be a u.store.View call as not to block all other readers, no?

nishanttotla · 2018-01-31T23:32:22Z

+	}
+
+	// Make a deep copy of the service and task spec for the comparison.
+	serviceTaskSpec := *s.Spec.Task.Copy()


@anshulpundir does this copy over any map objects in the spec correctly?

sungwonh · 2018-01-31T23:33:25Z

+	// Compare the task placement constraints.
+	var placementConstraintsChanged bool
+	if (s.Spec.Task.Placement != nil && t.Spec.Placement == nil) ||
+		(s.Spec.Task.Placement == nil && t.Spec.Placement != nil) ||


I think first two comparisons are not necessary because DeepEqual returns false if one of its arguments is nil.

aaronlehmann · 2018-02-01T05:11:39Z

+	// can satisfy the changed constraints.
+	if u.isTaskPlacementConstraintsOnlyDirty(t) && u.nodeMatches(t) {
+		return false
+	}


This should go inside orchestrator.IsTaskDirty. Otherwise, when the orchestrators reconcile services, they will replace tasks that were intentionally not updated here.

Thx for pointing this out. Done in next patch!

aaronlehmann · 2018-02-01T05:13:08Z

+		return true
+	}
+
+	constraints, _ := constraint.Parse(u.newService.Spec.Task.Placement.Constraints)


FWIW I believe there are other places where we ignore the error. The idea is that it's validated elsewhere, and if it's somehow invalid, there's nothing that can be done here except spamming the logs, which we probably don't want to do. If parsing fails, constraints will be empty, which is the fallback behavior we would want anyway.

aaronlehmann · 2018-02-01T05:14:13Z


+// IsTaskPlacementConstraintsOnlyDirty checks if the Placement field alone
+// in the spec has changed.
+func IsTaskPlacementConstraintsOnlyDirty(s *api.Service, t *api.Task) bool {


areOnlyTaskPlacementConstraintsDirty?

aaronlehmann · 2018-02-01T05:16:54Z

+
+	match := false
+	u.store.Update(func(tx store.Tx) error {
+		n := store.GetNode(tx, t.NodeID)


Check that the node was found (n != nil).

aaronlehmann · 2018-02-01T05:22:05Z

+	}
+
+	// Make a deep copy of the service and task spec for the comparison.
+	serviceTaskSpec := *s.Spec.Task.Copy()


aaronlehmann · 2018-02-01T05:22:30Z

+		(s.Spec.Task.Placement == nil && t.Spec.Placement != nil) ||
+		!reflect.DeepEqual(serviceTaskSpec.Placement, t.Spec.Placement) {
+		placementConstraintsChanged = true
+	}


I suggest moving this to the top of the function and returning early if placementConstraintsChanged is false, to avoid the overhead of doing deep copies and extra comparisons in the case where the placement constraints did not change.

Makes sense. Will do.

dperny · 2018-02-06T21:32:33Z

+	u.store.View(func(tx store.ReadTx) {
+		n = store.GetNode(tx, t.NodeID)
+	})
+	return orchestrator.IsTaskDirty(u.newService, t, n)


What happens if the node spec changes between the read and the check? Would it be too heavy, or cause locking conflicts, if we checked the constraints inside of the read tx?

This is a legit concern, but I think it's reasonable to expect users to not make frequent consecutive updates to the node and service specs.

Oh just saw @aaronlehmann's comment, never mind.

aaronlehmann · 2018-02-06T21:38:55Z

What happens if the node spec changes between the read and the check? Would it be too heavy, or cause locking conflicts, if we checked the constraints inside of the read tx?

The orchestrator (or is it the task reaper?) will act on the node spec change and replace any tasks that need to be replaced.

dperny · 2018-02-06T22:17:47Z

Ah, alright. LGTM then.

anshulpundir · 2018-02-07T01:09:29Z

ping @dperny @nishanttotla @aaronlehmann

nishanttotla · 2018-02-07T01:32:33Z


-// IsTaskDirty determines whether a task matches the given service's spec.
-func IsTaskDirty(s *api.Service, t *api.Task) bool {
+// IsTaskDirty determines whether a task matches the given service's spec and


@anshulpundir this is not a blocker, but I think this comment could be more clear. More specifically, describing the condition under which it returns true/false.

…traints change and are satisfied by the assigned node Signed-off-by: Anshul Pundir <anshul.pundir@docker.com>

anshulpundir changed the title ~~[orchestrator/updater] Do not reschedule tasks if only placement constraints change and are satisfied by the assigned node~~ [orchestrator/updater] Do not reschedule tasks if only placement constraints change and are satisfied by the assigned node [WIP] Jan 31, 2018

anshulpundir requested review from aaronlehmann and nishanttotla January 31, 2018 21:23

anshulpundir changed the title ~~[orchestrator/updater] Do not reschedule tasks if only placement constraints change and are satisfied by the assigned node [WIP]~~ [orchestrator/updater] Do not reschedule tasks if only placement constraints change and are satisfied by the assigned node Jan 31, 2018

nishanttotla added the area/orchestration label Jan 31, 2018

dperny requested changes Jan 31, 2018

View reviewed changes

nishanttotla reviewed Jan 31, 2018

View reviewed changes

sungwonh reviewed Jan 31, 2018

View reviewed changes

aaronlehmann reviewed Feb 1, 2018

View reviewed changes

anshulpundir force-pushed the restart branch from 93d29be to 104b5ad Compare February 6, 2018 01:22

dperny reviewed Feb 6, 2018

View reviewed changes

dperny approved these changes Feb 6, 2018

View reviewed changes

anshulpundir force-pushed the restart branch from 104b5ad to 6639dea Compare February 7, 2018 00:31

nishanttotla reviewed Feb 7, 2018

View reviewed changes

nishanttotla approved these changes Feb 7, 2018

View reviewed changes

[orchestrator/updater] Do not reschedule tasks if only placement cons…

13a9e78

…traints change and are satisfied by the assigned node Signed-off-by: Anshul Pundir <anshul.pundir@docker.com>

anshulpundir force-pushed the restart branch from 6639dea to 13a9e78 Compare February 7, 2018 18:57

nishanttotla merged commit 969a83f into moby:master Feb 7, 2018

nishanttotla mentioned this pull request Feb 9, 2018

Services' tasks should not get rescheduled if their placement constraints get updated but are still satisfied #2434

Closed

thaJeztah mentioned this pull request Feb 10, 2018

Bump SwarmKit to f74983e7c015a38a81c8642803a78b8322cf7eac moby/moby#36274

Merged

Conversation

anshulpundir commented Jan 31, 2018

Uh oh!

codecov Bot commented Jan 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaronlehmann commented Feb 6, 2018 via email

Uh oh!

dperny commented Feb 6, 2018

Uh oh!

anshulpundir commented Feb 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov Bot commented Jan 31, 2018 •

edited

Loading