Add first-class KubernetesPersistentVolumeResource (and fix related YAML serializer bugs)#16929
Open
mitchdenny wants to merge 7 commits into
Open
Add first-class KubernetesPersistentVolumeResource (and fix related YAML serializer bugs)#16929mitchdenny wants to merge 7 commits into
mitchdenny wants to merge 7 commits into
Conversation
Contributor
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 16929Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 16929" |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to make the Kubernetes publisher emit deployable YAML by preventing empty {} mappings in generated PersistentVolume / PersistentVolumeClaim manifests (notably for DefaultStorageType = "pvc"), and adds regression tests/snapshots to cover the affected paths.
Changes:
- Make several V1 spec sub-properties nullable (removing eager
= new()) so YAML serialization omits them when empty. - Add regression tests and Verify snapshots for
pvcandhostpathstorage type publishing, including an explicit{}-substring invariant. - Update XML docs/remarks on the affected spec properties to explain the null-default behavior and why it matters for Kubernetes apply.
Show a summary per file
| File | Description |
|---|---|
| tests/Aspire.Hosting.Kubernetes.Tests/KubernetesPublisherTests.cs | Adds regression tests for pvc and hostpath storage type publishing and asserts no empty {} mappings appear in emitted YAML. |
| tests/Aspire.Hosting.Kubernetes.Tests/Snapshots/KubernetesPublisherTests.PublishAsync_WithPvcStorageType_GeneratesValidPersistentVolumeAndClaim#00.verified.yaml | New snapshot covering the generated Deployment for pvc storage type. |
| tests/Aspire.Hosting.Kubernetes.Tests/Snapshots/KubernetesPublisherTests.PublishAsync_WithPvcStorageType_GeneratesValidPersistentVolumeAndClaim#01.verified.yaml | New snapshot covering the generated PersistentVolume for pvc storage type. |
| tests/Aspire.Hosting.Kubernetes.Tests/Snapshots/KubernetesPublisherTests.PublishAsync_WithPvcStorageType_GeneratesValidPersistentVolumeAndClaim#02.verified.yaml | New snapshot covering the generated PersistentVolumeClaim for pvc storage type. |
| tests/Aspire.Hosting.Kubernetes.Tests/Snapshots/KubernetesPublisherTests.PublishAsync_WithHostPathStorageType_GeneratesValidPersistentVolumeAndClaim.verified.yaml | New snapshot covering the generated Deployment for hostpath storage type. |
| src/Aspire.Hosting.Kubernetes/Resources/PersistentVolumeSpecV1.cs | Makes ClaimRef, HostPath, Local, and NodeAffinity nullable to avoid emitting empty mappings. |
| src/Aspire.Hosting.Kubernetes/Resources/PersistentVolumeClaimSpecV1.cs | Makes DataSource, DataSourceRef, and Selector nullable to avoid emitting empty mappings. |
| src/Aspire.Hosting.Kubernetes/Resources/VolumeNodeAffinityV1.cs | Makes Required nullable to avoid emitting required: {} when unset. |
Copilot's findings
- Files reviewed: 8/8 changed files
- Comments generated: 2
Comment on lines
+1
to
+15
| --- | ||
| apiVersion: "v1" | ||
| kind: "PersistentVolume" | ||
| metadata: | ||
| name: "service-data-pv" | ||
| labels: | ||
| app.kubernetes.io/name: "{{ .Chart.Name }}" | ||
| app.kubernetes.io/component: "service" | ||
| app.kubernetes.io/instance: "{{ .Release.Name }}" | ||
| spec: | ||
| storageClassName: "managed-csi" | ||
| accessModes: | ||
| - "ReadWriteOnce" | ||
| capacity: | ||
| storage: "10Gi" |
Comment on lines
+695
to
+700
| public async Task PublishAsync_WithHostPathStorageType_GeneratesValidPersistentVolumeAndClaim() | ||
| { | ||
| // Regression test that exercises the hostPath emission path of | ||
| // CreatePersistentVolume in addition to the PVC path. With the bug fixes the rendered | ||
| // PV correctly contains a populated `hostPath` mapping while still omitting the empty | ||
| // `claimRef`/`local`/`nodeAffinity` blocks that previously broke `kubectl apply`. |
mitchdenny
changed the title
Member
Author
|
Tracking issue: #16999 (created retroactively to capture the design decisions and context for this work). Now in the 13.4 milestone alongside the future-work items called out in that issue. |
1 task
mitchdenny
force-pushed
the
mitchdenny/k8s-persistent-volumes-research
branch
from
7d0624d to
485e96f
Compare
Contributor
|
Re-running the failed jobs in the CI workflow for this pull request because 4 jobs were identified as retry-safe transient failures in the CI run attempt.
Matched test failure patterns (1 test)
|
mitchdenny
force-pushed
the
mitchdenny/k8s-persistent-volumes-research
branch
from
4746841 to
d830d5b
Compare
16 tasks
The generated PersistentVolume and PersistentVolumeClaim manifests for the
Kubernetes publisher contain `claimRef: {}`, `hostPath: {}`, `local: {}`,
`nodeAffinity: { required: {} }`, `dataSource: {}`, `dataSourceRef: {}` and
`selector: {}` blocks that `kubectl apply` and Helm reject.
Root cause: `PersistentVolumeSpecV1`, `PersistentVolumeClaimSpecV1` and
`VolumeNodeAffinityV1` eagerly initialize their complex sub-properties to
`new()`. The Kubernetes publishing pipeline configures the YAML serializer
with `OmitNull` and a `YamlIEnumerableSkipEmptyObjectGraphVisitor`, but
neither catches a non-collection complex object whose own properties are all
null/empty, so the wrapping property still emits an empty mapping.
Make the affected properties nullable and drop the eager `= new()` so the
serializer's `OmitNull` configuration suppresses them. Conditional setters
in `CreatePersistentVolume` continue to assign new instances when needed,
so behaviour is unchanged for properties the publisher actually populates.
Adds two regression tests covering `DefaultStorageType = "pvc"` (the
issue #14096 / #16504 repro) and `DefaultStorageType = "hostpath"` so the
emission paths now have snapshot coverage they previously lacked.
Fixes #14096
Fixes #16504
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Introduces an app-model resource for Kubernetes persistent volumes so users can describe storage requirements per-volume rather than relying on the environment-wide DefaultStorageType / DefaultStorageClassName knobs. New public surface (all marked [Experimental(\"ASPIRECOMPUTE002\")]): - KubernetesPersistentVolumeResource sibling of KubernetesIngressResource / KubernetesGatewayResource. Carries StorageClassName, Capacity, AccessModes, and a Kubernetes metadata.annotations channel for CSI / dynamic-provisioner hints. Renders to a v1 PersistentVolumeClaim at publish time. - AddPersistentVolume(name) factory on IResourceBuilder<KubernetesEnvironmentResource>. - Builder modifiers WithStorageClass, WithCapacity, WithAccessMode, and WithVolumeAnnotation (each with a parameter-aware overload where applicable). - PersistentVolumeAccessMode enum (ReadWriteOnce / ReadOnlyMany / ReadWriteMany / ReadWriteOncePod). - WithPersistentVolume<T>(volume) extension on IResourceBuilder<T> where T : IComputeResource — name-match binding against an existing ContainerMountAnnotation source. Lets integrations like AddPostgres + WithDataVolume() pick up a rich PVC without code changes. - WithPersistentVolume<T>(volume, mountPath, isReadOnly) overload that also creates the ContainerMountAnnotation. Closes #9430 by enabling ProjectResource (and any IComputeResource) to mount persistent storage. Publisher integration: - KubernetesEnvironmentResource.ProcessPersistentVolumeResources resolves expressions and builds the PersistentVolumeClaim manifest, falling back to the env-wide DefaultStorageReadWritePolicy / DefaultStorageSize / DefaultStorageClassName when the volume does not override them. - KubernetesPublishingContext writes each generated PVC as a standalone Helm template under templates/<volume>/<volume>.yaml, mirroring how Ingress and Gateway resources are emitted. - KubernetesResource.CreateApplication promotes any workload bound to a persistent volume to a StatefulSet (no opt-out), preserving stable pod identity and avoiding the RWO + Deployment + replicas foot gun. - ResourceExtensions.WithPodSpecVolumes routes pod volumes[] entries through the bound PVC by claim name, leaving unbound volumes to fall through to the existing env-default storage type path. Additional bug fixes (same root cause as PR 1): - StatefulSetSpecV1.PersistentVolumeClaimRetentionPolicy and UpdateStrategy are now nullable. Previously emitted invalid empty {} mappings whenever a workload was rendered as a StatefulSet (now triggered far more often by the new auto-promote rule). - StatefulSetUpdateStrategyV1.RollingUpdate is now nullable for the same reason. Tests: 3 new KubernetesPublisherTests covering name-match container binding with StatefulSet promotion, ProjectResource mount-path binding, and the fallthrough-to-env-default behavior for unbound volumes on a workload that also has a bound volume. All 151 K8s + 37 Azure K8s tests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The `FallsThroughForUnboundVolumes` test mounted an unbound volume at
`/tmp/scratch`. On Linux that path matches `Path.GetTempPath()`, which
Verify automatically scrubs to `{TempPath}`, so the received YAML diverged
from the snapshot recorded on Windows (where `/tmp/` is a literal path).
Switch the mount path to `/srv/scratch` and refresh the verified snapshot.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Two new CLI E2E tests in Aspire.Cli.EndToEnd.Tests that deploy a real Aspire app to a KinD cluster, write data through a workload bound to a KubernetesPersistentVolumeResource, restart the pod, and verify the data survives — proving the durability claim of first-class persistent volumes end-to-end against the rancher local-path-provisioner. KubernetesDeployWithPersistentVolumeTests Postgres + WithDataVolume() + WithPersistentVolume(volume) name-match binding. Exercises StatefulSet auto-promotion, generated PVC binding, and round-trips a row through pg-statefulset-0 across a kubectl pod delete. KubernetesDeployWithProjectPersistentVolumeTests ProjectResource + WithPersistentVolume(volume, mountPath) overload. Closes the end-to-end story for #9430. Project writes /srv/data/marker.txt, pod is deleted, marker is read back from the same PVC after the StatefulSet controller recreates the pod. Both tests slot into the existing KubernetesDeployTestHelpers pipeline (KinD cluster + local registry + Helm + interactive aspire deploy) so they run on every PR alongside the existing K8s deploy E2E suite, with no Azure cost. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…lumes Validates the auto-generated TypeScript bindings for the persistent-volume API surface (addPersistentVolume / withStorageClass / withCapacity / withKubernetesPersistentVolumeMount) end-to-end: a TypeScript Express node app declares a first-class persistent volume in apphost.ts, mounts it at /srv/data via the mount-path overload, deploys to a KinD cluster, writes a marker file, gets its pod deleted, and reads the marker back from the same PVC after the StatefulSet controller recreates the pod. This closes the cross-language story for the PV API: the C# tests (KubernetesDeployWithPersistentVolumeTests / WithProjectPersistentVolumeTests) prove the C# API end-to-end, this test proves the TypeScript SDK lowering hits the same publisher path. A TS Postgres variant is intentionally not added — the C# Postgres test already covers the name-match overload and the connection-string-driven workload, so a TS Postgres durability test would duplicate the durability proof while adding npm pg + connection string wiring that is orthogonal to what this PR changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
PG test:
- WithDataVolume() no-arg generates {AppHost}.{hash}-pg-data; the dot is
invalid in StatefulSet podSpec volumes[].name. Pass an explicit name
to dodge the bug (filing separate product issue for the name-match
binding to normalize). Also switch the curl loop sentinel to a runtime-
only token so WaitUntilTextAsync doesn't race-match the typed echo.
TS test:
- Replace HTTP/port-forward verification with kubectl exec writes/reads
directly into the mounted PV. The Node template's HTTP listener wasn't
reachable through the K8s Service in CI (HTTP_000), which is orthogonal
to what this PR tests. kubectl exec is a more direct durability proof
and removes ~80 lines of port-forward plumbing.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Outer bash on the test runner was expanding $(cat /srv/data/marker.txt) locally (producing empty, since the marker file lives inside the pod, not on the host) before the command ever reached `kubectl exec`. Switch the `sh -c` argument to single quotes so the substitution is deferred to the pod's sh. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
mitchdenny
force-pushed
the
mitchdenny/k8s-persistent-volumes-research
branch
from
d830d5b to
f1c4555
Compare
Contributor
|
Re-running the failed jobs in the CI workflow for this pull request because 1 job was identified as retry-safe transient failures in the CI run attempt.
|
Contributor
|
❌ CLI E2E Tests failed — 98 passed, 1 failed, 5 unknown (commit Failed Tests
View all recordings
📹 Recordings uploaded automatically from CI run #26351772900 |
Contributor
|
Not for 13.4, moving. |
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 free
to 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.
Tracking issue: #16999
Description Adds first-class app-model support for Kubernetes persistent volumes, plus the underlying YAML serializer bug fixes that the new code path exposes. Two layers in this PR: ### 1. First-class
KubernetesPersistentVolumeResource(new feature) A workload-independent app-model resource for Kubernetes persistent volumes, mirroring howKubernetesIngressResourceandKubernetesGatewayResourcealready work. Each volume is a realIResourcewith its own configuration (storage class, capacity, access modes,metadata.annotations) ΓÇö instead of relying on the environment-wideDefaultStorageType/DefaultStorageClassNameknobs that today force every volume in the env to share a single shape.csharp var k8s = builder.AddKubernetesEnvironment("k8s"); var pgData = k8s.AddPersistentVolume("pg-data") .WithStorageClass("managed-csi") .WithCapacity("20Gi") .WithAccessMode(PersistentVolumeAccessMode.ReadWriteOnce) .WithVolumeAnnotation("disk.csi.azure.com/skuName", "Premium_LRS"); // Name-match overload ΓÇö Postgres' WithDataVolume() emits a ContainerMountAnnotation // with source `pg-data`; the binding picks it up and routes the pod's volumes[] // entry through the generated PVC. builder.AddPostgres("pg") .WithDataVolume() .WithPersistentVolume(pgData); // Mount-path overload ΓÇö works for projects (closes #9430) and any IComputeResource. builder.AddProject<MyApi>("api") .WithPersistentVolume(pgData, "/srv/data");Workloads bound to a persistent volume are auto-promoted toStatefulSet(no opt-out), per the design doc, since aDeployment+ RWO PVC + replicas is broken-by-design and the abstraction's whole purpose is to encode the correct defaults. All new public APIs are marked[Experimental("ASPIRECOMPUTE002")], matching the package's existing experimental surface. ### 2. YAML serializer bug fixes (foundation work) Closes #14096 and #16504, plus extends the same fix toStatefulSetSpecV1/StatefulSetUpdateStrategyV1(which previously emitted invalidvolumeClaimRetentionPolicy: {},updateStrategy: {}, androllingUpdate: {}mappings whenever any workload was rendered as aStatefulSetΓÇö now triggered far more often by the new auto-promote rule). Root cause for all of these: complex sub-properties on V1 spec types were eagerly initialized tonew(), so the YAML serializer'sOmitNullconfiguration couldn't drop them. Making the properties nullable lets the existing filters do the right thing. ## Issues closed - Fixes #14096 ΓÇöDefaultStorageType = "pvc"produces invalid PV YAML. - Fixes #16504 ΓÇö PVC generation emits invalid emptydataSource/dataSourceRef/selectorblocks. - Closes #9430 ΓÇö Ability to define a PVC or useWithVolumeon aProjectResource. ## Related work - Supersedes the design intent of #16503 (PVC-only emission). With first-class volumes, that's now the natural shape:AddPersistentVolume(...).WithStorageClass(...)emits a PVC and lets the cluster's StorageClass dynamically provision the PV. The per-workloadWithDynamicProvisioning(bool)API in #16503 doesn't survive this redesign ΓÇö but the same scenario is covered. - Touches the same area as the 2024volume.v0discussion (#1521). ## Tests - Three newKubernetesPublisherTestscovering name-match container binding with StatefulSet promotion,ProjectResourcemount-path binding, and the fallthrough-to-env-default behavior for unbound volumes on a workload that also has a bound volume. - Two new regression tests for the originalDefaultStorageType = "pvc"andhostpathpaths. - All 151Aspire.Hosting.Kubernetes.Testsand 37Aspire.Hosting.Azure.Kubernetes.Testspass.