control: Add ResolveImageConfig()

[ijc]

This is consistent with the gateway and frontend apis.

The benefit of a server side lookup is that the cache outlives a single client
invocation.

Signed-off-by: Ian Campbell [email protected]

[tonistiigi]

I agree that client.Client should provide an interface that is the same as gateway.Client. So client should provide this feature but not sure about the current implementation.

The problem with the suggested solution is that it does not work on private images.

The benefit of a server side lookup is that the cache outlives a single client
invocation.

If this is the only benefit it could also be achieved with client side cache.

[ijc]

If this is the only benefit it could also be achieved with client side cache.

That's what we have today, the problem is that cache only lives as long as the client invocation, so two consecutive back to back runs do not benefit from the cache. I'm talking about a "one shot" build tool type client scenario rather than e.g. docker engine where the client persists over multiple builds.

I suppose the client could build a persistent cache on disk somewhere. Perhaps helpers in the client library could be provided for that?

Is the issue with private images not the same for the gateway.Client case (and for that matter the frontend case)? I imagine the solution for both cases would look pretty similar? some sort of proxying of credentials perhaps? combined with tagging the cache entries appropriately I guess...

Does the underlying imageMetaResolver code do auth and I'm just missing it?

[tonistiigi]

Is the issue with private images not the same for the gateway.Client case

No, in gateway client private images work automatically through the build session.

Does the underlying imageMetaResolver code do auth and I'm just missing it?

If you are thinking about the llb/imagemetaresolver then that is a helper function that directly works on the client with default clean resolvers/transports.

[ijc]

Is the issue with private images not the same for the gateway.Client case

No, in gateway client private images work automatically through the build session.

OK. Perhaps the answer is to similarly attach a session to the client.ResolveImageConfig call so it can also use the auth stuff?

Does the underlying imageMetaResolver code do auth and I'm just missing it?

If you are thinking about the llb/imagemetaresolver then that is a helper function that directly works on the client with default clean resolvers/transports.

Does "clean" here imply "unauthenticated" or something else?

The daemon side equivalent is the one in util/pull/resolver.go, right? So really this PR should be hooking into that and not using llb/imagemetaresolver, which is what I thought:

 w, err := c.opt.WorkerController.GetDefault()
 .. w.ResolveImageConfig

was doing, although maybe I've followed an interface wrongly somewhere along the line..,.

[tonistiigi]

OK. Perhaps the answer is to similarly attach a session to the client.ResolveImageConfig call so it can also use the auth stuff?

Yes. That is something that I meant with getting access to the gateway.Client. Basically, there should be a function where you set the session and kv options in the client.Client that returns you the gateway.Client where all the solve/resolve functions would be scoped to the correct session/locals etc. That is a bigger change but I think a better approach and will reduce the gap between client and frontend development.

Does "clean" here imply "unauthenticated" or something else?

Yes, unauthenticated. I meant doing a pull with default containerd client functions directly. Another point is resolving the local images that the "clean" approach can't do but this probably should.

[ijc]

Yes. That is something that I meant with getting access to the gateway.Client. Basically, there should be a function where you set the session and kv options in the client.Client that returns you the gateway.Client where all the solve/resolve functions would be scoped to the correct session/locals etc. That is a bigger change but I think a better approach and will reduce the gap between client and frontend development.

I think that would be awesome! I've been trying to keep my client code in a state where flipping to using frontend wouldn't be too much effort, so far quite incompletely though. The main sticking point I've noticed there is Solve returning a ref which can be used with ReadFile, I'm sure it's possible but as you say it'd be a bigger change, seems worth it though.

[ijc]

there should be a function where you set the session and kv options in the client.Client that returns you the gateway.Client

@tonistiigi I had originally been assuming you were talking about a client side wrapper for existing client.Client functionality, but on the way in to the office this morning it occurred to me that you might have actually meant a client.Client gRPC method to return a new handle to a daemon side gateway object, which suddenly made a lot of sense (and might be a whole lot easier too).

I'd like to take a look at this but don't want to go down the wrong path since they are rather different implementation wise.

[tonistiigi]

We could do it in two ways. First would be to add methods like

NewBuild()
Solve(buildID) Result
Status(buildID) // already exists
DeleteBuild(buildID)
// Release(buildID) // releases results but leaves build metadata

