Set READONLY flag in CSI PV based on PVC accessmode #469

humblec · 2020-08-21T13:07:26Z

The provisioner sidecar now has an argument called `controller-publish-readonly` which sets the value of CSI PV spec `readonly` field value based on the PVC access mode. If this flag is set to `true` and the PVC access mode only contains the `ROX` access mode, the controller automatically sets `PersistentVolume.spec.CSIPersistentVolumeSource.readOnly` field to `true`.

Signed-off-by: Humble Chirammal hchiramm@redhat.com

humblec · 2020-08-21T13:28:38Z

@msau42 this PR try to address the volume readonly issue or AI item mentioned in sig call. As mentioned in one of the sig call I still have doubt on :kubernetes/kubernetes#70505 (comment). Appreciated if you can share your thoughts on this PR and the mentioned comment.

humblec · 2020-08-21T14:40:17Z

/assign @msau42
/assign @jsafrane

humblec · 2020-08-21T17:15:40Z

/release-note-none

humblec · 2020-08-24T13:26:49Z

/assign @xing-yang

gnufied · 2020-08-24T14:11:17Z

pkg/controller/controller.go

+			pvReadOnly = true
+		}
+	}
+


I am not sure if presence of ROX accessMode means volume should be controller published as readonly. To me, these can be orthogonal concerns. cc @bswartz

true.. but regardless of how controller publish, the PV source could still set it RO .

yes but interpreting ROX as readonly in PV source should be done with caution. It could make the PV unmountable in case journal needs to be replayed or something else.

Yes I was going to argue the same. Volumes should not be controller-published as read-only unless the PV is marked read-only. There are too many good reasons for publishing them writable to the node and then having the node re-mount them as read-only to the pod.

Journal-replays are one use case. Pending expand operations are another.

I interpret PVs with the readOnly: true flag to be PERMANENTLY read-only, which you almost never want in practice. We might want to make it easier to cause that PV flag to get set to true in some cases, but I think the decision for ControllerPublishVolume should look at that flag only and not look at access modes.

I agree the FS operations may need it RW while controller publish. But SP driver is free to do the operation or default to RW while controller publishing or node staging. Isnt it ?

I interpret PVs with the readOnly: true flag to be PERMANENTLY read-only, which you almost never want in practice.

I could think of a scenario where user provision a new volume as "RO" with dataSource set to PVC or Snapshot. Isnt it a valid use case where user want a PV to be READONLY?

@saad-ali @msau42 @jsafrane @xing-yang inviting your thoughts .

the pod tries to come up, the mount fails because the kernel is unable to replay the journal on a readonly device

I'd like to question this part. I expect filesystems to be prepared for this case. Consider a normal device that degrades such that writes start to fail. The normal reaction of the kernel is to remount the filesystem as read-only. Being able to mount read-only after a system reboot to extract more data seems like a sensible feature to support - much better than refusing to mount completely.

Whether data and files are consistent is another question, but it's not different from protecting against a sudden power loss.

First of all, I want to be clear that I'm not trying to block this issue. I think it's a good change, and I'd like to see it merge.

I was just trying to answer the question of what potential downsides this change could have, because it does introduce this new corner case with journaled filesystems. We shouldn't be surprised to see support cases from people with problems mounting readonly volumes that were cloned from in-use filesystems or snapshots taken of in-use filesystems, if journaling is in use.

There are some workarounds we can either recommend to users, or possibly even build into our CSI drivers to automatically overcome this class of problems. The most obvious workaround is to create a volume that's writable and use that instead. Or, create a writable volume, attach that to a pod that does nothing (to let the journal replay), then clone that volume after the pod has terminated cleanly (so the journal is empty).

The specific kernel error I see when I reproduce this problem with ext4 is:
[587843.214656] EXT4-fs (nbd2): INFO: recovery required on readonly filesystem
[587843.214657] EXT4-fs (nbd2): write access unavailable, cannot proceed (try mounting with noload)

Possibly CSI drivers could use the -o noload option with ext4 filesystems when mounting readonly to avoid the problem.

When using XFS, the kernel errors look like this:
[588571.823845] XFS (nbd1): Mounting V5 Filesystem
[588571.881098] XFS (nbd1): recovery required on read-only device.
[588571.881100] XFS (nbd1): write access unavailable, cannot proceed.
[588571.881101] XFS (nbd1): log mount/recovery failed: error -30
[588571.881123] XFS (nbd1): log mount failed

Some searching turns up the -o norecovery options for XFS which might also work.

