Deploy PushApkWorker to staging with Puppet

RESOLVED WORKSFORME

Status

Release Engineering
Release Automation: Other
RESOLVED WORKSFORME
2 years ago
2 years ago

People

(Reporter: jlorenzo, Assigned: jlorenzo)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
PushApkWorker[1] works locally. Time to put it live!

[1] https://github.com/JohanLorenzo/pushapkworker
(Assignee)

Updated

2 years ago
Blocks: 1306311
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Depends on: 1307110
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Summary: Deploy PushApkWorker with Puppet → Deploy PushApkWorker to staging with Puppet
(Assignee)

Comment 6

2 years ago
Created attachment 8798047 [details]
PushApkWorker (fork of signingscript)

Here's a first version of PushApkWorker. The exposed configuration is compatible with attachment 8797155 [details]. Its documentation may need enhancements.

It also relies on scriptworker 0.7.0, at the moment. I saw 0.8.0 exposes the download piece, which could be useful here. I'd prefer to upgrade it in a a follow up, though. 

Regarding the direct use of the jarsigner binary, I haven't found any python lib. The examples searchable on GitHub also show a binary wrapper.
Attachment #8798047 - Flags: review?(aki)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 9

2 years ago
mozreview-review
Comment on attachment 8797155 [details]
Bug 1306307 - Deploy PushApkWorker with Puppet

https://reviewboard.mozilla.org/r/82758/#review82220

::: modules/pushapkworker/templates/config.json.erb:5
(Diff revision 7)
> +{
> +    "provisioner_id": "<%= scope.lookupvar("config::scriptworker_provisioner_id") %>",
> +    "worker_group": "<%= scope.lookupvar("config::pushapk_scriptworker_worker_group") %>",
> +    "worker_type": "<%= scope.lookupvar("config::pushapk_scriptworker_worker_type") %>",
> +    "worker_id": "<%= @hostname %>",

For the record, worker IDs can be longer than 22 characters. My current staging machine is named "dev-linux64-ec2-jlorenzo" (25 characters). Based on `hg grep` worker_id is defined by either:
* hostname
* fqdn
* a hardcoded string

I'm not sure what's the best option here. Truncating hostnames doesn't seem like a good idea, either. Is there a way to make puppet to let something like an incremental value, here?
(Assignee)

Updated

2 years ago
Blocks: 1307826
(Assignee)

Updated

2 years ago
No longer blocks: 1306311, 1307826
(Assignee)

Updated

2 years ago
Blocks: 1307826

Comment 10

2 years ago
mozreview-review
Comment on attachment 8797155 [details]
Bug 1306307 - Deploy PushApkWorker with Puppet

https://reviewboard.mozilla.org/r/82758/#review82236

I think this works.
If you want to proceed with your staging instance, you can hardcode your workerId to a <=22char string.  If you want to proceed with a more production-ready instance now, that works as well.

::: manifests/moco-nodes.pp:1167
(Diff revision 7)
>      include toplevel::server::signingscriptworker
>  }
>  
>  ## Loaners
>  
> +node "dev-linux64-ec2-jlorenzo.dev.releng.use1.mozilla.com" {

As mentioned above, we may want to create a permanent location for this for landing, e.g. pushapkworker-1.

You can keep dev-linux64-ec2-jlorenzo and use it in your staging puppet env, but for landing it might be good to create that production env.
Attachment #8797155 - Flags: review?(aki) → review+

Comment 11

2 years ago
Comment on attachment 8798047 [details]
PushApkWorker (fork of signingscript)

I think the below is good to fix before we're fully reliant on this in production as tier1, but doesn't block usage on date.  I'm happy to discuss any of it.  r+ with a request for followup fixes.

README:
> # install pushapkworker from pypi
> python setup.py develop

Nit: `python setup.py develop` installs from the local source tree, rather than pypi, though the dependencies are downloaded from pypi.
`pip install pushapkworker` would install pushapkworker from pypi.

tox.ini:
> usedevelop=True

I think @lundjordan found that if you create a `test/__init__.py`, this is no longer necessary.

I think `pushapkworker/data/signing_task_schema.json` should be renamed to `pushapk_task_schema.json` or something.

`JarSigner`: this can probably be a function.  I don't object too strongly to it being an object, but noting that it's an object with one method, and the __init__ function only sets some defaults.

```
        try:
            self.binary_path = context.config['jarsigner_binary']
        except KeyError:
            self.binary_path = 'jarsigner'
```

This could be

```
    self.binary_path = context.config.get("jarsigner_binary", "jarsigner")
```

Same with `self.certificate_aliases`.

`utils.py` re-defines `download_file`, when it's in scriptworker.  I'm ok with this duplication if it's known and conscious.  One reason to not import from scriptworker is scriptworker requires python 3.5, but as written, so does `pushapkworker.utils.download_file`.

Similarly, `task.py` defines `download_files`, which is similar to `scriptworker.task.download_artifacts`, and `validate_task_schema`, which is similar to `scriptworker.client.validate_json_schema`.  Are these duplications intentional?

```python
    log.info('Downloading APKs...')
    with aiohttp.ClientSession() as base_ssl_session:
        orig_session = context.session
        context.session = base_ssl_session
        downloaded_apks = await download_files(context)
        context.session = orig_session
```

In signingscript, I replaced the `context.session` because the signing servers have a special SSL context, and downloading artifacts needed a normal SSL context.  I don't think you need this same workaround.

Outside of that, [google docstrings](http://sphinxcontrib-napoleon.readthedocs.io/en/latest/example_google.html) and full test coverage would be great as a goal.  I know signingscript is currently missing those; it's on my todo list.
Attachment #8798047 - Flags: review?(aki) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 13

2 years ago
Thanks for the detailed reviews Aki!

I made the changes on pushapkwoker[1]. For the record, I started the worker with scriptworker 0.6.0 (and not 0.7.0, like I said comment 6), that's why the most of the duplication was there. As the worker uses 0.7.0 anyway, I removed that.

I also published the worker on Pypi, just for parity with the README.

We're now at 78% of coverage[2]. Testing must be done in script.py, mainly. I'll handle that. 

Regarding google docstring. I'm not too sure of the end usage here. As our packages are mostly final executable, I wonder what kind of people are willing to import *worker in their own python.

[1] https://github.com/mozilla-releng/pushapkworker/compare/336e65738bf1097481d917fe83c53014b72cf240...0.1.1
[2] https://coveralls.io/builds/8217114
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 20

2 years ago
The staging configuration should be in a stable state from now on. Here's what basically changed since Aki's review:
* A rebase on top of the latest tip (this added changes related to balrogworker and beetmoverworker)
* Use mozapkpublisher and pushapkscript (renamed from pushapkworker) 0.1.3
* Make a part of the configuration dependent of the type of the environment 

I'm gonna now work on the actual production configuration. This will be handled in bug 1307826. Closing this bug, without landing the staging changes onto hg.m.o/build/puppet.

Interdiff changes: https://reviewboard.mozilla.org/r/82758/diff/8-14/
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WORKSFORME
(Assignee)

Updated

2 years ago
Blocks: 1320901
You need to log in before you can comment on or make changes to this bug.