Bug 316 - Load steps YAML from Servo repo #368

shinglyu · 2016-05-10T07:33:56Z

This should fix #316

I only tested it with buildbot checkconfig master (in the servo-master1 VM). I did try to execute the step in the python interactive console, but some part requires extensive mocking, so I gave up and submit a PR first.

This change is

shinglyu · 2016-05-10T07:34:17Z

r? @aneeshusa @edunham

edunham · 2016-05-11T20:35:46Z

Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

buildbot/master/files/config/master.cfg, line 141 [r1] (raw file):

    """
    Builder which uses DynamicServoIntreeYAMLFactory to run steps from a 
    yaml file in the Servo source tree.

This comment is unclear to me -- I expect that the yaml builder would only parse the YAML file from the Servo repo then hand that config back to the master, yet the comment makes it sound like the yaml builder will be what then runs the loaded steps.

Comments from Reviewable

aneeshusa · 2016-05-13T03:48:04Z

buildbot/master/files/config/factories.py

+    yaml_path = ""
+
+    def __init__(self, builder_name, environment, yaml_path):
+        print(yaml_path)


No need to print this.

aneeshusa · 2016-05-13T03:48:13Z

buildbot/master/files/config/factories.py

+        self.yaml_path = yaml_path
+
+    def run(self):
+        print(self.yaml_path)


No need to print this.

aneeshusa · 2016-05-13T03:50:13Z

buildbot/master/files/config/factories.py

+                                         decodeRC={0: SUCCESS, 1: SUCCESS})]
+
+        # util.BuildFactory is an old-style class so we cannot use super()
+        # but must hardcode the superclass here


Comment doesn't apply here.

aneeshusa · 2016-05-13T03:52:01Z

buildbot/master/files/config/factories.py

+
+        # util.BuildFactory is an old-style class so we cannot use super()
+        # but must hardcode the superclass here
+        self.build.steps = pkill_step + dynamic_steps


Let's use += instead of = for now, so that we have a record of the StepsYAMLParsingStep in the buildbot logs.

aneeshusa · 2016-05-13T03:53:40Z

buildbot/master/files/config/factories.py

+            dynamic_steps = [self.make_step(command) for command in commands]
+        except Exception as e:  # Bad step configuration, fail build
+            print(str(e))
+            dynamic_steps = [BadConfigurationStep(e)]


Instead of creating a BadConfigurationStep to raise an exception, we should just raise the exception here since we're already in a step. (It was a separate step before because the parsing was done at Buildbot start-up time, and raising an exception them would kill the Buildbot master.)

aneeshusa · 2016-05-13T03:55:40Z

buildbot/master/files/config/factories.py

+    """
+    Step which resolves the in-tree YAML and dynamically add test steps
+    """
+    yaml_path = ""


A default of "" seems dangerous. Let's just remove this line entirely.

aneeshusa · 2016-05-13T03:56:07Z

buildbot/master/files/config/factories.py

+    Step which resolves the in-tree YAML and dynamically add test steps
+    """
+    yaml_path = ""
+


We should set haltOnFailure = True and flunkOnFailure = True here, just as we do for BadConfigurationStep.

aneeshusa · 2016-05-13T03:58:11Z

buildbot/master/files/config/factories.py

+    """
+
+    def __init__(self, builder_name, environment):
+        self.environment = environment


No need to stash a copy of this locally, we can remove this line.

aneeshusa · 2016-05-13T03:58:22Z

buildbot/master/files/config/factories.py

+
+        # util.BuildFactory is an old-style class so we cannot use super()
+        # but must hardcode the superclass here
+        # TODO: pass environments


Looks like you took care of this TODO already.

aneeshusa · 2016-05-13T03:59:21Z

buildbot/master/files/config/factories.py

+    """
+    Smart factory which takes a list of shell commands from a yaml file
+    and creates the appropriate Buildbot Steps. Uses heuristics to infer
+    Step type, if there are any logfiles, etc.


Please update this comment to make it clear that the YAML file is coming from the main servo/servo repo (and capitalize YAML also).

aneeshusa · 2016-05-13T04:00:53Z

buildbot/master/files/config/factories.py

@@ -103,6 +103,86 @@ def make_step(self, command):
        return step_class(**step_kwargs)


