Test-Layout s390x #483

madhu-pillai · 2023-08-07T16:45:34Z

…ate the layout feature to existing encrypted root partition function. Updated the Documentation updated email address

…ate the layout feature to existing encrypted root partition function. Updated the Documentation

… accomodate the layout feature to existing encrypted root partition function. Updated the Documentation" This reverts commit a054be9. reverts

…mmit and accomodate the layout feature to existing encrypted root partition function. Updated the Documentation"" This reverts commit c3c1000. reverting - test

madhu-pillai

@travier , I am finding difficulty in co-operating statement with s390x layout with eckd and zfcp disk.

prestist

Hey firstly thank you so much for working on this. Could we get a issue link as well attached to your commit.

I took a quick look over the PR and I have a few comments, I will have to revisit this once your commits are cleaned up and are easier to follow. Please be sure to include a issue link as well to help my understanding of these changes.

Notably I noticed your commit messages were a bit lengthy and duplicated. It would help me quite a bit if you could do some fixup commits to your changes so that your commits are more appropriate for the changes they entail and not duplicated.

Additionally I did notice your CI is failing on quite a few things that will need to be looked at.

prestist · 2023-08-09T18:36:43Z

config/fcos/v1_6_exp/translate.go

+
+		switch {
+		case wantMBR && len(sd) != 0:
+			luksDevice = sd + strconv.Itoa(2)


Can we get a comment explaining the luks device value?

Also why strconv.Itoa()? and not "2" and is there a better way so there is not string magic?

Will add the comment explaining the luks device value..

Also why strconv.Itoa()? and not "2" and is there a better way so there is not string magic?
Yes will add, sd + "2"

prestist · 2023-08-09T18:39:17Z

config/fcos/v1_6_exp/schema.go

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

 type BootDeviceLuks struct {


Commit 561ce19 should be amended with a724048

I'll create a new PR with less clutter on spaces and new line. This PR was not supposed to raised, As i need some assistance is some of the logic in coding which i am not able to meet. I'll work upon.

prestist · 2023-08-09T18:42:26Z

config/fcos/v1_6_exp/translate.go

-		return r
+
+	if wantLuksDevice && wantLuks {
+		panic("can't happen")	


This is not a very useful panic. Lets add more detail on why this can't happen.

I'll remove and add another logic to check.

prestist · 2023-08-09T18:43:50Z

config/fcos/v1_6_exp/translate.go

@@ -144,11 +143,11 @@ func (c Config) processBootDevice(config *types.Config, ts *translate.Translatio
 		wantEFIPart = true
 	case *layout == "ppc64le":
 		wantPRePPart = true
-	case *layout == "s390x-zfcp":
+	case *layout == "s390x-zfcp" && wantLuksDevice:


What makes is "&& wantLuksDevice" relevant to the layout?

The intention is as per the design validation in github issue #453 (comment) by Benjamin. The concern is Sometimes user forgot to provide the device like dasda or sd in luks.device and tend to use the label. For that purpose i made a condition. So that User must provide the device corresponding to the layout.

I'll re-write with another logic as if the wantLuksDevice could not found it panics and crashes rather than just giving an error message like device is must for s390x layouts.

prestist · 2023-08-09T18:44:11Z

config/fcos/v1_6_exp/translate.go

 		wantMBR = true
-	case *layout == "s390x-eckd":
+	case *layout == "s390x-eckd" && wantLuksDevice:


prestist · 2023-08-09T18:59:50Z

config/fcos/v1_6_exp/translate.go

 		}
+		// luksDevice := "/dev/disk/by-partlabel/root"


Commented out code is a no no.

Will remove the commented out code.

prestist · 2023-08-09T19:00:04Z

config/fcos/v1_6_exp/translate.go

 	}
-
+    
 	// create root filesystem


Will remove the extra line.

prestist · 2023-08-09T19:01:01Z

config/fcos/v1_6_exp/translate.go

@@ -326,9 +299,6 @@ func (c Config) processBootDevice(config *types.Config, ts *translate.Translatio
 	case wantMirror:
 		// RAID without LUKS
 		rootDevice = "/dev/md/md-root"
-	case wantLuksDevice:


So we added a case and then removed it. This makes reviewing this a bit confusing, this change should have been amended to the originating commit.

I'll make sure that i create a new branch with PR and update without any re-write on existing code.

prestist · 2023-08-09T19:02:23Z

docs/config-fcos-v1_3.md

@@ -198,7 +198,7 @@ The Fedora CoreOS configuration is a YAML document conforming to the following s
    * **_should_exist_** (boolean): whether or not the group with the specified `name` should exist. If omitted, it defaults to true. If false, then Ignition will delete the specified group.
    * **_system_** (boolean): whether or not the group should be a system group. This only has an effect if the group doesn't exist yet.
 * **_boot_device_** (object): describes the desired boot device configuration. At least one of `luks` or `mirror` must be specified.
-  * **_layout_** (string): the disk layout of the target OS image. Supported values are `aarch64`, `ppc64le`, and `x86_64`. Defaults to `x86_64`.
+  * **_layout_** (string): the disk layout of the target OS image. Supported values are `aarch64`, `ppc64le`, `s390x-zfcp`, `s390x-eckd`, `s390x-virt` and `x86_64`. Defaults to `x86_64`.


"aarch64, ppc64le, and x86_64"

ie : "aarch64, ppc64le,s390x-eckd, s390x-virt, s390x-zfcp, and x86_64. Defaults to x86_64".

I think these were alphabetical lets keep it that way :)

Will update that.

prestist · 2023-08-09T19:07:15Z

docs/config-fcos-v1_6-exp.md

  * **_luks_** (object): describes the clevis configuration for encrypting the root filesystem.
    * **_tang_** (list of objects): describes a tang server. Every server must have a unique `url`.
      * **url** (string): url of the tang server.
      * **thumbprint** (string): thumbprint of a trusted signing key.
      * **_advertisement_** (string): the advertisement JSON. If not specified, the advertisement is fetched from the tang server during provisioning.
    * **_tpm2_** (boolean): whether or not to use a tpm2 device.
+    * **device** (string): Specifically for s390x `eckd` and `zfcp` disk without `mirror`.


If "device" is specifically for s390x maybe we can name it more explicitly rather then "device"? wdyt?

how about luks.device-s390x ?

madhu-pillai · 2023-08-10T02:41:06Z

Hi @prestist , Here is the issue link. #453. I'll come back with rest of the correction.

Madhu added 8 commits August 2, 2023 21:38

Added s390x layout feature

561ce19

Added s390x layout feature

a724048

Added the layout specific features for s390x-zfcp, eckd and virt

7065052

Extra functionality has been removed from previous commit and accomod…

a054be9

…ate the layout feature to existing encrypted root partition function. Updated the Documentation updated email address

Extra functionality has been removed from previous commit and accomod…

cf9728d

…ate the layout feature to existing encrypted root partition function. Updated the Documentation

Revert "Extra functionality has been removed from previous commit and…

c3c1000

… accomodate the layout feature to existing encrypted root partition function. Updated the Documentation" This reverts commit a054be9. reverts

Revert "Revert "Extra functionality has been removed from previous co…

58c42a8

…mmit and accomodate the layout feature to existing encrypted root partition function. Updated the Documentation"" This reverts commit c3c1000. reverting - test

Added Mirror Check with Luks Device for s390x layout

83d6f29

madhu-pillai commented Aug 8, 2023

View reviewed changes

madhu-pillai marked this pull request as ready for review August 8, 2023 14:58

prestist reviewed Aug 9, 2023

View reviewed changes

madhu-pillai closed this Aug 12, 2023

madhu-pillai changed the title ~~Layout s390x~~ Test-Layout s390x Aug 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test-Layout s390x #483

Test-Layout s390x #483

madhu-pillai commented Aug 7, 2023 •

edited by travier

Loading

madhu-pillai left a comment

prestist left a comment •

edited

Loading

prestist Aug 9, 2023

prestist Aug 9, 2023

madhu-pillai Aug 10, 2023

prestist Aug 9, 2023

madhu-pillai Aug 10, 2023

prestist Aug 9, 2023

madhu-pillai Aug 10, 2023

prestist Aug 9, 2023

madhu-pillai Aug 10, 2023

prestist Aug 9, 2023

prestist Aug 9, 2023

madhu-pillai Aug 10, 2023

prestist Aug 9, 2023

madhu-pillai Aug 10, 2023

prestist Aug 9, 2023

madhu-pillai Aug 10, 2023

prestist Aug 9, 2023

madhu-pillai Aug 10, 2023

prestist Aug 9, 2023

madhu-pillai Aug 10, 2023

madhu-pillai commented Aug 10, 2023

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

		type BootDeviceLuks struct {

Test-Layout s390x #483

Test-Layout s390x #483

Conversation

madhu-pillai commented Aug 7, 2023 • edited by travier Loading

madhu-pillai left a comment

Choose a reason for hiding this comment

prestist left a comment • 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

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

madhu-pillai commented Aug 10, 2023

madhu-pillai commented Aug 7, 2023 •

edited by travier

Loading

prestist left a comment •

edited

Loading