multi-worker daemon #176

AkihiroSuda · 2017-11-24T08:10:17Z

put multiples workers in a single binary ("-tags containerd standalone")
add worker selector to LLB vertex metadata
s/worker/executor/g
introduce the new "worker" concept multi-worker daemon #176 (comment)
fix up CLI
fix up tests
allow using multiples workers (requires inter-vertex cache copier, HUGE!) --> will be separate PR

Implementation notes:

"Workers" are renamed to "executors" now
The new "worker" instance holds an "executor" instance and its
related stuffs such as the snapshotter
The default worker is "runc-overlay"

Signed-off-by: Akihiro Suda [email protected]

tonistiigi

For the flags, my suggestion would be --containerd-worker=(true,false,auto), --oci-worker=(true,false,auto). --containerd-worker-addr=unix:///containerd.sock, --oci-worker-runtime=runc, --oci-worker-snapshotter=overlay. Default would be auto on both cases and would detect if socket or runtime binary is present. Ideally could also specify multiple containerd/runc workers and assign them names but this could be a follow up if there isn't a good idea atm(maybe just make all these flags slices).

tonistiigi · 2017-11-27T21:16:25Z

control/control.go

@@ -63,7 +45,9 @@ func (c *Controller) Register(server *grpc.Server) error {
 }

 func (c *Controller) DiskUsage(ctx context.Context, r *controlapi.DiskUsageRequest) (*controlapi.DiskUsageResponse, error) {
-	du, err := c.opt.CacheManager.DiskUsage(ctx, client.DiskUsageInfo{
+	// FIXME mw0
+	mw0 := c.opt.MetaWorkers[0]


I think this is probably what the TODO is for but it should iterate over the workers and add worker ID to the DiskUsageInfo (can be done is separate PR).

tonistiigi · 2017-11-27T21:18:23Z

control/control.go

@@ -101,8 +85,10 @@ func (c *Controller) Solve(ctx context.Context, req *controlapi.SolveRequest) (*

 	var expi exporter.ExporterInstance
 	var err error
+	// FIXME mw0
+	mw0 := c.opt.MetaWorkers[0]


This is actually tricky, as the exporter should come from the worker that has the returned reference. We may need to delay this so that the solver loads this.

tonistiigi · 2017-11-27T21:22:26Z

control/control.go

@@ -202,7 +188,9 @@ func (c *Controller) Session(stream controlapi.Control_SessionServer) error {
 	logrus.Debugf("session started")
 	conn, opts := grpchijack.Hijack(stream)
 	defer conn.Close()
-	err := c.opt.SessionManager.HandleConn(stream.Context(), conn, opts)
+	// FIXME mw0


I think SessionManager probably needs to remain in the manager and we may need to do some redirect to the actual worker per vertex. Even if we could figure out the worker that needs to load local files, for pull credentials multiple workers would need to use the same session.

Another idea would be to add some logic in the client so that every session request already goes to a specific worker.

tonistiigi · 2017-11-27T21:25:39Z

control/control_standalone_test.go

-	"golang.org/x/net/context"
-)
-
-func TestControl(t *testing.T) {


Where is this test now? Would be good to keep something that makes sure that the different components work together.

tonistiigi · 2017-11-27T21:32:04Z

metaworker/metaworker.go

-		Content:       pd.ContentStore,
-		Snapshotter:   pd.Snapshotter,
-		MetadataStore: md,
+func NewMetaWorker(opt MetaWorkerOpt) (*MetaWorker, error) {


I'd call metaWorker WorkerController and add methods like Add(name, worker),Get(constraint) to it that the init would call and that could be later called when attaching remote workers.

Instead of MetaWorkerOpt, runc.New(cliopts) could return a worker interface that the Add() accepts. To keep this shared code there could be a util pkg with a function that sets up the default configuration by converting a struct.

What name do you suggest for the type (X) that will be used for WorkerController.Add(name string, worker X) error and WorkerController.Get(constraint Constraint) (X, error)?

It will be a structure composed of a "bare" worker, a snapshotter, and some other stuffs.
(as in the MetaWorker structure in the current WIP PR, but without sessionmgr and so on)

Also, let me know whether it is the right approach to create X instances for each of snapshotters. (containerd-overlay, containerd-btrfs, containerd-blahblahblah instance rather than the single containerd instance)

I guess this model simplifies the design for local workers, but on second thought this might result in creating annoying number of remote worker instances.

Is the correct way that that X is called a Worker (current /worker could become /executor) and has methods for implementing

buildkit/solver/solver.go

Lines 67 to 76 in 3e4200c

type Op interface {

// CacheKey returns a persistent cache key for operation.

CacheKey(context.Context) (digest.Digest, error)

// ContentMask returns a partial cache checksum with content paths to the

// inputs. User can combine the content checksum of these paths to get a valid

// content based cache key.

ContentMask(context.Context) (digest.Digest, [][]string, error)

// Run runs an operation and returns the output references.

Run(ctx context.Context, inputs []Reference) (outputs []Reference, err error)

}

for any pb.Op the solver/manager may provide and then internals like snapshotters are not exposed at all? If this is correct then maybe call it OpWorker first and then name switches in separate PR.

For the second question, my understanding is that containerd will have introspection API that can provide us these values. I think we should support all that containerd supports but not sure if we want to do this automatically/by default. Even if we show them all we should only enable one automatically as otherwise having more snapshotters will make builds slower(still have the same CPU but need to copy data between drivers). I think it would also be fine to just add the default one and if users want more they can enable them with cli flags.

tonistiigi · 2017-11-27T21:37:48Z

solver/pb/ops.proto

+
+// WorkerConstraint is WIP. Will be changed.
+message WorkerConstraint {
+	string worker_name = 1; // e.g. "runc-overlay", "containerd-overlay", "containerd-btrfs"


I know this is WIP so maybe already planned, but I don't think worker name should be in here. We could have a worker_type/worker_brand (in addition to arch, os, labels etc). If you have 20 workers they would all have different names and adding/removing nodes would change them so this is not something that the client should know.

tonistiigi · 2017-11-27T21:42:13Z

hack/dockerfiles/test.Dockerfile

+FROM alpine AS buildkit-buildd
+COPY --from=buildd /usr/bin/buildd /usr/bin/
+COPY --from=buildctl /usr/bin/buildctl /usr/bin/
+COPY --from=runc /usr/bin/runc /usr/bin/


nit: this can be combined with the standalone stage(or make a common shared stage).

tonistiigi · 2017-11-27T21:48:51Z

allow using multiples workers (requires inter-vertex cache copier, HUGE!)

We could do it so that first you can only use the default one, after that you can choose worker but whole build must match a specific worker, then implement cache and make it faster.

- [X] put multiples workers in a single binary ("-tags containerd standalone") - [X] add worker selector to LLB vertex metadata - [X] s/worker/executor/g - [ ] introduce the new "worker" concept moby#176 (comment) - [ ] fix up CLI - [ ] allow using multiples workers (requires inter-vertex cache copier, HUGE!) Implementation notes: - A new structure called "metaworker" holds a worker instance and its related stuffs such as the snapshotter - For containerd, we have separate workers for each of the available snapshotters: containerd-overlay, containerd-btrfs, ... - The default worker is "runc-overlay" Signed-off-by: Akihiro Suda <[email protected]>

- [X] put multiples workers in a single binary ("-tags containerd standalone") - [X] add worker selector to LLB vertex metadata - [X] s/worker/executor/g - [X] introduce the new "worker" concept moby#176 (comment) - [X] fix up CLI - [ ] fix up tests - [ ] allow using multiples workers (requires inter-vertex cache copier, HUGE!) Implementation notes: - "Workers" are renamed to "executors" now - The new "worker" instance holds an "executor" instance and its related stuffs such as the snapshotter - For containerd, we have separate workers for each of the available snapshotters: containerd-overlay, containerd-btrfs, ... - The default worker is "runc-overlay" Signed-off-by: Akihiro Suda <[email protected]>

- [X] put multiples workers in a single binary ("-tags containerd standalone") - [X] add worker selector to LLB vertex metadata - [X] s/worker/executor/g - [WIP] introduce the new "worker" concept moby#176 (comment) - [X] fix up CLI - [ ] fix up tests - [ ] allow using multiples workers (requires inter-vertex cache copier, HUGE!) --> will be separate PR Implementation notes: - "Workers" are renamed to "executors" now - The new "worker" instance holds an "executor" instance and its related stuffs such as the snapshotter - For containerd, we have separate workers for each of the available snapshotters: containerd-overlay, containerd-btrfs, ... However, only the default one is used, unless `containerd-worker-multiple-snapshotter` is specified. - The default worker is "runc-overlay" Signed-off-by: Akihiro Suda <[email protected]>

- [X] put multiples workers in a single binary ("-tags containerd standalone") - [X] add worker selector to LLB vertex metadata - [X] s/worker/executor/g - [X] introduce the new "worker" concept moby#176 (comment) - [X] fix up CLI - [X] fix up tests - allow using multiples workers (requires inter-vertex cache copier, HUGE!) --> will be separate PR Implementation notes: - "Workers" are renamed to "executors" now - The new "worker" instance holds an "executor" instance and its related stuffs such as the snapshotter - For containerd, we have separate workers for each of the available snapshotters: containerd-overlay, containerd-btrfs, ... However, only the default one is used, unless `containerd-worker-multiple-snapshotter` is specified. - The default worker is "runc-overlay" Signed-off-by: Akihiro Suda <[email protected]>

AkihiroSuda · 2017-12-11T09:45:50Z

We could do it so that first you can only use the default one, after that you can choose worker but whole build must match a specific worker, then implement cache and make it faster.

I think the PR is now mergeable, PTAL?

tonistiigi

Mostly just readability comments.

tonistiigi · 2017-12-12T01:48:19Z

cmd/buildd/main.go

+var (
+	appFlags []cli.Flag
+	// key: priority (+: less preferred, -: more preferred)
+	workerCtors = make(map[int]func(c *cli.Context, common *worker.CommonOpt, root string) ([]*worker.Worker, error), 0)


rename to workerInitializers

The part that sets these up doesn't read too well(including the *Ctor naming). I'd define struct initializer { fn func(), priority int} and call registerInitializer with it. Should also simplify the priority sorting code. Another way would be to define ociWorker and containerdWorker pointers in here and set them in corresponding tagged files. Then you could just define priorityList := []..{ociWorker, containerdWorker} here and skip if it was nil in the setup phase.

tonistiigi · 2017-12-12T01:54:39Z

cmd/buildd/main.go

+	if err != nil {
+		return nil, err
+	}
+	commonOpt := &worker.CommonOpt{


Rename to InitOpt, add root to it or don't define it at all(SessionManager will not probably be shared in the future). Don't define this in worker package as a worker shouldn't know if some options were shared while some specific implementations were created. Just set to regular options before NewWorker.

tonistiigi · 2017-12-12T01:56:35Z

cmd/buildd/main.go

+	commonOpt := &worker.CommonOpt{
+		SessionManager: sessionManager,
+	}
+	wc, err := workerController(c, commonOpt, root)


newWorkerController

tonistiigi · 2017-12-12T02:02:24Z

cmd/buildd/main_containerd.go


-	return control.NewContainerd(root, socket)
+func skipContainerd(socket string) bool {


validContainerdSocket()

tonistiigi · 2017-12-12T02:03:22Z

cmd/buildd/main_containerd.go

+		return false
+	}
+	socketPath := strings.TrimPrefix(socket, "unix://")
+	if _, err := os.Stat(socketPath); os.IsNotExist(err) {


Should try to actually dial and call inspection API. You can just leave TODO comment atm.

tonistiigi · 2017-12-12T02:23:59Z

worker/worker.go

+	Name            string
+	MetadataStore   *metadata.Store
+	Executor        executor.Executor
+	BaseSnapshotter ctdsnapshot.Snapshotter // not blobmapping one


follow-up: just require blobmappingSnapshotter

tonistiigi · 2017-12-12T02:29:07Z

solver/source.go

@@ -21,7 +22,8 @@ type sourceOp struct {
 	src source.SourceInstance
 }

-func newSourceOp(_ Vertex, op *pb.Op_Source, sm *source.Manager) (Op, error) {
+func newSourceOp(_ Vertex, op *pb.Op_Source, w *worker.Worker) (Op, error) {
+	sm := w.SourceManager


Maybe avoid accidental dependencies. If the worker becomes interface this can depend on a smaller interface.

tonistiigi · 2017-12-12T02:32:45Z

solver/solver.go

 }

 func NewLLBSolver(opt LLBOpt) *Solver {
 	var s *Solver
 	s = New(func(v Vertex) (Op, error) {
+		w, err := DetermineVertexWorker(opt.WorkerController, v)


follow-up: worker can't be determined here, both constraints and the workers of the inputs are needed.

tonistiigi · 2017-12-12T02:35:05Z

solver/solver.go

-	Worker           worker.Worker
-	InstructionCache InstructionCache
-	ImageSource      source.Source
+	WorkerController *worker.Controller


follow-up: I guess this isn't LLBOpt anymore as the controller is not LLB-specific. Only the workers it manages are.

tonistiigi · 2017-12-12T02:37:13Z

solver/exec.go

@@ -22,15 +23,17 @@ const execCacheType = "buildkit.exec.v0"
 type execOp struct {
 	op        *pb.ExecOp
 	cm        cache.Manager
-	w         worker.Worker
+	exe       executor.Executor


nit: use exec as exe is commonly used for executable

- [X] put multiples workers in a single binary ("-tags containerd standalone") - [X] add worker selector to LLB vertex metadata - [X] s/worker/executor/g - [X] introduce the new "worker" concept moby#176 (comment) - [X] fix up CLI - [X] fix up tests - allow using multiples workers (requires inter-vertex cache copier, HUGE!) --> will be separate PR Implementation notes: - "Workers" are renamed to "executors" now - The new "worker" instance holds an "executor" instance and its related stuffs such as the snapshotter - The default worker is "runc-overlay" Signed-off-by: Akihiro Suda <[email protected]>

AkihiroSuda · 2017-12-12T05:44:25Z

Updated PR.

I'll open PRs to support mixing up multiple snapshotters (should be useful for docker/for-linux#72) and support transferring caches across workers.
After that we can work on remote workers.

- [X] put multiples workers in a single binary ("-tags containerd standalone") - [X] add worker selector to LLB vertex metadata - [X] s/worker/executor/g - [X] introduce the new "worker" concept moby#176 (comment) - [X] fix up CLI - [X] fix up tests - allow using multiples workers (requires inter-vertex cache copier, HUGE!) --> will be separate PR Implementation notes: - "Workers" are renamed to "executors" now - The new "worker" instance holds an "executor" instance and its related stuffs such as the snapshotter - The default worker is "runc-overlay" Signed-off-by: Akihiro Suda <[email protected]>

AkihiroSuda · 2017-12-12T06:35:11Z

CI green now

tonistiigi

LGTM

tonistiigi · 2017-12-12T18:12:33Z

Follow-ups before multi-worker support:

Remove buildd tags or negate them (no_containerd etc.). Make integration tests use buildd
Remove NewWorkerOpts constructors. Initialize worker directly. Worker knows how to resolve llb operations.
Some introspection API to see what worker is enabled/available.

- [X] put multiples workers in a single binary ("-tags containerd standalone") - [X] add worker selector to LLB vertex metadata - [X] s/worker/executor/g - [X] introduce the new "worker" concept moby/buildkit#176 (comment) - [X] fix up CLI - [X] fix up tests - allow using multiples workers (requires inter-vertex cache copier, HUGE!) --> will be separate PR Implementation notes: - "Workers" are renamed to "executors" now - The new "worker" instance holds an "executor" instance and its related stuffs such as the snapshotter - The default worker is "runc-overlay" Signed-off-by: Akihiro Suda <[email protected]>

AkihiroSuda force-pushed the multiworker branch from c7af1d2 to 83e81c6 Compare November 24, 2017 08:20

vdemeester requested review from vdemeester, dnephin and tonistiigi November 24, 2017 10:20

tonistiigi reviewed Nov 27, 2017

View reviewed changes

tonistiigi mentioned this pull request Nov 30, 2017

Rework integration tests #182

Merged

AkihiroSuda force-pushed the multiworker branch 2 times, most recently from e3e3115 to a8f93a9 Compare December 7, 2017 04:37

AkihiroSuda force-pushed the multiworker branch from a8f93a9 to 20237fe Compare December 7, 2017 05:19

AkihiroSuda force-pushed the multiworker branch from 20237fe to 88732f1 Compare December 8, 2017 04:20

AkihiroSuda force-pushed the multiworker branch from 88732f1 to c30f516 Compare December 8, 2017 04:32

tonistiigi mentioned this pull request Dec 9, 2017

containerd: use lease instead of gc.root label #159

Closed

AkihiroSuda force-pushed the multiworker branch from c30f516 to 2eaaaf7 Compare December 11, 2017 06:03

AkihiroSuda force-pushed the multiworker branch from 2eaaaf7 to 6f1d8dd Compare December 11, 2017 07:56

AkihiroSuda force-pushed the multiworker branch from 6f1d8dd to 949001d Compare December 11, 2017 08:11

AkihiroSuda force-pushed the multiworker branch from 949001d to f366cf4 Compare December 11, 2017 09:03

AkihiroSuda force-pushed the multiworker branch from f366cf4 to 6331de6 Compare December 11, 2017 09:39

AkihiroSuda changed the title ~~[WIP, DNM] multi-worker daemon~~ multi-worker daemon Dec 11, 2017

AkihiroSuda force-pushed the multiworker branch from 6331de6 to f054853 Compare December 11, 2017 09:44

tonistiigi mentioned this pull request Dec 11, 2017

Add support for setting user #203

Merged

tonistiigi reviewed Dec 12, 2017

View reviewed changes

AkihiroSuda force-pushed the multiworker branch from f054853 to 65b73aa Compare December 12, 2017 05:41

AkihiroSuda force-pushed the multiworker branch from 65b73aa to e182411 Compare December 12, 2017 05:47

AkihiroSuda force-pushed the multiworker branch from e182411 to 8899776 Compare December 12, 2017 06:02

AkihiroSuda force-pushed the multiworker branch from 8899776 to c3aa849 Compare December 12, 2017 06:18

tonistiigi approved these changes Dec 12, 2017

View reviewed changes

tonistiigi merged commit 60ea0ba into moby:master Dec 12, 2017

tonistiigi mentioned this pull request Oct 12, 2018

multi-worker in a single buildd binary #161

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

multi-worker daemon #176

multi-worker daemon #176

AkihiroSuda commented Nov 24, 2017 •

edited

Loading

tonistiigi left a comment

tonistiigi Nov 27, 2017

tonistiigi Nov 27, 2017

tonistiigi Nov 27, 2017

tonistiigi Nov 27, 2017

tonistiigi Nov 27, 2017 •

edited

Loading

AkihiroSuda Nov 28, 2017

AkihiroSuda Nov 29, 2017

tonistiigi Nov 29, 2017

tonistiigi Nov 27, 2017

tonistiigi Nov 27, 2017

tonistiigi commented Nov 27, 2017

AkihiroSuda commented Dec 11, 2017

tonistiigi left a comment

tonistiigi Dec 12, 2017

tonistiigi Dec 12, 2017

tonistiigi Dec 12, 2017

tonistiigi Dec 12, 2017

tonistiigi Dec 12, 2017

tonistiigi Dec 12, 2017

tonistiigi Dec 12, 2017

tonistiigi Dec 12, 2017

tonistiigi Dec 12, 2017

tonistiigi Dec 12, 2017

AkihiroSuda commented Dec 12, 2017

AkihiroSuda commented Dec 12, 2017

tonistiigi left a comment

tonistiigi commented Dec 12, 2017

	type Op interface {
	// CacheKey returns a persistent cache key for operation.
	CacheKey(context.Context) (digest.Digest, error)
	// ContentMask returns a partial cache checksum with content paths to the
	// inputs. User can combine the content checksum of these paths to get a valid
	// content based cache key.
	ContentMask(context.Context) (digest.Digest, [][]string, error)
	// Run runs an operation and returns the output references.
	Run(ctx context.Context, inputs []Reference) (outputs []Reference, err error)
	}


		return control.NewContainerd(root, socket)
		func skipContainerd(socket string) bool {

multi-worker daemon #176

multi-worker daemon #176

Conversation

AkihiroSuda commented Nov 24, 2017 • edited Loading

tonistiigi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonistiigi Nov 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonistiigi commented Nov 27, 2017

AkihiroSuda commented Dec 11, 2017

tonistiigi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkihiroSuda commented Dec 12, 2017

AkihiroSuda commented Dec 12, 2017

tonistiigi left a comment

Choose a reason for hiding this comment

tonistiigi commented Dec 12, 2017

AkihiroSuda commented Nov 24, 2017 •

edited

Loading

tonistiigi Nov 27, 2017 •

edited

Loading