+class StepsYAMLParsingStep(buildstep.BuildStep):
+    """
+    Step which resolves the in-tree YAML and dynamically add test steps


style nit: Period at the end of this line.

aneeshusa · 2016-05-13T04:06:23Z

buildbot/master/files/config/factories.py

+            dynamic_steps = [BadConfigurationStep(e)]
+
+        # TODO: windows compatibility (use a custom script for this?)
+        pkill_step = [steps.ShellCommand(command=["pkill", "-x", "servo"],


Let's not put this step in a list here, since pkill_step is singular.

Instead, we can wrap it in a list by adding a line here:

static_steps = [pkill_step]

and later on using static_steps + dynamic_steps.

(We could also directly use [pkill_step] + dynamic_steps, but I think that's a little less clear.)

aneeshusa · 2016-05-13T04:32:02Z

Since we're just testing the YAML loading, let's get rid of linux-rel-yaml and rename linux-yaml to linux-dev-yaml. Using the debug build will be faster and use less extra resources.

You'll also need to make a PR to servo/servo to add a linux-dev-yaml section in the etc/ci/buildbot_steps.yml file, which will need to land before this does.

DynamicServoIntreeYAMLFactory is a pretty long name - once we confirm this is working we should rename it to DynamicServoFactory to replace the current version in a follow-up, but we may want to use something shorter now as well.

I believe we need to create and invoke a Buildbot RemoteCommand(http://docs.buildbot.net/current/developer/cls-remotecommands.html) (or possibly RemoteShellCommand?). From my understanding, steps are run on the master, and use BuildStep.runCommand(https://docs.buildbot.net/current/developer/cls-buildsteps.html#buildbot.process.buildstep.BuildStep.runCommand) to actually run commands on the worker. (N.B.: RemoteCommand is just a Buildbot class representing a command to run on the worker, not a Buildbot Step, i.e. it's not a special type of ShellCommand. Same for RemoteShellCommand. This tripped me up a bunch.) Because the buildbot_steps.yml file is on the worker, we need to invoke runCommand to extract the contents of the file on the worker and send them back. I haven't looked into this 100%, but https://github.com/isotoma/buildbot_travis/blob/master/buildbot_travis/steps/base.py should be a decent example. I'll try to look into this more to provide more concrete advice.

Please also address the test suite failures from Travis.

shinglyu · 2016-05-13T07:07:47Z

@aneeshusa So in my StepsYAMLParsingStep::run(), I should run a RemoteCommand to get the content of the YAML file. (Something like cat /etc/ci/buildbot_steps.yml), then parse it and generate the dynamic steps?

shinglyu · 2016-05-13T09:21:27Z

@anneeshusa Also I have a question regarding @edunham 's comment. If I change the StepsYAMLParsingStep::run() to get the YAML back with RemoteCommand, and then I call the self.build.steps += ..., does that effectively make the DynamicServoYAMLBuilder (the old DynamicServoIntreeYAMLBuilder) run the parsed step? Or the self.build.steps is run by other Builders?

Or perhaps I should just return the parsed YAML content back to the master, and let the master start another builder with the parsed steps? But I don't see a clear place in the master.cfg to do this?

aneeshusa · 2016-05-13T16:46:08Z

As far as I understand, you're correct that you'll need to runCommand a RemoteCommand (or maybe RemoteShellCommand), then parse the output and generate the dynamic steps. I highly recommend cloning the buildbot source tree, checking out the eight branch, and reading the Buildbot code to figure out exactly how this works - I'm not 100% sure.

DynamicServoYAMLBuilder should pick up on the new steps and continue to run them. Buildbot 9 (next major version) explicitly adds support for this behavior: docs here and code here.

I think the end result should look pretty close to the GenerateStagesCommand in the linked docs.

bors-servo · 2016-05-14T00:34:05Z

☔ The latest upstream changes (presumably #373) made this pull request unmergeable. Please resolve the merge conflicts.

shinglyu · 2016-05-16T06:16:41Z

@aneeshusa Thanks! Let me read that and get back to you.

shinglyu · 2016-05-23T02:18:14Z

@aneeshusa I ran the script throught REPL and didn't see any error, but I might need a full setup to varify it works.

aneeshusa · 2016-06-08T09:05:45Z

buildbot/master/files/config/factories.py

@@ -103,6 +104,95 @@ def make_step(self, command):
        return step_class(**step_kwargs)


+class StepsYAMLParsingStep(buildstep.ShellMixin, buildstep.BuildStep):
+    """
+    Step which resolves the in-tree YAML and dynamically add test steps.


grammar nit: add -> adds

aneeshusa · 2016-06-08T09:07:48Z

buildbot/master/files/config/factories.py