ResolveImageConfig(buildID, ...)
ListBuilds()

ReadFile(Result, file)
StatFile(Result, file)

to the control API. It should be possible to implement gateway.Client with this API. The current Solve would be implemented as New+Solve+Delete combo.

The second option would be to actually add the full gateway proto API to the control API as well. With the caveat that to make these requests, a buildID needs to be set in the context. We will still need NewBuild/(Release)/DeleteBuild in the control API with possibly some automatic cleanup when the client goes away.

I'm not fully convinced, what method is better so suggestions are welcome. A benefit for the second one could be that we get version support for free from the gateway API.

[ijc]

some automatic cleanup when the client goes away.

That's the trickiest bit, I think. It looks like it should be possible for all the state about a buildID to be held daemon side (so e.g. no client side TempDir associated with a Result), in which case perhaps it is sufficient to provide DeleteBuild as a best-effort thing on the client's side, but then a full cleanup can be hooked off the client shutdown on the server side?

Given that at the go level I think I would most naturally expect to be able to do something like

import (
    buildkit "github.com/moby/buildkit/client"
)
...

    c := buildkit.New(ctx, opts.Addr, bkopts...)
    ...
    gw := c.NewBuild(ctx)
    ...

to get a gw which is something implementing the github.com/moby/buildkit/frontend/gateway/client.Client interface which I could then pass to the actual "builder" (where currently I pass the c).

It seems like it would be easiest is the buildID were just part of the underlying "proxy" struct providing that interface (alongside the github.com/moby/buildkit/client.Client it needs to access the control api) and then the wrappers would just use it when calling the client functions (modified as you have above). Doing a nice interface at the Go level via contexts seems like it wouldn't work so cleanly on first glance.

I was also considering whether it would be possible to dynamically add new grpc servers on the server side, corresponding to the creation of a new "buildID", and have a client to that be returned (kind of like the inverse of the FSSync mechanism). Seems more complex than either of what you proposed though.

[tonistiigi]

but then a full cleanup can be hooked off the client shutdown on the server side?

Another way to accomplish this would be to associate a timeout with the newBuild and then client would ping to extend it. If something happens to the client and the timeout is reached, the references are released. The build itself should probably also have a state values: active and complete, with only active builds having the possibility to use the gateway API.

Doing a nice interface at the Go level via contexts seems like it wouldn't work so cleanly on first glance.

I'm not sure I understand this. Is this the second option? If yes, I don't understand the complications.

I was also considering whether it would be possible to dynamically add new grpc servers on the server side, corresponding to the creation of a new "buildID", and have a client to that be returned (kind of like the inverse of the FSSync mechanism). Seems more complex than either of what you proposed though.

Problems with these solutions are that they are hard to document as they work on a hijacked stream. If it is possible to avoid, I'd just expose the grpc services directly. Eg. adding buildID to the context of gateway methods seems much cleaner than this.

[tonistiigi]

By "adding buildID to the context" I mean adding it to the grpc metadata https://godoc.org/google.golang.org/grpc/metadata that is transported through the context. Not that the user itself needs to pass it along when making the gateway queries.

[ijc]

I'm not sure I understand this. Is this the second option? If yes, I don't understand the complications.

It was based on a misunderstanding of the second option (thought you meant context.Context), I think I get it now.

I'll have a go at hacking up something based on gRPC contexts over the next days.

[ijc]

Played with this a bit

The current Solve would be implemented as New+Solve+Delete combo

In order to satisfy/implement the gateway variant of Solve interface I think we need to decompose controlapi.Solve one more step and split out the export phasetoo, so we can have a Solve which returns some sort of ref which can be used by the clients side gateway to satisfy gateway.Reference.

Then the existing controlapi Solve would actually become above New+Solve+Export+Delete.

I've not actually tried this yet. Since I've mostly been playing with your other alternative.

The second option would be to actually add the full gateway proto API to the control API as well.

There are some RPC name clashes (the most obvious being Solve). Did you envisage adding each gateway method Foo as controlapi.GatewayFoo or actually having Controller.Register call RegisterLLBBridgeServer with some suitable derived object (or equivalently punting that to the caller e.g. buildkitd)? I think the former is just a slight variant on your first suggestion so probably not that one, so I guess the second is closer to what you were thinking?

