Skip to content

cephfs: move fscrypt lock into the CephFS RADOS namespace - #6539

Open
Greenpepper15 wants to merge 5 commits into
ceph:develfrom
Greenpepper15:wip-cephfs-fscrypt-lock-namespace
Open

cephfs: move fscrypt lock into the CephFS RADOS namespace#6539
Greenpepper15 wants to merge 5 commits into
ceph:develfrom
Greenpepper15:wip-cephfs-fscrypt-lock-namespace

Conversation

@Greenpepper15

@Greenpepper15 Greenpepper15 commented Sep 4, 2026

Copy link
Copy Markdown

Describe what this PR does

Fixes the first item of #6531: the RADOS lock that serializes fscrypt setup for encrypted volumes lives in the
default namespace of the metadata pool, while every other object Ceph-CSI creates for a CephFS cluster lives
in the configured RADOS namespace (cephFS.radosNamespace, default csi).

This PR places the lock in the RADOS namespace configured by cephFS.radosNamespace.

Is there anything that requires special attention

Is the change backward compatible?

Yes, I implemented a transitional legacy lock mechanism that keeps mixed-version deployments safe. This
mechanism makes nodes that include the changes of this PR try to get both the new lock (placed in
cephFS.radosNamespace) and the old lock (placed in the default RADOS namespace).

However, the "old lock" in the default RADOS namespace is not required in two cases. If the CephX capabilities
of the node cannot access the default namespace, the mount continues without it (such a deployment could never
stage an encrypted volume before this change, so no old nodeplugin can be holding the old lock). Volumes
without a RADOS namespace (static volumes) take only one lock, in the default namespace, because both locks
would be the same object. Any other failure to take the old lock fails the mount.

The legacy lock is marked with a TODO for removal once upgrades from releases that lock in the default
namespace are no longer supported.

Do you have any questions?

  • Is protecting the rolling-upgrade edge case worth the code it takes? The core fix is the single
    SetNamespace() call. The rest of the PR exists to keep an old nodeplugin and an upgraded one serialized
    while updating. The serialization only matters when the same encrypted volume is staged on a non-updated
    ceph-csi instance and an updated ceph-csi instance at the same time. If that window is considered
    unimportant, the legacy lock can be dropped (together with the sentinel-error commit it depends on and its
    e2e check). Instead the upgrade documentation would warn operators not to start pods that mount encrypted
    volumes while the nodeplugin upgrade is running. Ceph-CSI cannot enforce this since a nodeplugin does not
    know that a rolling upgrade is in progress, and the legacy lock is exactly the mechanism that serializes the
    two sides. Avoiding the race would be left entirely to the operator.

  • Is the e2e test's dependency on RADOS lock internals acceptable? The lock is released before
    NodeStageVolume returns, so the test can not observe it held. Instead it checks for the object that
    cls_lock leaves behind on unlock, which holds for every lock type except LOCK_EXCLUSIVE_EPHEMERAL. Should
    cls_lock ever stop leaving the object behind, or the fscrypt lock become ephemeral, the test breaks without
    any product regression. An e2e test was asked for in cephfs: cephFS.radosNamespace is not applied consistently #6531, so if this dependency is not acceptable, I would
    rework the check rather than drop it. However suggestions for a better way to observe the lock's namespace
    are welcome (I am unsure how to do it in a different manner).

Related issues

Part of #6531.

Future concerns

  • Remove the transitional legacy lock once upgrades from releases that take the fscrypt lock in the default
    namespace are no longer supported (marked with a TODO in the code).

AI assistance: this code was developed with the help of an AI assistant (Claude). Each commit carries the
Assisted-by: Claude Code <noreply@anthropic.com> trailer, per AGENTS.md.

Checklist:

  • Commit Message Formatting: Commit titles and messages follow
    guidelines in the developer
    guide
    .
  • Reviewed the developer guide on Submitting a Pull
    Request
  • Pending release
    notes

    updated with breaking and/or notable changes for the next major release.
  • Documentation has been updated, if necessary.
  • Unit tests have been added, if necessary.
  • Integration tests have been added, if necessary.

Show available bot commands

These commands are normally not required, but in case of issues, leave any of
the following bot commands in an otherwise empty comment in this PR:

  • /retest ci/centos/<job-name>: retest the <job-name> after unrelated
    failure (please report the failure too!)

CI job ordering.

Depends-on: #6497

@Greenpepper15
Greenpepper15 requested review from a team as code owners September 4, 2026 10:30
@mergify mergify Bot added the component/cephfs Issues related to CephFS label Sep 4, 2026
nixpanic
nixpanic previously approved these changes Sep 8, 2026
@nixpanic
nixpanic requested a review from a team September 8, 2026 15:56
Madhu-1
Madhu-1 previously approved these changes Sep 9, 2026
Comment thread internal/cephfs/nodeserver.go
@nixpanic

