1252003 - Add "publish to balrog" builder

Status: RESOLVED FIXED

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

Description

[Rail Aliiev [:rail]]

We still need to automate publishing updates to final channels:
* beta or release for RCs
* beta for betas
* release for dot releases

Comment 1

[Kim Moir [:kmoir] ET]

Attachment: bug1252003bbcustom.patch (WIP)

Comment 2

[Kim Moir [:kmoir] ET]

Attachment: bug1252003releasetasks.patch

Comment 3

[Kim Moir [:kmoir] ET]

Attachment: bug1252003intree.patch (WIP)

I don't like this approach because it just copies parts of scripts/release/updates.py that are used for balrog submission, seems like we could refactor for less duplication (comments to this effect in bug 1210539)

Comment 4

[Kim Moir [:kmoir] ET]

So for the updates builder we have intree files that specify things like the balrog url, channels etc
Kims-MacBook-Pro:releases kmoir$ pwd
/Users/kmoir/hg/mozilla-inbound/testing/mozharness/configs/releases
Kims-MacBook-Pro:releases kmoir$ ls *updates_*
dev_updates_firefox_beta.py	dev_updates_firefox_release.py	updates_firefox_beta.py		updates_firefox_release.py
Kims-MacBook-Pro:releases kmoir$ cat updates_firefox_beta.py

config = {
    "log_name": "updates_beta",
    "repo": {
        "repo": "https://hg.mozilla.org/build/tools",
        "revision": "default",
        "dest": "tools",
        "vcs": "hg",
    },
    "push_dest": "ssh://hg.mozilla.org/build/tools",
    "shipped-locales-url": "https://hg.mozilla.org/releases/mozilla-beta/raw-file/{revision}/browser/locales/shipped-locales",
    "ignore_no_changes": True,
    "ssh_user": "ffxbld",
    "ssh_key": "~/.ssh/ffxbld_rsa",
    "archive_domain": "archive.mozilla.org",
    "archive_prefix": "https://archive.mozilla.org/pub",
    "previous_archive_prefix": "https://archive.mozilla.org/pub",
    "download_domain": "download.mozilla.org",
    "balrog_url": "https://aus5.mozilla.org",
    "balrog_username": "ffxbld",
    "update_channels": {
        "beta": {
            "version_regex": r"^(\d+\.\d+(b\d+)?)$",
            "requires_mirrors": True,
            "patcher_config": "mozBeta-branch-patcher2.cfg",
            "update_verify_channel": "beta-localtest",
            "mar_channel_ids": [],
            "channel_names": ["beta", "beta-localtest", "beta-cdntest"],
            "rules_to_update": ["firefox-beta-cdntest", "firefox-beta-localtest"],
        },
    },
    "balrog_use_dummy_suffix": False,
}

For the publish to balrog builder we use just need to update one rule, for example for ff beta is is 91.  It feels like a lot of overhead to create another file for this builder besides updates_firefox_beta.py and then make a new file with a pared down version of scripts/release/updates.py that only submits to balrog.  Wondering if there is better approach then my wip patches.

Comment 5

[Rail Aliiev [:rail]]

I think you can reuse these configs. I'd just add another variable similar to "rules_to_update", something like 
.......
    "update_channels": {
        "beta": {
            "version_regex": r"^(\d+\.\d+(b\d+)?)$",
            "requires_mirrors": True,
            "patcher_config": "mozBeta-branch-patcher2.cfg",
            "update_verify_channel": "beta-localtest",
            "mar_channel_ids": [],
            "channel_names": ["beta", "beta-localtest", "beta-cdntest"],
            "rules_to_update": ["firefox-beta-cdntest", "firefox-beta-localtest"],
            # Added this, not sure if this is the best name :)
            "publish_rules": ["firefox-beta"],
        },
    },
.....

I used the alias instead of the rule id. This way we don't need to worry about them being different in prod and dev

Comment 6

[Kim Moir [:kmoir] ET]

Attachment: bug1252003intree-2.patch

these changes are against date

Comment 7

[Kim Moir [:kmoir] ET]

