Split agent reply timeout from execution timeout #170

Closed
wants to merge 1 commit into
from

Projects

None yet

5 participants

@ncdc
ncdc commented Mar 7, 2014

If the agent's action times out in the middle of sending a reply, it may
send a partial message over the wire, which can lead to errors in the
messaging broker.

Run the agent's action in its own timeout block, and if it finishes,
yield the replies in a separate timeout block. This way, it should be
less likely that a partial message is sent out over the wire.

@ncdc ncdc Split agent reply timeout from execution timeout
If the agent's action times out in the middle of sending a reply, it may
send a partial message over the wire, which can lead to errors in the
messaging broker.

Run the agent's action in its own timeout block, and if it finishes,
yield the replies in a separate timeout block. This way, it should be
less likely that a partial message is sent out over the wire.
31c335a
@puppetcla

Waiting for CLA signature by @ncdc

@ncdc - We require a Contributor License Agreement (CLA) for people who contribute to Puppet, but we have an easy click-through license with instructions, which is available at https://cla.puppetlabs.com/

Note: if your contribution is trivial and you think it may be exempt from the CLA, please post a short reply to this comment with details. http://docs.puppetlabs.com/community/trivial_patch_exemption.html

@ncdc
ncdc commented Mar 10, 2014

I think this patch is pretty trivial. Please consider accepting it without my signing the CLA. Thanks.

@ploubser ploubser commented on the diff Mar 26, 2014
lib/mcollective/optionparser.rb
@@ -174,6 +174,10 @@ def add_common_options
@options[:publish_timeout] = pt
end
+ @parser.on("--agent_reply_timeout TIMEOUT", Integer, "Timeout for publishing replies from agent actions to remote callers.") do |pt|
@ploubser
ploubser Mar 26, 2014 Contributor

Since this is a change to the server side code adding the value to the optionparser isn't needed. Even if the options are set on the cli they are not passed to the server. See mco rpc rpcutil ping --agent_reply_timeout 0

@ploubser
Contributor

Thanks for the contribution and sorry for taking to long to get back to you. Bandwidth has been a bit limited.

Unfortunately this commit will introduce new functionality so you will have to sign the cla to get this in.

@ncdc
ncdc commented Mar 27, 2014

@ploubser I'm not comfortable signing the CLA as it's currently written. Any chance of striking parts of it?

@ploubser
Contributor

@geekygirldawn is this something we can do?

@geekygirldawn

@ncdc - we can negotiate custom agreements for companies, but we don't normally do them for individuals. Did you want the changes for you as an individual or for Red Hat? Can you email me (dawn at puppetlabs.com) with details about which part you wanted to strike? Depending on the extent of the changes, I might be able to do a one-off regardless of individual vs. company.

@geekygirldawn

@ploubser - I've talked to @ncdc and the change he wanted to the CLA has been requested by so many people that I'm just going to make the change in the CLA app itself. It may take a couple of days for @jeffmccune to update the CLA app with the new version of the agreement, but as soon as we've updated it, we can get @ncdc to sign and you can proceed with this pull request.

@ploubser
Contributor

@geekygirldawn Thanks!

@MikaelSmith
Member

@ncdc did you end up reviewing the CLA? I'm not sure if this change was ever made, and @geekygirldawn isn't around to help me follow up.

Since this adds a new configuration option, I'd like to see an MCO ticket for it. Also, you'll need a rebase.

@ncdc
ncdc commented May 17, 2016

I'm not actively working on MCO any more. It's ok to close.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment