From 9496499ed39f250b4f851087cef3122c43fa85f3 Mon Sep 17 00:00:00 2001 From: druppelt <44848632+druppelt@users.noreply.github.com> Date: Sat, 14 Sep 2024 16:36:06 +0200 Subject: [PATCH] fix: only include initContainer for rolling out pods For normally running pods init containers can be ignored. During init phase of a pod, so before it becomes ready, either the init or normal containers are running. Use the highest value of both for each resource. Use these `MaxResources` for all non-ready pods. Refs: #17 --- internal/calc/calc.go | 59 ++++++++++--- internal/calc/calc_test.go | 135 +++++++++++++++++++++++++++++- internal/calc/cronjob.go | 4 +- internal/calc/daemonset.go | 4 +- internal/calc/deployment.go | 16 ++-- internal/calc/deploymentConfig.go | 17 ++-- internal/calc/deployment_test.go | 14 ++-- internal/calc/job.go | 4 +- internal/calc/pod.go | 4 +- internal/calc/pod_test.go | 35 +++++++- internal/calc/statefulset.go | 60 +++++++++++-- 11 files changed, 305 insertions(+), 47 deletions(-) diff --git a/internal/calc/calc.go b/internal/calc/calc.go index 942710d..1823f0a 100644 --- a/internal/calc/calc.go +++ b/internal/calc/calc.go @@ -69,6 +69,15 @@ type Resources struct { MemoryMax resource.Quantity } +// PodResources contain the sum of the resources required by the initContainer, the normal containers +// and the maximum the pod can require at any time for each resource quantity. +// In other words, max(Containers.MinCPU, InitContainers.MinCPU), max(Containers.MaxCPU, InitContainers.MaxCPU), etc. +type PodResources struct { + Containers Resources + InitContainers Resources + MaxResources Resources +} + // ConvertToResources converts a kubernetes/openshift ResourceRequirements struct to a Resources struct func ConvertToResources(req *v1.ResourceRequirements) Resources { return Resources{ @@ -89,6 +98,11 @@ func (r Resources) Add(y Resources) Resources { return r } +// MulInt32 multiplies all resource values by the given multiplier. +func (r Resources) MulInt32(y int32) Resources { + return r.Mul(float64(y)) +} + // Mul multiplies all resource values by the given multiplier. func (r Resources) Mul(y float64) Resources { // TODO check if overflow issues due to milli instead of value are to be expected @@ -100,33 +114,47 @@ func (r Resources) Mul(y float64) Resources { return r } -func podResources(podSpec *v1.PodSpec) (r *Resources) { - r = new(Resources) +func calcPodResources(podSpec *v1.PodSpec) (r *PodResources) { + r = new(PodResources) for i := range podSpec.Containers { container := podSpec.Containers[i] - r.CPUMin.Add(*container.Resources.Requests.Cpu()) - r.CPUMax.Add(*container.Resources.Limits.Cpu()) - r.MemoryMin.Add(*container.Resources.Requests.Memory()) - r.MemoryMax.Add(*container.Resources.Limits.Memory()) + r.Containers.CPUMin.Add(*container.Resources.Requests.Cpu()) + r.Containers.CPUMax.Add(*container.Resources.Limits.Cpu()) + r.Containers.MemoryMin.Add(*container.Resources.Requests.Memory()) + r.Containers.MemoryMax.Add(*container.Resources.Limits.Memory()) } for i := range podSpec.InitContainers { container := podSpec.InitContainers[i] - r.CPUMin.Add(*container.Resources.Requests.Cpu()) - r.CPUMax.Add(*container.Resources.Limits.Cpu()) - r.MemoryMin.Add(*container.Resources.Requests.Memory()) - r.MemoryMax.Add(*container.Resources.Limits.Memory()) + r.InitContainers.CPUMin.Add(*container.Resources.Requests.Cpu()) + r.InitContainers.CPUMax.Add(*container.Resources.Limits.Cpu()) + r.InitContainers.MemoryMin.Add(*container.Resources.Requests.Memory()) + r.InitContainers.MemoryMax.Add(*container.Resources.Limits.Memory()) } + r.MaxResources.CPUMin = maxQuantity(r.Containers.CPUMin, r.InitContainers.CPUMin) + r.MaxResources.CPUMax = maxQuantity(r.Containers.CPUMax, r.InitContainers.CPUMax) + r.MaxResources.MemoryMin = maxQuantity(r.Containers.MemoryMin, r.InitContainers.MemoryMin) + r.MaxResources.MemoryMax = maxQuantity(r.Containers.MemoryMax, r.InitContainers.MemoryMax) + return } +func maxQuantity(q1, q2 resource.Quantity) resource.Quantity { + if q1.MilliValue() > q2.MilliValue() { + return q1 + } + + return q2 +} + // ResourceQuotaFromYaml decodes a single yaml document into a k8s object. Then performs a type assertion // on the object and calculates the resource needs of it. // Currently supported: +// * apps.openshift.io/v1 - DeploymentConfig // * apps/v1 - Deployment // * apps/v1 - StatefulSet // * apps/v1 - DaemonSet @@ -189,7 +217,16 @@ func ResourceQuotaFromYaml(yamlData []byte) (*ResourceUsage, error) { return usage, nil case *appsv1.StatefulSet: - return statefulSet(*obj), nil + usage, err := statefulSet(*obj) + if err != nil { + return nil, CalculationError{ + Version: gvk.Version, + Kind: gvk.Kind, + err: err, + } + } + + return usage, nil case *appsv1.DaemonSet: return daemonSet(*obj), nil case *batchV1.Job: diff --git a/internal/calc/calc_test.go b/internal/calc/calc_test.go index e4a22df..8caf253 100644 --- a/internal/calc/calc_test.go +++ b/internal/calc/calc_test.go @@ -2,9 +2,10 @@ package calc import ( "errors" - "k8s.io/apimachinery/pkg/api/resource" "testing" + "k8s.io/apimachinery/pkg/api/resource" + "github.com/stretchr/testify/require" ) @@ -549,6 +550,138 @@ spec: memory: 2Gi terminationGracePeriodSeconds: 30` +var multiContainerPod = ` +--- +apiVersion: v1 +kind: Pod +metadata: + labels: + app: mypod + name: mypod +spec: + containers: + - image: mypod + imagePullPolicy: Always + name: myapp + resources: + limits: + cpu: "1" + memory: 4Gi + requests: + cpu: 250m + memory: 2Gi + - image: mypod2 + imagePullPolicy: Always + name: myapp2 + resources: + limits: + cpu: "750m" + memory: 3Gi + requests: + cpu: 150m + memory: 1Gi + terminationGracePeriodSeconds: 30` + +var initContainerPod = ` +--- +apiVersion: v1 +kind: Pod +metadata: + labels: + app: mypod + name: mypod +spec: + initContainers: + - image: mypod + imagePullPolicy: Always + name: myapp + resources: + limits: + cpu: "500m" + memory: 1Gi + requests: + cpu: 250m + memory: 1Gi + containers: + - image: mypod + imagePullPolicy: Always + name: myapp + resources: + limits: + cpu: "1" + memory: 4Gi + requests: + cpu: 250m + memory: 2Gi + terminationGracePeriodSeconds: 30` + +var bigInitContainerPod = ` +--- +apiVersion: v1 +kind: Pod +metadata: + labels: + app: mypod + name: mypod +spec: + initContainers: + - image: mypod + imagePullPolicy: Always + name: myapp + resources: + limits: + cpu: "2" + memory: 5Gi + requests: + cpu: 1 + memory: 3Gi + containers: + - image: mypod + imagePullPolicy: Always + name: myapp + resources: + limits: + cpu: "1" + memory: 4Gi + requests: + cpu: 250m + memory: 2Gi + terminationGracePeriodSeconds: 30` + +// the idea here is that for some resources init is bigger and for other the normal container is bigger +var mediumInitContainerPod = ` +--- +apiVersion: v1 +kind: Pod +metadata: + labels: + app: mypod + name: mypod +spec: + initContainers: + - image: mypod + imagePullPolicy: Always + name: myapp + resources: + limits: + cpu: "2" + memory: 3Gi + requests: + cpu: 100m + memory: 3Gi + containers: + - image: mypod + imagePullPolicy: Always + name: myapp + resources: + limits: + cpu: "1" + memory: 4Gi + requests: + cpu: 250m + memory: 2Gi + terminationGracePeriodSeconds: 30` + var normalDaemonSet = ` apiVersion: apps/v1 kind: DaemonSet diff --git a/internal/calc/cronjob.go b/internal/calc/cronjob.go index 349c086..2287a0a 100644 --- a/internal/calc/cronjob.go +++ b/internal/calc/cronjob.go @@ -3,10 +3,10 @@ package calc import batchV1 "k8s.io/api/batch/v1" func cronjob(cronjob batchV1.CronJob) *ResourceUsage { - podResources := podResources(&cronjob.Spec.JobTemplate.Spec.Template.Spec) + podResources := calcPodResources(&cronjob.Spec.JobTemplate.Spec.Template.Spec).MaxResources resourceUsage := ResourceUsage{ - Resources: *podResources, + Resources: podResources, Details: Details{ Version: cronjob.APIVersion, Kind: cronjob.Kind, diff --git a/internal/calc/daemonset.go b/internal/calc/daemonset.go index 08e38ee..9f3c3e0 100644 --- a/internal/calc/daemonset.go +++ b/internal/calc/daemonset.go @@ -5,10 +5,10 @@ import ( ) func daemonSet(dSet appsv1.DaemonSet) *ResourceUsage { - podResources := podResources(&dSet.Spec.Template.Spec) + podResources := calcPodResources(&dSet.Spec.Template.Spec).MaxResources resourceUsage := ResourceUsage{ - Resources: *podResources, + Resources: podResources, Details: Details{ Version: dSet.APIVersion, Kind: dSet.Kind, diff --git a/internal/calc/deployment.go b/internal/calc/deployment.go index 5c26144..25f8a4f 100644 --- a/internal/calc/deployment.go +++ b/internal/calc/deployment.go @@ -13,8 +13,11 @@ import ( // strategy are taken into account. func deployment(deployment appsv1.Deployment) (*ResourceUsage, error) { //nolint:funlen // disable function length linting var ( - maxUnavailable int32 // max amount of unavailable pods during a deployment - maxSurge int32 // max amount of pods that are allowed in addition to replicas during deployment + maxUnavailable int32 // max amount of unavailable pods during a deployment + maxSurge int32 // max amount of pods that are allowed in addition to replicas during deployment + maxNonReadyPodCount int32 // max pods that are not ready during deployment, + // so either running init containers or already running normal containers, + // but probes haven't succeeded yet ) replicas := deployment.Spec.Replicas @@ -37,6 +40,7 @@ func deployment(deployment appsv1.Deployment) (*ResourceUsage, error) { //nolint switch strategy.Type { case appsv1.RecreateDeploymentStrategyType: // kill all existing pods, then recreate new ones at once -> no overhead on recreate + maxNonReadyPodCount = *replicas maxUnavailable = *replicas maxSurge = 0 case "": @@ -93,14 +97,14 @@ func deployment(deployment appsv1.Deployment) (*ResourceUsage, error) { //nolint maxSurge = int32(maxSurgeInt) + // maxNonReadyPodCount is the max number of pods potentially in init phase during a deployment + maxNonReadyPodCount = maxSurge + maxUnavailable default: return nil, fmt.Errorf("deployment: %s deployment strategy %q is unknown", deployment.Name, strategy.Type) } - _ = maxUnavailable // fix complaining compiler. will need this field in the future - - podResources := podResources(&deployment.Spec.Template.Spec) - newResources := podResources.Mul(float64(*replicas + maxSurge)) + podResources := calcPodResources(&deployment.Spec.Template.Spec) + newResources := podResources.Containers.MulInt32(*replicas - maxUnavailable).Add(podResources.MaxResources.MulInt32(maxNonReadyPodCount)) resourceUsage := ResourceUsage{ Resources: newResources, diff --git a/internal/calc/deploymentConfig.go b/internal/calc/deploymentConfig.go index d9cea28..1e50dee 100644 --- a/internal/calc/deploymentConfig.go +++ b/internal/calc/deploymentConfig.go @@ -13,8 +13,11 @@ import ( // strategy are taken into account. func deploymentConfig(deploymentConfig openshiftAppsV1.DeploymentConfig) (*ResourceUsage, error) { //nolint:funlen // disable function length linting var ( - maxUnavailable int32 // max amount of unavailable pods during a deployment - maxSurge int32 // max amount of pods that are allowed in addition to replicas during deployment + maxUnavailable int32 // max amount of unavailable pods during a deployment + maxSurge int32 // max amount of pods that are allowed in addition to replicas during deployment + maxNonReadyPodCount int32 // max pods that are not ready during deployment, + // so either running init containers or already running normal containers, + // but probes haven't succeeded yet ) replicas := deploymentConfig.Spec.Replicas @@ -37,6 +40,7 @@ func deploymentConfig(deploymentConfig openshiftAppsV1.DeploymentConfig) (*Resou switch strategy.Type { case openshiftAppsV1.DeploymentStrategyTypeRecreate: // kill all existing pods, then recreate new ones at once -> no overhead on recreate + maxNonReadyPodCount = replicas maxUnavailable = replicas maxSurge = 0 case "": @@ -92,15 +96,16 @@ func deploymentConfig(deploymentConfig openshiftAppsV1.DeploymentConfig) (*Resou } maxSurge = int32(maxSurgeInt) + + // maxNonReadyPodCount is the max number of pods potentially in init phase during a deployment + maxNonReadyPodCount = maxSurge + maxUnavailable default: return nil, fmt.Errorf("deploymentConfig: %s deploymentConfig strategy %q is unknown", deploymentConfig.Name, strategy.Type) } - _ = maxUnavailable // fix complaining compiler. will need this field in the future - - podResources := podResources(&deploymentConfig.Spec.Template.Spec) + podResources := calcPodResources(&deploymentConfig.Spec.Template.Spec) strategyResources := ConvertToResources(&deploymentConfig.Spec.Strategy.Resources) - newResources := podResources.Mul(float64(replicas + maxSurge)).Add(strategyResources) + newResources := podResources.Containers.MulInt32(replicas - maxUnavailable).Add(podResources.MaxResources.MulInt32(maxNonReadyPodCount)).Add(strategyResources) resourceUsage := ResourceUsage{ Resources: newResources, diff --git a/internal/calc/deployment_test.go b/internal/calc/deployment_test.go index 5f666e7..4f45db2 100644 --- a/internal/calc/deployment_test.go +++ b/internal/calc/deployment_test.go @@ -87,14 +87,12 @@ func TestDeployment(t *testing.T) { strategy: appsv1.RollingUpdateDeploymentStrategyType, }, { - name: "deployment with init container(s)", - deployment: initContainerDeployment, - // TODO the expectation shouldn't be for the init container resources to be considered for all containers, - // just the amount that can be in non-ready state simultaneously - cpuMin: resource.MustParse("1200m"), - cpuMax: resource.MustParse("4400m"), - memoryMin: resource.MustParse("8592Mi"), - memoryMax: resource.MustParse("17184Mi"), + name: "deployment with init container(s)", + deployment: initContainerDeployment, + cpuMin: resource.MustParse("1000m"), + cpuMax: resource.MustParse("4000m"), + memoryMin: resource.MustParse("8Gi"), + memoryMax: resource.MustParse("16Gi"), replicas: 3, maxReplicas: 4, strategy: appsv1.RollingUpdateDeploymentStrategyType, diff --git a/internal/calc/job.go b/internal/calc/job.go index 714c256..378e1cd 100644 --- a/internal/calc/job.go +++ b/internal/calc/job.go @@ -3,10 +3,10 @@ package calc import batchV1 "k8s.io/api/batch/v1" func job(job batchV1.Job) *ResourceUsage { - podResources := podResources(&job.Spec.Template.Spec) + podResources := calcPodResources(&job.Spec.Template.Spec).MaxResources resourceUsage := ResourceUsage{ - Resources: *podResources, + Resources: podResources, Details: Details{ Version: job.APIVersion, Kind: job.Kind, diff --git a/internal/calc/pod.go b/internal/calc/pod.go index 18f1630..79f86ac 100644 --- a/internal/calc/pod.go +++ b/internal/calc/pod.go @@ -3,10 +3,10 @@ package calc import v1 "k8s.io/api/core/v1" func pod(pod v1.Pod) *ResourceUsage { - podResources := podResources(&pod.Spec) + podResources := calcPodResources(&pod.Spec).MaxResources resourceUsage := ResourceUsage{ - Resources: *podResources, + Resources: podResources, Details: Details{ Version: pod.APIVersion, Kind: pod.Kind, diff --git a/internal/calc/pod_test.go b/internal/calc/pod_test.go index 1839fbe..43ac76c 100644 --- a/internal/calc/pod_test.go +++ b/internal/calc/pod_test.go @@ -21,13 +21,46 @@ func TestPod(t *testing.T) { strategy appsv1.StatefulSetUpdateStrategyType }{ { - name: "ok", + name: "normal pod", pod: normalPod, cpuMin: resource.MustParse("250m"), cpuMax: resource.MustParse("1"), memoryMin: resource.MustParse("2Gi"), memoryMax: resource.MustParse("4Gi"), }, + { + name: "pod with multiple containers", + pod: multiContainerPod, + cpuMin: resource.MustParse("400m"), + cpuMax: resource.MustParse("1750m"), + memoryMin: resource.MustParse("3Gi"), + memoryMax: resource.MustParse("7Gi"), + }, + { + name: "pod with small init container", + pod: initContainerPod, + cpuMin: resource.MustParse("250m"), + cpuMax: resource.MustParse("1"), + memoryMin: resource.MustParse("2Gi"), + memoryMax: resource.MustParse("4Gi"), + }, + { + name: "pod with big init container", + pod: bigInitContainerPod, + cpuMin: resource.MustParse("1"), + cpuMax: resource.MustParse("2"), + memoryMin: resource.MustParse("3Gi"), + memoryMax: resource.MustParse("5Gi"), + }, + { + // This testcase is for taking the max of init and normal containers for each resource + name: "pod with a similar sized init container to the normal containers", + pod: mediumInitContainerPod, + cpuMin: resource.MustParse("250m"), + cpuMax: resource.MustParse("2"), + memoryMin: resource.MustParse("3Gi"), + memoryMax: resource.MustParse("4Gi"), + }, } for _, test := range tests { diff --git a/internal/calc/statefulset.go b/internal/calc/statefulset.go index 2dab41b..59b104e 100644 --- a/internal/calc/statefulset.go +++ b/internal/calc/statefulset.go @@ -1,15 +1,22 @@ package calc import ( + "errors" + "math" + appsv1 "k8s.io/api/apps/v1" + "k8s.io/apimachinery/pkg/util/intstr" ) // calculates the cpu/memory resources a single statefulset needs. Replicas are taken into account. -func statefulSet(s appsv1.StatefulSet) *ResourceUsage { +func statefulSet(s appsv1.StatefulSet) (*ResourceUsage, error) { var ( - replicas int32 + replicas int32 + maxUnavailable int32 ) + strategy := s.Spec.UpdateStrategy + // https://github.com/kubernetes/api/blob/v0.18.4/apps/v1/types.go#L117 if s.Spec.Replicas != nil { replicas = *s.Spec.Replicas @@ -17,8 +24,49 @@ func statefulSet(s appsv1.StatefulSet) *ResourceUsage { replicas = 1 } - podResources := podResources(&s.Spec.Template.Spec) - newResources := podResources.Mul(float64(replicas)) + // https://kubernetes.io/docs/concepts/workloads/controllers/statefulset/#update-strategies + switch strategy.Type { + case appsv1.OnDeleteStatefulSetStrategyType: + // OnDelete doesn't do anything until you kill pods, which it then replaces with the newer ones. + // The most expensive case would be killing all pods at once, with the init containers being more expensive than the normal container. + maxUnavailable = replicas + case "": + // RollingUpdate is the default and can be an empty string. If so, set the defaults and continue calculation. + defaultMaxUnavailable := intstr.FromInt32(1) + strategy = appsv1.StatefulSetUpdateStrategy{ + Type: appsv1.RollingUpdateStatefulSetStrategyType, + RollingUpdate: &appsv1.RollingUpdateStatefulSetStrategy{ + MaxUnavailable: &defaultMaxUnavailable, + }, + } + + fallthrough + case appsv1.RollingUpdateStatefulSetStrategyType: + // RollingUpdate updates each Pod one at a time. It waits until an updated Pod is Running and Ready before continuing with the next pod. + // There is an alpha feature to support rollout of multiple pods at once with `.spec.updateStrategy.rollingUpdate.maxUnavailable` + var maxUnavailableValue intstr.IntOrString + + if strategy.RollingUpdate == nil { + maxUnavailableValue = intstr.FromInt32(1) + } else { + maxUnavailableValue = *strategy.RollingUpdate.MaxUnavailable + } + + // docs say, that the absolute number is calculated by rounding up. + maxUnavailableInt, err := intstr.GetScaledValueFromIntOrPercent(&maxUnavailableValue, int(replicas), true) + if err != nil { + return nil, err + } + + if maxUnavailableInt < math.MinInt32 || maxUnavailableInt > math.MaxInt32 { + return nil, errors.New("maxUnavailableInt out of int32 boundaries") + } + + maxUnavailable = int32(maxUnavailableInt) + } + + podResources := calcPodResources(&s.Spec.Template.Spec) + newResources := podResources.Containers.MulInt32(replicas - maxUnavailable).Add(podResources.MaxResources.MulInt32(maxUnavailable)) resourceUsage := ResourceUsage{ Resources: newResources, @@ -27,10 +75,10 @@ func statefulSet(s appsv1.StatefulSet) *ResourceUsage { Kind: s.Kind, Name: s.Name, Replicas: replicas, - Strategy: string(s.Spec.UpdateStrategy.Type), + Strategy: string(strategy.Type), MaxReplicas: replicas, }, } - return &resourceUsage + return &resourceUsage, nil }