Fixes #38267 - rake ovirt:drop

[stejskalleos]

Rake task for removal of Ovirt compute resources and its data

• Disassociate all hosts from the CR
• Update host groups
• Remove Compute resource & its profiles

Note:

• The code removal will follow in the upcoming PR
• The rake task should work with removed Ovirt code
• This rake task is going to be cherry-picked to earlier versions, so users can run it before upgrading their instances
• It will be a migration as well.

[stejskalleos]

@Lennonka text review please. This should also be documented in the upgrading workflow. The versions to chery-pick have not been decided yet.

[chris1984]

@stejskalleos want me to test this?

[ekohl]

Without looking at the exact code I'd expect an RFC for such a user visible change as well as deprecation notices in the release notes.

The code removal will follow in the upcoming PR

This will also need packaging changes since we have a foreman-ovirt subpackage.

[stejskalleos]

Without looking at the exact code I'd expect an RFC for such a user visible change as well as deprecation notices in the release notes.

Good point, I'll create an RFC.

I was under the impression that the deprecation was announced, but looking at the documentation code, it seems like it was deprecated only for Satellite.
@Lennonka together with this PR we should announce deprecation of the Ovirt for the upcoming upstream version
The Ovirt code can then be removed in the next version.

This will also need packaging changes since we have a foreman-ovirt subpackage.

Yop. Packaging and installer PRs will be in the scope.

[stejskalleos]

@chris1984 QE's will be testing it before the merge, but if you have time, It will be good to have it tested by dev as well

[ekohl]

On the rake task I'm wondering if we should somehow generalize it so it could be used for more compute resources. I have some vague recollection about users wanting to disassociate VMs from the UI, but I think that was also from before you could avoid deleting a VM when you deleted the Foreman host.

[ekohl]

Technically you can link them back up so I'm not sure it's really that irreversible. Just quite a bit of work.

[Lennonka]

Just a couple of nitpicks. + I agree with Ewoud

[ekohl]

@ColeHiggins2 for your visibility: this is something to keep track of for Foreman 3.14.

[ShimShtein]

@stejskalleos I think you also need to remove compute_attributes from the relevant compute profiles.

[stejskalleos]

@ekohl @Lennonka Log messages fixed

@ShimShtein Compute Attrs should be removed together with the compute resource, see https://github.com/theforeman/foreman/blob/develop/app/models/compute_resource.rb#L28

[stejskalleos]

• Support for OVIRT_ASSUME_YES
• Listing affected compute resources in the warning
• RHEV -> RHV

[stejskalleos]

Not sure why the CI is failing but the PR is ready for re-review. cc @chris1984 @ekohl

[stejskalleos]

And it's all green! @nofaralfasi can you review please?

[ekohl]

This rake task is going to be cherry-picked to earlier versions, so users can run it before upgrading their instances

To be explicit: you want this in 3.14?

[nofaralfasi]

On the rake task I'm wondering if we should somehow generalize it so it could be used for more compute resources.

Why would a user want to remove all compute resources of a provider while keeping the hosts? Is that a valid use case?
In general, I agree that we should make it more flexible. However, perhaps we should consider a different RFC, like one for completely removing a compute resource - which would also delete all associated hosts and hostgroups.
Alternatively, we could add an option that prompts the user to decide if they want to remove the hosts and hostgroups as well (maybe something we should consider for this PR as well).

[ekohl]

Why would a user want to remove all compute resources of a provider while keeping the hosts? Is that a valid use case?

Depends on their migration plan. This way they don't need to migrate their hosts, but they can no longer do things like power management or delete the actual VM. IMHO that's valid. It may also be that they will bulk migrate them to some other platform and don't want to import them again. Instead, they can (if Foreman supports the compute resource) link them back up to the new CR.

In general, I agree that we should make it more flexible. However, perhaps we should consider a different RFC, like one for completely removing a compute resource - which would also delete all associated hosts and hostgroups.

I think that's dangerous. Deleting hosts will terrify any sysadmin. Worst case you wipe out an entire infrastructure. Hostgroups may also be mixed.

Alternatively, we could add an option that prompts the user to decide if they want to remove the hosts and hostgroups as well (maybe something we should consider for this PR as well).

A noop mode is generally speaking a good idea, listing what would be affected.

[stejskalleos]

To be explicit: you want this in 3.14?

It would be nice to have it there, as it gives users more time for migration and preparation. But it's not a requirement.

like one for completely removing a compute resource

I want to leave that up to the user. As @ekohl said, removing hosts and host groups might be terrifying.

A noop mode is generally speaking a good idea, listing what would be affected.

I'll add it; that's a good idea.

[stejskalleos]

Updated:

• Added pretend mode for listing affected resources.
• Using User.as_anonymous_admin instead of .unscoped
• Improved description and help message.

[stejskalleos]

And it's 🍏
@nofaralfasi review please
@amolpati30 ready for QE

[amolpati30]

@stejskalleos
Tested with this patch, and it successfully removes the Compute Resource, Compute Profile, and RHV from the Host Group. However, testing for the Host was not possible as the manifest is not working on stream.

[stejskalleos]

@nofaralfasi any comments, or can we merge?

[nofaralfasi]

Another suggestion: You have multiple methods that print a message and exit. Why not refactor them into a single method that takes a message and an exit status as arguments?

[stejskalleos]

Another suggestion: You have multiple methods that print a message and exit. Why not refactor them into a single method that takes a message and an exit status as arguments?

Most of them have multi-line output, and storing that output in a variable and then passing it to the method is not very readable, IMO. So, in this case, I chose readability over DRY.

[nofaralfasi]

I left all my notes and let Leos decide which ones to address. Other than that, the code looks good to me, but I didn’t test it.

[stejskalleos]

@nofaralfasi Sorry, my bad, I accidentally missed the prompt_user_confirmation rename. Now it should be 1:1 with your comments.

[amolpati30]

@stejskalleos
I have tested and LGTM.