Running my software update test for Mozmill for Firefox 3.5.3 on beta channel gives me some strange output for the different upgrade paths. While running a complete update the update service always returns a partial one. With the help of Robert, who pointed me to the update snippet below, we were able to identify the problem: https://aus2.mozilla.org/update/3/Firefox/3.5/20090624025744/WINNT_x86-msvc/en-US/release/Windows_NT%206.0/default/default/update.xml?force=1 A complete and partial update is offered for the channel. Both updates have exactly the same information. This is bogus. Please remove the partial entry from the snippet for a complete update.

Further it would be great if a test could be implemented on releng side which let us stop adding such bogus entries.

The update generator for releases always provides a partial and complete update. For all-but-the most-recent release we don't have a genuine partial and so we use the complete update for both - eg 3.5.3 is coming up soon and there will be a partial from 3.5.2 with fallback to complete, while 3.5.1 and older be offered a complete update for both. This is what we've done since the updater was made useful in Firefox 1.5, and I have a vague memory it was required to work around a bug in the updater at that time. It now results in confusing MozMill output. Note that for older nightlies only a complete is provided, so presumably that works just fine.

If you recall / find the bug where this was necessary please let me know. I suspect it was before I started working on app update and the only reason I can think of where this might fix something was the bug with file permissions.

The code that generates the identical partial & complete is at http://mxr.mozilla.org/seamonkey/source/tools/patcher/patcher2.pl#1104 That comment was added by preed on 10-Jul-06 (when this file was in a different source repo), so it was possibly needed for Firefox 1.5.0.5.

(In reply to comment #2) > This is what we've done since the updater was made useful in Firefox 1.5, and I > have a vague memory it was required to work around a bug in the updater at that > time. That is exactly why that code was written. We've been searching for a bug on this for a couple of days, and I'm honestly not sure that one was ever filed. Rob: As I remember, the problem had to do with the state flow in the app update process, something about the app getting into an inconsistent state if there weren't two updates to try (because there was some assumption there would be?) It may have had to do with getting into a loop if there was only a complete update offered but that complete update failed to apply for some reason. I would recommend some testing around that particular use case before reverting this "bogus" code.

I'll double check it. I do know there have been several times in the past when QA has filed bugs or talked with me about app update not offering a complete when testing a failed partial which app update handles properly by not offering the complete when the partial is the same as the complete.

I was able to get the updater into a hung state with a malformed mar file offered by complete only, I can put that up somewhere if it'd be useful. The update failed to apply then Shiretoko just sat there trying to contact the download server. Which makes me wonder what happens if you take an two-day old nightly and make the complete to fail somehow. I tried setting a platform.ini read-only but the rw permissions on the parent directory allowed the file to be renamed and deleted (OSX).

I modified the mar after download to force a mar apply failure (not a hang) and was offered to download a complete so there is definitely a bug in the update service. I haven't checked if this also occurs when both a complete and a partial are in the update snippet and would appreciate feedback on this. Note that the hash check of the mar provides protection against mar file corruption during the download so the only cases where this would happen are when the generated mar file is corrupt or if something modifies the mar file after it has been downloaded. I noticed that the following snippet doesn't have a partial and a complete that both specify the same mar. https://aus2.mozilla.org/update/3/Firefox/3.5.4pre/20090830042116/WINNT_x86-msvc/en-US/nightly/Windows_NT%206.0/default/default/update.xml?force=1

(In reply to comment #8) > I modified the mar after download to force a mar apply failure (not a hang) and > was offered to download a complete so there is definitely a bug in the update > service. I haven't checked if this also occurs when both a complete and a > partial are in the update snippet and would appreciate feedback on this. So I guess this bug comes down to how often complete updates fail for end-users, since it appears that the updater will get into a confused state if we offer only a complete and that fails. I forget we are with the file-in-use problem but that seems like a likely candidate. IIRC there are a reasonable number of people using releases that end up in update loops, where both the partial and complete fail to apply. > I noticed that the following snippet doesn't have a partial and a complete that > both specify the same mar. > https://aus2.mozilla.org/update/3/Firefox/3.5.4pre/20090830042116/WINNT_x86-msvc/en-US/nightly/Windows_NT%206.0/default/default/update.xml?force=1 That's right, the nightly update system has offered a complete-only for a very long time, if you're more than one build behind.

I think this is a Future goal. We've lived with this setup for years and years now.

Will require some rewriting in patcher, with a configuration switch as bug 538533 may not land on some older branches.

Mass move of bugs from Release Engineering:Future -> Release Engineering. See http://coop.deadsquid.com/2010/02/kiss-the-future-goodbye/ for more details.

With the fixes on bug 538533 and other other update dialog changes we have a real problem on trunk with our Mozmill update tests now. Because of this bug the tests do not pass anymore for complete fallback updates. There is an inconsistency between the updates offered and the ui behavior. As Rob said on bug 567258 comment 34 we should really get this bug fixed, without having to add more and more workarounds on the client or testing side. QA really doesn't want to run manual tests anymore. So can we please get an ETA for a fix?

> QA really doesn't want to run manual tests anymore. Speaking to this point, I'd like to throw a time comparison out there... With Mozmill, we can check approximately 30 update paths (3 builds using 2 locales across 5 platforms) in 30 minutes per channel. Manually, in the same amount of time, we only complete 5 updates paths (3 builds using 2 locales across a single platform).

Sounds like we'll need to fix this before 4.0b2, if the changes have already landed on trunk.

I spoke with Henrik and Rob about this yesterday. In summary: Offering a <partial> which is the same as the <complete> could possibly affect a real life situation, but it is unlikely. Mozmill currently uses size-of-mar to differentiate between a partial and complete update, because it doesn't have access to the updates.xml file when it needs this information. For this reason, we need to stop offering a <partial> block when it matches the <complete>. Henrik, is that an accurate summary?

That sounds perfect. Given such a change I would be able to trust the nsIUpdate.isCompleteUpdate attribute again and we can make sure that a) we are following the specs for updates and b) our automation can work without workarounds.

Ok, now that we're on the same page I'll see what I can do here. No promises that it'll be ready for b2, but I'm sure it'll make it for b3.

I generated a set of snippets on the 'nofakepartialtest' channel, with the patch applied. They look correct to me, and diff'ing them against the real ones is promising. Henrik, can you run Mozmill against them?

I run updates on OS X manually and automatically for a4 and a5 on the nofakepartialtest channel and it looks fantastic now. With a4 we are really using complete patches now. Here the results: * 3.7a4 => 4.0b1, minor, en-US, complete, nofakepartialtest, 2010-07-16, '''PASS''' ** Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a4) Gecko/20100407 MozillaDeveloperPreview/3.7a4 ID:20100407105356 ** Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:2.0b1) Gecko/20100630 Firefox/4.0b1 ID:20100630131607 ** Passed 14 :: Failed 0 :: Skipped 0 * 3.7a5 => 4.0b1, minor, en-US, partial, nofakepartialtest, 2010-07-16, '''PASS''' ** Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a5) Gecko/20100610 MozillaDeveloperPreview/3.7a5 ID:20100610053455 ** Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:2.0b1) Gecko/20100630 Firefox/4.0b1 ID:20100630131607 ** Passed 14 :: Failed 0 :: Skipped 0

Comment on attachment 457849 [details] [diff] [review] potential patch Based on my and Henrik's testing, this is ready for review. We'll need to create a new UPDATE_PACKAGING_R? tag after we land this, and have mozilla-central use it. When bug 576939 lands, we'll have to use it on 1.9.2, too. 1.9.1 won't get these fixes AFAIK, so we'll stay with UPDATE_PACKAGING_R11 there. If we happen to have a bug we need to fix, we can branch from that rev. I'm working on update verify improvements to go along with this still.

This patch makes update verify more versatile in how it selects which patch types it looks for. It defaults to the complete only, which means the n-2 and earlier releases look the same in the verify configs. The n-1 release needs patch_types="partial complete" set in the verify config. Still to come: An update-verify-bump.pl patch to do that.

