Support Hierarchical Cohorts in Cache/Snapshot #3006

gabesaba · 2024-09-06T12:50:25Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Support HierarchicalCohorts (#79), with cycle-detection, in the cache and snapshot.

Special notes for your reviewer:

I wrote tests which check resource usage. I will add them to this PR once #3005 merges.

Does this PR introduce a user-facing change?

NONE

netlify · 2024-09-06T12:50:41Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`9aae9ea`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/66e803a271318a0009a33e42

gabesaba · 2024-09-06T12:52:01Z

/assign @mimowo

alculquicondor · 2024-09-06T14:14:29Z

/cc

alculquicondor · 2024-09-06T17:57:16Z

pkg/cache/algorithm.go

+// getRoot returns the root of the Cohort tree IFF there was not a
+// cycle.
+func getRoot(c *cohort) (*cohort, bool) {
+	checker := cycleChecker{cycles: make(map[string]bool, 8)}


clarify in a comment that 8 is a reasonable upper-bound for hierarchy height

Deleted in this revision.

alculquicondor · 2024-09-06T18:02:58Z

pkg/cache/algorithm.go

hierarchy.go?

Deleted in this revision. We now have cycle.go in hierarchy package

alculquicondor · 2024-09-06T18:06:05Z

pkg/cache/algorithm.go

+	return getRootHelper(c), true
+}
+
+func getRootHelper(c *cohort) *cohort {


Suggested change

func getRootHelper(c *cohort) *cohort {

func getRoot(c *cohort) *cohort {

I would expect the scheduling cycle to call this function directly, as opposed to check for cycles again.

alculquicondor · 2024-09-06T18:06:27Z

pkg/cache/algorithm.go

+
+// getRoot returns the root of the Cohort tree IFF there was not a
+// cycle.
+func getRoot(c *cohort) (*cohort, bool) {


Suggested change

func getRoot(c *cohort) (*cohort, bool) {

func getRootUnsafe(c *cohort) (*cohort, bool) {

shouldn't the other function be called unsafe, as it will enter an infinite loop if called with cycle?

alculquicondor · 2024-09-06T18:07:46Z

pkg/cache/algorithm.go

+	if c.cycles[cohort.GetName()] {
+		return true
+	}


Suggested change

if c.cycles[cohort.GetName()] {

return true

}

if cycle, seen := c.cycles[cohort.GetName()]; seen {

return cycle

}

?

This is better, thanks :)

alculquicondor · 2024-09-06T18:18:28Z

pkg/cache/snapshot.go

@@ -84,12 +84,19 @@ func (c *Cache) Snapshot() Snapshot {
 		ResourceFlavors:          make(map[kueue.ResourceFlavorReference]*kueue.ResourceFlavor, len(c.resourceFlavors)),
 		InactiveClusterQueueSets: sets.New[string](),
 	}
+	cycleChecker := cycleChecker{cycles: make(map[string]bool, len(c.hm.Cohorts))}


why do you need to check at this point?
I think the information should be stored in the cache somehow. We need to communicate it to the CQ and cohort statuses anyways.

I'm storing this structure in the hierarchy.Manager (and thus cache) now, but it will still be called again during snapshotting - using the memoized result.

alculquicondor · 2024-09-12T14:51:10Z

pkg/cache/cache_test.go

+				_ = cache.AddCohort(cohort)
 				return nil


Suggested change

_ = cache.AddCohort(cohort)

return nil

return cache.AddCohort(cohort)

alculquicondor · 2024-09-12T15:04:49Z

pkg/cache/cache_test.go

+			t.Fatal("Expected success")
+		}
+
+		t.Run("before move", func(t *testing.T) {


do not use t.Run for non-independent tests cases

gabesaba · 2024-09-13T13:53:52Z

/retest

alculquicondor · 2024-09-13T17:59:15Z

pkg/controller/core/cohort_controller.go

@@ -104,7 +104,9 @@ func (r *CohortReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
 	}

 	log.V(2).Info("Cohort is being created or updated", "resources", cohort.Spec.ResourceGroups)
-	r.cache.AddOrUpdateCohort(&cohort)
+	if err := r.cache.AddOrUpdateCohort(&cohort); err != nil {
+		return ctrl.Result{}, err


Doesn't this cause a requeue into the reconciler?

Maybe we should log the issue, but then don't return the error.

That makes sense, updated.

alculquicondor

/lgtm
/approve

k8s-ci-robot · 2024-09-16T13:24:14Z

LGTM label has been added.

Git tree hash: c3d67eb01af97818021dc8b9379cf9317d75678c

k8s-ci-robot · 2024-09-16T13:24:15Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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~~ [alculquicondor]

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

gabesaba · 2024-09-20T13:37:37Z

Only attaching release note to a single PR, where we define the API #2693

/release-note-edit

NONE

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. labels Sep 6, 2024

k8s-ci-robot requested review from alculquicondor and trasc September 6, 2024 12:50

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 6, 2024

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 6, 2024

k8s-ci-robot assigned mimowo Sep 6, 2024

gabesaba force-pushed the hc_cache branch from a191e28 to 61962b0 Compare September 6, 2024 14:01

gabesaba force-pushed the hc_cache branch from 61962b0 to 0b224b3 Compare September 6, 2024 14:36

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 6, 2024

alculquicondor reviewed Sep 6, 2024

View reviewed changes

gabesaba mentioned this pull request Sep 11, 2024

Implement Cohort Reconciler #2971

Merged

gabesaba force-pushed the hc_cache branch 2 times, most recently from 2578618 to f7c6a41 Compare September 12, 2024 14:23

alculquicondor reviewed Sep 12, 2024

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 13, 2024

gabesaba mentioned this pull request Sep 13, 2024

Implement Cohort cycle detection in hierarchy.Manager #3040

Merged

gabesaba force-pushed the hc_cache branch from f7c6a41 to 09fc68c Compare September 13, 2024 13:30

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 13, 2024

gabesaba force-pushed the hc_cache branch from 09fc68c to 25e763f Compare September 13, 2024 14:38

alculquicondor reviewed Sep 13, 2024

View reviewed changes

Support Hierarchical Cohorts in Cache/Snapshot

9aae9ea

gabesaba force-pushed the hc_cache branch from 25e763f to 9aae9ea Compare September 16, 2024 10:08

alculquicondor reviewed Sep 16, 2024

View reviewed changes

k8s-ci-robot assigned alculquicondor Sep 16, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 16, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 16, 2024

k8s-ci-robot merged commit f6ebfe5 into kubernetes-sigs:main Sep 16, 2024
16 checks passed

k8s-ci-robot added this to the v0.9 milestone Sep 16, 2024

gabesaba deleted the hc_cache branch September 16, 2024 13:51

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Sep 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Hierarchical Cohorts in Cache/Snapshot #3006

Support Hierarchical Cohorts in Cache/Snapshot #3006

gabesaba commented Sep 6, 2024 •

edited by k8s-ci-robot

Loading

netlify bot commented Sep 6, 2024 •

edited

Loading

gabesaba commented Sep 6, 2024

alculquicondor commented Sep 6, 2024

alculquicondor Sep 6, 2024

gabesaba Sep 12, 2024

alculquicondor Sep 6, 2024

gabesaba Sep 12, 2024

alculquicondor Sep 6, 2024

alculquicondor Sep 6, 2024

alculquicondor Sep 6, 2024

gabesaba Sep 12, 2024

alculquicondor Sep 6, 2024

gabesaba Sep 12, 2024

alculquicondor Sep 6, 2024

gabesaba Sep 12, 2024

alculquicondor Sep 12, 2024

gabesaba Sep 13, 2024

alculquicondor Sep 12, 2024

gabesaba Sep 13, 2024

gabesaba commented Sep 13, 2024

alculquicondor Sep 13, 2024

gabesaba Sep 16, 2024

alculquicondor left a comment

k8s-ci-robot commented Sep 16, 2024

k8s-ci-robot commented Sep 16, 2024

gabesaba commented Sep 20, 2024

	func getRootHelper(c cohort) cohort {
	func getRoot(c cohort) cohort {

	func getRoot(c cohort) (cohort, bool) {
	func getRootUnsafe(c cohort) (cohort, bool) {

	_ = cache.AddCohort(cohort)
	return nil
	return cache.AddCohort(cohort)

Support Hierarchical Cohorts in Cache/Snapshot #3006

Support Hierarchical Cohorts in Cache/Snapshot #3006

Conversation

gabesaba commented Sep 6, 2024 • edited by k8s-ci-robot Loading

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 Sep 6, 2024 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

gabesaba commented Sep 6, 2024

alculquicondor commented Sep 6, 2024

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

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

gabesaba commented Sep 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alculquicondor left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 16, 2024

k8s-ci-robot commented Sep 16, 2024

gabesaba commented Sep 20, 2024

gabesaba commented Sep 6, 2024 •

edited by k8s-ci-robot

Loading

netlify bot commented Sep 6, 2024 •

edited

Loading