Small fixes to MultiKueue KEP #8131
Conversation
k8s-ci-robot
added
release-note-none
✅ Deploy Preview for kubernetes-sigs-kueue canceled.
|
k8s-ci-robot
added
the
size/M
kshalot
left a comment
kshalot
left a comment
There was a problem hiding this comment.
I glanced at the rest of the KEP and noticed some more things that could be fixed grammar-wise ("an new" -> "a new", "less jobs" -> "fewer jobs" etc.), but I'm not saying you should fix those. It does make me wonder whether spellchecking couldn't be automated in some way.
| While the Workload is pending, the dispatcher can add or remove clusters from the list, and the Workload | ||
| Controller synchronizes the value of the field with the subset of worker clusters with the Workload copy. | ||
| While the workload is pending, the dispatcher can add or remove clusters from the list, and the MultiKueue Workload | ||
| Controller synchronizes the value of the field with the subset of worker clusters with the workload copy. |
There was a problem hiding this comment.
Huge nit: Question to your changes to capitalization ("Workload" -> "workload"). I see that in some places the word is still capitalized. If I understand correctly, the difference would be:
- "Workload" refers to the Kueue workload entity.
- "workload" refers to some specific unit of work, a specific workload.
But this logic would also apply here if I understand correctly.
There was a problem hiding this comment.
My intent was to de-capitalize all occurrences of "workload", except the legitimate cases of Capitalized Proper Names (specifically, MultiKueue Workload Controller), and Capitalized Section Headings.
I just wasn't too meticulous in doing that.
Fixed now.
keps/693-multikueue/README.md
Outdated
| the Kueue's internal Workload object. Only after the workload is admitted and other clusters | ||
| are cleaned-up the real job will be created, to match the Workload. That gives the guarantee | ||
| When distributing a workload across clusters, the MultiKueue Workload Controller will first create | ||
| the Kueue's internal Workload object. Only after the workload is admitted on one cluster and cleaned |
There was a problem hiding this comment.
How about this?
| the Kueue's internal Workload object. Only after the workload is admitted on one cluster and cleaned | |
| an internal Kueue Workload object. Only after the workload is admitted on one cluster and cleaned |
So it's clear that it's an instance rather than some singleton.
I'd also consider expanding this sentence to say where this object is created (IIUC this would be the spec.nominatedClusterNames worker clusters).
There was a problem hiding this comment.
Thanks. I like the direction, though I did it a bit differently.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: kshalot, olekzabl 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 |
keps/693-multikueue/README.md
Outdated
| * Support for cluster role sharing (worker & manager inside one cluster) | ||
| although proved to be possible after kubernetes/enhancements#4370 was merged. | ||
| * distribute running Jobs across multiple clusters, and reconcile partial | ||
| * Distribute a running Job across multiple clusters, and reconcile partial |
There was a problem hiding this comment.
Maybe: "distribute and run a single job across multiple clusters"
Fair point, though such minor things weren't my focus. I rather wanted to fix stuff which actually distracted me in reading. (And that part would be harder to detect automatically, I imagine). |
4 participants
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
It's a bag of fixes for small issues which I randomly spotted while reading the KEP.
Specifically:
listTypeofnominatedClusterNamesto reality. (It was changed fromsettoatomicin this commit).Which issue(s) this PR fixes:
Special notes for your reviewer:
Does this PR introduce a user-facing change?