llb: per-vertex metadata (e.g. IgnoreCache) #114

AkihiroSuda · 2017-09-07T07:47:02Z

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

This PR is an initial step to add per-vertex metadata, of which structure is defined in solver/pb/ops.proto:

message Metadata {
	// key = LLB op digest string. Currently, empty string is not expected but may change in the future.
	map<string, MetadataEntry> entries = 1 [(gogoproto.castkey) = "github.com/opencontainers/go-digest.Digest", (gogoproto.nullable) = false];
}

message MetadataEntry {
	bool ignore_cache = 1;
	// TODO: add worker constraint, etc.
}

Currently, only ignore_cache is implemented.
buildctl build --no-cache sets ignore_cache for all the vertices.

Future version will contain worker constraint and so on: #62 (comment) #41 #81

@tonistiigi

tonistiigi · 2017-09-07T18:33:08Z

client/solve.go

-func (c *Client) Solve(ctx context.Context, r io.Reader, opt SolveOpt, statusChan chan *SolveStatus) error {
+// Solve calls Solve on the controller.
+// def and metadata are empty if opt.Frontend is set.
+func (c *Client) Solve(ctx context.Context, def [][]byte, metadata pb.Metadata, opt SolveOpt, statusChan chan *SolveStatus) error {


Avoid protobuf types in client pkg

tonistiigi · 2017-09-07T18:41:14Z

solver/pb/ops.proto

 }

 message BuildInput {
 	int64 input = 1 [(gogoproto.customtype) = "InputIndex", (gogoproto.nullable) = false];
+}
+
+message Metadata {


Is this really related to LLB definition at all, or controller definition? How about defining

struct { Definition [][]byte Metadata map[digest.Digest]MetadataEntry }

that would be what llb.Marshal generates and client.Solve takes as an input. buildctl can also understand that and override if needed. Then a script generating llb can also define it. For example, worker constraints need to be defined by the script not by the override in buildctl.

tonistiigi · 2017-09-07T18:45:15Z

solver/solver.go

@@ -479,6 +485,12 @@ func (s *Solver) getRef(ctx context.Context, g *vertex, index Index) (ref Refere
 			return false, err
 		}
 		if lookupRef != nil {
+			if md, ok := g.SysMetadata().(pb.MetadataEntry); ok && md.IgnoreCache {


Doesn't this only work for the last vertex?

AkihiroSuda · 2017-10-02T05:03:00Z

I'll rebase today

AkihiroSuda · 2017-10-02T09:16:21Z

solver/jobs.go

@@ -210,6 +211,9 @@ func (j *job) getRef(ctx context.Context, v *vertex, index Index) (Reference, er
 }

 func getRef(s VertexSolver, ctx context.Context, v *vertex, index Index, cache InstructionCache) (Reference, error) {
+	if v.sysMetadata.IgnoreCache {
+		logrus.Warnf("Unimplemented OpMetadata: IgnoreCache (%s, %s)", v.digest, v.name)


@tonistiigi
I'll try to implement cache invalidation in another PR, please review the data structure?

tonistiigi

Some comments. Most could be addressed in a follow-up. Overall looks good

tonistiigi · 2017-10-02T18:30:32Z

client/llb/exec.go

@@ -92,12 +93,12 @@ func (e *ExecOp) Validate() error {
 	return nil
 }

-func (e *ExecOp) Marshal() ([]byte, error) {
+func (e *ExecOp) Marshal() ([]byte, *pb.OpMetadata, error) {


I'd just (*llb.Definition, error). This can be follow-up

This function just returns []byte representation of single Op while llb.Definition contains [][]byte representation of multiple Ops.
Should we modify definition of llb.Definition?

ah, sorry. I mixed this up with https://github.com/moby/buildkit/pull/114/files#diff-23cdf6d75aab7a60db2eaa3d2b59710eR51 . No change required.

tonistiigi · 2017-10-02T18:34:01Z

cmd/buildctl/build.go

@@ -49,9 +53,36 @@ var buildCommand = cli.Command{
 			Name:  "frontend-opt",
 			Usage: "Define custom options for frontend",
 		},
+		cli.BoolFlag{
+			Name:  "no-cache",


should we remove this, for now, to avoid confusion if it doesn't work yet?

I put "not implemented" to the command line help string.
This will be soonly implemented.

tonistiigi · 2017-10-02T18:34:45Z

cmd/buildctl/build.go

+		}
+	} else {
+		if clicontext.Bool("no-cache") {
+			return errors.New("no-cache is not supported for frontends")


should we map these to frontend-opt ? (follow-up)

tonistiigi · 2017-10-02T18:38:22Z

client/llb/marshal.go

 	}
-	return nil
+	l := make([]byte, 4)


There is probably no reason to add a length if we just marshal the whole object. Just write/read the protobuf object.

tonistiigi · 2017-10-02T18:39:49Z

client/llb/state.go

@@ -48,48 +48,54 @@ func (s State) Value(k interface{}) interface{} {
 	return s.ctx.Value(k)
 }

-func (s State) Marshal() ([][]byte, error) {
-	list, err := marshal(s.Output().Vertex(), nil, map[digest.Digest]struct{}{}, map[Vertex]struct{}{})
+func (s State) Marshal() (Definition, error) {


nit: I like when functions like this return pointers. As it contains slices/maps it shares the memory anyway.

tonistiigi · 2017-10-02T18:43:18Z

solver/vertex.go

@@ -17,6 +18,7 @@ type Vertex interface {
 	// Sys returns an internal value that is used to execute the vertex. Usually
 	// this is capured by the operation resolver method during solve.
 	Sys() interface{}
+	SysMetadata() pb.OpMetadata


just Metadata(), and maybe return solver.Metadata interface? Atm the solver should be usable without llb definition.

Yes, updated PR.

However, Solver still has depend on the pb package:

func (s *Solver) Solve(ctx context.Context, id string, f frontend.Frontend, def *pb.Definition, exp exporter.ExporterInstance, frontendOpt map[string]string) error

Ok, yes I think I broke it when I moved the protobuf-to-vertex conversion to the job.load. Need to think about it later if there is a way around that.

AkihiroSuda · 2017-10-03T05:27:08Z

@tonistiigi updated

cache invalidation itself it not implemented yet Signed-off-by: Akihiro Suda <[email protected]>

AkihiroSuda · 2017-10-03T05:48:26Z

CI green

tonistiigi · 2017-10-03T17:36:53Z

LGTM

prevent long lived connection reuse

This was referenced Sep 7, 2017

RFC: control data structure for caching / source credential / worker spec #81

Closed

RFC: Distributed BuildKit (Swarm/Kubernetes/Mesos..) #62

Closed

AkihiroSuda force-pushed the llb-metadata branch from 545b19e to 0cee46f Compare September 7, 2017 08:00

tonistiigi reviewed Sep 7, 2017

View reviewed changes

tonistiigi mentioned this pull request Sep 27, 2017

solver: fix shared request progress and cancellation #130

Merged

AkihiroSuda force-pushed the llb-metadata branch from 0cee46f to e76c61a Compare October 2, 2017 09:13

AkihiroSuda commented Oct 2, 2017

View reviewed changes

AkihiroSuda force-pushed the llb-metadata branch from e76c61a to 2d881b2 Compare October 2, 2017 09:27

tonistiigi reviewed Oct 2, 2017

View reviewed changes

AkihiroSuda force-pushed the llb-metadata branch from 2d881b2 to 282b17f Compare October 3, 2017 05:27

llb: per-vertex metadata (e.g. IgnoreCache)

f89ffc2

cache invalidation itself it not implemented yet Signed-off-by: Akihiro Suda <[email protected]>

AkihiroSuda force-pushed the llb-metadata branch from 282b17f to f89ffc2 Compare October 3, 2017 05:36

tonistiigi mentioned this pull request Oct 3, 2017

load frontends from images #134

Merged

tonistiigi merged commit f419a8c into moby:master Oct 3, 2017

alexcb added a commit to alexcb/buildkit that referenced this pull request Feb 17, 2023

Merge pull request moby#114 from earthly/acb/timeout-tweaks

d6934ee

prevent long lived connection reuse

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

llb: per-vertex metadata (e.g. IgnoreCache) #114

llb: per-vertex metadata (e.g. IgnoreCache) #114

AkihiroSuda commented Sep 7, 2017

tonistiigi Sep 7, 2017

tonistiigi Sep 7, 2017

tonistiigi Sep 7, 2017

AkihiroSuda commented Oct 2, 2017

AkihiroSuda Oct 2, 2017 •

edited

Loading

tonistiigi left a comment

tonistiigi Oct 2, 2017

AkihiroSuda Oct 3, 2017

tonistiigi Oct 3, 2017

tonistiigi Oct 2, 2017

AkihiroSuda Oct 3, 2017

tonistiigi Oct 2, 2017

tonistiigi Oct 2, 2017

tonistiigi Oct 2, 2017

tonistiigi Oct 2, 2017

AkihiroSuda Oct 3, 2017

tonistiigi Oct 3, 2017 •

edited

Loading

AkihiroSuda commented Oct 3, 2017

AkihiroSuda commented Oct 3, 2017

tonistiigi commented Oct 3, 2017

llb: per-vertex metadata (e.g. IgnoreCache) #114

llb: per-vertex metadata (e.g. IgnoreCache) #114

Conversation

AkihiroSuda commented Sep 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkihiroSuda commented Oct 2, 2017

AkihiroSuda Oct 2, 2017 • edited Loading

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

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

tonistiigi Oct 3, 2017 • edited Loading

Choose a reason for hiding this comment

AkihiroSuda commented Oct 3, 2017

AkihiroSuda commented Oct 3, 2017

tonistiigi commented Oct 3, 2017

AkihiroSuda Oct 2, 2017 •

edited

Loading

tonistiigi Oct 3, 2017 •

edited

Loading