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 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.

Signed-off-by: Francesco Romani <[email protected]>

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 = updateDeferredAnnotation(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 := updateDeferredAnnotation(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 updateDeferredAnnotation(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=" + util.DeferUpdate.String()
+	}
+	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,56 @@ 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
+// DeferMode controls the deferred feature. Default is "never", which is the value
+// assumed when no annotation is set, and which cause all objects to be processed
+// immediately (never deferred)
+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
 }