Attachment: bug1252003bbcustom-2.patch

Comment 8

[Kim Moir [:kmoir] ET]

Comment on attachment 8736484 [details] [diff] [review]
bug1252003releasetasks.patch

I don't have a good way to test these patches other than to land them on date, releasetasks, bbcustom etc. but because of the change freeze I can't do that right now.

Comment 9

[Rail Aliiev [:rail]]

Comment on attachment 8736871 [details] [diff] [review]
bug1252003intree-2.patch

Review of attachment 8736871 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/mozharness/configs/releases/dev_updates_firefox_release.py
@@ +30,5 @@
> +            "mar_channel_ids": [
> +                "firefox-mozilla-beta-dev", "firefox-mozilla-release-dev",
> +            ],
> +            "channel_names": ["beta-dev", "beta-dev-localtest", "beta-dev-cdntest"],
> +            "rules_to_update": ["firefox-beta-dev-cdntest", "firefox-beta-dev-localtest"],

beta-dev also needs publish_rules pointing to firefox-beta, so we can publish RCs to beta

::: testing/mozharness/configs/releases/updates_firefox_release.py
@@ +27,5 @@
> +            "mar_channel_ids": [
> +                "firefox-mozilla-beta", "firefox-mozilla-release",
> +            ],
> +            "channel_names": ["beta", "beta-localtest", "beta-cdntest"],
> +            "rules_to_update": ["firefox-beta-cdntest", "firefox-beta-localtest"],

beta also needs publish_rules.

BTW, I'm not sure why this patch creates already existing file, see https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/configs/releases/updates_firefox_release.py

::: testing/mozharness/scripts/release/publish_balrog.py
@@ +22,5 @@
> +# PublishBalrog {{{1
> +
> +
> +class PublishBalrog(MercurialScript, BuildbotMixin,
> +                    MercurialRepoManipulationMixin):

I don't think you need MercurialRepoManipulationMixin here.

@@ +29,5 @@
> +            "action": "store",
> +            "dest": "hg_user",
> +            "type": "string",
> +            "default": "ffxbld <[e-mail]>",
> +            "help": "Specify what user to use to commit to hg.",

No need to commit, this option is redundant.

@@ +35,5 @@
> +        [['--ssh-user', ], {
> +            "action": "store",
> +            "dest": "ssh_user",
> +            "type": "string",
> +            "help": "SSH username with hg.mozilla.org permissions",

The same here

@@ +42,5 @@
> +            "action": "store",
> +            "dest": "ssh_key",
> +            "type": "string",
> +            "help": "Path to SSH key.",
> +        }],

The same here

@@ +84,5 @@
> +
> +        self.config["platforms"] = [p.strip() for p in
> +                                    props["platforms"].split(",")]
> +        self.config["channels"] = [c.strip() for c in
> +                                   props["channels"].split(",")]

some of the props will be irrelevant, see my comment below.

@@ +106,5 @@
> +            repos=self.query_repos())
> +
> +    def submit_to_balrog(self):
> +        for _, channel_config in self.query_channel_configs():
> +            self._submit_to_balrog(channel_config)

I'm not sure if you can use this approach, esp for RCs - you don't want to submit everything at once. Probably we should explicitly pass channel names via --channel.

@@ +114,5 @@
> +        auth = os.path.join(os.getcwd(), self.config['credentials_file'])
> +        cmd = [
> +            self.query_exe("python"),
> +            os.path.join(dirs["abs_tools_dir"],
> +                         "scripts/build-promotion/balrog-release-pusher.py")]

Hmm, I think we cannot reuse the same script, because it deals with actual release blobs, but we only need to deal with rules.

Probably we need to copy https://dxr.mozilla.org/build-central/source/tools/scripts/updates/balrog-release-shipper.py to scripts/build-promotion/ and change it to work without release configs: pass --version --product --build-number and a list of --rules to be passed to ReleasePusher.run()

Comment 10

[Rail Aliiev [:rail]]

Comment on attachment 8736484 [details] [diff] [review]
bug1252003releasetasks.patch

Review of attachment 8736484 [details] [diff] [review]:
-----------------------------------------------------------------

