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
Services #1378
Conversation
PyLint problems:
|
|
||
.. 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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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..
There was a problem hiding this comment.
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): |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
LGTM, minus the comments I made. |
just updated the branch with review fixes |
And, I am sorry I did notice that before, I think it's a very good candidate to be mentioned in the release notes. |
added release note, and simplify the link from cfg-services.rst |
Waiting for Travis then, unless you'd like at least one more person to have a look at the changes. |
👍 |
c = BuildmasterConfig = masterConfig() | ||
c['slaves'] = [BuildSlave("local1", "localpw")] | ||
c['protocols'] = {"pb": {"port": "tcp:0:interface=127.0.0.1"}} | ||
""" % self.__class__.__module__)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
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 A few concerns, though:
|
Very cool patch :) I dont have any comments beyond anything others have said already. |
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.
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.
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:
so the configuration is:
Obviously I can implement the base class, and mechanism in this PR, but the real unification will be one service by one service |
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. |
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.
This is something I can fix easily |
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:
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.
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 |
So you are asking me to try a proposal with more |
Yes, and general enough that we can use it for other configured objects, too. |
Does this code more reflects your views? I still need to find some time to make it work, but I think it should. |
some more work to make the integration test work. Thanks for your comment, I think this new design is indeed much better. |
|
||
* `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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
addressed review comments, and finish the doc and tests |
@tardyp you also need to rebase the branch: it's currently unmergeable. |
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>
rebased |
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.