KEP 168: Visibility for cluster queue #1069

stuton · 2023-08-18T14:40:44Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

First iteration: support visibility for ClusterQueues

Which issue(s) this PR fixes:

Part of #168

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Expose cluster queue information about pending workloads

k8s-ci-robot · 2023-08-18T14:40:53Z

Hi @stuton. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

netlify · 2023-08-18T14:40:54Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`3b1f7c1`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/64f9f96ab1bb420007599076

stuton · 2023-08-18T14:41:33Z

/assign

alculquicondor · 2023-08-21T14:10:49Z

/ok-to-test

apis/config/v1beta1/configuration_types.go

apis/kueue/v1beta1/clusterqueue_types.go

charts/kueue/templates/rbac/role.yaml

apis/config/v1beta1/defaults.go

main.go

pkg/config/validation.go

pkg/config/config.go

pkg/queue/manager.go

pkg/queue/cluster_queue_interface.go

pkg/queue/cluster_queue_impl.go

pkg/config/config.go

pkg/config/validation.go

pkg/queue/manager.go

mimowo · 2023-08-28T13:00:26Z

/assign

mimowo

LGTM, minor naming nits

pkg/controller/core/clusterqueue_controller_test.go

mimowo · 2023-09-07T16:34:18Z

/lgtm
@alculquicondor over to you
@stuton it would be nice if you could squash the commits. I will renew lgtm.

alculquicondor

Some recommendations that can be done as follow ups or now

/hold
/approve

alculquicondor · 2023-09-07T20:28:54Z

pkg/controller/core/clusterqueue_controller.go

+	}
+}
+
+var DefaultOptions = ClusterQueueReconcilerOptions{}


Does it need to be exported?

It does not need to be exported. But there is a collision with another defaultOptions in the package. We can rename it to defaultCqOptions. Ticketed: #1100

alculquicondor · 2023-09-07T20:38:09Z

pkg/controller/core/clusterqueue_controller.go