+            yield self.runCommand(cmd)
+
+            if cmd.didFail():
+                raise Exception(str(cmd.result()))


I don't think there's a result method on RemoteShellCommand.
It may be better to capture stderr and output that, or maybe return a message with the return code (cmd.rc).

aneeshusa · 2016-06-08T09:10:50Z

buildbot/master/files/config/factories.py

+                                                    collectStdout=True)
+            yield self.runCommand(cmd)
+
+            if cmd.didFail():


We should check that the result is SUCCESS, not just that it didn't fail, because there are other possible results (e.g. WARNING, RETRY, etc.). We can use this snippet from the Buildbot 9 docs:

result = cmd.results() if result != util.SUCCESS: # raise here # continue extracting stdout and building steps, etc. here

aneeshusa · 2016-06-08T09:12:28Z

buildbot/master/files/config/factories.py

+                commands = builder_steps[self.builder_name]
+                dynamic_steps = [self.make_step(command) for command in commands]
+        except Exception as e:  # Bad step configuration, fail build
+            # Capture the expception and re-raise with a friendly message


spelling: expception -> exception

aneeshusa · 2016-06-08T09:12:50Z

buildbot/master/files/config/factories.py

+
+        self.build.steps += static_steps + dynamic_steps
+
+        defer.returnValue(cmd.reslts())


We can just re-use the result variable I mentioned adding earlier.

aneeshusa · 2016-06-08T09:22:36Z

buildbot/master/files/config/factories.py

+    def run(self):
+        try:
+            full_yaml_path = os.path.join(self.build.workdir, self.yaml_path)
+            print_yaml_cmd = "cat {}".format(full_yaml_path)


I believe the command will be run in the workdir by default, so just passing self.yaml_path as a relative path should work. Also, I don't think the current code wouldn't work for Windows, as the os.path.join would take place on the master (and use /), but the generated command would actually execute on the worker.

aneeshusa · 2016-06-08T09:23:09Z

buildbot/master/files/config/factories.py

+            if cmd.didFail():
+                raise Exception(str(cmd.result()))
+            else:
+                builder_steps = yaml.load(cmd.stdout)


We should use yaml.safe_load.

aneeshusa · 2016-06-08T09:25:29Z

buildbot/master/files/config/factories.py

+        self.yaml_path = yaml_path
+
+    @defer.inlineCallbacks
+    def run(self):


Just for now, can you also wrap the entire run function in a try/except block? I want to make sure that the Buildbot master process doesn't come down even if this code doesn't work properly. Once it's working we can get rid of it.

That was addressed but GitHub didn't know

aneeshusa · 2016-06-08T09:26:30Z

buildbot/master/files/config/factories.py

+    flunkOnFailure = True
+
+    def __init__(self, builder_name, environment, yaml_path):
+        self.builder_name = builder_name


I believe you also need to setup the shell mixin and call BuildStep.__init__ as done in the example in the Buildbot Nine docs.

aneeshusa · 2016-06-08T09:28:00Z

Please also rebase when you address the review comments. Looks pretty close, so I'm hoping this works on the first try!

aneeshusa · 2016-06-17T15:30:25Z

buildbot/master/files/config/master.cfg

@@ -158,6 +177,9 @@ c['builders'] = [
        factory=factories.doc,
        category="auto",
    ),
+    # Testing the In Tree YAML build
+    DynamicServoYAMLBuilder("linux-dev-yaml", LINUX_SLAVES,
+                            envs.build_linux_headless)


envs.build_linux_headless has been removed; use envs.build_linux instead. (I think it might fit on the previous line, not sure.)

aneeshusa · 2016-06-17T15:35:24Z

buildbot/master/files/config/factories.py

+                result = cmd.results()
+                if result != util.SUCCESS:
+                    raise Exception(
+                        "Command failed with return code: {}" + str(cmd.rc)


I think you meant to use .format() here.

aneeshusa · 2016-06-17T15:45:02Z

buildbot/master/files/config/factories.py

+                # Capture the exception and re-raise with a friendly message
+                raise Exception("Bad configuration, " +
+                                "unable to convert to steps" +
+                                str(e))


Let's change this block so that the error message is uninterrupted (e.g. greppable) and a little friendlier:

raise Exception("Bad steps configuration for {}: {}".format( self.builder_name, str(e) ))

aneeshusa · 2016-06-17T15:50:52Z

buildbot/master/files/config/factories.py

+
+            defer.returnValue(result)
+        except Exception as e:
+            print(str(e))


Please re-raise the exception so that the build fails and we're notified. (Otherwise it may silently succeed if the new steps haven't been added yet!)

