search

kube-object-storage / lib-bucket-provisioner

Library for the dynamic provisioning of object store buckets to be used by object store providers.
Apache License 2.0
20 stars 22 forks source link

Handle ob/obc=nil when clean up provision failures #201

Closed BlaineEXE closed 3 years ago

BlaineEXE commented 3 years ago 
If there are failures during provisioning, the OB or OBC can be nil
when the cleanup function runs on an error. Allow both OB and OBC
to keep their previous info if create/update of OB/OBC fails. If the
create/update succeeds, fill in the OB/OBC info with the latest
successful info.

If OB/OBC become nil on create/update before cleanup, it can result in
the provisioner's Delte() call failing to completely clean up resources
from the failed Provision().

The setBasicOBInfo function called after provisioning and before the
deferred cleanup ops was also making debugging this issue more mentally
cumbersome. Revise code flow to still update the necessary OB
information needed for cleanup before returning errors from
provisioning. This does not change behavior but makes the code flow more
conceptually linear.

Signed-off-by: Blaine Gardner blaine.gardner@redhat.com

jeffvance commented 3 years ago

Why not also check for nil in addition to empty here? Eg: if ob == nil || ob.Spec.ClaimRef == nil || *ob.Spec.ClaimRef == emptyRef {

Would this eliminate the need for createdOB in updateOnSuccess? This OB copy and the OBC copy below add a GC burden that may not be necessary.

jeffvance commented 3 years ago

deleteSecretAndWait (called by deleteExistingSecretAndConfigMapIfExist) gets the Secret. But we already have the Secret so I don't see the need to re-get it. This is a nit comment that perhaps can be looked into in a separate PR. Does not impact merge-ability of this pr.

BlaineEXE commented 3 years ago

deleteSecretAndWait (called by deleteExistingSecretAndConfigMapIfExist) gets the Secret. But we already have the Secret so I don't see the need to re-get it. This is a nit comment that perhaps can be looked into in a separate PR. Does not impact merge-ability of this pr.

I did also notice that today. Let's handle that in an update if that sounds good? Created https://github.com/kube-object-storage/lib-bucket-provisioner/issues/202 for this.