cephfs: add SetSubVolCSIMetadata to set CSI metadata on subvolumes#6174
Conversation
4c9d310 to
e7fb4f3
Compare
| // CephFS PVs always have "fsName" in their volume attributes (required | ||
| // StorageClass parameter), while RBD PVs do not. | ||
| _, isCephFS := pv.Spec.CSI.VolumeAttributes["fsName"] | ||
| if isCephFS { |
There was a problem hiding this comment.
Instead of doing a if-return-action and standard-action, put the difference in handling in a switch/case. This makes the code easier to read and better maintainable.
With this approach, it is not clear if there should be a rbdVolID != volumeHandler check for CephFS too. Possibly it was missed as it is done at the end of the function.
There was a problem hiding this comment.
For CephFS, we are only setting the CSI metadata and no new volumeID is generated to compare. rbdVolID != volumeHandler is only for RBD PVC.
There was a problem hiding this comment.
Yes, I understand that, but we try to keep this generic flow in functions:
- validate parameters, return on error
- do something, return on error
- do something else, return on error
- final things
- return success
The point is, that the if isCephFS { section returns from the function on success. This isn't structured well, as CephFS/RBD are handled both in this function, but not equally (for the lack of better phrasing).
To keep the understandable and clear readable structure above, it is better to handle the differences between CephFS/RBD in a switch (or if/else if you prefer that).
My 1st impression was that the handling for CephFS/RBD should be done in their own functions. But because it is a single call for CephFS, and only two actions for RBD, placing them inside a switch statement is clean enough.
There was a problem hiding this comment.
With only two cases, I'll prefer if/else directly.
82cc455 to
ab3b7d6
Compare
| // Determine PV type from volume attributes and dispatch accordingly. | ||
| // CephFS PVs always have "fsName" in their volume attributes (required | ||
| // StorageClass parameter), while RBD PVs do not. | ||
| _, isCephFS := pv.Spec.CSI.VolumeAttributes["fsName"] | ||
| if isCephFS { |
There was a problem hiding this comment.
@Rakshith-R we already pass type=controller for the omap sidecars.
|
Can be rebased and |
|
@Mergifyio rebase |
Add support for setting CSI metadata (PV/PVC info) on CephFS subvolumes. The PV controller now distinguishes between CephFS and RBD PVs and dispatches accordingly. Signed-off-by: Praveen M <m.praveen@ibm.com>
✅ Branch has been successfully rebased |
ab3b7d6 to
196d60a
Compare
|
/test ci/centos/mini-e2e-helm/k8s-1.35 |
|
/test ci/centos/mini-e2e/k8s-1.35 |
|
/retest ci/centos/mini-e2e/k8s-1.35 |
|
@Mergifyio rebase |
|
/test ci/centos/k8s-e2e-external-storage/1.33 |
|
/test ci/centos/upgrade-tests-cephfs |
|
/test ci/centos/k8s-e2e-external-storage/1.35 |
|
/test ci/centos/k8s-e2e-external-storage/1.34 |
|
/test ci/centos/mini-e2e-helm/k8s-1.33 |
|
/test ci/centos/upgrade-tests-rbd |
|
/test ci/centos/mini-e2e-helm/k8s-1.34 |
|
/test ci/centos/mini-e2e-helm/k8s-1.35 |
|
/test ci/centos/mini-e2e/k8s-1.33 |
|
/test ci/centos/mini-e2e/k8s-1.34 |
|
/test ci/centos/mini-e2e/k8s-1.35 |
☑️ Nothing to do, the required conditions are not metDetails
|
Merge Queue Status
This pull request spent 5 seconds in the queue, with no time running CI. Required conditions to merge
|
Describe what this PR does
Add support for setting CSI metadata (PV/PVC info) on CephFS subvolumes. The PV controller now distinguishes between CephFS and RBD PVs and dispatches accordingly.
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:
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 unrelatedfailure (please report the failure too!)