Make requeuing Strategy mutable #1934

mimowo · 2024-04-02T07:45:52Z

What type of PR is this?

/kind feature
/kind api-change

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

There seems to be that StrictFIFO misreports the number of pending workloads in the status and in metrics (the count flakes), when there is a blocking workload. I'm looking into this, I repeated the issue on main branch, independently of changes here. See repro test: #1936.

Does this PR introduce a user-facing change?

Make ClusterQueue queueingStrategy field mutable. The field can be mutated while there are pending workloads.

netlify · 2024-04-02T07:46:08Z

✅ Deploy Preview for kubernetes-sigs-kueue ready!

Name	Link
🔨 Latest commit	`1cc4e3c`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/660edba7dc1bc300082e497e
😎 Deploy Preview	https://deploy-preview-1934--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.

mimowo · 2024-04-02T09:03:49Z

/assign @yaroslava-serdiuk

mimowo · 2024-04-03T13:34:00Z

/assign @alculquicondor

alculquicondor · 2024-04-03T17:54:30Z

pkg/queue/cluster_queue_interface.go

+// to sort workloads. The function sorts workloads based on their priority.
+// When priorities are equal, it uses the workload's creation or eviction
+// time.
+func queueOrderingFunc(wo workload.Ordering) func(a, b *workload.Info) bool {


Nit:
I guess this function could be moved to the _impl file, and not be passed as a parameter.

Also maybe the interface is no longer necessary (in favor of a plain pointer to the implementation).

I guess this function could be moved to the _impl file, and not be passed as a parameter.

Makes sense, done.

Also maybe the interface is no longer necessary (in favor of a plain pointer to the implementation).

No strong view here, let me try how this would look like, I will put in a separate commit.

Done in 493cc34

alculquicondor · 2024-04-03T17:56:11Z

pkg/queue/cluster_queue_impl.go

@@ -83,6 +85,7 @@ func newClusterQueueImpl(
 func (c *clusterQueueBase) Update(apiCQ *kueue.ClusterQueue) error {
 	c.rwm.Lock()
 	defer c.rwm.Unlock()
+	c.queueingStrategy = apiCQ.Spec.QueueingStrategy


Shouldn't we move all the inadmissible pods back to the heap when changing to StrictFIFO?

Maybe add a unit test? Or integration.

Shouldn't we move all the inadmissible pods back to the heap when changing to StrictFIFO?

This is already happening, because when the spec.queueingStrategy is updated, then specUpdated=true here

This in turn calls queueAllInadmissibleWorkloadsInCohort within cohort here.

Maybe add a unit test? Or integration.

This is covered by the integration test. You can see this in this section. First, we assert ExpectPendingWorkloadsMetric(0, 1), meaning one inadmissible pending. After the change to StrictFIFO we assert ExpectPendingWorkloadsMetric(1, 0) meaning it was moved to the heap.

As for the unit test I can try to add but the logic is scattered across clusterqueue_controller: Update and Reconcile, but also manager and cache, so seems involving, and the integration test is needed anyway,

Thanks for checking

alculquicondor

/approve

Just a nit

pkg/queue/cluster_queue.go

alculquicondor · 2024-04-04T15:46:37Z

pkg/queue/cluster_queue_impl.go

@@ -83,6 +85,7 @@ func newClusterQueueImpl(
 func (c *clusterQueueBase) Update(apiCQ *kueue.ClusterQueue) error {
 	c.rwm.Lock()
 	defer c.rwm.Unlock()
+	c.queueingStrategy = apiCQ.Spec.QueueingStrategy


Thanks for checking

tenzen-y

Overall LGTM
Could you update the API document as well?

kueue/apis/kueue/v1beta1/clusterqueue_types.go

Line 54 in 1aa8ade

// across the queues in this ClusterQueue. This field is immutable.

tenzen-y · 2024-04-04T15:59:12Z

/release-note-edit

Make ClusterQueue queueingStrategy field mutable. The field can be mutated while there are pending workloads.

k8s-ci-robot · 2024-04-04T16:56:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, 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~~ [alculquicondor]

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

alculquicondor · 2024-04-04T17:01:45Z

/lgtm

k8s-ci-robot · 2024-04-04T17:01:50Z

LGTM label has been added.

Git tree hash: 36d2136e77d46e4e66ef27a86f9a04b6f3509a47

tenzen-y

Probably, we need to re-generate API documentation as well.

tenzen-y · 2024-04-04T17:05:34Z

Probably, we need to re-generate API documentation as well.

Oh, it has already been regenerated. NVM

* Make requeuingStrategy mutable * Rebase and comments * Drop the ClusterQueue interface * cleanup

k8s-ci-robot requested review from denkensk and trasc April 2, 2024 07:45

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 2, 2024

mimowo changed the title ~~Make requeuing Strategy mutable~~ Make requeuing Strategy mutable [WIP] Apr 2, 2024

mimowo force-pushed the mutable-requeuing-strategy branch 2 times, most recently from c8aa824 to 168a14d Compare April 2, 2024 09:01

mimowo changed the title ~~Make requeuing Strategy mutable [WIP]~~ Make requeuing Strategy mutable Apr 2, 2024

k8s-ci-robot assigned yaroslava-serdiuk Apr 2, 2024

mimowo force-pushed the mutable-requeuing-strategy branch from 168a14d to a64ce8f Compare April 2, 2024 11:08

k8s-ci-robot assigned alculquicondor Apr 3, 2024

alculquicondor reviewed Apr 3, 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 Apr 3, 2024

mimowo force-pushed the mutable-requeuing-strategy branch from a64ce8f to f8d3b1d Compare April 4, 2024 06:58

Make requeuingStrategy mutable

f04ee61

mimowo force-pushed the mutable-requeuing-strategy branch from f8d3b1d to 88dfe6a Compare April 4, 2024 07:02

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

Rebase and comments

0ab2501

mimowo force-pushed the mutable-requeuing-strategy branch from 88dfe6a to 0ab2501 Compare April 4, 2024 07:06

Drop the ClusterQueue interface

493cc34

alculquicondor reviewed Apr 4, 2024

View reviewed changes

tenzen-y reviewed Apr 4, 2024

View reviewed changes

cleanup

1cc4e3c

mimowo force-pushed the mutable-requeuing-strategy branch from 1af7fbb to 1cc4e3c Compare April 4, 2024 16:56

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

tenzen-y reviewed Apr 4, 2024

View reviewed changes

k8s-ci-robot merged commit 3228c68 into kubernetes-sigs:main Apr 4, 2024
14 checks passed

k8s-ci-robot added this to the v0.7 milestone Apr 4, 2024

vsoch pushed a commit to researchapps/kueue that referenced this pull request Apr 18, 2024

Make requeuing Strategy mutable (kubernetes-sigs#1934)

7997852

* Make requeuingStrategy mutable * Rebase and comments * Drop the ClusterQueue interface * cleanup

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make requeuing Strategy mutable #1934

Make requeuing Strategy mutable #1934

mimowo commented Apr 2, 2024 •

edited by k8s-ci-robot

netlify bot commented Apr 2, 2024 •

edited

mimowo commented Apr 2, 2024

mimowo commented Apr 3, 2024

alculquicondor Apr 3, 2024

mimowo Apr 4, 2024

mimowo Apr 4, 2024

alculquicondor Apr 3, 2024

mimowo Apr 4, 2024 •

edited

alculquicondor Apr 4, 2024

alculquicondor left a comment

alculquicondor Apr 4, 2024

tenzen-y left a comment

tenzen-y commented Apr 4, 2024

k8s-ci-robot commented Apr 4, 2024

alculquicondor commented Apr 4, 2024

k8s-ci-robot commented Apr 4, 2024

tenzen-y left a comment

tenzen-y commented Apr 4, 2024

Make requeuing Strategy mutable #1934

Make requeuing Strategy mutable #1934

Conversation

mimowo commented Apr 2, 2024 • edited by k8s-ci-robot

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

netlify bot commented Apr 2, 2024 • edited

✅ Deploy Preview for kubernetes-sigs-kueue ready!

mimowo commented Apr 2, 2024

mimowo commented Apr 3, 2024

alculquicondor Apr 3, 2024

Choose a reason for hiding this comment

mimowo Apr 4, 2024

Choose a reason for hiding this comment

mimowo Apr 4, 2024

Choose a reason for hiding this comment

alculquicondor Apr 3, 2024

Choose a reason for hiding this comment

mimowo Apr 4, 2024 • edited

Choose a reason for hiding this comment

alculquicondor Apr 4, 2024

Choose a reason for hiding this comment

alculquicondor left a comment

Choose a reason for hiding this comment

alculquicondor Apr 4, 2024

Choose a reason for hiding this comment

tenzen-y left a comment

Choose a reason for hiding this comment

tenzen-y commented Apr 4, 2024

k8s-ci-robot commented Apr 4, 2024

alculquicondor commented Apr 4, 2024

k8s-ci-robot commented Apr 4, 2024

tenzen-y left a comment

Choose a reason for hiding this comment

tenzen-y commented Apr 4, 2024

mimowo commented Apr 2, 2024 •

edited by k8s-ci-robot

netlify bot commented Apr 2, 2024 •

edited

mimowo Apr 4, 2024 •

edited