-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
OCPBUGS-10640-13: 4.13 Added clarification point to disk partition BM… #73361
OCPBUGS-10640-13: 4.13 Added clarification point to disk partition BM… #73361
Conversation
@dfitzmau: This pull request references Jira Issue OCPBUGS-10640, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
🤖 Tue Mar 19 11:35:39 - Prow CI generated the docs preview: |
@dfitzmau: all tests passed! Full PR test history. Your PR dashboard. 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. |
/label peer-review-needed |
/remove-label peer-review-needed |
/lgtm |
/label peer-review-needed |
@dfitzmau: This pull request references Jira Issue OCPBUGS-10640, which is invalid:
Comment In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall. Added a few nitpicks and some other changes for you to consider. Thanks, @dfitzmau!
@@ -67,31 +67,34 @@ The `--copy-network` option only copies networking configuration found under `/e | |||
[id="installation-user-infra-machines-advanced_disk_{context}"] | |||
== Disk partitioning | |||
|
|||
// This content is not modularized, so any updates to this "Disk partitioning" section should be checked against the module created for vSphere UPI parity in the module file named `installation-disk-partitioning.adoc` for consistency until such time as this large assembly can be modularized. | |||
The disk partitions are created on {product-title} cluster nodes during the {op-system-first} installation. Each {op-system} node of a particular architecture uses the same partition layout, unless the default partitioning configuration is overridden. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The disk partitions are created on {product-title} cluster nodes during the {op-system-first} installation. Each {op-system} node of a particular architecture uses the same partition layout, unless the default partitioning configuration is overridden. | |
Disk partitions are created on {product-title} cluster nodes during the {op-system-first} installation. Each {op-system} node of a particular architecture uses the same partition layout, unless you override the default partitioning configuration. |
- Omit definite article. It doesn't look you're referring back to specific partitions here.
- Avoid passive voice.
[IMPORTANT] | ||
==== | ||
For disk sizes larger than 100GB, and especially disk sizes larger than 1TB, create a separate `/var` partition. See "Creating a separate `/var` partition" and this link:https://access.redhat.com/solutions/5587281[Red Hat Knowledgebase article] for more information. | ||
The use of a custom partition scheme on your node could result in {product-title} not monitoring or alerting on some node partitions. If you are overriding the default partitioning, see link:https://access.redhat.com/articles/4766521[Understanding OpenShift File System Monitoring (eviction conditions)] for more information about how {product-title} monitors your host file systems. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of a custom partition scheme on your node could result in {product-title} not monitoring or alerting on some node partitions. If you are overriding the default partitioning, see link:https://access.redhat.com/articles/4766521[Understanding OpenShift File System Monitoring (eviction conditions)] for more information about how {product-title} monitors your host file systems. | |
The use of a custom partition scheme on your node might result in {product-title} not monitoring or alerting on some node partitions. If you are overriding the default partitioning, see link:https://access.redhat.com/articles/4766521[Understanding OpenShift File System Monitoring (eviction conditions)] for more information about how {product-title} monitors your host file systems. |
Per the ISG, don't use could
"in the present tense to mean might or can".
==== | ||
|
||
* Retaining existing partitions: For a brownfield installation where you are reinstalling {product-title} on an existing node and want to retain data partitions installed from your previous operating system, there are both boot arguments and options to `coreos-installer` that allow you to retain existing data partitions. | ||
You might want to add a separate storage partition for your containers and container images. For example, by mounting `/var/lib/containers` in a separate partition, the kubelet separately monitor `/var/lib/containers` as the `imagefs` directory and the root file system as the `nodefs` directory. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might want to add a separate storage partition for your containers and container images. For example, by mounting `/var/lib/containers` in a separate partition, the kubelet separately monitor `/var/lib/containers` as the `imagefs` directory and the root file system as the `nodefs` directory. | |
[In situations such as...], you might want to add a separate storage partition for your containers and container images. For example, by mounting `/var/lib/containers` in a separate partition, the kubelet separately monitors `/var/lib/containers` as the `imagefs` directory and the root file system as the `nodefs` directory. |
- I'd suggest adding a clause at the beginning for this sentence to add clarification about when a user might want to add a separate storage partition. I think it'd be good for users to know when doing so would be useful.
- Fix subject-verb disagreement typo.
==== | ||
The use of custom partitions could result in those partitions not being monitored by {product-title} or alerted on. If you are overriding the default partitioning, see link:https://access.redhat.com/articles/4766521[Understanding OpenShift File System Monitoring (eviction conditions)] for more information about how {product-title} monitors your host file systems. | ||
If you resized your disk size to host a larger file system, consider creating a separate `/var/lib/containers` partition. This is important for a disk formatted to `xfs`, where a high number of allocation groups might cause CPU time issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you resized your disk size to host a larger file system, consider creating a separate `/var/lib/containers` partition. This is important for a disk formatted to `xfs`, where a high number of allocation groups might cause CPU time issues. | |
If you have resized your disk size to host a larger file system, consider creating a separate `/var/lib/containers` partition. This is important for a disk formatted to `xfs`, where a high number of allocation groups might cause CPU time issues. |
- Adding a present participle indicates more clearly that you're talking about a present situation, imo. WDYT?
- Per the ISG avoid ambiguous pronoun references; what does "This" refer to? Try to rephrase the last sentence to avoid a "this is" construction at all (see the ISG word usage guidance for "there is, there are" for an example.
Thanks, @bburt-rh . I'll aim to get this PR merged and then address yoru feedback so that all branches have similar changes. |
/label merge-review-needed |
I see that you will be addressing peer review comments after merge so I will go ahead and merge. |
@dfitzmau: Jira Issue OCPBUGS-10640 is in an unrecognized state (Release Pending) and will not be moved to the MODIFIED state. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
SEE #67707 FOR SME APPROVAL.
OCPBUGS-10640
Version(s):
4.13
For 4.13 and 4.12, ensure you do not remove:
Link to docs preview:
Disk partioning
Additional information: