Skip to content

Commit

Permalink
Merge pull request #5971 from simonpasquier/fix-5970
Browse files Browse the repository at this point in the history
fix: update reloader requests and limits when updated
  • Loading branch information
simonpasquier committed Oct 20, 2023
2 parents 67cb98d + a33cb4c commit 7890689
Show file tree
Hide file tree
Showing 5 changed files with 198 additions and 81 deletions.
16 changes: 10 additions & 6 deletions pkg/operator/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,10 +72,12 @@ func DefaultConfig(cpu, memory string) Config {
// ContainerConfig holds some configuration for the ConfigReloader sidecar
// that can be set through prometheus-operator command line arguments
type ContainerConfig struct {
CPURequests Quantity
CPULimits Quantity
MemoryRequests Quantity
MemoryLimits Quantity
// The struct tag are needed for github.com/mitchellh/hashstructure to take
// the field values into account when generating the statefulset hash.
CPURequests Quantity `hash:"string"`
CPULimits Quantity `hash:"string"`
MemoryRequests Quantity `hash:"string"`
MemoryLimits Quantity `hash:"string"`
Image string
EnableProbes bool
}
Expand Down Expand Up @@ -106,8 +108,10 @@ type Quantity struct {
q resource.Quantity
}

// String implements the flag.Value interface
func (q *Quantity) String() string {
var _ = fmt.Stringer(Quantity{})

// String implements the flag.Value and fmt.Stringer interfaces.
func (q Quantity) String() string {
return q.q.String()
}

Expand Down
94 changes: 94 additions & 0 deletions test/e2e/config_reloader_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
// Copyright 2023 The prometheus-operator Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package e2e

import (
"context"
"testing"

"github.com/stretchr/testify/require"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/resource"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

operatorFramework "github.com/prometheus-operator/prometheus-operator/test/framework"
)

func testConfigReloaderResources(t *testing.T) {
testCtx := framework.NewTestCtx(t)
defer testCtx.Cleanup(t)

ns := framework.CreateNamespace(context.Background(), t, testCtx)
framework.SetupPrometheusRBACGlobal(context.Background(), t, testCtx, ns)

// Start Prometheus operator with the default resource requirements for the
// config reloader.
_, err := framework.CreateOrUpdatePrometheusOperatorWithOpts(
context.Background(),
operatorFramework.PrometheusOperatorOpts{
Namespace: ns,
AllowedNamespaces: []string{ns},
},
)
require.NoError(t, err)

p := framework.MakeBasicPrometheus(ns, "instance", "instance", 1)
p, err = framework.CreatePrometheusAndWaitUntilReady(context.Background(), ns, p)
require.NoError(t, err)

// Update the Prometheus operator deployment with new resource requirements
// for the config reloader.
var (
cpuRequest = resource.MustParse("14m")
cpuLimit = resource.MustParse("14m")
memRequest = resource.MustParse("61Mi")
memLimit = resource.MustParse("62Mi")
)
_, err = framework.CreateOrUpdatePrometheusOperatorWithOpts(
context.Background(),
operatorFramework.PrometheusOperatorOpts{
Namespace: ns,
AllowedNamespaces: []string{ns},
AdditionalArgs: []string{
"--config-reloader-cpu-limit=" + cpuLimit.String(),
"--config-reloader-cpu-request=" + cpuRequest.String(),
"--config-reloader-memory-limit=" + memLimit.String(),
"--config-reloader-memory-request=" + memRequest.String(),
},
},
)
require.NoError(t, err)

sts, err := framework.KubeClient.AppsV1().StatefulSets(p.Namespace).Get(context.Background(), "prometheus-"+p.Name, metav1.GetOptions{})
require.NoError(t, err)

var resources []corev1.ResourceRequirements
var containers []corev1.Container
containers = append(containers, sts.Spec.Template.Spec.Containers...)
containers = append(containers, sts.Spec.Template.Spec.InitContainers...)
for _, c := range containers {
if c.Name == "config-reloader" || c.Name == "init-config-reloader" {
resources = append(resources, c.Resources)
}
}
require.Equal(t, 2, len(resources))

for _, r := range resources {
require.True(t, cpuLimit.Equal(r.Limits[corev1.ResourceCPU]), "expected %s, got %s", cpuLimit, r.Limits[corev1.ResourceCPU])
require.True(t, cpuRequest.Equal(r.Requests[corev1.ResourceCPU]), "expected %s, got %s", cpuRequest, r.Requests[corev1.ResourceCPU])
require.True(t, memLimit.Equal(r.Limits[corev1.ResourceMemory]), "expected %s, got %s", memLimit, r.Limits[corev1.ResourceMemory])
require.True(t, memRequest.Equal(r.Requests[corev1.ResourceMemory]), "expected %s, got %s", memRequest, r.Requests[corev1.ResourceMemory])
}
}
11 changes: 6 additions & 5 deletions test/e2e/main_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -370,11 +370,12 @@ func TestDenylist(t *testing.T) {
func TestPromInstanceNs(t *testing.T) {
skipPrometheusTests(t)
testFuncs := map[string]func(t *testing.T){
"AllNs": testPrometheusInstanceNamespacesAllNs,
"AllowList": testPrometheusInstanceNamespacesAllowList,
"DenyList": testPrometheusInstanceNamespacesDenyList,
"NamespaceNotFound": testPrometheusInstanceNamespacesNamespaceNotFound,
"ScrapeConfigLifecycle": testScrapeConfigLifecycle,
"AllNs": testPrometheusInstanceNamespacesAllNs,
"AllowList": testPrometheusInstanceNamespacesAllowList,
"DenyList": testPrometheusInstanceNamespacesDenyList,
"NamespaceNotFound": testPrometheusInstanceNamespacesNamespaceNotFound,
"ScrapeConfigLifecycle": testScrapeConfigLifecycle,
"ConfigReloaderResources": testConfigReloaderResources,
}

for name, f := range testFuncs {
Expand Down
100 changes: 70 additions & 30 deletions test/framework/framework.go
Original file line number Diff line number Diff line change
Expand Up @@ -195,14 +195,21 @@ func (f *Framework) MakeEchoDeployment(group string) *appsv1.Deployment {
}
}

// CreateOrUpdatePrometheusOperator creates or updates a Prometheus Operator Kubernetes Deployment
// inside the specified namespace using the specified operator image. Semver is used
// to control the installation for different version of Prometheus Operator. In addition
// one can specify the namespaces to watch, which defaults to all namespaces.
// Returns the CA, which can bs used to access the operator over TLS
type PrometheusOperatorOpts struct {
Namespace string
AllowedNamespaces []string
DeniedNamespaces []string
PrometheusNamespaces []string
AlertmanagerNamespaces []string
EnableAdmissionWebhook bool
ClusterRoleBindings bool
EnableScrapeConfigs bool
AdditionalArgs []string
}

func (f *Framework) CreateOrUpdatePrometheusOperator(
ctx context.Context,
ns string,
namespace string,
namespaceAllowlist,
namespaceDenylist,
prometheusInstanceNamespaces,
Expand All @@ -211,12 +218,37 @@ func (f *Framework) CreateOrUpdatePrometheusOperator(
createClusterRoleBindings,
createScrapeConfigCrd bool,
) ([]FinalizerFn, error) {
return f.CreateOrUpdatePrometheusOperatorWithOpts(
ctx,
PrometheusOperatorOpts{
Namespace: namespace,
AllowedNamespaces: namespaceAllowlist,
DeniedNamespaces: namespaceDenylist,
PrometheusNamespaces: prometheusInstanceNamespaces,
AlertmanagerNamespaces: alertmanagerInstanceNamespaces,
EnableAdmissionWebhook: createResourceAdmissionHooks,
ClusterRoleBindings: createClusterRoleBindings,
EnableScrapeConfigs: createScrapeConfigCrd,
},
)
}

// CreateOrUpdatePrometheusOperatorWithOpts creates or updates a Prometheus
// Operator Kubernetes Deployment inside the specified namespace using the
// specified operator image. Semver is used to control the installation for
// different versions of Prometheus Operator. In addition one can specify the
// namespaces to watch, which defaults to all namespaces. It returns a slice
// of functions to tear down the deployment.
func (f *Framework) CreateOrUpdatePrometheusOperatorWithOpts(
ctx context.Context,
opts PrometheusOperatorOpts,
) ([]FinalizerFn, error) {

var finalizers []FinalizerFn

_, err := f.createOrUpdateServiceAccount(
ctx,
ns,
opts.Namespace,
fmt.Sprintf("%s/rbac/prometheus-operator/prometheus-operator-service-account.yaml", f.exampleDir),
)
if err != nil {
Expand All @@ -234,17 +266,20 @@ func (f *Framework) CreateOrUpdatePrometheusOperator(
return nil, errors.Wrap(err, "failed to update prometheus cluster role")
}

if createClusterRoleBindings {
if _, err := f.createOrUpdateClusterRoleBinding(ctx, ns, fmt.Sprintf("%s/rbac/prometheus-operator/prometheus-operator-cluster-role-binding.yaml", f.exampleDir)); err != nil {
if opts.ClusterRoleBindings {
// Grant permissions on all namespaces.
if _, err := f.createOrUpdateClusterRoleBinding(ctx, opts.Namespace, fmt.Sprintf("%s/rbac/prometheus-operator/prometheus-operator-cluster-role-binding.yaml", f.exampleDir)); err != nil {
return nil, errors.Wrap(err, "failed to create or update prometheus cluster role binding")
}
} else {
namespaces := namespaceAllowlist
namespaces = append(namespaces, prometheusInstanceNamespaces...)
namespaces = append(namespaces, alertmanagerInstanceNamespaces...)
// Grant permissions on specific namespaces.
var namespaces []string
namespaces = append(namespaces, opts.AllowedNamespaces...)
namespaces = append(namespaces, opts.PrometheusNamespaces...)
namespaces = append(namespaces, opts.AlertmanagerNamespaces...)

for _, n := range namespaces {
if _, err := f.CreateOrUpdateRoleBindingForSubjectNamespace(ctx, n, ns, fmt.Sprintf("%s/prometheus-operator-role-binding.yaml", f.resourcesDir)); err != nil {
if _, err := f.CreateOrUpdateRoleBindingForSubjectNamespace(ctx, n, opts.Namespace, fmt.Sprintf("%s/prometheus-operator-role-binding.yaml", f.resourcesDir)); err != nil {
return nil, errors.Wrap(err, "failed to create or update prometheus operator role binding")
}
}
Expand Down Expand Up @@ -320,7 +355,7 @@ func (f *Framework) CreateOrUpdatePrometheusOperator(
return nil, errors.Wrap(err, "initialize PrometheusAgent v1alpha1 CRD")
}

if createScrapeConfigCrd {
if opts.EnableScrapeConfigs {
err = f.CreateOrUpdateCRDAndWaitUntilReady(ctx, monitoringv1alpha1.ScrapeConfigName, func(opts metav1.ListOptions) (runtime.Object, error) {
return f.MonClientV1alpha1.ScrapeConfigs(v1.NamespaceAll).List(ctx, opts)
})
Expand All @@ -329,12 +364,12 @@ func (f *Framework) CreateOrUpdatePrometheusOperator(
}
}

certBytes, keyBytes, err := certutil.GenerateSelfSignedCertKey(fmt.Sprintf("%s.%s.svc", prometheusOperatorServiceDeploymentName, ns), nil, nil)
certBytes, keyBytes, err := certutil.GenerateSelfSignedCertKey(fmt.Sprintf("%s.%s.svc", prometheusOperatorServiceDeploymentName, opts.Namespace), nil, nil)
if err != nil {
return nil, errors.Wrap(err, "failed to generate certificate and key")
}

if err := f.CreateOrUpdateSecretWithCert(ctx, certBytes, keyBytes, ns, prometheusOperatorCertsSecretName); err != nil {
if err := f.CreateOrUpdateSecretWithCert(ctx, certBytes, keyBytes, opts.Namespace, prometheusOperatorCertsSecretName); err != nil {
return nil, errors.Wrap(err, "failed to create or update prometheus-operator TLS secret")
}

Expand All @@ -343,7 +378,7 @@ func (f *Framework) CreateOrUpdatePrometheusOperator(
return nil, err
}

// Make sure only one version of prometheus operator when update
// Make sure only that only one instance of the Prometheus operator is running during update.
deploy.Spec.Strategy.Type = appsv1.RecreateDeploymentStrategyType

deploy.Spec.Template.Spec.Containers[0].Args = append(deploy.Spec.Template.Spec.Containers[0].Args, "--log-level=debug")
Expand Down Expand Up @@ -373,28 +408,28 @@ func (f *Framework) CreateOrUpdatePrometheusOperator(

deploy.Name = prometheusOperatorServiceDeploymentName

for _, ns := range namespaceAllowlist {
for _, ns := range opts.AllowedNamespaces {
deploy.Spec.Template.Spec.Containers[0].Args = append(
deploy.Spec.Template.Spec.Containers[0].Args,
fmt.Sprintf("--namespaces=%v", ns),
)
}

for _, ns := range namespaceDenylist {
for _, ns := range opts.DeniedNamespaces {
deploy.Spec.Template.Spec.Containers[0].Args = append(
deploy.Spec.Template.Spec.Containers[0].Args,
fmt.Sprintf("--deny-namespaces=%v", ns),
)
}

for _, ns := range prometheusInstanceNamespaces {
for _, ns := range opts.PrometheusNamespaces {
deploy.Spec.Template.Spec.Containers[0].Args = append(
deploy.Spec.Template.Spec.Containers[0].Args,
fmt.Sprintf("--prometheus-instance-namespaces=%v", ns),
)
}

for _, ns := range alertmanagerInstanceNamespaces {
for _, ns := range opts.AlertmanagerNamespaces {
deploy.Spec.Template.Spec.Containers[0].Args = append(
deploy.Spec.Template.Spec.Containers[0].Args,
fmt.Sprintf("--alertmanager-instance-namespaces=%v", ns),
Expand All @@ -412,15 +447,20 @@ func (f *Framework) CreateOrUpdatePrometheusOperator(

// The addition of rule admission webhooks requires TLS, so enable it and
// switch to a more common https port
if createResourceAdmissionHooks {
if opts.EnableAdmissionWebhook {
deploy.Spec.Template.Spec.Containers[0].Args = append(
deploy.Spec.Template.Spec.Containers[0].Args,
"--web.enable-tls=true",
fmt.Sprintf("--web.listen-address=%v", ":8443"),
)
}

err = f.CreateOrUpdateDeploymentAndWaitUntilReady(ctx, ns, deploy)
deploy.Spec.Template.Spec.Containers[0].Args = append(
deploy.Spec.Template.Spec.Containers[0].Args,
opts.AdditionalArgs...,
)

err = f.CreateOrUpdateDeploymentAndWaitUntilReady(ctx, opts.Namespace, deploy)
if err != nil {
return nil, err
}
Expand All @@ -430,33 +470,33 @@ func (f *Framework) CreateOrUpdatePrometheusOperator(
return finalizers, errors.Wrap(err, "cannot parse service file")
}

service.Namespace = ns
service.Namespace = opts.Namespace
service.Spec.ClusterIP = ""
service.Spec.Ports = []v1.ServicePort{{Name: "https", Port: 443, TargetPort: intstr.FromInt(8443)}}

if _, err := f.CreateOrUpdateServiceAndWaitUntilReady(ctx, ns, service); err != nil {
if _, err := f.CreateOrUpdateServiceAndWaitUntilReady(ctx, opts.Namespace, service); err != nil {
return finalizers, errors.Wrap(err, "failed to create or update prometheus operator service")
}

if createResourceAdmissionHooks {
webhookService, b, err := f.CreateOrUpdateAdmissionWebhookServer(ctx, ns, webhookServerImage)
if opts.EnableAdmissionWebhook {
webhookService, b, err := f.CreateOrUpdateAdmissionWebhookServer(ctx, opts.Namespace, webhookServerImage)
if err != nil {
return nil, errors.Wrap(err, "failed to create webhook server")
}

finalizer, err := f.createOrUpdateMutatingHook(ctx, b, ns, fmt.Sprintf("%s/prometheus-operator-mutatingwebhook.yaml", f.resourcesDir))
finalizer, err := f.createOrUpdateMutatingHook(ctx, b, opts.Namespace, fmt.Sprintf("%s/prometheus-operator-mutatingwebhook.yaml", f.resourcesDir))
if err != nil {
return nil, errors.Wrap(err, "failed to create or update mutating webhook for PrometheusRule objects")
}
finalizers = append(finalizers, finalizer)

finalizer, err = f.createOrUpdateValidatingHook(ctx, b, ns, fmt.Sprintf("%s/prometheus-operator-validatingwebhook.yaml", f.resourcesDir))
finalizer, err = f.createOrUpdateValidatingHook(ctx, b, opts.Namespace, fmt.Sprintf("%s/prometheus-operator-validatingwebhook.yaml", f.resourcesDir))
if err != nil {
return nil, errors.Wrap(err, "failed to create or update validating webhook for PrometheusRule objects")
}
finalizers = append(finalizers, finalizer)

finalizer, err = f.createOrUpdateValidatingHook(ctx, b, ns, fmt.Sprintf("%s/alertmanager-config-validating-webhook.yaml", f.resourcesDir))
finalizer, err = f.createOrUpdateValidatingHook(ctx, b, opts.Namespace, fmt.Sprintf("%s/alertmanager-config-validating-webhook.yaml", f.resourcesDir))
if err != nil {
return nil, errors.Wrap(err, "failed to create or update validating webhook for AlertManagerConfig objects")
}
Expand Down

0 comments on commit 7890689

Please sign in to comment.