Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dockerfile: support for outline requests #2841

Merged
merged 10 commits into from
Aug 9, 2022
Merged

Conversation

tonistiigi
Copy link
Member

Adds a new subrequest frontend.outline that returns
all the build arguments the current build definition
takes without actually running the build.

This allows getting information about the possible build parameters without needing to study the whole Dockerfile. Every build arg also includes the current value, description and location of the build argument. Description is parsed from the associated comment.

I also plan to add a request that lists all possible build targets.

The second commit adds ability to buildctl to see the result of the subrequests (at least when they return result.* metadata). Previously this was only possible with the Go API.

buildctl build --frontend dockerfile.v0 --local dockerfile=. --local context=. --opt requestid=frontend.subrequests.describe
buildctl build --frontend dockerfile.v0 --local dockerfile=. --local context=. --opt requestid=frontend.outline
{
  "args": [
    {
      "Name": "RUNC_VERSION",
      "Description": "",
      "Value": "v1.0.2",
      "Location": {
        "ranges": [
          {
            "start": {
              "Line": 3
            },
            "end": {
              "Line": 3
            }
          }
        ]
      }
    },
    {
      "Name": "BUILDKIT_TARGET",
      "Description": "",
      "Value": "buildkitd",
      "Location": {
        "ranges": [
          {
            "start": {
              "Line": 10
            },
            "end": {
              "Line": 10
            }
          }
        ]
      }
    },
    {
      "Name": "ALPINE_VERSION",

@AkihiroSuda
Copy link
Member

What would be the expected use cases? 👀

@tonistiigi
Copy link
Member Author

What would be the expected use cases? 👀

When you clone a project and want to build it or thinking what flags docker build takes you currently need to open the Dockerfile and study it to understand what parameters are possible. Even doing that is complicated if there are many stages and you can't easily visualize what stages depend on other stages. This allows getting a list of all accepted parameters and their descriptions so users can see what is being built and what configuration options they have.

args = append(args, outline.Arg{
Name: a.definition.Key,
Value: a.value,
Description: a.definition.Comment,
Copy link
Member

@crazy-max crazy-max May 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if the comments used as description for args and stages should have a special markup to avoid unwanted comments like https://github.com/crazy-max/moby/blob/82ede62397d3de933e79483f84d3a8092587485f/Dockerfile#L237-L239

I have this use case for example where I want to explicitly tell what stages the user should care about: https://github.com/crazy-max/moby/blob/82ede62397d3de933e79483f84d3a8092587485f/Dockerfile#L618-L643

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It already does this

@AkihiroSuda
Copy link
Member

Needs rebase

@ktock
Copy link
Collaborator

ktock commented Jul 25, 2022

SGTM about the feature but how should multi-staged Dockerfile be handled by this feature?
Currently it seems possible that the outline loses info for some args on multi-stage Dockerfile.
For example, the following Dockerfile contains FOO at line 4 and 8 but outline shows arg only of line 4.

ARG FOO

FROM busybox AS foo-1
ARG FOO=foo1
RUN echo $FOO > /foo1

FROM busybox AS foo-2
ARG FOO=foo2
RUN echo $FOO > /foo2

FROM scratch
COPY --from=foo-1 /foo1 /foo1
COPY --from=foo-2 /foo2 /foo2
buildctl build --frontend dockerfile.v0 --local dockerfile=/tmp/ctx --local context=/tmp/ctx --opt requestid=frontend.outline
{
  "args": [
    {
      "Name": "FOO",
      "Description": "",
      "Value": "foo1",
      "Location": {
        "ranges": [
          {
            "start": {
              "Line": 4
            },
            "end": {
              "Line": 4
            }
          }
        ]
      }
    }
  ],
  "sources": [
    "QVJHIEZPTwoKRlJPTSBidXN5Ym94IEFTIGZvby0xCkFSRyBGT089Zm9vMQpSVU4gZWNobyAkRk9PID4gL2ZvbzEKCkZST00gYnVzeWJveCBBUyBmb28tMgpBUkcgRk9PPWZvbzIKUlVOIGVjaG8gJEZPTyA+IC9mb28yCgpGUk9NIHNjcmF0Y2gKQ09QWSAtLWZyb209Zm9vLTEgL2ZvbzEgL2ZvbzEKQ09QWSAtLWZyb209Zm9vLTIgL2ZvbzIgL2ZvbzIK"
  ]
}

@tonistiigi
Copy link
Member Author

@ktock A build arg can only have one value because there is only one way to pass that in. What you are seeing there is the default because no value was set. I don't think this is a practical case where arg with different default values are used together as there is no way to control them differently. So in practical cases there would either be two args or they would have the same config if they are controlled by the same build arg. We could consider separating out the default and passed value, although for the user flow they would be combined.

I'm going to do a second pass of this based on feedback. Will add JSON and Text output separately and some versioning so that we can detect when client/daemon is newer than expected and returns a modified structure.

@ktock
Copy link
Collaborator

ktock commented Jul 27, 2022

@ktock A build arg can only have one value because there is only one way to pass that in. What you are seeing there is the default because no value was set. I don't think this is a practical case where arg with different default values are used together as there is no way to control them differently. So in practical cases there would either be two args or they would have the same config if they are controlled by the same build arg. We could consider separating out the default and passed value, although for the user flow they would be combined.

Thank you for the explanation. SGTM.

@tonistiigi tonistiigi force-pushed the outline branch 2 times, most recently from fc9c0aa to 656f6fb Compare August 1, 2022 05:20
Copy link
Collaborator

@ktock ktock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM

This allows getting a list of all accepted parameters and their descriptions so users can see what is being built and what configuration options they have.

Sounds like godoc but for Dockerfile (dockerfiledoc)?

Even doing that is complicated if there are many stages and you can't easily visualize what stages depend on other stages

Maybe we'll need a field to show the dependency in the targets.Target struct.

args = append(args, outline.Arg{
Name: a.definition.Key,
Value: a.value,
Description: a.definition.Comment,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can we add a description to the FROM of the final stage without name?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not define a name if you want to add a description? It would still work as a default target but would be descriptive.

@tonistiigi
Copy link
Member Author

@bpaquet S3 CI seems to be failing quite reliably after the rebase here but I don't quite see what could be the connection or where it is failing. Could you take a look.

cmd/buildctl/build.go Outdated Show resolved Hide resolved
Adds a new subrequest frontend.outline that returns
all the build arguments the current build definition
takes without actually running the build.

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Make it easier for all buildkit clients to have
formatted subrequest results.

Version can be used for client to detect if frontend
returns structure that is doesn’t know about. Then
client can make a choice to just print the already
formatted contents.

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
@tonistiigi
Copy link
Member Author

Maybe we'll need a field to show the dependency in the targets.Target struct.

@ktock I've added the base name and platform to the target struct. I don't know a sensible way to print them though so atm they are just in the JSON output.

I've also added versioning so these structures can be updated in the future(and client can detect if the version is different than what they know) and moved the text printer helpers from docker/buildx#1100 to here so more clients can print that output directly.

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
"github.com/moby/buildkit/solver/pb"
)

const RequestSubrequestsOutline = "frontend.outline"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels weird to me, since the describe request is of the form "frontend.subrequests.describe". I get that that describe is meta about the other subrequests, but it means that we have the weird feature that a client querying on buildx has: outline, targets and subrequests.describe. I think we should be able to hide from the user that we have subrequests behind --print, at least on the buildx side - maybe this is something to solve on the client instead, not sure.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is bit weird. But I didn't want users to need to type in --print subrequests.outline. On the same time I don't want to break subrequests.describe because we already released that some time ago and maybe somebody is using it. I also think that --print describe printing the list of possible commands is a bit misleading(user would more likely expect it to describe their build).

return err
}

if txt, ok := subMetadata["result.txt"]; ok {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would we want a case for result.json as well? Feels like we could format it here, in case the frontend hasn't done it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We would need a specific printer for this and I don't want buildctl to have too much hardcoded info about specific requests.

Or are you saying that if there is a result.json we should only print that directly and not the full result like it is in current commit.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we have an exception for result.txt, I'm just wondering why we shouldn't also have one for result.json. I think at the least, we should print directly and not the whole result if we find a result.json similar to result.txt.

Although maybe for buildctl, since it's a dev tool, we should assume no knowledge of the response, and always print out everything?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and always print out everything?

That what it does atm. There is a special case for result.txt, so if it exists then the expectation is that frontend has already formatted the result and we can just print that output. Otherwise we just print whatever frontend provided(including result.json). If it makes more sense I can make it so that it only prints result.json if it exists and skips the rest.

Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been using ListTargets and Dockerfile2Outline to great affect here: https://github.com/deislabs/gnarly/blob/a4b11b88d008a64ae12c75712718d0400a309c7f/generate.go#L29-L49

Definitely looking forward to having this in the API.

frontend/dockerfile/dockerfile2llb/convert.go Outdated Show resolved Hide resolved
Even though this is a pretty invalid use-case there are
cases where these could carry some information.

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
@tonistiigi tonistiigi merged commit 8488654 into moby:master Aug 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants