-
Notifications
You must be signed in to change notification settings - Fork 70
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
Test-Layout s390x #483
Test-Layout s390x #483
Changes from 1 commit
561ce19
a724048
7065052
a054be9
cf9728d
c3c1000
58c42a8
83d6f29
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,7 +32,7 @@ type BootDevice struct { | |
|
||
type BootDeviceLuks struct { | ||
Discard *bool `yaml:"discard"` | ||
Device string `yaml:"device"` | ||
Device string `yaml:"device"` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit:This change should have been part of the originating commit. |
||
Tang []base.Tang `yaml:"tang"` | ||
Threshold *int `yaml:"threshold"` | ||
Tpm2 *bool `yaml:"tpm2"` | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -117,17 +117,12 @@ func (c Config) processBootDevice(config *types.Config, ts *translate.Translatio | |
|
||
// check for high-level features | ||
wantLuks := util.IsTrue(c.BootDevice.Luks.Tpm2) || len(c.BootDevice.Luks.Tang) > 0 | ||
wantLuksDevice := len(c.BootDevice.Luks.Device) > 0 | ||
wantMirror := len(c.BootDevice.Mirror.Devices) > 0 | ||
|
||
if !wantLuks && !wantMirror { | ||
return r | ||
} | ||
|
||
if wantLuksDevice && wantLuks { | ||
panic("can't happen") | ||
} | ||
|
||
// compute layout rendering options | ||
var wantBIOSPart bool | ||
var wantEFIPart bool | ||
|
@@ -143,11 +138,11 @@ func (c Config) processBootDevice(config *types.Config, ts *translate.Translatio | |
wantEFIPart = true | ||
case *layout == "ppc64le": | ||
wantPRePPart = true | ||
case *layout == "s390x-zfcp" && wantLuksDevice: | ||
case *layout == "s390x-zfcp" && !wantMirror: | ||
wantMBR = true | ||
case *layout == "s390x-eckd" && wantLuksDevice: | ||
case *layout == "s390x-eckd" && !wantMirror: | ||
wantDasd = true | ||
case *layout == "s390x-virt" && !wantLuksDevice: | ||
case *layout == "s390x-virt": | ||
wantBIOSPart = true | ||
wantEFIPart = true | ||
default: | ||
|
@@ -254,35 +249,8 @@ func (c Config) processBootDevice(config *types.Config, ts *translate.Translatio | |
rendered.Storage.Filesystems = append(rendered.Storage.Filesystems, bootFilesystem) | ||
} | ||
|
||
// encrypted root partition | ||
//encrypted root partition | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: why did we remove a space in the comment it makes it less readable. |
||
if wantLuks { | ||
luksDevice := "/dev/disk/by-partlabel/root" | ||
if wantMirror { | ||
luksDevice = "/dev/md/md-root" | ||
} | ||
clevis, ts2, r2 := translateBootDeviceLuks(c.BootDevice.Luks, options) | ||
rendered.Storage.Luks = []types.Luks{{ | ||
Clevis: clevis, | ||
Device: &luksDevice, | ||
Discard: c.BootDevice.Luks.Discard, | ||
Label: util.StrToPtr("luks-root"), | ||
Name: "root", | ||
WipeVolume: util.BoolToPtr(true), | ||
}} | ||
lpath := path.New("yaml", "boot_device", "luks") | ||
rpath := path.New("json", "storage", "luks", 0) | ||
renderedTranslations.Merge(ts2.PrefixPaths(lpath, rpath.Append("clevis"))) | ||
renderedTranslations.AddTranslation(lpath.Append("discard"), rpath.Append("discard")) | ||
for _, f := range []string{"device", "label", "name", "wipeVolume"} { | ||
renderedTranslations.AddTranslation(lpath, rpath.Append(f)) | ||
} | ||
renderedTranslations.AddTranslation(lpath, rpath) | ||
renderedTranslations.AddTranslation(lpath, path.New("json", "storage", "luks")) | ||
r.Merge(r2) | ||
} | ||
|
||
//encrypted root partition for s390x | ||
if wantMBR || wantDasd { | ||
var luksDevice string | ||
dasd := dasdRe.FindString(c.BootDevice.Luks.Device) | ||
sd := sdRe.FindString(c.BootDevice.Luks.Device) | ||
|
@@ -292,9 +260,15 @@ func (c Config) processBootDevice(config *types.Config, ts *translate.Translatio | |
luksDevice = sd + strconv.Itoa(2) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we get a comment explaining the luks device value? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also why strconv.Itoa()? and not "2" and is there a better way so there is not string magic? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
case wantDasd && len(dasd) != 0: | ||
luksDevice = dasd + strconv.Itoa(2) | ||
case wantMirror: | ||
luksDevice = "/dev/md/md-root" | ||
default: | ||
panic("Incorrect Device Parameter") | ||
luksDevice = "/dev/disk/by-partlabel/root" | ||
} | ||
// luksDevice := "/dev/disk/by-partlabel/root" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Commented out code is a no no. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will remove the commented out code. |
||
// if wantMirror { | ||
// luksDevice = "/dev/md/md-root" | ||
// } | ||
clevis, ts2, r2 := translateBootDeviceLuks(c.BootDevice.Luks, options) | ||
rendered.Storage.Luks = []types.Luks{{ | ||
Clevis: clevis, | ||
|
@@ -314,9 +288,8 @@ func (c Config) processBootDevice(config *types.Config, ts *translate.Translatio | |
renderedTranslations.AddTranslation(lpath, rpath) | ||
renderedTranslations.AddTranslation(lpath, path.New("json", "storage", "luks")) | ||
r.Merge(r2) | ||
|
||
} | ||
|
||
// create root filesystem | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. see above. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will remove the extra line. |
||
var rootDevice string | ||
switch { | ||
|
@@ -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: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll make sure that i create a new branch with PR and update without any re-write on existing code. |
||
//Only Luks for s390x | ||
rootDevice = "/dev/mapper/root" | ||
default: | ||
panic("can't happen") | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: White space after '}' |
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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`. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "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 :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will update that. |
||
* **_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. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -209,13 +209,14 @@ The Fedora CoreOS configuration is a YAML document conforming to the following s | |
* **_should_exist_** (list of strings): the list of kernel arguments that should exist. | ||
* **_should_not_exist_** (list of strings): the list of kernel arguments that should not exist. | ||
* **_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`. | ||
* **_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`. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If "device" is specifically for s390x maybe we can name it more explicitly rather then "device"? wdyt? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how about |
||
* **_threshold_** (integer): sets the minimum number of pieces required to decrypt the device. Default is 1. | ||
* **_discard_** (boolean): whether to issue discard commands to the underlying block device when blocks are freed. Enabling this improves performance and device longevity on SSDs and space utilization on thinly provisioned SAN devices, but leaks information about which disk blocks contain data. If omitted, it defaults to false. | ||
* **_mirror_** (object): describes mirroring of the boot disk for fault tolerance. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commit 561ce19 should be amended with a724048
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.