Closed
Bug 920485
Opened 11 years ago
Closed 11 years ago
New tooltool deployment
Categories
(Release Engineering :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: sbruno, Assigned: sbruno)
References
Details
Attachments
(11 files, 8 obsolete files)
(deleted),
patch
|
sbruno
:
checked-in+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
rail
:
review+
sbruno
:
checked-in+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
rail
:
review+
sbruno
:
checked-in+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
rail
:
review+
sbruno
:
checked-in+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
rail
:
review+
sbruno
:
checked-in+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
rail
:
review+
sbruno
:
checked-in+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
rail
:
review+
bhearsum
:
checked-in-
|
Details | Diff | Splinter Review |
(deleted),
patch
|
rail
:
review+
sbruno
:
checked-in+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
rail
:
review+
sbruno
:
checked-in+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
rail
:
review+
sbruno
:
checked-in+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mshal
:
review+
sbruno
:
checked-in+
|
Details | Diff | Splinter Review |
A number of changes have been made to the tooltool codebase in the past weeks.
It's now time to go live with all these changes!
The purpose of this bug is to keep track of the status of the deployment of all the pending changes.
Assignee | ||
Updated•11 years ago
|
Assignee | ||
Comment 1•11 years ago
|
||
Current status
--------------
There are pending patch approval requests on Bugs 772190 and 887174.
When these will be completed as well, I will merge all the changes to a consolidated version.
Once this is completed, we will need to make sure that the new tooltool is distributed, the upload mechanism is in place, and the environment variables required to enable the cache cleanup.
This bug will be used to track availability of updated documentation as well.
Assignee | ||
Comment 2•11 years ago
|
||
Current status
--------------
Bug 887174 has been r+'d; Bug 772190 has also been r+'d, apart from the email notification mechanism which is currently being reviewed.
Since the email notification mechanism is a relatively small patch, I started already merging the patches together.
Assignee | ||
Comment 3•11 years ago
|
||
Current status
--------------
The only blocking part at this stage is the mail notification mechanism, which is part of Bug 772190.
A first patch had been r-'d by rail; I now included his observations in a second patch which is currently being reviewed.
Assignee | ||
Comment 4•11 years ago
|
||
I finally completed the merge of changes 768123, 772190, 858635, 887174 and 887179 on top of the existing codebase (identified by git commit Id 394f8bd58be564267cc786104170ceacba72e056).
The resulting codebase is https://github.com/simone-mozilla/tooltool/tree/212c7b33a9d971aca2d370a463750645f60e7f44.
This patch has been created with the following diff command:
git diff -U8 394f8 212c7b33a9d971aca2d370a463750645f60e7f44
Assignee | ||
Comment 5•11 years ago
|
||
After some tests, I found some functional conflicts between some of the previously merged Bugs, precisely Bug 887174 and Bug 887179.
This patch is a set of extra changes, to be applied on top of the previous changes, that were necessary to solve this conflict.
Command to generate patch:
git diff -U8 212c7b33a9d971aca2d370a463750645f60e7f44 9fd02fcf7fe83978909aad595bef0d4e9e1ac0ee
Github tree: https://github.com/simone-mozilla/tooltool/commits/Bug_920485
Attachment #820699 -
Flags: review?(rail)
Assignee | ||
Comment 6•11 years ago
|
||
Current status
--------------
All changes related to tooltool.py and sync.py have been merged and are available in the previous two patches.
The cache related changes (Bug 858635) require the deployment of patch https://bugzilla.mozilla.org/attachment.cgi?id=758509 as well (affecting purge_builds.py), AND the setting on all involved machines of the environment variables TOOLTOOL_HOME and TOOLTOOL_CACHE.
Comment 7•11 years ago
|
||
Comment on attachment 820699 [details] [diff] [review]
Bug_920485_2_extra-changes.diff
Review of attachment 820699 [details] [diff] [review]:
-----------------------------------------------------------------
Some nits below. Otherwise looks good.
::: tooltool.py
@@ +437,5 @@
>
> + # cleanup temp file in case of issues
> + if fetched_path:
> + return os.path.split(fetched_path)[1]
> + else:
can you remove the trailing space here
@@ +443,5 @@
> os.remove(temp_path)
> + except OSError:
> + pass
> + return None
> +
the same here
Attachment #820699 -
Flags: review?(rail) → review+
Assignee | ||
Comment 8•11 years ago
|
||
Attachment #824170 -
Flags: review?(rail)
Assignee | ||
Comment 9•11 years ago
|
||
The previous patch has been created with the following command:
git diff -U8 8181a768f2c546abdbf6488cc6676070a5b8bc0a 5d9f3aab05af0714bc672f959b2b051d46e63569 > minor1.diff
Updated•11 years ago
|
Attachment #824170 -
Flags: review?(rail) → review+
Assignee | ||
Comment 10•11 years ago
|
||
Current status
--------------
All patches are ready to be landed (and I will be doing this since I now have commit access to all the relevant repos ( btw, thanks Rail for landing previous changes!)).
In order to enable multiple servers support, it would be better to have the new upload mechanism in place; this is currently on hold since the server used to test the upload mechanism will probably not be the actual prod server ( see Bug 930029 ).
Therefore I will start landing the cache related fix first (and the corresponding cleanup code).
As soon as the actual production upload servers will be ready, I will land the remaining patches.
As a final note, Bug 884347 has been reopened and assigned to another reviewer to perform an extra OpSec review.
Depends on: 884347
Assignee | ||
Comment 11•11 years ago
|
||
Small amendment: actually it will not be necessary to land separately the cache related code changes and the other ones since, even if an upload server is not available, the corresponding changes in the tooltool client do not introduce any backward incompatibilities.
Assignee | ||
Comment 12•11 years ago
|
||
The actual rollout has finally begun!
All changes to the tooltool client have now been pushed to the http://hg.mozilla.org/build/puppet repo.
After verifying that it does not cause problems to the trees and it is as backward compatible as it is supposed to be, I'll proceed with the next phases of the rollout:
1. Enabling the cache
2. Pointing tooltool to the new virtual hosts as per Bug 882712
3. Switch to the new upload procedure and make sure that proper documentation is available for uploaders
Assignee | ||
Comment 13•11 years ago
|
||
Status update
The new tooltool is now being distributed by puppet and used in all builds.
Apparently no bacwward incompatibilities were introduced with the changes.
Now I am working on the rollout of the cache related changes.
Basically there are two calling scripts for the tooltool fetch command, which could benefit of the new cache feature:
http://mxr.mozilla.org/build/source/tools/scripts/tooltool/fetch_and_unpack.sh
and
http://mxr.mozilla.org/build/source/mozharness/mozharness/mozilla/tooltool.py
Furthermore, the cache folder needs to be setup for each platform in a conventient location.
I am going to upload the corresponding patches.
Assignee | ||
Comment 14•11 years ago
|
||
Assignee | ||
Comment 15•11 years ago
|
||
Assignee | ||
Comment 16•11 years ago
|
||
The previous patches could be landed as they are, but this would not introduce any different behaviour since a third patch is needed in order to configure the actual folder to be used for caching and passing this info to both the mozharness tooltool script and to the fetch one.
I will submit the patches for review as soon as also this last bit is ready.
Assignee | ||
Comment 17•11 years ago
|
||
These changes set the TOOLTOOL_CACHE env variable on unix based build machines. This variable will tell tooltool to use the specified folder as local cache folder.
Attachment #8363626 -
Attachment is obsolete: true
Attachment #8363627 -
Attachment is obsolete: true
Assignee | ||
Comment 18•11 years ago
|
||
This patch enables the tooltool cache option in all mozharness calls, when the TOOLTOOL_CACHE environment variable is set.
Assignee | ||
Comment 19•11 years ago
|
||
Creation via puppet of the folder to be used as tooltool local cache.
Assignee | ||
Comment 20•11 years ago
|
||
Enables tooltool cache in non-mozharness call + purge logic in purge_builds.py
Assignee | ||
Updated•11 years ago
|
Attachment #8367232 -
Flags: review?(rail)
Assignee | ||
Updated•11 years ago
|
Attachment #8367234 -
Flags: review?(rail)
Assignee | ||
Updated•11 years ago
|
Attachment #8367235 -
Flags: review?(rail)
Assignee | ||
Updated•11 years ago
|
Attachment #8367236 -
Flags: review?(rail)
Assignee | ||
Updated•11 years ago
|
Attachment #820695 -
Flags: checked-in+
Assignee | ||
Updated•11 years ago
|
Attachment #820699 -
Flags: checked-in+
Assignee | ||
Updated•11 years ago
|
Attachment #824170 -
Flags: checked-in+
Assignee | ||
Comment 21•11 years ago
|
||
As previous version of 920485_cache_buildbot-config.diff, plus setting of TOOLTOOL_HOME to enable purge.
Attachment #8367232 -
Attachment is obsolete: true
Attachment #8367232 -
Flags: review?(rail)
Attachment #8367288 -
Flags: review?(rail)
Updated•11 years ago
|
Attachment #8367235 -
Flags: review?(rail) → review+
Updated•11 years ago
|
Attachment #8367236 -
Flags: review?(rail) → review+
Updated•11 years ago
|
Attachment #8367288 -
Flags: review?(rail) → review+
Comment 22•11 years ago
|
||
Comment on attachment 8367234 [details] [diff] [review]
920485_cache_mozharness.diff
>> aki
I'm not a big fan of using environment variables, especially if we can avoid it. It makes it harder to reproduce a job (you need to copy paste the env). I'd rather explicitly pass a parameter and hard code the paths in the mozharness configs.
Attachment #8367234 -
Flags: review?(rail) → review?(aki)
Assignee | ||
Comment 23•11 years ago
|
||
Thanks for the reviews Rail!
Some considerations:
At the moment, TOOLTOOL_CACHE is defined in two points:
- as an environment variable, which is used by fetch_and_unpack.sh and by the purge script
- in puppet, to create the cache folder
I understand the direct injection of env variables in the script is not an elegant solution. I also noticed that it is not consistent with how mozharness is currently managing configuration settings. But if we decide to hardcode it in the mozharness configuration, we will have the same information in one more point - which is also not ideal.
Let's see what Aki says (I bet he'll agree not to use the env var!), and I'll fix the patch accordingly.
Thanks again!
Comment 24•11 years ago
|
||
Comment on attachment 8367234 [details] [diff] [review]
920485_cache_mozharness.diff
We have a precedent of using an env var:
http://hg.mozilla.org/build/mozharness/file/778d2473a929/mozharness/base/vcs/mercurial.py#l445
Could you follow suit, so that we can set this via config file or env var?
cache_dir=self.config.get('tooltool_cache_dir', os.environ.get("TOOLTOOL_CACHE"))
Attachment #8367234 -
Flags: review?(aki) → review+
Assignee | ||
Comment 25•11 years ago
|
||
I changed the patch following the pattern recommended by Aki (thanks Aki!).
Attachment #8367234 -
Attachment is obsolete: true
Attachment #8368593 -
Flags: review?(rail)
Comment 26•11 years ago
|
||
Comment on attachment 8368593 [details] [diff] [review]
920485_cache_mozharness.diff
Review of attachment 8368593 [details] [diff] [review]:
-----------------------------------------------------------------
A couple of issues and we are ready to go! :)
::: mozharness/mozilla/testing/unittest.py
@@ +173,5 @@
> """ Currently dependent on both TooltoolMixin and TestingMixin)"""
>
> def install_emulator_from_tooltool(self, manifest_path):
> dirs = self.query_abs_dirs()
> + if self.tooltool_fetch(manifest_path, output_dir=dirs['abs_work_dir'], cache_dir=os.environ.get("TOOLTOOL_CACHE")):
Any reason why not use the self.config approach here too?
::: scripts/b2g_build.py
@@ +652,5 @@
> manifest = os.path.abspath(os.path.join(config_dir, gecko_config['tooltool_manifest']))
> self.tooltool_fetch(manifest=manifest,
> bootstrap_cmd=gecko_config.get('tooltool_bootstrap_cmd'),
> + output_dir=dirs['work_dir'],
> + cache_dir=gecko_config.get('tooltool_cache', os.environ.get("TOOLTOOL_CACHE", None)))
Hmmmm, I think you need to use self.config here. gecko_config lives in the mozilla-central tree...
Attachment #8368593 -
Flags: review?(rail) → review-
Assignee | ||
Comment 27•11 years ago
|
||
New Attempt!
Attachment #8368593 -
Attachment is obsolete: true
Attachment #8368644 -
Flags: review?(rail)
Comment 28•11 years ago
|
||
Comment on attachment 8368644 [details] [diff] [review]
920485_cache_mozharness.diff
Review of attachment 8368644 [details] [diff] [review]:
-----------------------------------------------------------------
r+ with some fixes:
::: mozharness/mozilla/testing/unittest.py
@@ +173,5 @@
> """ Currently dependent on both TooltoolMixin and TestingMixin)"""
>
> def install_emulator_from_tooltool(self, manifest_path):
> dirs = self.query_abs_dirs()
> + cache_dir=self.config.get('tooltool_cache', os.environ.get("TOOLTOOL_CACHE", None))
You used self.config['tooltool_config'].get('tooltool_cache'... below. You should use it here too.
::: scripts/b2g_build.py
@@ +652,5 @@
> manifest = os.path.abspath(os.path.join(config_dir, gecko_config['tooltool_manifest']))
> self.tooltool_fetch(manifest=manifest,
> bootstrap_cmd=gecko_config.get('tooltool_bootstrap_cmd'),
> + output_dir=dirs['work_dir'],
> + cache_dir=self.config.get('tooltool_cache', os.environ.get("TOOLTOOL_CACHE", None)))
The same here.
Attachment #8368644 -
Flags: review?(rail) → review+
Assignee | ||
Updated•11 years ago
|
Attachment #8367235 -
Flags: checked-in+
Assignee | ||
Updated•11 years ago
|
Attachment #8367236 -
Flags: checked-in+
Assignee | ||
Updated•11 years ago
|
Attachment #8367288 -
Flags: checked-in+
Assignee | ||
Updated•11 years ago
|
Attachment #8368644 -
Flags: checked-in+
Comment 29•11 years ago
|
||
something is in production
Comment 30•11 years ago
|
||
Comment on attachment 8368644 [details] [diff] [review]
920485_cache_mozharness.diff
Backed out for bustage:
http://ftp.mozilla.org/pub/mozilla.org/b2g/tinderbox-builds/mozilla-b2g28_v1_3-emulator/1396888023/b2g_mozilla-b2g28_v1_3_emulator_dep-bm74-build1-build19.txt.gz
Attachment #8368644 -
Flags: checked-in+ → checked-in-
Assignee | ||
Comment 31•11 years ago
|
||
Patch 8368644 has been backed out since it references a non existing key self.config["tooltool_config"] in b2g_build.py, busting the trees.
This patch is conceptually identical to the previous one, apart from using self.config.get('tooltool_cache', ...) instead of self.config["tooltool_config"].get('tooltool_cache', ...) in file b2g_config.py
Attachment #8402781 -
Flags: review?(rail)
Assignee | ||
Comment 32•11 years ago
|
||
Removed all cache-related references to self.config['tooltool_config']; cache related values will be read solely from self.config directly.
Attachment #8402781 -
Attachment is obsolete: true
Attachment #8402781 -
Flags: review?(rail)
Assignee | ||
Updated•11 years ago
|
Attachment #8402809 -
Flags: review?(rail)
Comment 33•11 years ago
|
||
Comment on attachment 8402809 [details] [diff] [review]
920485_cache_mozharness_2.diff
Review of attachment 8402809 [details] [diff] [review]:
-----------------------------------------------------------------
b2g_build.py and mobile_l10n.py use different config hierarchy for tooltool_cache (config->tooltool_cache vs config->tooltool_config->tooltool_cache). Can you unify them?
Attachment #8402809 -
Flags: review?(rail) → review-
Assignee | ||
Updated•11 years ago
|
Attachment #8402832 -
Flags: review?(rail)
Updated•11 years ago
|
Attachment #8402832 -
Flags: review?(rail) → review+
Assignee | ||
Comment 35•11 years ago
|
||
I had a look to the output of the builds on tbpl and I realized that another bit is missing to actually enable the cache, which is the attached patch to tooltool_wrapper.sh
Attachment #8403281 -
Flags: review?(rail)
Updated•11 years ago
|
Attachment #8403281 -
Flags: review?(rail) → review+
Assignee | ||
Updated•11 years ago
|
Attachment #8402832 -
Flags: checked-in+
Assignee | ||
Updated•11 years ago
|
Attachment #8403281 -
Flags: checked-in+
Assignee | ||
Comment 36•11 years ago
|
||
This extra bit is needed for tooltool_wrapper.sh to be able to read the TOOLTOOL_CACHE folder and actually enable the cache.
Attachment #8415508 -
Flags: review?(rail)
Updated•11 years ago
|
Attachment #8415508 -
Flags: review?(rail) → review+
Assignee | ||
Comment 37•11 years ago
|
||
Checked in: https://hg.mozilla.org/build/buildbotcustom/rev/7cca3204421d
Waiting for reconfig to go live!
Assignee | ||
Updated•11 years ago
|
Attachment #8415508 -
Flags: checked-in+
Assignee | ||
Comment 38•11 years ago
|
||
Further changes to tooltool_wrapper.sh to correctly read and use TOOLTOOL_CACHE env variable
Attachment #8416206 -
Flags: review?(mshal)
Assignee | ||
Comment 39•11 years ago
|
||
Simpler solution for you to review mshal!
Attachment #8416206 -
Attachment is obsolete: true
Attachment #8416206 -
Flags: review?(mshal)
Attachment #8416217 -
Flags: review?(mshal)
Comment 40•11 years ago
|
||
Comment on attachment 8416217 [details] [diff] [review]
cache_tools_03.diff
Looks good!
Attachment #8416217 -
Flags: review?(mshal) → review+
Assignee | ||
Comment 41•11 years ago
|
||
Comment on attachment 8416217 [details] [diff] [review]
cache_tools_03.diff
https://hg.mozilla.org/build/tools/rev/ccf936487928
Attachment #8416217 -
Flags: checked-in+
Assignee | ||
Comment 42•11 years ago
|
||
All the pieces are deployed now, I think this can be finally closed!
Status: NEW → RESOLVED
Closed: 11 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
•