1478935 - Prevent releaseduty from starting the ship graph when the push one hasn't been kicked off

Status: RESOLVED FIXED

(Release Engineering :: Release Automation: Other, enhancement)

Description

[Johan Lorenzo [:jlorenzo]]

Today, instead of kicking off the push graph for devedition 62.0b12, the ship one was started[1]. Due to the way taskgraph works, it also scheduled the missing push tasks. That's why nothing broke and devedition push and ship tasks were done at the same time.

This is probably something ship-it v2[2] will improve: instead of pasting shell commands, it will be just a matter of clicking buttons. Moreover, the UI prevents from clicking a phase button if previous phases aren't scheduled yet.

While ship-it v2 officially makes to production, we may want to prevent this kind of footgun. We can modify trigger_action.py[3] to enforce a push graph to be passed if the action flavor contains "ship_".

Does this sound reasonable to you, Aki and Rail?


[1] https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&revision=23bc64507f53b0cba44c72f72539716adcf02d68&selectedJob=190452585
[2] https://shipit-ui-workflow.staging.mozilla-releng.net/
[3] https://dxr.mozilla.org/build-central/rev/c70700db492bf81c9f5551d9cfdfea851fb7e2ce/tools/buildfarm/release/trigger_action.py#102

Comment 1

[Johan Lorenzo [:jlorenzo]]

Status update, :jcristau from release management is okay to leave devedition 62.0b12 in this state until QE greenlights the build

Comment 2

[Rail Aliiev [:rail]]

We can enhance https://dxr.mozilla.org/build-central/rev/c70700db492bf81c9f5551d9cfdfea851fb7e2ce/tools/buildfarm/release/trigger_action.py#20 SUPPORTED_ACTIONS so the structure also contains dependencies 

Something like this maybe:

SUPPORTED_ACTIONS = [
    {"name": "promote_firefox_partners", "requires": ["promote_firefox"]},
    ...
    {"name": "push_firefox", "requires": ["promote_firefox"]},
    ...
    {"name": "ship_firefox", "requires": ["promote_firefox", "push_firefox"]},
    {"name": "ship_firefox_rc", "requires": ["promote_firefox"]},
    ...
]

Then we can add some checks:
* compare the number of passed task ids with required. This should be very trivial.
* fetch the passed tasks and compare their definitions (task.extra.*.release_promotion_flavor) with the required one and also check the run result.

Does it make sense?

Comment 3

[Aki Sasaki (not active)]

1) This is intentional. The idea is that if we want to push and ship something simultaneously, we can go straight to the ship phase and skip the push phase.

1a) however, because beetmover and balrog change their behavior based on what phase we're in, rather than what phase they're targeting, this breaks in certain cases. For instance, beetmover in the promote shipping-phase graph will push to candidates. The same task in the push shipping-phase will attempt to push to releases. This means that if we skip the promote phase and go straight to the push phase, nothing pushes to candidates and the beetmover tasks try to push to releases (and the task that should be the candidates beetmover task has the wrong task definition  for a push to releases task anyway). This is a bug we'll have to fix if we want (1) to work properly.

2) If the potential footgun of accidentally skipping a step is large, and the convenience gained by being able to skip a step is small (sounds like it is), then we can force ourselves to run every step individually. If we want to push+ship something at once, someone will have to push two buttons in a row rather than one. That's fine with me if that solution makes the most sense.

Comment 4

[Johan Lorenzo [:jlorenzo]]

Attachment: Bug 1478935 - Prevent releaseduty from starting the ship graph when the push one hasn't been kicked off

Prevent releaseduty from starting the ship graph when the push one hasn't been kicked off

Comment 5

[Johan Lorenzo [:jlorenzo]]

Thank you guys for the context!

I agree with Aki's points. I think point 2 outweighs the comfort of shipping all things at once, in one graph. I think pushing 2 buttons (or running 2 commands) is better for humans => it makes things explicit => we do 2 steps. Moreover, it fits the releasewarrior workflow: last Friday, I worked around the graphs by adding it twice in releasewarrior[1] - this may confuse future reader, by the way. Finally, it's still possible to schedule the push graph, even if the promote graph has started yet (and same for the ship one). 

The cost of kicking off a ship graph (instead of the ship-rc, for instance) on mozilla-release is too high. Reverting each action requires manual and error-prone actions. Mihai listed them on IRC, this Friday:

> a) release-balrog-scheduling-devedition, b) release-bouncer-aliases-devedition, c) release-mark-as-shipped-devedition, d) release-version-bump-devedition and e) release-notify-ship-devedition

I think we should prevent from this. I'm glad this was caught against DevEdition :) 

So I went ahead, and implemented Rail's proposal. I tested the changes manually locally. Tell me what you guys think!

[1] https://github.com/mozilla-releng/releasewarrior-data/blob/master/inflight/devedition/devedition-devedition-62.0b12.md#graphs

Comment 6

[Phabricator Automation]

Comment on attachment 8995976 [details]
Bug 1478935 - Prevent releaseduty from starting the ship graph when the push one hasn't been kicked off

Rail Aliiev [:rail] ⌚️ET has approved the revision.

https://phabricator.services.mozilla.com/D2513

Comment 7

[Phabricator Automation]

Comment on attachment 8995976 [details]
Bug 1478935 - Prevent releaseduty from starting the ship graph when the push one hasn't been kicked off

Aki Sasaki [:aki] has approved the revision.

https://phabricator.services.mozilla.com/D2513

Comment 8

[Johan Lorenzo [:jlorenzo]]

Landed on default at https://hg.mozilla.org/build/tools/rev/98d1e5b4035a399737ff06e0a76f6cae369696b3

Updated:
 * Staging from b283f44ed551 to 98d1e5b4035a on bm83
 * Prod from 61c5335d64e2 to 98d1e5b4035a on bm85

Re-tested against a fennec and a firefox staging release on bm83.