Merge pull request #4108 from Tal-or/pp_unique_name

CNF-12951:hypershift:performanceprofile: associate profile name with user input
openshift · Jul 9, 2024 · c7faae0 · c7faae0
2 parents 6577bad + dfff37a
commit c7faae0
Show file tree

Hide file tree

Showing 6 changed files with 71 additions and 33 deletions.
diff --git a/hypershift-operator/controllers/nodepool/manifests.go b/hypershift-operator/controllers/nodepool/manifests.go
@@ -4,6 +4,7 @@ import (
 	"fmt"
 
 	hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1"
+	"github.com/openshift/hypershift/support/util"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	capiv1 "sigs.k8s.io/cluster-api/api/v1beta1"
@@ -12,6 +13,10 @@ import (
 const (
 	EC2VolumeDefaultSize int64  = 16
 	EC2VolumeDefaultType string = "gp3"
+
+	// QualifiedNameMaxLength is the maximal name length allowed for k8s object
+	// https://github.com/kubernetes/kubernetes/blob/957c9538670b5f7ead2c9ba9ceb9de081d66caa4/staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go#L34
+	QualifiedNameMaxLength = 63
 )
 
 func machineDeployment(nodePool *hyperv1.NodePool, controlPlaneNamespace string) *capiv1.MachineDeployment {
@@ -72,11 +77,11 @@ func TunedConfigMap(namespace, name string) *corev1.ConfigMap {
 	}
 }
 
-func PerformanceProfileConfigMap(namespace, name string) *corev1.ConfigMap {
+func PerformanceProfileConfigMap(namespace, name, nodePoolName string) *corev1.ConfigMap {
 	return &corev1.ConfigMap{
 		ObjectMeta: metav1.ObjectMeta{
 			Namespace: namespace,
-			Name:      "perfprofile-" + name,
+			Name:      util.ShortenName(name, nodePoolName, QualifiedNameMaxLength),
 		},
 	}
 }
diff --git a/hypershift-operator/controllers/nodepool/nodepool_controller.go b/hypershift-operator/controllers/nodepool/nodepool_controller.go
@@ -45,6 +45,7 @@ import (
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	serializer "k8s.io/apimachinery/pkg/runtime/serializer/json"
@@ -682,7 +683,7 @@ func (r *NodePoolReconciler) reconcile(ctx context.Context, hcluster *hyperv1.Ho
 	}
 
 	// Validate tuningConfig input.
-	tunedConfig, performanceProfileConfig, err := r.getTuningConfig(ctx, nodePool)
+	tunedConfig, performanceProfileConfig, performanceProfileConfigMapName, err := r.getTuningConfig(ctx, nodePool)
 	if err != nil {
 		SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{
 			Type:               hyperv1.NodePoolValidTuningConfigConditionType,
@@ -735,12 +736,14 @@ func (r *NodePoolReconciler) reconcile(ctx context.Context, hcluster *hyperv1.Ho
 		}
 	}
 
-	performanceProfileConfigMap := PerformanceProfileConfigMap(controlPlaneNamespace, nodePool.Name)
 	if performanceProfileConfig == "" {
-		if _, err := supportutil.DeleteIfNeeded(ctx, r.Client, performanceProfileConfigMap); err != nil {
+		// at this point in time, we no longer know the name of the ConfigMap in the HCP NS
+		// so, we remove it by listing by a label unique to PerformanceProfile
+		if err := deleteConfigByLabel(ctx, r.Client, map[string]string{PerformanceProfileConfigMapLabel: "true"}); err != nil {
 			return ctrl.Result{}, fmt.Errorf("failed to delete performanceprofileConfig ConfigMap: %w", err)
 		}
 	} else {
+		performanceProfileConfigMap := PerformanceProfileConfigMap(controlPlaneNamespace, performanceProfileConfigMapName, nodePool.Name)
 		if result, err := r.CreateOrUpdate(ctx, r.Client, performanceProfileConfigMap, func() error {
 			return reconcilePerformanceProfileConfigMap(performanceProfileConfigMap, nodePool, performanceProfileConfig)
 		}); err != nil {
@@ -2169,10 +2172,11 @@ func (r *NodePoolReconciler) getConfig(ctx context.Context,
 
 func (r *NodePoolReconciler) getTuningConfig(ctx context.Context,
 	nodePool *hyperv1.NodePool,
-) (string, string, error) {
+) (string, string, string, error) {
 	var (
 		configs                              []corev1.ConfigMap
 		tunedAllConfigPlainText              []string
+		performanceProfileConfigMapName      string
 		performanceProfileAllConfigPlainText []string
 		errors                               []error
 	)
@@ -2206,6 +2210,7 @@ func (r *NodePoolReconciler) getTuningConfig(ctx context.Context,
 			tunedAllConfigPlainText = append(tunedAllConfigPlainText, string(manifestTuned))
 		}
 		if manifestPerformanceProfile != nil {
+			performanceProfileConfigMapName = config.Name
 			performanceProfileAllConfigPlainText = append(performanceProfileAllConfigPlainText, string(manifestPerformanceProfile))
 		}
 	}
@@ -2218,7 +2223,7 @@ func (r *NodePoolReconciler) getTuningConfig(ctx context.Context,
 	sort.Strings(tunedAllConfigPlainText)
 	sort.Strings(performanceProfileAllConfigPlainText)
 
-	return strings.Join(tunedAllConfigPlainText, "\n---\n"), strings.Join(performanceProfileAllConfigPlainText, "\n---\n"), utilerrors.NewAggregate(errors)
+	return strings.Join(tunedAllConfigPlainText, "\n---\n"), strings.Join(performanceProfileAllConfigPlainText, "\n---\n"), performanceProfileConfigMapName, utilerrors.NewAggregate(errors)
 
 }
 
@@ -3226,3 +3231,19 @@ func globalConfigString(hcluster *hyperv1.HostedCluster) (string, error) {
 func payloadConfigHash(config, targetVersion, pullSecretName, globalConfig string) string {
 	return supportutil.HashSimple(config + targetVersion + pullSecretName + globalConfig)
 }
+
+func deleteConfigByLabel(ctx context.Context, c client.Client, lbl map[string]string) error {
+	cmList := &corev1.ConfigMapList{}
+	if err := c.List(ctx, cmList, &client.ListOptions{
+		LabelSelector: labels.SelectorFromSet(lbl),
+	}); err != nil {
+		return err
+	}
+	for i := range cmList.Items {
+		cm := &cmList.Items[i]
+		if _, err := supportutil.DeleteIfNeeded(ctx, c, cm); err != nil {
+			return err
+		}
+	}
+	return nil
+}
diff --git a/hypershift-operator/controllers/nodepool/nodepool_controller_test.go b/hypershift-operator/controllers/nodepool/nodepool_controller_test.go
@@ -1047,12 +1047,13 @@ status: {}
 
 	namespace := "test"
 	testCases := []struct {
-		name           string
-		nodePool       *hyperv1.NodePool
-		tuningConfig   []client.Object
-		tunedExpect    string
-		perfprofExpect string
-		error          bool
+		name               string
+		nodePool           *hyperv1.NodePool
+		tuningConfig       []client.Object
+		tunedExpect        string
+		perfprofExpect     string
+		perfProfNameExpect string
+		error              bool
 	}{
 		{
 			name: "gets a single valid TunedConfig",
@@ -1175,9 +1176,10 @@ status: {}
 					BinaryData: nil,
 				},
 			},
-			tunedExpect:    "",
-			perfprofExpect: perfprofOneDefaulted,
-			error:          false,
+			tunedExpect:        "",
+			perfprofExpect:     perfprofOneDefaulted,
+			perfProfNameExpect: "perfprofOne",
+			error:              false,
 		},
 		{
 			name: "Should be at most one PerformanceProfileConfig per NodePool",
@@ -1242,7 +1244,7 @@ status: {}
 			error:          true,
 		},
 		{
-			name: "PerformanceProfiles and Tuned Configs could cohexists",
+			name: "PerformanceProfiles and Tuned Configs could coexists",
 			nodePool: &hyperv1.NodePool{
 				ObjectMeta: metav1.ObjectMeta{
 					Namespace: namespace,
@@ -1291,9 +1293,10 @@ status: {}
 					},
 				},
 			},
-			tunedExpect:    tuned1Defaulted + "\n---\n" + tuned2Defaulted,
-			perfprofExpect: perfprofOneDefaulted,
-			error:          false,
+			tunedExpect:        tuned1Defaulted + "\n---\n" + tuned2Defaulted,
+			perfprofExpect:     perfprofOneDefaulted,
+			perfProfNameExpect: "perfprofOne",
+			error:              false,
 		},
 	}
 
@@ -1305,7 +1308,7 @@ status: {}
 				Client: fake.NewClientBuilder().WithObjects(tc.tuningConfig...).Build(),
 			}
 
-			td, pp, err := r.getTuningConfig(context.Background(), tc.nodePool)
+			td, pp, ppName, err := r.getTuningConfig(context.Background(), tc.nodePool)
 
 			if tc.error {
 				g.Expect(err).To(HaveOccurred())
@@ -1320,6 +1323,10 @@ status: {}
 				t.Errorf("actual Performance Profile config differs from expected: %s", diff)
 				t.Logf("got:\n%s\n, expected:\n%s\n", pp, tc.perfprofExpect)
 			}
+			if diff := cmp.Diff(ppName, tc.perfProfNameExpect); diff != "" {
+				t.Errorf("Performance Profile config name differ from expected: %s", diff)
+				t.Logf("got:\n%s\n, expected:\n%s\n", ppName, tc.perfProfNameExpect)
+			}
 		})
 	}
 }

diff --git a/support/util/route.go b/support/util/route.go
@@ -26,17 +26,17 @@ func ShortenRouteHostnameIfNeeded(name, namespace string, baseDomain string) str
 	if len(name)+len(namespace)+1 < validation.DNS1123LabelMaxLength {
 		return ""
 	} else {
-		return fmt.Sprintf("%s.%s", strings.TrimSuffix(shortenName(name+"-"+namespace, "", validation.DNS1123LabelMaxLength), "-"), baseDomain)
+		return fmt.Sprintf("%s.%s", strings.TrimSuffix(ShortenName(name+"-"+namespace, "", validation.DNS1123LabelMaxLength), "-"), baseDomain)
 	}
 }
 
-// shortenName returns a name given a base ("deployment-5") and a suffix ("deploy")
+// ShortenName returns a name given a base ("deployment-5") and a suffix ("deploy")
 // It will first attempt to join them with a dash. If the resulting name is longer
 // than maxLength: if the suffix is too long, it will truncate the base name and add
 // an 8-character hash of the [base]-[suffix] string.  If the suffix is not too long,
 // it will truncate the base, add the hash of the base and return [base]-[hash]-[suffix]
 // Source: openshift/origin v3.9.0 pkg/api/apihelpers/namer.go
-func shortenName(base, suffix string, maxLength int) string {
+func ShortenName(base, suffix string, maxLength int) string {
 	if maxLength <= 0 {
 		return ""
 	}

diff --git a/support/util/route_test.go b/support/util/route_test.go
@@ -55,7 +55,7 @@ func TestShortenName(t *testing.T) {
 
 		for j, test := range tests {
 			t.Run(fmt.Sprintf("test-%d-%d", i, j), func(t *testing.T) {
-				result := shortenName(test.base, test.suffix, kvalidation.DNS1123SubdomainMaxLength)
+				result := ShortenName(test.base, test.suffix, kvalidation.DNS1123SubdomainMaxLength)
 				if result != test.expected {
 					t.Errorf("Got unexpected result. Expected: %s Got: %s", test.expected, result)
 				}
@@ -66,14 +66,14 @@ func TestShortenName(t *testing.T) {
 
 func TestShortenNameIsDifferent(t *testing.T) {
 	shortName := randSeq(32)
-	deployerName := shortenName(shortName, "deploy", kvalidation.DNS1123SubdomainMaxLength)
-	builderName := shortenName(shortName, "build", kvalidation.DNS1123SubdomainMaxLength)
+	deployerName := ShortenName(shortName, "deploy", kvalidation.DNS1123SubdomainMaxLength)
+	builderName := ShortenName(shortName, "build", kvalidation.DNS1123SubdomainMaxLength)
 	if deployerName == builderName {
 		t.Errorf("Expecting names to be different: %s\n", deployerName)
 	}
 	longName := randSeq(kvalidation.DNS1123SubdomainMaxLength + 10)
-	deployerName = shortenName(longName, "deploy", kvalidation.DNS1123SubdomainMaxLength)
-	builderName = shortenName(longName, "build", kvalidation.DNS1123SubdomainMaxLength)
+	deployerName = ShortenName(longName, "deploy", kvalidation.DNS1123SubdomainMaxLength)
+	builderName = ShortenName(longName, "build", kvalidation.DNS1123SubdomainMaxLength)
 	if deployerName == builderName {
 		t.Errorf("Expecting names to be different: %s\n", deployerName)
 	}
@@ -84,7 +84,7 @@ func TestShortenNameReturnShortNames(t *testing.T) {
 	for maxLength := 0; maxLength < len(base)+2; maxLength++ {
 		for suffixLen := 0; suffixLen <= maxLength+1; suffixLen++ {
 			suffix := randSeq(suffixLen)
-			got := shortenName(base, suffix, maxLength)
+			got := ShortenName(base, suffix, maxLength)
 			if len(got) > maxLength {
 				t.Fatalf("len(GetName(%[1]q, %[2]q, %[3]d)) = len(%[4]q) = %[5]d; want %[3]d", base, suffix, maxLength, got, len(got))
 			}

diff --git a/test/e2e/nodepool_nto_performanceprofile_test.go b/test/e2e/nodepool_nto_performanceprofile_test.go
@@ -6,6 +6,7 @@ package e2e
 import (
 	"context"
 	"fmt"
+	"github.com/openshift/hypershift/support/util"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -14,7 +15,6 @@ import (
 
 	hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1"
 	"github.com/openshift/hypershift/hypershift-operator/controllers/manifests"
-
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -37,7 +37,6 @@ spec:
     node-role.kubernetes.io/worker-cnf: ""
 `
 	controllerGeneratedPPConfig = "hypershift.openshift.io/performanceprofile-config"
-	ppConfigMapNamePrefix       = "perfprofile-"
 )
 
 type NTOPerformanceProfileTest struct {
@@ -107,7 +106,7 @@ func (mc *NTOPerformanceProfileTest) Run(t *testing.T, nodePool hyperv1.NodePool
 	controlPlaneNamespace := manifests.HostedControlPlaneNamespace(mc.hostedCluster.Namespace, mc.hostedCluster.Name)
 	t.Logf("Hosted control plane namespace is %s", controlPlaneNamespace)
 
-	e2eutil.EventuallyObjects(t, ctx, "performance profile ConfigMap to exist with correct labels and annotations",
+	e2eutil.EventuallyObjects(t, ctx, "performance profile ConfigMap to exist with correct name labels and annotations",
 		func(ctx context.Context) ([]*corev1.ConfigMap, error) {
 			list := &corev1.ConfigMapList{}
 			err := mc.managementClient.List(ctx, list, crclient.InNamespace(controlPlaneNamespace), crclient.MatchingLabels(map[string]string{
@@ -126,6 +125,12 @@ func (mc *NTOPerformanceProfileTest) Run(t *testing.T, nodePool hyperv1.NodePool
 			},
 		},
 		[]e2eutil.Predicate[*corev1.ConfigMap]{
+			func(configMap *corev1.ConfigMap) (done bool, reasons string, err error) {
+				if want, got := util.ShortenName(performanceProfileConfigMap.Name, nodePool.Name, nodepool.QualifiedNameMaxLength), configMap.Name; want != got {
+					return false, fmt.Sprintf("expected performance profile ConfigMap name to be '%s', got '%s'", want, got), nil
+				}
+				return true, fmt.Sprintf("performance profile ConfigMap name is as expected"), nil
+			},
 			func(configMap *corev1.ConfigMap) (done bool, reasons string, err error) {
 				if diff := cmp.Diff(map[string]string{
 					nodepool.PerformanceProfileConfigMapLabel: configMap.Labels[nodepool.PerformanceProfileConfigMapLabel],