[GitHub] [yunikorn-core] FrankYang0529 commented on a diff in pull request #600: [YUNIKORN-1858] Handle group resource usage properly during config ch…

["FrankYang0529 (via GitHub)" <gi...@apache.org>]

FrankYang0529 commented on code in PR #600:
URL: https://github.com/apache/yunikorn-core/pull/600#discussion_r1286898707


##########
pkg/scheduler/ugm/manager.go:
##########
@@ -421,32 +425,25 @@ func (m *Manager) clearEarlierSetLimits(userLimits map[string]bool, groupLimits
 							zap.String("group", gt.groupName),
 							zap.String("application id", appID),
 							zap.String("queue path", queuePath))
-						// removing the linkage only happens here by setting it to nil and deleting app id
-						// but group resource usage so far remains as it is because we don't have app id wise resource usage with in group as of now.
-						// YUNIKORN-1858 handles the group resource usage properly
-						// In case of only one (last) application, group tracker would be removed from the manager.
+						// simply remove the linkage between the user and group by setting gt to nil for the given app.
+						// but in case of group, specific app resource usage has to be decremented
 						ut.setGroupForApp(appID, nil)
-						gt.removeApp(appID)
-						if len(gt.getTrackedApplications()) == 0 {
-							log.Log(log.SchedUGM).Debug("Is this app the only running application in group?",
-								zap.String("user", u),
-								zap.String("group", gt.groupName),
-								zap.Int("no. of applications", len(gt.getTrackedApplications())),
-								zap.String("application id", appID),
-								zap.String("queue path", queuePath))
-							delete(m.groupTrackers, g)
+						removeQT, decreased := gt.decreaseTrackedResourceUsage(appID, true)
+						if decreased {
+							if removeQT {
+								log.Log(log.SchedUGM).Debug("Removing group from manager",
+									zap.String("group", gt.groupName),
+									zap.String("queue path", queuePath),
+									zap.String("application", appID))
+								delete(m.groupTrackers, gt.groupName)
+							}

Review Comment:
   I tried to apply the change to #595, and the following test case couldn't pass. I think we may also check `m.userWildCardLimitsConfig` or `m.groupWildCardLimitsConfig` before removing trackers.
   
   ```go
   func TestMultipleGroupLimitChange(t *testing.T) {
   	setupUGM()
   
   	manager := GetUserManager()
   	conf := createConfigWithLimits([]configs.Limit{
   		createLimit(nil, []string{"group1", "group2"}, largeResource, 2),
   		createLimit(nil, []string{"*"}, mediumResource, 1),
   	})
   	assert.NilError(t, manager.UpdateConfig(conf.Queues[0], "root"))
   
   	user1 := security.UserGroup{User: "user1", Groups: []string{"group1"}}
   	user2 := security.UserGroup{User: "user2", Groups: []string{"group2"}}
   	user3 := security.UserGroup{User: "user3", Groups: []string{"group3"}}
   
   	usage, err := resources.NewResourceFromConf(mediumResource)
   	if err != nil {
   		t.Errorf("new resource create returned error or wrong resource: error %t, res %v", err, usage)
   	}
   
   	// all users can increate usage within the quota
   	increased := manager.IncreaseTrackedResource(queuePathParent, "test-app-1-1", usage, user1)
   	assert.Equal(t, increased, true, "unable to increase tracked resource: queuepath "+queuePathParent+", app test-app-1-1, res "+usage.String())
   
   	increased = manager.IncreaseTrackedResource(queuePathParent, "test-app-2-1", usage, user2)
   	assert.Equal(t, increased, true, "unable to increase tracked resource: queuepath "+queuePathParent+", app test-app-2-1, res "+usage.String())
   
   	increased = manager.IncreaseTrackedResource(queuePathParent, "test-app-3-1", usage, user3)
   	assert.Equal(t, increased, true, "unable to increase tracked resource: queuepath "+queuePathParent+", app test-app-3-1, res "+usage.String())
   
   	// remove group2 from the specific group
   	conf.Queues[0].Queues[0].Limits[0].Groups = []string{"group1"}
   	assert.NilError(t, manager.UpdateConfig(conf.Queues[0], "root"))
   
   	// user1 still can increase usage within the quota
   	increased = manager.IncreaseTrackedResource(queuePathParent, "test-app-1-2", usage, user1)
   	assert.Equal(t, increased, true, "unable to increase tracked resource: queuepath "+queuePathParent+", app test-app-1-2, res "+usage.String())
   
   	// user2 can't increase usage more than wildcard limit
   	increased = manager.IncreaseTrackedResource(queuePathParent, "test-app-2-2", usage, user2)
   	assert.Equal(t, increased, false, "should not increase tracked resource: queuepath "+queuePathParent+", app test-app-2-2, res "+usage.String())
   
   	// user3 can't increase usage more than wildcard limit
   	increased = manager.IncreaseTrackedResource(queuePathParent, "test-app-3-2", usage, user3)
   	assert.Equal(t, increased, false, "should not increase tracked resource: queuepath "+queuePathParent+", app test-app-3-2, res "+usage.String())
   }
   ```



-- 
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: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org