ceph-csi icon indicating copy to clipboard operation
ceph-csi copied to clipboard

WIP: Set object lock for volumes for cephfs encryption

Open Sunnatillo opened this issue 1 year ago • 1 comments

Describe what this PR does

Provide some context for the reviewer

Is there anything that requires special attention

Do you have any questions?

Is the change backward compatible?

Are there concerns around backward compatibility?

Provide any external context for the change, if any.

For example:

  • Kubernetes links that explain why the change is required
  • CSI spec related changes/catch-up that necessitates this patch
  • golang related practices that necessitates this change

Related issues

Mention any github issues relevant to this PR. Adding below line will help to auto close the issue once the PR is merged.

Fixes: #issue_number

Future concerns

List items that are not part of the PR and do not impact it's functionality, but are work items that can be taken up subsequently.

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!)

Sunnatillo avatar Jun 27 '24 13:06 Sunnatillo

@Sunnatillo can you please check the CI failures?

Madhu-1 avatar Jul 03 '24 09:07 Madhu-1

@mergifyio rebase

nixpanic avatar Jul 11 '24 12:07 nixpanic

rebase

❌ Pull request can't be updated with latest base branch changes

Mergify needs the author permission to update the base branch of the pull request. @Nordix needs to authorize modification on its head branch.

mergify[bot] avatar Jul 11 '24 12:07 mergify[bot]

Thank you for reviews @nixpanic, @Madhu-1.

I rebased to latest devel branch.

Sunnatillo avatar Jul 11 '24 12:07 Sunnatillo

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

ceph-csi-bot avatar Jul 11 '24 13:07 ceph-csi-bot

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

ceph-csi-bot avatar Jul 11 '24 13:07 ceph-csi-bot

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

ceph-csi-bot avatar Jul 11 '24 13:07 ceph-csi-bot

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

ceph-csi-bot avatar Jul 11 '24 13:07 ceph-csi-bot

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

ceph-csi-bot avatar Jul 11 '24 13:07 ceph-csi-bot

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

ceph-csi-bot avatar Jul 11 '24 13:07 ceph-csi-bot

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

ceph-csi-bot avatar Jul 11 '24 13:07 ceph-csi-bot

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

ceph-csi-bot avatar Jul 11 '24 13:07 ceph-csi-bot

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

ceph-csi-bot avatar Jul 11 '24 13:07 ceph-csi-bot

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

ceph-csi-bot avatar Jul 11 '24 13:07 ceph-csi-bot

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

ceph-csi-bot avatar Jul 11 '24 13:07 ceph-csi-bot

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

ceph-csi-bot avatar Jul 11 '24 13:07 ceph-csi-bot

/test ci/centos/upgrade-tests-cephfs

ceph-csi-bot avatar Jul 11 '24 13:07 ceph-csi-bot

/test ci/centos/upgrade-tests-rbd

ceph-csi-bot avatar Jul 11 '24 13:07 ceph-csi-bot

@nixpanic, I see fscrypt.Unlock call for RBD fscrypt. Do we need to do the same here as well? do we need to https://github.com/ceph/ceph-csi/blob/e71a95fece24de8de8f1de51201a7314af4f3480/internal/rbd/nodeserver.go#L467-L474

iPraveenParihar avatar Jul 11 '24 14:07 iPraveenParihar

@nixpanic, I see fscrypt.Unlock call for RBD fscrypt. Do we need to do the same here as well?

No, for RBD that is not needed. RBD with a filesystem is never attached to more than one workernode at the same time, so initialization/unlocking of the filesystem will always be on a single node. It is different with ReadWriteMany RBD volumes in block-mode with encryption. The LUKS calls would benefit from a similar locking.

nixpanic avatar Jul 11 '24 15:07 nixpanic

Is there a config to use the lock? We built an image from devel branch (https://github.com/ceph/ceph-csi/tree/devel) and tried it out. However pods failed to come up and we got the following error:

Warning FailedMount 15s (x6 over 31s) kubelet MountVolume.MountDevice failed for volume "pvc-4642810a-7f00-4ae0-8583-ddbcee6ed314" : rpc error: code = Internal desc = Failed to lock volume ID 0001-0009-rook-ceph-00000000000000ad-d71d2949-db34-434e-8008-5497dad5c70b: rados: ret=-1, Operation not permitted

z2000l avatar Jul 23 '24 20:07 z2000l

I have tested this and encountered the same error as @z2000l. @Sunnatillo, could you please document any additional settings required or anything we might be overlooking?

NymanRobin avatar Jul 24 '24 06:07 NymanRobin