Merge lp:~pfalcon/linaro-android-mirror/per-host-config into lp:linaro-android-mirror

[Paul Sokolovsky]

This patch allows to specify individual configuration for each upstream git host we mirror. Currently implemented setting for stale check (don't sync with host if was synced recently enough) and number of concurrent sync jobs (-jN).

This should alleviate issues which were seen/reported with mirror service during last week:
 * git.omapzoom.org needs -j1 to sync reliably, but forcing all hosts to -j1 does affect mirroring performance (Patrik Ryd)
 * yesterday, android.git.kernel.org was barely accessible, failing to sync with connection timed out's, and consequently failing all the builds during a day.

[Guilherme Salgado (salgado)]

Hi Paul,

This looks good to me; I've just a couple suggestions below.

 review approve

> === modified file 'linaro_android_mirror/mirrorer.py'
> --- linaro_android_mirror/mirrorer.py 2011-06-02 13:34:21 +0000
> +++ linaro_android_mirror/mirrorer.py 2011-06-03 07:52:34 +0000
> @@ -1,6 +1,7 @@
> from collections import defaultdict
> import errno
> import os
> +import time
>
> from twisted.internet.defer import (
> Deferred,
> @@ -19,9 +20,10 @@
>
> _directory_locks = defaultdict(DeferredLock)
>
> -def check_cmd_status(data, target_directory):
> +def check_cmd_status(data, host_config):
> # print "check_cmd_status:", data
> stdout, stderr, exitcode = data
> + target_directory = os.path.join(settings.MIRROR_PREFIX, host_config["host"])
> if exitcode != 0:
> log.msg("error executing command: stdout: '%s', stderr: '%s', exitcode: %d" % (stdout, stderr, exitcode))
> raise Exception(stderr)
> @@ -46,6 +48,26 @@
> symlink('manifests')
> symlink('manifests.git')
> symlink('repo')
> +
> + if hostname in settings.UPSTREAM_HOSTS:
> + host_config = settings.UPSTREAM_HOSTS[hostname]
> + else:
> + host_config = settings.UPSTREAM_HOSTS["*"]
> + host_config["host"] = hostname

I'm slightly concerned about the fact that this is a long-running
process changing a mutable setting. I see that the host will be
overwritten every time this function gets called, so it shouldn't be a
problem, but we might have problems if the recipient of host_config
decides to change it further. We can easily avoid that by making a copy
of the host config (e.g.
copy.copy(settings.UPSTREAM_HOSTS[hostname|"*"])) and passing that on to
the callback.

> +
> + # Get last successful sync timestamp
> + stamp = os.path.join(target_directory, '.synced')
> + try:
> + stamp_mtime = os.stat(stamp).st_mtime
> + except OSError:
> + stamp_mtime = 0
> +
> + # Check for fresh enough
> + if time.time() - stamp_mtime < host_config.get("stale", 0):
> + log.msg("Mirror %s was synced %ds ago, stale threshold (%ds) not reached" %
> + (target_directory, time.time() - stamp_mtime, host_config.get("stale", 0)))
> + return
> +
> manifest_path = os.path.join(target_directory, '.repo/manifest.xml')
> try:
> os.unlink(manifest_path)
> @@ -54,7 +76,7 @@
> raise
> with open(manifest_path, 'w') as manifest_file:
> manifest_file.write(manifest)
> - args = ['sync'] + settings.REPO_SYNC_EXTRA_ARGS
> + args = ['sync'] + settings.REPO_SYNC_EXTRA_ARGS + ['-j%d' % host_config.get("jobs", 1)]

Is it worth allowing host configs to not specify the number of jobs to
use? I'm thinking that it may be better to make it required instead of
providing a default that is far from ideal.

> print "running", settings.REPO, args, 'in', target_directory
> if settings.DRY_RUN:
> d = Deferred()
> @@ -62,7 +84,7 @@
> else:
> d = getProcessOutputAndValue(
> ...

review: Approve

[Paul Sokolovsky (pfalcon)]

Hello Guilherme,

On Fri, 03 Jun 2011 14:24:31 -0000
Guilherme Salgado <email address hidden> wrote:

> Review: Approve
> Hi Paul,
>
> This looks good to me; I've just a couple suggestions below.
>
[]
> > +
> > + if hostname in settings.UPSTREAM_HOSTS:
> > + host_config = settings.UPSTREAM_HOSTS[hostname]
> > + else:
> > + host_config = settings.UPSTREAM_HOSTS["*"]
> > + host_config["host"] = hostname
>
> I'm slightly concerned about the fact that this is a long-running
> process changing a mutable setting. I see that the host will be
> overwritten every time this function gets called, so it shouldn't be a
> problem, but we might have problems if the recipient of host_config
> decides to change it further. We can easily avoid that by making a
> copy of the host config (e.g.
> copy.copy(settings.UPSTREAM_HOSTS[hostname|"*"])) and passing that on
> to the callback.

Good catch. It's probably harmless as it is now, but I have more ideas
what dynamic data to pass to callback, and having that pollute global
config is bad idea.

[]
> > @@ -54,7 +76,7 @@
> > raise
> > with open(manifest_path, 'w') as manifest_file:
> > manifest_file.write(manifest)
> > - args = ['sync'] + settings.REPO_SYNC_EXTRA_ARGS
> > + args = ['sync'] + settings.REPO_SYNC_EXTRA_ARGS + ['-j%d'
> > % host_config.get("jobs", 1)]
>
> Is it worth allowing host configs to not specify the number of jobs to
> use? I'm thinking that it may be better to make it required instead
> of providing a default that is far from ideal.

Yes, I'm a big proponent of sensible defaults ;-). And -j1 is pretty
good as that's what you get when you just type "repo sync". It's also
environment-friendly, because issues with android.git is apparently due
to (relative) lot of people abusing big -j values (I saw -j50 in blogs).

Thanks,
Paul

[lp:~pfalcon/linaro-android-mirror/per-host-config]

47. By Paul Sokolovsky on 2011-06-03

Make sure we don't change UPSTREAM_HOSTS when passing extra data to callback.

[Guilherme Salgado (salgado)]

> > > @@ -54,7 +76,7 @@
> > > raise
> > > with open(manifest_path, 'w') as manifest_file:
> > > manifest_file.write(manifest)
> > > - args = ['sync'] + settings.REPO_SYNC_EXTRA_ARGS
> > > + args = ['sync'] + settings.REPO_SYNC_EXTRA_ARGS + ['-j%d'
> > > % host_config.get("jobs", 1)]
> >
> > Is it worth allowing host configs to not specify the number of jobs to
> > use? I'm thinking that it may be better to make it required instead
> > of providing a default that is far from ideal.
>
> Yes, I'm a big proponent of sensible defaults ;-). And -j1 is pretty
> good as that's what you get when you just type "repo sync". It's also
> environment-friendly, because issues with android.git is apparently due
> to (relative) lot of people abusing big -j values (I saw -j50 in blogs).

Oh, if that's the default for repo sync then it should be fine. Another
alternative would be to not pass the '-j' argument when it's not
specified in the host config; I think that'd be even better (would make
sure we always use repo's default), but it's not a big deal.

=== modified file 'linaro_android_mirror/mirrorer.py'
--- linaro_android_mirror/mirrorer.py	2011-06-02 13:34:21 +0000
+++ linaro_android_mirror/mirrorer.py	2011-06-03 19:35:23 +0000
@@ -1,6 +1,8 @@
 from collections import defaultdict
 import errno
 import os
+import time
+import copy
 
 from twisted.internet.defer import (
     Deferred,
@@ -19,9 +21,10 @@
 
 _directory_locks = defaultdict(DeferredLock)
 
-def check_cmd_status(data, target_directory):
+def check_cmd_status(data, host_data):
 #    print "check_cmd_status:", data
     stdout, stderr, exitcode = data
+    target_directory = os.path.join(settings.MIRROR_PREFIX, host_data["host"])
     if exitcode != 0:
         log.msg("error executing command: stdout: '%s', stderr: '%s', exitcode: %d" % (stdout, stderr, exitcode))
         raise Exception(stderr)
@@ -46,6 +49,29 @@
             symlink('manifests')
             symlink('manifests.git')
             symlink('repo')
+
+        if hostname in settings.UPSTREAM_HOSTS:
+            host_data = settings.UPSTREAM_HOSTS[hostname]
+        else:
+            host_data = settings.UPSTREAM_HOSTS["*"]
+        # We'll use host_data to pass extra data to callback,
+        # so copy it to make sure we don't pollute settings.UPSTREAM_HOSTS
+        host_data = copy.copy(host_data)
+        host_data["host"] = hostname
+
+        # Get last successful sync timestamp
+        stamp = os.path.join(target_directory, '.synced')
+        try:
+            stamp_mtime = os.stat(stamp).st_mtime
+        except OSError:
+            stamp_mtime = 0
+
+        # Check for fresh enough
+        if time.time() - stamp_mtime < host_data.get("stale", 0):
+            log.msg("Mirror %s was synced %ds ago, stale threshold (%ds) not reached" %
+                    (target_directory, time.time() - stamp_mtime, host_data.get("stale", 0)))
+            return
+
         manifest_path = os.path.join(target_directory, '.repo/manifest.xml')
         try:
             os.unlink(manifest_path)
@@ -54,7 +80,7 @@
                 raise
         with open(manifest_path, 'w') as manifest_file:
             manifest_file.write(manifest)
-        args = ['sync'] + settings.REPO_SYNC_EXTRA_ARGS
+        args = ['sync'] + settings.REPO_SYNC_EXTRA_ARGS + ['-j%d' % host_data.get("jobs", 1)]
         print "running", settings.REPO, args, 'in', target_directory
         if settings.DRY_RUN:
             d = Deferred()
@@ -62,7 +88,7 @@
         else:
             d = getProcessOutputAndValue(
                 settings.REPO, args, path=target_directory)
-        return d.addCallback(lambda data: check_cmd_status(data, target_directory))
+        return d.addCallback(lambda data: check_cmd_status(data, host_data))
     return d.run(_mirror)
 
 

=== modified file 'linaro_android_mirror/settings.py'
--- linaro_android_mirror/settings.py	2011-05-13 09:30:26 +0000
+++ linaro_android_mirror/settings.py	2011-06-03 19:35:23 +0000
@@ -2,4 +2,13 @@
 MIRROR_PREFIX = '/mnt/mirror'
 DRY_RUN = False
 REPO = '/usr/local/bin/repo'
-REPO_SYNC_EXTRA_ARGS = ['--no-repo-verify', '-j8']
+REPO_SYNC_EXTRA_ARGS = ['--no-repo-verify']
+
+# stale - don't sync if last successfully synced less than this time in seconds ago
+# jobs - use -jN repo sync param
+UPSTREAM_HOSTS = {
+    "android.git.kernel.org": {"stale": 60*60, "jobs": 6},
+    "git.omapzoom.org": {"stale": 30*60, "jobs": 1},
+    "git.linaro.org": {"stale": 1*60, "jobs": 8},
+    "*": {"stale": 10*60, "jobs": 4},
+}