Closed
Bug 1309125
Opened 8 years ago
Closed 8 years ago
Allow configuration of the download throttle via the update.xml
Categories
(Toolkit :: Application Update, defect, P2)
Toolkit
Application Update
Tracking
()
RESOLVED
FIXED
mozilla52
People
(Reporter: robert.strong.bugs, Assigned: robert.strong.bugs)
References
Details
Attachments
(5 files, 5 obsolete files)
4.72 KB,
patch
|
robert.strong.bugs
:
review+
ritu
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
13.83 KB,
patch
|
robert.strong.bugs
:
review+
|
Details | Diff | Splinter Review |
4.97 KB,
patch
|
robert.strong.bugs
:
review+
ritu
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
15.15 KB,
patch
|
Details | Diff | Splinter Review | |
15.95 KB,
patch
|
robert.strong.bugs
:
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.
Assignee | ||
Comment 1•8 years ago
|
||
Bug 1309127 is also of interest though it is not a dependency.
Assignee | ||
Updated•8 years ago
|
Priority: -- → P2
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → robert.strong.bugs
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•8 years ago
|
||
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.
Assignee | ||
Comment 3•8 years ago
|
||
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)
Assignee | ||
Comment 4•8 years ago
|
||
Attachment #8800102 -
Attachment is obsolete: true
Assignee | ||
Comment 5•8 years ago
|
||
Try push https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7a88aab0cea5cc4445098891765115c937b1126
Assignee | ||
Comment 6•8 years ago
|
||
The patch to change the interval will be a Firefox patch to change the preference.
Comment 7•8 years ago
|
||
(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)
Assignee | ||
Comment 8•8 years ago
|
||
(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.
Assignee | ||
Comment 9•8 years ago
|
||
Comment on attachment 8800141 [details] [diff] [review] patch - client code rev1 Try run looks good
Attachment #8800141 -
Flags: review?(mhowell)
Assignee | ||
Updated•8 years ago
|
Attachment #8800142 -
Flags: review?(mhowell)
Comment 10•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #8800142 -
Flags: review?(mhowell) → review+
Assignee | ||
Comment 11•8 years ago
|
||
(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!
Assignee | ||
Comment 12•8 years ago
|
||
comment fixed
Attachment #8800141 -
Attachment is obsolete: true
Attachment #8800346 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
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
Assignee | ||
Updated•8 years ago
|
Summary: Allow configuration of the throttle via the update.xml → Allow configuration of the download throttle via the update.xml
Assignee | ||
Comment 13•8 years ago
|
||
Matt, I think this would be a little safer for limiting the maximum interval
Attachment #8800352 -
Flags: review?(mhowell)
Updated•8 years ago
|
Attachment #8800352 -
Flags: review?(mhowell) → review+
Assignee | ||
Comment 14•8 years ago
|
||
Attachment #8800346 -
Attachment is obsolete: true
Attachment #8800352 -
Attachment is obsolete: true
Attachment #8800360 -
Flags: review+
Assignee | ||
Comment 15•8 years ago
|
||
Attachment #8800142 -
Attachment is obsolete: true
Attachment #8800361 -
Flags: review+
Comment 16•8 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
Comment 17•8 years ago
|
||
(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.
Assignee | ||
Comment 18•8 years ago
|
||
[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.
status-firefox50:
--- → affected
status-firefox51:
--- → affected
tracking-firefox50:
--- → ?
tracking-firefox51:
--- → ?
Assignee | ||
Updated•8 years ago
|
status-firefox49:
--- → wontfix
status-firefox52:
--- → affected
status-firefox-esr45:
--- → wontfix
Comment 19•8 years ago
|
||
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)
Assignee | ||
Comment 20•8 years ago
|
||
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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3f592df38fe2 https://hg.mozilla.org/mozilla-central/rev/9837e6ec214e
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Comment 23•8 years ago
|
||
(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)
Assignee | ||
Comment 24•8 years ago
|
||
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?
Assignee | ||
Comment 25•8 years ago
|
||
Attachment #8801864 -
Flags: review+
Assignee | ||
Comment 26•8 years ago
|
||
Assignee | ||
Comment 27•8 years ago
|
||
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+
Assignee | ||
Comment 31•8 years ago
|
||
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.
Assignee | ||
Comment 32•8 years ago
|
||
Pushed to mozilla-aurora https://hg.mozilla.org/releases/mozilla-aurora/rev/9a7b518552dda0626b87e9ae5974c7e81d9add0e https://hg.mozilla.org/releases/mozilla-aurora/rev/f1e1d246b097319bcf0ac011ec6122516eaf08a8
(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)
Assignee | ||
Comment 34•8 years ago
|
||
Yes and I'll land it after I test locally
Flags: needinfo?(robert.strong.bugs)
Assignee | ||
Comment 35•8 years ago
|
||
Pushed to mozilla-beta https://hg.mozilla.org/releases/mozilla-beta/rev/4d6033fadbc589a5f6170803e6af65ed8de5c96d https://hg.mozilla.org/releases/mozilla-beta/rev/5f2813aee171e56a58fdc136a7024a4278fc3767
Assignee | ||
Comment 37•7 years ago
|
||
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.
Description
•