New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

builder: fix build cache hash for broken symlink #34271

Merged
merged 2 commits into from Oct 31, 2017

Conversation

Projects
None yet
9 participants
@tonistiigi
Member

tonistiigi commented Jul 26, 2017

fixes #34260

This makes build cache hash for broken symlink in a directory to match the path of resolved path like it appears in the original tarsum version

// We set sum to path by default for the case where GetFile returns nil.
// The usual case is if relative path is empty.
return path, nil // backwards compat TODO: see if really needed
. There are still plenty of cases where this isn't quite correct, some are described in moby/buildkit#38 . I think we need to rethink what exact data to hash in there. The current is an accidental combination of some tar headers and automatic symlink following because of the ADD/COPY semantics.

@justincormack @dnephin

Signed-off-by: Tonis Tiigi tonistiigi@gmail.com

@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi Jul 26, 2017

Member

This broke unit tests that checked for a hash of a missing file. It is possible to determine between a broken followed symlink and missing file but there is no utility function for that atm. I didn't find anything that actually relies on the not-found error being produced so I updated the tests. When this logic gets updated we should make sure the error case is put back.

Member

tonistiigi commented Jul 26, 2017

This broke unit tests that checked for a hash of a missing file. It is possible to determine between a broken followed symlink and missing file but there is no utility function for that atm. I didn't find anything that actually relies on the not-found error being produced so I updated the tests. When this logic gets updated we should make sure the error case is put back.

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Jul 27, 2017

Member

Looks like windows failure is related?

Member

dnephin commented Jul 27, 2017

Looks like windows failure is related?

@vieux

This comment has been minimized.

Show comment
Hide comment
@vieux

vieux Aug 3, 2017

Collaborator

ping @tonistiigi ^

Collaborator

vieux commented Aug 3, 2017

ping @tonistiigi ^

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Sep 29, 2017

Member

ping @tonistiigi should someone carry this?

/cc @johnstep @salah-khan

Member

thaJeztah commented Sep 29, 2017

ping @tonistiigi should someone carry this?

/cc @johnstep @salah-khan

builder: fix build cache hash for broken symlink
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi Oct 3, 2017

Member

Fixed the unit test. CI still red but this time I don't think it is related.

Member

tonistiigi commented Oct 3, 2017

Fixed the unit test. CI still red but this time I don't think it is related.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Oct 3, 2017

Member

Windows failure is now;

00:12:48 ----------------------------------------------------------------------
00:12:48 FAIL: docker_api_logs_test.go:18: DockerSuite.TestLogsAPIWithStdout
00:12:48 
00:12:48 docker_api_logs_test.go:50:
00:12:48     c.Fatal("timeout waiting for logs to exit")
00:12:48 ... Error: timeout waiting for logs to exit
00:12:48 

Which was marked racey; #33301

I'll trigger CI once more for good measures

Member

thaJeztah commented Oct 3, 2017

Windows failure is now;

00:12:48 ----------------------------------------------------------------------
00:12:48 FAIL: docker_api_logs_test.go:18: DockerSuite.TestLogsAPIWithStdout
00:12:48 
00:12:48 docker_api_logs_test.go:50:
00:12:48     c.Fatal("timeout waiting for logs to exit")
00:12:48 ... Error: timeout waiting for logs to exit
00:12:48 

Which was marked racey; #33301

I'll trigger CI once more for good measures

@vieux

This comment has been minimized.

Show comment
Hide comment
@vieux

vieux Oct 3, 2017

Collaborator

ok looks like the windows failure is racey

Collaborator

vieux commented Oct 3, 2017

ok looks like the windows failure is racey

@thaJeztah thaJeztah added this to backlog in maintainers-session Oct 5, 2017

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Oct 5, 2017

Member

ping @AkihiroSuda could you have a look at this one?

Member

thaJeztah commented Oct 5, 2017

ping @AkihiroSuda could you have a look at this one?

@thaJeztah thaJeztah removed this from backlog in maintainers-session Oct 5, 2017

@AkihiroSuda

This comment has been minimized.

Show comment
Hide comment
@AkihiroSuda

AkihiroSuda Oct 10, 2017

Member

ping @tonistiigi

LGTM after adding the comment

Member

AkihiroSuda commented Oct 10, 2017

ping @tonistiigi

LGTM after adding the comment

Fix tests after Hash() behavior change
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah
Member

thaJeztah commented Oct 31, 2017

ping @justincormack PTAL

@yongtang yongtang merged commit 3ab20a8 into moby:master Oct 31, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 37511 has succeeded
Details
janky Jenkins build Docker-PRs 46204 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 6603 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 17784 has succeeded
Details
z Jenkins build Docker-PRs-s390x 6402 has succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment