Only KeystoneAPI finalizer from sub-resources when actually needed

[abays]

We are seeing the cleanup of certain KUTTL tests hang. This happens because KeystoneAPI fails to fully delete when multiple service CRs (Nova, Glance, Placement, etc) are being deleted at the same time. This is more likely to occur in operators that have a lot of other operator dependencies in their KUTTL testing (for instance, Octavia). KeystoneService is used in the enumeration below, but this also applies for KeystoneEndpoints.

1. The problem is that the deleted KeystoneService resource for all the OSP service CRs is being requeued one more time after it has removed its own self-finalizer and is then waiting to be fully deleted.
2. And why is it requeued? Because it's watching itself and the removal of its self-finalizer will queue it for one more read (because it's an update to its metadata).
3. It's a race though. Since the self-finalizer is gone, k8s will be pinged to remove the KeystoneService from the cache and etcd at the same time as that object has been requeued for one more read.
4. If k8s removes it first, no problem. But if the reconcile happens first, then what happens is that the KeystoneService adds a finalizer back onto KeystoneAPI (

   keystone-operator/controllers/keystoneservice_controller.go

   Lines 191 to 202 in e5f8627

   	//
   	// Add a finalizer to the KeystoneAPI for this service instance, as we do not want
   	// the KeystoneAPI to disappear before this service in the case where this service
   	// is deleted
   	//
   	if controllerutil.AddFinalizer(keystoneAPI, fmt.Sprintf("%s-%s", helper.GetFinalizer(), instance.Name)) {
   	err := r.Update(ctx, keystoneAPI)
   	if err != nil {
   	return ctrl.Result{}, err
   	}
   	}

   ).
5. And then, later in that same reconcile, where the deletion logic tries to remove that same finalizer from the KeystoneAPI, you can hit the "object has been modified" error if other threads for other KeystoneServices are also modifying the KeystoneAPI.
6. And if you hit that error, you're now in a state where the KeystoneService is going to be completely gone at the end of the reconcile (because its self-finalizer was removed in the last reconcile and won't be removed in this one, hence it won't cause a requeue and k8s will completely wipe it from etcd) and yet it has left a finalizer on KeystoneAPI, forever preventing the deletion of the KeystoneAPI.

To prevent this problem, we are moving the Update() to add a finalizer to the KeystoneAPI in the KeystoneService and KeystoneEndpoint reconcile loops to a location that only executes when that finalizer is actually needed. Beforehand we were potentially always adding the finalizer to KeystoneAPI even if the KeystoneService or KeystoneEndpoint had no need of it anymore (i.e. during deletion, after user/service/endpoint had been removed from the OSP Keystone database).

[stuggi]

/test keystone-operator-build-deploy-kuttl

[stuggi]

/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abays, stuggi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [abays,stuggi]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[stuggi]

/test keystone-operator-build-deploy-kuttl

[abays]

/test keystone-operator-build-deploy-kuttl