-
Notifications
You must be signed in to change notification settings - Fork 223
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
support running workloads in visibility endpoint #2145
base: main
Are you sure you want to change the base?
support running workloads in visibility endpoint #2145
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: KunWuLuan The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Hi @KunWuLuan. 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 Once the patch is verified, the new status will be reflected by the 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-sigs/prow repository. |
✅ Deploy Preview for kubernetes-sigs-kueue canceled.
|
da55391
to
6cb9546
Compare
/ok-to-test |
96d5e5b
to
a3b368b
Compare
/cc @PBundyra |
do we need to query running workloads for local queues? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
My first pass.
Please add e2e/integration tests and update KEP regarding that. Please add cases which cover:
- pending and running workloads are assigned to a CQ
- a workload changes its status from pending to admitted
- a workload changes its status from admitted to finished
I am leaning towards implementing that. It will be useful for the KueueCtl. It might be a separate PR though |
Yes, a separate PR would be better. |
Signed-off-by: KunWuLuan <kunwuluan@gmail.com>
646f495
to
f8134df
Compare
Signed-off-by: KunWuLuan <kunwuluan@gmail.com>
f8134df
to
b17c351
Compare
Thank you for addressing my comments. Please add missing test scenarios. |
Signed-off-by: KunWuLuan <kunwuluan@gmail.com>
fb688a2
to
4460d3e
Compare
@PBundyra Hi, I think all questions have been addressed. Please have a look if you have time. Thanks very much. |
@@ -83,7 +128,7 @@ implementing this enhancement to ensure the enhancements have also solid foundat | |||
|
|||
#### Unit Tests | |||
|
|||
New unit tests should be added testing the functionality for jobs and pods. | |||
New unit tests should be added testing the functionality for new api. | |||
|
|||
#### Integration tests | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update this section
|
||
### Goals | ||
|
||
* Support list workloads that are admitted and not finished. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Support list workloads that are admitted and not finished. | |
* Support listing running workloads in a ClusterQueue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also add information about the LocalQueue endpoint, even if it won't be implemented immediately please.
Please add reference to this KEP
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
kubectl get --raw "/apis/visibility.kueue.x-k8s.io/v1alpha1/clusterqueues/cluster-queue/runningworkloads" | ||
``` | ||
|
||
We will show priority and localqueue information in response. Like this: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update
We will add a new visibility endpoint in Kueue. | ||
``` go | ||
// RunningWorkload is a user-facing representation of a running workload that summarizes the relevant information for | ||
// assumed resources in the cluster queue. | ||
type RunningWorkload struct { | ||
metav1.ObjectMeta `json:"metadata,omitempty"` | ||
|
||
// Priority indicates the workload's priority | ||
Priority int32 `json:"priority"` | ||
// AdmissionTime indecates the time workloads admitted | ||
AdmissionTime metav1.Time `json:"admissionTime"` | ||
} | ||
|
||
// +k8s:openapi-gen=true | ||
// +kubebuilder:object:root=true | ||
|
||
// RunningWorkloadsSummary contains a list of running workloads in the context | ||
// of the query (within LocalQueue or ClusterQueue). | ||
type RunningWorkloadsSummary struct { | ||
metav1.TypeMeta `json:",inline"` | ||
metav1.ObjectMeta `json:"metadata,omitempty"` | ||
|
||
Items []RunningWorkload `json:"items"` | ||
} | ||
|
||
// +kubebuilder:object:root=true | ||
type RunningWorkloadsSummaryList struct { | ||
metav1.TypeMeta `json:",inline"` | ||
metav1.ListMeta `json:"metadata,omitempty"` | ||
|
||
Items []RunningWorkloadsSummary `json:"items"` | ||
} | ||
|
||
// +kubebuilder:object:root=true | ||
// +k8s:openapi-gen=true | ||
// +k8s:conversion-gen:explicit-from=net/url.Values | ||
// +k8s:defaulter-gen=true | ||
|
||
// RunningWorkloadOptions are query params used in the visibility queries | ||
type RunningWorkloadOptions struct { | ||
metav1.TypeMeta `json:",inline"` | ||
|
||
// Offset indicates position of the first pending workload that should be fetched, starting from 0. 0 by default | ||
Offset int64 `json:"offset"` | ||
|
||
// Limit indicates max number of pending workloads that should be fetched. 1000 by default | ||
Limit int64 `json:"limit,omitempty"` | ||
} | ||
``` | ||
|
||
Users can list the running workloads by using | ||
``` bash | ||
kubectl get --raw "/apis/visibility.kueue.x-k8s.io/v1alpha1/clusterqueues/cluster-queue/runningworkloads" | ||
``` | ||
|
||
We will show priority and localqueue information in response. Like this: | ||
``` | ||
{"kind":"RunningWorkloadsSummary","apiVersion":"visibility.kueue.x-k8s.io/v1alpha1","metadata":{"creationTimestamp":null},"items":[{"metadata":{"name":"job-sample-job-jz228-ef938","namespace":"default","creationTimestamp":"2024-05-06T02:15:26Z","ownerReferences":[{"apiVersion":"batch/v1","kind":"Job","name":"sample-job-jz228","uid":"2de8a359-4c95-4159-b677-0279066149b6"}]},"priority":0,"admissionTime":"xxxx"}]} | ||
``` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move this section to Design Details, and add a REST path to the endpoint (not only kubectl usage)
status: draft | ||
creation-date: 2024-04-29 | ||
reviewers: | ||
- "@alculquicondor" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- "@alculquicondor" | |
- "@alculquicondor" | |
- "@pbundyra" |
@@ -0,0 +1,22 @@ | |||
title: List Admitted And Not Finished Workloads | |||
kep-number: 1834 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update
stage: beta | ||
|
||
# The most recent milestone for which work toward delivery of this KEP has been | ||
# done. This can be the current (upcoming) milestone, if it is being actively | ||
# worked on. | ||
latest-milestone: "v0.9" | ||
|
||
# The milestone at which this feature was, or is targeted to be, at each stage. | ||
milestone: | ||
alpha: "v0.9" | ||
beta: "v0.9" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update
Thank you for addressing my comments. Please add e2e/integration tests as I've mentioned above. You can extend existing e2e tests. Another thing we're missing here are RBAC roles. See this file for the reference. Thank you |
## Motivation | ||
|
||
Jsonpath support in kubectl is limited, and we can not filter resources by condition in this way. By adding a new | ||
visibility endpoint, users can list running workloads by `kubectl get`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This functionality is already being worked on in kueuectl list workloads (see: https://github.com/kubernetes-sigs/kueue/blob/8d7f709225bb828c38758a22ccf892f304b1bd0c/keps/2076-kueuectl/README.md#list-workloads
and
#2195). Is there another reason why we might need this extra api?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KunWuLuan before we go further with this PR could you please address that?
PR needs rebase. 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-sigs/prow repository. |
What type of PR is this?
/kind feature
What this PR does / why we need it:
support running workloads in visibility endpoint
Which issue(s) this PR fixes:
Fixes #1776
Special notes for your reviewer:
Does this PR introduce a user-facing change?