CNTRLPLANE-3460: Refactor defrag controller to lower impact of defrag#1618
CNTRLPLANE-3460: Refactor defrag controller to lower impact of defrag#1618dusk125 wants to merge 7 commits into
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughDefragController's ChangesDefragController single-member refactor and test updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/operator/defragcontroller/defragcontroller_test.go`:
- Around line 464-468: The test uses a tiny non-zero defragBudget which can be
flaky; set the DefragController's defragBudget to 0 (or switch the controller to
use a controllable/test clock) so budget exhaustion is deterministic. Locate
where defragBudget is set in the test setup (the variable named defragBudget
used to construct the controller) and replace 1 * time.Nanosecond with 0 (or
adapt the controller constructor to accept a mock clock and use that in the
test) so calling controller.sync(context.TODO(),
factory.NewSyncContext("defrag-controller", eventRecorder)) always observes an
exhausted budget.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e0de8aac-2c9a-42a2-9dee-693e18c04822
📒 Files selected for processing (2)
pkg/operator/defragcontroller/defragcontroller.gopkg/operator/defragcontroller/defragcontroller_test.go
dusk125
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@dusk125: This pull request references CNTRLPLANE-3460 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the sub-task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
dusk125
changed the title
dusk125
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
de3f881 to
28f0bd1
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/operator/defragcontroller/defragcontroller.go (1)
184-190: ⚡ Quick winError chain is lost by double-formatting.
The error is first formatted with
%vintoerrMsg, then wrapped withfmt.Errorf("%s", errMsg). This loses the original error chain, preventing callers from usingerrors.Is/errors.Asfor inspection.♻️ Suggested fix to preserve error chain
- errMsg := fmt.Sprintf("failed defrag on member: %s, memberID: %x: %v", member.Name, member.ID, err) - recorder.Eventf("DefragControllerDefragmentFailed", errMsg) + recorder.Eventf("DefragControllerDefragmentFailed", "failed defrag on member: %s, memberID: %x: %v", member.Name, member.ID, err) c.numDefragFailures++ if c.numDefragFailures >= maxDefragFailuresBeforeDegrade { c.setDegraded(ctx, recorder) } - return fmt.Errorf("%s", errMsg) + return fmt.Errorf("failed defrag on member: %s, memberID: %x: %w", member.Name, member.ID, err)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/operator/defragcontroller/defragcontroller.go` around lines 184 - 190, The error is being double-formatted which loses the error chain: first with fmt.Sprintf using %v into errMsg, then wrapped with fmt.Errorf("%s", errMsg). This prevents callers from using errors.Is or errors.As for inspection. In the return statement at the end of this block, replace fmt.Errorf("%s", errMsg) with fmt.Errorf that includes the descriptive message prefix and wraps the original err variable using the %w verb instead of %v, so the error chain is preserved while maintaining the same log message for the recorder.Eventf call.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/operator/defragcontroller/defragcontroller.go`:
- Around line 184-190: The error is being double-formatted which loses the error
chain: first with fmt.Sprintf using %v into errMsg, then wrapped with
fmt.Errorf("%s", errMsg). This prevents callers from using errors.Is or
errors.As for inspection. In the return statement at the end of this block,
replace fmt.Errorf("%s", errMsg) with fmt.Errorf that includes the descriptive
message prefix and wraps the original err variable using the %w verb instead of
%v, so the error chain is preserved while maintaining the same log message for
the recorder.Eventf call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f3c9859e-40e4-4363-bee9-a4aff47a4ba3
📒 Files selected for processing (3)
pkg/etcdcli/helpers.gopkg/operator/defragcontroller/defragcontroller.gopkg/operator/defragcontroller/defragcontroller_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/etcdcli/helpers.go
|
With the improvements from openshift/etcd#378, the current implementation should be sufficient to lower the impact of defrag. The changes to etcd can take defrag disruption from 30s to 100ms. The current implementation of the changes to the defrag controller run a single defrag, on the most fragmented member, then check the member health on the next sync (once every 11 minutes) and only continue defragging if all members are healthy. |
0934524 to
c92f5ae
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/operator/defragcontroller/defragcontroller.go (1)
264-268:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard against division by zero when
ondiskis zero.If
DbSizeis zero (unlikely but possible edge case), this function returnsNaN(wheninuseis also 0) or-Inf, which can cause unpredictable behavior in the sorting comparator at lines 168-173.Proposed fix
func checkFragmentationPercentage(ondisk, inuse int64) float64 { + if ondisk <= 0 { + return 0 + } diff := float64(ondisk - inuse) fragmentedPercentage := (diff / float64(ondisk)) * 100 return math.Round(fragmentedPercentage*100) / 100 }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/operator/defragcontroller/defragcontroller.go` around lines 264 - 268, The checkFragmentationPercentage function performs division by ondisk without checking if it is zero, which results in NaN or Inf and causes unpredictable behavior in the sorting comparator. Add a guard clause at the start of the checkFragmentationPercentage function to check if ondisk is less than or equal to zero, and return 0.0 in that case before performing the division calculation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/operator/defragcontroller/defragcontroller.go`:
- Around line 156-158: The error message in the status nil check at the failing
condition does not identify which member had the validation failure. Modify the
fmt.Errorf call to include the member or endpoint identifier (such as a member
name, IP address, or other relevant context variable that should be available in
the surrounding scope) in addition to a descriptive message, so the error
clearly indicates which specific member's endpoint status validation failed.
---
Outside diff comments:
In `@pkg/operator/defragcontroller/defragcontroller.go`:
- Around line 264-268: The checkFragmentationPercentage function performs
division by ondisk without checking if it is zero, which results in NaN or Inf
and causes unpredictable behavior in the sorting comparator. Add a guard clause
at the start of the checkFragmentationPercentage function to check if ondisk is
less than or equal to zero, and return 0.0 in that case before performing the
division calculation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b38cd00a-ae65-4251-85d7-30bfeb973803
📒 Files selected for processing (1)
pkg/operator/defragcontroller/defragcontroller.go
c92f5ae to
29119b9
Compare
|
/retest-required |
2 similar comments
|
/retest-required |
|
/retest-required |
Changes the defrag controller to defrag a single member per sync cycle instead of all members in one pass. This reduces the blast radius of defrag operations and avoids write blocks by preemptively transferring leadership away from the defrag target. After a successful leader transfer, the controller requeues with a short delay to allow etcd to settle before proceeding with defragmentation. Defrag failures no longer cause immediate requeue; instead the controller waits for the next periodic sync. The operator client informer is registered as a bare informer since the controller only uses it for cache warmup, not watch-triggered syncs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
25d7cc7 to
dffee92
Compare
| // If the leader is in the first slot, and we have other members to defrag, defrag the others first, | ||
| // so we limit the number of times we move the leader during defrag. |
There was a problem hiding this comment.
in a high churn environment, wouldn't it be possible for leader to keep getting skipped while other members are repeatedly defragged and refragged? Especially since with this change defrag happens more slowly.
Perhaps there should be a threshold at which the leader is selected for defrag even when there are other candidates.
There was a problem hiding this comment.
This is a good point, let me have a little think about this... it's tangentially related to us being slower to react to the database getting close to quota. I've been noodling on a "fast mode" or something that detects these scenarios and does a requeue faster so it's not ~30m to defrag all 3 members
There was a problem hiding this comment.
I'm open to suggestions as well
There was a problem hiding this comment.
My first thought is setting a "critical threshold" above the standard 45% (say 65% for now) which will force the leader to defrag. But with high churn that could end up always selecting the leader and forcing it to move on every sync if each member is above 65% by the time it runs. No good.
Seems like it would need to record which members have already been defragged recently in order to ensure the leader isn't starved while also avoiding selecting the leader too eagerly.
There was a problem hiding this comment.
If we're not overly concerned with recovering the exact state on CEO restart, we could just store that data like the failed defrag count?
There was a problem hiding this comment.
I was beginning to think it looked a lot like the original looped version with extra steps, so that's a good call. Certainly much more straightforward this way.
There was a problem hiding this comment.
There was a bug where a defrag would fail and immediately try to defrag the next member.
I am a bit confused here as to where this bug occurs.
keep track of who's been defragged
Don't we always have this information via isEndpointBackendFragmented?
There was a problem hiding this comment.
This was in the previous implementation in the first commit in which a single member was defragged each sync. In that case you don't know which members were previously defragged, only which members are currently over the fragmentation threshold. So if a cluster is fragmenting fast enough it would be possible for the same members to keep going over the threshold by the next sync, and the leader would always get skipped.
There was a problem hiding this comment.
I wonder how likely that is - maybe we could create an alert for such high fragmentation rate?
I suppose we could also add failsafe and prefer a leader when it goes over a high percentage? e.g. 85%?
There was a problem hiding this comment.
I wonder how likely that is - maybe we could create an alert for such high fragmentation rate?
Probably not common, but while running churn tests with kube-burner I have seen fragmentation jump up ~3GB in under a minute.
I suppose we could also add failsafe and prefer a leader when it goes over a high percentage? e.g. 85%?
My worry with a hard threshold to always select the leader is that if all members are highly fragmented it would end up selecting the leader for defrag every time. Or at the very least, for the first two defrags.
Sync 1:
- etcd1 (leader, 86%) - etcd2 (86%) - etcd3 (86%)
- defrag etcd1 since it is over critical threshold, leader moves to etcd2
Sync 2:
- etcd1 (0%) - etcd2 (leader, 86%) - etcd3 (86%)
- defrag etcd2 since it is over critical threshold, leader moves to etcd1 (in the best case)
...
Replace the cached member client with the non-cached etcd client for the defrag controller's member health and list calls. The defrag controller syncs infrequently enough that caching provides no benefit, and the cached client's 1-minute TTL could mask unhealthy members between rapid defrag operations. Increase the leader transfer settle time from 5s to 10s (renamed from leaderTransferSettleTime to defragSettleTime) to allow sufficient time for the cluster to stabilize after a leadership change. After a successful defrag, if there are remaining defrag targets, requeue with the shorter defragSettleTime rather than waiting for the full ~11 minute compaction-aligned resync period. Non-leader defrags don't block cluster writes, and the live health check on each sync ensures the previously defragged member has recovered before proceeding.
Track which members have been successfully defragged during the current defrag cycle via a defraggedMembers set. When picking the next defrag target, skip the leader only if there are non-leader targets that haven't been defragged yet this cycle. Once all non-leaders have been handled, allow the leader through for defrag (with leader transfer). The set is cleared when no members need defragmentation, starting a fresh cycle on the next compaction interval. This addresses the scenario where in a high-churn environment, all members stay above the fragmentation threshold across sync cycles. Without this tracking, len(defragTargets) > 1 would always be true and the leader would be perpetually skipped in favor of re-fragmented non-leaders.
|
Considering a different approach, before merging, I'll squash if the loop is the way we want to go |
| // If the leader is in the first slot, and we have other members to defrag, defrag the others first, | ||
| // so we limit the number of times we move the leader during defrag. |
There was a problem hiding this comment.
I was beginning to think it looked a lot like the original looped version with extra steps, so that's a good call. Certainly much more straightforward this way.
806c73f to
855e1e0
Compare
855e1e0 to
f4bf8bb
Compare
|
/retest-required |
MoveLeader must be sent to the current leader, but the client pool may route to any member, causing consistent "etcdserver: not leader" errors and falling through to blocking leader defrag every time. Fix by accepting the leader member at the call site and creating a dedicated client connected directly to the leader endpoint, following the same pattern used by Defragment(). Also enable the StopGRPCServiceOnDefrag feature gate so etcd reports NOT_SERVING during defrag, allowing load-balancer-aware clients to route away from the member being defragged. Additionally fix the DefragControllerLeaderTransferFailed event which used Warning instead of Warningf, leaving %s uninterpolated.
|
/pipeline |
|
/pipeline required |
|
Scheduling required tests: |
| // If the leader is in the first slot, and we have other members to defrag, defrag the others first, | ||
| // so we limit the number of times we move the leader during defrag. |
There was a problem hiding this comment.
I wonder how likely that is - maybe we could create an alert for such high fragmentation rate?
I suppose we could also add failsafe and prefer a leader when it goes over a high percentage? e.g. 85%?
a8869f0 to
40ead87
Compare
|
/pipeline required |
|
Scheduling required tests: |
| for _, member := range members { | ||
| // filter out learner members since they don't support the defragment API call | ||
| // and filter out unstarted members | ||
| if member.IsLearner || len(member.ClientURLs) == 0 { |
There was a problem hiding this comment.
Is there any possibility of a member having either of these conditions (e.g. becoming a learner, not sure how that works) after it was already added to defragTargets in a previous sync?
There was a problem hiding this comment.
Maybe....? it would be unlikely unless the cluster is scaling or something like that where we have quorum but additional members that haven't been started, or are new and haven't been promoted yet.
How come?
There was a problem hiding this comment.
Here they are intentionally not being added to the list. But if they are already on the list and then later become a learner then they may attempt to defrag and then fail, possibly becoming a blocker that is stuck in the list.
There was a problem hiding this comment.
May not be a possible scenario. More generally, I am wondering what happens if there is a member in the defragTargets list that repeatedly fails to defragment, or if a member was removed from the cluster but is still in the list. Seems like that could prevent other members from even being considered for defragmentation.
There was a problem hiding this comment.
Ahhh good catch, I don't think it's likely, but it's easy enough to protect against.
Enable the NonBlockingDefrag etcd feature gate so that defragmentation does not hold exclusive locks for the entire copy operation, avoiding write stalls during defrag. Also change the defrag controller to only defrag one member per sync cycle rather than immediately requeueing for the next member. This lets the normal 11-minute resync handle subsequent members, giving the cluster a full compaction interval to recover between defrags.
40ead87 to
680d8a1
Compare
| recorder.Eventf("DefragControllerDefragmentAttempt", "Attempting defrag on member: %s, memberID: %x, dbSize: %d, dbInUse: %d, leader ID: %d", defragTargetMember.Name, defragTargetMember.ID, defragTargetStatus.DbSize, defragTargetStatus.DbSizeInUse, defragTargetStatus.Leader) | ||
| if _, err := c.defragClient.Defragment(ctx, defragTargetMember); err != nil { | ||
| errMsg := fmt.Sprintf("failed defrag on member: %s, memberID: %x: %v", defragTargetMember.Name, defragTargetMember.ID, err) | ||
| recorder.Eventf("DefragControllerDefragmentFailed", errMsg) |
There was a problem hiding this comment.
I still see the event suggestion unresolved
278e4ea to
3cc0bca
Compare
|
Some changes we were discussing in slack:
|
3cc0bca to
ccc4a52
Compare
|
@dusk125: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
5 participants
Summary
wait.Pollrecovery loop and leader-last ordering — with upstream etcd defrag improvements (openshift/etcd#378) reducing disruption from ~30s to ~100ms, the simpler one-per-sync approach is sufficientTest plan
TestNewDefragControllerMultiSyncsvalidates degraded/recovery across sync cycles🤖 Generated with Claude Code
Summary by CodeRabbit