OCPBUGS-38795: Fix defer status during recommended profile change #1142
Conversation
MarSik
force-pushed
the
fix-defer-status
branch
from
337382c
to
0b0488f
Compare
openshift-ci
bot
added
the
approved
|
After the fix (with few extra logging changes): |
MarSik
changed the title
openshift-ci-robot
added
jira/severity-important
|
@MarSik: This pull request references Jira Issue OCPBUGS-38795, 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. |
|
/jira refresh |
openshift-ci-robot
added
the
jira/valid-bug
|
@MarSik: This pull request references Jira Issue OCPBUGS-38795, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (liqcui@redhat.com), skipping review request. 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
removed
the
jira/invalid-bug
MarSik
force-pushed
the
fix-defer-status
branch
3 times, most recently
from
c6fd3da
to
712316c
Compare
Process recommended profile change even when the profile itself has not changed itself. The internal profile fingerprint tracking was not updated during recommended profile update with no internal changes.
MarSik
force-pushed
the
fix-defer-status
branch
from
712316c
to
d22c3d2
Compare
|
/retest-required |
|
@MarSik: 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-sigs/prow repository. I understand the commands that are listed here. |
|
/label acknowledge-critical-fixes-only We need this in time for 4.17 and all the tests seem to be passing. |
openshift-ci
bot
added
the
acknowledge-critical-fixes-only
|
/cc @jmencak |
| @@ -1029,6 +1029,7 @@ func (c *Controller) changeSyncerTuneD(change Change) (synced bool, err error) { | |||
| // Cache the value written to tunedRecommendFile. | |||
| c.daemon.recommendedProfile = change.recommendedProfile | |||
| klog.V(1).Infof("recommended TuneD profile updated from %q to %q [inplaceUpdate=%v nodeRestart=%v]", prevRecommended, change.recommendedProfile, inplaceUpdate, change.nodeRestart) | |||
| changeRecommend = true | |||
There was a problem hiding this comment.
Good catch with the variable not being set anywhere in this function. However, can we possibly get rid of the variable completely? Would it work if we replaced it further down just by change.nodeRestart ? I haven't tested this (yet), but looking at the code it should work. It would also resolve the slight mis-placement of the variable setting below the logging message and its not very intuitive name.
There was a problem hiding this comment.
I was thinking about that, but there are two separate triggers for this flow:
- !inplaceUpdate -- the recommended profile was changed for a live system
- change.nodeRestart -- first execution after reboot
The !inplaceUpdate is actually the case that was missed and caused the bug.
Both variables are later used for flow branching and the changeRecommend bool is the combination of both. I agree it is not easily visible or understandable, but I was a bit afraid to touch the logic tbh.
There was a problem hiding this comment.
Yeah you're right. I was just testing that and my suggestion didn't work. I agree !inplaceUpdate causes that issue.
I was a bit afraid to touch the logic tbh.
Understood. Well, I wonder if the entire flow needs a bit of cleanup / adding comments once @ffromani is back. In the meantime, I guess we can merge if this fixes the issue.
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jmencak, MarSik 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 |
|
@MarSik: Jira Issue OCPBUGS-38795: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-38795 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] Distgit: cluster-node-tuning-operator |
…enshift#1142) * OCPBUGS-38795: Fix defer status during recommended profile change Process recommended profile change even when the profile itself has not changed itself. The internal profile fingerprint tracking was not updated during recommended profile update with no internal changes. * Add e2e test for defer=update
) * OCPBUGS-28647: tuned: distinguish deferred updates (#1129) * OCPBUGS-28647: tuned: distinguish deferred updates To fully support the usecase described in OCPBUGS-28647 and fix the issue, we need to further distinguish between first-time profile change and in-place profile change. This is required to better support a GitOps flow. The key distinction is if the recommended profile changes or not, and there's a desire to defer application of changes only when a profile is updated (e.g. sysctl modified), not the first time it is applied. Thus: - first-time profile change is a change which triggers a change of the recommended profile. - in-place profile update is a change which does NOT cause a switch to a TuneD profile with a different name. This involves changes to only the contents of the currently used profile. We change the way the annotation is used. We now require a value, which can be either - always: every Tuned object annotated this way will have its application deferred. - update: every Tuned object annotated this way will be processed as usual (and as it wasn't annotated) if it's a first-time profile change, but its in-place updates will be deferred. - a new internal value "never" is also added to be used internally to mean the deferred feature is disabled entirely. User can use this value but it will explicitly disable the feature (which is disabled already by default), thus is redundant and not recommended. Signed-off-by: Francesco Romani <fromani@redhat.com> * e2e: drop tags, use labels now that we have the more powerful ginkgo labels, we can stop using tags for newer tests. Signed-off-by: Francesco Romani <fromani@redhat.com> --------- Signed-off-by: Francesco Romani <fromani@redhat.com> * OCPBUGS-38795: Fix defer status during recommended profile change (#1142) * OCPBUGS-38795: Fix defer status during recommended profile change Process recommended profile change even when the profile itself has not changed itself. The internal profile fingerprint tracking was not updated during recommended profile update with no internal changes. * Add e2e test for defer=update * NO-JIRA: deferred updates: fix in-place update handling on reboot (#1162) * log: make sure to have high verbosity in tests Signed-off-by: Francesco Romani <fromani@redhat.com> * e2e: deferred: stricter testing add stricter check for the node condition when validating OCPBUGS-38795 Signed-off-by: Francesco Romani <fromani@redhat.com> * deferred: tuned: fix reload trigger when inplace update There are conditions on which we should not set the reload flag. This avoid regression in the test "Profile deferred when applied should trigger changes when applied fist, then deferred when edited, if tuned restart should be kept deferred" Signed-off-by: Francesco Romani <fromani@redhat.com> * deferred: tuned: clarify comments and logs document better the logic about processing edits Signed-off-by: Francesco Romani <fromani@redhat.com> --------- Signed-off-by: Francesco Romani <fromani@redhat.com> * Omit unbackported logging support --------- Signed-off-by: Francesco Romani <fromani@redhat.com> Co-authored-by: Francesco Romani <fromani@redhat.com> Co-authored-by: Martin Sivák <msivak@redhat.com>
Process recommended profile change even when the profile itself has not changed itself. The internal profile fingerprint tracking was not updated during recommended profile update with no internal changes.