[YUNIKORN-2907] Avoid extraneous logging during queue config processing (#1009)

Closes: #1009

Signed-off-by: Craig Condit <[email protected]>

Author: Michael

pkg/common/security/acl.go

@@ -46,12 +46,15 @@ func (a *ACL) setAllAllowed(part string) {
 	a.allAllowed = part == common.Wildcard
 }
 
-// set the user list in the ACL, invalid user names are ignored
-func (a *ACL) setUsers(userList []string) {
+// set the user list in the ACL, invalid user names are ignored.
+// If the silence flag is set to true, the function will not log when setting the users.
+func (a *ACL) setUsers(userList []string, silence bool) {
 	a.users = make(map[string]bool)
 	// special case if the user list is just the wildcard
 	if len(userList) == 1 && userList[0] == common.Wildcard {
-		log.Log(log.Security).Info("user list is wildcard, allowing all access")
+		if !silence {
+			log.Log(log.Security).Info("user list is wildcard, allowing all access")
+		}
 		a.allAllowed = true
 		return
 	}
@@ -64,23 +67,28 @@ func (a *ACL) setUsers(userList []string) {
 		// check the users validity
 		if userNameRegExp.MatchString(user) {
 			a.users[user] = true
-		} else {
+		} else if !silence {
 			log.Log(log.Security).Info("ignoring user in ACL definition",
 				zap.String("user", user))
 		}
 	}
 }
 
 // set the group list in the ACL, invalid group names are ignored
-func (a *ACL) setGroups(groupList []string) {
+// If the silence flag is set to true, the function will not log when setting the groups.
+func (a *ACL) setGroups(groupList []string, silence bool) {
 	a.groups = make(map[string]bool)
 	// special case if the wildcard was already set
 	if a.allAllowed {
-		log.Log(log.Security).Info("ignoring group list in ACL: wildcard set")
+		if !silence {
+			log.Log(log.Security).Info("ignoring group list in ACL: wildcard set")
+		}
 		return
 	}
 	if len(groupList) == 1 && groupList[0] == common.Wildcard {
-		log.Log(log.Security).Info("group list is wildcard, allowing all access")
+		if !silence {
+			log.Log(log.Security).Info("group list is wildcard, allowing all access")
+		}
 		a.users = make(map[string]bool)
 		a.allAllowed = true
 		return
@@ -94,15 +102,15 @@ func (a *ACL) setGroups(groupList []string) {
 		// check the group validity
 		if groupRegExp.MatchString(group) {
 			a.groups[group] = true
-		} else {
+		} else if !silence {
 			log.Log(log.Security).Info("ignoring group in ACL",
 				zap.String("group", group))
 		}
 	}
 }
 
 // create a new ACL from scratch
-func NewACL(aclStr string) (ACL, error) {
+func NewACL(aclStr string, silence bool) (ACL, error) {
 	acl := ACL{}
 	if aclStr == "" {
 		return acl, nil
@@ -116,9 +124,9 @@ func NewACL(aclStr string) (ACL, error) {
 	// trim and check for wildcard
 	acl.setAllAllowed(aclStr)
 	// parse users and groups
-	acl.setUsers(strings.Split(fields[0], common.Separator))
+	acl.setUsers(strings.Split(fields[0], common.Separator), silence)
 	if len(fields) == 2 {
-		acl.setGroups(strings.Split(fields[1], common.Separator))
+		acl.setGroups(strings.Split(fields[1], common.Separator), silence)
 	}
 	return acl, nil
 }

pkg/common/security/acl_test.go

@@ -155,7 +155,7 @@ func TestACLCreate(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.input, func(t *testing.T) {
-			got, err := NewACL(tt.input)
+			got, err := NewACL(tt.input, false)
 			if err != nil {
 				t.Errorf("parsing failed for string: %s", tt.input)
 			}
@@ -177,7 +177,7 @@ func TestNewACLErrorCase(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.caseName, func(t *testing.T) {
-			if _, err := NewACL(tt.acl); err == nil {
+			if _, err := NewACL(tt.acl, false); err == nil {
 				t.Errorf("parsing %s string should be failed", tt.acl)
 			}
 		})
@@ -238,7 +238,7 @@ func TestACLAccess(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(fmt.Sprintf("vistor %v, acl %s", tt.visitor, tt.acl), func(t *testing.T) {
-			acl, err := NewACL(tt.acl)
+			acl, err := NewACL(tt.acl, false)
 			if err != nil {
 				t.Error("the number of space should not be more than 2 because the number of categories only include users and groups")
 			}

pkg/scheduler/context.go

@@ -365,7 +365,7 @@ func (cc *ClusterContext) updateSchedulerConfig(conf *configs.SchedulerConfig, r
 		part, ok := cc.partitions[p.Name]
 		if ok {
 			// make sure the new info passes all checks
-			_, err = newPartitionContext(p, rmID, nil)
+			_, err = newPartitionContext(p, rmID, nil, true)
 			if err != nil {
 				return err
 			}
@@ -379,7 +379,7 @@ func (cc *ClusterContext) updateSchedulerConfig(conf *configs.SchedulerConfig, r
 			// not found: new partition, no checks needed
 			log.Log(log.SchedContext).Info("added partitions", zap.String("partitionName", partitionName))
 
-			part, err = newPartitionContext(p, rmID, cc)
+			part, err = newPartitionContext(p, rmID, cc, false)
 			if err != nil {
 				return err
 			}

pkg/scheduler/context_test.go

@@ -86,7 +86,7 @@ func createTestContext(t *testing.T, partitionName string) *ClusterContext {
 			},
 		},
 	}
-	partition, err := newPartitionContext(conf, "test", context)
+	partition, err := newPartitionContext(conf, "test", context, false)
 	assert.NilError(t, err, "partition create should not have failed with error")
 	context.partitions[partition.Name] = partition
 	return context

pkg/scheduler/objects/queue.go

@@ -114,8 +114,9 @@ func newBlankQueue() *Queue {
 }
 
 // NewConfiguredQueue creates a new queue from scratch based on the configuration
-// lock free as it cannot be referenced yet
-func NewConfiguredQueue(conf configs.QueueConfig, parent *Queue) (*Queue, error) {
+// lock free as it cannot be referenced yet.
+// If the silence flag is set to true, the function will neither log the queue creation nor send a queue event.
+func NewConfiguredQueue(conf configs.QueueConfig, parent *Queue, silence bool) (*Queue, error) {
 	sq := newBlankQueue()
 	sq.Name = strings.ToLower(conf.Name)
 	sq.QueuePath = strings.ToLower(conf.Name)
@@ -128,7 +129,7 @@ func NewConfiguredQueue(conf configs.QueueConfig, parent *Queue) (*Queue, error)
 	sq.updateMaxRunningAppsMetrics()
 
 	// update the properties
-	if err := sq.applyConf(conf); err != nil {
+	if err := sq.applyConf(conf, silence); err != nil {
 		return nil, errors.Join(errors.New("configured queue creation failed: "), err)
 	}
 
@@ -145,10 +146,13 @@ func NewConfiguredQueue(conf configs.QueueConfig, parent *Queue) (*Queue, error)
 	} else {
 		sq.UpdateQueueProperties()
 	}
-	sq.queueEvents = schedEvt.NewQueueEvents(events.GetEventSystem())
-	log.Log(log.SchedQueue).Info("configured queue added to scheduler",
-		zap.String("queueName", sq.QueuePath))
-	sq.queueEvents.SendNewQueueEvent(sq.QueuePath, sq.isManaged)
+
+	if !silence {
+		sq.queueEvents = schedEvt.NewQueueEvents(events.GetEventSystem())
+		log.Log(log.SchedQueue).Info("configured queue added to scheduler",
+			zap.String("queueName", sq.QueuePath))
+		sq.queueEvents.SendNewQueueEvent(sq.QueuePath, sq.isManaged)
+	}
 
 	return sq, nil
 }
@@ -310,21 +314,22 @@ func filterParentProperty(key string, value string) string {
 func (sq *Queue) ApplyConf(conf configs.QueueConfig) error {
 	sq.Lock()
 	defer sq.Unlock()
-	return sq.applyConf(conf)
+	return sq.applyConf(conf, false)
 }
 
 // applyConf applies all the properties to the queue from the config.
-// lock free call, must be called holding the queue lock or during create only
-func (sq *Queue) applyConf(conf configs.QueueConfig) error {
+// lock free call, must be called holding the queue lock or during create only.
+// If the silence flag is set to true, the function will not log when setting users and groups.
+func (sq *Queue) applyConf(conf configs.QueueConfig, silence bool) error {
 	// Set the ACLs
 	var err error
-	sq.submitACL, err = security.NewACL(conf.SubmitACL)
+	sq.submitACL, err = security.NewACL(conf.SubmitACL, silence)
 	if err != nil {
 		log.Log(log.SchedQueue).Error("parsing submit ACL failed this should not happen",
 			zap.Error(err))
 		return err
 	}
-	sq.adminACL, err = security.NewACL(conf.AdminACL)
+	sq.adminACL, err = security.NewACL(conf.AdminACL, silence)
 	if err != nil {
 		log.Log(log.SchedQueue).Error("parsing admin ACL failed this should not happen",
 			zap.Error(err))

pkg/scheduler/objects/queue_test.go

@@ -2267,7 +2267,7 @@ func TestNewConfiguredQueue(t *testing.T) {
 			},
 		},
 	}
-	parent, err := NewConfiguredQueue(parentConfig, nil)
+	parent, err := NewConfiguredQueue(parentConfig, nil, false)
 	assert.NilError(t, err, "failed to create queue: %v", err)
 	assert.Equal(t, parent.Name, "parent_queue")
 	assert.Equal(t, parent.QueuePath, "parent_queue")
@@ -2287,7 +2287,7 @@ func TestNewConfiguredQueue(t *testing.T) {
 			Guaranteed: getResourceConf(),
 		},
 	}
-	childLeaf, err := NewConfiguredQueue(leafConfig, parent)
+	childLeaf, err := NewConfiguredQueue(leafConfig, parent, false)
 	assert.NilError(t, err, "failed to create queue: %v", err)
 	assert.Equal(t, childLeaf.QueuePath, "parent_queue.leaf_queue")
 	assert.Assert(t, childLeaf.template == nil)
@@ -2304,13 +2304,26 @@ func TestNewConfiguredQueue(t *testing.T) {
 		Name:   "nonleaf_queue",
 		Parent: true,
 	}
-	childNonLeaf, err := NewConfiguredQueue(NonLeafConfig, parent)
+	childNonLeaf, err := NewConfiguredQueue(NonLeafConfig, parent, false)
 	assert.NilError(t, err, "failed to create queue: %v", err)
 	assert.Equal(t, childNonLeaf.QueuePath, "parent_queue.nonleaf_queue")
 	assert.Assert(t, reflect.DeepEqual(childNonLeaf.template, parent.template))
 	assert.Equal(t, len(childNonLeaf.properties), 0)
 	assert.Assert(t, childNonLeaf.guaranteedResource == nil)
 	assert.Assert(t, childNonLeaf.maxResource == nil)
+
+	// case 2: do not send queue event when silence flag is set to true
+	events.Init()
+	eventSystem := events.GetEventSystem().(*events.EventSystemImpl) //nolint:errcheck
+	eventSystem.StartServiceWithPublisher(false)
+	rootConfig := configs.QueueConfig{
+		Name: "root",
+	}
+	_, err = NewConfiguredQueue(rootConfig, nil, true)
+	assert.NilError(t, err, "failed to create queue: %v", err)
+	time.Sleep(time.Second)
+	noEvents := eventSystem.Store.CountStoredEvents()
+	assert.Equal(t, noEvents, uint64(0), "expected 0 event, got %d", noEvents)
 }
 
 func TestResetRunningState(t *testing.T) {
@@ -2333,11 +2346,11 @@ func TestResetRunningState(t *testing.T) {
 	parent.MarkQueueForRemoval()
 	assert.Assert(t, parent.IsDraining(), "parent should be marked as draining")
 	assert.Assert(t, leaf.IsDraining(), "leaf should be marked as draining")
-	err = parent.applyConf(emptyConf)
+	err = parent.applyConf(emptyConf, false)
 	assert.NilError(t, err, "failed to update parent")
 	assert.Assert(t, parent.IsRunning(), "parent should be running again")
 	assert.Assert(t, leaf.IsDraining(), "leaf should still be marked as draining")
-	err = leaf.applyConf(emptyConf)
+	err = leaf.applyConf(emptyConf, false)
 	assert.NilError(t, err, "failed to update leaf")
 	assert.Assert(t, leaf.IsRunning(), "leaf should be running again")
 }
@@ -2360,7 +2373,7 @@ func TestNewRecoveryQueue(t *testing.T) {
 		Properties:    map[string]string{configs.ApplicationSortPolicy: "fair"},
 		ChildTemplate: configs.ChildTemplate{Properties: map[string]string{configs.ApplicationSortPolicy: "fair"}},
 	}
-	parent, err = NewConfiguredQueue(parentConfig, nil)
+	parent, err = NewConfiguredQueue(parentConfig, nil, false)
 	assert.NilError(t, err, "failed to create queue: %v", err)
 	recoveryQueue, err := NewRecoveryQueue(parent)
 	assert.NilError(t, err, "failed to create recovery queue: %v", err)

pkg/scheduler/objects/utilities_test.go

@@ -97,7 +97,7 @@ func createManagedQueuePropsMaxApps(parentSQ *Queue, name string, parent bool, m
 			Guaranteed: guarRes,
 		}
 	}
-	queue, err := NewConfiguredQueue(queueConfig, parentSQ)
+	queue, err := NewConfiguredQueue(queueConfig, parentSQ, false)
 	if err != nil {
 		return nil, err
 	}