Skip to content

Conversation

@chrisnegus
Copy link
Contributor

Primarily addresses OSDOCS-1431, but also OSDOCS-1427, OSDOCS-1434, and OSDOCS-1440.

@openshift-ci-robot openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 23, 2020
@openshift-docs-preview-bot

The preview will be available shortly at:

@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 23, 2020
@vikram-redhat vikram-redhat added this to the Future Release milestone Sep 23, 2020
@vikram-redhat vikram-redhat added the peer-review-needed Signifies that the peer review team needs to review this PR label Sep 23, 2020
@vikram-redhat
Copy link
Contributor

@codyhoag to peer review this.

@codyhoag codyhoag self-requested a review September 23, 2020 12:47
@codyhoag
Copy link
Contributor

@chrisnegus a couple high level comments before diving in:

  1. Is this information intended to cover the IPI steps for bare metal? All other installer sections have IPI/UPI docs in separate assemblies, whereas this PR combines them together. Since bare metal is a different beast, wasn't sure if they were combined for a reason? According to Installer team release information, bare metal UPI and IPI (new) are supported for OCP 4.6.
  2. There are several procedures outlined in the new installation-user-infra-machines-advanced.adoc module. Those procedures need to be broken out into separate modules (one procedure per module).

As discussed, I'll dive in deeper after QE/SME reviews. Thanks!

@chrisnegus
Copy link
Contributor Author

@codyhoag I can separate the procedures into different modules. Can you point me to the installer team release information about bare metal UPI and IPI?

@codyhoag
Copy link
Contributor

@miabbott @jlebon @bgilbert can you review? Thanks!

Copy link
Contributor

@bobfuru bobfuru left a comment

Choose a reason for hiding this comment

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

Nicely done! 😎 I agree with @codyhoag that the procedures should be broken into separate modules, so let me know if that's something I can help you with. Most of my comments are either suggested rewording for readability and/or ISG matching, so you can take or leave a lot of it. 👍

@bobfuru bobfuru added peer-review-done Signifies that the peer review team has reviewed this PR and removed peer-review-needed Signifies that the peer review team needs to review this PR labels Sep 25, 2020
Copy link
Contributor

Choose a reason for hiding this comment

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

This whole sentence seems misleading to me. I'm not sure what "identify artifacts to include" means, and the last two items imply a degree of support for partitioning and network setup that might be expected in traditional OS installers, when in fact our support is very limited. Rather than trying to list coreos-installer features here, I'd recast this to just say that you can manually run the installer from a shell prompt, passing it options to configure some details of the installed system.

Copy link
Contributor

Choose a reason for hiding this comment

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

We will probably not have a container for 4.6, but an RPM or coreos-installer binary that can be fetched from somewhere. Details are still being worked out.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 2, 2020
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 2, 2020
@bobfuru bobfuru merged commit ec23526 into openshift:master Oct 2, 2020
@bobfuru
Copy link
Contributor

bobfuru commented Oct 2, 2020

/cherrypick enterprise-4.6

@openshift-cherrypick-robot

@bobfuru: #25709 failed to apply on top of branch "enterprise-4.6":

Applying: Updated bare metal install for coreos-installer
.git/rebase-apply/patch:88: trailing whitespace.
configuration before installing the system. 
.git/rebase-apply/patch:102: trailing whitespace.
 
.git/rebase-apply/patch:106: trailing whitespace.
provide support for installation on disks with 4k sectors. 
.git/rebase-apply/patch:583: trailing whitespace.
. Review the “Advanced {op-system} bare metal installation configuration” 
warning: 4 lines add whitespace errors.
Using index info to reconstruct a base tree...
M	modules/installation-user-infra-machines-pxe.adoc
Falling back to patching base and 3-way merge...
Auto-merging modules/installation-user-infra-machines-pxe.adoc
CONFLICT (content): Merge conflict in modules/installation-user-infra-machines-pxe.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 Updated bare metal install for coreos-installer
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".

Details

In response to this:

/cherrypick enterprise-4.6

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.

@bobfuru
Copy link
Contributor

bobfuru commented Oct 2, 2020

Merged #26091 to fix CP merge conflict.

Copy link
Contributor

@bgilbert bgilbert left a comment

Choose a reason for hiding this comment

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

It looks as though most of the comments on installation-user-infra-machines-advanced.adoc have not been addressed?

which allows you to prepare the permanent system in a variety of ways
before first boot. In particular, you can run the `coreos-installer`
command to identify various artifacts to include, work with disk partitions,
and set up networking. In some cases, you can configure features on
Copy link
Contributor

Choose a reason for hiding this comment

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

This text has also not been updated in a couple places.

@bobfuru
Copy link
Contributor

bobfuru commented Oct 14, 2020

I am hoping that the feedback that was missed in this PR has been addressed now in #26442.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

branch/enterprise-4.6 peer-review-done Signifies that the peer review team has reviewed this PR size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants