doc: add recommendation for nfs in external cluster #13876

parth-gr · 2024-03-05T13:49:31Z

with recent discussion we come to conclusion
nfs can be suported easily in the external cluster so adding the design that we discussed

closes: #13277

Checklist:

Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide.
Reviewed the developer guide on Submitting a Pull Request
Pending release notes updated with breaking and/or notable changes for the next minor release.
Documentation has been updated, if necessary.
Unit tests have been added, if necessary.
Integration tests have been added, if necessary.

travisn · 2024-03-07T19:57:25Z

Documentation/CRDs/Cluster/external-cluster.md

@@ -214,6 +214,46 @@ Create the object store resources:
 If encryption or compression on the wire is needed, specify the `--v2-port-enable` flag.
 If the v2 address type is present in the `ceph quorum_status`, then the output of 'ceph mon data' i.e, `ROOK_EXTERNAL_CEPH_MON_DATA` will use the v2 port(`3300`).

+### NFS storage


I wonder if we should include a section on external ceph in the main nfs.md doc, then include a link to that section from this doc. The export creation seems like a duplicate from that doc as well. @BlaineEXE thoughts?

That makes sense to me, let's wait for Blaine suggestion

Oops. Yes, I would prefer to document this in the NFS section and link to it from here.

github-actions · 2024-04-08T20:01:49Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in two weeks if no further activity occurs. Thank you for your contributions.

mergify · 2024-04-08T20:02:37Z

This pull request has merge conflicts that must be resolved before it can be merged. @parth-gr please rebase it. https://rook.io/docs/rook/latest/Contributing/development-flow/#updating-your-fork

Documentation/CRDs/ceph-nfs-crd.md

travisn · 2024-04-23T17:26:06Z

Documentation/CRDs/Cluster/external-cluster/external-cluster.md

@@ -253,6 +253,10 @@ Consume the S3 Storage, in two different ways:
 If encryption or compression on the wire is needed, specify the `--v2-port-enable` flag.
 If the v2 address type is present in the `ceph quorum_status`, then the output of 'ceph mon data' i.e, `ROOK_EXTERNAL_CEPH_MON_DATA` will use the v2 port(`3300`).

+### NFS storage
+
+Make use of the [NFS client running on the external ceph standalone cluster](../../ceph-nfs-crd.md).


Suggested change

Make use of the [NFS client running on the external ceph standalone cluster](../../ceph-nfs-crd.md).

