[TASK] Correct controller reference management for Workspace owned resources
- 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
Fix how the Workspace controller manages controller references on owned resources (StatefulSet,
Service, VirtualService) to handle re-adoption and version skew correctly.
Today, if a Workspace is deleted and re-created with the same name, the existing owned resources
retain the old Workspace's controller reference. The reconciler's call to
`SetControllerReference` fails because a different controller is already set. The resources
become unmanageable — updates fail silently and the workspace enters an inconsistent state.
Additionally, the owner reference field indexes in `internal/helper/index.go` match on exact
`APIVersion` string (e.g., `kubeflow.org/v1beta1`). This will break if the CRD is ever promoted
to a new version, since the owner reference on existing resources will still reference the old
API version.
This task addresses three tightly coupled fixes:
#### 1. `ReplaceWorkspaceAsController` helper
Create a helper function that safely replaces the controller reference on an owned resource:
- If the resource is already controlled by the given Workspace, no-op
- If controlled by a different Workspace, remove the old controller reference and set the new one
- If controlled by a non-Workspace resource, return an error (refuse to steal from other controllers)
Location: `internal/helper/helper.go`
#### 2. Apply to all owned resource types in reconciliation
During reconciliation, when the controller finds an existing owned resource (StatefulSet, Service,
VirtualService), call `ReplaceWorkspaceAsController` before updating its fields. This ensures
the current Workspace is always the controller of its resources.
Also fix the `SetControllerReference` error handling for freshly generated resources. Currently,
failure to set the controller reference on a just-generated resource produces a user-visible
`WorkspaceStateError`. This should be an internal error (log + return error) since it indicates
a programming bug, not a user-actionable problem.
Location: `internal/controller/workspace_controller.go`
#### 3. Fix owner reference index matching
Change the owner reference index functions to compare by API group rather than exact API version
string. This ensures the indexes continue to work across CRD version promotions.
Before:
```go
if owner.APIVersion != kubefloworgv1beta1.GroupVersion.String() || owner.Kind != OwnerKindWorkspace {
```
After:
```go
ownerGV, err := schema.ParseGroupVersion(owner.APIVersion)
if err != nil {
return nil
}
if ownerGV.Group != kubefloworgv1beta1.GroupVersion.Group || owner.Kind != OwnerKindWorkspace {
```
Location: `internal/helper/index.go` — all three index functions (StatefulSet, Service, VirtualService)
#### Goals
- `ReplaceWorkspaceAsController` helper function
- Apply controller reference replacement to all 3 owned resource types (StatefulSet, Service,
VirtualService)
- Fix `SetControllerReference` error handling on freshly generated resources
- Fix owner reference index matching to use API group
- Unit tests for the helper function
- Unit tests for the index matching fix
#### Key Files
| File | Changes |
|------|---------|
| `workspaces/controller/internal/helper/helper.go` | Add `ReplaceWorkspaceAsController` |
| `workspaces/controller/internal/helper/index.go` | Fix owner matching to use API group |
| `workspaces/controller/internal/controller/workspace_controller.go` | Apply controller reference replacement + fix error handling |
### Acceptance Criteria
- [ ] `ReplaceWorkspaceAsController` correctly replaces controller reference from one Workspace to another
- [ ] `ReplaceWorkspaceAsController` is a no-op when the resource is already controlled by the target Workspace
- [ ] `ReplaceWorkspaceAsController` returns an error when the resource is controlled by a non-Workspace resource
- [ ] Controller reference replacement is applied to all 3 owned resource types (StatefulSet, Service, VirtualService)
- [ ] `SetControllerReference` failures on freshly generated resources are treated as internal errors,
not user-visible `WorkspaceStateError`
- [ ] Owner reference index functions match by API group, not exact API version string
- [ ] Unit tests cover: re-adoption between Workspaces, no-op for same Workspace, rejection of
non-Workspace controller, API version skew matching
- [ ] Existing e2e tests pass without regression
Contributor guide
Assessment
This issue has not been assessed yet.