Remove timeout on service disable.#317
Conversation
Currently in certain contexts (disabling the last central node), the timeout of 30 seconds is not enough. I cannot personally see why this needs a timeout as its not expected to hang in anyway and if its hanging that's a problem that wont be solved via timing out. Fixes: lp-2143333 Signed-off-by: MJ Ponsonby <[email protected]>
|
Just my two cents here
Having a client side command that never times out could lead to even more frustrating scenarios. Especially if the command is part of an automation, like the microovn charm, hooks (event handlers?) could get stuck silently waiting forever for the command to finish. Letting the command eventually time out gives you an explicit error. This change presumably helped you in the scenario described in lp-2143333, so it's just a matter of finding the right value for the timeout, right? |
|
oh hello Martin! hope you've been doing well lately. I did consider just increasing it but then I couldn't really justify the timeout in any form. As mentioned in the commit, if the disable service function is hanging that's a big problem and terminating half way through would make it worse in most cases |
|
Hi MJ, I'm doing alright, thank you :) Yeah, I guess it comes down to "what's worse". Either terminating in the middle of potentially still running "downscale" action, or risking forever hanging commands. Btw, did you find out what the root cause of the unusually long "disable" command was? I tried to reproduce it locally, and on a fresh cluster, disabling the last central node never took more than a second. Perhaps the charm CI machine is just very busy. |
2 participants
Currently in certain contexts (disabling the last central node), the timeout of 30 seconds is not enough. I cannot personally see why this needs a timeout as its not expected to hang in anyway and if its hanging that's a problem that wont be solved via timing out.
Fixes: lp-2143333