perfprof: deferred updates only for tuned objects

The original intent of the deferred updates is to address tuned-triggered reboots. By design, performanceprofiles fans out objects managed by different independent controllers. Guaranteeing deferred updates for the performanceprofile is thus a much harder problem than tuned. To meet these conflicting needs, we augment the lower-level tuned annotation to accept a comma-separated, lowercase, list of components whose updates should be deferred. So far we will only support tuned. xref: https://issues.redhat.com/browse/OCPBUGS-28647 Signed-off-by: Francesco Romani <[email protected]>
openshift · Aug 6, 2024 · d50fa39 · d50fa39
1 parent 31e133b
commit d50fa39
Show file tree

Hide file tree

Showing 5 changed files with 103 additions and 75 deletions.
diff --git a/pkg/apis/performanceprofile/v2/performanceprofile_types.go b/pkg/apis/performanceprofile/v2/performanceprofile_types.go
@@ -34,6 +34,18 @@ const PerformanceProfileEnablePhysicalRpsAnnotation = "performance.openshift.io/
 // that ignores the removal of all RPS settings when realtime workload hint is explicitly set to false.
 const PerformanceProfileEnableRpsAnnotation = "performance.openshift.io/enable-rps"
 
+// PerformanceProfileDeferredUpdateAnnotation is an advanced annotation which request the
+// object generated and govenred by PerformanceProfile to be updated in a deferred way,
+// minimizing the number of reboots. Please check the `DeferredUpdate*` constants to
+// learn which objects support deferred updates.
+const PerformanceProfileDeferredUpdateAnnotation = "performance.openshift.io/deferred"
+
+const (
+	// DeferredUpdateTuned requests the tuned object generated from performanceprofile
+	// to be applied in a deferred way, minimizing the node reboots.
+	DeferredUpdateTuned = "tuned"
+)
+
 // PerformanceProfileSpec defines the desired state of PerformanceProfile.
 type PerformanceProfileSpec struct {
 	// CPU defines a set of CPU related parameters.

diff --git a/pkg/performanceprofile/controller/performanceprofile/components/handler/handler.go b/pkg/performanceprofile/controller/performanceprofile/components/handler/handler.go
@@ -50,7 +50,7 @@ func (h *handler) Apply(ctx context.Context, obj client.Object, recorder record.
 	}
 	// set missing options
 	opts.MachineConfig.MixedCPUsEnabled = opts.MixedCPUsFeatureGateEnabled && profileutil.IsMixedCPUsEnabled(profile)
-	opts.Tuned.DeferredEnabled = ntoutil.HasDeferredUpdateAnnotation(profile.Annotations)
+	opts.Tuned.DeferredEnabled = ntoutil.HasAnnotation(profile.Annotations, performancev2.PerformanceProfileDeferredUpdateAnnotation, performancev2.DeferredUpdateTuned)
 
 	ctrRuntime, err := h.getContainerRuntimeName(ctx, profile, opts.ProfileMCP)
 	if err != nil {

diff --git a/pkg/performanceprofile/controller/performanceprofile_controller_test.go b/pkg/performanceprofile/controller/performanceprofile_controller_test.go
@@ -845,7 +845,7 @@ var _ = Describe("Controller", func() {
 				skipForHypershift() // deferred updates not supported in hypershift yet
 
 				prof := profile.DeepCopy()
-				prof.Annotations = ntoutil.ToggleDeferredUpdateAnnotation(prof.Annotations, true)
+				prof.Annotations = ntoutil.AddAnnotation(prof.Annotations, performancev2.PerformanceProfileDeferredUpdateAnnotation, performancev2.DeferredUpdateTuned)
 				r := newFakeReconciler(prof, profileMCP, infra, clusterOperator)
 
 				Expect(reconcileTimes(r, request, 2)).To(Equal(reconcile.Result{}))

diff --git a/pkg/util/annotations.go b/pkg/util/annotations.go
@@ -1,30 +1,63 @@
 package util
 
 import (
+	"strings"
+
+	"k8s.io/apimachinery/pkg/util/sets"
+
 	tunedv1 "github.com/openshift/cluster-node-tuning-operator/pkg/apis/tuned/v1"
 )
 
-func HasDeferredUpdateAnnotation(anns map[string]string) bool {
+const (
+	AnnotationValueAny = "*" // "" is treated as the same
+)
+
+func HasAnnotation(anns map[string]string, key string, values ...string) bool {
 	if anns == nil {
 		return false
 	}
-	_, ok := anns[tunedv1.TunedDeferredUpdate]
-	return ok
+	annVals, ok := anns[key]
+	if !ok {
+		return false
+	}
+	if len(annVals) == 0 || len(values) == 0 {
+		// annotation value == "", treated as wildcard like AnnotationValueAny. But faster.
+		// if nothing to check, we only need to check for the ann presence, and if
+		// we got this far, we have it.
+		return true
+	}
+	annSet := sets.New[string](parseValuesAnnotation(annVals)...)
+	return annSet.Has(AnnotationValueAny) || annSet.HasAll(values...)
 }
 
-func SetDeferredUpdateAnnotation(anns map[string]string, tuned *tunedv1.Tuned) map[string]string {
-	if anns == nil {
-		anns = make(map[string]string)
-	}
-	return ToggleDeferredUpdateAnnotation(anns, HasDeferredUpdateAnnotation(tuned.Annotations))
+func AddAnnotation(anns map[string]string, key, val string) map[string]string {
+	ret := cloneMapStringString(anns)
+	ret[key] = val
+	return ret
 }
 
-func ToggleDeferredUpdateAnnotation(anns map[string]string, toggle bool) map[string]string {
+func DelAnnotation(anns map[string]string, key string) map[string]string {
 	ret := cloneMapStringString(anns)
+	delete(ret, key)
+	return ret
+}
+
+func ToggleDeferredUpdateAnnotation(anns map[string]string, toggle bool) map[string]string {
 	if toggle {
-		ret[tunedv1.TunedDeferredUpdate] = ""
-	} else {
-		delete(ret, tunedv1.TunedDeferredUpdate)
+		return AddAnnotation(anns, tunedv1.TunedDeferredUpdate, "")
+	}
+	return DelAnnotation(anns, tunedv1.TunedDeferredUpdate)
+}
+
+func HasDeferredUpdateAnnotation(anns map[string]string, values ...string) bool {
+	return HasAnnotation(anns, tunedv1.TunedDeferredUpdate, values...)
+}
+
+func parseValuesAnnotation(s string) []string {
+	items := strings.Split(s, ",")
+	ret := make([]string, 0, len(items))
+	for _, item := range items {
+		ret = append(ret, strings.TrimSpace(item))
 	}
 	return ret
 }

diff --git a/pkg/util/annotations_test.go b/pkg/util/annotations_test.go
@@ -3,15 +3,13 @@ package util
 import (
 	"reflect"
 	"testing"
-
-	tunedv1 "github.com/openshift/cluster-node-tuning-operator/pkg/apis/tuned/v1"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
 func TestHasDeferredUpdateAnnotation(t *testing.T) {
 	testCases := []struct {
 		name     string
 		anns     map[string]string
+		vals     []string
 		expected bool
 	}{
 		{
@@ -45,90 +43,75 @@ func TestHasDeferredUpdateAnnotation(t *testing.T) {
 			},
 			expected: true,
 		},
-	}
-	for _, tt := range testCases {
-		t.Run(tt.name, func(t *testing.T) {
-			got := HasDeferredUpdateAnnotation(tt.anns)
-			if got != tt.expected {
-				t.Errorf("got=%v expected=%v", got, tt.expected)
-			}
-		})
-	}
-}
-
-func TestSetDeferredUpdateAnnotation(t *testing.T) {
-	testCases := []struct {
-		name     string
-		anns     map[string]string
-		tuned    *tunedv1.Tuned
-		expected map[string]string
-	}{
 		{
-			name:     "nil",
-			tuned:    &tunedv1.Tuned{},
-			expected: map[string]string{},
+			name: "found single",
+			anns: map[string]string{
+				"tuned.openshift.io/deferred": "foo",
+			},
+			vals:     []string{"foo"},
+			expected: true,
 		},
 		{
-			name: "nil-add",
-			tuned: &tunedv1.Tuned{
-				ObjectMeta: metav1.ObjectMeta{
-					Annotations: map[string]string{
-						"tuned.openshift.io/deferred": "",
-					},
-				},
-			},
-			expected: map[string]string{
-				"tuned.openshift.io/deferred": "",
+			name: "found partial",
+			anns: map[string]string{
+				"tuned.openshift.io/deferred": "foo,bar",
 			},
+			vals:     []string{"foo"},
+			expected: true,
 		},
 		{
-			name: "existing-add",
+			name: "found partial",
 			anns: map[string]string{
-				"foobar": "42",
-			},
-			tuned: &tunedv1.Tuned{
-				ObjectMeta: metav1.ObjectMeta{
-					Annotations: map[string]string{
-						"tuned.openshift.io/deferred": "",
-					},
-				},
+				"tuned.openshift.io/deferred": "bar",
 			},
-			expected: map[string]string{
-				"foobar":                      "42",
+			vals:     []string{"foo", "bar"},
+			expected: false,
+		},
+		{
+			name: "missing single",
+			anns: map[string]string{
 				"tuned.openshift.io/deferred": "",
 			},
+			vals:     []string{"foo"},
+			expected: true, // no ann value means accept everything
 		},
 		{
-			name:     "nil-remove",
-			tuned:    &tunedv1.Tuned{},
-			expected: map[string]string{},
+			name: "missing partial",
+			anns: map[string]string{
+				"tuned.openshift.io/deferred": "bar,foo",
+			},
+			vals:     []string{"foo", "bar", "baz"},
+			expected: false,
 		},
 		{
-			name: "existing-remove",
+			name: "wildcard - implicit",
 			anns: map[string]string{
-				"foobar":                      "42",
 				"tuned.openshift.io/deferred": "",
 			},
-			tuned: &tunedv1.Tuned{},
-			expected: map[string]string{
-				"foobar": "42",
-			},
+			vals:     []string{"foo", "bar", "baz"},
+			expected: true,
 		},
 		{
-			name: "missing-remove",
+			name: "wildcard - explicit",
 			anns: map[string]string{
-				"foobar": "42",
+				"tuned.openshift.io/deferred": "*",
 			},
-			tuned: &tunedv1.Tuned{},
-			expected: map[string]string{
-				"foobar": "42",
+			vals:     []string{"foo", "bar", "baz"},
+			expected: true,
+		},
+		{
+			name: "wildcard - mixed",
+			anns: map[string]string{
+				"tuned.openshift.io/deferred": "*,foo",
 			},
+			vals:     []string{"foo", "bar", "baz"},
+			expected: true,
 		},
 	}
 	for _, tt := range testCases {
 		t.Run(tt.name, func(t *testing.T) {
-			got := SetDeferredUpdateAnnotation(tt.anns, tt.tuned)
-			if !reflect.DeepEqual(got, tt.expected) {
+			got := HasDeferredUpdateAnnotation(tt.anns, tt.vals...)
+			if got != tt.expected {
 				t.Errorf("got=%v expected=%v", got, tt.expected)
 			}
 		})