Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Oct 30, 2017

Related to #5623

@ghost ghost self-assigned this Oct 30, 2017
@ghost ghost added this to the Next Release milestone Oct 30, 2017
@ghost
Copy link
Author

ghost commented Oct 31, 2017

@lpsantil Hi Louis, this is the follow up PR for #5623. Please review the files changed. Thanks.

@ghost ghost mentioned this pull request Oct 31, 2017
@lpsantil
Copy link

LGTM

@ghost
Copy link
Author

ghost commented Nov 1, 2017

[rev_history]
|xref:../scaling_performance/optimizing_storage.adoc#scaling-performance-optimizing-storage[Optimizing Storage]
|Added the xref:../scaling_performance/optimizing_storage.adoc#general-storage-guidelines[General Storage Guidelines] section for optimizing OpenShift storage.
%

@ghost
Copy link
Author

ghost commented Nov 1, 2017

@openshift/team-documentation Please peer review this PR and revision history.

Copy link
Contributor

Choose a reason for hiding this comment

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

s/OpenShift/{product-title}

Copy link
Contributor

Choose a reason for hiding this comment

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

We can set column widths to [cols="1,4,3",options="header"] instead, as it renders better.

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe s/capabilities will vary widely/capabilities varies widely

Choose a reason for hiding this comment

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

If the latter then it's 'capabilities vary widely'

Copy link
Contributor

Choose a reason for hiding this comment

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

s/via/through

Copy link
Contributor

Choose a reason for hiding this comment

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

Usually I prefer for Yes and 🞩 for No. Up to you if you want to change that. 😇

Copy link
Contributor

@gaurav-nelson gaurav-nelson Nov 1, 2017

Choose a reason for hiding this comment

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

We do explain the full form later but maybe we should explain the full form of ROX and RWX before we use them?

Copy link
Contributor

Choose a reason for hiding this comment

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

For this section, I think we should use points for each sentence, like:

In a non-scaled/HA {product-title} registry cluster deployment:

* The preferred storage back-end type is object storage followed by block
storage.  The storage back-end type does not need to support ReadWriteMany
* (RWX) access mode.  The storage back-end type must ensure read-after-write
* consistency.  All NAS back-end types (excluding CNS/CRS GlusterFS as it uses
an object storage interface) are expressly not recommended for {product-title}
Registry cluster deployment with production workloads.  
* While hostPath volumes are configurable for a non-scaled/HA {product-title}
Registry cluster deployment, they are not recommended.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same for Scaled registry, metrics, logging, and applications section if possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also highlight whats not recommend by using admonitions? I am not sure. 😕 ❔

Copy link

@lpsantil lpsantil Nov 2, 2017

Choose a reason for hiding this comment

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

@gaurav-nelson YES! We do need to mention what to use. Here is an email from openshift-users email list from yesterday, 11/1/2017.

I am confused over persistent storage for logging (elasticsearch).

The latest advanced installer docs [1]
specifically describes how to define using
NFS for persistent storage, but the docs for
"aggregating container logs" [2] says that
NFS should not be used (except in one
particular scenario) and seems to suggest
that the only really suitable scenario is to
use a volume (disk) directly mounted to
each logging node.

Could someone clarify the situtation?

The above misreading is not exactly the case for Logging.

My goal is to reference this page from the Registry, Metrics, and Logging install docs once this PR lands. There are probably a couple other places which could do with a reference to this doc as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Btrfs is the correct term https://en.wikipedia.org/wiki/Btrfs

Copy link
Contributor

@gaurav-nelson gaurav-nelson Nov 2, 2017

Choose a reason for hiding this comment

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

Should it be Portable Operating System Interface (POSIX) ?

@gaurav-nelson
Copy link
Contributor

Some comments and questions form me @tmorriso-rh rest LGTM 📈

Copy link

@ncbaratta ncbaratta left a comment

Choose a reason for hiding this comment

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

I need to do another review after the suggested changes are made.

Choose a reason for hiding this comment

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

delete 'sometimes'

Choose a reason for hiding this comment

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

delete 'sometimes'

Choose a reason for hiding this comment

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

"storage back-end types" is very clunky. IBM styleguide does suggest using something more detailed than 'back-end' if possible and 'types' I also feel need rewording.

Would 'storage systems' make just as much sense? Or does that change the meaning?

Choose a reason for hiding this comment

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

This goes for all occurrences of this phrase that follow.

Copy link

@lpsantil lpsantil Nov 6, 2017

Choose a reason for hiding this comment

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

