Ticket #296 (assigned enhancement)

Opened 6 months ago

Last modified 3 months ago

should have a way of running builds after X seconds of idle

Reported by: bhearsum Assigned to: bhearsum (accepted)
Priority: major Milestone: 0.8.0
Component: buildprocess Version: 0.7.7
Keywords: Cc: Pike, dustin

Description

Personally, I feel that the Periodic scheduler should behave this way by default. Either way, it's trivial to add this feature to it, or subclass it to an Idle scheduler.

I've got code for this, what do people prefer?

Attachments

idleScheduler-v2.diff (5.1 kB) - added by bhearsum on 06/04/08 12:27:04.
idle scheduler
builder-busyness.diff (4.4 kB) - added by bhearsum on 06/09/08 12:00:20.
expose builder busyness in a simple way (w/ tests)
moreSchedulerWatchers.diff (2.5 kB) - added by bhearsum on 06/25/08 13:17:42.
add more types of watchers for Schedulers
idleScheduler-again.diff (10.4 kB) - added by bhearsum on 06/25/08 13:18:18.
Idle Scheduler, again + docs + tests
notifySchedulers.diff (2.9 kB) - added by bhearsum on 07/08/08 08:30:06.
[v3] scheduler notification for buildsets
schedulersUseNotify.diff (4.2 kB) - added by bhearsum on 07/08/08 08:30:25.
[v3] enable schedulers to use buildset notification
idleScheduler.diff (8.2 kB) - added by bhearsum on 07/08/08 08:30:51.
[v3] idle scheduler using buildset scheduler notification

Change History

06/04/08 12:27:04 changed by bhearsum

  • attachment idleScheduler-v2.diff added.

idle scheduler

06/04/08 12:27:52 changed by bhearsum

  • owner set to bhearsum.
  • status changed from new to assigned.

Here's a possible implementation. I went with subclassing to avoid breaking existing installations. I'm doing real-world testing of it the next day or two.

06/04/08 16:05:10 changed by Pike

  • cc set to Pike.

It's be cool if "idle" really meant "not building anything". Getting a change doesn't really imply a build, and builds may not end at a specific time.

If we really did idle builders, that'd probably mean stopService on startBuildset and stopService on finishBuildset (or what the callbacks are called)

06/04/08 17:21:15 changed by bhearsum

Yeah, that might be better. Could probably just subscribe a method of the scheduler to the build. I'll give this some more thought.

(follow-ups: ↓ 6 ↓ 7 ) 06/04/08 19:12:04 changed by dustin

I'm *clearly* missing something, but how is this different from a regular Scheduler with a treeStableTimer?

06/04/08 19:15:15 changed by dustin

  • cc changed from Pike to Pike, dustin.

(in reply to: ↑ 4 ) 06/05/08 01:33:19 changed by Pike

Replying to dustin:

I'm *clearly* missing something, but how is this different from a regular Scheduler with a treeStableTimer?

treeStableTimer waits for n seconds to build a change set. IdleScheduler? builds every, say, 30 minutes if there's no triggered build.

One use case would be a performance test build, where, if you don't have new builds to test, you could gather more data on existing builds.

(in reply to: ↑ 4 ) 06/05/08 03:39:12 changed by bhearsum

Replying to dustin:

I'm *clearly* missing something, but how is this different from a regular Scheduler with a treeStableTimer?

The patch I attached is an obviously silly implementation and probably no different than a regular Scheduler w/ treeStableTimer. Pike's last comment is an accurate description.

06/05/08 07:23:22 changed by Pike

Should the scheduler be tied to a particular builder? Set of builders?

Might be more performant and compact to set up if one could have one IdleScheduler? for a bunch of related builds, but I'm not sure what idle would mean then. Ben, do we have a case where we'd actually would want to say "build all these builders when all of them had been idle for at least n secs"?

I guess the scheduler could then directly call into control.getBuilder(foo).requestBuildSoon(req) with some "idle force" BuildRequest?.

06/06/08 11:49:38 changed by bhearsum

So, the more I delve into this the harder of a problem it becomes. My first attempt was obviously silly, so I went and augmented Periodic to try and do it. As it turns out, if you have more than one Builder for any given branch you end up needing 1 Scheduler + 1 Periodic per Builder, otherwise you get an incorrect amount of Idle timer. This is a very common use case for us, I don't know about others. I've been thinking that it may be easier/better to implement it as part of Builder. How do others feel about that?

(follow-up: ↓ 13 ) 06/06/08 13:31:16 changed by Pike

I think it should be fairly easy to hook up a scheduler for a set of builders. Not that I tried.

I see two different scenarios:

a) whenever a single builder in a set is idle for n secs, force a build b) whenever all builders in a set have been idle for n secs, force a build on all

The back-end logic differs a bit. Anywho, the scheduler would subscribe to the BuilderStatus? objects for all its builders, and reset a timer, pretty much like treeStableTimer does, based on the logic (a vs b), and if the timer fires, it goes to the master, getBuilder(), and requestBuild with a fake BuildRequest?.

I wouldn't put that logic into the Builder itself, though.

(follow-up: ↓ 14 ) 06/06/08 14:36:20 changed by dustin

This sounds like the problem needs to be broken into sub-problems. Buildbot could probably do with a more sophisticated notion of "busy" vs. "idle", so that might be the first patch. The second patch would then add scheduling atop that new notion. I'm just throwing out ideas here :)

