Expose available ResourceFlavors from the ClusterQueue in the LocalQueue status. #3143
Conversation
k8s-ci-robot
added
release-note
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mbobrovskyi 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 |
k8s-ci-robot
added
cncf-cla: yes
✅ Deploy Preview for kubernetes-sigs-kueue ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
/cc @alculquicondor |
pkg/cache/cache.go
Outdated
| availableFlavors := set.New[kueue.ResourceFlavorReference]() | ||
| for _, rg := range cqImpl.ResourceGroups { | ||
| for _, fl := range rg.Flavors { | ||
| if _, ok := c.resourceFlavors[fl]; ok { |
There was a problem hiding this comment.
The question, should we check that the ResourceFlavor exists? Or just enough the list of available on CQ? In the first case, we would also need to reconcile LocalQueues after updating the ResourceFlavors.
@mimowo @alculquicondor WDYT?
There was a problem hiding this comment.
Just adding a few thoughts: on running a quick example, where RF is not available, but CQ and LQ are created, the LQ status appears to be:
Status:
Admitted Workloads: 0
Conditions:
Last Transition Time: 2024-09-27T02:46:32Z
Message: Can't submit new workloads to clusterQueue
Observed Generation: 1
Reason: ClusterQueueIsInactive
Status: False
Type: Active
Flavor Usage:
Name: default-flavor
Resources:
Name: cpu
Total: 0
Name: memory
Total: 0
Flavors Reservation:
Name: default-flavor
Resources:
Name: cpu
Total: 0
Name: memory
Total: 0
Pending Workloads: 0
Reserving Workloads: 0
The source of truth for populating the status of LQ is the CQ. And the LQ also explicitly bubbles up that the ClusterQueue is in inactive status. Considering the same approach here, it is probably enough to look at the CQ spec for the referenced flavours to populate the status, as we are anyway bubbling up the errors from CQ. One thing that we may change to be more explicit could be to rename the field to be - availableFlavorsInClusterQueue to be more explicit that it refers to the snapshot from the cluster queue's spec, rather than the resource flavour's status on cluster?
There was a problem hiding this comment.
One thing that we may change to be more explicit could be to rename the field to be - availableFlavorsInClusterQueue to be more explicit that it refers to the snapshot from the cluster queue's spec, rather than the resource flavour's status on cluster?
Yeah, availableFlavorsInClusterQueue sounds better. Thanks!
There was a problem hiding this comment.
@varshaprasad96 thank you for the input.
In the KEP I propose a slightly more flexible API as a list of extendable objects: (see related threads: one and two).
Then, we could in the future convey more information (say about the RF status). As the model is more flexible I think we don't need to necessarily indicate the source of the information in the field name. I think we could just describe the meaning in API comment, and shorten the name. WDYT?
|
/retest pull-kueue-test-integration-main |
|
@mbobrovskyi: The
Use
In response to this:
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. |
|
/test pull-kueue-test-integration-main Due to #3144. |
mbobrovskyi
force-pushed
the
feature/expose-flavors-in-local-queue-status
branch
from
c9a0f42
to
61629ef
Compare
|
Is there any decision to implement this API in the LocalQueue level? |
|
I don't see any high-level concerns. Do you have any? |
mbobrovskyi
force-pushed
the
feature/expose-flavors-in-local-queue-status
branch
from
61629ef
to
eba6ab5
Compare
mbobrovskyi
changed the title
|
I don't have any high-level concerns about the approach. However, from the process perspective it might be better to include a small one-pager KEP for it? WDYT @alculquicondor @tenzen-y ? |
|
Yeah, it doesn't hurt |
Basically, I agree with this enhancement. |
I did not indicate that we should expose that information. But, evaluations would be worth it. |
Created #3181. |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Expose available ResourceFlavors from the ClusterQueue in the LocalQueue status.
Which issue(s) this PR fixes:
Fixes #3122
Special notes for your reviewer:
Does this PR introduce a user-facing change?