Make use of the [NFS client running on the external ceph standalone cluster](../../ceph-nfs-crd.md#external-clusters).

travisn · 2024-04-23T17:27:24Z

Documentation/CRDs/ceph-nfs-crd.md

+ceph nfs export create cephfs <nfs-client-name> /test <filesystem-name>
+```
+
+Create the PV,


nit: Instead of a comma, use a colon when followed by a snippet.

Suggested change

Create the PV,

Create the PV:

travisn · 2024-04-23T17:30:26Z

Documentation/CRDs/ceph-nfs-crd.md

+
+## External Clusters
+
+Make use of the NFS client running on the external ceph standalone cluster.


What is specific to external clusters for NFS? I guess for internal mode, the client will be external to the cluster so we don't have an example of PVC/PV in that case. So in this case, the Rook cluster is the one consuming the NFS with a PVC/PV. Maybe a bit more explanation in this paragraph would be helpful for the user.

Rook cluster is the one consuming the NFS

Rook is not consuming it, they're directly exposing the external nfs client to the kubernetes application.

Documentation/CRDs/Cluster/external-cluster/external-cluster.md

travisn

Just one more nit, and I'll wait to approve until @BlaineEXE can review

Documentation/CRDs/ceph-nfs-crd.md

Madhu-1 · 2024-04-25T06:05:05Z

Documentation/CRDs/ceph-nfs-crd.md

+
+## External Clusters
+
+User Applications can make use of the NFS client running on the external ceph standalone cluster.


we already have NFS CSI driver why users need to create static pv/pvc to access it from intree code?

Yes we had two ways, the first one looked more direct.

So if the user already has the nfs exports, they can use this document to create the pv and pvcs https://docs.openshift.com/container-platform/4.14/storage/persistent_storage/persistent-storage-nfs.html
Moreover if they don't have the exports created they can run the simple ceph cmd
ceph nfs export create cephfs my-nfs /test myfs

Secondly we can integrate this feature similar way we have other addons,
Steps would be creating a new nfs-storage class from the python script and then ceph-csi will take care of the export and create the pv for us. (and need update on the csi-users permissions)

NFS is not deprecated, but kubernetes already have the official doc for it https://github.com/kubernetes/examples/tree/master/staging/volumes/nfs
https://kubernetes.io/docs/concepts/storage/volumes/#nfs
why do we need to reimplement it?

That's correct I can add the link to that content, thanks!
These are the examples listed,
https://github.com/kubernetes/examples/tree/master/staging/volumes/nfs

Updated, how about now

with recent discussion we come to conclusion nfs can be suported easily in the external cluster so adding the design that we discussed closes: rook#13277 Signed-off-by: parth-gr <partharora1010@gmail.com>

parth-gr · 2024-04-29T16:13:37Z

@BlaineEXE ^^

BlaineEXE

In general, I think that it is good for Rook to provide a suggestion for users on how to consume the external NFS export. This is especially true because we recommend a workflow that is different from RBD/CephFS with CSI.

That said, I think there are changes we could make to help users understand more generally, and specifically what to do to consume from an external NFS.

BlaineEXE · 2024-04-30T18:48:39Z

Documentation/CRDs/Cluster/external-cluster/external-cluster.md

@@ -253,6 +253,10 @@ Consume the S3 Storage, in two different ways:
 If encryption or compression on the wire is needed, specify the `--v2-port-enable` flag.
 If the v2 address type is present in the `ceph quorum_status`, then the output of 'ceph mon data' i.e, `ROOK_EXTERNAL_CEPH_MON_DATA` will use the v2 port(`3300`).

+### NFS storage
+
+Make use of the [NFS client running on the external ceph standalone cluster](../../ceph-nfs-crd.md#external-clusters).


"client" is not quite correct here. the external cluster runs the server, but "service" is probably the best term since I think that's how cephadm/dashboard refers to it

Ceph should be capitalized in docs unless it is the ceph CLI command, which should be in code format as here

I am concerned that the phrasing "Make use of..." might lead some users to assume that this step is mandatory. Let's rephrase to clarify that it's only if users want to do so

Suggested change

Make use of the [NFS client running on the external ceph standalone cluster](../../ceph-nfs-crd.md#external-clusters).

Rook suggests a different mechanism for making use of an [NFS service running on the external Ceph standalone cluster](../../ceph-nfs-crd.md#external-clusters), if desired.

BlaineEXE · 2024-04-30T18:51:38Z

Documentation/CRDs/ceph-nfs-crd.md

@@ -204,3 +204,17 @@ the size of the cluster.
    ceph orch set backend ""
    ceph mgr module disable rook
    ```
+
+## External Clusters


This topic doesn't explicitly relate to the CephNFS CRD. It would be best to house this in the StorageConfiguration/NFS section. Probably the nfs.md file, at the end of the doc.

BlaineEXE · 2024-04-30T18:54:06Z

Documentation/CRDs/ceph-nfs-crd.md

@@ -204,3 +204,17 @@ the size of the cluster.
    ceph orch set backend ""
    ceph mgr module disable rook
    ```
+
+## External Clusters


I think this title can provide more clarity

Headings in this doc don't capitalize every word, only the first word and proper nouns, so "Clusters" should be "clusters"

Suggested change

## External Clusters

## Consuming NFS from an external source

I chose "source" here over "cluster" or "ceph cluster" because, in theory, the recommendations also apply when the external source isn't Ceph. Users can ignore the ceph-specific things if they want.

BlaineEXE · 2024-04-30T18:55:59Z

Documentation/CRDs/ceph-nfs-crd.md

+ceph nfs export create cephfs <nfs-client-name> /test <filesystem-name>
+```
+
+Create the [PV and PVC](https://github.com/kubernetes/examples/tree/master/staging/volumes/nfs) using `nfs-client-server-ip`. It will mount NFS volumes with PersistentVolumes and then mount the PVCs in the [user Pod Application](https://kubernetes.io/docs/concepts/storage/volumes/#nfs) to utilize the NFS type storage.


How do users know what the nfs-client-server-ip is? Where do they find that info?
Let's show this in the docs, or explain it.

Given that it's in code format, I assume that it's something from ceph CLI output?

BlaineEXE · 2024-04-30T18:59:35Z

Documentation/CRDs/ceph-nfs-crd.md

@@ -204,3 +204,17 @@ the size of the cluster.
    ceph orch set backend ""
    ceph mgr module disable rook
    ```
+
+## External Clusters
+


The doc is recommending that users choose a strategy for this that isn't specific to Rook. I think it would be helpful for us to preface this in case users are confused. Something like this summarizes it. I think that it is also generally helpful to begin major doc sections with a short summary of what the section will help users achieve. We can accomplish both here with something like this

For consuming NFS services and exports external to the Kubernetes cluster (including those backed by an external Ceph cluster), Rook recommends using Kubernetes' regular NFS consumption model.

BlaineEXE · 2024-04-30T19:00:17Z

Documentation/CRDs/ceph-nfs-crd.md

+
+## External Clusters
+
+User Applications can make use of the NFS client running on the external ceph standalone cluster.


Update wording to reflect my other "client", "Ceph" suggestions.

Also, "Applications" doesn't need to be capitalized.

BlaineEXE · 2024-04-30T19:03:34Z

Documentation/CRDs/ceph-nfs-crd.md

+Procedure:
+
+Export the nfs client to a particular cephfs filesystem:
+


I'm not sure how to best put this into words, but I have seen this pattern in our docs more than once, and I'm always a little confused by it.

Generally, we don't need to use a colon (:) at the end of a sentence unless it immediately precedes and introduces:

a code block

a list

a block quote

The above is a prime example.

In this case "Procedure:" seems like maybe it should just be a section header (which shouldn't end in a colon). Or maybe it can be omitted entirely since the following line does a better job of explaining the subsequent code block.

BlaineEXE · 2024-04-30T19:07:30Z

Documentation/CRDs/ceph-nfs-crd.md

+```yaml
+ceph nfs export create cephfs <nfs-client-name> /test <filesystem-name>
+```


To be honest, I don't think Rook should document this. Users should defer to Ceph docs for instructions on how to create exports.

Instead, Rook docs should explain how to use a Ceph export's configurations (maybe it's already existing, maybe users create it just for Rook, it doesn't matter) in the PV/PVC.

I think this command should more rightly be a ceph nfs export get <service> <export-name> command, and then we can show how to use the example output of that command to configure the PV/PVCs.

mergify · 2024-05-01T15:42:46Z

This pull request has merge conflicts that must be resolved before it can be merged. @parth-gr please rebase it. https://rook.io/docs/rook/latest/Contributing/development-flow/#updating-your-fork

BlaineEXE · 2024-05-01T23:21:32Z

Please update the commit title to match the recently updated PR title, or something similar. We are no longer adding a design doc.

parth-gr requested review from BlaineEXE and travisn March 5, 2024 13:49

travisn reviewed Mar 7, 2024

View reviewed changes

github-actions bot added the stale Labeled by the stale bot label Apr 8, 2024

github-actions bot removed the stale Labeled by the stale bot label Apr 10, 2024

parth-gr requested a review from travisn April 22, 2024 11:51

parth-gr force-pushed the external-nfs branch from 4ab5b31 to ae85439 Compare April 22, 2024 11:51

travisn requested changes Apr 22, 2024

View reviewed changes

Documentation/CRDs/ceph-nfs-crd.md Outdated Show resolved Hide resolved

Documentation/CRDs/ceph-nfs-crd.md Outdated Show resolved Hide resolved

Documentation/CRDs/ceph-nfs-crd.md Outdated Show resolved Hide resolved

parth-gr force-pushed the external-nfs branch from ae85439 to 6e13444 Compare April 23, 2024 14:21

parth-gr requested a review from travisn April 23, 2024 14:22

travisn requested changes Apr 23, 2024

View reviewed changes

parth-gr force-pushed the external-nfs branch from 6e13444 to 6fab27a Compare April 24, 2024 11:53

parth-gr requested a review from travisn April 24, 2024 15:35

travisn reviewed Apr 24, 2024

View reviewed changes

Documentation/CRDs/ceph-nfs-crd.md Outdated Show resolved Hide resolved

travisn mentioned this pull request Apr 24, 2024

Support NFS with external cluster #13277

Open

parth-gr force-pushed the external-nfs branch from 6fab27a to f4e74cb Compare April 25, 2024 05:49

parth-gr requested a review from travisn April 25, 2024 05:49

Madhu-1 requested changes Apr 25, 2024

View reviewed changes

parth-gr requested a review from Madhu-1 April 25, 2024 06:17

parth-gr force-pushed the external-nfs branch from f4e74cb to d0003c7 Compare April 25, 2024 07:39

doc: add design to support nfs in external cluster

79385d9

with recent discussion we come to conclusion nfs can be suported easily in the external cluster so adding the design that we discussed closes: rook#13277 Signed-off-by: parth-gr <partharora1010@gmail.com>

parth-gr force-pushed the external-nfs branch from d0003c7 to 79385d9 Compare April 25, 2024 07:41

BlaineEXE requested changes Apr 30, 2024

View reviewed changes

BlaineEXE changed the title ~~doc: add design to support nfs in external cluster~~ doc: add recommendation for nfs in external cluster May 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: add recommendation for nfs in external cluster #13876

doc: add recommendation for nfs in external cluster #13876

parth-gr commented Mar 5, 2024

travisn Mar 7, 2024

parth-gr Mar 8, 2024

BlaineEXE Apr 10, 2024

parth-gr Apr 22, 2024

github-actions bot commented Apr 8, 2024

mergify bot commented Apr 8, 2024

travisn Apr 23, 2024

travisn Apr 23, 2024

travisn Apr 23, 2024

parth-gr Apr 24, 2024

travisn left a comment

Madhu-1 Apr 25, 2024

parth-gr Apr 25, 2024 •

edited

Madhu-1 Apr 25, 2024

parth-gr Apr 25, 2024

parth-gr Apr 25, 2024

parth-gr commented Apr 29, 2024

BlaineEXE left a comment

BlaineEXE Apr 30, 2024

BlaineEXE Apr 30, 2024

BlaineEXE Apr 30, 2024

BlaineEXE Apr 30, 2024

BlaineEXE Apr 30, 2024

BlaineEXE Apr 30, 2024

BlaineEXE Apr 30, 2024

BlaineEXE Apr 30, 2024

mergify bot commented May 1, 2024

BlaineEXE commented May 1, 2024

	Make use of the [NFS client running on the external ceph standalone cluster](../../ceph-nfs-crd.md).
	Make use of the [NFS client running on the external ceph standalone cluster](../../ceph-nfs-crd.md#external-clusters).


		## External Clusters

		Make use of the NFS client running on the external ceph standalone cluster.


		## External Clusters

		User Applications can make use of the NFS client running on the external ceph standalone cluster.

	Make use of the [NFS client running on the external ceph standalone cluster](../../ceph-nfs-crd.md#external-clusters).
	Rook suggests a different mechanism for making use of an [NFS service running on the external Ceph standalone cluster](../../ceph-nfs-crd.md#external-clusters), if desired.

	## External Clusters
	## Consuming NFS from an external source

		Procedure:

		Export the nfs client to a particular cephfs filesystem:

doc: add recommendation for nfs in external cluster #13876

Are you sure you want to change the base?

doc: add recommendation for nfs in external cluster #13876

Conversation

parth-gr commented Mar 5, 2024

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

github-actions bot commented Apr 8, 2024

mergify bot commented Apr 8, 2024

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

travisn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parth-gr Apr 25, 2024 • 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

parth-gr commented Apr 29, 2024

BlaineEXE 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

mergify bot commented May 1, 2024

BlaineEXE commented May 1, 2024

parth-gr Apr 25, 2024 •

edited