Issue 2547 [FIXED] by kassemsandarusi · Pull Request #1370 · buildbot/buildbot

kassemsandarusi · 2014-11-20T04:40:13Z

This was a rather small issue, but i took the liberty of taking this opportunity to move the management of buildslaves into the buildslavemanager. This is my first patch so i would appreciate any feedback if possible.

…rvice parent. Added tests for BuildslaveManager. Updated old tests to conform to new service parent. Updated minor documents to conform to new change. Fixes :bb🐛`2547`.

…ious commit fixes buildbot#2547.

benallard · 2014-11-20T10:12:04Z

master/buildbot/process/botmaster.py

Any reason to leave this commented out ?

And I think the body of the function should go away as well.

sa2ajj · 2014-11-20T13:38:42Z

👍

Two notes:

I think @benallard has a valid question (I think that comment could just be removed)
such a substantial change is worth a note in release notes

djmitche · 2014-11-20T14:11:21Z

master/buildbot/buildslave/manager.py

This TODO can go away, too!

djmitche · 2014-11-20T14:13:14Z

What @sa2ajj said :)

sa2ajj · 2014-11-20T14:15:07Z

So things to do:

remove extra comments
add release notes
remove obsolete TODO item

and then we are good to go.

Thanks, @kassemsandarusi

…. Added onto the release notes.

kassemsandarusi · 2014-11-20T19:11:38Z

I removed some of the remaining obselete items within Botmaster as well.

sa2ajj · 2014-11-20T19:16:32Z

Excellent. Let's wait for the Travis to say its opinion :)

benallard · 2014-11-20T19:27:02Z

👍

kassemsandarusi · 2014-11-20T23:19:05Z

Not fair Travis failing me on purpose. :P

Make it possible to have slaves and builders with the same name Fixes ticket:2547

sa2ajj · 2014-11-21T07:24:08Z

Thank you, @kassemsandarusi :)

sa2ajj · 2014-11-21T07:26:09Z

Interesting: we lost unicodeness somewhere...

sa2ajj · 2014-11-21T08:28:55Z

For the record Trac ticket is 3058.

kassemsandarusi · 2014-11-21T08:36:47Z

I'll take a look at it tonight.

…547" This reverts commit c422c95, reversing changes made to e9655c7.

…547" This reverts commit c422c95, reversing changes made to 41dfe5b.

sa2ajj · 2014-11-21T11:16:18Z

@delanne said on IRC today: since I fetch the buildbot upstream (master branch), my buildbot master doesn't schedule builds ;(

benallard · 2014-11-21T11:36:15Z

Looks like we really need more testing around the slave handling stuff (TRAC-3016 among others)

kassemsandarusi · 2014-11-21T20:27:40Z

Well this is embarassing. Do you have any idea where I should look? I've tried searching for references of anything that changed.

edit: for the record, GMT -6 (CST)

benallard · 2014-11-21T20:36:25Z

I don't believe what @delanne saw was related to this PR. It was most probably another manifestation of TRAC-3051, just that restarting his master fixed it for him (only at the same time, he reverted the PR).

kassemsandarusi · 2014-11-21T20:43:51Z

Oh I see, thanks. I thought my commit was reverted out of the master for a second.

sa2ajj · 2014-11-21T20:44:52Z

No it was not.

However, we definitely need some integration tests.

kassemsandarusi · 2014-11-21T20:47:52Z

Right, I am actually looking right now into cleaning up the current unit tests and adding integration tests.

sa2ajj · 2014-11-21T20:50:22Z

Then, hopefully, we either see where the problem could be or will be more certain that things are OK 😄

sa2ajj · 2014-11-23T18:39:11Z

@kassemsandarusi the test failure is not related to your change at all. It looks like the tags can be returned in different order, as a result comparison fails.

sa2ajj · 2014-11-23T18:39:25Z

But more tests are welcome :)

delanne · 2014-11-24T07:50:44Z

@benallard, I don't think it's a manifestation of trac 3051. before this PR, my buildbot correctly schedules jobs. After this PR is doesn't. If I revert the 2 commits, then it's working.

benallard · 2014-11-24T07:59:12Z

@delanne: Because you restarted your master ! You had the exact same symptoms as TRAC-3051, and I had the trouble before this PR. Beside, my master runs fine with this one in.

delanne · 2014-11-24T08:02:09Z

So, why when I restart my master without the 2 commits it works, and when I restart my master with this 2 commits, it doesn't. That's weird.

delanne · 2014-11-24T08:03:47Z

without these 2 commits, in my twistd.log, I can see:

2014-11-24 09:02:42+0100 [-] Latent buildslave cmgwLinterSlave1 attached to cmgw_linty
2014-11-24 09:02:42+0100 [-] Latent buildslave cmgwLinterSlave1 attached to cmgw_build_u14
2014-11-24 09:02:42+0100 [-] Latent buildslave cmgwSlave2 attached to cmgw_build_u12
2014-11-24 09:02:42+0100 [-] Latent buildslave cmgwSlave1 attached to cmgw_build_u14
2014-11-24 09:02:42+0100 [-] Latent buildslave ctafSlave1 attached to ctaf_py27
2014-11-24 09:02:42+0100 [-] Latent buildslave ctafSlave1 attached to ctaf_linty
2014-11-24 09:02:42+0100 [-] Latent buildslave ctafSlave1 attached to ctaf_sandbox
2014-11-24 09:02:42+0100 [-] Latent buildslave ctafSlave1 attached to ctaf_documentation
2014-11-24 09:02:42+0100 [-] Latent buildslave ctafSlave2 attached to ctaf_py27
2014-11-24 09:02:42+0100 [-] Latent buildslave ctafSlave2 attached to ctaf_linty
2014-11-24 09:02:42+0100 [-] Latent buildslave ctafSlave2 attached to ctaf_sandbox
2014-11-24 09:02:42+0100 [-] Latent buildslave ctafSlave2 attached to ctaf_documentation
2014-11-24 09:02:42+0100 [-] Latent buildslave ctafExecutorSlave1 attached to cmgw_ctaf_test

