orchestrator: Use task history to evaluate restart policy #2332

aaronlehmann · 2017-07-26T00:46:46Z

Previously, restart conditions other than "OnAny" were honored on a best-effort basis. A service-level reconciliation, for example after a leader election, would see that not enough tasks were running, and start replacement tasks regardless of the restart policy. This limited the usefulness of the other restart conditions.

This change is similar to #2327, but instead of adding a DontRestart flag, the orchestrator figures out on the fly whether a task should be restarted or not, using historic tasks for context.

In addition to changing the reconciliation logic, this adds manager-side timestamps to TaskStatus, and changes the task reaper to preserve at least MaxAttempts + 1 tasks. It has the side effect of allowing the restart policy to be applied correctly across leader elections (previously, this state was tracked locally).

It's an alternative to both #2290 and #2327.

ping @aluzzardi @cyli

codecov · 2017-07-26T00:58:27Z

Codecov Report

Merging #2332 into master will decrease coverage by 0.07%.
The diff coverage is 83.07%.

@@            Coverage Diff             @@
##           master    #2332      +/-   ##
==========================================
- Coverage   60.35%   60.28%   -0.08%     
==========================================
  Files         128      128              
  Lines       26002    26048      +46     
==========================================
+ Hits        15694    15702       +8     
- Misses       8910     8944      +34     
- Partials     1398     1402       +4

cyli · 2017-07-26T22:02:15Z

manager/orchestrator/restart/restart.go

-		serviceID: t.ServiceID,
+	instanceTuple := orchestrator.SlotTuple{
+		Slot:      t.Slot,
+		ServiceID: t.ServiceID,
 	}

 	// Instance is not meaningful for "global" tasks, so they need to be


Non-blocking: "Slot ID" instead of "Instance" in this comment, maybe?

cyli · 2017-07-26T22:49:43Z

