kola/tests/misc: add kola test to validate the boot-mirror RAID1 by sohankunkerkar · Pull Request #1880 · coreos/coreos-assembler

sohankunkerkar · 2020-11-18T14:33:52Z

Not mergeable until this PR gets merged.
This will add a kola test to validate the boot-mirror RAID1 changes in both BIOS and UEFI modes.

bgilbert · 2020-12-19T03:59:57Z

+	case *unprivqemu.Cluster:
+		m, err = pc.NewMachineWithQemuOptions(bootmirror, options)
+	default:
+		m, err = pc.NewMachine(bootmirror)


In the non-unpriv-qemu case, the test won't succeed, since it won't have additional disks. And we correctly restrict the platform list to qemu-unpriv, so we shouldn't ever get here. I think it's probably okay to just do the .(*unprivqemu.Cluster) type assertion and let it panic if we're not a qemu-unpriv cluster.

bgilbert · 2020-12-19T04:03:55Z

-
-// checkIfMountpointIsRaid will check if a given machine has a device of type
-// raid1 mounted at the given mountpoint. If it does not, the test is failed.
-func checkIfMountpointIsRaid(c cluster.TestCluster, m platform.Machine, mountpoint string) {


In general, significant movement of code should be done in a separate commit to make the diff easier to read.

There's no technical requirement to move this code to a separate file. Since raid.go and boot-mirror.go are in the same package, they can share private functions. But I'm okay with the move if you prefer.

bgilbert · 2020-12-19T04:11:31Z

                cosa_cmd("kola --basic-qemu-scenarios")
                cosa_cmd("kola run --parallel 8")
                cosa_cmd("kola run --qemu-firmware uefi boot-mirror")
+                cosa_cmd("kola run --qemu-firmware uefi boot-mirror-luks")


You should be able to combine these two lines:

cosa_cmd("kola run --qemu-firmware uefi boot-mirror*")

bgilbert · 2020-12-19T04:13:32Z

+	register.RegisterTest(&register.Test{
+		Run:                  runBootMirrorLUKSTest,
+		ClusterSize:          0,
+		Name:                 `boot-mirror-luks`,


These names are usually dotted paths in a hierarchy. Let's find an appropriate place in the hierarchy to put these tests.

bgilbert · 2020-12-19T04:13:42Z

+		Run:                  runBootMirrorLUKSTest,
+		ClusterSize:          0,
+		Name:                 `boot-mirror-luks`,
+		Flags:                []register.Flag{},


Unnecessary line; can be dropped.

bgilbert · 2020-12-19T04:14:26Z

+		Name:                 `boot-mirror-luks`,
+		Flags:                []register.Flag{},
+		Platforms:            []string{"qemu-unpriv"},
+		ExcludeArchitectures: []string{"s390x", "ppc64le", "aarch64"}, // no TPM support for s390x, ppc64le, aarch64 in qemu


arithx · 2021-02-09T06:57:44Z

+		// aarch64 and ppc64le are added temporarily to avoid test failure
+		// until we support specifying FCC directly in kola.
+		ExcludeArchitectures: []string{"aarch64", "ppc64le", "s390x"}, // no TPM support for s390x in qemu
+		ExcludeFirmwares:     []string{"uefi"},


Would be good to leave a comment referencing the reason why it's disabled (and a link to an issue if available).

arithx · 2021-02-09T07:00:17Z

+	rootOutput := c.MustSSH(m, "sudo mdadm --export --detail /dev/md/md-root")
+	if !strings.Contains(string(rootOutput), "/dev/vda4") || !strings.Contains(string(rootOutput), "/dev/vdb4") {
+		c.Fatalf("root raid device missing; expected devices: %v", string(rootOutput))
+	}
+	bootOutput := c.MustSSH(m, "sudo mdadm --export --detail /dev/md/md-boot")
+	if !strings.Contains(string(bootOutput), "/dev/vda3") || !strings.Contains(string(bootOutput), "/dev/vdb3") {
+		c.Fatalf("root raid device missing; expected devices: %v", string(rootOutput))
+	}


Any reason these can't be moved into bootMirrorSanityTest so they're both inside a named run block as well as run on both LUKS & non-LUKS? (Same with below checks but into verifyBootMirrorAfterReboot)

Any reason these can't be moved into bootMirrorSanityTest so they're both inside a named run block as well as run on both LUKS & non-LUKS? (Same with below checks but into verifyBootMirrorAfterReboot)

Actually, both tests are using a different number of devices, so the conditional statement would differ in each case.

coreos.boot-mirror- 3 (removing /dev/sdc(3/4))

coreos.boot-mirror.luks- 2 (removing /dev/sdb(3/4))

bgilbert

The test itself looks okay to me. I'll let @arithx or @jlebon handle the final approval.

jlebon

Looks good to me overall. Very nice work on this Sohan!

jlebon · 2021-03-01T15:29:42Z

/lgtm

openshift-ci-robot · 2021-03-01T15:29:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jlebon, sohankunkerkar

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [jlebon,sohankunkerkar]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot added the do-not-merge/work-in-progress label Nov 18, 2020

openshift-ci-robot requested review from jlebon and miabbott November 18, 2020 14:33

openshift-ci-robot added the approved label Nov 18, 2020

sohankunkerkar force-pushed the add-boot-mirror branch 6 times, most recently from d86e879 to 0c6243f Compare November 25, 2020 19:25

sohankunkerkar force-pushed the add-boot-mirror branch 12 times, most recently from 4ddf631 to abd8e15 Compare December 8, 2020 20:49

sohankunkerkar force-pushed the add-boot-mirror branch 5 times, most recently from 8ba0544 to 728b9ec Compare December 10, 2020 16:52

sohankunkerkar force-pushed the add-boot-mirror branch 2 times, most recently from 95725b0 to aba71f8 Compare December 18, 2020 04:47

bgilbert reviewed Dec 19, 2020

View reviewed changes

sohankunkerkar force-pushed the add-boot-mirror branch from 2c6cf16 to 40251f2 Compare February 8, 2021 03:30

sohankunkerkar mentioned this pull request Feb 8, 2021

Add support for providing Butane configs in native kola tests #2035

Closed

arithx reviewed Feb 9, 2021

View reviewed changes

sohankunkerkar mentioned this pull request Feb 10, 2021

RFE: Support running the boot-mirror RAID1 test on UEFI #2039

Open

sohankunkerkar force-pushed the add-boot-mirror branch from 40251f2 to 430948c Compare February 10, 2021 11:03

bgilbert reviewed Feb 11, 2021

View reviewed changes

Comment thread mantle/platform/qemu.go Outdated

Comment thread mantle/kola/tests/misc/boot-mirror.go Outdated

Comment thread mantle/kola/tests/misc/boot-mirror.go

sohankunkerkar force-pushed the add-boot-mirror branch from 430948c to 7d1a7cf Compare February 12, 2021 03:58

sohankunkerkar requested a review from bgilbert February 23, 2021 04:36

sohankunkerkar force-pushed the add-boot-mirror branch from 7d1a7cf to 92d2977 Compare February 23, 2021 11:37

bgilbert reviewed Feb 24, 2021

View reviewed changes

Comment thread mantle/kola/tests/misc/boot-mirror.go

sohankunkerkar force-pushed the add-boot-mirror branch from 92d2977 to 7d7458e Compare February 24, 2021 05:56

sohankunkerkar added 2 commits February 24, 2021 02:38

mantle: go mod tidy

8c30923

platform: add qemu specific information for boot-mirror RAID1 tests

f85bc91

sohankunkerkar force-pushed the add-boot-mirror branch from 7d7458e to 846b3f7 Compare February 24, 2021 07:38

bgilbert reviewed Feb 25, 2021

View reviewed changes

Comment thread mantle/kola/tests/misc/boot-mirror.go Outdated

sohankunkerkar force-pushed the add-boot-mirror branch from 846b3f7 to fd159e3 Compare February 25, 2021 09:10

sohankunkerkar requested a review from jlebon February 25, 2021 13:43

jlebon reviewed Feb 25, 2021

View reviewed changes

Comment thread mantle/kola/tests/util/luks.go Outdated

jlebon reviewed Feb 25, 2021

View reviewed changes

Comment thread mantle/kola/tests/util/luks.go Outdated

jlebon approved these changes Feb 25, 2021

View reviewed changes

Comment thread mantle/kola/tests/misc/boot-mirror.go Outdated

sohankunkerkar added 3 commits February 28, 2021 23:56

util: refactor lukstest logic for better utilization

45ba8f2

kola: add support for filtering tests by firmware

c605548

kola: add test to validate boot-mirror RAID1 changes

1b88d51

sohankunkerkar force-pushed the add-boot-mirror branch from fd159e3 to 1b88d51 Compare March 1, 2021 05:08

sohankunkerkar requested a review from jlebon March 1, 2021 15:27

openshift-ci-robot assigned jlebon Mar 1, 2021

openshift-ci-robot added the lgtm label Mar 1, 2021

openshift-merge-robot merged commit aee1d3e into coreos:master Mar 1, 2021

Conversation

sohankunkerkar commented Nov 18, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bgilbert left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jlebon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jlebon commented Mar 1, 2021

Uh oh!

openshift-ci-robot commented Mar 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants