fix(distribution): digest cache should not be moved if it was an auth #36509

xujihui1985 · 2018-03-07T11:40:29Z

- How I did it
when push mount candidate to registry, first check if error is autherror, then check if user has authConfig
if both autherror while user has not login, do not remove the local cache.
- How to verify it
push the image without login, and login again, you still can mount the shared layer
- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

thaJeztah · 2018-03-07T15:50:20Z

ping @dmcgowan @tonistiigi PTAL

xujihui1985 · 2018-03-12T01:35:41Z

ping @dmcgowan

aaronlehmann · 2018-03-12T15:10:08Z

distribution/push_v2.go

@@ -580,14 +600,14 @@ func getMaxMountAndExistenceCheckAttempts(layer PushLayer) (maxMountAttempts, ma
 		// then fallback to upload
 		return 4, 3, true

-	// middle sized blobs; if we could not get the size, assume we deal with middle sized blob
+		// middle sized blobs; if we could not get the size, assume we deal with middle sized blob


This comment should not have the extra indentation.

sorry for my ide auto format the code

aaronlehmann · 2018-03-12T15:10:14Z

distribution/push_v2.go

 	case size > smallLayerMaximumSize, err != nil:
 		// 1st attempt to mount blobs of average size few times
 		// 2nd try at most 1 existence check if there's an existing mapping to the target repository
 		// then fallback to upload
 		return 3, 1, false

-	// small blobs, do a minimum number of checks
+		// small blobs, do a minimum number of checks


This comment should not have the extra indentation.

aaronlehmann · 2018-03-12T15:11:29Z

distribution/push_v2_test.go

 	"github.com/docker/docker/distribution/metadata"
 	"github.com/docker/docker/layer"
 	"github.com/docker/docker/pkg/progress"
 	"github.com/opencontainers/go-digest"
+	"github.com/docker/docker/api/types"
+	"github.com/docker/docker/registry"
+	"net/url"


Put this above with "net/http".

aaronlehmann · 2018-03-12T15:13:53Z

distribution/push_v2.go

@@ -397,8 +416,9 @@ func (pd *v2PushDescriptor) Upload(ctx context.Context, progressOutput progress.
 			return distribution.Descriptor{}, retryOnError(err)
 		}
 	}
