Skip to content

Store userid in metadata#5445

Merged
mergify[bot] merged 4 commits intoceph:develfrom
iPraveenParihar:store-userid-in-metadata
Aug 26, 2025
Merged

Store userid in metadata#5445
mergify[bot] merged 4 commits intoceph:develfrom
iPraveenParihar:store-userid-in-metadata

Conversation

@iPraveenParihar
Copy link
Copy Markdown
Contributor

Describe what this PR does

Problem

Without a mechanism to track nodeID:userID mappings per volume, it becomes challenging to:

Monitor which userIDs are actively being used across the cluster
Make informed decisions about key rotation, allowing safe cleanup of old keys

Proposed Solution

The solution involves tracking userID information by storing it in the metadata of RBD images and CephFS subvolumes using the key: .[rbd|cephfs].csi.ceph.com/userID/:

refer design doc (here)for more details:

Checklist:

  • Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide.
  • Reviewed the developer guide on [Submitting a Pull Request](https://github.com/ceph/ceph csi/blob/devel/docs/development-guide.md#development-workflow)
  • 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!)

@iPraveenParihar iPraveenParihar force-pushed the store-userid-in-metadata branch 5 times, most recently from 6efc0f4 to 4cb8950 Compare July 23, 2025 13:13
Comment thread internal/cephfs/driver.go Outdated
fs.cd = csicommon.NewCSIDriver(conf.DriverName, util.DriverVersion, conf.NodeID, conf.InstanceID,
conf.EnableFencing)
fs.cd = csicommon.NewCSIDriver(conf.DriverName, util.DriverVersion, conf.NodeID, conf.InstanceID)
fs.cd.SetEnableFencing(conf.EnableFencing)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of having Set... functions for everything, consider passing options like WithFencing to NewCSIDriver(). Similar to how gRPC CallOptions work, that makes it a little more dynamic and easier extendible.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed now, Thanks for the suggestion.

Copy link
Copy Markdown
Member

@nixpanic nixpanic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

surely there should be e2e tests for this?

