622980 - runslave.py should extract basedir from buildbot.tac

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Dustin J. Mitchell [:dustin] (he/him)]

right now, runslave is keying off the slave name to determine the basedir - this doesn't make a lot of sense!  That information should be in one place - probably in the slave allocator's database.

But runslave needs to know where to put the new buildbot.tac.  The easiest solution is for runslave to just extract the basedir from buildbot.tac.

Comment 1

[Dustin J. Mitchell [:dustin] (he/him)]

There's a bit of a race condition here, in the sense that runslave.py usually looks for DO_NOT_START in the basedir before requesting the tac.  I don't see why that shouldn't go in the opposite order, though.

Comment 2

[Dustin J. Mitchell [:dustin] (he/him)]

I don't think DO_NOT_START will be necessary in the final analysis, so the race condition is not important.

Comment 3

[Dustin J. Mitchell [:dustin] (he/him)]

Attachment: m622980-puppet-manifests-r1.patch

This adds tests, since I added some complex code.

runslave.py needs to be able to work without the allocator, so it needs to be able to figure out the basedir on its own.  But we'd prefer to take the allocator's word for the basedir when possible.

It occurs to me as I post this that having two sources of truth may lead us astray: if the allocator and runslave eventually disagree about the basedir for a slave, we won't find out until the allocator is down and the slave fails to start.  So maybe it's better for runslave to just know what the basedirs are natively?  What do you think?

Comment 4

[[e-mail] (:bhearsum)]

Comment on attachment 510829 [details] [diff] [review]
m622980-puppet-manifests-r1.patch

>diff --git a/modules/buildslave/files/runslave.py b/modules/buildslave/files/runslave.py

>+    def extract_basedir(self, page):
>+        mo = re.search("^basedir *= *([ur]?['\"].*['\"])", page, re.M|re.I)

I think you want '\s*' instead of ' *' in both places.

>+        if mo:
>+            basedir_cmd = mo.group(1)
>+            basedir = eval(basedir_cmd.strip())
>+            return basedir
>+        else:
>+            return None

If this returns None, self.basedir will end up being None, which will make run() fail. Any reason not to return guess_basedir(self.options.slavename) here, and pull it out of get_filename()


>+
>+class BuildbotTac(unittest.TestCase):
>+
>+    def setUp(self):
>+        self.old_guess_basedir = runslave.guess_basedir
>+        self.old_urlopen = urllib2.urlopen
>+        self.basedir = os.path.abspath('__testdir')

Per IRC, better to mktemp() for this.

Can you file a bug about having preproduction run the tests?

Comment 5

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 510829 [details] [diff] [review]
m622980-puppet-manifests-r1.patch

(In reply to comment #4)
> I think you want '\s*' instead of ' *' in both places.

Yes

> If this returns None, self.basedir will end up being None, which will make
> run() fail. Any reason not to return guess_basedir(self.options.slavename)
> here, and pull it out of get_filename()

no, because get_filename will call it.  That's not particularly cleanly organized, so I'll fix it up.

> Per IRC, better to mktemp() for this.

Yes, good point.

> Can you file a bug about having preproduction run the tests?

Done.

Comment 6

[Dustin J. Mitchell [:dustin] (he/him)]

Attachment: m622980-puppet-manifests-r2.patch

Updated version to make the fallbacks for finding the basedir more explicit, and better-tested.

Comment 7

[[e-mail] (:bhearsum)]

Comment on attachment 511530 [details] [diff] [review]
m622980-puppet-manifests-r2.patch

Looks good to me!

Comment 8

[Dustin J. Mitchell [:dustin] (he/him)]

m622980-puppet-manifests-r2.patch staged successfully and pushed as f98e628a4936.  Deployed to all masters.