[YUNIKORN-2834] [shim] Add non-YuniKorn allocation tracking logic

[pbacsko]

What is this PR for?

Add foreign pod handling logic to the shim. Existing code which deals with occupiedResources will be removed in a follow-up PR.

What type of PR is it?

• - Bug Fix
• - Improvement
• - Feature
• - Documentation
• - Hot Fix
• - Refactoring

Todos

• - Task

What is the Jira issue?

https://issues.apache.org/jira/browse/YUNIKORN-2791

How should this be tested?

Screenshots (if appropriate)

Questions:

• - The licenses files need update.
• - There is breaking changes for older versions.
• - It needs documentation.

[codecov]

Codecov Report

Attention: Patch coverage is 80.85106% with 9 lines in your changes missing coverage. Please review.

Project coverage is 68.65%. Comparing base (e9b05eb) to head (9be1a49).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/cache/context.go	47.05%	6 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #918      +/-   ##
==========================================
+ Coverage   68.41%   68.65%   +0.23%     
==========================================
  Files          70       70              
  Lines        7653     7675      +22     
==========================================
+ Hits         5236     5269      +33     
+ Misses       2208     2200       -8     
+ Partials      209      206       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[craigcondit]

I think we need to handle orphan pods for foreign pods similar to yunikorn ones. In other words, if a pod references a non-existent node, it needs to be "orphaned" and not sent to the core. If a node appears, any currently orphaned pods (foreign or otherwise) which reference the new node need to be adopted and registered with the core.

[pbacsko]

I think we need to handle orphan pods for foreign pods similar to yunikorn ones. In other words, if a pod references a non-existent node, it needs to be "orphaned" and not sent to the core. If a node appears, any currently orphaned pods (foreign or otherwise) which reference the new node need to be adopted and registered with the core.

Makes sense. I'll update the PR with these changes tomorrow.

[craigcondit]

Also, I'm pretty sure the removal of the context lock broke the logic around ensuring orphan pod handling is done correctly (and is likely responsible for some of the resource mismatches in 1.6.0). This PR was where this was done: #859

I think we need to re-add the locks here. The context lock was not used to guard the context object itself, but to act as a "meta-lock" to ensure that we don't process node and pod events simultaneously. I believe the fix we did in 1.5.2 was sufficient, and we should not have removed the locking in #859.

[pbacsko]

I think we need to handle orphan pods for foreign pods similar to yunikorn ones. In other words, if a pod references a non-existent node, it needs to be "orphaned" and not sent to the core. If a node appears, any currently orphaned pods (foreign or otherwise) which reference the new node need to be adopted and registered with the core.

@craigcondit I looked at the code in more details, here are my findings:

1. Adding orphan foreign pod: handled inside updateForeignPod():

if oldPod == nil && utils.IsAssignedPod(pod) && !utils.IsPodTerminated(pod) {
		if ctx.schedulerCache.UpdatePod(pod) {
			// pod was accepted by a real node
			log.Log(log.ShimContext).Debug("pod is assigned to a node, trigger occupied resource update",
				zap.String("namespace", pod.Namespace),
				zap.String("podName", pod.Name),
				zap.String("podStatusBefore", podStatusBefore),
				zap.String("podStatusCurrent", string(pod.Status.Phase)))
			allocReq := common.CreateAllocationForForeignPod(pod)
			if err := ctx.apiProvider.GetAPIs().SchedulerAPI.UpdateAllocation(allocReq); err != nil {
				log.Log(log.ShimContext).Error("failed to add foreign allocation to the core",
					zap.Error(err))
			}
		} else {
			// pod is orphaned (references an unknown node)
			log.Log(log.ShimContext).Info("skipping occupied resource update for assigned orphaned pod",
				zap.String("namespace", pod.Namespace),
				zap.String("podName", pod.Name),
				zap.String("nodeName", pod.Spec.NodeName))
		}
		return
	}

2. Pod becomes assigned to a real node: handled together with YK pods (adoptedPods slice)
3. Orphan pod update: same as addition, handled in updateForeignPod()
4. Deleting orphan pod: this is where probably the only change is necessary, don't send message to the core:

func (ctx *Context) deleteForeignPod(pod *v1.Pod) {
	releaseReq := common.CreateReleaseRequestForForeignPod(string(pod.UID), constants.DefaultPartition)
	if err := ctx.apiProvider.GetAPIs().SchedulerAPI.UpdateAllocation(releaseReq); err != nil { <--- don't send if orphan pod
		log.Log(log.ShimContext).Error("failed to remove foreign allocation from the core",
			zap.Error(err))
	}

	log.Log(log.ShimContext).Debug("removing pod from cache", zap.String("podName", pod.Name))
	ctx.schedulerCache.RemovePod(pod)
}

Is there anything else?

[craigcondit]

Is there anything else?

That seems pretty complete. I'd like to see some tests that verify correctness before and after each of these scenarios. The locking definitely needs to be there in the context regardless.

[pbacsko]

Added tests for orphan pods.

I didn't do anything with 4) because it's much safer to always send a release request.

[craigcondit]

@pbacsko Can you rebase? YUNIKORN-2910 got merged.

[chenyulin0719]

Overall LGTM, some minor comments.

[chenyulin0719]

+1 LGTM