gRPC resource provision-deprovision race condition

[BlaineEXE]

Bug Report

What happened:

I have identified what I believe to be a possible race condition in COSI's API/proto spec that allows for a backend resource to be left (orphaned) without cleanup.

What you expected to happen:

Backend resource orphans should be minimized as much as possible.

How to reproduce this bug (as minimally and precisely as possible):

Imagine this case:

1. A new Bucket is created
2. The DriverCreateBucket gRPC call is made, and it returns successfully, with a bucket_id
3. When COSI records the bucket_id into Bucket.status, a temporary kube API error occurs
4. Simultaneously, the Bucket resource is deleted
5. The next time COSI reconciles the Bucket, it has a deletion timestamp, and the bucket_id is not recorded on the Bucket. Because there is no bucket_id, the DriverDeleteBucket cannot be called, but the Bucket resource can be cleaned up successfully, with COSI believing the previous provision to have been unsuccessful.

I'm quite certain that this corner case is possible; however, it seems like it would be extremely rare in production.

Anything else relevant for this bug report?:

I looked at the analogous CSI spec to see if it has notes or different info tracking.

From a high-level and naive view, it appears that CSI also has the potential to encounter this same race condition. However, this assumes the Kubernetes PV reconciler does not handle this case with special internal logic. It's hard for me to imagine that sig-storage hasn't identified this and mitigated it or at least discussed it.

I have 2 follow-up questions to help manage this in COSI:

1. Is this corner case infrequent enough that COSI need not prioritize its resolution?
2. How does Kubernetes/CSI mitigate this corner case?
3. Is it reasonable to update COSI's KEP to use the bucket_name idempotency key used for DriverCreateBucket instead of bucket_id when issuing DriverDeleteBucket to avoid this race condition?

Resources and logs to submit:
N/A

Copy all relevant COSI resources here in yaml format:

# BucketClass
# BucketAccessClass
# BucketClaim
# Bucket
# BucketAccess

# Copy COSI controller pod logs here

# Copy COSI sidecar logs here for the relevant driver

Environment:

• Kubernetes version (use kubectl version), please list client and server:
• Sidecar version (provide the release tag or commit hash):
• Provisoner name and version (provide the release tag or commit hash):
• Cloud provider or hardware configuration:
• OS (e.g: cat /etc/os-release):
• Kernel (e.g. uname -a):
• Install tools:
• Network plugin and version (if this is a network-related bug):
• Others:

[shanduur]

I see two viable directions to mitigate this:

• If a Bucket has a deletion timestamp but no bucketID, the controller could still attempt provisioning, before calling deletion. This trades some extra calls for fewer orphans.
• Alternatively, extending the API with something like GetBucket would allow the controller to confirm whether backend provisioning actually completed and reconcile accordingly; this is more explicit but requires API changes.

IMHO this race is extremely unlikely but not impossible, so it’s reasonable to document it today and consider mitigation once operational experience justifies the added complexity.

[BlaineEXE]

I think either of those options could work. Also, we have a DriverGetExistingBucket call in the v1a2 KEP, so option 2 may be somewhat easier.

I think there is a different way where we could also avoid the extra calls. I don't think we need to have bucket_id or account_id in the spec at all. name (as proto calls it) is an idempotency key, and it is globally unique.

Most drivers will probably use name as the bucket_id or account_id, and others will probably deterministically generate the X_id from name using prefixes/suffixes. I think we can establish that determinism as a proto requirement and plan to use name for follow-up references to provisioned resources.

[xing-yang]

In external-provisioner, there is logic to handle final vs non-final error. If it is still on-going, then it is still non-final. If a CSI driver returns a non-final error, the provisioner will eventually make a call to CSI driver to delete the volume when it is finally created successfully. I assume that logic should handle this case as well. Does COSI have logic to handle final vs non-final errors?

[BlaineEXE]

COSI does handle retryable vs non-retryable errors, but I don't think that's quite the same as final/non-final. COSI doesn't persistently store the state we would need to handle this corner case.

Mateusz offered a couple suggestions that sound good. As I have thought on it more since yesterday, I think bucket_id is probably an important concept for backends that might not want to generate backend resource names that are deterministic, possibly as a security concern. I think that could be a little pedantic, but I don't think it will hurt to keep bucket_id/account_id.

[addendum] On the other hand, DriverCreateBucket does need to be idempotent, and it only takes name as an idempotency key. If the driver can handle converting name into the right backend ID for every DriverCreateBucket call, shouldn't it be reasonable to assume they can do the same thing for deletion and other follow-up actions. I think I want to ask sig-storage today if they think bucket_id is critical for some reason.

[BlaineEXE]

name : the idempotency key used for creating the bucket
bucket_id or "ID" : ID returned by driver that identifes a created backend bucket

Notes from sig-storage discussion:

• Sunny is working on resolving similar corner cases in CSI currently
• some backends might not know the ID until after the resource create operation succeeds

Outline of discussed options when COSI is deleting a Bucket with no recorded ID:

1. Call Create to get an ID, then Delete afterwards
   • Pro: simple
   • Con: could get stuck in deletion loop if creation can't succeed
