Repository navigation
fix(harbor): guarantee CPU for redis sentinels - #3446
Merged
Aleksei Sviridkin (lexfrei) merged 3 commits intoJul 28, 2026
Merged
Conversation
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
github-actions
Bot
added
size/M
dosubot
Bot
added
the
area/testing
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughHarbor Redis Sentinel resources are increased and covered by a new test. Harbor failure handling now collects RedisFailover diagnostics and allows a longer readiness wait. Previous-container log collection behavior is clarified in documentation and comments. ChangesHarbor Redis reliability
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
myasnikovdaniil
approved these changes
Jul 27, 2026
myasnikovdaniil
left a comment
myasnikovdaniil
left a comment
Contributor
There was a problem hiding this comment.
LGTM — every claim checks out.
Business context: Harbor's bundled Redis Sentinels stay NotReady until the operator elects a master; on CPU-contended runners that convergence tail exceeded the 6m assert and timed out the harbor-test HelmRelease.
- The sentinel resource bump to requests == limits (250m CPU / 128Mi) produces Guaranteed QoS, which is what buys the scheduling shares and eviction priority the master election needs under contention.
- The new
tests/redis_test.yamlactually runs in CI —packages/system/harbor/Makefilehas atest: helm unittest .target — and passes (8 tests).documentIndex: 1is correct:redis.yamlrenders the Secret as document 0 and the RedisFailover as document 1. - The
podLogscapability correction (three copies) is accurate, and the 6m→10m wait matches the siblingharbor-test-systembudget and the documented longer-wait allowance.
No blockers. As the PR body notes, the sizing is mechanism-backed (CFS shares scale with requests) rather than metric-proven, and it costs an extra 600m CPU / 192Mi requested per Harbor instance; the new failure-path diagnostics (RedisFailover describe, operator and rfr-/rfs- pod logs) are the right hedge if the flake recurs.
The inline RedisFailover ran its three sentinels at 50m CPU request and a 100m limit. Sentinels only become Ready after the redis-operator elects a master and rewrites their monitored-master config, and on a CPU-contended node CFS shares cycles proportionally to the request, so underprovisioned sentinels converge slowly and delay harbor readiness behind them. Raise the sentinels to Guaranteed QoS (requests equal limits, 250m CPU / 128Mi) so election is never starved or throttled under load. The redis data container is unchanged. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
When harbor's redis sentinels never converge, the suite times out on the HelmRelease assert with no evidence of why: the catch pod-log selector matches neither the redis-operator (it runs in another namespace) nor the rfr-/rfs- pods, and the RedisFailover CRD ships no status subresource to describe-assert on. Add a RedisFailover describe plus a best-effort script that dumps the operator log and the rfr-/rfs- pod states and logs, so the stalled reconcile gate is visible in the failure output. Also widen the harbor-test HR wait from 6m to 10m: readiness is gated on the operator electing a redis master, which normally takes 2-3m but runs long on a CPU-committed runner. This matches the harbor-test-system budget in the same suite and the justified longer waits allowed by the e2e conventions. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
The e2e conventions, the previous-logs capture script, and the global chainsaw catch all claimed the v0.2.15 podLogs collector exposes only container/tail/selector. It also accepts name, namespace, timeout and the cluster fields; only the previous option is genuinely missing. The wrong list steers catch authors toward imperative scripts for cross-namespace log capture that podLogs handles declaratively. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Aleksei Sviridkin (lexfrei)
force-pushed
the
fix/harbor-sentinel-convergence
branch
from
July 27, 2026 19:48
e4d015e to
a7ce437
Compare
2 tasks done
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
2 participants
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
The harbor e2e suite intermittently times out on the
harbor-testHelmRelease because the bundled Redis Sentinel set does not converge in time. Sentinels are NotReady by design until the redis-operator elects a master and rewrites their monitored-master config. Normal convergence is 2-3 minutes; on a CPU-contended e2e runner the tail stretches past the old 6m assert.Three changes:
packages/system/harbor: the inline RedisFailover ran its three sentinels at 50m CPU request / 100m limit. Now they run at Guaranteed QoS (requests == limits, 250m CPU / 128Mi), so master election gets scheduling shares and is not throttled under node contention. A helm unit test pins the sizing. This reserves 750m CPU / 384Mi per Harbor instance for the sentinel trio, up from 150m / 192Mi requested, paid by every install. It also diverges from the standalone redis app on purpose: its preset-based sentinels are Burstable with a 25m CPU request, and matching that would lower the request and make contention worse.harbor-testHelmRelease wait grows from 6m to 10m, same as theharbor-test-systembudget in the same suite and the documented longer-wait allowance for harbor.previousis missing. The wrong list steered catch authors toward imperative scripts for captures podLogs handles declaratively.Caveat: the failing run carried no CPU metrics, so the resource fix is mechanism-backed (CFS shares cycles proportionally to requests), not metric-proven. If the flake recurs, the new diagnostics show which reconcile gate stalled.
Closes #3445
Screenshots
Not a UI change.
Downstream repositories
Trigger-map walk against the diff:
packages/system/harbor/(template and tests) is not in the website app lists (those coverpackages/apps/andpackages/extra/) and touches novalues.schema.json, no values keys, no ApplicationDefinition.hack/e2e-chainsaw/andhack/e2e-capture-previous-logs.shappear in no trigger.docs/agents/e2e-testing.mdis repo-internal agent documentation. Node prerequisites,hack/package.mk, and namespaces are untouched.Release note
Summary by CodeRabbit