+	return cq.Status.PendingWorkloadsStatus
+}
+
+func (r *ClusterQueueReconciler) Start(ctx context.Context) error {


what calls this function? Is it internally called within SetupWithManager?

It feels a bit like an overload to make the Reconciler also be the one that calls the reconciler? If possible, I would split it into its own struct.

I have traced this a little. The clusterqueue_controller.Start is called from here: https://github.com/kubernetes-sigs/controller-runtime/blob/2a553d6f910dfc4b0796142ba1e059639a21c54a/pkg/manager/runnable_group.go#L223C17-L223C22.

So, the Start is registered in the runnable Other group here, and Started here: https://github.com/kubernetes-sigs/controller-runtime/blob/2a553d6f910dfc4b0796142ba1e059639a21c54a/pkg/manager/internal.go#L417.

The start we call from within Kueue:

kueue/cmd/kueue/main.go

Line 175 in 330505e

if err := mgr.Start(ctx); err != nil {

.

So calling SetupWithManager does not call directly the Start, but registers the runnable.

I see, seems possible. There could be something like ClusterQueueSnapshotManager.

Ticketed: #1099

alculquicondor · 2023-09-07T20:41:14Z

pkg/queue/cluster_queue_impl.go

+	}
+	for _, e := range c.inadmissibleWorkloads {
+		elements = append(elements, e)
+	}


could we release the lock at this point?

Yes, the sorting can be done outside the lock.

Ticketed as: #1098

k8s-ci-robot · 2023-09-07T20:44:46Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

mimowo · 2023-09-08T09:21:44Z

Some recommendations that can be done as follow ups or now

/hold /approve

All are good points, but I think it will be better to do follow ups as @stuton is taking a week off, so waiting with the PR would risk solving merge conflicts.

mimowo · 2023-09-08T09:23:02Z

xref: #1098

mimowo · 2023-09-08T09:25:08Z

/hold cancel
As suggested the remaining recommendations can be fixed as follow ups (#1069 (review)).

mimowo · 2023-09-08T09:38:19Z

/test pull-kueue-verify-main

tenzen-y · 2023-09-08T10:37:40Z

@stuton @mimowo Thank you for this great work. As I can see KEP, this feature is enabled only when featureGatete, QueueVisibility is enabled. However, I can not see the implementation of the featureGate in this PR. Is this intended?

mimowo · 2023-09-08T10:51:52Z

@stuton @mimowo Thank you for this great work. As I can see KEP, this feature is enabled only when featureGatete, QueueVisibility is enabled. However, I can not see the implementation of the featureGate in this PR. Is this intended?

@tenzen-y it's an omission. Would you like to create an issue to follow up?

tenzen-y · 2023-09-08T11:12:13Z

@stuton @mimowo Thank you for this great work. As I can see KEP, this feature is enabled only when featureGatete, QueueVisibility is enabled. However, I can not see the implementation of the featureGate in this PR. Is this intended?

@tenzen-y it's an omission. Would you like to create an issue to follow up?

Yes, I prefer to disable this feature as a default since this feature seems to be a bit aggressive. Any thought? > @alculquicondor

alculquicondor · 2023-09-08T13:33:09Z

Sure thing, let's put it under a feature gate.

tenzen-y · 2023-09-08T15:54:21Z

Created: #1102

k8s-ci-robot requested review from ahg-g and trasc August 18, 2023 14:40

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 18, 2023

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 18, 2023

k8s-ci-robot assigned stuton Aug 18, 2023

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 21, 2023

alculquicondor reviewed Aug 21, 2023

View reviewed changes

stuton force-pushed the kep-168 branch 3 times, most recently from 84205b9 to cd06b83 Compare August 22, 2023 08:53

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. 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 Aug 24, 2023

stuton force-pushed the kep-168 branch from 21b17ee to 032c77f Compare August 24, 2023 11:51

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

alculquicondor reviewed Aug 24, 2023

View reviewed changes

alculquicondor mentioned this pull request Aug 24, 2023

Add support for single pods #1072

Merged

stuton force-pushed the kep-168 branch from 81a2384 to 09e73cf Compare August 25, 2023 12:08

alculquicondor reviewed Aug 25, 2023

View reviewed changes

pkg/config/config.go Outdated Show resolved Hide resolved

pkg/config/validation.go Outdated Show resolved Hide resolved

pkg/config/validation.go Outdated Show resolved Hide resolved

pkg/queue/manager.go Outdated Show resolved Hide resolved

k8s-ci-robot assigned mimowo Aug 28, 2023

mimowo reviewed Sep 7, 2023

View reviewed changes

pkg/controller/core/clusterqueue_controller_test.go Outdated Show resolved Hide resolved

pkg/controller/core/clusterqueue_controller_test.go Outdated Show resolved Hide resolved

pkg/controller/core/clusterqueue_controller_test.go Outdated Show resolved Hide resolved

update naming

3b1f7c1

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

alculquicondor reviewed Sep 7, 2023

View reviewed changes

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

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

This was referenced Sep 8, 2023

Decouple the ClusterQueue snapshots queue from the CQ reconciler #1099

Closed

Do not export DefaultOptions is not needed #1100

Closed

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

k8s-ci-robot merged commit 61ab18f into kubernetes-sigs:main Sep 8, 2023

k8s-ci-robot added this to the v0.5 milestone Sep 8, 2023

mimowo mentioned this pull request Sep 11, 2023

Add QueueVisibility to featureGate #1102

Closed

stuton mentioned this pull request Oct 25, 2023

REQUEST: New membership for stuton kubernetes/org#4543

Closed

9 tasks

alculquicondor mentioned this pull request Oct 25, 2023

Adding featuregate for QueueVisibility #1139

Merged

mimowo mentioned this pull request Nov 23, 2023

Introduce an on-demand API endpoint for fetching pending workloads in a cluster queue #1251

Merged

trasc deleted the kep-168 branch March 12, 2024 08:59

trasc restored the kep-168 branch March 12, 2024 08:59

trasc deleted the kep-168 branch March 12, 2024 08:59

alculquicondor mentioned this pull request Apr 3, 2024

Count inflight workload as pending #1936

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KEP 168: Visibility for cluster queue #1069

KEP 168: Visibility for cluster queue #1069

stuton commented Aug 18, 2023 •

edited

Loading

k8s-ci-robot commented Aug 18, 2023

netlify bot commented Aug 18, 2023 •

edited

Loading

stuton commented Aug 18, 2023

alculquicondor commented Aug 21, 2023

mimowo commented Aug 28, 2023

mimowo left a comment

mimowo commented Sep 7, 2023

alculquicondor left a comment

alculquicondor Sep 7, 2023

mimowo Sep 8, 2023 •

edited

Loading

alculquicondor Sep 7, 2023

mimowo Sep 8, 2023 •

edited

Loading

mimowo Sep 8, 2023

alculquicondor Sep 7, 2023

mimowo Sep 8, 2023 •

edited

Loading

mimowo Sep 8, 2023

k8s-ci-robot commented Sep 7, 2023

mimowo commented Sep 8, 2023 •

edited

Loading

mimowo commented Sep 8, 2023

mimowo commented Sep 8, 2023

mimowo commented Sep 8, 2023

tenzen-y commented Sep 8, 2023

mimowo commented Sep 8, 2023

tenzen-y commented Sep 8, 2023 •

edited

Loading

alculquicondor commented Sep 8, 2023

tenzen-y commented Sep 8, 2023

KEP 168: Visibility for cluster queue #1069

KEP 168: Visibility for cluster queue #1069

Conversation

stuton commented Aug 18, 2023 • edited Loading

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?

k8s-ci-robot commented Aug 18, 2023

netlify bot commented Aug 18, 2023 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

stuton commented Aug 18, 2023

alculquicondor commented Aug 21, 2023

mimowo commented Aug 28, 2023

mimowo left a comment

Choose a reason for hiding this comment

mimowo commented Sep 7, 2023

alculquicondor left a comment

Choose a reason for hiding this comment

alculquicondor Sep 7, 2023

Choose a reason for hiding this comment

mimowo Sep 8, 2023 • edited Loading

Choose a reason for hiding this comment

alculquicondor Sep 7, 2023

Choose a reason for hiding this comment

mimowo Sep 8, 2023 • edited Loading

Choose a reason for hiding this comment

mimowo Sep 8, 2023

Choose a reason for hiding this comment

alculquicondor Sep 7, 2023

Choose a reason for hiding this comment

mimowo Sep 8, 2023 • edited Loading

Choose a reason for hiding this comment

mimowo Sep 8, 2023

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 7, 2023

mimowo commented Sep 8, 2023 • edited Loading

mimowo commented Sep 8, 2023

mimowo commented Sep 8, 2023

mimowo commented Sep 8, 2023

tenzen-y commented Sep 8, 2023

mimowo commented Sep 8, 2023

tenzen-y commented Sep 8, 2023 • edited Loading

alculquicondor commented Sep 8, 2023

tenzen-y commented Sep 8, 2023

stuton commented Aug 18, 2023 •

edited

Loading

netlify bot commented Aug 18, 2023 •

edited

Loading

mimowo Sep 8, 2023 •

edited

Loading

mimowo Sep 8, 2023 •

edited

Loading

mimowo Sep 8, 2023 •

edited

Loading

mimowo commented Sep 8, 2023 •

edited

Loading

tenzen-y commented Sep 8, 2023 •

edited

Loading