Skip to content
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

c8d: Fix building Dockerfiles that have FROM scratch #46284

Merged
merged 3 commits into from Aug 23, 2023

Conversation

vvoland
Copy link
Contributor

@vvoland vvoland commented Aug 21, 2023

- What I did
Fix running containers with empty Image.

- How I did it
See commits.

- How to verify it
c8d integration tests (#45232) from TestDockerCLIBuildSuite:

- DONE 250 tests, 13 skipped, 58 failures in 352.552s
+ DONE 250 tests, 13 skipped, 42 failures in 406.027s

Before

$ printf 'FROM scratch\nLABEL asdf=1' | DOCKER_BUILDKIT=0 docker build -t asdf -
DEPRECATED: The legacy builder is deprecated and will be removed in a future release.
            BuildKit is currently disabled; enable it by removing the DOCKER_BUILDKIT=0
            environment-variable.

Sending build context to Docker daemon  2.048kB
Step 1/2 : FROM scratch
 --->
Step 2/2 : LABEL asdf=1
invalid reference format

After

$ printf 'FROM scratch\nLABEL asdf=1' | DOCKER_BUILDKIT=0 docker build -t asdf -
DEPRECATED: The legacy builder is deprecated and will be removed in a future release.
            BuildKit is currently disabled; enable it by removing the DOCKER_BUILDKIT=0
            environment-variable.

Sending build context to Docker daemon  2.048kB
Step 1/2 : FROM scratch
 --->
Step 2/2 : LABEL asdf=1
 ---> Running in 08bc1d65adb1
 ---> Removed intermediate container 08bc1d65adb1
 ---> 7072ecaf239f
Successfully built 7072ecaf239f
Successfully tagged asdf:latest

- Description for the changelog

containerd integration: Fix building Dockerfiles with `FROM scratch` instruction when using the legacy builder (non buildkit).

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

@vvoland vvoland added area/builder status/2-code-review kind/bugfix PR's that fix bugs area/builder/classic-builder Issues affecting the classic builder containerd-integration Issues and PRs related to containerd integration labels Aug 21, 2023
@vvoland vvoland added this to the 25.0.0 milestone Aug 21, 2023
@thaJeztah
Copy link
Member

LOL, wondering now if we can somehow skip the --> for these.

Step 1/2 : FROM scratch
 --->
Step 2/2 : LABEL asdf=1
 ---> Running in 08bc1d65adb1
 ---> Removed intermediate container 08bc1d65adb1
 ---> 7072ecaf239f
Successfully built 7072ecaf239f
Successfully tagged asdf:latest

(not something new, so not for this PR; looks like the same is the case without containerd-integration)

@vvoland vvoland force-pushed the c8d-legacybuilder-fix-from-scratch branch from 90566c1 to 1cc0069 Compare August 22, 2023 15:17
If the diff is empty and don't produce an empty layer.

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
This allows the legacy builder to apply changes to the `FROM scratch`
layer.

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
If the lease doesn't exit (for example when creating the container
failed), just ignore the not found error.

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
@vvoland vvoland force-pushed the c8d-legacybuilder-fix-from-scratch branch from 1cc0069 to bedcc94 Compare August 22, 2023 15:32
@@ -224,6 +224,25 @@ func ApplyUncompressedLayer(dest string, layer io.Reader, options *TarOptions) (
return applyLayerHandler(dest, layer, options, false)
}

// IsEmpty checks if the tar archive is empty (doesn't contain any entries).
func IsEmpty(rd io.Reader) (bool, error) {
Copy link
Member

@thaJeztah thaJeztah Aug 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And now I'm wondering; could we still detect this using "Ye olde sha" of the empty tar?

Goodbye '511136ea3c5a64f264b78b5433614aec563103b4d4702f3ba7d4d2698e22c158',
it was nice knowing you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a fast path.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^^ for future reference; this was reverted; it turned out that there was no canonical digest for "empty archive", and the code below was not "too heavy", so it wouldn't gain us much.

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@thaJeztah thaJeztah merged commit 9b9348c into moby:master Aug 23, 2023
203 checks passed
@thaJeztah
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/builder/classic-builder Issues affecting the classic builder area/builder containerd-integration Issues and PRs related to containerd integration kind/bugfix PR's that fix bugs process/cherry-picked status/2-code-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

containerd-integration: classic builder fails when building FROM scratch
3 participants