1461490 - Add an allowlist of expected update verify differences

Status: RESOLVED FIXED

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

Description

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

There are a some cases where we end up with differences after doing old+update vs new build:

1, up until 59.0b3 we had a preprocessed line in channel-prefs.js, which could change between builds. That was removed by bug 1431342. Example diff:
Found diffs for complete update from https://aus5.mozilla.org/update/3/Firefox/58.0/20171127135700/Linux_x86-gcc3/en-US/beta-localtest/default/default/default/update.xml?force=1
diff -r source/firefox/defaults/pref/channel-prefs.js target/firefox/defaults/pref/channel-prefs.js
5d4
< //@line 6 "/builds/worker/workspace/build/src/browser/app/profile/channel-prefs.js"


2, when we push RC builds to the beta channel we expect the RC build to specify the release channel and to only accept release mar files. eg:
Found diffs for complete update from https://aus5.mozilla.org/update/3/Firefox/60.0/20180426170554/Linux_x86-gcc3/en-US/release-localtest/default/default/default/update.xml?force=1
diff -r source/firefox/defaults/pref/channel-prefs.js target/firefox/defaults/pref/channel-prefs.js
5c5
< pref("app.update.channel", "beta");
---
> pref("app.update.channel", "release");
diff -r source/firefox/update-settings.ini target/firefox/update-settings.ini
5c5
< ACCEPTED_MAR_CHANNEL_IDS=firefox-mozilla-beta,firefox-mozilla-release
---
> ACCEPTED_MAR_CHANNEL_IDS=firefox-mozilla-release


3, related to 2, we now test RC -> latest beta, and hit a channel-prefs.js difference but not one for update-settings.ini:
Found diffs for complete update from https://aus5.mozilla.org/update/3/Firefox/60.0/20180503143129/Linux_x86-gcc3/th/beta-localtest/default/default/default/update.xml?force=1
diff -r source/firefox/defaults/pref/channel-prefs.js target/firefox/defaults/pref/channel-prefs.js
5c5
< pref("app.update.channel", "release");
---
> pref("app.update.channel", "beta");


Since bug 1446160 we fail on any differences, so we need to explicitly whitelist these to avoid releaseduty inspecting logs manually.

Comment 1

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

Note to self: also documented at https://github.com/mozilla-releng/releasewarrior-2.0/blob/86aa37b5fe64b01f66b37eb453bd1d4f4208aab0/docs/misc-operations/analyze-update-verify-logs.md#known-differences. Should update that when done here.

Comment 2

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

WIP: working 12/12 update verify that previously got hung up on the javascript comments in channel-prefs.js:
https://tools.taskcluster.net/groups/cWh9ZYLtTHuYmVYRizzhYw/tasks/cWh9ZYLtTHuYmVYRizzhYw/details

Code is in https://hg.mozilla.org/users/nthomas_mozilla.com/tools/, mostly 2e9fab62aee1.

Possibly an issue with diff-summary.log not being populated.

Comment 3

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

Attachment: Bug 1461490 - update verify allowlist followup for windows ESR52,

This reimplements our source+update vs target comparison in Python, allowing the
definition of a set of transforms to resolve expected differences. These relate to
preprocessed lines in channel-prefs.js (which depend on CI implementation), and
updating users on the beta channel with X.0 candidate builds for the release
channel (RCs). The transforms
 * are restricted to specific files and channel prefixes,
 * may act on the source or target directory,
 * can delete lines and/or replace line content

The comparison script exits with status 2 on any file differences remaining after
applying transforms, or if files/directories are found only in source or target.
This signals a test failure to the calling script. Previously files/directories on
only one side didn't change the exit status, merely logging a warning. Only
unresolved differences make it into the diff-summary.log artifact on the update
verify tasks.

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

Comment 4

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

Retesting some recent releases:

Firefox 61.0b6:    https://tools.taskcluster.net/groups/GRAMDnbMT6K_zsACzqgRnw
DevEdition 61.0b6: https://tools.taskcluster.net/groups/IMoe6W_rSHS0QjbGolBVsg

Firefox 60.0 build2, release channel: https://tools.taskcluster.net/groups/AsEYwgWRQBSkXjlVd7jN0w
Firefox 60.0 build2, beta  channel:   https://tools.taskcluster.net/groups/Po7Tmi92SzGfD7dPE0JjHA

Firefox 52.8.0esr: https://tools.taskcluster.net/groups/IaNP62N5Qk2i4akWtUFbww
Firefox 60.0esr  : Not possible due to configuration generation issue, see bug 1462120

Comment 5

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

Doc update - https://github.com/mozilla-releng/releasewarrior-2.0/pull/147

Comment 6

[Aki Sasaki (not active)]

Comment on attachment 8979135 [details]
Bug 1461490 - update verify allowlist followup for windows ESR52,

https://reviewboard.mozilla.org/r/245376/#review251452

::: release/common/unpack.sh:107
(Diff revision 1)
>      fi
>  
>      if [ ! -z $update_settings_string ]; then
>         echo "Modifying update-settings.ini"
>         cat  "${update_settings_file}" | sed -e "s/^ACCEPTED_MAR_CHANNEL_IDS.*/ACCEPTED_MAR_CHANNEL_IDS=${update_settings_string}/" > "${update_settings_file}.new"
> -       diff -u "{$update_settings_file}" "${update_settings_file}.new"
> +       diff -u "${update_settings_file}" "${update_settings_file}.new"

weird, how did this work before?

Comment 7

[Aki Sasaki (not active)]

Thanks!

Comment 8

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

(In reply to Aki Sasaki [:aki] from comment #6)
> weird, how did this work before?

The call to diff wasn't working, but no 'set -e' so it was just a log line that didn't fail the job, eg

Modifying update-settings.ini
diff: {firefox/update-settings.ini}: No such file or directory

in https://taskcluster-artifacts.net/O4sRM_ALTAqf5JyHHYs6SQ/0/public/logs/live_backing.log from 61.0b4.

Comment 9

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

Comment on attachment 8979135 [details]
Bug 1461490 - update verify allowlist followup for windows ESR52,

https://hg.mozilla.org/build/tools/rev/e94302d4b6061799e5a7fff698d482e672aceeda
https://github.com/mozilla-releng/releasewarrior-2.0/commit/f0200e61feaffc1fcfea3787735e6bd26f4ccc9c (but I forgot to update the changelog)

Comment 10

[Aki Sasaki (not active)]

I'm guessing this is done; is that correct? If not, we should probably move the trello card back :)

Comment 11

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

All done!

Comment 12

[[e-mail] (:bhearsum)]

I _think_ this broke on Windows (which we still run it on for esr52). We ended up with output like:
Comparing source/bin with target/bin...
Directories only in source/bin:
  source/bin\gmp-clearkey\0.1
  source/bin\fonts
  source/bin\browser\VisualElements
  source/bin\browser
  source/bin\browser\extensions
  source/bin\uninstall
  source/bin\browser\features
  source/bin\defaults\pref
  source/bin\gmp-clearkey
  source/bin\defaults
Directories only in target/bin:
  target/bin\browser\VisualElements
  target/bin\defaults
  target/bin\browser\extensions
  target/bin\browser\features
  target/bin\gmp-clearkey
  target/bin\browser
  target/bin\uninstall
  target/bin\fonts
  target/bin\defaults\pref
  target/bin\gmp-clearkey\0.1


...which is just a difference in ordering. I used https://gist.github.com/mozbhearsum/f827a919707445bbb0ced6884f761d7f to verify that only the ordering was different for all of the 52.8.1esr windows update verify logs.

Might need a sorted() somewhere in the script for a quick workaround?

Comment 13

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

Thanks for the headsup. The source/bin prefix shouldn't be in output, which indicates this code is faulty in the presence of a forward slash:

# trim off directory prefix for easier comparison
    all_dirs = [d.replace(path + '/', '') for d in all_dirs]

I'll figure out a patch.

Comment 14

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

Comment on attachment 8979135 [details]
Bug 1461490 - update verify allowlist followup for windows ESR52,

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

Comment 15

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

The new patch trims off the path prefix and one more character from the full path. I verified it fixes Win32 ESR52, and doesn't regress mac ESR52 or 60.0.2 mac/windows. The use of sorted() is just a pretty printing change.

Comment 16

[[e-mail] (:bhearsum)]

Comment on attachment 8979135 [details]
Bug 1461490 - update verify allowlist followup for windows ESR52,

https://reviewboard.mozilla.org/r/245376/#review256180

Comment 17

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

Landed: https://hg.mozilla.org/build/tools/rev/88b450a68f2444542486da1421f85f3c56225d37

Spot verification: 
* 52.8.1esr fixed (rerun on default instead of tag)
   https://tools.taskcluster.net/groups/ZtQzYPSjSL2O2OzrZnrKDw/tasks/ZtQzYPSjSL2O2OzrZnrKDw/details 
* 61.0b12 still green (rerun)
   https://tools.taskcluster.net/groups/BFGv0jLNQGakqg872NYa5g/tasks/Cv425sO_TOeXlz22W8fKAQ/runs/1