s390x: add support for IBM Z DASD disks #153

nikita-dubrovskii · 2020-02-11T10:38:41Z

Installation on s390 dasd disks is a bit tricky,
this patch makes coreos-installer able to format,
create partitions, and finally install CoreOS on s390.

Signed-off-by: Nikita Dubrovskii nikita@linux.ibm.com

nikita-dubrovskii · 2020-02-11T10:42:57Z

Hi Colin. We had a short discussion in Brno (DevConf). Could you please review this patch?

bgilbert

Thanks for doing this! These are initial impressions; I haven't done a detailed review.

src/install.rs

src/s390x/dasd.rs

src/s390x/zipl.rs

src/s390x/dasd.rs

src/s390x/zipl.rs

cgwalters

OK so I only skimmed this, and I only have superficial knowledge of what we need for dasd; I know we've talked about it a few times and my recollection is that it boils down to the kernel needing to "activate" devices effectively?

Is there a good reference for this stuff? OK from a quick search the top two (here) results are:

Which look useful. Though what would help me in this PR is perhaps a little bit more elaboration on what "support" here means. Perhaps a block comment in the s390x.rs file or a README-s390x.md?

Also, did you learn Rust just for this? If so (or actually regardless) it's excellent code and really well done!

cgwalters · 2020-03-04T23:41:29Z

Does this have any intersection with the discussions around coreos-installer and multipath? I think that one was going to end up on the legacy branch first?

src/s390x/zipl.rs

cgwalters · 2020-03-04T23:45:23Z

My overall feeling here is if you've tested this and the rest of the s390x team is happy with it, I'm inclined to merge.

nikita-dubrovskii · 2020-03-05T08:45:16Z

Does this have any intersection with the discussions around coreos-installer and multipath? I think that one was going to end up on the legacy branch first?

No intersection, Tuan is working on multipath for legacy branch. And i've started working on multipath for master branch

nikita-dubrovskii · 2020-03-05T08:55:23Z

OK so I only skimmed this, and I only have superficial knowledge of what we need for dasd; I know we've talked about it a few times and my recollection is that it boils down to the kernel needing to "activate" devices effectively?

Exactly, we have to prepare (format & make partitions) DASD device and then tell kernel about it
rd.dasd=xxxxx .... (and networking as well rd.znet=xxxxx ..)

Is there a good reference for this stuff? OK from a quick search the top two (here) results are:
* https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/7/html/installation_guide/chap-post-installation-configuration-s390
* https://www.kernel.org/doc/Documentation/s390/DASD

Yes, i read this.

Though what would help me in this PR is perhaps a little bit more elaboration on what "support" here means. Perhaps a block comment in the s390x.rs file or a README-s390x.md?

Ok, makes sense

Also, did you learn Rust just for this? If so (or actually regardless) it's excellent code and really well done!

Yep, i've tried several times before, but was too busy with C++ projects and had no chance to do smth in Rust. thx ))

cgwalters

OK I gave this another look; some optional changes to make some code more idiomatic/avoid unwraps.

src/s390x/dasd.rs

src/s390x/zipl.rs

nikita-dubrovskii

This one breaks the installation. I'm debugging it

src/install.rs

bgilbert · 2020-07-07T21:36:47Z

I think this is ready for review!

bgilbert · 2020-07-07T21:42:27Z

@jlebon proposed squashing this series down to one commit before merging. Any preferences?

nikita-dubrovskii · 2020-07-08T12:59:42Z

@jlebon proposed squashing this series down to one commit before merging. Any preferences?

hmm, it's too big for one commit. maybe we can have smth like:

s390x: add support for IBM Z DASD disks
clean-ups
s390x: use ioctls to get DASD parameters
s390x: switch from fdisk to gptman
s390x: drop dd-lib
s390x: stream download directly to disk
clean-ups
s390x: trigger udev_settle after disk format
s390x: remove variable substitutions from kargs
s390x: use '/dev/dasd' as a pattern

