Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Services #1378

Merged
merged 14 commits into from Nov 26, 2014
Merged

Services #1378

merged 14 commits into from Nov 26, 2014

Conversation

tardyp
Copy link
Member

@tardyp tardyp commented Nov 22, 2014

This is an attempt to make custom AsyncMultiService usable inside an advanced master.cfg or a buildbot plugin.

You can start review by looking at the doc in the end of the patch. I hope this explains the idea.

@sa2ajj
Copy link
Contributor

sa2ajj commented Nov 22, 2014

PyLint problems:

************* Module buildbot.config
W: 34, 0: Unused import defer (unused-import)
W: 30, 0: Unused import ReconfigurableServiceMixin (unused-import)
W: 33, 0: Unused import service (unused-import)


.. class:: CustomService

This class is provided for advanced users as an interface to insert their own :py:class:`buildbot.util.service.AsyncMultiService` in a buildbot master.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have a practical use case for this already?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, our android process includes 6 singletons we have issues to reconfigure according to the master life cycle. (mostly txrequests based rest services clients)
The reconfigurableServiceMixin was something that looked a good solution to clean that up, hence this upstream first contribution. The next problem will be to back port that on our buildbot eight frankenstein..

Copy link

Choose a reason for hiding this comment

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

+1

return
self.services = {}
for serviceFactory in config_dict['services']:
if not isinstance(serviceFactory, util_service.CustomServiceFactory):
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should use Interface instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

CustomServiceFactory is equivalent purpose as BuilderFactory. Some might want to override methods, but I dont see too much use for that. So yes, this is really a class pattern, not an interface

@sa2ajj
Copy link
Contributor

sa2ajj commented Nov 22, 2014

LGTM, minus the comments I made.

@tardyp
Copy link
Member Author

tardyp commented Nov 23, 2014

just updated the branch with review fixes

@sa2ajj
Copy link
Contributor

sa2ajj commented Nov 23, 2014

And, I am sorry I did notice that before, I think it's a very good candidate to be mentioned in the release notes.

@tardyp
Copy link
Member Author

tardyp commented Nov 23, 2014

added release note, and simplify the link from cfg-services.rst

@sa2ajj sa2ajj removed the needs work label Nov 23, 2014
@sa2ajj
Copy link
Contributor

sa2ajj commented Nov 23, 2014

Waiting for Travis then, unless you'd like at least one more person to have a look at the changes.

@sa2ajj
Copy link
Contributor

sa2ajj commented Nov 23, 2014

👍

c = BuildmasterConfig = masterConfig()
c['slaves'] = [BuildSlave("local1", "localpw")]
c['protocols'] = {"pb": {"port": "tcp:0:interface=127.0.0.1"}}
""" % self.__class__.__module__))
Copy link
Member

Choose a reason for hiding this comment

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

Nice!

@djmitche
Copy link
Member

I like this! At Mozilla, we could use this to run some of our ancillary master tasks, such as feeding build status updates into rabbitmq. I think that we can emulate support for c['status'] using c['services'], which is a much nicer model.

A few concerns, though:

  • CustomServiceFactory seems bulky (why can't I just instantiate my custom class directly?) and lacks functionality I'd expect from it (ability to add or remove custom services at reconfig)
  • This doesn't appear to allow clustered services. I imagine that many users will want this (to run a service on exactly one master).
  • The factory/instance model is a new way of handling reconfigs, different from that used for changesources or schedulers. Right now, changesources are added or removed based on their name, and then reconfigured (although I notice that they don't do a good job of updating their config based on the new object in that case!). Schedulers, which don't have names, are simply all removed and re-added on every reconfig. It'd be great to introduce a consistent way to handle all three types of services (or at least services and changesouces, since they both have names).

@jaredgrubb
Copy link
Member

Very cool patch :) I dont have any comments beyond anything others have said already.

@tardyp
Copy link
Member Author

tardyp commented Nov 24, 2014

@djmitche

Why can't I just instantiate my custom class directly?

This is the solution I came with to separate instantiation from configuration. It a similar problem as with the steps, which are instanciated at config time, and re-instanciated at runtime. I think the method with overriding new is too magic, and I'd like to avoid that especially to avoid telling people that the instanciated object might not actually be the one that is used at runtime.

clustered services

For now, I think this use case would require a lot more work (db coordination, etc). So I'd like to pass on this one.

unify the reconfig model with changesource and schedulers

That is a good point. I find that my proposal is better in term of separation of concerns (configuration VS instantiation) so I would tend to argue toward switching changesource and schedulers to that model. The design is actually similar to the way locks are implemented, where the API SlaveLock is actually a RealSlaveLock factory. The model would then be:

  • rename CustomService -> ReconfigurableService
  • rename CustomServiceFactory -> ReconfigurableServiceFactory
class RealMailStatus(ReconfigurableService):
      def configureCustomService(self, arg1, arg2)
           # do stuff

class MailStatus(ReconfigurableServiceFactory):
      name = "mail_status"
      real_class = RealMailStatus
      # __init__ comes from CustomServiceFactory, which calls check_args()
      def check_args(self, arg1, arg2)
           # check arg1, and arg2

so the configuration is:

c['services'] = [ MailStatus(foo, bar) ]

Obviously I can implement the base class, and mechanism in this PR, but the real unification will be one service by one service

@djmitche
Copy link
Member

Can you explain more of the benefit? I don't understand why there's an issue with instantiation vs. configuration. And I think there's still the bug where instantiation only happens on startup.

@tardyp
Copy link
Member Author

tardyp commented Nov 24, 2014

What I dont like in the current solution is that a running Scheduler, on reconfig would needs to know where it is configured in the new_config, and then take in its sibling Scheduler's attributes the change of the config, and apply them in its own attributes, while doing side effects needed to take these params in account. (actually I realize schedulers are not reconfigurableMixins, so this is not implemented)

In my design, the factory object contains these configuration and only that, and the service itself does not have to know where it is configured.

And I think there's still the bug where instantiation only happens on startup.

This is something I can fix easily

@djmitche
Copy link
Member

Ah, I see, thanks. You're right, the implementation for schedulers is not good right now!

It seems like you're solving two problems here, then:

  1. easier reconfiguration of services
  2. loading custom services

I'd like the first of those to be more general, so that we can use it for schedulers, changesources, and builders, all of which have this awkardness. With that in place, the second one becomes pretty simple.

__new__ is definitely magic, but since tomprince landed it in BuildStep we haven't had any trouble with BuildStep constructors, lost arguments, etc. So I'd lean in that direction.

The tricky bits are (a) not requiring that the same arguments be handled in the constructor and in the reconfig method and (b) checking arguments during the config parsing, not at reconfig time. Your implementation is explicit about only passing config args to the reconfig method, but if we just prohibit custom classes from implementing a constructor (and allow them to implement checkArgs and reconfigServiceWithConfig methods), that issue could be avoided.

@tardyp
Copy link
Member Author

tardyp commented Nov 24, 2014

So you are asking me to try a proposal with more __new__ like magic, and only one class needed for both instance, and config holder?

@djmitche
Copy link
Member

Yes, and general enough that we can use it for other configured objects, too.

@tardyp
Copy link
Member Author

tardyp commented Nov 24, 2014

Does this code more reflects your views?
tardyp@e639175?diff=split

I still need to find some time to make it work, but I think it should.

@tardyp
Copy link
Member Author

tardyp commented Nov 24, 2014

some more work to make the integration test work. Thanks for your comment, I think this new design is indeed much better.
Still need to finish the unit tests, and update the doc and rn


* `service_name`: configure the unique name which is used to access the service using `self.master.namedServices[service_name]`
* `service_class`: subclass of py:class`buildbot.util.service.CustomService`, that will be instanciated
* `args, kwargs`: parameters used to configure the custom service
Copy link

Choose a reason for hiding this comment

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

Question: do we stick with the pattern "config is not sent during init" but during reconfig? And then how the configuration is sent to the service? How do we describe "send this part of the config" to the reconfigService method?

Copy link
Member Author

Choose a reason for hiding this comment

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

I need to update the doc

Copy link

Choose a reason for hiding this comment

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

ok, because I don't see how this can be done automatically. (calling reconfigure with the right settings)

@tardyp
Copy link
Member Author

tardyp commented Nov 25, 2014

addressed review comments, and finish the doc and tests

@sa2ajj
Copy link
Contributor

sa2ajj commented Nov 26, 2014

@tardyp you also need to rebase the branch: it's currently unmergeable.

Pierre Tardy added 14 commits November 26, 2014 09:08
CustomService is an AsyncMultiService that can be custom to one process.

Custom services are attached to the master, started, and reconfigured asynchronously

Signed-off-by: Pierre Tardy <pierre.tardy@intel.com>
Signed-off-by: Pierre Tardy <pierre.tardy@intel.com>
Need more unittests for the CustomServiceFactory..

Signed-off-by: Pierre Tardy <pierre.tardy@intel.com>
Signed-off-by: Pierre Tardy <pierre.tardy@intel.com>
Signed-off-by: Pierre Tardy <pierre.tardy@intel.com>
Signed-off-by: Pierre Tardy <pierre.tardy@intel.com>
- doc fixing
- definitly move ReconfigurableServiceMixin to buildbot.util.service

Signed-off-by: Pierre Tardy <pierre.tardy@intel.com>
Signed-off-by: Pierre Tardy <pierre.tardy@intel.com>
Signed-off-by: Pierre Tardy <pierre.tardy@intel.com>
untested

Signed-off-by: Pierre Tardy <pierre.tardy@intel.com>
new services, and deleted services works as expected on reconfig

Signed-off-by: Pierre Tardy <pierre.tardy@intel.com>
Signed-off-by: Pierre Tardy <pierre.tardy@intel.com>
Signed-off-by: Pierre Tardy <pierre.tardy@intel.com>
@tardyp
Copy link
Member Author

tardyp commented Nov 26, 2014

rebased

sa2ajj pushed a commit that referenced this pull request Nov 26, 2014
@sa2ajj sa2ajj merged commit 44ff9c6 into buildbot:master Nov 26, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants