Skip to content

Conversation

bobfuru
Copy link
Contributor

@bobfuru bobfuru commented May 15, 2020

@bobfuru bobfuru added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. branch/enterprise-4.5 labels May 15, 2020
@bobfuru bobfuru added this to the Future Release milestone May 15, 2020
@openshift-ci-robot openshift-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 May 15, 2020
@openshift-docs-preview-bot

The preview will be available shortly at:

@bobfuru bobfuru force-pushed the OSDOCS-930-manila branch 5 times, most recently from 139530e to e80aba9 Compare May 19, 2020 17:13
@bobfuru
Copy link
Contributor Author

bobfuru commented May 19, 2020

@duanwei33 PTAL?

@bobfuru bobfuru force-pushed the OSDOCS-930-manila branch 2 times, most recently from ec53882 to f2c78a6 Compare May 20, 2020 14:53
@duanwei33
Copy link

@bobfuru Sorry for the delay, I think there is still some update for this feature, I list some items not align with our test:

  1. Project name should be manila-csi instead of csi-manila
  2. After "Installing the CSI Manila Operator" and before "Dynamically provisioning CSI Manila volumes", there is one more step to install ManilaDriver, which will create csi driver related resource and storage class (It is more like install LocalVolume in local storage), it can be installed from web or client.
  3. In "Dynamically provisioning CSI Manila volumes", the storage class name for manila should be "csi-manila-ceph" instead of "csi-manila-nfs"
  4. Just suggest only keep command line way in "Dynamically provisioning CSI Manila volumes" to simple the doc (Just a suggestion)
  5. "pvc-clone.yaml" should be updated to "pvc-manila.yaml".
  6. Not very sure the note for RWO:

Use RWO mode if you want to prevent additional Pods from being dynamically provisioned.

I can provide env with manila operator if you needed.
BTW, just wondering when the doc should be finished reviewed by QE, I know the GA date is June 18th?

@duanwei33
Copy link

Hi Bob,
I checked some pr, seems something changed like namespace, install mode, driver name.
I will give another review next week after verified ON_QA bugs.

@bobfuru bobfuru force-pushed the OSDOCS-930-manila branch 2 times, most recently from 43674a2 to ea21b50 Compare May 28, 2020 20:58
@bobfuru
Copy link
Contributor Author

bobfuru commented May 28, 2020

Hi Wei,

Thanks for the feedback. See my inline comments below...

@bobfuru Sorry for the delay, I think there is still some update for this feature, I list some items not align with our test:

That makes sense. Because I didn't have CSI Manila Operator in OLM, I was mostly guessing. :)

  1. Project name should be manila-csi instead of csi-manila
    OK, changed. Do you think we need this step? I think a default namespace is created if we leave this out.
  1. After "Installing the CSI Manila Operator" and before "Dynamically provisioning CSI Manila volumes", there is one more step to install ManilaDriver, which will create csi driver related resource and storage class (It is more like install LocalVolume in local storage), it can be installed from web or client.

I added a section for instructions about installing ManilaDriver (or whatever name it will be called). So the Operator doesn't deploy the driver? I thought that's one of the features of CSI Operators? I'm not sure what the steps are to do this, so please fill in/comment on them as you have more details.

  1. In "Dynamically provisioning CSI Manila volumes", the storage class name for manila should be "csi-manila-ceph" instead of "csi-manila-nfs"

Fixed.

  1. Just suggest only keep command line way in "Dynamically provisioning CSI Manila volumes" to simple the doc (Just a suggestion)

Because we have console instructions, I'm inclined to keep those here, along with CLI.

  1. "pvc-clone.yaml" should be updated to "pvc-manila.yaml".

Fixed!

  1. Not very sure the note for RWO:

Use RWO mode if you want to prevent additional Pods from being dynamically provisioned.

This came from a BZ enhancement for local storage Operator install, so I thought it would also be useful context here. If you think it's incorrect, please let me know and I'll remove.

I can provide env with manila operator if you needed.

Yes, that would be most helpful, thanks!

BTW, just wondering when the doc should be finished reviewed by QE, I know the GA date is June 18th?

As of now, our Docs freeze date is June 8, but I know GA is now July 9, so we will probably have additional time for this doc to be finished with QE review. Or is June 8 even possible for you?

@bobfuru
Copy link
Contributor Author

bobfuru commented May 28, 2020

Hi Bob,
I checked some pr, seems something changed like namespace, install mode, driver name.
I will give another review next week after verified ON_QA bugs.

Perfect, thank you, @duanwei33 !! I did what I could for now based on the info I have (and no test env). Appreciate you looking now and again next week!!

@bobfuru
Copy link
Contributor Author

bobfuru commented Jun 1, 2020

@jeana-redhat PTAL for clarity, typos, links, etc. Thanks!

@bobfuru bobfuru force-pushed the OSDOCS-930-manila branch from ea21b50 to 99466b4 Compare June 3, 2020 19:07
Copy link
Contributor

@jeana-redhat jeana-redhat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestions, mostly ISG or minimalism related. Anything I asked as a question is something I'm uncertain about just because I'm still getting up to speed - if the question is irrelevant feel free to just ignore rather than explaining so you don't get caught up in that.

@jboxman
Copy link
Contributor

jboxman commented Jun 3, 2020

@jeana-redhat, I don't usually drive-by comment, but this is impressive peer review work and I wanted to say so!

@bobfuru bobfuru added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 4, 2020
@bobfuru bobfuru force-pushed the OSDOCS-930-manila branch 5 times, most recently from 05c06ba to a613756 Compare June 5, 2020 19:17
@bobfuru bobfuru removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 5, 2020
@bobfuru bobfuru force-pushed the OSDOCS-930-manila branch 3 times, most recently from ab01b7b to 46fe558 Compare June 29, 2020 15:47
@bobfuru
Copy link
Contributor Author

bobfuru commented Jun 29, 2020

Note that there is related work in #22978. Once that PR is merged, the same guidance should be applied to this PR to use {rh-openstack-first} and {rh-openstack} attributes.

Done.

@bobfuru bobfuru force-pushed the OSDOCS-930-manila branch from 46fe558 to 114caa6 Compare June 29, 2020 16:29
@bobfuru bobfuru force-pushed the OSDOCS-930-manila branch from 114caa6 to a024f09 Compare June 29, 2020 21:31
Copy link

@tombarron tombarron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've posted remarks on a few nits or small issues, but this looks good and is very close to merge IMO.
But I think manila-csi should be included in the section dedicated to dynamic provisioning as well [1].
It could appear in the "Available dynamic provisioning plug-ins" table right after OpenStack cinder. The provisioner plug-in name is "manila.csi.openstack.org". Notes could just be a link back to the manila-csi docs that you are currently writing. There should be a "RHOSP Manila CSI object definition" section that says the Manila CSI storage classes are automatically created when the Operator and Driver are installed as covered in [link](to this section you are writing now).

[1] https://osdocs-930-manila--ocpdocs.netlify.app/openshift-enterprise/latest/storage/dynamic-provisioning.html

@bobfuru bobfuru force-pushed the OSDOCS-930-manila branch 3 times, most recently from a3a9b02 to 58d48a4 Compare June 30, 2020 16:53
@bobfuru
Copy link
Contributor Author

bobfuru commented Jun 30, 2020

But I think manila-csi should be included in the section dedicated to dynamic provisioning as well [1].
It could appear in the "Available dynamic provisioning plug-ins" table right after OpenStack cinder. The provisioner plug-in name is "manila.csi.openstack.org". Notes could just be a link back to the manila-csi docs that you are currently writing. There should be a "RHOSP Manila CSI object definition" section that says the Manila CSI storage classes are automatically created when the Operator and Driver are installed as covered in [link](to this section you are writing now).

Great idea! I've added CSI Manila to the plug-ins table and added a section for the object definition. As you said, this is redundant but conforms to how we've laid out the rest of the page.

Preview of this addition here: https://osdocs-930-manila--ocpdocs.netlify.app/openshift-enterprise/latest/storage/dynamic-provisioning.html#available-plug-ins_dynamic-provisioning

@bobfuru bobfuru force-pushed the OSDOCS-930-manila branch from 58d48a4 to 91d64af Compare June 30, 2020 17:02
@bobfuru bobfuru force-pushed the OSDOCS-930-manila branch from 91d64af to 57961f1 Compare June 30, 2020 17:30
@tombarron
Copy link