2. Create new Get(name) RPC to allow drivers to find an ID given an idempotency key
   • Pro: relatively simple
   • Con: still requires driver to know ID before create succeeds (may not be possible)
3. Create new GenerateId(name) RPC to ask drivers for an ID before Create begins. COSI stores the ID before starting Create operation.
   • Pro: relatively simple
   • Con: still requires driver to know ID before create succeeds (may not be possible)
4. Remove the concept of ID entirely, and rely on name for everything
   • Pro: simple for COSI
   • Con: a subset of drivers that don't know ID before create succeeds will need to keep some sort of database to look this up, which could be an expensive operation
5. Create a new DeleteByName(name, params) RPC call that is used in this corner case
   • Pro: realtively simple. drivers with deterministic name->id logic would have no trouble.
   • Con: challenging or impossible for drivers that don't know the ID before creation
6. Create a new CreateOrFailWithId(name, params) RPC call that is used when COSI wants to delete but doesn't know if Create will succeed or fail
   • Con: this doesn't practically seem different from option 2 (Get(name)) but is more complex

In general, COSI drivers that don't know the ID before creation could work around some of these challenges by keeping an internal database of created resources with their given IDs. This database needn't be complex (could just be a text file), but it would still suffer from the same race condition(s) COSI has, but handling would need to be internal to the driver.

Also, a good clarification is that I could only record the options that have been proposed. There could be other options that I'm not aware of yet.

[BlaineEXE]

After writing all of the above out, my opinion of the best option here is to create a new DeleteByName(name, params) call. The majority of object storage protocls can create buckets (and users) with an ID given as input, so this should be really easy for most driver implementations. For the (estimated small) subset of drivers that might not know the ID before creation, we can allow them to determine what their best internal strategy is for dealing with this case. Realistically speaking, this corner case should be rare, and drivers could conceivably still provide info for manual cleanup even when orphans are left around.

COSI could also try to minimize the corner case by taking a hybrid approach.

1. COSI attempts to call Create() first
   a. If this succeeds, record the ID if possible, but don't stop if there is a recording error
2. Then move to deletion
   a. If Create() succeeded, use the normal Delete(ID)
   b. If Create() failed, use the new DeleteByName(name) call

[bswartz]

1. Call Create to get an ID, then Delete afterwards

This is the status quo, right?

2. Create new Get(name) RPC to allow drivers to find an ID given an idempotency key

I like this somewhat. I need to think about how it could go wrong.

3. Create new GenerateId(name) RPC to ask drivers for an ID before Create begins. COSI stores the ID before starting Create operation.

As Hemant mentioned, for many drivers even this simple call would require generating state that needs to be cleaned up, so you still need to delete. This may not rule out this approach -- it could just force everyone into a multi-phase create while still requiring COs to call delete to avoid resource leaks.

4. Remove the concept of ID entirely, and rely on `name` for everything

This has some significant downsides. Some SPs rely on the ability to encode additional information into the ID which they rely on in later calls. Also CSI driver can currently ensure uniqueness of IDs. Forcing them to take the name might create situations where re-used names cause conflicts in a way that they shouldn't (consider two different CSI instances sharing a storage backend).

5. Create a new DeleteByName(name, params) RPC call that is used in this corner case

I like this somewhat. I need to think about how it could go wrong.

6. Create a new CreateOrFailWithId(name, params) RPC call that is used when COSI wants to delete but doesn't know if Create will succeed or fail

I like this idea, but it's awkward to implement in gRPC, because gRPC doesn't allow a response message except with successful calls. You'd need to have special error semantics where the RPC would return SUCCESS with an error code in a return message field, which almost certainly violates a style guide somewhere.

[bswartz]

After writing all of the above out, my opinion of the best option here is to create a new DeleteByName(name, params) call.

One of the first issues that comes to mind (thinking about the CSI version of this -- not COSI) is that Create takes secrets and Delete does not take secrets, because Delete is expected to work even when the secrets are lost. I could imagine a situation where it's not possible to map the name to an ID without the secrets (because the secret is used to talk to an external system that performs this function). This would force us to decide to either (1) require secrets get passed into DeleteByName, which violates the guideline that it should be possible to cleanup even when secrets are lost or (2) not pass in secrets to this RPC which could make it impossible to implement in some cases.

[BlaineEXE]

Thanks for the quick feedback, Ben, and I look forward to your digested thoughts later.

Xing also mentioned in our COSI huddle that this corner case can be reduced (but not toally eliminated) by internally keeping a queue of create operations that are currently in progress but not persisted. This can allow COSI to make better decisions internally in this case but does still risk orphans if the reconciler is restarted and the queue lost

This is a purely-internal fix we will plan to add to COSI later if we still need it based on how this convo is concluding.

[gnufied]

Delete does not take secrets, because Delete is expected to work even when the secrets are lost.

In CSI DeleteVolume takes secrets as well.

DeleteByName(name, params)

I like this idea. If a COSI driver wants deletion to be failsafe they must implement this RPC call.