Update firefox-latest, firefox-stub, firefox-latest-euballot bouncer aliases as a part of post-release builder (and same for beta)

RESOLVED FIXED

Status

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: rail, Assigned: rail)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(10 attachments, 9 obsolete attachments)

4.80 KB, patch
nthomas
: review+
Details | Diff | Splinter Review
17.70 KB, patch
nthomas
: review+
Details | Diff | Splinter Review
51.18 KB, patch
nthomas
: review+
Details | Diff | Splinter Review
1.08 KB, patch
bhearsum
: review+
Details | Diff | Splinter Review
922 bytes, patch
bhearsum
: review+
Details | Diff | Splinter Review
5.55 KB, patch
bhearsum
: review+
Details | Diff | Splinter Review
5.04 KB, patch
bhearsum
: review+
Details | Diff | Splinter Review
2.14 KB, patch
bhearsum
: review+
Details | Diff | Splinter Review
1.97 KB, patch
bhearsum
: review+
Details | Diff | Splinter Review
1.00 KB, patch
bhearsum
: review+
Details | Diff | Splinter Review
Assignee

Description

6 years ago
From https://bounceradmin.allizom.org/api/docs/create_update_alias:

create_update_alias
=====

Create or update a product alias


Auth
----
Needs HTTP authentication. Staff members only.

Syntax:
-------
``http://.../api/create_update_alias``

Required GET Parameters:
------------------------
*None.*

Optional GET Parameters:
------------------------
*None.*

Required POST Parameters:
-------------------------

* ``alias``: The alias that you wish to create or update.
* ``related_product``: The product name you wish to have the alias redirect


Optional POST Parameters:
-------------------------
*None.*

Error Codes:
------------
Error code *0* denotes a general error (e.g., a general exception due
to server misconfiguration). The error text gives more information.

Tuxedo-specific error codes for this command:


* ``101``: Unknown validation error occurred
* ``102``: Alias name not provided
* ``103``: Related product name not provided
* ``104``: The product name specified was not a valid product name
* ``105``: The alias name specified matches the name of an existing product and must be different


Returns:
--------
XML

Example:
--------

Success:

    <success>Created/updated alias firefox-latest</success>

Failure:

    <error number="101">
        alias name is required.
    </error>