bgilbert · 2020-07-08T15:35:10Z

I'll let @jlebon chime in here, but I think his idea is to avoid leaking development history into the master branch. The merged commit would list you as the primary author, and me as a co-author.

I think the prep changes that affect the rest of the codebase (e.g. adding a callback to write_image) should still be separate commits, though.

jlebon

No major issues, looks sane overall!

jlebon · 2020-07-08T16:01:08Z

.travis.yml

+    - rust: beta
+      arch: s390x
+    - rust: nightly
+      arch: s390x


Wait, what? /me reads https://docs.travis-ci.com/user/multi-cpu-architectures/.

Travis CI can test on amd64, ppc64le (IBM Power CPUs), s390x (IBM Z CPUs) and arm64 (run on ARMv8 compliant CPUs) if the operating system is Linux.

Whoa, didn't realize this! We could use that in cosa at least to get some multi-arch coverage, even if just of ./build.sh (though definitely worth checking if it also has virt support, but likely not).

jlebon · 2020-07-08T16:10:03Z

scripts/coreos-installer-service

@@ -10,6 +10,9 @@ PERSIST_KERNEL_NET_PARAMS=("ipv6.disable" "net.ifnames" "net.naming-scheme")
 # List from https://www.mankier.com/7/dracut.cmdline#Description-Network
 PERSIST_DRACUT_NET_PARAMS=("ip" "ifname" "rd.route" "bootdev" "BOOTIF" "rd.bootif" "nameserver" "rd.peerdns" "biosdevname" "vlan" "bond" "team" "bridge" "rd.net.timeout.carrier" "coreos.no_persist_ip")

+# IBM S390X params to persist
+PERSIST_S390X_PARAMS=("rd.dasd" "rd.zfcp" "rd.znet" "zfcp.allow_lun_scan" "cio_ignore")


Hmm, do we really need this? There is a push in general to not add support for more kargs than we already have and instead direct users towards scripting coreos-installer via Ignition: #124.

As I understand it:

rd.dasd activates a DASD device, without which a DASD boot device won't show up

rd.zfcp activates a Fibre Channel device, without which an FC boot device won't show up

rd.znet activates a network device, without which it won't be available from the initramfs

zfcp.allow_lun_scan might be needed to configure activation of certain FC devices

cio_ignore tells the kernel not to attach to specified devices on boot

So I suspect they might all be needed. @nikita-dubrovskii, is that correct? Any additional details?

So I suspect they might all be needed.

Exactly, we have to persist those kargs.

src/install.rs

src/s390x/dasd.rs

src/s390x/zipl.rs

jlebon · 2020-07-08T19:35:45Z

src/s390x/zipl.rs

+fn get_boot_kargs<P: AsRef<Path>>(boot_config: P) -> Result<String> {
+    let contents = read_to_string(&boot_config)
+        .chain_err(|| format!("reading {}", boot_config.as_ref().display()))?;
+    // read kargs from options line


Regex is fine, though it'd be cleaner and probably less lines overall to just iterate over the lines and find the one that starts with options. See e.g. bls_entry_delete_and_append_kargs.

Eh, I find this cleaner than open-coding an iteration and starts_with(). Can change it if there's a strong preference though.

Not a strong preference. Regex just seemed like overkill for the task at hand, but meh...

As i remember, some of you asked me to use regex for substring-search operations )

Yup, that was me. 🙂

jlebon · 2020-07-08T19:46:29Z

I'll let @jlebon chime in here, but I think his idea is to avoid leaking development history into the master branch. The merged commit would list you as the primary author, and me as a co-author.

I think the prep changes that affect the rest of the codebase (e.g. adding a callback to write_image) should still be separate commits, though.

Yeah exactly. I think the rule of thumb I personally use is something like: any commit that's just changing content that's all new in the same PR should normally just be squashed in. To take a random example, is this commit valuable to have on its own on the master branch? It's just modifying a README.md file which master hasn't even seen yet.