This turned out a lot bigger than I'd hoped, but since we don't pull the update verify bumper from a tag we're forced to make both behaviours possible in the same script. The bumper script: I added --test-older-partials (better name wanted!) to the command line options. Anything that wants the old behaviour should pass this. Anything wanting the new behaviour shouldn't. There's an ugly hack for major updates around line 317 to make sure we don't try and test a partial there. If we ever ship partials for major updates we'll have to adjust that. The configs: For 1.9.1 and 1.9.2, which will be using the old behaviour, we need to set patch_types="partial complete" for all past releases. For 1.9.3/2.0, we have to set it for the most recent build in the config, otherwise the bumper will fail next time. I've done outside-of-Buildbot tests with this, which have worked. I'm not comfortable landing this until I do an end-to-end run, possibly 2, with it. I plan to do that on Monday. Next up, accompanying changes to Buildbot factories and configs.

Comment on attachment 457940 [details] [diff] [review] updated tools repo patch, includes config updates and bumper update >diff --git a/release/update-verify-bump.pl b/release/update-verify-bump.pl >+ --test-older-partials/-P When passed, all old release will be marked for I didn't think of a better (serious) name. > for(my $i=0; $i < scalar(@origFile); $i++) { > my $line = $origFile[$i]; >- $line =~ s/from.*$//; >+ my $removeStr = $testOlderPartials ? 'from' : 'patch_types'; >+ $line =~ s/$removeStr.*$//; > $strippedFile[$i] = $line; A comment would be useful here. >+ if ( ! $majorMode ) { >+ $data[1] .= ' patch_types="partial complete"' And here, if only because we're unlikely to find our way back to your good your good comments on this bug. >--- a/release/updates/moz193-firefox-linux.cfg >+release="3.7a5" product="Firefox" platform="Linux_x86-gcc3" build_id="20100610052642" locales="en-US" channel="betatest" patch_types="partial complete" from="/firefox/releases/devpreview/1.9.3a5/linux-i686/%locale%/mozilladeveloperpreview-3.7a5.tar.bz2" aus_server="https://aus2.mozilla.org" ftp_server="stage-old.mozilla.org/pub/mozilla.org" to="/firefox/nightly/4.0b1-candidates/build2/linux-i686/%locale%/firefox-4.0b1.tar.bz2" patch_types="partial complete" 'patch_types' gets added twice to all these moz193-firefox configs. Guess that's harmless since it's going to get dropped quickly by the 4.0b2 bump.

Comment on attachment 457849 [details] [diff] [review] potential patch >Index: patcher2.pl >+ #PrintProgress(total => $totalPastUpdates, >+ # current => ++$patchInfoFilesCreated, >+ # string => "$prettyPrefix/$fromAusPlatform/$locale/$channel/partial"); >+ #write_patch_info(patch => $completePatch, >+ # schemaVer => $patchLocaleNode->{'schema'}); > print("done\n"); Please comment out the print as well. r+ with that nit.

Comment on attachment 457947 [details] [diff] [review] --test-older-partials support in configs >diff --git a/mozilla2-staging/release_master.py b/mozilla2-staging/release_master.py > if majorUpdateRepoPath: > # Not attached to any Scheduler >- major_update_factory = MajorUpdateFactory( >+ major_update_factory = ajorUpdateFactory( Oops! And bump the patcher tag in these once that all lands.

Thanks for the speedy review, Nick. I'll get a run through staging done ASAP, but I'm not sure this will make it for b2.

Based on the fact that I haven't even been able to start a staging run yet I don't think this will make beta 2.

I don't see any justification for this being nominated as a blocker.

Testing on this is going well. I'm planning to ask for review on the remaining patches before the end of my day. Hope to get this landed this week.

Comment on attachment 457948 [details] [diff] [review] --test-older-partials support in custom This one is ready for review

Comment on attachment 457947 [details] [diff] [review] --test-older-partials support in configs This one too. For now, testOlderPartials is True for all 1.9.1 and 1.9.2 based release configs. When we do the first release with bug 576939 (1.9.2 only) we need to flip this to False. For trunk and mozilla-2.0 this remains False for the forseeable future.

Comment on attachment 457947 [details] [diff] [review] --test-older-partials support in configs So gozer should look at this, but I'd suggest you should add a comment to all the 1.9.2 configs to say when it should be flipped to false, so that people don't forget. And on that theme it would be good to add a comment explaining what the value is and why it never needs changing on some branches.

(In reply to comment #38) > Comment on attachment 457947 [details] [diff] [review] > --test-older-partials support in configs > > So gozer should look at this, but I'd suggest you should add a comment to all > the 1.9.2 configs to say when it should be flipped to false, so that people > don't forget. > > And on that theme it would be good to add a comment explaining what the value > is and why it never needs changing on some branches. Good idea -- I'll do both of those.

Only change here is the addition of comments. I'll be updating https://wiki.mozilla.org/Release:Release_Automation_on_Mercurial:Documentation with full documentation on the option.

There's a few changes in this patch: * s/--test-older-partials\|P/--test-older-partials/ -- because Perl complained that the "P" conflicted with the lowercase "p" for product. * Add comments as requested * Merge 1.9.2, 2.0 verify configs ** Gets rid of double patch_types in 1.9.3 configs With this and the other patches I did three updates test runs in staging. The first two were re-runs of 4.0b1 & 4.0b2. As expected, no partials were generated for n-2 builds and older. The 3rd test was a re-run of 3.5.10 updates. I diff'ed filenames against the real snippets, from aus2-staging, and there was no differences. I didn't verify snippet contents beyond glancing, but that and update verify looked fine. You can find diffs of the config bumps in http://hg.mozilla.org/users/stage-ffxbld/tools, and snippets in /opt/aus2/snippets/staging on staging-stage.b.m.o.

Comment on attachment 460668 [details] [diff] [review] updated configs patch w/ comments I'm not sure I have fully understood the difference yet, but I trust your description of what trees need what are correct. ;-)

I found the bug in the mozmill shared test module for app update and posted a patch to fix mozmill in bug 567258 so removing dependency. It would still be a good thing to fix this but mozmill doesn't depend on this and the client is fine as well... it just offers to download the complete which is the same as the partial as it has always done.

(In reply to comment #43) > I found the bug in the mozmill shared test module for app update and posted a > patch to fix mozmill in bug 567258 so removing dependency. > > It would still be a good thing to fix this but mozmill doesn't depend on this > and the client is fine as well... it just offers to download the complete which > is the same as the partial as it has always done. So...this means that there's no reason to stop offering partials which match the complete?

I will have to look at Robs patch and also test it. I will not be able to do that today. And I don't want to give an answer right away.

(In reply to comment #44) > (In reply to comment #43) > > I found the bug in the mozmill shared test module for app update and posted a > > patch to fix mozmill in bug 567258 so removing dependency. > > > > It would still be a good thing to fix this but mozmill doesn't depend on this > > and the client is fine as well... it just offers to download the complete which > > is the same as the partial as it has always done. > > So...this means that there's no reason to stop offering partials which match > the complete? My own opinion is that e shouldn't offer partials that are actually complete mars. If the client side fails on a partial that is actually a complete it will then downloads the same patch again and it will fail in the exact same way which I believe will be annoying to users.

Ben, I think just fixing this bug on trunk should suffice. It would be cool to wait a day or two for Henrik to review the mozmill patch though I am quite certain it fixes the problems mozmill has been having.

Nick, Gozer -- any idea when you'll be able to review these patches?

Comment on attachment 460668 [details] [diff] [review] updated configs patch w/ comments thunderbird/release_master.py needs patching to pass this new argument too, I believe. Otherwise, r=gozer

Comment on attachment 460752 [details] [diff] [review] updated tools patch >diff --git a/release/update-verify-bump.pl b/release/update-verify-bump.pl >+ # Bug 514040 changed the defautl behaviour of update verify to test only Nit, typo 'defautl' >+ # the complete MAR. Since we don't currently use partials in major update >+ # mode we can only override this for non-major update tests. >+ if ( ! $majorMode ) { >+ $data[1] .= ' patch_types="partial complete"' >+ } But we do give a partial and complete updates for MUs, at least we are for 3.5.11 & 3.0.19 -> to 3.6.8. Can you change the condition here ? It should depend on testOlderPartials shouldn't it ?

Comment on attachment 460668 [details] [diff] [review] updated configs patch w/ comments >diff --git a/mozilla2-staging/release-firefox-mozilla-1.9.2.py b/mozilla2-staging/release-firefox-mozilla-1.9.2.py >+# Needs to be flipped to False when bug 576939 lands. Update verify configs >+# need to be stripped of "patch_types" for n-2 and older releases as well. >+testOlderPartials = True I'm wondering if we should bother doing this for 1.9.2. Builds from 3.6.9 on will be OK with a complete-only snippet, but everything older will still have the bug which requires a partial and complete to be given, right ? I'm not sure how we would decide that we had enough users on 3.6.9 (or later) to use a complete-only snippet. Given Mozmill has not been sorted out I would suggest just setting it to True without the comment. Same for mozilla2/release-firefox-mozilla-1.9.2. >diff --git a/mozilla2-staging/release-firefox-mozilla-2.0.py b/mozilla2-staging/release-firefox-mozilla-2.0.py > patcherToolsTag = 'UPDATE_PACKAGING_R11' Why isn't patcherToolsTag isn't changing in the m-2.0 and m-c configs ? >diff --git a/mozilla2/release_master.py b/mozilla2/release_master.py >@@ -567,16 +568,17 @@ if majorUpdateRepoPath: > ausHost=branchConfig['aus2_host'], > ausServerUrl=ausServerUrl, > hgSshKey=hgSshKey, > hgUsername=hgUsername, > clobberURL=branchConfig['base_clobber_url'], > oldRepoPath=sourceRepoPath, > triggerSchedulers=['major_update_verify'], > releaseNotesUrl=majorUpdateReleaseNotesUrl, >+ testOlderPartials=False I don't follow this change, given we know we're updating from buggy clients on 3.5.x. And we'd end up using an older patcherToolsTag when generating a 3.5.x to 3.6.y MU (and hence get partial+complete snippets) and only test the complete path. Is that the point ?

Nick, all of your comments make sense. I'll fix up my patches and re-post. I'll update the other release_master.py's too, Gozer.

> >+ # the complete MAR. Since we don't currently use partials in major update > >+ # mode we can only override this for non-major update tests. > >+ if ( ! $majorMode ) { > >+ $data[1] .= ' patch_types="partial complete"' > >+ } > > But we do give a partial and complete updates for MUs, at least we are for > 3.5.11 & 3.0.19 -> to 3.6.8. Can you change the condition here ? It should > depend on testOlderPartials shouldn't it ? Not sure what got into my head here, you're completely right, though. (In reply to comment #51) > Comment on attachment 460668 [details] [diff] [review] > updated configs patch w/ comments > > >diff --git a/mozilla2-staging/release-firefox-mozilla-1.9.2.py b/mozilla2-staging/release-firefox-mozilla-1.9.2.py > >+# Needs to be flipped to False when bug 576939 lands. Update verify configs > >+# need to be stripped of "patch_types" for n-2 and older releases as well. > >+testOlderPartials = True > > I'm wondering if we should bother doing this for 1.9.2. Builds from 3.6.9 on > will be OK with a complete-only snippet, but everything older will still have > the bug which requires a partial and complete to be given, right ? I'm not sure > how we would decide that we had enough users on 3.6.9 (or later) to use a > complete-only snippet. Given Mozmill has not been sorted out I would suggest > just setting it to True without the comment. Same for > mozilla2/release-firefox-mozilla-1.9.2. Very good points. I spoke with Whimboo and he says that Mozmill should cope fine on 1.9.2 if we continue to serve the fake partials. > >diff --git a/mozilla2-staging/release-firefox-mozilla-2.0.py b/mozilla2-staging/release-firefox-mozilla-2.0.py > > patcherToolsTag = 'UPDATE_PACKAGING_R11' > > Why isn't patcherToolsTag isn't changing in the m-2.0 and m-c configs ? Oversight (In reply to comment #49) > Comment on attachment 460668 [details] [diff] [review] > updated configs patch w/ comments > > thunderbird/release_master.py needs patching to pass this new argument too, I > believe. Otherwise, r=gozer Fixed.

I didn't do any full test runs, I think the changes are simple enough not to warrant them. Doesn't affect snippet generation, in any case.

Comment on attachment 465705 [details] [diff] [review] updated build/tools patch >diff --git a/release/update-verify-bump.pl b/release/update-verify-bump.pl >+ # Bug 514040 changed the default behaviour of update verify to test only >+ # the complete MAR. This is a bit of an abuse of what testOlderPartials >+ # means, but we need a way to distinguish between major updates from >+ # branches which need a partial the same as the complete, and those that >+ # don't. (ASSUMPTION: ) Minor updates always have partials, so they >+ # need this regardless of branch. Just need to fix up the ASSUMPTION. The update verify configs got a quick skim.

Comment on attachment 465699 [details] [diff] [review] update buildbot-configs patch Looks fine, sorry about the rot.

Created UPDATE_PACKAGING_R12 in m-c. Landing the rest shortly.

Comment on attachment 457948 [details] [diff] [review] --test-older-partials support in custom changeset: 900:0ca63af1dac1

Comment on attachment 465699 [details] [diff] [review] update buildbot-configs patch changeset: 2875:433624398b4a

Comment on attachment 465705 [details] [diff] [review] updated build/tools patch changeset: 2878:55b30e93cdc2

Comment on attachment 457849 [details] [diff] [review] potential patch Checking in patcher2.pl; /cvsroot/mozilla/tools/patcher/patcher2.pl,v <-- patcher2.pl new revision: 1.39; previous revision: 1.38 done

cvs co -r UPDATE_PACKAGING_R11 mozilla/client.mk cd mozilla make -f client.mk MOZ_CO_PROJECT=all checkout cvs up -A tools/update-packaging cvs up -AdP tools/release cvs up -AdP tools/patcher cvs -q tag UPDATE_PACKAGING_R12 2>&1 | tee ../tag.log grep -v '^T' ../tag.log cvs tag -b UPDATE_PACKAGING_R12_minibranch client.mk cvs up -r UPDATE_PACKAGING_R12_minibranch client.mk edited client.mk, bumped to UPDATE_PACKAGING_R12 commited client.mk /cvsroot/mozilla/client.mk,v <-- client.mk new revision: 1.314.2.1.2.1.2.1.2.1.2.1.2.1.2.1.2.1.2.1.2.1.2.1.2.1.2.1; previous revision: 1.314.2.1.2.1.2.1.2.1.2.1.2.1.2.1.2.1.2.1.2.1.2.1.2.1 done foo-ix-blah:mozilla bhearsum$ cvs -q tag UPDATE_PACKAGING_R12 client.mk W client.mk : UPDATE_PACKAGING_R12 already exists on version 1.314.2.1.2.1.2.1.2.1.2.1.2.1.2.1.2.1.2.1.2.1.2.1.2.1 : NOT MOVING tag to version 1.314.2.1.2.1.2.1.2.1.2.1.2.1.2.1.2.1.2.1.2.1.2.1.2.1.2.1 foo-ix-blah:mozilla bhearsum$ cvs -q tag -F UPDATE_PACKAGING_R12 client.mk T client.mk As mentioned, I updated m-c with the new tag, like so: hg tag -r UPDATE_PACKAGING_R11 UPDATE_PACKAGING_R12 1.9.2 and 1.9.1 do not need it. 2.0 will pull it in in the next merge. We are all done here now. Thanks for your great reviews, Nick!

Comment on attachment 465705 [details] [diff] [review] updated build/tools patch I just found this patch in my 'hg out' queue. It only landed just now :-(

Looks fantastic! Verified by checking on beta channel for b5 -> b7. Only the complete update gets listed: https://aus2.mozilla.org/update/3/Firefox/4.0b5/20100831070010/Darwin_x86-gcc3-u-ppc-i386/en-US/beta/Darwin%2010.4.0/default/default/update.xml?force=1 Marking as verified fixed.