You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@yunikorn.apache.org by ch...@apache.org on 2024/08/07 08:47:28 UTC
(yunikorn-k8shim) branch master updated: [YUNIKORN-2782] Cleanup dead code in cache/context (#888)
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 <bl...@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 <ch...@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-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: issues-help@yunikorn.apache.org