search

logicminds / rubyipmi

Command line wrapper for ipmitool and freeipmi
GNU Lesser General Public License v2.1
35 stars 32 forks source link

add timeout to runcmd #17

Closed crhan closed 7 years ago

crhan commented 10 years ago

BMC are unstable and vulnerability, so it is important to add timeout to ipmi call.

  1. default 5 seconds timeout
  2. raise Rubyipmi::IpmiTimeout after @timeout seconds
  3. make the spec all pass
crhan commented 10 years ago

Sorry, I don't have a ruby18 env. And I finally find that Ruby18 cannot support this kind of operation. see here

crhan commented 10 years ago

err... timeout working for sleep call but not working for real ipmitool call. still trying

logicminds commented 10 years ago

I am not sure this is required since the commands that are called should have a timeout built into them. Furthermore I would rather use the timeout argument within the command if any rather than impose a command to complete in the given timeout period.

crhan commented 10 years ago

Ipmitool does not have a configurable timeout options. and one ipmi request over lan will complete in 1 second in average, so that's why I need a timeout implementation in the ruby lib.

And my timeout implementation is very tricky(using open3 block, thread join and process group), but works anyway. Complete this feature for online use is the first thing.

logicminds commented 10 years ago

Wouldn't this timeout functionality put the process in a blocking state while waiting for response?

crhan commented 10 years ago

Sounds like perform this action in a async way? Good point but too hard for me, is there any example to implement that?

crhan commented 10 years ago

This timeout mechanism work so well in my situation. Unless your bmc is 100% perfectly functional, this will help a lot.

logicminds commented 10 years ago

Will take a look, probably a good idea to have a timeout. IPMI and BMC are so chaotic sometimes.

logicminds commented 10 years ago

@crhan any chance you can rebase this against the current master and redo the PR to merge into master?

crhan commented 10 years ago

err. Sorry, I have no time to spent on it right now. And this crhan/rubyipmi@c75d9177 is my running version on production environment, covering over 100,000 machines. And it works fine(just for the ipmitool part) .

logicminds commented 9 years ago

@crhan I am going to update the code to support timeouts using popen as there are other things I need to support. Have you had any issues on your repo with changing to popen?

crhan commented 9 years ago

nope, it works fine for the past year. and the timeout commit is precisely at https://github.com/crhan/rubyipmi/commit/16873514b12d1ed18b1a5bcdcd630205b47bd24c

have a look

logicminds commented 7 years ago

I think a timeout is something that we should add. This pr is old so I am closing for now. But we should be able to add this back in a future request. created #42 to track this.