Repository navigation
feat(scheduler): set scheduling-class label on mutated pods - #2621
Aleksei Sviridkin (lexfrei) merged 1 commit into
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configurationConfiguration used: Repository: cozystack/cozystack/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: cozystack/cozystack/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe webhook now adds the selected scheduling-class label to Pods alongside the scheduler name and annotation. Tests cover successful injection, label preservation, and cases where injection does not occur. ChangesScheduling class injection
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Feature Merge Risk: ⚪ Minimal · up to The change adds scheduling-class labels while preserving existing scheduling metadata and unrelated Pod labels. No actionable merge-blocking risk is identified; mergeable subject to normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The label uses the existing class-selection checks and is published alongside the existing scheduling metadata. No introduced security vulnerability was established, but downstream selector scope, tenant permissions, and membership cleanup remain unverified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
8bc7135 to
299994b
Compare
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
LGTM — additive, backward compatible, and the new tests pin both the injection and the no-op paths.
Business context: a SchedulingClass whose podAffinity rules use a labelSelector cannot match an annotation, so the webhook now also sets scheduler.cozystack.io/scheduling-class as a Pod label alongside the existing annotation.
What I verified: the annotation and schedulerName behaviour is untouched, existing Pod labels are preserved (TestApplySchedulingClass_PreservesExistingLabels), and non-Pod objects plus a missing SchedulingClass CR both remain no-ops. As the code stands, the class name is resolved from the namespace's own scheduler.cozystack.io/scheduling-class label, so the value is already constrained to a valid label value — copying it onto the Pod introduces no new validation risk. All five new tests pass locally.
Non-blocking follow-ups
- Once a per-application source for the class lands (#2622 reads it from
spec.schedulingClassin user-supplied values), the value will no longer be pre-validated by having to be a namespace label. A Pod label value is limited to 63 characters and a restricted charset, while aSchedulingClassobject name may be up to 253; a class name outside the label-value grammar would make the Pod fail apiserver validation after this webhook mutates it. Worth constraining the field where it is introduced. - The label only lands on Pods admitted after this ships. A
SchedulingClasswhosepodAffinity.labelSelectortargets the label will not match Pods created before the upgrade until they are recreated — worth a line in the release note so operators do not expect immediate co-location. - No documentation change accompanies this. The
SchedulingClassdocs should mention that mutated Pods now carry the label, since selecting on it is the entire point of the change.
|
This PR has had no activity for 60 days and was marked |
|
Out of draft. This was approved on 9 July and then sat as a draft, which is why it never came up for merge — my oversight, not a pending change. It still reports Nothing has changed in the branch since the approval, so this is ready whenever someone has a moment. |
299994b to
2a15ad9
Compare
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
mattia-eleuteri LGTM. I rebased this onto current main. Main now has its own webhook_test.go, so your tests are appended to it, and main's test that lists every patch operation now expects the new label too. Removing the label line from webhook.go turns three tests red.
2a15ad9 to
85201c2
Compare
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
mattia-eleuteri I rebased this onto current main and resolved the conflict with #2667 by keeping both changes.
The lineage-controller-webhook now sets a scheduler.cozystack.io/scheduling-class label on mutated pods (in addition to the existing annotation of the same name). This unlocks SchedulingClass authors writing podAffinity rules whose labelSelector targets pods across different applications that share the same class. A natural pattern becomes: define a class whose name is the affinity-group identifier, attach it to several applications, and have the scheduler co-locate them at the chosen topology key (region, zone, hostname). Behavior preserved when the SchedulingClass CR is missing: pods are left untouched (no scheduler injection, no label) so they do not stay Pending on a non-existent scheduler. Tests cover label/annotation/schedulerName injection, existing label preservation, non-Pod no-op, no-class no-op, and missing-CR no-op. Signed-off-by: mattia-eleuteri <[email protected]>
85201c2 to
250f89d
Compare
What this PR does
Extends the lineage-controller-webhook so that, on every Pod it mutates with a
SchedulingClass, it also sets a label of the same name (in addition to the existing annotation).Today, when a SchedulingClass is in effect on a tenant, the webhook injects:
spec.schedulerName: cozystack-schedulerscheduler.cozystack.io/scheduling-class: <name>This is enough for the cozystack-scheduler to look the class up and apply its
nodeAffinity,nodeSelector, and topology rules. However, it is not enough to write aSchedulingClasswhosespec.podAffinityruleslabelSelectortargets pods across multiple applications, becauselabelSelectormatches labels and not annotations.This PR adds the label, leaving the annotation untouched for backward compatibility:
The constants
SchedulingClassLabelandSchedulingClassAnnotationexported bygithub.com/cozystack/cozystack-scheduler/pkg/apis/v1alpha1already share the same value (scheduler.cozystack.io/scheduling-class), so no new key is introduced.Why this is useful: a single SchedulingClass can now express "co-locate every pod that uses me at the same topology" with a rule like:
When several applications share this class, the second-and-later pods will be constrained to the region the first one landed on — without hard-coding any region in the class itself.
Tests
Adds a
webhook_test.go(none existed in the package). Five unit tests, usingclientfake.NewClientBuilder+dynamicfake.NewSimpleDynamicClientWithCustomListKinds, cover:schedulerNameare all set when a class exists.SchedulingClassCR does not exist are a no-op (the existing defensive behavior, preserved).Release note
Summary by CodeRabbit