/cc @adshmh @kolyshkin @vdemeester

I couldn't find a reference why nil was also considered "service is converged", so perhaps this is not the right change; it just stood out a bit, so wondering if this was correct.

Codecov Report

❗ No coverage uploaded for pull request base (master@de640c9). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #38474   +/-   ##
=========================================
  Coverage          ?   36.63%           
=========================================
  Files             ?      608           
  Lines             ?    45174           
  Branches          ?        0           
=========================================
  Hits              ?    16550           
  Misses            ?    26336           
  Partials          ?     2288

ok, that doesn't seem to work indeed;

14:23:13 === RUN   TestServiceUpdateLabel
14:23:45 --- FAIL: TestServiceUpdateLabel (31.85s)
14:23:45     daemon.go:296: [d0cbb2acf2c65] waiting for daemon to start
14:23:45     daemon.go:328: [d0cbb2acf2c65] daemon started
14:23:45     update_test.go:35: timeout hit after 30s: waiting for service qsdarx3yw9unpnkxgik5yco84 to be updated
14:23:45     daemon.go:283: [d0cbb2acf2c65] exiting daemon

Looking at the failure

14:36:48 === RUN   TestServiceUpdateLabel
14:37:20 --- FAIL: TestServiceUpdateLabel (31.93s)
14:37:20     daemon.go:296: [d78c00ad8edaf] waiting for daemon to start
14:37:20     daemon.go:328: [d78c00ad8edaf] daemon started
14:37:20     update_test.go:35: timeout hit after 30s: waiting for service l8u8xz2frkylmuzwyrlf0wjnx to be updated
14:37:20     daemon.go:283: [d78c00ad8edaf] exiting daemon

Maybe, UpdateStatus is not updated in some cases (like updating labels as it might only update the spec and no desired state change)

Maybe, UpdateStatus is not updated in some cases (like updating labels as it might only update the spec and no desired state change)

@vdemeester Yes, that's what I was thinking as well. I'd have to check with the SwarmKit people; I think that may be troublesome if for some updates we have to wait for it to reach UpdateStateCompleted, and for other updates we're ok with it being nil.

(Without having dug into it); wondering if the flakiness in the TestServiceUpdateConfigs case is that for that test we actually want it to reach UpdateStateCompleted and it goes something like;

• (A) Service status becomes UpdateStatus = nil
  • we think it completed (but it's actually still in the process of updating)
• (B) Service status becomes UpdateStatus = UpdateStateCompleted
  • it actually completed, and version is incremented
• (C) we update the service again (using the old version from (A))
• (D) test fails ("update out of sequence"), because version from (A) was not the last version; we should've used the version from (B).

ping @aaronlehmann @dperny @anshulpundir could you have a look at this? Wondering if you know if my suspicion above could be right 🤗

@thaJeztah afaik, updating only service specs does no actually generate update tasks.
How about if we also update/add some container labels with it?

Another thing which I was thinking that how much of these tests actually should be on here and how much on Swarmkit side?

Perhaps the issue is that ServiceCreate is asynchronous, so will return immediately, however, the service's tasks are not yet running/converged. Once they converge, the Version may be updated 🤔

Do we need something like is done on the CLI; https://github.com/docker/cli/blob/ae1618713f83e7da07317d579d0675f578de22fa/cli/command/service/progress/progress.go#L75-L205 and https://github.com/docker/cli/blob/ae1618713f83e7da07317d579d0675f578de22fa/cli/command/service/progress/progress.go#L273-L330 ?

Another thing which I was thinking that how much of these tests actually should be on here and how much on Swarmkit side?

That's a good question; I do think we need some integration tests here (as this is the integration point of swarmkit into docker); not sure we need all of them

Closing as #38499 got merged