Merge pull request #439 from chiragkyal/perf-config

CM-1033: Add performance tuning parameters to the supported args whitelist

Author: openshift-merge-bot[bot]

pkg/controller/certmanager/deployment_overrides_validation.go

@@ -2,6 +2,7 @@ package certmanager
 
 import (
 	"fmt"
+	"strconv"
 	"unsafe"
 
 	appsv1 "k8s.io/api/apps/v1"
@@ -20,6 +21,13 @@ import (
 	certmanagerinformer "github.com/openshift/cert-manager-operator/pkg/operator/informers/externalversions/operator/v1alpha1"
 )
 
+const (
+	argConcurrentWorkers       = "--concurrent-workers"
+	argKubeAPIQPS              = "--kube-api-qps"
+	argKubeAPIBurst            = "--kube-api-burst"
+	argMaxConcurrentChallenges = "--max-concurrent-challenges"
+)
+
 // withContainerArgsValidateHook validates the container args with those that
 // are supported by the operator.
 func withContainerArgsValidateHook(certmanagerinformer certmanagerinformer.CertManagerInformer, deploymentName string) func(operatorSpec *operatorv1.OperatorSpec, deployment *appsv1.Deployment) error {
@@ -65,6 +73,14 @@ func withContainerArgsValidateHook(certmanagerinformer certmanagerinformer.CertM
 		// Duration of the initial certificate request backoff when a certificate request fails. The backoff
 		// duration is exponentially increased based on consecutive failures, up to a maximum of 32 hours.
 		"--certificate-request-minimum-backoff-duration",
+		// The number of concurrent workers for each controller. (default 5)
+		argConcurrentWorkers,
+		// Maximum queries-per-second to the Kubernetes API server. (default 20)
+		argKubeAPIQPS,
+		// Maximum burst queries-per-second to the Kubernetes API server. Must be >= kube-api-qps. (default 50)
+		argKubeAPIBurst,
+		// Maximum number of challenges that can be scheduled as 'processing' at once. (default 60)
+		argMaxConcurrentChallenges,
 	}
 	supportedCertManagerWebhookArgs := []string{
 		// Log Level
@@ -95,7 +111,10 @@ func withContainerArgsValidateHook(certmanagerinformer certmanagerinformer.CertM
 		case certmanagerControllerDeployment:
 			if certmanager.Spec.ControllerConfig != nil {
 				common.ParseArgMap(argMap, certmanager.Spec.ControllerConfig.OverrideArgs)
-				return validateArgs(argMap, supportedCertManagerArgs)
+				if err := validateArgs(argMap, supportedCertManagerArgs); err != nil {
+					return err
+				}
+				return validatePerformanceArgs(argMap)
 			}
 		case certmanagerWebhookDeployment:
 			if certmanager.Spec.WebhookConfig != nil {
@@ -115,6 +134,49 @@ func withContainerArgsValidateHook(certmanagerinformer certmanagerinformer.CertM
 	}
 }
 
+// validatePerformanceArgs performs sanity checks on the performance tuning
+// arguments to catch invalid configurations.
+func validatePerformanceArgs(argMap map[string]string) error {
+	// Validate that integer args are positive.
+	positiveIntArgs := []string{argConcurrentWorkers, argMaxConcurrentChallenges, argKubeAPIBurst}
+	parsedInts := make(map[string]int)
+	for _, arg := range positiveIntArgs {
+		if valStr, ok := argMap[arg]; ok {
+			val, err := strconv.Atoi(valStr)
+			if err != nil {
+				return fmt.Errorf("validation failed: %s value must be a positive integer, got %q", arg, valStr)
+			}
+			if val <= 0 {
+				return fmt.Errorf("validation failed: %s must be greater than 0, got %d", arg, val)
+			}
+			parsedInts[arg] = val
+		}
+	}
+
+	// Validate QPS is a positive float32
+	var qps float32
+	var qpsOk bool
+	if qpsStr, ok := argMap[argKubeAPIQPS]; ok {
+		val, err := strconv.ParseFloat(qpsStr, 32)
+		if err != nil {
+			return fmt.Errorf("validation failed: %s value must be numeric, got %q", argKubeAPIQPS, qpsStr)
+		}
+		if val <= 0 {
+			return fmt.Errorf("validation failed: %s must be greater than 0, got %v", argKubeAPIQPS, val)
+		}
+		qps = float32(val)
+		qpsOk = true
+	}
+
+	// Validate burst >= qps when both are specified.
+	burst, burstOk := parsedInts[argKubeAPIBurst]
+	if qpsOk && burstOk && float32(burst) < qps {
+		return fmt.Errorf("validation failed: --kube-api-burst (%d) must be >= --kube-api-qps (%v)", burst, qps)
+	}
+
+	return nil
+}
+
 // withContainerEnvValidateHook validates the container env with those that
 // are supported by the operator.
 func withContainerEnvValidateHook(certmanagerinformer certmanagerinformer.CertManagerInformer, deploymentName string) func(operatorSpec *operatorv1.OperatorSpec, deployment *appsv1.Deployment) error {

pkg/controller/certmanager/deployment_overrides_validation_test.go

@@ -311,6 +311,183 @@ func TestWithContainerArgsValidateHook(t *testing.T) {
 			deploymentName: certmanagerControllerDeployment,
 			wantErrMsg:     `validation failed due to unsupported arg "--totally-unknown-flag"="value"`,
 		},
+		{
+			name: "controller accepts performance tuning flags",
+			certManagerObj: v1alpha1.CertManager{
+				ObjectMeta: metav1.ObjectMeta{Name: "cluster"},
+				Spec: v1alpha1.CertManagerSpec{
+					ControllerConfig: &v1alpha1.DeploymentConfig{
+						OverrideArgs: []string{
+							"--concurrent-workers=20",
+							"--kube-api-qps=150",
+							"--kube-api-burst=300",
+							"--max-concurrent-challenges=300",
+						},
+					},
+				},
+			},
+			deploymentName: certmanagerControllerDeployment,
+		},
+		{
+			name: "controller accepts burst equal to qps",
+			certManagerObj: v1alpha1.CertManager{
+				ObjectMeta: metav1.ObjectMeta{Name: "cluster"},
+				Spec: v1alpha1.CertManagerSpec{
+					ControllerConfig: &v1alpha1.DeploymentConfig{
+						OverrideArgs: []string{
+							"--kube-api-qps=100",
+							"--kube-api-burst=100",
+						},
+					},
+				},
+			},
+			deploymentName: certmanagerControllerDeployment,
+		},
+		{
+			name: "controller accepts only kube-api-qps without burst",
+			certManagerObj: v1alpha1.CertManager{
+				ObjectMeta: metav1.ObjectMeta{Name: "cluster"},
+				Spec: v1alpha1.CertManagerSpec{
+					ControllerConfig: &v1alpha1.DeploymentConfig{
+						OverrideArgs: []string{"--kube-api-qps=150"},
+					},
+				},
+			},
+			deploymentName: certmanagerControllerDeployment,
+		},
+		{
+			name: "controller accepts only kube-api-burst without qps",
+			certManagerObj: v1alpha1.CertManager{
+				ObjectMeta: metav1.ObjectMeta{Name: "cluster"},
+				Spec: v1alpha1.CertManagerSpec{
+					ControllerConfig: &v1alpha1.DeploymentConfig{
+						OverrideArgs: []string{"--kube-api-burst=200"},
+					},
+				},
+			},
+			deploymentName: certmanagerControllerDeployment,
+		},
+		{
+			name: "controller accepts decimal kube-api-qps value",
+			certManagerObj: v1alpha1.CertManager{
+				ObjectMeta: metav1.ObjectMeta{Name: "cluster"},
+				Spec: v1alpha1.CertManagerSpec{
+					ControllerConfig: &v1alpha1.DeploymentConfig{
+						OverrideArgs: []string{
+							"--kube-api-qps=150.5",
+							"--kube-api-burst=200",
+						},
+					},
+				},
+			},
+			deploymentName: certmanagerControllerDeployment,
+		},
+		{
+			name: "controller rejects burst less than qps",
+			certManagerObj: v1alpha1.CertManager{
+				ObjectMeta: metav1.ObjectMeta{Name: "cluster"},
+				Spec: v1alpha1.CertManagerSpec{
+					ControllerConfig: &v1alpha1.DeploymentConfig{
+						OverrideArgs: []string{
+							"--kube-api-qps=100",
+							"--kube-api-burst=50",
+						},
+					},
+				},
+			},
+			deploymentName: certmanagerControllerDeployment,
+			wantErrMsg:     `validation failed: --kube-api-burst (50) must be >= --kube-api-qps (100)`,
+		},
+		{
+			name: "controller rejects non-numeric kube-api-qps",
+			certManagerObj: v1alpha1.CertManager{
+				ObjectMeta: metav1.ObjectMeta{Name: "cluster"},
+				Spec: v1alpha1.CertManagerSpec{
+					ControllerConfig: &v1alpha1.DeploymentConfig{
+						OverrideArgs: []string{
+							"--kube-api-qps=abc",
+							"--kube-api-burst=100",
+						},
+					},
+				},
+			},
+			deploymentName: certmanagerControllerDeployment,
+			wantErrMsg:     `validation failed: --kube-api-qps value must be numeric, got "abc"`,
+		},
+		{
+			name: "controller rejects non-numeric kube-api-burst",
+			certManagerObj: v1alpha1.CertManager{
+				ObjectMeta: metav1.ObjectMeta{Name: "cluster"},
+				Spec: v1alpha1.CertManagerSpec{
+					ControllerConfig: &v1alpha1.DeploymentConfig{
+						OverrideArgs: []string{
+							"--kube-api-qps=100",
+							"--kube-api-burst=xyz",
+						},
+					},
+				},
+			},
+			deploymentName: certmanagerControllerDeployment,
+			wantErrMsg:     `validation failed: --kube-api-burst value must be a positive integer, got "xyz"`,
+		},
+		{
+			name: "controller rejects zero concurrent-workers",
+			certManagerObj: v1alpha1.CertManager{
+				ObjectMeta: metav1.ObjectMeta{Name: "cluster"},
+				Spec: v1alpha1.CertManagerSpec{
+					ControllerConfig: &v1alpha1.DeploymentConfig{
+						OverrideArgs: []string{"--concurrent-workers=0"},
+					},
+				},
+			},
+			deploymentName: certmanagerControllerDeployment,
+			wantErrMsg:     `validation failed: --concurrent-workers must be greater than 0, got 0`,
+		},
+		{
+			name: "controller rejects negative max-concurrent-challenges",
+			certManagerObj: v1alpha1.CertManager{
+				ObjectMeta: metav1.ObjectMeta{Name: "cluster"},
+				Spec: v1alpha1.CertManagerSpec{
+					ControllerConfig: &v1alpha1.DeploymentConfig{
+						OverrideArgs: []string{"--max-concurrent-challenges=-5"},
+					},
+				},
+			},
+			deploymentName: certmanagerControllerDeployment,
+			wantErrMsg:     `validation failed: --max-concurrent-challenges must be greater than 0, got -5`,
+		},
+		{
+			name: "controller rejects zero kube-api-qps",
+			certManagerObj: v1alpha1.CertManager{
+				ObjectMeta: metav1.ObjectMeta{Name: "cluster"},
+				Spec: v1alpha1.CertManagerSpec{
+					ControllerConfig: &v1alpha1.DeploymentConfig{
+						OverrideArgs: []string{
+							"--kube-api-qps=0",
+							"--kube-api-burst=50",
+						},
+					},
+				},
+			},
+			deploymentName: certmanagerControllerDeployment,
+			wantErrMsg:     `validation failed: --kube-api-qps must be greater than 0, got 0`,
+		},
+		{
+			name: "controller rejects negative kube-api-burst",
+			certManagerObj: v1alpha1.CertManager{
+				ObjectMeta: metav1.ObjectMeta{Name: "cluster"},
+				Spec: v1alpha1.CertManagerSpec{
+					ControllerConfig: &v1alpha1.DeploymentConfig{
+						OverrideArgs: []string{
+							"--kube-api-qps=20",
+							"--kube-api-burst=-1",
+						},
+					},
+				},
+			},
+			deploymentName: certmanagerControllerDeployment,
+			wantErrMsg:     `validation failed: --kube-api-burst must be greater than 0, got -1`,
+		},
 		{
 			name: "controller validates only controllerConfig webhook override args ignored",
 			certManagerObj: v1alpha1.CertManager{

test/e2e/overrides_test.go

@@ -68,6 +68,11 @@ var _ = Describe("Overrides test", Ordered, Label("Platform:Generic"), func() {
 				"--v=5",
 
 				"--metrics-listen-address=0.0.0.0:9401",
+
+				"--concurrent-workers=20",
+				"--kube-api-qps=150",
+				"--kube-api-burst=300",
+				"--max-concurrent-challenges=300",
 			}
 			err := addOverrideArgs(certmanageroperatorclient, certmanagerControllerDeployment, args)
 			Expect(err).NotTo(HaveOccurred())
@@ -148,6 +153,48 @@ var _ = Describe("Overrides test", Ordered, Label("Platform:Generic"), func() {
 		})
 	})
 
+	Context("When adding valid performance tuning args with kube-api-burst >= kube-api-qps", func() {
+
+		It("should add the performance args to the cert-manager controller deployment", func() {
+
+			By("Adding valid performance tuning args to the cert-manager operator object")
+			args := []string{"--concurrent-workers=10", "--kube-api-qps=50", "--kube-api-burst=100", "--max-concurrent-challenges=120"}
+			err := addOverrideArgs(certmanageroperatorclient, certmanagerControllerDeployment, args)
+			Expect(err).NotTo(HaveOccurred())
+
+			By("Waiting for cert-manager controller status to become available")
+			err = verifyOperatorStatusCondition(certmanageroperatorclient.OperatorV1alpha1(), GenerateConditionMatchers(
+				[]PrefixAndMatchTypeTuple{{certManagerController, MatchAllConditions}}, validOperatorStatusConditions,
+			))
+			Expect(err).NotTo(HaveOccurred())
+
+			By("Waiting for the args to be added to the cert-manager controller deployment")
+			err = verifyDeploymentArgs(k8sClientSet, certmanagerControllerDeployment, args, true)
+			Expect(err).NotTo(HaveOccurred())
+		})
+	})
+
+	Context("When adding kube-api-burst less than kube-api-qps", func() {
+
+		It("should reject the config and degrade the controller", func() {
+
+			By("Adding burst < qps args to the cert-manager operator object")
+			args := []string{"--kube-api-qps=100", "--kube-api-burst=50"}
+			err := addOverrideArgs(certmanageroperatorclient, certmanagerControllerDeployment, args)
+			Expect(err).NotTo(HaveOccurred())
+
+			By("Waiting for cert-manager controller status to become degraded")
+			err = verifyOperatorStatusCondition(certmanageroperatorclient.OperatorV1alpha1(), GenerateConditionMatchers(
+				[]PrefixAndMatchTypeTuple{{certManagerController, MatchAnyCondition}}, invalidOperatorStatusConditions,
+			))
+			Expect(err).NotTo(HaveOccurred())
+
+			By("Checking if the args are not added to the cert-manager controller deployment")
+			err = verifyDeploymentArgs(k8sClientSet, certmanagerControllerDeployment, args, false)
+			Expect(err).NotTo(HaveOccurred())
+		})
+	})
+
 	Context("When adding invalid cert-manager webhook override args", func() {
 
 		It("should not add the args to the cert-manager webhook deployment", func() {