Put limits on containers created by admission-webhook (#2188)

Signed-off-by: Albert Safin <albert.safin@xored.com>
networkservicemesh · Sep 21, 2020 · 7bda1b1 · 7bda1b1
1 parent 8f0e4e2
commit 7bda1b1
Show file tree

Hide file tree

Showing 12 changed files with 139 additions and 49 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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),
 			},
 		},
 	})