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

[MULTIARCH-3164] add NBDE encryption for IBM Z #58373

Merged
merged 2 commits into from
May 15, 2023

Conversation

SNiemann15
Copy link
Contributor

@SNiemann15 SNiemann15 commented Apr 6, 2023

Version(s): 4.13 +

Issue: https://issues.redhat.com/browse/MULTIARCH-3164

Link to docs preview:

QE review:

  • QE has approved this change.

Additional information:

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 6, 2023
Copy link

@techrustlings techrustlings left a comment

Choose a reason for hiding this comment

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

Done with the review.

====
endif::ibm-z-kvm[]

. Create a parameter file that includes `ignition.platform.id=metal` and `ignition.firstboot=true`.

Choose a reason for hiding this comment

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

kindly replace ignition.firstboot=true to only ignition.firstboot.

+
[source,terminal]
----
$ coreos-installer pxe customize rhcos-<version>-live-initramfs.x86_64.img \

Choose a reason for hiding this comment

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

rhcos-<version>-live-initramfs.x86_64.img is not required. as we already providing the path to the initramfs in the next line 68.

+
[source,terminal]
----
$ coreos-installer pxe customize rhcos-<version>-live-initramfs.x86_64.img \

Choose a reason for hiding this comment

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

rhcos--live-initramfs.x86_64.img need removal as we already providing the initramfs file location on line number 68.


. Create a parameter file that includes `ignition.platform.id=metal` and `ignition.firstboot=true`.
+
The following example kernel parameter utane configuration for a control plane node creates a file named `master-storage.bu` for disk encryption:ecify the matching rootfs artifact for the kernel and initramfs you are booting. Only HTTP and HTTPS protocols are supported.

Choose a reason for hiding this comment

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

spell error replace utane to butane and encryption:ecify

@openshift-ci openshift-ci bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 7, 2023
@ocpdocs-previewbot
Copy link

ocpdocs-previewbot commented Apr 11, 2023

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

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

@openshift-ci openshift-ci bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 12, 2023
tang:
- url: http://12.23.21.58:7500
thumbprint: QcPr_NHFJammnRCA3fFMVdNBwjs
threshold: 1
Copy link
Contributor

Choose a reason for hiding this comment

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

The threshold defaults to 1, so this could be omitted.

+
[NOTE]
====
To encrypt DASD disks you must add `device: /dev/disk/by-label/root` to the Ignition file that is generated by Butane.
Copy link
Contributor

Choose a reason for hiding this comment

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

This one line isn't enough detail; we should give step-by-step instructions.

But, stepping back a bit: please don't ask users to manually modify Butane output. It's not how Butane is supposed to be used. Also, there's no guarantee that Butane's x86_64 template will meet the needs of s390x in the future, since s390x considerations are out of scope there. Instead, please use a Butane config that describes the necessary configuration in the storage section, avoiding boot_device altogether. See coreos/butane#453 for more info.

----
rd.neednet=1 \
console=ttysclp0 \
coreos.inst.install_dev=dasda \ <1>
Copy link
Contributor

Choose a reason for hiding this comment

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

Omitting /dev/ is legacy syntax.

Suggested change
coreos.inst.install_dev=dasda \ <1>
coreos.inst.install_dev=/dev/dasda \ <1>

rd.neednet=1 \
console=ttysclp0 \
coreos.inst.install_dev=dasda \ <1>
ignition.firstboot=true ignition.platform.id=metal \
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
ignition.firstboot=true ignition.platform.id=metal \
ignition.firstboot ignition.platform.id=metal \

Copy link

@techrustlings techrustlings left a comment

Choose a reason for hiding this comment

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

Updated the configuration as suggested by Benjamin.

+
[source,yaml]
----
variant: openshift
Copy link

@techrustlings techrustlings Apr 17, 2023

Choose a reason for hiding this comment

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

The shortcut method works with s390x zfcp, but not works with dasd. So editing the ignition is not recommended. Define the storage configuration directly in the butane. Kindly remove the line from 41 to 52 and add the following config as example.

variant: openshift
version: 4.13.0
metadata:
  name: master-storage
  labels:
    machineconfiguration.openshift.io/role: master
storage:
  filesystems:
    - device: /dev/mapper/root
      format: xfs
      label: root
      wipe_filesystem: true
  luks:
    - clevis:
        tang:
          - thumbprint: QcPr_NHFJammnRCA3fFMVdNBwjs
            url: http://12.23.21.58:7500
      device: /dev/disk/by-partlabel/root
      label: luks-root
      name: root
      wipe_volume: true

+
[NOTE]
====
To encrypt DASD disks you must add `device: /dev/disk/by-label/root` to the Ignition file that is generated by Butane.

Choose a reason for hiding this comment

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

Replace line 60 To encrypt DASD disks you must add device: /dev/disk/by-label/root to the Ignition file that is generated by Butane. <- To -> To encrypt DASD disks you must add device: /dev/disk/by-label/root to the butane file

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems clearer to make this a footnote on the relevant Butane config line.

rd.znet=qeth,0.0.bdd0,0.0.bdd1,0.0.bdd2,layer2=1 \
rd.zfcp=0.0.5677,0x600606680g7f0056,0x034F000000000000
----
<1> For installations on DASD-type disks, add `coreos.inst.install_dev=dasda`. Omit this value for FCP-type disks.

Choose a reason for hiding this comment

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

Kindly replace coreos.inst.install_dev=dasda to coreos.inst.install_dev=/dev/dasda

----
rd.neednet=1 \
console=ttysclp0 \
ignition.firstboot=true ignition.platform.id=metal \

Choose a reason for hiding this comment

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

ignition.firstboot=true to ignition.firstboot

@SNiemann15 SNiemann15 force-pushed the ibmz_nbde branch 2 times, most recently from 9c0d889 to 1200491 Compare May 4, 2023 13:24
@@ -55,7 +55,7 @@ $ virt-install \
--network network={virt_network_parm} \
--boot hd \
--location {media_location},kernel={rhcos_kernel},initrd={rhcos_initrd} \
--extra-args "rd.neednet=1 coreos.inst=yes coreos.inst.install_dev=vda coreos.live.rootfs_url={rhcos_liveos} ip={ip}::{default_gateway}:{subnet_mask_length}:{vn_name}:enc1:none:{MTU} nameserver={dns} coreos.inst.ignition_url={rhcos_ign}" \
--extra-args "rd.neednet=1 coreos.inst=yes coreos.inst.install_dev=/dev/vda coreos.live.rootfs_url={rhcos_liveos} ip={ip}::{default_gateway}:{subnet_mask_length}:{vn_name}:enc1:none:{MTU} nameserver={dns} coreos.inst.ignition_url={rhcos_ign}" \
Copy link
Contributor

Choose a reason for hiding this comment

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

coreos.inst=yes is obsolete.

Suggested change
--extra-args "rd.neednet=1 coreos.inst=yes coreos.inst.install_dev=/dev/vda coreos.live.rootfs_url={rhcos_liveos} ip={ip}::{default_gateway}:{subnet_mask_length}:{vn_name}:enc1:none:{MTU} nameserver={dns} coreos.inst.ignition_url={rhcos_ign}" \
--extra-args "rd.neednet=1 coreos.inst.install_dev=/dev/vda coreos.live.rootfs_url={rhcos_liveos} ip={ip}::{default_gateway}:{subnet_mask_length}:{vn_name}:enc1:none:{MTU} nameserver={dns} coreos.inst.ignition_url={rhcos_ign}" \

ifndef::ibm-z-kvm[]
<1> For installations on DASD-type disks, replace with `device: /dev/disk/by-label/root`.
endif::ibm-z-kvm[]

Copy link
Contributor

Choose a reason for hiding this comment

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

The instructions don't say what to do with the Butane config once created. I assume we want to render it to a MachineConfig and add that to the install templates? We usually give explicit steps for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have a link to Creating machine configs with Butane in the additional resources.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, but I think there's a risk that users will follow the steps verbatim and won't realize that additional actions are implied.

Choose a reason for hiding this comment

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

Just below the line no 71 a Note section will remind the user to follow the Adding day 1 kernel arguments. Like below .

Generate the MachineConfig object from master-storage.bu by using butane and place it in the Openshift installation directory as described in "Adding day-1 kernel arguments"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lets tackle this issue after 4.13 GA.

name: root
wipe_volume: true
openshift:
fips: true
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we requiring FIPS mode? If not, we should footnote that this is optional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not required will remove the line.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since this has been reinstated, would it make sense to footnote it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it makes sense. I added the callout text we use in the sample install-config.yaml for FIPS

label: root
wipe_filesystem: true
luks:
- clevis:
Copy link
Contributor

Choose a reason for hiding this comment

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

In the FIPS case, the luks entry should also include:

options:
  - --cipher
  - aes-cbc-essiv:sha256

