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

Bug 1886229: Clarify multipath support in release notes #30697

Merged
merged 1 commit into from May 5, 2021

Conversation

jlebon
Copy link
Member

@jlebon jlebon commented Mar 19, 2021

Make it clearer that multipathing is only supported when activated using
the documented MachineConfig and that it must not be enabled before
this.

@openshift-ci-robot openshift-ci-robot added the bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. label Mar 19, 2021
@openshift-ci-robot
Copy link

@jlebon: This pull request references Bugzilla bug 1886229, which is invalid:

  • expected the bug to be open, but it isn't
  • expected the bug to be in one of the following states: NEW, ASSIGNED, ON_DEV, POST, POST, but it is CLOSED (ERRATA) instead
  • expected Bugzilla bug 1886229 to depend on a bug in one of the following states: VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), but no dependents were found

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 1886229: Clarify multipath support in release notes

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.

@openshift-ci-robot openshift-ci-robot added bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 19, 2021
@@ -91,7 +90,7 @@ Previously, {op-system} DHCP kernel parameters were not working as expected beca
[id="ocp-4-7-rhcos-supports-multipath"]
==== {op-system} supports multipath

{op-system} now supports multipath on the primary disk, allowing stronger resilience to hardware failure so that you can set up {op-system} on top of multipath to achieve higher host availability. See link:https://bugzilla.redhat.com/show_bug.cgi?id=1886229[*BZ#1886229*] for more information.
{op-system} now supports multipath on the primary disk, allowing stronger resilience to hardware failure so that you can set up {op-system} on top of multipath to achieve higher host availability. Note that multipathing should only be enabled via kernel arguments within a MachineConfig as documented. Specifically, it shouldn't be enabled at installation time. See link:https://bugzilla.redhat.com/show_bug.cgi?id=1886229[*BZ#1886229*] for more information.
Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to link to the part of the docs where we show the multipath MachineConfig, but... while I see it in the repo, I don't see that page anywhere in the live site. Are we missing an include?

Copy link
Member

@miabbott miabbott Mar 23, 2021

Choose a reason for hiding this comment

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

It looks like the MachineConfigs with multipath configs are hidden behind an if statement for IBM Z systems:

https://raw.githubusercontent.com/openshift/openshift-docs/enterprise-4.7/modules/installation-complete-user-infra.adoc

ifdef::ibm-z,ibm-power[]
. For an installation with FCP, additional steps are required to enable multipathing. 
.. To enable multipathing on master nodes, apply the following machine config file: 
+
[source,yaml]
--
apiVersion: machineconfiguration.openshift.io/v1
kind: MachineConfig
metadata:
  labels:
    machineconfiguration.openshift.io/role: "master"
  name: 99-master-kargs-mpath
spec:
  kernelArguments:
    - 'rd.multipath=default'
    - 'root=/dev/disk/by-label/dm-mpath-root'
--

.. To enable multipathing on worker nodes, apply the following machine config file: 
+
[source,yaml]

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahhh, good find! Oddly, doing a search for "multipath" on the docs site doesn't find that section.

So yeah, I think we should drop the ifdef here and have it included for not just IBM Z.

Copy link
Member

@miabbott miabbott Mar 23, 2021

Choose a reason for hiding this comment

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

Ahhh, good find! Oddly, doing a search for "multipath" on the docs site doesn't find that section.

Yeah, I find the search on the site to be a bit unreliable

So yeah, I think we should drop the ifdef here and have it included for not just IBM Z.

Agreed!

I'd also suggest tweaking the order; having it as the final bit of info after the cluster has been installed seems out of place.

@netlify
Copy link

netlify bot commented Mar 19, 2021

Deploy preview for osdocs ready!

Built with commit b53b6ee

https://deploy-preview-30697--osdocs.netlify.app

@bobfuru bobfuru self-requested a review March 23, 2021 14:15
@bobfuru
Copy link
Contributor

bobfuru commented Mar 23, 2021

Thanks for this, @jlebon.
Our site search is due for improvements and they are in the works. :) I often have better luck searching https://github.com/openshift/openshift-docs if that helps.

So yeah, I think we should drop the ifdef here and have it included for not just IBM Z.

I've created https://bugzilla.redhat.com/show_bug.cgi?id=1942192 and will soon be adding the multipath steps to non-Power/Z install docs as well. Once that is settled and the ifdef is removed, we could look at pointing this PR to that section.

I'd also suggest tweaking the order; having it as the final bit of info after the cluster has been installed seems out of place.

@miabbott Earlier in the doc under Creating Red Hat Enterprise Linux CoreOS (RHCOS) machines (step 3, bullet 5), there are instructions for multipath settings.

For installations on FCP-type disks, complete the following tasks:

i. Use rd.zfcp=<adapter>,<wwpn>,<lun> to specify the FCP disk where RHCOS is to be installed. For multipathing repeat this step for each additional path.

ii. For multipathing, set the following parameter: rd.multipath=default.

iii. For multipathing, set the install device as: coreos.inst.install_dev=/dev/mapper/mpatha.
...

Do you think the machine config examples should be moved up here? Maybe we continue in this part by saying "multipathing is only supported when activated by applying the following machine config files" and then showing the master and worker node machine configs examples.

@miabbott
Copy link
Member

miabbott commented Mar 24, 2021

Do you think the machine config examples should be moved up here? Maybe we continue in this part by saying "multipathing is only supported when activated by applying the following machine config files" and then showing the master and worker node machine configs examples.

Based on this proposed update from @jlebon, it seems like we shouldn't be suggesting the use of kernel arguments at install time and should only be showing how to enable multipathing after installation.

Note that multipathing should only be enabled via kernel arguments within a MachineConfig as documented. Specifically, it shouldn't be enabled at installation time.

Unless this caveat doesn't apply to installations on Z systems?

Perhaps the use of MachineConfigs to enable multipath would fall under the "Post-installation configuration" section of the docs?

@jlebon
Copy link
Member Author

jlebon commented Mar 24, 2021

Do you think the machine config examples should be moved up here? Maybe we continue in this part by saying "multipathing is only supported when activated by applying the following machine config files" and then showing the master and worker node machine configs examples.

Based on this proposed update from @jlebon, it seems like we shouldn't be suggesting the use of kernel arguments at install time and should only be showing how to enable multipathing after installation.

Note that multipathing should only be enabled via kernel arguments within a MachineConfig as documented. Specifically, it shouldn't be enabled at installation time.

Unless this caveat doesn't apply to installations on Z systems?

It applies to all platforms. It's confusing because coreos-installer does support installing to devicemapper targets like multipath, so one could turn on multipath during the install phase, but it needs to be clear that they can't use e.g. --append-karg rd.multipath=default to have the first boot be multipath. So it's just easier to simplify it down to "only turn on multipath via MachineConfig".

@SNiemann15
Copy link
Contributor

Do you think the machine config examples should be moved up here? Maybe we continue in this part by saying "multipathing is only supported when activated by applying the following machine config files" and then showing the master and worker node machine configs examples.

Based on this proposed update from @jlebon, it seems like we shouldn't be suggesting the use of kernel arguments at install time and should only be showing how to enable multipathing after installation.
Note that multipathing should only be enabled via kernel arguments within a MachineConfig as documented. Specifically, it shouldn't be enabled at installation time.
Unless this caveat doesn't apply to installations on Z systems?

It applies to all platforms. It's confusing because coreos-installer does support installing to devicemapper targets like multipath, so one could turn on multipath during the install phase, but it needs to be clear that they can't use e.g. --append-karg rd.multipath=default to have the first boot be multipath. So it's just easier to simplify it down to "only turn on multipath via MachineConfig".

If you apply any changes that need to be verified by the IBM Z/P SMEs let me know then I'll get them in the loop.

@openshift-ci-robot openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 30, 2021
@bobfuru bobfuru removed their request for review March 30, 2021 18:33
@bobfuru
Copy link
Contributor

bobfuru commented Apr 20, 2021

Based on this proposed update from @jlebon, it seems like we shouldn't be suggesting the use of kernel arguments at install time and should only be showing how to enable multipathing after installation.
Unless this caveat doesn't apply to installations on Z systems?

It applies to all platforms. It's confusing because coreos-installer does support installing to devicemapper targets like multipath, so one could turn on multipath during the install phase, but it needs to be clear that they can't use e.g. --append-karg rd.multipath=default to have the first boot be multipath. So it's just easier to simplify it down to "only turn on multipath via MachineConfig".

Hi @jlebon - According to @wvoesch, with Z systems, multipathing must be turned on during installation. See his comment. I don't think what you are introducing in this PR is contradicting what he is saying, and it seems that the Z docs - multipathing instructions are fine as they are, but I want to double-check with you both.

Could you please review step 3 - 5th bullet (FCP) to be sure the info is correct as is in the following procedure? https://docs.openshift.com/container-platform/4.7/installing/installing_ibm_z/installing-ibm-z.html#installation-user-infra-machines-iso-ibm-z_installing-ibm-z

@SNiemann15

@bobfuru
Copy link
Contributor

bobfuru commented Apr 20, 2021

@jlebon Apologies for the delay on this. I left a comment/clarification about Z systems but if that seems fine, then this LGTM.

After I merge #31140, we'll have the link to line 101 in this PR and can uncomment by updating to:

For more information, see "Enabling multipathing with kernel arguments on RHCOS" in the xref:../post_installation_configuration/machine-configuration-tasks.adoc#machine-configuration-tasks[Post-installation machine configuration tasks] documentation for more information.

(Unfortunately, we currently have a limitation with linking directly to the multipath module .adoc file.)

Thank you!

Cc: @miabbott

@bobfuru
Copy link
Contributor

bobfuru commented Apr 23, 2021

Hi @jlebon - left one comment about linking to the new multipath docs path. Let me know if you want to update that and then I can get this merged for you. Thanks!

@jlebon
Copy link
Member Author

jlebon commented Apr 27, 2021

Updated this now with a mention of the new page!

@bobfuru
Copy link
Contributor

bobfuru commented Apr 27, 2021

Thank you, @jlebon - I added a closing ==== tag in the note. This LGTM!

PREVIEW LINK: https://deploy-preview-30697--osdocs.netlify.app/openshift-enterprise/latest/release_notes/ocp-4-7-release-notes.html#ocp-4-7-rhcos-supports-multipath

@miabbott please ack if this is lgtm.

@bobfuru bobfuru added the peer-review-done Signifies that the peer review team has reviewed this PR label Apr 27, 2021
Make it clearer that multipathing is only supported when activated using
the documented MachineConfig and that it must not be enabled before
this.
@miabbott
Copy link
Member

miabbott commented May 5, 2021

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 5, 2021
@bobfuru bobfuru merged commit 3844a86 into openshift:enterprise-4.7 May 5, 2021
@openshift-ci-robot
Copy link

@jlebon: Bugzilla bug 1886229 is in an unrecognized state (CLOSED (ERRATA)) and will not be moved to the MODIFIED state.

In response to this:

Bug 1886229: Clarify multipath support in release notes

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch/enterprise-4.7 bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. lgtm Indicates that a PR is ready to be merged. peer-review-done Signifies that the peer review team has reviewed this PR size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants