Make DominantResourceShare Compatible with Cohorts #4097

gabesaba · 2025-01-29T15:55:18Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Prepare for #3759 by making DominantResourceShare compatible with Cohorts. This is done by refactoring dominantResourceShare to use the hierarchicalResourceNode interface, which Cohort and CohortSnapshot implement.

Additionally, we do some simplifications, deletions, moves:

remove multiplication by magic number in dominantResourceNode (was used to model subtracting resources)
delete resourceGroupNode interface (used only in tests)
delete netQuotaNode interface (not needed after refactor)
Move FairSharing code to its own file
Move ClusterQueueSnapshot's DominantResourceShare methods to clusterqueue_snapshot file

Special notes for your reviewer:

Please review commits independently for a more readable diff. The 2nd commit (should be) a pure move

Does this PR introduce a user-facing change?

NONE

netlify · 2025-01-29T15:55:38Z

✅ Deploy Preview for kubernetes-sigs-kueue ready!

Name	Link
🔨 Latest commit	`91c2f48`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/679cb24933d21300088aa70a
😎 Deploy Preview	https://deploy-preview-4097--kubernetes-sigs-kueue.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

gabesaba · 2025-01-30T10:23:06Z

/assign @PBundyra

PBundyra · 2025-01-31T08:53:27Z

pkg/cache/fair_sharing.go

+	"sigs.k8s.io/kueue/pkg/resources"
+)
+
+type dominantResourceShareNode interface {


Could you please add a description of the interface and its methods?

PBundyra · 2025-01-31T09:01:33Z

pkg/cache/clusterqueue.go

@@ -646,84 +638,8 @@ func workloadBelongsToLocalQueue(wl *kueue.Workload, q *kueue.LocalQueue) bool {
 	return wl.Namespace == q.Namespace && wl.Spec.QueueName == q.Name
 }

-// The methods below implement several interfaces. See
-// dominantResourceShareNode, resourceGroupNode, and netQuotaNode.
+// Implements dominantResourceShareNode interface.


Could we assert this type implements the interface?
https://github.com/kubernetes-sigs/kueue/blob/main/pkg/controller/tas/topology_ungater.go#L77-L78

Is there a clear advantage to this pattern? Even without this check, the code will fail to compile if we forget one of the methods, since we use clusterQueue as a dominantResourceShareNode

I see, I had the impression that the code wouldn't fail during compilation, but rather during runtime. Thanks for clarifying

I think indeed it does not matter for compilation, but can you quickly test if adding these improves integration with vs-code?

IIRC adding these was quickly letting me know which functions are missing (not implemented), but don't recall details, so it might be not accurate.

I just checked: there is no difference in the error message when adding these, versus when trying to use the type

Makes sense, being curious I also checked what happens in vs-code. When we have the check the error is here:

Otherwise it is also compile-time, but lands at usage

I think adding vs not adding the checks makes marginal difference and it is not something I would worry about either way.

PBundyra · 2025-01-31T09:02:44Z

pkg/cache/clusterqueue_snapshot.go

+	return c.Name
+}
+
+// Implements dominantResourceShareNode interface.


PBundyra · 2025-01-31T09:03:13Z

pkg/cache/clusterqueue_snapshot.go

+// DominantResourceShare returns a value from 0 to 1,000,000 representing the maximum of the ratios
+// of usage above nominal quota to the lendable resources in the cohort, among all the resources
+// provided by the ClusterQueue, and divided by the weight.
+// If zero, it means that the usage of the ClusterQueue is below the nominal quota.
+// The function also returns the resource name that yielded this value.
+// Also for a weight of zero, this will return 9223372036854775807.


Should this description be moved to dominantResourceShare?

PBundyra · 2025-01-31T09:10:00Z

/retest

PBundyra · 2025-01-31T09:16:14Z

pkg/cache/clusterqueue_snapshot.go

-func (c *ClusterQueueSnapshot) usageFor(fr resources.FlavorResource) int64 {
-	return c.ResourceNode.Usage[fr]
-}
+// The methods below implement hierarchicalResourceNode interface.


PBundyra · 2025-01-31T09:20:37Z

Since this is a clean up PR would you mind adding could you add description to hierarchicalResourceNode and ResourceNode?

PBundyra · 2025-01-31T09:31:50Z

pkg/cache/resource_node_test.go

Could we please follow the standard test pattern here, where we create separate type for test scenarios, and then run them in loop, so it's easily extendable?

We can add this later, if we decide to extend the test. I don't think that it makes sense to make it extensible now, when there may only ever be one test (YAGNI)

I see, do you anticipate we won't need more tests despite implementing FairSharing for Hierarchical Cohorts?

gabesaba · 2025-01-31T11:22:55Z

Please compare:
https://github.com/kubernetes-sigs/kueue/compare/f8cdd8e10fc1b3a92c912fec8834965052f5e61b..91c2f48aaafdc65df9c36f195978f8597aeb4db1

PBundyra · 2025-01-31T12:35:54Z

pkg/cache/fair_sharing.go

+// provided by the ClusterQueue, and divided by the weight.
+// If zero, it means that the usage of the ClusterQueue is below the nominal quota.
+// The function also returns the resource name that yielded this value.
+// Also for a weight of zero, this will return 9223372036854775807.


Suggested change

// Also for a weight of zero, this will return 9223372036854775807.

// Also for a weight of zero, this will return maxInt.

PBundyra · 2025-01-31T12:55:51Z

/lgtm
cc @mimowo

k8s-ci-robot · 2025-01-31T12:55:57Z

LGTM label has been added.

Git tree hash: 9ffdbe429220e20fb0c5abad717939a3936a5716

mimowo · 2025-01-31T13:43:24Z

/approve
Looks like a great cleanup PR! Feel free to address the remaining comments (#4097 and #4097) in a follow up. None of them looks blocking.

k8s-ci-robot · 2025-01-31T13:43:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gabesaba, mimowo

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [mimowo]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

) * Update DominantResourceShare to work with Cohorts * Move DominantResourceShare methods to more appropriate files * Cleanup resourceGroupNode, used only in tests * Update documentation of several types

gabesaba added 2 commits January 29, 2025 15:46

Update DominantResourceShare to work with Cohorts

45c9b89

Move DominantResourceShare methods to more appropriate files

803411c

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Jan 29, 2025

k8s-ci-robot requested review from mbobrovskyi and tenzen-y January 29, 2025 15:55

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 29, 2025

gabesaba force-pushed the dominant_resource_for_cohorts branch from 50fec32 to daf9f5f Compare January 29, 2025 16:12

Cleanup resourceGroupNode, used only in tests

f8cdd8e

gabesaba force-pushed the dominant_resource_for_cohorts branch from daf9f5f to f8cdd8e Compare January 30, 2025 10:22

k8s-ci-robot assigned PBundyra Jan 30, 2025

PBundyra reviewed Jan 31, 2025

View reviewed changes

pkg/cache/clusterqueue_snapshot.go

return c.Name

}

// Implements dominantResourceShareNode interface.

Copy link

Contributor

PBundyra Jan 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

PBundyra reviewed Jan 31, 2025

View reviewed changes

Update documentation of several types

91c2f48

gabesaba force-pushed the dominant_resource_for_cohorts branch from f8cdd8e to 62edcbc Compare January 31, 2025 11:20

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 31, 2025

gabesaba force-pushed the dominant_resource_for_cohorts branch from 62edcbc to 91c2f48 Compare January 31, 2025 11:21

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 31, 2025

PBundyra reviewed Jan 31, 2025

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 31, 2025

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 31, 2025

k8s-ci-robot merged commit 0236a85 into kubernetes-sigs:main Jan 31, 2025
17 of 18 checks passed

k8s-ci-robot added this to the v0.11 milestone Jan 31, 2025

gabesaba deleted the dominant_resource_for_cohorts branch January 31, 2025 14:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make DominantResourceShare Compatible with Cohorts #4097

Make DominantResourceShare Compatible with Cohorts #4097

gabesaba commented Jan 29, 2025

netlify bot commented Jan 29, 2025 •

edited

Loading

gabesaba commented Jan 30, 2025

PBundyra Jan 31, 2025 •

edited

Loading

gabesaba Jan 31, 2025

PBundyra Jan 31, 2025

gabesaba Jan 31, 2025

PBundyra Jan 31, 2025

mimowo Jan 31, 2025

gabesaba Jan 31, 2025

mimowo Jan 31, 2025

PBundyra Jan 31, 2025

PBundyra Jan 31, 2025

gabesaba Jan 31, 2025

PBundyra commented Jan 31, 2025

PBundyra Jan 31, 2025

PBundyra commented Jan 31, 2025

PBundyra Jan 31, 2025

gabesaba Jan 31, 2025

PBundyra Jan 31, 2025

gabesaba commented Jan 31, 2025

PBundyra Jan 31, 2025

PBundyra commented Jan 31, 2025

k8s-ci-robot commented Jan 31, 2025

mimowo commented Jan 31, 2025

k8s-ci-robot commented Jan 31, 2025

	// Also for a weight of zero, this will return 9223372036854775807.
	// Also for a weight of zero, this will return maxInt.

Make DominantResourceShare Compatible with Cohorts #4097

Make DominantResourceShare Compatible with Cohorts #4097

Conversation

gabesaba commented Jan 29, 2025

What type of PR is this?

What this PR does / why we need it:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

netlify bot commented Jan 29, 2025 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kueue ready!

gabesaba commented Jan 30, 2025

PBundyra Jan 31, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PBundyra commented Jan 31, 2025

Choose a reason for hiding this comment

PBundyra commented Jan 31, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabesaba commented Jan 31, 2025

Choose a reason for hiding this comment

PBundyra commented Jan 31, 2025

k8s-ci-robot commented Jan 31, 2025

mimowo commented Jan 31, 2025

k8s-ci-robot commented Jan 31, 2025

netlify bot commented Jan 29, 2025 •

edited

Loading

PBundyra Jan 31, 2025 •

edited

Loading