We need this to ensure that we're using a FIPS 140-2 compatible cipher mode. The default cipher mode is too new for 140-2 (but is supported in 140-3). Normally Butane handles this if it sees boot_device.luks and also openshift.fips: true, but since we're not using boot_device we need to do this ourselves.

I'd be inclined to footnote it and say that it's only needed in FIPS mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not needed because I removed FIPS entry.

Choose a reason for hiding this comment

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

@SNiemann15 the comment from @bgilbert catched my eyes.
in case a customer want to have fips mode & a change for LUKS is required to adjust the cipher, we might want to document that. Because encryption is kind of base for compliance rules which also includes FIPS.

----
$ coreos-installer pxe customize \
/root/rhcos-bootfiles/rhcos-<release>-live-initramfs.s390x.img \
--dest-device /dev/sda --dest-karg-append \
Copy link
Contributor

Choose a reason for hiding this comment

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

--dest-device does the same thing as coreos.inst.install_dev in the next step. I think it's better to only use --dest-device, and footnote the different values that should be used for virt/ECKD/FCP.

Choose a reason for hiding this comment

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

We do not require --dest-device in initramfs file for Dasda configuration. It has been mentioned in the call out section just below the example.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm suggesting that the instructions either consistently use only --dest-device or consistently use only coreos.inst.install_dev, rather than sometimes using both and having the latter override the former.

Choose a reason for hiding this comment

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

We used --dest-device in coreos-installer command, and the coreos.inst.install_dev we used in the kernel param file which we punch during the installation. Are you suggesting we use --dest-device in kernel param file too?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I'm suggesting that we either use --dest-device in the coreos-installer pxe customize command or coreos.inst.install_dev in the kernel param file. It's unnecessary and confusing to use both.

- clevis:
tang:
- thumbprint: QcPr_NHFJammnRCA3fFMVdNBwjs
url: http://12.23.21.58:7500
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be clearer to use e.g. http://clevis.example.com/ to show that this shouldn't be used as-is.

Choose a reason for hiding this comment

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

labels:
machineconfiguration.openshift.io/role: master
storage:
filesystems:
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Butane doesn't care either way, but I'd be inclined to put this section after the luks section to make the execution order clearer.

Choose a reason for hiding this comment

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

variant: openshift
version: 4.13.0
metadata:
name: master-storage
labels:
machineconfiguration.openshift.io/role: master
storage:
luks:
- clevis:
tang:
- thumbprint: QcPr_NHFJammnRCA3fFMVdNBwjs
url: http://clevis.example.com:7500
device: /dev/disk/by-partlabel/root
label: luks-root
name: root
wipe_volume: true
filesystems:
- device: /dev/mapper/root
format: xfs
label: root
wipe_filesystem: true

fips: true
----
ifndef::ibm-z-kvm[]
<1> For installations on DASD-type disks, replace with `device: /dev/disk/by-label/root`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Using /dev/disk/by-label/root rather than e.g. /dev/dasda3 (where the user has to fill in the correct disk instead of /dev/dasda) is a bit of a hack, but it does simplify the docs, and I think I'm okay with it. If we're using this approach, maybe it's worth simplifying by using /dev/disk/by-label/root for the virt case too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Any thoughts on doing this?

Choose a reason for hiding this comment

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

Hi @bgilbert , /dev/disk/by-partlabel/root works with KVM too. During the testing on KVM and zVM the configuration are similar. I did not done explicit test using /dev/disk/by-label/root in virtual (kvm) and zVM case because on the first shot it worked.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm suggesting that, if we're going to use the /dev/disk/by-label/root hack, we might as well use it in all cases to avoid the footnote and additional explanation. It should work equally well everywhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

To be clear, this is a cleanup, not a blocker.

+
[NOTE]
====
Before first boot, you must customize the initramfs for each node in the cluster and add PXE kernel parameters.
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, this note confuses me. Why is it only shown in the non-KVM case, and what does it add that the rest of the doc hasn't already said?

Choose a reason for hiding this comment

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

We need it KVM case as well.

<1> For installations on DASD-type disks, replace with `device: /dev/disk/by-label/root`.
endif::ibm-z-kvm[]

. Create a customized initramfs file, by running the following command:
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't explicitly say that the customized initramfs should be used instead of the default initramfs when booting the machine. Should we?

Choose a reason for hiding this comment

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

We do not need to call that out explicitly.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that leaves users to fill in the gaps, and there's little harm in calling it out explicitly.

Choose a reason for hiding this comment

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

Ok, I think we are already providing the Note just below the command. So explicitly calling "customised" may not be relevant.

@SNiemann15 SNiemann15 changed the title WIP [MULTIARCH-3164] add NBDE encryption for IBM Z [MULTIARCH-3164] add NBDE encryption for IBM Z May 8, 2023
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 8, 2023
- clevis:
tang:
- thumbprint: QcPr_NHFJammnRCA3fFMVdNBwjs
url: http://<clevis.example.com>:7500
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, the angle brackets read as slightly confusing to me.

Choose a reason for hiding this comment

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

We need to remove angle bracket..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok will remove. Usually variables that get replaced have those brackets. But I also thought looks strange in this case.

fips: true
----
ifndef::ibm-z-kvm[]
<1> For installations on DASD-type disks, replace with `device: /dev/disk/by-label/root`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Any thoughts on doing this?

<1> For installations on DASD-type disks, replace with `device: /dev/disk/by-label/root`.
endif::ibm-z-kvm[]

. Create a customized initramfs file, by running the following command:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that leaves users to fill in the gaps, and there's little harm in calling it out explicitly.

@SNiemann15 SNiemann15 force-pushed the ibmz_nbde branch 2 times, most recently from 840311f to 1d9cfb3 Compare May 10, 2023 07:05
@SNiemann15
Copy link
Contributor Author

/label peer-review-needed

QE review is still in progress.

@openshift-ci openshift-ci bot added the peer-review-needed Signifies that the peer review team needs to review this PR label May 10, 2023
@openshift-ci
Copy link

openshift-ci bot commented May 10, 2023

@ocpdocs-previewbot: user ocpdocs-previewbot is not trusted for pull request #58373

@madhu-pillai
Copy link

Looks good to me.

Copy link
Contributor

@snarayan-redhat snarayan-redhat left a comment

Choose a reason for hiding this comment

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

Overall looks good! Few suggestions and nits.


.Prerequisites

* You set up the External Tang Server. See link:https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/8/html/security_hardening/configuring-automated-unlocking-of-encrypted-volumes-using-policy-based-decryption_security-hardening#network-bound-disk-encryption_configuring-automated-unlocking-of-encrypted-volumes-using-policy-based-decryption[Network-bound disk encryption] for instructions.
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
* You set up the External Tang Server. See link:https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/8/html/security_hardening/configuring-automated-unlocking-of-encrypted-volumes-using-policy-based-decryption_security-hardening#network-bound-disk-encryption_configuring-automated-unlocking-of-encrypted-volumes-using-policy-based-decryption[Network-bound disk encryption] for instructions.
* You have set up the External Tang Server. See link:https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/8/html/security_hardening/configuring-automated-unlocking-of-encrypted-volumes-using-policy-based-decryption_security-hardening#network-bound-disk-encryption_configuring-automated-unlocking-of-encrypted-volumes-using-policy-based-decryption[Network-bound disk encryption] for instructions.


. Create Butane config files for the control plane and compute nodes.
+
The following example Butane configuration for a control plane node creates a file named `master-storage.bu` for disk encryption:
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 following example Butane configuration for a control plane node creates a file named `master-storage.bu` for disk encryption:
The following example of Butane configuration for a control plane node creates a file named `master-storage.bu` for disk encryption:


.Procedure

. Create Butane config files for the control plane and compute nodes.
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
. Create Butane config files for the control plane and compute nodes.
. Create Butane configuration files for the control plane and compute nodes.

+
[NOTE]
====
Before first boot, you must customize the initramfs for each node in the cluster and add PXE kernel parameters.
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
Before first boot, you must customize the initramfs for each node in the cluster and add PXE kernel parameters.
Before first boot, you must customize the initramfs for each node in the cluster, and add PXE kernel parameters.

Comment on lines +134 to +142
rd.neednet=1 \
console=ttysclp0 \
ignition.firstboot ignition.platform.id=metal \
coreos.live.rootfs_url=http://10.19.17.25/redhat/ocp/rhcos-413.86.202302201445-0/rhcos-413.86.202302201445-0-live-rootfs.s390x.img \
coreos.inst.ignition_url=http://bastion.ocp-cluster1.example.com:8080/ignition/master.ign \
ip=10.19.17.2::10.19.17.1:255.255.255.0::enbdd0:none nameserver=10.19.17.1 \
zfcp.allow_lun_scan=0 \
rd.znet=qeth,0.0.bdd0,0.0.bdd1,0.0.bdd2,layer2=1 \
rd.zfcp=0.0.5677,0x600606680g7f0056,0x034F000000000000
Copy link
Contributor

Choose a reason for hiding this comment

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

Just mentioning that this block doesn't have callouts unlike the previous ifndef::ibm-z-kvm[] block, in case you think it's a miss. If it is intentional, then great.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Intentional :-)


. Create a parameter file that includes `ignition.platform.id=metal` and `ignition.firstboot`.
+
Example kernel parameter file for the control plane machine:
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
Example kernel parameter file for the control plane machine:
.Example kernel parameter file for the control plane machine:


. Create a parameter file that includes `ignition.platform.id=metal` and `ignition.firstboot`.
+
Example kernel parameter file for the control plane machine:
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
Example kernel parameter file for the control plane machine:
.Example kernel parameter file for the control plane machine:

----
endif::ibm-z-kvm[]
+
Write all options in the parameter file as a single line and make sure you have no newline characters.
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be a note or added to the step above. I would prefer a note consideirng the structure and reowrding it to be something like:
Ensure that all options in the parameter file are in a single line, and have no newline characters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would like to keep this active voice. But having it as a note is a good suggestion.

@snarayan-redhat snarayan-redhat 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 May 11, 2023
@SNiemann15
Copy link
Contributor Author

/label merge-review-needed

@openshift-ci openshift-ci bot added the merge-review-needed Signifies that the merge review team needs to review this PR label May 11, 2023
@adellape adellape added the merge-review-in-progress Signifies that the merge review team is reviewing this PR label May 11, 2023
@adellape
Copy link
Contributor

@SNiemann15 Please squash to 1 commit and re-queue for merge review. Also, I couldn't tell per the earlier comment whether QE review was completed or not at this point?

Thank you!

@adellape adellape removed merge-review-in-progress Signifies that the merge review team is reviewing this PR merge-review-needed Signifies that the merge review team needs to review this PR labels May 11, 2023
@SNiemann15
Copy link
Contributor Author

@SNiemann15 Please squash to 1 commit and re-queue for merge review. Also, I couldn't tell per the earlier comment whether QE review was completed or not at this point?

Thank you!

@adellape Really strange I squashed yesterday I assume the squash got stuck in the github issues.
Yes QE review is completed in the meantime. Both Holger and Benjamin ack'd.

@SNiemann15
Copy link
Contributor Author

/label merge-review-needed

@openshift-ci openshift-ci bot added the merge-review-needed Signifies that the merge review team needs to review this PR label May 12, 2023
@bscott-rh bscott-rh added the merge-review-in-progress Signifies that the merge review team is reviewing this PR label May 15, 2023
Comment on lines +92 to +99
$ coreos-installer pxe customize \
/root/rhcos-bootfiles/rhcos-<release>-live-initramfs.s390x.img \
--dest-device /dev/sda --dest-karg-append \
ip=<ip-address>::<gateway-ip>:<subnet-mask>::<network-device>:none \
--dest-karg-append nameserver=<nameserver-ip> \
--dest-karg-append rd.neednet=1 -o \
/root/rhcos-bootfiles/<Node-name>-initramfs.s390x.img
----
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not going to hold up the PR for this, but I would strongly recommend coming back later and adding annotations/callouts to describe the user-replaced values in this block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll pick that up in the follow-up PR we are planning post GA.

[id="additional-resources_configure-nbde-ibm-z-kvm"]
.Additional resources

* xref:../../installing/install_config/installing-customizing.adoc#installing-customizing[Creating machine configs with Butane].
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
* xref:../../installing/install_config/installing-customizing.adoc#installing-customizing[Creating machine configs with Butane].
* xref:../../installing/install_config/installing-customizing.adoc#installation-special-config-butane_installing-customizing[Creating machine configs with Butane]

Please remove the period from end of the additional resources xrefs in these 4 assemblies as they are not complete sentences. I also updated the xref anchor to go directly to the "Creating machine configs with Butane" section which could be updated in all 4 assemblies. Thank you!

@bscott-rh bscott-rh added this to the Planned for 4.13 GA milestone May 15, 2023
@bscott-rh bscott-rh removed merge-review-in-progress Signifies that the merge review team is reviewing this PR merge-review-needed Signifies that the merge review team needs to review this PR labels May 15, 2023
@bscott-rh bscott-rh merged commit e38fb5e into openshift:main May 15, 2023
@bscott-rh
Copy link
Contributor

/cherrypick enterprise-4.13

@openshift-cherrypick-robot

@bscott-rh: new pull request created: #60007

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.

@bscott-rh
Copy link
Contributor

Merged and cherrypicked to enterprise-4.13.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch/enterprise-4.13 peer-review-done Signifies that the peer review team has reviewed 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.

10 participants