LGTM, Bob. Thanks for your work on this one!

@bobfuru bobfuru merged commit a95bd70 into openshift:master Jun 30, 2020
@bobfuru bobfuru deleted the OSDOCS-930-manila branch June 30, 2020 17:49
@bobfuru
Copy link
Contributor Author

bobfuru commented Jun 30, 2020

/cherrypick enterprise-4.5

@openshift-cherrypick-robot

@bobfuru: new pull request created: #23391

In response to this:

/cherrypick enterprise-4.5

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.

wking added a commit to wking/openshift-docs that referenced this pull request Oct 8, 2020
xrefs aren't allowed in modules:

  $ grep -A1 'You must not include xrefs' modules/mod-docs-ocp-conventions.adoc
  You must not include xrefs in modules or create an xref to a module. You can
  only use xrefs to link from one assembly to another.

And we can't move this module out into an assembly directory, because
it is consumed from two places:

  $ git --no-pager grep dynamic-provisioning-available-plugins
  post_installation_configuration/storage-configuration.adoc:include::modules/dynamic-provisioning-available-plugins.adoc[leveloffset=+3]
  storage/dynamic-provisioning.adoc:include::modules/dynamic-provisioning-available-plugins.adoc[leveloffset=+1]

We could:

a. Trim down post_installation_configuration to just xref the storage
   assembly, instead of duping so much of it's content via modules.
b. Drop the module and instead inline (some of) it with copy/paste in
   both assemblies.
c. Remove the xref.

With this commit, I'm going with (c), removing the xref which was
added in 57961f1 ([STOR] Include section on the OpenStack Manila
CSI Driver, 2020-05-12, openshift#22199).
openshift-cherrypick-robot pushed a commit to openshift-cherrypick-robot/openshift-docs that referenced this pull request Oct 8, 2020
xrefs aren't allowed in modules:

  $ grep -A1 'You must not include xrefs' modules/mod-docs-ocp-conventions.adoc
  You must not include xrefs in modules or create an xref to a module. You can
  only use xrefs to link from one assembly to another.

And we can't move this module out into an assembly directory, because
it is consumed from two places:

  $ git --no-pager grep dynamic-provisioning-available-plugins
  post_installation_configuration/storage-configuration.adoc:include::modules/dynamic-provisioning-available-plugins.adoc[leveloffset=+3]
  storage/dynamic-provisioning.adoc:include::modules/dynamic-provisioning-available-plugins.adoc[leveloffset=+1]

We could:

a. Trim down post_installation_configuration to just xref the storage
   assembly, instead of duping so much of it's content via modules.
b. Drop the module and instead inline (some of) it with copy/paste in
   both assemblies.
c. Remove the xref.

With this commit, I'm going with (c), removing the xref which was
added in 57961f1 ([STOR] Include section on the OpenStack Manila
CSI Driver, 2020-05-12, openshift#22199).
openshift-cherrypick-robot pushed a commit to openshift-cherrypick-robot/openshift-docs that referenced this pull request Oct 8, 2020
xrefs aren't allowed in modules:

  $ grep -A1 'You must not include xrefs' modules/mod-docs-ocp-conventions.adoc
  You must not include xrefs in modules or create an xref to a module. You can
  only use xrefs to link from one assembly to another.

And we can't move this module out into an assembly directory, because
it is consumed from two places:

  $ git --no-pager grep dynamic-provisioning-available-plugins
  post_installation_configuration/storage-configuration.adoc:include::modules/dynamic-provisioning-available-plugins.adoc[leveloffset=+3]
  storage/dynamic-provisioning.adoc:include::modules/dynamic-provisioning-available-plugins.adoc[leveloffset=+1]

We could:

a. Trim down post_installation_configuration to just xref the storage
   assembly, instead of duping so much of it's content via modules.
b. Drop the module and instead inline (some of) it with copy/paste in
   both assemblies.
c. Remove the xref.

With this commit, I'm going with (c), removing the xref which was
added in 57961f1 ([STOR] Include section on the OpenStack Manila
CSI Driver, 2020-05-12, openshift#22199).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.