@nengard I think "storage systems" is too specific, IMO. That would refer to a specific vendor or even vendor solution for vendors that offer more than one product/solution.

I think agree with you now. Maybe, you're searching for "storage technology".

Choose a reason for hiding this comment

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

We should be sure to spell out HA the first time it appears in a chapter.

Choose a reason for hiding this comment

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

Should 'hostPath' be styled as a literal?

Copy link
Author

Choose a reason for hiding this comment

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

Yes.

Choose a reason for hiding this comment

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

Issue 'with' production workloads.

Choose a reason for hiding this comment

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

Define 'PV' the first time you use it.

Choose a reason for hiding this comment

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

Does "OpenShift Container Platform" have an attribute?

Choose a reason for hiding this comment

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

comma after 'mechanisms'

Choose a reason for hiding this comment

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

If the latter then it's 'capabilities vary widely'

@ghost
Copy link
Author

ghost commented Nov 7, 2017

@nengard and @lpsantil Changes made. PTAL. Thanks.

Choose a reason for hiding this comment

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

Not sure we want to say 'topic'.

Choose a reason for hiding this comment

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

"The following table lists the available persistent storage options for {product-title}."

Copy link

Choose a reason for hiding this comment

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

Technology (or technologies) is a appropriate here. Most of the technologies are only supported buy purchasing them from a 3rd party.

Choose a reason for hiding this comment

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

Thanks @lpsantil so then:

"The following table lists the available persistent storage technologies for {product-title}."

Choose a reason for hiding this comment

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

Is this necessary? "This type of storage is presented to the operating system as a block device."

Again, try to minimize the words. A table is supposed to be easy to browse through:

"Referred to as a Storage Area Network (SAN). This type of storage is suitable for applications that need full control of storage and operate at a low level on files bypassing the file system. It is usually a non-shareable type of storage, which means that only one client at a time can mount an endpoint of this type."

Consider doing the same will the other descriptions below.

Copy link

Choose a reason for hiding this comment

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

I was intentionally this verbose because not all OpenShift administrators will have strong Storage experience. The verbosity here provides baseline understanding w/o resorting to citation heavy descriptions.

Choose a reason for hiding this comment

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

I think we can still get across the same amount of information without so many extraneous words.

Choose a reason for hiding this comment

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

Are footnotes common in these books? If not I'd use Notes not Footnotes. Footnotes are a very academic paper thing and not common in technical documentation.

Choose a reason for hiding this comment

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

Remove "s" from "provides"

Copy link
Author

Choose a reason for hiding this comment

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

"GlusterFS provide interfaces..." ?

Choose a reason for hiding this comment

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

what is ROX

Copy link

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

We should make that clear on the first usage. Always spell it out the first time.

Choose a reason for hiding this comment

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

why isn't Btrfs monospaced as well?

Choose a reason for hiding this comment

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

There was no hyphen in thin pool above - so either add up there, or remove here.

Copy link
Author

Choose a reason for hiding this comment

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

Thin pool is a noun, and thin-pool is an adjective, correct?

Choose a reason for hiding this comment

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

You are right, ignore my comment ;) hehe

Choose a reason for hiding this comment

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

Add hyphen? or leave and remove above.

Choose a reason for hiding this comment

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

thin-pool or thin pool - be consistent.

Copy link

Choose a reason for hiding this comment

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

s/all NAS/NAS/

Copy link

Choose a reason for hiding this comment

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

s/all NAS/NAS/

@ghost
Copy link
Author

ghost commented Nov 8, 2017

@nengard PTAL.

@ghost
Copy link
Author

ghost commented Nov 9, 2017

@nengard @lpsantil I used bullet lists in the table to help with readability. I agree with Louis that the information is important since this a guidelines document. So, hoping the lists makes the table easier to read. PTAL.

@ncbaratta ncbaratta added the peer-review-done Signifies that the peer review team has reviewed this PR label Nov 9, 2017
@ghost ghost merged commit 9d43121 into openshift:master Nov 13, 2017
@vikram-redhat
Copy link
Contributor

@tmorriso-rh - Hey Traci - this hasn't been picked the 3.7 stage branch.

@vikram-redhat vikram-redhat modified the milestones: Next Release, Staging, Published - 21/Nov/2017 Nov 20, 2017
@ghost ghost deleted the PR-5623-adding-storage-best-practices branch November 30, 2017 19:38
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

branch/enterprise-3.6 branch/enterprise-3.7 peer-review-done Signifies that the peer review team has reviewed this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants