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 ipmi::default_channel param to fix chicken and egg failure #89

Merged
merged 1 commit into from
Sep 24, 2024

Conversation

jhoblitt
Copy link
Owner

@jhoblitt jhoblitt commented Sep 24, 2024

On a host which does not have ipmitool installed (or working), ipmi.default is undefined and looking up ipmi.default.channel fails. This is a race condition that prevents puppet from being able to install ipmitool to make the fact functional on a subsequent agent run.

Resolves #88

On a host which does not have ipmitool installed (or working),
`ipmi.default` is undefined and looking up `ipmi.default.channel` fails.
This is a race condition that prevents puppet from being able to install
ipmitool to make the fact functional on a subsequent agent run.
@jhoblitt jhoblitt added the bug label Sep 24, 2024
@jhoblitt
Copy link
Owner Author

@nathanlcarlson Could you review this PR?

@nathanlcarlson
Copy link
Contributor

@jhoblitt I think it looks okay. Would it be sensible to make the getting of the default channel into a function that uses ipmitool? That way it can be obtained if needed (i.e. the user didn't provide one) and ipmitool will be present to do so because of the require ipmi::install?

Comment on lines +16 to +19
$_real_lan_channel = $lan_channel ? {
undef => $ipmi::default_channel,
default => $lan_channel,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

From my previous comment, the undef block here would call a function (to be defined in lib/puppet/functions) that gets the first detected LAN channel from ipmitool output.

@nathanlcarlson
Copy link
Contributor

nathanlcarlson commented Sep 24, 2024

I think the current solution in this PR works fine. It just would require 2 puppet runs, with the first having some potentially confusing errors, to apply everything if the LAN channel is actually not channel 1 and the user is relying on the default channel being detected.

@jhoblitt
Copy link
Owner Author

@jhoblitt I think it looks okay. Would it be sensible to make the getting of the default channel into a function that uses ipmitool? That way it can be obtained if needed (i.e. the user didn't provide one) and ipmitool will be present to do so because of the require ipmi::install?

I thought that deferred functions were still evaluated before enforcement but it seems that this changed in 7.17: https://www.puppet.com/docs/puppet/7/release_notes_puppet.html#release_notes_puppet_x-7-17-0 So it sounds like it should be possible to order the catalog such that the channel function runs after the ipmitool package is installed.

@nathanlcarlson
Copy link
Contributor

I think I may be confused, apologies. I was thinking of "Custom Functions", but those run on the Puppet server and are part of catalog compilation. Maybe "Custom Resources" is more what I was thinking (it even lists "Running a command line tool"), but that would definitely change the scope of this problem.

@jhoblitt
Copy link
Owner Author

jhoblitt commented Sep 24, 2024

I think I may be confused, apologies. I was thinking of "Custom Functions", but those run on the Puppet server and are part of catalog compilation. Maybe "Custom Resources" is more what I was thinking (it even lists "Running a command line tool"), but that would definitely change the scope of this problem.

As I mentioned, I think it is workable with deferred functions but it isn't something I've tried before.

Rewriting the defines as custom types is #36 and I don't have the time available to tackle that today.

I think I should go ahead and merge this PR and to get a fixed version on the forge.

Copy link
Contributor

@nathanlcarlson nathanlcarlson left a comment

Choose a reason for hiding this comment

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

Sounds good

@jhoblitt jhoblitt merged commit 5b681e6 into master Sep 24, 2024
20 checks passed
@jhoblitt jhoblitt deleted the bugfix/ipmi.default.channel branch September 24, 2024 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ipmi::network is broken when ipmitool is not installed and the lan_channel param is unset
2 participants