-
Notifications
You must be signed in to change notification settings - Fork 69
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
Fixes #9810 - IPMI facter extensions multichannel support #7
Conversation
@ares can you take a look please? |
end if output | ||
|
||
if attributes.keys.empty? | ||
Facter.debug("Running ipmitool didn't give any information") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe update the message so we know for what channel we have no info
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, actually I've changed it and only report when the first channel was not yet found.
I didn't tested but except small nits it looks good to me. I'm not IPMI expert so I don't know whether it's reasonable to consider IPMI with low channels as default but I suppose it does not do any harm. |
Thanks I will fix these and release 2.2. |
1149051
to
44a1d46
Compare
Ok refactored a bit. Tested:
Ignore the two errors, not related (testing it on non-discovery machine). @orrabin please merge at will. |
I have a vague memory that we used these facts somewhere, maybe it was staypuft, @ares maybe remembers? |
yes, these ipmi facts are used in staypuft and foreman has builtin support for them so similarly as it creates other interfaces, it can create bmc interface based on it |
so does it mean we need another patch for foreman?
|
I don't think so, my understanding is that ipmi_* are always set to first channel we found, ipmi_n_* are just additional if someone would need to read info from more channels. AFAIK Foreman currently support only one bmc interface per host. |
@ohadlevy to give you some more context - this was created for Staypuft team, there is a downstream BZ link. They read it in their codebase. The only issue is that some HP boxes have IPMI on channel 2 instead of 1. This patch fixes it and reports correctly in |
@stbenjam or @orrabin any chance you can take a look? I'd like to merge this and backport this downstream. Important patch, to test this you either need to boot image on hardware with BMC on 2nd channel, or to copy this file and use facter on a different (non-image) machine. I can provide you a hardware box if needed. |
To test this: build and run discovery on IPMI enabled hardware. If you don't have any, I can provide you a shell on such a box and testing can be done manually running facter with this extension. I'd like to have this in the new fdi release. Thanks! |
@lzap Yea, I'll need a hardware box with IPMI to test this, if you can give me access to one. |
FWIW, I have a setup with several HP boxes that report IPMI on channel 2 and this patch has worked fine with them. It also doesn't seem to break anything on other machines that have a more traditional config with IPMI on channel 1. |
Have you extended the beaker machine I reserved to you? @stbenjam ? :-) |
I know nothing about this... what beaker machine? |
Hmm sent you an email with reservation. Later, |
I only have the e-mail from a few minutes ago, but anyway I'll take a look when I have time |
This patch now fixes ipmi_facts and also add ipmi_N_facts (where N is channel
number) for those who would like to get information from multiple channels for
any reason.
Example run of facter with this patch on IBM server with IPMI on channel 1:
The same on HP box with IPMI on channel 2:
@MHulan can you take a look?