OCPBUGS-18640: [release-4.14][manual] backport performance profile ow…

…ner reference ehnancements (#989)

* NO-JIRA: perfprof: render: make ownerReference optional (#907)

* perfprof: render: make ownerReference optional

When we set OwnerReference for dependant object, we must
use the object UID the apiserver set. Apiserver owns this field
(obviously) so setting it when missing, like the code did in the past,
is wrong and will cause conflicts on updates.

I the pre-render flow we can't wait for the apiserver to set a UID,
so the only possible option is to not set any OwnerReference at all.

Signed-off-by: Francesco Romani <fromani@redhat.com>

* perfprof: render: add weak owner reference support

There are cases on which we can't use the  real owner reference
in the render flow, because:
- we don't own the performanceprofile uid
- we can't wait to learn back the apiserver-decided uid because
  we need to render all the manifest at once.

so, to group objects together and enable to safely and easily
rebuild the ownerReference graph later, we add an annotation
which serves as weaker owner reference, using the profile name
and adding it to all the related objects.

Note this form of weak reference is meant to serve to a placeholder
for the real ownerReference which should be added by the relevant
controller later on, when the cluster is started and is running.

Signed-off-by: Francesco Romani <fromani@redhat.com>

---------

Signed-off-by: Francesco Romani <fromani@redhat.com>
(cherry picked from commit 0e31943)
(cherry picked from commit 5308f47)

* release-4.14: render: align expected machineconfig

the backported expected machineconfig output was including the
changes in 4.15, we need to update with the 4.14-specific content,
excluding unrelated changes.

Signed-off-by: Francesco Romani <fromani@redhat.com>

* render: perfprofile: don't annotate perfprof (#951)

don't add weak ref to the perfprof and
don't write back an updated version in output.

Signed-off-by: Francesco Romani <fromani@redhat.com>
(cherry picked from commit fd8ea4e)
(cherry picked from commit 34926bd)

* e2e: testdata: remove the annotated profile

after #951 merged, we don't need these anymore

Signed-off-by: Francesco Romani <fromani@redhat.com>
(cherry picked from commit 0d157b5)

* CNF-11145: Enhance render sync to include bootstrap rendering tests (#932)

* Enhance render sync to include bootstrap rendering tests

* make render-sync

(cherry picked from commit e225a86)

* Add support to inject owner-ref argument to render command (#960)

We need this change because we want to use also non default values for the owner-ref like the value none.

(cherry picked from commit 631e03c)

* release-4.14: hack: branch-specific updates

remove lines not relevant for this branch

Signed-off-by: Francesco Romani <fromani@redhat.com>

---------

Signed-off-by: Francesco Romani <fromani@redhat.com>
Co-authored-by: Martin Sivák <msivak@redhat.com>
Co-authored-by: Ronny Baturov <rbaturov@redhat.com>

hack/render-sync.sh

@@ -1,16 +1,41 @@
 #!/usr/bin/env bash
-
 WORKDIR=$(dirname "$(realpath "$0")")/..
-ARTIFACT_DIR=$(mktemp -d)
-
-cd "${WORKDIR}" || { echo "failed to change dir to ${WORKDIR}"; exit; }
-_output/cluster-node-tuning-operator render \
---asset-input-dir "${WORKDIR}"/test/e2e/performanceprofile/cluster-setup/manual-cluster/performance,"${WORKDIR}"/test/e2e/performanceprofile/cluster-setup/base/performance \
---asset-output-dir "${ARTIFACT_DIR}"
-
-cp "${ARTIFACT_DIR}"/*  "${WORKDIR}"/test/e2e/performanceprofile/testdata/render-expected-output/default
-for f in  "${WORKDIR}"/test/e2e/performanceprofile/testdata/render-expected-output/default/*
-do
-  sed -i "s/uid:.*/uid: \"\"/" "${f}"
-done
-rm -r "${ARTIFACT_DIR}"
+
+function join_by { local IFS="$1"; shift; echo "$*"; }
+
+function rendersync() {
+  INPUT_DIRS=()
+  EXTRA_ARGS=()
+  if [[ "$1" =~ ^-.* ]]; then
+      while [[ "x$1" != "x--" ]]; do
+          EXTRA_ARGS+=("$1")
+          shift
+      done
+      shift
+  fi
+  while (( $# > 1 )); do
+    INPUT_DIRS+=("${WORKDIR}/test/e2e/performanceprofile/cluster-setup/$1")
+    shift
+  done
+  INPUT_DIRS=$(join_by , ${INPUT_DIRS[@]})
+
+  OUTPUT_DIR=$1
+
+  echo "== Rendering ${INPUT_DIRS} into ${OUTPUT_DIR}"
+
+  ARTIFACT_DIR=$(mktemp -d)
+
+  cd "${WORKDIR}" || { echo "failed to change dir to ${WORKDIR}"; exit; }
+  _output/cluster-node-tuning-operator render ${EXTRA_ARGS[@]} \
+  --asset-input-dir "${INPUT_DIRS}" \
+  --asset-output-dir "${ARTIFACT_DIR}"
+
+  cp "${ARTIFACT_DIR}"/*  "${WORKDIR}"/test/e2e/performanceprofile/testdata/render-expected-output/${OUTPUT_DIR}
+  for f in  "${WORKDIR}"/test/e2e/performanceprofile/testdata/render-expected-output/${OUTPUT_DIR}/*
+  do
+    sed -i "s/uid:.*/uid: \"\"/" "${f}"
+  done
+  rm -r "${ARTIFACT_DIR}"
+}
+
+rendersync manual-cluster/performance base/performance default

pkg/performanceprofile/cmd/render/cmd.go

@@ -31,6 +31,7 @@ import (
 type renderOpts struct {
 	assetsInDir  string
 	assetsOutDir string
+	ownerRefMode string
 }
 
 // NewRenderCommand creates a render command.
@@ -39,6 +40,7 @@ type renderOpts struct {
 // needed to generate the machine configs.
 func NewRenderCommand() *cobra.Command {
 	renderOpts := renderOpts{}
+	renderOpts.SetDefaults()
 
 	cmd := &cobra.Command{
 		Use:   "render",
@@ -60,9 +62,14 @@ func NewRenderCommand() *cobra.Command {
 	return cmd
 }
 
+func (r *renderOpts) SetDefaults() {
+	r.ownerRefMode = ownerRefModeLabelName
+}
+
 func (r *renderOpts) AddFlags(fs *pflag.FlagSet) {
 	fs.StringVar(&r.assetsInDir, "asset-input-dir", components.AssetsDir, "Input path for the assets directory. (Can be a comma separated list of directories.)")
 	fs.StringVar(&r.assetsOutDir, "asset-output-dir", r.assetsOutDir, "Output path for the rendered manifests.")
+	fs.StringVar(&r.ownerRefMode, "owner-ref", r.ownerRefMode, "Add Owner Reference to rendered manifests. Accepted values: 'none' to disable; 'k8s' for proper owner reference; 'label-name' to use just a label.")
 	// environment variables has precedence over standard input
 	r.readFlagsFromEnv()
 }
@@ -75,18 +82,23 @@ func (r *renderOpts) readFlagsFromEnv() {
 	if assetsOutDir := os.Getenv("ASSET_OUTPUT_DIR"); len(assetsOutDir) > 0 {
 		r.assetsOutDir = assetsOutDir
 	}
+	if ownerRefMode, ok := os.LookupEnv("OWNER_REF"); ok {
+		r.ownerRefMode = ownerRefMode
+	}
 }
 
 func (r *renderOpts) Validate() error {
+	if !isValidOwnerRefMode(r.ownerRefMode) {
+		return fmt.Errorf("unsupported owner reference: %q", r.ownerRefMode)
+	}
 	if len(r.assetsOutDir) == 0 {
 		return fmt.Errorf("asset-output-dir must be specified")
 	}
-
 	return nil
 }
 
 func (r *renderOpts) Run() error {
-	return render(r.assetsInDir, r.assetsOutDir)
+	return render(r.ownerRefMode, r.assetsInDir, r.assetsOutDir)
 }
 
 func addKlogFlags(cmd *cobra.Command) {

pkg/performanceprofile/cmd/render/render.go

@@ -38,7 +38,6 @@ import (
 	"k8s.io/apimachinery/pkg/runtime/serializer"
 	"k8s.io/apimachinery/pkg/types"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
-	"k8s.io/apimachinery/pkg/util/uuid"
 
 	"k8s.io/klog"
 )
@@ -47,6 +46,16 @@ const (
 	clusterConfigResourceName = "cluster"
 )
 
+const (
+	ownerRefModeNone      = "none"
+	ownerRefModeK8S       = "k8s"
+	ownerRefModeLabelName = "label-name"
+)
+
+const (
+	weakOwnerReferenceNameLabel = "performance.openshift.io/weak-owner-reference-name"
+)
+
 var (
 	manifestScheme  = runtime.NewScheme()
 	codecFactory    serializer.CodecFactory
@@ -68,9 +77,8 @@ func init() {
 
 // Render will traverse the input directory and generate the proper performance profile files
 // in to the output dir based on PerformanceProfile manifests contained in the input directory.
-func render(inputDir, outputDir string) error {
-
-	klog.Info("Rendering files into: ", outputDir)
+func render(ownerRefMode, inputDir, outputDir string) error {
+	klog.Infof("Rendering files into: %s (ownerRefMode=%v)", outputDir, ownerRefMode)
 
 	// Read asset directory fileInfo
 	filePaths, err := listFiles(inputDir)
@@ -181,45 +189,82 @@ func render(inputDir, outputDir string) error {
 			return err
 		}
 
-		uid := pp.UID
-		if uid == types.UID("") {
-			uid = uuid.NewUUID()
-		}
-
-		or := []v1.OwnerReference{
-			{
-				Kind:       pp.Kind,
-				Name:       pp.Name,
-				APIVersion: pp.APIVersion,
-				UID:        uid,
-			},
-		}
-
-		for _, componentObj := range components.ToObjects() {
-			componentObj.SetOwnerReferences(or)
+		if ownerRefMode == ownerRefModeK8S {
+			err = addOwnerReference(components, pp)
+			if err != nil {
+				return err
+			}
+		} else if ownerRefMode == ownerRefModeLabelName {
+			err = addWeakOwnerReferenceLabel(components, pp)
+			if err != nil {
+				return err
+			}
 		}
 
 		for kind, manifest := range components.ToManifestTable() {
-			b, err := yaml.Marshal(manifest)
+			err = writeObject(outputDir, fmt.Sprintf("%s_%s.yaml", pp.Name, strings.ToLower(kind)), manifest)
 			if err != nil {
 				return err
 			}
+		}
+	}
 
-			fileName := fmt.Sprintf("%s_%s.yaml", pp.Name, strings.ToLower(kind))
-			fullFilePath := filepath.Join(outputDir, fileName)
-			klog.Info("Writing file: ", fullFilePath)
+	return nil
+}
 
-			err = os.WriteFile(fullFilePath, b, 0644)
-			if err != nil {
-				return err
-			}
-			klog.Info(fileName)
+func writeObject(outputDir, fileName string, manifest interface{}) error {
+	fullFilePath := filepath.Join(outputDir, fileName)
+	klog.Infof("Writing file: %s -> %s", fileName, fullFilePath)
+
+	b, err := yaml.Marshal(manifest)
+	if err != nil {
+		return err
+	}
+
+	return os.WriteFile(fullFilePath, b, 0644)
+}
+
+func addOwnerReference(components *manifestset.ManifestResultSet, pp *performancev2.PerformanceProfile) error {
+	if pp == nil || pp.UID == types.UID("") {
+		return fmt.Errorf("Missing UID from performance profile")
+	}
+
+	or := []v1.OwnerReference{
+		{
+			Kind:       pp.Kind,
+			Name:       pp.Name,
+			APIVersion: pp.APIVersion,
+			UID:        pp.UID,
+		},
+	}
+
+	for _, componentObj := range components.ToObjects() {
+		componentObj.SetOwnerReferences(or)
+	}
+
+	return nil
+}
+
+func addWeakOwnerReferenceLabel(components *manifestset.ManifestResultSet, pp *performancev2.PerformanceProfile) error {
+	lab := weakOwnerReferenceNameLabel // shortcut
+	val := pp.Name                     // shortcut
+
+	for _, componentObj := range components.ToObjects() {
+		labels := componentObj.GetLabels()
+		if labels == nil {
+			labels = make(map[string]string)
 		}
+		labels[lab] = val
+		componentObj.SetLabels(labels)
 	}
 
 	return nil
 }
 
+func isValidOwnerRefMode(val string) bool {
+	return val == ownerRefModeNone || val == ownerRefModeK8S || val == ownerRefModeLabelName
+}
+
 // 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

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

@@ -12,8 +12,10 @@ import (
 )
 
 const (
-	defaultExpectedDir = "default"
-	pinnedExpectedDir  = "pinned"
+	defaultExpectedDir   = "default"
+	pinnedExpectedDir    = "pinned"
+	bootstrapExpectedDir = "bootstrap"
+	noRefExpectedDir     = "no-ref"
 )
 
 var (
@@ -67,6 +69,39 @@ var _ = Describe("render command e2e test", func() {
 			)
 			runAndCompare(cmd, defaultExpectedDir)
 		})
+
+		It("Must fail to restore legacy and wrong legacy owner reference if uid is missing", func() {
+			cmdline := []string{
+				filepath.Join(binPath, "cluster-node-tuning-operator"),
+				"render",
+				"--asset-input-dir", strings.Join(assetsInDirs, ","),
+				"--asset-output-dir", assetsOutDir,
+				"--owner-ref", "k8s",
+			}
+			fmt.Fprintf(GinkgoWriter, "running: %v\n", cmdline)
+
+			cmd := exec.Command(cmdline[0], cmdline[1:]...)
+			_, err := cmd.Output()
+			Expect(err).To(HaveOccurred(), logStderr(err))
+		})
+
+		It("Must not set any owner reference if disabled explicitely", func() {
+			cmdline := []string{
+				filepath.Join(binPath, "cluster-node-tuning-operator"),
+				"render",
+				"--asset-input-dir", strings.Join(assetsInDirs, ","),
+				"--asset-output-dir", assetsOutDir,
+				"--owner-ref", "none",
+			}
+			fmt.Fprintf(GinkgoWriter, "running: %v\n", cmdline)
+
+			cmd := exec.Command(cmdline[0], cmdline[1:]...)
+			cmd.Env = append(cmd.Env,
+				fmt.Sprintf("ASSET_INPUT_DIR=%s", strings.Join(assetsInDirs, ",")),
+				fmt.Sprintf("ASSET_OUTPUT_DIR=%s", assetsOutDir),
+			)
+			runAndCompare(cmd, noRefExpectedDir)
+		})
 	})
 
 	Context("With pinned cluster resources", func() {
@@ -82,7 +117,6 @@ var _ = Describe("render command e2e test", func() {
 
 			cmd := exec.Command(cmdline[0], cmdline[1:]...)
 			runAndCompare(cmd, pinnedExpectedDir)
-
 		})
 
 		It("Given legacy SNO pinned infrastructure status, should render cpu partitioning configs", func() {
@@ -120,7 +154,7 @@ func cleanArtifacts() {
 
 func runAndCompare(cmd *exec.Cmd, dir string) {
 	_, err := cmd.Output()
-	Expect(err).ToNot(HaveOccurred())
+	Expect(err).ToNot(HaveOccurred(), logStderr(err))
 
 	outputAssetsFiles, err := os.ReadDir(assetsOutDir)
 	Expect(err).ToNot(HaveOccurred())
@@ -142,3 +176,10 @@ func runAndCompare(cmd *exec.Cmd, dir string) {
 			diff)
 	}
 }
+
+func logStderr(err error) string {
+	if exitErr, ok := err.(*exec.ExitError); ok {
+		return fmt.Sprintf("error running the command: [[%s]]", exitErr.Stderr)
+	}
+	return fmt.Sprintf("error running the command: [[%s]]", err)
+}

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

@@ -2,12 +2,9 @@ apiVersion: machineconfiguration.openshift.io/v1
 kind: KubeletConfig
 metadata:
   creationTimestamp: null
+  labels:
+    performance.openshift.io/weak-owner-reference-name: manual
   name: performance-manual
-  ownerReferences:
-  - apiVersion: performance.openshift.io/v2
-    kind: PerformanceProfile
-    name: manual
-    uid: ""
 spec:
   kubeletConfig:
     apiVersion: kubelet.config.k8s.io/v1beta1

test/e2e/performanceprofile/testdata/render-expected-output/default/manual_machineconfig.yaml

@@ -4,12 +4,8 @@ metadata:
   creationTimestamp: null
   labels:
     machineconfiguration.openshift.io/role: worker-cnf
+    performance.openshift.io/weak-owner-reference-name: manual
   name: 50-performance-manual
-  ownerReferences:
-  - apiVersion: performance.openshift.io/v2
-    kind: PerformanceProfile
-    name: manual
-    uid: ""
 spec:
   baseOSExtensionsContainerImage: ""
   config:

test/e2e/performanceprofile/testdata/render-expected-output/default/manual_node.yaml

@@ -2,12 +2,9 @@ apiVersion: config.openshift.io/v1
 kind: Node
 metadata:
   creationTimestamp: null
+  labels:
+    performance.openshift.io/weak-owner-reference-name: manual
   name: cluster
-  ownerReferences:
-  - apiVersion: performance.openshift.io/v2
-    kind: PerformanceProfile
-    name: manual
-    uid: ""
 spec:
   cgroupMode: v1
 status: {}

test/e2e/performanceprofile/testdata/render-expected-output/default/manual_runtimeclass.yaml

@@ -3,12 +3,9 @@ handler: high-performance
 kind: RuntimeClass
 metadata:
   creationTimestamp: null
+  labels:
+    performance.openshift.io/weak-owner-reference-name: manual
   name: performance-manual
-  ownerReferences:
-  - apiVersion: performance.openshift.io/v2
-    kind: PerformanceProfile
-    name: manual
-    uid: ""
 scheduling:
   nodeSelector:
     node-role.kubernetes.io/worker-cnf: ""