-
Notifications
You must be signed in to change notification settings - Fork 251
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
Update AllocatableResourceGeneration #2984
Update AllocatableResourceGeneration #2984
Conversation
✅ Deploy Preview for kubernetes-sigs-kueue ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
/assign @alculquicondor |
/retest |
Good point! Given this fixes a bug - should we add a release note? Also, can we add a test which demonstrates the fix (can be in follow up)? |
@@ -154,6 +154,7 @@ func (r ResourceNode) calculateLendable() map[corev1.ResourceName]int64 { | |||
} | |||
|
|||
func updateClusterQueueResourceNode(cq *clusterQueue) { | |||
cq.AllocatableResourceGeneration += 1 |
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.
Note to myself and potentially other reviewers: I see, so when updating any CQ (see here) we either call updateClusterQueueResourceNode
directly, or call it for each child via updateCohortResourceNode
. This way any CQ update entails generation update of all CQs in the cohort.
LGTM, but I would like to better understand the impact of the change on the end user - please add release note if there is any. Then, the PR might be categorized as bugfix and me may consider cherry-picking. |
Discussed offline. I will add a release note and classify this as a bug. I don't think the test is worth the effort. It would have to be an integration test, as we are deleting the old field which we could check doesn't increase in a unit test. Additionally, I think we're well covered by testing the |
/kind bug |
We have also discussed the alternative approach of keeping the Generation in the tree root cohort (or ClusterQueue) only. However, this would be more involving as (1) some CQs are not part of any cohort, (2) would need non-trivial updates in case of HierarchicalCohorts when the root is deleted, or added. Finally, the implemented approach does not increase computational complexity as it injects the bumps to already existing function invocations. /lgtm |
LGTM label has been added. Git tree hash: 3908dbea0bddb06325d2d74bf71e834b5ee3aa15
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gabesaba, 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:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind cleanup
/kind bug
What this PR does / why we need it:
We update
AllocatableResourceGeneration
to be compatible withHierarchicalCohorts
(#79). We delete this number from Cohorts, and only store it in the ClusterQueue. After this change, if an update occurs in any part of the tree, we bump the ClusterQueues' numbers when running the resource update.The previous implementation would be complex with
HierarchicalCohorts
- we would have to do a root traversal to see if any of the generations increased. Additionally, it was non-monotonic and contained a bug: if a ClusterQueue was deleted, the Cohort's generation could decrease, despite the available resources of the Cohort having had changed.Does this PR introduce a user-facing change?