manager/orchestrator/restart/restart.go


 	var next *list.Element
 	for e := restartInfo.restartedInstances.Front(); e != nil; e = next {
 		next = e.Next()

 		if e.Value.(restartedInstance).timestamp.After(lookback) {
+			for e2 := restartInfo.restartedInstances.Back(); e2 != nil; e2 = e2.Prev() {


Apologies, I don't know this super well, so I would like to verify my understanding:

restartInfo.restartedInstances is a doubly-linked list where the front is the oldest restarted instance, and the back is the newest restarted instance?

This block does 2 things simultaneously: clearing any recorded history that happened before lookback, and also identifying the number of restarts between lookback and the timestamp for this task?

If so, it might be slightly easier to read if this were broken into 2 separate loops - one that reaps restart history, and the other that ignores any restarts that happened after the task status timestamp.

But if it truncates the restart history relative to the timestamp of the task, wouldn't calling ShouldRestart on a later task truncate history that might be needed for an earlier task? Or would that history never be needed again?

Also, what would cause restartedInstances to have more restarts than the last status update of the task?

restartInfo.restartedInstances is a doubly-linked list where the front is the oldest restarted instance, and the back is the newest restarted instance?

Correct

This block does 2 things simultaneously: clearing any recorded history that happened before lookback, and also identifying the number of restarts between lookback and the timestamp for this task?

Yeah, this is a mess.

If so, it might be slightly easier to read if this were broken into 2 separate loops - one that reaps restart history, and the other that ignores any restarts that happened after the task status timestamp.

I agree. There's no reason for them to be nested. Thanks for the suggestion. I've moved the inner loop after the outer one. This actually revealed a bug, because the wrong iteration variable was being used :(.

But if it truncates the restart history relative to the timestamp of the task, wouldn't calling ShouldRestart on a later task truncate history that might be needed for an earlier task? Or would that history never be needed again?

That's right that the history would never be needed again. We would only ever make a restart decision for the latest task in a slot.

I just discovered a minor case where we do expect to have the history for older tasks, and I've pushed a fix for it.

Also, what would cause restartedInstances to have more restarts than the last status update of the task?

ShouldRestart gets called on older tasks as well, for example when updatableAndDeadSlots calls IsTaskUpdatable. That said, I took a closer look at this and the logic is pretty bogus. An older task could get updated by the updater, which doesn't really make sense - it should only consider the most recent task. I've pushed another commit that redoes this. Now shouldRestart should only be called on the most recent task. I've left the loop that ignores restarts which happened after the task was restarted, because it seems most correct to have it in place, but it shouldn't be necessary anymore, and I'm fine with removing it.

I've left the loop that ignores restarts which happened after the task was restarted, because it seems most correct to have it in place, but it shouldn't be necessary anymore, and I'm fine with removing it.

I'm good with leaving it in - as you said, it's not wrong.

cyli · 2017-07-26T23:42:05Z

api/types.proto

+
+	// AppliedBy gives the node ID of the manager that applied this task
+	// status update to the Task object.
+	string applied_by = 7;


Non-blocking question - more information is always helpful for debugging, but what is this intended to be used for?

The thinking is that since we're storing a timestamp, it's useful to know what frame of reference that timestamp comes from, in case we later add logic to handle clock skew.

I'm fine with removing this for now. I suppose it could be useful for debugging, but that's not a very strong argument for having it.

That'd be a cool thing to have eventually - would we store historical skew data between the manager nodes for the adjustment?

aluzzardi

LGTM

cyli · 2017-07-27T20:25:50Z

manager/orchestrator/replicated/update_test.go

@@ -284,10 +284,6 @@ func testUpdaterRollback(t *testing.T, rollbackFailureAction api.UpdateConfig_Fa
 	assert.Equal(t, observedTask.Status.State, api.TaskStateNew)
 	assert.Equal(t, observedTask.Spec.GetContainer().Image, "image1")

-	observedTask = testutils.WatchTaskCreate(t, watchCreate)


Why does the rollback only create 2 of the old tasks in this case?

I had an elaborate explanation ready to go for this, but I went to verify it, and it turned out to be incorrect. The updater actually does roll back all three tasks. I've restored the check.

This timestamp will be useful for tracking restart history, since the manager clocks may more trustworthy than agent clocks. Signed-off-by: Aaron Lehmann <[email protected]>

Previously, restart conditions other than "OnAny" were honored on a best-effort basis. A service-level reconciliation, for example after a leader election, would see that not enough tasks were running, and start replacement tasks regardless of the restart policy. This limited the usefulness of the other restart conditions. This change makes the orchestrators check historic tasks to figure out if a task should be restarted or not, on service reconciliation. Signed-off-by: Aaron Lehmann <[email protected]>

Signed-off-by: Aaron Lehmann <[email protected]>

…iliation decisions Only look at the most recent task to see if it should be restarted. Signed-off-by: Aaron Lehmann <[email protected]>

Signed-off-by: Aaron Lehmann <[email protected]>

aaronlehmann · 2017-07-27T20:39:19Z

This PR passes integration-cli tests.

cyli · 2017-07-27T20:56:36Z

LGTM, thanks for all the explanations!

aaronlehmann · 2017-07-27T20:58:43Z

Let's go for it then. Hopefully this doesn't cause any regressions - it's a fairly involved change. But I think we found a good model and we'll fix a longstanding orchestration issue.

cyli reviewed Jul 26, 2017

View reviewed changes

aluzzardi approved these changes Jul 27, 2017

View reviewed changes

cyli reviewed Jul 27, 2017

View reviewed changes

aaronlehmann added 8 commits July 27, 2017 13:36

Timestamp task status updates on the manager side as well

1ac8c8a

This timestamp will be useful for tracking restart history, since the manager clocks may more trustworthy than agent clocks. Signed-off-by: Aaron Lehmann <[email protected]>

Reconstruct task restart history based on historic tasks

a7a78fb

Signed-off-by: Aaron Lehmann <[email protected]>

Change the task reaper to always retained at least MaxAttempts+1 tasks

4f6cd4d

Signed-off-by: Aaron Lehmann <[email protected]>

updater: Simpler and more reliable skipping of already-shutdown tasks

0976bbd

Signed-off-by: Aaron Lehmann <[email protected]>

restart: Clean up for loops

112794b

Signed-off-by: Aaron Lehmann <[email protected]>

orchestrator: Don't get confused by historic tasks when making reconc…

d489fa5

…iliation decisions Only look at the most recent task to see if it should be restarted. Signed-off-by: Aaron Lehmann <[email protected]>

restart: Instance -> Slot

1b7b99d

Signed-off-by: Aaron Lehmann <[email protected]>

aaronlehmann force-pushed the restart-based-on-task-history branch from 50f3d19 to 1b7b99d Compare July 27, 2017 20:37

aaronlehmann merged commit 253ec71 into moby:master Jul 27, 2017

This was referenced Jul 27, 2017

Design issue: Desired state, restart policy and orchestrator #932

Closed

orchestrator: Flag tasks that shouldn't be restarted #2327

Closed

[WIP] Respect restart policy during service reconciliation #2290

Closed

aaronlehmann mentioned this pull request Oct 5, 2017

Swarm mode in 17.09.0-ce not replace completed/failed tasks as designed moby/moby#35080

Open

thaJeztah mentioned this pull request Jul 24, 2018

[orchestrator] Fix task sorting #2712

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

orchestrator: Use task history to evaluate restart policy #2332

orchestrator: Use task history to evaluate restart policy #2332

aaronlehmann commented Jul 26, 2017

codecov bot commented Jul 26, 2017 •

edited

Loading

cyli Jul 26, 2017

cyli Jul 26, 2017

aaronlehmann Jul 27, 2017

cyli Jul 27, 2017

cyli Jul 26, 2017

aaronlehmann Jul 26, 2017

cyli Jul 27, 2017

aluzzardi left a comment

cyli Jul 27, 2017

aaronlehmann Jul 27, 2017

aaronlehmann commented Jul 27, 2017

cyli commented Jul 27, 2017

aaronlehmann commented Jul 27, 2017

orchestrator: Use task history to evaluate restart policy #2332

orchestrator: Use task history to evaluate restart policy #2332

Conversation

aaronlehmann commented Jul 26, 2017

codecov bot commented Jul 26, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aluzzardi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronlehmann commented Jul 27, 2017

cyli commented Jul 27, 2017

aaronlehmann commented Jul 27, 2017

codecov bot commented Jul 26, 2017 •

edited

Loading