Make runner responsible for buildbot startup on CentOS

RESOLVED FIXED

Status

Release Engineering
General Automation
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: ianconnolly, Assigned: ianconnolly)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

3 years ago
Created attachment 8460571 [details] [diff] [review]
manage_buildbot.diff

Pulling this out of Bug 1028191 because :Callek rightly pointed out that this is a subproblem and putting it in there is confusing.

Now to address his feedback.

>>> I didn't look too deeply at this patch yet, but I worry that it forces the >>> runner-startup model on all buildslave's, including servo and possibly qa, >>> etc. Even if they don't use the releng::slave toplevel, which is where runner >>> is setup/used.

This is something I definitely hadn't considered -- any suggestions to making it releng specific?

>>> I also see modules/buildslave/manifests/startup/runner.pp doesn't have any >>> requirements on runner itself being present...

also ++ on this, as with the above.
(Assignee)

Updated

3 years ago
Attachment #8460571 - Flags: review?(dustin)
(Assignee)

Updated

3 years ago
Blocks: 1042358
Comment on attachment 8460571 [details] [diff] [review]
manage_buildbot.diff

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

This looks good, aside from the class not doing what it says :)

Thanks for cleaning out the now-unused code.

::: modules/buildslave/manifests/startup/runner.pp
@@ +10,5 @@
> +    exec {
> +        'bb-service-delete':
> +            command => '/bin/rm /etc/rc3.d/*buildbot',
> +            refreshonly => true;
> +    }

I believe Callek said this in the other bug, but this class isn't doing what it promises to do:

Promise: "Set up buildbot to start via runner"
Actual:  "Disable initd startup of Buildbot"

This class should include runner and runner::tasks::buildbot.

::: modules/runner/templates/tasks/buildbot.py.erb
@@ +44,5 @@
> +    if not (os.path.isfile(BUILDSLAVE_CMD)
> +            and os.access(BUILDSLAVE_CMD, os.X_OK)):
> +        exit(0)
> +
> +    print "Starting buildbot via runslave.py."

Once this is all said and done, it'd be good to see runslave.py merged into this file.  Not least because runslave.py has a bunch of old cruft in it about NSCA reporting, finding basedirs, etc.

::: modules/toplevel/manifests/slave/releng/build/mock.pp
@@ +27,4 @@
>      include runner::tasks::checkout_tools
>      include runner::tasks::purge_builds
>      include runner::tasks::update_shared_repos
> +    include runner::tasks::buildbot

So, this shouldn't be here -- it should be handled in the buildslave::startup::runner class.
Attachment #8460571 - Flags: review?(dustin) → review-
(Assignee)

Comment 2

3 years ago
Created attachment 8462022 [details] [diff] [review]
updated_buildbot.diff

Feedback? rather than r? on the new diff because I'm concerned about the removal of the `requires` check on runslave.py -- should I be expressing that in runner::tasks::buildbot?
Attachment #8460571 - Attachment is obsolete: true
Attachment #8462022 - Flags: feedback?(dustin)
(Assignee)

Comment 3

3 years ago
Also the Class[buildslave::install] check.
Comment on attachment 8462022 [details] [diff] [review]
updated_buildbot.diff

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

This looks good to me!

The requires => .. for the Service resource weren't really necessary, since buildbot doesn't actuall start until puppet is done.  Similarly here: as long as runner.py and buildslave::install are installed by the time puppet finishes successfully, everything's fine.  Still, it wouldn't hurt to include buildslave::install in runner::tasks::buildbot, and require it, just to make it clear that the task won't work without buildslave installed.  If that ends up with a circular dependency, then drop the requires, but keep the include.
Attachment #8462022 - Flags: feedback?(dustin) → feedback+
(Assignee)

Comment 5

3 years ago
Created attachment 8462718 [details] [diff] [review]
includes-manage-buildbot.diff

Not getting any circular dependency errors when I apply this from the current production puppet config -- seems okay.
Attachment #8462022 - Attachment is obsolete: true
Attachment #8462718 - Flags: review?(dustin)
Attachment #8462718 - Flags: review?(dustin) → review+
(Assignee)

Comment 6

3 years ago
Comment on attachment 8462718 [details] [diff] [review]
includes-manage-buildbot.diff

Setting this before I forget. Not intended for check-in today, I swear.
Attachment #8462718 - Flags: checked-in?
(Assignee)

Updated

3 years ago
Depends on: 1045730
(Assignee)

Updated

3 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 7

3 years ago
Created attachment 8464853 [details] [diff] [review]
includes_manage_buildbot_2.diff

Carrying the r+ over from :dustin as it's basically the same -- just making sure we kill *all* of the buildbot init symlinks this time.
Attachment #8462718 - Attachment is obsolete: true
Attachment #8462718 - Flags: checked-in?
Attachment #8464853 - Flags: review+
Attachment #8464853 - Flags: checked-in?

Updated

3 years ago
Attachment #8464853 - Flags: checked-in? → checked-in+
(Assignee)

Comment 8

3 years ago
:sparkles: :cake: :sparkles:
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.