Make Service.UpdateStatus non-ambiguous by thaJeztah · Pull Request #2804 · moby/swarmkit

thaJeztah · 2019-01-10T16:35:38Z

See moby/moby#38520 for some background on this change

In situations where only annotations of a service are updated, SwarmKit never
marks a service as UpdateStateUpdating; the service's UpdateStatus is set
to nil, and never updated after.

Because of this, it's not possible to verify if an update actually happened /
reconciled, unless resorting to hacky solutions, such as comparing the services
Version before and after.

Even those hacky solutions are not useful, because this requires knowledge
about which conditions require tasks to be updated, and which conditions not (
and require UpdateStatus to be checked). Such knowledge should be internal to
SwarmKit, and not needed for users of the API.

This patch modifies the behavior code to try to make this situation
non-ambigious, by marking services as "updated" in all cases where the service
has reconciled.

Note that this patch causes an extra update to be performed (to write the
service's UpdateStatus to the store). If this is a concern, we could possibly
modify controlapi.UpdateService() to write the UpdateStatus immediately,
instead of setting it to nil.

thaJeztah · 2019-01-10T16:37:45Z

manager/orchestrator/update/updater.go

 	}
 }
+
+func (u *Updater) annotationsUpdated(service *api.Service) bool {


This was my naive way to double-check that no tasks had to be updated; I couldn't find a canonical way to determine if it's a spec-only change, or if tasks (containers) had to be updated; perhaps there is somewhere

what's the problem with trusting the dirtySlots check? (keeping in mind that I haven't expanded that part of the code yet)

IIUC;

The dirtySlots check checks if any of the containers (tasks) have to be updated, so that the reconciliation loop has to check for that to finish

If len(dirtySlots) == 0, no containers have to be updated

At that point, there's three possibilities;

service.UpdateStatus == updating or rollback_started - this means a reconciliation loop was started and has now finished (so completeUpdate)

service.UpdateStatus == nil, but nothing changed (annotationsUpdated == false); apparently there was nothing to update; don't update the Service.UpdateStatus, because nothing happened

service.UpdateStatus == nil and annotations changed; this means we actually updated something, and are now done; run completeUpdate to set the Service.UpdateStatus

Perhaps we don't have to take 2. into account, and always call completeUpdate update the UpdateStatus once we get at this point; I wasn't sure if there's any other detection before this to cancel the update earlier (if nothing changed); also not sure if running completeUpdate in the "nothing changed" situation would have side-effects

I think the DeepEqual is okay. When comparing a task spec to a service spec, there is some subtlety - see IsTaskDirty.

Are annotations the only things that can change without causing tasks to become dirty? I would have thought there were other cases, but can't think of any examples offhand. It might make sense to call this something like serviceSpecChanged just to be defensive.

thaJeztah · 2019-01-10T16:37:54Z

ping @dperny @aaronlehmann PTAL - this seems to work, but perhaps there's a reason for this being implemented this way

thaJeztah · 2019-01-10T16:39:14Z

manager/orchestrator/update/updater.go

