diff --git a/packages/system/multus/patches/customize-deployment.patch b/packages/system/multus/patches/customize-deployment.patch index e4afcf707..c83a085b8 100644 --- a/packages/system/multus/patches/customize-deployment.patch +++ b/packages/system/multus/patches/customize-deployment.patch @@ -41,43 +41,46 @@ labels: tier: node app: multus -@@ -157,13 +158,33 @@ +@@ -157,13 +158,32 @@ - name: kube-multus image: ghcr.io/k8snetworkplumbingwg/multus-cni:snapshot-thick command: [ "/usr/src/multus-cni/bin/multus-daemon" ] -+ # Upstream sets 50Mi for both the request and the limit. This patch -+ # already raised the request to 100Mi and DELETED the memory limit -+ # outright, leaving only the CPU one -- which left the pod cgroup with -+ # no memory.max, and the Talos userspace OOM handler (v1.12+) only -+ # ever kills cgroups that lack it. So dropping the limit to survive -+ # 50Mi is what made multus an eviction candidate on every node. ++ # Keep a memory limit: without one the pod cgroup has no memory.max, ++ # and the Talos userspace OOM handler (v1.12+) only ever kills cgroups ++ # that lack it, which makes multus an eviction candidate on every node. + # -+ # Restore a limit, sized as a backstop rather than a ceiling: 14d peak ++ # Size it as a backstop rather than a ceiling: 14d peak + # is 230Mi on a 3-node dev cluster and this grows with pods per node, + # so upstream's 50Mi was never survivable and a fitted value would + # trade a rare pressure-driven kill for a deterministic one. + # -+ # 1Gi rather than a tighter multiple of that peak, because the peak is -+ # the part least worth trusting here: multus-daemon holds per-pod -+ # state and a 3-node dev cluster understates pods per node more than -+ # it understates anything else measured for this change. The scar this -+ # avoids is the vertical-pod-autoscaler updater, which OOM-looped at a -+ # 110Mi limit for the same reason -- it scaled with pods in the ++ # Far above that peak rather than a tighter multiple of it, because the ++ # peak is the part least worth trusting here: multus-daemon holds ++ # per-pod state and a 3-node dev cluster understates pods per node more ++ # than it understates anything else measured here. The scar ++ # this avoids is the vertical-pod-autoscaler updater, which OOM-looped ++ # at a 110Mi limit for the same reason -- it scaled with pods in the + # cluster -- and was given a limit 2.5x its measured request to stop + # it. A limit costs nothing until it is hit; only the request reserves. ++ # ++ # The values themselves, and why there is no CPU limit, are in ++ # values.yaml. resources: - requests: - cpu: "100m" +- requests: +- cpu: "100m" - memory: "50Mi" -+ memory: "100Mi" - limits: - cpu: "100m" +- limits: +- cpu: "100m" - memory: "50Mi" -+ memory: "1Gi" ++ {{- if not (kindIs "map" .Values.resources) }}{{- fail (printf "resources: expected a map, got %s" (kindOf .Values.resources)) }}{{- end }} ++ {{- range $key := list "limits" "requests" }}{{- $value := index $.Values.resources $key }}{{- if not (or (kindIs "invalid" $value) (kindIs "map" $value)) }}{{- fail (printf "resources.%s: expected a map, got %s" $key (kindOf $value)) }}{{- end }}{{- end }} ++ {{- if not (dig "limits" "memory" "" .Values.resources) }}{{- fail "resources.limits.memory must be set: without it the pod cgroup has no memory.max and the Talos OOM handler may evict multus on any node" }}{{- end }} ++ {{- if not (dig "requests" "memory" "" .Values.resources) }}{{- fail "resources.requests.memory must be set: without it the request defaults to the limit and a node with less allocatable memory is left without multus" }}{{- end }} ++ {{- toYaml .Values.resources | nindent 12 }} securityContext: privileged: true terminationMessagePolicy: FallbackToLogsOnError -@@ -209,10 +230,14 @@ +@@ -209,10 +229,14 @@ - "/host/opt/cni/bin" - "-t" - "thick" @@ -92,7 +95,7 @@ securityContext: privileged: true terminationMessagePolicy: FallbackToLogsOnError -@@ -220,6 +245,81 @@ +@@ -220,6 +244,81 @@ - name: cnibin mountPath: /host/opt/cni/bin mountPropagation: Bidirectional diff --git a/packages/system/multus/templates/multus-daemonset-thick.yml b/packages/system/multus/templates/multus-daemonset-thick.yml index 33b1a03e1..ee4a33d4a 100644 --- a/packages/system/multus/templates/multus-daemonset-thick.yml +++ b/packages/system/multus/templates/multus-daemonset-thick.yml @@ -158,33 +158,32 @@ spec: - name: kube-multus image: ghcr.io/cozystack/cozystack/multus-cni:v1.6.0@sha256:0a92e861ead179dd6b46f475fd8878d44b3cfa40ce4d7a8ff92f790d54250d25 command: [ "/usr/src/multus-cni/bin/multus-daemon" ] - # Upstream sets 50Mi for both the request and the limit. This patch - # already raised the request to 100Mi and DELETED the memory limit - # outright, leaving only the CPU one -- which left the pod cgroup with - # no memory.max, and the Talos userspace OOM handler (v1.12+) only - # ever kills cgroups that lack it. So dropping the limit to survive - # 50Mi is what made multus an eviction candidate on every node. + # Keep a memory limit: without one the pod cgroup has no memory.max, + # and the Talos userspace OOM handler (v1.12+) only ever kills cgroups + # that lack it, which makes multus an eviction candidate on every node. # - # Restore a limit, sized as a backstop rather than a ceiling: 14d peak + # Size it as a backstop rather than a ceiling: 14d peak # is 230Mi on a 3-node dev cluster and this grows with pods per node, # so upstream's 50Mi was never survivable and a fitted value would # trade a rare pressure-driven kill for a deterministic one. # - # 1Gi rather than a tighter multiple of that peak, because the peak is - # the part least worth trusting here: multus-daemon holds per-pod - # state and a 3-node dev cluster understates pods per node more than - # it understates anything else measured for this change. The scar this - # avoids is the vertical-pod-autoscaler updater, which OOM-looped at a - # 110Mi limit for the same reason -- it scaled with pods in the + # Far above that peak rather than a tighter multiple of it, because the + # peak is the part least worth trusting here: multus-daemon holds + # per-pod state and a 3-node dev cluster understates pods per node more + # than it understates anything else measured here. The scar + # this avoids is the vertical-pod-autoscaler updater, which OOM-looped + # at a 110Mi limit for the same reason -- it scaled with pods in the # cluster -- and was given a limit 2.5x its measured request to stop # it. A limit costs nothing until it is hit; only the request reserves. + # + # The values themselves, and why there is no CPU limit, are in + # values.yaml. resources: - requests: - cpu: "100m" - memory: "100Mi" - limits: - cpu: "100m" - memory: "1Gi" + {{- if not (kindIs "map" .Values.resources) }}{{- fail (printf "resources: expected a map, got %s" (kindOf .Values.resources)) }}{{- end }} + {{- range $key := list "limits" "requests" }}{{- $value := index $.Values.resources $key }}{{- if not (or (kindIs "invalid" $value) (kindIs "map" $value)) }}{{- fail (printf "resources.%s: expected a map, got %s" $key (kindOf $value)) }}{{- end }}{{- end }} + {{- if not (dig "limits" "memory" "" .Values.resources) }}{{- fail "resources.limits.memory must be set: without it the pod cgroup has no memory.max and the Talos OOM handler may evict multus on any node" }}{{- end }} + {{- if not (dig "requests" "memory" "" .Values.resources) }}{{- fail "resources.requests.memory must be set: without it the request defaults to the limit and a node with less allocatable memory is left without multus" }}{{- end }} + {{- toYaml .Values.resources | nindent 12 }} securityContext: privileged: true terminationMessagePolicy: FallbackToLogsOnError diff --git a/packages/system/multus/tests/multus_test.yaml b/packages/system/multus/tests/multus_test.yaml index 2855daadc..e3ceaed35 100644 --- a/packages/system/multus/tests/multus_test.yaml +++ b/packages/system/multus/tests/multus_test.yaml @@ -35,7 +35,7 @@ tests: asserts: - equal: path: spec.template.spec.containers[?(@.name=="kube-multus")].resources.limits.memory - value: 1Gi + value: 4Gi - equal: path: spec.template.spec.containers[?(@.name=="kube-multus")].resources.requests.memory value: 100Mi @@ -43,6 +43,110 @@ tests: path: spec.template.spec.initContainers[?(@.name=="install-multus-binary")].resources.limits.memory value: 128Mi + # Every pod sandbox on the node goes through kube-multus, so a CPU limit + # throttles the whole node's CNI queue: after a CNI outage the backlog + # outgrows kubelet's retry interval and never drains. + - it: kube-multus has a CPU request but no CPU limit + template: templates/multus-daemonset-thick.yml + documentSelector: + path: kind + value: DaemonSet + asserts: + - equal: + path: spec.template.spec.containers[?(@.name=="kube-multus")].resources.requests.cpu + value: 100m + - notExists: + path: spec.template.spec.containers[?(@.name=="kube-multus")].resources.limits.cpu + + - it: kube-multus resources are overridable from values + template: templates/multus-daemonset-thick.yml + documentSelector: + path: kind + value: DaemonSet + set: + resources: + limits: + cpu: "2" + memory: 8Gi + asserts: + - equal: + path: spec.template.spec.containers[?(@.name=="kube-multus")].resources + value: + requests: + cpu: 100m + memory: 100Mi + limits: + cpu: "2" + memory: 8Gi + + # Deep-merge keeps both memory keys for every partial override; only an + # explicit null drops them. Every case below must stop the render. + - it: refuses to render without a memory limit + set: + resources: + limits: + memory: null + asserts: + - failedTemplate: + errorMessage: 'resources.limits.memory must be set: without it the pod cgroup has no memory.max and the Talos OOM handler may evict multus on any node' + + - it: refuses to render with resources.limits cleared + set: + resources: + limits: null + asserts: + - failedTemplate: + errorMessage: 'resources.limits.memory must be set: without it the pod cgroup has no memory.max and the Talos OOM handler may evict multus on any node' + + - it: refuses to render with resources cleared + set: + resources: null + asserts: + - failedTemplate: + errorMessage: 'resources: expected a map, got invalid' + + - it: refuses to render with resources that are not a map + set: + resources: big + asserts: + - failedTemplate: + errorMessage: 'resources: expected a map, got string' + + - it: refuses resources.limits that are not a map + set: + resources: + limits: 4Gi + asserts: + - failedTemplate: + errorMessage: 'resources.limits: expected a map, got string' + + - it: refuses resources.requests that are not a map + set: + resources: + requests: false + asserts: + - failedTemplate: + errorMessage: 'resources.requests: expected a map, got bool' + + # An absent request defaults to the limit, so every node would reserve 4Gi + # and one with less allocatable memory would be left without multus. + - it: refuses to render without a memory request + set: + resources: + requests: + memory: null + asserts: + - failedTemplate: + errorMessage: 'resources.requests.memory must be set: without it the request defaults to the limit and a node with less allocatable memory is left without multus' + + - it: refuses to render with resources.requests cleared + set: + resources: + requests: null + asserts: + - failedTemplate: + errorMessage: 'resources.requests.memory must be set: without it the request defaults to the limit and a node with less allocatable memory is left without multus' + # Guards the cozystack CNI-ordering fix (multusMasterCNI pinned to the cilium # conflist) so multus does not auto-detect kube-ovn ahead of cilium at node # startup and bypass the cilium CNI chain. This is a hand-edit that lives in diff --git a/packages/system/multus/values.yaml b/packages/system/multus/values.yaml index 14d015aa4..e1eeb25c1 100644 --- a/packages/system/multus/values.yaml +++ b/packages/system/multus/values.yaml @@ -6,3 +6,19 @@ ## render rather than resolving to off. ## stageCniPlugins: false + +## @param resources resources of the kube-multus container +## Every pod sandbox on the node waits on this container, which is why there is no CPU limit: +## after a CNI outage left ~600 dead sandboxes for 220 pods on one node, multus at 100m answered +## slower than kubelet's two-minute CRI deadline, every call was retried and the queue only grew. +## With the CPU limit lifted it OOM-looped at 1Gi and cleared the backlog in minutes at 4Gi. +## +## The render fails without limits.memory or requests.memory: the comment above `resources` in +## templates/multus-daemonset-thick.yml explains why a memory limit is kept at any size. +## +resources: + requests: + cpu: 100m + memory: 100Mi + limits: + memory: 4Gi