Skip to content

feat(scheduler): set scheduling-class label on mutated pods - #2621

Merged
Aleksei Sviridkin (lexfrei) merged 1 commit into
cozystack:mainfrom
mattia-eleuteri:feat/sched-coloc-label
Oct 1, 2026
Merged

Aleksei Sviridkin (lexfrei) merged 1 commit into
cozystack:mainfrom
mattia-eleuteri:feat/sched-coloc-label

Conversation

@mattia-eleuteri

@mattia-eleuteri mattia-eleuteri commented May 12, 2026 •

Copy link
Copy Markdown
Collaborator

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-scheduler
  • annotation scheduler.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 a SchedulingClass whose spec.podAffinity rules labelSelector targets pods across multiple applications, because labelSelector matches labels and not annotations.

This PR adds the label, leaving the annotation untouched for backward compatibility:

podLabels := obj.GetLabels()
if podLabels == nil {
    podLabels = make(map[string]string)
}
podLabels[schedulerapi.SchedulingClassLabel] = schedulingClass
obj.SetLabels(podLabels)

The constants SchedulingClassLabel and SchedulingClassAnnotation exported by github.com/cozystack/cozystack-scheduler/pkg/apis/v1alpha1 already 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:

apiVersion: cozystack.io/v1alpha1
kind: SchedulingClass
metadata:
  name: co-region-prod
spec:
  podAffinity:
    requiredDuringSchedulingIgnoredDuringExecution:
    - labelSelector:
        matchLabels:
          scheduler.cozystack.io/scheduling-class: co-region-prod
      topologyKey: topology.kubernetes.io/region

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, using clientfake.NewClientBuilder + dynamicfake.NewSimpleDynamicClientWithCustomListKinds, cover:

  • Label, annotation, and schedulerName are all set when a class exists.
  • Existing pod labels are preserved (no clobbering).
  • Non-Pod objects are a no-op.
  • Pods in a namespace without a scheduling-class label are a no-op.
  • Pods whose referenced SchedulingClass CR does not exist are a no-op (the existing defensive behavior, preserved).

Release note

feat(scheduler): the lineage-controller-webhook now sets a scheduler.cozystack.io/scheduling-class label on mutated pods (in addition to the existing annotation), enabling SchedulingClass authors to write podAffinity rules that match pods across applications.

Summary by CodeRabbit

  • New Features
    • Pods with a valid scheduling class now receive its label alongside the scheduler configuration and annotation.
    • Existing Pod labels are preserved when the scheduling-class label is added.
  • Bug Fixes
    • Scheduling-class settings are applied only when the namespace references a scheduling class that exists. Non-Pods and namespaces without a scheduling-class label are left unchanged.

@coderabbitai

coderabbitai Bot commented May 12, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

⚙️ Run configuration

Configuration used: Repository: cozystack/cozystack/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: f18375c8-0170-4e2a-9134-0ae4d457eefb

📥 Commits

Reviewing files that changed from the base of the PR and between 85201c2 and 250f89d.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: cozystack/cozystack/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 22bfeafc-f323-4ce9-80ac-145ee2da46f3

📥 Commits

Reviewing files that changed from the base of the PR and between 299994b and 85201c2.

📒 Files selected for processing (2)
  • internal/lineagecontrollerwebhook/webhook.go
  • internal/lineagecontrollerwebhook/webhook_test.go

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.


📝 Walkthrough

Walkthrough

The 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.

Changes

Scheduling class injection

Layer / File(s) Summary
Apply scheduling-class label
internal/lineagecontrollerwebhook/webhook.go
applySchedulingClass sets the selected scheduling-class label alongside the scheduler name and annotation. Its documentation describes the label's use in pod affinity selectors.
Validate injection and skip conditions
internal/lineagecontrollerwebhook/webhook_test.go
Tests cover successful injection, existing label preservation, non-Pod objects, namespaces without a scheduling-class label, and missing SchedulingClass resources.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Feature

Merge Risk: ⚪ Minimal · up to 85201

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 Review

Security architecture risk: 🔵 Low · up to 85201

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
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The direct write is limited to the admitted Pod, but the new label can influence placement through policies selecting other Pods with the same class. Effective cross-namespace and tenant exposure depends on downstream policy scope and permissions, which were not established.

Trust Boundaries and Controls

  • observed — The changed label remains behind the existing non-Pod guard, class-selection path, and SchedulingClass existence check. Clean-object tests cover non-Pods, an absent namespace class, and a missing class resource; they do not establish authorization or label authenticity for downstream consumers.

Resilience and Maintainability Implications

  • inferred — With an unchanged selected class and successful lookup, repeated mutation writes the same scheduling values. Freshness across concurrent class changes or deletion is not guaranteed by this handler: lookup and admission publication are separate operations, and skipped lookup does not revoke prior metadata. This does not by itself establish a security failure.

Hardening Proposals

  • proposed — If downstream policies use this label for isolation rather than placement alone, define its namespace scope, permitted writers, and removal semantics explicitly. Do not treat its presence as authenticated tenant or class membership without those controls.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 54.55% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: adding the scheduling-class label to mutated Pods.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added area/uncategorized PR auto-labeler could not map title scope to a known area/*; please review kind/feature Categorizes issue or PR as related to a new feature size/L This PR changes 100-499 lines, ignoring generated files labels May 12, 2026
@lexfrei Aleksei Sviridkin (lexfrei) added area/api Issues or PRs related to the cozystack-api aggregated API server area/platform Issues or PRs related to platform infrastructure (bundle, flux, talos, installer) and removed area/uncategorized PR auto-labeler could not map title scope to a known area/*; please review labels May 25, 2026
@lexfrei Aleksei Sviridkin (lexfrei) removed the area/platform Issues or PRs related to platform infrastructure (bundle, flux, talos, installer) label Jul 3, 2026
@mattia-eleuteri
mattia-eleuteri force-pushed the feat/sched-coloc-label branch from 8bc7135 to 299994b Compare July 9, 2026 07:29
@github-actions github-actions Bot added the area/uncategorized PR auto-labeler could not map title scope to a known area/*; please review label Jul 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. Once a per-application source for the class lands (#2622 reads it from spec.schedulingClass in 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 a SchedulingClass object 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.
  2. The label only lands on Pods admitted after this ships. A SchedulingClass whose podAffinity.labelSelector targets 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.
  3. No documentation change accompanies this. The SchedulingClass docs should mention that mutated Pods now carry the label, since selecting on it is the entire point of the change.

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown

This PR has had no activity for 60 days and was marked lifecycle/stale.
It will be closed in 14 days unless commented or labelled lifecycle/frozen.

@github-actions github-actions Bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale label Sep 8, 2026
@mattia-eleuteri
mattia-eleuteri marked this pull request as ready for review September 21, 2026 12:50
@mattia-eleuteri

Copy link
Copy Markdown
Collaborator Author

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 MERGEABLE against current main, and the only red checks are the Build packages/* and Build Talos jobs that fail on every fork PR for want of registry push rights.

Nothing has changed in the branch since the approval, so this is ready whenever someone has a moment.

@github-actions github-actions Bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale label Sep 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]>
@lexfrei
Aleksei Sviridkin (lexfrei) merged commit 97fc435 into cozystack:main Oct 1, 2026
55 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/api Issues or PRs related to the cozystack-api aggregated API server area/uncategorized PR auto-labeler could not map title scope to a known area/*; please review kind/feature Categorizes issue or PR as related to a new feature size/L This PR changes 100-499 lines, ignoring generated files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants