OCPBUGS-18052: feat: added logic to handle legacy sno install (#778)

* feat: added logic to handle legacy sno install

updated documentation to correctly refer to feature as 4.14
changed variables to be more consistent with current naming pattern

Signed-off-by: ehila <ehila@redhat.com>

* doc: added comment and updated var name

Signed-off-by: ehila <ehila@redhat.com>

* refactor: updated function calls to use non-deprecated versions

Signed-off-by: ehila <ehila@redhat.com>

* feat: add e2e test for rendering default and legacy cpu partitioning

Signed-off-by: ehila <ehila@redhat.com>

---------

Signed-off-by: ehila <ehila@redhat.com>

Author: eggfoobar

cmd/cluster-node-tuning-operator/main.go

@@ -266,9 +266,14 @@ func removePerformanceOLMOperator(cfg *rest.Config) error {
 	return nil
 }
 
-// During upgrade from 4.12 -> 4.13, this logic will update the authoritative flag in the new
+// During upgrade from 4.13 -> 4.14, this logic will update the authoritative flag in the new
 // Infrastructures.Status.CPUPartitioning to migrate Single Node clusters to the new method.
-// TODO: Revisit after 4.13 to remove logic when no longer needed.
+// Note:
+// This method will also execute during fresh installs on SNO using the legacy method. We are
+// generating the bootstrap files during that process, when this method then executes, it will only
+// update the API flag as intended, and since the bootstrap configs will already exist, nothing will happen.
+//
+// TODO: Revisit after 4.14 to remove logic when no longer needed.
 func migratePinnedSingleNodeInfraStatus(cfg *rest.Config, scheme *apiruntime.Scheme) error {
 	k8sclient, err := client.New(cfg, client.Options{Scheme: scheme})
 	if err != nil {

pkg/performanceprofile/cmd/render/render.go

@@ -16,13 +16,15 @@ limitations under the License.
 package render
 
 import (
+	"encoding/json"
 	"fmt"
 	"os"
 	"path/filepath"
 	"strings"
 
 	"sigs.k8s.io/yaml"
 
+	igntypes "github.com/coreos/ignition/v2/config/v3_2/types"
 	apicfgv1 "github.com/openshift/api/config/v1"
 	mcfgv1 "github.com/openshift/machine-config-operator/pkg/apis/machineconfiguration.openshift.io/v1"
 
@@ -86,6 +88,7 @@ func render(inputDir, outputDir string) error {
 	var (
 		perfProfiles []*performancev2.PerformanceProfile
 		mcPools      []*mcfgv1.MachineConfigPool
+		mcConfigs    []*mcfgv1.MachineConfig
 		infra        *apicfgv1.Infrastructure
 		ctrcfgs      []*mcfgv1.ContainerRuntimeConfig
 	)
@@ -102,7 +105,7 @@ func render(inputDir, outputDir string) error {
 			return fmt.Errorf("error parsing manifests from %s: %w", file.Name(), err)
 		}
 
-		// Decode manifest files and
+		// Decode manifest files
 		for idx, m := range manifests {
 			obji, err := runtime.Decode(runtimeDecoder, m.Raw)
 			if err != nil {
@@ -118,6 +121,8 @@ func render(inputDir, outputDir string) error {
 				perfProfiles = append(perfProfiles, obj)
 			case *mcfgv1.MachineConfigPool:
 				mcPools = append(mcPools, obj)
+			case *mcfgv1.MachineConfig:
+				mcConfigs = append(mcConfigs, obj)
 			case *apicfgv1.Infrastructure:
 				if obj.Name == clusterConfigResourceName {
 					infra = obj
@@ -134,18 +139,23 @@ func render(inputDir, outputDir string) error {
 		klog.Warning("zero performance profiles were found")
 	}
 
-	var pinnedMode *apicfgv1.CPUPartitioningMode
+	var partitioningMode *apicfgv1.CPUPartitioningMode
 	if infra != nil {
-		pinnedMode = &infra.Status.CPUPartitioning
+		partitioningMode = &infra.Status.CPUPartitioning
 	}
 
-	if err := genBootstrapWorkloadPinningManifests(pinnedMode, outputDir, defaultMCPNames...); err != nil {
+	if isLegacySNOWorkloadPinningMethod(mcConfigs, infra, partitioningMode) {
+		legacyAllNodes := apicfgv1.CPUPartitioningAllNodes
+		partitioningMode = &legacyAllNodes
+	}
+
+	if err := genBootstrapWorkloadPinningManifests(partitioningMode, outputDir, defaultMCPNames...); err != nil {
 		return err
 	}
 
 	// If the user supplies extra machine pools, we ingest them here
 	for _, pool := range mcPools {
-		if err := genBootstrapWorkloadPinningManifests(pinnedMode, outputDir, pool.Name); err != nil {
+		if err := genBootstrapWorkloadPinningManifests(partitioningMode, outputDir, pool.Name); err != nil {
 			return err
 		}
 	}
@@ -166,7 +176,7 @@ func render(inputDir, outputDir string) error {
 			return fmt.Errorf("render: could not determine high-performance runtime class container-runtime for profile %q; %w", pp.Name, err)
 		}
 
-		components, err := manifestset.GetNewComponents(pp, mcp, pinnedMode, defaultRuntime)
+		components, err := manifestset.GetNewComponents(pp, mcp, partitioningMode, defaultRuntime)
 		if err != nil {
 			return err
 		}
@@ -210,6 +220,49 @@ func render(inputDir, outputDir string) error {
 	return nil
 }
 
+// isLegacySNOWorkloadPinningMethod provides a check for situations where the user is creating an SNO cluster with the
+// legacy method for CPU Partitioning. In order to make sure the bootstrap and running cluster MCs are synced up we check the MCs
+// provided by the user, if any one of them contain the file addition to `/etc/kubernetes/openshift-workload-pinning` and have not set
+// the API CPUPartitioningAllNodes at install time, we assume a legacy intention and alter the flag to generate the new bootstrap manifests.
+//
+// Note:
+//   - This will only trigger when Control plane is SNO or the CPU Partitioning API is NOT set to AllNodes
+//   - We do not alter the API flag here, when NTO starts up in cluster, it will notice the flag and
+//     update the flag and ignore the create error since the files already exist.
+func isLegacySNOWorkloadPinningMethod(mcs []*mcfgv1.MachineConfig, infra *apicfgv1.Infrastructure, partitioningMode *apicfgv1.CPUPartitioningMode) bool {
+
+	// If we can't determine SNO topology, we return.
+	if infra == nil {
+		return false
+	}
+
+	if infra.Status.ControlPlaneTopology != apicfgv1.SingleReplicaTopologyMode || (partitioningMode != nil && *partitioningMode == apicfgv1.CPUPartitioningAllNodes) {
+		return false
+	}
+
+	// This file name is stable and currently hardcoded in kubelet
+	// https://github.com/openshift/kubernetes/blob/ba1825544533d273d86b405195ee791e500b74c7/pkg/kubelet/managed/managed.go#L31
+	const kubernetesPinningConfFile = "/etc/kubernetes/openshift-workload-pinning"
+
+	for _, mc := range mcs {
+		ign := &igntypes.Config{}
+		err := json.Unmarshal(mc.Spec.Config.Raw, ign)
+		if err != nil {
+			klog.Errorf("skipping legacy check on mc (%s) unable to marshal raw config to ignition struct: %s", mc.Name, err)
+			continue
+		}
+
+		for _, file := range ign.Storage.Files {
+			if file.Node.Path == kubernetesPinningConfFile {
+				klog.Infof("mc (%s) contains file path (%s), using legacy signal for workload pinning", mc.Name, kubernetesPinningConfFile)
+				return true
+			}
+		}
+	}
+
+	return false
+}
+
 // genBootstrapWorkloadPinningManifests is used to generate the appropriate bootstrap workload pinning manifests
 // based on the cluster CPU Partitioning Mode and the given MachineConfigPool names. The created manifests are the
 // default state configs for the node which make no assumption for which CPU's are used for workload pinning.

test/e2e/performanceprofile/cluster-setup/pinned-cluster/default/infrastructure.yaml

@@ -0,0 +1,12 @@
+apiVersion: config.openshift.io/v1
+kind: Infrastructure
+metadata:
+  name: cluster
+spec:
+  cloudConfig:
+    name: ""
+  platformSpec:
+    aws: {}
+    type: AWS
+status:
+  cpuPartitioning: AllNodes

test/e2e/performanceprofile/cluster-setup/pinned-cluster/single-node-legacy/infrastructure.yaml

@@ -0,0 +1,12 @@
+apiVersion: config.openshift.io/v1
+kind: Infrastructure
+metadata:
+  name: cluster
+spec:
+  cloudConfig:
+    name: ""
+  platformSpec:
+    aws: {}
+    type: AWS
+status:
+  controlPlaneTopology: SingleReplica

test/e2e/performanceprofile/cluster-setup/pinned-cluster/single-node-legacy/mc.yaml

@@ -0,0 +1,17 @@
+apiVersion: machineconfiguration.openshift.io/v1
+kind: MachineConfig
+metadata:
+  labels:
+    machineconfiguration.openshift.io/role: master
+  name: 02-master-workload-partitioning
+spec:
+  config:
+    ignition:
+      version: 3.2.0
+    storage:
+      files:
+        - contents:
+            source: data:text/plain;charset=utf-8;base64,ewogICJtYW5hZ2VtZW50IjogewogICAgImNwdXNldCI6ICIiCiAgfQp9Cg==
+          mode: 420
+          overwrite: true
+          path: /etc/kubernetes/openshift-workload-pinning

test/e2e/performanceprofile/functests-render-command/1_render_command/render_test.go

@@ -2,7 +2,6 @@ package __render_command_test
 
 import (
 	"fmt"
-	"io/ioutil"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -12,11 +11,18 @@ import (
 	. "github.com/onsi/gomega"
 )
 
+const (
+	defaultExpectedDir = "default"
+	pinnedExpectedDir  = "pinned"
+)
+
 var (
-	assetsOutDir string
-	assetsInDirs []string
-	ppDir        string
-	testDataPath string
+	assetsOutDir       string
+	assetsInDirs       []string
+	ppDir              string
+	testDataPath       string
+	defaultPinnedDir   string
+	snoLegacyPinnedDir string
 )
 
 var _ = Describe("render command e2e test", func() {
@@ -25,6 +31,8 @@ var _ = Describe("render command e2e test", func() {
 		assetsOutDir = createTempAssetsDir()
 		assetsInDir := filepath.Join(workspaceDir, "test", "e2e", "performanceprofile", "cluster-setup", "base", "performance")
 		ppDir = filepath.Join(workspaceDir, "test", "e2e", "performanceprofile", "cluster-setup", "manual-cluster", "performance")
+		defaultPinnedDir = filepath.Join(workspaceDir, "test", "e2e", "performanceprofile", "cluster-setup", "pinned-cluster", "default")
+		snoLegacyPinnedDir = filepath.Join(workspaceDir, "test", "e2e", "performanceprofile", "cluster-setup", "pinned-cluster", "single-node-legacy")
 		testDataPath = filepath.Join(workspaceDir, "test", "e2e", "performanceprofile", "testdata")
 		assetsInDirs = []string{assetsInDir, ppDir}
 	})
@@ -41,7 +49,7 @@ var _ = Describe("render command e2e test", func() {
 			fmt.Fprintf(GinkgoWriter, "running: %v\n", cmdline)
 
 			cmd := exec.Command(cmdline[0], cmdline[1:]...)
-			runAndCompare(cmd)
+			runAndCompare(cmd, defaultExpectedDir)
 
 		})
 
@@ -57,7 +65,39 @@ var _ = Describe("render command e2e test", func() {
 				fmt.Sprintf("ASSET_INPUT_DIR=%s", strings.Join(assetsInDirs, ",")),
 				fmt.Sprintf("ASSET_OUTPUT_DIR=%s", assetsOutDir),
 			)
-			runAndCompare(cmd)
+			runAndCompare(cmd, defaultExpectedDir)
+		})
+	})
+
+	Context("With pinned cluster resources", func() {
+		It("Given default pinned infrastructure status, should render cpu partitioning configs", func() {
+
+			cmdline := []string{
+				filepath.Join(binPath, "cluster-node-tuning-operator"),
+				"render",
+				"--asset-input-dir", defaultPinnedDir,
+				"--asset-output-dir", assetsOutDir,
+			}
+			fmt.Fprintf(GinkgoWriter, "running: %v\n", cmdline)
+
+			cmd := exec.Command(cmdline[0], cmdline[1:]...)
+			runAndCompare(cmd, pinnedExpectedDir)
+
+		})
+
+		It("Given legacy SNO pinned infrastructure status, should render cpu partitioning configs", func() {
+
+			cmdline := []string{
+				filepath.Join(binPath, "cluster-node-tuning-operator"),
+				"render",
+				"--asset-input-dir", snoLegacyPinnedDir,
+				"--asset-output-dir", assetsOutDir,
+			}
+			fmt.Fprintf(GinkgoWriter, "running: %v\n", cmdline)
+
+			cmd := exec.Command(cmdline[0], cmdline[1:]...)
+			runAndCompare(cmd, pinnedExpectedDir)
+
 		})
 	})
 
@@ -68,7 +108,7 @@ var _ = Describe("render command e2e test", func() {
 })
 
 func createTempAssetsDir() string {
-	assets, err := ioutil.TempDir("", "assets")
+	assets, err := os.MkdirTemp("", "assets")
 	Expect(err).ToNot(HaveOccurred())
 	fmt.Printf("assets` output dir at: %q\n", assets)
 	return assets
@@ -78,21 +118,21 @@ func cleanArtifacts() {
 	os.RemoveAll(assetsOutDir)
 }
 
-func runAndCompare(cmd *exec.Cmd) {
+func runAndCompare(cmd *exec.Cmd, dir string) {
 	_, err := cmd.Output()
 	Expect(err).ToNot(HaveOccurred())
 
-	outputAssetsFiles, err := ioutil.ReadDir(assetsOutDir)
+	outputAssetsFiles, err := os.ReadDir(assetsOutDir)
 	Expect(err).ToNot(HaveOccurred())
 
-	refPath := filepath.Join(testDataPath, "render-expected-output")
+	refPath := filepath.Join(testDataPath, "render-expected-output", dir)
 	fmt.Fprintf(GinkgoWriter, "reference data at: %q\n", refPath)
 
 	for _, f := range outputAssetsFiles {
-		refData, err := ioutil.ReadFile(filepath.Join(refPath, f.Name()))
+		refData, err := os.ReadFile(filepath.Join(refPath, f.Name()))
 		Expect(err).ToNot(HaveOccurred())
 
-		data, err := ioutil.ReadFile(filepath.Join(assetsOutDir, f.Name()))
+		data, err := os.ReadFile(filepath.Join(assetsOutDir, f.Name()))
 		Expect(err).ToNot(HaveOccurred())
 
 		diff, err := getFilesDiff(data, refData)

test/e2e/performanceprofile/testdata/render-expected-output/manual_kubeletconfig.yaml renamed to test/e2e/performanceprofile/testdata/render-expected-output/default/manual_kubeletconfig.yaml

@@ -0,0 +1,42 @@
+apiVersion: machineconfiguration.openshift.io/v1
+kind: MachineConfig
+metadata:
+  creationTimestamp: null
+  labels:
+    machineconfiguration.openshift.io/role: master
+  name: 01-master-cpu-partitioning
+spec:
+  baseOSExtensionsContainerImage: ""
+  config:
+    ignition:
+      config:
+        replace:
+          verification: {}
+      proxy: {}
+      security:
+        tls: {}
+      timeouts: {}
+      version: 3.2.0
+    passwd: {}
+    storage:
+      files:
+      - contents:
+          source: data:text/plain;charset=utf-8;base64,CnsKICAibWFuYWdlbWVudCI6IHsKICAgICJjcHVzZXQiOiAiIgogIH0KfQo=
+          verification: {}
+        group: {}
+        mode: 420
+        path: /etc/kubernetes/openshift-workload-pinning
+        user: {}
+      - contents:
+          source: data:text/plain;charset=utf-8;base64,CltjcmlvLnJ1bnRpbWUud29ya2xvYWRzLm1hbmFnZW1lbnRdCmFjdGl2YXRpb25fYW5ub3RhdGlvbiA9ICJ0YXJnZXQud29ya2xvYWQub3BlbnNoaWZ0LmlvL21hbmFnZW1lbnQiCmFubm90YXRpb25fcHJlZml4ID0gInJlc291cmNlcy53b3JrbG9hZC5vcGVuc2hpZnQuaW8iCnJlc291cmNlcyA9IHsgImNwdXNoYXJlcyIgPSAwLCAiY3B1c2V0IiA9ICIiIH0K
+          verification: {}
+        group: {}
+        mode: 420
+        path: /etc/crio/crio.conf.d/01-workload-pinning-default.conf
+        user: {}
+    systemd: {}
+  extensions: null
+  fips: false
+  kernelArguments: null
+  kernelType: ""
+  osImageURL: ""