test-masters should test that universal masters still work

RESOLVED FIXED

Status

Release Engineering
General
--
major
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: jhford, Assigned: jhford)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [buildbotcustom-testing])

Attachments

(4 attachments)

I use universal masters to do my testing.  There is also a check that is disabled for multi-master mode, which is that every builder name in a scheduler refers to a real builder.  Turns out that this check is really useful.

I am not sure how multi master works 100%, but my guess is that the scheduler is creating builds for this nonexistant builder which never end up running.  Using the universal master.cfg with checkconfig does pick up this issue.

We should make sure that all of our masters are able to checkconfig when using a universal master.cfg.  Whether we do this by adding the master to production_masters.json, create a second json file or hardcode two universal masters is not important to me.

Example is the following changesets.

(testing)~/mozilla/testing $ hg id -R buildbotcustom/
7cb892e55990
(testing)~/mozilla/testing $ hg id -R buildbot-configs/
e599bb8e99e8 tip

With a multi-master setup, nothing errors out.  With a universal master.cfg file, I get:

AssertionError: <buildbotcustom.scheduler.SpecificNightly-props instance at 0x105175200> uses unknown builder Android Debug mozilla-central nightly
make: *** [checkconfig-build] Error 1

When I go back to the multi master.cfg, I can verify that the builder isn't actually created

(testing)~/mozilla/testing $ curl -i http://localhost:8501/builders/Android%20Debug%20mozilla-central%20nightly
HTTP/1.1 404 Not Found
Date: Wed, 19 Oct 2011 22:41:02 GMT
Content-Length: 173
Content-Type: text/html
Server: TwistedWeb/8.2.0

<html>
        <head><title>404 - No Such Resource</title></head>
        <body><h1>No Such Resource</h1>
            <p>No such child resource.</p>
        </body></html>
Severity: normal → major
Whiteboard: [buildbotcustom-testing]
to be clear, the priority for the reasons to fix this issue are, in order:
1) do a check that we can't do without testing universal masters
2) make development and testing easier
Blocks: 690860
I have a patch coming up shortly
Created attachment 568296 [details]
test-masters.sh log

As you can see, there are two deprecation warnings as well as *no* failure, even though there is actually failure due to bug 690860.
Assignee: nobody → jhford
Created attachment 568297 [details]
log of setup-masters.py with my changes

(should have mentioned, that test-masters.sh log I attached was run with my patch)

Note:
1) no deprecation warnings, easier to read
2) test status (TEST-PASS, TEST-FAIL, TEST-INFO) lines printed at useful points during the test
3) There is a summary at the end of the test with the number of passing and failing tests
4) if a master fails, the test goes on.  With the long time it takes to test the code, this is very useful to see narrow problems down, imo
5) last, but most important, it catches fallout from bug 690860 which is not caught by test-masters.sh

Not present in the log is the ability to limit by role.  This is only available for json testing.  This is useful you are only working on one master type.  It wouldn't be hard to add a -h/--host flag to test a specific master.

(testing)~/mozilla/testing/buildbot-configs $ ./setup-master.py -l -R try
bm04-try1
bm06-try1
bm09-try1
bm14-try1
pm02-try
try-mtv
Created attachment 568298 [details] [diff] [review]
buildbot-configs v1

This patch adds the ability to run tests to setup-masters.  I have attached a lot of the output from this patch.  I have also run test-masters.sh against this patch so I think I haven't broken the commonly used interfaces.  Extra verification would be great though!

It might seem odd to do the universal master logic in load_master_json.  This is the only safe place to do it because load_master_json is the expert on what files go in each master.  Changing this would be a significant change to the program.

This works with the -8 flag, and I would assume the -7 flag.

I also modified the __name__=='__main__' flow a bit to allow for more than one 'command'.  If I wasn't concerned with the interface used by other programs, I would have done things a little differently, like have explicit commands like:

./setup-masters.py list
./setup-masters.py test
./setup-masters.py create master-name master-dir

Ideas for further improvement:
-use python logging module
-write unit tests for setup-master.py
-rename setup-master.py to setup_master.py so other python modules can import it
-save logs from creating the master
-automatically checkconfig a master when its created
Attachment #568298 - Flags: review?(catlee)
Created attachment 568301 [details] [diff] [review]
interdiff for what's required to replace test-masters.sh with setup-masters.py --test

This patch converts test-masters.sh into a shortcut to launch ./setup-master.py with the familiar test-masters.sh syntax.

It also adds the TEMP environment variable to the test routine in setup-master.py
Attachment #568301 - Flags: feedback?
Comment on attachment 568298 [details] [diff] [review]
buildbot-configs v1

Review of attachment 568298 [details] [diff] [review]:
-----------------------------------------------------------------

::: setup-master.py
@@ +87,5 @@
> +        log = open(test_log_filename)
> +        log_size = os.path.getsize(test_log_filename)
> +        # We expect that the reconfig done message is before the last K of output
> +        if log_size > 1024:
> +            log.seek(log_size - 1024)

Is this really necessary?

@@ +100,5 @@
> +                print "TEST-FAIL checkconfig returned 0 for %s but didn't print 'Config file is good!'" % \
> +                        self.name
> +            else:
> +                print "TEST-FAIL %s failed to run checkconfig" % self.name
> +            #print "TEST-INFO log: %s, remnants of test: %s" % (test_log_filename, test_dir)

left over debugging code? remove it or uncomment it.
Attachment #568298 - Flags: review?(catlee) → review+
(In reply to Chris AtLee [:catlee] from comment #7)
> Comment on attachment 568298 [details] [diff] [review] [diff] [details] [review]
> buildbot-configs v1
> 
> Review of attachment 568298 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: setup-master.py
> @@ +87,5 @@
> > +        log = open(test_log_filename)
> > +        log_size = os.path.getsize(test_log_filename)
> > +        # We expect that the reconfig done message is before the last K of output
> > +        if log_size > 1024:
> > +            log.seek(log_size - 1024)
> 
> Is this really necessary?

No, but I think its a safe and good optimization.  It allows us to avoid iterating over the entire log, which could be quite long, especially if there are local debugging print statements.

> @@ +100,5 @@
> > +                print "TEST-FAIL checkconfig returned 0 for %s but didn't print 'Config file is good!'" % \
> > +                        self.name
> > +            else:
> > +                print "TEST-FAIL %s failed to run checkconfig" % self.name
> > +            #print "TEST-INFO log: %s, remnants of test: %s" % (test_log_filename, test_dir)
> 
> left over debugging code? remove it or uncomment it.

Removed.  This was from before I had the test summary at the end.


http://hg.mozilla.org/build/buildbot-configs/rev/d087a4d9c2a8
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Product: mozilla.org → Release Engineering
Attachment #568301 - Flags: feedback?
You need to log in before you can comment on or make changes to this bug.