Skip to content

Commit 1f74ad4

Browse files
lexfreilexfrei
authored
fix(multus): drop the CPU limit and make resources configurable (#4830)
<!-- Thank you for making a contribution! Here are some tips for you: - Use Conventional Commits for the PR title: `type(scope): description` - Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore - Scopes are not an exhaustive list — pick the most specific scope for the change and extend the list when a genuinely new area appears. Examples: - System components: dashboard, platform, operator, cilium, kube-ovn, linstor, fluxcd, cluster-api - Managed apps: postgres, mariadb, redis, kafka, clickhouse, virtual-machine, kubernetes - Development and maintenance: api, hack, tests, ci, docs, maintenance - Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or add a `BREAKING CHANGE:` footer - If it's a work in progress, consider creating this PR as a draft. - Don't hesistate to ask for opinion and review in the community chats, even if it's still a draft. - Add the label `kind/backport` if it's a bugfix that needs to be backported to a previous version. --> ## What this PR does The multus daemon was too small to recover a node after a CNI outage, and its limits could not be raised from values. The 100m CPU limit and the 1Gi memory limit of `kube-multus` were hardcoded in the patched upstream template. On a node with about 600 dead sandboxes and 220 pods waiting, multus was throttled in every CFS period. Its calls took longer than kubelet's two-minute CRI deadline, so every call was retried and the queue only grew. With the CPU limit lifted it OOM-looped at 1Gi. With 2 CPU and 4Gi it cleared the backlog in about three minutes. Now the resources come from a new `resources` value. The default keeps the same requests (100m CPU, 100Mi memory), has no CPU limit, and raises the memory limit to 4Gi. The memory limit stays on purpose. Without it the pod cgroup has no `memory.max`, and multus becomes a candidate for the Talos userspace OOM handler again. The chart now refuses to render when an override removes the memory limit or the memory request. Without a request Kubernetes uses the limit as the request, so a node with less allocatable memory than the limit would be left without multus. The change is in `patches/customize-deployment.patch`, so `make update` keeps it. I applied the patch to upstream v4.3.0 and got the committed template, except the image references. The platform reverts a direct edit of the child `cozystack.multus` Package, so a running cluster sets the value through the new `networking.multus.resources` key of the platform Package. The key is passed on only when set, and a value that is not a map stops the render. The system-defaults LimitRange in the namespace sets memory only, so it does not bring a CPU limit back. Closes #4829 ### Screenshots <!-- REQUIRED for UI changes: attach screenshots or screen recordings demonstrating the visual impact of your changes. PRs with UI changes without screenshots will not be merged. --> Not a UI change. ### Downstream repositories <!-- Cozystack is upstream for repositories that are NOT kept in sync with it automatically, and nothing in CI compares the two sides, so a change here can break them silently. Which change forces what, and which file to touch, is the trigger map in docs/agents/contributing.md under "Downstream Repositories". Walk it before you tick anything. IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM TO FILL IN: 1. Walk the trigger map against the actual diff of this PR, file by file. Do not decide from the PR title or from your memory of what you changed. 2. For every repository you tick, open the follow-up PR there and link it on the same line. A ticked box with no link claims work that does not exist, which is worse than an empty box. Search that repository for an open PR or issue covering it first, and link that instead of filing a duplicate. 3. When the follow-up is out of scope, or needs a decision that is not yours to make, open an issue in that repository instead, link that, and say so in the PR body. Do not push a speculative PR into another repository just to fill a line here. 4. Do not tick "No downstream repository is affected" to make the checklist go away. If you are unsure, leave every box empty and say so in the PR body, so a human decides. --> - [ ] No downstream repository is affected by this change - [x] [cozystack/website](https://github.com/cozystack/website) - follow-up: cozystack/website#747 - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: The diff adds a platform value, so the hand-written key table on the website needs it. The follow-up PR there is a draft until this one merges. ### Release note <!-- Write a release note: - Explain what has changed internally and for users. - Start with the same `type(scope):` prefix as in the PR title - Follow the guidelines at https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md. --> ```release-note fix(multus): the kube-multus container no longer has a CPU limit, and its memory limit is raised from 1Gi to 4Gi, so a node with hundreds of pods can recover after a CNI outage. Its resources can be overridden through the new `networking.multus.resources` platform value. An override that removes the memory limit or the memory request now fails the render. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added optional CNI plugin staging during Multus startup, configurable through chart values. Staging skips when the plugin directory is unavailable and reports individual copy failures while continuing. * Made Multus resource settings configurable, with defaults of 100m CPU and 100Mi memory requests and a 4Gi memory limit. Rendering requires memory requests and limits. * Updated the Multus deployment to use the `cozy-multus` namespace and DaemonSet name, and Cilium as the master CNI configuration. * **Bug Fixes** * Added a 128Mi memory limit to the CNI installation step. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 parents fa9f09e + ddd00f8 commit 1f74ad4

8 files changed

Lines changed: 350 additions & 44 deletions

File tree

‎packages/core/platform/templates/_helpers.tpl‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -185,6 +185,31 @@ false
185185
{{- end -}}
186186
{{- end }}
187187

188+
{{/*
189+
Values of the cozystack.multus Package. Call as (list $ <stageCniPlugins default>);
190+
emits YAML. networking.multus.resources is passed only when set, so an unset key
191+
leaves the multus chart default in charge.
192+
*/}}
193+
{{- define "cozystack.platform.multus.values" -}}
194+
{{- $root := index . 0 -}}
195+
{{- $values := dict "stageCniPlugins" (eq (include "cozystack.platform.stageCniPlugins" .) "true") -}}
196+
{{- $multus := index ($root.Values.networking | default dict) "multus" -}}
197+
{{- if kindIs "invalid" $multus -}}
198+
{{- $multus = dict -}}
199+
{{- end -}}
200+
{{- if not (kindIs "map" $multus) -}}
201+
{{- fail (printf "networking.multus: expected a map, got %s" (kindOf $multus)) -}}
202+
{{- end -}}
203+
{{- $resources := index $multus "resources" -}}
204+
{{- if not (kindIs "invalid" $resources) -}}
205+
{{- if not (kindIs "map" $resources) -}}
206+
{{- fail (printf "networking.multus.resources: expected a map, got %s" (kindOf $resources)) -}}
207+
{{- end -}}
208+
{{- $_ := set $values "resources" $resources -}}
209+
{{- end -}}
210+
{{- toYaml $values -}}
211+
{{- end }}
212+
188213
{{/*
189214
Common system packages shared between the isp-full, isp-full-generic, isp-slim and
190215
isp-slim-generic bundles.

‎packages/core/platform/templates/bundles/system.yaml‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@
5757
networking.stageCniPlugins turns it off, for nodes whose operator
5858
manages that directory. The value is always emitted rather than left to
5959
the chart default, so the rendered Package states which way it went. */ -}}
60-
{{- $multusComponents := dict "multus" (dict "values" (dict "stageCniPlugins" (eq (include "cozystack.platform.stageCniPlugins" (list $ true)) "true"))) -}}
60+
{{- $multusComponents := dict "multus" (dict "values" (include "cozystack.platform.multus.values" (list $ true) | fromYaml)) -}}
6161
{{include "cozystack.platform.package.full" (list "cozystack.multus" "default" $ $multusComponents) }}
6262
{{include "cozystack.platform.package.default" (list "cozystack.linstor" $) }}
6363
{{include "cozystack.platform.package" (list "cozystack.cozystack-scheduler" "linstor" $) }}
@@ -149,7 +149,7 @@
149149
that rewrite is the only path a later plugin version has onto an existing
150150
node, so installing only when absent would pin each node to whichever
151151
version arrived there first. */ -}}
152-
{{- $multusComponents := dict "multus" (dict "values" (dict "stageCniPlugins" (eq (include "cozystack.platform.stageCniPlugins" (list $ true)) "true"))) -}}
152+
{{- $multusComponents := dict "multus" (dict "values" (include "cozystack.platform.multus.values" (list $ true) | fromYaml)) -}}
153153
{{include "cozystack.platform.package.full" (list "cozystack.multus" "default" $ $multusComponents) }}
154154
{{- /* Pass talos.enabled: false to linstor for generic Linux */ -}}
155155
{{- $linstorComponents := dict "linstor" (dict "values" (dict "talos" (dict "enabled" false))) -}}

‎packages/core/platform/tests/bundles_multus_resources_test.yaml‎

Lines changed: 147 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,147 @@
1+
suite: networking.multus.resources bundle wiring
2+
templates:
3+
- templates/bundles/system.yaml
4+
release:
5+
name: cozystack
6+
namespace: cozy-system
7+
tests:
8+
# Unset leaves the multus chart default in charge rather than freezing a copy
9+
# of it into every Package.
10+
- it: passes no resources when the key is unset
11+
set:
12+
bundles:
13+
system:
14+
enabled: true
15+
variant: isp-full
16+
documentSelector:
17+
path: metadata.name
18+
value: cozystack.multus
19+
asserts:
20+
- notExists:
21+
path: spec.components.multus.values.resources
22+
- equal:
23+
path: spec.components.multus.values.stageCniPlugins
24+
value: true
25+
26+
- it: passes an override on talos next to stageCniPlugins
27+
set:
28+
bundles:
29+
system:
30+
enabled: true
31+
variant: isp-full
32+
networking:
33+
multus:
34+
resources:
35+
limits:
36+
memory: 8Gi
37+
documentSelector:
38+
path: metadata.name
39+
value: cozystack.multus
40+
asserts:
41+
- equal:
42+
path: spec.components.multus.values
43+
value:
44+
stageCniPlugins: true
45+
resources:
46+
limits:
47+
memory: 8Gi
48+
49+
- it: passes an override on generic Linux
50+
set:
51+
bundles:
52+
system:
53+
enabled: true
54+
variant: isp-full-generic
55+
networking:
56+
multus:
57+
resources:
58+
requests:
59+
cpu: 500m
60+
documentSelector:
61+
path: metadata.name
62+
value: cozystack.multus
63+
asserts:
64+
- equal:
65+
path: spec.components.multus.values.resources
66+
value:
67+
requests:
68+
cpu: 500m
69+
70+
- it: passes an override when multus is opted in on a slim variant
71+
values:
72+
- ../values-isp-slim-generic.yaml
73+
set:
74+
bundles:
75+
enabledPackages:
76+
- cozystack.multus
77+
networking:
78+
multus:
79+
resources:
80+
limits:
81+
memory: 8Gi
82+
documentSelector:
83+
path: metadata.name
84+
value: cozystack.multus
85+
asserts:
86+
- equal:
87+
path: spec.components.multus.values.resources.limits.memory
88+
value: 8Gi
89+
90+
- it: reads an explicit null as unset
91+
set:
92+
bundles:
93+
system:
94+
enabled: true
95+
variant: isp-full
96+
networking:
97+
multus:
98+
resources: null
99+
documentSelector:
100+
path: metadata.name
101+
value: cozystack.multus
102+
asserts:
103+
- notExists:
104+
path: spec.components.multus.values.resources
105+
106+
- it: refuses resources that are not a map
107+
set:
108+
bundles:
109+
system:
110+
enabled: true
111+
variant: isp-full
112+
networking:
113+
multus:
114+
resources: 4Gi
115+
asserts:
116+
- failedTemplate:
117+
errorMessage: 'networking.multus.resources: expected a map, got string'
118+
119+
# An uncommented `multus:` with nothing under it arrives as null.
120+
- it: reads a null multus block as unset
121+
set:
122+
bundles:
123+
system:
124+
enabled: true
125+
variant: isp-full
126+
networking:
127+
multus: null
128+
documentSelector:
129+
path: metadata.name
130+
value: cozystack.multus
131+
asserts:
132+
- notExists:
133+
path: spec.components.multus.values.resources
134+
135+
# false is the spelling most likely to mean "turn multus off", which this key
136+
# does not do, so it must not pass as unset.
137+
- it: refuses a multus block that is not a map
138+
set:
139+
bundles:
140+
system:
141+
enabled: true
142+
variant: isp-full
143+
networking:
144+
multus: false
145+
asserts:
146+
- failedTemplate:
147+
errorMessage: 'networking.multus: expected a map, got bool'

‎packages/core/platform/values.yaml‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -177,6 +177,15 @@ networking:
177177
# the child Package cozystack.multus directly is reverted by the platform
178178
# reconcile.
179179
# stageCniPlugins: true
180+
# Resources of the kube-multus container, merged over the defaults in
181+
# packages/system/multus/values.yaml, which also say why there is no CPU limit.
182+
# Unset keeps those defaults. The multus chart refuses to render without a
183+
# memory limit and a memory request. Set on a running cluster the same way as
184+
# stageCniPlugins above.
185+
# multus:
186+
# resources:
187+
# limits:
188+
# memory: 8Gi
180189

181190
# Service publishing and ingress configuration
182191
publishing:

‎packages/system/multus/patches/customize-deployment.patch‎

Lines changed: 25 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -41,43 +41,46 @@
4141
labels:
4242
tier: node
4343
app: multus
44-
@@ -157,13 +158,33 @@
44+
@@ -157,13 +158,32 @@
4545
- name: kube-multus
4646
image: ghcr.io/k8snetworkplumbingwg/multus-cni:snapshot-thick
4747
command: [ "/usr/src/multus-cni/bin/multus-daemon" ]
48-
+ # Upstream sets 50Mi for both the request and the limit. This patch
49-
+ # already raised the request to 100Mi and DELETED the memory limit
50-
+ # outright, leaving only the CPU one -- which left the pod cgroup with
51-
+ # no memory.max, and the Talos userspace OOM handler (v1.12+) only
52-
+ # ever kills cgroups that lack it. So dropping the limit to survive
53-
+ # 50Mi is what made multus an eviction candidate on every node.
48+
+ # Keep a memory limit: without one the pod cgroup has no memory.max,
49+
+ # and the Talos userspace OOM handler (v1.12+) only ever kills cgroups
50+
+ # that lack it, which makes multus an eviction candidate on every node.
5451
+ #
55-
+ # Restore a limit, sized as a backstop rather than a ceiling: 14d peak
52+
+ # Size it as a backstop rather than a ceiling: 14d peak
5653
+ # is 230Mi on a 3-node dev cluster and this grows with pods per node,
5754
+ # so upstream's 50Mi was never survivable and a fitted value would
5855
+ # trade a rare pressure-driven kill for a deterministic one.
5956
+ #
60-
+ # 1Gi rather than a tighter multiple of that peak, because the peak is
61-
+ # the part least worth trusting here: multus-daemon holds per-pod
62-
+ # state and a 3-node dev cluster understates pods per node more than
63-
+ # it understates anything else measured for this change. The scar this
64-
+ # avoids is the vertical-pod-autoscaler updater, which OOM-looped at a
65-
+ # 110Mi limit for the same reason -- it scaled with pods in the
57+
+ # Far above that peak rather than a tighter multiple of it, because the
58+
+ # peak is the part least worth trusting here: multus-daemon holds
59+
+ # per-pod state and a 3-node dev cluster understates pods per node more
60+
+ # than it understates anything else measured here. The scar
61+
+ # this avoids is the vertical-pod-autoscaler updater, which OOM-looped
62+
+ # at a 110Mi limit for the same reason -- it scaled with pods in the
6663
+ # cluster -- and was given a limit 2.5x its measured request to stop
6764
+ # it. A limit costs nothing until it is hit; only the request reserves.
65+
+ #
66+
+ # The values themselves, and why there is no CPU limit, are in
67+
+ # values.yaml.
6868
resources:
69-
requests:
70-
cpu: "100m"
69+
- requests:
70+
- cpu: "100m"
7171
- memory: "50Mi"
72-
+ memory: "100Mi"
73-
limits:
74-
cpu: "100m"
72+
- limits:
73+
- cpu: "100m"
7574
- memory: "50Mi"
76-
+ memory: "1Gi"
75+
+ {{- if not (kindIs "map" .Values.resources) }}{{- fail (printf "resources: expected a map, got %s" (kindOf .Values.resources)) }}{{- end }}
76+
+ {{- 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 }}
77+
+ {{- 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 }}
78+
+ {{- 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 }}
79+
+ {{- toYaml .Values.resources | nindent 12 }}
7780
securityContext:
7881
privileged: true
7982
terminationMessagePolicy: FallbackToLogsOnError
80-
@@ -209,10 +230,14 @@
83+
@@ -209,10 +229,14 @@
8184
- "/host/opt/cni/bin"
8285
- "-t"
8386
- "thick"
@@ -92,7 +95,7 @@
9295
securityContext:
9396
privileged: true
9497
terminationMessagePolicy: FallbackToLogsOnError
95-
@@ -220,6 +245,81 @@
98+
@@ -220,6 +244,81 @@
9699
- name: cnibin
97100
mountPath: /host/opt/cni/bin
98101
mountPropagation: Bidirectional