Actually, the entire outermost try/except block isn't helpful and we don't need it, because Buildbot handles exceptions safely (e.g. http://build.servo.org/builders/mac-rel-css/builds/1606). Please remove it, sorry for the go-around.


        Bug 316 - Load steps YAML from Servo repo

aneeshusa · 2016-06-26T02:16:01Z

This looks good to me, let's see what Buildbot thinks of it. @bors-servo r+

bors-servo · 2016-06-26T02:16:03Z

📌 Commit a2dad1e has been approved by aneeshusa


        Auto merge of #368 - shinglyu:yaml, r=aneeshusa

Bug 316 - Load steps YAML from Servo repo This should fix #316 I only tested it with `buildbot checkconfig master` (in the `servo-master1` VM). I did try to execute the step in the python interactive console, but some part requires extensive mocking, so I gave up and submit a PR first.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/saltfs/368)

bors-servo · 2016-06-26T02:16:05Z

⌛ Testing commit a2dad1e with merge b9a190b...

bors-servo · 2016-06-26T02:29:06Z

☀️ Test successful - travis

aneeshusa reviewed May 13, 2016
View changes

buildbot/master/files/config/factories.py Outdated

yaml_path = ""

def __init__(self, builder_name, environment, yaml_path):

print(yaml_path)

This comment has been minimized.

Sign in to view

aneeshusa May 13, 2016
Member

No need to print this.

aneeshusa reviewed May 13, 2016
View changes

buildbot/master/files/config/factories.py Outdated

self.yaml_path = yaml_path

def run(self):

print(self.yaml_path)

This comment has been minimized.

Sign in to view

aneeshusa May 13, 2016
Member

No need to print this.

aneeshusa reviewed May 13, 2016
View changes

buildbot/master/files/config/factories.py Outdated

"""

def __init__(self, builder_name, environment):

self.environment = environment

This comment has been minimized.

Sign in to view

aneeshusa May 13, 2016
Member

No need to stash a copy of this locally, we can remove this line.

aneeshusa reviewed May 13, 2016
View changes

aneeshusa reviewed Jun 8, 2016
View changes

buildbot/master/files/config/factories.py Outdated

self.build.steps += static_steps + dynamic_steps

defer.returnValue(cmd.reslts())

This comment has been minimized.

Sign in to view

aneeshusa Jun 8, 2016
Member

We can just re-use the result variable I mentioned adding earlier.

aneeshusa reviewed Jun 8, 2016
View changes

buildbot/master/files/config/factories.py Outdated

if cmd.didFail():

raise Exception(str(cmd.result()))

else:

builder_steps = yaml.load(cmd.stdout)

This comment has been minimized.

Sign in to view

aneeshusa Jun 8, 2016
Member

We should use yaml.safe_load.

aneeshusa reviewed Jun 8, 2016
View changes

shinglyu force-pushed the shinglyu:yaml branch 2 times, most recently from 5f2dad9 to 3ae16e1 Jun 16, 2016

aneeshusa reviewed Jun 17, 2016
View changes

Bug 316 - Load steps YAML from Servo repo

Loading status checks…

a2dad1e

shinglyu force-pushed the shinglyu:yaml branch from 3ae16e1 to a2dad1e Jun 21, 2016

aneeshusa mentioned this pull request Jun 26, 2016

Move buildbot logic to scripts in `/etc/ci` #316

Closed

4 of 4 tasks complete


		self.build.steps += static_steps + dynamic_steps

		defer.returnValue(cmd.reslts())

servo / saltfs

Join GitHub today

Bug 316 - Load steps YAML from Servo repo #368

Bug 316 - Load steps YAML from Servo repo #368

Conversation

shinglyu commented May 10, 2016 • edited by larsbergstrom

shinglyu commented May 10, 2016

edunham commented May 11, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

aneeshusa commented May 13, 2016 • edited

shinglyu commented May 13, 2016

shinglyu commented May 13, 2016

aneeshusa commented May 13, 2016

bors-servo commented May 14, 2016

shinglyu commented May 16, 2016

shinglyu commented May 23, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

aneeshusa commented Jun 8, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

aneeshusa commented Jun 26, 2016

bors-servo commented Jun 26, 2016

bors-servo commented Jun 26, 2016

bors-servo commented Jun 26, 2016

shinglyu commented May 10, 2016 •

edited by larsbergstrom

aneeshusa commented May 13, 2016 •

edited