Add host rebuild feature

[holmesb]

via new build argument, which is aligned with host's build flag in Foreman webui. Method and bmc_success are deprecated (functionality is removed), pending removal of these arguments. Thanks to @MrFreezeex for contributing bmc_success removal code. Fixes issue #60

[MrFreezeex]

I think it would be nice to deprecate the method argument and not remove as it was present in the provider for a long time (and much more likely to be used than bmc_success)... Defaulting build to true and force it to false if method equals to image should be sufficient.

[holmesb]

I think it would be nice to deprecate the method argument and not remove as it was present in the provider for a long time (and much more likely to be used than bmc_success)... Defaulting build to true and force it to false if method equals to image should be sufficient.

Not sure if you wrote this before or after my issue comment. Method is marked as deprecated, and it is still in the schema, so will not error for people who define it. But its = build functionality is removed. = interface never did anything anyway. Method is redundant because its functionality is handled by build and enable_bmc. These are more aligned with the Foreman webui, and are more obvious what they do.

Are you saying during the deprecation period, you would like to keep the "if method = build, set build to true" logic? But what about if method = build, but build = false, which wins? Likewise, when "method = interface, but build = true"? It's not clear to the user what will happen, so simplest\cleaner to remove the = build functionality but keep it in the schema, as I've done.

[MrFreezeex]

Not sure if you wrote this before or after my issue comment. Method is marked as deprecated, and it is still in the schema, so will not error for people who define it. But its = build functionality is removed. = interface never did anything anyway. Method is redundant because its functionality is handled by build and enable_bmc. These are more aligned with the Foreman webui, and are more obvious what they do.

Are you saying during the deprecation period, you would like to keep the "if method = build, set build to true" logic? But what about if method = build, but build = false, which wins? Likewise, when "method = interface, but build = true"? It's not clear to the user what will happen, so simplest\cleaner to remove the = build functionality but keep it in the schema, as I've done.

Hmm you are right and thinking about it, it's true that it was only in the last release that method started to influence something. Like you did should be fine then :+1:. Although, I am not sure if Removed errors when you try to use the argument? If it just does a warning it should be fine to put Removed back on method IMO.

Also BTW you should sync back the documentation on the resource schema (which should be generated with the ./cmd/autodoc in case you missed it). And in case you missed it there was this comment https://github.com/HanseMerkur/terraform-provider-foreman/pull/64#discussion_r867834553 that you marked as resolved:

Since that's `enable_bmc` which now does all the power states, you should also add an error case if `enable_bmc` is `true` while `managed` or `build` is `false` (and maybe even add it to documentation, but an error could be sufficient).

[holmesb]

not sure if Removed errors

Now if user includes method = * it doesn't error, but if I uncomment Removed, it will: Error: "method": [REMOVED] The argument is handled by enable_bmc instead.

