fcos: add s390x boot_device sugar #484

madhu-pillai · 2023-08-12T16:21:38Z

Hi,

I've closed the earlier pr 483 due to some errors. Here is the new PR.

As per the GitHub issue [https://github.com//issues/453#issuecomment-1655441505]. The PR contains following conditional changes exclusively for s390x.

Added layout specific to s390x .

s390x-zfcp s390x-eckd and s390x-virt .

Added boot_device.luks.device specifically for s390x-zfcp s390x-eckd and forbidden for other arch including s390x-virt.
The configuration will generate only with boot_device.luks and it fails if mirror boot_device.mirror configuration is specified for s390x-eckd and s390x-zfcp .

prestist

This is a high level code review, I will come back for a more in-depth one once I understand the issue better.

We prob want to change the commit message to be something like

"fcos: add s390x boot_device sugar"

Additionally in the future if working on an issue its customary to address issues raised in the PR rather then closing and re-making them. It makes it more easy to review and know what is changed.

edit: structurally regarding the issue this is attempting to solve I am a bit confused, trying to formulate questions as I go.

config/common/errors.go

prestist · 2023-08-14T18:05:55Z

config/fcos/v1_6_exp/translate.go

@@ -133,6 +135,13 @@ func (c Config) processBootDevice(config *types.Config, ts *translate.Translatio
 		wantEFIPart = true
 	case *layout == "ppc64le":
 		wantPRePPart = true
+	case *layout == "s390x-eckd":


Since s390x-zfcp is the same as s390x-eckd, we could have both cases fall into the same logic
IE

case x: case y: dowork = true

Yes logically it does not do anything with two different layout with one input.
But the plan was user to force use /dev/dasd[a-z] for eckd and /dev/sd[a-z] so that during the layout preference it set appropriate root device; let say for dasd /dev/dasda2. But , I was not able to achieve that under translate.go. I tried with regexp. Somehow it was crashing with memory error. I thought once created the PR will add the device arch once it is discuss within PR.. That's why made it open.

1, imported regexp. 2. then did `diskRe = regexp.MustCompile("(/dev/dasd[a-z]$|/dev/sd[a-z]$)") 3. diskRe.FindString(*c.BootDevice.Luks.Device) while validating with appropriate layouts getting panic on memory error

Can we use the following conditions?

In translate.go will have one logic as you corrected

case s390x-zfcp: case s390x-eckd: wantS390X = true

below condition in validate.go to fix , so that user force to use either dasd[a-z] or sd[a-z]. If not then it give an error.

var dasdRe = regexp.MustCompile("(/dev/dasd[a-z]$)") var sdRe = regexp.MustCompile("(/dev/sd[a-z]$)") if d.Layout != nil && (*d.Layout == "s390x-zfcp" || *d.Layout == "s390x-eckd") { if util.NilOrEmpty(d.Luks.Device) { r.AddOnError(c.Append(*d.Layout), common.ErrNoLuksBootDevice) } if len(d.Mirror.Devices) > 0 { r.AddOnError(c.Append(*d.Layout), common.ErrMirrorNotSupport) } if *d.Layout == "s390x-zfcp" && util.NotEmpty(d.Luks.Device) && !sdRe.MatchString(*d.Luks.Device) { r.AddOnError(c.Append(*d.Layout), common.ErrNoLuksBootDevice) } if *d.Layout == "s390x-eckd" && util.NotEmpty(d.Luks.Device) && !dasdRe.MatchString(*d.Luks.Device) { r.AddOnError(c.Append(*d.Layout), common.ErrNoLuksBootDevice) } }

prestist · 2023-08-14T18:08:18Z

config/fcos/v1_6_exp/translate.go

+		var luksDevice string
+		var device_s390x string
+		switch {
+		case wantMBR:


So what is the difference between the work done in the wantMBR case and the wantDasd case? to my eye they are doing the same thing?

I've explained above...

here we provide only one condition common to both layouts under switch statement.

var device_S390x string switch { case wantS390x: device_s390x = *c.BootDevice.Luks.Device luksDevice = device_s390x + "2"

For more readability I made the changes as follows.

case *layout == "s390x-eckd": wantS390x = true case *layout == "s390x-zfcp": wantS390x = true

// encrypted root partition if wantLuks { var luksDevice string var device_s390x string switch { case wantS390x: //Luks Device for dasd and zFCP-scsi device_s390x = *c.BootDevice.Luks.Device luksDevice = device_s390x + "2"

config/fcos/v1_6_exp/validate.go

docs/config-fcos-v1_6-exp.md

prestist · 2023-08-14T18:28:14Z

config/fcos/v1_6_exp/schema.go

@@ -32,6 +32,7 @@ type BootDevice struct {

 type BootDeviceLuks struct {
 	Discard   *bool       `yaml:"discard"`
+	Device    *string     `yaml:"device-s390x"`


Why are we adding a new field?

Correct me if am wrong.
I added the Device for s390x disk as new field. So that when user generate ignition from butane config they must provide s390x device like /dev/dasda or /dev/sda. Else it use the label which we do not expect for s390x.

madhu-pillai · 2023-08-22T11:40:49Z

Hi @prestist , Can you please review the comments?

config/fcos/v1_6_exp/validate.go

madhu-pillai · 2023-09-20T08:49:47Z

Hi Team,

Would you please check if the patch can be approved? or further review required?

Thanks
Madhu

cgwalters · 2023-09-22T13:43:15Z

I'm not really a maintainer here and I don't know the code involved, but in case it's useful I can say this looks superficially sane to me at least!

cgwalters · 2023-09-22T13:43:36Z

I'd recommend squashing your changes into one commit (and avoid merge commits back from main) etc.

travier · 2023-09-22T14:17:56Z

This will need a full rebase on top of #491 once it lands.

madhu-pillai · 2023-10-09T04:33:46Z

Hi,
Changes have been done as suggested. Kindly review.

nikita-dubrovskii · 2023-10-09T10:37:34Z

@madhu-pillai sorry, by mistake instead of suggestion i've committed a change on top of yours. One question: FCP disks have /dev/sda3 as BOOT and /dev/sda4 as ROOT , so using 2 for partition is only valid for ECKD disks, isn't it?

nikita-dubrovskii · 2023-10-09T11:42:54Z

config/fcos/v1_6_exp/schema.go

@@ -32,6 +32,7 @@ type BootDevice struct {

 type BootDeviceLuks struct {
 	Discard   *bool       `yaml:"discard"`
+	Device    *string     `yaml:"s390x-device"`


Suggested change

Device *string `yaml:"s390x-device"`

Device *string `yaml:"blockdev"`

Is prefix s390x- help in identifying what layout the user trying to use and avoid confusion on device parameter from other arch?

My suggestion is to keep this as device and document that it's only valid on the s390x-eckd and s390x-zfcp layouts. Having the s390x- prefix makes it feel less integrated than other arches.

config/fcos/v1_6_exp/validate.go

nikita-dubrovskii · 2023-10-09T13:52:13Z

docs/config-openshift-v4_15-exp.md

  * **_luks_** (object): describes the clevis configuration for encrypting the root filesystem.
+    * **s390x-device** (string): describes device specific to s390x `dasd[a-z]` or `sd[a-z]`.


Suggested change

* **s390x-device** (string): describes device specific to s390x `dasd[a-z]` or `sd[a-z]`.

* **s390x-device** (string): underlying block device`dasd[a-z]` or `sd[a-z]`.

Initially i was using device naming device however steven perstist reviewed and suggest to prefix s390x -device.

madhu-pillai · 2023-10-10T06:12:50Z

@madhu-pillai sorry, by mistake instead of suggestion i've committed a change on top of yours. One question: FCP disks have /dev/sda3 as BOOT and /dev/sda4 as ROOT , so using 2 for partition is only valid for ECKD disks, isn't it?

That is right... 2 is only valid for dasd disk

madhu-pillai · 2023-10-10T06:15:44Z

Accidently click on close PR. Kindly ignore..

nikita-dubrovskii · 2023-10-12T14:16:23Z

Almost LGTM.
@prestist what's about renaming s390x-device to blockdevice or blockdev? There is only DASD which is s390x specific

madhu-pillai · 2023-10-13T15:30:26Z

Have fixed the doc and translate and validate version check.

madhu-pillai · 2023-10-16T07:15:27Z

Hi, Could you help me how to run the test similar to the CI?
As I am getting different test logs when i run from Mac and fedora?

I just running ./test from the butane directory.

madhu-pillai · 2023-10-17T06:02:46Z

Looks like the version of the fcos and OpenShift causing the failure. Now I have fixed and run the unit test worked Ok.

nikita-dubrovskii · 2023-10-17T12:21:18Z

/cc @prestist

prestist · 2023-10-23T13:29:22Z

Sorry for the delay; I plan on taking a look at this today.

jlebon

Overall approach LGTM. Mostly minor things and one concern re. BIOS/EFI.

jlebon · 2023-10-26T15:35:34Z

config/fcos/v1_6_exp/schema.go

@@ -32,6 +32,7 @@ type BootDevice struct {

 type BootDeviceLuks struct {
 	Discard   *bool       `yaml:"discard"`
+	Device    *string     `yaml:"s390x-device"`


My suggestion is to keep this as device and document that it's only valid on the s390x-eckd and s390x-zfcp layouts. Having the s390x- prefix makes it feel less integrated than other arches.

jlebon · 2023-10-26T15:40:33Z

config/fcos/v1_6_exp/translate.go

+	case *layout == "s390x-virt":
+		wantBIOSPart = true
+		wantEFIPart = true


This doesn't look right; we don't have BIOS or EFI partitions on s390x. Why is this there?

Okay, I think I added this because Benjamin suggested that KVM Virt does not need /dev/vda as /dev/disk/by-partlabel similar to x86. I'll correct by removing this case statement and append following case statement.

case *layout == "s390x-eckd" || *layout == "s390x-zfcp" || *layout == "s390x-virt"

Initially I used device but Steve suggested something specific to s390x that's why I added s390x-device. I'll correct that to device.

I'll correct by removing this case statement and append following case statement.

Yes, that makes sense to me.

Initially I used device but Steve suggested something specific to s390x that's why I added s390x-device. I'll correct that to device.

Yeah, I think it's debatable either way but I would lean towards device. Having to repeat "s390x" in both the layout name and this field's name is a bit awkward.

Yeah; 100% thinking about it again and having talked to @jlebon I agree device is certainly better.

config/common/errors.go

docs/examples.md

jlebon · 2023-10-26T15:59:26Z

docs/examples.md

 ### Mirrored boot disk

+This example uses the shortcut `boot_device` syntax to configure an encrypted root filesystem in s390x KVM unlocked with a network Tang server.


Let's move this example down to be the last of this subsection and keep the more common scenarios as first?

Yes, That is right. I'll move that to the last subsection.

docs/release-notes.md

jlebon · 2023-10-26T16:53:59Z

docs/release-notes.md

@@ -63,6 +64,7 @@ key](https://getfedora.org/security/).
 - Document `key_file` `compression` field  _(openshift 4.8.0 - 4.9.0)_
 - Document support for special mode bits and `arn` URLs _(r4e 1.1.0+)_
 - Improve rendering of spec docs on docs site
+- Document `luks.s390x-device` spec _(fcos, openshift 4.14.0+)_


Not sure if we need this given that it's a new feature. I think this section is more for doc changes for pre-existing features.

Okay, I shall remove this.

jlebon · 2023-10-26T16:54:34Z

internal/doc/butane.yaml

+            - name: s390x-device
+              transforms:
+                - regex: $
+                  replacement: the whole-disk device (not partitions), referenced by their absolute path. One device must be specified with s390x-* layout except s390x-virt.


We should mention the expected prefixes.

Does that mean change to - name: device ?

I mean mention that on s390x-eckd, it should start with /dev/dasd and similarly for the other layout.

But yes, the field name itself will also need to be updated when we rename it.

jlebon

Some minor comments, but LGTM overall!

Were you able to do a final test on this with all three layouts?

internal/doc/butane.yaml

prestist

Thank you @madhu-pillai and @nikita-dubrovskii LGTM; I am deferring my approval to @jlebon's

I have two small nits;

No big deal but would be nice to fix.

Thank you again for all your work.

prestist · 2023-10-31T22:59:58Z

config/fcos/v1_6_exp/translate.go

+	case *layout == "s390x-virt":
+		wantBIOSPart = true
+		wantEFIPart = true


Yeah; 100% thinking about it again and having talked to @jlebon I agree device is certainly better.

docs/release-notes.md

Co-authored-by: Nikita Dubrovski <[email protected]> Co-authored-by: Jonathan Lebon <[email protected]>

jlebon

Just did a small tweak to the transform regex bits.
Nice work! Thanks for working through all the comments.

It depends on fcos 1.6.0-experimental See: coreos#484 See: coreos#514

It depends on fcos 1.6.0-experimental See: coreos#484 See: coreos#514 Fixes: coreos#517

prestist reviewed Aug 14, 2023

View reviewed changes

madhu-pillai changed the title ~~Added s390 layout feature~~ fcos: add s390x boot_device sugar Aug 15, 2023

nikita-dubrovskii reviewed Sep 5, 2023

View reviewed changes

config/fcos/v1_6_exp/validate.go Outdated Show resolved Hide resolved

config/fcos/v1_6_exp/validate.go Outdated Show resolved Hide resolved

config/fcos/v1_6_exp/validate.go Outdated Show resolved Hide resolved

madhu-pillai force-pushed the s390x-layout branch from 7662ec6 to 4ac37f1 Compare October 3, 2023 05:51

nikita-dubrovskii reviewed Oct 9, 2023

View reviewed changes

config/fcos/v1_6_exp/validate.go Outdated Show resolved Hide resolved

nikita-dubrovskii reviewed Oct 9, 2023

View reviewed changes

madhu-pillai closed this Oct 10, 2023

madhu-pillai reopened this Oct 10, 2023

madhu-pillai force-pushed the s390x-layout branch from 3805b29 to ae67e22 Compare October 11, 2023 05:44

madhu-pillai force-pushed the s390x-layout branch from ae67e22 to 6a4a8b0 Compare October 13, 2023 15:19

madhu-pillai force-pushed the s390x-layout branch from 6a4a8b0 to 7f9b2c2 Compare October 17, 2023 06:01

nikita-dubrovskii approved these changes Oct 17, 2023

View reviewed changes

jlebon reviewed Oct 26, 2023

View reviewed changes

madhu-pillai force-pushed the s390x-layout branch from 02914a5 to 6c80516 Compare October 31, 2023 18:08

jlebon approved these changes Oct 31, 2023

View reviewed changes

internal/doc/butane.yaml Outdated Show resolved Hide resolved

internal/doc/butane.yaml Show resolved Hide resolved

prestist reviewed Oct 31, 2023

View reviewed changes

madhu-pillai force-pushed the s390x-layout branch from 4503eb1 to f8338bd Compare November 2, 2023 07:49

spec/v1.6: Add support for boot_device sugar on s390x

0d2fa0f

Co-authored-by: Nikita Dubrovski <[email protected]> Co-authored-by: Jonathan Lebon <[email protected]>

jlebon force-pushed the s390x-layout branch from f8338bd to 0d2fa0f Compare November 2, 2023 16:02

jlebon approved these changes Nov 2, 2023

View reviewed changes

jlebon enabled auto-merge November 2, 2023 16:03

jlebon merged commit 3c0589f into coreos:main Nov 2, 2023
8 checks passed

travier added a commit to travier/butane that referenced this pull request Feb 20, 2024

docs: Move s390x boot_devices sugar to ocp 4.16.0-experimental

7b94e53

It depends on fcos 1.6.0-experimental See: coreos#484 See: coreos#514

travier added a commit to travier/butane that referenced this pull request Feb 20, 2024

docs: Move s390x boot_devices sugar to ocp 4.16.0-experimental

8ff12e2

It depends on fcos 1.6.0-experimental See: coreos#484 See: coreos#514 Fixes: coreos#517

travier mentioned this pull request Feb 20, 2024

docs: Move s390x boot_devices sugar to ocp 4.16.0-experimental #518

Merged

yasminvalim pushed a commit to yasminvalim/butane that referenced this pull request Mar 15, 2024

docs: Move s390x boot_devices sugar to ocp 4.16.0-experimental

ad0fd81

It depends on fcos 1.6.0-experimental See: coreos#484 See: coreos#514 Fixes: coreos#517

	Device *string `yaml:"s390x-device"`
	Device *string `yaml:"blockdev"`

		* _luks_ (object): describes the clevis configuration for encrypting the root filesystem.
		* s390x-device (string): describes device specific to s390x `dasd[a-z]` or `sd[a-z]`.

	* s390x-device (string): describes device specific to s390x `dasd[a-z]` or `sd[a-z]`.
	* s390x-device (string): underlying block device`dasd[a-z]` or `sd[a-z]`.

		### Mirrored boot disk

		This example uses the shortcut `boot_device` syntax to configure an encrypted root filesystem in s390x KVM unlocked with a network Tang server.

fcos: add s390x boot_device sugar #484

fcos: add s390x boot_device sugar #484

Conversation

madhu-pillai commented Aug 12, 2023

prestist left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madhu-pillai Aug 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madhu-pillai commented Aug 22, 2023

madhu-pillai commented Sep 20, 2023

cgwalters commented Sep 22, 2023

cgwalters commented Sep 22, 2023

travier commented Sep 22, 2023

madhu-pillai commented Oct 9, 2023

nikita-dubrovskii commented Oct 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madhu-pillai commented Oct 10, 2023

madhu-pillai commented Oct 10, 2023

nikita-dubrovskii commented Oct 12, 2023

madhu-pillai commented Oct 13, 2023

madhu-pillai commented Oct 16, 2023

madhu-pillai commented Oct 17, 2023

nikita-dubrovskii commented Oct 17, 2023

prestist commented Oct 23, 2023

jlebon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlebon left a comment

Choose a reason for hiding this comment

prestist left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlebon left a comment

Choose a reason for hiding this comment

prestist left a comment •

edited

Loading

madhu-pillai Aug 15, 2023 •

edited

Loading