Closed Bug 920485 Opened 9 years ago Closed 8 years ago

New tooltool deployment

Categories

(Release Engineering :: General, defect)

x86
All
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: sbruno, Assigned: sbruno)

References

Details

Attachments

(11 files, 8 obsolete files)

37.20 KB, patch
sbruno
: checked-in+
Details | Diff | Splinter Review
5.00 KB, patch
rail
: review+
sbruno
: checked-in+
Details | Diff | Splinter Review
2.17 KB, patch
rail
: review+
sbruno
: checked-in+
Details | Diff | Splinter Review
2.14 KB, patch
rail
: review+
sbruno
: checked-in+
Details | Diff | Splinter Review
2.20 KB, patch
rail
: review+
sbruno
: checked-in+
Details | Diff | Splinter Review
13.51 KB, patch
rail
: review+
sbruno
: checked-in+
Details | Diff | Splinter Review
4.78 KB, patch
rail
: review+
bhearsum
: checked-in-
Details | Diff | Splinter Review
3.02 KB, patch
rail
: review+
sbruno
: checked-in+
Details | Diff | Splinter Review
824 bytes, patch
rail
: review+
sbruno
: checked-in+
Details | Diff | Splinter Review
785 bytes, patch
rail
: review+
sbruno
: checked-in+
Details | Diff | Splinter Review
1.15 KB, 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.
Depends on: 768123
No longer depends on: 778123
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.
Blocks: 923050
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.
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.
Depends on: 882712
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
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)
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 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+
The previous patch has been created with the following command:
git diff -U8 8181a768f2c546abdbf6488cc6676070a5b8bc0a 5d9f3aab05af0714bc672f959b2b051d46e63569 > minor1.diff
Attachment #824170 - Flags: review?(rail) → review+
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
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.
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
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.
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.
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
Attached patch 920485_cache_mozharness.diff (obsolete) — Splinter Review
This patch enables the tooltool cache option in all mozharness calls, when the TOOLTOOL_CACHE environment variable is set.
Creation via puppet of the folder to be used as tooltool local cache.
Enables tooltool cache in non-mozharness call + purge logic in purge_builds.py
Attachment #8367232 - Flags: review?(rail)
Attachment #8367234 - Flags: review?(rail)
Attachment #8367235 - Flags: review?(rail)
Attachment #8367236 - Flags: review?(rail)
Attachment #820695 - Flags: checked-in+
Attachment #820699 - Flags: checked-in+
Attachment #824170 - Flags: checked-in+
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)
Attachment #8367235 - Flags: review?(rail) → review+
Attachment #8367236 - Flags: review?(rail) → review+
Attachment #8367288 - Flags: review?(rail) → review+
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)
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 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+
Attached patch 920485_cache_mozharness.diff (obsolete) — Splinter Review
I changed the patch following the pattern recommended by Aki (thanks Aki!).
Attachment #8367234 - Attachment is obsolete: true
Attachment #8368593 - Flags: review?(rail)
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-
New Attempt!
Attachment #8368593 - Attachment is obsolete: true
Attachment #8368644 - Flags: review?(rail)
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+
Attachment #8367235 - Flags: checked-in+
Attachment #8367236 - Flags: checked-in+
Attachment #8367288 - Flags: checked-in+
Attachment #8368644 - Flags: checked-in+
something is in production
Attached patch 920485_cache_mozharness_2.diff (obsolete) — Splinter Review
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)
Attached patch 920485_cache_mozharness_2.diff (obsolete) — Splinter Review
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)
Attachment #8402809 - Flags: review?(rail)
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-
Trying again!
Attachment #8402809 - Attachment is obsolete: true
Attachment #8402832 - Flags: review?(rail)
Attachment #8402832 - Flags: review?(rail) → review+
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)
Attachment #8403281 - Flags: review?(rail) → review+
Attachment #8402832 - Flags: checked-in+
Attachment #8403281 - Flags: checked-in+
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)
Attachment #8415508 - Flags: review?(rail) → review+
Checked in: https://hg.mozilla.org/build/buildbotcustom/rev/7cca3204421d
Waiting for reconfig to go live!
Attachment #8415508 - Flags: checked-in+
Attached patch cache_tools_02.diff (obsolete) — Splinter Review
Further changes to tooltool_wrapper.sh to correctly read and use TOOLTOOL_CACHE env variable
Attachment #8416206 - Flags: review?(mshal)
Simpler solution for you to review mshal!
Attachment #8416206 - Attachment is obsolete: true
Attachment #8416206 - Flags: review?(mshal)
Attachment #8416217 - Flags: review?(mshal)
Comment on attachment 8416217 [details] [diff] [review]
cache_tools_03.diff

Looks good!
Attachment #8416217 - Flags: review?(mshal) → review+
No longer depends on: 882712
All the pieces are deployed now, I think this can be finally closed!
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Component: Tools → General
You need to log in before you can comment on or make changes to this bug.