Allow configuration of the download throttle via the update.xml

RESOLVED FIXED in Firefox 50

Status

()

defect
P2
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: rstrong, Assigned: rstrong)

Tracking

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 wontfix, firefox-esr45 wontfix, firefox50+ fixed, firefox51+ fixed, firefox52 fixed)

Details

Attachments

(5 attachments, 5 obsolete attachments)

4.72 KB, patch
rstrong
: review+
Details | Diff | Splinter Review
13.83 KB, patch
rstrong
: review+
Details | Diff | Splinter Review
4.97 KB, patch
rstrong
: review+
Details | Diff | Splinter Review
15.15 KB, patch
Details | Diff | Splinter Review
15.95 KB, patch
rstrong
: review+
Details | Diff | Splinter Review
The throttling was introduced when the mirror network went down and now that we have the CDN it should be possible to download full speed. In case this does cause problems it should be possible to throttle the download based on the update.xml.
Bug 1309127 is also of interest though it is not a dependency.
Assignee: nobody → robert.strong.bugs
Status: NEW → ASSIGNED
I think this should be uplifted so this is a simplified patch. I'll file a new bug to make the additional changes for nightly after this bakes on nightly.
Ben and Nick, to safely go with unthrottled background downloads on the client we are going to make it so balrog can control the interval between downloading chunks. That way if there are any issues with the CDN it will be possible to go back to the way it was with a new property in the update.xml. I will likely go with the name backgroundInterval.
Flags: needinfo?(nthomas)
Flags: needinfo?(bhearsum)
The patch to change the interval will be a Firefox patch to change the preference.
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #3)
> Ben and Nick, to safely go with unthrottled background downloads on the
> client we are going to make it so balrog can control the interval between
> downloading chunks. That way if there are any issues with the CDN it will be
> possible to go back to the way it was with a new property in the update.xml.
> I will likely go with the name backgroundInterval.

