Count inflight workload as pending #1936

mimowo · 2024-04-02T09:11:47Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #1937

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Fix the counter of pending workloads in cluster queue status. 

The counter would not count the head workload for StrictFIFO queues, if the workload cannot get admitted.

This change also includes the blocked workload in the metrics and the visibility API for the list of pending workloads.

netlify · 2024-04-02T09:12:10Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`f95af59`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/660d5e6e5101a00008b8cf7e

mimowo · 2024-04-02T09:26:41Z

/hold
Experimentation for now

mimowo · 2024-04-03T11:53:33Z

/hold cancel
Ready for review

mimowo · 2024-04-03T13:33:36Z

/assign @alculquicondor

alculquicondor · 2024-04-03T15:20:48Z

pkg/queue/cluster_queue_impl.go

@@ -153,6 +157,7 @@ func (c *clusterQueueBase) Delete(w *kueue.Workload) {
 	key := workload.Key(w)


🤔 did we forget to lock the mutex? Or is it locked by all callers?

It is locked by the only called currently, that is DeleteFromLocalQueue

can we rename it to delete instead?

Actually, it's also called from the queue Manager, via the ClusterQueue interface.

But we already access the manager through a mutex. Now I'm wondering why we added a dedicated mutex to the ClusterQueue.

Oh it was for the status snapshoter #1069

That's right, sorry for confusion, somehow I missed it in vs-code when using the "all references" option.

IIRC we added the mutex to ClusterQueue so that the Snapshot operation (needed for visibility) only requires cq-scoped mutex, and this operation could be expensive.

I suppose there might still be problematic scenarios, for example, the event handlers triggering a Workload deletion while the snapshoter is running.

So we should always be locking the mutex. However, maybe we can get rid of the snapshotter in the near future in favor of the visibility API.

I suppose there might still be problematic scenarios, for example, the event handlers triggering a Workload deletion while the snapshoter is running.

The main purpose of the cs-scoped mutex is to make sure the heap structure can be iterated and copied so that we don't miss any element due to concurrent modifications.

So we should always be locking the mutex.

yes

However, maybe we can get rid of the snapshotter in the near future in favor of the visibility API.

I think we can get rid of the snapshotter that persists data in etcd, but the Snapshot() function itself is used by the visibility API to get consistent view of the workloads, so I don't see a way of dropping the cq-scoped mutex, unless:

we use the manager-scoped mutex, but this would impact performance

don't use mutex for Snapshot() at all, but this would impact consistency of results during updates

alculquicondor · 2024-04-03T15:23:22Z

pkg/queue/cluster_queue_impl.go

@@ -235,7 +247,11 @@ func (c *clusterQueueBase) Pending() int {
 }

 func (c *clusterQueueBase) PendingActive() int {


Is this ever called without locking?

No, it is called from two places:

cq.Pending() - this is using read lock scoped to the cq

manager.reportPendingWorkloads(), but all invocations of this function take manager lock, which is broader range than cq

alculquicondor

/lgtm
/approve

k8s-ci-robot · 2024-04-03T17:36:51Z

LGTM label has been added.

Git tree hash: 787aa61cb564ef774a812027fb97db41299f7826

k8s-ci-robot · 2024-04-03T17:36:52Z

[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:

~~pkg/OWNERS~~ [alculquicondor,mimowo]
~~test/OWNERS~~ [alculquicondor,mimowo]

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

tenzen-y · 2024-04-03T17:39:07Z

@alculquicondor @mimowo Isn't this a bug?

mimowo · 2024-04-04T06:50:27Z

@alculquicondor @mimowo Isn't this a bug?

yes, it is, let me update the labels:

/kind-remove cleanup
/kind bug

tenzen-y · 2024-04-04T08:12:52Z

@alculquicondor @mimowo Isn't this a bug?

yes, it is, let me update the labels:

/kind-remove cleanup /kind bug

Also, should we do cherry-pick to v0.6.

mimowo · 2024-04-04T08:45:28Z

Also, should we do cherry-pick to v0.6.

I would vote in favor of cherry-picking, the risk seems small. On the other hand it wasn't reported by users, but me when I was working on the feature: #1873, so may not be critical.

I checked that there is a small conflict with #1815, if we include #1815, then it cherry-picks without conflict.

alculquicondor · 2024-04-04T13:57:56Z

This doesn't seem urgent enough for a cherry-pick IMO

tenzen-y · 2024-04-04T15:02:11Z

Considering conflicts, I'm ok without cherry-pick.

* Test StrictFiFO misreporting metrics * Cleanup and expand the integration test

k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Apr 2, 2024

k8s-ci-robot requested review from alculquicondor and kerthcet April 2, 2024 09:11

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 2, 2024

mimowo changed the title ~~Test StrictFiFO misreporting metrics [DO NOT MERGE, EXPERIMENT]~~ Test StrictFiFO flaking when reporting metrics [DO NOT MERGE, EXPERIMENT] Apr 2, 2024

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 2, 2024

mimowo mentioned this pull request Apr 2, 2024

Make requeuing Strategy mutable #1934

Merged

mimowo force-pushed the main-pending-metric-experiment branch from 4fe6430 to a829ccf Compare April 2, 2024 11:13

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

mimowo force-pushed the main-pending-metric-experiment branch 2 times, most recently from e579455 to dc25ca6 Compare April 2, 2024 11:22

k8s-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 2, 2024

mimowo force-pushed the main-pending-metric-experiment branch from dc25ca6 to aff3522 Compare April 2, 2024 15:33

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

mimowo force-pushed the main-pending-metric-experiment branch 2 times, most recently from 2508913 to 0aec355 Compare April 3, 2024 08:49

mimowo changed the title ~~Test StrictFiFO flaking when reporting metrics [DO NOT MERGE, EXPERIMENT]~~ Fix StrictFiFO undercounting pending workloads when blocked Apr 3, 2024

mimowo changed the title ~~Fix StrictFiFO undercounting pending workloads when blocked~~ Fix StrictFiFO undercounting pending workloads Apr 3, 2024

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Apr 3, 2024

mimowo force-pushed the main-pending-metric-experiment branch 2 times, most recently from bad5983 to 6ea1e82 Compare April 3, 2024 09:36

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 3, 2024

mimowo changed the title ~~Fix StrictFiFO undercounting pending workloads~~ Count inflight workload as pending Apr 3, 2024

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 3, 2024

Test StrictFiFO misreporting metrics

119b2e1

mimowo force-pushed the main-pending-metric-experiment branch from 6ea1e82 to 119b2e1 Compare April 3, 2024 12:49

mimowo force-pushed the main-pending-metric-experiment branch 3 times, most recently from a610da2 to 78163a4 Compare April 3, 2024 13:25

k8s-ci-robot assigned alculquicondor Apr 3, 2024

Cleanup and expand the integration test

f95af59

mimowo force-pushed the main-pending-metric-experiment branch from 78163a4 to f95af59 Compare April 3, 2024 13:49

alculquicondor reviewed Apr 3, 2024

View reviewed changes

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

k8s-ci-robot merged commit b0a02c6 into kubernetes-sigs:main Apr 3, 2024
14 checks passed

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

k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Apr 4, 2024

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

Count inflight workload as pending (kubernetes-sigs#1936)

67f44fa

* Test StrictFiFO misreporting metrics * Cleanup and expand the integration test

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Count inflight workload as pending #1936

Count inflight workload as pending #1936

mimowo commented Apr 2, 2024 •

edited

netlify bot commented Apr 2, 2024 •

edited

mimowo commented Apr 2, 2024

mimowo commented Apr 3, 2024

mimowo commented Apr 3, 2024

alculquicondor Apr 3, 2024

mimowo Apr 3, 2024

alculquicondor Apr 3, 2024

alculquicondor Apr 3, 2024

alculquicondor Apr 3, 2024

mimowo Apr 3, 2024

alculquicondor Apr 3, 2024

mimowo Apr 4, 2024

alculquicondor Apr 3, 2024

mimowo Apr 3, 2024

alculquicondor left a comment

k8s-ci-robot commented Apr 3, 2024

k8s-ci-robot commented Apr 3, 2024

tenzen-y commented Apr 3, 2024

mimowo commented Apr 4, 2024

tenzen-y commented Apr 4, 2024

mimowo commented Apr 4, 2024

alculquicondor commented Apr 4, 2024

tenzen-y commented Apr 4, 2024

		@@ -153,6 +157,7 @@ func (c clusterQueueBase) Delete(w kueue.Workload) {
		key := workload.Key(w)

		@@ -235,7 +247,11 @@ func (c *clusterQueueBase) Pending() int {
		}

		func (c *clusterQueueBase) PendingActive() int {

Count inflight workload as pending #1936

Count inflight workload as pending #1936

Conversation

mimowo commented Apr 2, 2024 • edited

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 canceled.

mimowo commented Apr 2, 2024

mimowo commented Apr 3, 2024

mimowo commented Apr 3, 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

alculquicondor left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 3, 2024

k8s-ci-robot commented Apr 3, 2024

tenzen-y commented Apr 3, 2024

mimowo commented Apr 4, 2024

tenzen-y commented Apr 4, 2024

mimowo commented Apr 4, 2024

alculquicondor commented Apr 4, 2024

tenzen-y commented Apr 4, 2024

mimowo commented Apr 2, 2024 •

edited

netlify bot commented Apr 2, 2024 •

edited