Skip to content

Commit 7dd1849

Browse files
chiragkyalchiragkyal
committed
Add performance tuning parameters to the supported args whitelist
Add --concurrent-workers, --kube-api-qps, --kube-api-burst, and --max-concurrent-challenges to the controller's supported overrideArgs, allowing to tune cert-manager throughput via the CertManager CR. Signed-off-by: chiragkyal <ckyal@redhat.com>
1 parent 514b381 commit 7dd1849

3 files changed

Lines changed: 287 additions & 1 deletion

File tree

‎pkg/controller/certmanager/deployment_overrides_validation.go‎

Lines changed: 63 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package certmanager
22

33
import (
44
"fmt"
5+
"strconv"
56
"unsafe"
67

78
appsv1 "k8s.io/api/apps/v1"
@@ -20,6 +21,13 @@ import (
2021
certmanagerinformer "github.com/openshift/cert-manager-operator/pkg/operator/informers/externalversions/operator/v1alpha1"
2122
)
2223

24+
const (
25+
argConcurrentWorkers = "--concurrent-workers"
26+
argKubeAPIQPS = "--kube-api-qps"
27+
argKubeAPIBurst = "--kube-api-burst"
28+
argMaxConcurrentChallenges = "--max-concurrent-challenges"
29+
)
30+
2331
// withContainerArgsValidateHook validates the container args with those that
2432
// are supported by the operator.
2533
func withContainerArgsValidateHook(certmanagerinformer certmanagerinformer.CertManagerInformer, deploymentName string) func(operatorSpec *operatorv1.OperatorSpec, deployment *appsv1.Deployment) error {
@@ -65,6 +73,14 @@ func withContainerArgsValidateHook(certmanagerinformer certmanagerinformer.CertM
6573
// Duration of the initial certificate request backoff when a certificate request fails. The backoff
6674
// duration is exponentially increased based on consecutive failures, up to a maximum of 32 hours.
6775
"--certificate-request-minimum-backoff-duration",
76+
// The number of concurrent workers for each controller. (default 5)
77+
argConcurrentWorkers,
78+
// Maximum queries-per-second to the Kubernetes API server. (default 20)
79+
argKubeAPIQPS,
80+
// Maximum burst queries-per-second to the Kubernetes API server. Must be >= kube-api-qps. (default 50)
81+
argKubeAPIBurst,
82+
// Maximum number of challenges that can be scheduled as 'processing' at once. (default 60)
83+
argMaxConcurrentChallenges,
6884
}
6985
supportedCertManagerWebhookArgs := []string{
7086
// Log Level
@@ -95,7 +111,10 @@ func withContainerArgsValidateHook(certmanagerinformer certmanagerinformer.CertM
95111
case certmanagerControllerDeployment:
96112
if certmanager.Spec.ControllerConfig != nil {
97113
common.ParseArgMap(argMap, certmanager.Spec.ControllerConfig.OverrideArgs)
98-
return validateArgs(argMap, supportedCertManagerArgs)
114+
if err := validateArgs(argMap, supportedCertManagerArgs); err != nil {
115+
return err
116+
}
117+
return validatePerformanceArgs(argMap)
99118
}
100119
case certmanagerWebhookDeployment:
101120
if certmanager.Spec.WebhookConfig != nil {
@@ -115,6 +134,49 @@ func withContainerArgsValidateHook(certmanagerinformer certmanagerinformer.CertM
115134
}
116135
}
117136

137+
// validatePerformanceArgs performs sanity checks on the performance tuning
138+
// arguments to catch invalid configurations.
139+
func validatePerformanceArgs(argMap map[string]string) error {
140+
// Validate that numeric args are positive integers where applicable.
141+
positiveIntArgs := []string{argConcurrentWorkers, argMaxConcurrentChallenges}
142+
for _, arg := range positiveIntArgs {
143+
if valStr, ok := argMap[arg]; ok {
144+
val, err := strconv.Atoi(valStr)
145+
if err != nil {
146+
return fmt.Errorf("validation failed: %s value must be a positive integer, got %q", arg, valStr)
147+
}
148+
if val <= 0 {
149+
return fmt.Errorf("validation failed: %s must be greater than 0, got %d", arg, val)
150+
}
151+
}
152+
}
153+
154+
// Validate QPS and burst are numeric and positive.
155+
positiveFloatArgs := []string{argKubeAPIQPS, argKubeAPIBurst}
156+
parsedFloats := make(map[string]float64)
157+
for _, arg := range positiveFloatArgs {
158+
if valStr, ok := argMap[arg]; ok {
159+
val, err := strconv.ParseFloat(valStr, 64)
160+
if err != nil {
161+
return fmt.Errorf("validation failed: %s value must be numeric, got %q", arg, valStr)
162+
}
163+
if val <= 0 {
164+
return fmt.Errorf("validation failed: %s must be greater than 0, got %v", arg, val)
165+
}
166+
parsedFloats[arg] = val
167+
}
168+
}
169+
170+
// Validate burst >= qps when both are specified.
171+
qps, qpsOk := parsedFloats[argKubeAPIQPS]
172+
burst, burstOk := parsedFloats[argKubeAPIBurst]
173+
if qpsOk && burstOk && burst < qps {
174+
return fmt.Errorf("validation failed: --kube-api-burst (%v) must be >= --kube-api-qps (%v)", burst, qps)
175+
}
176+
177+
return nil
178+
}
179+
118180
// withContainerEnvValidateHook validates the container env with those that
119181
// are supported by the operator.
120182
func withContainerEnvValidateHook(certmanagerinformer certmanagerinformer.CertManagerInformer, deploymentName string) func(operatorSpec *operatorv1.OperatorSpec, deployment *appsv1.Deployment) error {

‎pkg/controller/certmanager/deployment_overrides_validation_test.go‎

Lines changed: 177 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -311,6 +311,183 @@ func TestWithContainerArgsValidateHook(t *testing.T) {
311311
deploymentName: certmanagerControllerDeployment,
312312
wantErrMsg: `validation failed due to unsupported arg "--totally-unknown-flag"="value"`,
313313
},
314+
{
315+
name: "controller accepts performance tuning flags",
316+
certManagerObj: v1alpha1.CertManager{
317+
ObjectMeta: metav1.ObjectMeta{Name: "cluster"},
318+
Spec: v1alpha1.CertManagerSpec{
319+
ControllerConfig: &v1alpha1.DeploymentConfig{
320+
OverrideArgs: []string{
321+
"--concurrent-workers=20",
322+
"--kube-api-qps=150",
323+
"--kube-api-burst=300",
324+
"--max-concurrent-challenges=300",
325+
},
326+
},
327+
},
328+
},
329+
deploymentName: certmanagerControllerDeployment,
330+
},
331+
{
332+
name: "controller accepts burst equal to qps",
333+
certManagerObj: v1alpha1.CertManager{
334+
ObjectMeta: metav1.ObjectMeta{Name: "cluster"},
335+
Spec: v1alpha1.CertManagerSpec{
336+
ControllerConfig: &v1alpha1.DeploymentConfig{
337+
OverrideArgs: []string{
338+
"--kube-api-qps=100",
339+
"--kube-api-burst=100",
340+
},
341+
},
342+
},
343+
},
344+
deploymentName: certmanagerControllerDeployment,
345+
},
346+
{
347+
name: "controller accepts only kube-api-qps without burst",
348+
certManagerObj: v1alpha1.CertManager{
349+
ObjectMeta: metav1.ObjectMeta{Name: "cluster"},
350+
Spec: v1alpha1.CertManagerSpec{
351+
ControllerConfig: &v1alpha1.DeploymentConfig{
352+
OverrideArgs: []string{"--kube-api-qps=150"},
353+
},
354+
},
355+
},
356+
deploymentName: certmanagerControllerDeployment,
357+
},
358+
{
359+
name: "controller accepts only kube-api-burst without qps",
360+
certManagerObj: v1alpha1.CertManager{
361+
ObjectMeta: metav1.ObjectMeta{Name: "cluster"},
362+
Spec: v1alpha1.CertManagerSpec{
363+
ControllerConfig: &v1alpha1.DeploymentConfig{
364+
OverrideArgs: []string{"--kube-api-burst=200"},
365+
},
366+
},
367+
},
368+
deploymentName: certmanagerControllerDeployment,
369+
},
370+
{
371+
name: "controller accepts decimal kube-api-qps value",
372+
certManagerObj: v1alpha1.CertManager{
373+
ObjectMeta: metav1.ObjectMeta{Name: "cluster"},
374+
Spec: v1alpha1.CertManagerSpec{
375+
ControllerConfig: &v1alpha1.DeploymentConfig{
376+
OverrideArgs: []string{
377+
"--kube-api-qps=150.5",
378+
"--kube-api-burst=200",
379+
},
380+
},
381+
},
382+
},
383+
deploymentName: certmanagerControllerDeployment,
384+
},
385+
{
386+
name: "controller rejects burst less than qps",
387+
certManagerObj: v1alpha1.CertManager{
388+
ObjectMeta: metav1.ObjectMeta{Name: "cluster"},
389+
Spec: v1alpha1.CertManagerSpec{
390+
ControllerConfig: &v1alpha1.DeploymentConfig{
391+
OverrideArgs: []string{
392+
"--kube-api-qps=100",
393+
"--kube-api-burst=50",
394+
},
395+
},
396+
},
397+
},
398+
deploymentName: certmanagerControllerDeployment,
399+
wantErrMsg: `validation failed: --kube-api-burst (50) must be >= --kube-api-qps (100)`,
400+
},
401+
{
402+
name: "controller rejects non-numeric kube-api-qps",
403+
certManagerObj: v1alpha1.CertManager{
404+
ObjectMeta: metav1.ObjectMeta{Name: "cluster"},
405+
Spec: v1alpha1.CertManagerSpec{
406+
ControllerConfig: &v1alpha1.DeploymentConfig{
407+
OverrideArgs: []string{
408+
"--kube-api-qps=abc",
409+
"--kube-api-burst=100",
410+
},
411+
},
412+
},
413+
},
414+
deploymentName: certmanagerControllerDeployment,
415+
wantErrMsg: `validation failed: --kube-api-qps value must be numeric, got "abc"`,
416+
},
417+
{
418+
name: "controller rejects non-numeric kube-api-burst",
419+
certManagerObj: v1alpha1.CertManager{
420+
ObjectMeta: metav1.ObjectMeta{Name: "cluster"},
421+
Spec: v1alpha1.CertManagerSpec{
422+
ControllerConfig: &v1alpha1.DeploymentConfig{
423+
OverrideArgs: []string{
424+
"--kube-api-qps=100",
425+
"--kube-api-burst=xyz",
426+
},
427+
},
428+
},
429+
},
430+
deploymentName: certmanagerControllerDeployment,
431+
wantErrMsg: `validation failed: --kube-api-burst value must be numeric, got "xyz"`,
432+
},
433+
{
434+
name: "controller rejects zero concurrent-workers",
435+
certManagerObj: v1alpha1.CertManager{
436+
ObjectMeta: metav1.ObjectMeta{Name: "cluster"},
437+
Spec: v1alpha1.CertManagerSpec{
438+
ControllerConfig: &v1alpha1.DeploymentConfig{
439+
OverrideArgs: []string{"--concurrent-workers=0"},
440+
},
441+
},
442+
},
443+
deploymentName: certmanagerControllerDeployment,
444+
wantErrMsg: `validation failed: --concurrent-workers must be greater than 0, got 0`,
445+
},
446+
{
447+
name: "controller rejects negative max-concurrent-challenges",
448+
certManagerObj: v1alpha1.CertManager{
449+
ObjectMeta: metav1.ObjectMeta{Name: "cluster"},
450+
Spec: v1alpha1.CertManagerSpec{
451+
ControllerConfig: &v1alpha1.DeploymentConfig{
452+
OverrideArgs: []string{"--max-concurrent-challenges=-5"},
453+
},
454+
},
455+
},
456+
deploymentName: certmanagerControllerDeployment,
457+
wantErrMsg: `validation failed: --max-concurrent-challenges must be greater than 0, got -5`,
458+
},
459+
{
460+
name: "controller rejects zero kube-api-qps",
461+
certManagerObj: v1alpha1.CertManager{
462+
ObjectMeta: metav1.ObjectMeta{Name: "cluster"},
463+
Spec: v1alpha1.CertManagerSpec{
464+
ControllerConfig: &v1alpha1.DeploymentConfig{
465+
OverrideArgs: []string{
466+
"--kube-api-qps=0",
467+
"--kube-api-burst=50",
468+
},
469+
},
470+
},
471+
},
472+
deploymentName: certmanagerControllerDeployment,
473+
wantErrMsg: `validation failed: --kube-api-qps must be greater than 0, got 0`,
474+
},
475+
{
476+
name: "controller rejects negative kube-api-burst",
477+
certManagerObj: v1alpha1.CertManager{
478+
ObjectMeta: metav1.ObjectMeta{Name: "cluster"},
479+
Spec: v1alpha1.CertManagerSpec{
480+
ControllerConfig: &v1alpha1.DeploymentConfig{
481+
OverrideArgs: []string{
482+
"--kube-api-qps=20",
483+
"--kube-api-burst=-1",
484+
},
485+
},
486+
},
487+
},
488+
deploymentName: certmanagerControllerDeployment,
489+
wantErrMsg: `validation failed: --kube-api-burst must be greater than 0, got -1`,
490+
},
314491
{
315492
name: "controller validates only controllerConfig webhook override args ignored",
316493
certManagerObj: v1alpha1.CertManager{

‎test/e2e/overrides_test.go‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,11 @@ var _ = Describe("Overrides test", Ordered, Label("Platform:Generic"), func() {
6868
"--v=5",
6969

7070
"--metrics-listen-address=0.0.0.0:9401",
71+
72+
"--concurrent-workers=20",
73+
"--kube-api-qps=150",
74+
"--kube-api-burst=300",
75+
"--max-concurrent-challenges=300",
7176
}
7277
err := addOverrideArgs(certmanageroperatorclient, certmanagerControllerDeployment, args)
7378
Expect(err).NotTo(HaveOccurred())
@@ -148,6 +153,48 @@ var _ = Describe("Overrides test", Ordered, Label("Platform:Generic"), func() {
148153
})
149154
})
150155

156+
Context("When adding valid performance tuning args with kube-api-burst >= kube-api-qps", func() {
157+
158+
It("should add the performance args to the cert-manager controller deployment", func() {
159+
160+
By("Adding valid performance tuning args to the cert-manager operator object")
161+
args := []string{"--concurrent-workers=10", "--kube-api-qps=50", "--kube-api-burst=100", "--max-concurrent-challenges=120"}
162+
err := addOverrideArgs(certmanageroperatorclient, certmanagerControllerDeployment, args)
163+
Expect(err).NotTo(HaveOccurred())
164+
165+
By("Waiting for cert-manager controller status to become available")
166+
err = verifyOperatorStatusCondition(certmanageroperatorclient.OperatorV1alpha1(), GenerateConditionMatchers(
167+
[]PrefixAndMatchTypeTuple{{certManagerController, MatchAllConditions}}, validOperatorStatusConditions,
168+
))
169+
Expect(err).NotTo(HaveOccurred())
170+
171+
By("Waiting for the args to be added to the cert-manager controller deployment")
172+
err = verifyDeploymentArgs(k8sClientSet, certmanagerControllerDeployment, args, true)
173+
Expect(err).NotTo(HaveOccurred())
174+
})
175+
})
176+
177+
Context("When adding kube-api-burst less than kube-api-qps", func() {
178+
179+
It("should reject the config and degrade the controller", func() {
180+
181+
By("Adding burst < qps args to the cert-manager operator object")
182+
args := []string{"--kube-api-qps=100", "--kube-api-burst=50"}
183+
err := addOverrideArgs(certmanageroperatorclient, certmanagerControllerDeployment, args)
184+
Expect(err).NotTo(HaveOccurred())
185+
186+
By("Waiting for cert-manager controller status to become degraded")
187+
err = verifyOperatorStatusCondition(certmanageroperatorclient.OperatorV1alpha1(), GenerateConditionMatchers(
188+
[]PrefixAndMatchTypeTuple{{certManagerController, MatchAnyCondition}}, invalidOperatorStatusConditions,
189+
))
190+
Expect(err).NotTo(HaveOccurred())
191+
192+
By("Checking if the args are not added to the cert-manager controller deployment")
193+
err = verifyDeploymentArgs(k8sClientSet, certmanagerControllerDeployment, args, false)
194+
Expect(err).NotTo(HaveOccurred())
195+
})
196+
})
197+
151198
Context("When adding invalid cert-manager webhook override args", func() {
152199

153200
It("should not add the args to the cert-manager webhook deployment", func() {

0 commit comments

Comments
 (0)