Commits which refactor existing code as prep should definitely still be kept.

bgilbert · 2020-07-08T21:39:49Z

@nikita-dubrovskii If you agree, I'll refactor into three commits:

The generic parts of "s390x: stream download directly to disk"
The generic parts of "install: correctly detect DASDs if they're behind a symlink"
Everything else, with Author: you and Co-authored-by: me

I can do that in a new PR if you'd like the history to be preserved here.

nikita-dubrovskii · 2020-07-09T07:56:13Z

I can do that in a new PR if you'd like the history to be preserved here.

I don't mind, for me any approach is Ok.

Return the symlink target if a link, or the original path if not.

Allow callers to provide a custom callback for writing image data to disk.

Support formatting, creating partitions, and installing onto s390x DASD disks. Co-authored-by: Benjamin Gilbert <bgilbert@redhat.com>

bgilbert · 2020-07-10T01:19:07Z

Updated and squashed!

nikita-dubrovskii requested a review from cgwalters February 11, 2020 10:41

bgilbert reviewed Feb 13, 2020

View reviewed changes

nikita-dubrovskii force-pushed the s390 branch from 0abfd32 to a4d8639 Compare February 14, 2020 11:40

bgilbert reviewed Feb 14, 2020

View reviewed changes

src/s390x/zipl.rs Outdated Show resolved Hide resolved

nikita-dubrovskii force-pushed the s390 branch 2 times, most recently from 4a53f10 to 9a2f048 Compare February 20, 2020 13:03

nikita-dubrovskii force-pushed the s390 branch 2 times, most recently from 52a7b3a to e26949d Compare March 3, 2020 07:47

cgwalters reviewed Mar 4, 2020

View reviewed changes

src/s390x/zipl.rs Outdated Show resolved Hide resolved

cgwalters reviewed Mar 4, 2020

View reviewed changes

src/s390x/zipl.rs Outdated Show resolved Hide resolved

nikita-dubrovskii force-pushed the s390 branch from e26949d to c6af5ae Compare March 9, 2020 09:51

nikita-dubrovskii requested review from bgilbert and cgwalters March 10, 2020 09:14

nikita-dubrovskii force-pushed the s390 branch from c6af5ae to c7b8c35 Compare March 11, 2020 14:30

nikita-dubrovskii mentioned this pull request Mar 11, 2020

s390x: put coreos-installer into initramfs coreos/fedora-coreos-config#294

Closed

nikita-dubrovskii force-pushed the s390 branch from c7b8c35 to 22a2441 Compare March 12, 2020 15:36

nikita-dubrovskii force-pushed the s390 branch 2 times, most recently from 0e900a5 to 7ccf76a Compare March 20, 2020 13:07

nikita-dubrovskii force-pushed the s390 branch from 7ccf76a to 272b3eb Compare April 6, 2020 10:05

nikita-dubrovskii force-pushed the s390 branch from 272b3eb to 574ed09 Compare April 20, 2020 12:05

nikita-dubrovskii changed the title ~~Add support for IBM s390x dasd disks~~ s390x: add support for IBM Z DASD disks Apr 20, 2020

nikita-dubrovskii force-pushed the s390 branch from 574ed09 to c634338 Compare April 23, 2020 18:47

cgwalters reviewed Apr 28, 2020

View reviewed changes

nikita-dubrovskii force-pushed the s390 branch from c634338 to 02917e1 Compare April 29, 2020 12:15

nikita-dubrovskii commented Jul 7, 2020

View reviewed changes

bgilbert reviewed Jul 7, 2020

View reviewed changes

src/install.rs Outdated Show resolved Hide resolved

bgilbert marked this pull request as ready for review July 7, 2020 21:36

bgilbert force-pushed the s390 branch 4 times, most recently from 4501977 to aff9055 Compare July 8, 2020 16:28

jlebon approved these changes Jul 8, 2020

View reviewed changes

bgilbert force-pushed the s390 branch 2 times, most recently from a9ea689 to 08d4764 Compare July 8, 2020 21:34

bgilbert mentioned this pull request Jul 9, 2020

Release coreos-installer 0.3.0 #297

Closed

31 tasks

bgilbert force-pushed the s390 branch 3 times, most recently from ca76ff0 to 6864523 Compare July 9, 2020 21:27

bgilbert and others added 3 commits July 9, 2020 19:46

io: add utility function to resolve a possible symlink

d72b200

Return the symlink target if a link, or the original path if not.

download: split write callback from write_image()

b3f24ed

Allow callers to provide a custom callback for writing image data to disk.

s390x: add support for IBM Z DASD disks

85cc684

Support formatting, creating partitions, and installing onto s390x DASD disks. Co-authored-by: Benjamin Gilbert <bgilbert@redhat.com>

bgilbert force-pushed the s390 branch from 6864523 to 85cc684 Compare July 10, 2020 01:17

jlebon approved these changes Jul 10, 2020

View reviewed changes

bgilbert merged commit f9c3ecb into coreos:master Jul 10, 2020

nikita-dubrovskii deleted the s390 branch July 13, 2020 07:37

jlebon mentioned this pull request Jul 24, 2020

rdcore: add stream-hash subcommand #305

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

s390x: add support for IBM Z DASD disks #153

s390x: add support for IBM Z DASD disks #153

nikita-dubrovskii commented Feb 11, 2020

nikita-dubrovskii commented Feb 11, 2020 •

edited

Loading

bgilbert left a comment

cgwalters left a comment

cgwalters commented Mar 4, 2020

cgwalters commented Mar 4, 2020

nikita-dubrovskii commented Mar 5, 2020

nikita-dubrovskii commented Mar 5, 2020 •

edited

Loading

cgwalters left a comment

nikita-dubrovskii left a comment

bgilbert commented Jul 7, 2020

bgilbert commented Jul 7, 2020

nikita-dubrovskii commented Jul 8, 2020

bgilbert commented Jul 8, 2020

jlebon left a comment

jlebon Jul 8, 2020

jlebon Jul 8, 2020

bgilbert Jul 8, 2020

nikita-dubrovskii Jul 9, 2020

jlebon Jul 8, 2020

bgilbert Jul 8, 2020

jlebon Jul 8, 2020

nikita-dubrovskii Jul 9, 2020

bgilbert Jul 9, 2020

jlebon commented Jul 8, 2020

bgilbert commented Jul 8, 2020

nikita-dubrovskii commented Jul 9, 2020

bgilbert commented Jul 10, 2020

s390x: add support for IBM Z DASD disks #153

s390x: add support for IBM Z DASD disks #153

Conversation

nikita-dubrovskii commented Feb 11, 2020

nikita-dubrovskii commented Feb 11, 2020 • edited Loading

bgilbert left a comment

Choose a reason for hiding this comment

cgwalters left a comment

Choose a reason for hiding this comment

cgwalters commented Mar 4, 2020

cgwalters commented Mar 4, 2020

nikita-dubrovskii commented Mar 5, 2020

nikita-dubrovskii commented Mar 5, 2020 • edited Loading

cgwalters left a comment

Choose a reason for hiding this comment

nikita-dubrovskii left a comment

Choose a reason for hiding this comment

bgilbert commented Jul 7, 2020

bgilbert commented Jul 7, 2020

nikita-dubrovskii commented Jul 8, 2020

bgilbert commented Jul 8, 2020

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

jlebon commented Jul 8, 2020

bgilbert commented Jul 8, 2020

nikita-dubrovskii commented Jul 9, 2020

bgilbert commented Jul 10, 2020

nikita-dubrovskii commented Feb 11, 2020 •

edited

Loading

nikita-dubrovskii commented Mar 5, 2020 •

edited

Loading