Is there a default number of tries before backing off if no response is returned for claim?

Is no response meant to indicate transient error, while explicit error should indicate definite error that cannot be overcome by a subsequent repetition of prepare call for that claim ?

There's no such semantic difference. kubelet treats a missing response as if the prepare call hadn't been invoked and will retry like it normally does - at least that was the goal. I need to double-check whether the implementation really does it.

The expectation was that drivers only return less entries when there was some kind of error that they reported. I suspect that returning no error and less entries is not currently caught in the post-processing.

The wording was indeed unfortunate. The intend was to treat that situation like an error, so the description should also state it like one. Changed.

The necessary check in the code was indeed missing - added.

While doing so, I found some cut-and-paste errors in error messages - fixed.

Here's the full diff: https://github.com/kubernetes/kubernetes/compare/3b74a711a02abc495b351499033593f9831959c3..fd74b7b8ddf7313af813f5c5e8cbb4923f69c534

nit: but I'd prefer to split this apart, i.e.:

pbclaim := &drapb.Claim{
	Namespace:      resourceClaim.Namespace,
	Uid:            string(resourceClaim.UID),
	Name:           resourceClaim.Name,
	ResourceHandle: resourceHandle.Data,
}

batches[pluginName] = append(batches[pluginName], pbclaim)

Okay, changed.

Did this change get reverted as well?

This still doesn't seem to be changed...

There' something still strange to me about this Error field. It is solely for reporting here? Or is it used / interpreted elsewhere?

It's only checked here. The content of the Error field then only gets passed on as part of the error created here... except that err should have been result.Error! Fixed.

Same comment as above for splitting this

Changed.

This is a slight change in semantics from before, since previously we would remove pod references as NodeUnprepareResources() was called successfully on each individual claim. I'm not saying it's not OK to do it this new way, but we should acknowledge that it's a slight change in semantics from before.

Let's keep the original semantic. I moved this code into the for loop above, directly after checking the per-claim result.

Due to the code above for:

		// Is the resource already prepared? Then add the pod UID to it.
		if claimInfo := m.cache.get(claimName, pod.Namespace); claimInfo != nil {
			// We delay checkpointing of this change until this call
			// returns successfully. It is OK to do this because we
			// will only return successfully from this call if the
			// checkpoint has succeeded. That means if the kubelet is
			// ever restarted before this checkpoint succeeds, the pod
			// whose resources are being prepared would never have
			// started, so it's OK (actually correct) to not include it
			// in the cache.
			claimInfo.addPodReference(pod.UID)
			continue
		}

It means we include claims in these batches if they haven't already been prepared (presumably by some other pod or a different round of preparing this pod where some claim allocations failed).

I'm not sure how I feel about that. Given that the underlying NodePrepareResources() call needs to be idempotent, I'd rather send it the full list of claims linked to the pod anytime NodePrepareResources() is called, and let it explicitly ignore the ones that have already been prepared.

I don't have a strong preference either way. As you prefer sending the full list, let's do that.

It leads to one additional corner case: all claims for a driver might have been prepared already. In that case we shouldn't call NodePrepareResources, should we?

It doesn't hurt if we do, the driver should just not actually do any preparation.

One thing this makes me realize though is that that we originally discussed with @byako to both (1) batch claim requests and (2) pass pod info to the Prepare/Unprepare calls so that the driver would be aware of which pod's claims it was currently processing. If the pod info was also being passed, then it would definitely make sense to pass the full list of claims to Prepare/Unprepare even if there was nothing for the driver to do (so that it could keep track of this mapping if it wanted).

I presume @byako had a use case for passing the pod info (and I remember having a use-case for it as well), but its been a while since I looked at it and I can't remember of the top of my head why it was useful.

When passing pod info, what information should that be? Name/namespace/uid?

The problem then becomes the NodeUnprepareResources: it doesn't get called for a pod when some other pod is still using the claim, so the driver will not be able to determine when a pod stops using a claim. Is that okay?

We have decided to avoid passing pod info in this iteration as well as keep the original semantics of only including claims in the batch if they are currently unprepared.

Just to clarify the use case - the Pod info was used to map in CRD for what Pod the claim was allocated, so when the first time the plugin is called to prepare one out of N claims for the Pod (without batch of claims) - plugin has a chance of finding on its own from CRD all the claims for the same Pod and prepare them all at once because HW only allows one configuration at a time. Example is SR-IOV Virtual Functions - after first N Virtual Functions were created with one config call, subsequent ones (coming from other claims for the same HW) can only be created after first is removed.

The Pod info is not needed in case the plugin is given all claims' info for the Pod because there is no need to traverse CRD to find all claims allocated for the Pod.

Passing only unprepared claims should be fine, I've no opinion if it's good or bad. The only case when it might be important, that I can think of, is when on previous prepare call some claims succeeded and some did not, and IMHO this should only happen when the claims were for different HW. There should not be a case when some claims for HW A succeeded, and some other claims for same HW B in the same prepare call failed. If this was the case, then the driver should unprepare automatically first claims that succeeded for HW A on the same call and report them as failed as well (because subsequent claims might not be accepted by HW A) so the next prepare call would have all claims for HW A again.

If we batch the pod reference deletions at the end like this, then I think the call to syncToCheckpoint () within this loop can simply be removed. We call it again just after the loop and without a bunch of gRPC calls between the checkpoints as before (which is why we wanted to checkpoint early in case one of the NodeUnprepareResource() calls failed).

I think the syncToCheckpoint at the end of each function also was redundant. Doing it once after each gRPC call should be enough. Changed.

... except when the entire call was skipped. Then the final sync is needed.

I actually meant the other way around -- the final one is necessary -- the intermediate ones are not.

So prune them even further, i.e. do it potentially after multiple NodePrepareResources calls? That can happen when there are multiple plugins involved.

I was wondering how we were going to handle the old API. This seems like a reasonable solution -- i.e. always make calls using the new API in manager.go but convert them under the hood to the old API (if necessary) from within the client.

Is there no way to just check which API a plugin registered with, rather than attempt a call on the new API can fall back to the old one if it fails? If not, is there some way to make this more generic instead of inline in this call?

We would need some persistent state for the plugin. Perhaps this can be handled as part of #118619?

OK, let's leave this as is and I'll work with @TommyStarK to clean this up in his PR.

OK;, I'm fine with it. I just wanted to make sure we were conscious of why we would do it this way vs. the other.

We can add it later without bumping the API as you mention. I think it's fine as is for now. (Though a little weird to me to just pass back an arbitrary string just for logging purposes).

We have decided to avoid passing pod info in this iteration as well as keep the original semantics of only including claims in the batch if they are currently unprepared.

The comment that begins this loop is now incorrect as we don't call NodeUnprepareResource() from within this loop anymore. That comment should be moved below to the new loop with:

for pluginName, claims := range batches {

Please double check with the PrepareResources() call and try and make the comments consistent.

The "only for the last pod that references the claim" seemed redundant to me. I dropped it in the updated comments.

Done, with "for all claims in each batch" - there's one for each plugin.

OK, let's leave this as is and I'll work with @TommyStarK to clean this up in his PR.

Can we make this:

cdi_devices = 1 [(gogoproto.customname) = "CDIDevices"];

So that it is referenced in go as CDIDevices instead of CdiDevices.

Done.

This still doesn't seem to be changed...