1447673 - adjust beetmoverscript to handle partner repacks

Status: RESOLVED FIXED

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

Comment 1

[Mihai Tabara [:mtabara]⌚️GMT]

Attachment: Beetmoverscript changes for partner repacks

@aki:

Status: I went ahead with the solution we've discussed last week with full artifact paths coming from in-tree in the upstreamArtifacts

I still have a list of improvements to the PR, but throughing this to you for an initial feedback.

Also, quick question: should we have EME-free repacks follow the pattern with full paths instead of artifacts in the upstreamArtifacts? Or should it be an exception treated as such in the beetmoverscript side. 

Both are sort of straightforward to implement, just wanted to ask for your opinion.

Comment 2

[Aki Sasaki (not active)]

Hm, let's discuss with Nick, because he's the one porting the script that will be creating the artifact paths.

Nick: the concept here is that we'll lay out the artifact paths the way we want them to look on S3. So if we want

public://.../version-candidates/partner-repacks/path/to/binary.exe, then we can save the artifact as releng/partner/path/to/target.exe in the build task. The signing task will preserve the path, and then beetmover can use the existing relative path rather than having to get all the metadata that allows it to build the relative path itself.

(Same for private; I'm hoping private://.../version-buildnum/path/to/binary.exe works there too.)

I suppose I have 3 questions:

- does this sound good to you?
- is this straightforward for the build script?
- can eme-free's build script also follow this pattern?

I'm guessing beetmover can handle the prefix before the path/to, e.g. pub/firefox/candidates/$version-candidates/partner-repacks

Comment 3

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

Yes, that sounds good. I think the repackage jobs will need to preserve the artifact path as well as signing, but we can probably handle by setting name and path correctly in the task definition.

The repacking script has all the config information and should be able to construct the path to create the files without issue, I'll know for sure in the next day or so. And we have it at task definition time to declare specific artifacts.

EME-free uses the same script as everything else, just different configs via a different manifest url, so that should be fine. A couple of questions there though. 

- Will we use the releng/partner prefix for EME-free, or the usual public/build ? It looks like maple is doing the latter, which is fine by me, but beetmover would have to be OK too.

- For public bits in general, should we allow repack.cfg to continue to specify the output_dir ? eg partner-repacks/mailru/v3/mailru/%(platform)s/%(locale)s, %(platform)s-EME-free/%(locale)s. It's quite attractive to store that in one place and feed it to the repacking script and to beetmover, but I'm skating over the implementation details in the latter. We've talked about config parameters to enable upload to private/public locations too.

Comment 4

[Mihai Tabara [:mtabara]⌚️GMT]

(In reply to Nick Thomas [:nthomas] (UTC+13) from comment #3)
> EME-free uses the same script as everything else, just different configs via
> a different manifest url, so that should be fine. A couple of questions
> there though. 

this is great to hear!

> - Will we use the releng/partner prefix for EME-free, or the usual
> public/build ? It looks like maple is doing the latter, which is fine by me,
> but beetmover would have to be OK too.

For private + public ones I've hardcoded this in constants.py but Aki was suggesting we can send that via the task definition. If worse comes to worst, I guess I can just add the 'public/build' to the list. Or we can have it parameterized in the task-definition so that beetmover jobs just consume it. So we have options, this shouldn't be a problem downstream
 
> - For public bits in general, should we allow repack.cfg to continue to
> specify the output_dir ? eg
> partner-repacks/mailru/v3/mailru/%(platform)s/%(locale)s,
> %(platform)s-EME-free/%(locale)s. It's quite attractive to store that in one
> place and feed it to the repacking script and to beetmover, but I'm skating
> over the implementation details in the latter. We've talked about config
> parameters to enable upload to private/public locations too.

Not sure I understand precisely. But if you're saying that ideally we have some parameter that we can pass down that says where each artifact go where, that is music to my ears. The way we'd like to implement (and so far my PR is) beetmover is that it should be as "consistent" as possible. That is, if we can make EME-free-repacks more consistent with the rest of the private + public repackages, that's ideal. Less exceptions means cleaner implementation.

I'll double check with Aki when he comes online to make sure I understood, but sounds like the current approach is on a good path so I can follow-up with testing + in-tree patches.
Thanks!

Comment 5

[Mihai Tabara [:mtabara]⌚️GMT]

To sum-up current state. Beetmoverscript PR is ready with 100% code coverage here[1]. It covers private + public artifacts, not yet EME-free. We can get them for free sans any other change here if we make them look more like the "public tasks". More below.

What do we still need to do?

In puppet:
* we need to amend the script_config.json[2] for beetmover to encompass `push-to-partner` action too
* we need to amend the script_config.json[3] production section (well, only when we're production ready theoretically) with `partner` section, something like this[4]

In-tree:
a) all-tasks (regardless if they are private or public partner tasks)
* set payload:build_number
* set payload.version set
* nick's script needs to amend the artifacts to preserve absolute path and start with `/releng/partner`. We can then pass it via tasks payload or just rely on the hardcoded version from beetmover[5]
* change their action s/push-to-candidates/push-to-partner
* fix the locale in upstreamArtifacts which now says locale = eme-free instead of en-US or w\e other locale

b) the private beetmover jobs with public artifacts
* set payload.is_partner_repack_public: true for public beetmover subset of tasks. The variable is hardcoded but can be passed down the same way as `releng/partner` if we wanted to. See it in beetmover here[6] 

What will happen assuming in-tree fixes are done?
a) private artifacts should be pushed under http://ftp.stage.mozaws.net/pub/firefox/candidates/$version-candidates/$build_number/partner-repacks/$partner/$version-$build_number/$partner-variant/v1/...
b) public artifacts should be pushed under http://ftp.stage.mozaws.net/pub/firefox/candidates/$version-candidates/$build_number/partner-repacks/$partner/$partner-variant/v1/...

(so there's a subtle version-buildno between $partner and $partner-variant that makes the difference. This is valid only for dep testing in staging to be able to test these in staging. More explanations here[7].

c) we get EME-free for free if we're changing their upstreamArtifacts paths to be more like the public ones: e.g. releng/partner/%(platform)s-EME-free/%(locale)s


[1]: https://github.com/mozilla-releng/beetmoverscript/pull/112/
[2]: https://hg.mozilla.org/build/puppet/file/tip/modules/beetmover_scriptworker/templates/base_script_config.json.erb
[3]: https://hg.mozilla.org/build/puppet/file/tip/modules/beetmover_scriptworker/templates/prod_script_config.json.erb#l39
[4]: https://github.com/mozilla-releng/beetmoverscript/pull/112/files#diff-3a83cd669a41d9a6a85f33103bcf7a9cR69
[5]: https://github.com/mozilla-releng/beetmoverscript/pull/112/files#diff-67ea3dc94b3e0a4aaeb35e229f9254bdR169
[6]: https://github.com/mozilla-releng/beetmoverscript/pull/112/files#diff-67ea3dc94b3e0a4aaeb35e229f9254bdR171
[7]: https://github.com/mozilla-releng/beetmoverscript/pull/112/files#r179570877

Comment 6

[Aki Sasaki (not active)]

Attachment: puppet-patch

Comment 7

[Mihai Tabara [:mtabara]⌚️GMT]

Comment on attachment 8969001 [details] [diff] [review]
puppet-patch

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

We might want to revisit the script_config.json model in beetmover. It's slightly confusing having to add these in two different places. I think @jlorenzo had a slightly different approach in the rest of scripts where all these values were generated in settings.pp rather than templates. But I guess in the end, we might end up having the same logic as we need `dep` in both dev and prod environments.

Comment 8

[Aki Sasaki (not active)]

(In reply to Mihai Tabara [:mtabara]⌚️GMT from comment #7)
> We might want to revisit the script_config.json model in beetmover. It's
> slightly confusing having to add these in two different places. I think
> @jlorenzo had a slightly different approach in the rest of scripts where all
> these values were generated in settings.pp rather than templates. But I
> guess in the end, we might end up having the same logic as we need `dep` in
> both dev and prod environments.

Sure. I don't have a strong opinion here, but I'd prefer if the *script modules in puppet were more consistent, so when we need to roll out changes to all scriptworkers, it's easier.

Comment 9

[Aki Sasaki (not active)]

https://hg.mozilla.org/build/puppet/rev/7ff26405d6d2

Comment 10

[Mihai Tabara [:mtabara]⌚️GMT]

(In reply to Aki Sasaki [:aki] from comment #8)
> (In reply to Mihai Tabara [:mtabara]⌚️GMT from comment #7)
> > We might want to revisit the script_config.json model in beetmover. It's
> > slightly confusing having to add these in two different places. I think
> > @jlorenzo had a slightly different approach in the rest of scripts where all
> > these values were generated in settings.pp rather than templates. But I
> > guess in the end, we might end up having the same logic as we need `dep` in
> > both dev and prod environments.
> 
> Sure. I don't have a strong opinion here, but I'd prefer if the *script
> modules in puppet were more consistent, so when we need to roll out changes
> to all scriptworkers, it's easier.

Sounds good, bug 1455231 is now tracking that work.

Comment 11

[Aki Sasaki (not active)]

Thanks!