You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@yunikorn.apache.org by pb...@apache.org on 2024/06/07 13:35:49 UTC
(yunikorn-k8shim) branch branch-1.5 updated: [YUNIKORN-2637] finalizePods should ignore pods like registerPods (#847)
This is an automated email from the ASF dual-hosted git repository.
pbacsko pushed a commit to branch branch-1.5
in repository https://gitbox.apache.org/repos/asf/yunikorn-k8shim.git
The following commit(s) were added to refs/heads/branch-1.5 by this push:
new 16b0f5cf [YUNIKORN-2637] finalizePods should ignore pods like registerPods (#847)
16b0f5cf is described below
commit 16b0f5cf1ff7325da24383324e610535f6f6fcb3
Author: Wilfred Spiegelenburg <wi...@apache.org>
AuthorDate: Fri Jun 7 15:24:20 2024 +0200
[YUNIKORN-2637] finalizePods should ignore pods like registerPods (#847)
If a pod was in a terminal state during registration it is skipped. The
same principal should apply to finalisePods:
* only check pods that were added in registerPods.
* remove finished pods in the finalizePods call if they were registered.
* new pods are not added in finalizePods
Closes: #847
Signed-off-by: Peter Bacsko <pb...@cloudera.com>
(cherry picked from commit 8e680afee341c57e4b33a5432c4abd186946bb9e)
---
pkg/cache/context.go | 20 +++++++++----
pkg/cache/context_test.go | 73 +++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 88 insertions(+), 5 deletions(-)
diff --git a/pkg/cache/context.go b/pkg/cache/context.go
index 199ab128..3f162856 100644
--- a/pkg/cache/context.go
+++ b/pkg/cache/context.go
@@ -1691,7 +1691,7 @@ func (ctx *Context) finalizeNodes(existingNodes []*v1.Node) error {
}
func (ctx *Context) registerPods() ([]*v1.Pod, error) {
- log.Log(log.ShimContext).Info("Starting node registration...")
+ log.Log(log.ShimContext).Info("Starting pod registration...")
// list all pods via the informer
pods, err := ctx.apiProvider.GetAPIs().PodInformer.Lister().List(labels.Everything())
@@ -1706,14 +1706,16 @@ func (ctx *Context) registerPods() ([]*v1.Pod, error) {
})
// add all pods to the context
- for _, pod := range pods {
- // skip terminated pods
+ for i, pod := range pods {
+ // skip terminated pods: we do not add or finalise them later
if utils.IsPodTerminated(pod) {
+ pods[i] = nil
continue
}
ctx.AddPod(pod)
}
+ log.Log(log.ShimContext).Info("Finished pod registration...")
return pods, nil
}
@@ -1728,13 +1730,21 @@ func (ctx *Context) finalizePods(existingPods []*v1.Pod) error {
// convert the pod list into a map
podMap := make(map[types.UID]*v1.Pod)
for _, pod := range pods {
+ // if the pod is terminated finalising should remove it if it was running in register
+ if utils.IsPodTerminated(pod) {
+ continue
+ }
podMap[pod.UID] = pod
}
- // find any existing nodes that no longer exist
+ // find any existing pods that no longer exist
for _, pod := range existingPods {
+ // skip if the pod was already terminated during register
+ if pod == nil {
+ continue
+ }
if _, ok := podMap[pod.UID]; !ok {
- // node no longer exists, delete it
+ // pod no longer exists, delete it
log.Log(log.ShimContext).Info("Removing pod which went away during initialization",
zap.String("namespace", pod.Namespace),
zap.String("name", pod.Name),
diff --git a/pkg/cache/context_test.go b/pkg/cache/context_test.go
index d67c3b82..7b277cbb 100644
--- a/pkg/cache/context_test.go
+++ b/pkg/cache/context_test.go
@@ -2336,3 +2336,76 @@ func foreignPod(podName, memory, cpu string) *v1.Pod {
},
}
}
+
+func TestRegisterPods(t *testing.T) {
+ context := initContextForTest()
+
+ pods, err := context.registerPods()
+ assert.NilError(t, err, "register pods with empty setup should not fail")
+ assert.Equal(t, len(pods), 0, "should have returned an empty pod list")
+
+ var api *client.MockedAPIProvider
+ switch v := context.apiProvider.(type) {
+ case *client.MockedAPIProvider:
+ api = v
+ default:
+ t.Fatalf("api type not recognized")
+ }
+ pod1 := newPodHelper("yunikorn-test-00001", namespace, "UID-00001", "node-1", "yunikorn-test-00001", v1.PodRunning)
+ pod2 := newPodHelper("yunikorn-test-00002", namespace, "UID-00002", "node-1", "yunikorn-test-00002", v1.PodRunning)
+ pod3 := newPodHelper("yunikorn-test-00003", namespace, "UID-00003", "node-1", "yunikorn-test-00003", v1.PodSucceeded)
+ pod4 := newPodHelper("yunikorn-test-00004", namespace, "UID-00004", "node-1", "yunikorn-test-00004", v1.PodRunning)
+
+ api.GetPodListerMock().AddPod(pod1)
+ api.GetPodListerMock().AddPod(pod2)
+ api.GetPodListerMock().AddPod(pod3)
+ api.GetPodListerMock().AddPod(pod4)
+
+ pods, err = context.registerPods()
+ assert.NilError(t, err, "register pods should not have failed")
+ assert.Assert(t, assertListerPods(pods, 3), "should have returned 3 running pods in the list")
+
+ pod1Cache, _ := context.schedulerCache.GetPod(string(pod1.UID))
+ assert.Assert(t, pod1Cache != nil, "expected to find pod 1 in cache")
+ pod2Cache, _ := context.schedulerCache.GetPod(string(pod2.UID))
+ assert.Assert(t, pod2Cache != nil, "expected to find pod 2 in cache")
+ pod3Cache, _ := context.schedulerCache.GetPod(string(pod3.UID))
+ assert.Assert(t, pod3Cache == nil, "not expected to find pod 3 in cache")
+ pod4Cache, _ := context.schedulerCache.GetPod(string(pod4.UID))
+ assert.Assert(t, pod4Cache != nil, "expected to find pod 4 in cache")
+
+ // prep for finalising the pods
+ // new pod added (should be ignored)
+ pod5 := newPodHelper("yunikorn-test-00005", namespace, "UID-00005", "node-1", "yunikorn-test-00005", v1.PodRunning)
+ api.GetPodListerMock().AddPod(pod5)
+ // update pod 1 is now marked as terminated (will be removed)
+ pod1.Status = v1.PodStatus{
+ Phase: v1.PodSucceeded,
+ }
+ api.GetPodListerMock().AddPod(pod1)
+ // remove pod 4 as if it was removed from K8s (will be removed)
+ api.GetPodListerMock().DeletePod(pod4)
+
+ err = context.finalizePods(pods)
+ assert.NilError(t, err, "finalize pods should not have failed")
+ pod1Cache, _ = context.schedulerCache.GetPod(string(pod1.UID))
+ assert.Assert(t, pod1Cache == nil, "not expected to find pod 1 in cache")
+ pod2Cache, _ = context.schedulerCache.GetPod(string(pod2.UID))
+ assert.Assert(t, pod2Cache != nil, "expected to find pod 2 in cache")
+ pod3Cache, _ = context.schedulerCache.GetPod(string(pod3.UID))
+ assert.Assert(t, pod3Cache == nil, "not expected to find pod 3 in cache")
+ pod4Cache, _ = context.schedulerCache.GetPod(string(pod4.UID))
+ assert.Assert(t, pod4Cache == nil, "not expected to find pod 4 in cache")
+ pod5Cache, _ := context.schedulerCache.GetPod(string(pod5.UID))
+ assert.Assert(t, pod5Cache == nil, "not expected to find pod 5 in cache")
+}
+
+func assertListerPods(pods []*v1.Pod, count int) bool {
+ counted := 0
+ for _, pod := range pods {
+ if pod != nil {
+ counted++
+ }
+ }
+ return count == counted
+}
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: issues-help@yunikorn.apache.org