OCPBUGS-49728: Adapt MCC to use LayeredNodeState and remove LayeredPoolState #4841
Conversation
openshift-ci-robot
added
jira/severity-moderate
|
@djoshy: This pull request references Jira Issue OCPBUGS-49728, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: 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-robot
added
the
jira/valid-bug
openshift-ci
bot
requested review from
sergiordlr,
cheesesashimi and
RishabhSaini
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: djoshy 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
| if mosb != nil && mosc != nil { | ||
| mosbState := ctrlcommon.NewMachineOSBuildState(mosb) | ||
| // It seems like pool image annotations are no longer being used, so node specific checks were required here | ||
| if layered && mosbState.IsBuildSuccess() && mosb.Spec.MachineConfig.Name == pool.Spec.Configuration.Name && isNodeDoneAt(node, pool, layered) && lns.IsCurrentImageEqualToBuild(mosc) { |
There was a problem hiding this comment.
I've updated this check to remove MachineOSBuild successful condition comparison, since I think this can be solely determined by the node's annotation against the MOSB and MOSC objects while layered. Happy to add it back if I'm missing something here!
| return val == "" || !ok | ||
| // If the MOSC does not have an image, but the node has an older image annotation, the image is still likely | ||
| // being built. | ||
| return false |
There was a problem hiding this comment.
Wanted to double check this thought: This function will now only be called when the node is layered. As a result, if the image annotation is missing for whatever reason, we should always return false. In the old implementation, this was used in non layered mode too, so it returned true when the annotation was missing.
| // we need to ensure the node controller is triggered at all the same times | ||
| // when using this new system | ||
| // we know the mosc+mosb can trigger one another and cause a build, but if the node controller | ||
| // can't set this anno, and subsequently cannot trigger the daemon to update, we need to rework. | ||
| lns.SetDesiredStateFromMachineOSConfig(mosc, mosb) |
There was a problem hiding this comment.
I removed these checks because:
- The
getAllCandidateMachinesfunction already returns a list of viable candidate nodes, which does this by comparing the node's annotation against the MCP/OCL objects as required. - This list is then passed to
updateCandidateMachineswhich truncates the list based on the pool's capacity. - Finally, the list is passed to the above
updateCandidateNodefunction which was essentially re-doing the checksgetAllCandidateMachinesperformed.
The last check in step (3) seemed excessive, so I removed those checks from this function to simplify things and make it easier to read.
|
@djoshy: 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-sigs/prow repository. I understand the commands that are listed here. |
- What I did
LayeredNodeStateobjectMachineConfigPoolsLayeredNodeStatestructI have a couple of open questions, for which I'll leave comments below!
- How to verify it