update verification fails for Firefox 54.0b6 chunks {1,2,3,4} on all platforms

RESOLVED FIXED

Status

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mtabara, Assigned: aki)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 8 obsolete attachments)

There's something fishy going on with update verification. All platforms fail for chunks {1,2,3,4} but everything else is green.
(Reporter)

Comment 1

2 years ago
So this is something weird. The "firefox mozilla-beta win64 l10n repack 1/10"[1] task is still shown as running in TC, but its corresponding down-the-stream "firefox mozilla-beta win64 l10n repack artifacts 1/10"[2] is green. Build API[3] shows it indeed as runnning so I'm not sure what's going on here. I remember :jlorenzo was mentioning that I might see some miscommunication between TC and BBB but the fact that I see the artifacts job green dazzles me.

Not sure if this is related to the update verification failing tasks.
Will dig more on that.

[1]: https://tools.taskcluster.net/task-inspector/#5JZel4P-Ryu3_PZLS28GaA/
[2]: https://tools.taskcluster.net/task-group-inspector/#/lRoNILIzRdi1-blSu6URlw/-IDPlGVVSIqkwkW6j_ZGqw?_k=0ukl0n
[3]: http://buildbot-master70.bb.releng.use1.mozilla.com:8001/builders/release-mozilla-beta_firefox_win64_l10n_repack/builds/102
(Reporter)

Comment 2

2 years ago
The patch directory path is not valid for this application (../../update)
cat: update/update.log: No such file or directory
cat: update/update.status: No such file or directory
FAIL: update status was not successful: 
FAIL: check_updates returned failure for WINNT_x86-msvc-x86 downloads/Firefox Setup 54.0b5.exe vs. downloads/Firefox Setup 54.0b6.exe: 1

from https://mozilla-release-logs.s3.amazonaws.com/mozilla-beta/firefox-54.0b6/build1/win32_beta_update_verification_1_12-win32-KeU_EtqcSuWbMz6HpiYbYA-0


... and 

Found updater at Contents/MacOS/updater.app/Contents/MacOS/org.mozilla.updater
The patch directory path is not valid for this application (../../update)
cat: update/update.log: No such file or directory
cat: update/update.status: No such file or directory
FAIL: update status was not successful: 
FAIL: check_updates returned failure for Darwin_x86_64-gcc3-u-i386-x86_64 downloads/Firefox 54.0b5.dmg vs. downloads/Firefox 54.0b6.dmg: 1

from https://mozilla-release-logs.s3.amazonaws.com/mozilla-beta/firefox-54.0b6/build1/macosx64_beta_update_verification_2_12-macosx64-nQGxiHJvR1CJcY5KfxlXYg-0
(Reporter)

Comment 3

2 years ago
Could be a subset of locales that are not correctly taken into account or something. It's only failing for {1,2,3,4} chunks across all platforms.
(Reporter)

Comment 4

2 years ago
14:08:36 @rail> mtabara: sounds like related to https://hg.mozilla.org/mozilla-central/rev/702bca2e601f
14:08:37 <~mtabara> could be. thing that dazzled me is that it only occurs to a subset of the chunks, not all of them
14:09:02 <@rail> hmm
14:09:03 <~mtabara> plus, that's .. mozilla-central
14:09:31 <~mtabara> 10 days ago, so those changes should not be on beta
14:09:33 — ~mtabara double checks
14:10:07 <@rail> I bet it was uplifted
14:10:26 <@rail> "The patch directory path is not valid for this application" is from there
14:10:38 <~mtabara> then you're certainly right
14:17:29 <@rail> ah, it doesn't accept relative paths anymore
14:17:32 <@rail> sweet
14:18:35 <@rail> https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/common/updatecommon.cpp#201
14:21:12 <@rail> probably we need to fix https://dxr.mozilla.org/build-central/source/tools/release/common/check_updates.sh#59-68
(Reporter)

Comment 5

2 years ago
So specifically for this 54.0b6 release, :FlorinMezei and :avaida helped us with manual testing. Also, TIL that:

FlorinMezei: "I don't think manual testing is necessary, as we covered this already in the ondemand updated tests. The tests cover the Direct and Fallback update scenarios, they interact with the UI,and they have all passed on beta-cdntest. There were 23 total jobs that tested the update from 54.0b5 to 54.0b6, on 9 different Operating Systems (2 x Linux, 3 x OS X, 4 x Windows), for en-US and 10 other locales. I think this should be enough confirmation."

We still need to fix our update verification side of automation for upcoming betas.
(Assignee)

Comment 6

2 years ago
Do we just insert $PWD ?

    if [ "$use_old_updater" = "1" ]; then
        $PWD/../../update/$updater_bin $PWD/../../update $PWD 0
    else
        $PWD/../../update/$updater_bin $PWD/../../update $PWD $PWD 0
    fi

(Quoted if we want to be safe)?
(Assignee)

Comment 7

2 years ago
Posted patch abspath.diff (obsolete) — Splinter Review
Let's tag this with FIREFOX_54_0b6_BUILD1_RUNTIME and rerun the failed tasks... esp windows.
Attachment #8866443 - Flags: review?(rail)
(Assignee)

Updated

2 years ago
Attachment #8866443 - Flags: review?(nthomas)
(Assignee)

Comment 8

2 years ago
Posted patch abspath2.diff (obsolete) — Splinter Review
Attachment #8866443 - Attachment is obsolete: true
Attachment #8866443 - Flags: review?(rail)
Attachment #8866443 - Flags: review?(nthomas)
Attachment #8866569 - Flags: review?(nthomas)
(Assignee)

Comment 9

2 years ago
Posted patch abspath3.diffSplinter Review
abspaths for all args :)
Attachment #8866569 - Attachment is obsolete: true
Attachment #8866569 - Flags: review?(nthomas)
Attachment #8866571 - Flags: review?(nthomas)
Comment on attachment 8866571 [details] [diff] [review]
abspath3.diff

lgtm, lets try it.
Attachment #8866571 - Flags: review?(nthomas) → review+
(Assignee)

Comment 12

2 years ago
(In reply to Aki Sasaki [:aki] from comment #11)
> https://hg.mozilla.org/build/tools/rev/
> 0c5eeea4f3b618b3a946868252f78fa1ed9b9b53
> bug 1363334 - use abspaths for update tests. r=nthomas

This patch appears to fix non-windows update tests.
Windows is exiting with

Found updater at updater.exe
The patch directory path is not valid for this application (c:/builds/moz2_slave/rel-m-beta_fx_w64_u_v-00000000/scripts/release/updates/update)
cat: update/update.log: No such file or directory
cat: update/update.status: No such file or directory
FAIL: update status was not successful: 
FAIL: check_updates returned failure for WINNT_x86_64-msvc-x64 downloads/Firefox Setup 54.0b5.exe vs. downloads/Firefox Setup 54.0b6.exe: 1

Something in this ifdef block is breaking.
https://hg.mozilla.org/mozilla-central/rev/702bca2e601f#l9.79
(Assignee)

Comment 13

2 years ago
Posted patch abspath4.diff (obsolete) — Splinter Review
(Assignee)

Comment 14

2 years ago
Landed and tagged.
Retriggered all windows update verify tests + 1 linux + 1 mac.
(Assignee)

Comment 16

2 years ago
I failed to push earlier.  Once I did push, I retriggered windows and 1 linux and 1 mac.

The linux and mac failed -- hangs.
Windows failed:
../common/check_updates.sh: line 76: \\c\\builds\\moz2_slave\\rel-m-beta_fx_w32_u_v-00000000\\scripts\\release\\updates\\update\: No such file or directory

I think we need to back this out and find another fix.
(Assignee)

Comment 17

2 years ago
https://hg.mozilla.org/build/tools/rev/64973b395ed7336acdd15624c8aea53959174538
bug 1363334 - back out d7ab9245abc4 and 5792f31efcba. r=bustage
(Assignee)

Comment 18

2 years ago
mtabara - if you want to take a stab at it, osx and linux are working but windows isn't, and the windows patch breaks osx and linux =\  I think we can fix it via bash scripting, but it may be tricky.
Flags: needinfo?(mtabara)
(Reporter)

Comment 19

2 years ago
(In reply to Aki Sasaki [:aki] from comment #18)
> mtabara - if you want to take a stab at it, osx and linux are working but
> windows isn't, and the windows patch breaks osx and linux =\  I think we can
> fix it via bash scripting, but it may be tricky.

Sounds good, will have a look. Thanks for all the hard work so far!
Flags: needinfo?(mtabara)
(Assignee)

Comment 20

2 years ago
:rstrong - do you have any pointers here?  abspath fixes linux/mac update tests.  Windows is breaking, and there isn't any logging about what specifically is breaking.

I see two current problems with my windows patch [1]:  a) I should probably change /c/... to c:/... before translating slashes to backslashes, and b) i'm referencing $updater_bin before we define it.  I'm not sure what's up with the last \: in comment 16.