nixpanic commented Sep 9, 2026

Copy link
Copy Markdown
Member

Wait for #6497 to be merged before starting full e2e and merging on this PR.

@mergify

mergify Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Merge Protections

🟢 All 2 merge protections satisfied — ready to merge.

Show 2 satisfied protections

🟢 ⛓️ Depends-On Requirements

Requirement based on the presence of Depends-On in the body of the pull request

🟢 🚦 Auto-queue

When all merge protections are satisfied, this pull request will be queued automatically.

@nixpanic

Copy link
Copy Markdown
Member

/queue

@ceph-csi-bot

Copy link
Copy Markdown
Collaborator

@Mergifyio rebase

@mergify

mergify Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

rebase

❌ This pull request comes from a fork and cannot be rebased

Details

GitHub refuses an OAuth token on its rebase API for a fork, so rebasing one means impersonating a GitHub user to force-push the contributor's branch. Mergify does not do that.

Use the update action or the @mergifyio update command instead: it brings the pull request up to date by merging the base branch into it, and needs no impersonation. It only has something to do when the pull request is behind its base branch, so if what the branch needs is a linear history, its author has to rebase it themselves.

@Greenpepper15

Greenpepper15 commented Sep 11, 2026

Copy link
Copy Markdown
Author

Should I rebase manually?
I don't want to confuse the mergify bot workflow so I will wait with rebasing until I get a confirmation.

@nixpanic

Copy link
Copy Markdown
Member

Should I rebase manually? I don't want to confuse the mergify bot workflow so I will wait with rebasing until I get a confirmation.

Hey @Greenpepper15 , yes, please rebase. The approvals will be dropped, but I'll get them added as soon as possible again. After the rebase, we'll start the full CI cycle.

In the mean time, we're looking into Mergify changes that allow rebasing/updating again.

Thanks!

David Mohren added 4 commits September 11, 2026 09:14
LockExclusive folds every unexpected return value into one generic
error. Return a distinguishable error for EPERM and EACCES, so that
callers can tell a denied lock apart from a failed one. Locking is a
RADOS class operation, so it needs the class-exec permission on the
pool, and caps that cover the RADOS namespace of the IO context.

Signed-off-by: David Mohren <david.mohren@clyso.com>
Assisted-by: Claude Code <noreply@anthropic.com>
The RADOS lock that serializes the fscrypt setup of an encrypted
volume should be placed in the RADOS namespace configured by
cephFS.radosNamespace and not in the default namespace of the
metadata pool, like every other object Ceph-CSI creates there.

Signed-off-by: David Mohren <david.mohren@clyso.com>
Assisted-by: Claude Code <noreply@anthropic.com>
Handle the rolling-upgrade window in which upgraded nodes take the
fscrypt lock in cephFS.radosNamespace while not-yet-upgraded nodes
still take it in the default namespace, unserialized against each
other.

Take both locks. The new lock is exclusively used when the old one is
not available. That is the case when the credentials have no
permission for the default namespace (such a deployment could never
stage an encrypted volume before this change, so no old node can hold
the old lock), and when the volume has no RADOS namespace (both locks
would be the same object).

Signed-off-by: David Mohren <david.mohren@clyso.com>
Assisted-by: Claude Code <noreply@anthropic.com>
Mount an encrypted volume and check that the fscrypt lock object
exists in the CephFS RADOS namespace, and, while the transitional
legacy lock exists, in the default namespace as well. The lock is
released before NodeStageVolume returns; the check relies on cls_lock
leaving the object behind on unlock.

Signed-off-by: David Mohren <david.mohren@clyso.com>
Assisted-by: Claude Code <noreply@anthropic.com>
@Greenpepper15
Greenpepper15 force-pushed the wip-cephfs-fscrypt-lock-namespace branch from cc135b2 to b087b71 Compare September 11, 2026 07:27
@mergify
mergify Bot dismissed stale reviews from Madhu-1 and nixpanic September 11, 2026 07:28

Pull request has been modified.

@nixpanic

Copy link
Copy Markdown
Member

/queue

@ceph-csi-bot

Copy link
Copy Markdown
Collaborator

@Mergifyio rebase

@mergify

mergify Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

rebase

❌ This pull request comes from a fork and cannot be rebased

Details

GitHub refuses an OAuth token on its rebase API for a fork, so rebasing one means impersonating a GitHub user to force-push the contributor's branch. Mergify does not do that.

Use the update action or the @mergifyio update command instead: it brings the pull request up to date by merging the base branch into it, and needs no impersonation. It only has something to do when the pull request is behind its base branch, so if what the branch needs is a linear history, its author has to rebase it themselves.

@nixpanic

Copy link
Copy Markdown
Member

@Greenpepper15 , no need to do anything about these Mergify things, we're still testing it out.

