-
Notifications
You must be signed in to change notification settings - Fork 436
MCO-1580: MCO-1581: Achieving parity with MCO node disruption frequency #4996
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MCO-1580: MCO-1581: Achieving parity with MCO node disruption frequency #4996
Conversation
b3d97fc
to
a69117f
Compare
a69117f
to
435bbef
Compare
@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. |
8452b7a
to
32555ad
Compare
ef4dc03
to
621dc6c
Compare
db06f06
to
8313511
Compare
451b199
to
01d220a
Compare
@cheesesashimi or @umohnani8 is going to give the final LGTM on this. We have agreed to override tests if needed, as the ocl test has passed (many times) locally, and I have gotten consistent e2e-gcp passes in previous iterations of this PR |
01d220a
to
a76e003
Compare
…g and reusing MOSB's with updated rendered spec
a76e003
to
97cecae
Compare
/lgtm |
/unhold |
/retest-required |
@@ -1561,20 +1419,17 @@ func newMachineConfigDiff(oldConfig, newConfig *mcfgv1.MachineConfig) (*machineC | |||
units: !reflect.DeepEqual(oldIgn.Systemd.Units, newIgn.Systemd.Units), | |||
kernelType: canonicalizeKernelType(oldConfig.Spec.KernelType) != canonicalizeKernelType(newConfig.Spec.KernelType), | |||
extensions: !(extensionsEmpty || reflect.DeepEqual(oldConfig.Spec.Extensions, newConfig.Spec.Extensions)), | |||
}, nil | |||
} | |||
oclEnabled: (oldOCLImage != "" || newOCLImage != "") || (oldOCLImage != "" && newOCLImage != ""), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the first block and second block here seems redundant? i.e. if either of oldOCL or newOCL is not empty we consider it enabled already, what's the second check for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forget why that was added. I pulled the code down and ran the tests locally and we can probably do away with the second part of this check (the oldOCLImage != "" && newOCLImage != ""
section).
/override ci/prow/e2e-gcp-op-single-node Those tests are known failures with gcp infra, so overriding to reduce retests /hold @cheesesashimi please unhold if you're happy with the state of the PR |
@yuqi-zhang: Overrode contexts on behalf of yuqi-zhang: ci/prow/e2e-gcp-op, ci/prow/e2e-gcp-op-single-node 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. |
return maybeAddMachineConfigInfo(ccAnnotation, err) | ||
} | ||
// We might be able to drop this method entirely since I'm not sure if it's actually needed. | ||
func (dn *Daemon) triggerUpdateWithMachineConfig(currentConfig, desiredConfig *mcfgv1.MachineConfig, _ bool) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (non-blocking): Remove the bool
parameter since it's not being used.
@@ -1561,20 +1419,17 @@ func newMachineConfigDiff(oldConfig, newConfig *mcfgv1.MachineConfig) (*machineC | |||
units: !reflect.DeepEqual(oldIgn.Systemd.Units, newIgn.Systemd.Units), | |||
kernelType: canonicalizeKernelType(oldConfig.Spec.KernelType) != canonicalizeKernelType(newConfig.Spec.KernelType), | |||
extensions: !(extensionsEmpty || reflect.DeepEqual(oldConfig.Spec.Extensions, newConfig.Spec.Extensions)), | |||
}, nil | |||
} | |||
oclEnabled: (oldOCLImage != "" || newOCLImage != "") || (oldOCLImage != "" && newOCLImage != ""), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forget why that was added. I pulled the code down and ran the tests locally and we can probably do away with the second part of this check (the oldOCLImage != "" && newOCLImage != ""
section).
return mcDiff, nil | ||
// If OCL is enabled, compute OS update and revertFromOCL separately. | ||
diff.osUpdate = oldOCLImage != newOCLImage || force | ||
diff.revertFromOCL = newOCLImage == "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought (non-blocking): We can tighten up this check by doing something like diff.revertFromOCL = newOCLImage == "" && oldOCLImage != ""
.
|
||
// Delete the third MOSB and the image should be deleted also | ||
t.Logf("Deleting MachineOSBuild %q", thirdMOSB.Name) | ||
// waitForMOSCToGetNewPullspec(ctx, t, cs, moscName, secondImagePullspec) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought (non-blocking): If this isn't being used, it's better to delete the line and let Git worry about what was once here.
/unhold |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cheesesashimi, dkhater-redhat, umohnani8 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 |
/override ci/prow/e2e-gcp-op-single-node Reapplying |
@yuqi-zhang: Overrode contexts on behalf of yuqi-zhang: ci/prow/e2e-gcp-op, ci/prow/e2e-gcp-op-single-node 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. |
/test unit |
@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. |
a4ec776
into
openshift:main
[ART PR BUILD NOTIFIER] Distgit: ose-machine-config-operator |
- What I did
Updated the reconciler so that only changes to kernel arguments, OSImageURL, or extension bundles will trigger a new image build and node reboot. All other MachineConfig changes now reuse the existing MOSB and not cause a node reboot.
- How to verify it
Apply an MC that only tweaks a live-patchable field, observe new MOSB creation (with same image) no node drain and reboot.
Apply an MC that changes kernel args, OSImageURL, or adds/removes an extension, observe new MOSB creation (with new image), node drain and reboot.
- Description for the changelog