[YUNIKORN-2907] Queue config processing log spew #1009

kousei47747 · 2024-12-26T09:53:15Z

What is this PR for?

Silence the logs of shadow queue structure during configuration update.

What type of PR is it?

What is the Jira issue?

https://issues.apache.org/jira/browse/YUNIKORN-2907

codecov · 2024-12-28T13:12:33Z

Codecov Report

Attention: Patch coverage is 91.17647% with 6 lines in your changes missing coverage. Please review.

Project coverage is 82.50%. Comparing base (7391aeb) to head (f15ede1).
Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/scheduler/partition.go	82.60%	1 Missing and 3 partials ⚠️
pkg/scheduler/context.go	50.00%	1 Missing ⚠️
pkg/scheduler/objects/queue.go	91.66%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1009      +/-   ##
==========================================
+ Coverage   82.48%   82.50%   +0.02%     
==========================================
  Files          97       97              
  Lines       15627    15637      +10     
==========================================
+ Hits        12890    12902      +12     
+ Misses       2457     2456       -1     
+ Partials      280      279       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

chia7712

@kousei47747 thanks for this patch. There is one small question remaining. Please take a look.

pkg/scheduler/partition.go

blueBlue0102 · 2024-12-29T06:40:26Z

pkg/scheduler/objects/queue.go

+	return queue, err
+}
+
+// NewConfiguredShadowQueue creates a new queue from scratch based on the configuration and logs at debug level


Hi @kousei47747, thank you for your contribution!

Could you provide more details about the differences between NewConfiguredQueue and NewConfiguredShadowQueue?
Currently, it appears the only difference is the log level, which may be confusing for those unfamiliar with shadow queues.

It would be helpful to explain when developers should use NewConfiguredQueue versus NewConfiguredShadowQueue.

@blueBlue0102 thanks for reviewing! I will provide more detailed description in the new patch.

wilfred-s · 2024-12-31T03:37:31Z

I dropped a large comment into the jira to provide some guidance and point out another issue.

kousei47747 · 2024-12-31T04:56:08Z

@wilfred-s thanks for reviewing! I've replied on jira, please take a look.

kousei47747 · 2024-12-31T17:51:14Z

I’ve updated the PR based on the guidance and noticed that NewPlacementManager also logs. Should it be silenced too?

pbacsko

See comments. I'm not sure about the current approach. But if you keep it, please make sure it's tested properly. PartitionContext does not have the best unit test coverage (at least direct coverage), but new code should definitely be covered.

pkg/scheduler/partition.go

blueBlue0102 · 2025-02-09T15:02:44Z

If we need to silence all logs when creating shadow queues, it seems that some places were missed.

For example:

ACL.setUsers():

Related code: acl.go#L54

Call stack:

NewConfiguredQueue
  └── sq.applyConf(conf)
        └── security.NewACL()
              └── acl.setUsers()

It seems that either adding an additional parameter or creating new functions adds quite a lot of complexity to the codebase, but I don't have a good idea for a third approach at the moment.

kousei47747 · 2025-02-11T09:32:33Z

@blueBlue0102 good catch! I've updated the PR to silence ACL.setUsers() and NewPlacementManager. PTAL

pbacsko · 2025-02-13T07:38:09Z

pkg/scheduler/partition.go

@@ -191,17 +199,18 @@ func (pc *PartitionContext) updatePartitionDetails(conf configs.PartitionConfig)
 	return ugm.GetUserManager().UpdateConfig(queueConf, conf.Queues[0].Name)


What about this one? This is still called regardless of the value of silence.

Thanks for reviewing! updatePartitionDetails is called only when performing the real update, so I think we can skip silence here.

yunikorn-core/pkg/scheduler/context.go

Lines 372 to 374 in 34ca3b2

// checks passed perform the real update

log.Log(log.SchedContext).Info("updating partitions", zap.String("partitionName", partitionName))

err = part.updatePartitionDetails(p)

blueBlue0102

Thanks for the patches! It seems there are still some places that were missed. It made me think of another approach.

