Fix image resolve detection by thaJeztah · Pull Request #386 · docker/cli

thaJeztah · 2017-07-26T17:59:08Z

When using --resolve-image change, the image that was resolved during deploy, as well as Platform information were overwritten by the image specified in the docker-compose.yaml. As a result, services were always re-deployed.

fixes moby/moby#34242

- What I did

If QueryRegistry is "false", preserve the image and Platform information from the existing service.

UPDATE:
removed preserving the platform information, as is discussed in moby/moby#34268

- How to verify it

cat > docker-compose.yml <<"EOF"
version: "3.3"
services:
    redis:
        image: redis:3.2.8
EOF

docker stack deploy -c docker-compose.yml repro \
&& docker service inspect repro_redis -f '{{json .Spec}}' | jq . > before.json \
&& docker stack deploy -c docker-compose.yml --resolve-image changed repro \
&& docker service inspect repro_redis -f '{{json .Spec}}' | jq . > after.json \
&& docker stack rm repro

The diff should be empty:

git diff --no-index before.json after.json

Before this change, the diff would show:

diff --git a/before.json b/after.json
index f65ecfff..0eecbc51 100644
--- a/before.json
+++ b/after.json
@@ -23,14 +23,7 @@
       "Delay": 5000000000,
       "MaxAttempts": 0
     },
