Current coverage is 55.77% (diff: 85.57%)

Merging #1380 into master will increase coverage by 0.38%

@@             master      #1380   diff @@
==========================================
  Files            82         82          
  Lines         12885      12971    +86   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           7138       7235    +97   
+ Misses         4757       4739    -18   
- Partials        990        997     +7   

Powered by Codecov. Last update 27fbaef...4bf8072

I also thought about adding a timeout on task startup, so that if a
task does not reach RUNNING within the given amount of time, it
counts as a failure and is taken account when evaluating if the
update is above or below its failure threshold.

This should be taken care of by the executor. If the task doesn't start in a certain amount of time we report failure.

LGTM

Updated to record the service version (from Meta) in the UpdateStatus, and propagate this version information to tasks that are created by the update. This will allow us to match up tasks with the update that created them.

Rebased. ping @aluzzardi

Discussed rollbacks with @aluzzardi and came up with a few conclusions:

• Counting any failures that happen during the update period is too non-deterministic. We're thinking of supporting a configurable monitoring period that applies on a per-task basis. In other words, if this is set to 30 seconds, a task failing during the first 30 seconds after it's updated will count as a failure, and after that it will not count as a failure. This is similar to only watching for failures during the "update delay" that optionally happens after a task is updated, but it lets us decouple the pause between updates with the monitoring period, and put in place a more useful default for the monitoring period (as opposed to the 0s default for pausing between updates).
• If a task fails during the update, and then continues to fail as it's restarted, that should only count as a single failure.
• Looking at failed tasks in the data store isn't a good way of counting failures. This is too sensitive to task reaper settings, among other things. We should either count failures with local state, a counter in UpdateStatus on the service object, or some other approach that doesn't rely on historic task objects.
• In the model where we have a configurable monitoring period that applies to each task separately, it's not clear how to make this work seamlessly when a manager fails over. Keeping a failure count in the service object is one approach, but even this is not bulletproof (a manager can fail while attempting to update this counter). Maintaining this extra state also increases complexity. For now, it may be best not to make any guarantees about accurate failure accounting across leaders, and consider adding this as a future enhancement. The worst case scenario is that a leader failover resets the failure count during an update, and inhibits a rollback that otherwise would have happened. This is a corner case that I don't know if many people care about (however, as a counterpoint, I don't really like adding special cases where things behave differently depending whether the leader changes).

I've updated this according to the comments above. I removed the ServiceVersion stuff, because I don't think it will be necessary under the model where we monitor tasks individually for specific time periods, instead of watching for failures of any tasks associated with the current version of the service. I also added MonitoringPeriod and clarified some comments to more closely match the mode of operation discussed above.

@aluzzardi PTAL

Added a commit to implement AllowedFailureFraction and MonitoringPeriod in the updater. Currently this uses only local state so the failure count will reset if there's a leader failover. However, I made it evaluate the failure fraction as totalFailures / len(dirtySlots), so if another leader takes over partway through the update, the correct proportion of task failures will trigger the failure action. I'm wondering whether this is a good idea or not, since it means that if the leader changes before updating the last task of many, and that last task fails to update, it could trigger a rollback.

Currently this change just deals with pausing the update because rollbacks aren't implemented yet. Implementing rollbacks as a next step is relatively straightforward.

Added a commit that implements rollback and adds a unit test for it. PTAL.

I was wondering about this use case:

• Start a rolling update. spec=2 previous_spec=1
• In the middle of it, do another update. spec=3 previous_spec=2
• 3 is broken - we'll rollback to 2, no questions asked
• turns out 2 was broken too - we just didn't notice it because we didn't finish the rollout

To mitigate this problem, what if we update previous_spec ONLY if an update is not already in progress? In the previous example, we would have kept spec=3 previous_spec=1

That seems reasonable. I don't know which behavior would be least surprising though.

Overall LGTM.

I have a few points I'd like to make regarding some preferences. I don't have strong arguments for them, except perhaps n2: I do really think we shouldn't add more timestamps.

1. Threshold: Should this be an absolute number? I don't have a strong opinion, absolute numbers seem more "predictable" to me (from a user's POI) but I can see the appeal for a fractional threshold

2. Status: I'd avoid adding new timestamps into UpdateStatus. Instead, ideally, a Service should have a history (service created, service scaled, service updated, ...). This allows the user to see what happened when. UpdateStatus is only there to provide real time update information (see Rolling update failure thresholds and rollback #1380 (comment)). I'd go as far as saying that there could be a single timestamp for UpdateStatus.

3. Auto rollback vs manual rollback. The bulk of the PR design LGTM (thresholds, previous spec, ...). The one thing I'm wondering about is: Should we do automatic rollback at all or should we just provide a CLI based service update --rollback? These changes allow the implementation client side (by having the client inspect UpdateStatus and push previous_spec back). A few reasons for this:

3.a) It's "more" user predictable: A rollback will look to the user as if nothing happened. Either he'll think the update didn't run or maybe she won't notice that we rolled back (tough to notice it's still running a hash rather than another one, and at some point the update was in progress)

3.b) It's more configurable. Going 'back' manually provides the same options as going forward: failure action, parallelism, delay ... Maybe if a deployment goes bad you want to go back in lockstep (parallelism = 0) with no delay.

3.c) If we have manual rollback, we can always add auto rollback later. We can't take it away though

3. Auto rollback vs manual rollback. The bulk of the PR design LGTM (thresholds, previous spec, ...). The one thing I'm wondering about is: Should we do automatic rollback at all or should we just provide a CLI based service update --rollback? These changes allow the implementation client side (by having the client inspect UpdateStatus and push previous_spec back). A few reasons for this:

3.a) It's "more" user predictable: A rollback will look to the user as if nothing happened. Either he'll think the update didn't run or maybe she won't notice that we rolled back (tough to notice it's still running a hash rather than another one, and at some point the update was in progress)

3.b) It's more configurable. Going 'back' manually provides the same options as going forward: failure action, parallelism, delay ... Maybe if a deployment goes bad you want to go back in lockstep (parallelism = 0) with no delay.

3.c) If we have manual rollback, we can always add auto rollback later. We can't take it away though

I'm almost convinced but I have a few questions/comments about manual rollback.

1. Implementing rollback on the client side would mean that "previous spec" becomes the spec from the aborted update, so running --rollback multiple times would alternate between the two specs. Is that the behavior we want?

2. After a successful rollback, the update status would show "update complete". This seems slightly confusing because from the user's perspective the update wasn't completed - it was rolled back.

If we decide to go with client-side rollback, I'm wondering about the possibility of leaving the server-side rollback parts of this PR in place so we can continue to experiment with it, but not exposing the feature through engine-api, so we have the option in the future to remove it. What do you think?

Updated PR to mark automatic rollback as experimental. The integration won't expose it for now. It will provide a way to start a rollback manually, instead (--rollback).