[nerdctl build] Set default output type=image #3604

TinaMor · 2024-10-28T20:24:05Z

PR Description

Linked Issue: containerd/nerdctl#3629

This PR resolves #3629 for nerdctl build command when passed with:

--progress auto, --progress tty, or --progress flag is not specified (which then defaults to --progress auto)
--output flag is not specified

Example:

nerdctl build -t <TAG> --file .\Dockerfile .

Background

The issue occurs because the build progress output and the tarball are both sent to stdout on WIndows.

Solution

nerdctl/pkg/cmd/builder/build.go

Line 216 in 0d7dc8e

output = "type=docker"

When a user does not specify the --output flag, we default it to use type=image instead of type=docker because the image is actually uploaded to containerd image store. According to buildctl documentation, type=image is the native type for containerd.

Additional tasks

Only use the default image name when user has not specified one

Not included in this PR

When default type=oci

nerdctl/pkg/cmd/builder/build.go

Line 220 in 0d7dc8e

output = "type=oci"
type=docker
Currenly fails with archive/tar: invalid tar header moby/buildkit/issues/5528

References

buildctl output: containerd image store

TBBle · 2024-11-01T05:13:06Z

I need to work through this later, but I'm unclear how the current code works on non-Windows. How do those platforms distinguish a tar file from log output on stdout?

They shouldn't be passing through a termInal on the way, the terminal is where nerdctl's stdio is connected, so I don't understand what's different on Windows.

apostasie · 2024-11-01T08:10:46Z

Hey @TBBle
I’ll have a look later today.
Curious too.

TinaMor · 2024-11-05T09:16:49Z

@TBBle, @apostasie

Still investigating the issue. This is a temp fix and the PR is WIP (may and may not be the final solution).
Currrently trying to understand the following:

What is the performance impact of this fix if using a larger image eg mcr.microsoft.com/windows/servercore:ltsc2022 vs mcr.microsoft.com/windows/nanoserver:ltsc2022
Is it the Windows terminal?
Why are the build output and tarball all going to the same location on Windows alone?
How does it work for progress=plain?

apostasie · 2024-11-05T11:18:38Z

Deeply confused about this.

Why are the build output and tarball all going to the same location on Windows alone?

This does not make any sense.
I am squinting at nerdctl source and cannot figure it out.

Now suspecting buildctl might be the problem.

apostasie · 2024-11-05T11:21:09Z

buildctl "output" goes to stderr.

Is it the case on windows?

foo="$(buildctl --addr=unix:///run/user/501/buildkit/buildkitd.sock build --progress=auto --frontend=dockerfile.v0 --local=context=/tmp/ --output=type=docker --local=dockerfile=/tmp --opt=filename=Dockerfile)"

or

buildctl --addr=unix:///run/user/501/buildkit/buildkitd.sock build --progress=auto --frontend=dockerfile.v0 --local=context=/tmp/ --output=type=docker --local=dockerfile=/tmp --opt=filename=Dockerfile >/dev/null

TinaMor · 2024-11-13T13:59:31Z

pkg/cmd/builder/build.go

-			output = "type=docker"
+			// https://github.com/moby/buildkit?tab=readme-ov-file#output
+			// type=image is the native type for containerd
+			output = "type=image"


Default to type=image instead of type=docker. See #3604 (comment)

cc @profnandaa

TinaMor · 2024-11-13T14:00:18Z

