the distinction between IsPodDone and IsPodTerminal is going to be pretty confusing for callers... I expect people to choose incorrectly... when should someone use IsPodTerminal instead of IsPodDone?

I am not sure. Perhaps all users of IsPodTerminal should also check for the additional condition in IsPodDone? Then we don't need two functions. Let's see... there are exactly two calls outside of tests:

• kubernetes/pkg/controller/job/job_controller.go

  Line 1008 in 51429cb

  if podutil.IsPodTerminal(pod) || considerPodFailed || finishedCond != nil || job.DeletionTimestamp != nil {

• kubernetes/pkg/controller/util/endpoint/controller_utils.go

  Lines 92 to 112 in 51429cb

  	// ShouldPodBeInEndpoints returns true if a specified pod should be in an
  	// Endpoints or EndpointSlice resource. Terminating pods are only included if
  	// includeTerminating is true.
  	func ShouldPodBeInEndpoints(pod *v1.Pod, includeTerminating bool) bool {
  	// "Terminal" describes when a Pod is complete (in a succeeded or failed phase).
  	// This is distinct from the "Terminating" condition which represents when a Pod
  	// is being terminated (metadata.deletionTimestamp is non nil).
  	if podutil.IsPodTerminal(pod) {
  	return false
  	}
  	if len(pod.Status.PodIP) == 0 && len(pod.Status.PodIPs) == 0 {
  	return false
  	}
  	if !includeTerminating && pod.DeletionTimestamp != nil {
  	return false
  	}
  	return true
  	}

The first looks like it would be okay with the semantic from IsPodDone. The second has explicit handling of DeletionTimestamp, so it's probably not okay to change it.

Let's defer this. I'll make IsPodDone local to pkg/controller/resourceclaim/controller.go for now and unless someone cares enough, will probably simply keep it there.

/cc @alculquicondor - you suggested moving this to pkg/api/v1/pod/util.go earlier.

See https://github.com/kubernetes/kubernetes/compare/e68f74fc3978920769a4791ef4fb7aca5a426e7f..a514f40131c298ef7ca375e5ab8fa8ddafa3e74d

@liggitt : okay to move ahead without the pkg/api/v1/pod change?

If yes, then please approve the auth change.

Nit, but if you reverse the if condition, you can make the code less complicated with:

if len(podName) == 0 {
     logger.V(5).Info("claim not generated for a pod", "claim", klog.KObj(claim))
     return nil
}
// the rest goes here

this allows the code be less indented, which is much more readable, and follows the general guidelines to "fail fast" which roughly the idea here 😉

In this particular case I prefer the current approach because it's not really a failure that gets checked for but rather a "do I need to do this thing here, if not, continue". If we ever add other things that might need to be done for a claim, then they would go before the current "return nil" at the end. That wouldn't work if some code in the middle returns.

OTOH, perhaps that's a reason to split up the function into smaller parts. Well, not now, okay?