601287 - pre-tagging sanity check step for build automation

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P5)

Description

[Lukas Blakk [:lsblakk] use ?needinfo]

Ran into a few issues on 3.5.14 release that might have been caught by either a buildbot step before tagging or perhaps a wrapper script around the sendchange to kick off automation:

1. Not having done a reconfig before kicking off automation - need a way to check running code to make sure it matches the configs you've pulled && updated
2. Extra space in relbranch so that it doesn't match what's available in repo's branch tags
3. Not having bumped the sourceRepoRevision between a build1 and build2 -- this is tricky because how do we know if the changeset used incorporates the version bumps or not?

Comment 1

[[e-mail] (:bhearsum)]

I don't think anyone will be getting to this soon.

Comment 2

[Lukas Blakk [:lsblakk] use ?needinfo]

could use the branch stated in the sendchange, to check running configs against.

Comment 3

[[e-mail] (:bhearsum)]

> 2. Extra space in relbranch so that it doesn't match what's available in repo's
> branch tags

Someone suggested today that we should actually verify that the relbranch and sourceRepoRevision exist in the repository. This probably negates the need for that specific check.

> 3. Not having bumped the sourceRepoRevision between a build1 and build2 -- this
> is tricky because how do we know if the changeset used incorporates the version
> bumps or not?

We can pull version.txt/milestone.txt for that revision and verify that they're versions match what's in the release config. We should only do this for buildNumber >= 2.

Additionally, we should also check that the following local files on disk match the tagged revisions in buildbot-configs:
- release config for the branch you're working on
- l10n-changests for the branch you're working on

Syed kindly volunteered to work on this :)

Comment 4

[Syed Albiz [:salbiz]]

Attachment: wrapper script to sendchange

implements the checks listed in comments. I wonder if I misunderstood the last one, since it seems unlikely that you would have a release_config that didn't exist in the tagged repo, and for example, it doesn't catch the case where you forget to pull/update between buildN..N+1, since the tag for buildN still exists, but has been superseded by the tag for buildN+1

Comment 5

[[e-mail] (:bhearsum)]

