OCPBUGS-32105: Fix race to mark node Joined #823
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@zaneb: This pull request references Jira Issue OCPBUGS-32105, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
added
the
size/S
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #823 +/- ##
==========================================
+ Coverage 54.74% 57.32% +2.57%
==========================================
Files 16 16
Lines 3394 3803 +409
==========================================
+ Hits 1858 2180 +322
- Misses 1364 1416 +52
- Partials 172 207 +35
|
If a host is in the Installed state already (which can occur when the assisted-installer-controller sets the progress to Done), don't try to set the progress to Joined as it will not only never succeed, but also take 30+ minutes of unlogged retries inside the client before an error is returned. This narrows the window in which this can occur, but if the bootstrap assisted-installer reads the Host before the assisted-installer-controller updates the status, this could still occur. Ensure any failed requests are retried by not adding the Node to the readyMasters list until the Progress has been set to either Joined or Done (the latter triggers a change of Status to Installed). Improve debugging by not logging different request_ids for messages corresponding to a single request.
Since 4xx error codes indicate a problem on the client side, most of them cannot be usefully retried at the HTTP transport level. e.g. if a 409 Conflict is returned in response to a PUT request, then we need to fetch the resource again with a GET before creating a new PUT request. Blocking for 30+ minutes in the original PUT call (without logging) is not helpful; we want the transport to return immediately so we can try again. Retry on only those 4xx error codes where it is conceivable that trying the same request again might work.
openshift-ci-robot
added
jira/valid-bug
|
@zaneb: This pull request references Jira Issue OCPBUGS-32105, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
In response to this: 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 openshift-eng/jira-lifecycle-plugin repository. |
| @@ -776,16 +776,18 @@ func (i *installer) updateReadyMasters(nodes *v1.NodeList, readyMasters *[]strin | |||
| ctx := utils.GenerateRequestContext() | |||
| log := utils.RequestIDLogger(ctx, i.log) | |||
| log.Infof("Found a new ready master node %s with id %s", node.Name, node.Status.NodeInfo.SystemUUID) | |||
| *readyMasters = append(*readyMasters, node.Name) | |||
There was a problem hiding this comment.
I think better to leave it as is, in case we failed to update status but node is ready we better to exit and continue installation, no? Nothing critical in not setting Joined state it will be handle in controller afterwards
There was a problem hiding this comment.
Yeah, it's not as big a deal as I originally thought, because the controller will handle it anyway. But this does guarantee that everything is in the state we expect before we carry on to other work in here. It's still robust against unexpected nodes joining, and if the controller does win the race then this will still work on the next attempt.
| if !ok { | ||
| return fmt.Errorf("node %s is not in inventory hosts", node.Name) | ||
| } | ||
| ctx = utils.GenerateRequestContext() |
There was a problem hiding this comment.
Don't we want to set request id?
There was a problem hiding this comment.
It's already set on line 776. We don't need to generate another one, we haven't even made a request with the first one yet. And this made it really hard to debug, since the request ID that showed up in the assisted-service log never appeared in the logs here.
|
/retest-required |
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tsorya, zaneb 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
bot
added
the
approved
|
@zaneb: all tests passed! 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. |
|
@zaneb: Jira Issue OCPBUGS-32105: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-32105 has been moved to the MODIFIED state. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
[ART PR BUILD NOTIFIER] This PR has been included in build ose-agent-installer-csr-approver-container-v4.16.0-202405021917.p0.gf548d32.assembly.stream.el9 for distgit ose-agent-installer-csr-approver. |
|
[ART PR BUILD NOTIFIER] This PR has been included in build ose-agent-installer-orchestrator-container-v4.16.0-202405021917.p0.gf548d32.assembly.stream.el9 for distgit ose-agent-installer-orchestrator. |
|
Fix included in accepted release 4.16.0-0.nightly-2024-05-03-091818 |
|
/cherry-pick release-4.15 |
|
@zaneb: new pull request created: #859 In response to this:
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-sigs/prow repository. |
|
/cherry-pick release-ocm-2.10 |
|
@carbonin: new pull request created: #900 In response to this:
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-sigs/prow repository. |
In the race between assisted-installer on the bootstrap node and assisted-installer-controller on the cluster control plane to mark nodes as Joined, a win for the assisted-installer-controller would cause the bootstrapping process to lock up for 30+ minutes.
Prevent this by not retrying HTTP requests that receive a 409 response at the HTTP transport level. Instead, retry at the logic level and avoid making requests that cannot succeed.