with the 2 commits, I don't see these lines => It seems that slaves aren't attached to builders.

benallard · 2014-11-24T08:31:21Z

Then you know what's left to do ;) Roll up your sleeves, and start hacking ! It's free software after all !

sa2ajj · 2014-11-24T08:32:38Z

@benallard there's an ongoing discussion on IRC atm.

delanne · 2014-11-24T09:27:32Z

$ git diff
diff --git a/master/buildbot/buildslave/manager.py b/master/buildbot/buildslave/manager.py
index d8a665e..628da86 100644
--- a/master/buildbot/buildslave/manager.py
+++ b/master/buildbot/buildslave/manager.py
@@ -62,6 +62,7 @@ class BuildslaveManager(config.ReconfigurableServiceMixin,

     name = "buildslaves"
     PING_TIMEOUT = 10
+    reconfig_priority = 127

     def __init__(self, master):
         service.MultiService.__init__(self)

This patch fix the issue for me.

djmitche · 2014-11-24T13:14:47Z

Good fix - I missed that in review.

kassemsandarusi added 4 commits November 19, 2014 22:34

Separated buildslaves from botmaster. BuildslaveManager is now the se…

ab58fe3

…rvice parent. Added tests for BuildslaveManager. Updated old tests to conform to new service parent. Updated minor documents to conform to new change. Fixes :bb🐛`2547`.

Merge branch 'master' into issue_2547

5e85ccd

Minor fixes that conform to buildbot coding/patch specification. Prev…

d78f60b

…ious commit fixes buildbot#2547.

Fixed code to conform with what pep8 is expecting.

ff3af15

benallard reviewed Nov 20, 2014
View reviewed changes

sa2ajj added needs work needs more review labels Nov 20, 2014

djmitche reviewed Nov 20, 2014
View reviewed changes

master/buildbot/buildslave/manager.py Outdated

Copy link
Copy Markdown

Member

djmitche Nov 20, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This TODO can go away, too!

djmitche removed the needs more review label Nov 20, 2014

Removed obselete TODO. Removed obselete Botmaster comments and fields…

9d63176

…. Added onto the release notes.

sa2ajj added needs more review and removed needs work labels Nov 20, 2014

Fixed imports.

41dfe5b

sa2ajj pushed a commit that referenced this pull request Nov 21, 2014

Merge pull request #1370 from kassemsandarusi/issue_2547

c422c95

Make it possible to have slaves and builders with the same name Fixes ticket:2547

sa2ajj merged commit c422c95 into buildbot:master Nov 21, 2014

sa2ajj added the merge-me label Nov 21, 2014

delanne pushed a commit to delanne/buildbot that referenced this pull request Nov 21, 2014

Revert "Merge pull request buildbot#1370 from kassemsandarusi/issue_2…

fffee5c

…547" This reverts commit c422c95, reversing changes made to e9655c7.

delanne pushed a commit to delanne/buildbot that referenced this pull request Nov 21, 2014

Revert "Merge pull request buildbot#1370 from kassemsandarusi/issue_2…

c6eb75c

…547" This reverts commit c422c95, reversing changes made to 41dfe5b.

delanne mentioned this pull request Nov 25, 2014

List all changes related to a build. #1305

Merged

Conversation

kassemsandarusi commented Nov 20, 2014

Uh oh!

benallard Nov 20, 2014

Choose a reason for hiding this comment

Uh oh!

benallard Nov 20, 2014

Choose a reason for hiding this comment

Uh oh!

sa2ajj commented Nov 20, 2014

Uh oh!

djmitche Nov 20, 2014

Choose a reason for hiding this comment

Uh oh!

djmitche commented Nov 20, 2014

Uh oh!

sa2ajj commented Nov 20, 2014

Uh oh!

kassemsandarusi commented Nov 20, 2014

Uh oh!

sa2ajj commented Nov 20, 2014

Uh oh!

benallard commented Nov 20, 2014

Uh oh!

kassemsandarusi commented Nov 20, 2014

Uh oh!

sa2ajj commented Nov 21, 2014

Uh oh!

sa2ajj commented Nov 21, 2014

Uh oh!

sa2ajj commented Nov 21, 2014

Uh oh!

kassemsandarusi commented Nov 21, 2014

Uh oh!

sa2ajj commented Nov 21, 2014

Uh oh!

benallard commented Nov 21, 2014

Uh oh!

kassemsandarusi commented Nov 21, 2014

Uh oh!

benallard commented Nov 21, 2014

Uh oh!

kassemsandarusi commented Nov 21, 2014

Uh oh!

sa2ajj commented Nov 21, 2014

Uh oh!

kassemsandarusi commented Nov 21, 2014

Uh oh!

sa2ajj commented Nov 21, 2014

Uh oh!

sa2ajj commented Nov 23, 2014

Uh oh!

sa2ajj commented Nov 23, 2014

Uh oh!

delanne commented Nov 24, 2014

Uh oh!

benallard commented Nov 24, 2014

Uh oh!

delanne commented Nov 24, 2014

Uh oh!

delanne commented Nov 24, 2014

Uh oh!

benallard commented Nov 24, 2014

Uh oh!

sa2ajj commented Nov 24, 2014

Uh oh!

delanne commented Nov 24, 2014

Uh oh!

djmitche commented Nov 24, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants