Add docs for preventing volume mode conversion #32673

RaunakShah · 2022-03-31T04:54:15Z

Documentation changes for feature to prevent unauthorised volume mode conversion

KEP - https://github.com/kubernetes/enhancements/tree/master/keps/sig-storage/3141-prevent-volume-mode-conversion

RaunakShah · 2022-03-31T04:54:22Z

cc @xing-yang

netlify · 2022-03-31T05:15:35Z

✅ Deploy Preview for kubernetes-io-main-staging ready!

Name	Link
🔨 Latest commit	`dc09063`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/62568a4f8633c50009e8c3e9
😎 Deploy Preview	https://deploy-preview-32673--kubernetes-io-main-staging.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

xing-yang · 2022-03-31T11:45:48Z

content/en/docs/concepts/storage/volume-snapshots.md

@@ -120,6 +120,7 @@ spec:
  driver: hostpath.csi.k8s.io
  source:
    volumeHandle: ee0cfb94-f8d4-11e9-b2d8-0242ac110002
+  sourceVolumeMode: Filesystem


It's possible that a user may be reading this doc but is using an older version of CRDs and controllers. So we need to add notes right next to this field to clarify how it can be used.

@RaunakShah : This conversation was marked as resolved and I did not see @xing-yang 's comments actually addressed. Could you clarify if this was agreed upon to not be added eventually, please?

Wasn't it agreed that users need not be developers and therefore introducing version specific information was too much effort for them? I added a note in Converting the volume mode of a Snapshot that describes how a user can determine if their API supports this field.

mehabhalodiya · 2022-04-05T16:52:30Z

/assign @didicodes
/sig storage

sftim

This PR also needs to register the snapshot.storage.kubernetes.io/allowVolumeModeChange annotation, by updating https://kubernetes.io/docs/reference/labels-annotations-taints/

Please add that detail. Also, I spotted an incorrect annotation value.

content/en/docs/concepts/storage/volume-snapshots.md

nate-double-u · 2022-04-11T23:44:22Z

Hello, this is a friendly reminder from the 1.24 docs release team that the Docs' complete deadline — All PRs reviewed and ready to merge — is EOD Tuesday, April 12th, 2022. Please finish up any remaining technical reviews and edits, and reach out if you need any help.

content/en/docs/concepts/storage/volume-snapshots.md

xing-yang · 2022-04-12T14:03:51Z

/lgtm

Review was stale

sftim

This PR needs to register the snapshot.storage.kubernetes.io/allowVolumeModeChange annotation, by updating https://kubernetes.io/docs/reference/labels-annotations-taints/

sftim · 2022-04-12T17:38:22Z

The PR mentions an unregistered annotation. We will note the tech LGTM for further reviews.
/lgtm cancel

content/en/docs/concepts/storage/volume-snapshots.md

xing-yang · 2022-04-19T20:29:54Z

/lgtm

k8s-ci-robot · 2022-04-19T20:30:12Z

LGTM label has been added.

Git tree hash: 7e7cef152482c526cf4b7faa5c653f0df604adf1

RaunakShah · 2022-04-20T07:49:53Z

Please see feedback / query in #32673 (comment)

@sftim @divya-mohan0209 I've removed all references to v6.0.0 in the latest commit and used the suggestion by @xing-yang in this comment - #32673 (comment)

Can you review this again?

divya-mohan0209

Added some feedback and unresolved some comments since I did not find them addressed. PTAL and let us know, if any queries.

divya-mohan0209 · 2022-04-22T05:12:20Z

content/en/docs/reference/labels-annotations-taints/_index.md

+Used on: VolumeSnapshotContent
+
+Value can either be `true` or `false`.
+This determines whether a user can modify the mode of the source volume when a `PVC` is being created from a `VolumeSnapshot`.


@RaunakShah : Please can you have a look at this suggestion? It doesn't seem to have been addressed and we think it would add value.

divya-mohan0209 · 2022-04-22T05:15:47Z

content/en/docs/concepts/storage/volume-snapshots.md

@@ -120,6 +120,7 @@ spec:
  driver: hostpath.csi.k8s.io
  source:
    volumeHandle: ee0cfb94-f8d4-11e9-b2d8-0242ac110002
+  sourceVolumeMode: Filesystem


@RaunakShah : This conversation was marked as resolved and I did not see @xing-yang 's comments actually addressed. Could you clarify if this was agreed upon to not be added eventually, please?

divya-mohan0209 · 2022-04-22T05:16:43Z

content/en/docs/concepts/storage/volume-snapshots.md

  volumeSnapshotRef:
    name: new-snapshot-test
    namespace: default
 ```

 `snapshotHandle` is the unique identifier of the volume snapshot created on the storage backend. This field is required for the pre-provisioned snapshots. It specifies the CSI snapshot id on the storage system that this `VolumeSnapshotContent` represents.
+`sourceVolumeMode` is the mode of the volume whose snapshot is taken. The value of the `sourceVolumeMode` field can be either `Filesystem` or `Block`. If the source volume mode is not specified, Kubernetes treats the snapshot as if the source volume's mode is unknown. Support for this field is available in VolumeSnapshot client v6.0.0 and higher.
+


I don't see this addressed either, ptal.

divya-mohan0209 · 2022-04-22T05:20:57Z

content/en/docs/reference/labels-annotations-taints/_index.md

@@ -493,6 +493,18 @@ you through the steps you follow to apply a seccomp profile to a Pod or to one o
 its containers. That tutorial covers the supported mechanism for configuring seccomp in Kubernetes,
 based on setting `securityContext` within the Pod's `.spec`.

+### snapshot.storage.kubernetes.io/allowVolumeModeChange
+
+Example: `snapshot.storage.kubernetes.io/allowVolumeModeChange: true`


@RaunakShah : PTAL since this hasn't been addressed and we believe it improves readability of the doc.

divya-mohan0209 · 2022-04-22T05:23:21Z

content/en/docs/reference/labels-annotations-taints/_index.md

+This determines whether a user can modify the mode of the source volume when a `PVC` is being created from a `VolumeSnapshot`.
+
+See [Converting the volume mode of a Snapshot](/docs/concepts/storage/volume-snapshots/#convert-volume-mode) for more information.
+


@xing-yang : From a personal standpoint, I'd err on the side of too much information rather than too less.
In the spirit of what I just said, what we could potentially rewrite it as is suggested above.

divya-mohan0209 · 2022-04-22T05:25:24Z

content/en/docs/reference/labels-annotations-taints/_index.md

+Value can either be `true` or `false`.
+This determines whether a user can modify the mode of the source volume when a `PVC` is being created from a `VolumeSnapshot`.
+
+See [Converting the volume mode of a Snapshot](/docs/concepts/storage/volume-snapshots/#convert-volume-mode) for more information.


Suggested change

See [Converting the volume mode of a Snapshot](/docs/concepts/storage/volume-snapshots/#convert-volume-mode) for more information.

Refer to the documentation on [Converting the volume mode of a Snapshot](/docs/concepts/storage/volume-snapshots/#convert-volume-mode) and the [Kubernetes CSI Developer Documentation](https://kubernetes-csi.github.io/docs/) for more information.

Suggested change

See [Converting the volume mode of a Snapshot](/docs/concepts/storage/volume-snapshots/#convert-volume-mode) for more information.

See [Converting the volume mode of a Snapshot](/docs/concepts/storage/volume-snapshots/#convert-volume-mode) for more information.

divya-mohan0209 · 2022-04-22T05:25:24Z

content/en/docs/reference/labels-annotations-taints/_index.md

+Value can either be `true` or `false`.
+This determines whether a user can modify the mode of the source volume when a `PVC` is being created from a `VolumeSnapshot`.
+
+See [Converting the volume mode of a Snapshot](/docs/concepts/storage/volume-snapshots/#convert-volume-mode) for more information.


Suggested change

See [Converting the volume mode of a Snapshot](/docs/concepts/storage/volume-snapshots/#convert-volume-mode) for more information.

Refer to [Converting the volume mode of a Snapshot](/docs/concepts/storage/volume-snapshots/#convert-volume-mode) and the [Kubernetes CSI Developer Documentation](https://kubernetes-csi.github.io/docs/) for more information.

sftim · 2022-04-22T10:08:49Z

content/en/docs/concepts/storage/volume-snapshots.md

+```yaml
+$ kubectl get crd volumesnapshotcontent -o yaml
+```


this isn't YAML

don't include the command prompt

sftim · 2022-04-22T10:09:36Z

content/en/docs/concepts/storage/volume-snapshots.md

+```yaml
+$ kubectl get crd volumesnapshotcontent -o yaml
+```
+


After running kubectl get crd volumesnapshotcontent, does a success exit indicate support for the sourceVolumeMode field? If not, we should document how to actually check.

nate-double-u · 2022-04-22T18:05:33Z

Hi Everyone!

I appreciate the effort going into this, but this PR has a little less than a week to be resolved and merged into dev-1.24; everything needs to be complete and merged in by next Thursday (April 28, 2022) as we will be freezing the k/website repo on the following Monday.

/cc @kubernetes/sig-docs-leads @JamesLaverack @xing-yang

Review is now stale (feedback still applies)

sftim · 2022-04-23T11:08:39Z

As this is an alpha feature, perhaps we can update the release notes with a comment to make it clear that the docs for it are not finished, ship what we have, and then make sure that we don't graduate the feature to beta until they're improved?

divya-mohan0209 · 2022-04-23T12:37:19Z

I'm normally not on the side of holding back a feature because of a doc. However I do feel like this doc needs a lot of work and possibly, a rewrite. So +1 to the suggestion by @sftim if the feature owner/release lead are okay with this suggestion.

nate-double-u · 2022-04-25T16:55:41Z

+1 to @sftim's suggestion. I'm happy to approve this as is if either @sftim or @divya-mohan0209 give this an /lgtm and @AuraSinis (the v1.24 release notes lead) agrees we can put this into the release notes.

@AuraSinis, could you provide next steps on adding an update to the release notes?

edit: at this point, it think @AuraSinis agreeing would be enough to merge this in. I'd also like to see an issue opened as a followup, but that can be done later.

AuraSinis · 2022-04-25T17:20:10Z

If no action is required I can just put in a note about docs still in-situ manually, easy-peasy.

nate-double-u · 2022-04-25T17:21:42Z

Thanks @AuraSinis!
@RaunakShah, @xing-yang, do you agree with this path?

xing-yang · 2022-04-26T12:26:55Z

@nate-double-u That's fine with me.

RaunakShah · 2022-04-26T16:07:43Z

@nate-double-u this works for me too

nate-double-u · 2022-04-26T17:38:00Z

Thanks everyone, I appreciate all the effort you've put in!

/lgtm
/approve

k8s-ci-robot · 2022-04-26T17:38:19Z

LGTM label has been added.

Git tree hash: a12fdd6736f73b4a9e164e03951068d5efe965e0

k8s-ci-robot · 2022-04-26T17:38:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nate-double-u

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:

~~content/en/OWNERS~~ [nate-double-u]

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

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 31, 2022

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 31, 2022

k8s-ci-robot requested a review from saad-ali March 31, 2022 04:54

k8s-ci-robot added the language/en Issues or PRs related to English language label Mar 31, 2022

k8s-ci-robot requested a review from thockin March 31, 2022 04:54

k8s-ci-robot added the sig/docs Categorizes an issue or PR as relevant to SIG Docs. label Mar 31, 2022

xing-yang reviewed Mar 31, 2022

View reviewed changes

xing-yang mentioned this pull request Apr 5, 2022

Prevent unauthorised volume mode conversion during volume restore kubernetes/enhancements#3141

Closed

10 tasks

k8s-ci-robot assigned didicodes Apr 5, 2022

k8s-ci-robot added the sig/storage Categorizes an issue or PR as relevant to SIG Storage. label Apr 5, 2022

RaunakShah changed the title ~~[WIP] Add docs for preventing volume mode conversion~~ Add docs for preventing volume mode conversion Apr 5, 2022

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 5, 2022

sftim previously requested changes Apr 6, 2022

View reviewed changes

content/en/docs/concepts/storage/volume-snapshots.md Outdated Show resolved Hide resolved

content/en/docs/concepts/storage/volume-snapshots.md Outdated Show resolved Hide resolved

xing-yang reviewed Apr 12, 2022

View reviewed changes

content/en/docs/concepts/storage/volume-snapshots.md Outdated Show resolved Hide resolved

k8s-ci-robot assigned xing-yang Apr 12, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 12, 2022

sftim previously requested changes Apr 12, 2022

View reviewed changes

k8s-ci-robot assigned sftim Apr 12, 2022

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 12, 2022

k8s-ci-robot requested review from didicodes and xing-yang April 12, 2022 17:38

sftim reviewed Apr 12, 2022

View reviewed changes

content/en/docs/concepts/storage/volume-snapshots.md Outdated Show resolved Hide resolved

RaunakShah added 2 commits April 20, 2022 01:41

Remove v6 reference

66a1911

Remove v6 reference

02c43d2

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 19, 2022

divya-mohan0209 requested changes Apr 22, 2022

View reviewed changes

Some formatting changes

b656cd8

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 22, 2022

k8s-ci-robot requested review from sftim and xing-yang April 22, 2022 10:01

sftim reviewed Apr 22, 2022

View reviewed changes

sftim previously requested changes Apr 22, 2022

View reviewed changes

k8s-ci-robot requested a review from JamesLaverack April 22, 2022 18:05

k8s-ci-robot assigned nate-double-u Apr 26, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 26, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 26, 2022

k8s-ci-robot merged commit 212a936 into kubernetes:dev-1.24 Apr 26, 2022

		This determines whether a user can modify the mode of the source volume when a `PVC` is being created from a `VolumeSnapshot`.

		See [Converting the volume mode of a Snapshot](/docs/concepts/storage/volume-snapshots/#convert-volume-mode) for more information.

	See [Converting the volume mode of a Snapshot](/docs/concepts/storage/volume-snapshots/#convert-volume-mode) for more information.
	Refer to the documentation on [Converting the volume mode of a Snapshot](/docs/concepts/storage/volume-snapshots/#convert-volume-mode) and the [Kubernetes CSI Developer Documentation](https://kubernetes-csi.github.io/docs/) for more information.

	See [Converting the volume mode of a Snapshot](/docs/concepts/storage/volume-snapshots/#convert-volume-mode) for more information.
	Refer to [Converting the volume mode of a Snapshot](/docs/concepts/storage/volume-snapshots/#convert-volume-mode) and the [Kubernetes CSI Developer Documentation](https://kubernetes-csi.github.io/docs/) for more information.

Add docs for preventing volume mode conversion #32673

Add docs for preventing volume mode conversion #32673

Conversation

RaunakShah commented Mar 31, 2022

RaunakShah commented Mar 31, 2022

netlify bot commented Mar 31, 2022 • edited Loading

✅ Deploy Preview for kubernetes-io-main-staging ready!

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

mehabhalodiya commented Apr 5, 2022

sftim left a comment

Choose a reason for hiding this comment

nate-double-u commented Apr 11, 2022 • edited Loading

xing-yang commented Apr 12, 2022

sftim left a comment

Choose a reason for hiding this comment

sftim commented Apr 12, 2022

xing-yang commented Apr 19, 2022

k8s-ci-robot commented Apr 19, 2022

RaunakShah commented Apr 20, 2022 • edited Loading

divya-mohan0209 left a 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

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

sftim Apr 22, 2022 • edited Loading

Choose a reason for hiding this comment

nate-double-u commented Apr 22, 2022

sftim commented Apr 23, 2022

divya-mohan0209 commented Apr 23, 2022

nate-double-u commented Apr 25, 2022 • edited Loading

AuraSinis commented Apr 25, 2022

nate-double-u commented Apr 25, 2022 • edited Loading

xing-yang commented Apr 26, 2022

RaunakShah commented Apr 26, 2022

nate-double-u commented Apr 26, 2022

k8s-ci-robot commented Apr 26, 2022

k8s-ci-robot commented Apr 26, 2022

netlify bot commented Mar 31, 2022 •

edited

Loading

nate-double-u commented Apr 11, 2022 •

edited

Loading

RaunakShah commented Apr 20, 2022 •

edited

Loading

sftim Apr 22, 2022 •

edited

Loading

nate-double-u commented Apr 25, 2022 •

edited

Loading

nate-double-u commented Apr 25, 2022 •

edited

Loading