DRAFT MCO-1580: MCO-1581: Achieving parity with MCO node disruption frequency #4871
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
openshift-ci
bot
added
the
do-not-merge/work-in-progress
dkhater-redhat
changed the title
openshift-ci
bot
added
the
approved
openshift-merge-robot
added
the
needs-rebase
dkhater-redhat
force-pushed
the
skip-ignition-apply
branch
from
3a40dec to
e9a40c2
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dkhater-redhat 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 |
dkhater-redhat
commented
Mar 11, 2025
pkg/controller/build/buildrequest/assets/Containerfile.on-cluster-build-template
Outdated
Show resolved
Hide resolved
dkhater-redhat
force-pushed
the
skip-ignition-apply
branch
from
e9a40c2 to
5a2b37b
Compare
openshift-merge-robot
removed
the
needs-rebase
dkhater-redhat
force-pushed
the
skip-ignition-apply
branch
5 times, most recently
from
95ee82b to
a12c4de
Compare
| } | ||
| ) | ||
|
|
||
| func HashMachineConfigSpec(spec mcfgv1.MachineConfigSpec) (string, error) { |
There was a problem hiding this comment.
praise: Nicely done with this!
suggestion: Migrate rendercontroller to use this function instead of the one it's currently using. We can also migrate the MachineOSBuild function to use this too if we make it slightly more generic like this:
func GetHashForName(in interface{}) (string, error) {
data, err := yaml.Marshal(in)
if err != nil {
return "", err
}
h, err := hashData(data)
if err != nil {
return "", err
}
return fmt.Sprintf("%x", h), nil
}
dkhater-redhat
force-pushed
the
skip-ignition-apply
branch
2 times, most recently
from
b328026 to
96a9e52
Compare
dkhater-redhat
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@dkhater-redhat: This pull request references MCO-1580 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set. 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. |
dkhater-redhat
force-pushed
the
skip-ignition-apply
branch
4 times, most recently
from
a22eaf1 to
35c352e
Compare
dkhater-redhat
commented
Mar 31, 2025
| containerMC.Spec.Config = runtime.RawExtension{ | ||
| Raw: []byte(`{ | ||
| "ignition": { | ||
| "version": "3.4.0" |
dkhater-redhat
force-pushed
the
skip-ignition-apply
branch
2 times, most recently
from
9c9e192 to
71240a9
Compare
dkhater-redhat
commented
Apr 3, 2025
| @@ -1258,6 +1294,11 @@ func (ctrl *Controller) updateCandidateNode(mosc *mcfgv1.MachineOSConfig, mosb * | |||
| // getAllCandidateMachines returns all possible nodes which can be updated to the target config, along with a maximum | |||
| // capacity. It is the reponsibility of the caller to choose a subset of the nodes given the capacity. | |||
| func getAllCandidateMachines(layered bool, config *mcfgv1.MachineOSConfig, build *mcfgv1.MachineOSBuild, pool *mcfgv1.MachineConfigPool, nodesInPool []*corev1.Node, maxUnavailable int) ([]*corev1.Node, uint) { | |||
| if layered && (config == nil || build == nil) { | |||
dkhater-redhat
force-pushed
the
skip-ignition-apply
branch
16 times, most recently
from
6274734 to
e928d4c
Compare
dkhater-redhat
force-pushed
the
skip-ignition-apply
branch
from
e928d4c to
45f9661
Compare
openshift-merge-robot
removed
the
needs-rebase
Occasionally, there is a delay between the time that a new rendered MachineConfig is produced and OCL begins a build. However, a couple of things happen in the interim: - The RenderController updates the MachineConfigPool. Because of the delay mentioned above, the NodeController begins updating all of the nodes with only the new rendered MachineConfig. The OS image remains the same because the NodeController is not ensuring that the image pullspec on the MachineOSConfig is the same as the MachineOSBuild. - Because of the work done to the MCD in openshift#4825, the original check that we had to determine whether the image pullspecs were the same is no longer present. Additionally, the logic change there makes it possible for an OS update to always occur whenever OCL is enabled, further bypassing that check. This fixes that by doing two things: 1. Update the Node Controller to ensure that the both the MachineOSBuild's MachineConfig reference matches the MCP's current rendered MachineConfig while also checking that the MachineOSConfig's image pullspec matches the MachineOSBuild's. In the situation where the MachineOSBuild's pullspec is empty, this check will fail and the Node Controller will requeue. 2. Update the MCD so that even when OCL is enabled, if the OS images are the same, the OS update process is skipped.
dkhater-redhat
force-pushed
the
skip-ignition-apply
branch
from
45f9661 to
a50cdbc
Compare
dkhater-redhat
commented
Apr 8, 2025
Comment on lines
-202
to
-204
| Finalizers: []string{ | ||
| metav1.FinalizerDeleteDependents, | ||
| }, |
There was a problem hiding this comment.
did this need to be deleted
|
@dkhater-redhat: 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. |
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. 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. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
- What I did
- How to verify it
- Description for the changelog