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

Fix latent buildslave's _soft_disconnect method #646

Merged
merged 1 commit into from Feb 26, 2013

Conversation

Projects
None yet
3 participants
@seankelly
Member

seankelly commented Feb 26, 2013

This derives entirely from https://gist.github.com/seankelly/5034964,
primarily written by Jc2k.

return AbstractBuildSlave.disconnect(self)
if self.build_wait_timeout < 0:
yield AbstractBuildSlave.disconnect(self)
return

This comment has been minimized.

@djmitche

djmitche Feb 26, 2013

Member

This diff looks like it reverses the meaning. And when is build_wait_timeout less than zero? Isn't the real test whether it equals zero?

@djmitche

djmitche Feb 26, 2013

Member

This diff looks like it reverses the meaning. And when is build_wait_timeout less than zero? Isn't the real test whether it equals zero?

This comment has been minimized.

@seankelly

seankelly Feb 26, 2013

Member

That happens before; it looks like _setBuildWaitTimer. Truthfully, the conditional is a bit bizarre. I'm not sure it should ever trigger. With the 'not' though, it doesn't work properly.

@seankelly

seankelly Feb 26, 2013

Member

That happens before; it looks like _setBuildWaitTimer. Truthfully, the conditional is a bit bizarre. I'm not sure it should ever trigger. With the 'not' though, it doesn't work properly.

This comment has been minimized.

@djmitche

djmitche Feb 26, 2013

Member

Let's find out. Best not to add "magic" code.

@djmitche

djmitche Feb 26, 2013

Member

Let's find out. Best not to add "magic" code.

This comment has been minimized.

@djmitche

djmitche Feb 26, 2013

Member

From the docs: "If it is less than 0 it will never automatically shutdown."

So in this case, a disconnect is just a disconnect, not a shutdown, so the code is correct. Can you add a comment in there to we don't go scratching our heads again?

@djmitche

djmitche Feb 26, 2013

Member

From the docs: "If it is less than 0 it will never automatically shutdown."

So in this case, a disconnect is just a disconnect, not a shutdown, so the code is correct. Can you add a comment in there to we don't go scratching our heads again?

This comment has been minimized.

@Jc2k

Jc2k Feb 26, 2013

Contributor

This is all my doing.

Originally the conditional was "if not timeout", about 3 months or so ago a clumsy merge of some monkey patches I collected in various libvirt deployments should have changed it to "if timeout < 0" - and i utterly failed.

-1 has a special meaning - -1 is when a slave is spawned by a latent slave but not automatically reaped. I used to use this to spin up VM's for people to poke at. In my instance I had a web form that would call stop_instance when a human asked.

@Jc2k

Jc2k Feb 26, 2013

Contributor

This is all my doing.

Originally the conditional was "if not timeout", about 3 months or so ago a clumsy merge of some monkey patches I collected in various libvirt deployments should have changed it to "if timeout < 0" - and i utterly failed.

-1 has a special meaning - -1 is when a slave is spawned by a latent slave but not automatically reaped. I used to use this to spin up VM's for people to poke at. In my instance I had a web form that would call stop_instance when a human asked.

yield defer.DeferredList([
AbstractBuildSlave.disconnect(self),
self.insubstantiate(fast)
])

This comment has been minimized.

@djmitche

djmitche Feb 26, 2013

Member

You probably want fireOnFirstErrback and consumeErrors here, otherwise errors in either of these deferreds will be logged as "unhandled"

@djmitche

djmitche Feb 26, 2013

Member

You probably want fireOnFirstErrback and consumeErrors here, otherwise errors in either of these deferreds will be logged as "unhandled"

This comment has been minimized.

@seankelly

seankelly Feb 26, 2013

Member

Have to ask @Jc2k that.

@seankelly

seankelly Feb 26, 2013

Member

Have to ask @Jc2k that.

This comment has been minimized.

@Jc2k

Jc2k Feb 26, 2013

Contributor

You are probably right - I just left it as I found it.

@Jc2k

Jc2k Feb 26, 2013

Contributor

You are probably right - I just left it as I found it.

@djmitche djmitche merged commit 100a4ce into buildbot:master Feb 26, 2013

mzdaniel pushed a commit to mzdaniel/buildbot that referenced this pull request Aug 2, 2013

Merge pull request buildbot#646 from kirang89/master
- Documentation: Fixed typos in README
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment