Fixes #36393 - Do not rebuild managed hosts in registration

[stejskalleos]

No description provided.

[theforeman-bot]

Issues: #36393

[stejskalleos]

@nofaralfasi review please

[ekohl]

I'm tempted to say that a host should by default be created as an unmanaged host from the registration workflow, in which case build mode doesn't matter.

[stejskalleos]

I'm tempted to say that a host should by default be created as an unmanaged host from the registration workflow

Usually they are, but when you use registration for provisioned hosts, then they are managed.

[ekohl]

I'm not quite sure I follow. In my mind how it works is that before you register a host then there is no Host DB entry. After registration there's an unmanaged Host DB entry. If the user want to provision it, they have to later make it a managed host.

[stejskalleos]

I'm not quite sure I follow. In my mind how it works is that before you register a host then there is no Host DB entry.

Not in this case, because the host record is already there, from the provisioning.
Users first provision the host, then use global registration for the subscription configuration and other stuff happening in host_init_config template.

Or in different case it doesn't have to be right after the provisioning, but later when they want to re-register the host to a different capsule. With the current state, it will also re-run all assigned ansible roles, which is not desired behavior.

See the BZ comment for more info.

[ekohl]

I never thought of using registration to alter an existing host. To me registration implies something is always new. A good example of us needing to capture the scope of a feature.

[ares]

I'd be mostly in favor of what @evgeni suggests. I think that the initial configuration is important in the regular (new host, unknown to Foreman) registration flow and I believe that's the majority of registration calls. But I can see the need of re-registering existing hosts where the post provisioning steps may not be desired.

IIRC setting the build mode to true had two good outcomes. One is, host build status is created and one can observe the host record in Foreman and could easily tell whether it's still being built (registered), it's switched to built only as the last step. Second, thanks to that, the build data can be saved, this would contain information about errors that appeared during the registration.

So I'd no set the build always to false, instead I'd agree the second part of the proposal (get the PR in as is, add the optional flag as a separate PR).

[ekohl]

IIRC setting the build mode to true had two good outcomes. One is, host build status is created and one can observe the host record in Foreman and could easily tell whether it's still being built (registered), it's switched to built only as the last step. Second, thanks to that, the build data can be saved, this would contain information about errors that appeared during the registration.

This does give a risk of data loss. If the system is in build mode and reboots, it may end up reprovisioning which can destroy data on the systems. It feels like we're abusing one mechanism for another.

[stejskalleos]

@ares @ekohl so which path of fix are we going to choose?
I'm for merging it as it is right now and later adding a new optional flag for host-rebuild.

[ares]

My understanding is, the current behavior is, we always set the build=true (#setBuild). So while the concern about the data loss is valid, it's applicable to current version. This PR disables this for managed hosts, in other words, in the use case of using GR for existing hosts. That's why I'm in favor of merging as is to fix the reported issue for re-registration and we can continue debating whether we should also stop setting the build to true for newly registered hosts in a separate PR.

So I'm in favor of merging as is, as long as my understanding of the proposed change is correct.

[stejskalleos]

I still fail to see why the controller can't default to build=false and only call setBuild if explicitly asked for.

@ekohl We use it for tracking how long it took to register the host (and also for tracking the state of registration)

So I'm in favor of merging as is, as long as my understanding of the proposed change is correct.

I agree, it's for the upcoming downstream release, we should fix the issue ASAP and then debate over the usage of build during the registration

[stejskalleos]

Failing tests are not related, they are failing everywhere.
I'll take a look where's the problem

[stejskalleos]

@ares @ekohl @evgeni any other comments? It's for upcoming downstream release, would be nice to get it in ASAP

[ekohl]

I'll also note that I haven't really looked at managed vs unmanaged hosts code before. There's more logic it doesn't take into account. For example, there's code that says it can't be in build mode if it's an image if I read this right:

foreman/app/models/host/managed.rb

Lines 507 to 509 in b657d26

	def can_be_built?
	managed? && !image_build? && !build?
	end

I still fail to see why the controller can't default to build=false and only call setBuild if explicitly asked for. Or not even support it at all. IMHO registration is really something else than provisioning and that it tries to use it feels like a risk. Are there other callers to the registration controller that ever call it without host[build]=false now?

[ekohl]

Since I don't have a setup to verify this I'm going to reason based on reading the code. What I'm trying to understand is if any TFTP record is written.

Looking at https://github.com/theforeman/foreman/blob/develop/app/models/concerns/orchestration/tftp.rb then it checks host.managed?. Since it only enters build mode for unmanaged hosts it makes me think it solves the immediate problem.

Long term we really should have some documentation about the Host model and its life cycle. I think nobody really has some sort of state diagram and what the various transitions mean.