Closed Bug 1461490 Opened 6 years ago Closed 6 years ago

Add an allowlist of expected update verify differences

Categories

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

Product:
Release Engineering
Component:
Release Automation: Updates
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: nthomas, Assigned: nthomas)

References

Details

(Whiteboard: [releaseduty])

Attachments

(1 file)

Bug 1461490 - update verify allowlist followup for windows ESR52,
59 bytes, text/x-review-board-request
bhearsum
: review+
nthomas
: checked-in+
Details 
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.
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.
Whiteboard: [releaseduty]
Summary: Add a whitelist for expected update verify differences → Add an allowlist of expected update verify differences
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 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?
Attachment #8979135 - Flags: review?(aki) → review+
Thanks!
(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.
I'm guessing this is done; is that correct? If not, we should probably move the trello card back :)
All done!
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
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?
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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 on attachment 8979135 [details]
Bug 1461490 - update verify allowlist followup for windows ESR52,

https://reviewboard.mozilla.org/r/245376/#review256180
Attachment #8979135 - Flags: review?(bhearsum) → review+
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
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.