Add priority_class label to kueue_local_queue_resource_reservation#8426
Add priority_class label to kueue_local_queue_resource_reservation#8426andrewseif wants to merge 2 commits intokubernetes-sigs:mainfrom
Conversation
k8s-ci-robot
added
release-note-none
✅ Deploy Preview for kubernetes-sigs-kueue ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @andrewseif. Thanks for your PR. I'm waiting for a github.com 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. DetailsInstructions 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: andrewseif The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
k8s-ci-robot
added
size/M
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
| for _, flavor := range queue.Status.FlavorsReservation { | ||
| for _, r := range flavor.Resources { | ||
| metrics.ReportLocalQueueResourceReservations(localQueueReferenceFromLocalQueue(queue), string(flavor.Name), string(r.Name), resource.QuantityToFloat(&r.Total), tracker) | ||
| metrics.ReportLocalQueueResourceReservations(localQueueReferenceFromLocalQueue(queue), string(flavor.Name), string(r.Name), resource.QuantityToFloat(&r.Total), "", tracker) |
There was a problem hiding this comment.
Not sure I got why empty string?
There was a problem hiding this comment.
as far as I understand, we are reporting aggregate metrics from queue.Status.FlavorsReservation, which represents the total resources across all workloads, without per-priority-class breakdown.
Since the data structure doesn't contain priority class information here, I passed an empty string "" to indicate "all priority classes combined" or "unspecified priority class".
I am not sure this is correct, I would love to know your thoughts, about what should be passed here
There was a problem hiding this comment.
Since the data structure doesn't contain priority class information here, I passed an empty string "" to indicate "all priority classes combined" or "unspecified priority class".
What’s the idea behind this? Why do we need a useless label?
There was a problem hiding this comment.
The idea in the original ticket #8249 , is to add priority class to local_queue_resource_reservation.
But we also need to add priorityClass to ReportLocalQueueResourceReservations, which means I need to add it to this for loop, but the current data structure(AKA LocalQueue ) doesn't have the priority.
Now I know we need to get the priority for each workload inside the queue, but I'm not sure how we can do that without doing some changes.
We could also just remove the PriorityClass from the report function?
There was a problem hiding this comment.
We could also just remove the PriorityClass from the report function?
Do we have this? I thought you’d added it.
|
Please add an integration test to cover this. |
|
lets put this PR on hold for now, until we get further info about the design changes. |
|
/hold |
k8s-ci-robot
added
the
do-not-merge/hold
|
@andrewseif: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. DetailsInstructions 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. I understand the commands that are listed here. |
|
PR needs rebase. DetailsInstructions 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. |
k8s-ci-robot
added
the
needs-rebase
3 participants
What type of PR is this?
/kind feature
What this PR does / why we need it:
Add priority_class label to kueue_local_queue_resource_reservation
Which issue(s) this PR fixes:
Part of #8249
Special notes for your reviewer:
Does this PR introduce a user-facing change?
no