Closed
Bug 540598
Opened 15 years ago
Closed 14 years ago
pushing a release to mirrors should be scripted
Categories
(Release Engineering :: General, defect, P3)
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+
rail
:
checked-in+
|
Details | Diff | Splinter Review |
11.30 KB,
patch
|
rail
:
review+
rail
:
checked-in+
|
Details | Diff | Splinter Review |
2.19 KB,
patch
|
bhearsum
:
review+
rail
:
checked-in+
|
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.
Reporter | ||
Updated•15 years ago
|
Component: Release Engineering → Release Engineering: Future
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.
Component: Release Engineering: Future → Release Engineering
Priority: -- → P3
Updated•15 years ago
|
Whiteboard: [automation]
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?
Assignee | ||
Updated•14 years ago
|
Assignee: nobody → 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.
Attachment #489138 -
Flags: feedback?(catlee)
Attachment #489138 -
Flags: feedback?(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.
Attachment #489138 -
Flags: feedback?(bhearsum) → feedback-
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.
Assignee | ||
Comment 6•14 years ago
|
||
Looks like buildbotcustom.env imports from config.py files are unused. ./test-masters.sh blessed me as well. :)
Attachment #489793 -
Flags: review?(catlee)
Updated•14 years ago
|
Attachment #489793 -
Flags: review?(catlee) → review+
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.
Attachment #489138 -
Flags: feedback?(catlee) → feedback+
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
Attachment #489793 -
Flags: checked-in+
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
Attachment #489138 -
Attachment is obsolete: true
Attachment #492323 -
Flags: review?(bhearsum)
Assignee | ||
Comment 10•14 years ago
|
||
Attachment #492324 -
Flags: review?(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!
Attachment #492323 -
Flags: review?(bhearsum)
Attachment #492323 -
Flags: review-
Attachment #492323 -
Flags: feedback+
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.
Attachment #492324 -
Flags: review?(bhearsum) → review+
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. :)
Attachment #492323 -
Attachment is obsolete: true
Attachment #492603 -
Flags: review?(bhearsum)
Assignee | ||
Comment 14•14 years ago
|
||
* Set timeout to 3h
* keeping r+
Attachment #492324 -
Attachment is obsolete: true
Attachment #492604 -
Flags: review+
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.
Attachment #492603 -
Flags: review?(bhearsum) → review+
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? ;)
Attachment #492603 -
Attachment is obsolete: true
Attachment #492946 -
Flags: review+
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!
Assignee | ||
Comment 18•14 years ago
|
||
A nit. Added a line which prints which commands fails while testing permissions.
Attachment #492946 -
Attachment is obsolete: true
Attachment #493190 -
Flags: review+
Assignee | ||
Comment 19•14 years ago
|
||
Added missing builddir builder property.
Attachment #492604 -
Attachment is obsolete: true
Attachment #493192 -
Flags: review+
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.
Depends on: 588150
Assignee | ||
Comment 21•14 years ago
|
||
Refreshed against bug 613226 (s/'category': 'release'/'category': builderPrefix('')/g)
Attachment #493192 -
Attachment is obsolete: true
Attachment #493400 -
Flags: review+
Assignee | ||
Comment 22•14 years ago
|
||
Forgot to add the builders to notify_builders.
Attachment #493400 -
Attachment is obsolete: true
Attachment #494346 -
Flags: review?(bhearsum)
Reporter | ||
Updated•14 years ago
|
Attachment #494346 -
Flags: review?(bhearsum) → review+
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
Attachment #493190 -
Flags: checked-in+
Assignee | ||
Comment 24•14 years ago
|
||
Comment on attachment 494346 [details] [diff] [review]
buildbotcustom changes
http://hg.mozilla.org/build/buildbotcustom/rev/9e61ed1d232a
Attachment #494346 -
Flags: checked-in+
Assignee | ||
Comment 25•14 years ago
|
||
Tested and worked fine in Firefox 4.0b8 release.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Product: mozilla.org → Release Engineering
You need to log in
before you can comment on or make changes to this bug.
Description
•