kubeflow / kubeflow/notebooks

[TASK] Watch Warning Events for owned Pod/StatefulSet

Open
#1,376 1 comment 0 reactions 1 assignee Claimed by @siyuanfoundation 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

The `WorkspaceReconciler` today does not subscribe to the Kubernetes `Event` stream for its owned Pod and StatefulSet. Warning Events (`FailedScheduling`, `FailedMount`, `FailedCreatePodSandBox`, PVC binding failures, image pull backoffs) are already consumed by `generateWorkspaceState` — see the `lastStsWarningEvent` / `lastPodWarningEvent` blocks in `workspace_controller.go` — but only when reconciliation happens to run for another reason. Between triggers, `status.state` and `status.stateMessage` are stale.

This task wires up a Warning-Event watch on the Workspace controller, backed by a new controller-runtime field indexer keyed on the UIDs of the currently-owned Pod and STS, recorded in `Workspace` status. State generation logic itself is not changed — only the set of triggers that cause it to run.

#### Key Files to Modify

```
| File | Role |
| ---- | ---- |
| `workspaces/controller/api/v1beta1/workspace_types.go` | Add `UID` field to `WorkspacePodStatus`; add new `WorkspaceStatefulSetStatus` type; add `PodTemplateStatefulSet` field to `WorkspaceStatus` |
| `workspaces/controller/api/v1beta1/zz_generated.deepcopy.go` | Regenerated by `make generate` |
| `workspaces/controller/manifests/kustomize/base/crd/kubeflow.org_workspaces.yaml` | Regenerated by `make manifests` |
| `workspaces/controller/internal/helper/index.go` | Register a new multi-value field indexer on `Workspace` in `SetupManagerFieldIndexers`, keyed on the UIDs recorded in `status.podTemplatePod` and `status.podTemplateStatefulSet`; export the index key as a new `Index*` constant next to `IndexEventInvolvedObjectUidField` |
| `workspaces/controller/internal/controller/workspace_controller.go` | Add `generateWorkspaceStatefulSetStatus(sts *appsv1.StatefulSet)` helper mirroring `generateWorkspacePodStatus`; populate `status.PodTemplatePod.UID` and `status.PodTemplateStatefulSet` in `generateWorkspaceStatus`; add `Watches(&corev1.Event{}, ...)` in `SetupWithManager` with a Warning-only predicate and a mapper that uses the new indexer |
| `workspaces/controller/internal/controller/workspace_controller_test.go` | Unit tests for the helper, indexer key function, predicate, and Event → reconcile-request mapper |
```

#### Before / After (observable status shape)

Before:
```yaml
status:
podTemplatePod:
name: ws-my-workspace-0
nodeName: node-1
serviceAccountName: ws-my-workspace
containers: [...]
# no podTemplateStatefulSet
```

After:
```yaml
status:
podTemplatePod:
name: ws-my-workspace-0
uid: 5f1e... # new
nodeName: node-1
serviceAccountName: ws-my-workspace
containers: [...]
podTemplateStatefulSet: # new
name: ws-my-workspace
uid: 3ab2...
```

#### Field Indexer Implementation

Register a single multi-value indexer on `Workspace` inside `helper.SetupManagerFieldIndexers` (`workspaces/controller/internal/helper/index.go`), alongside the existing indexers. Declare a new exported `Index*` constant next to `IndexEventInvolvedObjectUidField` for the field key.

A single multi-value index (rather than one indexer per owned kind) lets the Event mapper resolve either a Pod or an STS Event through the same `fields.OneTermEqualSelector` call, keeping the Event → reconcile mapping to one indexed list lookup regardless of which kind of resource the Event references. Emitting nothing when both UIDs are empty naturally excludes paused / pre-creation Workspaces from the index.

```go
// in workspaces/controller/internal/helper/index.go

const (
// ...existing constants...
IndexWorkspaceOwnedResourceUIDField = ".status.ownedResourceUIDs"
)

// inside SetupManagerFieldIndexers, alongside the other IndexField calls:
if err := mgr.GetFieldIndexer().IndexField(
context.Background(),
&kubefloworgv1beta1.Workspace{},
IndexWorkspaceOwnedResourceUIDField,
func(rawObj client.Object) []string {
ws := rawObj.(*kubefloworgv1beta1.Workspace)
uids := make([]string, 0, 2)
if uid := ws.Status.PodTemplatePod.UID; uid != "" {
uids = append(uids, string(uid))
}
if uid := ws.Status.PodTemplateStatefulSet.UID; uid != "" {
uids = append(uids, string(uid))
}
return uids
},
); err != nil {
return err
}
```

The Event mapper in `workspace_controller.go` then resolves the owning Workspace with a
single indexed list call:

```go
func (r *WorkspaceReconciler) mapEventToRequest(ctx context.Context, obj client.Object) []reconcile.Request {
event, ok := obj.(*corev1.Event)
if !ok || event.InvolvedObject.UID == "" {
return nil
}

workspaces := &kubefloworgv1beta1.WorkspaceList{}
listOpts := &client.ListOptions{
FieldSelector: fields.OneTermEqualSelector(
helper.IndexWorkspaceOwnedResourceUIDField,
string(event.InvolvedObject.UID),
),
Namespace: event.InvolvedObject.Namespace,
}
if err := r.List(ctx, workspaces, listOpts); err != nil {
return nil
}

requests := make([]reconcile.Request, 0, len(workspaces.Items))
for _, ws := range workspaces.Items {
requests = append(requests, reconcile.Request{
NamespacedName: client.ObjectKeyFromObject(&ws),
})
}
return requests
}
```

Because the indexer reads from `status`, the reconciler must have written the current UIDs before the next Event fires — satisfied by populating `status.podTemplatePod.uid` and `status.podTemplateStatefulSet.{name,uid}` inside `generateWorkspaceStatus` on the same code path that already sets `status.podTemplatePod`. On Pod recreation, a Warning Event on the new Pod may briefly not map to any Workspace until the next reconcile updates status; the existing `Owns(&StatefulSet{})` and label-filtered `Pod` watch guarantees that reconcile
runs, after which subsequent Events map correctly.

Two small notes:

1.  IndexWorkspaceOwnedResourceUIDField = ".status.ownedResourceUIDs"  — I picked a synthetic field path that reads like the existing constants ( .metadata.controller ,  .spec.kind ). It doesn't correspond to a literal JSON path since the field is synthesized from two UIDs, but that's how controller-runtime field indexer keys are typically named. Rename freely if you prefer something like  .status.ownedUID .
2. Namespace scoping — the mapper filters by  event.InvolvedObject.Namespace , which matches how  generateWorkspaceState  already lists Events ( Namespace: statefulSet.Namespace ). This prevents cross-namespace collisions if a UID somehow reappeared (defense in depth; UIDs are cluster-unique in practice).

### Acceptance Criteria

### Acceptance Criteria

- [ ] `WorkspaceStatus` contains `podTemplatePod` and `podTemplateStatefulSet` fields, each
exposing at least a `name` and `uid`, populated during reconciliation and empty when
the underlying resource does not exist
- [ ] A field indexer on `Workspace` returns the owning Workspace for a given owned-resource
UID, covering both the Pod and the StatefulSet
- [ ] The controller reconciles a Workspace in response to a Warning `corev1.Event` whose
`involvedObject.uid` matches one of its owned resources; Normal Events do not trigger
reconciles
- [ ] The Event → reconcile mapping resolves via a single indexed lookup, not by listing all
Workspaces

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.