In summary, I like this change but I'd urge that CSI driver authors consider special handling for mounting of volumes published read-only at the controller level. And if users do get stuck with unmountable volumes, be ready to suggest the workaround of using a writable volume.

@bswartz @pohly thanks for the inputs.. First of all good discussion about the concerned area.

Create an writable volume, with an ext4 (or other journaling) filesystem
Pod attaches to the volume, and begins writing data
Node containing that pod crashes, loses power, or otherwise dies suddenly
Take a snapshot of the volume before it is attached to another pod
....

Let me share some thoughts here: In general, a storage vendor does seizing of in flight i/os and also put write barriers in place to make sure the data is persisted correctly while snapshot is taken. Thats one of the main requirement of consistent snapshots. Thus, in above scenario, it would be ideally achieved by the SP. From CSI layer, I assume it should not be of much concern. iow, a snapshot taken on an in used volume or snapshot taken after a crash or node/pod loss comes pretty much to the same situation and SP or any layer responsible at storage low level should take care of it. While it comes to controller publish/node stage, again, decision can be made by SP to node stage or controller publish either by respecting the pv spec or not.
Regardless, the mention about the possibilities of a 'unclean journal' situation and a workaround/solution is indeed a good point 👍 . I am not sure about the different mount options of ext4 or xfs mentioned, have to look into the details of these.
About the direction of this PR and its completion, I feel its good wait some more input from storage sig.

Probably the best thing to do before merging this fix would be to add documentation to the CSI spec around the PUBLISH_READONLY controller capability. Drivers that assert that capability will need to be prepared to mount unmodifiable volumes on the nodes, which means understanding how to deal with unclean journals of each of the fstypes that your driver supports.

While we wait for others to share their thoughts, let me rebase the PR and keep it updated with latest code.

humblec · 2020-09-04T13:53:50Z

@msau42 @xing-yang @saad-ali @jsafrane ptal 👍

xing-yang · 2020-09-04T15:17:52Z

@humblec In the proposal described in kubernetes/kubernetes#70505, there is also the following. I don't see that is addressed by this PR?

Add new csiControllerPublishVolumeReadOnly StorageClass parameter to CSI external-provisioner

humblec · 2020-09-04T16:35:11Z

@humblec In the proposal described in kubernetes/kubernetes#70505, there is also the following. I don't see that is addressed by this PR?
Add new csiControllerPublishVolumeReadOnly StorageClass parameter to CSI external-provisioner

@xing-yang I was in an impression that, the SC param is an optional one. Or is it mandatory that, we will expose the SC param too ?

xing-yang · 2020-10-22T15:10:04Z

@humblec In the proposal described in kubernetes/kubernetes#70505, there is also the following. I don't see that is addressed by this PR?

Add new csiControllerPublishVolumeReadOnly StorageClass parameter to CSI external-provisioner

@xing-yang I was in an impression that, the SC param is an optional one. Or is it mandatory that, we will expose the SC param too ?

@humblec based on kubernetes/kubernetes#70505, "Add new csiControllerPublishVolumeReadOnly StorageClass parameter to CSI external-provisioner" should be addressed as well.

@saad-ali can you chime in as you wrote the original proposal.

humblec · 2020-10-23T10:43:49Z

@humblec In the proposal described in kubernetes/kubernetes#70505, there is also the following. I don't see that is addressed by this PR?

Add new csiControllerPublishVolumeReadOnly StorageClass parameter to CSI external-provisioner

@xing-yang I was in an impression that, the SC param is an optional one. Or is it mandatory that, we will expose the SC param too ?

@humblec based on kubernetes/kubernetes#70505, "Add new csiControllerPublishVolumeReadOnly StorageClass parameter to CSI external-provisioner" should be addressed as well.

Somehow I was in an impression that its either 1 or 2 in that proposal :), so I was trying clarify. I will wait for @saad-ali thought to double confirm.

@saad-ali can you chime in as you wrote the original proposal.

humblec · 2021-07-13T15:02:09Z

[Changes in the last refresh compared to previous version]

new flag has been introduced in the provisioner sidecar
Removed SC param setting which was the initial workflow based on the referenced issue
Test cases have been added for provisioner sidecar flag to be true and false against MULTI_NODE_MULTI_READER capability

PTAL.. Thanks 👍

msau42 · 2021-08-04T18:36:24Z

cmd/csi-provisioner/csi-provisioner.go

