Code review comment for lp://qastaging/~johannes.erdfelt/nova/deferred-delete-instance

Revision history for this message
Brian Lamar (blamar) wrote :

> blamar: the rest of delete and force_delete are identical, but the
> preconditions are different. Both require different states to perform the
> action requested. This is to ensure that if you delete an instance twice, it
> doesn't get hard deleted. And that if the forceDelete action is called, it
> doesn't do a hard delete unless it's already in a soft delete state.
>
> The logic can be pushed up into the OS API level, but that leaves the compute
> API level open to being called incorrectly.
>
> An alternative would be to pass a flag to delete() to differentiate between
> the two. Or the common logic could be split out to a _delete() method leaving
> delete() and force_delete() to just check with the correct state.
>
> After thinking about it, I'm leaning towards a flag to delete().

I'm not a huge fan of binary flags so I'd lean towards the _delete() method, but either way the code cleanup would be welcome so it's your call.

I do agree with Waldon's suggestion of FLAGS.enable_instance_soft_delete & FLAGS.reclaim_instance_interval just because it is a little awkward to have reclaim_instance_interval = 0 disable soft deletes in my opinion.

« Back to merge proposal