Closed Bug 1083584 Opened 8 years ago Closed 8 years ago

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

Categories

(Release Engineering :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mtabara, Assigned: mtabara)

References

Details

(Whiteboard: [funsize])

Attachments

(2 files, 2 obsolete files)

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
Assignee: mitchell → tabara.mihai
Component: Miscellaneous → Tools
Product: mozilla.org → Release Engineering
QA Contact: hwine
Hardware: x86 → All
Whiteboard: [funsize]
Version: other → unspecified
Changes to tools/update-packaging/make_incremental_update.sh to enable funsize contact.
Assignee: tabara.mihai → mtabara
Attachment #8505870 - Flags: review?(bhearsum)
Attachment #8505870 - Flags: feedback?(rail)
Attachment #8505870 - Flags: feedback?(nthomas)
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...
Attachment #8505870 - Flags: feedback?(rail) → feedback+
(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.
Attachment #8505870 - Attachment is obsolete: true
Attachment #8505870 - Flags: review?(bhearsum)
Attachment #8505870 - Flags: feedback?(nthomas)
Attachment #8505888 - Flags: review?(bhearsum)
Attachment #8505888 - Flags: feedback?(rail)
Attachment #8505888 - Flags: feedback?(nthomas)
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.
Attachment #8505888 - Flags: feedback?(rail) → feedback+
(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 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).
Attachment #8505888 - Flags: review?(bhearsum) → feedback+
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.
Attachment #8505888 - Flags: feedback?(nthomas) → feedback+
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
Attachment #8508285 - Flags: review?(robert.strong.bugs)
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
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.
Attachment #8508285 - Attachment is obsolete: true
Attachment #8508285 - Flags: review?(robert.strong.bugs)
Attachment #8509130 - Flags: review?(robert.strong.bugs)
Comment on attachment 8509130 [details] [diff] [review]
Patch to allow partial patches to be cached with funsize.

Looks straightforward enough
Attachment #8509130 - Flags: review?(robert.strong.bugs) → review+
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
Attachment #8509130 - Flags: checked-in+
https://hg.mozilla.org/mozilla-central/rev/63f4c2e0a51a
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Blocks: funsize
Component: Tools → General
You need to log in before you can comment on or make changes to this bug.