FrankYang0529 commented on code in PR #1050:
URL: https://github.com/apache/yunikorn-k8shim/pull/1050#discussion_r3571158150
##########
pkg/cache/context_test.go:
##########
@@ -717,92 +744,75 @@ func TestAddUpdatePodForeign(t *testing.T) {
// pod is not assigned to any node
pod1 := foreignPod(podName1, "1G", "500m")
pod1.Status.Phase = v1.PodPending
- pod1.Spec.NodeName = ""
-
- // validate add (pending, no node assigned)
- allocRequest = nil
- context.AddPod(pod1)
- assert.Assert(t, allocRequest == nil, "unexpected update")
- pod := context.schedulerCache.GetPod(string(pod1.UID))
- assert.Assert(t, pod == nil, "unassigned pod found in cache")
-
- // validate update (no change)
- allocRequest = nil
- pod1Upd := pod1.DeepCopy()
- context.UpdatePod(pod1, pod1Upd)
- assert.Assert(t, allocRequest == nil, "unexpected update")
- pod = context.schedulerCache.GetPod(string(pod1.UID))
- assert.Assert(t, pod == nil, "unassigned pod found in cache")
- // pod is assigned to a node but still in pending state, should update
pod2 := foreignPod(podName2, "1G", "500m")
- pod2.Status.Phase = v1.PodPending
pod2.Spec.NodeName = Host1
+ pod2.Status.Phase = v1.PodRunning
- // validate add
- context.AddPod(pod2)
- assert.Assert(t, allocRequest != nil, "update expected")
- assertAddForeignPod(t, podName2, Host1, allocRequest)
- pod = context.schedulerCache.GetPod(string(pod2.UID))
- assert.Assert(t, pod != nil, "pod not found in cache")
-
- // validate update (no change)
- allocRequest = nil
- pod2Upd := pod2.DeepCopy()
- context.UpdatePod(pod2, pod2Upd)
- assert.Assert(t, allocRequest == nil, "unexpected update")
- pod = context.schedulerCache.GetPod(string(pod2.UID))
- assert.Assert(t, pod != nil, "pod not found in cache")
Review Comment:
Should we add this case back?
```diff
diff --git a/pkg/cache/context_test.go b/pkg/cache/context_test.go
index deb5bb8c..e8cc54d0 100644
--- a/pkg/cache/context_test.go
+++ b/pkg/cache/context_test.go
@@ -752,28 +752,37 @@ func TestUpdateForeignPod(t *testing.T) {
pod3 := pod2.DeepCopy()
pod3.Status.Phase = v1.PodFailed
+ pod1Assigned := pod1.DeepCopy()
+ pod1Assigned.Spec.NodeName = Host1
+ pod1Assigned.Status.Phase = v1.PodRunning
+
tests := []struct {
name string
oldPod *v1.Pod
newPod *v1.Pod
+ precache bool // pod already tracked in the scheduler cache
before the update
allocate bool
cached bool
release bool
}{
- {"add not assigned", nil, pod1, false, false, false},
- {"add assign", nil, pod2, true, true, false},
- {"add terminated", nil, pod3, false, false, false},
- {"update no change", pod1, pod1.DeepCopy(), false, true, false},
- {"update assign", pod1, pod2, true, true, false},
- {"update terminated", pod2, pod3, true, false, true},
+ {"add not assigned", nil, pod1, false, false, false, false},
+ {"add assign", nil, pod2, false, true, true, false},
+ {"add terminated", nil, pod3, false, false, false, false},
+ {"update no change", pod1, pod1.DeepCopy(), true, false, true,
false},
+ // assigned pod re-sent with no resource change: dedup must
skip the core update
+ {"update assigned no change", pod2, pod2.DeepCopy(), true,
false, true, false},
+ {"update assign", pod1, pod2, true, true, true, false},
+ // create delivered as an update, pod not yet in the cache:
must allocate
+ {"update create as assign", pod1, pod1Assigned, false, true,
true, false},
+ {"update terminated", pod2, pod3, true, true, false, true},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
- if tc.oldPod == nil {
- context.schedulerCache.RemovePod(tc.newPod) //
this might log spew...
- } else {
+ if tc.precache {
context.schedulerCache.UpdatePod(tc.oldPod)
+ } else {
+ context.schedulerCache.RemovePod(tc.newPod) //
this might log spew...
}
allocRequest = nil
context.UpdatePod(tc.oldPod, tc.newPod)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
