feat: add configurable StatefulSet metadata to WorkspaceKind - #1351
feat: add configurable StatefulSet metadata to WorkspaceKind#1351christian-heusel wants to merge 7 commits into
Conversation
Admins can already attach labels and annotations to Workspace Pods via `spec.podTemplate.podMetadata`, but the generated StatefulSet object itself carries only the controller's `workspace-name` label and no annotations, so there was no way to label or annotate the workload resource (for cost allocation, GitOps tracking, org conventions, etc.). Add an optional `spec.podTemplate.statefulSetMetadata` with `labels` and `annotations` maps, backed by a new `WorkspaceKindStatefulSetMetadata` type that mirrors `WorkspaceKindPodMetadata`. This commit is API surface only; a follow-up wires it into StatefulSet generation. When unset the field is omitted and generated StatefulSets are unchanged, so existing WorkspaceKinds are unaffected. Assisted-by: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Christian Heusel <christian@heusel.eu>
`copyLabelFields` and `copyAnnotationFields` only looped over the target map, so a key present on the desired object but missing from the target was never flagged: `requireUpdate` stayed false and the Update() was skipped, even though the desired map (which is returned wholesale) would have added it. In practice a label or annotation added to a resource only reached the cluster if some other field also changed. This was harmless while StatefulSets carried a single fixed controller label, but it blocks admin-managed metadata: a label added to a WorkspaceKind would not propagate to already-running StatefulSets. Add a second loop over the desired map in both helpers so additions are detected. The two-loop form (rather than a DeepEqual) keeps nil and empty maps equivalent and avoids spurious reconciles. The helpers are shared by CopyStatefulSetFields and CopyServiceFields, so both now propagate additions correctly. Assisted-by: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Christian Heusel <christian@heusel.eu>
The `statefulSetMetadata` field added to the WorkspaceKind CRD was API surface only, the controller did not read it. Resolve it in `generateStatefulSet()` and set the labels and annotations on the StatefulSet's own ObjectMeta, so admins can label and annotate the workload resource itself (for cost allocation, GitOps tracking, org conventions, and selectors that target the StatefulSet rather than its Pods). The admin labels are merged with `labels.Merge` so the controller's `workspace-name` label always wins and cannot be hijacked. Unlike pod metadata, statefulset metadata is only sourced from the WorkspaceKind, there is no per-Workspace override. `helper.CopyStatefulSetFields` already reconciles the StatefulSet's own labels and annotations, so no change was needed there. Assisted-by: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Christian Heusel <christian@heusel.eu>
Add a `statefulSetMetadata` example next to `podMetadata` in the JupyterLab WorkspaceKind sample, so admins can see how to label and annotate the StatefulSet object itself. Assisted-by: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Christian Heusel <christian@heusel.eu>
The backend embeds the `WorkspaceKindPodTemplate` type from the controller verbatim in its create and update models, so the new `statefulSetMetadata` field is already served by the WorkspaceKind endpoints without any changes to the models or handlers. Only the generated OpenAPI spec was left stale. Regenerate it with `make swag`, which adds the field to the `v1beta1.WorkspaceKindPodTemplate` definition and introduces the new `v1beta1.WorkspaceKindStatefulSetMetadata` definition. The spawner facing models are deliberately left untouched, as they intentionally carry no pod spec details today. Assisted-by: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Christian Heusel <christian@heusel.eu>
The GET/list WorkspaceKind endpoints return a hand-written read model (`WorkspaceKindListItem`) that surfaces `podMetadata` but omitted the new `statefulSetMetadata`, so clients could not read the StatefulSet labels and annotations an admin configured. Map `spec.podTemplate.statefulSetMetadata` into the response model alongside `podMetadata`, copying the maps so the response never aliases the WorkspaceKind. Regenerate the OpenAPI spec, which adds the `workspacekinds.StatefulSetMetadata` definition and the field on `workspacekinds.PodTemplate`. The example WorkspaceKind test fixture now sets the field, and the list handler test asserts it is surfaced. Assisted-by: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Christian Heusel <christian@heusel.eu>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
The controller unit tests cover generateStatefulSet directly, but nothing exercised statefulSetMetadata end-to-end against a live cluster. Extend the Operator e2e test to assert that the StatefulSet generated for the sample Workspace carries the labels and annotations configured in the sample WorkspaceKind's spec.podTemplate.statefulSetMetadata. Assisted-by: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Christian Heusel <christian@heusel.eu>
e93b98e to
472ecbb
Compare
andyatmiami
left a comment
There was a problem hiding this comment.
@christian-heusel - thanks for getting this necessary PR raised.
minor nitpicks and suggestions - but nothing here that concerning (outside the alignment of the webhook validation)
let me know if you have any issue/disagreement with my comments. i'd expect we can get this merged after your next push 💯
| Annotations map[string]string `json:"annotations,omitempty"` | ||
| } | ||
|
|
||
| type WorkspaceKindStatefulSetMetadata struct { |
There was a problem hiding this comment.
| type WorkspaceKindStatefulSetMetadata struct { | |
| // WorkspaceKindStatefulSetMetadata defines labels and annotations applied to the Workspace StatefulSet. | |
| type WorkspaceKindStatefulSetMetadata struct { |
There was a problem hiding this comment.
Missing webhook validation for statefulSetMetadata labels and annotations — workspaces/controller/internal/webhook/workspacekind_webhook.go (not changed — but should have been)
- didn't have a better way to call this out 😇
The structurally identical podMetadata field has webhook validation that calls v1validation.ValidateLabels and apivalidation.ValidateAnnotations in both ValidateCreate and ValidateUpdate:
// ValidateCreate
allErrs = append(allErrs, v.validatePodTemplatePodMetadata(workspaceKind)...)
// ValidateUpdate
if !equality.Semantic.DeepEqual(newWorkspaceKind.Spec.PodTemplate.PodMetadata, oldWorkspaceKind.Spec.PodTemplate.PodMetadata) {
allErrs = append(allErrs, v.validatePodTemplatePodMetadata(newWorkspaceKind)...)
}The new statefulSetMetadata has the same structure (map[string]string labels and annotations) but no corresponding validation. Without it, an admin can set invalid label keys (e.g., containing spaces) or invalid annotation values that the API server will reject when the controller tries to create/update the StatefulSet, putting the Workspace into an error state with no clear indication that the WorkspaceKind itself is misconfigured.
There was a problem hiding this comment.
No test coverage for nil StatefulSetMetadata (default values path) —
The CRD field StatefulSetMetadata is an optional pointer (*WorkspaceKindStatefulSetMetadata). The conversion in funcs.go correctly handles nil by initializing empty maps, but NewExampleWorkspaceKind always sets it to a non-nil value with populated maps. No test verifies the nil-to-empty-maps path.
| var item1 models.WorkspaceKindListItem | ||
| for _, wsk := range response.Data { | ||
| if wsk.Name == workspaceKind1Name { | ||
| item1 = wsk | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing guard assertion in test lookup loop
If workspaceKind1Name doesn't match any item, item1 stays zero-valued and subsequent assertions fail with a confusing empty-map message. Consider adding a failesafe a la:
Expect(item1.Name).To(Equal(workspaceKind1Name), "WorkspaceKind %q not found in response", workspaceKind1Name)
| stsAnnotations := make(map[string]string) | ||
| stsLabels := make(map[string]string) | ||
| if workspaceKind.Spec.PodTemplate.StatefulSetMetadata != nil { | ||
| maps.Copy(stsAnnotations, workspaceKind.Spec.PodTemplate.StatefulSetMetadata.Annotations) | ||
| maps.Copy(stsLabels, workspaceKind.Spec.PodTemplate.StatefulSetMetadata.Labels) | ||
| } |
There was a problem hiding this comment.
| stsAnnotations := make(map[string]string) | |
| stsLabels := make(map[string]string) | |
| if workspaceKind.Spec.PodTemplate.StatefulSetMetadata != nil { | |
| maps.Copy(stsAnnotations, workspaceKind.Spec.PodTemplate.StatefulSetMetadata.Annotations) | |
| maps.Copy(stsLabels, workspaceKind.Spec.PodTemplate.StatefulSetMetadata.Labels) | |
| } | |
| var stsAnnotations, stsLabels map[string]string | |
| if m := workspaceKind.Spec.PodTemplate.StatefulSetMetadata; m != nil { | |
| stsAnnotations = maps.Clone(m.Annotations) | |
| stsLabels = maps.Clone(m.Labels) | |
| } |
I think we can express this a little more compactly - albeit its certainly a "nit" comment
| stsLabels := make(map[string]string) | ||
| stsAnnotations := make(map[string]string) | ||
| if wsk.Spec.PodTemplate.StatefulSetMetadata != nil { | ||
| // NOTE: we copy the maps to avoid creating a reference to the original maps. | ||
| maps.Copy(stsLabels, wsk.Spec.PodTemplate.StatefulSetMetadata.Labels) | ||
| maps.Copy(stsAnnotations, wsk.Spec.PodTemplate.StatefulSetMetadata.Annotations) | ||
| } |
Motivation
This PR the first step toward supporting Kueue in Notebooks v2.
cc @kannon92
Kueue gates admission of a workload via the
kueue.x-k8s.io/queue-namelabel on the managed object — for a Notebook Workspace that object is the StatefulSet the controller generates. Today an admin can label/annotate the Workspace Pods (spec.podTemplate.podMetadata) but has no way to put a label on the StatefulSet itself, so a Workspace cannot be opted into a KueueLocalQueue.
This PR adds an admin-only
spec.podTemplate.statefulSetMetadata(labels + annotations) that is applied to the generated StatefulSet's own metadata. Beyond Kueue it is also useful for cost allocation, GitOps/ArgoCD tracking, and org-wide labeling conventions.The field is optional and omitted by default, so existing WorkspaceKinds are completely unaffected.
Related work & roadmap
Label/metadata support for Notebooks v2, so workloads can integrate with systems
like Kueue:
podMetadata(Pods)statefulSetMetadata(StatefulSet)WorkspaceKind.spec.podTemplateWorkspaceKind.spec.podTemplate.options.podConfig[].specThe per-
podConfigvariants (letting a specific pod config carry its ownmetadata, e.g. routing only GPU pod configs to a queue) are planned as
follow-ups and are not part of this PR.
References
Documentation page on using Kueue to run Notebook kubernetes-sigs/kueue#3878 (comment)
Design notes
podMetadata, there is noper-Workspace override and no per-
podConfigvariant (yet) — it is a singledefault sourced from the WorkspaceKind.
labels.Mergesonotebooks.kubeflow.org/workspace-namealways wins andcannot be overridden.
copyLabelFields/copyAnnotationFieldsonlydetected changed/removed keys, never purely added ones, so a label added to
a WorkspaceKind would not propagate to already-running StatefulSets until some
other field changed. Both helpers now also iterate the desired map. (These are
shared with
CopyServiceFields, which benefits from the same correction.)controller's
WorkspaceKindPodTemplateverbatim); this PR also surfaces it inthe hand-written read/list response model so
GET /workspacekindsreturnsit.
Not included / follow-up
podConfigpodMetadata/statefulSetMetadatasupport (see roadmap).npm run generate:api(regeneratingsrc/generated/from the newswagger) — intentionally left as a separate follow-up.