I don't see why this would be an issue. It looks like it will be very similar to promptWaitTime in that it lives on the <update> line, and is optional?
Flags: needinfo?(bhearsum)
(In reply to Ben Hearsum (:bhearsum) from comment #7)
> I don't see why this would be an issue. It looks like it will be very
> similar to promptWaitTime in that it lives on the <update> line, and is
> optional?

That is correct. I'd like to get this uplifted to beta this cycle and it would be good to be able to set this in balrog if we ever need to fallback to the old behavior.
Comment on attachment 8800141 [details] [diff] [review]
patch - client code rev1

Try run looks good
Attachment #8800141 - Flags: review?(mhowell)
Comment on attachment 8800141 [details] [diff] [review]
patch - client code rev1

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

r+ with one minor issue to fix.

::: toolkit/mozapps/update/nsUpdateService.js
@@ +1527,5 @@
>    this.promptWaitTime = getPref("getIntPref", PREF_APP_UPDATE_PROMPTWAITTIME, 43200);
> +  let interval = getPref("getIntPref", PREF_APP_UPDATE_BACKGROUNDINTERVAL,
> +                          DOWNLOAD_BACKGROUND_INTERVAL);
> +  // Don't allow the interval from preferences to be greater than 10 minutes.
> +  this.backgroundInterval = Math.min(interval, 6000);

This is in seconds, right? So 600, not 6000.
Attachment #8800141 - Flags: review?(mhowell) → review+
Attachment #8800142 - Flags: review?(mhowell) → review+
(In reply to Matt Howell [:mhowell] from comment #10)
> Comment on attachment 8800141 [details] [diff] [review]
> patch - client code rev1
> 
> Review of attachment 8800141 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with one minor issue to fix.
> 
> ::: toolkit/mozapps/update/nsUpdateService.js
> @@ +1527,5 @@
> >    this.promptWaitTime = getPref("getIntPref", PREF_APP_UPDATE_PROMPTWAITTIME, 43200);
> > +  let interval = getPref("getIntPref", PREF_APP_UPDATE_BACKGROUNDINTERVAL,
> > +                          DOWNLOAD_BACKGROUND_INTERVAL);
> > +  // Don't allow the interval from preferences to be greater than 10 minutes.
> > +  this.backgroundInterval = Math.min(interval, 6000);
> 
> This is in seconds, right? So 600, not 6000.
Damn... thanks for catching that!
Posted patch patch - client code rev2 (obsolete) — Splinter Review
comment fixed
Attachment #8800141 - Attachment is obsolete: true
Attachment #8800346 - Flags: review+
Summary: Don't throttle the mar download by default and allow configuration of the throttle via the update.xml → Allow configuration of the throttle via the update.xml
Summary: Allow configuration of the throttle via the update.xml → Allow configuration of the download throttle via the update.xml
Posted patch _meh (obsolete) — Splinter Review
Matt, I think this would be a little safer for limiting the maximum interval
Attachment #8800352 - Flags: review?(mhowell)
Attachment #8800352 - Flags: review?(mhowell) → review+
Attachment #8800346 - Attachment is obsolete: true
Attachment #8800352 - Attachment is obsolete: true
Attachment #8800360 - Flags: review+

Comment 16

3 years ago
Pushed by rstrong@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f592df38fe2
Client code for Bug 1309125 - Allow configuration of the download throttle via the update.xml. r=mhowell
https://hg.mozilla.org/integration/mozilla-inbound/rev/9837e6ec214e
Test code for Bug 1309125 - Allow configuration of the download throttle via the update.xml. r=mhowell
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #8)
> (In reply to Ben Hearsum (:bhearsum) from comment #7)
> > I don't see why this would be an issue. It looks like it will be very
> > similar to promptWaitTime in that it lives on the <update> line, and is
> > optional?
> 
> That is correct. I'd like to get this uplifted to beta this cycle and it
> would be good to be able to set this in balrog if we ever need to fallback
> to the old behavior.

I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1309660 for the Balrog side.
[Tracking Requested - why for this release]:
One of the issues found in bug 1284915 which affects update orphaning is that throttling the mar download allows the client more time to experience an error that causes the download to start from the beginning. This bug along with bug 1309668 (a simple pref change) will make it so we don't throttle the mar download by default and if there are problems the throttle can be put back in place via the update update xml file served to clients.
oremj - with this change, Firefox users on the beta and release channels will download updates at full speed, rather than 300000 bytes every 60 seconds. Bug 1309660 will allow us to add a delay back in, eg if we have issues with CDN bandwidth, but only for Firefox users which make updates requests after we make a config change in Balrog. Are you OK with this approach on behalf of CloudOps ?
Flags: needinfo?(nthomas) → needinfo?(oremj)
Note that I did discuss this with someone (sorry but I forgot which person) that works with the CDN at the last all hands. Having said that, thanks for looping in oremj.

Comment 21

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3f592df38fe2
https://hg.mozilla.org/mozilla-central/rev/9837e6ec214e
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52

Updated

3 years ago
Depends on: 1309961
Adding tracking for 51, same as bug 1309668
(In reply to Nick Thomas [:nthomas] from comment #19)
> oremj - with this change, Firefox users on the beta and release channels
> will download updates at full speed, rather than 300000 bytes every 60
> seconds. Bug 1309660 will allow us to add a delay back in, eg if we have
> issues with CDN bandwidth, but only for Firefox users which make updates
> requests after we make a config change in Balrog. Are you OK with this
> approach on behalf of CloudOps ?

Yeah, this should be fine. It's unlikely we will need to throttle requests to our CDNs, but it is reassuring that a throttling mechanism is still available via balrog.
Flags: needinfo?(oremj)
Comment on attachment 8800360 [details] [diff] [review]
patch - client code rev3

Approval Request Comment
[Feature/regressing bug #]: Part of the future mitigation strategy for bug 1284915 coment #55
[User impact if declined]: This provides the ability to download updates at full speed safely ( see bug 1309668 ) since this allows us to change the interval via balrog back to what it was before the change.
[Describe test coverage new/current, TreeHerder]: there are in tree tests.
[Risks and why]: Minimal since there are several fallbacks.
[String/UUID change made/needed]: None
Attachment #8800360 - Flags: approval-mozilla-aurora?
Comment on attachment 8801864 [details] [diff] [review]
patch for beta

Note: for the patch in bug 1309668 to be uplifted we want this patch to mitigate any problems in the unlikely event that they occur with the CDN.

Approval Request Comment
[Feature/regressing bug #]: Part of the future mitigation strategy for bug 1284915 coment #55
[User impact if declined]: This provides the ability to download updates at full speed safely ( see bug 1309668 ) since this allows us to change the interval via balrog back to what it was before the change.
[Describe test coverage new/current, TreeHerder]: there are in tree tests.
[Risks and why]: Minimal since there are several fallbacks.
[String/UUID change made/needed]: None
Attachment #8801864 - Flags: approval-mozilla-beta?
Comment on attachment 8801864 [details] [diff] [review]
patch for beta

This prevents update orphaning, Beta50+
Attachment #8801864 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Hi Rob, should we also uplift to Aurora51?
Flags: needinfo?(robert.strong.bugs)
Flags: needinfo?(robert.strong.bugs)
Comment on attachment 8800360 [details] [diff] [review]
patch - client code rev3

Aurora51+
Attachment #8800360 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Yes and the approval request for aurora is on the original patch since it applies cleanly to aurora.

(In reply to Ritu Kothari (:ritu) from comment #28)
> Comment on attachment 8801864 [details] [diff] [review]
> patch for beta
> 
> This prevents update orphaning, Beta50+
Note: this should lessen update orphaning.
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #26)
> Created attachment 8801867 [details] [diff] [review]
> patch tests with fix for bug 1309961

Is this supposed to land with the beta patch? I'm hitting conflicts trying to apply this patch.
Flags: needinfo?(robert.strong.bugs)
Yes and I'll land it after I test locally
Flags: needinfo?(robert.strong.bugs)
Ritu, this is a heads up for release drivers that this bug and bug 1309668 make it so that updates should download faster and that this will be seen when updating from Firefox 50 or greater to a newer version.
Flags: needinfo?(rkothari)
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #37)
> Ritu, this is a heads up for release drivers that this bug and bug 1309668
> make it so that updates should download faster and that this will be seen
> when updating from Firefox 50 or greater to a newer version.

Got it! Thanks for letting me know. I will also share this with other folks in my team who will look for any crash spikes after the release updates are turned on at 25% on go-live day.
Flags: needinfo?(rkothari)
You need to log in before you can comment on or make changes to this bug.