This is an automated email from the ASF dual-hosted git repository. chia7712 pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/yunikorn-k8shim.git
The following commit(s) were added to refs/heads/master by this push: new ba192d4f [YUNIKORN-2782] Cleanup dead code in cache/context (#888) ba192d4f is described below commit ba192d4f7ae782424a8df6661784dad425966b7a Author: Tzu-Hua Lan <blue.tzu...@gmail.com> AuthorDate: Wed Aug 7 16:46:52 2024 +0800 [YUNIKORN-2782] Cleanup dead code in cache/context (#888) Closes: #888 Signed-off-by: Chia-Ping Tsai <chia7...@gmail.com> --- pkg/cache/context.go | 35 +------------------ pkg/cache/context_test.go | 75 ++--------------------------------------- pkg/cache/scheduler_callback.go | 2 +- pkg/shim/scheduler.go | 2 +- 4 files changed, 6 insertions(+), 108 deletions(-) diff --git a/pkg/cache/context.go b/pkg/cache/context.go index 1751ec89..211bf0e9 100644 --- a/pkg/cache/context.go +++ b/pkg/cache/context.go @@ -845,12 +845,6 @@ func (ctx *Context) ForgetPod(name string) { log.Log(log.ShimContext).Debug("unable to forget pod: not found in cache", zap.String("pod", name)) } -func (ctx *Context) UpdateApplication(app *Application) { - ctx.lock.Lock() - defer ctx.lock.Unlock() - ctx.applications[app.applicationID] = app -} - // IsTaskMaybeSchedulable returns true if a task might be currently able to be scheduled. This uses a bloom filter // cached from a set of taskIDs to perform efficient negative lookups. func (ctx *Context) IsTaskMaybeSchedulable(taskID string) bool { @@ -1024,34 +1018,7 @@ func (ctx *Context) getApplication(appID string) *Application { return nil } -func (ctx *Context) RemoveApplication(appID string) error { - ctx.lock.Lock() - if app, exist := ctx.applications[appID]; exist { - // get the non-terminated task alias - nonTerminatedTaskAlias := app.getNonTerminatedTaskAlias() - // check there are any non-terminated task or not - if len(nonTerminatedTaskAlias) > 0 { - ctx.lock.Unlock() - return fmt.Errorf("failed to remove application %s because it still has task in non-terminated tasks: %s", appID, strings.Join(nonTerminatedTaskAlias, ",")) - } - delete(ctx.applications, appID) - ctx.lock.Unlock() - // send the update request to scheduler core - rr := common.CreateUpdateRequestForRemoveApplication(app.applicationID, app.partition) - if err := ctx.apiProvider.GetAPIs().SchedulerAPI.UpdateApplication(rr); err != nil { - log.Log(log.ShimContext).Error("failed to send remove application request to core", zap.Error(err)) - } - - log.Log(log.ShimContext).Info("app removed", - zap.String("appID", appID)) - - return nil - } - ctx.lock.Unlock() - return fmt.Errorf("application %s is not found in the context", appID) -} - -func (ctx *Context) RemoveApplicationInternal(appID string) { +func (ctx *Context) RemoveApplication(appID string) { ctx.lock.Lock() defer ctx.lock.Unlock() if _, exist := ctx.applications[appID]; !exist { diff --git a/pkg/cache/context_test.go b/pkg/cache/context_test.go index 717c4eb0..ba661c39 100644 --- a/pkg/cache/context_test.go +++ b/pkg/cache/context_test.go @@ -324,75 +324,6 @@ func TestGetApplication(t *testing.T) { } func TestRemoveApplication(t *testing.T) { - // add 3 applications - context := initContextForTest() - app1 := NewApplication(appID1, queueNameA, testUser, testGroups, map[string]string{}, newMockSchedulerAPI()) - app2 := NewApplication(appID2, queueNameB, testUser, testGroups, map[string]string{}, newMockSchedulerAPI()) - app3 := NewApplication(appID3, queueNameC, testUser, testGroups, map[string]string{}, newMockSchedulerAPI()) - context.applications[appID1] = app1 - context.applications[appID2] = app2 - context.applications[appID3] = app3 - pod1 := &v1.Pod{ - TypeMeta: apis.TypeMeta{ - Kind: "Pod", - APIVersion: "v1", - }, - ObjectMeta: apis.ObjectMeta{ - Name: "remove-test-00001", - UID: uid1, - }, - } - pod2 := &v1.Pod{ - TypeMeta: apis.TypeMeta{ - Kind: "Pod", - APIVersion: "v1", - }, - ObjectMeta: apis.ObjectMeta{ - Name: "remove-test-00002", - UID: uid2, - }, - } - // New task to application 1 - // set task state in Pending (non-terminated) - task1 := NewTask(taskUID1, app1, context, pod1) - app1.taskMap[taskUID1] = task1 - task1.sm.SetState(TaskStates().Pending) - // New task to application 2 - // set task state in Failed (terminated) - task2 := NewTask(taskUID2, app2, context, pod2) - app2.taskMap[taskUID2] = task2 - task2.sm.SetState(TaskStates().Failed) - - // remove application 1 which have non-terminated task - // this should fail - assert.Equal(t, len(context.applications), 3) - err := context.RemoveApplication(appID1) - assert.Assert(t, err != nil) - assert.ErrorContains(t, err, "application app00001 because it still has task in non-terminated tasks: /remove-test-00001") - - app := context.GetApplication(appID1) - assert.Assert(t, app != nil) - - // remove application 2 which have terminated task - // this should be successful - err = context.RemoveApplication(appID2) - assert.Assert(t, err == nil) - - app = context.GetApplication(appID2) - assert.Assert(t, app == nil) - - // try remove again - // this should fail - err = context.RemoveApplication(appID2) - assert.Assert(t, err != nil) - assert.ErrorContains(t, err, "application app00002 is not found in the context") - - // make sure the other app is not affected - app = context.GetApplication(appID3) - assert.Assert(t, app != nil) -} - -func TestRemoveApplicationInternal(t *testing.T) { context := initContextForTest() app1 := NewApplication(appID1, queueNameA, testUser, testGroups, map[string]string{}, newMockSchedulerAPI()) app2 := NewApplication(appID2, queueNameB, testUser, testGroups, map[string]string{}, newMockSchedulerAPI()) @@ -401,17 +332,17 @@ func TestRemoveApplicationInternal(t *testing.T) { assert.Equal(t, len(context.applications), 2) // remove non-exist app - context.RemoveApplicationInternal(appID3) + context.RemoveApplication(appID3) assert.Equal(t, len(context.applications), 2) // remove app1 - context.RemoveApplicationInternal(appID1) + context.RemoveApplication(appID1) assert.Equal(t, len(context.applications), 1) _, ok := context.applications[appID1] assert.Equal(t, ok, false) // remove app2 - context.RemoveApplicationInternal(appID2) + context.RemoveApplication(appID2) assert.Equal(t, len(context.applications), 0) _, ok = context.applications[appID2] assert.Equal(t, ok, false) diff --git a/pkg/cache/scheduler_callback.go b/pkg/cache/scheduler_callback.go index 8ed487c4..728212bf 100644 --- a/pkg/cache/scheduler_callback.go +++ b/pkg/cache/scheduler_callback.go @@ -158,7 +158,7 @@ func (callback *AsyncRMCallback) UpdateApplication(response *si.ApplicationRespo zap.String("new status", updated.State)) switch updated.State { case ApplicationStates().Completed: - callback.context.RemoveApplicationInternal(updated.ApplicationID) + callback.context.RemoveApplication(updated.ApplicationID) case ApplicationStates().Resuming: app := callback.context.GetApplication(updated.ApplicationID) if app != nil && app.GetApplicationState() == ApplicationStates().Reserving { diff --git a/pkg/shim/scheduler.go b/pkg/shim/scheduler.go index 3e78d7ff..ebf3fb11 100644 --- a/pkg/shim/scheduler.go +++ b/pkg/shim/scheduler.go @@ -160,7 +160,7 @@ func (ss *KubernetesShim) schedule() { for _, app := range apps { if app.GetApplicationState() == cache.ApplicationStates().Failed { if app.AreAllTasksTerminated() { - ss.context.RemoveApplicationInternal(app.GetApplicationID()) + ss.context.RemoveApplication(app.GetApplicationID()) } continue } --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@yunikorn.apache.org For additional commands, e-mail: issues-h...@yunikorn.apache.org