[1] https://bugzilla.mozilla.org/attachment.cgi?id=8866595
Flags: needinfo?(robert.strong.bugs)
(In reply to Aki Sasaki [:aki] from comment #20)
> :rstrong - do you have any pointers here?  abspath fixes linux/mac update
> tests.  Windows is breaking, and there isn't any logging about what
> specifically is breaking.
Logging won't happen unless the path to the updates dir (1st argument) passes validation since that is where the log is written to.

> 
> I see two current problems with my windows patch [1]:  a) I should probably
> change /c/... to c:/... before translating slashes to backslashes, and b)
> i'm referencing $updater_bin before we define it.  I'm not sure what's up
> with the last \: in comment 16.
> 
> [1] https://bugzilla.mozilla.org/attachment.cgi?id=8866595
The paths must be full paths and use platform convention so windows will expect it in the format of c:\<path> etc.
A trailing backslash or forward slash should not be specified.
If the maintenance service is used the path to the updates directory (1st argument) should end with \updates\0

It looks like the error in comment #16 related to the last \: comes from the script and not app update so I'm not sure what is going on with it.
Flags: needinfo?(robert.strong.bugs)
(Assignee)

Comment 22

2 years ago
Posted patch abspath5.diff (obsolete) — Splinter Review
Fixes two things:

- changes /c/path/to/pwd to c:\\path\\to\\pwd
- defines the updater_abspath *after* we define updater_bin
Attachment #8866595 - Attachment is obsolete: true
Attachment #8866900 - Flags: review?(nthomas)
(Assignee)

Updated

2 years ago
Assignee: nobody → aki
(Assignee)

Comment 23

2 years ago
Posted patch abspath6.diff (obsolete) — Splinter Review
Slightly less confusing regex, since we're using comma delimiters.  Both work.

abspath{5,6}.diff also theoretically fix the linux+mac bustage in abspath4.diff.
Attachment #8866900 - Attachment is obsolete: true
Attachment #8866900 - Flags: review?(nthomas)
Attachment #8866902 - Flags: review?(nthomas)
(Assignee)

Comment 24

2 years ago
Comment on attachment 8866902 [details] [diff] [review]
abspath6.diff

I need to fix the actual commands =\  Sorry for the noise
Attachment #8866902 - Attachment is obsolete: true
Attachment #8866902 - Flags: review?(nthomas)
(Assignee)

Comment 25

2 years ago
Posted patch abspath7.diff (obsolete) — Splinter Review
Attachment #8866904 - Flags: review?(nthomas)
(Assignee)

Comment 26

2 years ago
Now that I look at it I'm wondering if we need another layer of backslashes, but this is worth a shot.
(Assignee)

Comment 27

2 years ago
Comment on attachment 8866904 [details] [diff] [review]
abspath7.diff

Nope, this'll break. Sorry sorry
Attachment #8866904 - Attachment is obsolete: true
Attachment #8866904 - Flags: review?(nthomas)
(Assignee)

Comment 28

2 years ago
Posted file test.sh
script i'm using to test levels of backslashes.

Output ends in

+ echo 'c:\\foo\\bar\\baz'
c:\foo\bar\baz
+ echo 'c:\\foo\\bar\\baz\\blah'
c:\foo\bar\baz\blah

so I need to double the number of backslashes in the non-sed lines.
(Assignee)

Comment 29

2 years ago
Posted patch abspath8.diff (obsolete) — Splinter Review
8th time's the charm?
Attachment #8866907 - Flags: review?(nthomas)
Comment on attachment 8866907 [details] [diff] [review]
abspath8.diff

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

I gave this a try on a windows worker and it still complained about the PATCH directory c:\\slave\\.... being invalid. My hand-waving theory for that is that we have to make calls like 
  c:\\\\slave\\\\\.... c:\\slave\\...
Wasn't able to verify that as the worker rebooted from under me.

::: release/common/check_updates.sh
@@ +46,5 @@
>    if [ -f update/update.status ]; then rm update/update.status; fi
>    if [ -f update/update.log ]; then rm update/update.log; fi
>  
>    if [ -d source/$platform_dirname ]; then
> +    if [ ".$is_windows" = "."] ; then

Need a space before the ] to avoid a bash error.

@@ +68,5 @@
>              break
>          fi
>      done
> +    # we need to define updater_abspath after defining updater_bin
> +    if [ ".$is_windows" = "."] ; then

Need a space before the ] to avoid a bash error.
Attachment #8866907 - Flags: review?(nthomas) → review-
(Assignee)

Comment 31

2 years ago
Posted patch abspath9.diff (obsolete) — Splinter Review
Attachment #8866907 - Attachment is obsolete: true
(Assignee)

Comment 32

2 years ago
(Assignee)

Updated

2 years ago
Attachment #8866996 - Attachment is obsolete: true
Comment on attachment 8867002 [details] [diff] [review]
abspath10.diff

Let's give it a go.
Attachment #8867002 - Flags: review+
(Assignee)

Comment 34

2 years ago
https://hg.mozilla.org/build/tools/rev/7a7a06cb6c1d7edbc23ef37ce90de30d89a791e0
bug 1363334 - trying 4 backslash 2 backslash 2 backslash 2 r=nthomas
(Assignee)

Comment 35

