OCPBUGS-94109: cherry-pick reconciler optimizations#503
Conversation
It's pointless since we only use node UUID, which is already known. Fix the unit test to check what it actually claims to check. Signed-off-by: Dmitry Tantsur <dtantsur@protonmail.com> (cherry picked from commit b6462d8)
The only actual purpose of the call there is to see if firmware components are supported by the Node. Which, in turn, is needed to determine if the HFC object needs to be created by the BMH controller. This commit changes the behavior slightly: now HFC is not created when the list of components is empty. This happens both when firmware settings are not supported and during early stages of enrollment. Most users will only see that the HFC resource may get created slightly later than before. Signed-off-by: Dmitry Tantsur <dtantsur@protonmail.com> (cherry picked from commit 6835bdb)
Replace five old delay variables with two: - shortRetryDelay (3s): used when we asked Ironic to do something and are waiting for it to finish (state changes, power operations, abort requests, lock retries) - longRetryDelay (10s): used when hardware is doing something slow like booting a ramdisk (inspection, cleaning, servicing, deployment) Also add random jitter (0-25%) to all delays so that many hosts don't hit Ironic at the exact same time. Signed-off-by: Sunnatillo <sunnat.samadov@est.tech>
Signed-off-by: Sunnatillo <sunnat.samadov@est.tech>
All of our production code calls TryInit right after NewProvisioner, so there is little point for them to be separate. A new ErrNotReady error indicates what returning ready=false used to indicate. This simplification is needed for my follow-up changes to improve caching of Ironic parameters between reconciliations. Signed-off-by: Dmitry Tantsur <dtantsur@protonmail.com>
Signed-off-by: Dmitry Tantsur <dtantsur@protonmail.com>
Currently, BMO checks the available features and the number of drivers on each reconciliation. These two calls add up when a lot of hosts are going through provisioning. It's not reasonable to expect Ironic to constantly change, and even if it suddenly crashes, the worst outcome is less optimal error messages. This change caches the result of the endpoint validation for 5 seconds (subject to further tuning). When Ironic resource is used for configuration, the resulting client is also cached. Only successful checks are cached, the behavior on errors is the same as before. Cache TTL can be tuned via environment variable IRONIC_CLIENT_CACHE_TTL, including an ability to opt out of caching by setting it to 0. The change has some implications for unit tests since bypassing API calls now requires a different approach. Signed-off-by: Dmitry Tantsur <dtantsur@protonmail.com>
The current code in BMO fails to build because of a missing import Error: hostfirmwarecomponents_controller.go:132:6: undefined: errors (typecheck) This commit fixes this issue. Signed-off-by: Iury Gregory Melo Ferreira <imelofer@redhat.com>
openshift-ci-robot
added
jira/valid-reference
|
@dtantsur: This pull request references Jira Issue OCPBUGS-94109, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn 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. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@dtantsur: This pull request references Jira Issue OCPBUGS-94109, which is valid. The bug has been moved to the POST state. 7 validation(s) were run on this bug
DetailsIn 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dtantsur The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
@dtantsur: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
|
/label backport-risk-assessed I realize that these changes carry certain risks, but I don't believe they're very high, and I'd also rather discover them in 4.22 than when preparing the next release. |
openshift-ci
Bot
added
the
backport-risk-assessed
|
/lgtm |
5 participants
Adds the following commits from 5.0: