bhearsum@mozilla.com (:bhearsum) Reporter
	Description • 15 years ago

Our 'push to mirrors' command has changed a lot recently, mainly the --exclude's that we use. We should move this logic into a script that knows how to push to mirrors, and includes/excludes the proper things.

We should also make this triggerable through the Waterfall, but protect it in some way so that a mis-click of 'Force Build' doesn't necessarily trigger a push. Catlee suggested requiring a specific property key/value to do this.

	Chris Cooper [:coop] (he/him)
	Comment 1 • 15 years ago

Mass move of bugs from Release Engineering:Future -> Release Engineering. See
http://coop.deadsquid.com/2010/02/kiss-the-future-goodbye/ for more details.

	Chris Cooper [:coop] (he/him)
	Comment 2 • 15 years ago

(In reply to comment #0)
> Catlee suggested requiring a specific property key/value to do this.

I like the sound of this. Perhaps if the key/value isn't provided, the command still gets executed in dry-run mode to show what would be done?

	Rail Aliiev [:rail] Assignee
	Comment 3 • 14 years ago

* Reuses some functions from bug 508896
* Needs "release_config" and "revision" properties to be set by sendchange or web interface
* CONFIG_FILE passes which global config file to use (production_config.py or stagign_config.py). We need to know some variables (stage username, key, server) which are not in the release config.
* To satisfy execfile('config.py') we need a local copy of buildbotcustom and CONFIG_FILE as 'localconfig.py'

There are also some parts, where I'm not sure:

* Should we separate and run the virus check beforehand?
* Another elegant way to check file/dir permissions without using pipes and subshells
* XulRunner push may require a command line switch, which forces productName
* XulRunner's rsyncd config file manipulations are not obvious

Comments and ideas are welcome.

	bhearsum@mozilla.com (:bhearsum) Reporter
	Comment 4 • 14 years ago

Comment on attachment 489138 [details] [diff] [review]
[WIP] push to mirrors


>+def getCandidatesDir(product, version, buildNumber, nightlyDir="nightly"):
>+    # can be used with rsync, eg host + ':' + getCandidatesDir()
>+    # and "http://' + host + getCandidatesDir()
>+    return '/pub/mozilla.org/' + product + '/' + nightlyDir + '/' + \
>+           str(version) + '-candidates/build' + str(buildNumber) + '/'

Hmm, I just put together a more robust version of this in bug, 608004, will it work for you instead? http://hg.mozilla.org/users/bhearsum_mozilla.com/tools/file/07fb9446e51d/lib/python/release/paths.py has it.


>+    # TODO: set env in buildbotcustom

I'm not sure what this comment means, or why you need buildbotcustom here -- can you explain?


>+def checkStagePermissions(ssh_command, productName, version, builNumber):
...
>+    errors = False
>+    for test_template in tests:
>+        test = test_template % candidates_dir
>+        command = ' '.join(stage_ssh_command)
>+        command += ' test "0" = "$(%s | wc -l)"' % test
>+        try:
>+            run_cmd(['/bin/sh', '-c', command])
>+        except CalledProcessError:
>+            errors = True

Hmmm, does this not work without the /bin/sh part?

>+
>+    if errors:
>+        raise CalledProcessError
>+
>+def runAntivirusCheck(ssh_command, productName, version, builNumber):
>+    candidates_dir = getCandidatesDir(productName, version, builNumber)
>+    run_cmd(stage_ssh_command + ['clamdscan', '-m', '-v', candidates_dir])

Do you know if clamdscan returns non-zero if it finds a problem?

>+
>+def pushToMirrors(ssh_command, productName, version, builNumber):
>+    source_dir = getCandidatesDir(productName, version, builNumber)
>+    target_dir = getReleasesDir(productName, version)
>+    # fail if target directory exists
>+    run_cmd(stage_ssh_command + ['test', '!', '-d', target_dir])
>+    run_cmd(stage_ssh_command + ['mkdir', '-p', target_dir])
>+    run_cmd(stage_ssh_command + ['rsync', '-av',
>+                                 '--exclude=*tests*',
>+                                 '--exclude=*crashreporter*',
>+                                 '--exclude=*.log',
>+                                 '--exclude=*.txt',
>+                                 '--exclude=*unsigned*',
>+                                 '--exclude=*update-backup*',
>+                                 source_dir, target_dir])

We're going to end up with differences per branch at some point, can you make sure it's possible to override or append to this in some way? I also think it would be good to define the list of excludes at the top rather than buried here.

>+
>+    # as cltbld @stage, manual for now
>+    #firefox = 'sed -i "s/$oldversion/$newversion/" /pub/mozilla.org/zz/rsyncd-mozilla-current.exclude'
>+    #xulrunner = 'echo "- xulrunner/releases/$oldXRversion" >> /pub/mozilla.org/zz/rsyncd-mozilla-releases.exclude'

When this is automated it should be more generic, I think. Aren't we going to have two separate invocations of this script -- one for Firefox and one for XULRunner?


>+    stage_ssh_command = ['ssh',
>+                         '-i', os.path.expanduser('~/.ssh/%s' %
>+                                                  cfg_vars['stage_ssh_key']),
>+                         '-l', cfg_vars['stage_username'],
>+                         cfg_vars['stage_server']]

I think this can be factored a bit. It would be nice to have a run_remote_cmd or something in util.commands. What do you think?


>+    checkStagePermissions(stage_ssh_command, productName, version, buildNumber)
>+    runAntivirusCheck(stage_ssh_command, productName, version, buildNumber)
>+    pushToMirrors(stage_ssh_command, productName, version, buildNumber)

What do we do in cases where we want to run the final checks & virus scan but not push to mirrors? That's going to happen at least every time we chemspill, possibly more. I wonder if it's better to separate those two groups into separate scripts.

	Rail Aliiev [:rail] Assignee
	Comment 5 • 14 years ago

(In reply to comment #4)
> Hmm, I just put together a more robust version of this in bug, 608004, will it
> work for you instead?
> http://hg.mozilla.org/users/bhearsum_mozilla.com/tools/file/07fb9446e51d/lib/python/release/paths.py
> has it.

Ah. Great that we already have it.

> I'm not sure what this comment means, or why you need buildbotcustom here --
> can you explain?

config.py contains "import buildbotcustom.env" statement. To satisfy it I need to have a copy of buildbotcustom. On the other hand looks like this import is not used anymore. Let me check and test this.

> Hmmm, does this not work without the /bin/sh part?

Pipes and subshell calls are shell specific. If we pass the command as a list, "|" symbol won't be interpreted as a pipe, for example. This is the reason why I'm looking for a way which doesn't use pipes and subshells.

> Do you know if clamdscan returns non-zero if it finds a problem?

Yeah, from clamdscan man page:
RETURN CODES
       0 : No virus found.
       1 : Virus(es) found.
       2 : An error occured.


> We're going to end up with differences per branch at some point, can you make
> sure it's possible to override or append to this in some way? I also think it
> would be good to define the list of excludes at the top rather than buried
> here.

OK.
 
> When this is automated it should be more generic, I think. Aren't we going to
> have two separate invocations of this script -- one for Firefox and one for
> XULRunner?

Yes. Only this function should behave differently depending on the product so far. 

> I think this can be factored a bit. It would be nice to have a run_remote_cmd
> or something in util.commands. What do you think?

+1

> What do we do in cases where we want to run the final checks & virus scan but
> not push to mirrors? That's going to happen at least every time we chemspill,
> possibly more. I wonder if it's better to separate those two groups into
> separate scripts.

It will be better to have 2 different scripts, one for file permission check and virus scan, and the second one for real mirror push.

	Rail Aliiev [:rail] Assignee
	Comment 6 • 14 years ago

Looks like buildbotcustom.env imports from config.py files are unused. ./test-masters.sh blessed me as well. :)

	Chris AtLee [:catlee]
	Comment 7 • 14 years ago

Comment on attachment 489138 [details] [diff] [review]
[WIP] push to mirrors

>diff --git a/scripts/release/push-to-mirrors.py b/scripts/release/push-to-mirrors.py
>new file mode 100755
>--- /dev/null
>+++ b/scripts/release/push-to-mirrors.py
>@@ -0,0 +1,114 @@
>+#!/usr/bin/env python
>+
>+import os, json, sys

You'll need to try and import simplejson first here.

>+def setupAndReadConfigs():
>+    prop_file = os.environ.get('PROPERTIES_FILE', 'buildprops.json')
>+    cfg_file = os.environ.get('CONFIG_FILE', 'production_config.py')
>+    # TODO: set env in buildbotcustom
>+    configs = os.environ.get('BUILDBOT_CONFIGS',
>+                             'http://hg.mozilla.org/build/buildbot-configs')
>+    buildbotcustom = os.environ.get('BUILDBOTCUSTOM',
>+                                    'http://hg.mozilla.org/build/buildbotcustom')
>+
>+    props = json.load(open(prop_file))
>+
>+    release_config = props.get('properties',{}).get('release_config')
>+    release_tag = props.get('properties',{}).get('revision')
>+    assert release_config and release_tag, \
>+            "release_config and release_tag should be provided."
>+
>+    config_py_dir = os.path.dirname(release_config)
>+
>+    clone(configs, 'buildbot-configs')
>+    update('buildbot-configs', revision=release_tag)
>+    clone(buildbotcustom, 'buildbotcustom')
>+    update('buildbotcustom', revision=release_tag)

buildbotcustom isn't needed any more, right?

>+    errors = False
>+    for test_template in tests:
>+        test = test_template % candidates_dir
>+        command = ' '.join(stage_ssh_command)
>+        command += ' test "0" = "$(%s | wc -l)"' % test
>+        try:
>+            run_cmd(['/bin/sh', '-c', command])

I think Ben asked this already, but why is calling /bin/sh directly necessary?  Don't we want to end up with something like
ssh stage.mozilla.org 'test "0" = "$(find %s ! -user ffxbld ! -path '*/contrib*' | wc -l)"'

Also, stage_ssh_command should be just ssh_command here, right?

>+        except CalledProcessError:
>+            errors = True
>+
>+    if errors:
>+        raise CalledProcessError
>+
>+def runAntivirusCheck(ssh_command, productName, version, builNumber):
>+    candidates_dir = getCandidatesDir(productName, version, builNumber)
>+    run_cmd(stage_ssh_command + ['clamdscan', '-m', '-v', candidates_dir])

I think you can only have one command argument to ssh, so I think this should be something like
ssh_command + ['clamdscan -m -v %s' % candidates_dir]

>+
>+def pushToMirrors(ssh_command, productName, version, builNumber):
>+    source_dir = getCandidatesDir(productName, version, builNumber)
>+    target_dir = getReleasesDir(productName, version)
>+    # fail if target directory exists
>+    run_cmd(stage_ssh_command + ['test', '!', '-d', target_dir])
>+    run_cmd(stage_ssh_command + ['mkdir', '-p', target_dir])
>+    run_cmd(stage_ssh_command + ['rsync', '-av',
>+                                 '--exclude=*tests*',
>+                                 '--exclude=*crashreporter*',
>+                                 '--exclude=*.log',
>+                                 '--exclude=*.txt',
>+                                 '--exclude=*unsigned*',
>+                                 '--exclude=*update-backup*',
>+                                 source_dir, target_dir])

Same comments here re: stage_ssh_command and arguments of ssh.

	Rail Aliiev [:rail] Assignee
	Comment 8 • 14 years ago

Comment on attachment 489793 [details] [diff] [review]
Remove unused imports

http://hg.mozilla.org/build/buildbot-configs/rev/fe074266df31

	Rail Aliiev [:rail] Assignee
	Comment 9 • 14 years ago

* added a wrapper to simplify the main script and unify usage
* since most of the bootstrap code is similar for permission check, virus check and rsync, I used the same file for them, but pre-push checks (perm and virus check for now) can be run separately by passing "check" or "push" arguments (see their usage in the next patch for buildbot-custom).
* run_remote_cmd added to lib/python/util/commands.py
* dropped buildbot-custom checkouts

	bhearsum@mozilla.com (:bhearsum) Reporter
	Comment 11 • 14 years ago

Comment on attachment 492323 [details] [diff] [review]
push to mirrors scripts

Why is compare_locales_repo_path a required config item?

Can you add 'partner-repacks' to the default excludes? Now that bug 560883 is fixed that could become an issue.

Can you make pushToMirrors capable of doing a dry run with rsync -n, and run that as part of pre_push_checks?

Would it be better to use urljoin() when protocol is present?

run_remote_cmd looks _awesome_, thanks for taking the time to factor it!

Can you explicitly chmod the topmost dir to rwxr-sr-x ?

Does this really need 6gb of space? Might wanna adjust how you're calling purge builds :)

This looks great overall, just a little bit more and it's there!

	bhearsum@mozilla.com (:bhearsum) Reporter
	Comment 12 • 14 years ago

Comment on attachment 492324 [details] [diff] [review]
buildbot-custom changes

Let's be a bit more safe about these timeouts, I suggest doubling them to 3 hours. The virus scan in particular could take that long in extradordinary circumstances. Also, please add a comment stating how long it is in hours for readability.

r=me with those changes.

	Rail Aliiev [:rail] Assignee
	Comment 13 • 14 years ago

(In reply to comment #11)
> Why is compare_locales_repo_path a required config item?

Silly copy/paste. :(
 
> Can you add 'partner-repacks' to the default excludes? Now that bug 560883 is
> fixed that could become an issue.

Done.

> Can you make pushToMirrors capable of doing a dry run with rsync -n, and run
> that as part of pre_push_checks?

Done. 
 
> Would it be better to use urljoin() when protocol is present?

Do you mean something like:
return urljoin('%s://%s' % (protocol, server), dir) ?
If yes, done.
 
> run_remote_cmd looks _awesome_, thanks for taking the time to factor it!

You are welcome! Thanks for the pushing me in the right direction.

> Can you explicitly chmod the topmost dir to rwxr-sr-x ?

Done.

> Does this really need 6gb of space? Might wanna adjust how you're calling purge
> builds :)

Heh, another silly copy/paste. :)

	Rail Aliiev [:rail] Assignee
	Comment 14 • 14 years ago

* Set timeout to 3h
* keeping r+

	bhearsum@mozilla.com (:bhearsum) Reporter
	Comment 15 • 14 years ago

Comment on attachment 492603 [details] [diff] [review]
push to mirrors scripts

Hmm, sorry, I actually meant urlunsplit :(. urljoin() doesn't really gain much over doing it manually....

Does 0.3 work with purge_builds.py?

r=me with the urlunsplit change.

	Rail Aliiev [:rail] Assignee
	Comment 16 • 14 years ago

(In reply to comment #15)
> Hmm, sorry, I actually meant urlunsplit :(. urljoin() doesn't really gain much
> over doing it manually....

OK.
return urlunsplit((protocol, server, dir, None, None))
 
> Does 0.3 work with purge_builds.py?

options.size is a float, and OptionParser parses the parameter very fine.

> r=me with the urlunsplit change.

Cool. I'm going to test it in staging these days. Mind to test it for 3.6.13 if everything goes fine? ;)

	bhearsum@mozilla.com (:bhearsum) Reporter
	Comment 17 • 14 years ago

(In reply to comment #16)
> Created attachment 492946 [details] [diff] [review]
> push to mirrors scripts
> 
> (In reply to comment #15)
> > Hmm, sorry, I actually meant urlunsplit :(. urljoin() doesn't really gain much
> > over doing it manually....
> 
> OK.
> return urlunsplit((protocol, server, dir, None, None))
> 
> > Does 0.3 work with purge_builds.py?
> 
> options.size is a float, and OptionParser parses the parameter very fine.
> 
> > r=me with the urlunsplit change.
> 
> Cool. I'm going to test it in staging these days. Mind to test it for 3.6.13 if
> everything goes fine? ;)

ssalbiz may be doing some signing testing against 3.6.13, but as long as you're not modifying files in the candidates dir, sounds good!

	Rail Aliiev [:rail] Assignee
	Comment 18 • 14 years ago

A nit. Added a line which prints which commands fails while testing permissions.

	Rail Aliiev [:rail] Assignee
	Comment 19 • 14 years ago

Added missing builddir builder property.

	Rail Aliiev [:rail] Assignee
	Comment 20 • 14 years ago

Staging run passed with the following local modifications:

1) a patch from bug 588150 was reverted
2) antivirus scan failed due to antivirus absence in staging
3) permission check failed due to different file permissions in staging (664 instead of 644, for example)

push_to_mirror builder worked fine.

Setting 588150 as a blocker. As soon the problem with external imports is solved there, we can go live with these patches.

	Rail Aliiev [:rail] Assignee
	Comment 21 • 14 years ago

Refreshed against bug 613226 (s/'category': 'release'/'category': builderPrefix('')/g)

	Rail Aliiev [:rail] Assignee
	Comment 22 • 14 years ago

Forgot to add the builders to notify_builders.

	Rail Aliiev [:rail] Assignee
	Comment 23 • 14 years ago

Comment on attachment 493190 [details] [diff] [review]
push to mirrors scripts

http://hg.mozilla.org/build/tools/rev/44437bfc4a1e

	Rail Aliiev [:rail] Assignee
	Comment 24 • 14 years ago

Comment on attachment 494346 [details] [diff] [review]
buildbotcustom changes

http://hg.mozilla.org/build/buildbotcustom/rev/9e61ed1d232a

	Rail Aliiev [:rail] Assignee
	Comment 25 • 14 years ago

Tested and worked fine in Firefox 4.0b8 release.