@@ -103,17 +103,13 @@ var (
 	nodeDeploymentImmediateBinding = flag.Bool("node-deployment-immediate-binding", true, "Determines whether immediate binding is supported when deployed on each node.")
 	nodeDeploymentBaseDelay        = flag.Duration("node-deployment-base-delay", 20*time.Second, "Determines how long the external-provisioner sleeps initially before trying to own a PVC with immediate binding.")
 	nodeDeploymentMaxDelay         = flag.Duration("node-deployment-max-delay", 60*time.Second, "Determines how long the external-provisioner sleeps at most before trying to own a PVC with immediate binding.")
+	publishROXVol                  = flag.Bool("publish-rox-vol", false, "This option enables PV to be marked as readonly at nodepublish call if PVC accessmode has been set to ROX.")


From the analysis at kubernetes/kubernetes#70505, this impacts ControllerPublish, not NodePublish

sure.. updated , thanks @msau42 . 👍

leaderelection interface has been defined which is not in use at the moment. This commit remove the same. Signed-off-by: Humble Chirammal <hchiramm@redhat.com>

k8s-ci-robot · 2021-08-06T10:55:42Z

@humblec: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Rerun command
pull-kubernetes-csi-external-provisioner-distributed-on-kubernetes-1-20	`0f89907`	link	`/test pull-kubernetes-csi-external-provisioner-distributed-on-kubernetes-1-20`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

msau42 · 2021-08-09T23:42:14Z

cmd/csi-provisioner/csi-provisioner.go

@@ -103,17 +103,13 @@ var (
 	nodeDeploymentImmediateBinding = flag.Bool("node-deployment-immediate-binding", true, "Determines whether immediate binding is supported when deployed on each node.")
 	nodeDeploymentBaseDelay        = flag.Duration("node-deployment-base-delay", 20*time.Second, "Determines how long the external-provisioner sleeps initially before trying to own a PVC with immediate binding.")
 	nodeDeploymentMaxDelay         = flag.Duration("node-deployment-max-delay", 60*time.Second, "Determines how long the external-provisioner sleeps at most before trying to own a PVC with immediate binding.")
+	publishROXVol                  = flag.Bool("publish-rox-vol", false, "This option enables PV to be marked as readonly at controllerpublish call if PVC accessmode has been set to ROX.")


What do you think of calling the flag something like "controller-publish-readonly".

Also in the help text, use the full csi call name "ControllerPublishVolume"

Can you also update the README?

@msau42 yeah, thats a good suggestion, considering we are on controller publish, controller-publish-readonly suite better. Let me update the flag and README.

@msau42 done.. thanks

msau42 · 2021-08-09T23:44:13Z

pkg/controller/controller_test.go

@@ -417,7 +417,7 @@ func TestCreateDriverReturnsInvalidCapacityDuringProvision(t *testing.T) {

 	pluginCaps, controllerCaps := provisionCapabilities()
 	csiProvisioner := NewCSIProvisioner(clientSet, 5*time.Second, "test-provisioner", "test",
-		5, csiConn.conn, nil, driverName, pluginCaps, controllerCaps, "", false, true, csitrans.New(), nil, nil, nil, nil, nil, false, defaultfsType, nil)
+		5, csiConn.conn, nil, driverName, pluginCaps, controllerCaps, "", false, true, csitrans.New(), nil, nil, nil, nil, nil, false, defaultfsType, nil, true)


We should investigate this separately, but I think the number of function arguments is getting to the point that we should probably have an options struct.

completely agree here, the arg has grown a lot and even the test case organization is difficult in general with current state. I will revisit this part in a followup PR. thanks.

based on newly added "controller-publish-read-only" and PVC accessmode setting `ReadOnly` field in CSI persistent volume source has been set to `true`. Signed-off-by: Humble Chirammal <hchiramm@redhat.com>

jsafrane · 2021-08-10T08:54:26Z

@humblec, please update release-note section of the first comment of this PR to reflect the latest changes.

humblec · 2021-08-10T09:00:30Z

@humblec, please update release-note section of the first comment of this PR to reflect the latest changes.

Sure @jsafrane

humblec · 2021-08-10T09:11:45Z

@humblec, please update release-note section of the first comment of this PR to reflect the latest changes.

Sure @jsafrane .

Now the release note has been updated, ptal.. Thanks @jsafrane

msau42 · 2021-08-10T17:03:59Z

/lgtm
/approve

k8s-ci-robot · 2021-08-10T17:04:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: humblec, msau42, saad-ali

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [msau42,saad-ali]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

chrishenzie · 2021-08-10T18:01:58Z

Adding a hold to give others an opportunity to review.

Removing now that PR is LGTM'd and approved.

/hold cancel

chrishenzie · 2021-08-10T18:16:03Z

/kind feature

Release tools update

k8s-ci-robot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Aug 21, 2020

k8s-ci-robot requested review from lpabon and msau42 August 21, 2020 13:07

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 21, 2020

humblec force-pushed the some-fox branch from 6a9ae2b to 31e668c Compare August 21, 2020 13:14

humblec force-pushed the some-fox branch from 31e668c to 18503b6 Compare August 21, 2020 13:36

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 21, 2020

humblec force-pushed the some-fox branch 2 times, most recently from 232c23e to b653dc2 Compare August 21, 2020 14:15

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 21, 2020

humblec force-pushed the some-fox branch from b653dc2 to 6dca2e0 Compare August 21, 2020 14:21

k8s-ci-robot assigned jsafrane and msau42 Aug 21, 2020

humblec force-pushed the some-fox branch from 6dca2e0 to 87fdbf5 Compare August 21, 2020 15:07

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Aug 21, 2020

k8s-ci-robot assigned xing-yang Aug 24, 2020

gnufied reviewed Aug 24, 2020

View reviewed changes

humblec force-pushed the some-fox branch from 754885e to 2dda6e4 Compare July 13, 2021 14:38

msau42 reviewed Aug 4, 2021

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 5, 2021

humblec force-pushed the some-fox branch from 2dda6e4 to ad43b5d Compare August 6, 2021 10:36

remove dangling leaderelection interface from provisioner

7a44dc6

leaderelection interface has been defined which is not in use at the moment. This commit remove the same. Signed-off-by: Humble Chirammal <hchiramm@redhat.com>

humblec force-pushed the some-fox branch from ad43b5d to a11e09f Compare August 6, 2021 10:49

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 6, 2021

humblec force-pushed the some-fox branch from a11e09f to bf16e67 Compare August 6, 2021 10:59

msau42 reviewed Aug 9, 2021

View reviewed changes

Add ROX capability based on provisioner arg and ROX accessmode

8d6b0d8

based on newly added "controller-publish-read-only" and PVC accessmode setting `ReadOnly` field in CSI persistent volume source has been set to `true`. Signed-off-by: Humble Chirammal <hchiramm@redhat.com>

humblec force-pushed the some-fox branch from bf16e67 to 8d6b0d8 Compare August 10, 2021 08:24

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Aug 10, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 10, 2021

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 10, 2021

k8s-ci-robot merged commit c0d7087 into kubernetes-csi:master Aug 10, 2021

k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Aug 10, 2021

msau42 mentioned this pull request Aug 11, 2021

Add changelog for v3.0.0 #665

Merged

msau42 moved this from To do to Done in K8s 1.22 Sep 28, 2021

tsmetana mentioned this pull request Nov 30, 2021

Can't use ReadOnlyMany volume restored from Snapshot with controller-publish-readonly provisioner option kubernetes-sigs/gcp-compute-persistent-disk-csi-driver#872

Closed

kbsonlong pushed a commit to kbsonlong/external-provisioner that referenced this pull request Dec 29, 2023

Merge pull request kubernetes-csi#469 from humblec/release-tools-update

5b3c736

Release tools update

Set READONLY flag in CSI PV based on PVC accessmode #469

Set READONLY flag in CSI PV based on PVC accessmode #469

Conversation

humblec commented Aug 21, 2020 • edited by chrishenzie

humblec commented Aug 21, 2020 • edited

humblec commented Aug 21, 2020

humblec commented Aug 21, 2020

humblec commented Aug 24, 2020

gnufied Aug 24, 2020 • edited

Choose a reason for hiding this comment

humblec Aug 25, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humblec Apr 28, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humblec commented Sep 4, 2020

xing-yang commented Sep 4, 2020

humblec commented Sep 4, 2020

xing-yang commented Oct 22, 2020

humblec commented Oct 23, 2020

humblec commented Jul 13, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 6, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsafrane commented Aug 10, 2021

humblec commented Aug 10, 2021

humblec commented Aug 10, 2021

msau42 commented Aug 10, 2021

k8s-ci-robot commented Aug 10, 2021

chrishenzie commented Aug 10, 2021

chrishenzie commented Aug 10, 2021

humblec commented Aug 21, 2020 •

edited by chrishenzie

humblec commented Aug 21, 2020 •

edited

gnufied Aug 24, 2020 •

edited

humblec Aug 25, 2020 •

edited

humblec Apr 28, 2021 •

edited

humblec commented Jul 13, 2021 •

edited

k8s-ci-robot commented Aug 6, 2021 •

edited