From 7bda1b173bd43d678745416c69e7197876aff332 Mon Sep 17 00:00:00 2001 From: xzfc Date: Mon, 21 Sep 2020 06:10:46 +0000 Subject: [PATCH] Put limits on containers created by admission-webhook (#2188) Signed-off-by: Albert Safin --- .circleci/config.yml | 1 + .mk/helm.mk | 3 +- .../templates/admission-webhook-secret.tpl | 2 + .../nsm/charts/admission-webhook/values.yaml | 1 + k8s/cmd/admission-webhook/const.go | 15 +++ k8s/cmd/admission-webhook/main.go | 4 + k8s/cmd/admission-webhook/mutate.go | 5 +- k8s/cmd/admission-webhook/patches.go | 107 ++++++++++-------- k8s/cmd/admission-webhook/utils.go | 20 ++++ test/kubetest/pods/common.go | 14 +++ test/kubetest/pods/coredns_injection.go | 4 + test/kubetest/pods/nsc.go | 12 ++ 12 files changed, 139 insertions(+), 49 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 083f01a7b9..402f8a5b65 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -155,6 +155,7 @@ jobs: export CONTAINER_TAG="ci_${CIRCLE_WORKFLOW_ID}_${CIRCLE_SHA1:8:8}" export REPO="networkservicemeshci" export CONTAINER_REPO=${REPO} + export ENFORCE_LIMITS="true" echo "CONTAINER_REPO=${CONTAINER_REPO}" if [ "${CIRCLE_BRANCH}" == "master" ]; then export ARTIFACTS_SAVE_ALWAYS=true diff --git a/.mk/helm.mk b/.mk/helm.mk index 4b6f0bfd89..5b6de34616 100644 --- a/.mk/helm.mk +++ b/.mk/helm.mk @@ -20,6 +20,7 @@ HELM_TIMEOUT?=300 INSECURE?=false PROMETHEUS?=true METRICS_COLLECTOR_ENABLED?=true +ENFORCE_LIMITS?=false TEST_OPTS:=--atomic \ --set org='${CONTAINER_REPO}',tag='${CONTAINER_TAG}' \ @@ -31,7 +32,7 @@ TEST_OPTS:=--atomic \ --set metricsCollectorEnabled='${METRICS_COLLECTOR_ENABLED}' \ --set global.JaegerTracing='true' \ --set spire.enabled='${SPIRE_ENABLED}',spire.org='${CONTAINER_REPO}',spire.tag='${CONTAINER_TAG}' \ - --set admission-webhook.org='${CONTAINER_REPO}',admission-webhook.tag='${CONTAINER_TAG}' \ + --set admission-webhook.org='${CONTAINER_REPO}',admission-webhook.tag='${CONTAINER_TAG}',admission-webhook.enforceLimits='${ENFORCE_LIMITS}' \ --set prefix-service.org='${CONTAINER_REPO}',prefix-service.tag='${CONTAINER_TAG}' \ --namespace '${NSM_NAMESPACE}' diff --git a/deployments/helm/nsm/charts/admission-webhook/templates/admission-webhook-secret.tpl b/deployments/helm/nsm/charts/admission-webhook/templates/admission-webhook-secret.tpl index b18dfc3c8a..bf9c48228b 100644 --- a/deployments/helm/nsm/charts/admission-webhook/templates/admission-webhook-secret.tpl +++ b/deployments/helm/nsm/charts/admission-webhook/templates/admission-webhook-secret.tpl @@ -48,6 +48,8 @@ spec: value: jaeger.nsm-system - name: JAEGER_AGENT_PORT value: "6831" + - name: ENFORCE_LIMITS + value: {{ .Values.enforceLimits | quote }} volumeMounts: - name: webhook-certs mountPath: /etc/webhook/certs diff --git a/deployments/helm/nsm/charts/admission-webhook/values.yaml b/deployments/helm/nsm/charts/admission-webhook/values.yaml index fbd7a9b9ee..db1beadafd 100644 --- a/deployments/helm/nsm/charts/admission-webhook/values.yaml +++ b/deployments/helm/nsm/charts/admission-webhook/values.yaml @@ -8,3 +8,4 @@ org: networkservicemesh tag: master pullPolicy: IfNotPresent clientNamespace: nsm-system +enforceLimits: false diff --git a/k8s/cmd/admission-webhook/const.go b/k8s/cmd/admission-webhook/const.go index 588354f53f..9a7ff01446 100644 --- a/k8s/cmd/admission-webhook/const.go +++ b/k8s/cmd/admission-webhook/const.go @@ -18,6 +18,7 @@ const ( tracerEnabledEnv = "TRACER_ENABLED" jaegerHostEnv = "JAEGER_AGENT_HOST" jaegerPortEnv = "JAEGER_AGENT_PORT" + enforceLimitsEnv = "ENFORCE_LIMITS" repoDefault = "networkservicemesh" initContainerDefault = "nsm-init" dnsInitContainerDefault = "nsm-dns-init" @@ -32,4 +33,18 @@ const ( volumePath = "/spec/volumes" containersPath = "/spec/containers" defaultPort = 443 + + // Keep in sync with ../../../test/kubetest/pods/limits.go. + // Limits for 'nsm-monitor' container. + nsmMonitorCPULimit = "100m" + nsmMonitorMemoryLimit = "15Mi" + // Limits for 'coredns' container. + corednsCPULimit = "100m" + corednsMemoryLimit = "15Mi" + // Limits for 'nsm-init' container. + nsmInitCPULimit = "500m" + nsmInitMemoryLimit = "20Mi" + // Limits for 'nsm-dns-init' container. + nsmDNSInitCPULimit = "500m" + nsmDNSInitMemoryLimit = "15Mi" ) diff --git a/k8s/cmd/admission-webhook/main.go b/k8s/cmd/admission-webhook/main.go index 2ab758b9eb..f0a8f80fc2 100644 --- a/k8s/cmd/admission-webhook/main.go +++ b/k8s/cmd/admission-webhook/main.go @@ -99,3 +99,7 @@ func getJaegerHost() string { func getJaegerPort() string { return os.Getenv(jaegerPortEnv) } + +func getEnforceLimits() bool { + return utils.EnvVar(enforceLimitsEnv).GetBooleanOrDefault(false) +} diff --git a/k8s/cmd/admission-webhook/mutate.go b/k8s/cmd/admission-webhook/mutate.go index 3f5fc7e522..77865ad1ed 100644 --- a/k8s/cmd/admission-webhook/mutate.go +++ b/k8s/cmd/admission-webhook/mutate.go @@ -27,8 +27,9 @@ func (s *nsmAdmissionWebhook) mutate(request *v1beta1.AdmissionRequest) *v1beta1 if err = checkNsmInitContainerDuplication(metaAndSpec.spec); err != nil { return errorReviewResponse(err) } - patch := createNsmInitContainerPatch(metaAndSpec.spec.InitContainers, value) - patch = append(patch, createDNSPatch(metaAndSpec, value)...) + imposeLimits := needToImposeLimits(metaAndSpec) + patch := createNsmInitContainerPatch(metaAndSpec.spec.InitContainers, value, imposeLimits) + patch = append(patch, createDNSPatch(metaAndSpec, value, imposeLimits)...) //append another patches applyDeploymentKind(patch, request.Kind.Kind) patchBytes, err := json.Marshal(patch) diff --git a/k8s/cmd/admission-webhook/patches.go b/k8s/cmd/admission-webhook/patches.go index 44f3450921..1ab4e9833b 100644 --- a/k8s/cmd/admission-webhook/patches.go +++ b/k8s/cmd/admission-webhook/patches.go @@ -15,56 +15,60 @@ type patchOperation struct { Value interface{} `json:"value,omitempty"` } -func createDNSPatch(tuple *podSpecAndMeta, annotationValue string) (patch []patchOperation) { +func createDNSPatch(tuple *podSpecAndMeta, annotationValue string, imposeLimits bool) (patch []patchOperation) { // TODO: now order of containers is important since nsmdp assign proper workspace only to the first container - patch = append(patch, addContainer(tuple.spec, - []corev1.Container{ + nsmDNSMonitorContainer := corev1.Container{ + Name: "nsm-dns-monitor", + Command: []string{"/bin/nsm-monitor"}, + Image: fmt.Sprintf("%s/%s:%s", getRepo(), "nsm-monitor", getTag()), + ImagePullPolicy: corev1.PullIfNotPresent, + Env: []corev1.EnvVar{ { - Name: "nsm-dns-monitor", - Command: []string{"/bin/nsm-monitor"}, - Image: fmt.Sprintf("%s/%s:%s", getRepo(), "nsm-monitor", getTag()), - ImagePullPolicy: corev1.PullIfNotPresent, - Env: []corev1.EnvVar{ - { - Name: "MONITOR_DNS_CONFIGS", - Value: "true", - }, - { - Name: client.AnnotationEnv, - Value: annotationValue, - }, - }, - VolumeMounts: []corev1.VolumeMount{{ - ReadOnly: false, - Name: "nsm-coredns-volume", - MountPath: "/etc/coredns", - }}, - Resources: corev1.ResourceRequirements{ - Limits: corev1.ResourceList{ - "networkservicemesh.io/socket": resource.MustParse("1"), - }, - }, + Name: "MONITOR_DNS_CONFIGS", + Value: "true", }, - })...) - patch = append(patch, addContainer(tuple.spec, - []corev1.Container{ { - Name: "coredns", - Image: "networkservicemesh/coredns:master", - ImagePullPolicy: corev1.PullIfNotPresent, - Args: []string{"-conf", "/etc/coredns/Corefile"}, - VolumeMounts: []corev1.VolumeMount{{ - ReadOnly: false, - Name: "nsm-coredns-volume", - MountPath: "/etc/coredns", - }}, - Resources: corev1.ResourceRequirements{ - Limits: corev1.ResourceList{ - "networkservicemesh.io/socket": resource.MustParse("1"), - }, - }, + Name: client.AnnotationEnv, + Value: annotationValue, + }, + }, + VolumeMounts: []corev1.VolumeMount{{ + ReadOnly: false, + Name: "nsm-coredns-volume", + MountPath: "/etc/coredns", + }}, + Resources: corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + "networkservicemesh.io/socket": resource.MustParse("1"), + }, + }, + } + + corednsContainer := corev1.Container{ + Name: "coredns", + Image: "networkservicemesh/coredns:master", + ImagePullPolicy: corev1.PullIfNotPresent, + Args: []string{"-conf", "/etc/coredns/Corefile"}, + VolumeMounts: []corev1.VolumeMount{{ + ReadOnly: false, + Name: "nsm-coredns-volume", + MountPath: "/etc/coredns", + }}, + Resources: corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + "networkservicemesh.io/socket": resource.MustParse("1"), }, - })...) + }, + } + + if imposeLimits { + applyLimits(&nsmDNSMonitorContainer, nsmMonitorCPULimit, nsmMonitorMemoryLimit) + applyLimits(&corednsContainer, corednsCPULimit, corednsMemoryLimit) + } + + patch = append(patch, addContainer(tuple.spec, []corev1.Container{nsmDNSMonitorContainer})...) + + patch = append(patch, addContainer(tuple.spec, []corev1.Container{corednsContainer})...) patch = append(patch, addVolume(tuple.spec, []corev1.Volume{{ @@ -79,7 +83,7 @@ func createDNSPatch(tuple *podSpecAndMeta, annotationValue string) (patch []patc return patch } -func createNsmInitContainerPatch(target []corev1.Container, annotationValue string) []patchOperation { +func createNsmInitContainerPatch(target []corev1.Container, annotationValue string, imposeLimits bool) []patchOperation { var patch []patchOperation namespace := getNamespace() @@ -147,6 +151,12 @@ func createNsmInitContainerPatch(target []corev1.Container, annotationValue stri MountPath: "/etc/coredns", }}, } + + if imposeLimits { + applyLimits(&nsmInitContainer, nsmInitCPULimit, nsmInitMemoryLimit) + applyLimits(&dnsNsmInitContainer, nsmDNSInitCPULimit, nsmDNSInitMemoryLimit) + } + value = append([]corev1.Container{dnsNsmInitContainer, nsmInitContainer}, target...) patch = append(patch, patchOperation{ @@ -198,3 +208,8 @@ func addContainer(spec *corev1.PodSpec, containers []corev1.Container) (patch [] return patch } + +func applyLimits(dst *corev1.Container, cpu, memory string) { + dst.Resources.Limits[corev1.ResourceCPU] = resource.MustParse(cpu) + dst.Resources.Limits[corev1.ResourceMemory] = resource.MustParse(memory) +} diff --git a/k8s/cmd/admission-webhook/utils.go b/k8s/cmd/admission-webhook/utils.go index e650a78d7e..7b25691e0d 100644 --- a/k8s/cmd/admission-webhook/utils.go +++ b/k8s/cmd/admission-webhook/utils.go @@ -14,6 +14,7 @@ import ( appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + qosv1 "k8s.io/kubernetes/pkg/apis/core/v1/helper/qos" "github.com/networkservicemesh/networkservicemesh/pkg/tools" ) @@ -127,3 +128,22 @@ func readRequest(r *http.Request) ([]byte, error) { } return body, nil } + +func needToImposeLimits(metaAndSpec *podSpecAndMeta) bool { + if getEnforceLimits() { + logrus.Infof("Running in CI, so impose limits on additional containers unconditionally") + return true + } + + pod := corev1.Pod{ + ObjectMeta: *metaAndSpec.meta, + Spec: *metaAndSpec.spec, + } + + if qosv1.GetPodQOS(&pod) == corev1.PodQOSGuaranteed { + logrus.Infof("The pod has a Guaranteed QOS class, so impose limits on additional containers") + return true + } + + return false +} diff --git a/test/kubetest/pods/common.go b/test/kubetest/pods/common.go index bc3dea6ee7..2104337ebd 100644 --- a/test/kubetest/pods/common.go +++ b/test/kubetest/pods/common.go @@ -28,6 +28,20 @@ const ( NSMRSServiceAccount = "nsmrs-acc" // ForwardPlaneServiceAccount service account for Forwarding Plane ForwardPlaneServiceAccount = "forward-plane-acc" + + // Keep in sync with ../../../k8s/cmd/admission-webhook/const.go. + // Limits for 'nsm-monitor' container. + nsmMonitorCPULimit = "100m" + nsmMonitorMemoryLimit = "15Mi" + // Limits for 'coredns' container. + corednsCPULimit = "100m" + corednsMemoryLimit = "15Mi" + // Limits for 'nsm-init' container. + nsmInitCPULimit = "500m" + nsmInitMemoryLimit = "20Mi" + // Limits for 'nsm-dns-init' container. + nsmDNSInitCPULimit = "500m" + nsmDNSInitMemoryLimit = "15Mi" ) // ForwardingPlane - Wrapper for getting a forwarding plane pod diff --git a/test/kubetest/pods/coredns_injection.go b/test/kubetest/pods/coredns_injection.go index bee1591487..635bbcacca 100644 --- a/test/kubetest/pods/coredns_injection.go +++ b/test/kubetest/pods/coredns_injection.go @@ -17,6 +17,8 @@ func InjectCorednsWithSharedFolder(template *v1.Pod) { Resources: v1.ResourceRequirements{ Limits: v1.ResourceList{ "networkservicemesh.io/socket": resource.NewQuantity(1, resource.DecimalSI).DeepCopy(), + v1.ResourceCPU: resource.MustParse(corednsCPULimit), + v1.ResourceMemory: resource.MustParse(corednsMemoryLimit), }, }, VolumeMounts: []v1.VolumeMount{{ @@ -58,6 +60,8 @@ func InjectCoredns(pod *v1.Pod, corednsConfigName string) *v1.Pod { Resources: v1.ResourceRequirements{ Limits: v1.ResourceList{ "networkservicemesh.io/socket": resource.NewQuantity(1, resource.DecimalSI).DeepCopy(), + v1.ResourceCPU: resource.MustParse(corednsCPULimit), + v1.ResourceMemory: resource.MustParse(corednsMemoryLimit), }, }, }) diff --git a/test/kubetest/pods/nsc.go b/test/kubetest/pods/nsc.go index fcd7157738..3672aba34f 100644 --- a/test/kubetest/pods/nsc.go +++ b/test/kubetest/pods/nsc.go @@ -68,6 +68,12 @@ func WrongNSCPodWebhook(name string, node *v1.Node) *v1.Pod { Name: "nsm-init", Image: "nsm-init:latest", ImagePullPolicy: v1.PullIfNotPresent, + Resources: v1.ResourceRequirements{ + Limits: v1.ResourceList{ + v1.ResourceCPU: resource.MustParse(nsmInitCPULimit), + v1.ResourceMemory: resource.MustParse(nsmInitMemoryLimit), + }, + }, }, }, }, @@ -138,6 +144,8 @@ func newDNSInitContainer(env map[string]string) v1.Container { Resources: v1.ResourceRequirements{ Limits: v1.ResourceList{ "networkservicemesh.io/socket": resource.NewQuantity(1, resource.DecimalSI).DeepCopy(), + v1.ResourceCPU: resource.MustParse(nsmDNSInitCPULimit), + v1.ResourceMemory: resource.MustParse(nsmDNSInitMemoryLimit), }, }, VolumeMounts: []v1.VolumeMount{{ @@ -164,6 +172,8 @@ func newInitContainer(env map[string]string) v1.Container { Resources: v1.ResourceRequirements{ Limits: v1.ResourceList{ "networkservicemesh.io/socket": resource.NewQuantity(1, resource.DecimalSI).DeepCopy(), + v1.ResourceCPU: resource.MustParse(nsmInitCPULimit), + v1.ResourceMemory: resource.MustParse(nsmInitMemoryLimit), }, }, }) @@ -184,6 +194,8 @@ func newMonitorContainer(env map[string]string) v1.Container { Resources: v1.ResourceRequirements{ Limits: v1.ResourceList{ "networkservicemesh.io/socket": resource.NewQuantity(1, resource.DecimalSI).DeepCopy(), + v1.ResourceCPU: resource.MustParse(nsmMonitorCPULimit), + v1.ResourceMemory: resource.MustParse(nsmMonitorMemoryLimit), }, }, })