This will require some changes after the script is modified.

Comment 11

[Kim Moir [:kmoir] ET]

Attachment: bug1252003bbustom-3.patch

Comment 12

[Kim Moir [:kmoir] ET]

Attachment: bug1252003tools.patch

Comment 13

[Kim Moir [:kmoir] ET]

Attachment: bug1252003intree-3.patch

Comment 14

[Kim Moir [:kmoir] ET]

Attachment: bug1252003releasetasks-2.patch

Comment 15

[Rail Aliiev [:rail]]

Comment on attachment 8739113 [details] [diff] [review]
bug1252003tools.patch

Review of attachment 8739113 [details] [diff] [review]:
-----------------------------------------------------------------

::: scripts/build-promotion/balrog-release-shipper.py
@@ +21,5 @@
> +    parser.add_option("-u", "--username", dest="username")
> +    parser.add_option("-V", "--version", dest="version")
> +    parser.add_option("-p", "--product", dest="product_name")
> +    parser.add_option("-b", "--build-number", dest="build_number")
> +    parser.add_option("-R", "--rules", dest="rule_ids")

this has to be a list (action="append") and --rule instead of --rules (you pass it multiple times, something like --rule 1 --rule 4 --rule 777), see http://hg.mozilla.org/build/tools/file/tip/scripts/build-promotion/balrog-release-pusher.py#l30

@@ +34,5 @@
> +
> +    for opt in ('api_root', 'credentials_file', 'username', "version", "product_name", "rule_ids",):
> +        if not getattr(options, opt):
> +            print >>sys.stderr, "Required option %s not present" % opt
> +            sys.exit(1)

If you switch to using ArgumentParser instead of OptionParser, you can get rid of this check by specifying required arguments as required=True.

Comment 16

[Rail Aliiev [:rail]]

Comment on attachment 8739135 [details] [diff] [review]
bug1252003intree-3.patch

Review of attachment 8739135 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/mozharness/scripts/release/publish_balrog.py
@@ +22,5 @@
> +# PublishBalrog {{{1
> +
> +
> +class PublishBalrog(MercurialScript, BuildbotMixin,
> +                    MercurialRepoManipulationMixin):

MercurialRepoManipulationMixin is unused, please remove.

@@ +54,5 @@
> +            self.warning("Skipping buildbot properties overrides")
> +            return
> +        # TODO: version and appVersion should come from repo
> +        props = self.buildbot_config["properties"]
> +        for prop in ['product', 'version', 'build_number', 'revision',

'revision' is not required by this script, please remove.

@@ +79,5 @@
> +        super(PublishBalrog, self).pull(
> +            repos=self.query_repos())
> +
> +    def submit_to_balrog(self):
> +        for _, channel_config in self.query_channel_configs():

This won't work for RCs, where we have to separate beta and release submission. Let's explicitly set the channel we want to use via buildbot properties.

For betas and dot releases we would have just one instance of this script called with either beta or release as channel.

For RCs you would need to create 2 tasks:

1) for publishing to beta from graph 1
2) for publishing to release from graph 2

@@ +92,5 @@
> +                         "scripts/build-promotion/balrog-release-shipper.py")]
> +        cmd.extend([
> +            "--api-root", self.config["balrog_api_root"],
> +            "--download-domain", self.config["download_domain"],
> +            "--archive-domain", self.config["archive_domain"],

I don't think that --download-domain and --archive-domain are valid args for your script.

@@ +102,5 @@
> +            "--username", self.config["balrog_username"],
> +            "--verbose",
> +        ])
> +        for c in channel_config["channel_names"]:
> +            cmd.extend(["--channel", c])

the same with --channel. AFAIK you need to pass rules instead. Hmm, or maybe you need to change the script to resolve rules using passed channel name.

@@ +104,5 @@
> +        ])
> +        for c in channel_config["channel_names"]:
> +            cmd.extend(["--channel", c])
> +        for r in channel_config["publish_rules"]:
> +            cmd.extend(["--rules", r])

See the comment above. You need to get the rule ids from the config, using passed channel name.