pkg/cmd/builder/build.go

 	if err != nil {
 		if r.N == 0 {
 			// Avoid confusing "unrecognized image format"
-			return errors.New("no image was built")
+			return fmt.Errorf("no image was built: %w", err)


Make the error more descriptive by adding the actual error

pkg/cmd/builder/build.go

- Use default image name only when user has not specified one Signed-off-by: Christine Murimi <[email protected]>

TBBle · 2024-11-14T01:51:54Z

When a user does not specify the --output flag, we default it to use type=image instead of type=docker because the image is actually uploaded to containerd image store. According to buildctl documentation, type=image is the native type for containerd.

Does this introduce a requirement that buildkit is configured to use the containerd worker? Or is that already a requirement for using nerdctl build for other reasons?

…amor/fix-build-with-tty

TinaMor · 2024-11-18T18:11:45Z

cmd/nerdctl/builder/builder_build_test.go

@@ -89,17 +89,17 @@ CMD ["echo", "nerdctl-build-test-string"]`, testutil.CommonImage)
 				Expected: test.Expects(0, nil, test.Equals("nerdctl-build-test-string\n")),
 			},
 			{
-				Description: "Successfully build with output docker, name cannot be used",


@apostasie I've removed this test for now. I may be missing something. What is this testing?

TinaMor · 2024-11-19T09:43:44Z

Does this introduce a requirement that buildkit is configured to use the containerd worker? Or is that already a requirement for using nerdctl build for other reasons?

@TBBle In Windows, containerd-worker is the default. There is ongoing work to support oci-worker on Windows. On Linux environments, [oci-worker is the default].(https://github.com/moby/buildkit/blob/5cd5a63de5d37512c24f20085607509b749df762/README.md#linux-setup)

Additionally, with the shift from docker to containerd, we then should switch to using type=image instead of type=docker as the default, for both Windows and Linux.

References:

@AkihiroSuda @jsturtevant What do you think?

AkihiroSuda · 2024-11-19T10:07:46Z

The OCI worker still has to be supported

TinaMor force-pushed the tinamor/fix-build-with-tty branch 4 times, most recently from 2d18366 to 01db27a Compare October 29, 2024 14:23

TinaMor changed the title ~~Separate TTY and tar output in nerdctl build with tty~~ Fix Windows tar parsing error by separating tty and tar outputs in buildctl Oct 29, 2024

TinaMor changed the title ~~Fix Windows tar parsing error by separating tty and tar outputs in buildctl~~ [Windows] Fix nerdctl build tar parsing error Oct 29, 2024

AkihiroSuda added the area/ci e.g., CI failure label Nov 1, 2024

TinaMor force-pushed the tinamor/fix-build-with-tty branch from 01db27a to 1e7bbf4 Compare November 6, 2024 15:24

TinaMor changed the title ~~[Windows] Fix nerdctl build tar parsing error~~ [nerdctl build] Set default output type=image Nov 6, 2024

TinaMor force-pushed the tinamor/fix-build-with-tty branch from 1e7bbf4 to 80dc420 Compare November 13, 2024 13:54

TinaMor commented Nov 13, 2024

View reviewed changes

pkg/cmd/builder/build.go Outdated Show resolved Hide resolved

TinaMor closed this Nov 13, 2024

TinaMor reopened this Nov 13, 2024

TinaMor force-pushed the tinamor/fix-build-with-tty branch from 9581c2c to 14392ff Compare November 13, 2024 16:03

[Windows] nerdctl build - Replace default type=docker with type=image

f4728b4

- Use default image name only when user has not specified one Signed-off-by: Christine Murimi <[email protected]>

TinaMor force-pushed the tinamor/fix-build-with-tty branch from 14392ff to f4728b4 Compare November 13, 2024 16:16

TinaMor added 2 commits November 18, 2024 18:46

Fix failing test

0a72edd

Merge branch 'main' of https://github.com/containerd/nerdctl into tin…

6811330

…amor/fix-build-with-tty

TinaMor commented Nov 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[nerdctl build] Set default output type=image #3604

[nerdctl build] Set default output type=image #3604

TinaMor commented Oct 28, 2024 •

edited

Loading

TBBle commented Nov 1, 2024 •

edited

Loading

apostasie commented Nov 1, 2024 •

edited

Loading

TinaMor commented Nov 5, 2024

apostasie commented Nov 5, 2024

apostasie commented Nov 5, 2024

TinaMor Nov 13, 2024 •

edited

Loading

TinaMor Nov 13, 2024

TBBle commented Nov 14, 2024

TinaMor Nov 18, 2024

TinaMor commented Nov 19, 2024 •

edited

Loading

AkihiroSuda commented Nov 19, 2024

[nerdctl build] Set default output type=image #3604

Are you sure you want to change the base?

[nerdctl build] Set default output type=image #3604

Conversation

TinaMor commented Oct 28, 2024 • edited Loading

PR Description

Background

Solution

Additional tasks

Not included in this PR

References

TBBle commented Nov 1, 2024 • edited Loading

apostasie commented Nov 1, 2024 • edited Loading

TinaMor commented Nov 5, 2024

apostasie commented Nov 5, 2024

apostasie commented Nov 5, 2024

TinaMor Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

TinaMor Nov 13, 2024

Choose a reason for hiding this comment

TBBle commented Nov 14, 2024

TinaMor Nov 18, 2024

Choose a reason for hiding this comment

TinaMor commented Nov 19, 2024 • edited Loading

AkihiroSuda commented Nov 19, 2024

TinaMor commented Oct 28, 2024 •

edited

Loading

TBBle commented Nov 1, 2024 •

edited

Loading

apostasie commented Nov 1, 2024 •

edited

Loading

TinaMor Nov 13, 2024 •

edited

Loading

TinaMor commented Nov 19, 2024 •

edited

Loading