Skip to content
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-3829]: Add etcd module from KCS article #57592

Merged
merged 1 commit into from
Mar 29, 2023

Conversation

tmalove
Copy link
Contributor

@tmalove tmalove commented Mar 22, 2023

OCPBUGS-3829

Version(s): 4.9+

Link to docs preview (updated 3/28): http://file.rdu.redhat.com/tlove/etcd-ocpbugs-3829-tlove-new/scalability_and_performance/recommended-host-practices.html#move-etcd-different-disk_recommended-host-practices

QE review:

  • [ x] QE has approved this change.

Additional information: access.redhat.com/solutions/6973069
This Jira is to add the content from this KCS article to the docs.

@openshift-ci openshift-ci bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 22, 2023
@ocpdocs-previewbot
Copy link

ocpdocs-previewbot commented Mar 22, 2023

🤖 Updated build preview is available at:
https://57592--docspreview.netlify.app

Build log: https://circleci.com/gh/ocpdocs-previewbot/openshift-docs/12477

@tmalove
Copy link
Contributor Author

tmalove commented Mar 23, 2023

@geliu2016 Will you /lgtm this module update? Thanks!

@tmalove tmalove changed the title [OCPBUGS-add module from KCS article [OCPBUGS-3829]: Add etcd module from KCS article Mar 23, 2023
@tmalove tmalove force-pushed the etcd-ocpbugs-3829-tlove-new branch from 1a85aae to 045145f Compare March 23, 2023 17:22
@michaelalang
Copy link

michaelalang commented Mar 23, 2023

lgtm

@openshift-ci
Copy link

openshift-ci bot commented Mar 23, 2023

@michaelalang: changing LGTM is restricted to collaborators

In response to this:

/lgtm

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.

$ oc replace -f etcd-mc.yml
----
+
The previous step prevents the nodes from rebooting.

Choose a reason for hiding this comment

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

@tmalove what means of "The previous step prevents the nodes from rebooting."? which previous step? thanks

Choose a reason for hiding this comment

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

I suppose it's "oc replace" step, exact?

@geliu2016
Copy link

/lgtm if you may double confirm my question above, thanks

@michaelalang
Copy link

@geliu2016 it just seems to be a mis understanding from the original KCS mapping to the Docs ...

$ oc replace -f etcd-mc.yml
this step is expected to not cause a reboot of the nodes

https://access.redhat.com/solutions/6973069

@geliu2016
Copy link

@geliu2016 it just seems to be a mis understanding from the original KCS mapping to the Docs ...

$ oc replace -f etcd-mc.yml
this step is expected to not cause a reboot of the nodes

https://access.redhat.com/solutions/6973069

Great to know, thank you, Michaela

@tmalove
Copy link
Contributor Author

tmalove commented Mar 24, 2023

@geliu2016 @michaelalang I will move that sentence (about ...prevents the nodes from...) into Step 4, for clarity. Thanks!

Copy link
Contributor

@lahinson lahinson left a comment

Choose a reason for hiding this comment

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

Nice work, Tami! I made a couple of very minor suggestions. Thanks for working on this one!

* The `MachineConfigPool` must match `metadata.labels[“machineconfiguration.openshift.io/role]`. This applies to a controller, worker, or a custom pool.
* The node's auxiliary storage device, such as `/dev/sdb`, must match the sdb. Change this reference in all places in the file.

NOTE: This procedure does not move parts of the root filesystem, such as /var/, to another disk or partition on an installed node.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
NOTE: This procedure does not move parts of the root filesystem, such as /var/, to another disk or partition on an installed node.
[NOTE]
====
This procedure does not move parts of the root file system, such as /var/, to another disk or partition on an installed node.
====

$ oc login -u ${ADMIN} -p ${ADMINPASSWORD} ${API} [... output omitted ...] $ oc create -f etcd-mc.yml machineconfig.machineconfiguration.openshift.io/98-var-lib-etcd created
----
+
The nodes update and reboot. After the reboot completes, the following events occur:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The nodes update and reboot. After the reboot completes, the following events occur:
The nodes are updated and rebooted. After the reboot is completed, the following events occur:

A couple of minor suggestions here. Unless the nodes update and reboot themselves, I think we need to adjust the wording to indicate that they are updated and rebooted by some other entity.

@tmalove tmalove force-pushed the etcd-ocpbugs-3829-tlove-new branch 2 times, most recently from d020905 to b0a0380 Compare March 28, 2023 17:53
@tmalove
Copy link
Contributor Author

tmalove commented Mar 28, 2023

/label peer-review-needed

@openshift-ci openshift-ci bot added the peer-review-needed Signifies that the peer review team needs to review this PR label Mar 28, 2023
@jldohmann jldohmann added this to the Continuous Release milestone Mar 28, 2023
Copy link
Contributor

@jldohmann jldohmann left a comment

Choose a reason for hiding this comment

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

it's looking good! left some feedback below


.Prerequisites

* The `MachineConfigPool` must match `metadata.labels[“machineconfiguration.openshift.io/role]`. This applies to a controller, worker, or a custom pool.
Copy link
Contributor

@jldohmann jldohmann Mar 28, 2023

Choose a reason for hiding this comment

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

there's is a quotation mark here that is not closed:

Suggested change
* The `MachineConfigPool` must match `metadata.labels[machineconfiguration.openshift.io/role]`. This applies to a controller, worker, or a custom pool.
* The `MachineConfigPool` must match `metadata.labels["machineconfiguration.openshift.io/role"]`. This applies to a controller, worker, or a custom pool.

or maybe it should be removed?

Suggested change
* The `MachineConfigPool` must match `metadata.labels[machineconfiguration.openshift.io/role]`. This applies to a controller, worker, or a custom pool.
* The `MachineConfigPool` must match `metadata.labels[machineconfiguration.openshift.io/role]`. This applies to a controller, worker, or a custom pool.

.Prerequisites

* The `MachineConfigPool` must match `metadata.labels[“machineconfiguration.openshift.io/role]`. This applies to a controller, worker, or a custom pool.
* The node's auxiliary storage device, such as `/dev/sdb`, must match the sdb. Change this reference in all places in the file.
Copy link
Contributor

Choose a reason for hiding this comment

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

is sdb a known abbreviation? ISG says to only use abbreviations if they're well known by the audience or the full form has been stated somewhere earlier in the text

Copy link
Contributor

@mjpytlak mjpytlak Mar 29, 2023

Choose a reason for hiding this comment

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

@tmalove @jldohmann makes a good point. Is "sdb" well known to our storage audience or is it an acronym that must be defined?

Copy link
Contributor

Choose a reason for hiding this comment

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

sdb is the block device name (/storage/disk) presented to the VM/Instance available on the path /dev/sdb.
In the last generation instances in AWS those standards have been changed to /dev/nvme{0...9}*, or nvme{0...9}*

For reference: https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/block-device-mapping-concepts.html

Copy link
Contributor

Choose a reason for hiding this comment

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

sdb is the block device name (/storage/disk) presented to the VM/Instance available on the path

Thanks @mtulio for the explanation. I will get you merged up @tmalove.

modules/move-etcd-different-disk.adoc Outdated Show resolved Hide resolved
modules/move-etcd-different-disk.adoc Outdated Show resolved Hide resolved
modules/move-etcd-different-disk.adoc Outdated Show resolved Hide resolved
modules/move-etcd-different-disk.adoc Outdated Show resolved Hide resolved
modules/move-etcd-different-disk.adoc Outdated Show resolved Hide resolved
modules/move-etcd-different-disk.adoc Outdated Show resolved Hide resolved
@jldohmann jldohmann added peer-review-done Signifies that the peer review team has reviewed this PR and removed peer-review-in-progress Signifies that the peer review team is reviewing this PR peer-review-needed Signifies that the peer review team needs to review this PR labels Mar 28, 2023
@tmalove tmalove force-pushed the etcd-ocpbugs-3829-tlove-new branch from b0a0380 to abd9049 Compare March 29, 2023 00:07
@tmalove tmalove force-pushed the etcd-ocpbugs-3829-tlove-new branch from b285146 to 62ad012 Compare March 29, 2023 00:49
@tmalove
Copy link
Contributor Author

tmalove commented Mar 29, 2023

/remove-label peer-review-done
/label merge-review-needed

@openshift-ci openshift-ci bot added merge-review-needed Signifies that the merge review team needs to review this PR and removed peer-review-done Signifies that the peer review team has reviewed this PR labels Mar 29, 2023
@mjpytlak mjpytlak added merge-review-in-progress Signifies that the merge review team is reviewing this PR and removed merge-review-needed Signifies that the merge review team needs to review this PR labels Mar 29, 2023
@mjpytlak
Copy link
Contributor

@tmalove For future reference, please do not remove peer-review-done. The squad uses that latter to make sure the peer review is complete ahead of a merge.

@tmalove
Copy link
Contributor Author

tmalove commented Mar 29, 2023

@tmalove For future reference, please do not remove peer-review-done. The squad uses that latter to make sure the peer review is complete ahead of a merge.

Apologies @mjpytlak ...I have it in my 'smart reply'. I will remove it now.

@mjpytlak
Copy link
Contributor

mjpytlak commented Mar 29, 2023

No worries @tmalove. However, ahead of a merge please address https://github.com/openshift/openshift-docs/pull/57592/files#r1151029235. Not sure if you intentionally left scb without defining it or missed Jessie's comment.

@mjpytlak mjpytlak merged commit 60f33d9 into openshift:main Mar 29, 2023
@mjpytlak
Copy link
Contributor

/cherrypick enterprise-4.13

@openshift-cherrypick-robot

@mjpytlak: new pull request created: #57980

In response to this:

/cherrypick enterprise-4.13

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.

@mjpytlak
Copy link
Contributor

/cherrypick enterprise-4.12

@openshift-cherrypick-robot

@mjpytlak: new pull request created: #57981

In response to this:

/cherrypick enterprise-4.12

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.

@mjpytlak
Copy link
Contributor

/cherrypick enterprise-4.11

@openshift-cherrypick-robot

@mjpytlak: new pull request created: #57982

In response to this:

/cherrypick enterprise-4.11

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.

@mjpytlak
Copy link
Contributor

/cherrypick enterprise-4.10

@openshift-cherrypick-robot

@mjpytlak: new pull request created: #57983

In response to this:

/cherrypick enterprise-4.10

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.

@mjpytlak
Copy link
Contributor

/cherrypick enterprise-4.9

@openshift-cherrypick-robot

@mjpytlak: #57592 failed to apply on top of branch "enterprise-4.9":

Applying: add module from KCS article
.git/rebase-apply/patch:146: trailing whitespace.
 [... output omitted ...] 
warning: 1 line adds whitespace errors.
Using index info to reconstruct a base tree...
M	scalability_and_performance/recommended-host-practices.adoc
Falling back to patching base and 3-way merge...
Auto-merging scalability_and_performance/recommended-host-practices.adoc
CONFLICT (content): Merge conflict in scalability_and_performance/recommended-host-practices.adoc
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 add module from KCS article
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherrypick enterprise-4.9

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.

@mjpytlak
Copy link
Contributor

@tmalove Looks like you have a conflict in 4.9. Please resolve it and give me a shout when the manual CP is ready to be merged. In the interim, I would get the other branches merged.

@tmalove
Copy link
Contributor Author

tmalove commented Mar 29, 2023

@tmalove Looks like you have a conflict in 4.9. Please resolve it and give me a shout when the manual CP is ready to be merged. In the interim, I would get the other branches merged.

Will do, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch/enterprise-4.9 branch/enterprise-4.10 branch/enterprise-4.11 branch/enterprise-4.12 branch/enterprise-4.13 merge-review-in-progress Signifies that the merge review team is reviewing this PR size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants