1127591 - Can't remove restriction in a rule

Status: RESOLVED FIXED

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

Description

[Nick Thomas [:nthomas] (UTC+12)]

For GMP v1.3 we want to remove the channel=nightly restriction, but I can't do that with either the old or new UI. It claims success but if you reload the rules the limit is still there.

Comment 1

[Nick Thomas [:nthomas] (UTC+12)]

POST payload:
{"backgroundRate":100,"buildID":null,"buildTarget":null,"channel":"","comment":null,"data_version":8,"distVersion":null,"distribution":null,"headerArchitecture":null,"locale":null,"mapping":"GMP-v1.3-201501271800","osVersion":null,"priority":30,"product":"GMP","rule_id":107,"update_type":"minor","version":">36.0","build_id":null,"build_target":null,"os_version":null,"dist_version":null,"header_arch":null,"csrf_token":"1422577132.48##fac88169fe4a41349f540930b7a65f381e619d45"}

Perhaps because channel is "" instead of null. If you look at the history of rule 107 then data_version 6 through 9 are me trying to remove the restriction but the rule not changing.

I worked around by duplicating the rule and setting channel to aurora.

Comment 2

[Nick Thomas [:nthomas] (UTC+12)]

This is actually in the backend, I think. The UI sends all the parts of the rule, but we end up at
 https://github.com/mozilla/balrog/blob/dcf6a3/auslib/admin/views/rules.py#L167
and 'what' is a dict containing only the non-null values. So if something is changing to null it's not given to updateRule, and never gets changed.

Comment 3

[Nick Thomas [:nthomas] (UTC+12)]

Attachment: [balrog] wip

This makes the UI work, but breaks half a dozen tests and the idea that you can send only the data you want to update. Gone as far as I care to on a Friday afternoon.

Comment 4

[[e-mail] (:bhearsum)]

A workaround for this in the meantime could be to delete the entire rule and add a new one.

Comment 5

[[e-mail] (:bhearsum)]

Turns out I have production db access. As naughty as it is, I went ahead and did this by hand to tidy things up before doing bug 1134919:
mysql> select * from rules where product="GMP";
+---------+----------+----------------------------+----------------+-------------+---------+---------+---------+-------------+---------+--------+-----------+--------------+-------------+--------------------+--------------+-----------------------------------------------------------+
| rule_id | priority | mapping                    | backgroundRate | update_type | product | version | channel | buildTarget | buildID | locale | osVersion | distribution | distVersion | headerArchitecture | data_version | comment                                                   |
+---------+----------+----------------------------+----------------+-------------+---------+---------+---------+-------------+---------+--------+-----------+--------------+-------------+--------------------+--------------+-----------------------------------------------------------+
|      67 |       10 | GMP-Firefox33-201410010830 |            100 | minor       | GMP     | >=33.0  | NULL    | NULL        | NULL    | NULL   | NULL      | NULL         | NULL        | NULL               |            2 | NULL                                                      |
|      68 |       20 | GMP-v1.3-201501271800      |            100 | minor       | GMP     | >=34.0  | NULL    | NULL        | NULL    | NULL   | NULL      | NULL         | NULL        | NULL               |            7 | NULL                                                      |
|     107 |       30 | GMP-v1.3-201501271800      |            100 | minor       | GMP     | >36.0   | nightly | NULL        | NULL    | NULL   | NULL      | NULL         | NULL        | NULL               |            9 | NULL                                                      |
|     108 |       30 | GMP-v1.3-201501271800      |            100 | minor       | GMP     | >36.0   | aurora  | NULL        | NULL    | NULL   | NULL      | NULL         | NULL        | NULL               |            1 | Can't remove nightly restriction on rule 107, bug 1127591 |
|     109 |       30 | GMP-v1.3-201501271800      |            100 | minor       | GMP     | >=36.0  | beta    | NULL        | NULL    | NULL   | NULL      | NULL         | NULL        | NULL               |            3 | Can't remove nightly restriction on rule 107, bug 1127591 |
+---------+----------+----------------------------+----------------+-------------+---------+---------+---------+-------------+---------+--------+-----------+--------------+-------------+--------------------+--------------+-----------------------------------------------------------+
5 rows in set (0.00 sec)

mysql> 
mysql> 
mysql> update rules set channel=NULL where product="GMP";
Query OK, 3 rows affected (0.00 sec)
Rows matched: 5  Changed: 3  Warnings: 0

mysql> select * from rules where product="GMP";
+---------+----------+----------------------------+----------------+-------------+---------+---------+---------+-------------+---------+--------+-----------+--------------+-------------+--------------------+--------------+-----------------------------------------------------------+
| rule_id | priority | mapping                    | backgroundRate | update_type | product | version | channel | buildTarget | buildID | locale | osVersion | distribution | distVersion | headerArchitecture | data_version | comment                                                   |
+---------+----------+----------------------------+----------------+-------------+---------+---------+---------+-------------+---------+--------+-----------+--------------+-------------+--------------------+--------------+-----------------------------------------------------------+
|      67 |       10 | GMP-Firefox33-201410010830 |            100 | minor       | GMP     | >=33.0  | NULL    | NULL        | NULL    | NULL   | NULL      | NULL         | NULL        | NULL               |            2 | NULL                                                      |
|      68 |       20 | GMP-v1.3-201501271800      |            100 | minor       | GMP     | >=34.0  | NULL    | NULL        | NULL    | NULL   | NULL      | NULL         | NULL        | NULL               |            7 | NULL                                                      |
|     107 |       30 | GMP-v1.3-201501271800      |            100 | minor       | GMP     | >36.0   | NULL    | NULL        | NULL    | NULL   | NULL      | NULL         | NULL        | NULL               |            9 | NULL                                                      |
|     108 |       30 | GMP-v1.3-201501271800      |            100 | minor       | GMP     | >36.0   | NULL    | NULL        | NULL    | NULL   | NULL      | NULL         | NULL        | NULL               |            1 | Can't remove nightly restriction on rule 107, bug 1127591 |
|     109 |       30 | GMP-v1.3-201501271800      |            100 | minor       | GMP     | >=36.0  | NULL    | NULL        | NULL    | NULL   | NULL      | NULL         | NULL        | NULL               |            3 | Can't remove nightly restriction on rule 107, bug 1127591 |
+---------+----------+----------------------------+----------------+-------------+---------+---------+---------+-------------+---------+--------+-----------+--------------+-------------+--------------------+--------------+-----------------------------------------------------------+

Comment 6

[[e-mail] (:bhearsum)]

I ended up deleting the extra two rules. So now we're at a single rule with version >=36.0 (which we need, otherwise Beta users won't get served anything).

Comment 7

[[e-mail] (:bhearsum)]

This is getting to be really bad because the new UI shows the change succeeding even though it doesn't. I'm going to try to fix it soon.

Comment 8

[Nick Thomas [:nthomas] (UTC+12)]

That'd be great. We had an accidental restriction added to the main rule for Firefox on release today, and had to duplicate and delete to remove it again. History gets lost in the process (at least in the UI).

Comment 9

[hwine]

Hit with 37.0.1 as well when I initially set version:'<37.0' on the main release rule, then removed it (shouldn't have been there).

My view of the rules looked as I expected. Fortunately, Nick was checking and saw the version still applied. After force-reload of page, I got correct change.

Worked around by duplicating "bad" rule; removing version constraint; saving; then deleting "bad" rule.

Worked, but appears to have lost all version history for the rule. Auditing would be challenging.

Comment 10

[[e-mail] (:bhearsum)]

(In reply to Nick Thomas [:nthomas] from comment #8)
> That'd be great. We had an accidental restriction added to the main rule for
> Firefox on release today, and had to duplicate and delete to remove it
> again. History gets lost in the process (at least in the UI).

Yikes! We're going to need to adjust the rule id in https://github.com/mozilla/build-buildbot-configs/blob/master/mozilla/release-firefox-mozilla-release.py.template to account for that.

Comment 11

[Nick Thomas [:nthomas] (UTC+12)]

Attachment: [buildbot-configs] Fix rule id for Firefox release

Comment 12

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 8588871 [details] [diff] [review]
[buildbot-configs] Fix rule id for Firefox release

https://hg.mozilla.org/build/buildbot-configs/rev/6882eafccffc

Comment 13

[[e-mail] (:bhearsum)]

Attachment: fix inconsistent rule column naming in forms + fix restriction removal

The change in release channel rule id scared me a bit - this bug could easily cause us to screw up a release by either not shipping it or shipping the wrong thing (because we forget to adjust the release config rule id, or adjust it wrong), so I dug into this more today.

I took a bit of a different approach than you because the ability to do partial updates of rules is important to some of submission scripts. Most notably, ReleasePusher only changes the mapping of a rule and I didn't want to need to update that.

The linchpin here ended up being checking request.form/json for each key, and only caring about it if it existed there. The other weird thing was that the TestClient silently threw away data that was set to None. So, eg:
        ret = self._post("/rules/5", data=dict(buildTarget=None, data_version=1))

...would end up buildTarget never reaching the app. I think this is what has made this bug so annoying to try to fix in the past.

I ended up doing a big search and replace to finally get rid of the naming inconsistency of these columns - it was even more of a pain to try and fix this before I did that.

In addition to the unit tests, I did some testing in Vagrant. I was able to make changes to rules, remove restrictions entirely, and I double checked that things like setting backgroundRate to 0 still worked too. I intend to do similar testing on aus4-admin-dev before pushing to prod, just to be sure.

Comment 14

[[e-mail] (:bhearsum)]

Attachment: adjust rule column names in ui

Comment 15

[Release Engineering SlaveAPI Service]

In production: https://hg.mozilla.org/build/buildbot-configs/rev/6882eafccffc

Comment 16

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 8589672 [details] [diff] [review]
fix inconsistent rule column naming in forms + fix restriction removal

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

Looks good to me.

::: auslib/admin/views/rules.py
@@ +116,5 @@
> +                continue
> +            # We need to check for each column in both the JSON style post
> +            # and the regular multipart form data. If the key is not present
> +            # in the request *OR* its value is None, it will not exist in
> +            # either of these data structures. We treat this cases as no-op

Do you recall why we support both JSON and regular form posts ? I'm wondering if we can simplify by dropping one. I know I'm going puzzle over this bit of code every time I touch it.

Comment 17

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 8589674 [details] [diff] [review]
adjust rule column names in ui

>diff --git a/app/js/services/rules_service.js b/app/js/services/rules_service.js
>     setDataAliases: function(rule) {
>       // change the rule so that it has the necessary keys
>       // that it needs for being posted to the back end
...
>+      rule.buildID = rule.buildID;
>+      rule.buildTarget = rule.buildTarget;
>+      rule.osVersion = rule.osVersion;
>+      rule.distVersion = rule.distVersion;
>+      rule.headerArchitecture = rule.headerArchitecture;

Looks like we can remove this, as well as its callers and a test. Feel free to do that when you land.

Comment 18

[[e-mail] (:bhearsum)]

(In reply to Nick Thomas [:nthomas] from comment #16)
> Comment on attachment 8589672 [details] [diff] [review]
> fix inconsistent rule column naming in forms + fix restriction removal
> 
> Review of attachment 8589672 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me.
> 
> ::: auslib/admin/views/rules.py
> @@ +116,5 @@
> > +                continue
> > +            # We need to check for each column in both the JSON style post
> > +            # and the regular multipart form data. If the key is not present
> > +            # in the request *OR* its value is None, it will not exist in
> > +            # either of these data structures. We treat this cases as no-op
> 
> Do you recall why we support both JSON and regular form posts ? I'm
> wondering if we can simplify by dropping one. I know I'm going puzzle over
> this bit of code every time I touch it.

The old UI + submitter tools use multipart data. The new UI uses JSON. I agree that we shouldn't need to support both...I filed bug 1153319 to look at moving the submitter tools to JSON.

Comment 19

[[e-mail] (:bhearsum)]

Comment on attachment 8589674 [details] [diff] [review]
adjust rule column names in ui

(In reply to Nick Thomas [:nthomas] from comment #17)
> Comment on attachment 8589674 [details] [diff] [review]
> adjust rule column names in ui
> 
> >diff --git a/app/js/services/rules_service.js b/app/js/services/rules_service.js
> >     setDataAliases: function(rule) {
> >       // change the rule so that it has the necessary keys
> >       // that it needs for being posted to the back end
> ...
> >+      rule.buildID = rule.buildID;
> >+      rule.buildTarget = rule.buildTarget;
> >+      rule.osVersion = rule.osVersion;
> >+      rule.distVersion = rule.distVersion;
> >+      rule.headerArchitecture = rule.headerArchitecture;
> 
> Looks like we can remove this, as well as its callers and a test. Feel free
> to do that when you land.

Good point, I got rid of this + references to it.

Comment 20

[[github robot]]

Commit pushed to master at https://github.com/mozilla/balrog

https://github.com/mozilla/balrog/commit/3bd4577e0613e7a6864b716d791c0020cf32352e
bug 1127591: Can't remove restriction in a rule. r=nthomas

Comment 21

[[e-mail] (:bhearsum)]

OK, this seems to be working in dev. I'll get it pushed to prod sometime next week.

Comment 22

[[e-mail] (:bhearsum)]

Push happened this morning, and I verified this to be working in production.

Comment 23

[Nick Thomas [:nthomas] (UTC+12)]

\o/

Comment 24

[[e-mail] (:bhearsum)]

Interestingly, I realized today that the portion of this never landed in production because I forgot to rebuild the app before pushing. AFAICT, the reason it worked despite that is because setDataAlias' reused the same Rule object - so even without the UI changes landed, the correct values were being passed. Eg, here's the data sent from a change I made in dev:
{"backgroundRate":1,"buildID":"321","buildTarget":null,"channel":"auroraaaa1111","comment":null,"data_version":16,"distVersion":null,"distribution":null,"headerArchitecture":null,"locale":null,"mapping":"Fennec-mozilla-aurora-nightly-latest","osVersion":null,"priority":100,"product":"Fennec","rule_id":7,"update_type":"minor","version":null,"build_id":"321","build_target":null,"os_version":null,"dist_version":null,"header_arch":null,"csrf_token":"1432303852.56##24f314a790723be7b8fe086618b355d35d742d71"}

It has both buildID and build_id.

I'm going to be updating the UI as part of bug 1162874, so the ui part of this will *truly* get picked up when that lands.

Comment 25

[[github robot]]

Commit pushed to master at https://github.com/mozilla/balrog

https://github.com/mozilla/balrog/commit/83058e671a797070a5e170dec377904e758c5147
[balrog-ui] bug 1127591: Can't remove restriction in a rule - rename rule args to match columns. r=nthomas