1021026 - pushing release builds to beta doesn't work in balrog

Status: RESOLVED FIXED

(Release Engineering :: Release Automation: Other, defect)

Description

[[e-mail] (:bhearsum)]

Looks like we're missing a fileUrl for that channel in the blob:
"fileUrls": {

    "release": "http://download.mozilla.org/?product=%PRODUCT%&os=%OS_BOUNCER%&lang=%LOCALE%",
    "releasetest": "http://download.mozilla.org/?product=%PRODUCT%&os=%OS_BOUNCER%&lang=%LOCALE%",
    "betatest": "http://stage.mozilla.org/pub/mozilla.org/Firefox/candidates/30.0-candidates/build1/update/%OS_FTP%/%LOCALE%/%FILENAME%"

},

Comment 1

[[e-mail] (:bhearsum)]

It's unclear to me what the right way of fixing this is. Right now we have this in the blob (the %PRODUCT% sub in fileUrls is filled in by bouncerProducts):
"bouncerProducts": {
    "partial": "Firefox-30.0-Partial-29.0.1",
    "complete": "Firefox-30.0-Complete"

},
"fileUrls": {
    "release": "http://download.mozilla.org/?product=%PRODUCT%&os=%OS_BOUNCER%&lang=%LOCALE%",
    "releasetest": "http://download.mozilla.org/?product=%PRODUCT%&os=%OS_BOUNCER%&lang=%LOCALE%",
    "betatest": "http://stage.mozilla.org/pub/mozilla.org/Firefox/candidates/30.0-candidates/build1/update/%OS_FTP%/%LOCALE%/%FILENAME%"

},

If we just add a "beta" entry that points at "http://download.mozilla.org/?product=%PRODUCT%&os=%OS_BOUNCER%&lang=%LOCALE%", users will get a 404, because the files aren't in the releases directory yet. %PRODUCT% corresponds to patch types down at the locale level, so once we have multiple partial support, that will work fine (we'll just need to add the extra partial entries to bouncerProducts). Completes are a different story though, any beta or release that's mapped to one of these blobs will end up with "complete" as their %PRODUCT%. I'm not sure how we can distinguish them sanely.

My only idea for fully fixing this right now is using a completely separate blob when pointing beta users at a release. In that one, we'd change bouncerProducts to be the ones that point to the candidates directory. I'm not a fan of this idea - it duplicates a lot of data and is somewhat confusing IMO. Hopefully we can come up with something better.

Comment 2

[[e-mail] (:bhearsum)]

I've been thinking some more about this.

Idea #1: The fact that we added support for _multiple_ completes may make it easier to do this within a single blob. We could have data like this:
"bouncerProducts": {
    "partials": {
        "Firefox-29.0.1-build1": "Firefox-30.0-Partial-29.0.1",
        "Firefox-29.0b9-build1": "Firefox-30.0-Partial-29.0b9"
    },
    "completes": {
        "*": "Firefox-30.0-Complete",
        "beta": "Firefox-30.0-Complete-build1"
    }
},
"fileUrls": {
    "release": "http://download.mozilla.org/?product=%PRODUCT%&os=%OS_BOUNCER%&lang=%LOCALE%",
    "releasetest": "http://download.mozilla.org/?product=%PRODUCT%&os=%OS_BOUNCER%&lang=%LOCALE%",
    "betatest": "http://stage.mozilla.org/pub/mozilla.org/Firefox/candidates/30.0-candidates/build1/update/%OS_FTP%/%LOCALE%/%FILENAME%"
    "beta": "http://download.mozilla.org/?product=%PRODUCT%&os=%OS_BOUNCER%&lang=%LOCALE%"
},
"platforms": {
    "Linux_x86-gcc3": {
        "en-US": {
            "partials": [
                {
                    "hashValue": "abcdef",
                    "filesize": 1234,
                    "from": "Firefox-29.0.1-build1"
                }
            ],
            "completes": [
                {
                    "hashValue": "abc123",
                    "filesize": 99999,
                    "from": "beta"
                },
                {
                    "hashValue": "abc123",
                    "filesize": 99999,
                    "from": "*"
                }
            ]
        }
    }
}

We'd have to special case the word "beta" like we do for "*". We'd also have to figure out how to make the expandRelease logic pick the right one for beta and release (as it stands now, both would get the first thing in the list).


Idea #2: A more invasive alternative could be to combine bouncerProducts and ftpFilenames with fileUrls. For example:
"fileUrls": {
    "release": {
        "partials": {
            "Firefox-29.0.1-build1": "http://download.mozilla.org/?product=Firefox-30.0-Partial-29.0.1&os=%OS_BOUNCER%&lang=%LOCALE%"
        },
        "completes": {
            "*": "http://download.mozilla.org/?product=Firefox-30.0-Complete&os=%OS_BOUNCER%&lang=%LOCALE%"
        }
    },
    "releasetest": {
        "partials": {
            "Firefox-29.0.1-build1": "http://download.mozilla.org/?product=Firefox-30.0-Partial-29.0.1&os=%OS_BOUNCER%&lang=%LOCALE%"
        },
        "completes": {
            "*": "http://download.mozilla.org/?product=Firefox-30.0-Complete&os=%OS_BOUNCER%&lang=%LOCALE%"
        }
    },
    "betatest": {
        "partials": {
            "Firefox-29.0.1-build1": "http://stage.mozilla.org/pub/mozilla.org/Firefox/candidates/30.0-candidates/build1/update/%OS_FTP%/%LOCALE%/firefox-29.0.1-30.0.partial.mar"
        }
        "completes": {
            "*": "http://stage.mozilla.org/pub/mozilla.org/Firefox/candidates/30.0-candidates/build1/update/%OS_FTP%/%LOCALE%/firefox-30.0.complete.mar"
        }
    },
    "beta": {
        "partials": {
            "Firefox-29.0b9-build1": "http://download.mozilla.org/?product=Firefox-30.0-Partial-29.0b9-build1&os=%OS_BOUNCER%&lang=%LOCALE%"
        }
        "completes": {
            "Firefox-29.0b9-build1": "http://download.mozilla.org/?product=Firefox-30.0-Complete-build1&os=%OS_BOUNCER%&lang=%LOCALE%"
        }
    }
}
(No need for extra entries at the locale level in this scenario.)