@nixpanic nixpanic added the ok-to-test Label to trigger E2E tests label Sep 11, 2026
@ceph-csi-bot

Copy link
Copy Markdown
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.36

@ceph-csi-bot

Copy link
Copy Markdown
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.34

@ceph-csi-bot

Copy link
Copy Markdown
Collaborator

/test ci/centos/upgrade-tests-cephfs

@ceph-csi-bot

Copy link
Copy Markdown
Collaborator

/test ci/centos/mini-e2e/k8s-1.36

@ceph-csi-bot

Copy link
Copy Markdown
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.35

@ceph-csi-bot

Copy link
Copy Markdown
Collaborator

/test ci/centos/mini-e2e/k8s-1.34

@ceph-csi-bot

Copy link
Copy Markdown
Collaborator

/test ci/centos/upgrade-tests-rbd

@ceph-csi-bot

Copy link
Copy Markdown
Collaborator

/test ci/centos/mini-e2e/k8s-1.35

@ceph-csi-bot ceph-csi-bot added ci/in-progress/e2e This label acts like a guard and prevents Mergify from adding the `ok-to-test` label again. and removed ok-to-test Label to trigger E2E tests labels Sep 11, 2026
@mergify mergify Bot removed the ci/in-progress/e2e This label acts like a guard and prevents Mergify from adding the `ok-to-test` label again. label Sep 11, 2026
@mergify

mergify Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Merge Queue Status

  • Entered queue2026-09-11 11:50 UTC · Rule: default · triggered by merge protections
  • 🟠 Checks running · in-place · dashboard
  • ⏳ Merge · ETA: 2026-09-11 11:50 UTC 🚀
Required conditions to merge
Waiting for
  • any of:
    • base=ci/centos
    • status-success=multi-arch-build
  • any of:
    • label=ci/skip/e2e
    • all of:
      • base~=^(release-.+)$
      • status-success=ci/centos/k8s-e2e-external-storage/1.33
      • status-success=ci/centos/k8s-e2e-external-storage/1.34
      • status-success=ci/centos/k8s-e2e-external-storage/1.35
      • status-success=ci/centos/mini-e2e-helm/k8s-1.33
      • status-success=ci/centos/mini-e2e-helm/k8s-1.34
      • status-success=ci/centos/mini-e2e-helm/k8s-1.35
      • status-success=ci/centos/mini-e2e/k8s-1.33
      • status-success=ci/centos/mini-e2e/k8s-1.34
      • status-success=ci/centos/mini-e2e/k8s-1.35
      • status-success=ci/centos/upgrade-tests-cephfs
      • status-success=ci/centos/upgrade-tests-rbd
    • all of:
      • base=release-v3.16
      • status-success=ci/centos/k8s-e2e-external-storage/1.32
      • status-success=ci/centos/k8s-e2e-external-storage/1.33
      • status-success=ci/centos/k8s-e2e-external-storage/1.34
      • status-success=ci/centos/mini-e2e-helm/k8s-1.32
      • status-success=ci/centos/mini-e2e-helm/k8s-1.33
      • status-success=ci/centos/mini-e2e-helm/k8s-1.34
      • status-success=ci/centos/mini-e2e/k8s-1.32
      • status-success=ci/centos/mini-e2e/k8s-1.33
      • status-success=ci/centos/mini-e2e/k8s-1.34
      • status-success=ci/centos/upgrade-tests-cephfs
      • status-success=ci/centos/upgrade-tests-rbd
    • all of:
      • status-success=ci/centos/k8s-e2e-external-storage/1.34
      • status-success=ci/centos/k8s-e2e-external-storage/1.35
      • status-success=ci/centos/k8s-e2e-external-storage/1.36
      • status-success=ci/centos/mini-e2e/k8s-1.34
      • status-success=ci/centos/mini-e2e/k8s-1.35
      • status-success=ci/centos/mini-e2e/k8s-1.36
      • status-success=ci/centos/upgrade-tests-cephfs
      • status-success=ci/centos/upgrade-tests-rbd
    • all of:
      • base=ci/centos
      • status-success=ci/centos/jjb-validate
      • status-success=ci/centos/job-validation
