Closed Bug 540598 Opened 10 years ago Closed 9 years ago

pushing a release to mirrors should be scripted

Categories

(Release Engineering :: General, defect, P3)

All
macOS
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bhearsum, Assigned: rail)

References

Details

(Whiteboard: [automation])

Attachments

(3 files, 8 obsolete files)

2.93 KB, patch
catlee
: review+
Details | Diff | Splinter Review
11.30 KB, patch
rail
: review+
Details | Diff | Splinter Review
2.19 KB, patch
bhearsum
: review+
Details | Diff | Splinter Review
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.
Component: Release Engineering → Release Engineering: Future
Mass move of bugs from Release Engineering:Future -> Release Engineering. See
http://coop.deadsquid.com/2010/02/kiss-the-future-goodbye/ for more details.
Component: Release Engineering: Future → Release Engineering
Priority: -- → P3
Whiteboard: [automation]
(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?
Assignee: nobody → rail
Attached patch [WIP] push to mirrors (obsolete) — Splinter Review
* 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.
Attachment #489138 - Flags: feedback?(catlee)
Attachment #489138 - Flags: feedback?(bhearsum)
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.
Attachment #489138 - Flags: feedback?(bhearsum) → feedback-
(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.
Looks like buildbotcustom.env imports from config.py files are unused. ./test-masters.sh blessed me as well. :)
Attachment #489793 - Flags: review?(catlee)
Attachment #489793 - Flags: review?(catlee) → review+
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.
Attachment #489138 - Flags: feedback?(catlee) → feedback+
Attached patch push to mirrors scripts (obsolete) — Splinter Review
* 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
Attachment #489138 - Attachment is obsolete: true
Attachment #492323 - Flags: review?(bhearsum)
Attached patch buildbot-custom changes (obsolete) — Splinter Review
Attachment #492324 - Flags: review?(bhearsum)
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!
Attachment #492323 - Flags: review?(bhearsum)
Attachment #492323 - Flags: review-
Attachment #492323 - Flags: feedback+
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.
Attachment #492324 - Flags: review?(bhearsum) → review+
Attached patch push to mirrors scripts (obsolete) — Splinter Review
(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. :)
Attachment #492323 - Attachment is obsolete: true
Attachment #492603 - Flags: review?(bhearsum)
Attached patch buildbot-custom changes (obsolete) — Splinter Review
* Set timeout to 3h
* keeping r+
Attachment #492324 - Attachment is obsolete: true
Attachment #492604 - Flags: review+
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.
Attachment #492603 - Flags: review?(bhearsum) → review+
Attached patch push to mirrors scripts (obsolete) — Splinter Review
(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? ;)
Attachment #492603 - Attachment is obsolete: true
Attachment #492946 - Flags: review+
(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!
A nit. Added a line which prints which commands fails while testing permissions.
Attachment #492946 - Attachment is obsolete: true
Attachment #493190 - Flags: review+
Attached patch buildbotcustom changes (obsolete) — Splinter Review
Added missing builddir builder property.
Attachment #492604 - Attachment is obsolete: true
Attachment #493192 - Flags: review+
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.
Depends on: 588150
Attached patch buildbotcustom changes (obsolete) — Splinter Review
Refreshed against bug 613226 (s/'category': 'release'/'category': builderPrefix('')/g)
Attachment #493192 - Attachment is obsolete: true
Attachment #493400 - Flags: review+
Depends on: 615097
Forgot to add the builders to notify_builders.
Attachment #493400 - Attachment is obsolete: true
Attachment #494346 - Flags: review?(bhearsum)
Attachment #494346 - Flags: review?(bhearsum) → review+
Tested and worked fine in Firefox 4.0b8 release.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.