Can't rebuild hosts

[holmesb]

Hi,

My experience and looking at the code suggest this provider does not support rebuilding hosts. Only initially building them when they are created. We would like to treat our physical hosts as immutable and rebuild them rather than upgrade. The only place the Build flag is being set to true is in resourceForemanHostCreate.

I think the build flag needs to be exposed as a simple argument that users can set.

I'm a bit confused why the bmc_success flag is exposed as an argument: (Optional) Tracks the partial state of BMC operations on host creation. If these operations fail, the host will be created in Foreman and this boolean will remain false. On the next terraform apply will trigger the host update to pick back up with the BMC operations.

Sounds like this should be handled under-the-hood by the provider, and not something the user needs to worry about.

The managed flag (formerly manage_build) is also a bit confusing. (Optional) Create host only, don't set build status or manage power states

"Don't set build status or manage power states" if false I assume? But if I set this to true, it doesn't actually set build status or reboot, except on host creation. The word "managed" is also just a bit vague.

I propose: removing bmc_success and managed and replace them with build. If build = true, then host gets built\rebuilt on next boot. In other words, host is set to build in Foreman. And if enable_bmc is also true, host gets powered-on\rebooted. I think build & enable_bmc are more intuitive and (obvious what they do) than managed and bmc_success. "Build" is aligned with Forman host terminology, so people who are familiar with Foreman will easily understand what it does.

We don't build VMs using Foreman here, but I assume that starting the VM at creation is handled by the hypervisor provider. VM rebuilds cannot be automated by this provider (when build = true) because we don't know how to reboot them. We don't want to have to maintain all the various hypervisors' reboot API calls in this provider. So I think we just need to trust the user is capable of knowing that if they set build = true, the next time the VM is rebooted, it will get rebuilt and they will have to manage this power-cycling themselves (eg make the api\cli reboot call outside of terraform). This is how I've handled VM reboots using another network-build terraform provider.

Thoughts\comments welcome.

[MrFreezeex]

Hi,

I'm a bit consused why the bmc_success flag is exposed as an argument: (Optional) Tracks the partial state of BMC operations on host creation. If these operations fail, the host will be created in Foreman and this boolean will remain false. On the next terraform apply will trigger the host update to pick back up with the BMC operations.

Sounds like this should be handled under-the-hood by the provider, and not something the user needs to worry about.

If I understand correctly that's already kind of the case, it's supposed to be an internal key and the fact that it is incorectly exposed to user is probably a cleanup to do... Also I am not even sure if someone sets it to false It would work correctly (like ignore rebuilding I guess?).

The managed flag (formerly manage_build) is also a bit confusing. (Optional) Create host only, don't set build status or manage power states

"Don't set build status or manage power states" if false I assume? But if I set this to true, it doesn't actually set build status or reboot, except on host creation. The word "managed" is also just a bit vague.

"managed" is literally a key in the foreman API and also IIRC present as "managed"/"unmanaged" in the web UI. So I am not in favor of removing this in general.

But setting "managed" to true also does a bit more than just set managed to "true" in foreman (like dealing with power state) so perhaps the provider should expose another key for that? The old name for handling power state was manage_build but maybe it should be something else (or just reintroduce it)?

Also yes documentation could always be improved I guess ;).

[MrFreezeex]

Actually I checked the code again and managed in fact also set "build" to true. So IMO the correct thing to do would be to have a "build" and a "managed" key as this would directly reflect the foreman terminology... WDYT?

EDIT: Oops the "build" is in fact reflected by the "method" key so this suggestion is not really correct (at least if we don't change the "method" key)... This is probably not clear at all in the documentation now though...

[holmesb]

"managed is literally a key in the foreman API" - ah ok, didn't make that association when I wrote the above, so agreed it's still needed.

I agree with your crossed-out sentence: have build and managed arguments. Method is redundant IMO. If you're building a physical, you can enable_bmc. If you're building a virtual, you set enable_bmc to false. So my proposed changes:

1. Remove bmc_success argument. I see you're already a step ahead of me ;-)
2. Remove method argument.
3. Add build argument - does the same as in Foreman webui

If build, managed, and enable_bmc, then perform power action. Power action occurs at creation and update.

Obvs these are breaking changes for people who currently set bmc_success and method. I think rebuild functionality is a major improvement for this provider, and the process of building is more obvious now. Worth a little user pain IMO. YMMV. I have added deprecation messages (PR incoming), and expect this should be the 0.5.0 release with the breaking changes highlighted in release notes:

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 removed. This functionality is provided by enable_bmc.
• Hosts will now only be rebooted\started and built if all three of the following arguments are enabled: managed, enable_bmc, and build.
• Hosts will now also be built\rebuilt during update, not just at creation.

[MrFreezeex]

The breaking change to bmc_success is IMO not really important as the feature doesn't seems to work anyway, so my PR should be sufficient I think (and it's also highly unlikely that anyone tried to set that in their terraform code).

For the method change, It's supposedly to choose between image and build, but after seeing the code the image value doesn't seems to do anything apart from being different than build which mean it essentially acts as a boolean... So for the method argument, I would propose to deprecate it and add the build argument as you proposed but with a DefaultFunc that would be method == "build" so we don't break users that had their method set to image.

Then not sure what you want to do with power actions, the current behavior is to power on when managed is true and when enable_bmc is true to power off/pxe boot/power on. Right now I don't have a specific opinion on what should be the behavior depending on the boolean sets...

Keep in mind that most argument have ForceNew set to true meaning that the host will be destroyed and recreated, so if you want to do something on host update regarding power actions you have to check for changes among the arguments that doesn't have ForceNew set to true (or change ForceNew on some arguments).

[holmesb]

"don't break users that had their method set to image" - ok, won't actually remove method, just make it do nothing (as = image already does), and advise (in docs) that users should remove it (since = build also does nothing now). Will actually be removed in some future release. For the moment I've left this line commented:

"method": &schema.Schema{
// Removed:  "The argument is handled by enable_bmc instead",`

So my plan was to make build a boolean. I can see the value in your logic: if make build either bmc or image instead, then can remove the enable_bmc argument. But I think feature parity with Foreman's build is more important. Foreman users expect build to be simply on or off as it is in the webui. A boolean involves less surprise and is easier to understand.

[MrFreezeex]

"don't break users that had their method set to image" - ok, won't actually remove method, just make it do nothing (as = image already does), and advise (in docs) that users should remove it (since = build also does nothing now). Will actually be removed in some future release. For the moment I've left this line commented:

"method": &schema.Schema{
// Removed:  "The argument is handled by enable_bmc instead",`

So my plan was to make build a boolean. I can see the value in your logic: if make build either bmc or image instead, then can remove the enable_bmc argument. But I think feature parity with Foreman's build is more important. Foreman users expect build to be simply on or off as it is in the webui. A boolean involves less surprise and is easier to understand.

Yes that's also what I meant, I was suggesting to add a DefaultFunc instead of Default to the build boolean to have a default based on the value of method (and then never use method again in the code), but apparently this doesn't seems possible :/. If you have an idea to smoothly deprecate the method argument that would probably be nice (also if you deprecate the argument you can set Deprecated instead of Removed)

[holmesb]

Ok PR is in. Hope you don't mind, but I've taken the liberty of incorporating your bmc_success removal PR as it makes sense that this is removed at the same time as the new build argument is introduced. I've given credit in the PR.

[MrFreezeex]

Ok PR is in. Hope you don't mind, but I've taken the liberty of incorporating your bmc_success removal PR as it makes sense that this is removed at the same time as the new build argument is introduced. I've given credit in the PR.

You should just include my commit then.

[holmesb]

You should just include my commit then.

I've cherry picked it, so it's in the commit log now

[agriffit79]

Sorry, late to the party on this. I appreciate the Foreman API supports a rebuild option, but is that really in the spirit of Terraform which aspires to be idempotent? Would it not be better to use taint to rebuild a host?

[MrFreezeex]

Sorry, late to the party on this. I appreciate the Foreman API supports a rebuild option, but is that really in the spirit of Terraform which aspires to be idempotent? Would it not be better to use taint to rebuild a host?

That's also why I am quite hesitant on rebuilding the host on any change (see https://github.com/HanseMerkur/terraform-provider-foreman/pull/64#issuecomment-1122355138 and the next comment). Other than that that's pretty much the existing behavior but with better names.

[holmesb]

Happy the new build arg is enough to close this issue. Rebooting can be handled outside of this provider