(In reply to comment #4)
> Created attachment 486499 [details] [diff] [review]
> wrapper script to sendchange
> 
> implements the checks listed in comments. I wonder if I misunderstood the last
> one, since it seems unlikely that you would have a release_config that didn't
> exist in the tagged repo

Yeah, when I say "the following local files on disk match the tagged revisions in buildbot-configs" I specifically mean comparing the checked in, tagged release config for the branch we're working on against the release_config.py found in the master directory"

Does that clear it up?

Comment 6

[[e-mail] (:bhearsum)]

(In reply to comment #4)
> Created attachment 486499 [details] [diff] [review]
> wrapper script to sendchange
> 
> implements the checks listed in comments. I wonder if I misunderstood the last
> one, since it seems unlikely that you would have a release_config that didn't
> exist in the tagged repo, and for example, it doesn't catch the case where you
> forget to pull/update between buildN..N+1, since the tag for buildN still
> exists, but has been superseded by the tag for buildN+1

Hmm, I think I see what you mean here. It _should_ still catch it so long as the _RELEASE tag has been updated, but there's no guarantee of that. Maybe we should compare against the _BUILDn tag instead -- can you think of any drawbacks to that?

Comment 7

[[e-mail] (:bhearsum)]

Comment on attachment 486499 [details] [diff] [review]
wrapper script to sendchange

Hmmm, I think compare() should do in a different module. I don't think it fits in commands.py. How about util/file.py?

testMakeHGUrl needs to be done differently -- do not depend on the network in a test! If you really want to test it like this you'll need to have the test set up a repo, and start serving it. I think that's overkill, though -- comparing an URL it returns vs. a manually constructed one is all you need to do IMHO.



>+def compareVersion(fileHandle, versionNumber):
>+    """Given an open readable file-handle look for the occurrence
>+       of the version # in the file"""
>+    log.info("looking for %s in %s" % (versionNumber, fileHandle.geturl()))
>+    ret = re.search(re.compile(re.escape(versionNumber), re.DOTALL), fileHandle.read())
>+    if ret:
>+        log.info("Found a match!")
>+    return ret

This check isn't quite specific enough -- there's always at least 2 different version numbers in any given release config. 

>+    if options.reconfig:
>+        reconfig()

This should happen at the end, and only if it's not a dry run.

>+    if options.check:
>+        from config import BRANCHES
>+        branchConfig = BRANCHES[releaseConfig['sourceRepoName']]
>+        verify_configs(
>+                releaseConfig['sourceRepoName'],
>+                "%s_BUILD%s" % (releaseConfig['baseTag'], releaseConfig['buildNumber']),
>+                branchConfig['hgurl'],
>+                releaseConfig['l10nRevisionFile'],
>+                )
>+        verify_repo(
>+                releaseConfig['sourceRepoPath'],
>+                releaseConfig['sourceRepoRevision'],
>+                branchConfig['hgurl']
>+                )
>+        if releaseConfig['buildNumber'] > 1:
>+            verify_build(
>+                    releaseConfig['sourceRepoPath'],
>+                    releaseConfig['sourceRepoRevision'],
>+                    branchConfig['hgurl'],
>+                    releaseConfig['version'],
>+                    releaseConfig['milestone']
>+                    )

Per IRC, version/repo/buildNumber need to get passed in rather than read from the config.

>+    if not options.dryrun:
>+        sendchange(
>+                releaseConfig['sourceRepoPath'],
>+                options.username,
>+                args[0]
>+                )


One overall improvement I'd like to see is for all of the checks to be run even if one of them fails.

Comment 8

[Syed Albiz [:salbiz]]

Attachment: update to run through entire set of checks even if some fail

Done. One change I didn't implement was changing compareVersion, since that doesn't actually hit the releaseConfigs, but the version/milestone.txt which should only contain one version string with no qualifiers. The releaseConfigs are just directly imported into the sanity.py script.

Comment 9

[[e-mail] (:bhearsum)]

Comment on attachment 486721 [details] [diff] [review]
update to run through entire set of checks even if some fail

>diff --git a/buildbot-helpers/sanity.py b/buildbot-helpers/sanity.py
>new file mode 100755
>--- /dev/null
>+++ b/buildbot-helpers/sanity.py
>@@ -0,0 +1,225 @@
>+#!/usr/bin/env python
>+"""%prog [-d|--dryrun] [-u|--username `username`] [-b|--bypasscheck]
>+        [-n|--no-reconfig] [-V| --version `version`] [-B --branch `branchname`]
>+        [-N|--build-number `buildnumber`] master:port

Looks like you need to remove the --no-reconfig option here.

>-from util.commands import run_cmd, get_output
>+from util.commands import run_cmd, get_output, compare

This import isn't valid anymore, and the compare tests should move to test_util_file.py anyways


This seems good overall, it'd be good to get one or two people to try it in staging, too.

Comment 10

[Syed Albiz [:salbiz]]

Attachment: fix script docstring, shuffle imports and unittests

Done. Definitely agree that it would be good to see this in a staging run, although I should mention it is set up to work only on the 0.8.0 release-configs at present. Does this need a reconfig window to land? I think other than adding make_hg_url to util.hg it doesn't even touch anything that could break.

Comment 11

[[e-mail] (:bhearsum)]

No reconfig window needed here, if it breaks something we can bypass it. I'm planning to do parts of a staging run today, I'd be happy to give it a try.

Comment 12

[Chris AtLee [:catlee]]

Comment on attachment 486903 [details] [diff] [review]
fix script docstring, shuffle imports and unittests

Just a few nits:

* name the script release_sanity.py or something to indicate that it's for releases

* parameters to verify_options shouldn't be _ prefixed

* verify_build exception message covers just milestone.txt, not other failures

* comments in the main section of the script to explain what's being checked

* "if not options.dryrun and test_success:" logic wrong if dry_run is set.  Won't tell you if you've passed tests or not.

Comment 13

[[e-mail] (:bhearsum)]

Hmm, got an exception running this in staging:
[cltbld@staging-master bhearsum]$ PYTHONPATH=tools/lib/python/:. python tools/buildbot-helpers/sanity.py -d -u bhearsum -V 3.6.12 -N 1 -B mozilla-1.9.2
2010-11-01 08:46:58,604 : ERROR : local configs do not match tagged revisions in repo
2010-11-01 08:46:58,630 : ERROR : local configs do not match tagged revisions in repo
2010-11-01 08:46:58,630 : ERROR : Error verifying configs
2010-11-01 08:46:58,631 : INFO : Checking for existence of http://hg.mozilla.org//users/bhearsum_mozilla.com/mozilla-1.9.2/rev/74e4421e0a30...
Traceback (most recent call last):
  File "tools/buildbot-helpers/sanity.py", line 202, in <module>
    branchConfig['hgurl']
  File "tools/buildbot-helpers/sanity.py", line 56, in verify_repo
    log.info("Got HTTP: %s!" % repo_page.getcode())
AttributeError: addinfourl instance has no attribute 'getcode'

Comment 14

[[e-mail] (:bhearsum)]

Ah, we also need to inherit the configs_repo_path and build_tools_repo_path found in master_localconfig.py -- this doesn't work too well in staging at the moment.

Comment 15

[[e-mail] (:bhearsum)]

Need to figure out how to look at the right release config in staging, too

Comment 16

[[e-mail] (:bhearsum)]

I'm having a lot of trouble running this in staging. Even after changing the hardcoding of config URLs I can't get it to pass. After adding some debugging prints I found that it appears to be getting redirected to the root of mozilla.org? Also strange is that only 1 of the verify_configs checks seems to fail? Here's the output, can you try to debug it?:
PYTHONPATH=tools/lib/python/:. python tools/buildbot-helpers/sanity.py -d -u bhearsum -V 3.6.12 -N 1 -B mozilla-1.9.2
http://hg.mozilla.org//users/bhearsum_mozilla.com/buildbot-configs/raw-file/FIREFOX_3_6_12_BUILD1/mozilla/staging_release-firefox-mozilla-1.9.2.py
FILE1 CONTENTS:
<!DOCTYPE html>
<html lang="en-US" dir="ltr">
<head>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"/>

    <meta name="DC.description" content="" />
    <meta name="DC.subject" content="" />
    <meta name="DC.creator" content="Happy Cog Studios - http://www.happycog.com" />
    <meta name="DC.creator" content="silverorange Inc - http://www.silverorange.com" />
    <meta name="DC.publisher" content="" />
    <meta name="DC.language" content="en" />
    <meta name="viewport" content="width=978" />

    <title>Mozilla.org - Home of the Mozilla Project</title>

...snip...

</body>
</html>

FILE2 CONTENTS:
releaseConfig = {}

releaseConfig['hgUsername']          = 'stage-ffxbld'
releaseConfig['hgSshKey']            = '~cltbld/.ssh/ffxbld_dsa'
releaseConfig['sourceRepoName']      = 'mozilla-1.9.2'
releaseConfig['sourceRepoClonePath'] = 'releases/mozilla-1.9.2'
releaseConfig['sourceRepoPath']      = 'users/bhearsum_mozilla.com/mozilla-1.9.2'
releaseConfig['sourceRepoRevision']  = '74e4421e0a30'
releaseConfig['relbranchOverride']   = ''
releaseConfig['l10nRepoClonePath']   = 'releases/l10n-mozilla-1.9.2'
releaseConfig['l10nRepoPath']        = 'users/bhearsum_mozilla.com'
releaseConfig['l10nRevisionFile']    = 'l10n-changesets_mozilla-1.9.2'
releaseConfig['mergeLocales']        = False
releaseConfig['cvsroot']             = ':ext:[e-mail]:/cvsroot'
releaseConfig['productName']         = 'firefox'
releaseConfig['appName']             = 'browser'
# Sometimes we need the application version to be different from what we "call"
# the build, eg public release candidates for a major release (3.1 RC1).
# appVersion and oldAppVersion are optional definitions used in places that
# don't care about what we call it. Eg, when version bumping we will bump to
# appVersion, not version.
releaseConfig['version']             = '3.6.12'
releaseConfig['appVersion']          = releaseConfig['version']
releaseConfig['milestone']           = '1.9.2.12'
releaseConfig['buildNumber']         = 1
releaseConfig['baseTag']             = 'FIREFOX_3_6_12'
releaseConfig['oldVersion']          = '3.6.11'
releaseConfig['oldAppVersion']       = releaseConfig['oldVersion']
releaseConfig['oldBuildNumber']      = 1
releaseConfig['oldBaseTag']          = 'FIREFOX_3_6_11'
releaseConfig['enUSPlatforms']       = ('linux', 'win32', 'macosx')
releaseConfig['l10nPlatforms']       = releaseConfig['enUSPlatforms']
releaseConfig['talosTestPlatforms']  = releaseConfig['enUSPlatforms']
releaseConfig['unittestPlatforms']   = releaseConfig['enUSPlatforms']
releaseConfig['xulrunnerPlatforms']  = releaseConfig['enUSPlatforms']
releaseConfig['patcherConfig']       = 'moz192-branch-patcher2.cfg'
releaseConfig['patcherToolsTag']     = 'UPDATE_PACKAGING_R11'
releaseConfig['binaryName']          = releaseConfig['productName'].capitalize()
releaseConfig['oldBinaryName']       = releaseConfig['binaryName']
releaseConfig['ftpServer']           = 'ftp.mozilla.org'
releaseConfig['stagingServer']       = 'staging-stage.build.mozilla.org'
releaseConfig['bouncerServer']       = 'download.mozilla.org'
releaseConfig['ausServerUrl']        = 'http://staging-stage.build.mozilla.org'
releaseConfig['ausUser']             = 'cltbld'
releaseConfig['ausSshKey']           = 'cltbld_dsa'
releaseConfig['releaseNotesUrl']     = None
releaseConfig['testOlderPartials']   = True
releaseConfig['useBetaChannel']      = 1
releaseConfig['verifyConfigs']       = {
    'linux':  'moz192-firefox-linux.cfg',
    'macosx': 'moz192-firefox-mac.cfg',
    'win32':  'moz192-firefox-win32.cfg'
}
releaseConfig['doPartnerRepacks']    = True
releaseConfig['partnersRepoPath']    = 'build/partner-repacks'
releaseConfig['majorUpdateRepoPath'] = None
# Tuxedo/Bouncer related
releaseConfig['tuxedoConfig']        = 'firefox-tuxedo.ini'
releaseConfig['tuxedoServerUrl']     = 'https://tuxedo.stage.mozilla.com/api/'
releaseConfig['skip_repo_setup']     = True

2010-11-01 10:00:04,083 : ERROR : local configs do not match tagged revisions in repo
FILE1 CONTENTS:
<!DOCTYPE html>
<html lang="en-US" dir="ltr">
<head>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"/>

    <meta name="DC.description" content="" />
    <meta name="DC.subject" content="" />
    <meta name="DC.creator" content="Happy Cog Studios - http://www.happycog.com" />
    <meta name="DC.creator" content="silverorange Inc - http://www.silverorange.com" />
    <meta name="DC.publisher" content="" />
    <meta name="DC.language" content="en" />
    <meta name="viewport" content="width=978" />

    <title>Mozilla.org - Home of the Mozilla Project</title>

...snip...

</body>
</html>

FILE2 CONTENTS:
de 7f2373ed4ebf
fr ca141e96484b

2010-11-01 10:00:04,112 : ERROR : local configs do not match tagged revisions in repo
2010-11-01 10:00:04,112 : ERROR : Error verifying configs
2010-11-01 10:00:04,112 : INFO : Checking for existence of http://hg.mozilla.org//users/bhearsum_mozilla.com/mozilla-1.9.2/rev/74e4421e0a30...
Traceback (most recent call last):
  File "tools/buildbot-helpers/sanity.py", line 203, in <module>
    branchConfig['hgurl']
  File "tools/buildbot-helpers/sanity.py", line 56, in verify_repo
    log.info("Got HTTP: %s!" % repo_page.getcode())
AttributeError: addinfourl instance has no attribute 'getcode'

Comment 17

[Syed Albiz [:salbiz]]

Attachment: rename, fix getcode() and other issues

The redirection issue was a quirk of urllib2 reacting to the double-slashes. Make_hg_url now trims extra slashes, and the testcase has been updated to reflect this. The script also imports the configs_repo_path and STAGING variables from localconfigs, so verify_configs should point to the correct file now. Tested on staging with pythonpath containing the cwd of the master containing the release_config.py and the tools/lib/python dir.

Comment 18

[Syed Albiz [:salbiz]]

Attachment: drop master_localconfig import, use cmd-line options for staging

Hmm, tested this on staging, and getting odd issues with import errors when pulling in master_localconfig. For testing purposes I've supplied this version of the patch which takes a -s/--staging option to indicate that it needs to grab staging release_configs for the current branch.

Comment 19

[Chris AtLee [:catlee]]

Comment on attachment 487565 [details] [diff] [review]
drop master_localconfig import, use cmd-line options for staging

Looks much better.

If possible, can you reorganize how the 'if not options.dryrun and test_success' blocks are done?  It's hard to figure out which variables have to be true/false to get to the final else: block.  Something like this maybe?

if test_success:
   if not options.dry_run:
      reconfig!
   else:
      tests pass!  not doing anything
else:
   FAIL!

Comment 20

[Syed Albiz [:salbiz]]

Attachment: refactor last stanza

Done. Still using a cmdline opt to determine whether or not to use staging configs.

Comment 21

[[e-mail] (:bhearsum)]

Comment on attachment 487648 [details] [diff] [review]
refactor last stanza

This is looking really good and it functioned as expected in staging once I passed the "-s" flag.

r+ with one small tweak: When differences are found in config files it would be really great to print a diff or at least the locations of the two files. It's a bit error prone to manually figure this out. Even if "Comparing http://hg.mozilla.org/build/buildbot-configs/.... to release_config.py" showed up in the INFO output, that'd be good enough.

Comment 22

[Syed Albiz [:salbiz]]

Attachment: add more verbose logging input when comparing tagged revisions

I've gone with adding the logging output showing which files were compared, adding diff-output seemed like it would be overkill, since the error case we seem to be trying to catch was a missed pull/update.

Comment 23

[[e-mail] (:bhearsum)]

Comment on attachment 487906 [details] [diff] [review]
add more verbose logging input when comparing tagged revisions

Great!

Comment 24

[[e-mail] (:bhearsum)]

Comment on attachment 487906 [details] [diff] [review]
add more verbose logging input when comparing tagged revisions

changeset:   1002:d3a78afea8cf

Comment 25

[[e-mail] (:bhearsum)]

Syed tells me that depending on what bug 502876 looks like we may need to update this a bit. Let's track that over there, though.