Closed Bug 1245941 Opened 8 years ago Closed 8 years ago

improve Balrog to handle multifile responses better

Categories

(Release Engineering Graveyard :: Applications: Balrog (backend), defect)

Product:
Release Engineering Graveyard
Component:
Applications: Balrog (backend)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: bhearsum, Assigned: vjoshi)

Details

Currently, Balrog's architecture requires a 1:1 rule:release mapping. Because some types of products (GMP currently, SystemAddons in the future) return multiple distinct files to the client, this means we end  up with one release for each possible combination of versions of those files. For example: we're currently shipping OpenH264 1.5.3 to all users but a different version of the CDM depending on your Firefox and Windows version. For each intersection of desired openh264 + CDM, we need a unique rule and release, which means we end up duplicating data into many release blobs and adding more rules than is necessary to handle something like OpenH264 (where everyone is just getting the same thing). This is going to get even worse when we add a third GMP plugin, and I suspect SystemAddons will end up with a lot more frequent updates as well.

This would be much easier to handle if we could keep all the data for a single thing (where "thing" would be a specific version of a set of bits, eg: OpenH264-1.5.3, CDM-v16) in one blob, and somehow allow a single update request to map to multiple blobs. A couple of ideas have been thrown out there:
* Map a single rule to multiple releases. This would let us easily grab multiple releases as part of a single request, but makes it harder to set things like throttle rate per-release.
* Match each update request to multiple rules. This would give us more flexibility in terms of matching, but it may cause increased complexity for humans parsing the rules.

There's probably other possibilities too.

Another thing worth considering is how what we do here may affect "property bags" (https://bugzilla.mozilla.org/show_bug.cgi?id=933152).
(In reply to Ben Hearsum (:bhearsum) from comment #0)
> * Match each update request to multiple rules. This would give us more
> flexibility in terms of matching, but it may cause increased complexity for
> humans parsing the rules.

I'd like to be quite careful about this. We may have to actually build the change and/or request simulator we've talked about several times, or some sort of test framework, to ensure we can verify changes.
(In reply to Nick Thomas [:nthomas] from comment #1)
> (In reply to Ben Hearsum (:bhearsum) from comment #0)
> > * Match each update request to multiple rules. This would give us more
> > flexibility in terms of matching, but it may cause increased complexity for
> > humans parsing the rules.
> 
> I'd like to be quite careful about this. We may have to actually build the
> change and/or request simulator we've talked about several times, or some
> sort of test framework, to ensure we can verify changes.

Yeah, definitely. One thing I was thinking might be useful is to try to recreate the current logic of the rules in a multi-matching world. That might give us a better idea of the pros and cons of both ways.
(In reply to Ben Hearsum (:bhearsum) from comment #2)
> (In reply to Nick Thomas [:nthomas] from comment #1)
> > (In reply to Ben Hearsum (:bhearsum) from comment #0)
> > > * Match each update request to multiple rules. This would give us more
> > > flexibility in terms of matching, but it may cause increased complexity for
> > > humans parsing the rules.
> > 
> > I'd like to be quite careful about this. We may have to actually build the
> > change and/or request simulator we've talked about several times, or some
> > sort of test framework, to ensure we can verify changes.
> 
> Yeah, definitely. One thing I was thinking might be useful is to try to
> recreate the current logic of the rules in a multi-matching world. That
> might give us a better idea of the pros and cons of both ways.

Sorry, just realized this comment was a bit of non-sequitar. What I meant to say was that I think it's worth looking at what the rules table might end up looking like in a multimatching world to see if it gets as crazy as our gut is suggesting.

I definitely agree that if we want to do something that makes it significantly harder for humans to reason about the rules, we need better tools and UI to assist us. I don't want to do anything that's going to increase the number of footguns we have.
I finally had a chance to start trying to draw out what the rules may look like for multimatching, and it became pretty clear right away that it's not really viable. For example, we currently have two rules (not including the channel-less one for really old OS') for the nightly channel. The higher priority one specifies a build target of "WINNT_x86-msvc-x64" and OS Version of "Windows_NT 5.2", and points at No-Update. The second one is the main nightly rule that points at Firefox-mozilla-central-nightly-latest. To write rules for this in multimatching, we'd have to specify a negative match for that build target and OS Version in the main update rule. This would get even worse for the release channel, where we have many matching rules, and the negative matching would need to cascade down.
It became clear pretty quickly that 1:M rule:release mapping really doesn't help simplify the GMP case. Doing that lets you reduce the number of releases you have, but not the number of rules. Without reducing the number of rules you don't help humans very much.

So, what if we took a new approach and mapped incoming products ("request products") to a list of products that we want to return ("response products"). Nick and I had a long discussion about this, and it this is how it might work:
* Upon getting a request, look up the list of response products based on the request product. For a request product of "Firefox" this would be "Firefox", for "GMP" it would be "OpenH264" and "EME".
* Generate the header of the XML (eg: <updates><addons> for GMP)
* For each response product:
** Evaluate the rules table as usual, filtering down to one matching rule
** Load the blob that the rule matches
** Generate the inner XML from the blob, append it to the existing XML
* Append the footer of the XML (eg: </addons></updates>)
* Return the fully formed XML

This means that most of the logic of the existing GMP rules ends up in response product-specific ones. Eg: Special cased OS' for EME are handled in rules that are "product: EME" instead of "product: GMP", which means we don't need extra copies of them when OpenH264 has different requirements. It also means that we will have truly product specific blobs like "EME-1.2" or "OpenH264-1.5.1". By extension, this means we have finer grained permissions. Rather than granting full "GMP" access, we can grant GMP plugin owners access to the specific plugins they own.

There's still some open questions here:
* How and where are product mappings stored?
** We don't want them hardcoded, they must be manageable through the UI
** Do we want the ability to change alias' based on the request? Eg: product: GMP, version <= 39.0 maps to EME only, but product: GMP, version >39.0 maps to EME + OpenH264
*** Could be an enhancement, can probably be handled through response product rules too
* What kind of UI enhancements do we need?
** Probably need to update rule/release filtering to match both request/response products. Eg: filtering on "GMP" will find you OpenH264 and EME releases or rules.
* How do we get the outer XML?
** If we require all products in a group to be the same blob type, you could pull it off any returned blob
** Another approach might be a two pass method:
*** Filter rules on request product (eg: GMP)
*** Grab blob the rule points to
*** Look up response products in it
*** Reevaluate rules table for each response product, get blobs
*** Use earlier logic to construct response, using the request blob for the outer xml and the response blobs for the inner
** This might solve the "how/where are mapping stored" issues as well



We need to think about this a bit more, but it feels like we're onto something good.
Varun is going to be taking a look at this. Thanks Varun!
Assignee: bhearsum → varunj.1011
Since this was originally filed, we've started using similar techniques for System Addon updates as we have for GMP. I spoke with Mark today about how this plan might work for them. The general idea will work, but System Addon updates work a touch different, so we may need follow-up work to make this totally viable for them, specifically:
* As of now, the plan is to only push known-good combinations of System Addons. Eg: we may test that Hello 2.0 and Pocket 3.0 work well together, but not test that Hello 2.0 and Pocket 2.0 work. At times where we ship new versions multiple System Addons at once, we may run into issues where we briefly return a bad combination of things. Something like bug 1141801 would probably solve this. In the meantime, we might be able to briefly shut off System Addon updates while we tweak all the individual rules, then turn them back on.

* This patch will make it possible from the server standpoint to have per-System Addon throttle rates, but we may not be able to use them unless we adjust the client to parse the response differently. Right now, the presence of one System Addon but abscence of another will cause Firefox to uninstall the missing one completely. We'd have to do dice rolls per Response product, so it would be quite common for only one System Addon to be returned. Depending what we want to do with the client, and whether or not individual throttle rates may cause untested combinations of System Addons to be returned, we may want to do System Addon throttling in the Request product rules (instead of response), at least temporarily.
So these are the parts this bug can be divided into, that'll probably be landed seperately:
1. Creating a new blob type for super-blobs (I've already started
exploring this)

2. Handling this blob specially:
We could handle the logic of creating the XML for the sub-rules inside the newly created blob type. This way, we wouldn't have to change anything in the existing code.
But it would be cleaner to have blobs that do not need to access the database, so while I will start implementing a blob that has access to the db, I will search for solutions that will remove this need.

3. Creating a UI to handle this. Some more brainstorming is needed before we finalize a way to complete this. Bug 1241760 is something that needs to be watched, since it could ease the creation of a new interface that shows our new, special rules.
Commit pushed to master at https://github.com/mozilla/balrog

https://github.com/mozilla/balrog/commit/93a6d3d5f30d9db4ccccb441e958953b6eb77c5e
bug 1245941: improve Balrog to handle multifile responses better (#77). r=bhearsum
(In reply to Ben Hearsum (:bhearsum) from comment #7)
> Since this was originally filed, we've started using similar techniques for
> System Addon updates as we have for GMP. I spoke with Mark today about how
> this plan might work for them. The general idea will work, but System Addon
> updates work a touch different, so we may need follow-up work to make this
> totally viable for them, specifically:
> * As of now, the plan is to only push known-good combinations of System
> Addons. Eg: we may test that Hello 2.0 and Pocket 3.0 work well together,
> but not test that Hello 2.0 and Pocket 2.0 work. At times where we ship new
> versions multiple System Addons at once, we may run into issues where we
> briefly return a bad combination of things. Something like bug 1141801 would
> probably solve this. In the meantime, we might be able to briefly shut off
> System Addon updates while we tweak all the individual rules, then turn them
> back on.
> 
> * This patch will make it possible from the server standpoint to have
> per-System Addon throttle rates, but we may not be able to use them unless
> we adjust the client to parse the response differently. Right now, the
> presence of one System Addon but abscence of another will cause Firefox to
> uninstall the missing one completely. We'd have to do dice rolls per
> Response product, so it would be quite common for only one System Addon to
> be returned. Depending what we want to do with the client, and whether or
> not individual throttle rates may cause untested combinations of System
> Addons to be returned, we may want to do System Addon throttling in the
> Request product rules (instead of response), at least temporarily.

Rhelmer and I talked a bit more about how this would work for System Addons. He emphasized that only fully tested combinations of addons be shipped out for the time being (this may change when everything is webext based), which means that per-addon throttles are an anti-feature for System Addons at the moment. Given that, I'm going to recommend that we stick with the current multi-addons blobs for System Addons for now.
I landed this to production today, and reworked the GMP rules to make use of it. Specifically, I:
* Created new blobs for each individual version of the CDM, OpenH264, and Widewine plugins.
* Created chains of rules for these products to handle their own version/platform requirements
* Created a new GMP rule that points at a Superblob, which has OpenH264, CDM, and Widevine listed as response products.

Before making the new GMP rule I created a rule for the fake "bhearsumtest" product that pointed at the superblob and compared all the URLs from https://wiki.mozilla.org/User:Bhearsum/GMP_Updates (plus a couple that should have plugins offered) vs. a version with "bhearsumtest" instead of "GMP". All responses were exactly the same, except for the ones with no plugins listed. The empty response has changed slightly, to:
 <?xml version="1.0"?>
 <updates>
-    <addons>
-    </addons>
 </updates>

...which I ran by rhelmer before pushing anything live, who said that's what the client tests expect as an empty response anyways - so this is an accidental impromevent!

As things stand now, all the rules on the "GMP" product except for the one I just created are effectively dead. I'm going to leave them in place for a day or two just in case something goes wrong and we need to revert. If things are still looking good on Wednesday I will delete them.

If this needs to be reverted quickly, deleting the new GMP rule that points at "GMP-Superblob" will revert Balrog back to the non-multifile rules.
(In reply to Ben Hearsum (:bhearsum) from comment #11)
> I landed this to production today, and reworked the GMP rules to make use of
> it. Specifically, I:
> * Created new blobs for each individual version of the CDM, OpenH264, and
> Widewine plugins.
> * Created chains of rules for these products to handle their own
> version/platform requirements
> * Created a new GMP rule that points at a Superblob, which has OpenH264,
> CDM, and Widevine listed as response products.
> 
> Before making the new GMP rule I created a rule for the fake "bhearsumtest"
> product that pointed at the superblob and compared all the URLs from
> https://wiki.mozilla.org/User:Bhearsum/GMP_Updates (plus a couple that
> should have plugins offered) vs. a version with "bhearsumtest" instead of
> "GMP". All responses were exactly the same, except for the ones with no
> plugins listed. The empty response has changed slightly, to:
>  <?xml version="1.0"?>
>  <updates>
> -    <addons>
> -    </addons>
>  </updates>
> 
> ...which I ran by rhelmer before pushing anything live, who said that's what
> the client tests expect as an empty response anyways - so this is an
> accidental impromevent!
> 
> As things stand now, all the rules on the "GMP" product except for the one I
> just created are effectively dead. I'm going to leave them in place for a
> day or two just in case something goes wrong and we need to revert. If
> things are still looking good on Wednesday I will delete them.

Everything still looks good and no issue have been reported. I removed the old rules. We're all done here! Thanks for all your work here Varun!
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Product: Release Engineering → Release Engineering Graveyard
You need to log in before you can comment on or make changes to this bug.