Repository navigation
[Backport release-1.6] fix(harbor): give the nginx proxy explicit resources so it survives the tenant LimitRange - #5026
Open
europrinter (yankawai) wants to merge 3 commits into
Conversation
…he tenant LimitRange With expose.type=clusterIP the goharbor chart deploys an nginx proxy pod, but the chart set no resources for it. In a tenant namespace the LimitRange defaults any container without resources to a 128Mi memory limit, so the proxy is OOMKilled (exit 137) and Harbor never comes up. Set the nginx proxy to t1.small (512Mi) and bump the portal (also nginx-based) from t1.nano (128Mi) to t1.micro (256Mi) for the same reason. Signed-off-by: IvanHunters <[email protected]> (cherry picked from commit 5a7e2d5) Signed-off-by: Yan Bondarenko <[email protected]>
Both containers run nginx with worker_processes auto, so their idle memory follows the CPU count of the node, whatever their own CPU limit is. In a tenant namespace a container without resources gets the LimitRange default of 128Mi, which the proxy outgrows on a node with many CPUs before it serves a request. The presets that keep both out of that default have no test, so dropping them or tying them to another component's values would go unnoticed. Assisted-by: LLM Signed-off-by: Yan Bondarenko <[email protected]> (cherry picked from commit 7c06345)
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
area/release
The case that resizes the other components looked only at nginx and the portal, so it would keep passing if those overrides stopped reaching the nested release, and then it could not catch nginx or the portal following another component's values. It now checks the core, registry and jobservice limits it sets and leaves trivy alone, as nothing in the case reads it. The portal case also checks its memory request, as the nginx case does. Assisted-by: LLM Signed-off-by: Yan Bondarenko <[email protected]> (cherry picked from commit 1d8bea0)
1 of 12 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Backport of #4095 to
release-1.6, with the helm-unittest suite from #5025. All three commits are cherry-picked without conflicts and keep their cherry-pick references, and the lines changed intemplates/harbor.yamlare the same as in #4095. On this branch the nginx proxy is rendered without resources, so in a tenant namespace it gets the LimitRange default of 250m CPU and 128Mi, and the portal runs att1.nano.On a v1.6.5 cluster a Harbor created with the defaults in a tenant namespace lost both
nginxpods to OOMKilled, with 5 and 6 restarts within minutes, on nodes with many CPU threads. The proxy config hasworker_processes auto, and nginx starts one worker per CPU of the node whatever the CPU limit says. Every worker allocates its own tables forworker_connections 3096, so the idle footprint grows with the node.Reproduced with Docker Desktop on arm64 (12 vCPU, kernel 6.12, cgroup v2) and the image the chart uses,
goharbor/nginx-photon:v2.15.1with nginx 1.26.3, which is amd64 only and runs emulated here. The config is thenginx.confrendered from this branch with the upstream hosts pointed at 127.0.0.1, and a larger node is stood in for by replacingautowith a worker count.memory.oom.groupis set on the container cgroup as kubelet does on cgroup v2, and memory ismemory.currentof that cgroup once the workers are up.docker run -d --platform linux/amd64 --memory 128m --memory-swap 128m --cpus 0.25 --user 10000:10000 \ --add-host myharbor-core:127.0.0.1 --add-host myharbor-portal:127.0.0.1 \ -v "$PWD/nginx.conf:/etc/nginx/nginx.conf:ro" goharbor/nginx-photon:v2.15.1autoon this hostThe idle column is measured with a 2Gi limit. Emulation adds to every worker. nginx 1.30.4 built natively from the same Photon base, with transparent huge pages off, takes about 1.56 MiB per worker against 2.08 MiB emulated, and there 128Mi still runs 80 workers and 512Mi still runs 320, while 96 and 384 are OOMKilled. Most of the per-worker cost is the connection tables. With
worker_connections 1024instead of 3096, 96 workers take 122 MiB emulated instead of 206 MiB.Under load the cgroup also pays for kernel socket buffers. With 96 workers at 512Mi and 1 CPU, the container peaked at 386 MiB with 100 clients downloading 64 MiB blobs through
/v2/from a stub upstream, touched the limit without a kill with 200, and was OOMKilled with 300. With one worker it peaked at 171 MiB under the same 300 clients. So on large nodes the worker count still takes most of the headroom. Capping it needs a change to the vendored nginx config, which this backport does not make.helm unittest .inpackages/apps/harborpasses 21 of 21 here, against 18 of 18 onrelease-1.6, and on therelease-1.6template the three new cases fail and the 18 existing ones pass. Apart from the generated passwords,helm templateof the app chart differs fromrelease-1.6only in the nginx and portal resources. Rendering the system chart with those values gives the nginx Deployment 1 CPU and 512Mi and the portal 500m and 256Mi, so both pods roll once on upgrade. I have not run the change on a live cluster.Screenshots
Not a UI change.
Downstream repositories
Two template lines and one helm-unittest suite in
packages/apps/harbor. No values key, schema, default, README or package change, so no trigger map entry applies.Release note