Refactor buildkit init in daemon startup by cpuguy83 · Pull Request #48950 · moby/moby

cpuguy83 · 2024-11-25T18:51:32Z

Refactor buildkit init in daemon startup

Move the "Daemon has completed initialization" log to where it has
actually completed initialization.
Move buildkkt init to its own function.

cpuguy83 · 2024-11-25T22:53:48Z

After some futher investigation, this buildkir PR seems to be resolving the issue: moby/buildkit#5550
It's also making docker buildx du basically instantaneous, rather than taking several minutes.

cpuguy83 · 2024-11-26T00:42:39Z

I've updated this to just remove the timeout minor refactor.
These are done in separate commits so the timeout change can be backported easily.

thaJeztah · 2024-11-26T08:58:51Z

Failure looks consistent; not sure how it relates directly though

=== Failed
=== FAIL: amd64.integration.volume TestVolumesRemoveSwarmEnabled (1.93s)
    volume_test.go:157: [dd55180fec946] error while stopping the daemon: exit status 2
    --- PASS: TestVolumesRemoveSwarmEnabled/volume_in_use (0.00s)
    --- PASS: TestVolumesRemoveSwarmEnabled/volume_not_in_use (0.01s)
    --- PASS: TestVolumesRemoveSwarmEnabled/non-existing_volume (0.00s)
    --- PASS: TestVolumesRemoveSwarmEnabled/non-existing_volume_force (0.00s)