+		case service.UpdateStatus == nil && force:
+			// Force marking the status as updated; to account for annotation-only updates.
+			service.UpdateStatus = &api.UpdateStatus{
+				StartedAt: ptypes.MustTimestampProto(time.Now()),


Not sure if this was really needed (setting StartedAt), but thought it would be "ok" for consistency. Happy to remove though

In situations where only annotations of a service are updated, SwarmKit never marks a service as `UpdateStateUpdating`; the service's `UpdateStatus` is set to `nil`, and never updated after. Because of this, it's not possible to verify if an update actually happened / reconciled, unless resorting to hacky solutions, such as comparing the services `Version` before and after. Even those hacky solutions are not useful, because this requires knowledge about _which_ conditions require tasks to be updated, and which conditions not ( and require `UpdateStatus` to be checked). Such knowledge should be internal to SwarmKit, and not needed for users of the API. This patch modifies the behavior code to try to make this situation non-ambigious, by marking services as "updated" in all cases where the service has reconciled. Note that this patch causes an extra update to be performed (to write the service's `UpdateStatus` to the store). If this is a concern, we could possibly modify `controlapi.UpdateService()` to write the `UpdateStatus` immediately, instead of setting it to `nil`. Signed-off-by: Sebastiaan van Stijn <[email protected]>

aaronlehmann · 2019-01-11T03:45:59Z

In situations where only annotations of a service are updated, SwarmKit never
marks a service as UpdateStateUpdating; the service's UpdateStatus is set
to nil, and never updated after.

I don't remember if there's a good reason for this. Unless it's like this for a good reason, temporarily setting UpdateStartUpdating on an annotation-only update might be a simpler solution and more consistent with other kinds of updates.

aaronlehmann · 2019-01-11T04:00:31Z

Okay, it took me a few minutes to regain the context.

I believe it's like this by design. If no tasks need to be updated, then technically no update is taking place even though the spec changed. Thus UpdateStatus is never populated.

I can see the consistency argument for always providing an UpdateStatus after making a chance to a service. I can't think of a practical downside (but don't take my word for it). The PR looks fine overall as far as I can tell.

aaronlehmann · 2019-01-11T04:02:41Z

manager/orchestrator/update/updater.go

 	}
 }
