ignition/mount.go: add tests for mounting additional disks #1128

zonggen · 2019-11-28T22:28:08Z

Adds tests for mounting additional disks at /var/lib/containers
and /var/log. Instead of mounting new disks, tests add new partitions
under the disk vda. Currently, there are four partitions, i.e. vda1
-vda4, so new partitions will start from vda5. Particularly, vda5 ->
/var/lib/containers, and vda6 -> /var/log.

Before running the test, add 2G more space to the disk vda since
vda5 and vda6 will each take 1G space. Detailed explanations can
be found at https://gist.github.com/zonggen/d888b1ece140f8592a70137d35050073

Closes: https://jira.coreos.com/browse/GRPA-1203
Signed-off-by: Allen Bai [email protected]

EDIT: barred adding space part

jlebon

First, awesome! Been thinking about adding a test like this too for the SELinux relabeling work I'm doing.

Before running the test, add 2G more space to the disk vda since vda5 and vda6 will each take 1G space.

Hmm, I'm a bit surprised you need to add space actually. With coreos/coreos-assembler#924, there should be a lot more headroom for more partitions. Can you confirm the images you tested had this patch? (Another, perhaps more realistic approach, is having kola attach another disk, but I think having tests for both cases is good).

This is a great start! For FCOS at least, we should be able to support many more mounts, including /var and /var/home (though we recently regressed on that last one, but it should be fixed soon!). This is what I use today to test partitioning and correct labeling: https://gist.github.com/jlebon/889d34b481a8c2abee618cf448a4c6c6. Once the /var/home regression is fixed, I can expand this test with this.

kola/tests/ignition/mount.go

jlebon · 2019-11-29T14:35:30Z

kola/tests/ignition/mount.go

