I was looking at reports raised by users, that accidentaly deleted production VMs. There were different workflows that lead to this
user didn’t know that when Host is linked to VM, host deletion triggers that. In UI we already ask for confirmation but in API, there’s no interactive confirmation dialog
there is a clean_backend_objects rake task in Katello cleaning objects such as duplicated hosts which didn’t check the VM assignment (now fixed)
For users who don’t create hosts by provisioning, but later do the VM association, this can be unexpected behavior, what’s worse with very destructive outcomes. So I worked on a small fix [1] that adds the global setting to first disassociate VM before host deletion. Given the possible impacts of deletion, I wonder if we should make it enabled by default, meaning VMs would not be deleted until user allows that.
I know this would change the current behavior of Foreman. I don’t think we can easily detect whether this is an existing setup or not given the way how we seed settings (initializer calls load_defaults method). And this is the reason I’m looking at users to see, what they would prefer.
I’d like to see the setting merged in 1.20 so if the discussion will take longer, I’m happy with merging the setting defaulting to existing behavior and we can change after 1.20.
Disable VM deletion by default in 1.20, users can reenable
Keep the things as are today, making it configurable
I’d be ok with this. Long term we should maybe think of a workflow that just shuts down a vm and actually deletes the VM after a grace period as it’s implemented in the expire hosts plugin.
My worst fear is that Foreman accidentally deletes all hosts with all associated VMs. This could ruin a company for good.
We use foreman with oVirt and the actual behavior is ok for us (deleting the underlying VM). As @ohadlevy suggest in the PR, for the webui worklfow, maybe an UI enhancement can solve this problem. For example maybe adding a checkbox “Delete the VM” on Delete host confirmation box (when the host VM is associated to a VM), with the default value reflecting the new global setting disassociate_vms_before_host_deletion should be fine and let the ability to the user to choose if he want to follow the default setting or not on each case.
For the API, maybe we can add a new optional param (set by default to the global setting value) to have the same behavior as the WebUI
@bagasse, that was in fact my longer term plan for UI. I already checked we have confirmation dialog component in patternfly-react so it should be relatively easy. I like the idea for API too. The question now though is, how much it would surprise/annoy you, if you had to switch this setting after you upgrade to 1.20 to get the behavior you are used to today.
To make it slightly easier to see what’s the preference, I’ll add a poll to the thread. Please reflect your vote there too.
@Marek_Hulan, this will not be annoying if you change the default behaviour to not delete VM by default, but IMHO, this will be not consistent with other integrations. For example in our case, when we delete an host, it will delete from FreeIPA(host and DNS), dhcp server and the VM (if the host is a VM). Anyway all will be fine for me, if you want to switch to more safe default setting in a future version, we will document it and switch to the current behaviour on upgrades or new installs.
My concern with flipping the default is that it will surprise a lot more users than are currently surprised when a VM is deleted by accident. Instead we’ll have a much larger segment surprised that their VM hosts have no capacity left (or that their EC2 is huge this month …).
Going with the “principle of least surprise” which is my general rule for user-facing things, I’d vote to keep it as is, and document the setting for those that need it. If it is merged as default, then can we please add some kind of notification to the user that the VM still exists and will consume resources / money until stopped? (+1 for the expire_hosts plugin here, maybe we should merge that to core…)
I agree that a check box on the delete confirmation makes sense longterm, that seems like a good option (and matches what we see elsewhere in the industry)
Given we have different ways how one got the the state of having the VM associated to the host object, one option would be to have a flag on the host, determining the default behavior? Something like VM delete protection.
for hosts provisioned with foreman, the protection would be turned off by default (the default set in Settings)
for hosts with vm associated later, the protection should be turned on by default (or let the user decide when associating the vm)
If the VM protection would be set to true, the checkbox on vm deletion would be disabled, if protection is off, the user could decide (with default set to delete).
And if we wanted to go really crazy, we could make the ‘delete protection’ extended to all the resources it’s linked to, and let the user decide what to clean and what not.
Just my 2c.
For short term, +1 to have at least setting about the default behaviour.
I think it must be simple and consistent for all hosts. So far I prefer global setting for default and possibility to override on action, rather than host, level.
I totally agree with your principle so I’m voting for making it configurable. Especially in big environments you would produce so much dead VMs. You might gonna say “cleaning things up is your task” but in practice nobody is doing that.
