Closed
Bug 1083584
Opened 10 years ago
Closed 10 years ago
Update tools/update-packaging/make_incremental_update.sh to allow funsize caching of patches
Categories
(Release Engineering :: General, defect)
Release Engineering
General
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: mtabara, Assigned: mtabara)
References
Details
(Whiteboard: [funsize])
Attachments
(2 files, 2 obsolete files)
1.82 KB,
patch
|
nthomas
:
feedback+
rail
:
feedback+
bhearsum
:
feedback+
|
Details | Diff | Splinter Review |
2.34 KB,
patch
|
robert.strong.bugs
:
review+
nthomas
:
checked-in+
|
Details | Diff | Splinter Review |
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 | ||
Updated•10 years ago
|
Assignee: mitchell → tabara.mihai
Component: Miscellaneous → Tools
Product: mozilla.org → Release Engineering
QA Contact: hwine
Hardware: x86 → All
Whiteboard: [funsize]
Version: other → unspecified
Assignee | ||
Comment 1•10 years ago
|
||
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 2•10 years ago
|
||
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+
Assignee | ||
Comment 3•10 years ago
|
||
(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 4•10 years ago
|
||
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+
Assignee | ||
Comment 5•10 years ago
|
||
(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•10 years ago
|
||
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+
Assignee | ||
Comment 7•10 years ago
|
||
The script behind MBSDIFF_HOOK looks like this: https://github.com/MihaiTabara/build-funsize/blob/master/client/update-packaging/funsize_common.sh
Comment 8•10 years ago
|
||
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+
Assignee | ||
Comment 9•10 years ago
|
||
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)
Assignee | ||
Comment 10•10 years ago
|
||
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
Assignee | ||
Comment 11•10 years ago
|
||
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 12•10 years ago
|
||
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 13•10 years ago
|
||
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+
Comment 14•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Updated•8 years ago
|
Component: Tools → General
You need to log in
before you can comment on or make changes to this bug.
Description
•