‎packages/system/multus/templates/multus-daemonset-thick.yml‎

Lines changed: 18 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -158,33 +158,32 @@ spec:
158158
- name: kube-multus
159159
image: ghcr.io/cozystack/cozystack/multus-cni:v1.6.0@sha256:0a92e861ead179dd6b46f475fd8878d44b3cfa40ce4d7a8ff92f790d54250d25
160160
command: [ "/usr/src/multus-cni/bin/multus-daemon" ]
161-
# Upstream sets 50Mi for both the request and the limit. This patch
162-
# already raised the request to 100Mi and DELETED the memory limit
163-
# outright, leaving only the CPU one -- which left the pod cgroup with
164-
# no memory.max, and the Talos userspace OOM handler (v1.12+) only
165-
# ever kills cgroups that lack it. So dropping the limit to survive
166-
# 50Mi is what made multus an eviction candidate on every node.
161+
# Keep a memory limit: without one the pod cgroup has no memory.max,
162+
# and the Talos userspace OOM handler (v1.12+) only ever kills cgroups
163+
# that lack it, which makes multus an eviction candidate on every node.
167164
#
168-
# Restore a limit, sized as a backstop rather than a ceiling: 14d peak
165+
# Size it as a backstop rather than a ceiling: 14d peak
169166
# is 230Mi on a 3-node dev cluster and this grows with pods per node,
170167
# so upstream's 50Mi was never survivable and a fitted value would
171168
# trade a rare pressure-driven kill for a deterministic one.
172169
#
173-
# 1Gi rather than a tighter multiple of that peak, because the peak is
174-
# the part least worth trusting here: multus-daemon holds per-pod
175-
# state and a 3-node dev cluster understates pods per node more than
176-
# it understates anything else measured for this change. The scar this
177-
# avoids is the vertical-pod-autoscaler updater, which OOM-looped at a
178-
# 110Mi limit for the same reason -- it scaled with pods in the
170+
# Far above that peak rather than a tighter multiple of it, because the
171+
# peak is the part least worth trusting here: multus-daemon holds
172+
# per-pod state and a 3-node dev cluster understates pods per node more
173+
# than it understates anything else measured here. The scar
174+
# this avoids is the vertical-pod-autoscaler updater, which OOM-looped
175+
# at a 110Mi limit for the same reason -- it scaled with pods in the
179176
# cluster -- and was given a limit 2.5x its measured request to stop
180177
# it. A limit costs nothing until it is hit; only the request reserves.
178+
#
179+
# The values themselves, and why there is no CPU limit, are in
180+
# values.yaml.
181181
resources:
182-
requests:
183-
cpu: "100m"
184-
memory: "100Mi"
185-
limits:
186-
cpu: "100m"
187-
memory: "1Gi"
182+
{{- if not (kindIs "map" .Values.resources) }}{{- fail (printf "resources: expected a map, got %s" (kindOf .Values.resources)) }}{{- end }}
183+
{{- 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 }}
184+
{{- 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 }}
185+
{{- 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 }}
186+
{{- toYaml .Values.resources | nindent 12 }}
188187
securityContext:
189188
privileged: true
190189
terminationMessagePolicy: FallbackToLogsOnError

0 commit comments

Comments
 (0)