Add parallel test for new baremetal resources by bfournie · Pull Request #26541 · openshift/origin

bfournie · 2021-10-26T13:03:48Z

Add a test to check that the hostfirmwaresettings and firmwareschema resources are created and populated with BIOS configuration settings.

ardaguclu

Thanks @bfournie, this looks good to me. I left some comments.

bfournie · 2021-10-28T21:23:00Z

/retest

ardaguclu · 2021-11-03T06:26:20Z

/lgtm
/test e2e-metal-ipi
/test e2e-metal-ipi-ovn-ipv6
/test e2e-metal-ipi-ovn-dualstack

andfasano · 2021-11-03T10:22:24Z

@bfournie looks like the tests failed in the latest run

bfournie · 2021-11-03T12:08:19Z

@bfournie looks like the tests failed in the latest run

Yeah its looks like the resources were created but occasionally can get 0 bios config settings returned from the provisioner (I had seen this on masters but I guess its possible on workers too). I will have a fix for that in bmo, until then I'll remove the check for settings not empty

bfournie · 2021-11-03T17:07:02Z

/test e2e-metal-ipi
/test e2e-metal-ipi-ovn-ipv6
/test e2e-metal-ipi-ovn-dualstack
/test e2e-gcp-upgrade

bfournie · 2021-11-04T13:40:27Z

/retest-required

bfournie · 2021-11-04T18:04:18Z

/test e2e-aws-serial
/test e2e-gcp-upgrade
/test e2e-metal-ipi
/test e2e-metal-ipi-ovn-ipv6
/test e2e-metal-ipi-ovn-dualstack

ardaguclu · 2021-11-08T06:20:32Z

/lgtm

andfasano · 2021-11-08T09:48:30Z

+		for _, h := range hosts.Items {
+			hostName := getStringField(h, "baremetalhost", "metadata.name")
+
+			g.By("check that each baremetalhost has a corresponding hostfirmwaresettings")


This message will be printed for each host, but it does not show any information about that. It could be improved by also adding the current hostname for more clarity

Good point. Included the hostname in the debug output.

andfasano · 2021-11-08T09:49:17Z

+
+			// Reenable this when fix to prevent settings with 0 entries is in BMO
+			// g.By("check that hostfirmwaresettings settings have been populated")
+			// expectStringMapField(*hfs, "hostfirmwaresettings", "status.settings").ToNot(o.BeEmpty())


Do we still need to keep this commented code?

Yes, per comment, the code should be uncommented when metal3-io/baremetal-operator#995 merges that will prevent empty settings fields.

andfasano · 2021-11-08T09:51:16Z

+			// expectStringMapField(*hfs, "hostfirmwaresettings", "status.settings").ToNot(o.BeEmpty())
+
+			g.By("check that hostfirmwaresettings conditions show resource is valid")
+			expectSliceField(*hfs, "hostfirmwaresettings", "status.conditions").ToNot(o.BeEmpty())


Isn't something already somehow addressed within the checkConditionStatus method?

I think its necessary to check that conditions is not empty but I've moved this check to checkConditionStatus and simplified it.

Add a test to check that the hostfirmwaresettings and firmwareschema resources are created and populated with BIOS configuration settings.

andfasano · 2021-11-08T14:54:37Z

/test e2e-metal-ipi
/test e2e-metal-ipi-ovn-ipv6
/test e2e-metal-ipi-ovn-dualstack
/test e2e-metal-ipi-virtualmedia
/test e2e-metal-ipi-serial

Changes look fine to me, let's see first a run on the various e2e-metal-ipi configurations

bfournie · 2021-11-08T16:12:52Z

/test e2e-metal-ipi
/test e2e-metal-ipi-ovn-ipv6
/test e2e-metal-ipi-ovn-dualstack
/test e2e-metal-ipi-virtualmedia
/test e2e-metal-ipi-serial
/test e2e-gcp

bfournie · 2021-11-08T19:08:07Z

/test e2e-metal-ipi-ovn-ipv6
/test e2e-metal-ipi-virtualmedia
/test e2e-agnostic-cmd
/test e2e-gcp-upgrade

bfournie · 2021-11-08T21:43:39Z

/test e2e-metal-ipi-ovn-ipv6
/test e2e-agnostic-cmd

bfournie · 2021-11-09T01:16:14Z

/retest

bfournie · 2021-11-09T11:47:54Z

/test e2e-metal-ipi-ovn-ipv6

andfasano · 2021-11-09T14:39:31Z

/lgtm

bfournie · 2021-11-09T15:11:06Z

/test e2e-metal-ipi-ovn-ipv6

bfournie · 2021-11-09T18:27:22Z

/assign @derekwaynecarr

andfasano · 2021-11-12T16:41:32Z

cc @ardaguclu

ardaguclu · 2021-11-12T16:59:06Z

/approve

bfournie · 2021-11-15T00:45:41Z

cc @bparees

bparees · 2021-11-15T14:29:26Z

/approve

openshift-ci · 2021-11-15T14:35:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andfasano, ardaguclu, bfournie, bparees, elfosardo

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:

~~test/extended/baremetal/OWNERS~~ [andfasano,ardaguclu,bparees]
~~test/extended/util/annotate/OWNERS~~ [bparees]

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

openshift-bot · 2021-11-15T17:55:13Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

bfournie · 2021-11-15T21:38:16Z

/test e2e-gcp

openshift-bot · 2021-11-16T01:41:12Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-11-16T02:20:13Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-ci Bot requested review from bparees and smarterclayton October 26, 2021 13:06

ardaguclu reviewed Oct 26, 2021

View reviewed changes

Comment thread test/extended/baremetal/hosts.go

Comment thread test/extended/baremetal/hosts.go Outdated

Comment thread test/extended/baremetal/hosts.go Outdated

Comment thread test/extended/baremetal/hosts.go Outdated

Comment thread test/extended/baremetal/hosts.go Outdated

andfasano reviewed Oct 26, 2021

View reviewed changes

Comment thread test/extended/baremetal/hosts.go Outdated

andfasano reviewed Oct 26, 2021

View reviewed changes

Comment thread test/extended/baremetal/hosts.go Outdated

andfasano reviewed Oct 26, 2021

View reviewed changes

Comment thread test/extended/baremetal/hosts.go Outdated

bfournie force-pushed the bm-hfs-resources branch 4 times, most recently from 1ccbffa to af4e179 Compare October 27, 2021 13:05

andfasano reviewed Oct 27, 2021

View reviewed changes

Comment thread test/extended/baremetal/hosts.go Outdated

bfournie force-pushed the bm-hfs-resources branch from af4e179 to e845562 Compare October 27, 2021 17:48

openshift-ci Bot assigned ardaguclu Nov 3, 2021

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Nov 3, 2021

bfournie force-pushed the bm-hfs-resources branch from e845562 to 109148c Compare November 3, 2021 13:44

openshift-ci Bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 3, 2021

bfournie force-pushed the bm-hfs-resources branch from 109148c to 14216cb Compare November 3, 2021 14:24

bfournie force-pushed the bm-hfs-resources branch 2 times, most recently from 66b9a1a to d1e05f9 Compare November 4, 2021 13:13

bfournie force-pushed the bm-hfs-resources branch 2 times, most recently from 7c44f8f to 18dd121 Compare November 4, 2021 14:17

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Nov 8, 2021

elfosardo approved these changes Nov 8, 2021

View reviewed changes

andfasano reviewed Nov 8, 2021

View reviewed changes

Add parallel test for new baremetal resources

d7719b2

Add a test to check that the hostfirmwaresettings and firmwareschema resources are created and populated with BIOS configuration settings.

bfournie force-pushed the bm-hfs-resources branch from 18dd121 to d7719b2 Compare November 8, 2021 13:02

openshift-ci Bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 8, 2021

openshift-ci Bot assigned andfasano Nov 9, 2021

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Nov 9, 2021

openshift-ci Bot assigned derekwaynecarr Nov 9, 2021

openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 15, 2021

openshift-merge-robot merged commit 5f2fd6c into openshift:master Nov 16, 2021

bfournie deleted the bm-hfs-resources branch November 16, 2021 11:33

Conversation

bfournie commented Oct 26, 2021

Uh oh!

ardaguclu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bfournie commented Oct 28, 2021

Uh oh!

ardaguclu commented Nov 3, 2021

Uh oh!

andfasano commented Nov 3, 2021

Uh oh!

bfournie commented Nov 3, 2021

Uh oh!

bfournie commented Nov 3, 2021

Uh oh!

bfournie commented Nov 4, 2021

Uh oh!

bfournie commented Nov 4, 2021

Uh oh!

ardaguclu commented Nov 8, 2021

Uh oh!

andfasano Nov 8, 2021

Choose a reason for hiding this comment

Uh oh!

bfournie Nov 8, 2021

Choose a reason for hiding this comment

Uh oh!

andfasano Nov 8, 2021

Choose a reason for hiding this comment

Uh oh!

bfournie Nov 8, 2021

Choose a reason for hiding this comment

Uh oh!

andfasano Nov 8, 2021

Choose a reason for hiding this comment

Uh oh!

bfournie Nov 8, 2021

Choose a reason for hiding this comment

Uh oh!

andfasano commented Nov 8, 2021

Uh oh!

bfournie commented Nov 8, 2021

Uh oh!

bfournie commented Nov 8, 2021

Uh oh!

bfournie commented Nov 8, 2021

Uh oh!

bfournie commented Nov 9, 2021

Uh oh!

bfournie commented Nov 9, 2021

Uh oh!

andfasano commented Nov 9, 2021

Uh oh!

bfournie commented Nov 9, 2021

Uh oh!

bfournie commented Nov 9, 2021

Uh oh!

andfasano commented Nov 12, 2021

Uh oh!

ardaguclu commented Nov 12, 2021

Uh oh!

bfournie commented Nov 15, 2021

Uh oh!

bparees commented Nov 15, 2021

Uh oh!

openshift-ci Bot commented Nov 15, 2021

Uh oh!

openshift-bot commented Nov 15, 2021

Uh oh!

bfournie commented Nov 15, 2021