+
+func (u *Updater) annotationsUpdated(service *api.Service) bool {


I think the DeepEqual is okay. When comparing a task spec to a service spec, there is some subtlety - see IsTaskDirty.

Are annotations the only things that can change without causing tasks to become dirty? I would have thought there were other cases, but can't think of any examples offhand. It might make sense to call this something like serviceSpecChanged just to be defensive.

aaronlehmann · 2019-01-11T04:03:30Z

manager/orchestrator/update/updater.go

+			service.UpdateStatus = &api.UpdateStatus{
+				StartedAt: ptypes.MustTimestampProto(time.Now()),
+				State:     api.UpdateStatus_COMPLETED,
+				Message:   "update completed",


Maybe mention in the message that no tasks needed to be updated?

dperny · 2019-01-15T18:55:31Z

LGTM.

Reverts the change to swarmkit that made all updates set UpdateStatus to Completed Signed-off-by: Drew Erny <[email protected]>

Reverts the change to swarmkit that made all updates set UpdateStatus to Completed Signed-off-by: Drew Erny <[email protected]> (cherry picked from commit c7d9599) Signed-off-by: Sebastiaan van Stijn <[email protected]>

Revert moby/swarmkit#2804

[19.03 backport] Revert moby/swarmkit#2804

Reverts the change to swarmkit that made all updates set UpdateStatus to Completed Signed-off-by: Drew Erny <[email protected]> (cherry picked from commit c7d9599e3d95ce588113dc351facb2befac062fa) Signed-off-by: Sebastiaan van Stijn <[email protected]> Upstream-commit: d0f4f42bd4646f1741acd0fa22699bf1ac553c03 Component: engine

[19.03 backport] Revert moby/swarmkit#2804 Upstream-commit: 0678d71038ff7a253e67392675485309b971ecb8 Component: engine

Reverts the change to swarmkit that made all updates set UpdateStatus to Completed Signed-off-by: Drew Erny <[email protected]> Upstream-commit: c7d9599e3d95ce588113dc351facb2befac062fa Component: engine

Revert moby/swarmkit#2804 Upstream-commit: 54043d2f6f60e95a341e42257c4c80f86a89af6e Component: engine

Reverts the change to swarmkit that made all updates set UpdateStatus to Completed Signed-off-by: Drew Erny <[email protected]> Signed-off-by: zach <[email protected]>

thaJeztah requested a review from aaronlehmann January 10, 2019 16:35

thaJeztah commented Jan 10, 2019

View reviewed changes

thaJeztah mentioned this pull request Jan 10, 2019

Bump SwarmKit to 1a0ebd43b2d156983a695f90e56f4ecba6ced902 moby/moby#38520

Merged

thaJeztah changed the title ~~Make Service.UpdateStatus non-ambigious~~ Make Service.UpdateStatus non-ambiguous Jan 10, 2019

thaJeztah force-pushed the make_update_status_non_ambiguous branch from 1572a5e to 171d814 Compare January 10, 2019 16:42

aaronlehmann approved these changes Jan 11, 2019

View reviewed changes

dperny merged commit 47b1cd4 into moby:master Jan 15, 2019

thaJeztah deleted the make_update_status_non_ambiguous branch April 10, 2019 12:51

thaJeztah mentioned this pull request Apr 10, 2019

19.03.0-beta1: docker service scale does not always wait to converge moby/moby#39023

Closed

This was referenced May 21, 2019

[19.03.0-beta1] Overall progress does not work when service is scaled up #2861

Closed

Revert "Make Service.UpdateStatus non-ambiguous" #2862

Merged

dperny added a commit to dperny/docker that referenced this pull request May 29, 2019

Revert moby/swarmkit#2804

c7d9599

Reverts the change to swarmkit that made all updates set UpdateStatus to Completed Signed-off-by: Drew Erny <[email protected]>

dperny mentioned this pull request May 29, 2019

Revert docker/swarmkit#2804 moby/moby#39281

Merged

thaJeztah mentioned this pull request Jun 3, 2019

[19.03 backport] Revert docker/swarmkit#2804 docker-archive/engine#252

Merged

tiborvass added a commit to moby/moby that referenced this pull request Jun 4, 2019

Merge pull request #39281 from dperny/vendor-swarmkit

54043d2

Revert moby/swarmkit#2804

andrewhsu added a commit to docker-archive/engine that referenced this pull request Jun 4, 2019

Merge pull request #252 from thaJeztah/19.03_bump_swarmkit

0678d71

[19.03 backport] Revert moby/swarmkit#2804

docker-jenkins pushed a commit to docker-archive/docker-ce that referenced this pull request Jun 4, 2019

Merge pull request #252 from thaJeztah/19.03_bump_swarmkit

3e3bfc1

[19.03 backport] Revert moby/swarmkit#2804 Upstream-commit: 0678d71038ff7a253e67392675485309b971ecb8 Component: engine

docker-jenkins pushed a commit to docker-archive/docker-ce that referenced this pull request Jun 4, 2019

Merge pull request #39281 from dperny/vendor-swarmkit

a110b7f

Revert moby/swarmkit#2804 Upstream-commit: 54043d2f6f60e95a341e42257c4c80f86a89af6e Component: engine

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Service.UpdateStatus non-ambiguous#2804

Make Service.UpdateStatus non-ambiguous#2804
dperny merged 1 commit intomoby:masterfrom
thaJeztah:make_update_status_non_ambiguous

thaJeztah commented Jan 10, 2019

Uh oh!

thaJeztah Jan 10, 2019

Uh oh!

dperny Jan 10, 2019

Uh oh!

thaJeztah Jan 10, 2019

Uh oh!

aaronlehmann Jan 11, 2019

Uh oh!

thaJeztah commented Jan 10, 2019

Uh oh!

thaJeztah Jan 10, 2019

Uh oh!

aaronlehmann commented Jan 11, 2019

Uh oh!

aaronlehmann commented Jan 11, 2019

Uh oh!

aaronlehmann Jan 11, 2019

Uh oh!

aaronlehmann Jan 11, 2019

Uh oh!

dperny commented Jan 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

thaJeztah commented Jan 10, 2019

Uh oh!

thaJeztah Jan 10, 2019

Choose a reason for hiding this comment

Uh oh!

dperny Jan 10, 2019

Choose a reason for hiding this comment

Uh oh!

thaJeztah Jan 10, 2019

Choose a reason for hiding this comment

Uh oh!

aaronlehmann Jan 11, 2019

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Jan 10, 2019

Uh oh!

thaJeztah Jan 10, 2019

Choose a reason for hiding this comment

Uh oh!

aaronlehmann commented Jan 11, 2019

Uh oh!

aaronlehmann commented Jan 11, 2019

Uh oh!

aaronlehmann Jan 11, 2019

Choose a reason for hiding this comment

Uh oh!

aaronlehmann Jan 11, 2019

Choose a reason for hiding this comment

Uh oh!

dperny commented Jan 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants