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.

Sign up for GitHub

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

Jump to bottom

Test-Layout s390x #483

Closed
wants to merge 8 commits into from
Closed

Test-Layout s390x #483

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
561ce19
Added s390x layout feature
Aug 2, 2023
a724048
Added s390x layout feature
Aug 2, 2023
7065052
Added the layout specific features for s390x-zfcp, eckd and virt
Aug 4, 2023
a054be9
Extra functionality has been removed from previous commit and accomod…
Aug 4, 2023
cf9728d
Extra functionality has been removed from previous commit and accomod…
Aug 5, 2023
c3c1000
Revert "Extra functionality has been removed from previous commit and…
Aug 5, 2023
58c42a8
Revert "Revert "Extra functionality has been removed from previous co…
Aug 5, 2023
83d6f29
Added Mirror Check with Luks Device for s390x layout
Aug 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion config/common/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ var (
ErrMountUnitNoFormat = errors.New("format is required if with_mount_unit is true")

// boot device
ErrUnknownBootDeviceLayout = errors.New("layout must be one of: aarch64, ppc64le, x86_64")
ErrUnknownBootDeviceLayout = errors.New("layout must be one of: aarch64, ppc64le, x86_64, s390x-zfcp, s390x-eckd, s390x-virt")
ErrTooFewMirrorDevices = errors.New("mirroring requires at least two devices")

// partition
Expand Down
2 changes: 1 addition & 1 deletion config/fcos/v1_6_exp/schema.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ type BootDevice struct {

type BootDeviceLuks struct {
Copy link
Contributor

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

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 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.

Discard *bool `yaml:"discard"`
Device *string `yaml:"device"`
Device string `yaml:"device"`
Tang []base.Tang `yaml:"tang"`
Threshold *int `yaml:"threshold"`
Tpm2 *bool `yaml:"tpm2"`
Expand Down
32 changes: 17 additions & 15 deletions config/fcos/v1_6_exp/translate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,15 +117,15 @@ 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.BootDeviceLuks.Device) > 0 && 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
}

// s390x zfcp and dasd does not support mirror
if wantLuksDevice && wantMirror {
return r

if wantLuksDevice && wantLuks {
panic("can't happen")
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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 remove and add another logic to check.

}

// compute layout rendering options
Expand All @@ -134,7 +134,6 @@ func (c Config) processBootDevice(config *types.Config, ts *translate.Translatio
var wantPRePPart bool
var wantMBR bool
var wantDasd bool
var wantKVM bool
layout := c.BootDevice.Layout
switch {
case layout == nil || *layout == "x86_64":
Expand All @@ -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:
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

wantMBR = true
case *layout == "s390x-eckd":
case *layout == "s390x-eckd" && wantLuksDevice:
Copy link
Contributor

Choose a reason for hiding this comment

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

See above

wantDasd = true
case *layout == "s390x-virt":
case *layout == "s390x-virt" && !wantLuksDevice:
Copy link
Contributor

Choose a reason for hiding this comment

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

See above

wantBIOSPart = true
wantEFIPart = true
default:
Expand Down Expand Up @@ -283,18 +282,18 @@ func (c Config) processBootDevice(config *types.Config, ts *translate.Translatio
}

//encrypted root partition for s390x
if wantLuksDevice {
if wantMBR || wantDasd {
var luksDevice string
dasd := dasdRe.FindString(c.BootDeviceLuks.Device)
sd := sdRe.FindString(c.BootDeviceLuks.Device)
dasd := dasdRe.FindString(c.BootDevice.Luks.Device)
sd := sdRe.FindString(c.BootDevice.Luks.Device)

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

Choose a reason for hiding this comment

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

Can we get a comment explaining the luks device value?

Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?
Yes will add, sd + "2"

case wantDasd && len(dasd) != 0:
luksDevice = dasd + strconv.Itoa(2)
default:
panic("can't happen")
panic("Incorrect Device Parameter")
Copy link
Contributor

Choose a reason for hiding this comment

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

If we could give the Device parameter that is incorrect it would go a long way for debugging.

}
clevis, ts2, r2 := translateBootDeviceLuks(c.BootDevice.Luks, options)
rendered.Storage.Luks = []types.Luks{{
Expand Down Expand Up @@ -327,9 +326,12 @@ func (c Config) processBootDevice(config *types.Config, ts *translate.Translatio
case wantMirror:
// RAID without LUKS
rootDevice = "/dev/md/md-root"
case wantLuksDevice:
//Only Luks for s390x
rootDevice = "/dev/mapper/root"
default:
panic("can't happen")
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: White space after '}'

rootFilesystem := types.Filesystem{
Device: rootDevice,
Format: util.StrToPtr("xfs"),
Expand Down Expand Up @@ -420,4 +422,4 @@ func buildGrubConfig(gb Grub) string {
}
superUserCmd := fmt.Sprintf("set superusers=\"%s\"\n", strings.Join(allUsers, " "))
return "# Generated by Butane\n\n" + superUserCmd + strings.Join(cmds, "\n") + "\n"
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: random change in formatting.