Driver doesn't throw a useful error with unformatted readonly disks

[davidz627]

Pre-formatted Disks

Disks that have been pre-formatted behave "properly" with readOnly on the PV Spec.

Caveat: NodeStage technically doesn't mount the disk with the ro option (from Pod.spec.volumes.persistentVolumeClaim.readOnly), but the NodePublish bind-mount should bind mount as ro.

Unformatted Disks

This CSI Driver

When an un-formatted disk is Attached as readOnly with AccessMode of Mount the driver will try to format a file-system on the disk (no matter whether Pod.spec.volumes.persistentVolumeClaim.readOnly is set or not) and fail with error:

 MountVolume.MountDevice failed for volume "podpv" : rpc error: code = Internal desc = Failed to format and mount device from ("/dev/disk/by-id/google-disk2") to ("/var/lib/kubelet/plugins/kubernetes.io/csi/pv/podpv/globalmount") with fstype ("ext4") and options ([]): exit status 1

In-tree Plugin

When an un-formatted disk is attached as readOnly with AccessMode of Mount the behavior of the (if Pod.spec.volumes.persistentVolumeClaim.readOnly is set) is to throw a nice error:

MountVolume.MountDevice failed for volume "podpv" : failed to mount unformatted volume as read only

if Pod.spec.volumes.persistentVolumeClaim.readOnly is not set we get the error:

MountVolume.MountDevice failed for volume "podpv" : exit status 1

Conclusions

AFAIK In the end the only difference is error messages but I thought I'd write this up since I did the exploration.

To give better error messages the NodeStage needs to know that the mount was requested as ReadOnly so that it can detect this error and throw a nicer error. (if put into mount options for FormatAndMount we get the same error behavior as in-tree)

There are 2 options:

• Add the ro option to the MountFlags passed in to the driver from the Kubernetes side that comes from the pvSpec
  • Issue might be that different drivers want to interpret readOnly mount differently and that ro in MountFlags only works for the PD model (?)
• Add a readOnly bool to the NodeStageVolume operations that is propagated from the pvSpec.
  • Significant change as it requires CSI Spec addition

[davidz627]

/cc @msau42 @saad-ali @hantaowang
after investigation this doesn't seem super high priority. let me know if you think differently

[msau42]

It seems like an oversight that NodeStageVolume doesn't pass in a readonly flag?

Agree since the end behavior is that they both result in error it's not high priority to fix. Is there anyway we can return a better error message than "exit code 1"? Can we not get the actual error code/msg from the format command?

[davidz627]

Is there anyway we can return a better error message than "exit code 1"

This is the behavior of in-tree plugin :) we could open a separate issue there.

[msau42]

I was referring to csi driver:

exit status 1

[davidz627]

/help
another solution could be to expose the actual stderr from the mount call as @msau42 pointed out is exit status 1 right now. This stderr may contain information about not being able to format disk because of readonly device [unconfirmed].

[k8s-ci-robot]

@davidz627:
This request has been marked as needing help from a contributor.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

/help
another solution could be to expose the actual stderr from the mount call as @msau42 pointed out is exit status 1 right now. This stderr may contain information about not being able to format disk because of readonly device [unconfirmed].

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[davidz627]

/remove-lifecycle stale

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[davidz627]

/remove-lifecycle stale
fairly low priority

[davidz627]

kubernetes/utils#134

adds the detailed error output that contains the "cant format readonly disk" message.
Need to update go mod version to new utils after merge

[davidz627]

/assign