-	defer layerUpload.Close()
-
+	if layerUpload != nil {


It doesn't seem right to allow layerUpload to be nil and then immediately afterwards call pd.uploadUsingSession with layerUpload. Why would layerUpload be nil when bs.Create returned no error a few lines above?

yes, this is paranoid, when I test the code with mock method I thought that may be the case when error is empty while layerUpload is nil too, and that should not happened

aaronlehmann · 2018-03-12T15:14:27Z

distribution/push_v2.go

+				switch e := e.(type) {
+				case errcode.Error:
+					if e.Code == errcode.ErrorCodeUnauthorized {
+						logrus.Debugf("error code is unauthoriedError")


unauthoriedError is misspelled.

aaronlehmann · 2018-03-12T15:19:29Z

distribution/push_v2.go

+		if isUnauthorizedError && !pd.pushState.hasAuthInfo {
+			// break the attempt loop as user has not login
+			logrus.Infoln("failed to push image to registry because unauthorized error and user not login")
+			break


I think this should continue instead of break. There might be other repos which do not require credentials to mount the blob. The continue can go inside the error checking code, instead of setting an isUnauthorizedError variable (it would mean having a label on the candidate loop and using continue candidateLoop).

this is modified as you suggested

codecov · 2018-03-13T11:14:30Z

Codecov Report

❗ No coverage uploaded for pull request base (master@72ba7f5). Click here to learn what that means.
The diff coverage is 90%.

@@            Coverage Diff            @@
##             master   #36509   +/-   ##
=========================================
  Coverage          ?   34.82%           
=========================================
  Files             ?      612           
  Lines             ?    45412           
  Branches          ?        0           
=========================================
  Hits              ?    15814           
  Misses            ?    27524           
  Partials          ?     2074

thaJeztah · 2018-03-13T14:10:50Z

PowerPC failing is due to a flaky test;

https://jenkins.dockerproject.org/job/Docker-PRs-powerpc/8973/console

12:00:49 --- FAIL: TestLinksEtcHostsContentMatch (0.53s)
12:00:49 	assertions.go:226: 
                          
	Error Trace:	links_linux_test.go:46
12:00:49 		
	Error:      	Not equal: 
12:00:49 		
	            	expected: "127.0.0.1\tlocalhost\n::1\tlocalhost ip6-localhost ip6-loopback\nfe00::0\tip6-localnet\nff00::0\tip6-mcastprefix\nff02::1\tip6-allnodes\nff02::2\tip6-allrouters\n172.17.0.2\t6fe75c06bdcd\n"
12:00:49 		
	            	received: ""

I see that one is being fixed through #36569

dmcgowan · 2018-03-13T22:40:39Z

distribution/push_v2.go

+					if e.Code == errcode.ErrorCodeUnauthorized && !pd.pushState.hasAuthInfo {
+						// continue to try other candidate as other repos may not require credentials to mount the blob
+						logrus.Debugf("failed to push layer to registry because unauthorized error and user not login")
+						continue candidateLoop


Does the layer upload need to be cancelled here? Maybe instead of a continue statement just ensure that the remove doesn't happen so there are fewer early exit/continue conditions.

this is my first attempt, what do you think of this?

as @aaronlehmann mentioned

I think this should continue instead of break. There might be other repos which do not require credentials to mount the blob. The continue can go inside the error checking code, instead of setting an isUnauthorizedError variable (it would mean having a label on the candidate loop and using continue candidateLoop).

is that the reason when authorizedError happened, it should continue to check other candidates?

the main difference is do we break here when first authorizedError met or continue to try other candidates

do we have some conclusion on this so that I can have another commit before this weekend? @dmcgowan @aaronlehmann : )

@dmcgowan is right that this needs to cancel the layer upload.

How about adding && !isUnauthorizedError to if len(mountCandidate.SourceRepository) > 0... below, instead of the continue?

How about adding && !isUnauthorizedError to if len(mountCandidate.SourceRepository) > 0... below, instead of the continue?

it looks like it will still continue to try next candidate which is not necessary?

aaronlehmann · 2018-03-16T15:00:46Z

it looks like it will still continue to try next candidate which is not necessary?

I think it's a good idea to try the next candidate, but don't feel strongly.

error local digest cache will be removed when error occured on push image but it should not be removed if it is an auth error while on auth was provided moby#36309 Signed-off-by: 慕陶 <jihui.xjh@alibaba-inc.com>

xujihui1985 · 2018-03-17T09:31:20Z

distribution/push_v2.go

 		if len(mountCandidate.SourceRepository) > 0 &&
+			!(isUnauthorizedError && !pd.pushState.hasAuthInfo) &&


@dmcgowan @aaronlehmann please have a look at this commit, hope that makes you happy

@dmcgowan let me know if you have any concern with this commit.

xujihui1985 · 2018-03-21T00:36:17Z

can we proceed this PR...? 🤓

dmcgowan · 2018-03-21T18:02:48Z

LGTM

thaJeztah

LGTM (thanks for reviewing @aaronlehmann @dmcgowan)

xujihui1985 · 2018-03-23T03:06:14Z

janky status seems not updated, actually it has finished

vdemeester · 2018-03-23T10:17:39Z

janky is green 👼 not sure why the check status is not up-to-date

GordonTheTurtle added dco/no Automatically set by a bot when one of the commits lacks proper signature status/0-triage labels Mar 7, 2018

xujihui1985 force-pushed the master branch from a038690 to 0d5dfaa Compare March 7, 2018 13:25

GordonTheTurtle added the dco/no Automatically set by a bot when one of the commits lacks proper signature label Mar 7, 2018

xujihui1985 force-pushed the master branch from 0d5dfaa to 47d9070 Compare March 7, 2018 13:26

GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Mar 7, 2018

moby deleted a comment from GordonTheTurtle Mar 7, 2018

thaJeztah added area/distribution status/2-code-review and removed status/0-triage labels Mar 7, 2018

thaJeztah requested review from dmcgowan, stevvooe and aaronlehmann March 12, 2018 11:27

aaronlehmann reviewed Mar 12, 2018

View reviewed changes

xujihui1985 force-pushed the master branch from 47d9070 to 865326d Compare March 13, 2018 10:29

GordonTheTurtle added the dco/no Automatically set by a bot when one of the commits lacks proper signature label Mar 13, 2018

xujihui1985 force-pushed the master branch from 865326d to da12883 Compare March 13, 2018 10:30

GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Mar 13, 2018

xujihui1985 force-pushed the master branch from da12883 to 2aa353a Compare March 13, 2018 11:14

xujihui1985 force-pushed the master branch from 2aa353a to 3687327 Compare March 13, 2018 11:23

dmcgowan reviewed Mar 13, 2018

View reviewed changes

xujihui1985 force-pushed the master branch from 3687327 to a0bb1e3 Compare March 17, 2018 09:28

fix(distribution): digest cache should not be moved if it was an auth

8b387b1

error local digest cache will be removed when error occured on push image but it should not be removed if it is an auth error while on auth was provided moby#36309 Signed-off-by: 慕陶 <jihui.xjh@alibaba-inc.com>

xujihui1985 force-pushed the master branch from a0bb1e3 to 8b387b1 Compare March 17, 2018 09:29

xujihui1985 commented Mar 17, 2018

View reviewed changes

aaronlehmann approved these changes Mar 17, 2018

View reviewed changes

moby deleted a comment from GordonTheTurtle Mar 21, 2018

thaJeztah approved these changes Mar 21, 2018

View reviewed changes

thaJeztah added status/4-merge rebuild/* and removed status/2-code-review labels Mar 21, 2018

GordonTheTurtle removed the rebuild/* label Mar 21, 2018

GordonTheTurtle assigned vdemeester Mar 22, 2018

vdemeester added the rebuild/janky label Mar 22, 2018

GordonTheTurtle removed the rebuild/janky label Mar 22, 2018

vdemeester merged commit c3b3be5 into moby:master Mar 23, 2018

thaJeztah mentioned this pull request Mar 23, 2018

[18.03] fix(distribution): digest cache should not be moved if it was an auth error docker/docker-ce#477

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(distribution): digest cache should not be moved if it was an auth #36509

fix(distribution): digest cache should not be moved if it was an auth #36509

xujihui1985 commented Mar 7, 2018 •

edited by thaJeztah

thaJeztah commented Mar 7, 2018

xujihui1985 commented Mar 12, 2018

aaronlehmann Mar 12, 2018

xujihui1985 Mar 13, 2018

aaronlehmann Mar 12, 2018

aaronlehmann Mar 12, 2018

aaronlehmann Mar 12, 2018

xujihui1985 Mar 13, 2018

aaronlehmann Mar 12, 2018

aaronlehmann Mar 12, 2018

xujihui1985 Mar 13, 2018

codecov bot commented Mar 13, 2018 •

edited

thaJeztah commented Mar 13, 2018

dmcgowan Mar 13, 2018

xujihui1985 Mar 13, 2018

xujihui1985 Mar 13, 2018

xujihui1985 Mar 14, 2018

xujihui1985 Mar 15, 2018

aaronlehmann Mar 16, 2018

xujihui1985 Mar 16, 2018

aaronlehmann commented Mar 16, 2018 via email

xujihui1985 Mar 17, 2018

xujihui1985 Mar 19, 2018

xujihui1985 commented Mar 21, 2018

dmcgowan commented Mar 21, 2018

thaJeztah left a comment

xujihui1985 commented Mar 23, 2018

vdemeester commented Mar 23, 2018

		if len(mountCandidate.SourceRepository) > 0 &&
		!(isUnauthorizedError && !pd.pushState.hasAuthInfo) &&

fix(distribution): digest cache should not be moved if it was an auth #36509

fix(distribution): digest cache should not be moved if it was an auth #36509

Conversation

xujihui1985 commented Mar 7, 2018 • edited by thaJeztah

thaJeztah commented Mar 7, 2018

xujihui1985 commented Mar 12, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 13, 2018 • edited

Codecov Report

thaJeztah commented Mar 13, 2018

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

aaronlehmann commented Mar 16, 2018 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xujihui1985 commented Mar 21, 2018

dmcgowan commented Mar 21, 2018

thaJeztah left a comment

Choose a reason for hiding this comment

xujihui1985 commented Mar 23, 2018

vdemeester commented Mar 23, 2018

xujihui1985 commented Mar 7, 2018 •

edited by thaJeztah

codecov bot commented Mar 13, 2018 •

edited