At first I had been trying to make the Solve method change behaviour based on the presence of a buildid in metadata, in hindsight that seems to be a wrong approach (ugly hacks involving a "ref exporter", unclear about the resulting ref ownership or cleanup, multiple ref case a bit tricky too, this is how I concluded the need to split Solve→Solve+Export in the other option too BTW).

I'll try the RegisterLLBBridgeServer approach now instead.

[tonistiigi]

Then the existing controlapi Solve would actually become above New+Solve+Export+Delete.

There is a Return method in the gateway proto that takes a reference map and exporter options. The actual exporter name(and default options) can be set with new. (Unless we see a use case of invoking exporter multiple times).

Did you envisage adding each gateway method Foo as controlapi.GatewayFoo or actually having Controller.Register call RegisterLLBBridgeServer with some suitable derived object

The latter. The receiver would validate the presence of buildid in metadata before continuing.

[ijc]

@tonistiigi The head commit of my ijc/client-gateway branch is a first cut of your second proposal. Setup and use only, no teardown nor many of the other bits you'd need (no filesyn, no opts), but WDYT does the basic shape pass the initial sniff test?

[tonistiigi]

@ijc Basically yes, but for the client public API I'd expect something like.

client.Build(ctx, BuildOpt{
	    Exporter          string
	    ExporterAttrs     map[string]string
	    ExporterOutput    io.WriteCloser // for ExporterOCI and ExporterDocker
	    ExporterOutputDir string         // for ExporterLocal
	    LocalDirs         map[string]string
	    SharedKey         string
	    ExportCache       string
	    ExportCacheAttrs  map[string]string
	    ImportCache       []string
	    Session           []session.Attachable
            StatusChan        chan *SolveStatus
}, func(context.Context, gw.Client) (*gw.Result, error) {

})

The returned result would be exported and refs automatically released. We may want leave options to add something like ls/rm/view_trace in the future.

[ijc]

There is a Return method in the gateway proto that takes a reference map and exporter options. The actual exporter name(and default options) can be set with new.

I see:

service LLBBridge {
«...»
        rpc Return(ReturnRequest) returns (ReturnResponse);
}

message Result {
        oneof result {
                string ref = 1;
                RefMap refs = 2;
        }
        map<string, bytes> metadata = 10;
}
«...»
message ReturnRequest {
        Result result = 1;
        google.rpc.Status error = 2;
}

message ReturnResponse {
}

But I don't see where the exporter stuff is in that, nor can see anything like that in the backend code. In particular the only place I see Worker.Exporter being called is in Controller.Solve. Although there's a bunch of back and forward compat code in the various Solve/Return methods which might be confusing me into missing it.

[tonistiigi]

The Return method doesn't do the export, it lets the main solver know that the frontend(gw) has completed all its work and what refs were the result of that work. Then the llbsolver regains the control of the build and performs the export using the config already sent to it with build request(with additional metadata that gw can produce). Don't worry about the compatibility code too much as it is all in gateway that doesn't do export itself. Export is in llbsolver.

We could either change the Return handler to block until the export has completed as well. Or in client.Build the function would not return before statusChan has been closed, even if the gw build function returned. I think the latter would be much easier.

[ijc]

it lets the main solver know

So far there is no "main solver" in my code, so there is nothing at the moment which would act on any export requests.

At the point where I've called the user provided BuildFunc and have a *Result in my hand I am currently using the same code as grpclient/Run (in fact i did a hacky refactor for now to reuse the code). I "just" need to do something with the *Result I got from Return, but so far I don't see an existing RPC call I should be using.

Should I be calling Solve, a bit like grpclient/Run does at the end (in the !export block) in order to do the export? Or should there be a new controlapi.Export, or perhaps I should do the export in the controlapi wrapped version of Return?

I've pushed my current state to https://github.com/ijc/buildkit/tree/client-gateway (it's several commits though and very hacky).

[ijc]

Ah, I think I need to reuse the tail of Solver.Solve (from var exporterResponse map[string]string onwards). I just need to decide where that should happen -- perhaps DeleteBuild (more like FinishBuild then) should do this and return an ExportResponse similar to what Solve returns?

[ijc]

I got something working and pushed it to the branch. Ugly/WIP as all hell though, will clean up next week.

[ijc]

Obsoleted by #533