+func init() {
+	// verify mount to `/var/lib/containers`
+	register.Register(&register.Test{
+		Name:        "coreos.ignition.mount.var-lib-containers",


Hmm, do we actually need to split this out into three separate tests instead of just the combined one?

The Jira card stated it as three different tests so I thought we wanted three separate ones here, though I can't see any reason not to just make a combined one.

Yeah, I think it'd be cleaner. WDYT about having two tests:

one where we create the two partitions on the primary disk

one where we attach two additional disks, one for each partition

?

SGTM 👍

UPDATE:

Added tests for both adding partitions and disks

kola/tests/ignition/mount.go

zonggen · 2019-11-29T15:12:17Z

Hmm, I'm a bit surprised you need to add space actually. With coreos/coreos-assembler#924, there should be a lot more headroom for more partitions. Can you confirm the images you tested had this patch?

You're right! Just did a rebuild and there was a lot of space left for more partitions. Here are the results:
FCOS with one extra partition:

[core@coreos ~] $ lsblk
NAME   MAJ:MIN RM  SIZE RO TYPE MOUNTPOINT
sr0     11:0    1 1024M  0 rom  
vda    252:0    0    8G  0 disk 
|-vda1 252:1    0  384M  0 part /boot
|-vda2 252:2    0  127M  0 part /boot/efi
|-vda3 252:3    0    1M  0 part 
|-vda4 252:4    0  2.2G  0 part /sysroot
`-vda5 252:5    0  5.3G  0 part /var/log

FCOS with two extra partitions:

[core@coreos ~]$ lsblk
NAME   MAJ:MIN RM  SIZE RO TYPE MOUNTPOINT
sr0     11:0    1 1024M  0 rom  
vda    252:0    0    8G  0 disk 
|-vda1 252:1    0  384M  0 part /boot
|-vda2 252:2    0  127M  0 part /boot/efi
|-vda3 252:3    0    1M  0 part 
|-vda4 252:4    0  2.2G  0 part /sysroot
|-vda5 252:5    0    1G  0 part /var/lib/containers
`-vda6 252:6    0    1G  0 part /var/log

The impression of having to add more space comes from the first few attempts to create partitions where the newly created partitions only has ~100k space and the system refused to mount the partition to /var/lib/containers since not enough space. Fixed by cleaning containers with podman system prune -a.

zonggen · 2019-11-29T15:15:37Z

(Another, perhaps more realistic approach, is having kola attach another disk, but I think having tests for both cases is good).

Or adding support to COSA to attach another disk

jlebon · 2019-11-29T17:29:09Z

(Another, perhaps more realistic approach, is having kola attach another disk, but I think having tests for both cases is good)

So reading the initial requirements for this work, it does seem like it was explicitly about setting up the partitions on a secondary disk. Looking at the kola test, I think you can do this via platform.MachineOptions. See e.g. tests/misc/raid.go:

	options := platform.MachineOptions{
		AdditionalDisks: []platform.Disk{
			{Size: "520M", DeviceOpts: []string{"serial=secondary"}},
		},
	}

Up to you whether you want to work on that here, or as a follow-up PR!

zonggen · 2019-11-29T18:01:24Z

I think it makes sense to add it here. Thanks for the pointer!
Updating

arithx

A quick skim leaves the impression that coreos.ignition.mount.var-lib-containers & coreos.ignition.mount.var-log are just duplicated split out versions of coreos.ignition.mount.multiple (other than coreos.ignition.mount.multiple specifying the exact sizing of the partitions instead of letting Ignition/sgdisk pick). I'd probably lean towards dropping the former tests and just leaving the mount.multiple test in their current state.

arithx · 2019-12-02T16:29:34Z

kola/tests/ignition/mount.go

+func mountMultiple(c cluster.TestCluster) {
+	m := c.Machines()[0]
+
+	out := c.MustSSH(m, "cat /proc/mounts")


With the way that you've written these tests the validation could be split out into a sub-function to reduce duplication, e.x.:

func mountValidate(c cluster.TestCluster, m platform.Machine, mountContents, path string) { if strings.Contains(mountContents, path) { c.Fatalf("No partition mounted to %s", path) } fPath := filepath.Join(path, "hello.txt") fileContents := c.MustSSH(m, fPath) if string(fileContents) != "hello world" { c.Fatalf("Failed to write content to %s", fPath) } } func mountMultiple(c cluster.TestCluster) { m := c.Machines()[0] mountContents := c.MustSSH(m, "cat /proc/mounts") mountValidate(c, m, mountContents, "/var/lib/containers") mountValidate(c, m, mountContents, "/var/lib/log") }

Thanks for sharing the code! I've updated with helper functions based on your comment.

cgwalters · 2019-12-02T19:07:53Z

kola/tests/ignition/mount.go

+		Run:         mountVarLibContainers,
+		ClusterSize: 1,
+		UserData: conf.Ignition(`{
+			"ignition": {"version": "2.2.0"},


Before we add many more tests that use nontrivial Ignition, we're really going to need to migrate the Ignition translation from cosa to here, and only write it one time. @ajeddeloh may have newer code.

👍 on this, this would be very helpful
So for now maybe only test ignition v3 on FCOS?

@ajeddeloh Do you indeed have code ready to go we could use for this?

I think we can just hackily have v2 and v3 Ignition for now so we can get this test in and circle back to deduping once we have a solid translator in place. OTOH, if there's code ready to go to accomplish this, then let's definitely leverage that.

jlebon · 2019-12-10T21:21:53Z

@zonggen Is this ready to go? Does this pass on both FCOS and RHCOS?

zonggen · 2019-12-10T21:59:22Z

The tests are passing FCOS. Only FCOS(3.0.0) cases are tested, since as @cgwalters suggested we might be adding translation logic later. Should we add RHCOS(2.2.0) cases here as well?

zonggen · 2019-12-11T17:42:32Z

Another issue is that when testing with v2.2.0, adding ignition files with COSA would work but running inside kola test suite wouldn't.

For example, with code_here, running cosa run -i partitions.ign would work without issue but running kola run -b rhcos --qemu-image rhcos-43.81.201912021843.0-qemu.x86_64.qcow2 coreos.ignition.mount.partitions would hang. And the same code with v3.0.0 would run without problem.

v2.2.0 kola example:

[coreos-assembler]$ /mantle/bin/kola run -b rhcos --qemu-image rhcos-43.81.201912021843.0-qemu.x86_64.qcow2 coreos.ignition.mount.partitions
=== RUN   coreos.ignition.mount.partitions
^Cqemu-system-x86_64: terminating on signal 2

jlebon · 2019-12-11T18:46:12Z

Another issue is that when testing with v2.2.0, adding ignition files with COSA would work but running inside kola test suite wouldn't.

You should be able to debug kola issues by looking at the console and/or journal output in _kola_temp/....

zonggen · 2019-12-11T21:46:36Z

I was originally following instructions_here, where kola run -b rhcos ... was used, but it turns out that --distro rhcos --ignition-version v2 should be used instead of only -b rhcos since by default kola would inject v3 config which is not valid under RHCOS, maybe make a PR against the documentation?

After that, new_partitions.ign would still fail when mounting the device (console.txt), which I'm currently stuck on. Extra help is much appreciated..

jlebon · 2019-12-11T22:43:05Z

Ohhh wow, this is the 40rhcos-fips module failing to run ignition-files... Sigh, I knew my sins would come back to haunt me. I think I was operating in Ignition v2 mode, where the files stage doesn't care about filesystems. But in v0.x, it of course does. Thanks for this, will fix!

jlebon · 2019-12-13T16:59:43Z

OK, so I was playing with your partitions.ign Ignition config here, and even with the 40rhcos-fips issue fixed, there seems to be a race condition that happens between the encryption stuff and the disks stage. Anyway, sent a fix for that too. Once it's in, that Ignition config should work (it does locally for me!).

Just to make the Ignition config a bit more realistic. Covers a recent regression we had in that area: coreos#1128 (comment)

zonggen · 2019-12-13T17:55:36Z

The tests will work now for both RHCOS and FCOS, though I didn't hit the regression.. ready for another look whenever you have time! @jlebon

jlebon · 2019-12-13T19:08:39Z

kola/tests/ignition/mount.go

+		c.Fatal(err)
+	}
+
+	// reboot it to make sure it comes up again


How about we check the partitions were mounted before rebooting too?

jlebon · 2019-12-13T19:08:49Z

kola/tests/ignition/mount.go

+	mountValidate(c, m, string(mountContents), "/var/log")
+}
+
+func testMountPartitions(c cluster.TestCluster) {


I like the reboot check. Let's do that in this test too?

jlebon · 2019-12-13T19:09:51Z

kola/tests/ignition/mount.go

+
+// Validate partition is mounted to `path` and `path`/hello.txt is written
+func mountValidate(c cluster.TestCluster, m platform.Machine, mountContents, path string) {
+	if !strings.Contains(mountContents, path) {


I'd add a space to the path here so that e.g. /var/log/systemd doesn't match.

SGTM! Updated.

jlebon · 2019-12-13T19:10:26Z

kola/tests/ignition/mount.go

+	// make sure partitions are mounted and files are written
+	mountContents := c.MustSSH(m, "cat /proc/mounts")
+	mountValidate(c, m, string(mountContents), "/var/lib/containers")
+	mountValidate(c, m, string(mountContents), "/var/log")


Maybe let's just factor out this bit too into a separate function?

Updated with helper function.

Adds tests for mounting additional disks at /var/lib/containers and /var/log. Instead of mounting new disks, tests add new partitions under the disk `vda`. Currently, there are four partitions, i.e. vda1 -vda4, so new partitions will start from vda5. Particularly, vda5 -> /var/lib/containers, and vda6 -> /var/log. Before running the test, add 2G more space to the disk `vda` since `vda5` and `vda6` will each take 1G space. Detailed explanations can be found at https://gist.github.com/zonggen/d888b1ece140f8592a70137d35050073 Closes: https://jira.coreos.com/browse/GRPA-1203 Signed-off-by: Allen Bai <[email protected]> ignition/mount.go: use /dev/disk/dy-partlabel/ and write files Addresses improvments mentioned in reviews. Signed-off-by: Allen Bai <[email protected]> ignition/mount.go: add test for mounting new disks Previously used new partitions under vda to test mounting volumes to /var/lib/containers and /var/log. Add new tests for creating two new disks vdb and vdc and mounting them to /var/lib/containers and /var/log corrrespondingly. Signed-off-by: Allen Bai <[email protected]> kola/mount.go: Add test for RHCOS with ignition v2.2.0 Signed-off-by: Allen Bai <[email protected]> kola/mount.go: Add helper function and check before reboot Signed-off-by: Allen Bai <[email protected]>

zonggen · 2019-12-13T20:34:14Z

Ran the tests on RHCOS and FCOS, passing.
Rebased and squash'd the commits

jlebon

Yup, tested working.
Nice work!

jlebon reviewed Nov 29, 2019

View reviewed changes

arithx reviewed Dec 2, 2019

View reviewed changes

cgwalters reviewed Dec 2, 2019

View reviewed changes

jlebon mentioned this pull request Dec 13, 2019

kola/fips: Add a partition to Ignition config #1142

Merged

jlebon added a commit to jlebon/mantle that referenced this pull request Dec 13, 2019

kola/fips: Add a partition to Ignition config

cb792ff

Just to make the Ignition config a bit more realistic. Covers a recent regression we had in that area: coreos#1128 (comment)

zonggen force-pushed the mount-var branch from 2a5571f to 2af8dd8 Compare December 13, 2019 17:58

jlebon reviewed Dec 13, 2019

View reviewed changes

zonggen force-pushed the mount-var branch from c0c0913 to a91a3d4 Compare December 13, 2019 20:33

jlebon approved these changes Dec 13, 2019

View reviewed changes

jlebon merged commit 5b9ffbc into coreos:master Dec 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ignition/mount.go: add tests for mounting additional disks #1128

ignition/mount.go: add tests for mounting additional disks #1128

zonggen commented Nov 28, 2019 •

edited

Loading

jlebon left a comment

jlebon Nov 29, 2019

zonggen Nov 29, 2019 •

edited

Loading

jlebon Nov 29, 2019

zonggen Nov 29, 2019 •

edited

Loading

zonggen commented Nov 29, 2019 •

edited

Loading

zonggen commented Nov 29, 2019

jlebon commented Nov 29, 2019

zonggen commented Nov 29, 2019

arithx left a comment

arithx Dec 2, 2019

zonggen Dec 2, 2019 •

edited

Loading

cgwalters Dec 2, 2019

zonggen Dec 2, 2019 •

edited

Loading

jlebon Dec 11, 2019

jlebon commented Dec 10, 2019

zonggen commented Dec 10, 2019

zonggen commented Dec 11, 2019 •

edited

Loading

jlebon commented Dec 11, 2019

zonggen commented Dec 11, 2019

jlebon commented Dec 11, 2019

jlebon commented Dec 13, 2019 •

edited

Loading

zonggen commented Dec 13, 2019

jlebon Dec 13, 2019

zonggen Dec 13, 2019

jlebon Dec 13, 2019

zonggen Dec 13, 2019

jlebon Dec 13, 2019

zonggen Dec 13, 2019

jlebon Dec 13, 2019

zonggen Dec 13, 2019

zonggen commented Dec 13, 2019

jlebon left a comment

ignition/mount.go: add tests for mounting additional disks #1128

ignition/mount.go: add tests for mounting additional disks #1128

Conversation

zonggen commented Nov 28, 2019 • edited Loading

jlebon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zonggen Nov 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zonggen Nov 29, 2019 • edited Loading

Choose a reason for hiding this comment

zonggen commented Nov 29, 2019 • edited Loading

zonggen commented Nov 29, 2019

jlebon commented Nov 29, 2019

zonggen commented Nov 29, 2019

arithx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zonggen Dec 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zonggen Dec 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlebon commented Dec 10, 2019

zonggen commented Dec 10, 2019

zonggen commented Dec 11, 2019 • edited Loading

jlebon commented Dec 11, 2019

zonggen commented Dec 11, 2019

jlebon commented Dec 11, 2019

jlebon commented Dec 13, 2019 • edited Loading

zonggen commented Dec 13, 2019

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

zonggen commented Dec 13, 2019

jlebon left a comment

Choose a reason for hiding this comment

zonggen commented Nov 28, 2019 •

edited

Loading

zonggen Nov 29, 2019 •

edited

Loading

zonggen Nov 29, 2019 •

edited

Loading

zonggen commented Nov 29, 2019 •

edited

Loading

zonggen Dec 2, 2019 •

edited

Loading

zonggen Dec 2, 2019 •

edited

Loading

zonggen commented Dec 11, 2019 •

edited

Loading

jlebon commented Dec 13, 2019 •

edited

Loading