This causes some duplication (particularly for release+releasetest, but maybe other things in the future), but doesn't require any additional special casing. This would likely require an entire new blob format, and I'd probably want to block on bug 1026070 if we were to go this route.


Idea #3: When we first create bouncer products, point the complete entry (eg Firefox-30.0-Complete) at the candidates directory. After we push files to mirrors, change it to point at the releases directory. This is the simplest option, but probably the most confusing, and possibly risky.


Nick, I'd love to get your thoughts on these options (or something I haven't thought of) at some point.

Comment 3

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

I like idea#2 best, since it actually makes things clearer by making them explicit. To reduce duplication, we could have a "default" in fileUrls to cover release + releasetest, and override for betatest and beta. 

Here's another way. It's based on your idea #1 with small changes & fixes
"bouncerProducts": {
    "partials": {
        "Firefox-29.0.1-build1": "Firefox-30.0-Partial-29.0.1",
        // version fix, added buildN for N > 2 case
        "Firefox-30.0b9-build1": "Firefox-30.0-build1-Partial-30.0b9" 
    },
    "completes": {
        "*": "Firefox-30.0-Complete",
        // no extra mapping here
    }
},
"fileUrls": {
    "release": "http://download.mozilla.org/?product=%PRODUCT%&os=%OS_BOUNCER%&lang=%LOCALE%",
    "releasetest": "http://download.mozilla.org/?product=%PRODUCT%&os=%OS_BOUNCER%&lang=%LOCALE%",
    "betatest": "http://stage.mozilla.org/pub/mozilla.org/Firefox/candidates/30.0-candidates/build1/update/%OS_FTP%/%LOCALE%/%FILENAME%"
    "beta": "http://download.mozilla.org/?product=%PRODUCT%&os=%OS_BOUNCER%&lang=%LOCALE%"
},
"platforms": {
    "Linux_x86-gcc3": {
        "en-US": {
            "partials": [
                {
                    "hashValue": "abcdef",
                    "filesize": 1234,
                    "from": "Firefox-29.0.1-build1"
                }
                {  // explicitly adding the beta partial
                    "hashValue": "fedcba",
                    "filesize": 321,
                    "from": "Firefox-30.0b9-build1"
                }
            ],
            "completes": [
                {
                    "hashValue": "abc123",
                    "filesize": 99999,
                    "from": "*"
                }
            ]
        }
    }
}

When we setup bouncer we add products Firefox-30.0-Complete-Candidates and Firefox-30.0-Complete-Releases, using firefox/candidates/ and firefox/releases/ URLs respectively. Then alias Firefox-30.0-Complete to Firefox-30.0-Complete-Candidates for beta users. After we do mirror push and bouncer has uptake on Firefox-30.0-Complete-Releases then change the alias to point to that. This would be less intrusive than changing Firefox-30.0-Complete locations and having disruption from not having sentry run on them yet - it should be smooth if the CDN can pull the new files quickly (which we implicitly rely on already). Would also prime the CDN for release users.

Comment 4

[[e-mail] (:bhearsum)]

Oooh, I forgot about alias'. That's certainly better than changing the actual Location IMO. The only thing I don't like about this plan is that things are a little bit more opaque. Your tweaked version of my idea #1 seems like it may be the simplest thing to implement.

I do like your tweak to idea #2 though -- I don't see any downside implementation-wise if we have a default to reduce duplication.

The deciding factor here may be the timeline.

Comment 5

[[e-mail] (:bhearsum)]

Catlee and I chatted about this today too - he pointed out that we could skirt the issue by pointing the Beta channel directly at ftp.mozilla.org rather than Bouncer. This would expedite this bug, though I still think we should adjust the schema later. This could have IT and Metrics impacts, so needinfo? on Jake and Anurag.

Anurag, I think you mentioned in e-mail a few others who may want to weigh in?

Comment 6

[Jake Maul [:jakem]]

Rather than use ftp.mozilla.org, I'd vastly prefer download.cdn.mozilla.net. The CDN caching is very good, and Beta users are already on CDN, so this would be no change from an infrastructure perspective. Using ftp.mozilla.org directly causes me concern over load on that cluster, so that's less desirable.

However, this may be moot anyway as I suspect the Metrics concerns will be greater.

Comment 7

[Anurag Phadke[:[e-mail]]]

[e-mail], [e-mail], [e-mail], [e-mail], [e-mail]:

As per comment #5, moving beta channel directly to FTP (instead of the dmo -> cdn) route would result in download numbers for beta channel (for specific requests) to be under counting until we merge the logs in our DWH (sometime in Q3).

AFAIK, other than topcat (not sure if anybody uses topcat), there are no dashboards that currently look at the download numbers. If you run adhoc queries, then the numbers will be under-reported, until you modify the query to also look for domain='ftp.mozilla.org'.

I have NEEDINFO'ed everyone who might be affected with this change, can you say a yay/nay via comment on the bug so bhearsum can go ahead and make the changes?

thx,
-anurag

Comment 8

[[e-mail] (:bhearsum)]

(In reply to Jake Maul [:jakem] from comment #6)
> Rather than use ftp.mozilla.org, I'd vastly prefer download.cdn.mozilla.net.
> The CDN caching is very good, and Beta users are already on CDN, so this
> would be no change from an infrastructure perspective. Using ftp.mozilla.org
> directly causes me concern over load on that cluster, so that's less
> desirable.

Ah, I didn't realize we went through the CDN for these. I definitely don't want to change that. If we go through with this, download.cdn.mozilla.net is fine.

(In reply to Anurag Phadke[:[e-mail]] from comment #7)
> As per comment #5, moving beta channel directly to FTP (instead of the dmo
> -> cdn) route would result in download numbers for beta channel (for
> specific requests) to be under counting until we merge the logs in our DWH
> (sometime in Q3).
> 
> AFAIK, other than topcat (not sure if anybody uses topcat), there are no
> dashboards that currently look at the download numbers. If you run adhoc
> queries, then the numbers will be under-reported, until you modify the query
> to also look for domain='ftp.mozilla.org'.

Just to be clear here (apologies if I wasn't before): I'm only talking about requests for MARs, and only during the period where we push the RC to the Beta channel. Nothing regarding installers would change.

Comment 9

[Robert Strong (they/them - no direct email)]

No problem afaic. We used to serve all of the mars from the ftp mirror network and it shouldn't be a problem as long as they support range requests as was required in the past.

Comment 10

[[e-mail] (:bhearsum)]

(In reply to Anurag Phadke[:[e-mail]] from comment #7)
> [e-mail], [e-mail], [e-mail],
> [e-mail], [e-mail]:
> 
> As per comment #5, moving beta channel directly to FTP (instead of the dmo
> -> cdn) route would result in download numbers for beta channel (for
> specific requests) to be under counting until we merge the logs in our DWH
> (sometime in Q3).
> 
> AFAIK, other than topcat (not sure if anybody uses topcat), there are no
> dashboards that currently look at the download numbers. If you run adhoc
> queries, then the numbers will be under-reported, until you modify the query
> to also look for domain='ftp.mozilla.org'.

Based on Jake's input, we're now talking about download.cdn.mozilla.net instead of ftp.mozilla.org. I'm not sure if that makes a difference to anyone or not...

Comment 11

[John Jensen]

Hi all,

I've read the bug but don't understand the possible impact of this change:

1) what prompted this problem/change?
2) does this mean that beta updates no longer go through bouncer?
3) what metrics are impacted? just website downloads?

Comment 12

[[e-mail] (:bhearsum)]

(In reply to John Jensen from comment #11)
> Hi all,
> 
> I've read the bug but don't understand the possible impact of this change:

To be clear: no change has happened yet -- we're talking about how to implement pushing RC builds to the beta channel with new the AUS software (Balrog) that we're working on.

> 1) what prompted this problem/change?

We're anxious to get Balrog live for the beta channel sooner rather than later, so that we can get some wider scale testing ASAP. Pointing beta users directly at download.cdn.mozilla.net for RC builds is a temporary measure that will let us get to this point sooner. The medium term solution is more complicated/risky, and we're to avoid delaying for that.

> 2) does this mean that beta updates no longer go through bouncer?

"Normal" betas (generally, that means beta 1 through 9) would be unchanged, they'll continue to go through Bouncer. The only change is that the updates to RC builds that we push to beta users in the last week of each cycle would bypass Bouncer.

> 3) what metrics are impacted? just website downloads?

Website downloads are not impacted at all. This change would only impact updates done through AUS (MARs), and only for the RC builds, as described above.


We do plan on moving to a more robust solution that will put updates to RC builds back into Bouncer when we're less pressed for time.

Comment 13

[Anurag Phadke[:[e-mail]]]

Given this only affects updates pushed as MARs and only specific builds on beta, it won't significantly impact download numbers. 
John - would u be okay with the switch, we do get FTP logs in our DWH and adding the counts would involve appending hive query with "or domain ='ftp.mozilla.org'"

Comment 14

[[e-mail] (:bhearsum)]

(In reply to Anurag Phadke[:[e-mail]] from comment #13)
> Given this only affects updates pushed as MARs and only specific builds on
> beta, it won't significantly impact download numbers. 
> John - would u be okay with the switch, we do get FTP logs in our DWH and
> adding the counts would involve appending hive query with "or domain
> ='ftp.mozilla.org'"

Sorry Anurag, this will be "download.cdn.mozilla.net" -- after Jake's comment #6 we determined that using "ftp.mozilla.org" directly would probably kill the FTP cluster. Do you also have metrics for "download.cdn.mozilla.net"?

Comment 15

[Anurag Phadke[:[e-mail]]]

Jake - do bouncer logs for d.c.m.n go through Zeus?

Comment 16

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

For clarity, the possibility we're exploring is updates downloaded directly to download.cdn.mozilla.net, bypassing download.m.o. Seems to me that we would have to get logs from the 3 CDN providers in order to count download counts into our metrics solutions.

Comment 17

[Anurag Phadke[:[e-mail]]]

nthomas - AFAIK, CDNs don't provide us raw logs.. 

IIUC the d.c.m.n solution is temporarily and bhearsum + team will patch it to use d.m.o as soon as they get some breathing room.

:bhearsum - right?

-anurag

Comment 18

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

Ben is now on PTO for the next two weeks, luckily I work with him every day! I was responding to comment #15, which I was having trouble parsing.  Perhaps you could expand on it ?

Comment 19

[Jake Maul [:jakem]]

(In reply to Anurag Phadke[:[e-mail]] from comment #15)
> Jake - do bouncer logs for d.c.m.n go through Zeus?

No... that's the primary CDN property, and as you indicated in comment 17 we don't currently get raw logs from that. We *can*, but it's something we'd have to set up. Non-trivial because we'd be getting 3 different sets of logs, one from each vendor, and they would be totally different- different log format, file name schema, rotation schedule, and delivery mechanism.

Probably still worthwhile to do at some point, but not a quick-fix.

Comment 20

[bhavana bajaj [:bajaj]]

Switching the NI to :lsblakk here

Comment 21

[Lukas Blakk [:lsblakk] use ?needinfo]

I'm OK with altered counts on the RC MAR downloads as we do not currently rely on those for any specific needs.  When we push the final RC to the beta population the priority is having as much of the Beta audience as possible updated to that build to get the data we want before release.

Comment 22

[John Jensen]

FWIW, I am OK with this. 

I don't think this is a gotcha, but I am going to raise it anyways. In the 5-10 days there will be some very important tests regarding search provider usage launched in Beta. I am told by Engineering that from a release/IT perspective, these will look exactly the same as usual -- just another standard Beta release.

My question is: is there the slightest potential that any of the changes discussed here could have a negative effect on these tests? That is, users not getting updates quickly? I am guessing "no" but want to confirm. If there is any potential of this I would ask that this be delayed for ~2-4 weeks.

Comment 23

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

There's no danger to worry about, we won't be implementing on that timescale.

Comment 24

[[e-mail] (:bhearsum)]

(In reply to Anurag Phadke[:[e-mail]] from comment #17)
> nthomas - AFAIK, CDNs don't provide us raw logs.. 
> 
> IIUC the d.c.m.n solution is temporarily and bhearsum + team will patch it
> to use d.m.o as soon as they get some breathing room.

Yeah, it's a temporary solution. I can't imagine it lasting more than 3 release cycles (hopefully fewer).

Given that everyone seems to be OK with direct linking to the CDN as a short term workaround, I'm going to proceed with that for now, and I'll come back to implement this in a way that lets us go through bouncer still later.

Thanks for all your input everyone!

Comment 25

[[e-mail] (:bhearsum)]

Attachment: wip hacky patch

I still have vacation brain, so feedback only for now. I did some quick tests with this on aus4-dev. You can see the blob that was created (after submitting an en-US build by hand) here:
https://aus4-admin-dev.allizom.org/releases/Firefox-30.0-build2/data

And I have rules for {release,beta}{,test} set-up:
https://aus4-dev.allizom.org/update/3/Firefox/29.0/20140318013849/Linux_x86_64-gcc3/en-US/beta/Linux%203.11.0-17-generic%20%28GTK%202.28.20%29/default/default/update.xml?force=1
https://aus4-dev.allizom.org/update/3/Firefox/29.0/20140318013849/Linux_x86_64-gcc3/en-US/release/Linux%203.11.0-17-generic%20%28GTK%202.28.20%29/default/default/update.xml?force=1
https://aus4-dev.allizom.org/update/3/Firefox/29.0/20140318013849/Linux_x86_64-gcc3/en-US/betatest/Linux%203.11.0-17-generic%20%28GTK%202.28.20%29/default/default/update.xml?force=1
https://aus4-dev.allizom.org/update/3/Firefox/29.0/20140318013849/Linux_x86_64-gcc3/en-US/releasetest/Linux%203.11.0-17-generic%20%28GTK%202.28.20%29/default/default/update.xml?force=1


There's a few (possibly obvious) issues:
* displayVersion doesn't indicate an RC # - I haven't figured out a way to fix this yet. It might be a mega hack, or we could decide to live without it.
* No test channel with download.cdn.mozilla.net. We can't use "releasetest" for this, because we need it to stay pointed at bouncer (and we don't want to be modifying blobs mid-release process). We might be able to use betatest, depending on the impact it may have on our update verify and QA's early update tests. If betatest isn't useful, we may have invent a new channel name for it - we've talked about that a bit in bug 986990, so we should pay mind to that if we need to go here.

Comment 26

[[e-mail] (:bhearsum)]

(In reply to Ben Hearsum [:bhearsum] from comment #25)
> There's a few (possibly obvious) issues:
> * displayVersion doesn't indicate an RC # - I haven't figured out a way to
> fix this yet. It might be a mega hack, or we could decide to live without it.

A non-hacky way of doing this could be to support per-channel displayVersions, like we do for fileUrls. We could reduce duplication by support a default with "*" like we do for other things.

I'm hoping there's a less extreme option than that, though - we're quickly adding such a level of indirection to a lot of different fields, and we should avoid doing so unless we have a good reason IMO.

> * No test channel with download.cdn.mozilla.net. We can't use "releasetest"
> for this, because we need it to stay pointed at bouncer (and we don't want
> to be modifying blobs mid-release process). We might be able to use
> betatest, depending on the impact it may have on our update verify and QA's
> early update tests. If betatest isn't useful, we may have invent a new
> channel name for it - we've talked about that a bit in bug 986990, so we
> should pay mind to that if we need to go here.

I did a bit more thinking/looking at this and I think we will have to use a new channel name. Pointing update verify at the CDN will no doubt slow it down. Based on my reading of bug 986990, we should use "beta-cdntest" for that. Whimboo told me that QA's tests should be fine with such a change, but it's probably something we should make more noise about if we end up doing it.
17:15 < bhearsum> i'm wondering how hardcoded channel names are. for example, if we wanted to run update tests against a channel called "foobar", is that possible 
                  already?
17:15 < whimboo> bhearsum: sure. you are free to use whatever you want

Comment 27

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

Comment on attachment 8451862 [details] [diff] [review]
wip hacky patch

This is fine in the short term. 

Having the 'RC n' in the display version would be good to have for beta users, but I suspect most will never see any UI including it. Perhaps we can include it when we first submit the release blob, and then do a POST to remove it just prior to changing the release channel mapping. IIRC you can update just part of the blob.

beta-cdntest sounds OK too, and would be a simple extension of this patch.

Comment 28

[[e-mail] (:bhearsum)]

Attachment: set up beta and beta-cdntest channels

(In reply to Nick Thomas [:nthomas] from comment #27)
> Comment on attachment 8451862 [details] [diff] [review]
> wip hacky patch
> 
> This is fine in the short term. 
> 
> Having the 'RC n' in the display version would be good to have for beta
> users, but I suspect most will never see any UI including it. Perhaps we can
> include it when we first submit the release blob, and then do a POST to
> remove it just prior to changing the release channel mapping. IIRC you can
> update just part of the blob.

If we do this, we'll have to be extra careful during point releases, because "X.Y RC Z" will be the default displayVersion when channel == "release", I think. I lean towards not bothering with it (for now at least) if you think most users won't even see it, though. Lukas, do you feel strongly about "RC Z" being included in the display version when we push RCs to the beta channel?

In any case, here's an updated patch that sets up both the beta and beta-cdntest channels. If we can live without displayVersion having "RC n" in it, it should be all we need to move forward here.

Comment 29

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

Comment on attachment 8453044 [details] [diff] [review]
set up beta and beta-cdntest channels

If we're hacking, then we could test 
   channel == 'release' and re.match('\d+.0$', version)
but whatever Lukas says.

Comment 30

[[e-mail] (:bhearsum)]

Comment on attachment 8453044 [details] [diff] [review]
set up beta and beta-cdntest channels

Whoops, jumped the gun here.

Comment 31

[[e-mail] (:bhearsum)]

(In reply to Ben Hearsum [:bhearsum] from comment #28)
> Created attachment 8453044 [details] [diff] [review]
> set up beta and beta-cdntest channels
> 
> (In reply to Nick Thomas [:nthomas] from comment #27)
> > Comment on attachment 8451862 [details] [diff] [review]
> > wip hacky patch
> > 
> > This is fine in the short term. 
> > 
> > Having the 'RC n' in the display version would be good to have for beta
> > users, but I suspect most will never see any UI including it. Perhaps we can
> > include it when we first submit the release blob, and then do a POST to
> > remove it just prior to changing the release channel mapping. IIRC you can
> > update just part of the blob.
> 
> If we do this, we'll have to be extra careful during point releases, because
> "X.Y RC Z" will be the default displayVersion when channel == "release", I
> think. I lean towards not bothering with it (for now at least) if you think
> most users won't even see it, though. Lukas, do you feel strongly about "RC
> Z" being included in the display version when we push RCs to the beta
> channel?

Meant to needinfo you on this, whoops!

Comment 32

[Lukas Blakk [:lsblakk] use ?needinfo]

Sorry for the delay, no I do not feel strongly about the RC being in the display when on Beta as most people do not see this.  Occasionally I have had to mention in a bug that we push the RC to beta but that's rare enough.

Comment 33

[[e-mail] (:bhearsum)]

(In reply to Lukas Blakk [:lsblakk] from comment #32)
> Sorry for the delay, no I do not feel strongly about the RC being in the
> display when on Beta as most people do not see this.  Occasionally I have
> had to mention in a bug that we push the RC to beta but that's rare enough.

Thanks Lukas! Given that, I'm going to go ahead and re-land this patch.

Comment 34

[[e-mail] (:bhearsum)]

With bug 1039715 fixed, and the patch here landed we're able to ship RCs to Beta users through Balrog with the quickfix described in comment #8.

I'll be coming back to this bug within the next few months to fix it properly, so that RCs go through Bouncer again (instead of directly to download.cdn.mozilla.net).

Comment 35

[[e-mail] (:bhearsum)]

(In reply to Nick Thomas [:nthomas] from comment #3)
> I like idea#2 best, since it actually makes things clearer by making them
> explicit. To reduce duplication, we could have a "default" in fileUrls to
> cover release + releasetest, and override for betatest and beta. 

I think I'm going to have time to come back to this soon, and I'm planning to take this approach. The default may complicate fallback channel logic a bit, but I think reducing the duplication is worthwhile even if it does.

Comment 36

[[e-mail] (:bhearsum)]

Oh, and the more I think about it, the less I feel strongly about blocking this on bug 1026070.

We *will* need to upgrade the nightly blobs even though they don't use fileUrls/bouncerProducts/ftpFilenames (because release builds use the same scripts to submit data). I _think_ even after updating the tools to use v4, the nightly builds will be able to continue submitting to their v3 blobs (which is probably a bug we should fix). If that's the case, I can wait for a round of nightlies to finish and then adjust schema_version in their blobs afterwards. Come to think of it, I can probably just adjust schema_version in their blobs when I land the new version. I'll look into this some more and come up with a tested plan before landing though.

Comment 37

[[e-mail] (:bhearsum)]

Attachment: create schema v4 to enable proper RC support

I think this patch is mostly self explanatory. The main complexity is the movement of _getUrl, _getFtpFilename, and _getBouncerProduct into SeparatedFileUrlsMixin, because they're no longer common to all schema versions.

In the end, I ended up writing code to migrate v3 blobs -> v4. Rather than try to fix bug 1026070 as part of this I just wrote a simple script to do upgrades. When this lands, we'll need IT to run it on the production database. I wasn't sure about attaching it to the blob class, but I couldn't think of a better place. It may go away, move, or get replaced when bug 1022649 is fixed.

I also moved most of the command line scripts from the root of the repo into scripts/. I left admin.py and balrog-server.py because it seemed reasonable to keep them beside their wsgi counterparts - I can move them too if you want, though.

Comment 38

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

Comment on attachment 8492142 [details] [diff] [review]
create schema v4 to enable proper RC support

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

r+ even though I've got lots of comments. Land with chagnes, or happy to take another look after fixes & merging against other changes you have pending.

Please update the docstring in auslib.blob.createXML() for the class changes.

::: auslib/blobs/apprelease.py
@@ +647,5 @@
> +            ]
> +            url = None
> +            for c in channels:
> +                if c in self['fileUrls']:
> +                    url = self['fileUrls'][c][patchKey][from_]

The blob should be consistent (ie fileUrls and platform-locale info), but there's no error handling here if it isn't ? Was expecting we'd have the same .get().get() style and break only if url was not None.

@@ +742,5 @@
> +                }
> +            }
> +        }
> +    }
> +    # for the benefit of createXML and createSnippetv2

'and createSnippetv2' can go here and in ReleaseBlobV3.

@@ +745,5 @@
> +    }
> +    # for the benefit of createXML and createSnippetv2
> +    optional_ = ('billboardURL', 'showPrompt', 'showNeverForVersion',
> +                 'showSurvey', 'actions', 'openURL', 'notificationURL',
> +                 'alertURL')

Please make sure to add interpolable_ here when you merge master in.

::: auslib/test/blobs/test_apprelease.py
@@ +837,5 @@
> +                    "partials": [
> +                        {
> +                            "filesize": 8,
> +                            "from": "h1",
> +                            "hashValue": 9

I just realized if that we can't express different 'filesize' and 'hashValue' if the channels point to different files, unless the 'from' differs too. Maybe that's an edge case, and doesn't make a difference for beta on release, but worth a comment somewhere ?

@@ +1039,5 @@
> +                        {
> +                            "filesize": 4,
> +                            "from": "g1",
> +                            "hashValue": 5
> +                        }

Might be worth chucking a fileUrl in for both of these, otherwise the blob is nonsensical.

::: scripts/3to4.py
@@ +17,5 @@
> +if __name__ == "__main__":
> +    from argparse import ArgumentParser
> +
> +    parser = ArgumentParser()
> +    parser.add_argument("--db", dest="db", required=True)

It may be convenient to IT to be able to point at the existing admin ini file to read this.

@@ +19,5 @@
> +
> +    parser = ArgumentParser()
> +    parser.add_argument("--db", dest="db", required=True)
> +    parser.add_argument("--name", dest="name", required=True)
> +    parser.add_argument("releases", metavar="release", nargs="+")

Both of these could do with a help string.

Comment 39

[[github robot]]

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

https://github.com/mozilla/balrog/commit/44422d40f798c103872c28936d30f4ac920250a5
bug 1021026: pushing release builds to beta doesn't work in balrog - add new blob type that supports more diverse configuration of fileUrls, primarily to support pushing release builds to the beta channel without bypassing bouncer. r=nthomas

Comment 40

[[e-mail] (:bhearsum)]

Attachment: patch with comments addressed

There's very little code change here, so I'm not asking for explicit review before landing. Feel free to comment though - I can make changes in follow-ups.

The push to prod is likely to happen tomorrow rather than today, because I still need to prep the blobs and rules for bug 1013354. This should be a no-op when it first goes to prod - we won't start hitting the V4 code paths until I update the client side tools to use v4 and do some blob migrations for nightlies.

(In reply to Nick Thomas [:nthomas] from comment #38)
> ::: auslib/test/blobs/test_apprelease.py
> @@ +837,5 @@
> > +                    "partials": [
> > +                        {
> > +                            "filesize": 8,
> > +                            "from": "h1",
> > +                            "hashValue": 9
> 
> I just realized if that we can't express different 'filesize' and
> 'hashValue' if the channels point to different files, unless the 'from'
> differs too. Maybe that's an edge case, and doesn't make a difference for
> beta on release, but worth a comment somewhere ?

Good idea. In a future world, this sort of thing would be in some sort of blob creation or description doc, or perhaps near the blob validator that we don't yet have. I'll stick it in the V4 format for now for lack of a better place.

The argument could also be made that if you're serving different MARs, you're a different release (and thus a different blob). It would shock me if this assumption wasn't invalid at some point, though.

Comment 41

[[e-mail] (:bhearsum)]

The Balrog patches were pushed to production today. We're not done here yet though - I need to update the client side tools to use the v4 blob format. Once that's done, we'll be serving release builds to beta users through Bouncer after the next release. I hope to get to this before we build 33.0, but I've been pulled into an urgent project so I can't make any promises.

Comment 42

[[e-mail] (:bhearsum)]

Attachment: fix forbidden domain checking

I discovered that I busted this while I was testing my changes to the submitters, oops. This logic should probably move elsewhere. I'll file a follow-up for that if you agree.

Comment 43

[[e-mail] (:bhearsum)]

Attachment: use schema v4 in balrog data submitters

This is a big scary looking patch to support v4 data submission. The patch is horribly ugly, so I suggest looking at just the new file if you're having trouble grokking anything.

Now that we're certainly not using it anymore, I dropped support for v2 as well. https://github.com/mozilla/balrog/blob/44422d40f798c103872c28936d30f4ac920250a5/auslib/blobs/apprelease.py#L676 talks about what's new in v4, so you may want to give that a read.

The overall idea of this patch is to factor out fileUrls, bouncerProducts, and ftpFilenames into methods, so that v3 and v4 can both be supported.

The best validation I can give you is a dev run against 33.0b9. Submitting the Mac en-US build and running the release creator resulted in:
https://aus4-admin-dev.allizom.org/releases/Firefox-33.0b9-build1/data

And we can see that all of the update channels return the correct thing:
https://aus4-dev.allizom.org/update/3/Firefox/33.0/20140902185629/Darwin_x86_64-gcc3-u-i386-x86_64/en-US/beta/default/default/default/update.xml
https://aus4-dev.allizom.org/update/3/Firefox/33.0/20140902185629/Darwin_x86_64-gcc3-u-i386-x86_64/en-US/beta-cdntest/default/default/default/update.xml
https://aus4-dev.allizom.org/update/3/Firefox/33.0/20140902185629/Darwin_x86_64-gcc3-u-i386-x86_64/en-US/beta-localtest/default/default/default/update.xml

I'm not sure this needs review from two people, but after fighting with it more than I expected I'm not sure anymore. Better safe than sorry here!

Comment 44

[[e-mail] (:bhearsum)]

Attachment: updated patch

Rail noticed a couple of things that I already had fixed locally (I attached the wrong patch), and one other thing that I missed. Changes in this patch:
* Fix issue with updateChannels name in _getFileUrls (the method variable and local one had the same name, which broke adding channels other than *).
* Fix bouncerProduct in the same method to use a bouncer product name instead of file name
* Optionally add extraPartials, rather than requiring it in the release config) (I think this code dies when bug 908134 is fixed.)
* Adjust creation of some subdicts in fileUrls to make sure they don't get overridden with each loop iteration.

Comment 45

[Rail Aliiev [:rail]]

Comment on attachment 8503159 [details] [diff] [review]
updated patch

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

::: lib/python/balrog/submitter/cli.py
@@ +189,5 @@
> +        for channel in uniqueChannels:
> +            data["fileUrls"][channel] = {
> +                "completes": {}
> +            }
> +            if channel in ('betatest', 'esrtest') or "localtest" in channel:

Looks like this condition doesn't make sense because we have only "*" and "localtest" in the list

@@ +216,5 @@
> +                                            protocol='http')
> +                    filename = "%s-%s-%s.partial.mar" % (productName.lower(), previousVersion, version)
> +                    data["fileUrls"][channel]["partials"][from_] = "%s/%s" % (dir_, filename)
> +                else:
> +                    bouncerProduct = "%s-%s-partial-%s" % (productName.lower(), version, previousVersion)

bouncer is case insensitive, you can drop lower() if you want.

Comment 46

[[e-mail] (:bhearsum)]

Attachment: fix channel conditions

(In reply to Rail Aliiev [:rail] from comment #45)
> Comment on attachment 8503159 [details] [diff] [review]
> updated patch
> 
> Review of attachment 8503159 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: lib/python/balrog/submitter/cli.py
> @@ +189,5 @@
> > +        for channel in uniqueChannels:
> > +            data["fileUrls"][channel] = {
> > +                "completes": {}
> > +            }
> > +            if channel in ('betatest', 'esrtest') or "localtest" in channel:
> 
> Looks like this condition doesn't make sense because we have only "*" and
> "localtest" in the list

Whoops, I should have this condition in the loop further up, too. This patch fixes that.

> @@ +216,5 @@
> > +                                            protocol='http')
> > +                    filename = "%s-%s-%s.partial.mar" % (productName.lower(), previousVersion, version)
> > +                    data["fileUrls"][channel]["partials"][from_] = "%s/%s" % (dir_, filename)
> > +                else:
> > +                    bouncerProduct = "%s-%s-partial-%s" % (productName.lower(), version, previousVersion)
> 
> bouncer is case insensitive, you can drop lower() if you want.

Yeah. aus3 snippets use lower case though, and being consistent makes it easier to do sanity checks between aus3 and aus4.

Comment 47

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

Comment on attachment 8502621 [details] [diff] [review]
fix forbidden domain checking

>diff --git a/auslib/db.py b/auslib/db.py
>+        for c in data.get('fileUrls', {}).values():
>+            for from_ in c.values():
>+                for url in from_.values():
>+                    domain = urlparse(url)[1]

It looks like you're removing support for schema 3 and older. And I really love some expanded testing around this.

Comment 48

[[e-mail] (:bhearsum)]

(In reply to Nick Thomas [:nthomas] from comment #47)
> Comment on attachment 8502621 [details] [diff] [review]
> fix forbidden domain checking
> 
> >diff --git a/auslib/db.py b/auslib/db.py
> >+        for c in data.get('fileUrls', {}).values():
> >+            for from_ in c.values():
> >+                for url in from_.values():
> >+                    domain = urlparse(url)[1]
> 
> It looks like you're removing support for schema 3 and older. And I really
> love some expanded testing around this.

Hm, good catch. I'll fix this, and add tests (which probably would've caught this before review if I'd had them =\).

Comment 49

[[e-mail] (:bhearsum)]

Attachment: forbidden domain fixes, fixed

Shamefully, the original patch here didn't even pass the existing tests - I'm sorry for wasting your time with it. This patch adds comprehensive tests for forbidden domain checking for each blob type - and it passes all of them.

I'm planning to file a follow-up for cleaning up the forbidden domain checking, because it really should be part of the blobs like the TODO says. I'm pretty sure the reason it isn't is because there was no global place I could find to store the whitelisted domains at the time.

Comment 50

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

Comment on attachment 8506362 [details] [diff] [review]
forbidden domain fixes, fixed

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

r- because we noticed that a blob will only have the first url tested. Would be good to change one of the containsForbiddenDomain() names, to avoid confusion.

Totally agree with the TODO, but doesn't have to be right now.

Comment 51

[[e-mail] (:bhearsum)]

Attachment: fix bug with multiple urls; rename one of the fnuctions to be less confusing

Comment 52

[[github robot]]

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

https://github.com/mozilla/balrog/commit/b134ae46c56782c36f9f553246fbea1241317cb1
bug 1021026: pushing release builds to beta doesn't work in balrog. r=nthomas

Comment 53

[[e-mail] (:bhearsum)]

Comment on attachment 8506423 [details] [diff] [review]
fix bug with multiple urls; rename one of the fnuctions to be less confusing

Pushed this patch, will have it deployed to production later today. The tools patch will wait until this goes into production.

Comment 54

[[e-mail] (:bhearsum)]

Comment on attachment 8505468 [details] [diff] [review]
fix channel conditions

I landed this, and it's working fine with the still running l10n nightlies + alder nightlies (I updated their tools directories on the fly).

Comment 55

[[e-mail] (:bhearsum)]

Attachment: remove beta channel hack

With this all landed we can remove the hack that sets beta/beta-cdntest to download.cdn.mozilla.net. The default file urls point to bouncer, which is what we want for both beta-cdntest and beta.

Comment 56

[[e-mail] (:bhearsum)]

Comment on attachment 8507077 [details] [diff] [review]
remove beta channel hack

We _should_ be all done here, but I'm leaving this open until we have a release that runs smoothly with it.

Comment 57

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

Bug 1086202 is for the short-term fix of 34.0b2 build1, lets fix up the submission code here.

Comment 58

[[e-mail] (:bhearsum)]

Attachment: fix problems with localtest urls and missing partials

Two stupid mistakes:
1) I forgot to add in %supdate/%%OS_FTP%%/%%LOCALE%% to all of the v4 urls, as Nick described in bug 1086202.
2) The loop for partials in the v4 creator resets the dict after each iteration - which means we only end up with the last one.

I tested this on staging by:
* Deleting existing 33.0b9-build1, 34.0b1-build2 blobs on aus4-dev
* Uploading the 33.0b9-build1 and 34.0b1-build2 blobs from aus4 prod to aus4-dev
* Rerunning the linux64 en-US balrog submission against dev
* Rerunning the release creator against dev
* Update beta-localtest rule to point at 34.0b2-build1

I eyeballed the blob and it looks good, I also tested 33.0b9 and 34.0b1 update URLs:
https://aus4-dev.allizom.org/update/3/Firefox/33.0/20141002185629/Linux_x86_64-gcc3/en-US/beta-localtest/default/default/default/update.xml
https://aus4-dev.allizom.org/update/3/Firefox/34.0/20141014134955/Linux_x86_64-gcc3/en-US/beta-localtest/default/default/default/update.xml

Both MARs in each URL give me the MAR I was expecting.

Comment 59

[Rail Aliiev [:rail]]

Comment on attachment 8508643 [details] [diff] [review]
fix problems with localtest urls and missing partials

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

lgtm

Comment 60

[[e-mail] (:bhearsum)]

This worked well for 33.0.1. URLs like these all get partials:
https://aus3.mozilla.org/update/3/Firefox/32.0.3/20140923175406/Darwin_x86_64-gcc3-u-i386-x86_64/zu/release/default/cturra/default/update.xml?force=1
https://aus3.mozilla.org/update/3/Firefox/33.0/20141011015303/Linux_x86-gcc3/en-US/release/default/cturra/default/update.xml?force=1
https://aus3.mozilla.org/update/3/Firefox/32.0.2/20140917194002/WINNT_x86-msvc/ta/release/default/cturra/default/update.xml?force=1

And older URLs get just a complete:
https://aus3.mozilla.org/update/3/Firefox/32.0.1/20140915194002/WINNT_x86-msvc/ta/release/default/cturra/default/update.xml?force=1

Leaving this open until we actually see a beta work correctly (hopefully tomorrow).

Comment 61

[[e-mail] (:bhearsum)]

Everything worked fine for 34.0b3.

Thanks again to RelMan/IT/Metrics for letting us use the short term hack. We're now back to the point where RCs being pushed to Beta users will be server through Bouncer.