cephfs: disallow creating small volumes from snapshot/volume #2788
Conversation
|
@Mergifyio rebase |
|
|
✅ Branch has been successfully rebased |
|
@ceph/ceph-csi-maintainers @ceph/contributors need a review if anyone has free time. |
|
@Mergifyio rebase |
✅ Branch has been successfully rebased |
|
Actually this will never happen from Kube CO and also on Openshift. In our documentation we have mentioned, only those as tested or Known to Work https://github.com/ceph/ceph-csi#known-to-work-co-platforms .. The confusion this could bring here is, if we started to think about other COs , then many areas have to be looked upon in various code paths.. |
we never mentioned that this will won't work or we should only fix it as per the kubernetes/OCP requirement. most of the time we will try to align with the standard CSI specification. |
|
@nixpanic do you have any comments on this one? |
internal/cephfs/controllerserver.go
Outdated
| @@ -199,6 +234,11 @@ func (cs *ControllerServer) CreateVolume( | |||
| defer parentVol.Destroy() | |||
| } | |||
|
|
|||
| err = checkValidCreateVolumeRequest(volOptions, parentVol, pvID, sID) | |||
| if err != nil { | |||
| return nil, err | |||
There was a problem hiding this comment.
This just returns the error, instead of converting it to a gRPC error. The checkValidCreateVolumeRequest() function takes care of that, but it would be cleaner to do it here.
Ideally top-level gRPC functions are the only ones that return gRPC errors, all other functions should just return plain errors so that they can be re-used outside of the direct gRPC flow.
|
/retest ci/centos/mini-e2e-helm/k8s-1.22 |
|
/retest ci/centos/upgrade-tests-cephfs |
|
/retest ci/centos/k8s-e2e-external-storage/1.22 |
|
/retest ci/centos/mini-e2e-helm/k8s-1.21 |
|
/retest ci/centos/k8s-e2e-external-storage/1.22 |
|
/retest ci/centos/mini-e2e-helm/k8s-1.22 |
|
/retest ci/centos/mini-e2e-helm/k8s-1.23 |
|
This pull request now has conflicts with the target branch. Could you please resolve conflicts and force push the corrected changes? 🙏 |
mergify
bot
dismissed stale reviews from nixpanic and yati1998
Pull request has been modified.
|
@Mergifyio rebase |
✅ Branch has been successfully rebased |
|
/retest ci/centos/mini-e2e-helm/k8s-1.21 |
|
/retest ci/centos/mini-e2e-helm/k8s-1.21 |
as per the CSI standard the size is optional parameter, as we are allowing the clone to a bigger size today we need to block the clone to a smaller size as its a have side effects like data corruption etc. Note:- Even though this check is present in kubernetes sidecar as CSI is CO independent adding the check here. fixes: ceph#2718 Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
|
/retest ci/centos/mini-e2e-helm/k8s-1.21 |
|
@Mergifyio rebase |
☑️ Nothing to do
|
|
@Mergifyio refresh |
✅ Pull request refreshed |
as per the CSI standard, the size is an optional parameter, as we are allowing the clone to a bigger size today we need to block the clone to a smaller size as its a have side effects like data corruption, etc.
Note:- Even though this check is present in the kubernetes sidecar as CSI is CO independent adding the check here.
fixes: #2718
Signed-off-by: Madhu Rajanna madhupr007@gmail.com