920485 - New tooltool deployment

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Simone Bruno [:simone]]

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.

Comment 1

[Simone Bruno [:simone]]

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.

Comment 2

[Simone Bruno [:simone]]

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.

Comment 3

[Simone Bruno [:simone]]

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.

Comment 4

[Simone Bruno [:simone]]

Attachment: Bug_920485_1_merge.diff

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

Comment 5

[Simone Bruno [:simone]]

Attachment: Bug_920485_2_extra-changes.diff

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

Comment 6

[Simone Bruno [:simone]]

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

[Rail Aliiev [:rail]]

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

Comment 8

[Simone Bruno [:simone]]

Attachment: While running some testing for Bug 930029 I fixed some minor issues in this patch.

Comment 9

[Simone Bruno [:simone]]

The previous patch has been created with the following command:
git diff -U8 8181a768f2c546abdbf6488cc6676070a5b8bc0a 5d9f3aab05af0714bc672f959b2b051d46e63569 > minor1.diff

Comment 10

[Simone Bruno [:simone]]

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.

Comment 11

[Simone Bruno [:simone]]

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.

Comment 12

[Simone Bruno [:simone]]

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

Comment 13

[Simone Bruno [:simone]]

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.

Comment 14

[Simone Bruno [:simone]]

Attachment: Enabling the cache - changes to fetch_and_unpack.sh script

Comment 15

[Simone Bruno [:simone]]

Attachment: Enabling the cache - changes to mozharness

Comment 16

[Simone Bruno [:simone]]

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.

Comment 17

[Simone Bruno [:simone]]

Attachment: 920485_cache_buildbot-config.diff

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.

Comment 18

[Simone Bruno [:simone]]

Attachment: 920485_cache_mozharness.diff

This patch enables the tooltool cache option in all mozharness calls, when the TOOLTOOL_CACHE environment variable is set.

Comment 19

[Simone Bruno [:simone]]

Attachment: 920485_cache_puppet.diff

Creation via puppet of the folder to be used as tooltool local cache.

Comment 20

[Simone Bruno [:simone]]

Attachment: 920485_cache_tools.diff

Enables tooltool cache in non-mozharness call + purge logic in purge_builds.py

Comment 21

[Simone Bruno [:simone]]

Attachment: 920485_cache_buildbot-config.diff

As previous version of 920485_cache_buildbot-config.diff, plus setting of TOOLTOOL_HOME to enable purge.

Comment 22

[Rail Aliiev [:rail]]

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.

Comment 23

[Simone Bruno [:simone]]

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

[Aki Sasaki (not active)]

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"))

Comment 25

[Simone Bruno [:simone]]

Attachment: 920485_cache_mozharness.diff

I changed the patch following the pattern recommended by Aki (thanks Aki!).

Comment 26

[Rail Aliiev [: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...

Comment 27

[Simone Bruno [:simone]]

Attachment: 920485_cache_mozharness.diff

New Attempt!

Comment 28

[Rail Aliiev [: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.

Comment 29

[[e-mail] (:bhearsum)]

something is in production

Comment 30

[[e-mail] (:bhearsum)]

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

Comment 31

[Simone Bruno [:simone]]

Attachment: 920485_cache_mozharness_2.diff

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

Comment 32

[Simone Bruno [:simone]]

Attachment: 920485_cache_mozharness_2.diff

Removed all cache-related references to self.config['tooltool_config']; cache related values will be read solely from self.config directly.

Comment 33

[Rail Aliiev [: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?

Comment 34

[Simone Bruno [:simone]]

Attachment: 920485_cache_mozharness_2.diff

Trying again!

Comment 35

[Simone Bruno [:simone]]

Attachment: 920485_cache_tools_2.diff

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

Comment 36

[Simone Bruno [:simone]]

Attachment: cache_buildbotcustom

This extra bit is needed for tooltool_wrapper.sh to be able to read the TOOLTOOL_CACHE folder and actually enable the cache.

Comment 37

[Simone Bruno [:simone]]

Checked in: https://hg.mozilla.org/build/buildbotcustom/rev/7cca3204421d
Waiting for reconfig to go live!

Comment 38

[Simone Bruno [:simone]]

Attachment: cache_tools_02.diff

Further changes to tooltool_wrapper.sh to correctly read and use TOOLTOOL_CACHE env variable

Comment 39

[Simone Bruno [:simone]]

Attachment: cache_tools_03.diff

Simpler solution for you to review mshal!

Comment 40

[Michael Shal [:mshal]]

Comment on attachment 8416217 [details] [diff] [review]
cache_tools_03.diff

Looks good!

Comment 41

[Simone Bruno [:simone]]

Comment on attachment 8416217 [details] [diff] [review]
cache_tools_03.diff

https://hg.mozilla.org/build/tools/rev/ccf936487928

Comment 42

[Simone Bruno [:simone]]

All the pieces are deployed now, I think this can be finally closed!