@@ +106,5 @@
> +            cmd.extend(["--channel", c])
> +        for r in channel_config["publish_rules"]:
> +            cmd.extend(["--rules", r])
> +        for p in self.config["platforms"]:
> +            cmd.extend(["--platform", p])

--platform is not a valid arg for your script.

@@ +110,5 @@
> +            cmd.extend(["--platform", p])
> +        for v, build_number in self.query_matching_partials(channel_config):
> +            partial = "{version}build{build_number}".format(
> +                version=v, build_number=build_number)
> +            cmd.extend(["--partial-update", partial])

the same for --partial-update

@@ +112,5 @@
> +            partial = "{version}build{build_number}".format(
> +                version=v, build_number=build_number)
> +            cmd.extend(["--partial-update", partial])
> +        if channel_config["requires_mirrors"]:
> +            cmd.append("--requires-mirrors")

the same for --requires-mirrors

@@ +114,5 @@
> +            cmd.extend(["--partial-update", partial])
> +        if channel_config["requires_mirrors"]:
> +            cmd.append("--requires-mirrors")
> +        if self.config["balrog_use_dummy_suffix"]:
> +            cmd.append("--dummy")

the same for --dummy

Comment 17

[Rail Aliiev [:rail]]

Comment on attachment 8739210 [details] [diff] [review]
bug1252003releasetasks-2.patch

Review of attachment 8739210 [details] [diff] [review]:
-----------------------------------------------------------------

::: releasetasks/templates/firefox/publish_balrog.tmpl
@@ +2,5 @@
> +
> +-
> +    taskId: "{{ stableSlugId(buildername) }}"
> +    requires:
> +        - "{{ stableSlugId('post_release_human_decision') }}"

This may not work for RC builds:

* in the first graph we need a human decision task to trigger only this task with "beta" set as the channel 
* in the second graph the post release human decision task will unblock this task with "release" set a the channel (plus some other tasks)
* for betas and dot releases it should behave as RC graph 2, but in graph 1

@@ +31,5 @@
> +                script_repo_revision: "{{ mozharness_changeset }}"
> +                release_promotion: true
> +                revision: "{{ revision }}"
> +                balrog_api_root: {{ balrog_api_root }}
> +                channels: {{ release_channels | sort() | join(", ") }}

I'm not quite sure if you can use release_channels here... We need to make sure we don't add "release" for RCs in the first graph here.

@@ +32,5 @@
> +                release_promotion: true
> +                revision: "{{ revision }}"
> +                balrog_api_root: {{ balrog_api_root }}
> +                channels: {{ release_channels | sort() | join(", ") }}
> +                platforms: {{ en_US_config["platforms"] | sort() | join(", ") }}

I don't think you need `platforms`.

Comment 18

[Kim Moir [:kmoir] ET]

Attachment: bug1252003tools-2.patch

Comment 19

[Kim Moir [:kmoir] ET]

Attachment: bug1252003intree-4.patch

Comment 20

[Kim Moir [:kmoir] ET]

Attachment: bug1252003releasetasks-3.patch

This is still not right.  I don't understand the mechanics of how to call the second graph as described in comment 16

https://bugzilla.mozilla.org/show_bug.cgi?id=1252003#c16

Comment 21

[Rail Aliiev [:rail]]

Comment on attachment 8743957 [details] [diff] [review]
bug1252003tools-2.patch

Review of attachment 8743957 [details] [diff] [review]:
-----------------------------------------------------------------

Yay, almost there! Some nits below

::: scripts/build-promotion/balrog-release-shipper.py
@@ +14,5 @@
> +if __name__ == '__main__':
> +
> +    from argparse import ArgumentParser
> +    parser = ArgumentParser()
> +#    parser.add_argument("-p", "--build-properties", dest="build_properties")

would be great to get rid of this when you are ready to land

@@ +23,5 @@
> +    parser.add_argument("-p", "--product", dest="product_name" required=True)
> +    parser.add_argument("-b", "--build-number", dest="build_number", required=True)
> +    parser.add_argument("-R", "--rules", dest="rule_ids", action="append", required=True)
> +    parser.add_argument("-v", "--verbose", dest="verbose", action="store_true")
> +    options, args = parser.parse_args()

