1324767 - Simplify input parameters for decision task

Status: RESOLVED WORKSFORME

(Taskcluster Graveyard :: Docker Images, defect)

Description

[Jonas Finnemann Jensen (:jonasfj)]

Basically, remove the need for specifying command in .taskcluster.yml

This will make it a lot easier to verify the definition later.

Comment 1

[Jonas Finnemann Jensen (:jonasfj)]

Attachment: Bug 1324767 - Simplify decision task definition;

Setting more environment variables, ENTRYPOINT and CMD in the
Dockerfile for the decision image as well as taking inputs from
environment variables in ./mach taskgraph decision, allows us
to greatly simplify the decision task definition in
.taskcluster.yml.

This is useful as a general cleanup, making it easier to
see what is input and what is template. But also simplifies
validation of task definitions, which is required when validating
the Chain-Of-Trust artifacts before signing.

Review commit: https://reviewboard.mozilla.org/r/100182/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/100182/

Comment 2

[Jonas Finnemann Jensen (:jonasfj)]

Comment on attachment 8820718 [details]
Bug 1324767 - Simplify decision task definition;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100182/diff/1-2/

Comment 3

[Jonas Finnemann Jensen (:jonasfj)]

Note to self:
  Remember to publish a final docker image to taskcluster registry and updated the hash, before setting
  checkin-needed.

Comment 4

[Jonas Finnemann Jensen (:jonasfj)]

Note: If landed as-is this also fixes bug 1324766.
But we might want to file a follow up bug for removing /var/lib/apt/lists/ from all the other images.

Comment 5

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8820718 [details]
Bug 1324767 - Simplify decision task definition;

https://reviewboard.mozilla.org/r/100182/#review100660

I'd like to get :gps's feedback on this, too.

::: taskcluster/mach_commands.py:32
(Diff revision 2)
> +        Action for use with argparse that supports loading option from
> +        environment variable if not specified.

Why continue to allow these as arguments, too?  `./mach taskgraph decision` should *only* be invoked in a decision task, so there's no need to support a variety of invocation styles..

::: taskcluster/mach_commands.py:177
(Diff revision 2)
> +    @CommandArgument('--artifacts',
> +                     required=True, action=EnvDefault, envvar='ARTIFACTS_FOLDER',
> +                     help='Folder that artifacts should be written to')

This seems a little magical, since the unlike the other env vars, this one is not set in the task definition.  It's particularly unusual in that only the decision task works this way.

I think the reasoning is that the directory is created in the Dockerfile, and the env var is set there, thereby co-locating the definition of the directory.  But tasks assume lots of things about their docker image, such as the 'worker' username.  I don't think the artifacts directory deserves special treatment.

Also, I prefer the word "directory" to "folder" :)

::: testing/docker/decision/Dockerfile:40
(Diff revision 2)
> +ENTRYPOINT    ["run-task", "--vcs-checkout=/home/worker/checkouts/gecko", "--"]
> +CMD           ["bash", "-cx", "cd /home/worker/checkouts/gecko && ./mach --log-no-times taskgraph decision"]

I like this!

People trying to hack the taskgraph, who don't know to read the docs, often start by trying to run `./mach taskgraph decision` locally, because that's what they see in the task definition.  This nicely hides the command :)

Comment 6

[Dustin J. Mitchell [:dustin] (he/him)]

Oh, I totally forgot -- you'll need to make the same adjustments to the action task (taskcluster/taskgraph/action.yml)

Comment 7

[Jonas Finnemann Jensen (:jonasfj)]

Comment on attachment 8820718 [details]
Bug 1324767 - Simplify decision task definition;

https://reviewboard.mozilla.org/r/100182/#review100660

> Why continue to allow these as arguments, too?  `./mach taskgraph decision` should *only* be invoked in a decision task, so there's no need to support a variety of invocation styles..

I guess it's nice in that the inputs is documented in a relatively sane manner.
But yeah, if you were to run locally it would probably be easier to write a script that sets the env vars, than supplying all the arguments on the commandline.

> I like this!
> 
> People trying to hack the taskgraph, who don't know to read the docs, often start by trying to run `./mach taskgraph decision` locally, because that's what they see in the task definition.  This nicely hides the command :)

IMO we should try to have `ENTRYPOINT ["run-task", "...", "--"]` for all our images and `CMD ["./mach", "help"]`.
Then `task.payload.command = ["./mach", "crashtest"]` and all the parameters specified by env vars.

