OCPBUGS-32469: Remove tuned/rendered object (#1036)

* Remove tuned/rendered object

The NTO operand is controlled by the operator by updates to two
resources.  Its corresponding k8s Tuned Profile resource and
tuned/rendered object, which contains a list of all TuneD (daemon)
profiles.

While this setup works for most cases, there is a problem with this
approach when a cluster administator changes both a current TuneD
profile content and (at the same) time switches to a new TuneD profile
completely.  Then, depending on the k8s object update timing, we could
see two TuneD daemon reloads instead of just one.

Remove the tuned/rendered object and carry TuneD (daemon) profiles
directly in the Tuned Profile k8s objects.

* Fix e2e-gcp-pao-workloadhints, minor code improvements

* Another "happy path" in TunedProfiles()

* Added a unit test for TunedProfiles()

* No errors when TunedRenderedResourceName was already removed

---------

Co-authored-by: Jiri Mencak <[email protected]>

manifests/20-profile.crd.yaml

@@ -74,6 +74,25 @@ spec:
                 required:
                 - tunedProfile
                 type: object
+              profile:
+                description: Tuned profiles.
+                items:
+                  description: A Tuned profile.
+                  properties:
+                    data:
+                      description: Specification of the Tuned profile to be consumed
+                        by the Tuned daemon.
+                      type: string
+                    name:
+                      description: Name of the Tuned profile to be used in the recommend
+                        section.
+                      minLength: 1
+                      type: string
+                  required:
+                  - data
+                  - name
+                  type: object
+                type: array
             required:
             - config
             type: object

pkg/apis/tuned/v1/tuned_types.go

@@ -153,6 +153,9 @@ type Profile struct {
 
 type ProfileSpec struct {
 	Config ProfileConfig `json:"config"`
+	// Tuned profiles.
+	// +optional
+	Profile []TunedProfile `json:"profile"`
 }
 
 type ProfileConfig struct {

pkg/manifests/manifests.go

@@ -4,15 +4,12 @@ import (
 	"bytes"
 	"io"
 	"os"
-	"sort"
 
 	appsv1 "k8s.io/api/apps/v1"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/yaml"
 
 	tunedv1 "github.com/openshift/cluster-node-tuning-operator/pkg/apis/tuned/v1"
 	ntoconfig "github.com/openshift/cluster-node-tuning-operator/pkg/config"
-	"k8s.io/klog/v2"
 )
 
 const (
@@ -26,47 +23,6 @@ func MustAssetReader(asset string) io.Reader {
 	return bytes.NewReader(MustAsset(asset))
 }
 
-func TunedRenderedResource(tunedSlice []*tunedv1.Tuned) *tunedv1.Tuned {
-	cr := &tunedv1.Tuned{
-		ObjectMeta: metav1.ObjectMeta{
-			Name:      tunedv1.TunedRenderedResourceName,
-			Namespace: ntoconfig.WatchNamespace(),
-		},
-		Spec: tunedv1.TunedSpec{
-			Recommend: []tunedv1.TunedRecommend{},
-		},
-	}
-
-	tunedProfiles := []tunedv1.TunedProfile{}
-	m := map[string]tunedv1.TunedProfile{}
-
-	for _, tuned := range tunedSlice {
-		if tuned.Name == tunedv1.TunedRenderedResourceName {
-			// Skip the "rendered" Tuned resource itself
-			continue
-		}
-		tunedRenderedProfiles(tuned, m)
-	}
-	for _, tunedProfile := range m {
-		if tunedProfile.Name == nil {
-			// This should never happen (openAPIV3Schema validation); ignore invalid profiles
-			continue
-		}
-		tunedProfiles = append(tunedProfiles, tunedProfile)
-	}
-
-	// The order of Tuned resources is variable and so is the order of profiles
-	// within the resource itself.  Sort the rendered profiles by their names for
-	// simpler change detection.
-	sort.Slice(tunedProfiles, func(i, j int) bool {
-		return *tunedProfiles[i].Name < *tunedProfiles[j].Name
-	})
-
-	cr.Spec.Profile = tunedProfiles
-
-	return cr
-}
-
 func TunedDaemonSet() *appsv1.DaemonSet {
 	ds, err := NewDaemonSet(MustAssetReader(TunedDaemonSetAsset))
 	if err != nil {
@@ -126,20 +82,3 @@ func NewTuned(manifest io.Reader) (*tunedv1.Tuned, error) {
 	}
 	return &o, nil
 }
-
-func tunedRenderedProfiles(tuned *tunedv1.Tuned, m map[string]tunedv1.TunedProfile) {
-	if tuned.Spec.Profile != nil {
-		for _, v := range tuned.Spec.Profile {
-			if v.Name != nil && v.Data != nil {
-				if existingProfile, found := m[*v.Name]; found {
-					if *v.Data == *existingProfile.Data {
-						klog.Infof("duplicate profiles names %s but they have the same contents", *v.Name)
-					} else {
-						klog.Errorf("ERROR: duplicate profiles named %s with different contents found in Tuned CR %q", *v.Name, tuned.Name)
-					}
-				}
-				m[*v.Name] = v
-			}
-		}
-	}
-}

pkg/operator/controller.go

@@ -397,10 +397,11 @@ func (c *Controller) sync(key wqKey) error {
 		}
 	}
 
-	klog.V(2).Infof("sync(): Tuned %s", tunedv1.TunedRenderedResourceName)
-	err = c.syncTunedRendered(cr)
+	klog.V(2).Infof("sync(): enableInformers")
+	// Enable/disable node/pod informers based on existing TuneD CRs.
+	err = c.enableInformers()
 	if err != nil {
-		return fmt.Errorf("failed to sync Tuned %s: %v", tunedv1.TunedRenderedResourceName, err)
+		return fmt.Errorf("failed to enable/disable informers: %v", err)
 	}
 
 	if key.name != tunedv1.TunedDefaultResourceName {
@@ -428,11 +429,6 @@ func (c *Controller) sync(key wqKey) error {
 		return err
 	}
 
-	if key.name == tunedv1.TunedRenderedResourceName {
-		// Do not start unused MachineConfig pruning unnecessarily for the rendered resource
-		return nil
-	}
-
 	// Tuned CR change can also mean some MachineConfigs the operator created are no longer needed;
 	// removal of these will also rollback host settings such as kernel boot parameters.
 	if ntoconfig.InHyperShift() {
@@ -514,16 +510,12 @@ func (c *Controller) syncTunedDefault() (*tunedv1.Tuned, error) {
 	return cr, nil
 }
 
-func (c *Controller) syncTunedRendered(tuned *tunedv1.Tuned) error {
+func (c *Controller) enableInformers() error {
 	tunedList, err := c.listers.TunedResources.List(labels.Everything())
 	if err != nil {
 		return fmt.Errorf("failed to list Tuned: %v", err)
 	}
 
-	crMf := ntomf.TunedRenderedResource(tunedList)
-	crMf.ObjectMeta.OwnerReferences = getDefaultTunedRefs(tuned)
-	crMf.Name = tunedv1.TunedRenderedResourceName
-
 	nodeLabelsUsed := c.pc.tunedsUseNodeLabels(tunedList)
 	if err = c.enableNodeInformer(nodeLabelsUsed); err != nil {
 		return fmt.Errorf("failed to enable Node informer: %v", err)
@@ -536,35 +528,6 @@ func (c *Controller) syncTunedRendered(tuned *tunedv1.Tuned) error {
 		return fmt.Errorf("failed to enable Pod informer: %v", err)
 	}
 
-	cr, err := c.listers.TunedResources.Get(tunedv1.TunedRenderedResourceName)
-	if err != nil {
-		if errors.IsNotFound(err) {
-			klog.V(2).Infof("syncTunedRendered(): Tuned %s not found, creating one", crMf.Name)
-			_, err = c.clients.Tuned.TunedV1().Tuneds(ntoconfig.WatchNamespace()).Create(context.TODO(), crMf, metav1.CreateOptions{})
-			if err != nil {
-				return fmt.Errorf("failed to create Tuned %s: %v", crMf.Name, err)
-			}
-			// Tuned created successfully
-			klog.Infof("created Tuned %s", crMf.Name)
-			return nil
-		}
-		return fmt.Errorf("failed to get Tuned %s: %v", tunedv1.TunedRenderedResourceName, err)
-	}
-
-	if reflect.DeepEqual(crMf.Spec.Profile, cr.Spec.Profile) {
-		klog.V(2).Infof("syncTunedRendered(): Tuned %s doesn't need updating", crMf.Name)
-		return nil
-	}
-	cr = cr.DeepCopy() // never update the objects from cache
-	cr.Spec = crMf.Spec
-
-	klog.V(2).Infof("syncTunedRendered(): updating Tuned %s", cr.Name)
-	_, err = c.clients.Tuned.TunedV1().Tuneds(ntoconfig.WatchNamespace()).Update(context.TODO(), cr, metav1.UpdateOptions{})
-	if err != nil {
-		return fmt.Errorf("failed to update Tuned %s: %v", cr.Name, err)
-	}
-	klog.Infof("updated Tuned %s", cr.Name)
-
 	return nil
 }
 
@@ -627,6 +590,7 @@ func (c *Controller) syncDaemonSet(tuned *tunedv1.Tuned) error {
 func (c *Controller) syncProfile(tuned *tunedv1.Tuned, nodeName string) error {
 	var (
 		tunedProfileName string
+		profilesAll      []tunedv1.TunedProfile
 		mcLabels         map[string]string
 		operand          tunedv1.OperandConfig
 		nodePoolName     string
@@ -661,23 +625,19 @@ func (c *Controller) syncProfile(tuned *tunedv1.Tuned, nodeName string) error {
 	}
 
 	if ntoconfig.InHyperShift() {
-		tunedProfileName, nodePoolName, operand, err = c.pc.calculateProfileHyperShift(nodeName)
+		tunedProfileName, profilesAll, nodePoolName, operand, err = c.pc.calculateProfileHyperShift(nodeName)
 		if err != nil {
 			return err
 		}
 	} else {
-		tunedProfileName, mcLabels, operand, err = c.pc.calculateProfile(nodeName)
+		tunedProfileName, profilesAll, mcLabels, operand, err = c.pc.calculateProfile(nodeName)
 		if err != nil {
 			return err
 		}
 	}
 
 	metrics.ProfileCalculated(profileMf.Name, tunedProfileName)
 
-	if err != nil {
-		return fmt.Errorf("failed to get Tuned %s: %v", tunedv1.TunedRenderedResourceName, err)
-	}
-
 	profile, err := c.listers.TunedProfiles.Get(profileMf.Name)
 	if err != nil {
 		if errors.IsNotFound(err) {
@@ -694,6 +654,7 @@ func (c *Controller) syncProfile(tuned *tunedv1.Tuned, nodeName string) error {
 			profileMf.Spec.Config.TunedProfile = tunedProfileName
 			profileMf.Spec.Config.Debug = operand.Debug
 			profileMf.Spec.Config.TuneDConfig = operand.TuneDConfig
+			profileMf.Spec.Profile = profilesAll
 			profileMf.Status.Conditions = tunedpkg.InitializeStatusConditions()
 			_, err = c.clients.Tuned.TunedV1().Profiles(ntoconfig.WatchNamespace()).Create(context.TODO(), profileMf, metav1.CreateOptions{})
 			if err != nil {
@@ -754,6 +715,7 @@ func (c *Controller) syncProfile(tuned *tunedv1.Tuned, nodeName string) error {
 	if profile.Spec.Config.TunedProfile == tunedProfileName &&
 		profile.Spec.Config.Debug == operand.Debug &&
 		reflect.DeepEqual(profile.Spec.Config.TuneDConfig, operand.TuneDConfig) &&
+		reflect.DeepEqual(profile.Spec.Profile, profilesAll) &&
 		profile.Spec.Config.ProviderName == providerName {
 		klog.V(2).Infof("syncProfile(): no need to update Profile %s", nodeName)
 		return nil
@@ -762,6 +724,7 @@ func (c *Controller) syncProfile(tuned *tunedv1.Tuned, nodeName string) error {
 	profile.Spec.Config.TunedProfile = tunedProfileName
 	profile.Spec.Config.Debug = operand.Debug
 	profile.Spec.Config.TuneDConfig = operand.TuneDConfig
+	profile.Spec.Profile = profilesAll
 	profile.Spec.Config.ProviderName = providerName
 	profile.Status.Conditions = tunedpkg.InitializeStatusConditions()
 
@@ -1274,6 +1237,30 @@ func (c *Controller) enablePodInformer(enable bool) error {
 	return nil
 }
 
+// Remove this function and associated code in the future.  This is for cleanup during upgrades only.
+// The rendered resource is no longer used.
+func (c *Controller) removeTunedRendered() error {
+	var err error
+
+	_, err = c.listers.TunedResources.Get(tunedv1.TunedRenderedResourceName)
+	if err != nil {
+		if errors.IsNotFound(err) {
+			// Do not create any noise when TunedRenderedResourceName is not found (was already removed).
+			err = nil
+		} else {
+			err = fmt.Errorf("failed to get Tuned %s: %v", tunedv1.TunedRenderedResourceName, err)
+		}
+	} else {
+		err = c.clients.Tuned.TunedV1().Tuneds(ntoconfig.WatchNamespace()).Delete(context.TODO(), tunedv1.TunedRenderedResourceName, metav1.DeleteOptions{})
+		if err != nil {
+			err = fmt.Errorf("failed to delete Tuned %s: %v", tunedv1.TunedRenderedResourceName, err)
+		} else {
+			klog.Infof("deleted Tuned %s", tunedv1.TunedRenderedResourceName)
+		}
+	}
+	return err
+}
+
 func (c *Controller) removeResources() error {
 	var lastErr error
 	dsMf := ntomf.TunedDaemonSet()
@@ -1293,18 +1280,10 @@ func (c *Controller) removeResources() error {
 		}
 	}
 
-	_, err = c.listers.TunedResources.Get(tunedv1.TunedRenderedResourceName)
-	if err != nil {
-		if !errors.IsNotFound(err) {
-			lastErr = fmt.Errorf("failed to get Tuned %s: %v", tunedv1.TunedRenderedResourceName, err)
-		}
-	} else {
-		err = c.clients.Tuned.TunedV1().Tuneds(ntoconfig.WatchNamespace()).Delete(ctx, tunedv1.TunedRenderedResourceName, metav1.DeleteOptions{})
-		if err != nil {
-			lastErr = fmt.Errorf("failed to delete Tuned %s: %v", tunedv1.TunedRenderedResourceName, err)
-		} else {
-			klog.Infof("deleted Tuned %s", tunedv1.TunedRenderedResourceName)
-		}
+	// Remove this code in the future.  This is for cleanup during upgrades only.
+	// The rendered resource is no longer used.
+	if err := c.removeTunedRendered(); err != nil {
+		lastErr = err
 	}
 
 	profileList, err := c.listers.TunedProfiles.List(labels.Everything())
@@ -1451,6 +1430,12 @@ func (c *Controller) run(ctx context.Context) error {
 		return fmt.Errorf("failed to wait for caches to sync")
 	}
 
+	// Remove this code in the future.  This is for cleanup during upgrades only.
+	// The rendered resource is no longer used.
+	if err := c.removeTunedRendered(); err != nil {
+		klog.Error(err)
+	}
+
 	klog.V(1).Info("starting events processor")
 	go wait.Until(c.eventProcessor, time.Second, ctx.Done())
 	klog.Info("started events processor/controller")

pkg/operator/hypershift.go

@@ -101,7 +101,7 @@ func (c *Controller) syncHostedClusterTuneds() error {
 	}
 	// Anything left in hcMap should be deleted
 	for tunedName := range hcTunedMap {
-		if tunedName != tunedv1.TunedDefaultResourceName && tunedName != tunedv1.TunedRenderedResourceName {
+		if tunedName != tunedv1.TunedDefaultResourceName {
 			klog.V(1).Infof("deleting stale Tuned %s in hosted cluster", tunedName)
 			err = c.clients.Tuned.TunedV1().Tuneds(ntoconfig.WatchNamespace()).Delete(context.TODO(), tunedName, metav1.DeleteOptions{})
 			if err != nil {