Comment thread cmd/cephcsi.go Outdated
Comment thread PendingReleaseNotes.md Outdated
- rbd: add support for [CSI Snapshot Metadata Service RPCs](https://github.com/container-storage-interface/spec/blob/master/spec.md#snapshot-metadata-service-rpcs)
- feature: handle non graceful node shutdown [PR](https://github.com/ceph/ceph-csi/pull/5429/)
- refer design doc for more details - [here](docs/design/proposals/non-graceful-node-shutdown.md)
- feature: set nodeId:userId mapping metadata [PR](https://github.com/ceph/ceph-csi/pull/5445)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already feature section. we dont need to add feature: here

Comment thread internal/cephfs/controllerserver.go Outdated
// removeUserIdMapping attempts to remove nodeId:userId mapping metadata from the subvolume.
//
// Parameters:
// - nodeId: The ID of the node that may be fenced.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fenced?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo, addressed.

Comment thread internal/cephfs/controllerserver.go Outdated
Comment thread internal/csi-common/driver.go Outdated
return d.enableFencing
}

func (d *CSIDriver) SetEnableFencing(value bool) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

value is too generate please use better names

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed now,

Comment thread internal/csi-common/driver.go Outdated
Comment thread internal/rbd/controllerserver.go Outdated
@iPraveenParihar
Copy link
Copy Markdown
Contributor Author

Thanks @Madhu-1, for the early reviews.
Am working on avoiding duplications.

@iPraveenParihar iPraveenParihar force-pushed the store-userid-in-metadata branch 2 times, most recently from 2cb79f8 to ca1ef84 Compare July 24, 2025 11:19
@iPraveenParihar iPraveenParihar marked this pull request as ready for review July 24, 2025 11:28
@iPraveenParihar iPraveenParihar requested a review from Madhu-1 July 24, 2025 11:29
@iPraveenParihar
Copy link
Copy Markdown
Contributor Author

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

@iPraveenParihar
Copy link
Copy Markdown
Contributor Author

@Rakshith-R @nixpanic PTAL

@iPraveenParihar
Copy link
Copy Markdown
Contributor Author

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

Comment thread e2e/pod.go Outdated
Comment thread e2e/rbd.go
Comment thread e2e/cephfs.go
Comment thread internal/rbd/controllerserver.go
Comment thread internal/rbd/controllerserver.go
@iPraveenParihar iPraveenParihar force-pushed the store-userid-in-metadata branch from ca1ef84 to fcaf5c2 Compare July 24, 2025 13:36
@iPraveenParihar iPraveenParihar force-pushed the store-userid-in-metadata branch 2 times, most recently from 5ae51ff to 590381a Compare July 24, 2025 15:07
@iPraveenParihar
Copy link
Copy Markdown
Contributor Author

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

@iPraveenParihar iPraveenParihar force-pushed the store-userid-in-metadata branch from 590381a to b96d4d8 Compare July 24, 2025 16:07
@iPraveenParihar
Copy link
Copy Markdown
Contributor Author

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

@iPraveenParihar iPraveenParihar force-pushed the store-userid-in-metadata branch from b96d4d8 to 4baffa6 Compare July 24, 2025 17:14
@iPraveenParihar
Copy link
Copy Markdown
Contributor Author

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

@iPraveenParihar iPraveenParihar force-pushed the store-userid-in-metadata branch from 4baffa6 to 7c07363 Compare July 25, 2025 02:55
@iPraveenParihar
Copy link
Copy Markdown
Contributor Author

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

1 similar comment
@iPraveenParihar
Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Collaborator

@Madhu-1 Madhu-1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small nits and few previous comments need to be addressed in E2E, LGTM

Comment thread PendingReleaseNotes.md Outdated
- feature: handle non graceful node shutdown [PR](https://github.com/ceph/ceph-csi/pull/5429/)
- handle non graceful node shutdown [PR](https://github.com/ceph/ceph-csi/pull/5429/)
- refer design doc for more details - [here](docs/design/proposals/non-graceful-node-shutdown.md)
- set nodeId:userId mapping metadata [PR](https://github.com/ceph/ceph-csi/pull/5445)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nodeId:userId mapping in metadata

Comment thread e2e/pod.go Outdated
Comment thread charts/ceph-csi-cephfs/values.yaml
@iPraveenParihar iPraveenParihar changed the title [WIP]: Store userid in metadata Store userid in metadata Aug 25, 2025
@iPraveenParihar iPraveenParihar force-pushed the store-userid-in-metadata branch from 5178685 to b20201d Compare August 25, 2025 11:23
@iPraveenParihar
Copy link
Copy Markdown
Contributor Author

/test ci/centos/mini-e2e-helm/k8s-1.33

Copy link
Copy Markdown
Contributor

@Rakshith-R Rakshith-R left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@Rakshith-R Rakshith-R dismissed Madhu-1’s stale review August 26, 2025 11:09

Dismissing requested changes since they have been addressed.

@Rakshith-R
Copy link
Copy Markdown
Contributor

@Mergifyio queue

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Aug 26, 2025

queue

✅ The pull request has been merged automatically

Details

The pull request has been merged automatically at df84cab

This commit does the following operations:

1. In NodeStageVolume(): sets nodeId:userId mapping in image metadata.
   The userId is the ceph user used for mounting the subvolume.

2. In ControllerUnpulishVolume(): removes the nodeId:userId mapping from
   the image metadata.

Signed-off-by: Praveen M <m.praveen@ibm.com>
This commit does the following operations:

1. In NodeStageVolume(): sets nodeId:userId mapping in subvolume metadata.
   The userId is the ceph user used for mounting the subvolume.

2. In ControllerUnpulishVolume(): removes the nodeId:userId mapping from
   the subvolume metadata.

Signed-off-by: Praveen M <m.praveen@ibm.com>
Signed-off-by: Praveen M <m.praveen@ibm.com>
Signed-off-by: Praveen M <m.praveen@ibm.com>
@mergify mergify bot force-pushed the store-userid-in-metadata branch from b20201d to d033047 Compare August 26, 2025 11:10
@mergify mergify bot added the ok-to-test Label to trigger E2E tests label Aug 26, 2025
@ceph-csi-bot
Copy link
Copy Markdown
Collaborator

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

@ceph-csi-bot
Copy link
Copy Markdown
Collaborator

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

@ceph-csi-bot
Copy link
Copy Markdown
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.32

@ceph-csi-bot
Copy link
Copy Markdown
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.31

@ceph-csi-bot
Copy link
Copy Markdown
Collaborator

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

@ceph-csi-bot
Copy link
Copy Markdown
Collaborator

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

@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/upgrade-tests-rbd

@ceph-csi-bot
Copy link
Copy Markdown
Collaborator

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

@ceph-csi-bot
Copy link
Copy Markdown
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.33

@ceph-csi-bot
Copy link
Copy Markdown
Collaborator

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

@ceph-csi-bot ceph-csi-bot removed the ok-to-test Label to trigger E2E tests label Aug 26, 2025
@mergify mergify bot merged commit df84cab into ceph:devel Aug 26, 2025
37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants