Uh oh!
There was an error while loading. Please reload this page.
Add opt-in cleanup for persistent volumes - #248
Open
David Negstad (danegsta) wants to merge 1 commit into
Open
Conversation
Track DCP-created persistent volumes by workload and remove them only when cleanup is invoked with --volumes. Preserve adopted volumes and coordinate creation and cleanup through resource leases. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 94a89aca-02b4-4c6e-8220-0de0ec26ccae
Contributor
There was a problem hiding this comment.
Pull request overview
Adds opt-in cleanup for DCP-created persistent volumes while preserving adopted volumes by default.
Changes:
- Records newly created persistent volumes in the state store.
- Adds
dcp cleanup --volumeswith dependency-ordered removal and reporting. - Adds migration, controller, cleanup, and integration coverage.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
README.md | Documents workload tracking and volume cleanup. |
api/v1/container_types_test.go | Tests volume lease keys. |
api/v1/container_volume_types.go | Adds volume lease-key generation. |
controllers/volume_controller.go | Adds leasing and persistent-volume recording. |
internal/commands/workload.go | Updates workload-ID help text. |
internal/containers/volume_orchestrator.go | Exposes runtime names for volume records. |
internal/dcp/commands/cleanup.go | Implements opt-in volume cleanup. |
internal/dcp/commands/cleanup_test.go | Tests cleanup flags, ordering, and removal. |
internal/dcpctrl/commands/run_controllers.go | Configures volume persistence tracking. |
internal/statestore/migrations/000001_initial/000002_persistent_volumes.up.sql | Adds the persistent-volume table. |
internal/statestore/persistent_volumes.go | Implements volume record persistence. |
internal/statestore/schema.go | Registers schema version 1.2. |
internal/statestore/store_test.go | Tests migration recovery and volume records. |
test/integration/advanced_test_env.go | Configures volume reconciler dependencies. |
test/integration/standard_test_env.go | Passes workload configuration to volume reconciliation. |
test/integration/volume_controller_test.go | Tests recording and adopted-volume preservation. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+266
to
+270
| if persistErr := r.upsertPersistentVolumeRecord(ctx, vol, inspectedVolume, log); persistErr != nil { | ||
| removeErr := removeVolume(context.WithoutCancel(ctx), r.orchestrator, vol.Spec.Name) | ||
| persistErr = errors.Join(persistErr, removeErr) | ||
| log.Error(persistErr, "Could not persist ContainerVolume workload record", "ResourceKey", vol.GetLeaseKey()) | ||
| return setContainerVolumeState(vol, apiv1.ContainerVolumeStatePending) | additionalReconciliationNeeded |
Comment on lines
+760
to
+770
| _, initialInspectErr := orchestrator.InspectVolumes(ctx, containers.InspectVolumesOptions{Volumes: []string{volumeName}}) | ||
| if errors.Is(initialInspectErr, containers.ErrNotFound) { | ||
| return nil | ||
| } | ||
| if initialInspectErr != nil { | ||
| return initialInspectErr | ||
| } | ||
| _, removeErr := orchestrator.RemoveVolumes(ctx, containers.RemoveVolumesOptions{ | ||
| Volumes: []string{volumeName}, | ||
| }) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Persistent volumes currently outlive workload cleanup with no supported way to remove the volumes DCP created. This adds an explicit
dcp cleanup --volumes <workload-id>option while preserving volumes by default.The change records newly created persistent volumes in the state store with their workload and runtime, then includes those records in dependency-ordered cleanup after containers have been removed. Existing volumes that DCP adopts are deliberately not recorded or removed. Volume reconciliation and cleanup coordinate through resource leases, and removal avoids force mode so Podman cannot delete unrelated containers using a volume.
The state-store addition is an additive major-version-1 minor migration with interrupted-migration recovery coverage. Cleanup reporting now includes volume counts and failures.
Validation:
make lintmake test