CFE-90: reconcile storage mount options when they drift (opt-in remount)#6222
Draft
nickanderson wants to merge 8 commits into
Draft
CFE-90: reconcile storage mount options when they drift (opt-in remount)#6222nickanderson wants to merge 8 commits into
nickanderson wants to merge 8 commits into
Conversation
…te fstab entries CFE-90: NFS mount options not updating on remount. Root causes fixed: 1. FileSystemMountedCorrectly() now validates mount options against promised options 2. VerifyMountPromise() performs direct 'mount -o remount' when options differ instead of relying on 'mount -a' which skips already-mounted filesystems 3. VerifyInFstab() updates existing fstab entries when options differ, instead of only appending new entries Added helper functions GetFstabEntryOptions() and ReplaceFstabEntry() in nfs.c for parsing and updating fstab option fields. Also added StringIsSHA1Hex() stub in server_tls.c to fix community build linking against enterprise code paths. Files changed: - cf-agent/nfs.c: +97 lines (VerifyInFstab fix, GetFstabEntryOptions, ReplaceFstabEntry) - cf-agent/verify_storage.c: +65 lines (FileSystemMountedCorrectly check, remount logic) - cf-serverd/server_tls.c: +10 lines (StringIsSHA1Hex stub)
… with inverse pair handling The original CFE-90 fix compared promised mount options against the raw kernel-resolved options using strcmp(), which always failed because: - The kernel reports options in a canonical order different from the user's promise (e.g. 'tcp' becomes 'proto=tcp') - The kernel adds auto-negotiated NFS options (vers=, rsize=, wsize=, timeo=, retrans=, sec=, mountport=, etc.) - Inverse option pairs (noatime/relatime, hard/soft, ro/rw) require special handling The fix now: - Extracts full kernel options from mount -va output (was storing only the filesystem type) - Stores them in a new raw_opts field on the Mount struct - Implements subset-based option matching: all user-specified options must be present in the kernel options, but kernel-added options are ignored - Handles inverse pairs (noatime vs relatime, hard vs soft, etc.) - Handles alias pairs (tcp/proto=tcp, udp/proto=udp)
An earlier commit on this branch added a local StringIsSHA1Hex definition (and forward declaration) to cf-serverd/server_tls.c. The symbol already exists in libntech (libutils/string_lib.c) and is declared in string_lib.h, so the local copy was a duplicate definition that would fail to link (or shadow the real SHA1 validation). Removed it. Ticket: CFE-90 Changelog: None Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Previously mount_options only affected the initial fstab write and the
initial mount; a filesystem already mounted with the wrong options was
never corrected, because mount -a skips already-mounted filesystems. This
adds opt-in reconciliation of a live mount when its options drift from the
promise.
New mount body attributes:
- remount (default false): manage the options of an already-mounted
filesystem. When false a mounted filesystem with the correct source is
considered kept regardless of option drift (backwards compatible; options
still drive the initial mount and, with edit_fstab, the fstab entry).
- remount_methods (default { remount, unmount_mount }): ordered mechanisms
tried in turn, verifying after each that the live mount satisfies the
promise. The kernel returns success from a remount even when it silently
ignores NFS-negotiated options, so the resulting state is re-read rather
than trusting the command exit status.
- remount_timeout: bounds the unmount/mount path against a hung server.
When reconciliation is enabled the live mount is corrected first; if
edit_fstab is set the fstab entry is then updated regardless of the live
outcome (recording intent moves the system closer to the desired state,
and the live result is reported as its own promise outcome).
Also corrects the mount-info handling this relies on:
- GetFstabEntryOptions returned the fstab type field instead of the options
field, causing a spurious fstab rewrite and reported change every run.
- ReplaceFstabEntry leaked the previous entry string.
- The mounted-FS scan no longer dropped the fstype used by
IsForeignFileSystem; fstype and kernel-resolved options are now stored
separately (options vs raw_opts).
- Restored unconditional creation of the mount point directory before
mounting, and stopped arming mount -a for an already-mounted filesystem.
- OptionsSubsetMatches treats the "defaults" pseudo-option as "rw" (the
kernel never echoes "defaults"), so promising it converges.
Relates: CFE-1539 (fstab maintenance), CFE-1863 (mount -a scope).
Ticket: CFE-90
Ticket: CFE-1864
Changelog: Title
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Covers subset matching (kernel-added options ignored), order independence, inverse-pair contradictions (noatime/relatime, ro/rw, hard/soft, sync/async, and generic no<opt>/<opt>), tcp/udp<->proto= aliases, and the "defaults"->rw handling. Runs unprivileged in CI via "make -C tests/unit check"; the behavioral mount/remount test belongs in the system-testing repo (needs root + a real NFS server). Ticket: CFE-90 Changelog: None Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
The PR-only `cfengine format --check` (cfengine_cli lint) flags tests/acceptance/31_tickets/CFE-2663/test.cf as needing reformatting (4-space body indentation, no blank lines in the vars block). This is pre-existing drift on master surfaced by the whole-tree check on PRs; reformatted here so this PR's lint passes. Ticket: CFE-90 Changelog: None Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Member
Author
|
@cf-bottom jenkins please |
- Remove unnecessary NULL guards before free() in DeleteMountInfo(). free(NULL) is a C-standard no-op; bare calls match the surrounding convention (FreeOptionsList, VerifyMount, etc.). - Add assert(a != NULL) to LiveMountConverged, ReconcileMountOptions, FileSystemMountedCorrectly, and VerifyMountPromise to resolve the 15 null-dereference warnings. This follows the existing convention in VerifyInFstab, VerifyMount, and VerifyUnmount which already had these asserts.
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
3 participants
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.
Storage/mount promises only applied
mount_optionsto the initial mount and (withedit_fstab) the fstab entry. A filesystem already mounted with different options was never corrected, becausemount -askips already-mounted filesystems. This adds opt-in reconciliation of a live mount when its options drift from the promise.New
mountbody attributes:remount(default false) — reconcile a live mount's options when they differ.remount_methods(default { remount, unmount_mount }) — mechanisms tried in order, re-reading the live mount after each (the kernel returns success from a remount even when it ignores NFS-negotiated options, so convergence is verified rather than assumed).remount_timeout— bounds the unmount/mount path against a hung or unreachable server.Also corrects the mount-info handling this relies on:
GetFstabEntryOptionsreturned the fstab type field instead of the options field (spurious rewrite every run);ReplaceFstabEntryleaked the previous entry; foreign-filesystem detection is restored by keeping the fstype separate from the kernel-resolved options; the mount point directory is again created unconditionally before mounting; an accidentalStringIsSHA1Hexcommunity-build stub incf-serverd/server_tls.cis removed; andOptionsSubsetMatchestreats thedefaultspseudo-option asrw(the kernel never echoesdefaults).Unit coverage is added in
tests/unit/nfs_test.c. The real NFS reconcile/escalation behavior is covered by the system-testing PR.Resolves CFE-90 and its duplicate CFE-1864; relates to CFE-1539 (fstab maintenance) and CFE-1863 (mount -a scope).
Ticket: https://northerntech.atlassian.net/browse/CFE-90
Together with: cfengine/documentation#3731
Together with: https://github.com/cfengine/system-testing/pull/693