thaJeztah · 2024-11-26T09:02:52Z

 	c, err := createAndStartCluster(cli, d)
 	if err != nil {
-		log.G(ctx).Fatalf("Error starting cluster component: %v", err)
+		return errors.Wrap(err, "error starting cluster component")


Wondering if we want to both log and return this one (not sure if the error shows up in logs?)

edit: answering myself; it looks like we do this already for som other errors further up. Looking it such errors were logged, I noticed that for windows we explicitly send it to the logs (but only when running as service), but for unix we don't;

on Windows;

moby/cmd/dockerd/docker_windows.go

Lines 28 to 33 in 2f6953f

err = cli.start(ctx)

if service != nil {

// When running as a service, log the error, so that it's sent to

// the event-log.

log.G(ctx).Error(err)

}

on Linux;

moby/cmd/dockerd/docker_unix.go

Lines 12 to 14 in 2f6953f

func runDaemon(ctx context.Context, cli *daemonCLI) error {

return cli.start(ctx)

}

huh, I don't remember changing this line of code.
Maybe I saw it was return fmt.Errorf(...) with a %v and not properly wrapped...

thaJeztah · 2024-11-26T09:26:47Z

 	// Get a the current daemon config, because the daemon sets up config
 	// during initialization. We cannot user the cli.Config for that reason,
 	// as that only holds the config that was set by the user.
 	//
 	// FIXME(thaJeztah): better separate runtime and config data?
 	daemonCfg := d.Config()
-	routerOpts, err := newRouterOptions(routerCtx, &daemonCfg, d, c)
+
+	bb, sm, err := initBuildkit(ctx, &daemonCfg, d)


This is the only place where daemonCfg is used, so perhaps we should move the code (and comment?) above inside initBuildkit as it's already having access to the daemon, and uses it for various other purposes.

The code comment was added in 00c9785 to prevent buildkit being initialised with the wrong "host-gateway-ip";

fix "host-gateway-ip" label not set for builder workers #47187

thaJeztah · 2024-11-26T09:28:19Z

+	bb, sm, err := initBuildkit(ctx, &daemonCfg, d)
 	if err != nil {
-		return err
+		return fmt.Errorf("error initializing buildkit")


Looks like we're discarding the original error here

thaJeztah · 2024-11-26T09:33:24Z

 	bb, err := buildbackend.NewBackend(d.ImageService(), manager, bk, d.EventsService)
 	if err != nil {


On a fun note; it looks like NewBackend never return an error; not for this PR, but wondering if we should even get rid of the constructor (or remove the error return)

moby/api/server/backend/build/backend.go

Lines 40 to 43 in 2f6953f

// NewBackend creates a new build backend from components

func NewBackend(components ImageComponent, builder Builder, buildkit *buildkit.Builder, es *daemonevents.Events) (*Backend, error) {

return &Backend{imageComponent: components, builder: builder, buildkit: buildkit, eventsService: es}, nil

}

thaJeztah · 2024-11-26T09:34:32Z

-		return routerOptions{}, errors.Wrap(err, "failed to create buildmanager")
+		return nil, nil, errors.Wrap(err, "failed to create buildmanager")


As we're updating, perhaps we should change this to failed to create build backend, but see above; looks like this never returns an error.

thaJeztah · 2024-11-26T09:37:21Z

-	cluster        *cluster.Cluster
-}
+func initBuildkit(ctx context.Context, config *config.Config, d *daemon.Daemon) (*buildbackend.Backend, *session.Manager, error) {
+	log.G(ctx).Info("Initializing buildkit")


Nit/question: do we use (lowercase) buildkit or BuildKit in our logs?

thaJeztah · 2024-11-26T09:38:14Z

-		return routerOptions{}, errors.Wrap(err, "failed to create buildmanager")
+		return nil, nil, errors.Wrap(err, "failed to create buildmanager")
 	}
+	return bb, sm, nil


Would it make sense to log here that we finished initialising BuildKit? To make it the counterpart of;

log.G(ctx).Info("Completed initializing BuildKit")

or

log.G(ctx).Info("Finished initializing BuildKit")

thaJeztah · 2024-11-26T09:40:13Z

I'll move the first commit to a separate PR so that we can test that in isolation and backport it.

edit: created PR:

Remove buildkit init timeout #48953

cpuguy83 · 2024-11-27T17:00:23Z

This should be good to go now.

thaJeztah

DOH! I started a review, and have a branch with some of the changes (mostly to look wha they'd look like); I'm AFK for some hours but can push those later for discussion

thaJeztah · 2024-11-27T15:14:08Z

+	ro.buildBackend = bb
+	ro.buildkit = bk
+	ro.sessionManager = sm


Wonder what it would look like if we had a separate type for the builder options so that we could return it instead of manipulating the routerOptions 🤔

thaJeztah · 2024-11-27T15:18:24Z

-		Rootless:            daemon.Rootless(config),
+		Rootless:            daemon.Rootless(&config),


note to self; perhaps daemon.Rootless would make more sense to be part of the config package. It's just to abstract that config.Rootless is only defined on Linux (not windows)

thaJeztah · 2024-11-27T15:20:18Z

+		Rootless:            daemon.Rootless(&config),
 		IdentityMapping:     d.IdentityMapping(),
 		DNSConfig:           config.DNSConfig,
 		ApparmorProfile:     daemon.DefaultApparmorProfile(),


note to self: maybe same for daemon.DefaultApparmorProfile 🤔

1. Move the "Daemon has completed initialization" log to where it has actually completed initialization. 2. Move buildkkt init to its own function. Signed-off-by: Brian Goff <[email protected]> Signed-off-by: Sebastiaan van Stijn <[email protected]>

thaJeztah · 2024-12-19T14:16:39Z

did a quick rebase after #49078 was merged (minor conflict)

vvoland

Do I understand correctly, that this PR doesn't actually extend the timeout, only refactors the code. And the actual init timeout was removed in #48953?

If so, I think the PR description needs updating 😅

thaJeztah · 2025-02-06T13:03:54Z

I updated the title, description and labels

I updated #49040 as well to squash it into 2 commits, and moved it out of draft; if others prefer that PR, we can merge that one instead.

cpuguy83 added kind/bugfix PR's that fix bugs area/builder/buildkit Build process/cherry-pick/27.x labels Nov 25, 2024

cpuguy83 requested review from dmcgowan and thaJeztah November 25, 2024 18:51

cpuguy83 force-pushed the buildkit_init_timeout branch from b338145 to 33813ea Compare November 25, 2024 19:15

cpuguy83 mentioned this pull request Nov 25, 2024

Use view transaction for metadata read moby/buildkit#5550

Merged

cpuguy83 force-pushed the buildkit_init_timeout branch from 33813ea to 1608d2f Compare November 26, 2024 00:41

thaJeztah added this to the 28.0.0 milestone Nov 26, 2024

thaJeztah added the status/2-code-review label Nov 26, 2024

thaJeztah requested changes Nov 26, 2024

View reviewed changes

thaJeztah mentioned this pull request Nov 26, 2024

Remove buildkit init timeout #48953

Merged

cpuguy83 force-pushed the buildkit_init_timeout branch 2 times, most recently from e29382f to b9ea8a1 Compare November 26, 2024 19:18

thaJeztah reviewed Nov 27, 2024

View reviewed changes

thaJeztah removed the process/cherry-pick/27.x label Nov 28, 2024

thaJeztah force-pushed the buildkit_init_timeout branch 2 times, most recently from 2c71f31 to b7591f8 Compare November 28, 2024 23:01

thaJeztah mentioned this pull request Dec 5, 2024

cmd/dockerd: refactor buildkit init in daemon startup #49040

Merged

thaJeztah force-pushed the buildkit_init_timeout branch from b7591f8 to 4c7eb3e Compare December 19, 2024 14:15

vvoland reviewed Jan 21, 2025

View reviewed changes

thompson-shaun assigned vvoland Jan 30, 2025

thaJeztah changed the title ~~Extend buildkit init timeout for large build caches~~ Refactor buildkit init in daemon startup Feb 6, 2025

thaJeztah added area/daemon Core Engine kind/refactor PR's that refactor, or clean-up code and removed kind/bugfix PR's that fix bugs labels Feb 6, 2025

vvoland closed this in #49040 Feb 6, 2025

	err = cli.start(ctx)
	if service != nil {
	// When running as a service, log the error, so that it's sent to
	// the event-log.
	log.G(ctx).Error(err)
	}

	func runDaemon(ctx context.Context, cli *daemonCLI) error {
	return cli.start(ctx)
	}

		bb, err := buildbackend.NewBackend(d.ImageService(), manager, bk, d.EventsService)
		if err != nil {

	// NewBackend creates a new build backend from components
	func NewBackend(components ImageComponent, builder Builder, buildkit buildkit.Builder, es daemonevents.Events) (*Backend, error) {
	return &Backend{imageComponent: components, builder: builder, buildkit: buildkit, eventsService: es}, nil
	}

		return routerOptions{}, errors.Wrap(err, "failed to create buildmanager")
		return nil, nil, errors.Wrap(err, "failed to create buildmanager")

		Rootless: daemon.Rootless(config),
		Rootless: daemon.Rootless(&config),

Conversation

cpuguy83 commented Nov 25, 2024 • edited by thaJeztah Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cpuguy83 commented Nov 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cpuguy83 commented Nov 26, 2024

Uh oh!

thaJeztah commented Nov 26, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Nov 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cpuguy83 commented Nov 27, 2024

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Dec 19, 2024

Uh oh!

vvoland left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Feb 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cpuguy83 commented Nov 25, 2024 •

edited by thaJeztah

Loading

cpuguy83 commented Nov 25, 2024 •

edited

Loading

thaJeztah commented Nov 26, 2024 •

edited

Loading