But (didn't know this until now), can change Removed to Deprecated, and it will warn: Warning: "method": [DEPRECATED] The argument is handled by enable_bmc instead I will add a Deprecated warning for method.

I added coverage for enable_bmc. Pls specify what further you're looking for.

I'll get my head around autodoc and update the PR

[MrFreezeex]

not sure if Removed errors

Now if user includes method = * it doesn't error, but if I uncomment Removed, it will: Error: "method": [REMOVED] The argument is handled by enable_bmc instead.

But (didn't know this until now), can change Removed to Deprecated, and it will warn: Warning: "method": [DEPRECATED] The argument is handled by enable_bmc instead I will add a Deprecated warning for method.

Thanks, Deprecated sounds perfect then!

I added coverage for enable_bmc. Pls specify what further you're looking for.

Like right now enable_bmc only work if both managed and build are true. So if you set enable_bmc to true it should produce an error if either managed or build are false, or at least put this clearly in the documentation. That would be easier for user to debug/know this behavior (~in general silently ignoring an option that the user pass sounds pretty terrible UX to me).

[holmesb]

Like right now enable_bmc only work if both managed and build are true. So if you set enable_bmc to true it should produce an error

I disagree, enable_bmc just exists to tell Terraform that host is physical and has a BMC. Not whether you want to manage and build\rebuild it. Power action depends on these factors too. Enable_bmc while not managed\build is not an error. We need to be sure that you want to blow your host away!! :

1. Is this something we can even boot\reboot? Because if its a VM, we can't using this provider. This is the question enable_bmc answers.
2. Do you want to manage this host using Foreman? This is the question managed answers.
3. Do you want to build\rebuild? This is what build answers.

Only if the answer to all three questions is yes\true do we take power action, and host actually gets booted\rebooted\built\rebuilt. Maybe should change enable_bmc to host_type, which takes either physical or virtual, but this would be another breaking change.

[MrFreezeex]

Like right now enable_bmc only work if both managed and build are true. So if you set enable_bmc to true it should produce an error

I disagree, enable_bmc just exists to tell Terraform that host is physical and has a BMC. Not whether you want to manage and build\rebuild it. Power action depends on these factors too. Enable_bmc while not managed\build is not an error. We need to be sure that you want to blow your host away!! :

1. Is this something we can even boot\reboot? Because if its a VM, we can't using this provider. This is the question enable_bmc answers.
2. Do you want to manage this host using Foreman? This is the question managed answers.
3. Do you want to build\rebuild? This is what build answers.

Only if the answer to all three questions is yes\true do we take power action, and host actually gets booted\rebooted\built\rebuilt. Maybe should change enable_bmc to host_type, which takes either physical or virtual, but this would be another breaking change.

Even if you put it in the documentation, a user quickly looking at the doc or copy pasting existing code that miss this should be reminded somehow that if he pass explictely enable_bmc to true and he sets managed or build to false that it doesn't work with at the very least a warning or an error...

Also Enable_bmc while not managed\build is not an error. We need to be sure that you want to blow your host away!! this is not at all a justification to not produce an error... If you return an error, the worst that can happen is terraform quitting early and not doing anything... It's quite the contrary: producing an error is safer and allows to not do something that the user don't know about (in this case silently ignoring an option that he explicitly want).

[holmesb]

Even if you put it in the documentation, a user quickly looking at the doc or copy pasting existing code that miss this should be reminded somehow that if he pass explictely enable_bmc to true and he sets managed or build to false that it doesn't work with at the very least a warning or an error...

I think we have different ideas about what enable_bmc should be doing. Until this PR, it power-actioned regardless of manage\build. But Foreman is a platform for building, not rebooting. Foreman is mainly intended to power-action only when the user wants to build. This PR currently disables the ability to power-action other than when you are building.

I think power-action without the intention of building is pretty niche, and not really the purpose of foreman\this provider, but I accept the UI supports this: This dialogue is available regardless of managed\build. I don't really like the name enable_bmc in this context, because it sounds like you're telling Terraform that this is a BMC host rather than "immediately reboot", but I'm happy to reinstate this immediate power-action behaviour (regardless of manage\build) to get this PR over the line. We'll always set the trio of managed\build\enable_bmc to the same true\false variable regardless, so end result is the same for us. I'll change this tomorrow.

Longer term, I think we should deprecate enable_bmc and come up with a better name. Maybe bmc_power_action is more obvious.

[MrFreezeex]

Even if you put it in the documentation, a user quickly looking at the doc or copy pasting existing code that miss this should be reminded somehow that if he pass explictely enable_bmc to true and he sets managed or build to false that it doesn't work with at the very least a warning or an error...

I think we have different ideas about what enable_bmc should be doing. Until this PR, it power-actioned regardless of manage\build. But Foreman is a platform for building, not rebooting. Foreman is mainly intended to power-action only when the user wants to build. This PR currently disables the ability to power-action other than when you are building.

I think power-action without the intention of building is pretty niche, and not really the purpose of foreman\this provider, but I accept the UI supports this: This dialogue is available regardless of managed\build. I don't really like the name enable_bmc in this context, because it sounds like you're telling Terraform that this is a BMC host rather than "immediately reboot", but I'm happy to reinstate this immediate power-action behaviour (regardless of manage\build) to get this PR over the line. We'll always set the trio of managed\build\enable_bmc to the same true\false variable regardless, so end result is the same for us. I'll change this tomorrow.

Are we really speaking of the same thing? Like I know you took this approach and I don't have anything to say about that. I'm just saying that if you choose to ignore an argument that the user explictely specified, you should add an error/warning. That would be something like that:

if enablebmc  && (!managed || !build) {
  // Warn/error as enablebmc is ignored here
}

If you now choose to reintroduce the previous behavior that would be something different of course... But in the current form of your code, the enable_bmc argument will just be ignored and the user will not be able to notice anything if, for instance, he provides the following code:

resource "foreman_host" "example" {
  name = "compute01.dc1.company.com"
  enable_bmc = true
  managed = false
  ...
}

That's the sort of thing when the user might suspect a problem in bmc, foreman and then finally read the doc of this provider or its code and realize the issue after a few hours of debugging. While this could have been prevented by a warning or an error telling the user that he should simply set managed to true if he doesn't want that enable_bmc is ignored... I don't see how this change anything in term of behavior? This is only a way to give a feedback to the user...

Longer term, I think we should deprecate enable_bmc and come up with a better name. Maybe bmc_power_action is more obvious.

Agreed

[holmesb]

Ok, so can either: A. Keep the three conditions: enable_bmc, managed, & build for rebooting in this PR. B. Switch back to only enable_bmc.

You are suggesting A with a warning\info message. I do not want to error, because IMO the fact host doesn't reboot isn't a problem. It just means you've created the host, but aren't ready to reboot & rebuild it. Erroring prevents further resources from being created.

Problem is: we're using plugin SDK v1.x and the latest Terraform fails to print any debug logs. I've set TF_LOGS=WARN, but the test warnings I've created aren't showing. Logs are just full of "we are tolerating it because it is using the legacy plugin SDK". Neither "log.Printf()", nor "log.Warn()" print anything to screen. My Terraform is latest (v1.1.9). Any ideas? Otherwise we're either looking at:

1. Make SDK v2.x upgrade a prereq for this PR
2. Don't print anything to screen when enable_bmc, but not manage\build.
3. Plan B above.

Personally, I don't think the plan to add a warning provides much value, because average user doesn't know to add TF_LOG env var, so #2 is my preference. Not sure I have time to do #1.

[MrFreezeex]

Ok, so can either: A. Keep the three conditions: enable_bmc, managed, & build for rebooting in this PR. B. Switch back to only enable_bmc.

You are suggesting 1 with a warning\info message. I do not want to error, because IMO the fact host doesn't reboot isn't a problem. It just means you've created the host, but aren't ready to reboot & rebuild it. Erroring means preventing further resources from being created.

Look this has absolutely nothing to do with Terraform or Foreman or that the host doesn't reboot, you could make this assumption for any given program... You chose to ignore an option in certain case and have good enough reasons to do so, that's fine to me. But then you need to tell it to the user, that's an UX problem and not anything else... It doesn't matter if yes we could technically do something about it, that's absolutely not the discussion here. Also the error message could very well be something around "This provider doesn't support this combination of option [...]" instead. Again that's not an error from Foreman, that's an error from the provider that chose to not support this combination of option.

Problem is: we're using plugin SDK v1.x and the latest Terraform fails to print any debug logs. I've set TF_LOGS=WARN, but the test warnings I've created aren't showing. Logs are just full of "we are tolerating it because it is using the legacy plugin SDK". Neither "log.Printf()", nor "log.Warn()" print anything to screen. My Terraform is latest (v1.1.9). Any ideas? Otherwise we're either looking at:

1. Make SDK v2.x upgrade a prereq for this PR

2. Don't print anything to screen when enable_bmc, but not manage\build.

3. Plan B above.

Personally, I don't think the plan to add a warning provides much value, because average user doesn't know to add TF_LOG env var, so #2 is my preference. Not sure I have time to do #1.

Well at lest logging in debuging mode would be a start, but yes that's not ideal... To have warning that needs to switch to {Create,Update}Context on sdkv2 that does return a diag instead of an error where you can specify warning. So yes that would probably need more work...

Again I don't have a specific opinion if you should restore the previous behavior or not... I just don't want to waste the time of an admin trying the provider and being mad for 4 hours while he enabled enable_bmc and wonders why it doesn't do anything... Returning an error or have a warning of some kind would allow him to understand right-away...

[holmesb]

Ok prior power behaviour restored: only condition is enable_bmc. I'll create an issue for sdkv2 upgrade to fix logging.

[MrFreezeex]

Ok prior power behaviour restored: only condition is enable_bmc. I'll create an issue for sdkv2 upgrade to fix logging.

Ok nice then it doesn't need an error or warning :). That probably will not improve anything for the logging though, I always set TF_LOGS to DEBUG to check those kind of message anyway, not sure of what the other level do.

[holmesb]

TF_LOGS=DEBUG doesn't display this provider's log messages for me either. Eg no "Created ForemanHost" message when a host is created.

[MrFreezeex]

TF_LOGS=DEBUG doesn't display this provider's log messages for me either. Eg no "Created ForemanHost" message when a host is created.

Oh wait ok I remember this provider has another parameter for that, it's in the README: export FOREMAN_PROVIDER_LOGLEVEL="DEBUG".

[holmesb]

Ok can see logs now, thanks. Who needs to approve @lhw @agriffit79? Since we have reverted to pre-PR power behaviour, the breaking changes are reduced to:

Breaking Changes:

• bmc_success argument removed. Power action is no longer taken retrospectively if prior execution fails. This change gives the user sole control over how power-states are managed.
• method argument deprecated. This functionality is provided by enable_bmc.
• Hosts will now also be built\rebuilt during update, not just at creation.

[MrFreezeex]

• Hosts will now also be built\rebuilt during update, not just at creation.

You also still have to define the list of change of parameters which triggers a rebuild on al update... https://github.com/HanseMerkur/terraform-provider-foreman/pull/64#discussion_r867836528

[holmesb]

• Hosts will now also be built\rebuilt during update, not just at creation.

You also still have to define the list of change of parameters which triggers a rebuild on al update... #64 (comment)

I ask again: for what benefit?

[MrFreezeex]

• Hosts will now also be built\rebuilt during update, not just at creation.

You also still have to define the list of change of parameters which triggers a rebuild on al update... #64 (comment)

I ask again: for what benefit?

I answer again by asking you a yes or no question: Do you think that updating comment should rebuild the host?

[holmesb]

Sorry for the confusion, for some reason my comments are "Pending" and only I can see:

[MrFreezeex]

Sorry for the confusion, for some reason my comments are "Pending" and only I can see:

Yes that's what I was referring to: do you think that updating comment while enable_bmc is set to true should rebuild the host? Apparently yes?

[holmesb]

I don't know why my comments above got stuck Pending for the past 24 hours, maybe some GitHub "feature" that responds to a new commit.

But I've just checked again, and changing a host's comment does not trigger host restart when enable_bmc is false (default). When enable_bmc is true, a reboot will always be triggered. This is expected to allow rebuilds.

[MrFreezeex]

When enable_bmc is true, a restart will always be triggered. This is expected to allow rebuilds.

This doesn't make much sense to me (even more if you just want to update comment or similar-ish fields), but perhaps @lhw will have another opinion on the subject.

Besides this and the small nits that I posted a few minutes ago the PR looks good to me 👍.

[holmesb]

if you just want to update comment or similar-ish fields

Generally you'd set enable_bmc on the command line (terraform apply -var=enable_bmc=true), you wouldn't have "true" in source for this very reason.

[lhw]

I'm currently on vacation but noticed my inbox counter increasing. Generally I think that the concept of a "rebuild" breaks with the idea of terraform which is more of a one and done approach to infrastructure. But I do get where this is coming from. I will check on this when I get back to office.

[holmesb]

Do you think it's worth breaking this PR down into bitesize chunks?

1. bmc_success removal
2. build add
3. method removal and enable_bmc=true triggers power-action on update

I expect 1 & 2 are simple and uncontroversial and bring the majority of the rebuild benefits. Currently it's not possible to simply configure host's build flag in this provider except via abstract arguments at creation. Can't set it at all during update.

It might be that 3 never gets merged because it's too divisive. In which case, rebooting can be handled "out-of-band" of Foreman and this provider.

[MrFreezeex]

IMO method and build should be done together (since method value is "build"), that could go in 2 without touching too much the rebuilding/power action behavior.

[holmesb]

IMO method and build should be done together (since method value is "build"), that could go in 2 without touching too much the rebuilding/power action behavior.

Sounds good @MrFreezeex. Yes method is redundant when build exists. Would you mind reopening your bmc_success removal PR since it already fulfils item 1?