-
Notifications
You must be signed in to change notification settings - Fork 247
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.
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
🌱 Re-inspection by annotation test added #1402
Conversation
/metal3-bmo-e2e-test |
Sorry for unrelated triggers... we have issues with prow so I'm debugging. |
/test unit |
1 similar comment
/test unit |
2af3d6d
to
9fdb68c
Compare
9fdb68c
to
245ef64
Compare
/metal3-bmo-e2e-test |
test/e2e/config/fixture.yaml
Outdated
@@ -31,6 +31,7 @@ intervals: | |||
inspection/wait-inspecting: ["5s", "10ms"] | |||
inspection/wait-available: ["5s", "1ms"] | |||
external-inspection/wait-available: ["5s", "1ms"] | |||
re-inspection/wait-available: ["5m", "1s"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably we can reuse here inspection/wait-available
?
Edit: I think probably inspection/wait-available
should be changed to default/wait-available
since this is what we have in ironic.yaml
. Probably missed to change it here before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a new one because the others were very small and re-inspection was taking some time. Should I change default/wait-available:
in both fixture
and ironic
to ["10m", "10s"]
and use that instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixture is different it should not take much time. Let me check!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
available after re-inspection almost takes 3 minutes I think:
STEP: waiting for the BMH to become available after re-inspection @ 10/31/23 08:40:01.681
STEP: checking that the hardware details are corrected after re-inspection @ 10/31/23 08:42:54.127
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For fixture it takes less than a second most of the time.
STEP: adding InspectAnnotation to re-inspect @ 10/31/23 10:54:30.718
STEP: waiting for the BMH to be in inspecting state after inspection annotaion @ 10/31/23 10:54:30.739
STEP: waiting for the BMH to become available after re-inspection @ 10/31/23 10:54:30.756
STEP: checking that the hardware details are corrected after re-inspection @ 10/31/23 10:54:30.961
Fixture is not what runs of you use the ci-e2e.sh
script. If you want to run it you need to check the bottom of the readme here: https://github.com/metal3-io/baremetal-operator/blob/main/test/e2e/README.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more thing! If you run into any issues running the tests with the fixture provider on Mac, please bring them up! I want this to be easy for developers to run 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was running tests in VM but I will try on my mac too.
test/e2e/config/ironic.yaml
Outdated
@@ -32,6 +32,7 @@ intervals: | |||
default/wait-registering: ["1m", "5s"] | |||
inspection/wait-registration-error: ["1m", "5s"] | |||
external-inspection/wait-available: ["20s", "1s"] | |||
re-inspection/wait-available: ["5m", "1s"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we could probably reuse default/wait-available
. I don't think re-inspection would have different timing from normal inspection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect!
test/e2e/inspection_test.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we don't need the changes in this file. It is enough with the separate re-inspection test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh I forgot to remove it. let me change it
test/e2e/re_inspection_test.go
Outdated
By("checking that the hardware details are corrected after re-inspection") | ||
key = types.NamespacedName{Namespace: bmh.Namespace, Name: bmh.Name} | ||
Expect(clusterProxy.GetClient().Get(ctx, key, &bmh)).To(Succeed()) | ||
Expect(bmh.Status.HardwareDetails.Hostname).To(Equal(rightHostName)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to make this configurable so that the test can be used in other contexts. I think it will be fine to just add a variable to the e2e config with the expected hostname. To verify that it works, we can check with the fixture provider. The hardware details that it returns are hard coded here:
baremetal-operator/pkg/provisioner/fixture/fixture.go
Lines 136 to 176 in 3874f1e
&metal3api.HardwareDetails{ | |
RAMMebibytes: 128 * 1024, | |
NIC: []metal3api.NIC{ | |
{ | |
Name: "nic-1", | |
Model: "virt-io", | |
MAC: "ab:cd:12:34:56:78", | |
IP: "192.168.100.1", | |
SpeedGbps: 1, | |
PXE: true, | |
}, | |
{ | |
Name: "nic-2", | |
Model: "e1000", | |
MAC: "12:34:56:78:ab:cd", | |
IP: "192.168.100.2", | |
SpeedGbps: 1, | |
PXE: false, | |
}, | |
}, | |
Storage: []metal3api.Storage{ | |
{ | |
Name: "disk-1 (boot)", | |
Rotational: false, | |
SizeBytes: metal3api.TebiByte * 93, | |
Model: "Dell CFJ61", | |
}, | |
{ | |
Name: "disk-2", | |
Rotational: false, | |
SizeBytes: metal3api.TebiByte * 93, | |
Model: "Dell CFJ61", | |
}, | |
}, | |
CPU: metal3api.CPU{ | |
Arch: "x86_64", | |
Model: "FancyPants CPU", | |
ClockMegahertz: 3.0 * metal3api.GigaHertz, | |
Flags: []string{"fpu", "hypervisor", "sse", "vmx"}, | |
Count: 1, | |
}, |
As you can see it does not set any hostname, so for fixture we would set ""
. If we can get the test passing on both fixture and ironic with separate e2e configs then it should be flexible enough for other environments also 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added in e2e config, please check
245ef64
to
8f19c5a
Compare
8f19c5a
to
ebbbefa
Compare
/metal3-bmo-e2e-test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome job!
/lgtm
/cc @kashifest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kashifest The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/test-ubuntu-integration-main |
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #1369