Simplify input parameters for decision task

RESOLVED WORKSFORME

Status

RESOLVED WORKSFORME
2 years ago
3 months ago

People

(Reporter: jonasfj, Assigned: jonasfj)

Tracking

Details

Attachments

(1 attachment)

(Assignee)

Description

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

This will make it a lot easier to verify the definition later.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 3

2 years ago
Note to self:
  Remember to publish a final docker image to taskcluster registry and updated the hash, before setting
  checkin-needed.
(Assignee)

Comment 4

2 years ago
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

2 years ago
mozreview-review
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 :)
Attachment #8820718 - Flags: review?(dustin) → review-
Attachment #8820718 - Flags: feedback?(gps)
Oh, I totally forgot -- you'll need to make the same adjustments to the action task (taskcluster/taskgraph/action.yml)
(Assignee)

Comment 7

2 years ago
mozreview-review-reply
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?
(Assignee)

Comment 8

2 years ago
@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?
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

2 years ago
mozreview-review
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 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!
Attachment #8820718 - Flags: feedback?(gps) → feedback+

Updated

2 years ago
Blocks: 1328719
(Assignee)

Updated

2 years ago
See Also: → bug 1335918
(Assignee)

Updated

2 years ago
Depends on: 1335955
(Assignee)

Updated

2 years ago
No longer depends on: 1335955
Jonas, where are we with this bug? Does it require JSON-e?
Flags: needinfo?(jopsen)
(Assignee)

Comment 14

a year ago
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.
Flags: needinfo?(jopsen)
Indeed, orthogonal to JSON-e -- thanks!
See Also: bug 1335918

Comment 16

10 months ago
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.
No longer blocks: 1328719
Found in triage today.

Jonas: is this still valid and on your radar?
Flags: needinfo?(jopsen)
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.
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

8 months ago
(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.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
Resolution: --- → WORKSFORME
(Assignee)

Comment 21

8 months ago
I think most of this actually landed at some point, I agree with the worksforme
Flags: needinfo?(jopsen)

Updated

3 months ago
Product: Taskcluster → Taskcluster Graveyard
You need to log in before you can comment on or make changes to this bug.