util: improve static PV handling - #6328
Conversation
mergify
Bot
added
ci/skip/e2e
satoru-takeuchi
changed the title
9a65e64 to
eb95e05
Compare
|
I couldn't find the best https://github.com/ceph/ceph-csi/blob/devel/docs/development-guide.md#commit-messages |
| // IsOlderOrStaticPV returns true when the error from GetControllerPublishSecretRef indicates | ||
| // that the volume is either a static PV (non-CSI-formatted volume handle) or an older PV | ||
| // provisioned without a controller-publish secret configured. | ||
| func IsOlderOrStaticPV(err error) bool { |
There was a problem hiding this comment.
This doesnt look like the nice check. because we might get this errors on some corner cases, we should not be considering this errors as the source of truth to identify if its a static volume or not.
There was a problem hiding this comment.
I see. My intention is to clarify the early returns can also happen if the target volume is static volume in addition to older volume. In addition, Early return is unavoidable even in corner cases.
How about changing the function name (e.g. ShouldSkipControllerPublishSecret)?
There was a problem hiding this comment.
@Madhu-1 I changed my mind. I created a helper function (GetControllerPublishSecrets) to get the secret and decide whether the handler return early rather than creating a helper(ShouldSkipControllerPublishSecret) just for skip decision.
Please review this PR again.
There was a problem hiding this comment.
looks much better than earlier implementation, i have requested for reviews
There was a problem hiding this comment.
Could you review this PR?
0e1662c to
81adf59
Compare
- Clarify static PV must not specify controllerPublishSecretRef. - Improve handling volumeHandle validation error. The related discussion: ceph#6290 Signed-off-by: Satoru Takeuchi <satoru.takeuchi@gmail.com>
81adf59 to
5be7488
Compare
Madhu-1
left a comment
Madhu-1
left a comment
There was a problem hiding this comment.
changes LGTM, we need to update the doc about what functionality user will not get if they dont set the secretRef
|
/test ci/centos/mini-e2e/k8s-1.36/cephfs |
|
/test ci/centos/mini-e2e/k8s-1.36/rbd |
2 participants
Describe what this PR does
The related discussion:
#6290
Is there anything that requires special attention
This PR hardens the spec of static PVs. However, I believe it's not such a big problem since admins have already suffered from troubles if
controllerPublishSecretRefis specified on static volumes. For instance,ControllerUnpublishVolumefails forever inGenVolFromVolIDunless volumeHandle validation succeeds by accident.Related issues
Nothing.
Future concerns
Nothing.
Checklist:
guidelines in the developer
guide.
Request
notes
updated with breaking and/or notable changes for the next major release.