Depends on: 863381
This is a dupe of bug 818254 (or the other way around - I don't care which!)
Also EUBallot, for bug 869662.
Blocks: 869662
Summary: Update firefox-latest (beta-latest) bouncer aliases as a part of post-release builder → Update firefox-latest, firefox-stub, firefox-latest-euballot bouncer aliases as a part of post-release builder (and same for beta)
Blocks: 921015
No longer blocks: 921015
Depends on: 921015
Assignee

Updated

6 years ago
Assignee: nobody → rail
Assignee

Comment 3

6 years ago
Posted patch tools (obsolete) — Splinter Review
Attachment #8340542 - Flags: review?(bhearsum)
Assignee

Comment 4

6 years ago
Posted patch buildbot-configs (obsolete) — Splinter Review
Attachment #8340543 - Flags: review?(bhearsum)
Assignee

Comment 5

6 years ago
Posted patch buildbotcustom (obsolete) — Splinter Review
Attachment #8340544 - Flags: review?
Assignee

Comment 6

6 years ago
The patches make sure that we add the stub installer as a separate entry in bouncer. Identical to euballot.
Assignee

Updated

6 years ago
Attachment #8340543 - Attachment description: bouncer_alias-buildbot-configs.diff → buildbot-configs
Assignee

Updated

6 years ago
Attachment #8340544 - Flags: review? → review?(bhearsum)
Comment on attachment 8340544 [details] [diff] [review]
buildbotcustom

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

Must we use a separate factory for this? It seems like we should be able to do all bouncer entries (plain, euballot, and stub) in one factory. If that's too difficult I guess this is OK, but it's unfortunate to add another builder just for this.
Attachment #8340544 - Flags: review?(bhearsum)
Attachment #8340542 - Flags: review?(bhearsum) → review+
Attachment #8340543 - Flags: review?(bhearsum) → review+
Assignee

Updated

6 years ago
Depends on: 748800
Assignee

Updated

5 years ago
Attachment #8340542 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8340543 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8340544 - Attachment is obsolete: true
Assignee

Comment 8

5 years ago
Posted patch stub-mozharness.diff (obsolete) — Splinter Review
Submit stub to bouncer
Attachment #8384769 - Flags: review?(bhearsum)
Comment on attachment 8384769 [details] [diff] [review]
stub-mozharness.diff

Drive by - does this give us an SSL product for stub ?
Assignee

Comment 10

5 years ago
(In reply to Nick Thomas [:nthomas] from comment #9)
> Comment on attachment 8384769 [details] [diff] [review]
> stub-mozharness.diff
> 
> Drive by - does this give us an SSL product for stub ?

Good catch. I just checked the "stub" products, they all are SSL-only. I'll refactor a bit and submit a new patch.
Assignee

Updated

5 years ago
Attachment #8384769 - Flags: review?(bhearsum)
Assignee

Comment 11

5 years ago
Posted patch stub-mozharness.diff (obsolete) — Splinter Review
Nick, what do you think about this change?

The idea is to move the logic to the config files. So instead of trying to handle installers, completes, euballot and stub as special cases we would treat them as bouncer products. The partials still need to be iterated.

I'm still not sure what to do with --platform. I tend to get rid of that argument and use whatever we have in the configs. This would simplify the command we call from buildbot. The downside of this is that we would need to updated the config (remove unused platforms) whenever we change the platform list in the release configs. On the other hand this would make the script more self contained.
Attachment #8384769 - Attachment is obsolete: true
Attachment #8386825 - Flags: feedback?(nthomas)
Comment on attachment 8386825 [details] [diff] [review]
stub-mozharness.diff

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

I'd like to keep the recent change of not submitting Solaris for betas, so some sort of platform control seems necessary. Perhaps leave all the platforms in the config, set a default set in the command line parser, and pass extra arg(s) for release builds ?

::: configs/releases/bouncer_firefox.py
@@ +82,4 @@
>          },
> +        "stub-installer": {
> +            "product-name": "Firefox-%(version)s-stub",
> +            "ssl-only": True,

(future?) enhancement: Now that we have the Firefox-%(version)-SSL product, we could ask www.mozilla.org to use os=win32-stub we could put this in the installer-ssl block.
Attachment #8386825 - Flags: feedback?(nthomas) → feedback+
Assignee

Updated

5 years ago
Blocks: 985813
Assignee

Comment 13

5 years ago
Posted patch stub-mozharness-1.diff (obsolete) — Splinter Review
Refactored script:
* Separated configs for Firefox beta/release/esr to make the configs less dependant on the release configs.
* No need to pass platforms, we process whatever we have in the configs
* No need to pass products, we process whatever we have in the configs
* Removed --no-locales, added need_shipped_locales() to implicitly check that
* EUBallot is just another product in *_release.py, so we can kill 1 builder in process/release.py
* Thunderbird has no *-SSL product, the default product is ssl-only
* If I comment out urlopen() it prints sane URLs (no chance to test agains staging bouncer)
* WCPGW?! :)

custom/configs patches incoming
Attachment #8386825 - Attachment is obsolete: true
Attachment #8393946 - Flags: review?(nthomas)
Assignee

Comment 14

5 years ago
* Kill --platform
* kill bouncer_add_euballot
Attachment #8393947 - Flags: review?(nthomas)
Assignee

Comment 15

5 years ago
* kill extraBouncerPlatforms
* update bouncer_submitter_config
* kill bouncer_add_euballot

I also updated staging *.py files because we don't update them automatically
Attachment #8393948 - Flags: review?(nthomas)
Assignee

Comment 16

5 years ago
The same patch, just changed the TB config to add <product>-<version> and <product>-<version>-SSL.
Attachment #8393946 - Attachment is obsolete: true
Attachment #8393946 - Flags: review?(nthomas)
Attachment #8394096 - Flags: review?(nthomas)
Attachment #8393947 - Flags: review?(nthomas) → review+
Attachment #8393948 - Flags: review?(nthomas) → review+
Comment on attachment 8394096 [details] [diff] [review]
stub-mozharness-2.diff

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

r+ with a couple of changes. I admit my eyes glazing over for the config files, so for more confidence you could look at setting up a tuxedo instance. eg submit using existing code into a clean db, then dump the db; repeat for new code, and diff.

::: configs/releases/bouncer_firefox_esr.py
@@ +81,5 @@
> +            },
> +        },
> +        "stub-installer": {
> +            "product-name": "Firefox-%(version)s-stub",
> +            "ssl-only": True,

We don't have this product currently. I'd leave it out unless there's been a request to add it.

::: configs/releases/bouncer_firefox_release.py
@@ +3,5 @@
> +    "shipped-locales-url": "https://hg.mozilla.org/%(repo)s/raw-file/%(revision)s/browser/locales/shipped-locales",
> +    "products": {
> +        "installer": {
> +            "product-name": "Firefox-%(version)s",
> +            "ssl-only": True,

Per bug 985813, will have to flip this to False

@@ +132,5 @@
> +        },
> +        "EUballot-installer": {
> +            "product-name": "Firefox-%(version)s-EUballot",
> +            "add-locales": False,
> +            "ssl-only": True,

This'll be a change. I think it's fine because no stub, just calling it out.
Attachment #8394096 - Flags: review?(nthomas) → review+
Patches to follow to update the aliases per the summary ?
Assignee

Comment 19

5 years ago
(In reply to Nick Thomas [:nthomas] from comment #18)
> Patches to follow to update the aliases per the summary ?

I haven't looked at those yet, but will be touching them soon.

(In reply to Nick Thomas [:nthomas] from comment #17)
> ::: configs/releases/bouncer_firefox_esr.py
> @@ +81,5 @@
> > +            },
> > +        },
> > +        "stub-installer": {
> > +            "product-name": "Firefox-%(version)s-stub",
> > +            "ssl-only": True,
> 
> We don't have this product currently. I'd leave it out unless there's been a
> request to add it.

The idea is to start adding this, so instead of updating Firefox-latest-stub product's path (and wait for sentry) we will delete this product and create an alias pointing to the versioned products.
 
> 
> @@ +132,5 @@
> > +        },
> > +        "EUballot-installer": {
> > +            "product-name": "Firefox-%(version)s-EUballot",
> > +            "add-locales": False,
> > +            "ssl-only": True,
> 
> This'll be a change. I think it's fine because no stub, just calling it out.

I'll leave it non-ssl for now.

Comment 23

5 years ago
Live in production.
Assignee

Comment 24

5 years ago
Posted patch alias_update-tools.diff (obsolete) — Splinter Review
* not tested
* need to pass use_credentials_file=True to postrelease_factory = ScriptFactory(...)
* example releas config entry (templated):

releaseConfig['bouncer_aliases'] = {
    'Firefox-{{ version }}': 'firefox-latest',
    'Firefox-{{ version }}-stub': 'firefox-stub',
    'Firefox-{{ version }}-EUBallot': 'firefox-latest-euballot',
}
* The entry above could be a list of dictionaries to be more descriptive:
releaseConfig['bouncer_aliases'] = [
    {"alias": "firefox-latest", "related_product": "Firefox-{{ version }}"},
    {"alias": "firefox-stub", "related_product": "Firefox-{{ version }}-stub"},
    ...
}
Attachment #8394870 - Flags: feedback?(bhearsum)
Comment on attachment 8394870 [details] [diff] [review]
alias_update-tools.diff

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

(In reply to Rail Aliiev [:rail] from comment #24)
> releaseConfig['bouncer_aliases'] = {
>     'Firefox-{{ version }}': 'firefox-latest',
>     'Firefox-{{ version }}-stub': 'firefox-stub',
>     'Firefox-{{ version }}-EUBallot': 'firefox-latest-euballot',
> }

I think it'd be better to use %(version)s and let stage-tasks interpolate at run time. I know release runner will sub this in now, but it creates another reconfig-time dependency, which we don't like.

> * The entry above could be a list of dictionaries to be more descriptive:
> releaseConfig['bouncer_aliases'] = [
>     {"alias": "firefox-latest", "related_product": "Firefox-{{ version }}"},
>     {"alias": "firefox-stub", "related_product": "Firefox-{{ version
> }}-stub"},
>     ...
> }

I don't have a preference for the format - do what you want there!
Attachment #8394870 - Flags: feedback?(bhearsum) → feedback-
Assignee

Updated

5 years ago
Duplicate of this bug: 818254
Assignee

Comment 27

5 years ago
typo: need to use an interpolated verison of product_name
Attachment #8395763 - Flags: review?(bhearsum)
Comment on attachment 8395763 [details] [diff] [review]
fix_product_name.diff

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

Need better variable names next time :)
Attachment #8395763 - Flags: review?(bhearsum) → review+
Assignee

Comment 30

5 years ago
(In reply to Ben Hearsum [:bhearsum] from comment #28)
> Need better variable names next time :)

+1000 :)
Assignee

Comment 31

5 years ago
Make ScriptFactory transfer the file with credentials
Attachment #8396093 - Flags: review?(bhearsum)
Assignee

Comment 32

5 years ago
* This is not completely tested because of bug 976101 :/
* The risk is very low because this patch add a step to be run at the end of the script at the very end of the release process. Even if it fails, it wouldn't block anything.
* I ran the script manually (with commented out real POST requests and other steps) and this is what I get 

Beta:
Updating firefox-beta-stub to point to Firefox-29.0b2-stub using https://bounceradmin.mozilla.com/api/create_update_alias
Updating firefox-beta-latest to point to Firefox-29.0b2 using https://bounceradmin.mozilla.com/api/create_update_alias

Release:
Updating firefox-latest-euballot to point to Firefox-28.0-EUBallot using https://bounceradmin.mozilla.com/api/create_update_alias
Updating firefox-stub to point to Firefox-28.0-stub using https://bounceradmin.mozilla.com/api/create_update_alias
Updating firefox-latest to point to Firefox-28.0 using https://bounceradmin.mozilla.com/api/create_update_alias

* Removed retry() because it prints the credentials
Attachment #8396097 - Flags: review?(bhearsum)
Assignee

Comment 33

5 years ago
* No need to update production *.py files

The plan to deploy this is as following:

* Create all needed aliases in Bouncer after 29.0b2 ships (after we run postrelease)
* switch to use aliases for 29.0b2 instead of product
* Land this before 29.0b3
* See how it works (WCPGW?!)
* Migrate 28.0 to use aliases (will need to create a versioned stub entry for 28.0 manually - easy with the current implementation of bouncer submitter, just needs a small config)
* 29.0 will be using this
Attachment #8396100 - Flags: review?(bhearsum)
Assignee

Updated

5 years ago
Attachment #8394870 - Attachment is obsolete: true
Assignee

Comment 35

5 years ago
The link above time out to me. Do they require MTV VPN?
(In reply to Rail Aliiev [:rail] from comment #35)
> The link above time out to me. Do they require MTV VPN?

They do, and per SecOps.
Attachment #8396093 - Flags: review?(bhearsum) → review+
Attachment #8396097 - Flags: review?(bhearsum) → review+
Attachment #8396100 - Flags: review?(bhearsum) → review+
Assignee

Comment 40

5 years ago
* The changes will be available for Firefox 29.0b3
* Added Firefox-29.0b1-stub and Firefox-28.0-stub products using the following local patch https://gist.github.com/rail/9775432
* Renamed the following products:
  Firefox-beta-stub -> Firefox-beta-stub.bak
  Firefox-stub -> Firefox-stub.bak
* Created the following aliases:
  Firefox-beta-stub -> Firefox-29.0b1-stub
  Firefox-stub -> Firefox-28.0-stub
* Updated https://wiki.mozilla.org/Release:Release_Automation_on_Mercurial:Updates#Update_Bouncer with new links to the corresponding aliases instead of products
Comment on attachment 8396100 [details] [diff] [review]
stub-buildbot-configs-2.diff

This patch is now live
Comment on attachment 8396093 [details] [diff] [review]
stub-buildbotcustom-1.diff

This patch is now live
Assignee

Comment 43

5 years ago
(In reply to Ben Hearsum [:bhearsum] from comment #28)
> Comment on attachment 8395763 [details] [diff] [review]
> fix_product_name.diff
> 
> Review of attachment 8395763 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Need better variable names next time :)

Let's doo eet!
Attachment #8397807 - Flags: review?(bhearsum)
Assignee

Comment 44

5 years ago
Attachment #8397808 - Flags: review?(bhearsum)
Attachment #8397808 - Flags: review?(bhearsum) → review+
Attachment #8397807 - Flags: review?(bhearsum) → review+
in production
Assignee

Comment 48

5 years ago
Posted patch vendor.diff (obsolete) — Splinter Review
r+ over my shoulder
Attachment #8398101 - Flags: review+
Assignee

Comment 51

5 years ago
Posted patch add certifi package (obsolete) — Splinter Review
because it failed:

retry: Giving up on <function do_update_bouncer_alias at 0xf976e0>
Traceback (most recent call last):
  File "/builds/slave/rel-m-beta-postrelease-0000000/scripts/scripts/release/stage-tasks.py", line 394, in <module>
    bouncer_aliases=bouncer_aliases)
  File "/builds/slave/rel-m-beta-postrelease-0000000/scripts/scripts/release/stage-tasks.py", line 254, in update_bouncer_aliases
    related_product_template, alias)
  File "/builds/slave/rel-m-beta-postrelease-0000000/scripts/scripts/release/stage-tasks.py", line 270, in update_bouncer_alias
    retry(do_update_bouncer_alias)
  File "/builds/slave/rel-m-beta-postrelease-0000000/scripts/lib/python/util/retry.py", line 34, in retry
    return action(*args, **kwargs)
  File "/builds/slave/rel-m-beta-postrelease-0000000/scripts/scripts/release/stage-tasks.py", line 268, in do_update_bouncer_alias
    requests.post(url, data=data, auth=auth, config={'danger_mode': True})
  File "/builds/slave/rel-m-beta-postrelease-0000000/scripts/lib/python/vendor/requests-0.10.8/requests/api.py", line 84, in post
    return request('post', url, data=data, **kwargs)
  File "/builds/slave/rel-m-beta-postrelease-0000000/scripts/lib/python/vendor/requests-0.10.8/requests/api.py", line 39, in request
    return s.request(method=method, url=url, **kwargs)
  File "/builds/slave/rel-m-beta-postrelease-0000000/scripts/lib/python/vendor/requests-0.10.8/requests/sessions.py", line 203, in request
    r.send(prefetch=prefetch)
  File "/builds/slave/rel-m-beta-postrelease-0000000/scripts/lib/python/vendor/requests-0.10.8/requests/models.py", line 502, in send
    cert_loc = __import__('certifi').where()
ImportError: No module named certifi
Attachment #8398796 - Flags: review?

Updated

5 years ago
Attachment #8398796 - Flags: review? → review+
Assignee

Comment 52

5 years ago
Comment on attachment 8398796 [details] [diff] [review]
add certifi package

This won't help unfortunately because:
requests.exceptions.SSLError: hostname 'bounceradmin.mozilla.com' doesn't match 'download.mozilla.org'
Attachment #8398796 - Attachment is obsolete: true
Assignee

Comment 53

5 years ago
Do not check SSL cert, just like plain urllib.

I tested it by running on the same slave with other stage tasks deleted from the script and it updated the Bouncer aliases!
Attachment #8398101 - Attachment is obsolete: true
Attachment #8398809 - Flags: review?(bhearsum)
Comment on attachment 8398809 [details] [diff] [review]
no_verify.diff

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

Is there a reason we can't access bounceradmin by the name on the certificate?
Comment on attachment 8398809 [details] [diff] [review]
no_verify.diff

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

13:20 < rail> bhearsum: re https://bugzilla.mozilla.org/show_bug.cgi?id=916181#c54, I think requests (or the version we use) can't do SNI
13:20 < bhearsum> no exception afterwards
13:20 < bhearsum> rail: SNI?
13:20 < rail> yeah, multiple SSL certs on 1 ip
13:20 < rail> so it gets the default one
13:20 -!- catlee is now known as catlee-afk
13:20 < bhearsum> beh
13:20 < rail> which is download.m.o
13:20 < bhearsum> securitae
13:21 < bhearsum> i didn't even know you could have multiple names on a cert that isn't a wildcard

We should really do something to fix this, this is bad.
Attachment #8398809 - Flags: review?(bhearsum) → review+
Depends on: 990792
Assignee

Comment 57

5 years ago
All done here!
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.