dont err for missing Secret in GetDatabaseByNameAndAccount #209
Conversation
The MariaDBAccount CR / controller currently does not add finalizers to the Secret that's referenced by the controller. This creates a situation where the Secret can be deleted and fully erased from the cluster, but the MariaDBAccount still refers to that named secret. When this happens, if a controller is reconciling the delete of its own CR as well as its MariaDBAccount, the lookup for MariaDBDatabase/MariaDBAccount must succeed so that db.DeleteFinalizer can be called. Prior to this change, the GetDatabaseByNameAndAccount returns a not-found, causing the calling controller to skip it and delete its own CR, leaving the MariaDBAccount and MariaDBDatabase dangling. the issue can be reproduced by building up an openstack env with keystone / galera / memcached / rabbitmq, then deleting the namespace. k8s will delete the secrets more quickly than it can remove the CRs since they are reconciling the delete.
zzzeek
requested review from
stuggi
and removed request for
dciabrin and
frenzyfriday
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: zzzeek The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
the other way to handle this would be for mariadbaccount controller to maintain finalizers in the Secret, let me know if that's a better way but i have not seen finalizers used in secrets, it seems like it would just add to the webs of finalizers that might hang around |
Yeah, I think none of our operators adds a Finalizer to the Secret passed in by name. The Finalizer based solution would be more k8s native. One downside of the current proposal is that after getting the facade the Secret in it might be nil. So the caller like https://github.com/openstack-k8s-operators/nova-operator/blob/7f619eed881eb7d736873c6454df7d70152231f4/controllers/novaapi_controller.go#L427-L432 now needs to check for nil pointer. Maybe having the extra finalizer handling logic here is cheaper than pushing adaptation patches to the affected operators to check for the nil pointer. (but I haven't checked how may operators are affected) |
|
well option three is we add additional parameters to the GetDatabaseByNameAndAccount, or, add a different function that's essentially "RemoveAllFinalizersYouCanFindForUs(DatabaseName, AccountName)" that doesnt rely on any lookups. downside for both of those is that we have to go through and change all the controllers again. |
|
anyway, Im not sure if this issue has impacted CI, when we saw MariaDBAccounts not being deleted, so let's make a call how to proceed here, if we want to add finalziers to secrets. |
|
whoops, that's wrong. the calling contorller adds its own finalizer |
|
ive added the finalizer version at #210, I think it's better. the finalizer name has to be namespaced to be in the Secret but I was able to figure that out |
|
abandoned in favor of #210 . in subsequent PRs I will start removing all the now-unused cruft from the functions here. |
note: alternate version, which is probably better, is to apply a finalizer to the secret. see #210
The MariaDBAccount CR / controller currently does not add finalizers to the Secret that's referenced by the controller. This creates a situation where the Secret can be deleted and fully erased from the cluster, but the
MariaDBAccount still refers to that named secret. When this happens, if a controller is reconciling the delete of its own CR as well as its MariaDBAccount, the lookup for MariaDBDatabase/MariaDBAccount must
succeed so that db.DeleteFinalizer can be called. Prior to this
change, the GetDatabaseByNameAndAccount returns a not-found, causing
the calling controller to skip it and delete its own CR, leaving
the MariaDBAccount and MariaDBDatabase dangling.
the issue can be reproduced by building up an openstack env with keystone / galera / memcached / rabbitmq, then deleting the namespace. k8s will delete the secrets more quickly than it can remove the CRs since they are reconciling the delete.