It's a lot easier to parse env vars later, if anyone would want to. And having a simple command like `./mach crashtest` as task command makes it easier to guess how to run the equivalent locally.

Fixing `run-task` such that it executes the subcommand in the directory of the checkout would be really nice too.
I don't like the `bash -cx "cd ... && ./mach ..."` pattern, but I think we fixed that in a separate patch.

thoughts?

Comment 8

[Jonas Finnemann Jensen (:jonasfj)]

@dustin, why do we need to touch action.yml?
The old parameters still works, and I expect actions don't use the "decision" action, or am I wrong about that?

Comment 9

[Dustin J. Mitchell [:dustin] (he/him)]

I think we mostly agree.  My r- in comment 5 were just implementation issues.

Decision tasks and action tasks are mirrors of one another, really, and run almost the same code, so they should be invoked the same way.

Comment 10

[Gregory Szorc [:gps]]

Comment on attachment 8820718 [details]
Bug 1324767 - Simplify decision task definition;

https://reviewboard.mozilla.org/r/100182/#review104290

::: taskcluster/mach_commands.py:30
(Diff revision 2)
>  from mozbuild.base import MachCommandBase
>  
>  ARTIFACT_URL = 'https://queue.taskcluster.net/v1/task/{}/artifacts/{}'
>  
>  
> +class EnvDefault(argparse.Action):

This is a useful class! If you have time, would you mind moving it to python/mach? decorators.py is the best existing home for it. But it isn't a "decorator." Feel free to throw it in a new "argparsing.py" or something.

::: testing/docker/decision/Dockerfile:40
(Diff revision 2)
> +VOLUME        /home/worker/checkouts
> +ENV           HG_STORE_PATH /home/worker/checkouts/hg-store
> +
> +# Set some sane defaults
> +WORKDIR       /home/worker/
> +ENTRYPOINT    ["run-task", "--vcs-checkout=/home/worker/checkouts/gecko", "--"]

Nit: path for ENTRYPOINT and CMD should be absolute. TBH, I'm not sure exactly what happens when it is relative. The Docker docs imply the array form feeds things into `execv()`. But I don't trust Docker. Absolute paths are un-ambigious.

Comment 11

[Gregory Szorc [:gps]]

Comment on attachment 8820718 [details]
Bug 1324767 - Simplify decision task definition;

Sorry for the latency. This change looks pretty straightforward and a good improvement to readability!

Comment 12

[Dustin J. Mitchell [:dustin] (he/him)]

https://github.com/taskcluster/json-e/issues/115

Comment 13

[Dustin J. Mitchell [:dustin] (he/him)]

Jonas, where are we with this bug? Does it require JSON-e?

Comment 14

[Jonas Finnemann Jensen (:jonasfj)]

I don't think this requires JSON-e, but this definitely could have bit-rotted...

The idea was just to not provide a task.payload.commad in decision task, and instead pass all parameters as environment variables.

Comment 15

[Dustin J. Mitchell [:dustin] (he/him)]

Indeed, orthogonal to JSON-e -- thanks!

Comment 16

[Aki Sasaki (not active)]

We have json-e now, and we're close to json-e verification in cot, so I don't think this blocks cot-v2 anymore.

Comment 17

[Chris Cooper [:coop] (he/him)]

Found in triage today.

Jonas: is this still valid and on your radar?

Comment 18

[Pete Moore [:pmoore][:pete]]

Does this mean in future you have to put the command in the docker image?

Won't that mean that no task will be able to use an off-the-shelf docker image, for example, using "ubuntu" for the docker image?

That might be good for gecko CI, I'm not sure it is good for taskcluster platform in general.

Comment 19

[Dustin J. Mitchell [:dustin] (he/him)]

This is definitely a gecko-only issue, and is not forcing anything.  But it is simplifying the task definition for decision and action tasks, which currently have a long list of command-line arguments that's rather difficult to read.

I suspect this has bitrotted, and it's much less critical with CoTv2 which just renders the JSON-e either way.

Comment 20

[Aki Sasaki (not active)]

(In reply to Dustin J. Mitchell [:dustin] pronoun: he from comment #19)
> and it's much less critical with CoTv2 which
> just renders the JSON-e either way.

+1. I'm ok without this now.

Comment 21

[Jonas Finnemann Jensen (:jonasfj)]

I think most of this actually landed at some point, I agree with the worksforme