CFE-90: reconcile storage mount options when they drift (opt-in remount)#6222
Draft
nickanderson wants to merge 13 commits into
Draft
CFE-90: reconcile storage mount options when they drift (opt-in remount)#6222nickanderson wants to merge 13 commits into
nickanderson wants to merge 13 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)
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Member
Author
|
@cf-bottom jenkins please |
nickanderson
added a commit
to nickanderson/core
that referenced
this pull request
Jul 9, 2026
- 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.
Member
Author
|
@cf-bottom jenkins please |
|
Sure, I triggered a build: Jenkins: https://ci.cfengine.com/job/pr-pipeline/14143/ Packages: http://buildcache.cfengine.com/packages/testing-pr/jenkins-pr-pipeline-14143/ |
Member
Author
|
@cf-bottom jenkins please |
|
Sure, I triggered a build: Jenkins: https://ci.cfengine.com/job/pr-pipeline/14145/ Packages: http://buildcache.cfengine.com/packages/testing-pr/jenkins-pr-pipeline-14145/ |
Member
Author
|
@cf-bottom jenkins please |
|
Sure, I triggered a build: Jenkins: https://ci.cfengine.com/job/pr-pipeline/14148/ Packages: http://buildcache.cfengine.com/packages/testing-pr/jenkins-pr-pipeline-14148/ |
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>
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>
- 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.
VerifyMount, VerifyUnmount, and the new ReconcileMountOptions reported PROMISE_RESULT_CHANGE from a bare `if (!DONTDO)` block. That defines the classes body's promise_repaired set even on a dry-run (-n) or an action_policy => "warn" promise - a run that changes nothing - so a dependent promise keyed on those classes would fire during a no-op run. Gate the work on MakingInternalChanges() instead, the EnforcePromise equivalent called out in CFE-3366 ((!DONTDO) && action != cfa_warn): normal mode acts and reports CHANGE, dry-run/simulate/warn logs a warning and reports WARN with no change. A remount is a live side effect not captured by the simulate sandbox, so MakingInternalChanges (normal mode only), not MakingChanges, is the correct gate. Also log the "device busy" interruptions at LOG_LEVEL_ERR instead of LOG_LEVEL_INFO (3 sites: VerifyInFstab removal, VerifyMount, VerifyUnmount). INFO is for changes actually made (cf. RecordChange); an INTERRUPTED outcome belongs at ERR (cf. RecordInterruption), which is also what the rest of the tree uses for that outcome. While restructuring, free the leaked options string on VerifyMount's two error returns and return `result` instead of a bare `1` from its busy branch. Ticket: CFE-3366 Changelog: Title Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
nickanderson
force-pushed
the
CFE-90/master_mount_options
branch
from
9de8d09 to
4aedfe0
Compare
Cites nfs(5) and mount(8) for the kernel-behavior assertions in OptionsSubsetMatches and LiveMountConverged: NFS options negotiated by client and server are reported in /proc/mounts, the "defaults" pseudo-option expands to rw,suid,dev,exec,auto,nouser,async, and NFS-specific options cannot be changed by a remount (so the resulting state is verified rather than the remount exit status trusted). Ticket: CFE-90 Changelog: None
Member
Author
|
@cf-bottom jenkins please |
|
Alright, I triggered a build: Jenkins: https://ci.cfengine.com/job/pr-pipeline/14151/ Packages: http://buildcache.cfengine.com/packages/testing-pr/jenkins-pr-pipeline-14151/ |
A storage promise whose filesystem was already mounted with the correct source reported "mounted as promised" and never touched fstab, so a missing fstab entry was not restored and an options change was not written to fstab (CFE-1539). VerifyInFstab now also runs on the mounted-correctly path (when edit_fstab is set), independent of the opt-in live 'remount': keeping fstab correct is the documented behavior of mount_options, while remounting a live filesystem stays the disruptive, opt-in part gated by 'remount'. The fstab options comparison is left exact (order-sensitive): fstab option order is significant for duplicated/conflicting options, and the earlier GetFstabEntryOptions fix already stops the every-run rewrites. Ticket: CFE-1539 Changelog: Title
nickanderson
force-pushed
the
CFE-90/master_mount_options
branch
from
87c824d to
a98564d
Compare
A storage promise for a not-yet-mounted filesystem armed CF_MOUNTALL, so CFEngine ran 'mount -a'/'mount -va' at the end of the pass. That mounts every unmounted entry in fstab - unrelated devices and foreign filesystem types included - as a side effect of a single promise (CFE-1863). The not-mounted path now mounts just the promised filesystem with VerifyMount (the existing surgical primitive), then persists it to fstab when edit_fstab is set. CF_MOUNTALL / MountAll are kept for the explicit 'mountfilesystems' agent control, which still means "mount everything in fstab". Ticket: CFE-1863 Changelog: Title
An unmount promise that named mount_source/mount_server was warned about as "probably an error", and the server was never used to select which mount to act on - so a policy could not unmount one specific mount (e.g. from a server being migrated away) without affecting others (CFE-2350). The bogus warning is removed, and the server (host) is now part of the "mounted correctly" identity check, gated on remount or unmount so it only engages when the promise opts into disruptive mount management. An unmount promise that finds a *different* filesystem at the mount point now leaves it - and its fstab entry - untouched. LiveMountConverged likewise treats the server as part of identity, so a remount-in-place that cannot change the server escalates to unmount_mount. Ticket: CFE-2350 Changelog: Title
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.It also completes the fstab-maintenance half of CFE-1539. Previously a filesystem already mounted with the correct source was reported "mounted as promised" and fstab was never consulted, so a missing fstab entry was not restored and an options change was not written until the mount happened to be redone.
VerifyInFstabnow runs on the mounted-correctly path too (whenedit_fstab => "true"), deliberately independent of the opt-in liveremount: keeping fstab correct is the documented behavior ofmount_options, while remounting a live filesystem is the disruptive part gated byremount.And it resolves CFE-1863: a storage promise for a not-yet-mounted filesystem used to arm
mount -a(mount -vaon Linux), which mounts every unmounted fstab entry — unrelated devices and foreign filesystem types included — as a side effect of a single promise. The not-mounted path now mounts just the promised filesystem surgically (VerifyMount), then persists it to fstab. Themount -amechanism (MountAll) is retained only for the explicitmountfilesystemsagent-control attribute, which still means "mount everything in fstab".Finally it resolves CFE-2350: an unmount promise that named
mount_source/mount_serverwas logged as "probably an error", and the server was never used to pick which mount to act on — so you couldn't unmount one specific mount (e.g. from a server being migrated away) without affecting others. The bogus warning is removed, and the server (host) is now part of the "mounted correctly" identity check, gated onremountorunmountso it only engages when the promise opts into disruptive mount management. An unmount promise that finds a different filesystem at the mount point leaves it — and its fstab entry — untouched, andLiveMountConvergedtreats the server as identity so a remount-in-place that can't change it escalates tounmount_mount.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). The fstab options comparison is left exact (order-sensitive), because fstab option order is significant for duplicated/conflicting options.Unit coverage is added in
tests/unit/nfs_test.c(option subset matching). The real NFS reconcile/escalation, fstab maintenance, and surgical single-filesystem mount behavior are covered by the system-testing PR.References for the kernel/OS behavior described above:
mount -askipping already-mounted filesystems, anddefaults=rw,suid,dev,exec,auto,nouser,async— mount(8)/proc/mounts) and NFS-specific options not modifiable on remount — nfs(5)Resolves CFE-90 and its duplicate CFE-1864, CFE-1539 (fstab maintenance), CFE-1863 (mount -a scope), and CFE-2350 (target a specific mount on unmount).
Ticket: https://northerntech.atlassian.net/browse/CFE-90
Together with: https://github.com/cfengine/system-testing/pull/693