fix: BUILDKIT_HOST env parsing when doing system prune #4115

Shubhranshu153 · 2025-04-17T01:03:56Z

Currently when we set "BUILDKIT_HOST" as env variable, nerdctl doesnt take that into consideration for system prune.

[root@lima-finch nerdctl]# export BUILDKIT_HOST=unix:///tmp/buildkit-env.sock 
[root@lima-finch nerdctl]# nerdctl system prune --all
INFO[0000] Using buildkit host from command line flag or environment variable. 
WARN[0000] BuildKit is not running. Build caches will not be pruned.  error="flag accessed but not defined: buildkit-host"

With the change:

[root@lima-finch nerdctl]# export BUILDKIT_HOST=unix:///tmp/buildkit-env.sock 
[root@lima-finch nerdctl]# nerdctl system prune --all
WARNING! This will remove:
  - all stopped containers
  - all networks not used by at least one container
  - all images without at least one container associated to them
  - all build cache
Are you sure you want to continue? [y/N] y

Note: helpers.AddStringFlag(cmd, "buildkit-host", nil, "", "BUILDKIT_HOST", "BuildKit address") this masks the logic for build commands but for prune command it doesn't work correctly

Would like to refactor not have the helper commands to parse it as this only works when we have the specific flag but for buildctl config BUILDKIT_HOST can be also set for commands which may not have the buildkit-host flag specifically: eg: system prune. Of we add other options like du etc for buildctl this will be also relevant

cmd/nerdctl/builder/builder_build_test.go

apostasie · 2025-04-17T02:57:39Z

@Shubhranshu153
A few nits on test + implementing something to ease your env manipulation.

Also, do we need to keep calling helpers.AddStringFlag( instead of a plain cmd.Flags().String?

Shubhranshu153 · 2025-04-17T16:59:28Z

helpers.AddStringFlag -> This one i want to remove from all places as i feel we are manipulating behavior artificially.
will refactor it.

Shubhranshu153 · 2025-04-17T17:14:00Z

As the GetBuildkitHost() is kind of high level function used in 3 different places do you think we should move it to inside buildkitutils?

cmd/nerdctl/builder/builder_build_test.go

apostasie · 2025-04-17T19:21:51Z

As the GetBuildkitHost() is kind of high level function used in 3 different places do you think we should move it to inside buildkitutils?

Personal opinion:
We already have one in buildkitutil.
As for the part that processes flags and environment variable, I would rather keep that (somewhere) under ./cmd (eg: that is cli concern, not library concern IMO)

cmd/nerdctl/builder/builder_builder_test.go

apostasie · 2025-04-17T19:29:59Z

Linking latest failures to their tickets:

rootful failure is CNIEnv concurrency issues #3556
docker failure I just commented inline of your PR

cmd/nerdctl/builder/builder_build_test.go

apostasie

One last comment on a test, but otherwise looks fine.

cc @AkihiroSuda @fahedouch for formal maintainer review.

fahedouch · 2025-04-18T15:17:52Z

cmd/nerdctl/builder/builder_build.go

@@ -253,6 +253,14 @@ func GetBuildkitHost(cmd *cobra.Command, namespace string) (string, error) {
 		}
 		return buildkitHost, nil
 	}
+
+	if buildkitHost := os.Getenv("BUILDKIT_HOST"); buildkitHost != "" {


Wouldn't it be more logical to introduce the --buildkit-host flag in the prune command?

Unless I'm mistaken, we wanted to have a single entry point for CLI flag and parameters, which is cmd. I think this ensures better management of flags, environment variables, and parameters related to the CLI.

Probably this should be a global flag as in --address ($CONTAINERD_ADDRESS)

nerdctl/cmd/nerdctl/main.go

Line 169 in 9791bbc

helpers.AddPersistentStringFlag(rootCmd, "address", []string{"a", "H"}, nil, []string{"host"}, aliasToBeInherited, cfg.Address, "CONTAINERD_ADDRESS", `containerd address, optionally with "unix://" prefix`)

I can add that as well along with the change.
But we should not deprecate the current flags for backward compatibility. Global flag will be overridden by the cmd specific options?

A global flag can be specified for a subcommand too, so the compatibility is not affected AFAICS

ok i can add it to the global flag and refactor the code. Makes sense to me.
Thanks

i started doing this change but i feel this requires a larger refactor and some testing, will want to put it in a separate PR.

AkihiroSuda · 2025-04-18T15:19:31Z

Needs rebase

AkihiroSuda · 2025-04-18T15:22:45Z

Note: helpers.AddStringFlag(cmd, "buildkit-host", nil, "", "BUILDKIT_HOST", "BuildKit address") this masks the logic for build commands but for prune command it doesn't work correctly

How is buildctl prune different from buildctl build for handling BUILDKIT_HOST? 🤔

apostasie · 2025-04-18T15:29:26Z

Note: helpers.AddStringFlag(cmd, "buildkit-host", nil, "", "BUILDKIT_HOST", "BuildKit address") this masks the logic for build commands but for prune command it doesn't work correctly

How is buildctl prune different from buildctl build for handling BUILDKIT_HOST? 🤔

I think @Shubhranshu153 was talking about nerdctl system prune (not nerdctl build prune).

apostasie · 2025-04-18T16:50:55Z

Note: helpers.AddStringFlag(cmd, "buildkit-host", nil, "", "BUILDKIT_HOST", "BuildKit address") this masks the logic for build commands but for prune command it doesn't work correctly

How is buildctl prune different from buildctl build for handling BUILDKIT_HOST? 🤔

I think @Shubhranshu153 was talking about nerdctl system prune (not nerdctl build prune).

eg: my understanding is that buildctl itself never look into BUILDKIT_HOST because we explicitly pass to it a --addr argument.

cmd/nerdctl/builder/builder_build_test.go

apostasie · 2025-04-18T19:31:17Z

@Shubhranshu153 main is bust right now.

Suggesting you wait for #4128 then rebase.

apostasie · 2025-04-18T21:35:56Z

Tagging unrelated failure: #4068

cmd/nerdctl/builder/builder_build_test.go

apostasie

One minor question about NoParallel on the first test, but otherwise LGTM.
Thanks @Shubhranshu153 !

Signed-off-by: Shubharanshu Mahapatra <shubhum@amazon.com>

Shubhranshu153 · 2025-04-23T23:55:38Z

Can we merge this? Thanks

AkihiroSuda

Thanks

apostasie reviewed Apr 17, 2025

View reviewed changes

cmd/nerdctl/builder/builder_build_test.go Outdated Show resolved Hide resolved

apostasie reviewed Apr 17, 2025

View reviewed changes

cmd/nerdctl/builder/builder_build_test.go Outdated Show resolved Hide resolved

apostasie reviewed Apr 17, 2025

View reviewed changes

cmd/nerdctl/builder/builder_build_test.go Outdated Show resolved Hide resolved

apostasie reviewed Apr 17, 2025

View reviewed changes

cmd/nerdctl/builder/builder_build_test.go Outdated Show resolved Hide resolved

apostasie reviewed Apr 17, 2025

View reviewed changes

cmd/nerdctl/builder/builder_build_test.go Outdated Show resolved Hide resolved

apostasie reviewed Apr 17, 2025

View reviewed changes

cmd/nerdctl/builder/builder_build_test.go Outdated Show resolved Hide resolved

apostasie reviewed Apr 17, 2025

View reviewed changes

cmd/nerdctl/builder/builder_build_test.go Outdated Show resolved Hide resolved

apostasie mentioned this pull request Apr 17, 2025

[Tigron]: allow per-command environment override #4116

Merged

apostasie reviewed Apr 17, 2025

View reviewed changes

cmd/nerdctl/builder/builder_build_test.go Outdated Show resolved Hide resolved

Shubhranshu153 force-pushed the fix-buildkit-host branch from ff6f1b1 to 848a4bf Compare April 17, 2025 17:48

apostasie reviewed Apr 17, 2025

View reviewed changes

cmd/nerdctl/builder/builder_build_test.go Outdated Show resolved Hide resolved

Shubhranshu153 force-pushed the fix-buildkit-host branch 3 times, most recently from 1c5f8d4 to 29a42b6 Compare April 17, 2025 19:02

apostasie reviewed Apr 17, 2025

View reviewed changes

cmd/nerdctl/builder/builder_builder_test.go Show resolved Hide resolved

apostasie reviewed Apr 17, 2025

View reviewed changes

cmd/nerdctl/builder/builder_builder_test.go Show resolved Hide resolved

Shubhranshu153 force-pushed the fix-buildkit-host branch from 29a42b6 to 491775d Compare April 17, 2025 19:49

Shubhranshu153 closed this Apr 17, 2025

Shubhranshu153 reopened this Apr 17, 2025

apostasie reviewed Apr 17, 2025

View reviewed changes

cmd/nerdctl/builder/builder_build_test.go Outdated Show resolved Hide resolved

Shubhranshu153 force-pushed the fix-buildkit-host branch from 491775d to f7710a0 Compare April 17, 2025 21:49

apostasie approved these changes Apr 17, 2025

View reviewed changes

AkihiroSuda added this to the v2.0.5 milestone Apr 18, 2025

fahedouch reviewed Apr 18, 2025

View reviewed changes

apostasie reviewed Apr 18, 2025

View reviewed changes

cmd/nerdctl/builder/builder_build_test.go Outdated Show resolved Hide resolved

Shubhranshu153 force-pushed the fix-buildkit-host branch 3 times, most recently from 619191a to 83abfb9 Compare April 18, 2025 18:56

Shubhranshu153 force-pushed the fix-buildkit-host branch from 83abfb9 to afbc1ba Compare April 18, 2025 21:04

Shubhranshu153 closed this Apr 18, 2025

Shubhranshu153 reopened this Apr 18, 2025

AkihiroSuda requested a review from apostasie April 21, 2025 00:41

apostasie reviewed Apr 21, 2025

View reviewed changes

cmd/nerdctl/builder/builder_build_test.go Outdated Show resolved Hide resolved

apostasie approved these changes Apr 21, 2025

View reviewed changes

fix: BUILDKIT_HOST env parsing

95fc284

Signed-off-by: Shubharanshu Mahapatra <shubhum@amazon.com>

Shubhranshu153 force-pushed the fix-buildkit-host branch from afbc1ba to 95fc284 Compare April 21, 2025 15:41

AkihiroSuda approved these changes Apr 24, 2025

View reviewed changes

AkihiroSuda merged commit 1673252 into containerd:main Apr 24, 2025
30 checks passed

github-actions bot mentioned this pull request May 4, 2025

Bump nerdctl from 1.7.6 to 2.0.5 kokyhm/kubespray#199

Open

fix: BUILDKIT_HOST env parsing when doing system prune #4115

fix: BUILDKIT_HOST env parsing when doing system prune #4115

Uh oh!

Conversation

Shubhranshu153 commented Apr 17, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

apostasie commented Apr 17, 2025

Uh oh!

Shubhranshu153 commented Apr 17, 2025

Uh oh!

Shubhranshu153 commented Apr 17, 2025

Uh oh!

Uh oh!

apostasie commented Apr 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

apostasie commented Apr 17, 2025

Uh oh!

Uh oh!

apostasie left a comment

Choose a reason for hiding this comment

Uh oh!

fahedouch Apr 18, 2025

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda Apr 18, 2025 • 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!

Shubhranshu153 Apr 18, 2025

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda Apr 18, 2025

Choose a reason for hiding this comment

Uh oh!

Shubhranshu153 Apr 18, 2025

Choose a reason for hiding this comment

Uh oh!

Shubhranshu153 Apr 18, 2025

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented Apr 18, 2025

Uh oh!

AkihiroSuda commented Apr 18, 2025

Uh oh!

apostasie commented Apr 18, 2025

Uh oh!

apostasie commented Apr 18, 2025

Uh oh!

Uh oh!

apostasie commented Apr 18, 2025

Uh oh!

apostasie commented Apr 18, 2025

Uh oh!

Uh oh!

apostasie left a comment

Choose a reason for hiding this comment

Uh oh!

Shubhranshu153 commented Apr 23, 2025

Uh oh!

AkihiroSuda left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

apostasie commented Apr 17, 2025 •

edited

Loading

AkihiroSuda Apr 18, 2025 •

edited

Loading