baremetal: Prevent race condition when adding HardwareDetails #3809
Conversation
|
/assign @stbenjam |
|
Nice. How concerned are we about actually hitting the race? Should we backport this to 4.5/4.4? |
You tell me ;) |
|
/test e2e-metal-ipi |
3 similar comments
|
/test e2e-metal-ipi |
|
/test e2e-metal-ipi |
|
/test e2e-metal-ipi |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
/test e2e-metal-ipi |
1 similar comment
|
/test e2e-metal-ipi |
|
Are we sure tihs is working properly? The failure looks like workers didn't come up. /test e2e-metal-ipi |
| @@ -66,6 +66,9 @@ func Hosts(config *types.InstallConfig, machines []machineapi.Machine) (*HostSet | |||
| ObjectMeta: metav1.ObjectMeta{ | |||
| Name: host.Name, | |||
| Namespace: "openshift-machine-api", | |||
| // Pause reconciliation until we can annotate with the initial | |||
| // status containing the HardwareDetails | |||
| Annotations: map[string]string{"baremetalhost.metal3.io/paused": ""}, | |||
There was a problem hiding this comment.
This annotation should only be set on masters. Moving it down to the block starting on line 84 should accomplish that.
We add HardwareDetails to a BareMetalHost resource by including them in initial Status data that is attached to the Host as an annotation. This annotation must be present before the BareMetalHost is first reconciled (thus creating the Status subresource), otherwise it will be ignored. It is not possible to add the annotation at the time the BareMetalHost is created. Therefore, we were previously relying on it being added before the baremetal-operator is up and running. In practice this works because the baremetal-operator takes a long time to start up, but in fact this is a race. Prevent the race by setting the baremetalhost.metal3.io/paused annotation on the CR at the time it is created, and removing it again when the status annotation is added. This annotation prevents the operator from attempting to reconcile the resource.
zaneb
force-pushed
the
hardwaredetails-race
branch
from
9e39b17
to
1f04061
Compare
|
/label platform/baremetal |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: stbenjam 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 |
openshift-ci-robot
added
the
approved
|
/lgtm |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
4 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
4 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
@zaneb: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
We add HardwareDetails to a BareMetalHost resource by including them in
initial Status data that is attached to the Host as an annotation. This
annotation must be present before the BareMetalHost is first reconciled
(thus creating the Status subresource), otherwise it will be ignored.
It is not possible to add the annotation at the time the BareMetalHost
is created. Therefore, we were previously relying on it being added
before the baremetal-operator is up and running. In practice this works
because the baremetal-operator takes a long time to start up, but in
fact this is a race.
Prevent the race by setting the baremetalhost.metal3.io/paused
annotation on the CR at the time it is created, and removing it again
when the status annotation is added. This annotation prevents the
operator from attempting to reconcile the resource.