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.

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.

Signed-off-by: Francesco Romani <fromani@redhat.com>

pkg/operator/controller.go

@@ -645,7 +645,7 @@ func (c *Controller) syncProfile(tuned *tunedv1.Tuned, nodeName string) error {
 			}
 
 			klog.V(2).Infof("syncProfile(): Profile %s not found, creating one [%s]", profileMf.Name, computed.TunedProfileName)
-			profileMf.Annotations = util.ToggleDeferredUpdateAnnotation(profileMf.Annotations, computed.Deferred)
+			profileMf.Annotations = toggleDeferred(profileMf.Annotations, computed.Deferred)
 			profileMf.Spec.Config.TunedProfile = computed.TunedProfileName
 			profileMf.Spec.Config.Debug = computed.Operand.Debug
 			profileMf.Spec.Config.TuneDConfig = computed.Operand.TuneDConfig
@@ -706,14 +706,14 @@ func (c *Controller) syncProfile(tuned *tunedv1.Tuned, nodeName string) error {
 		}
 	}
 
-	anns := util.ToggleDeferredUpdateAnnotation(profile.Annotations, computed.Deferred)
+	anns := toggleDeferred(profile.Annotations, computed.Deferred)
 
 	// Minimize updates
 	if profile.Spec.Config.TunedProfile == computed.TunedProfileName &&
 		profile.Spec.Config.Debug == computed.Operand.Debug &&
 		reflect.DeepEqual(profile.Spec.Config.TuneDConfig, computed.Operand.TuneDConfig) &&
 		reflect.DeepEqual(profile.Spec.Profile, computed.AllProfiles) &&
-		util.HasDeferredUpdateAnnotation(profile.Annotations) == util.HasDeferredUpdateAnnotation(anns) &&
+		util.GetDeferredUpdateAnnotation(profile.Annotations) == util.GetDeferredUpdateAnnotation(anns) &&
 		profile.Spec.Config.ProviderName == providerName {
 		klog.V(2).Infof("syncProfile(): no need to update Profile %s", nodeName)
 		return nil
@@ -732,11 +732,18 @@ func (c *Controller) syncProfile(tuned *tunedv1.Tuned, nodeName string) error {
 	if err != nil {
 		return fmt.Errorf("failed to update Profile %s: %v", profile.Name, err)
 	}
-	klog.Infof("updated profile %s [%s] (deferred=%v)", profile.Name, computed.TunedProfileName, util.HasDeferredUpdateAnnotation(profile.Annotations))
+	klog.Infof("updated profile %s [%s] (deferred=%v)", profile.Name, computed.TunedProfileName, util.GetDeferredUpdateAnnotation(profile.Annotations))
 
 	return nil
 }
 
+func toggleDeferred(anns map[string]string, mode util.DeferMode) map[string]string {
+	if util.IsDeferredUpdate(mode) {
+		return util.SetDeferredUpdateAnnotation(anns, mode)
+	}
+	return util.DeleteDeferredUpdateAnnotation(anns)
+}
+
 func (c *Controller) getProviderName(nodeName string) (string, error) {
 	node, err := c.listers.Nodes.Get(nodeName)
 	if err != nil {

pkg/operator/profilecalculator.go

@@ -153,15 +153,15 @@ func (pc *ProfileCalculator) nodeChangeHandler(nodeName string) (bool, error) {
 type ComputedProfile struct {
 	TunedProfileName string
 	AllProfiles      []tunedv1.TunedProfile
-	Deferred         bool
+	Deferred         util.DeferMode
 	MCLabels         map[string]string
 	NodePoolName     string
 	Operand          tunedv1.OperandConfig
 }
 
 type RecommendedProfile struct {
 	TunedProfileName string
-	Deferred         bool
+	Deferred         util.DeferMode
 	Labels           map[string]string
 	Config           tunedv1.OperandConfig
 }
@@ -302,7 +302,7 @@ func (pc *ProfileCalculator) calculateProfile(nodeName string) (ComputedProfile,
 
 type HypershiftRecommendedProfile struct {
 	TunedProfileName string
-	Deferred         bool
+	Deferred         util.DeferMode
 	NodePoolName     string
 	Config           tunedv1.OperandConfig
 }
@@ -732,7 +732,7 @@ func tunedProfiles(tunedSlice []*tunedv1.Tuned) []tunedv1.TunedProfile {
 
 type TunedRecommendInfo struct {
 	tunedv1.TunedRecommend
-	Deferred bool
+	Deferred util.DeferMode
 }
 
 // TunedRecommend returns a priority-sorted TunedRecommend slice out of
@@ -752,7 +752,7 @@ func TunedRecommend(tunedSlice []*tunedv1.Tuned) []TunedRecommendInfo {
 		for _, recommend := range tuned.Spec.Recommend {
 			recommendAll = append(recommendAll, TunedRecommendInfo{
 				TunedRecommend: recommend,
-				Deferred:       util.HasDeferredUpdateAnnotation(tuned.Annotations),
+				Deferred:       util.GetDeferredUpdateAnnotation(tuned.Annotations),
 			})
 		}
 	}

pkg/tuned/controller.go

@@ -149,7 +149,7 @@ type Change struct {
 	recommendedProfile string
 
 	// Is the current Change triggered by an object with the deferred annotation?
-	deferred bool
+	deferredMode util.DeferMode
 	// Text to convey in status message, if present.
 	message string
 }
@@ -180,8 +180,8 @@ func (ch Change) String() string {
 	if ch.recommendedProfile != "" {
 		items = append(items, fmt.Sprintf("recommendedProfile:%q", ch.recommendedProfile))
 	}
-	if ch.deferred {
-		items = append(items, "deferred:true")
+	if ch.deferredMode != "" {
+		items = append(items, fmt.Sprintf("deferredMode:%q", string(ch.deferredMode)))
 	}
 	if ch.message != "" {
 		items = append(items, fmt.Sprintf("message:%q", ch.message))
@@ -354,7 +354,7 @@ func (c *Controller) sync(key wqKeyKube) error {
 		if profile.Spec.Config.TuneDConfig.ReapplySysctl != nil {
 			change.reapplySysctl = *profile.Spec.Config.TuneDConfig.ReapplySysctl
 		}
-		change.deferred = util.HasDeferredUpdateAnnotation(profile.Annotations)
+		change.deferredMode = util.GetDeferredUpdateAnnotation(profile.Annotations)
 		// Notify the event processor that the Profile k8s object containing information about which TuneD profile to apply changed.
 		c.wqTuneD.Add(wqKeyTuned{kind: wqKindDaemon, change: change})
 
@@ -987,6 +987,7 @@ func (c *Controller) changeSyncerProfileStatus(change Change) (synced bool) {
 func (c *Controller) changeSyncerTuneD(change Change) (synced bool, err error) {
 	var restart bool
 	var reload bool
+	var inplaceUpdate bool // updating a profile already recommended
 	var cfgUpdated bool
 	var changeRecommend bool
 
@@ -1010,15 +1011,16 @@ func (c *Controller) changeSyncerTuneD(change Change) (synced bool, err error) {
 		}
 		reload = reload || changeProvider
 
-		if (c.daemon.recommendedProfile != change.recommendedProfile) || change.nodeRestart {
+		inplaceUpdate = (c.daemon.recommendedProfile == change.recommendedProfile)
+		if !inplaceUpdate || change.nodeRestart {
 			if err = TunedRecommendFileWrite(change.recommendedProfile); err != nil {
 				return false, err
 			}
-			klog.V(1).Infof("recommended TuneD profile changed from %q to %q [deferred=%v nodeRestart=%v]", c.daemon.recommendedProfile, change.recommendedProfile, change.deferred, change.nodeRestart)
+			klog.V(1).Infof("recommended TuneD profile changed from %q to %q [deferred=%v nodeRestart=%v]", c.daemon.recommendedProfile, change.recommendedProfile, change.deferredMode, change.nodeRestart)
 			// Cache the value written to tunedRecommendFile.
 			c.daemon.recommendedProfile = change.recommendedProfile
 			reload = true
-		} else if !change.deferred && (c.daemon.status&scDeferred != 0) {
+		} else if util.IsImmediateUpdate(change.deferredMode) && (c.daemon.status&scDeferred != 0) {
 			klog.V(1).Infof("detected deferred update changed to immediate after object update")
 			reload = true
 		} else {
@@ -1077,7 +1079,7 @@ func (c *Controller) changeSyncerTuneD(change Change) (synced bool, err error) {
 	}
 
 	// failures pertaining to deferred updates are not critical
-	_ = c.handleDaemonReloadRestartRequest(change, reload, restart)
+	_ = c.handleDaemonReloadRestartRequest(change, reload, restart, inplaceUpdate)
 
 	cfgUpdated, err = c.changeSyncerRestartOrReloadTuneD()
 	klog.V(2).Infof("changeSyncerTuneD() configuration updated: %v", cfgUpdated)
@@ -1088,19 +1090,32 @@ func (c *Controller) changeSyncerTuneD(change Change) (synced bool, err error) {
 	return err == nil, err
 }
 
-func (c *Controller) handleDaemonReloadRestartRequest(change Change, reload, restart bool) error {
+func treatAsImmediate(change Change, inplaceUpdate bool) (bool, string) {
+	if change.nodeRestart {
+		return true, "node restart"
+	}
+	if util.IsImmediateUpdate(change.deferredMode) {
+		return true, "immediate update"
+	}
+	if !inplaceUpdate && change.deferredMode == util.DeferUpdate {
+		return true, "recommend change with deferredMode=update"
+	}
+	return false, ""
+}
+
+func (c *Controller) handleDaemonReloadRestartRequest(change Change, reload, restart, inplaceUpdate bool) error {
 	if !reload && !restart {
 		// nothing to do
 		return nil
 	}
 
-	if !change.deferred || change.nodeRestart {
+	if ok, reason := treatAsImmediate(change, inplaceUpdate); ok {
 		if reload {
-			klog.V(2).Infof("immediate update, setting reload flag")
+			klog.V(2).Infof("%s: setting reload flag", reason)
 			c.daemon.restart |= ctrlReload
 		}
 		if restart {
-			klog.V(2).Infof("immediate update, setting restart flag")
+			klog.V(2).Infof("%s: setting restart flag", reason)
 			c.daemon.restart |= ctrlRestart
 		}
 		return nil
@@ -1321,7 +1336,7 @@ func (c *Controller) updateTunedProfileStatus(ctx context.Context, change Change
 	}
 
 	var message string
-	wantsDeferred := util.HasDeferredUpdateAnnotation(profile.Annotations)
+	wantsDeferred := util.IsDeferredUpdate(util.GetDeferredUpdateAnnotation(profile.Annotations))
 	isApplied := (c.daemon.profileFingerprintUnpacked == c.daemon.profileFingerprintEffective)
 	daemonStatus := c.daemon.status
 

pkg/tuned/controller_test.go

@@ -7,6 +7,7 @@ import (
 	"testing"
 
 	tunedv1 "github.com/openshift/cluster-node-tuning-operator/pkg/apis/tuned/v1"
+	"github.com/openshift/cluster-node-tuning-operator/pkg/util"
 )
 
 func TestRecommendFileRoundTrip(t *testing.T) {
@@ -295,7 +296,7 @@ func fullChange() Change {
 		provider:           "test-provider",
 		reapplySysctl:      true,
 		recommendedProfile: "test-profile",
-		deferred:           true,
+		deferredMode:       util.DeferAlways,
 		message:            "test-message",
 	}
 }

pkg/util/annotations.go

@@ -4,28 +4,53 @@ import (
 	tunedv1 "github.com/openshift/cluster-node-tuning-operator/pkg/apis/tuned/v1"
 )
 
-func HasDeferredUpdateAnnotation(anns map[string]string) bool {
-	if anns == nil {
-		return false
-	}
-	_, ok := anns[tunedv1.TunedDeferredUpdate]
-	return ok
+type DeferMode string
+
+const (
+	DeferNever  DeferMode = "never" // aka defer mode disabled aka immediate update
+	DeferAlways DeferMode = "always"
+	DeferUpdate DeferMode = "update"
+)
+
+func (dm DeferMode) String() string {
+	return string(dm)
+}
+
+func IsImmediateUpdate(value DeferMode) bool {
+	return value == DeferNever
 }
 
-func SetDeferredUpdateAnnotation(anns map[string]string, tuned *tunedv1.Tuned) map[string]string {
+func IsDeferredUpdate(value DeferMode) bool {
+	return value == DeferAlways || value == DeferUpdate
+}
+
+func GetDeferredUpdateAnnotation(anns map[string]string) DeferMode {
 	if anns == nil {
-		anns = make(map[string]string)
+		return DeferNever
+	}
+	val, ok := anns[tunedv1.TunedDeferredUpdate]
+	if !ok {
+		return DeferNever
 	}
-	return ToggleDeferredUpdateAnnotation(anns, HasDeferredUpdateAnnotation(tuned.Annotations))
+	value := DeferMode(val)
+	if !IsDeferredUpdate(value) {
+		return DeferNever
+	}
+	return value
 }
 
-func ToggleDeferredUpdateAnnotation(anns map[string]string, toggle bool) map[string]string {
+func SetDeferredUpdateAnnotation(anns map[string]string, value DeferMode) map[string]string {
 	ret := cloneMapStringString(anns)
-	if toggle {
-		ret[tunedv1.TunedDeferredUpdate] = ""
-	} else {
-		delete(ret, tunedv1.TunedDeferredUpdate)
+	if value == DeferNever {
+		return ret
 	}
+	ret[tunedv1.TunedDeferredUpdate] = string(value)
+	return ret
+}
+
+func DeleteDeferredUpdateAnnotation(anns map[string]string) map[string]string {
+	ret := cloneMapStringString(anns)
+	delete(ret, tunedv1.TunedDeferredUpdate)
 	return ret
 }