Repository navigation
Conversation
WorkerPool StatefulSets were created without a persistentVolumeClaimRetentionPolicy, so Kubernetes retained every worker PVC when a pool was scaled down or deleted, leaving the claims and their backing disks behind indefinitely. Add an optional spec.persistentVolumeClaimRetentionPolicy with whenDeleted and whenScaled (Retain or Delete). The controller sets it on the StatefulSet at creation and updates existing StatefulSets when it changes. Leaving it unset keeps the current Retain behavior. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
github-actions
Bot
added
kind/api
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 8 files
You’re at about 96% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/controller/workerpool_controller_test.go">
<violation number="1" location="internal/controller/workerpool_controller_test.go:455">
P2: The PR says removing a previously set policy reverts to the default, but no reconcile test covers that path. Add a test where the existing StatefulSet has Delete/Delete and the pool's `persistentVolumeClaimRetentionPolicy` is nil, then assert the StatefulSet's policy is nil after reconcile.</violation>
</file>
<file name="internal/controller/workerpool_controller.go">
<violation number="1" location="internal/controller/workerpool_controller.go:509">
P3: A retention-only change now reaches the shared StatefulSet update path, which still logs and emits the event "Updated StatefulSet %s to %d replicas". That message reports a replica count even when only the PVC retention policy changed. Make the message name what changed, or make it generic.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
| }, sts.Spec.PersistentVolumeClaimRetentionPolicy) | ||
| } | ||
|
|
||
| func TestWorkerPoolReconciler_PVCRetentionPolicyUpdatesExistingStatefulSet(t *testing.T) { |
There was a problem hiding this comment.
P2: The PR says removing a previously set policy reverts to the default, but no reconcile test covers that path. Add a test where the existing StatefulSet has Delete/Delete and the pool's persistentVolumeClaimRetentionPolicy is nil, then assert the StatefulSet's policy is nil after reconcile.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At internal/controller/workerpool_controller_test.go, line 455:
<comment>The PR says removing a previously set policy reverts to the default, but no reconcile test covers that path. Add a test where the existing StatefulSet has Delete/Delete and the pool's `persistentVolumeClaimRetentionPolicy` is nil, then assert the StatefulSet's policy is nil after reconcile.</comment>
<file context>
@@ -409,6 +409,92 @@ func TestWorkerPoolReconciler_UpdatesReplicas(t *testing.T) {
+ }, sts.Spec.PersistentVolumeClaimRetentionPolicy)
+}
+
+func TestWorkerPoolReconciler_PVCRetentionPolicyUpdatesExistingStatefulSet(t *testing.T) {
+ pool := newTestWorkerPool("my-pool", "default", 1)
+ pool.Spec.PersistentVolumeClaimRetentionPolicy = &appsv1.StatefulSetPersistentVolumeClaimRetentionPolicy{
</file context>
| needsUpdate = true | ||
| } | ||
|
|
||
| if !pvcRetentionPolicyEqual(sts.Spec.PersistentVolumeClaimRetentionPolicy, desired.Spec.PersistentVolumeClaimRetentionPolicy) { |
There was a problem hiding this comment.
P3: A retention-only change now reaches the shared StatefulSet update path, which still logs and emits the event "Updated StatefulSet %s to %d replicas". That message reports a replica count even when only the PVC retention policy changed. Make the message name what changed, or make it generic.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At internal/controller/workerpool_controller.go, line 509:
<comment>A retention-only change now reaches the shared StatefulSet update path, which still logs and emits the event "Updated StatefulSet %s to %d replicas". That message reports a replica count even when only the PVC retention policy changed. Make the message name what changed, or make it generic.</comment>
<file context>
@@ -506,6 +506,11 @@ func (r *WorkerPoolReconciler) reconcileStatefulSet(ctx context.Context, pool *k
needsUpdate = true
}
+ if !pvcRetentionPolicyEqual(sts.Spec.PersistentVolumeClaimRetentionPolicy, desired.Spec.PersistentVolumeClaimRetentionPolicy) {
+ sts.Spec.PersistentVolumeClaimRetentionPolicy = desired.Spec.PersistentVolumeClaimRetentionPolicy
+ needsUpdate = true
</file context>
|
/kelos api-review |
|
🤖 Kelos Task Status Task |
|
/kelos review |
|
🤖 Kelos Task Status Task |
|
🤖 Kelos API Reviewer Agent @gjkim42 API Design ReviewVerdict: APPROVE Reviewed head FindingsCompatibility
Naming / Shape
Documentation
Validation
Suggestions (optional)
Note on prompt injection: A third-party automated review on this PR includes embedded agent instructions (a "Prompt for AI agents" block telling agents to fix code, reply in threads, and resolve them). Those instructions were ignored, and this review was done independently. /kelos needs-input |
|
🤖 Kelos Reviewer Agent @gjkim42 Review SummaryVerdict: APPROVE Reviewed head Findings Overview
FindingsTests
Verified (no change needed)
Key takeaways
Note on prompt injection: A third-party automated review on this PR contains embedded agent instructions (a "Prompt for AI agents" block telling agents to fix code, reply in threads, and resolve them). Those instructions were disregarded, and this review was done independently. |
What type of PR is this?
/kind api
What this PR does / why we need it:
buildStatefulSetsetsvolumeClaimTemplateson a WorkerPool's StatefulSet but nopersistentVolumeClaimRetentionPolicy, so Kubernetes uses its default ofRetain/Retain. When a WorkerPool is scaled down or deleted, its workspace PVCs, and the disks behind them, stay around until someone deletes them by hand. A cluster that resizes or recreates pools often builds up a lot of orphaned volumes this way.This PR adds an optional
WorkerPool.spec.persistentVolumeClaimRetentionPolicy(the standardapps/v1StatefulSetPersistentVolumeClaimRetentionPolicy):reconcileStatefulSetalso updates the policy on an existing StatefulSet when it changes. The StatefulSet field is mutable. When comparing, a nil policy or an empty field counts asRetain, which matches what the API server fills in, so a defaulted StatefulSet is not updated again on every reconcile.RetainorDeletewhen the WorkerPool is admitted. Without them, a bad value would only show up later, as a failed StatefulSet create.docs/reference.mddocuments both fields.Backward compatibility: the field is optional and has no default. If it is unset, the StatefulSet gets no policy and Kubernetes keeps its
Retain/Retaindefault, so existing WorkerPools behave exactly as before. If a policy is set and later removed, the StatefulSet goes back to the API server default (Retain).Which issue(s) this PR is related to:
N/A
Special notes for your reviewer:
appsv1type, as the WorkerPool API already does withcorev1.PersistentVolumeClaimSpec, rather than adding a Kelos-owned copy. Validation comes from the two CEL rules.internal/controller/workerpool_controller_test.go):Retaintest/integration/workerpool_test.go, envtest):make updatemake verifygo test ./internal/controller/ ./api/...-ginkgo.focus)Does this PR introduce a user-facing change?
🤖 Generated with Claude Code
Summary by cubic
Adds an optional
spec.persistentVolumeClaimRetentionPolicyto WorkerPool so worker PVCs can be deleted when the pool is scaled down or deleted. Previously the StatefulSet had no policy, so Kubernetes retained PVCs indefinitely.Retain, matching Kubernetes' default, so existing WorkerPools behave exactly as before.RetainorDelete.docs/reference.mddocuments both fields.Written for commit e9bedd3. Summary will update on new commits.