feat(kubernetes): implement storage for site assets #139
No reviewers
Labels
No labels
Compat/Breaking
Kind/Bug
Kind/Documentation
Kind/Enhancement
Kind/Feature
Kind/Security
Kind/Testing
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
No milestone
No project
No assignees
3 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
thwap/thwap-pagesd!139
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/site-asset-storage"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
What
Implement configurable storage for site assets in
internal/kubernetes:kubernetes.storage_type(emptyDirdefault |pvc) andkubernetes.storage_class(empty → cluster default)pvcTemplatebuilds a per-site PersistentVolumeClaim (<site>-data, ReadWriteOnce, storage class, 1Gi)PublishSiteupserts the PVC when PVC storage is configured;DeleteSiteremoves itsitevolume source switches betweenemptyDirand the PVC/var/wwwwithversions/subdirectories; nginx root stays/var/www/sitesymlinkinit container creates/var/www/site → /var/www/versions/$SITE_VERSIONat pod startSymlinkVersion(root, version)helper creates/re-points the symlink (used by the publish flow in #73)Tests: fake-clientset coverage for emptyDir vs PVC volume sources, PVC publish/delete, and TempDir-based symlink tests (create + re-point).
Why
Phase 4 task #58 — completes the Kubernetes Integration phase. The storage plumbing the webhook receiver (#73) uses to serve published site content.
Testing
go build ./...go vet ./...go test -race ./internal/kubernetes/— 23 tests passgofmt -l ./cmd ./internal ./pkgcleangolangci-lint run— 0 issuesBreaking Changes
None.
Notes
SITE_VERSIONtov0.0.0for now; the publish flow (#73) will set the real version.Closes #56
Closes #57
Closes #58
Review Summary
Verified locally on the PR head (
ffdc6e5):go build,go vet,go mod verify,gofmt,golangci-lint runall clean; all 23 kubernetes tests pass with-race.The storage refactor is otherwise well done —
siteVolumecleanly switches between emptyDir and PVC,pvcTemplate/upsertPVC/deletePVCmirror the established upsert pattern,storage_classnil-handling correctly falls back to the cluster default, andSymlinkVersion(create + re-point, tested against a real TempDir) is solid.Blocking Issue
internal/kubernetes/templates.go— thesymlinkinit container's shell script writes to the wrong path. The init container mounts the site volume atsiteVolumePath(/var/www), but the command ismkdir -p /www/versions && ln -sfn /www/versions/"$SITE_VERSION" /www/site. Everything lands in the container's ephemeral rootfs at/www, not in the shared volume at/var/www. The static container mounts the same volume at/var/wwwand nginx'srootis/var/www/site, so/var/www/sitenever exists and published sites serve nothing.Empirically confirmed by rendering
deploymentTemplate(mount/var/wwwvs script/www/...). The fake clientset just stores the object, so the existing tests don't catch this.Fix: point the script at the mount path —
mkdir -p /var/www/versions && ln -sfn /var/www/versions/"$SITE_VERSION" /var/www/site(or mount the volume at/www). Consider a unit test asserting the init container's script references the same path as itsVolumeMounts.Non-blocking Suggestions
upsertPVCUpdate path is really create-or-error-on-drift. Fine for identical re-publishes, but worth knowing a storage-class change on an existing site will fail rather than migrate.SymlinkVersionisRemove+Symlink(non-atomic) — brief window without the link. Acceptable for now; arename-based swap would be cleaner later.The blocking item is a one-word path fix in the init container command.
Review changes addressed in
4b558c0:Blocking issue — the
symlinkinit container script now uses/var/www/versions/...and/var/www/site, matching the volume mount pathsiteVolumePath(/var/www). Previously the script wrote to/www, so the symlink landed in the ephemeral rootfs instead of the shared volume.Regression test added —
TestInitContainerScriptMatchesMountPathasserts the init container's command references its own volume mount path and creates the/var/www/sitesymlink.Verified:
go build,go vet,gofmt,golangci-lint runclean; all 24 kubernetes tests pass with-race.Review Summary
The blocking issue from the previous review is resolved.
symlinkinit container's command now operates on the actual mount path:mkdir -p /var/www/versions && ln -sfn /var/www/versions/"$SITE_VERSION" /var/www/site, matching the volume mount at/var/www. The symlink now lands in the shared volume where nginx'sroot /var/www/sitecan follow it.TestInitContainerScriptMatchesMountPathasserts the init container's script references its volume mount path and thesiteMountPathsymlink, closing exactly the gap that let the original bug slip through.Verified locally on the PR head (
4b558c0):go build,go vet,go mod verify,gofmt,golangci-lint runall clean; all 23 kubernetes tests pass with-race.No blocking issues. The non-blocking notes from the previous review (PVC spec immutability on upsert, non-atomic symlink swap) still stand but don't block.
Approving.