06/06/08 17:22:30 changed by Pike

Sounds to me like we're talking about partitions inside the master here. Group of builders, group of schedulers, group of changesources. Somewhat bundled together?

(in reply to: ↑ 10 ) 06/09/08 07:00:58 changed by bhearsum

Replying to Pike:

I think it should be fairly easy to hook up a scheduler for a set of builders. Not that I tried.

Well, I have a patch that works if you have 1 change source for 1 builder. For me, that means I need one change source per platform instead of one. I think that's silly. It would be bad to have multiple instances of a ChangeSource polling the exact same location when only one is needed.

(in reply to: ↑ 11 ) 06/09/08 07:01:44 changed by bhearsum

Replying to dustin:

This sounds like the problem needs to be broken into sub-problems. Buildbot could probably do with a more sophisticated notion of "busy" vs. "idle", so that might be the first patch. The second patch would then add scheduling atop that new notion. I'm just throwing out ideas here :)

Yeah, you're probably right.

Thanks for helping me iterate on this, both of you.

06/09/08 12:00:20 changed by bhearsum

  • attachment builder-busyness.diff added.

expose builder busyness in a simple way (w/ tests)

(follow-up: ↓ 16 ) 06/10/08 06:23:26 changed by Pike

I'd adjust the doc string to not talk about slaves, the rest looks good to me.

I'm not exactly sure if ~isBusy should be treated as isIdle in the edgecase when there are no slaves for that builder currently running. But it might be actually good to catch that in the scheduler code.

(in reply to: ↑ 15 ) 06/12/08 05:42:25 changed by bhearsum

Replying to Pike:

I'd adjust the doc string to not talk about slaves

Yeah, that makes sense.

I'm not exactly sure if ~isBusy should be treated as isIdle in the edgecase when there are no slaves for that builder currently running. But it might be actually good to catch that in the scheduler code.

I think that !isBusy in the Builder can be treated as isIdle. As you mention though, that case should be caught by whatever is triggering builds periodically.

06/25/08 13:16:58 changed by bhearsum

(I really wish there was a way to mark a patch as obsolete.)

In a minute I'm going to attach a couple patches that solve this bug. They basically implement the TODO i found in the Periodic scheduler:

# TODO: consider having this watch another (changed-based) scheduler and # merely enforce a minimum time between builds.

I avoided modifying the Periodic scheduler because I didn't want to cause unexpected behaviour change for people. There's not any harm I can see in having them both.

06/25/08 13:17:42 changed by bhearsum

  • attachment moreSchedulerWatchers.diff added.

add more types of watchers for Schedulers

06/25/08 13:18:18 changed by bhearsum

  • attachment idleScheduler-again.diff added.

Idle Scheduler, again + docs + tests

06/26/08 06:14:36 changed by bhearsum

So, I realized that my patch to add more Scheduler watchers doesn't work so well in a couple ways. It considers "build start" to be the same as "build set submitted", which is silly. It also won't work as intended for Schedulers that trigger multiple builders. Back to square one =\.

07/08/08 08:29:36 changed by bhearsum

Okay, here we go again...incoming series of patches that approach this in a different, better way.

07/08/08 08:30:06 changed by bhearsum

  • attachment notifySchedulers.diff added.

[v3] scheduler notification for buildsets

07/08/08 08:30:25 changed by bhearsum

  • attachment schedulersUseNotify.diff added.

[v3] enable schedulers to use buildset notification

07/08/08 08:30:51 changed by bhearsum

  • attachment idleScheduler.diff added.

[v3] idle scheduler using buildset scheduler notification

07/16/08 07:33:29 changed by bhearsum

I know this probably can't make 0.7.8 as it's a bit invasive, but if someone has few min to give me feedback that'd be great. I'm hoping to pre-land this in our Buildbot tree after 0.7.8.

08/30/08 13:20:28 changed by dustin

  • keywords set to review.

08/30/08 14:04:06 changed by dustin

  • keywords deleted.
  • milestone changed from undecided to 0.7.9.

Tracking this in git at

http://repo.or.cz/w/buildbot.git?a=shortlog;h=refs/remotes/dustin/bug296

My comments:

  • the notifyScheduler parameter should be described in some docstrings for the functions where it's available -- particularly in the interface. The one-oh work will hopefully have a more generic subscription system, so this may be a bit easier :)
  • in the second patch, you mention an unknown reason that the unit tests fail. Can you figure that out, or post the error here for others to puzzle over? The unit tests are fairly invasive, and make a lot of hidden assumptions about the source, so it may be that we need to tweak them somehow.
  • need some documentation

So not quite ready for review, but this looks pretty solid.

08/30/08 14:05:34 changed by dustin

Also, I get the following failure:

===============================================================================
[ERROR]: buildbot.test.test_run.Idle2.testTwoBranches

Traceback (most recent call last):
  File "/home/dustin/devel/projects/buildbot/t/buildbot/buildbot/test/test_run.py", line 376, in _testTwoBranches_branch1_2
    self.failUnlessEqual(self.isBuilding(), 1)
twisted.trial.unittest.FailTest: not equal:
a = 0
b = 1

09/02/08 05:42:19 changed by bhearsum

I'm really busy right now - I'm not going to have a chance to do anything about this before the release. I suspect that failure is due to the tests relying on timers, though.