Add Evaluate(Ref) Gateway API #3137
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
crazy-max
reviewed
Sep 29, 2022
tonistiigi
reviewed
Sep 30, 2022
frontend/gateway/gateway.go
Outdated
| func (lbf *llbBridgeForwarder) Evaluate(ctx context.Context, req *pb.EvaluateRequest) (*pb.EvaluateResponse, error) { | ||
| ctx = tracing.ContextWithSpanFromContext(ctx, lbf.callCtx) | ||
|
|
||
| _, err := lbf.getImmutableRef(ctx, req.Ref, "/") |
There was a problem hiding this comment.
Not part of your PR, but why does getImmutableRef() take a path ? 🤯
There was a problem hiding this comment.
👀 seemingly to print a deceiving error message apparently...
func (lbf *llbBridgeForwarder) getImmutableRef(ctx context.Context, id, path string) (cache.ImmutableRef, error) {
lbf.mu.Lock()
ref, ok := lbf.refs[id]
lbf.mu.Unlock()
if !ok {
return nil, errors.Errorf("no such ref: %v", id)
}
if ref == nil {
return nil, errors.Wrap(os.ErrNotExist, path)
}Technically it's possible for a value in Result.Ref to be nil, but printing file does not exist: <path> for that doesn't feel like the right message - empty ref feels like a more apt description of what's gone wrong. Definitely an edge case, a sane frontend should never produce nil refs like that.
I'll add a commit on top of the stack in this PR to tidy up the method, good catch.
jedevc
force-pushed
the
evaluate-gateway
branch
from
a4f2ade
to
ba565ad
Compare
crazy-max
approved these changes
Oct 3, 2022
tonistiigi
reviewed
Oct 12, 2022
Signed-off-by: Justin Chadwell <me@jedevc.com>
Always use the value of the evaluate field to force result generation, which previously was not performed for frontends. This improves API consistency, and ensures the value is used regardless of whether the solver uses a frontend, or a raw definition. Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Justin Chadwell <me@jedevc.com>
The StatFile fallback needs reworking to ensure that all refs in the result are evaluated, not just the first one found. Signed-off-by: Justin Chadwell <me@jedevc.com>
Remove path argument from llbBridgeForward.getImmutableRef, as it is only used to print a deceiving error message. Contents of the Result.Refs map should not be nil, and sane frontends should not produce them - printing that the file cannot be found doesn't indicate that this is an abnormal edge case. Signed-off-by: Justin Chadwell <me@jedevc.com>
jedevc
force-pushed
the
evaluate-gateway
branch
from
ba565ad
to
62fbd5e
Compare
tonistiigi
approved these changes
Oct 12, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is a replacement for #2947 (cherry-picking that commit for consistency, so that calling
Evaluateover each ref in a result is the same as settingEvaluate: truein theSolveRequest).This can be invoked on a ref using
ref.Evaluate()in a frontend - this allows for on-demand unlazying a reference, instead of needing to wait until export. Previously, as a hack, this could be done usingStatFile, however this has the disadvantage of needing to pull the image to be able to mount it, which isn't required to ensure that the image is unlazied.Additionally, the previous
Evaluatefield in theSolveRequestwas not sufficient, as this causes theSolveto block - we may want to perform other operations, before then determining whether toEvaluate, as in docker/buildx#1197.