2 years ago
17:45 <@aki> \o/ windows dies with FAIL: no <update/> found for https://aus5.mozilla.org/update/3/Firefox/54.0/20170508140042/WINNT_x86-msvc-x86/ast/beta-localtest/default/default/default/update.xml?force=1 instead of path issues

I think we retag the beta7 tag and re-run, and we're good.
(Assignee)

Comment 36

2 years ago
Beta7 mac update verify is failing, and I suspect the new certs.
(Assignee)

Comment 37

2 years ago
01:38 <@aki-away> update_status is 9
01:38 <@aki-away> instead of "succeeded"
01:39 <@aki-away> https://hg.mozilla.org/build/tools/file/tip/release/common/check_updates.sh#l96
01:42 <@aki-away> 9 might be https://hg.mozilla.org/mozilla-central/file/tip/toolkit/mozapps/update/common/errors.h#l25

:rstrong, does this mean we have problems with the new mac release cert?  Or is this a problem with our tests?
Flags: needinfo?(robert.strong.bugs)
An update status of 9 is ELEVATION_CANCELED
https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/common/errors.h#25

For Mac that is written here
https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/updater/updater.cpp#2605

I don't think that would be due to the new certs but I'm not certain. Are nightly builds using the new cert now? If so, I can verify.
Flags: needinfo?(robert.strong.bugs) → needinfo?(aki)
Since you have the status I assume you also have the log. Go ahead and attach it to this bug,
(Reporter)

Comment 40

2 years ago
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #39)
> Since you have the status I assume you also have the log. Go ahead and
> attach it to this bug,

One example here[1] from `macosx64 beta update verification 1/12`[2]

[1]: https://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-beta-macosx64/release-mozilla-beta_firefox_macosx64_update_verify-bm84-build1-build382.txt.gz
[2]: https://tools.taskcluster.net/task-group-inspector/#/t56GyORJTkyH3bF9icg-Dw/pjMWxhQtQ9eQlOuDhriLVw?_k=z6ibcb
5/10 Nightly elevates properly on Mac so as long as the new certificates were applied the same way on Beta as they were on Nightly I don't think it is the certificates and chances are it is the tests. The update.log file that is alongside the update.status should provide some insight as to what is going on. If the app is launched by your test or otherwise the log file could be moved to the parent directory and renamed to last-update.log.
I forgot that the log file isn't written to when an elevation request is cancelled.

I wasn't able to find the command line used to launch org.mozilla.updater in the link to the log. Can you provide that?
It appears that org.mozilla.updater is trying to elevate and that could be caused by the provided install directory path not passing the write access check due to the path provided.
(Reporter)

Comment 44

2 years ago
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #42)
> I forgot that the log file isn't written to when an elevation request is
> cancelled.
> 
> I wasn't able to find the command line used to launch org.mozilla.updater in
> the link to the log. Can you provide that?

Hm, not sure where to grab that to be honest. Let me ping Rail for some help.
:rail - where should I look for that?
Flags: needinfo?(rail)
(Reporter)

Comment 45

2 years ago
QE just confirmed that updating works smoothly for 54.0b7, I quote: "we’ve finished running ondemand update tests on the beta-cdntest channel, and all tests have passed (including the 27 updates on OS X 10.9, 10.10, and 10.11). The tests cover both the Direct and Fallback Update scenarios, and are very similar to how a real user would update."

So most likely the culprit lies within our tests.
Hmm, it doesn't look like we print the command in the logs.

maybe https://dxr.mozilla.org/build-central/source/tools/release/common/check_updates.sh#83 would help? Or we can enable printing (set -x?) and rerun a test to see what is going on.
Flags: needinfo?(rail)
(Assignee)

Comment 48

2 years ago
/builds/slave/rel-m-beta_fx_m64_u_v-00000000/scripts/release/updates/update/org.mozilla.updater /builds/slave/rel-m-beta_fx_m64_u_v-00000000/scripts/release/updates/update '/builds/slave/rel-m-beta_fx_m64_u_v-00000000/scripts/release/updates/source/*.app' '/builds/slave/rel-m-beta_fx_m64_u_v-00000000/scripts/release/updates/source/*.app' 0

hm, do we want single-quoted wildcards?
Flags: needinfo?(aki)
(Assignee)

Comment 49

2 years ago
Posted patch mac_fixSplinter Review
worth a shot?
Attachment #8867283 - Flags: review?(rail)

Updated

2 years ago
Attachment #8867283 - Flags: review?(rail) → review+
(Assignee)

Comment 50

2 years ago
https://hg.mozilla.org/build/tools/rev/8e3a24d5b89cec312795b164c8e28bb206d975a5
bug 1363334 - use ls to expand *.app in mac update tests. r=rail
(Assignee)

Updated

2 years ago
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.