-    "Placement": {
-      "Platforms": [
-        {
-          "Architecture": "amd64",
-          "OS": "linux"
-        }
-      ]
-    },
+    "Placement": {},
     "Networks": [
       {
         "Target": "njp31bnesn79ojar7vj34saqc",

- Description for the changelog

ping @aaronlehmann @nishanttotla @dnephin PTAL

aaronlehmann · 2017-07-26T18:09:08Z

cli/command/stack/deploy_composefile.go

 			}
-
+			if !updateOpts.QueryRegistry {
+				serviceSpec.TaskTemplate.ContainerSpec.Image = service.Spec.TaskTemplate.ContainerSpec.Image


What happens if you change the image in the compose file?

I believe in that case image != service.Spec.Labels[convert.LabelImage] would be false, so we wouldn't enter this block? I think changing this to an else makes that more obvious ( as commented)

The label stuff only applies when resolveImage is set to resolveImageChanged. What if resolveImage is set to "never"?

True, there needs to be another condition here on the else if

I initially created some code that simply did a compare between the actual image in the spec (stripping digest if the image in the docker-compose file didn't have one), then noticed it's actually using the label to compare

in the "else" case, UpdateService looks up image information, and applies the placement information (if that's what you meant)

I'm thinking these two blocks should look something like this (using a switch/case instead of an if/else if):

switch { // image should be updated by the server using QueryRegistry case resolveImage == resolveImageAlways || (resolveImage == resolveImageChanged && image != service.Spec.Labels[convert.LabelImage]): updateOpts.QueryRegistry = true // image hasn't change, so the serviceSpec needs to be updated to use the existing image that was // set by QueryRegistry on the previous deploy. Otherwise this will result in an incorrect service update. case image == service.Spec.Labels[convert.LabelImage]: serviceSpec.TaskTemplate.ContainerSpec.Image = service.Spec.TaskTemplate.ContainerSpec.Image }

dnephin · 2017-07-26T18:09:26Z

cli/command/stack/deploy_test.go

+				},
+			},
+		}
+		deployServices(ctx, client, spec, namespace, false, resolveImageChanged)


This test would continue to pass if serviceUpdateFunc() was not called, even if the test was failing. I think we need to adjust the assertions.

deployServices() can return an error, so we should assert.NoError(t, err) on that error.

Maybe instead of asserting in the fake, the fake could store the arg values, and the assertions could happen here after the call to deployServices() ?

Yes, I was indeed looking at that, but didn't have a clean solution, I'll do some more thinking to address that

dnephin · 2017-07-26T18:15:25Z

cli/command/stack/deploy_composefile.go

+					if serviceSpec.TaskTemplate.Placement == nil {
+						serviceSpec.TaskTemplate.Placement = &swarm.Placement{}
+					}
+					serviceSpec.TaskTemplate.Placement.Platforms = service.Spec.TaskTemplate.Placement.Platforms


This seems like the wrong place to set defaults.

If these values are not set in the compose file where do they come from?

This is in the update case; these are the values taken from the current service spec (resolved during initial deploy)

Why are they changed by the engine/swarmkit? Shouldn't it only change the image?

These were added based on the image information; i.e. only deploy a Windows image on a Windows host, Arm on Arm etc

But there are already some issues around it, so things may change

Does that mean we could leave these out of this PR? I'm not sure it's related to the original bug, but maybe I'm misunderstanding.

Wouldn't these be set in all cases, not just when QueryRegistry is enabled?

I considered moving this to the ServiceUpdate function but this problem only occurs with stack deploy, because it doesn't "diff" the same way as docker service update does. docker service update takes the existing service as a starting point, and only makes changes set through the -add / -rm flags, whereas stack deploy takes the compose-file as source of truth

I opened moby/moby#34268. I think we're going to need to fix that first

Once we have a package that is responsible for handling this logic I think at least these lines will move to that package. Possible the switch/case as well

This placement/platforms stuff is a bug in the client/ package. Let's not hack around it here.

If we want to fix this for 17.06 I'm not sure we have a good alternative at this moment (without the refactoring of the client package)

If this is only a bug fix for 17.06 let's commit it to only the 17.06 branch in docker-ce and leave the hack out of master. We can open an issue for the correct fix.

dnephin · 2017-07-26T18:20:59Z

cli/command/stack/deploy_composefile.go

 			}
-
+			if !updateOpts.QueryRegistry {
+				serviceSpec.TaskTemplate.ContainerSpec.Image = service.Spec.TaskTemplate.ContainerSpec.Image


I think the intent here would be more clear as an else instead of checking the flag set in the previous branch.

An inline comment about why this is necessary would also be great. I'm sure I'm going to forget. Something like this:

If the image hasn't changed the serviceSpec needs to be updated to use the existing image that was set by QueryRegistry on the previous deploy. Otherwise this will trigger an incorrect service update.

codecov-io · 2017-07-31T09:36:52Z

Codecov Report

Merging #386 into master will increase coverage by 0.2%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master     #386     +/-   ##
=========================================
+ Coverage   46.26%   46.47%   +0.2%     
=========================================
  Files         193      193             
  Lines       16093    16100      +7     
=========================================
+ Hits         7445     7482     +37     
+ Misses       8259     8225     -34     
- Partials      389      393      +4

thaJeztah · 2017-08-01T22:36:51Z

@dnephin updated PTAL

dnephin · 2017-08-02T15:04:51Z

cli/command/stack/deploy_composefile.go


 			if resolveImage == resolveImageAlways || (resolveImage == resolveImageChanged && image != service.Spec.Labels[convert.LabelImage]) {
 				updateOpts.QueryRegistry = true
+			} else {


In this case: resolveImage == resolveImageNever && image != service.Spec.Labels[convert.LabelImage] we would set the image back to the previous image, which is wrong, because the user requested an update to the image.

I think this needs to be:

switch { // image should be updated by the server using QueryRegistry case resolveImage == resolveImageAlways || (resolveImage == resolveImageChanged && image != service.Spec.Labels[convert.LabelImage]): updateOpts.QueryRegistry = true // image hasn't changed, so the serviceSpec needs to be updated to use the existing image that was // set by QueryRegistry on the previous deploy. Otherwise this will result in an incorrect service update. case image == service.Spec.Labels[convert.LabelImage]: serviceSpec.TaskTemplate.ContainerSpec.Image = service.Spec.TaskTemplate.ContainerSpec.Image }

Argh, you're right; changed it to the if/else based on your previous comment. I'm not entirely convinced using a "switch" is much clearer; wether or not to keep the existing information is based on updateOpts.QueryRegistry being true, which was why I had it like that in the first implementation

I don't think it should be based on updateOpts.QueryRegistry. updateOpts.QueryRegistry is set by comparing the existing state to the desired state of the service. Setting the Image should be based on that same comparison. updateOpts.QueryRegistry is an output, not the source of the state.

I'm not entirely convinced using a "switch" is much clearer

I find switch/case reads much better than any if/else, but it's true you could also implement this using if/else if

dnephin · 2017-08-02T15:05:13Z

cli/command/stack/deploy_composefile.go

+					if serviceSpec.TaskTemplate.Placement == nil {
+						serviceSpec.TaskTemplate.Placement = &swarm.Placement{}
+					}
+					serviceSpec.TaskTemplate.Placement.Platforms = service.Spec.TaskTemplate.Placement.Platforms


This placement/platforms stuff is a bug in the client/ package. Let's not hack around it here.

When re-deploying a stack without re-resolving the image digest, the service's ContainerSpec was updated with the image-reference as specified in the stack/compose file. As a result, the image-digest that was resolved in a previous deploy was overwritten, causing the service to be re-deployed. This patch preserves the previously resolve image-digest by copying it from the current service spec. A unit test is also added to verify that the image information in the service spec is not updated if QueryRegistry is disabled. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah · 2017-08-03T16:55:53Z

@dnephin updated, now only preserving the image digest / information

dnephin

Thanks! LGTM

dnephin · 2017-08-03T17:05:34Z

cli/command/stack/deploy_test.go

+			expectedQueryRegistry: false,
+			expectedImage:         "foobar:1.2.3@sha256:deadbeef",
+		},
+		// Image changed


I think this test covers 2 of the 4 cases. The other 2 are different resolveImage values.

We could add coverage for that in a follow up

vdemeester

LGTM 🐮

[master] bump VERSION to 18.02.0-ce-dev

thaJeztah added the status/2-code-review label Jul 26, 2017

thaJeztah requested review from dnephin and vdemeester as code owners July 26, 2017 17:59

GordonTheTurtle added the status/0-triage label Jul 26, 2017

aaronlehmann reviewed Jul 26, 2017

View reviewed changes

dnephin reviewed Jul 26, 2017

View reviewed changes

This was referenced Jul 26, 2017

Plugins on swarm moby/moby#33575

Merged

Remove application logic from client/ package moby/moby#34268

Open

thaJeztah removed the status/0-triage label Jul 31, 2017

thaJeztah force-pushed the fix-image-resolve-detection branch 2 times, most recently from 4518743 to 0ac5a39 Compare July 31, 2017 09:36

thaJeztah force-pushed the fix-image-resolve-detection branch 2 times, most recently from f55daa9 to bb2c7d1 Compare August 1, 2017 22:36

dnephin reviewed Aug 2, 2017

View reviewed changes

thaJeztah force-pushed the fix-image-resolve-detection branch from bb2c7d1 to d0bea64 Compare August 3, 2017 16:52

dnephin approved these changes Aug 3, 2017

View reviewed changes

vdemeester approved these changes Aug 22, 2017

View reviewed changes

vdemeester merged commit 8da1dae into docker:master Aug 22, 2017

GordonTheTurtle added this to the 17.08.0 milestone Aug 22, 2017

thaJeztah deleted the fix-image-resolve-detection branch September 19, 2017 15:48

zarnovican mentioned this pull request Jan 8, 2018

swarm: needless update with --resolve-image changed moby/moby#34242

Closed

nobiit pushed a commit to nobidev/docker-cli that referenced this pull request Nov 19, 2025

Merge pull request docker#386 from seemethere/bump_ver_18_02_dev

790deda

[master] bump VERSION to 18.02.0-ce-dev

Conversation

thaJeztah commented Jul 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

dnephin Jul 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

codecov-io commented Jul 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

thaJeztah commented Aug 1, 2017

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!

thaJeztah commented Aug 3, 2017

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

thaJeztah commented Jul 26, 2017 •

edited

Loading

dnephin Jul 26, 2017 •

edited

Loading

codecov-io commented Jul 31, 2017 •

edited

Loading