OCPBUGS-28647: deferred updates: cleanups (#1119)

* tuned: controller: pack profilesExtract return values in struct xref: #1019 (comment) Signed-off-by: Francesco Romani <fromani@redhat.com> * tuned: controller: align to left Simplify the flow aligning code to the left. No intended changes in behavior. xref: #1019 (comment) Signed-off-by: Francesco Romani <fromani@redhat.com> * tuned: controller: narrow down reapplySysctl Simplify the code with no intended changes in behavior. Signed-off-by: Francesco Romani <fromani@redhat.com> * e2e: deferred: add ginkgo labels and update the tags to match. This way we can easily skip the flaky tests The naming schema is made consistent with performanceprofile labels: test/e2e/performanceprofile/functests/utils/label/label.go note the recommended kube model is to use `CapitalizedNouns`, while we do `lowercase-dash-separated`. Signed-off-by: Francesco Romani <fromani@redhat.com> * e2e: skip flaky tests in nightly runs nightlies call `make test-e2e` so skip the flaky tests here. Presubmit lanes (= running pre-merge) should still run all tests Signed-off-by: Francesco Romani <fromani@redhat.com> * tuned: controller: fix run directory Fix the runtime directory to be forward compatible xref: #1019 (comment) Signed-off-by: Francesco Romani <fromani@redhat.com> --------- Signed-off-by: Francesco Romani <fromani@redhat.com>
openshift · Jul 23, 2024 · a869242 · a869242
1 parent 10bb4c6
commit a869242
Show file tree

Hide file tree

Showing 6 changed files with 54 additions and 37 deletions.
diff --git a/Makefile b/Makefile
@@ -101,7 +101,7 @@ $(GOBINDATA_BIN):
 
 test-e2e: $(BINDATA)
 	for d in core basic reboots reboots/sno deferred; do \
-	  KUBERNETES_CONFIG="$(KUBECONFIG)" $(GO) test -v -timeout 40m ./test/e2e/$$d -ginkgo.v -ginkgo.no-color -ginkgo.fail-fast || exit; \
+	  KUBERNETES_CONFIG="$(KUBECONFIG)" $(GO) test -v -timeout 40m ./test/e2e/$$d -ginkgo.v -ginkgo.no-color -ginkgo.fail-fast -ginkgo.label-filter=!flaky || exit; \
 	done
 
 .PHONY: test-e2e-local

diff --git a/pkg/tuned/cmd/render/render.go b/pkg/tuned/cmd/render/render.go
@@ -213,7 +213,7 @@ func render(inputDir []string, outputDir string, mcpName string) error {
 	for _, t := range tuneD {
 		tunedProfiles = append(tunedProfiles, t.Spec.Profile...)
 	}
-	_, _, _, _, err = tunedpkg.ProfilesExtract(tunedProfiles, recommendedProfile)
+	_, err = tunedpkg.ProfilesExtract(tunedProfiles, recommendedProfile)
 	if err != nil {
 		klog.Errorf("error extracting tuned profiles : %v", err)
 		return fmt.Errorf("error extracting tuned profiles: %w", err)

diff --git a/pkg/tuned/controller.go b/pkg/tuned/controller.go
@@ -80,7 +80,7 @@ const (
 	// be generous and give it 10s.
 	tunedGracefulExitWait = time.Second * time.Duration(10)
 	ocpTunedHome          = "/var/lib/ocp-tuned"
-	ocpTunedRunDir        = "/run/" + programName
+	ocpTunedRunDir        = "/run/ocp-tuned"
 	ocpTunedProvider      = ocpTunedHome + "/provider"
 	// With the less aggressive rate limiter, retries will happen at 100ms*2^(retry_n-1):
 	// 100ms, 200ms, 400ms, 800ms, 1.6s, 3.2s, 6.4s, 12.8s, 25.6s, 51.2s, 102.4s, 3.4m, 6.8m, 13.7m, 27.3m
@@ -389,15 +389,23 @@ func profilesEqual(profileFile string, profileData string) bool {
 	return profileData == string(content)
 }
 
+type ExtractedProfiles struct {
+	// True if the data in the to-be-extracted recommended profile or the profiles being
+	// included from the current recommended profile have changed.
+	Changed bool
+	// If the data changed, the fingerprint of the new profile, or "" otherwise.
+	Fingerprint string
+	// A map with successfully extracted TuneD profile names.
+	Names map[string]bool
+	// A map with names of TuneD profiles the current TuneD recommended profile depends on.
+	Dependencies map[string]bool
+}
+
 // ProfilesExtract extracts TuneD daemon profiles to tunedProfilesDirCustom directory.
 // Returns:
-//   - True if the data in the to-be-extracted recommended profile or the profiles being
-//     included from the current recommended profile have changed.
-//   - If the data changed, the fingerprint of the new profile, or "" otherwise.
-//   - A map with successfully extracted TuneD profile names.
-//   - A map with names of TuneD profiles the current TuneD recommended profile depends on.
+//   - ExtractedProfiles with the details of the operation performed
 //   - Error if any or nil.
-func ProfilesExtract(profiles []tunedv1.TunedProfile, recommendedProfile string) (bool, string, map[string]bool, map[string]bool, error) {
+func ProfilesExtract(profiles []tunedv1.TunedProfile, recommendedProfile string) (ExtractedProfiles, error) {
 	klog.Infof("profilesExtract(): extracting %d TuneD profiles (recommended=%s)", len(profiles), recommendedProfile)
 	// Get a list of TuneD profiles names the recommended profile depends on.
 	deps := profileDepends(recommendedProfile)
@@ -409,7 +417,7 @@ func ProfilesExtract(profiles []tunedv1.TunedProfile, recommendedProfile string)
 
 // profilesExtractPathWithDeps is like ProfilesExtract but takes explicit profiles root dir path and
 // explicit dependencies, so it's easier to test. To be used only internally.
-func profilesExtractPathWithDeps(profilesRootDir string, profiles []tunedv1.TunedProfile, recommendedProfile string, recommendedProfileDeps map[string]bool) (bool, string, map[string]bool, map[string]bool, error) {
+func profilesExtractPathWithDeps(profilesRootDir string, profiles []tunedv1.TunedProfile, recommendedProfile string, recommendedProfileDeps map[string]bool) (ExtractedProfiles, error) {
 	var (
 		change    bool            = false
 		extracted map[string]bool = map[string]bool{} // TuneD profile names present in TuneD CR and successfully extracted to tunedProfilesDirCustom
@@ -428,7 +436,11 @@ func profilesExtractPathWithDeps(profilesRootDir string, profiles []tunedv1.Tune
 		profileFile := filepath.Join(profileDir, tunedConfFile)
 
 		if err := os.MkdirAll(profileDir, os.ModePerm); err != nil {
-			return change, "", extracted, recommendedProfileDeps, fmt.Errorf("failed to create TuneD profile directory %q: %v", profileDir, err)
+			return ExtractedProfiles{
+				Changed:      change,
+				Names:        extracted,
+				Dependencies: recommendedProfileDeps,
+			}, fmt.Errorf("failed to create TuneD profile directory %q: %v", profileDir, err)
 		}
 
 		if recommendedProfileDeps[*profile.Name] {
@@ -444,15 +456,24 @@ func profilesExtractPathWithDeps(profilesRootDir string, profiles []tunedv1.Tune
 
 		err := os.WriteFile(profileFile, []byte(*profile.Data), 0644)
 		if err != nil {
-			return change, "", extracted, recommendedProfileDeps, fmt.Errorf("failed to write TuneD profile file %q: %v", profileFile, err)
+			return ExtractedProfiles{
+				Changed:      change,
+				Names:        extracted,
+				Dependencies: recommendedProfileDeps,
+			}, fmt.Errorf("failed to write TuneD profile file %q: %v", profileFile, err)
 		}
 		extracted[*profile.Name] = true
 		klog.V(2).Infof("profilesExtract(): extracted profile %q to %q (%d bytes)", *profile.Name, profileFile, len(*profile.Data))
 	}
 
 	profilesFP := profilesFingerprint(profiles, recommendedProfile)
 	klog.Infof("profilesExtract(): fingerprint of extracted profiles: %q", profilesFP)
-	return change, profilesFP, extracted, recommendedProfileDeps, nil
+	return ExtractedProfiles{
+		Changed:      change,
+		Fingerprint:  profilesFP,
+		Names:        extracted,
+		Dependencies: recommendedProfileDeps,
+	}, nil
 }
 
 // profilesRepackPath reconstructs the TunedProfile object from the data unpacked on the node
@@ -506,16 +527,17 @@ func profilesRepackPath(recommendFilePath, profilesRootDir string) ([]tunedv1.Tu
 //   - The synced profile fingerprint.
 //   - Error if any or nil.
 func profilesSync(profiles []tunedv1.TunedProfile, recommendedProfile string) (bool, string, error) {
-	change, profilesFP, extractedNew, recommendedProfileDeps, err := ProfilesExtract(profiles, recommendedProfile)
+	extracted, err := ProfilesExtract(profiles, recommendedProfile)
 	if err != nil {
-		return change, "", err
+		return extracted.Changed, "", err
 	}
 
 	dirEntries, err := os.ReadDir(tunedProfilesDirCustom)
 	if err != nil {
-		return change, "", err
+		return extracted.Changed, "", err
 	}
 
+	changed := extracted.Changed // shortcut, but we also modify before
 	// Deal with TuneD profiles absent from Tuned CRs, but still present in <tunedProfilesDirCustom>/<profile>/
 	for _, dirEntry := range dirEntries {
 		profile := dirEntry.Name()
@@ -529,26 +551,26 @@ func profilesSync(profiles []tunedv1.TunedProfile, recommendedProfile string) (b
 			continue
 		}
 
-		if extractedNew[profile] {
+		if extracted.Names[profile] {
 			// Do not delete the freshly extracted profiles.  These are the profiles present in the Profile CR.
 			continue
 		}
 		// This TuneD profile does not exist in the Profile CR, delete it.
 		profileDir := fmt.Sprintf("%s/%s", tunedProfilesDirCustom, profile)
 		err := os.RemoveAll(profileDir)
 		if err != nil {
-			return change, "", fmt.Errorf("failed to remove %q: %v", profileDir, err)
+			return changed, "", fmt.Errorf("failed to remove %q: %v", profileDir, err)
 		}
 		klog.Infof("profilesSync(): removed TuneD profile %q", profileDir)
 
-		if recommendedProfileDeps[profile] {
+		if extracted.Dependencies[profile] {
 			// This TuneD profile does not exist in the Profile CR, but the recommended profile depends on it.
 			// Trigger a change to report a configuration issue -- we depend on a profile that does not exist.
-			change = true
+			changed = true
 		}
 	}
 
-	return change, profilesFP, nil
+	return changed, extracted.Fingerprint, nil
 }
 
 // filterAndSortProfiles returns a slice of valid (non-nil name, non-nil data) profiles
@@ -1040,16 +1062,14 @@ func (c *Controller) changeSyncerTuneD(change Change) (synced bool, err error) {
 		}
 
 		// Does the current TuneD process have the reapply_sysctl option turned on?
-		reapplySysctl := c.tunedMainCfg.Section("").Key("reapply_sysctl").MustBool()
-		if reapplySysctl != change.reapplySysctl {
+		if reapplySysctl := c.tunedMainCfg.Section("").Key("reapply_sysctl").MustBool(); reapplySysctl != change.reapplySysctl {
 			klog.V(4).Infof("reapplySysctl rewriting configuration file")
-
 			if err = iniCfgSetKey(c.tunedMainCfg, "reapply_sysctl", !reapplySysctl); err != nil {
 				return false, err
 			}
 			err = iniFileSave(tunedMainConfPath, c.tunedMainCfg)
 			if err != nil {
-				return false, fmt.Errorf("failed to write global TuneD configuration file: %v", err)
+				return false, fmt.Errorf("failed to write global TuneD configuration file: %w", err)
 			}
 			klog.V(4).Infof("reapplySysctl triggering tuned restart")
 			restart = true // A complete restart of the TuneD daemon is needed due to configuration change in tuned-main.conf file.
@@ -1384,30 +1404,27 @@ func (c *Controller) storeDeferredUpdate(deferredFP string) (derr error) {
 //     If the file is absent, a node restart is assumed and true is returned.
 //   - Error if any.
 func (c *Controller) recoverAndClearDeferredUpdate() (string, bool, error) {
-	isReboot := false
-
 	deferredFP, err := os.ReadFile(tunedDeferredUpdatePersistentFilePath)
 	if err != nil {
 		if os.IsNotExist(err) {
 			klog.Infof("recover: no pending deferred change")
-			return "", isReboot, nil
+			return "", false, nil
 		}
 		klog.Infof("recover: failed to restore pending deferred change: %v", err)
-		return "", isReboot, err
+		return "", false, err
 	}
 	pendingFP := strings.TrimSpace(string(deferredFP))
 	err = os.Remove(tunedDeferredUpdatePersistentFilePath)
 	klog.Infof("recover: found pending deferred update: %q", pendingFP)
 
 	if _, errEph := os.Stat(tunedDeferredUpdateEphemeralFilePath); errEph != nil {
 		if os.IsNotExist(errEph) {
-			isReboot = true
-		} else {
-			klog.Infof("recover: failed to detect node restart, assuming not: %v", err)
-			return "", false, errEph
+			return pendingFP, true, err
 		}
+		klog.Infof("recover: failed to detect node restart, assuming not: %v", err)
+		return "", false, errEph
 	}
-	return pendingFP, isReboot, err
+	return pendingFP, false, err
 }
 
 func (c *Controller) informerEventHandler(workqueueKey wqKeyKube) cache.ResourceEventHandlerFuncs {

diff --git a/test/e2e/deferred/basic.go b/test/e2e/deferred/basic.go
@@ -19,7 +19,7 @@ import (
 	"github.com/openshift/cluster-node-tuning-operator/test/e2e/util"
 )
 
-var _ = ginkgo.Describe("[deferred][profile_status] Profile deferred", func() {
+var _ = ginkgo.Describe("[deferred][profile-status] Profile deferred", ginkgo.Label("deferred", "profile-status"), func() {
 	ginkgo.Context("when applied", func() {
 		var (
 			createdTuneds     []string

diff --git a/test/e2e/deferred/non_regression.go b/test/e2e/deferred/non_regression.go
@@ -18,7 +18,7 @@ import (
 	"github.com/openshift/cluster-node-tuning-operator/test/e2e/util"
 )
 
-var _ = ginkgo.Describe("[deferred][non_regression] Profile non-deferred", func() {
+var _ = ginkgo.Describe("[deferred][non-regression] Profile non-deferred", ginkgo.Label("deferred", "non-regression"), func() {
 	ginkgo.Context("when applied", func() {
 		var (
 			createdTuneds []string

diff --git a/test/e2e/deferred/restart.go b/test/e2e/deferred/restart.go
@@ -20,7 +20,7 @@ import (
 	"github.com/openshift/cluster-node-tuning-operator/test/e2e/util/wait"
 )
 
-var _ = ginkgo.Describe("[deferred][restart] Profile deferred", func() {
+var _ = ginkgo.Describe("[deferred][restart][slow][disruptive][flaky] Profile deferred", ginkgo.Label("deferred", "restart", "slow", "disruptive", "flaky"), func() {
 	ginkgo.Context("when restarting", func() {
 		var (
 			createdTuneds []string