worker: add annotation labels #232

AkihiroSuda · 2017-12-19T10:40:55Z

$ sudo ./bin/buildctl local-workers -v labels.org.mobyproject.buildkit.worker.executor==containerd 
ID:                                             containerd-overlayfs@ws01                                                                         
org.mobyproject.buildkit.worker.hostname:       ws01                     
org.mobyproject.buildkit.worker.os:             linux                    
org.mobyproject.buildkit.worker.arch:           amd64                    
org.mobyproject.buildkit.worker.executor:       containerd               
org.mobyproject.buildkit.worker.snapshotter:    overlayfs

Update #62 #41

AkihiroSuda · 2017-12-19T10:50:01Z

worker/base/worker.go

+	if daemonInstanceID == "" {
+		daemonInstanceID = hostname
+	}
+	id := executor + "-" + snapshotter + "@" + daemonInstanceID


Should we just use random string?

Yes, I think ID should be unique. We can show these fields on the non-verbose output of the command.

Store it in the state directory on first run.

tonistiigi · 2017-12-19T17:51:06Z

cmd/buildctl/localworkers.go

+)
+
+var localWorkersCommand = cli.Command{
+	Name:      "local-workers",


Add this under debug for now (and add comments to proto that this API will likely change). In the future I'd expect this to be part of some generic inspection API that shows remote workers, daemon info etc. but too early for that atm.

tonistiigi · 2017-12-19T17:52:23Z

cmd/buildctl/localworkers.go

+		return err
+	}
+
+	workers, err := c.ListLocalWorkers(appcontext.Context(), client.WithWorkerFilter(clicontext.Args()))


Why the "local" prefix everywhere? When remote workers become possible I'd expect them to be included in this output.

Should we keep this under --filter/-f flag still, like buildctl du.

tonistiigi · 2017-12-19T18:09:05Z

worker/base/worker.go

+	if daemonInstanceID == "" {
+		daemonInstanceID = hostname
+	}
+	id := executor + "-" + snapshotter + "@" + daemonInstanceID


Yes, I think ID should be unique. We can show these fields on the non-verbose output of the command.

tonistiigi · 2017-12-19T18:11:15Z

worker/base/worker.go

+	if daemonInstanceID == "" {
+		daemonInstanceID = hostname
+	}
+	id := executor + "-" + snapshotter + "@" + daemonInstanceID


Store it in the state directory on first run.

tonistiigi · 2017-12-19T18:14:54Z

cmd/buildctl/localworkers.go

+func printWorkersVerbose(tw *tabwriter.Writer, winfo []*client.WorkerInfo) {
+	for _, wi := range winfo {
+		printKV(tw, "ID", wi.ID)
+		for k, v := range wi.Labels {


We should keep this output in order.

tonistiigi · 2017-12-19T18:16:24Z

cmd/buildkitd/main.go

@@ -64,6 +64,14 @@ func main() {
 			Name:  "debug",
 			Usage: "enable debug output in logs",
 		},
+		cli.StringFlag{
+			Name:  "instance-id",


I think we can skip this for now if we use random IDs.

tonistiigi · 2017-12-19T18:17:17Z

cmd/buildkitd/main.go

+			Usage: "daemon instance id (required only when multiple daemons share the hostname)",
+		},
+		cli.StringSliceFlag{
+			Name:  "labels",


Aren't these worker specific?

tonistiigi · 2017-12-19T18:20:03Z

worker/runc/runc.go

@@ -68,11 +70,16 @@ func NewWorkerOpt(root string) (base.WorkerOpt, error) {

 	// TODO: call mdb.GarbageCollect . maybe just inject it into nsSnapshotter.Remove and csContent.Delete

+	id, xlabels := base.DetermineAnnotations(daemonInstanceID, "oci", "overlayfs")


No need for "Determine" here

tonistiigi · 2017-12-19T18:24:39Z

worker/filter.go

+	"github.com/containerd/containerd/filters"
+)
+
+func adaptWorker(o interface{}) filters.Adaptor {


Why does this func take interface{}?

AkihiroSuda · 2017-12-20T06:05:01Z

updated

AkihiroSuda · 2017-12-20T06:05:53Z

worker/base/worker.go

+
+// ID reads the worker id from the metadata store.
+// If not exist, generate a random one,
+func ID(meta *metadata.Store) (string, error) {


Is it ok to use metadata store here?

No, it should only be for snapshot metadata. There is an All() request in the snapshots initializer that reads in all records from here and expects them to be snapshots.

tonistiigi

You can add a note to readme about this command as well

tonistiigi · 2017-12-20T06:06:13Z

api/services/control/control.proto

 }

 message DiskUsageRequest {
-	string filter = 1;
+	string filter = 1; // FIXME: this should be containerd-compatible repeated string?


Yes. (Not in this PR)

tonistiigi · 2017-12-20T06:11:42Z

worker/base/worker.go

+
+// ID reads the worker id from the metadata store.
+// If not exist, generate a random one,
+func ID(meta *metadata.Store) (string, error) {


No, it should only be for snapshot metadata. There is an All() request in the snapshots initializer that reads in all records from here and expects them to be snapshots.

tonistiigi · 2017-12-20T06:16:11Z

cmd/buildctl/debug/workers.go

+}
+
+func sortMap(m map[string]string) [][2]string {
+	var s [][2]string


nit: you don't really need the [][2] construction in here as you only need to sort the keys and could read the value from map. sortedKeys(m) []string would probably be easier to follow. Also, initialize the slice with map length.

AkihiroSuda · 2017-12-20T06:58:23Z

updated

tonistiigi

One small nit.

LGTM

tonistiigi · 2017-12-20T07:01:30Z

worker/runc/runc.go

@@ -23,9 +23,11 @@ import (
 // NewWorkerOpt creates a WorkerOpt.
 // But it does not set the following fields:
 //  - SessionManager
-func NewWorkerOpt(root string) (base.WorkerOpt, error) {
+//
+// daemonInstanceID is optional unless multiple instances share the single hostname.


remove comment

tonistiigi · 2017-12-20T07:02:19Z

worker/containerd/containerd.go

@@ -19,17 +19,19 @@ import (
 // NewWorkerOpt creates a WorkerOpt.
 // But it does not set the following fields:
 //  - SessionManager
-func NewWorkerOpt(root string, address, snapshotterName string, opts ...containerd.ClientOpt) (base.WorkerOpt, error) {
+//
+// daemonInstanceID is optional unless multiple instances share the single hostname.


AkihiroSuda · 2017-12-20T07:03:56Z

done

AkihiroSuda · 2017-12-20T07:14:00Z

Flaky docker build: https://travis-ci.org/moby/buildkit/builds/319031791?utm_source=github_status&utm_medium=notification

Step 7/17 : RUN apk add --no-cache git make

 ---> Running in b148f1428a8b

fetch http://dl-cdn.alpinelinux.org/alpine/v3.6/main/x86_64/APKINDEX.tar.gz

fetch http://dl-cdn.alpinelinux.org/alpine/v3.6/community/x86_64/APKINDEX.tar.gz

(1/6) Installing libssh2 (1.8.0-r1)

(2/6) Installing libcurl (7.57.0-r0)

(3/6) Installing expat (2.2.0-r1)

(4/6) Installing pcre (8.41-r0)

(5/6) Installing git (2.13.5-r0)

ERROR: Failed to create usr/libexec/git-core/git-show-index: I/O error

ERROR: git-2.13.5-r0: IO ERROR

(6/6) Installing make (4.2.1-r0)

Executing busybox-1.26.2-r9.trigger

1 error; 166 MiB in 33 packages

Removing intermediate container b148f1428a8b

The command '/bin/sh -c apk add --no-cache git make' returned a non-zero code: 1

make: *** [bin/buildkitd] Error 1

Is this an issue of Docker? (Docker 17.11-ce, probably overlayfs driver)

tonistiigi · 2017-12-20T07:16:18Z

@AkihiroSuda Or network issue. BTW, Do you have permissions to restart CI?

AkihiroSuda · 2017-12-20T07:28:22Z

Yes

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

AkihiroSuda changed the title ~~Worker annotations~~ worker: add annotation labels Dec 19, 2017

AkihiroSuda commented Dec 19, 2017

View reviewed changes

AkihiroSuda force-pushed the worker-annotations branch from b2a8bac to edc6ce7 Compare December 19, 2017 10:52

tonistiigi reviewed Dec 19, 2017

View reviewed changes

AkihiroSuda force-pushed the worker-annotations branch from edc6ce7 to 8456e32 Compare December 20, 2017 06:05

AkihiroSuda commented Dec 20, 2017

View reviewed changes

AkihiroSuda force-pushed the worker-annotations branch from 8456e32 to f4c7297 Compare December 20, 2017 06:06

tonistiigi reviewed Dec 20, 2017

View reviewed changes

AkihiroSuda force-pushed the worker-annotations branch from f4c7297 to 6be96b1 Compare December 20, 2017 06:58

tonistiigi approved these changes Dec 20, 2017

View reviewed changes

AkihiroSuda force-pushed the worker-annotations branch from 6be96b1 to 9dcbdc0 Compare December 20, 2017 07:03

worker: add labels

84c0dd0

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

AkihiroSuda force-pushed the worker-annotations branch from 9dcbdc0 to 84c0dd0 Compare December 20, 2017 07:28

tonistiigi merged commit 35bef6e into moby:master Dec 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

worker: add annotation labels #232

worker: add annotation labels #232

AkihiroSuda commented Dec 19, 2017

AkihiroSuda Dec 19, 2017

tonistiigi Dec 19, 2017

tonistiigi Dec 19, 2017

tonistiigi Dec 19, 2017

tonistiigi Dec 19, 2017

tonistiigi Dec 19, 2017

tonistiigi Dec 19, 2017

tonistiigi Dec 19, 2017

tonistiigi Dec 19, 2017

tonistiigi Dec 19, 2017

tonistiigi Dec 19, 2017

tonistiigi Dec 19, 2017

tonistiigi Dec 19, 2017

AkihiroSuda commented Dec 20, 2017

AkihiroSuda Dec 20, 2017

tonistiigi Dec 20, 2017

tonistiigi left a comment

tonistiigi Dec 20, 2017

tonistiigi Dec 20, 2017

tonistiigi Dec 20, 2017

AkihiroSuda commented Dec 20, 2017

tonistiigi left a comment •

edited

Loading

tonistiigi Dec 20, 2017

tonistiigi Dec 20, 2017

AkihiroSuda commented Dec 20, 2017

AkihiroSuda commented Dec 20, 2017

tonistiigi commented Dec 20, 2017

AkihiroSuda commented Dec 20, 2017

		@@ -68,11 +70,16 @@ func NewWorkerOpt(root string) (base.WorkerOpt, error) {

		// TODO: call mdb.GarbageCollect . maybe just inject it into nsSnapshotter.Remove and csContent.Delete

		id, xlabels := base.DetermineAnnotations(daemonInstanceID, "oci", "overlayfs")

worker: add annotation labels #232

worker: add annotation labels #232

Conversation

AkihiroSuda commented Dec 19, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkihiroSuda commented Dec 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

AkihiroSuda commented Dec 20, 2017

tonistiigi left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkihiroSuda commented Dec 20, 2017

AkihiroSuda commented Dec 20, 2017

tonistiigi commented Dec 20, 2017

AkihiroSuda commented Dec 20, 2017

tonistiigi left a comment •

edited

Loading