kubeflow / kubeflow/notebooks

[TASK] Add StatefulSet generation and Pod revision guards to Workspace state derivation

Open
#1,374 1 comment 0 reactions 1 assignee Claimed by @kr1xh View on GitHub
kind/plan-task
Dominant language
No language data
Stars
84
Forks
149
Avg merge
5d 15h
Merged PRs (30d)
29

Description

### Certification

- [x] I certify I am an Epic Owner for Kubeflow Notebooks 2.0 and expected to create planning-related issues.

### Description

`generateWorkspaceState` in `workspaces/controller/internal/controller/workspace_controller.go` derives Workspace state from the StatefulSet and Pod objects in the controller's cache, but never accounts for the fact that Kubernetes sub-controllers reconcile asynchronously. In the window between a spec change being written and the StatefulSet sub-controller acting on it, the cached objects describe a mixture of old and new state — an old Pod running the previous revision, alongside a StatefulSet whose spec has already been updated. During this window the function can return a state that does not reflect Kubernetes' true in-progress reconciliation, and downstream consumers (including Activity Rules) can act on that transient signal.

Two conditions must be recognized as first-class signals and short-circuit the existing derivation:

1. **StatefulSet not yet reconciled** — the StatefulSet's `status.observedGeneration` lags its `metadata.generation`. In this state no derived signal is trustworthy, including the effects of our own writes for the pause transition. The check therefore sits at the very top of `generateWorkspaceState`, before the `pod == nil` branch (line 1553), and is unconditional with respect to `paused`. Once `observedGeneration` catches up, all existing branches — `Paused`, `Terminating`, `Running`, etc. — resume unchanged.

2. **Pod pre-dates the current StatefulSet revision** — the Pod's `controller-revision-hash` label does not match `statefulSet.Status.UpdateRevision`. The check sits immediately after the terminating-Pod short-circuit (line 1598) so that an already-deleting Pod continues to report `Terminating` rather than being reclassified.

The "is this object's generation observed?" test should be extracted into a small reusable helper on the StatefulSet type. Feature #1373 anticipates similar generation-lag checks for Service, VirtualService, and other owned resources; the helper makes adding those a localized change later without restructuring `generateWorkspaceState`. Do not add such checks in this task.

Two new message constants belong in the existing `stateMsg*` block near lines 79–102, following the naming convention already used there.

#### Key Files to Modify

| File | Role in the change |
|------|--------------------|
| `workspaces/controller/internal/controller/workspace_controller.go` | Home of `generateWorkspaceState` and the `stateMsg*` constant block — both new checks, the new constants, and the generation-observed helper land here |
| `workspaces/controller/internal/controller/workspace_controller_test.go` | Home of unit-test coverage for the state-derivation branches — the scenarios enumerated in Acceptance Criteria are added here |

### Acceptance Criteria

- [ ] When the Workspace's StatefulSet has `status.observedGeneration != metadata.generation`, `generateWorkspaceState` returns `WorkspaceStateUnknown` with message `Waiting for Kubernetes to reconcile StatefulSet`, regardless of the Pod currently in cache and regardless of whether the Workspace is paused
- [ ] When the Pod exists, is not terminating, and its `controller-revision-hash` label does not match `statefulSet.Status.UpdateRevision`, `generateWorkspaceState` returns `WorkspaceStatePending` with message `Waiting for Pod update`
- [ ] A paused Workspace reports `WorkspaceStateUnknown` during the window where the StatefulSet's `observedGeneration` lags `generation`, and returns to `WorkspaceStatePaused` once the StatefulSet sub-controller catches up
- [ ] A Pod with a non-nil `deletionTimestamp` still reports `WorkspaceStateTerminating`, even when its `controller-revision-hash` label does not match the StatefulSet's `updateRevision`
- [ ] The Pod-revision check is a no-op when `statefulSet.Status.UpdateRevision` is empty (fresh StatefulSet with no revision published yet)

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.