1398793 - Migrate update verify to TC

Status: RESOLVED FIXED

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

Comment 1

[[e-mail] (:bhearsum)]

I made a quick stab at trying to get one of these running by hand in Taskcluster. This is where I got: https://tools.taskcluster.net/groups/J7zVRl26RnGPHsXJe1C9XQ/tasks/J7zVRl26RnGPHsXJe1C9XQ/runs/0/artifacts

Most notably, the payload is:
{
  "maxRunTime": 3600,
  "command": [
    "c:\\\\mozilla-build\\\\msys\\\\bin\\\\bash.exe -c \"c:\\\\Program\\ Files\\\\Mercurial\\\\hg.exe clone https://hg.mozilla.org/build/tools && virtualenv foo && foo\\\\Scripts\\\\pip.exe install pypiwin32 &&         foo\\\\Scripts\\\\python.exe           tools\\\\scripts\\\\release\\\\updates\\\\chunked-verify.py --chunks 12 --this-chunk      1      --verify-config beta-firefox-win32.cfg\""
  ]
}

And it ends up failing with:
Traceback (most recent call last):
  File "z:/task_1508858692/tools/release/updates/../../buildfarm/utils/retry.py", line 24, in <module>
    from win32_util import kill, which
  File "z:\task_1508858692\tools\buildfarm\utils\win32_util.py", line 1, in <module>
    import win32api
ImportError: No module named win32api


My suspicion is that the python that we call the wget-wrapped-with-retry (https://github.com/mozilla/build-tools/blob/17948061c705f3edc27a546f4db29696e65e1440/release/updates/verify.sh#L24) ends up not using the python in the virtualenv, so win32api is not available.

Comment 2

[[e-mail] (:bhearsum)]

Something else we've talked about in the past is running the Windows & OS X update verify jobs on Linux (using a Linux updater binary instead of the updater binary out of the "from" builds). I was able to make this work pretty trivially by hacking the update verify scripts: https://gist.github.com/mozbhearsum/bb00831bfe571d560b55362d98ab1f53

We'd have to weight the pros/cons of this a bit more, but I suspect it's a much quicker path forward, and probably scales better/more cheaply (the OS X pool in Taskcluster appears to be at capacity quite a bit).

Comment 3

[[e-mail] (:bhearsum)]

I've e-mailed and spoken with some folks about the cross platform idea, and it looks like we'll be proceeding with that. A couple of random notes:
* Even when doing cross platform update verify, we want to make sure we use the same "from" version to make sure that we don't break when updater changes happen.
* We may want to use the same locale as the target platform too. This will slow down tests (because we'll need to download a different updater for every test), but it will keep us as close as possible to running the same updater that MARs will be applied with in the wild.

Things I still need to do:
* Finish making the necessary changes to update verify (almost done). I want to make sure that the new update verify code is fully compatible with the old configs, to make it possible to land this independently of config/task graph changes.
* Modify the update verify bumper script to start adding the new "updater_package" arg to update verify configs. I don't think we need to update the existing configs, because we regenerate them from scratch each time, so the bumper will take care of that.
* Modify releasetasks to use TC update verify for all platforms
* Update the docker image that update verify runs on to include '7z' and 'unzip', which are needed to unpack Mac and Windows builds.

Comment 4

[[e-mail] (:bhearsum)]

(In reply to Ben Hearsum (:bhearsum) from comment #3)
> * Update the docker image that update verify runs on to include '7z' and
> 'unzip', which are needed to unpack Mac and Windows builds.

I started looking at this and it looks like we need to upgrade the OS the current image uses to pick up a newer version of 7z. Because of that, we may as well do bug 1391983 at the same time. I'll continue the work on this in that bug.

Comment 5

[[e-mail] (:bhearsum)]

Attachment: implement (but disable) cross platform update verify

(flagging you both since you volunteered - feel free to fight over it, though!)

This implements all the necessary support in update verify to allow for cross platform tests, as well as some related code in the config bumper. Here are the highlights:
* Look for new "updater_package" variable in update verify configs, and use that instead of the "from" when applying updates. If it's not present, use "from".
* Add support for unpacking OS X builds on platforms that aren't Darwin (yay 7z!)
* De-tangle host & target assumptions in check_updates.sh - things related to how to run the updater and find diffs are host-dependent, while things about the "source" dir are target-dependent.
* Remove support for old updater paths - I doubt we need this anymore, and it simplifies the logic a bit.
* Set updater_package when creating update verify configs. If not passed to create-update-verify-config.py, we set it to the from package.

I tested this "by hand" in cross platform mode by editing some recent release tasks and re-running:
https://tools.taskcluster.net/groups/Xoxq0DhDS52QaeuQGPBbrQ/tasks/Xoxq0DhDS52QaeuQGPBbrQ/details
https://tools.taskcluster.net/groups/ThtN0-ZjS0e84Nnhr_64tg/tasks/ThtN0-ZjS0e84Nnhr_64tg/details
https://tools.taskcluster.net/groups/MxaXG7oeTZmexBhmd0C6Mg/tasks/MxaXG7oeTZmexBhmd0C6Mg/details
https://tools.taskcluster.net/groups/EiHFCHVjQESkLpw530eR-w/tasks/EiHFCHVjQESkLpw530eR-w/details
https://tools.taskcluster.net/groups/QBwZos8iSDCD4uKnloIyKg/tasks/QBwZos8iSDCD4uKnloIyKg/details

As well as in a staging release with cross platform mode disabled: https://tools.taskcluster.net/groups/Or3X9v9vS3yR0o4UzUflNA

It's also worth noting that when running in cross platform mode, we end up download an extra binary per test. It looks like running Windows update verify on Linux is ~3h faster, and runinng Mac on Linux is ~1h faster - so should still be a net win. We could look at further optimizing this later by changing chunking to group by locale & version (even across platforms)...but it may not even be worth it.

Comment 6

[Rail Aliiev [:rail]]

Comment on attachment 8923987 [details] [diff] [review]
implement (but disable) cross platform update verify

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

::: release/common/unpack.sh
@@ +29,4 @@
>                  echo "installing $pkg_file"
>                  ../common/unpack-diskimage.sh "$pkg_file" mnt $dir_name
> +            else
> +                7z x ../"$pkg_file" > /dev/null

Yay!!! Die stupid mount!

@@ +29,5 @@
>                  echo "installing $pkg_file"
>                  ../common/unpack-diskimage.sh "$pkg_file" mnt $dir_name
> +            else
> +                7z x ../"$pkg_file" > /dev/null
> +                if [ `ls | wc -l` -ne 1 ]

maybe use `ls -1` just to be safe? iirc ls won't print everything on a single line if you pipe it, but who knows...

::: release/updates/verify.sh
@@ +185,5 @@
> +        cached_download "${updater_package_filename}" "${updater_package_url}"
> +        unpack_build "$updater_platform" updater "$updater_package_filename" "$locale"
> +
> +        # Even on Windows, we want Unix-style paths for the updater, because of MSYS.
> +        cwd=$(\ls -d $PWD/updater/$platform_dirname)

Why do you need a backslash here?

Comment 7

[[e-mail] (:bhearsum)]

Attachment: crappy in tree chunked update verify

This gets us a reasonable looking set of tasks for linux & linux64 update verify. It hasn't been tested, but I started running into some issues with get_release_config that I think we should discuss and agree on before I go any further. Also, you should probably just ignore the buildbot-update-verify part for now - I haven't done much of anything with it yet.

You'll notice that I've taken away most of the conditional parts of get_release_config, and modified most consumers to only apply the parts they care about.

I took away the conditional parts because things like "version" (and the newly added "build_tools_repo_path") are useful to many release tasks, and it seemed very burdensome to need to dig into scriptworker.py and update a list of hardcodes every time we add a new task. On a more philosophical level, it also seemed odd to me that get_release_config changes its return value based on various things -- that essentially means that we could have a different release config per task (or maybe just per kind). I fully admit that I'm not an expert here, and there might be good reason for doing this.

Because of this, I had to update other consumers of get_release_config to not always copy in all values. Probably it doesn't hurt in most cases to have extra values, but I was trying to minimize change vs. the current state of maple. Most notably, the buildbot transform has a horrible hack in it to avoid copying non-simple values (because trying to do throws a validation error).

Eventually we're going to need everything from the releasetask configs and a bunch of stuff from the buildbot branch configs - so it would be good to figure out a strategy now, to avoid needing to refactor later on.

Comment 8

[[e-mail] (:bhearsum)]

(In reply to Rail Aliiev [:rail] ⌚️ET from comment #6)
> @@ +29,5 @@
> >                  echo "installing $pkg_file"
> >                  ../common/unpack-diskimage.sh "$pkg_file" mnt $dir_name
> > +            else
> > +                7z x ../"$pkg_file" > /dev/null
> > +                if [ `ls | wc -l` -ne 1 ]
> 
> maybe use `ls -1` just to be safe? iirc ls won't print everything on a
> single line if you pipe it, but who knows...

Probably a good idea, will do.

> ::: release/updates/verify.sh
> @@ +185,5 @@
> > +        cached_download "${updater_package_filename}" "${updater_package_url}"
> > +        unpack_build "$updater_platform" updater "$updater_package_filename" "$locale"
> > +
> > +        # Even on Windows, we want Unix-style paths for the updater, because of MSYS.
> > +        cwd=$(\ls -d $PWD/updater/$platform_dirname)
> 
> Why do you need a backslash here?

I don't know, this was just code I re-arranged :). It probably serves no purpose - I'll remove it.

Comment 9

[Aki Sasaki (not active)]

(In reply to Ben Hearsum (:bhearsum) from comment #8)
> (In reply to Rail Aliiev [:rail] ⌚️ET from comment #6)
> > > +        # Even on Windows, we want Unix-style paths for the updater, because of MSYS.
> > > +        cwd=$(\ls -d $PWD/updater/$platform_dirname)
> > 
> > Why do you need a backslash here?
> 
> I don't know, this was just code I re-arranged :). It probably serves no
> purpose - I'll remove it.

I believe \ls forces using the "real" ls, rather than an alias, e.g.

akimoz: /src/gecko/mozilla-unified/taskcluster (559e44e)(default) [09:46:45]
10832$ ls
./                 ci/                mach_commands.py   scripts/
../                docker/            mach_commands.pyc  taskgraph/
.yamllint          docs/              moz.build
akimoz: /src/gecko/mozilla-unified/taskcluster (559e44e)(default) [09:46:46]
10833$ \ls
ci                      docs                    mach_commands.pyc       scripts
docker                  mach_commands.py        moz.build               taskgraph

Removing it might not hurt, especially if the corresponding user doesn't have any aliases set.

Comment 10

[Aki Sasaki (not active)]

Comment on attachment 8924564 [details] [diff] [review]
crappy in tree chunked update verify

>diff --git a/taskcluster/ci/release-buildbot-update-verify/kind.yml b/taskcluster/ci/release-buildbot-update-verify/kind.yml
>new file mode 100644
>--- /dev/null
>+++ b/taskcluster/ci/release-buildbot-update-verify/kind.yml
<snip>
>+# TODO: how to make this support multiple platforms?

I think you can use the job-defaults: to specify the shared key/value pairs, and only override the product-platform-specific stuff in jobs:, kind of like you did in release-update-verify.

>diff --git a/taskcluster/ci/release-update-verify/kind.yml b/taskcluster/ci/release-update-verify/kind.yml
>new file mode 100644
>--- /dev/null
>+++ b/taskcluster/ci/release-update-verify/kind.yml
>@@ -0,0 +1,37 @@
>+# This Source Code Form is subject to the terms of the Mozilla Public
>+# License, v. 2.0. If a copy of the MPL was not distributed with this
>+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
>+
>+loader: taskgraph.loader.transform:loader
>+
>+# TODO: we probably hove some of these?
>+#kind-dependencies:
>+
>+transforms:
>+    - taskgraph.transforms.update_verify:transforms
>+    - taskgraph.transforms.task:transforms
>+
>+job-defaults:
>+    name: update-verify
>+    run-on-projects: [] # to make sure this never runs as part of CI
>+    worker-type: aws-provisioner-v1/gecko-{level}-b-linux
>+    worker:
>+        implementation: docker-worker
>+        os: linux
>+        docker-image:
>+            in-tree: "update-verify"
>+        max-run-time: 7200
>+    extra:
>+        product: firefox
>+        chunks: 12
>+
>+jobs:
>+    firefox-linux64:
>+        description: linux64 update verify
>+        attributes:
>+            build_platform: linux64
>+
>+    firefox-linux:
>+        description: linux update verify
>+        attributes:
>+            build_platform: linux
>diff --git a/taskcluster/taskgraph/target_tasks.py b/taskcluster/taskgraph/target_tasks.py
>--- a/taskcluster/taskgraph/target_tasks.py
>+++ b/taskcluster/taskgraph/target_tasks.py
>@@ -316,6 +316,7 @@ def target_tasks_mozilla_beta_desktop_pr
>     allow_kinds = [
>         'build', 'build-signing', 'repackage', 'repackage-signing',
>         'nightly-l10n', 'nightly-l10n-signing', 'repackage-l10n',
>+        'release-update-verify', 'release-buildbot-update-verify',
>     ]
>
>     def filter(task):
>@@ -334,6 +335,9 @@ def target_tasks_mozilla_beta_desktop_pr
>         if task.label in beta_tasks:
>             return True
>
>+        # TODO: do we need to filter on product, too? is this target tasks method only for firefox? if so, we should rename it

Rename to which? I agree that the current names are inconsistent and could use improvement.
And yes, we'll need to filter by product. We'll be adding 'devedition' to the release-update-verify kind, and we don't want to run them in a firefox graph.

>@@ -357,6 +361,8 @@ def target_tasks_publish_firefox(full_ta
>     )
>
>     def filter(task):
>+        if task.kind in ('release-update-verify', 'release-buildbot-update-verify'):
>+            return False

Hm, we can do this, or we can add to the previous_graph_kinds in taskcluster/taskgraph/actions/release_promotion.py

>diff --git a/taskcluster/taskgraph/transforms/job/buildbot.py b/taskcluster/taskgraph/transforms/job/buildbot.py
>--- a/taskcluster/taskgraph/transforms/job/buildbot.py
>+++ b/taskcluster/taskgraph/transforms/job/buildbot.py
>@@ -37,7 +37,12 @@ buildbot_run_schema = Schema({
>
> def bb_release_worker(config, worker, run):
>     # props
>-    release_props = get_release_config(config, force=True)
>+    release_props = {}
>+    release_config = get_release_config(config, force=True)
>+    for key, value in release_config.iteritems():
>+        # this is shit
>+        if not isinstance(value, (dict, list, tuple)):
>+            release_props[key] = value

Hm. Would release_props[key] = str(value) work?
`get_release_config` is now doing way more than we originally intended it to, and we probably need a way to figure out which key/value pairs go where... multiple functions may work, like `get_buildbot_release_config`? Known key allowlists per usage type may work.

I ran taskgraph-gen.py vs tip-of-maple and your patch, and diffed. We're adding a ton of stuff to non-update-verify tasks, which is probably fine for buildbot tasks, but may break beetmover scriptworker payload jsonschema checks.

Comment 11

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 8923987 [details] [diff] [review]
implement (but disable) cross platform update verify

Looks pretty good for me. I did spot this error finding the precomplete in the mac run [1], just for complete updates:

+ /tools/release/updates/updater/firefox/updater /tools/release/updates/update /tools/release/updates/source/Firefox.app /tools/release/updates/source/Firefox.app 0
Unable to init server: Could not connect: Connection refused
+ set +x
PATCH DIRECTORY /tools/release/updates/update
INSTALLATION DIRECTORY /tools/release/updates/source/Firefox.app
WORKING DIRECTORY /tools/release/updates/source/Firefox.app
UPDATE TYPE complete
GetManifestContents: error opening manifest file: /tools/release/updates/source/Firefox.app/precomplete
AddPreCompleteActions: error getting contents of precomplete manifest
PREPARE ADD Contents/_CodeSignature/CodeResources

It still succeeds, but if a file is deprecated it may not be removed properly in the test. The precomplete file seems to live in Contents/Resources/. Would you mind chasing this down ?

Not worried about "Unable to init server" message from trying to show some gnome ui.

[1] https://public-artifacts.taskcluster.net/ThtN0-ZjS0e84Nnhr_64tg/0/public/logs/live_backing.log

Comment 12

[Rail Aliiev [:rail]]

Comment on attachment 8924564 [details] [diff] [review]
crappy in tree chunked update verify

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

::: taskcluster/ci/release-buildbot-update-verify/kind.yml
@@ +22,5 @@
> +      run:
> +          using: buildbot
> +          product: firefox
> +          buildername: release-{branch}_firefox_win64_update_verify
> +          release-promotion: true # needed?

This is used only by one of the bouncer tasks. Feel free to get rid of it.

@@ +26,5 @@
> +          release-promotion: true # needed?
> +          properties:
> +              platform: win64
> +#      notifications:
> +#         completed:

Probably we will be disabling the "completed" notifications, feel free to remove it.

@@ +40,5 @@
> +#                     #- "{task[tags][createdForUser]}"
> +#                  default:
> +#                     - "release-drivers"
> +      extra:
> +          product: firefox

No chunking?

::: taskcluster/docker/update-verify/Dockerfile
@@ +6,5 @@
> +    # p7zip-full is for extracting Windows and OS X packages
> +    # Mercurial and Git are for cloning required repositories
> +    # wget is for downloading update.xml, installers, and MARs
> +    # libgtk-3-0 is required to run the Firefox updater
> +    && apt-get -q --yes install p7zip-full git mercurial wget libgtk-3-0 \

Do we need git here?

::: taskcluster/taskgraph/transforms/job/buildbot.py
@@ +39,5 @@
>      # props
> +    release_props = {}
> +    release_config = get_release_config(config, force=True)
> +    for key, value in release_config.iteritems():
> +        # this is shit

O_O :D

@@ +41,5 @@
> +    release_config = get_release_config(config, force=True)
> +    for key, value in release_config.iteritems():
> +        # this is shit
> +        if not isinstance(value, (dict, list, tuple)):
> +            release_props[key] = value

What is the purpose of this? to avoid copying nested structures?

::: taskcluster/taskgraph/transforms/task.py
@@ +974,5 @@
>          task_def['payload']['locale'] = worker['locale']
>      if release_config:
> +        task_def['payload']['version'] = release_config['version']
> +        task_def['payload']['build_number'] = release_config['build_number']
> +        task_def['payload']['next_version'] = release_config['next_version']

I'm not sure why this is required, but OK. :)

Comment 13

[[e-mail] (:bhearsum)]

(In reply to Aki Sasaki [:aki] from comment #10)
> >+        # TODO: do we need to filter on product, too? is this target tasks method only for firefox? if so, we should rename it
> 
> Rename to which? I agree that the current names are inconsistent and could
> use improvement.
> And yes, we'll need to filter by product. We'll be adding 'devedition' to
> the release-update-verify kind, and we don't want to run them in a firefox
> graph.

I think I understand now - we put "desktop" instead of  "firefox" here because it's used by multiple desktop products (firefox, devedition, maybe thunderbird someday).

> >diff --git a/taskcluster/taskgraph/transforms/job/buildbot.py b/taskcluster/taskgraph/transforms/job/buildbot.py
> >--- a/taskcluster/taskgraph/transforms/job/buildbot.py
> >+++ b/taskcluster/taskgraph/transforms/job/buildbot.py
> >@@ -37,7 +37,12 @@ buildbot_run_schema = Schema({
> >
> > def bb_release_worker(config, worker, run):
> >     # props
> >-    release_props = get_release_config(config, force=True)
> >+    release_props = {}
> >+    release_config = get_release_config(config, force=True)
> >+    for key, value in release_config.iteritems():
> >+        # this is shit
> >+        if not isinstance(value, (dict, list, tuple)):
> >+            release_props[key] = value
> 
> Hm. Would release_props[key] = str(value) work?
> `get_release_config` is now doing way more than we originally intended it
> to, and we probably need a way to figure out which key/value pairs go
> where... multiple functions may work, like `get_buildbot_release_config`?
> Known key allowlists per usage type may work.

In my mind, having One True release config seems ideal, and tasks should be smart enough to only grab the parts of them that they want. This is a bit more complicated for Buildbot ones, because of this generic buildbot bridge code, but maybe we can change that to have an additional transform for specific buildbot tasks (and avoid the need for this ugly hack).

Comment 14

[[e-mail] (:bhearsum)]

(In reply to Nick Thomas [:nthomas] (UTC+13) from comment #11)
> Comment on attachment 8923987 [details] [diff] [review]
> implement (but disable) cross platform update verify
> 
> Looks pretty good for me. I did spot this error finding the precomplete in
> the mac run [1], just for complete updates:
> 
> + /tools/release/updates/updater/firefox/updater
> /tools/release/updates/update /tools/release/updates/source/Firefox.app
> /tools/release/updates/source/Firefox.app 0
> Unable to init server: Could not connect: Connection refused
> + set +x
> PATCH DIRECTORY /tools/release/updates/update
> INSTALLATION DIRECTORY /tools/release/updates/source/Firefox.app
> WORKING DIRECTORY /tools/release/updates/source/Firefox.app
> UPDATE TYPE complete
> GetManifestContents: error opening manifest file:
> /tools/release/updates/source/Firefox.app/precomplete
> AddPreCompleteActions: error getting contents of precomplete manifest
> PREPARE ADD Contents/_CodeSignature/CodeResources
> 
> It still succeeds, but if a file is deprecated it may not be removed
> properly in the test. The precomplete file seems to live in
> Contents/Resources/. Would you mind chasing this down ?

Will do, thanks for catching it.

Comment 15

[[e-mail] (:bhearsum)]

(In reply to Rail Aliiev [:rail] ⌚️ET from comment #12)
> @@ +40,5 @@
> > +#                     #- "{task[tags][createdForUser]}"
> > +#                  default:
> > +#                     - "release-drivers"
> > +      extra:
> > +          product: firefox
> 
> No chunking?

As I mentioned, I haven't done any detailed work on buildbot-update-verify yet -- this will be implemented soon.

> ::: taskcluster/docker/update-verify/Dockerfile
> @@ +6,5 @@
> > +    # p7zip-full is for extracting Windows and OS X packages
> > +    # Mercurial and Git are for cloning required repositories
> > +    # wget is for downloading update.xml, installers, and MARs
> > +    # libgtk-3-0 is required to run the Firefox updater
> > +    && apt-get -q --yes install p7zip-full git mercurial wget libgtk-3-0 \
> 
> Do we need git here?

Technically no, but it was useful when testing :). I can get rid of it.

> @@ +41,5 @@
> > +    release_config = get_release_config(config, force=True)
> > +    for key, value in release_config.iteritems():
> > +        # this is shit
> > +        if not isinstance(value, (dict, list, tuple)):
> > +            release_props[key] = value
> 
> What is the purpose of this? to avoid copying nested structures?

Yep. Trying to copy nested structures throws a validation error. It's a horrible hack.

> ::: taskcluster/taskgraph/transforms/task.py
> @@ +974,5 @@
> >          task_def['payload']['locale'] = worker['locale']
> >      if release_config:
> > +        task_def['payload']['version'] = release_config['version']
> > +        task_def['payload']['build_number'] = release_config['build_number']
> > +        task_def['payload']['next_version'] = release_config['next_version']
> 
> I'm not sure why this is required, but OK. :)

I'm not sure where this diff is from, but the raw one is:
diff --git a/taskcluster/taskgraph/transforms/task.py b/taskcluster/taskgraph/transforms/task.py
--- a/taskcluster/taskgraph/transforms/task.py
+++ b/taskcluster/taskgraph/transforms/task.py
@@ -973,7 +973,9 @@ def build_beetmover_payload(config, task
> -        task_def['payload'].update(release_config)
> +        task_def['payload']['version'] = release_config['version']
> +        task_def['payload']['build_number'] = release_config['build_number']
> +        task_def['payload']['next_version'] = release_config['next_version']

I don't know if all of these parameters are necessary, but this task was getting them before I'm pretty sure...

Comment 16

[Aki Sasaki (not active)]

(In reply to Ben Hearsum (:bhearsum) from comment #13)
> (In reply to Aki Sasaki [:aki] from comment #10)
> > >+        # TODO: do we need to filter on product, too? is this target tasks method only for firefox? if so, we should rename it
> > 
> > Rename to which? I agree that the current names are inconsistent and could
> > use improvement.
> > And yes, we'll need to filter by product. We'll be adding 'devedition' to
> > the release-update-verify kind, and we don't want to run them in a firefox
> > graph.
> 
> I think I understand now - we put "desktop" instead of  "firefox" here
> because it's used by multiple desktop products (firefox, devedition, maybe
> thunderbird someday).

I don't think so. We probably should rename this to "firefox".
The reason we have to filter by product is because we all the tasks in the universe get sent to the target_tasks_method, and we have to filter everything out that we don't want. Devedition tasks will be somewhere else in the graph, but the kind will match, so the if block will include devedition tasks once they exist.

Comment 17

[Aki Sasaki (not active)]

Looks like this broke flake8 on maple: https://treeherder.mozilla.org/#/jobs?repo=maple&filter-searchStr=flake
You can check this before landing:

    ./mach lint -l flake8 taskcluster/

If you touch yaml, you can also `./mach lint -l yaml taskcluster/`

The `taskcluster/` is optional, but prevents you from linting the whole tree.

Comment 18

[Aki Sasaki (not active)]

(In reply to Aki Sasaki [:aki] from comment #17)
> Looks like this broke flake8 on maple:
> https://treeherder.mozilla.org/#/jobs?repo=maple&filter-searchStr=flake
> You can check this before landing:
> 
>     ./mach lint -l flake8 taskcluster/
> 
> If you touch yaml, you can also `./mach lint -l yaml taskcluster/`
> 
> The `taskcluster/` is optional, but prevents you from linting the whole tree.

Per Callek, 18:00 <Callek> Aki: there is also `./mach lint -l Foo -w` to lint touched things

The above works for any changes that haven't been committed yet.

Comment 20

[[e-mail] (:bhearsum)]

Because of how I'm organizing the work, I'm changing the scope of this bug a bit. My first focus is going to be on moving all update verify tasks into the tree, while still leaving Windows and Mac tests running through Buildbot. Once that's done, I'll look at moving Windows and Mac off of Buildbot.

Comment 21

[[e-mail] (:bhearsum)]

Attachment: allow channel to be specified for buildbot update verify

IIRC, we said that it would be good for buildbot update verify to use cdntest like Linux update verify does - this should make that possible. You can see it getting overridden (albeit with the wrong channel name...) in tasks like https://tools.taskcluster.net/groups/QhNYiURETkqk6kEanP5K8A/tasks/KCpxkR4TRPGfV3IndgISVQ/runs/0/artifacts

Comment 22

[[e-mail] (:bhearsum)]

Attachment: much improved in tree update verify

This patch should be 90% or so of what we need for in tree update verify. It implements both buildbot and TC update verify, chunking, and now uses by-* to deal with platform/product/branch differences. My latest run on maple is https://tools.taskcluster.net/groups/QhNYiURETkqk6kEanP5K8A, and I've been comparing it to a recent production Beta (https://tools.taskcluster.net/groups/SO7pdJTfTQ2sDyJdm91AVg). Notable differences:
* Channel is wrong (this is fixed in the patch already, just didn't have time for another run with it)
* Mac and Windows specify their channel, which I believe is desireable, as we already do it for Linux.
* A bunch of stuff in extra/env/properties. It appears to me that all of these differences make no difference at all.

Everything is failing in the runs because we don't have updates and other necessary upstream tasks ready in-tree yet. These tests _should_ pass once that's fixed, but it's possible we'll uncover new bugs still.

Comment 23

[[e-mail] (:bhearsum)]

If anyone is picking up the taskgraph parts of this while I'm away, the easiest way to find the isolated code for this in my git branch: https://github.com/mozbhearsum/gecko-dev/compare/update-verify-in-tree?expand=1 -- it's up-to-date (as of now) against maple, and contains only the update verify work.

Comment 24

[Aki Sasaki (not active)]

Comment on attachment 8927464 [details] [diff] [review]
much improved in tree update verify

Thanks!

> diff --git a/taskcluster/ci/release-buildbot-update-verify/kind.yml b/taskcluster/ci/release-buildbot-update-verify/kind.yml
> new file mode 100644
> index 000000000000..7c475c3c545e
> --- /dev/null
> +++ b/taskcluster/ci/release-buildbot-update-verify/kind.yml
> @@ -0,0 +1,107 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +loader: taskgraph.loader.transform:loader
> +
> +# TODO: we probably hove some of these?
> +#kind-dependencies:

Looks like this will be dummy tasks that depend on the various balrog submission tasks, so you can't populate this yet.

> +transforms:
> +    - taskgraph.transforms.buildbot_update_verify:transforms
> +    - taskgraph.transforms.job:transforms
> +    - taskgraph.transforms.task:transforms
> +
> +job-defaults:
> +    name: update-verify
> +    run-on-projects: [] # to make sure this never runs in CI
> +    worker-type: buildbot-bridge/buildbot-bridge
> +    run:
> +        using: buildbot
> +        properties:
> +            NO_BBCONFIG: "1"

I moved this into `job-defaults.worker.properties` .

> +jobs:
> +    # TODO: need beta channel tests when project = mozilla-release
> +    firefox-win64:

I'm wondering if we can specify `firefox` as the key here, and build the config from a platform list of `['win64', 'win32', 'macosx64']` in a generator loop.
Not a requirement. If it reduces the yaml config without adding too much complexity, it would be a good change.

> diff --git a/taskcluster/ci/release-update-verify/kind.yml b/taskcluster/ci/release-update-verify/kind.yml
> new file mode 100644
> index 000000000000..3fb18e3f8c07
> --- /dev/null
> +++ b/taskcluster/ci/release-update-verify/kind.yml
> @@ -0,0 +1,93 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +loader: taskgraph.loader.transform:loader
> +
> +# TODO: we probably hove some of these?
> +#kind-dependencies:

Same comment here for the deps.

> +jobs:
> +    firefox-linux64:

And same comment here for the platform list task generator loop. If we only tackle one of these, this is probably the one we want to spend time on, since it's not on buildbot? (is that right?)

> diff --git a/taskcluster/taskgraph/transforms/buildbot_update_verify.py b/taskcluster/taskgraph/transforms/buildbot_update_verify.py
> new file mode 100644
> index 000000000000..ff97707f54a6
<snip>
> +            if not chunked["run"].get("properties"):
> +                chunked["run"]["properties"] = {}
> +            chunked["run"]["properties"]["THIS_CHUNK"] = str(this_chunk)
> +            chunked["run"]["properties"]["TOTAL_CHUNKS"] = str(total_chunks)
> +            for thing in ("CHANNEL", "VERIFY_CONFIG"):
> +                thing = "run.properties.{}".format(thing)
> +                resolve_keyed_by(chunked, thing, thing, **config.params)

run.properties has moved to worker.properties .

>  def build_task(config, tasks):
>      for task in tasks:
>          level = str(config.params['level'])
>          worker_type = task['worker-type'].format(level=level)
>          provisioner_id, worker_type = worker_type.split('/', 1)
> +        branch = config.params['project']
>
>          routes = task.get('routes', [])
> -        scopes = [s.format(level=level) for s in task.get('scopes', [])]
> +        scopes = [s.format(level=level, branch=branch) for s in task.get('scopes', [])]

Should we go with `project` instead of `branch` since that's the terminology here?

Comment 25

[[e-mail] (:bhearsum)]

(In reply to Aki Sasaki [:aki] (back nov27) from comment #24)
> Looks like this will be dummy tasks that depend on the various balrog
> submission tasks, so you can't populate this yet.
> 

Yeah, as well as the "updates" builder.

> > +transforms:
> > +    - taskgraph.transforms.buildbot_update_verify:transforms
> > +    - taskgraph.transforms.job:transforms
> > +    - taskgraph.transforms.task:transforms
> > +
> > +job-defaults:
> > +    name: update-verify
> > +    run-on-projects: [] # to make sure this never runs in CI
> > +    worker-type: buildbot-bridge/buildbot-bridge
> > +    run:
> > +        using: buildbot
> > +        properties:
> > +            NO_BBCONFIG: "1"
> 
> I moved this into `job-defaults.worker.properties` .

I'm not sure I understand why, but okay!

> > +jobs:
> > +    # TODO: need beta channel tests when project = mozilla-release
> > +    firefox-win64:
> 
> I'm wondering if we can specify `firefox` as the key here, and build the
> config from a platform list of `['win64', 'win32', 'macosx64']` in a
> generator loop.
> Not a requirement. If it reduces the yaml config without adding too much
> complexity, it would be a good change.

If the platform list is still in kind.yml, this seems fine to me. I feel like putting information such as "platforms this runs for" at a high level keeps things a bit more readable. Feel free to overrule me though, I'm still pretty new to how we do things here. 

> >  def build_task(config, tasks):
> >      for task in tasks:
> >          level = str(config.params['level'])
> >          worker_type = task['worker-type'].format(level=level)
> >          provisioner_id, worker_type = worker_type.split('/', 1)
> > +        branch = config.params['project']
> >
> >          routes = task.get('routes', [])
> > -        scopes = [s.format(level=level) for s in task.get('scopes', [])]
> > +        scopes = [s.format(level=level, branch=branch) for s in task.get('scopes', [])]
> 
> Should we go with `project` instead of `branch` since that's the terminology
> here?

Yeah, good point. This is going to make the buildbot update verify kind a bit uglier (buildername will be templatized differently between the scopes (where we'll use {project}) and the run (where we'll use {branch}) -- but I think it's worth it to keep task.py more sane. I've made this change on maple.

Comment 26

[[e-mail] (:bhearsum)]

I've made some improvements to the in tree code based on your feedback and other things I discovered. These changes are landed on maple in https://hg.mozilla.org/projects/maple/rev/5b7f85dc7d4b8c969f3cf5290f09fe826cd95d87 and https://hg.mozilla.org/projects/maple/rev/b79c7c5d2a4f04c516082c5adfe5a595e52611b9. They include:
- Use test channels instead of live channels
- Use "project" instead of "branch" as replacement in scopes
- Use the correct tag for the tools repo
- Stub out a potential way to do beta-on-rc update verify. I've not yet figured out a way to test this.
- Add comments about required dependencies. As far as I can tell, none of them exist in-tree yet, so I can't start adding them.

Comment 27

[[e-mail] (:bhearsum)]

Cancelling needinfo for now because I realized that I still need to account for all the differences between beta vs rc vs esr.

Comment 28

[[e-mail] (:bhearsum)]

I've made some additional progress here, but I've also run into some new yaks that I haven't been able to shave. The current code on maple _should_ be capable of generating update verify builders for betas and RCs (including the beta channel update verify jobs), but dependencies still aren't sorted out due to a couple of issues:
1) shipping-product doesn't seem to be set correctly for beetmover-repackage jobs, which means release_deps doesn't add any dependencies. I added a horrible hack in https://hg.mozilla.org/projects/maple/rev/3843def81732bebf4404f81e4633c21035b30273 to workaround this, but I'm pretty sure it's not the right way forward
2) Even with my horrible hack, the promote decision task fails due to having > 100 dependencies. I started looking at depending on the post-beetmover-dummy task instead of beetmover-repackage directly, but that doesn't work because the shipping-phase doesn't match (ship vs. promote). I tried to add my own post-beetmover-promote-dummy, but _that_ didn't work because a) the tier was wrong (it had no tier, update verify is tier 1), and b) when i added a tier, I started getting errors about duplicate treeherder symbols.

I'd like some guidance before I go down any particular road here - I really don't even know if I'm on the right track. It feels like there should be a simpler solution to all of this.

Comment 29

[Aki Sasaki (not active)]

I think the right fix involves creating different jobs for each actual thing we want to do. I think the problem is beetmover-repackage is used for many things - repackaging signed builds, repackaging signed l10n, and I imagine in the future we'll use it for repackaging partner repacks as well. Having beetmover-repackage dynamically getting its info from its upstream jobs means we have to do the same for downstream jobs (we'll still have to get dynamic info for l10n, though?).

This sounds like a multiple step fix because we took shortcuts earlier. If we have the time, let's split this out to multiple correct fixes, and see who has time and headspace to take pieces on.

Comment 30

[[e-mail] (:bhearsum)]

Attachment: in tree update verify, almost done?

This patch is not landable, but contains all of the new code required for update verify tasks, and some other code that it depends on. Here's a summary:
* Beetmover dependencies are working (via post-beetmover-dummy)
** I had to modify the reverse_chunk_deps transform to set Treeherder symbols to avoid the non-matching-tiers verification error.
* Balrog dependencies are set, but won't show up until shipping-product/phase is set on the balrog kinds. I'm leaving this for Mihai, because my naive attempt didn't work.
** This includes a new post-balrog-dummy kind.
* Includes minor fixes for the update verify docker image.
* We can't be certain that the update verify commands are correct until upstreams are ready, and Balrog data is getting submitted correctly. But they look plausible.
* Devedition is not supported yet, but _should_ just be additional lines in the "jobs" sections.
* The "release" channel testing for RCs probably works, but additional work will be needed to support the "beta" channel testing we do there. I think it's best to do this as follow-up work, because it doesn't need to block us being ready to use in-tree on Beta.

Comment 31

[Aki Sasaki (not active)]

Comment on attachment 8932227 [details] [diff] [review]
in tree update verify, almost done?

This is looking good!

>--- /dev/null
>+++ b/taskcluster/taskgraph/transforms/buildbot_update_verify.py
>@@ -0,0 +1,45 @@
>+# This Source Code Form is subject to the terms of the Mozilla Public
>+# License, v. 2.0. If a copy of the MPL was not distributed with this
>+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
>+"""
>+Transform the beetmover task into an actual task description.
>+"""
>+
>+from __future__ import absolute_import, print_function, unicode_literals
>+
>+from copy import deepcopy
>+
>+from taskgraph.transforms.base import TransformSequence
>+from taskgraph.util.schema import resolve_keyed_by
>+
>+transforms = TransformSequence()
>+
>+
>[e-mail]
>+def add_command(config, tasks):
>+    for task in tasks:
>+        total_chunks = task["extra"]["chunks"]
>+        platform = task["attributes"]["build_platform"]
>+        product = task["extra"]["product"]
>+        buildername = "release-{branch}_" + product + "_" + platform + \
>+            "_update_verify"
>+
>+        for this_chunk in range(1, total_chunks+1):
>+            chunked = deepcopy(task)
>+            chunked["scopes"] = [
>+                "project:releng:buildbot-bridge:builder-name:{}".format(
>+                    # In scopes, "branch" is called "project"
>+                    buildername.replace("branch", "project")
>+                )

Hm, is there a reason you didn't specify {project} in buildername rather than specifying {branch} and then replacing it? I think both are specified in the subs.
This isn't a blocker; just trying to get consistent naming if possible. If something outside of the tree requires us to set {branch} then that's another matter.

Comment 32

[[e-mail] (:bhearsum)]

(In reply to Aki Sasaki [:aki] from comment #31)
> >[e-mail]
> >+def add_command(config, tasks):
> >+    for task in tasks:
> >+        total_chunks = task["extra"]["chunks"]
> >+        platform = task["attributes"]["build_platform"]
> >+        product = task["extra"]["product"]
> >+        buildername = "release-{branch}_" + product + "_" + platform + \
> >+            "_update_verify"
> >+
> >+        for this_chunk in range(1, total_chunks+1):
> >+            chunked = deepcopy(task)
> >+            chunked["scopes"] = [
> >+                "project:releng:buildbot-bridge:builder-name:{}".format(
> >+                    # In scopes, "branch" is called "project"
> >+                    buildername.replace("branch", "project")
> >+                )
> 
> Hm, is there a reason you didn't specify {project} in buildername rather
> than specifying {branch} and then replacing it? I think both are specified
> in the subs.
> This isn't a blocker; just trying to get consistent naming if possible. If
> something outside of the tree requires us to set {branch} then that's
> another matter.

I don't think "project" gets replaced in buildername, per https://dxr.mozilla.org/mozilla-central/source/taskcluster/taskgraph/transforms/job/buildbot.py#63 - maybe I should make that happen, instead of what I'm doing here?

Comment 33

[Aki Sasaki (not active)]

Aha, that seems like what I'm asking for.

`buildername = run['buildername'].format(branch=branch, project=branch)` ? which would simplify the above `add_command` transform.

I'm aware we shouldn't over-rotate here; the long-term fix is retiring buildbot-bridge. I'm hoping this is a simple change.

Comment 34

[[e-mail] (:bhearsum)]

Attachment: RC support

RC support means that we need to run update verify against the beta-localtest channel, but only for releases that ship to it. To do this, I've added new jobs, a "desktop_release_type" parameter. We filter out the new jobs unless this parameter is set to "rc". In my testing, this has made no change to any of the existing graphs, and caused the "secondary" jobs to be added to RC graphs (I've got a braindump patch queued up that adds new parameters files for these).

Comment 35

[[e-mail] (:bhearsum)]

Attachment: add desktop_release_type support to release-runner3

This patch is required once the Gecko one lands on maple - it sets desktop_release_type for all Firefox and Devedition releases. As noted in the patch, I'm setting it to "rc" for any non-Betas/ESRs that contain a Beta as a partial or look like a final release. I _think_ this should enable us to ship point releases to the Beta channel when necessary.

Comment 36

[[e-mail] (:bhearsum)]

Attachment: add desktop_release_type & RC parameters files

Comment 37

[Rail Aliiev [:rail]]

Comment on attachment 8933676 [details] [diff] [review]
add desktop_release_type support to release-runner3

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

wow, that's a lot of ifs :) But probably this is the easiest way to go

Comment 38

[Aki Sasaki (not active)]

Comment on attachment 8933675 [details] [diff] [review]
RC support

Let's tear out all the extra: product lines. We're now keying off of shipping-product.

Comment 39

[Aki Sasaki (not active)]

Comment on attachment 8933677 [details] [diff] [review]
add desktop_release_type & RC parameters files

a) just make sure a desktop_release_type of null works for us where appropriate, and b) an `hg cp` would have preserved history. Not a requirement, esp in braindump, though.

Comment 40

[[e-mail] (:bhearsum)]

(In reply to Nick Thomas [:nthomas] (UTC+13) from comment #11)
> Comment on attachment 8923987 [details] [diff] [review]
> implement (but disable) cross platform update verify
> 
> Looks pretty good for me. I did spot this error finding the precomplete in
> the mac run [1], just for complete updates:
> 
> + /tools/release/updates/updater/firefox/updater
> /tools/release/updates/update /tools/release/updates/source/Firefox.app
> /tools/release/updates/source/Firefox.app 0
> Unable to init server: Could not connect: Connection refused
> + set +x
> PATCH DIRECTORY /tools/release/updates/update
> INSTALLATION DIRECTORY /tools/release/updates/source/Firefox.app
> WORKING DIRECTORY /tools/release/updates/source/Firefox.app
> UPDATE TYPE complete
> GetManifestContents: error opening manifest file:
> /tools/release/updates/source/Firefox.app/precomplete
> AddPreCompleteActions: error getting contents of precomplete manifest
> PREPARE ADD Contents/_CodeSignature/CodeResources
> 
> It still succeeds, but if a file is deprecated it may not be removed
> properly in the test. The precomplete file seems to live in
> Contents/Resources/. Would you mind chasing this down ?
> 
> Not worried about "Unable to init server" message from trying to show some
> gnome ui.
> 
> [1]
> https://public-artifacts.taskcluster.net/ThtN0-ZjS0e84Nnhr_64tg/0/public/
> logs/live_backing.log

I dug into this a bit more and I _think_ this is because there is what appears to be a target architecture ifdef in the updater code around finding the precomplete file: https://dxr.mozilla.org/mozilla-central/rev/a928be5dacc3b544e29c0612b3f8cda6447df802/toolkit/mozapps/update/updater/updater.cpp#4074

There's also similar ifdefs for finding update-settings.ini and a bunch of stuff that probably doesn't matter for update verify (mostly around elevation and how to do filesytem/process stuff).

I think we can workaround this by symlining precomplete & update-settings to the expected locations.

Robert, I'm flagging you here just in case you have any thoughts on this, or know of other places where we might hit issues with assumptions about updater target architecture being the same as MAR architecture.

Comment 41

[Robert Strong (they/them - no direct email)]

It might be better to copy those files prior to updating and then deleting the copies afterwards since the update will update the precomplete file.

If the test updater is used (something we've recently discussed) I wouldn't be against modifying the code for the test updater to look in both locations.

I don't think there are any other issues besides this and nothing else jumped out at me when examining the code that would affect this test.

Comment 42

[[e-mail] (:bhearsum)]

Attachment: cross platform update verify w/ mac problems fixed

Thanks for your guidance!

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #41)
> It might be better to copy those files prior to updating and then deleting
> the copies afterwards since the update will update the precomplete file.

Good call - I tried symlinking and moving first, but copying was the right choice. I don't think I need to delete them afterwards since we just remove the entire directory after the test? I might be missing something though.

This patch is the same as the previous one except for the precomplete/update-settings.ini fix, and a couple of minor improvements.

I ran this on Taskcluster in https://tools.taskcluster.net/groups/WYDLVvvYT22HnKya0sVZ9Q/tasks/WYDLVvvYT22HnKya0sVZ9Q/runs/0/artifacts

Comment 43

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 8935404 [details] [diff] [review]
cross platform update verify w/ mac problems fixed

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

The interdiff looks good. I'm a bit surprised that precomplete and update-settings.ini don't show up in the source vs target diff (as 'Only in' lines). I think that's what rstrong means about deleting afterwards. Any thoughts on that ?

Comment 44

[Robert Strong (they/them - no direct email)]

Comment on attachment 8935404 [details] [diff] [review]
cross platform update verify w/ mac problems fixed

Talked about this with bhearsum over vidyo.

Comment 45

[[e-mail] (:bhearsum)]

(In reply to Nick Thomas [:nthomas] (UTC+13) from comment #43)
> The interdiff looks good. I'm a bit surprised that precomplete and
> update-settings.ini don't show up in the source vs target diff (as 'Only in'
> lines). I think that's what rstrong means about deleting afterwards. Any
> thoughts on that ?

This is because we're copying these files for both the source and the target builds. (I did an additional test with some extra debugging statements to verify: https://public-artifacts.taskcluster.net/Hd_lBKTQSqmKZuazLKTPSA/4/public/logs/live_backing.log)

Comment 46

[[e-mail] (:bhearsum)]

Comment on attachment 8927458 [details] [diff] [review]
allow channel to be specified for buildbot update verify

https://hg.mozilla.org/build/tools/rev/ece1728dbc86fcca79cb16eb85b56bf9904dc9b2

Comment 47

[[e-mail] (:bhearsum)]

Attachment: in-tree patch for cross platform update verify

This is what I think will be the necessary patch to enable cross platform update verify in tree. My local task graphs look plausible, at least. Once non-cross platform update verify is green, I'm going to start trying to test this out in staging releases - but I'm not sure it will be ready in time for our initial landing on central, so I will make sure it's easy to backout.

Comment 48

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 8935404 [details] [diff] [review]
cross platform update verify w/ mac problems fixed

Ah, that makes sense. r+

Comment 49

[[e-mail] (:bhearsum)]

Attachment: cross platform update verify gecko patch

I've tested this as much as I can locally (taskgraph diffs, limited mozharness runs), and it looks like it should work. The highlights are:
- Kill release-buildbot-update-verify kind & transform
- Add Mac/Windows platforms to release-update-verify kind
- Make config bump script look for "updater_platform" buildbot property, and pass that along to update verify config scripts.
- Add updater_platform to release-update-builder properties.

The latter two points make it possible to enable cross platform update verify for in-tree releases, while leaving them disabled for others - which is important so that we don't bust current esr/release/beta.

Comment 50

[Rail Aliiev [:rail]]

Comment on attachment 8937460 [details] [diff] [review]
cross platform update verify gecko patch

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

::: taskcluster/ci/release-update-verify/kind.yml
@@ +155,5 @@
> +                  jamun: "beta-firefox-win64.cfg"
> +                  maple: "beta-firefox-win64.cfg"
> +                  mozilla-beta: "beta-firefox-win64.cfg"
> +                  mozilla-release: "release-firefox-win64.cfg"
> +                  mozilla-esr52: "esr-firefox-win64.cfg"

What happens when we have 2 overlapping ESR releases? The value doesn't look train specific.

@@ +398,5 @@
> +                  maple: "aurora-localtest"
> +                  mozilla-aurora: "aurora-localtest"
> +                  mozilla-release: "release-localtest"
> +                  mozilla-esr52: "esr-localtest"
> +                  default: "default"

Hmmm, I think devedition is valid only for mozilla-beta (missing) and the dev branches. Also mozilla-aurora is not a thing anymore IIRC.

Comment 51

[[e-mail] (:bhearsum)]

(In reply to Rail Aliiev [:rail] ⌚️ET (away until Feb 5) from comment #50)
> Comment on attachment 8937460 [details] [diff] [review]
> cross platform update verify gecko patch
> 
> Review of attachment 8937460 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: taskcluster/ci/release-update-verify/kind.yml
> @@ +155,5 @@
> > +                  jamun: "beta-firefox-win64.cfg"
> > +                  maple: "beta-firefox-win64.cfg"
> > +                  mozilla-beta: "beta-firefox-win64.cfg"
> > +                  mozilla-release: "release-firefox-win64.cfg"
> > +                  mozilla-esr52: "esr-firefox-win64.cfg"
> 
> What happens when we have 2 overlapping ESR releases? The value doesn't look
> train specific.

Good catch, I'll fix this.


> @@ +398,5 @@
> > +                  maple: "aurora-localtest"
> > +                  mozilla-aurora: "aurora-localtest"
> > +                  mozilla-release: "release-localtest"
> > +                  mozilla-esr52: "esr-localtest"
> > +                  default: "default"
> 
> Hmmm, I think devedition is valid only for mozilla-beta (missing) and the
> dev branches. Also mozilla-aurora is not a thing anymore IIRC.

Also true. I'll update these blocks as well.

Comment 52

[[e-mail] (:bhearsum)]

Attachment: finalized cross platform update verify patch

I finally got a chance to run cross platform update verify in the full context of a release. The gecko patch worked perfectly, but it uncovered a couple of issues with the tools patch:
* ja-JP-mac verification failed, because we tried to download a 'ja-JP-mac' Linux updater. Fixing this is a little bit hacky, but easy.
* The precomplete file that I copy to the root of the install ends up with a lot of differences, because the MAR doesn't update it. To fix this, I remove these files after the updater runs but before the diff.

Full graphs with cross platform update verify are available at https://tools.taskcluster.net/groups/D-XtMTjnRj-UKQZXtaOUIw and https://tools.taskcluster.net/groups/RDmKsWGTTXG5AWA-VoHpMg.

I also reran one chunk per platform with this tools patch. You can find those at https://tools.taskcluster.net/groups/dZUwT3EvQMygdNBO0E2ppw/tasks/dZUwT3EvQMygdNBO0E2ppw/runs/0/artifacts, https://tools.taskcluster.net/groups/S0Tz53kGTJG53X4siWbkAA/tasks/S0Tz53kGTJG53X4siWbkAA/runs/0/artifacts, and https://tools.taskcluster.net/groups/K0KdIFuSRxKK2I5N9S-63g/tasks/K0KdIFuSRxKK2I5N9S-63g/runs/0/artifacts.

With all of this applied, the only remaining diffs in update verify are for the channel in Beta staging releases (we're still using "nightly-maple" there).

Comment 53

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 8938049 [details] [diff] [review]
finalized cross platform update verify patch

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

Interdiff to attachment 8935404 [details] [diff] [review] and the staging runs look good.

Comment 54

[[e-mail] (:bhearsum)]

Comment on attachment 8937460 [details] [diff] [review]
cross platform update verify gecko patch

This worked well in the staging release, time for real review.

Comment 55

[[e-mail] (:bhearsum)]

Comment on attachment 8937460 [details] [diff] [review]
cross platform update verify gecko patch

Looks like Rail is out for awhile, moving review.

Comment 56

[[e-mail] (:bhearsum)]

Comment on attachment 8938049 [details] [diff] [review]
finalized cross platform update verify patch

https://hg.mozilla.org/build/tools/rev/7326a6fae2cd2c7f688bf2c06943cc2b64063c58

Comment 57

[Aki Sasaki (not active)]

Comment on attachment 8937460 [details] [diff] [review]
cross platform update verify gecko patch

Schweet!

Comment 58

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae502b906e56
enable cross platform update verify for in-tree releases. r=aki

Comment 59

[[e-mail] (:bhearsum)]

With the cross platform update verify work landed, the last thing left here is to ensure that this all works for RCs. This testing is currently happening on birch.

Comment 60

[Eliza Balazs [:ebalazs_]]

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

Comment 61

[[e-mail] (:bhearsum)]

Comment on attachment 8938049 [details] [diff] [review]
finalized cross platform update verify patch

This has been backed out because it broke verification of really old updates:
../common/check_updates.sh: line 70: /builds/slave/rel-m-rel_fx_m64_u_v-000000000/scripts/release/updates/updater/Firefox.app/Contents/MacOS/updater.app/Contents/MacOS/org.mozilla.updater: No such file or directory

I purposely removed some code around this beacuse I thought it was dead - I'll add it back in.

Comment 62

[[e-mail] (:bhearsum)]

Attachment: cross platform update verify w/ support for ancient updater location on mac

This should fix the issues we encounter in 57.0.4 - it adds support for multiple updater binary locations back into update verify. This isn't needed for cross platform to work, but it is needed for mac-on-mac to work until we watershed away the old versions. Since there's the possibility of more point releases, and probably doing 58.0 without cross platform update verify, we'll need this for awhile.

I reran the failed 57.0.4 update verify chunk with this applied: http://buildbot-master86.bb.releng.scl3.mozilla.com:8001/builders/release-birch_firefox_macosx64_update_verify/builds/8

Comment 63

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 8940226 [details] [diff] [review]
cross platform update verify w/ support for ancient updater location on mac

>diff --git a/release/updates/verify.sh b/release/updates/verify.sh
>+        for updater_bin in $updater_bins; do
>+            if [ -e "$cwd/$updater_bin" ]; then
>+                echo "Found updater at $updater"

Should use $updater_bin instead of $updater here, to fix up log messages 'Found updater at null'. Looks good otherwise.

Comment 64

[[e-mail] (:bhearsum)]

Comment on attachment 8940226 [details] [diff] [review]
cross platform update verify w/ support for ancient updater location on mac

https://hg.mozilla.org/build/tools/rev/3e6086f4b4c5dc99a80c286ee1e2a67a54965869

Comment 65

[[e-mail] (:bhearsum)]

Attachment: set channel list correctly for updates builder

In testing on birch, I discovered that the "channels" property wasn't getting set correctly on the updates builder. It was set in the templates (https://dxr.mozilla.org/mozilla-central/rev/739484451a6399c7f156a0d960335606aa6c1221/taskcluster/ci/release-updates-builder/kind.yml#93), but it was getting overridden by some code that was added for the balrog publishing task.

This patch gets rid of the low level overriding of "channels", which fixes the updates builder task definition. I replaced it with "channels" properties in the balrog publishing kind.

Comment 66

[[e-mail] (:bhearsum)]

Attachment: install gtk2 libraries in update verify docker image

We don't need this if we end up adding a new watershed for Linux, but the packages are pretty small so it's probably best to add them just in case. We might need them for ESR, anyways.

Comment 67

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8940835 [details] [diff] [review]
set channel list correctly for updates builder

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

LGTM. I think there's one small nit to fix.

::: taskcluster/ci/release-updates-builder/kind.yml
@@ +103,4 @@
>                    default: "default"
>              repo_path:
>                 by-project:
> +                  jamun: "projects/birch"

Nit? s/jamun/birch

::: taskcluster/taskgraph/transforms/job/buildbot.py
@@ -39,4 @@
>          return 'https://balrog-admin.stage.mozaws.net/api'
>  
>  
> -def _get_balrog_channel(product, branch):

Glad to see this hack gone \o/

Comment 68

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2a923fb26cf
set channel list correctly for updates builder. r=jlorenzo
https://hg.mozilla.org/integration/mozilla-inbound/rev/21c347b469e7
install gtk2 libraries in update verify docker image. r=aki

Comment 69

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/f2a923fb26cf
https://hg.mozilla.org/mozilla-central/rev/21c347b469e7