argument parser returns only one item, usually used as `args`.

@@ +35,5 @@
> +    credentials = {}
> +    execfile(options.credentials_file, credentials)
> +    auth = (options.username, credentials['balrog_credentials'][options.username])
> +
> +    print options

would be great to get rid of this when you are ready to land

Comment 22

[Kim Moir [:kmoir] ET]

Attachment: bug1252003tools-3.patch

Comment 23

[Rail Aliiev [:rail]]

Comment on attachment 8744993 [details] [diff] [review]
bug1252003releasetasks-3.patch

Review of attachment 8744993 [details] [diff] [review]:
-----------------------------------------------------------------

I think something similar should work. publish_balrog2.tmpl is the same with  publish_balrog.tmpl and can be removed

::: releasetasks/templates/firefox/release_graph.yml.tmpl
@@ +134,4 @@
>          {% endmacro %}
>          {{ finalVerify_tasks()|indent(4) }}
>      {% endif %}
> +    {% if postrelease_publish_balrog_enabled %}

Probably you need to update the tests to enable/disable this feature, otherwise they will fail.

Also need to pass this variable from release-runner

Comment 24

[Rail Aliiev [:rail]]

Comment on attachment 8739112 [details] [diff] [review]
bug1252003bbustom-3.patch

Review of attachment 8739112 [details] [diff] [review]:
-----------------------------------------------------------------

::: process/release.py
@@ +2024,5 @@
>                  'product': product,
>              }
>          })
>  
> +    #publish to balrog human task

Can you remove the comment above when you land? The human decision task is in releasetasks AFAIK.

@@ +2032,5 @@
> +             "-c",  branch_config['updates_config'][product],
> +        ]
> +    }
> +    publish_balrog_buildername = "release-{branch}-{product}_publish_balrog".format(
> +        branch=branch_name, product=product)

Before you land this, can you run the tests either locally or by creating a throw-away PR against https://github.com/mozilla/build-buildbotcustom (see http://coopcoopbware.tumblr.com/post/120049392075/better-releng-patch-contribution-workflow for the details). We may hit the max buildername limit and you may need a patch like this: http://hg.mozilla.org/build/buildbotcustom/rev/7d70b0149b14

Comment 25

[Kim Moir [:kmoir] ET]

Attachment: bug1252003bbustom-4.patch

patch with comment removed

ran tests on github pr and they passed
https://github.com/mozilla/build-buildbotcustom/pull/3

Comment 26

[Rail Aliiev [:rail]]

Comment on attachment 8745475 [details] [diff] [review]
bug1252003tools-3.patch

Review of attachment 8745475 [details] [diff] [review]:
-----------------------------------------------------------------

Ship it!

Comment 27

[Release Engineering SlaveAPI Service]

In production: https://hg.mozilla.org/build/buildbotcustom/rev/efdb41734f8b

Comment 28

[Rail Aliiev [:rail]]

Comment on attachment 8744988 [details] [diff] [review]
bug1252003intree-4.patch

Review of attachment 8744988 [details] [diff] [review]:
-----------------------------------------------------------------

\o/

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e43945cb2c87

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e43945cb2c87

Comment 31

[Kim Moir [:kmoir] ET]

I still need to land some other patches for this bug

Comment 32

[Rail Aliiev [:rail]]

Comment on attachment 8744988 [details] [diff] [review]
bug1252003intree-4.patch

https://hg.mozilla.org/releases/mozilla-beta/rev/4936acc6a668
https://hg.mozilla.org/releases/mozilla-release/rev/19c1f7431744
https://hg.mozilla.org/releases/mozilla-esr45/rev/87a22e023b10

Comment 33

[Rail Aliiev [:rail]]

Attachment: Bug 1252003 - Add "publish to balrog" builder

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

Comment 34

[Rail Aliiev [:rail]]

My boss is not around this week :P, so I have some cycles to poke this one ;)

Comment 35

[Rail Aliiev [:rail]]