blueBlue0102 · 2025-02-14T06:28:57Z

pkg/common/security/acl.go

@@ -116,7 +119,7 @@ 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))


If we still want to use the silence flag approach, acl.setGroups also needs to be silenced.

Silence flag added to setGroups, thanks!

blueBlue0102 · 2025-02-14T06:54:45Z

pkg/scheduler/partition.go

@@ -79,7 +79,7 @@ type PartitionContext struct {
 	locking.RWMutex
 }

-func newPartitionContext(conf configs.PartitionConfig, rmID string, cc *ClusterContext) (*PartitionContext, error) {
+func newPartitionContext(conf configs.PartitionConfig, rmID string, cc *ClusterContext, silence bool) (*PartitionContext, error) {
 	if conf.Name == "" || rmID == "" {
 		log.Log(log.SchedPartition).Info("partition cannot be created",


If we still want to use the silence flag approach, we should check silence before logging.

This is logged only when there's a misconfiguration and will return instantly with an error, so no further logs will be generated. I think we can keep it for debugging. What do you think?

Keeping the log for misconfiguration make sense to me.

blueBlue0102 · 2025-02-14T08:01:25Z

pkg/scheduler/context.go

 			// make sure the new info passes all checks
-			_, err = newPartitionContext(p, rmID, nil)
+			_, err = newPartitionContext(p, rmID, nil, true)
 			if err != nil {


It seems we call newPartitionContext here only for validating configs.PartitionConfig.
If my understanding is correct, the main purpose of this JIRA is to avoid any side effects when validating the config before updatePartitionDetails,
not only:

Logging

Sending out events

but also including:

Loading the user manager and updating user settings.
ugm.GetUserManager().UpdateConfig() would be called in updatePartitionDetails() when validation passes.

etc.

So rather than adding a lot of flag parameters to each underlying function, extracting all the validation logic from newPartitionContext into another function or adding those validation logic into updatePartitionDetails might be a better approach.
Let newPartitionContext do what it needs to do without consideration of "silence". What do you think?

Thanks for the nice approach! It looks like the trade-off would be duplicate logic/methods when extracting it from stacked callers. I tried a similar approach in the first few commits, but it turned out to be a bit cumbersome. So I think I still prefer adding the silence flag.
Please let me know if I’m missing something!

pbacsko

LGTM

chenyulin0719

LGTM. Just a confirmation.

chenyulin0719 · 2025-02-18T16:17:09Z

pkg/scheduler/partition.go

-	return ugm.GetUserManager().UpdateConfig(queueConf, conf.Queues[0].Name)
+	if !silence {
+		return ugm.GetUserManager().UpdateConfig(queueConf, conf.Queues[0].Name)
+	}


@wilfred-s This change also fix the unnecessarily ugm reload issue which you mentioned in Jira.

@kousei47747 I think we don't need another Jira. Does it sound right to you?

@chenyulin0719 yes, thanks :)

ryankert01

+1, lgtm

[YUNIKORN-2907] Queue config processing log spew

64eed25

chenyulin0719 assigned kousei47747 Dec 28, 2024

chenyulin0719 self-requested a review December 28, 2024 13:11

chenyulin0719 requested a review from wilfred-s December 28, 2024 13:36

chia7712 reviewed Dec 28, 2024

View reviewed changes

pkg/scheduler/partition.go Outdated Show resolved Hide resolved

blueBlue0102 reviewed Dec 29, 2024

View reviewed changes

kousei47747 marked this pull request as draft December 29, 2024 08:27

Michael added 2 commits January 1, 2025 01:30

[YUNIKORN-2907] fix based on reviews

3f85580

[YUNIKORN-2997] remove redundant comment

7476680

kousei47747 marked this pull request as ready for review January 2, 2025 07:24

pbacsko requested changes Jan 23, 2025

View reviewed changes

pkg/scheduler/partition.go Outdated Show resolved Hide resolved

pkg/scheduler/partition.go Outdated Show resolved Hide resolved

pkg/scheduler/partition.go Outdated Show resolved Hide resolved

kousei47747 marked this pull request as draft January 24, 2025 02:42

kousei47747 and others added 4 commits February 6, 2025 15:54

Merge branch 'apache:master' into YUNIKORN-2907

a2be222

try the flag approach

cd4f4e0

[YUNIKORN-2907] fix lint issue

9a18381

[YUNIKORN-2907] fix lint issue

36bb742

kousei47747 marked this pull request as ready for review February 7, 2025 07:04

kousei47747 requested review from pbacsko, blueBlue0102 and chia7712 February 7, 2025 07:04

fix based on reviews

34ca3b2

pbacsko reviewed Feb 13, 2025

View reviewed changes

blueBlue0102 reviewed Feb 14, 2025

View reviewed changes

fix based on reviews

248027f

fix lint issue

f15ede1

kousei47747 requested review from pbacsko and blueBlue0102 February 18, 2025 09:10

pbacsko approved these changes Feb 18, 2025

View reviewed changes

chenyulin0719 approved these changes Feb 18, 2025

View reviewed changes

ryankert01 approved these changes Feb 20, 2025

View reviewed changes

craigcondit closed this in ee8d7fa Feb 20, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[YUNIKORN-2907] Queue config processing log spew #1009

[YUNIKORN-2907] Queue config processing log spew #1009

kousei47747 commented Dec 26, 2024 •

edited

Loading

codecov bot commented Dec 28, 2024 •

edited

Loading

chia7712 left a comment

blueBlue0102 Dec 29, 2024

kousei47747 Dec 31, 2024

wilfred-s commented Dec 31, 2024

kousei47747 commented Dec 31, 2024

kousei47747 commented Dec 31, 2024

pbacsko left a comment

blueBlue0102 commented Feb 9, 2025

kousei47747 commented Feb 11, 2025

pbacsko Feb 13, 2025

kousei47747 Feb 14, 2025 •

edited

Loading

blueBlue0102 left a comment

blueBlue0102 Feb 14, 2025

kousei47747 Feb 17, 2025

blueBlue0102 Feb 14, 2025

kousei47747 Feb 17, 2025 •

edited

Loading

chenyulin0719 Feb 18, 2025

blueBlue0102 Feb 14, 2025

kousei47747 Feb 17, 2025

pbacsko left a comment

chenyulin0719 left a comment

chenyulin0719 Feb 18, 2025

kousei47747 Feb 19, 2025

ryankert01 left a comment

		@@ -191,17 +199,18 @@ func (pc *PartitionContext) updatePartitionDetails(conf configs.PartitionConfig)
		return ugm.GetUserManager().UpdateConfig(queueConf, conf.Queues[0].Name)

	// checks passed perform the real update
	log.Log(log.SchedContext).Info("updating partitions", zap.String("partitionName", partitionName))
	err = part.updatePartitionDetails(p)

[YUNIKORN-2907] Queue config processing log spew #1009

[YUNIKORN-2907] Queue config processing log spew #1009

Conversation

kousei47747 commented Dec 26, 2024 • edited Loading

What is this PR for?

What type of PR is it?

What is the Jira issue?

codecov bot commented Dec 28, 2024 • edited Loading

Codecov Report

chia7712 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilfred-s commented Dec 31, 2024

kousei47747 commented Dec 31, 2024

kousei47747 commented Dec 31, 2024

pbacsko left a comment

Choose a reason for hiding this comment

blueBlue0102 commented Feb 9, 2025

kousei47747 commented Feb 11, 2025

Choose a reason for hiding this comment

kousei47747 Feb 14, 2025 • edited Loading

Choose a reason for hiding this comment

blueBlue0102 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kousei47747 Feb 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pbacsko left a comment

Choose a reason for hiding this comment

chenyulin0719 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryankert01 left a comment

Choose a reason for hiding this comment

kousei47747 commented Dec 26, 2024 •

edited

Loading

codecov bot commented Dec 28, 2024 •

edited

Loading

kousei47747 Feb 14, 2025 •

edited

Loading

kousei47747 Feb 17, 2025 •

edited

Loading