All queue conditions
  • any of [🔀 queue conditions]:
    • all of [📌 queue conditions of queue rule default]:
      • all of:
        • any of:
          • base=ci/centos
          • all of:
            • status-success=multi-arch-build
            • base!=ci/centos
            • status-success=codespell
            • status-success=go-test
            • status-success=golangci-lint
            • status-success=lint-extras
            • status-success=mod-check
            • status-success=uncommitted-code-check
            • any of:
              • status-success=commitlint
              • author=dependabot[bot]
        • any of:
          • all of:
            • label=ci/skip/e2e
            • base!=ci/centos
          • all of:
            • base~=^(release-.+)$
            • status-success=ci/centos/k8s-e2e-external-storage/1.33
            • status-success=ci/centos/k8s-e2e-external-storage/1.34
            • status-success=ci/centos/k8s-e2e-external-storage/1.35
            • status-success=ci/centos/mini-e2e-helm/k8s-1.33
            • status-success=ci/centos/mini-e2e-helm/k8s-1.34
            • status-success=ci/centos/mini-e2e-helm/k8s-1.35
            • status-success=ci/centos/mini-e2e/k8s-1.33
            • status-success=ci/centos/mini-e2e/k8s-1.34
            • status-success=ci/centos/mini-e2e/k8s-1.35
            • status-success=ci/centos/upgrade-tests-cephfs
            • status-success=ci/centos/upgrade-tests-rbd
          • all of:
            • base=release-v3.16
            • status-success=ci/centos/k8s-e2e-external-storage/1.32
            • status-success=ci/centos/k8s-e2e-external-storage/1.33
            • status-success=ci/centos/k8s-e2e-external-storage/1.34
            • status-success=ci/centos/mini-e2e-helm/k8s-1.32
            • status-success=ci/centos/mini-e2e-helm/k8s-1.33
            • status-success=ci/centos/mini-e2e-helm/k8s-1.34
            • status-success=ci/centos/mini-e2e/k8s-1.32
            • status-success=ci/centos/mini-e2e/k8s-1.33
            • status-success=ci/centos/mini-e2e/k8s-1.34
            • status-success=ci/centos/upgrade-tests-cephfs
            • status-success=ci/centos/upgrade-tests-rbd
          • all of:
            • status-success=ci/centos/k8s-e2e-external-storage/1.34
            • status-success=ci/centos/k8s-e2e-external-storage/1.35
            • status-success=ci/centos/k8s-e2e-external-storage/1.36
            • status-success=ci/centos/mini-e2e/k8s-1.34
            • status-success=ci/centos/mini-e2e/k8s-1.35
            • status-success=ci/centos/mini-e2e/k8s-1.36
            • status-success=ci/centos/upgrade-tests-cephfs
            • status-success=ci/centos/upgrade-tests-rbd
            • base=devel
          • all of:
            • base=ci/centos
            • status-success=ci/centos/jjb-validate
            • status-success=ci/centos/job-validation
        • #approved-reviews-by>=2
        • #changes-requested-reviews-by=0
        • approved-reviews-by=@ceph/ceph-csi-contributors
        • approved-reviews-by=@ceph/ceph-csi-maintainers
        • label!=DNM
        • status-success=DCO
      • github-review-approved [🛡 GitHub branch protection]
  • -closed [📌 queue requirement]
  • -conflict [📌 queue requirement]
  • -draft [📌 queue requirement]
  • depends-on = ceph/ceph-csi#6497 [⛓️ Feat/nfs export names #6497]
  • any of [📌 queue -> configuration change requirements]:
    • -mergify-configuration-changed
    • check-success = @mergify/Configuration changed
    • check-success = @mergify/Configuration has been deleted
  • any of [📌 queue requirement]:
    • check-success = @mergify/Mergify Merge Protections
    • check-neutral = @mergify/Mergify Merge Protections
    • check-skipped = @mergify/Mergify Merge Protections

@ceph-csi-bot ceph-csi-bot added ok-to-test Label to trigger E2E tests and removed queued/rebase labels Sep 11, 2026
@ceph-csi-bot

Copy link
Copy Markdown
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.36

@ceph-csi-bot

Copy link
Copy Markdown
Collaborator

/test ci/centos/upgrade-tests-cephfs

@ceph-csi-bot

Copy link
Copy Markdown
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.34

@ceph-csi-bot

Copy link
Copy Markdown
Collaborator

/test ci/centos/mini-e2e/k8s-1.36

@ceph-csi-bot

Copy link
Copy Markdown
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.35

@ceph-csi-bot

Copy link
Copy Markdown
Collaborator

/test ci/centos/upgrade-tests-rbd

@ceph-csi-bot

Copy link
Copy Markdown
Collaborator

/test ci/centos/mini-e2e/k8s-1.34

@ceph-csi-bot

Copy link
Copy Markdown
Collaborator

/test ci/centos/mini-e2e/k8s-1.35

@mergify mergify Bot added the queued label Sep 11, 2026
@ceph-csi-bot ceph-csi-bot added ci/in-progress/e2e This label acts like a guard and prevents Mergify from adding the `ok-to-test` label again. and removed ok-to-test Label to trigger E2E tests labels Sep 11, 2026
@mergify mergify Bot removed the ci/in-progress/e2e This label acts like a guard and prevents Mergify from adding the `ok-to-test` label again. label Sep 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component/cephfs Issues related to CephFS queued

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants