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

Consolr updates #412

Merged
merged 11 commits into from
Feb 19, 2016
Merged

Consolr updates #412

merged 11 commits into from
Feb 19, 2016

Conversation

defect
Copy link
Contributor

@defect defect commented Feb 15, 2016

In order to support more tools than ipmitool in consolr, I'd like to modularize it a bit. As you can see here, I've moved all the ipmitool-related code in to a class called Consolr::Runners::Ipmitool. The command loads one or more runners dynamically from the config, and chooses the first one that reports it can run on a given node (meaning the runner's can_run? method returns true).

An example use case could be an environment with mixed Openstack VMs and bare metal servers. An openstack runner would then handle all the VM assets and the ipmitool runner all the bare metal assets. This way, you can easily customize consolr to new tools by writing a Consolr::Runners::Yourtool class and then installing it in whichever way you prefer.

In order to maintain backwards compatibility I've made the ipmitool runner the default if no runners are specified in the config.

Finally, I'm adding an option (-S, or --status) which just returns the power status of the assets because it seemed like something that would be useful :)

@tumblr/collins

@byxorna
Copy link
Contributor

byxorna commented Feb 15, 2016

LGTM


def verify node
Net::Ping::External.new(node.ipmi.address).ping?
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[picky] whitespace

@roymarantz
Copy link
Contributor

IMO there should be some way to force using a particular runner.
Do you want to add missing ipmitool functions in the PR (e.g. soft reboot vs hard reboot) or is that too much out of scope.
I like the approach though.

@gtorre
Copy link
Contributor

gtorre commented Feb 17, 2016

Very nice. 👍

@defect
Copy link
Contributor Author

defect commented Feb 17, 2016

Added support for --soft-reboot and --soft-off. I also added a --runner option to specify a specific runner to use.

@william-richard
Copy link
Contributor

This LGTM
But, I think we should add a barebones example gem that would show people how to add their own, internal runners with custom logic.

@william-richard
Copy link
Contributor

Can you also add unit tests before merging?

defect added a commit that referenced this pull request Feb 19, 2016
@defect defect merged commit 8d2c1ef into master Feb 19, 2016
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.

5 participants