wilfred-s commented on code in PR #758:
URL: https://github.com/apache/yunikorn-core/pull/758#discussion_r1434727680
##########
pkg/scheduler/ugm/manager.go:
##########
@@ -401,6 +390,27 @@ func (m *Manager) clearEarlierSetLimits(newUserLimits
map[string]map[string]*Lim
m.clearEarlierSetUserLimits(newUserLimits)
}
+// clearEarlierSetUserWildCardLimits Traverse new wild card user config and
decide whether earlier usage needs to be cleared or not
+// by comparing with the existing config. If config set earlier not now, then
traverse all users, check whether wild card limit has been applied/used or not.
+// Reset earlier settings for the users only when wild card user limit has
been applied.
+func (m *Manager) clearEarlierSetUserWildCardLimits(newUserWildCardLimits
map[string]*LimitConfig) {
+ for queuePath := range m.userWildCardLimitsConfig {
+ hierarchy := strings.Split(queuePath, configs.DOT)
+ // Is queue path exists?
+ if _, ok := newUserWildCardLimits[queuePath]; !ok {
+ for u, ut := range m.userTrackers {
+ // Has wild card user limit applied for this
user?
+ if m.HasWildCardApplied(u, hierarchy) {
Review Comment:
Here we find out if it has a wildcard applied and then in line 407 we do the
work again to remove the settings. Why are we not just doing the reset directly?
##########
pkg/scheduler/ugm/group_tracker_test.go:
##########
@@ -32,14 +32,16 @@ func TestGTIncreaseTrackedResource(t *testing.T) {
// root->parent->child1->child12
// root->parent->child2
// root->parent->child12 (similar name like above leaf queue, but it is
being treated differently as similar names are allowed)
- GetUserManager()
+ manager := GetUserManager()
user := &security.UserGroup{User: "test", Groups: []string{"test"}}
groupTracker := newGroupTracker(user.User)
usage1, err := resources.NewResourceFromConf(map[string]string{"mem":
"10M", "vcore": "10"})
if err != nil {
t.Errorf("new resource create returned error or wrong resource:
error %t, res %v", err, usage1)
}
+
Review Comment:
Explain why we need this
##########
pkg/scheduler/ugm/manager.go:
##########
@@ -401,6 +390,27 @@ func (m *Manager) clearEarlierSetLimits(newUserLimits
map[string]map[string]*Lim
m.clearEarlierSetUserLimits(newUserLimits)
}
+// clearEarlierSetUserWildCardLimits Traverse new wild card user config and
decide whether earlier usage needs to be cleared or not
+// by comparing with the existing config. If config set earlier not now, then
traverse all users, check whether wild card limit has been applied/used or not.
+// Reset earlier settings for the users only when wild card user limit has
been applied.
+func (m *Manager) clearEarlierSetUserWildCardLimits(newUserWildCardLimits
map[string]*LimitConfig) {
+ for queuePath := range m.userWildCardLimitsConfig {
+ hierarchy := strings.Split(queuePath, configs.DOT)
Review Comment:
This should be inside the if `newUserWildCardLimits`
##########
pkg/scheduler/ugm/manager.go:
##########
@@ -401,6 +390,27 @@ func (m *Manager) clearEarlierSetLimits(newUserLimits
map[string]map[string]*Lim
m.clearEarlierSetUserLimits(newUserLimits)
}
+// clearEarlierSetUserWildCardLimits Traverse new wild card user config and
decide whether earlier usage needs to be cleared or not
+// by comparing with the existing config. If config set earlier not now, then
traverse all users, check whether wild card limit has been applied/used or not.
+// Reset earlier settings for the users only when wild card user limit has
been applied.
+func (m *Manager) clearEarlierSetUserWildCardLimits(newUserWildCardLimits
map[string]*LimitConfig) {
+ for queuePath := range m.userWildCardLimitsConfig {
+ hierarchy := strings.Split(queuePath, configs.DOT)
+ // Is queue path exists?
+ if _, ok := newUserWildCardLimits[queuePath]; !ok {
+ for u, ut := range m.userTrackers {
+ // Has wild card user limit applied for this
user?
+ if m.HasWildCardApplied(u, hierarchy) {
+ log.Log(log.SchedUGM).Debug("Need to
clear earlier set configs for user because wild card limit has been applied
earlier",
Review Comment:
This should be printed inside `resetUserEarlierUsage()` when we do the work
btw: the name of the function is not correct we do not reset usage we reset
the limit enforced.
##########
pkg/scheduler/ugm/manager.go:
##########
@@ -631,6 +641,21 @@ func (m *Manager) CanRunApp(queuePath, applicationID
string, user security.UserG
return userCanRunApp && groupCanRunApp
}
+func (m *Manager) HasWildCardApplied(user string, hierarchy []string) bool {
Review Comment:
This is only used in the clear case but I think we can fold this into the
clear itself and only traverse the hierarchy once.
I think that would remove `HasWildCardApplied()`, `ut.hasWildCardApplied()`
and `qt.hasWildCardApplied()`
##########
pkg/scheduler/ugm/manager_test.go:
##########
@@ -1425,8 +1493,10 @@ func TestUserGroupLimitChange(t *testing.T) {
//nolint:funlen
conf.Queues[0].Queues[0].Limits = tc.newLimits
assert.NilError(t, manager.UpdateConfig(conf.Queues[0],
"root"))
- increased =
manager.IncreaseTrackedResource(queuePathParent, TestApp2, usage, tc.user)
- assert.Equal(t, increased, false, "should not increase
tracked resource: queuepath "+queuePathParent+", app "+TestApp2+", res
"+usage.String())
+ if manager.CanRunApp(queuePathParent, TestApp2,
tc.user) {
Review Comment:
use assert to check return of `CanRunApp()` and fail test if not good
##########
pkg/scheduler/ugm/queue_tracker.go:
##########
@@ -56,6 +57,7 @@ func newQueueTracker(queuePath string, queueName string)
*QueueTracker {
maxResources: nil,
maxRunningApps: 0,
childQueueTrackers: make(map[string]*QueueTracker),
+ useWildCard: false,
Review Comment:
boolean zero value is false so no need to set it explicitly
##########
pkg/scheduler/ugm/manager_test.go:
##########
@@ -307,28 +304,19 @@ func TestUpdateConfig(t *testing.T) {
}
}
- userTrackers := manager.GetUsersResources()
- userTracker := userTrackers[0]
- groupTrackers := manager.GetGroupsResources()
- groupTracker := groupTrackers[0]
- assert.Equal(t, false, manager.isUserRemovable(userTracker))
- assert.Equal(t, false, manager.isGroupRemovable(groupTracker))
-
// configure max resource for root.parent lesser than current resource
usage. should be allowed to set but user cannot be allowed to do any activity
further
conf = createConfig(user.User, user.Groups[0], "memory", "50", 40, 4)
err = manager.UpdateConfig(conf.Queues[0], "root")
assert.NilError(t, err)
- increased := manager.IncreaseTrackedResource(queuePath1, TestApp1,
usage, user)
- if increased {
- t.Fatalf("unable to increase tracked resource: queuepath %s,
app %s, res %v", queuePath1, TestApp1, user)
- }
+ headroom := manager.Headroom(queuePath1, TestApp1, user)
+ assert.Equal(t, headroom.FitInMaxUndef(usage), false)
Review Comment:
use `assert.Assert()` for boolean checks (multiple instances found)
##########
pkg/scheduler/ugm/queue_tracker.go:
##########
@@ -448,8 +400,19 @@ func (qt *QueueTracker) canRunApp(hierarchy []string,
applicationID string, trac
} else if trackType == group {
config = m.getGroupWildCardLimitsConfig(qt.queuePath)
}
- if config != nil && config.maxApplications != 0 && running >
int(config.maxApplications) {
- return false
+ if config != nil && config.maxApplications != 0 {
Review Comment:
See above comment we should not do this as part of the headroom check. We do
it once as part of the queue tracker create and then depend on the config
update to maintain it
##########
pkg/scheduler/ugm/manager_test.go:
##########
@@ -1116,9 +1183,10 @@ func TestUserGroupLimit(t *testing.T) { //nolint:funlen
assert.NilError(t, err, fmt.Sprintf("can't create
resource from %v", tc.finalExpectedHeadroomResource))
headroom = manager.Headroom(queuePathParent, TestApp1,
tc.user)
assert.Equal(t, resources.Equals(headroom,
finalExpectedHeadroom), true, "final headroom is not expected")
-
- increased =
manager.IncreaseTrackedResource(queuePathParent, TestApp2, usage, tc.user)
- assert.Equal(t, increased, false, "should not increase
tracked resource: queuepath "+queuePathParent+", app "+TestApp2+", res
"+usage.String())
+ if manager.CanRunApp(queuePathParent, TestApp2,
tc.user) {
Review Comment:
use assert to check return of `CanRunApp()` and fail test if not good
##########
pkg/scheduler/ugm/manager_test.go:
##########
@@ -337,10 +325,93 @@ func TestUpdateConfig(t *testing.T) {
conf = createConfig(user.User, user.Groups[0], "memory", "50", 10, 10)
err = manager.UpdateConfig(conf.Queues[0], "root")
assert.NilError(t, err)
- increased = manager.IncreaseTrackedResource(queuePath1, TestApp1,
usage, user)
- if increased {
- t.Fatalf("unable to increase tracked resource: queuepath %s,
app %s, res %v", queuePath1, TestApp1, user)
+ headroom = manager.Headroom(queuePath1, TestApp1, user)
+ assert.Equal(t, headroom.FitInMaxUndef(usage), false)
+}
+
+func TestUseWildCard(t *testing.T) {
+ setupUGM()
+ manager := GetUserManager()
+ user := security.UserGroup{User: "user1", Groups: []string{"group1"}}
+
+ expectedResource, err :=
resources.NewResourceFromConf(map[string]string{"memory": "50", "vcores": "50"})
+ if err != nil {
Review Comment:
use `assert.NilError()` to check for nil error returns (multiple instances
found)
##########
pkg/scheduler/ugm/queue_tracker.go:
##########
@@ -266,6 +209,15 @@ func (qt *QueueTracker) headroom(hierarchy []string,
trackType trackingType) *re
if trackType == user {
if config :=
m.getUserWildCardLimitsConfig(qt.queuePath); config != nil {
headroom = config.maxResources.Clone()
+ log.Log(log.SchedUGM).Debug("Use wild card
limit settings as there is no limit set explicitly",
Review Comment:
Wildcard use implicitly means no limit set. No need to add that to the
message.
We also should only need this check as part of the create of a new queue
tracker. After that the config change should be setting or removing the value.
We should not need it at all at this point.
--
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]