I adapted Kim's patch, added some tests and will test it on jamun: https://github.com/mozilla/releasetasks/compare/master...rail:push_to_balrog?expand=1

Comment 36

[Jordan Lund (:jlund)]

Comment on attachment 8767717 [details]
Bug 1252003 - Add "publish to balrog" builder

https://reviewboard.mozilla.org/r/62130/#review58920

looks good assuming you define release_config["publish_to_balrog_channels"] in releasetasks. Speaking of which, do you have the releasetasks patch of this yet? `publish_to_balrog_channels` and this the mozharness script: https://hg.mozilla.org/releases/mozilla-beta/rev/4936acc6a668 suggest like we have or will have multiple channels to publish in one task (one mozharness script call). I don't think that's true now. Do you forsee the need for this added flexibility that's coded in?

::: buildfarm/release/releasetasks_graph_gen.py:103
(Diff revision 1)
>              branch=release_config["branch"],
>              revision=release_config["mozilla_revision"],
>              platforms=release_config['platforms']
>          ),
>          "extra_balrog_submitter_params": release_config['extra_balrog_submitter_params']
> +        "publish_to_balrog_channels": release_config["publish_to_balrog_channels"],

in case it's forgotten, this will assume the key is present in each: https://github.com/mozilla/releasetasks/tree/master/releasetasks/release_configs

Comment 37

[Rail Aliiev [:rail]]

(In reply to Jordan Lund (:jlund) from comment #36)
> Comment on attachment 8767717 [details]
> Bug 1252003 - Add "publish to balrog" builder
> 
> https://reviewboard.mozilla.org/r/62130/#review58920
> 
> looks good assuming you define release_config["publish_to_balrog_channels"]
> in releasetasks. Speaking of which, do you have the releasetasks patch of
> this yet?

Yes, testing https://github.com/mozilla/releasetasks/compare/master...rail:push_to_balrog2?expand=1 now.

> `publish_to_balrog_channels` and this the mozharness script:
> https://hg.mozilla.org/releases/mozilla-beta/rev/4936acc6a668 suggest like
> we have or will have multiple channels to publish in one task (one
> mozharness script call). I don't think that's true now. Do you forsee the
> need for this added flexibility that's coded in?

Yeah, we may want to simplify this in the future.

> 
> ::: buildfarm/release/releasetasks_graph_gen.py:103
> (Diff revision 1)
> >              branch=release_config["branch"],
> >              revision=release_config["mozilla_revision"],
> >              platforms=release_config['platforms']
> >          ),
> >          "extra_balrog_submitter_params": release_config['extra_balrog_submitter_params']
> > +        "publish_to_balrog_channels": release_config["publish_to_balrog_channels"],
> 
> in case it's forgotten, this will assume the key is present in each:
> https://github.com/mozilla/releasetasks/tree/master/releasetasks/
> release_configs

I'm not going to land this one until the releasetasks patch is ready. And yes, the config is there.

Comment 38

[Rail Aliiev [:rail]]

Comment on attachment 8767717 [details]
Bug 1252003 - Add "publish to balrog" builder

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

Comment 39

[Rail Aliiev [:rail]]

Comment on attachment 8767717 [details]
Bug 1252003 - Add "publish to balrog" builder

syntax fix + read mozharness_changeset from the task (PR incoming)

Comment 40

[Rail Aliiev [:rail]]

Attachment: PR for releasetasks

Comments are in the PR

Comment 41

[Rail Aliiev [:rail]]

Comment on attachment 8767717 [details]
Bug 1252003 - Add "publish to balrog" builder

https://hg.mozilla.org/build/tools/rev/0621d1e393dd

Comment 42

[Rail Aliiev [:rail]]

Deployed! Now to update the docs.

Comment 43

[Rail Aliiev [:rail]]

Attachment: update the docs

Comment 44

[Jordan Lund (:jlund)]

Comment on attachment 8768091 [details] [review]
update the docs

/me <3s documentation as a PR :)

Comment 45

[Kim Moir [:kmoir] ET]

I think this can be closed.  Thanks Rail for finishing it!