974082 - Don't reset mock environments if we don't have to

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Chris AtLee [:catlee]]

Most of our Linux based builds use mock to manage a chroot environment to do the build in. Currently we always reset and reinitialize the mock environment at the beginning of each build, even if we're going to install exactly the same packages over.

We should stop that! mock environments should only be reset when required.

Related: we should make the packages for different build types the same as much as possible, to avoid resetting when we don't need to.

Comment 1

[Chris AtLee [:catlee]]

Attachment: fastmock-.patch

The idea here is to put a file inside the mock environment that contains a hash of the list of packages installed. When we start a build, look at the file, and if it's the same as our list of packages, we can skip re-initializing the mock environment.

We still install packages in case there are new packages in the repos, but it's pretty quick (takes about 2s).

Comment 2

[[e-mail] (:bhearsum)]

Comment on attachment 8378267 [details] [diff] [review]
fastmock-.patch

Review of attachment 8378267 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozharness/mozilla/mock.py
@@ +35,5 @@
>  
> +    def delete_mock_files(self, mock_target, files):
> +        """Delete files from the mock environment `mock_target`. `files` should
> +        be an iterable of 2-tuples: (src, dst). Only the dst component is
> +        deleted."""

What's the point in files being a tuples? Seems like this should just receive a simple iterable.

@@ +137,5 @@
> +        mock_root = self.get_output_from_command(
> +            ['mock_mozilla', '-r', mock_target, '--print-root-path'])
> +        package_hash_file = os.path.join(mock_root, "builds/package_list.hash")
> +        if os.path.exists(package_hash_file):
> +            old_packages = self.read_from_file(package_hash_file)

s/old_packages/old_packages_hash/g?

@@ +146,3 @@
>  
> +        package_list_hash = hashlib.new('sha1')
> +        for p in sorted(mock_packages):

This code falls back on c['mock_packages'] if mock_packages is None. It seems like you should be doing that here too - otherwise this won't work for scripts like servo_build.py, where mock_packages are defined in the config.

@@ +149,5 @@
> +            package_list_hash.update(p)
> +        package_list_hash = package_list_hash.hexdigest()
> +
> +        did_init = True
> +        if old_packages != package_list_hash:

We talked a lot in person about what to do about packages with dependencies. It looks like you've decided not to take them into account, and only deal with explicitly listed packages. Are you worried at all about things breaking if a dependency changes?

Comment 3

[Chris AtLee [:catlee]]

Attachment: fastmock-.patch.0

I think this addresses your concerns. The function signature for delete_files is to make it easy to pass in the mock_files structure, which is already a list of tuples.

Comment 5

[Justin Wood (:Callek)]

Copy/Pasting an irc convo here, (note pastebins won't last very long, but theorycraft discussed is still useful)

[11:32:45]	Callek	catlee: for mock_ init stuff, depending on if deplist is a good thing vs time investment you might consider using `repoquery --tree-requires`
[11:33:02]	Callek	catlee: which will give you full deps and versions of said deps
[11:33:34]	=-=	coop|mtg is now known as coop
[11:33:40]	Callek	example output: https://callek.pastebin.mozilla.org/4338513
  ** ED NOTE: $  repoquery --tree-requires gcc
[11:35:37]	Callek	or you could do it the simple way if you want to concat a list for the hash: https://callek.pastebin.mozilla.org/4338522
  ** ED NOTE: $  repoquery -R --recursive gcc
[11:38:28]	catlee	Callek: huh, cool
[11:38:37]	catlee	so that would need to run inside mock I think?
[11:38:46]	catlee	or at least with the mock env's rpmdb
[11:38:52]	Callek	catlee: yea I think so
[11:39:16]	Callek	I haven't tested the theorycraft here, but it actually queries against the yum repo, not local installs
[11:39:59]	catlee	yeah
[11:40:07]	catlee	I wonder if that's worthwhile...
[11:40:07]	Callek	there is also | --installed limit query to installed pkgs only|
[11:40:22]	Callek	and lots of other options
[11:44:00]	Callek	catlee: for set-in-stone docs, I'm copy-pasting the convo above to bug, I'll leave up to you/others on the "is it worthwhile" front
[11:44:09]	catlee	sure

Comment 6

[Chris AtLee [:catlee]]

In production

Comment 7

[Chris AtLee [:catlee]]

And a bustage fix:
http://hg.mozilla.org/build/mozharness/rev/eccc8fbf5f95

Comment 8

[Chris AtLee [:catlee]]

Attachment: add libxml2 to emulator/emulator-ics

this is the only difference between the other 64-bit mock environments. adding libxml2 to these configs means we should avoid having to reset mock for any sequential b2g device builds.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8382280 [details] [diff] [review]
add libxml2 to emulator/emulator-ics

Review of attachment 8382280 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like only mako/config.json and emulator-jb/config.json already include libxml2. Should the other devices as well?

Comment 10

[Chris AtLee [:catlee]]

I think all the other devices are using the 32-bit environment. The package lists don't need to be the same between the environments for this optimization to work.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/edbd667e3a5b