Skip to content
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

add timeout to runcmd #17

Closed
wants to merge 37 commits into from
Closed

add timeout to runcmd #17

wants to merge 37 commits into from

Conversation

crhan
Copy link
Contributor

@crhan crhan commented Nov 5, 2013

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

logicminds and others added 30 commits July 1, 2013 23:01
Bugfix: bmc.info has different results with different models.
-- added vagrant file to automatically install freeipmi and test vm
   to enable set the bash environment variable: rubyipmi_debug=true
 -- fixed a bug where the command was returning the value and not the result
 -- fixed a bug where bmc-config was not raising an error when errors occured
logicminds and others added 7 commits October 14, 2013 12:26
1. ignore Gemfile.lock for multi ruby CI support
2. add ruby20 to travisCI
3. use 'bundler/gem_tasks' instead of jeweller
4. use rcov under ruby19
5. use simplecov in ruby19 & ruby20
1. default 5 seconds timeout
2. raise IpmiTimeout after @timeout seconds
3. make the spec all pass
remove a ruby19 new syntax
@crhan
Copy link
Contributor Author

crhan commented Nov 5, 2013

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

@crhan
Copy link
Contributor Author

crhan commented Nov 8, 2013

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

@logicminds
Copy link
Owner

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
Copy link
Contributor Author

crhan commented Dec 27, 2013

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
Copy link
Owner

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

@crhan
Copy link
Contributor Author

crhan commented Dec 27, 2013

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

@crhan
Copy link
Contributor Author

crhan commented Feb 1, 2014

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

@logicminds
Copy link
Owner

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

@logicminds
Copy link
Owner

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

@crhan
Copy link
Contributor Author

crhan commented Jun 5, 2014

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
Copy link
Owner

@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
Copy link
Contributor Author

crhan commented Jan 11, 2015

nope, it works fine for the past year. and the timeout commit is precisely at crhan@1687351

have a look

@logicminds
Copy link
Owner

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.

@logicminds logicminds closed this Apr 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants