From 97f61775008931660b1abfb4ec1dd9fd7cb4785f Mon Sep 17 00:00:00 2001 From: Anatolii Bazko Date: Thu, 25 May 2023 17:20:44 +0300 Subject: [PATCH] feat: Set empty CPU limits when possible (#1686) * feat: Set empty CPU limits when possible Signed-off-by: Anatolii Bazko --- .../che-operator.clusterserviceversion.yaml | 10 +- config/rbac/cluster_role.yaml | 8 +- .../devworkspace/solver/che_routing.go | 6 +- .../devworkspace/solver/che_routing_test.go | 20 +-- deploy/deployment/kubernetes/combined.yaml | 6 + .../objects/che-operator.ClusterRole.yaml | 6 + deploy/deployment/openshift/combined.yaml | 6 + .../objects/che-operator.ClusterRole.yaml | 6 + .../templates/che-operator.ClusterRole.yaml | 6 + .../dashboard/dashboard_deployment_test.go | 2 +- pkg/deploy/dashboard/deployment_dashboard.go | 4 +- pkg/deploy/deployment.go | 123 +++++++++------ pkg/deploy/deployment_test.go | 144 +++++++++++++++++- pkg/deploy/devfileregistry/devfileregistry.go | 7 +- .../devfileregistry_deployment.go | 8 +- .../devfileregistry_deployment_test.go | 7 +- pkg/deploy/gateway/gateway.go | 14 +- pkg/deploy/gateway/gateway_test.go | 7 +- pkg/deploy/pluginregistry/pluginregistry.go | 7 +- .../pluginregistry_deployment.go | 9 +- .../pluginregistry_deployment_test.go | 8 +- pkg/deploy/server/server_deployment.go | 4 +- pkg/deploy/server/server_deployment_test.go | 26 +--- 23 files changed, 327 insertions(+), 117 deletions(-) diff --git a/bundle/next/eclipse-che/manifests/che-operator.clusterserviceversion.yaml b/bundle/next/eclipse-che/manifests/che-operator.clusterserviceversion.yaml index 8255a2364..79e71b130 100644 --- a/bundle/next/eclipse-che/manifests/che-operator.clusterserviceversion.yaml +++ b/bundle/next/eclipse-che/manifests/che-operator.clusterserviceversion.yaml @@ -77,7 +77,7 @@ metadata: operators.operatorframework.io/project_layout: go.kubebuilder.io/v3 repository: https://github.com/eclipse-che/che-operator support: Eclipse Foundation - name: eclipse-che.v7.68.0-793.next + name: eclipse-che.v7.68.0-794.next namespace: placeholder spec: apiservicedefinitions: {} @@ -866,6 +866,12 @@ spec: - delete - update - use + - apiGroups: + - "" + resources: + - limitranges + verbs: + - list serviceAccountName: che-operator deployments: - name: che-operator @@ -1229,7 +1235,7 @@ spec: minKubeVersion: 1.19.0 provider: name: Eclipse Foundation - version: 7.68.0-793.next + version: 7.68.0-794.next webhookdefinitions: - admissionReviewVersions: - v1 diff --git a/config/rbac/cluster_role.yaml b/config/rbac/cluster_role.yaml index 26f49aa75..c1027f558 100644 --- a/config/rbac/cluster_role.yaml +++ b/config/rbac/cluster_role.yaml @@ -402,4 +402,10 @@ rules: - create - delete - update - - use \ No newline at end of file + - use + - apiGroups: + - "" + resources: + - limitranges + verbs: + - list \ No newline at end of file diff --git a/controllers/devworkspace/solver/che_routing.go b/controllers/devworkspace/solver/che_routing.go index f1a2a49dc..74d4ac315 100644 --- a/controllers/devworkspace/solver/che_routing.go +++ b/controllers/devworkspace/solver/che_routing.go @@ -199,10 +199,8 @@ func (c *CheRoutingSolver) provisionPodAdditions(objs *solvers.RoutingObjects, c }, } - if cheCluster.Spec.DevEnvironments.GatewayContainer != nil { - if err := deploy.CustomizeContainer(gatewayContainer, cheCluster.Spec.DevEnvironments.GatewayContainer); err != nil { - return err - } + if err := deploy.OverrideContainer(routing.GetNamespace(), gatewayContainer, cheCluster.Spec.DevEnvironments.GatewayContainer); err != nil { + return err } objs.PodAdditions.Containers = append(objs.PodAdditions.Containers, *gatewayContainer) diff --git a/controllers/devworkspace/solver/che_routing_test.go b/controllers/devworkspace/solver/che_routing_test.go index 4aa9ab983..a147cd205 100644 --- a/controllers/devworkspace/solver/che_routing_test.go +++ b/controllers/devworkspace/solver/che_routing_test.go @@ -20,8 +20,6 @@ import ( "github.com/stretchr/testify/assert" - // "k8s.io/apimachinery/pkg/api/resource" - dw "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" dwo "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" "github.com/devfile/devworkspace-operator/controllers/controller/devworkspacerouting/solvers" @@ -1094,7 +1092,8 @@ func TestOverrideGatewayContainerProvisioning(t *testing.T) { }, } - cheSolver.provisionPodAdditions(objs, cheCluster, routing) + err := cheSolver.provisionPodAdditions(objs, cheCluster, routing) + assert.NoError(t, err) assert.Equal(t, overrideCpuRequest.String(), objs.PodAdditions.Containers[0].Resources.Requests.Cpu().String()) assert.Equal(t, overrideMemoryRequest.String(), objs.PodAdditions.Containers[0].Resources.Requests.Memory().String()) @@ -1106,7 +1105,6 @@ func TestOverridePartialLimitsGatewayContainerProvisioning(t *testing.T) { overrideMemoryRequest := resource.MustParse("0") overrideCpuRequest := resource.MustParse("0") defaultMemoryLimit := resource.MustParse(constants.DefaultGatewayMemoryLimit) - defaultCpuLimit := resource.MustParse(constants.DefaultGatewayCpuLimit) cheCluster := &chev2.CheCluster{ ObjectMeta: metav1.ObjectMeta{ @@ -1147,11 +1145,12 @@ func TestOverridePartialLimitsGatewayContainerProvisioning(t *testing.T) { }, } - cheSolver.provisionPodAdditions(objs, cheCluster, routing) + err := cheSolver.provisionPodAdditions(objs, cheCluster, routing) + assert.NoError(t, err) assert.Equal(t, overrideCpuRequest.String(), objs.PodAdditions.Containers[0].Resources.Requests.Cpu().String()) assert.Equal(t, overrideMemoryRequest.String(), objs.PodAdditions.Containers[0].Resources.Requests.Memory().String()) - assert.Equal(t, defaultCpuLimit.String(), objs.PodAdditions.Containers[0].Resources.Limits.Cpu().String()) + assert.Empty(t, objs.PodAdditions.Containers[0].Resources.Limits[corev1.ResourceCPU]) assert.Equal(t, defaultMemoryLimit.String(), objs.PodAdditions.Containers[0].Resources.Limits.Memory().String()) } @@ -1204,7 +1203,8 @@ func TestOverrideGatewayEmptyContainerProvisioning(t *testing.T) { }, } - cheSolver.provisionPodAdditions(objs, cheCluster, routing) + err := cheSolver.provisionPodAdditions(objs, cheCluster, routing) + assert.NoError(t, err) assert.Equal(t, overrideCpuRequest.String(), objs.PodAdditions.Containers[0].Resources.Requests.Cpu().String()) assert.Equal(t, overrideMemoryRequest.String(), objs.PodAdditions.Containers[0].Resources.Requests.Memory().String()) @@ -1216,7 +1216,6 @@ func TestDefaultGatewayContainerProvisioning(t *testing.T) { defaultMemoryRequest := resource.MustParse(constants.DefaultGatewayMemoryRequest) defaultCpuRequest := resource.MustParse(constants.DefaultGatewayCpuRequest) defaultMemoryLimit := resource.MustParse(constants.DefaultGatewayMemoryLimit) - defaultCpuLimit := resource.MustParse(constants.DefaultGatewayCpuLimit) cheCluster := &chev2.CheCluster{ ObjectMeta: metav1.ObjectMeta{ @@ -1247,10 +1246,11 @@ func TestDefaultGatewayContainerProvisioning(t *testing.T) { }, } - cheSolver.provisionPodAdditions(objs, cheCluster, routing) + err := cheSolver.provisionPodAdditions(objs, cheCluster, routing) + assert.NoError(t, err) assert.Equal(t, defaultCpuRequest.String(), objs.PodAdditions.Containers[0].Resources.Requests.Cpu().String()) assert.Equal(t, defaultMemoryRequest.String(), objs.PodAdditions.Containers[0].Resources.Requests.Memory().String()) - assert.Equal(t, defaultCpuLimit.String(), objs.PodAdditions.Containers[0].Resources.Limits.Cpu().String()) + assert.Empty(t, objs.PodAdditions.Containers[0].Resources.Limits[corev1.ResourceCPU]) assert.Equal(t, defaultMemoryLimit.String(), objs.PodAdditions.Containers[0].Resources.Limits.Memory().String()) } diff --git a/deploy/deployment/kubernetes/combined.yaml b/deploy/deployment/kubernetes/combined.yaml index 3b391e647..0cf453c08 100644 --- a/deploy/deployment/kubernetes/combined.yaml +++ b/deploy/deployment/kubernetes/combined.yaml @@ -8123,6 +8123,12 @@ rules: - delete - update - use +- apiGroups: + - "" + resources: + - limitranges + verbs: + - list --- apiVersion: rbac.authorization.k8s.io/v1 kind: RoleBinding diff --git a/deploy/deployment/kubernetes/objects/che-operator.ClusterRole.yaml b/deploy/deployment/kubernetes/objects/che-operator.ClusterRole.yaml index 1820f247e..b31130897 100644 --- a/deploy/deployment/kubernetes/objects/che-operator.ClusterRole.yaml +++ b/deploy/deployment/kubernetes/objects/che-operator.ClusterRole.yaml @@ -403,3 +403,9 @@ rules: - delete - update - use +- apiGroups: + - "" + resources: + - limitranges + verbs: + - list diff --git a/deploy/deployment/openshift/combined.yaml b/deploy/deployment/openshift/combined.yaml index e8d6c2c7d..5cad47d1c 100644 --- a/deploy/deployment/openshift/combined.yaml +++ b/deploy/deployment/openshift/combined.yaml @@ -8123,6 +8123,12 @@ rules: - delete - update - use +- apiGroups: + - "" + resources: + - limitranges + verbs: + - list --- apiVersion: rbac.authorization.k8s.io/v1 kind: RoleBinding diff --git a/deploy/deployment/openshift/objects/che-operator.ClusterRole.yaml b/deploy/deployment/openshift/objects/che-operator.ClusterRole.yaml index 1820f247e..b31130897 100644 --- a/deploy/deployment/openshift/objects/che-operator.ClusterRole.yaml +++ b/deploy/deployment/openshift/objects/che-operator.ClusterRole.yaml @@ -403,3 +403,9 @@ rules: - delete - update - use +- apiGroups: + - "" + resources: + - limitranges + verbs: + - list diff --git a/helmcharts/next/templates/che-operator.ClusterRole.yaml b/helmcharts/next/templates/che-operator.ClusterRole.yaml index 1820f247e..b31130897 100644 --- a/helmcharts/next/templates/che-operator.ClusterRole.yaml +++ b/helmcharts/next/templates/che-operator.ClusterRole.yaml @@ -403,3 +403,9 @@ rules: - delete - update - use +- apiGroups: + - "" + resources: + - limitranges + verbs: + - list diff --git a/pkg/deploy/dashboard/dashboard_deployment_test.go b/pkg/deploy/dashboard/dashboard_deployment_test.go index c2abe61a5..cfed5eae1 100644 --- a/pkg/deploy/dashboard/dashboard_deployment_test.go +++ b/pkg/deploy/dashboard/dashboard_deployment_test.go @@ -71,7 +71,7 @@ func TestDashboardDeploymentResources(t *testing.T) { initObjects: []runtime.Object{}, memoryLimit: constants.DefaultDashboardMemoryLimit, memoryRequest: constants.DefaultDashboardMemoryRequest, - cpuLimit: constants.DefaultDashboardCpuLimit, + cpuLimit: "0", // CPU limit is not set when possible cpuRequest: constants.DefaultDashboardCpuRequest, cheCluster: &chev2.CheCluster{ ObjectMeta: metav1.ObjectMeta{ diff --git a/pkg/deploy/dashboard/deployment_dashboard.go b/pkg/deploy/dashboard/deployment_dashboard.go index be3e27d5f..f7b04b3ff 100644 --- a/pkg/deploy/dashboard/deployment_dashboard.go +++ b/pkg/deploy/dashboard/deployment_dashboard.go @@ -197,7 +197,9 @@ func (d *DashboardReconciler) getDashboardDeploymentSpec(ctx *chetypes.DeployCon } deploy.EnsurePodSecurityStandards(deployment, constants.DefaultSecurityContextRunAsUser, constants.DefaultSecurityContextFsGroup) - deploy.CustomizeDeployment(deployment, ctx.CheCluster.Spec.Components.Dashboard.Deployment) + if err := deploy.OverrideDeployment(ctx, deployment, ctx.CheCluster.Spec.Components.Dashboard.Deployment); err != nil { + return nil, err + } return deployment, nil } diff --git a/pkg/deploy/deployment.go b/pkg/deploy/deployment.go index e70575d62..648901c0e 100644 --- a/pkg/deploy/deployment.go +++ b/pkg/deploy/deployment.go @@ -17,6 +17,9 @@ import ( "sort" "strings" + k8shelper "github.com/eclipse-che/che-operator/pkg/common/k8s-helper" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/selection" "k8s.io/utils/pointer" @@ -96,39 +99,47 @@ func SyncDeploymentSpecToCluster( return provisioned, nil } -// CustomizeDeployment customize deployment -func CustomizeDeployment(deployment *appsv1.Deployment, customSettings *chev2.Deployment) error { - if customSettings == nil || len(customSettings.Containers) == 0 { - return nil - } +// OverrideDeployment with custom settings +func OverrideDeployment( + ctx *chetypes.DeployContext, + deployment *appsv1.Deployment, + overrideDeploymentSettings *chev2.Deployment) error { for index, _ := range deployment.Spec.Template.Spec.Containers { + var overrideContainerSettings *chev2.Container container := &deployment.Spec.Template.Spec.Containers[index] - customContainer := &customSettings.Containers[0] - if len(deployment.Spec.Template.Spec.Containers) != 1 { - customContainer = getContainerByName(container.Name, customSettings.Containers) - if customContainer == nil { - continue + if overrideDeploymentSettings != nil { + if len(deployment.Spec.Template.Spec.Containers) != 1 { + for _, c := range overrideDeploymentSettings.Containers { + if c.Name == container.Name { + overrideContainerSettings = &c + break + } + } + } else { + overrideContainerSettings = &overrideDeploymentSettings.Containers[0] } } - if err := CustomizeContainer(container, customContainer); err != nil { + if err := OverrideContainer(ctx.CheCluster.Namespace, container, overrideContainerSettings); err != nil { return err } } if !infrastructure.IsOpenShift() { - if customSettings.SecurityContext != nil { - if deployment.Spec.Template.Spec.SecurityContext == nil { - deployment.Spec.Template.Spec.SecurityContext = &corev1.PodSecurityContext{} - } + if overrideDeploymentSettings != nil { + if overrideDeploymentSettings.SecurityContext != nil { + if deployment.Spec.Template.Spec.SecurityContext == nil { + deployment.Spec.Template.Spec.SecurityContext = &corev1.PodSecurityContext{} + } - if customSettings.SecurityContext.FsGroup != nil { - deployment.Spec.Template.Spec.SecurityContext.FSGroup = pointer.Int64Ptr(*customSettings.SecurityContext.FsGroup) - } - if customSettings.SecurityContext.RunAsUser != nil { - deployment.Spec.Template.Spec.SecurityContext.RunAsUser = pointer.Int64Ptr(*customSettings.SecurityContext.RunAsUser) + if overrideDeploymentSettings.SecurityContext.FsGroup != nil { + deployment.Spec.Template.Spec.SecurityContext.FSGroup = pointer.Int64Ptr(*overrideDeploymentSettings.SecurityContext.FsGroup) + } + if overrideDeploymentSettings.SecurityContext.RunAsUser != nil { + deployment.Spec.Template.Spec.SecurityContext.RunAsUser = pointer.Int64Ptr(*overrideDeploymentSettings.SecurityContext.RunAsUser) + } } } } @@ -136,16 +147,37 @@ func CustomizeDeployment(deployment *appsv1.Deployment, customSettings *chev2.De return nil } -// CustomizeContainer customize container with custom settings. -func CustomizeContainer(container *corev1.Container, customSettings *chev2.Container) error { - container.Image = utils.GetValue(customSettings.Image, container.Image) +func OverrideContainer( + namespace string, + container *corev1.Container, + overrideSettings *chev2.Container) error { - container.ImagePullPolicy = customSettings.ImagePullPolicy + // Set empty CPU limits when possible: + // 1. If there is no LimitRange in the namespace + // 2. CPU limits is not overridden + // See details at https://github.com/eclipse/che/issues/22198 + if overrideSettings == nil || overrideSettings.Resources == nil || overrideSettings.Resources.Limits == nil || overrideSettings.Resources.Limits.Cpu == nil { + // use NonCachedClient to avoid cache LimitRange objects + if limitRanges, err := k8shelper.New().GetClientset().CoreV1().LimitRanges(namespace).List(context.TODO(), metav1.ListOptions{}); err != nil { + return err + } else if len(limitRanges.Items) == 0 { // no LimitRange in the namespace + delete(container.Resources.Limits, corev1.ResourceCPU) + } + } + + if overrideSettings == nil { + return nil + } + + // Image + container.Image = utils.GetValue(overrideSettings.Image, container.Image) + container.ImagePullPolicy = overrideSettings.ImagePullPolicy if container.ImagePullPolicy == "" { container.ImagePullPolicy = corev1.PullPolicy(utils.GetPullPolicyFromDockerImage(container.Image)) } - for _, env := range customSettings.Env { + // Env + for _, env := range overrideSettings.Env { index := utils.IndexEnv(env.Name, container.Env) if index == -1 { container.Env = append(container.Env, env) @@ -154,21 +186,22 @@ func CustomizeContainer(container *corev1.Container, customSettings *chev2.Conta } } - if customSettings.Resources != nil { - if customSettings.Resources.Requests != nil { - if customSettings.Resources.Requests.Memory != nil { - if customSettings.Resources.Requests.Memory.IsZero() { + // Resources + if overrideSettings.Resources != nil { + if overrideSettings.Resources.Requests != nil { + if overrideSettings.Resources.Requests.Memory != nil { + if overrideSettings.Resources.Requests.Memory.IsZero() { delete(container.Resources.Requests, corev1.ResourceMemory) } else { - container.Resources.Requests[corev1.ResourceMemory] = *customSettings.Resources.Requests.Memory + container.Resources.Requests[corev1.ResourceMemory] = *overrideSettings.Resources.Requests.Memory } } - if customSettings.Resources.Requests.Cpu != nil { - if customSettings.Resources.Requests.Cpu.IsZero() { + if overrideSettings.Resources.Requests.Cpu != nil { + if overrideSettings.Resources.Requests.Cpu.IsZero() { delete(container.Resources.Requests, corev1.ResourceCPU) } else { - container.Resources.Requests[corev1.ResourceCPU] = *customSettings.Resources.Requests.Cpu + container.Resources.Requests[corev1.ResourceCPU] = *overrideSettings.Resources.Requests.Cpu } } @@ -177,20 +210,20 @@ func CustomizeContainer(container *corev1.Container, customSettings *chev2.Conta } } - if customSettings.Resources.Limits != nil { - if customSettings.Resources.Limits.Memory != nil { - if customSettings.Resources.Limits.Memory.IsZero() { + if overrideSettings.Resources.Limits != nil { + if overrideSettings.Resources.Limits.Memory != nil { + if overrideSettings.Resources.Limits.Memory.IsZero() { delete(container.Resources.Limits, corev1.ResourceMemory) } else { - container.Resources.Limits[corev1.ResourceMemory] = *customSettings.Resources.Limits.Memory + container.Resources.Limits[corev1.ResourceMemory] = *overrideSettings.Resources.Limits.Memory } } - if customSettings.Resources.Limits.Cpu != nil { - if customSettings.Resources.Limits.Cpu.IsZero() { + if overrideSettings.Resources.Limits.Cpu != nil { + if overrideSettings.Resources.Limits.Cpu.IsZero() { delete(container.Resources.Limits, corev1.ResourceCPU) } else { - container.Resources.Limits[corev1.ResourceCPU] = *customSettings.Resources.Limits.Cpu + container.Resources.Limits[corev1.ResourceCPU] = *overrideSettings.Resources.Limits.Cpu } } @@ -223,16 +256,6 @@ func EnsurePodSecurityStandards(deployment *appsv1.Deployment, userId int64, gro } } -func getContainerByName(name string, containers []chev2.Container) *chev2.Container { - for _, c := range containers { - if c.Name == name { - return &c - } - } - - return nil -} - // MountSecrets mounts secrets into a container as a file or as environment variable. // Secrets are selected by the following labels: // - app.kubernetes.io/part-of=che.eclipse.org diff --git a/pkg/deploy/deployment_test.go b/pkg/deploy/deployment_test.go index 9b876190d..cf617b5a3 100644 --- a/pkg/deploy/deployment_test.go +++ b/pkg/deploy/deployment_test.go @@ -16,6 +16,10 @@ import ( "os" "reflect" + k8shelper "github.com/eclipse-che/che-operator/pkg/common/k8s-helper" + + "github.com/eclipse-che/che-operator/pkg/common/test" + "github.com/stretchr/testify/assert" "k8s.io/apimachinery/pkg/api/resource" @@ -707,11 +711,14 @@ func TestCustomizeDeploymentShouldNotUpdateResources(t *testing.T) { }, } - err := CustomizeDeployment(deployment, customizationDeployment) + ctx := test.GetDeployContext(nil, []runtime.Object{}) + err := OverrideDeployment(ctx, deployment, customizationDeployment) assert.Nil(t, err) + assert.Equal(t, "1", deployment.Spec.Template.Spec.Containers[0].Resources.Requests.Cpu().String()) assert.Equal(t, "100Mi", deployment.Spec.Template.Spec.Containers[0].Resources.Requests.Memory().String()) - assert.Equal(t, "2", deployment.Spec.Template.Spec.Containers[0].Resources.Limits.Cpu().String()) + // CPU limit is not set when possible + assert.Equal(t, "0", deployment.Spec.Template.Spec.Containers[0].Resources.Limits.Cpu().String()) assert.Equal(t, "200Mi", deployment.Spec.Template.Spec.Containers[0].Resources.Limits.Memory().String()) } @@ -790,8 +797,10 @@ func TestCustomizeDeploymentImagePullPolicy(t *testing.T) { t.Run(testCase.name, func(t *testing.T) { logf.SetLogger(zap.New(zap.WriteTo(os.Stdout), zap.UseDevMode(true))) - err := CustomizeDeployment(testCase.initDeployment, testCase.customizationDeployment) + ctx := test.GetDeployContext(nil, []runtime.Object{}) + err := OverrideDeployment(ctx, testCase.initDeployment, testCase.customizationDeployment) assert.Nil(t, err) + assert.Equal(t, testCase.expectedImagePullPolicy, testCase.initDeployment.Spec.Template.Spec.Containers[0].ImagePullPolicy) }) } @@ -873,9 +882,136 @@ func TestCustomizeDeploymentEnvVar(t *testing.T) { t.Run(testCase.name, func(t *testing.T) { logf.SetLogger(zap.New(zap.WriteTo(os.Stdout), zap.UseDevMode(true))) - err := CustomizeDeployment(testCase.initDeployment, testCase.customizationDeployment) + ctx := test.GetDeployContext(nil, []runtime.Object{}) + err := OverrideDeployment(ctx, testCase.initDeployment, testCase.customizationDeployment) assert.Nil(t, err) + assert.Equal(t, testCase.expectedEnv, testCase.initDeployment.Spec.Template.Spec.Containers[0].Env) }) } } +func TestOverrideContainerCpuLimit(t *testing.T) { + type testCase struct { + name string + container *corev1.Container + overrideSettings *chev2.Container + limitRange *corev1.LimitRange + expectedCpuLimit string + } + + cpuLimit500m := resource.MustParse("500m") + + testCases := []testCase{ + { + name: "No CPU limit, LimitRange does not exists", + container: &corev1.Container{ + Resources: corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("250m"), + }, + }, + }, + overrideSettings: &chev2.Container{}, + expectedCpuLimit: "", + }, + { + name: "No CPU limit, LimitRange does not exists", + container: &corev1.Container{ + Resources: corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("250m"), + }, + }, + }, + overrideSettings: nil, + expectedCpuLimit: "", + }, + { + name: "CPU limit is set, LimitRange exists", + container: &corev1.Container{ + Resources: corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("250m"), + }, + }, + }, + overrideSettings: &chev2.Container{}, + limitRange: &corev1.LimitRange{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: "eclipse-che", + }, + }, + expectedCpuLimit: "250m", + }, + { + name: "Overridden CPU limit, LimitRange does not exists", + container: &corev1.Container{ + Resources: corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("250m"), + }, + }, + }, + overrideSettings: &chev2.Container{ + Resources: &chev2.ResourceRequirements{ + Limits: &chev2.ResourceList{ + Cpu: &cpuLimit500m, + }, + }, + }, + expectedCpuLimit: "500m", + }, + { + name: "Overridden CPU limit, LimitRange exists", + container: &corev1.Container{ + Resources: corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("250m"), + }, + }, + }, + overrideSettings: &chev2.Container{ + Resources: &chev2.ResourceRequirements{ + Limits: &chev2.ResourceList{ + Cpu: &cpuLimit500m, + }, + }, + }, + limitRange: &corev1.LimitRange{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: "eclipse-che", + }, + }, + expectedCpuLimit: "500m", + }, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + k8sHelper := k8shelper.New() + + if testCase.limitRange != nil { + _, err := k8sHelper.GetClientset().CoreV1().LimitRanges("eclipse-che").Create(context.TODO(), testCase.limitRange, metav1.CreateOptions{}) + assert.NoError(t, err) + } + + err := OverrideContainer("eclipse-che", testCase.container, testCase.overrideSettings) + assert.NoError(t, err) + + if testCase.expectedCpuLimit == "" { + assert.Empty(t, testCase.container.Resources.Limits[corev1.ResourceCPU]) + } else { + assert.Equal(t, testCase.expectedCpuLimit, testCase.container.Resources.Limits.Cpu().String()) + } + + defer func() { + if testCase.limitRange != nil { + err := k8sHelper.GetClientset().CoreV1().LimitRanges("eclipse-che").Delete(context.TODO(), testCase.limitRange.Name, metav1.DeleteOptions{}) + assert.NoError(t, err) + } + }() + }) + } +} diff --git a/pkg/deploy/devfileregistry/devfileregistry.go b/pkg/deploy/devfileregistry/devfileregistry.go index 66d33478f..63cc350ab 100644 --- a/pkg/deploy/devfileregistry/devfileregistry.go +++ b/pkg/deploy/devfileregistry/devfileregistry.go @@ -106,8 +106,11 @@ func (d *DevfileRegistryReconciler) updateStatus(endpoint string, ctx *chetypes. } func (d *DevfileRegistryReconciler) syncDeployment(ctx *chetypes.DeployContext) (bool, error) { - spec := d.getDevfileRegistryDeploymentSpec(ctx) - return deploy.SyncDeploymentSpecToCluster(ctx, spec, deploy.DefaultDeploymentDiffOpts) + if spec, err := d.getDevfileRegistryDeploymentSpec(ctx); err != nil { + return false, err + } else { + return deploy.SyncDeploymentSpecToCluster(ctx, spec, deploy.DefaultDeploymentDiffOpts) + } } func (d *DevfileRegistryReconciler) createGatewayConfig() *gateway.TraefikConfig { diff --git a/pkg/deploy/devfileregistry/devfileregistry_deployment.go b/pkg/deploy/devfileregistry/devfileregistry_deployment.go index 2155cd88f..37941d6b2 100644 --- a/pkg/deploy/devfileregistry/devfileregistry_deployment.go +++ b/pkg/deploy/devfileregistry/devfileregistry_deployment.go @@ -23,7 +23,7 @@ import ( "k8s.io/apimachinery/pkg/api/resource" ) -func (d *DevfileRegistryReconciler) getDevfileRegistryDeploymentSpec(ctx *chetypes.DeployContext) *appsv1.Deployment { +func (d *DevfileRegistryReconciler) getDevfileRegistryDeploymentSpec(ctx *chetypes.DeployContext) (*appsv1.Deployment, error) { registryType := "devfile" registryImage := defaults.GetDevfileRegistryImage(ctx.CheCluster) registryImagePullPolicy := v1.PullPolicy(utils.GetPullPolicyFromDockerImage(registryImage)) @@ -51,6 +51,8 @@ func (d *DevfileRegistryReconciler) getDevfileRegistryDeploymentSpec(ctx *chetyp probePath) deploy.EnsurePodSecurityStandards(deployment, constants.DefaultSecurityContextRunAsUser, constants.DefaultSecurityContextFsGroup) - deploy.CustomizeDeployment(deployment, ctx.CheCluster.Spec.Components.DevfileRegistry.Deployment) - return deployment + if err := deploy.OverrideDeployment(ctx, deployment, ctx.CheCluster.Spec.Components.DevfileRegistry.Deployment); err != nil { + return nil, err + } + return deployment, nil } diff --git a/pkg/deploy/devfileregistry/devfileregistry_deployment_test.go b/pkg/deploy/devfileregistry/devfileregistry_deployment_test.go index d6be5ba95..3ade93d04 100644 --- a/pkg/deploy/devfileregistry/devfileregistry_deployment_test.go +++ b/pkg/deploy/devfileregistry/devfileregistry_deployment_test.go @@ -14,6 +14,8 @@ package devfileregistry import ( "os" + "github.com/stretchr/testify/assert" + "k8s.io/apimachinery/pkg/api/resource" "github.com/eclipse-che/che-operator/pkg/common/constants" @@ -50,7 +52,7 @@ func TestGetDevfileRegistryDeploymentSpec(t *testing.T) { initObjects: []runtime.Object{}, memoryLimit: constants.DefaultDevfileRegistryMemoryLimit, memoryRequest: constants.DefaultDevfileRegistryMemoryRequest, - cpuLimit: constants.DefaultDevfileRegistryCpuLimit, + cpuLimit: "0", // CPU limit is not set when possible cpuRequest: constants.DefaultDevfileRegistryCpuRequest, cheCluster: &chev2.CheCluster{ ObjectMeta: metav1.ObjectMeta{ @@ -104,7 +106,8 @@ func TestGetDevfileRegistryDeploymentSpec(t *testing.T) { ctx := test.GetDeployContext(testCase.cheCluster, []runtime.Object{}) devfileregistry := NewDevfileRegistryReconciler() - deployment := devfileregistry.getDevfileRegistryDeploymentSpec(ctx) + deployment, err := devfileregistry.getDevfileRegistryDeploymentSpec(ctx) + assert.NoError(t, err) test.CompareResources(deployment, test.TestExpectedResources{ diff --git a/pkg/deploy/gateway/gateway.go b/pkg/deploy/gateway/gateway.go index 8a6bd59b0..0d4ab853b 100644 --- a/pkg/deploy/gateway/gateway.go +++ b/pkg/deploy/gateway/gateway.go @@ -142,7 +142,10 @@ func syncAll(deployContext *chetypes.DeployContext) error { return err } - depl := getGatewayDeploymentSpec(deployContext) + depl, err := getGatewayDeploymentSpec(deployContext) + if err != nil { + return err + } if _, err := deploy.Sync(deployContext, depl, deploy.DefaultDeploymentDiffOpts); err != nil { // Failed to sync (update), let's delete and create instead if strings.Contains(err.Error(), "field is immutable") { @@ -424,7 +427,7 @@ experimental: } } -func getGatewayDeploymentSpec(ctx *chetypes.DeployContext) *appsv1.Deployment { +func getGatewayDeploymentSpec(ctx *chetypes.DeployContext) (*appsv1.Deployment, error) { terminationGracePeriodSeconds := int64(10) deployLabels, labelsSelector := deploy.GetLabelsAndSelector(GatewayServiceName) @@ -462,8 +465,11 @@ func getGatewayDeploymentSpec(ctx *chetypes.DeployContext) *appsv1.Deployment { } deploy.EnsurePodSecurityStandards(deployment, constants.DefaultSecurityContextRunAsUser, constants.DefaultSecurityContextFsGroup) - deploy.CustomizeDeployment(deployment, ctx.CheCluster.Spec.Networking.Auth.Gateway.Deployment) - return deployment + if err := deploy.OverrideDeployment(ctx, deployment, ctx.CheCluster.Spec.Networking.Auth.Gateway.Deployment); err != nil { + return nil, err + } + + return deployment, nil } func getContainersSpec(ctx *chetypes.DeployContext) []corev1.Container { diff --git a/pkg/deploy/gateway/gateway_test.go b/pkg/deploy/gateway/gateway_test.go index 8e5e26e3b..45866a269 100644 --- a/pkg/deploy/gateway/gateway_test.go +++ b/pkg/deploy/gateway/gateway_test.go @@ -304,7 +304,8 @@ func TestCustomizeGatewayDeploymentAllImages(t *testing.T) { } ctx := test.GetDeployContext(checluster, []runtime.Object{}) - deployment := getGatewayDeploymentSpec(ctx) + deployment, err := getGatewayDeploymentSpec(ctx) + assert.NoError(t, err) containers := deployment.Spec.Template.Spec.Containers assert.Equal(t, constants.GatewayContainerName, containers[0].Name) assert.Equal(t, "gateway-image", containers[0].Image) @@ -344,7 +345,9 @@ func TestCustomizeGatewayDeploymentSingleImage(t *testing.T) { } ctx := test.GetDeployContext(checluster, []runtime.Object{}) - deployment := getGatewayDeploymentSpec(ctx) + deployment, err := getGatewayDeploymentSpec(ctx) + assert.NoError(t, err) + containers := deployment.Spec.Template.Spec.Containers assert.Equal(t, constants.GatewayContainerName, containers[0].Name) assert.Equal(t, "gateway-image", containers[0].Image) diff --git a/pkg/deploy/pluginregistry/pluginregistry.go b/pkg/deploy/pluginregistry/pluginregistry.go index 6b491b6a1..b940ffbd3 100644 --- a/pkg/deploy/pluginregistry/pluginregistry.go +++ b/pkg/deploy/pluginregistry/pluginregistry.go @@ -116,8 +116,11 @@ func (p *PluginRegistryReconciler) updateStatus(endpoint string, ctx *chetypes.D } func (p *PluginRegistryReconciler) syncDeployment(ctx *chetypes.DeployContext) (bool, error) { - spec := p.getPluginRegistryDeploymentSpec(ctx) - return deploy.SyncDeploymentSpecToCluster(ctx, spec, deploy.DefaultDeploymentDiffOpts) + if spec, err := p.getPluginRegistryDeploymentSpec(ctx); err != nil { + return false, err + } else { + return deploy.SyncDeploymentSpecToCluster(ctx, spec, deploy.DefaultDeploymentDiffOpts) + } } func (p *PluginRegistryReconciler) createGatewayConfig(ctx *chetypes.DeployContext) *gateway.TraefikConfig { diff --git a/pkg/deploy/pluginregistry/pluginregistry_deployment.go b/pkg/deploy/pluginregistry/pluginregistry_deployment.go index 15b4c10b7..626e112ad 100644 --- a/pkg/deploy/pluginregistry/pluginregistry_deployment.go +++ b/pkg/deploy/pluginregistry/pluginregistry_deployment.go @@ -23,7 +23,7 @@ import ( "k8s.io/apimachinery/pkg/api/resource" ) -func (p *PluginRegistryReconciler) getPluginRegistryDeploymentSpec(ctx *chetypes.DeployContext) *appsv1.Deployment { +func (p *PluginRegistryReconciler) getPluginRegistryDeploymentSpec(ctx *chetypes.DeployContext) (*appsv1.Deployment, error) { registryType := "plugin" registryImage := defaults.GetPluginRegistryImage(ctx.CheCluster) registryImagePullPolicy := corev1.PullPolicy(utils.GetPullPolicyFromDockerImage(registryImage)) @@ -62,6 +62,9 @@ func (p *PluginRegistryReconciler) getPluginRegistryDeploymentSpec(ctx *chetypes } deploy.EnsurePodSecurityStandards(deployment, constants.DefaultSecurityContextRunAsUser, constants.DefaultSecurityContextFsGroup) - deploy.CustomizeDeployment(deployment, ctx.CheCluster.Spec.Components.PluginRegistry.Deployment) - return deployment + if err := deploy.OverrideDeployment(ctx, deployment, ctx.CheCluster.Spec.Components.PluginRegistry.Deployment); err != nil { + return nil, err + } + + return deployment, nil } diff --git a/pkg/deploy/pluginregistry/pluginregistry_deployment_test.go b/pkg/deploy/pluginregistry/pluginregistry_deployment_test.go index dc3e56803..164ea73ad 100644 --- a/pkg/deploy/pluginregistry/pluginregistry_deployment_test.go +++ b/pkg/deploy/pluginregistry/pluginregistry_deployment_test.go @@ -14,6 +14,7 @@ package pluginregistry import ( "github.com/eclipse-che/che-operator/pkg/common/constants" "github.com/eclipse-che/che-operator/pkg/common/test" + "github.com/stretchr/testify/assert" "k8s.io/apimachinery/pkg/api/resource" "k8s.io/utils/pointer" @@ -46,7 +47,7 @@ func TestGetPluginRegistryDeploymentSpec(t *testing.T) { initObjects: []runtime.Object{}, memoryLimit: constants.DefaultPluginRegistryMemoryLimitEmbeddedOpenVSXRegistry, memoryRequest: constants.DefaultPluginRegistryMemoryRequestEmbeddedOpenVSXRegistry, - cpuLimit: constants.DefaultPluginRegistryCpuLimit, + cpuLimit: "0", // CPU limit is not set when possible cpuRequest: constants.DefaultPluginRegistryCpuRequest, cheCluster: &chev2.CheCluster{ ObjectMeta: metav1.ObjectMeta{ @@ -67,7 +68,7 @@ func TestGetPluginRegistryDeploymentSpec(t *testing.T) { initObjects: []runtime.Object{}, memoryLimit: constants.DefaultPluginRegistryMemoryLimit, memoryRequest: constants.DefaultPluginRegistryMemoryRequest, - cpuLimit: constants.DefaultPluginRegistryCpuLimit, + cpuLimit: "0", // CPU limit is not set when possible cpuRequest: constants.DefaultPluginRegistryCpuRequest, cheCluster: &chev2.CheCluster{ ObjectMeta: metav1.ObjectMeta{ @@ -127,7 +128,8 @@ func TestGetPluginRegistryDeploymentSpec(t *testing.T) { ctx := test.GetDeployContext(testCase.cheCluster, []runtime.Object{}) pluginregistry := NewPluginRegistryReconciler() - deployment := pluginregistry.getPluginRegistryDeploymentSpec(ctx) + deployment, err := pluginregistry.getPluginRegistryDeploymentSpec(ctx) + assert.NoError(t, err) test.CompareResources(deployment, test.TestExpectedResources{ diff --git a/pkg/deploy/server/server_deployment.go b/pkg/deploy/server/server_deployment.go index 19c65d258..d862e7b79 100644 --- a/pkg/deploy/server/server_deployment.go +++ b/pkg/deploy/server/server_deployment.go @@ -271,7 +271,9 @@ func (s CheServerReconciler) getDeploymentSpec(ctx *chetypes.DeployContext) (*ap } deploy.EnsurePodSecurityStandards(deployment, constants.DefaultSecurityContextRunAsUser, constants.DefaultSecurityContextFsGroup) - deploy.CustomizeDeployment(deployment, ctx.CheCluster.Spec.Components.CheServer.Deployment) + if err := deploy.OverrideDeployment(ctx, deployment, ctx.CheCluster.Spec.Components.CheServer.Deployment); err != nil { + return nil, err + } return deployment, nil } diff --git a/pkg/deploy/server/server_deployment_test.go b/pkg/deploy/server/server_deployment_test.go index 946bf5b02..7d4fba302 100644 --- a/pkg/deploy/server/server_deployment_test.go +++ b/pkg/deploy/server/server_deployment_test.go @@ -12,27 +12,20 @@ package server import ( - "os" - "k8s.io/apimachinery/pkg/api/resource" - "github.com/eclipse-che/che-operator/pkg/common/chetypes" "github.com/eclipse-che/che-operator/pkg/common/constants" defaults "github.com/eclipse-che/che-operator/pkg/common/operator-defaults" "github.com/eclipse-che/che-operator/pkg/common/test" "github.com/eclipse-che/che-operator/pkg/common/utils" "github.com/stretchr/testify/assert" + "testing" + chev2 "github.com/eclipse-che/che-operator/api/v2" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/client-go/kubernetes/scheme" - "sigs.k8s.io/controller-runtime/pkg/client/fake" - logf "sigs.k8s.io/controller-runtime/pkg/log" - "sigs.k8s.io/controller-runtime/pkg/log/zap" - - "testing" ) func TestDeployment(t *testing.T) { @@ -57,7 +50,7 @@ func TestDeployment(t *testing.T) { initObjects: []runtime.Object{}, memoryLimit: constants.DefaultServerMemoryLimit, memoryRequest: constants.DefaultServerMemoryRequest, - cpuLimit: constants.DefaultServerCpuLimit, + cpuLimit: "0", // no CPU limit if LimitRange does not exists cpuRequest: constants.DefaultServerCpuRequest, cheCluster: &chev2.CheCluster{ ObjectMeta: metav1.ObjectMeta{ @@ -105,18 +98,7 @@ func TestDeployment(t *testing.T) { for _, testCase := range testCases { t.Run(testCase.name, func(t *testing.T) { - logf.SetLogger(zap.New(zap.WriteTo(os.Stdout), zap.UseDevMode(true))) - chev2.SchemeBuilder.AddToScheme(scheme.Scheme) - testCase.initObjects = append(testCase.initObjects) - cli := fake.NewFakeClientWithScheme(scheme.Scheme, testCase.initObjects...) - - ctx := &chetypes.DeployContext{ - CheCluster: testCase.cheCluster, - ClusterAPI: chetypes.ClusterAPI{ - Client: cli, - Scheme: scheme.Scheme, - }, - } + ctx := test.GetDeployContext(testCase.cheCluster, testCase.initObjects) server := NewCheServerReconciler() deployment, err := server.getDeploymentSpec(ctx)