OCPBUGS-28647: tuned: distinguish deferred updates #1129

ffromani · 2024-08-08T11:51:55Z

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 different TuneD profile. This usually, but not exclusively, involve changes of the sysctls.

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.

openshift-ci-robot · 2024-08-08T11:52:00Z

@ffromani: This pull request references Jira Issue OCPBUGS-28647, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.17.0) matches configured target version for branch (4.17.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @shajmakh

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

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.

The key distinction is if the recommended profile changes or not, and there's a desire to defer application of changes only if a profile is update, 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 trigger the recommended profile, and updates the setting, usually but not exclusively the sysctls.

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.

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.

pkg/util/annotations.go

pkg/tuned/controller.go

yanirq · 2024-08-08T12:39:30Z

/cc @MarSik @bartwensley

ffromani · 2024-08-08T12:52:37Z

/cc @jmencak

ffromani · 2024-08-08T14:49:59Z

/retest

jmencak · 2024-08-08T19:30:34Z

Thank you for the PR! I'll try to understand the code and the motivation tomorrow. Some initial thoughts.

The key distinction is if the recommended profile changes or not, and there's a desire to defer application of changes only if a profile is update, not the first time it is applied.

Do you mean "only if profile is updated"? Also, which profile? TuneD profile or the k8s CR? I assume the former, so we should be specific to make things clear.

* (in-place) profile update is a change which does NOT trigger the recommended profile, and updates the setting, usually but not exclusively the sysctls.

IIUC, s/NOT trigger the recommended profile/NOT cause a switch to a different TuneD 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

~~Is this correct? Shouldn't we say something along the lines "if at least 1 Tuned object annotated this way exists, profile applications will be deferred"? Similarly for "update".~~

Edit: I take this back, I was thinking about the old implementation.

Also, the code uses "never" DeferMode, should we document this in the PR description and the commit?

ffromani · 2024-08-09T06:14:45Z

thanks @jmencak , I will clarify the commit message indeed.

jmencak

First pass done, just a couple of nits. Changes make sense to me.

pkg/operator/controller.go

pkg/tuned/controller.go

jmencak · 2024-08-09T08:30:39Z

After some manual testing I see one potential issue with

tuned.openshift.io/deferred: "update"

Create the following manifest

apiVersion: tuned.openshift.io/v1
kind: Tuned
metadata:
  name: openshift-profile
  namespace: openshift-cluster-node-tuning-operator
  annotations:
    tuned.openshift.io/deferred: "update"
spec:
  profile:
  - data: |
      [main]
      summary=Custom OpenShift profile
      include=openshift-node
      [sysctl]
      kernel.shmmni=8192
    name: openshift-profile
  recommend:
  - match:
    - label: profile
    priority: 20
    profile: openshift-profile

Profile is applied. Now change the value of kernel.shmmni to something like kernel.shmmni=16384. Profile application is refused on the grounds of the deferred update.
Delete the TuneD pod responsible setting the profile => profile is applied by the new pod.

Questions:

Do we feel this is an issue? Should we make the information about the deferred state permanent so that TuneD pod restarts do not apply the changes?
Should we get this pre-merge tested?

ffromani · 2024-08-09T08:44:32Z

After some manual testing I see one potential issue with
[...]
2. Profile is applied. Now change the value of kernel.shmmni to something like kernel.shmmni=16384. Profile application is refused on the grounds of the deferred update.
3. Delete the TuneD pod responsible setting the profile => profile is applied by the new pod.
Questions:
* Do we feel this is an issue?  Should we make the information about the deferred state permanent so that `TuneD` pod restarts do not apply the changes?

yes, it's a bug.

* Should we get this pre-merge tested?

I guess yes

ffromani · 2024-08-09T12:17:42Z

/retest

jmencak · 2024-08-09T14:49:05Z

Infra issues
/retest

jmencak · 2024-08-09T15:05:33Z

Thank you for the fix, Francesco. I can confirm the issue is no longer present.
/lgtm
but it would be nice to do some basic pre-merge testing to verify the functionality @liqcui . I'm sure Liquan will have questions on how to test this so this could also help to improve the docs/commit message.
/hold
to give other reviewers a chance to review this.

jmencak

Thank you for the changes. I went through the code (mostly thinking about the semantics) one last time. It is tricky to assess all the corner cases, but the changes look good to me. I've also done some basic testing.

The only gray zone I found so far was when switching to a different profile using
"always" annotation and then updating it with a profile with "update"
deferred annotation. We do not define the behaviour in this case, so I
guess the current behaviour (to wait for a reboot) is fine.

I tried to make the commit message a bit more consistent and clearer.
Here is what I propose:

--- old	2024-08-13 13:38:14.916800476 +0200
+++ new	2024-08-13 13:55:13.104238181 +0200
@@ -1,6 +1,6 @@
 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
+profile change and in-place profile update. This is required to
 better support a GitOps flow.
 
 The key distinction is if the recommended profile changes or not,
@@ -11,17 +11,17 @@
 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 different TuneD profile. This usually, but not
-  exclusively, involve changes of the sysctls.
+- 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
+  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.
+  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

It would be nice if others, especially @MarSik / @yanirq could chime in from a higher level whether the changes in this PR are sufficient for what is needed from deferred updates.

pkg/tuned/controller.go

pkg/util/annotations.go

ffromani · 2024-08-13T13:38:19Z

/hold
for @MarSik and @yanirq review. (xref: #1129 (review))

jmencak · 2024-08-13T14:23:22Z

Thank you for the changes.
/lgtm
Found one typo change afeter. but that can be fixed later on if you prefer.

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 <[email protected]>

now that we have the more powerful ginkgo labels, we can stop using tags for newer tests. Signed-off-by: Francesco Romani <[email protected]>

ffromani · 2024-08-13T14:27:00Z

Thank you for the changes. /lgtm Found one typo change afeter. but that can be fixed later on if you prefer.

thanks, fixed

jmencak · 2024-08-13T14:31:02Z

/lgtm

openshift-ci · 2024-08-13T17:26:10Z

@ffromani: 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.

MarSik

/lgtm

I like it.

openshift-ci · 2024-08-14T08:09:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ffromani, 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:

~~OWNERS~~ [MarSik,ffromani]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jmencak · 2024-08-14T08:34:14Z

/unhold

openshift-ci-robot · 2024-08-14T08:36:50Z

@ffromani: Jira Issue OCPBUGS-28647: All pull requests linked via external trackers have merged:

Jira Issue OCPBUGS-28647 has been moved to the MODIFIED state.

In response to this:

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 different TuneD profile. This usually, but not exclusively, involve changes of the sysctls.

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.

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-bot · 2024-08-14T16:28:55Z

[ART PR BUILD NOTIFIER]

Distgit: cluster-node-tuning-operator
This PR has been included in build cluster-node-tuning-operator-container-v4.18.0-202408140944.p0.g3655f22.assembly.stream.el9.
All builds following this will include this PR.

yanirq · 2024-08-20T13:41:15Z

/cherry-pick release-4.17

openshift-cherrypick-robot · 2024-08-20T13:42:09Z

@yanirq: new pull request created: #1138

In response to this:

/cherry-pick release-4.17

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.

* 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 <[email protected]> * 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 <[email protected]> --------- Signed-off-by: Francesco Romani <[email protected]>

) * 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 <[email protected]> * 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 <[email protected]> --------- Signed-off-by: Francesco Romani <[email protected]> * 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 <[email protected]> * e2e: deferred: stricter testing add stricter check for the node condition when validating OCPBUGS-38795 Signed-off-by: Francesco Romani <[email protected]> * 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 <[email protected]> * deferred: tuned: clarify comments and logs document better the logic about processing edits Signed-off-by: Francesco Romani <[email protected]> --------- Signed-off-by: Francesco Romani <[email protected]> * Omit unbackported logging support --------- Signed-off-by: Francesco Romani <[email protected]> Co-authored-by: Francesco Romani <[email protected]> Co-authored-by: Martin Sivák <[email protected]>

openshift-ci-robot added jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. labels Aug 8, 2024

openshift-ci bot requested review from shajmakh, Tal-or and yanirq August 8, 2024 11:52

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 8, 2024

yanirq reviewed Aug 8, 2024

View reviewed changes

pkg/util/annotations.go Outdated Show resolved Hide resolved

yanirq reviewed Aug 8, 2024

View reviewed changes

pkg/tuned/controller.go Show resolved Hide resolved

openshift-ci bot requested review from bartwensley and MarSik August 8, 2024 12:39

ffromani force-pushed the tuned-deferred-updates-on-update branch from 6d75a91 to 5d4af08 Compare August 8, 2024 12:42

openshift-ci bot requested a review from jmencak August 8, 2024 12:52

ffromani force-pushed the tuned-deferred-updates-on-update branch from 5d4af08 to aef0cf0 Compare August 9, 2024 06:20

jmencak reviewed Aug 9, 2024

View reviewed changes

pkg/operator/controller.go Show resolved Hide resolved

pkg/operator/controller.go Outdated Show resolved Hide resolved

pkg/tuned/controller.go Outdated Show resolved Hide resolved

ffromani force-pushed the tuned-deferred-updates-on-update branch from aef0cf0 to 0f41473 Compare August 9, 2024 07:17

ffromani force-pushed the tuned-deferred-updates-on-update branch from b24c4f2 to e9952ee Compare August 9, 2024 13:17

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 9, 2024

openshift-ci bot assigned jmencak Aug 9, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 9, 2024

ffromani force-pushed the tuned-deferred-updates-on-update branch from a46d8ea to ddacbff Compare August 13, 2024 08:10

jmencak reviewed Aug 13, 2024

View reviewed changes

ffromani force-pushed the tuned-deferred-updates-on-update branch from ddacbff to 3baf4ec Compare August 13, 2024 13:36

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 13, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 13, 2024

ffromani added 2 commits August 13, 2024 16:26

e2e: drop tags, use labels

c94303e

now that we have the more powerful ginkgo labels, we can stop using tags for newer tests. Signed-off-by: Francesco Romani <[email protected]>

ffromani force-pushed the tuned-deferred-updates-on-update branch from 3baf4ec to c94303e Compare August 13, 2024 14:26

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 13, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 13, 2024

MarSik approved these changes Aug 14, 2024

View reviewed changes

openshift-ci bot assigned MarSik Aug 14, 2024

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 14, 2024

openshift-merge-bot bot merged commit 3655f22 into openshift:master Aug 14, 2024
16 checks passed

ffromani deleted the tuned-deferred-updates-on-update branch August 14, 2024 08:37

openshift-cherrypick-robot mentioned this pull request Aug 20, 2024

[release-4.17] OCPBUGS-38721: tuned: distinguish deferred updates #1138

Closed

yanirq mentioned this pull request Sep 1, 2024

[release-4.17] OCPBUGS-38721: tuned: distinguish deferred updates #1149

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCPBUGS-28647: tuned: distinguish deferred updates #1129

OCPBUGS-28647: tuned: distinguish deferred updates #1129

ffromani commented Aug 8, 2024 •

edited

Loading

openshift-ci-robot commented Aug 8, 2024

yanirq commented Aug 8, 2024

ffromani commented Aug 8, 2024

ffromani commented Aug 8, 2024

jmencak commented Aug 8, 2024 •

edited

Loading

ffromani commented Aug 9, 2024

jmencak left a comment

jmencak commented Aug 9, 2024

ffromani commented Aug 9, 2024

ffromani commented Aug 9, 2024

jmencak commented Aug 9, 2024

jmencak commented Aug 9, 2024

jmencak left a comment

ffromani commented Aug 13, 2024

jmencak commented Aug 13, 2024

ffromani commented Aug 13, 2024

jmencak commented Aug 13, 2024

openshift-ci bot commented Aug 13, 2024

MarSik left a comment

openshift-ci bot commented Aug 14, 2024

jmencak commented Aug 14, 2024

openshift-ci-robot commented Aug 14, 2024

openshift-bot commented Aug 14, 2024

yanirq commented Aug 20, 2024

openshift-cherrypick-robot commented Aug 20, 2024

OCPBUGS-28647: tuned: distinguish deferred updates #1129

OCPBUGS-28647: tuned: distinguish deferred updates #1129

Conversation

ffromani commented Aug 8, 2024 • edited Loading

openshift-ci-robot commented Aug 8, 2024

yanirq commented Aug 8, 2024

ffromani commented Aug 8, 2024

ffromani commented Aug 8, 2024

jmencak commented Aug 8, 2024 • edited Loading

ffromani commented Aug 9, 2024

jmencak left a comment

Choose a reason for hiding this comment

jmencak commented Aug 9, 2024

ffromani commented Aug 9, 2024

ffromani commented Aug 9, 2024

jmencak commented Aug 9, 2024

jmencak commented Aug 9, 2024

jmencak left a comment

Choose a reason for hiding this comment

ffromani commented Aug 13, 2024

jmencak commented Aug 13, 2024

ffromani commented Aug 13, 2024

jmencak commented Aug 13, 2024

openshift-ci bot commented Aug 13, 2024

MarSik left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Aug 14, 2024

jmencak commented Aug 14, 2024

openshift-ci-robot commented Aug 14, 2024

openshift-bot commented Aug 14, 2024

yanirq commented Aug 20, 2024

openshift-cherrypick-robot commented Aug 20, 2024

ffromani commented Aug 8, 2024 •

edited

Loading

jmencak commented Aug 8, 2024 •

edited

Loading