1083584 - Update tools/update-packaging/make_incremental_update.sh to allow funsize caching of patches

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Mihai Tabara [:mtabara]⌚️GMT]

In our attempt to optimize the partial MAR generating with funsize we want to cache the patches so that they can be reused (most of the partial files patches are the same across locales). For that, we use some external tools and service specified via env vars as follows:

FUNSIZE_URL - url where funsize fronend lies
MBSDIFF_HOOK - tool that contains the methods to communicate with funsize frontend
(optional)FUNSIZE_LOCAL_CACHE_DIR - local dir for caching on worker's disk

Comment 1

[Mihai Tabara [:mtabara]⌚️GMT]

Attachment: Patch to allow partial files to get uploaded to funsize.

Changes to tools/update-packaging/make_incremental_update.sh to enable funsize contact.

Comment 2

[Rail Aliiev [:rail]]

Comment on attachment 8505870 [details] [diff] [review]
Patch to allow partial files to get uploaded to funsize.

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

::: client/update-packaging/make_incremental_update.sh
@@ +191,5 @@
> +        $MBSDIFF "$olddir/$f" "$newdir/$f" "$workdir/$f.patch"
> +      else
> +        # if service enabled then check patch existence for retrieval
> +        if $MBSDIFF_HOOK -g "$olddir/$f" "$newdir/$f" "$workdir/$f.patch" \
> +            $FUNSIZE_URL; then

In overall it looks good to me. Just one optimization can be doe here.

You check if $MBSDIFF_HOOK is set but don't check $FUNSIZE_URL. I'd probably dropped $FUNSIZE_URL all together and just set it as a part of $MBSDIFF_HOOK. Something like:

export MBSDIFF_HOOK="myscript.exe -a https://funsize/api"

This would require some changes in your script, but it makes the call is more generic, so you don't rely on the current implementation. Maybe someone would like to use something else (like local cache, without any upload/download).

Does it make sense?

@@ +204,1 @@
>        $BZIP2 -z9 "$workdir/$f.patch"

Hmm, this line above means that you cache uncompressed patches... Using compressed cache would make the script  work faster, but would also need to add extra conditions to the patch...

Comment 3

[Mihai Tabara [:mtabara]⌚️GMT]

Attachment: Patch to allow partial files to get uploaded to funsize.

(Sorry, just realized I patched my local copy file instead of the m-c one)

Patch to allow partial files to get uploaded to funsize.

Changes to tools/update-packaging/make_incremental_update.sh to enable funsize contact.

Comment 4

[Rail Aliiev [:rail]]

Comment on attachment 8505888 [details] [diff] [review]
Patch to allow partial files to get uploaded to funsize.

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

comment #2 is still applicable.

Comment 5

[Mihai Tabara [:mtabara]⌚️GMT]

(In reply to Rail Aliiev [:rail] from comment #2)
> 
> In overall it looks good to me. Just one optimization can be doe here.
> 
> You check if $MBSDIFF_HOOK is set but don't check $FUNSIZE_URL. I'd probably
> dropped $FUNSIZE_URL all together and just set it as a part of
> $MBSDIFF_HOOK. Something like:
> 
> export MBSDIFF_HOOK="myscript.exe -a https://funsize/api"
> 
> This would require some changes in your script, but it makes the call is
> more generic, so you don't rely on the current implementation. Maybe someone
> would like to use something else (like local cache, without any
> upload/download).
>
> Does it make sense?

Yes, makes sense. Still, we should document this - how to actually use the funsize_common.sh, etc - very well somewhere - (maybe the funsize wiki page) or else it will look like black magic for an unknown developer reading this.
 
> Hmm, this line above means that you cache uncompressed patches... Using
> compressed cache would make the script  work faster, but would also need to
> add extra conditions to the patch...

Yea, this looks good. Will add it it necessary.

I'll wait for feedback from bhearsum and nthomas too before I re-patch to avoid spamming with this.

Comment 6

[[e-mail] (:bhearsum)]

Comment on attachment 8505888 [details] [diff] [review]
Patch to allow partial files to get uploaded to funsize.

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

(In reply to Rail Aliiev [:rail] from comment #2)
> You check if $MBSDIFF_HOOK is set but don't check $FUNSIZE_URL. I'd probably
> dropped $FUNSIZE_URL all together and just set it as a part of
> $MBSDIFF_HOOK. Something like:
> 
> export MBSDIFF_HOOK="myscript.exe -a https://funsize/api"
> 
> This would require some changes in your script, but it makes the call is
> more generic, so you don't rely on the current implementation. Maybe someone
> would like to use something else (like local cache, without any
> upload/download).

MOZ_SIGN_CMD might be a good model for this. It's a nearly complete command that just has a few args appended to it when invoked. It seems like FUNSIZE_LOCAL_CACHE_DIR migth be able to be defined in here too? It's hard to tell, because I don't see it getting used at all at the moment.

Rob strong probably needs to look over this at some point (for a final review, at least).

Comment 7

[Mihai Tabara [:mtabara]⌚️GMT]

The script behind MBSDIFF_HOOK looks like this: https://github.com/MihaiTabara/build-funsize/blob/master/client/update-packaging/funsize_common.sh

Comment 8

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

Comment on attachment 8505888 [details] [diff] [review]
Patch to allow partial files to get uploaded to funsize.

I agree it would be good to stash the bzipped diff in funsize, although you might have to store it using hashes from uncompressed files to fit in with this shell script. Dunno how to avoid that being confusing later other than copious comments.

I'm still a fan of modifying make_incremental_updates.py, because that doesn't decompress the complete mar, just uses mar -x on them.

Comment 9

[Mihai Tabara [:mtabara]⌚️GMT]

Attachment: Patch to allow partial patches to be cached with funsize.

Changes to tools/update-packaging/make_incremental_update.sh to enable funsize contact for caching partial patches.

Observation:
* fwiw, MBSDIFF script looks like this: https://github.com/MihaiTabara/build-funsize/blob/master/client/update-packaging/funsize_common.sh

Comment 10

[Mihai Tabara [:mtabara]⌚️GMT]

I'll add more info about funsize to enrich the context here.

The <funsize> generates partial MAR (Mozilla ARchive) files for updates from Version A to Version B of Firefox on demand. It is designed to iteratively replace the current way of generating partials during build process to get more flexibility and speed. The project will lie in the Elastic Beanstalk Amazon cloud as a webapp  providing access to its logic via GET/POST requests. It will interact with Amazon S3 to store & cache the patches of a partial mar file. 

Disclaimer:
* this new code will be turned on inside funsize machines only!
* we'll use the same code as we currently use on the build machines

Comment 11

[Mihai Tabara [:mtabara]⌚️GMT]

Attachment: Patch to allow partial patches to be cached with funsize.

Patch to allow partial patches to be cached with funsize.

(repatched the file, removed a flag from BZIP)

Comments #9 and #10 still apply to the context.

Comment 12

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

Comment on attachment 8509130 [details] [diff] [review]
Patch to allow partial patches to be cached with funsize.

Looks straightforward enough

Comment 13

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

Comment on attachment 8509130 [details] [diff] [review]
Patch to allow partial patches to be cached with funsize.

https://hg.mozilla.org/integration/mozilla-inbound/rev/63f4c2e0a51a

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/63f4c2e0a51a