NFS stability and performance enhancements#851
Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| if err != nil { | ||
| return 0, fmt.Errorf("open %s: %w", loopControlPath, err) | ||
| } | ||
| defer ctrl.Close() |
| if err != nil { | ||
| return "", fmt.Errorf("open backing file %s: %w", backingFile, err) | ||
| } | ||
| defer bf.Close() |
| if err != nil { | ||
| return "", fmt.Errorf("open loop device %s: %w", loopPath, err) | ||
| } | ||
| defer lf.Close() |
There was a problem hiding this comment.
Pull request overview
This PR updates the CloudHarness NFS server stack to improve stability during restarts/reschedules, add a watchdog/health endpoint, and introduce CI coverage for failover/multi-node mount behavior.
Changes:
- Upgraded the embedded
nfs-subdir-external-provisionerdependencies and modified provision/create/delete paths to use a new Go-basednfsvolhelper. - Added
nfsvol(Go) to mount existing loopback volumes concurrently at startup, regenerate stable per-PV exports, and run a watchdog with/healthz. - Extended Codefresh pipelines and samples values to build/deploy nfsserver and run a failover & multi-node stress test.
Reviewed changes
Copilot reviewed 23 out of 24 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| deployment/codefresh-test.yaml | Builds/publishes the nfsserver image and adds an NFS failover CI step; adjusts rollout waits and API test ordering. |
| deployment-configuration/codefresh-template-test.yaml | Mirrors the NFS failover CI step in the template and quotes cmd_ps. |
| applications/samples/deploy/values-test.yaml | Declares nfsserver as a hard dependency and enables NFS-backed volumes for tests. |
| applications/samples/deploy/values-prod.yaml | Fixes indentation for resource request memory. |
| applications/samples/deploy/values-minimal.yaml | Changes minimal dependency shape (now only soft with common). |
| applications/nfsserver/test/failover-test.sh | Adds an end-to-end failover/multi-node mount/ESTALE regression script. |
| applications/nfsserver/resources/run_nfs.sh | Switches bootstrap to nfsvol mount-all + starts watchdog; narrows NFS daemon version flags; adds ulimit clamp. |
| applications/nfsserver/resources/rmlimdir.sh | Converts to a thin wrapper around nfsvol delete. |
| applications/nfsserver/resources/remount.sh | Converts to a thin wrapper around nfsvol mount-all. |
| applications/nfsserver/resources/pre-startup.sh | Converts to a thin wrapper around nfsvol mount-all. |
| applications/nfsserver/resources/mklimdir.sh | Converts to a thin wrapper around nfsvol create (and mount-all for --mountonly). |
| applications/nfsserver/nfsvol/main.go | Introduces the nfsvol CLI entrypoint (mount-all/create/delete/watchdog). |
| applications/nfsserver/nfsvol/internal/watchdog/watchdog.go | Adds the mount watchdog loop and /healthz HTTP server. |
| applications/nfsserver/nfsvol/internal/mount/mount.go | Implements loop attach/mount lifecycle, concurrent bootstrap, and create/delete logic. |
| applications/nfsserver/nfsvol/internal/mount/exports.go | Implements deterministic fsid exports fragments under /etc/exports.d. |
| applications/nfsserver/nfsvol/internal/loop/loop.go | Implements loop device allocation/attach/detach and stale-loop cleanup. |
| applications/nfsserver/nfsvol/go.mod | Adds a new Go module for nfsvol. |
| applications/nfsserver/nfs-subdir-external-provisioner/go.mod | Updates Go/toolchain settings and bumps Kubernetes/provisioner dependencies. |
| applications/nfsserver/nfs-subdir-external-provisioner/cmd/nfs-subdir-external-provisioner/provisioner.go | Switches provision/delete to call nfsvol and adds parsing helpers/env defaults. |
| applications/nfsserver/deploy/values.yaml | Adds a configurable PDB toggle and adjusts comments. |
| applications/nfsserver/deploy/templates/pdb.yaml | Adds an optional PodDisruptionBudget template. |
| applications/nfsserver/deploy/templates/nfs-server.yaml | Forces Recreate, disables leader election, adds health probes/port, and sets SA name. |
| applications/nfsserver/README-PROD.md | Documents production caveats, outage behavior, and operational responsibilities. |
| applications/nfsserver/Dockerfile | Switches base image, installs e2fsprogs, builds/copies nfsvol, and runs go mod tidy during builds. |
|
|
||
| const ( | ||
| provisionerNameKey = "PROVISIONER_NAME" | ||
| annotationPrefix = "k8s-sigs.io" |
There was a problem hiding this comment.
annotationPrefix is declared but never used, which will cause the Go build to fail with an “declared and not used” compilation error. Remove the constant or wire it into the annotation parsing logic so the package compiles.
| annotationPrefix = "k8s-sigs.io" |
| # run pre startup script | ||
| bash -c "/usr/local/bin/pre-startup.sh" | ||
| # Mount all existing PVC volumes in parallel, then start the watchdog. | ||
| /usr/local/bin/nfsvol mount-all |
There was a problem hiding this comment.
nfsvol mount-all failures are currently ignored (the script doesn’t set -e and there’s no explicit exit-on-error). That can leave volumes unmounted while the container continues starting and the health endpoint reports OK. Please make startup fail fast if mount-all returns non-zero (e.g., enable set -e / check the exit code and exit 1).
| /usr/local/bin/nfsvol mount-all | |
| if ! /usr/local/bin/nfsvol mount-all; then | |
| echo "ERROR: /usr/local/bin/nfsvol mount-all failed" >&2 | |
| exit 1 | |
| fi |
| # -V 3: enable NFSv3 (matches client mount options elsewhere). | ||
| /usr/sbin/rpc.mountd -V 3 | ||
|
|
||
| /usr/sbin/exportfs -r | ||
| # -G 10 to reduce grace time to 10 seconds (the lowest allowed) | ||
| /usr/sbin/rpc.nfsd -G 10 -N 2 -V 3 -V 4 | ||
| /usr/sbin/rpc.nfsd -G 10 -V 3 |
There was a problem hiding this comment.
This script now starts only NFSv3 (rpc.mountd -V 3 / rpc.nfsd -V 3), but the default Helm values don’t include an explicit nfsvers=3 mountOption (it’s only present in values-local.yaml). Please ensure the StorageClass mountOptions force nfsvers=3 (or re-enable v4 on the server) so client mounts don’t depend on version-negotiation/fallback behavior.
| @@ -1,8 +1,8 @@ | |||
| harness: | |||
| secured: false | |||
| dependencies: | |||
There was a problem hiding this comment.
harness.dependencies.hard was removed from the minimal values. Helm templates in this repo assume both hard and soft are lists (e.g. they call concat .app.harness.dependencies.hard .app.harness.dependencies.soft), so omitting hard can cause template rendering failures. Add hard: [] (or keep the previous empty list) to make the values shape consistent.
| dependencies: | |
| dependencies: | |
| hard: [] |
| module metacell/nfsvol | ||
|
|
||
| go 1.24 | ||
|
|
||
| require golang.org/x/sys v0.33.0 |
There was a problem hiding this comment.
New Go module nfsvol adds a go.mod but does not include a go.sum. The repo’s other Go module (nfs-subdir-external-provisioner) commits go.sum, and omitting it makes dependency resolution less reproducible (and can break builds that run with -mod=readonly). Please add the generated go.sum for this module.
| # Find the loop device backing the test PVC on the server. | ||
| # mountpoint format: /exports/<pvName> where pvName includes the PV_NAME. | ||
| MOUNTPOINT=$(k exec "$NEW_NFS_POD" -- sh -c "ls -d /exports/*${PV_NAME}* 2>/dev/null | grep -v '\.exports$' | grep -v 'archived-' | head -1" || true) | ||
| if [ -z "$MOUNTPOINT" ]; then | ||
| log "skipping test 5: could not locate server-side mountpoint for $PV_NAME" | ||
| else | ||
| LOOPDEV=$(k exec "$NEW_NFS_POD" -- sh -c "losetup -a | grep '$MOUNTPOINT.quota' | cut -d: -f1" || true) | ||
| if [ -z "$LOOPDEV" ]; then |
There was a problem hiding this comment.
The mountpoint discovery for test 5 can return either the mount directory (/exports/<pv>) or the backing quota file (/exports/<pv>.quota), because the ls glob matches both and the filters don’t exclude .quota. If it picks the .quota file, the subsequent losetup grep uses "$MOUNTPOINT.quota" (becoming ...quota.quota) and the test will be skipped incorrectly. Filter out \.quota$ and/or assert the result is a directory (e.g., test -d).
| // Clean up any pre-existing state left from a failed previous attempt. | ||
| _ = Delete(mountpoint) | ||
|
|
||
| if err := createQuotaFile(quotaFile, sizeBytes); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| cmd := exec.Command("mkfs.ext4", "-F", quotaFile) | ||
| if out, err := cmd.CombinedOutput(); err != nil { | ||
| _ = os.Remove(quotaFile) | ||
| return fmt.Errorf("mkfs.ext4: %w\n%s", err, out) | ||
| } | ||
|
|
||
| if err := mountOne(mountpoint); err != nil { | ||
| return err |
There was a problem hiding this comment.
Create() calls Delete(mountpoint) as a cleanup step, but Delete() renames an existing ${mountpoint}.quota to ${mountpoint} (a regular file). If a previous failed attempt left a .quota behind, this makes mountpoint a file, and the subsequent mountOne() will fail at os.MkdirAll(mountpoint, ...) (“not a directory”). Consider either (a) making mountOne() remove/recreate mountpoint when it exists as a file, or (b) splitting Delete into “cleanup for recreate” vs “delete preserving data for archive” so Create doesn’t leave a file at mountpoint.
Closes CH-261
Implemented solution
How to test this PR
There is a test included in the deployment that tries some common stress cases
Sanity checks:
Breaking changes (select one):
breaking-changeand the migration procedure is well described abovePossible deployment updates issues (select one):
alert:deploymentTest coverage (select one):
Documentation (select one):
Nice to have (if relevant):