client: fix resource group config watch recovery#10883
Merged
ti-chi-bot[bot] merged 10 commits intoJun 12, 2026
Merged
Conversation
Signed-off-by: tongjian <1045931706@qq.com>
Signed-off-by: tongjian <1045931706@qq.com>
ti-chi-bot
Bot
added
release-note
📝 WalkthroughWalkthroughRefactors meta-storage watches to emit a structured metastorage.WatchResponse (Events + CompactRevision) and updates client producers, consumers, server handling, controller logic, and tests to use the new shape. ChangesMeta Storage Watch API Refactoring
Sequence DiagramsequenceDiagram
participant Ctrl as ResourceGroupController
participant MetaClient as MetaStorage Client
participant GRPCStream as gRPC Watch Stream
participant Channel as WatchResponse Channel
Ctrl->>MetaClient: Watch(ctx, key)
MetaClient->>GRPCStream: Start watch stream
GRPCStream-->>MetaClient: resp with Events / header
alt Stream receives events
MetaClient->>MetaClient: Build WatchResponse(Events)
MetaClient->>Channel: Send WatchResponse
Channel-->>Ctrl: Receive WatchResponse
Ctrl->>Ctrl: Process resp.Events
else DATA_COMPACTED detected
GRPCStream-->>MetaClient: header DATA_COMPACTED
MetaClient->>MetaClient: Extract CompactRevision from header
MetaClient->>MetaClient: Build WatchResponse with CompactRevision
MetaClient->>Channel: Send WatchResponse
Channel-->>Ctrl: Receive WatchResponse
Ctrl->>Ctrl: Advance cfgRevision from CompactRevision
else Stream error
MetaClient->>MetaClient: Log error and build error WatchResponse
MetaClient->>Channel: Send error WatchResponse
MetaClient->>GRPCStream: Close
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
ti-chi-bot
Bot
added
the
size/L
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/integrations/mcs/resourcemanager/resource_manager_test.go (1)
94-98: 💤 Low valueRedundant condition check.
Line 95 checks
op.Revision > 0again, but this is already guaranteed by the outer condition on line 94.♻️ Simplify the condition
if op.Revision > 0 { - if p.firstRevision == 0 && op.Revision > 0 { + if p.firstRevision == 0 { p.firstRevision = op.Revision } else if op.Revision == p.firstRevision { p.watchTimes++🤖 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 `@tests/integrations/mcs/resourcemanager/resource_manager_test.go` around lines 94 - 98, The inner conditional redundantly re-checks op.Revision > 0; simplify the block by removing that redundant check and just test p.firstRevision: in the scope where op.Revision > 0 is already true, replace the inner if (currently using p.firstRevision == 0 && op.Revision > 0) with if p.firstRevision == 0 to set p.firstRevision = op.Revision, and keep the else if op.Revision == p.firstRevision branch to increment p.watchTimes (references: op.Revision, p.firstRevision, p.watchTimes).
🤖 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 `@client/meta_storage_client.go`:
- Around line 168-180: The code incorrectly sets compactRevision from
header.GetRevision(); change it to use resp.GetCompactRevision() when
header.Error.Type == meta_storagepb.ErrorType_DATA_COMPACTED so the client
retries from the server-provided retry position (update the conditional that
reads resp.GetHeader() and header.Error to assign compactRevision =
resp.GetCompactRevision()); also correct the comment "If mets" to "If meets" and
ensure the metaStorageResp still populates CompactRevision from the corrected
variable.
In `@client/resource_group/controller/global_controller_test.go`:
- Around line 105-107: The mock Watch currently casts to chan
*metastorage.WatchResponse but other stubs still create/send chan
[]*meta_storagepb.Event, causing interface conversion panics; update all mock
stubs (those that currently return chan []*meta_storagepb.Event) and any test
sender paths to return/send chan *metastorage.WatchResponse instead, and when
sending wrap the event slice inside a &metastorage.WatchResponse{Events: events}
(so callers read resp.Events), keeping the MockResourceGroupProvider.Watch
signature and returned channel type consistent across the file.
In `@pkg/mcs/metastorage/server/grpc_service.go`:
- Around line 101-106: The Compact call in the failpoint path currently ignores
its returned error; capture the error from s.manager.client.Compact(s.ctx,
int64(rev)) and handle it instead of discarding it — e.g., if err != nil {
s.logger.Errorf("compact failed during failpoint: %v", err); /* or
return/propagate error as appropriate for the surrounding function */ } so
failures are visible during tests; update the failpoint.Inject block to check
the error and either log or propagate it.
---
Nitpick comments:
In `@tests/integrations/mcs/resourcemanager/resource_manager_test.go`:
- Around line 94-98: The inner conditional redundantly re-checks op.Revision >
0; simplify the block by removing that redundant check and just test
p.firstRevision: in the scope where op.Revision > 0 is already true, replace the
inner if (currently using p.firstRevision == 0 && op.Revision > 0) with if
p.firstRevision == 0 to set p.firstRevision = op.Revision, and keep the else if
op.Revision == p.firstRevision branch to increment p.watchTimes (references:
op.Revision, p.firstRevision, p.watchTimes).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 27d428ad-10b7-4a0a-812c-2a2fd1050e45
📒 Files selected for processing (10)
client/clients/metastorage/client.goclient/meta_storage_client.goclient/resource_group/controller/global_controller.goclient/resource_group/controller/global_controller_test.goclient/resource_manager_client.goclient/resource_manager_client_test.goclient/servicediscovery/resource_manager_service_discovery_test.gopkg/mcs/metastorage/server/grpc_service.gotests/integrations/client/client_test.gotests/integrations/mcs/resourcemanager/resource_manager_test.go
rleungx
reviewed
Jun 11, 2026
Signed-off-by: bufferflies <1045931706@qq.com>
Contributor
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #10883 +/- ##
==========================================
+ Coverage 79.14% 79.19% +0.04%
==========================================
Files 536 540 +4
Lines 74033 74434 +401
==========================================
+ Hits 58594 58948 +354
- Misses 11311 11329 +18
- Partials 4128 4157 +29
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
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)
tests/integrations/client/client_test.go (1)
1556-1564:⚠️ Potential issue | 🟡 MinorGuard watch channel receive to avoid nil deref (
res.Events)
client.Watchreturnschan *metastorage.WatchResponse; when the watch goroutine closesch, a receive can yieldres == nil, andres.Eventswill panic. Add anokcheck before iterating.Suggested fix
- case res := <-ch: - for _, r := range res.Events { + case res, ok := <-ch: + if !ok { + re.FailNow("watch channel closed unexpectedly") + } + for _, r := range res.Events { watchCount++ if r.GetType() == meta_storagepb.Event_DELETE { re.Equal(watchPrefix+string(r.PrevKv.Value), string(r.Kv.Key)) } else { re.Equal(watchPrefix+string(r.Kv.Value), string(r.Kv.Key)) } }🤖 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 `@tests/integrations/client/client_test.go` around lines 1556 - 1564, The test reads from the watch channel without guarding for a closed channel which can yield res == nil and cause a panic when accessing res.Events; change the receive to "res, ok := <-ch" (or check res == nil) and skip or break when !ok || res == nil before iterating over res.Events, so update the block that uses client.Watch / ch / WatchResponse / res.Events to handle a closed channel safely.
🤖 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/mcs/metastorage/server/grpc_service.go`:
- Around line 121-123: The Watch handler is reading from watchChan with "res :=
<-watchChan" without checking whether the channel was closed, which can cause a
tight loop on a nil/zero WatchResponse; modify the receive to use the two-value
form (res, ok := <-watchChan) in the Watch method and immediately handle !ok by
breaking/returning (or cancelling the watch and cleaning up) so the loop exits
when etcd closes the channel; ensure subsequent code that uses res (res.Err(),
res.Events) only runs when ok is true.
---
Outside diff comments:
In `@tests/integrations/client/client_test.go`:
- Around line 1556-1564: The test reads from the watch channel without guarding
for a closed channel which can yield res == nil and cause a panic when accessing
res.Events; change the receive to "res, ok := <-ch" (or check res == nil) and
skip or break when !ok || res == nil before iterating over res.Events, so update
the block that uses client.Watch / ch / WatchResponse / res.Events to handle a
closed channel safely.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 46eb51ce-b1a3-4847-a9bf-e45581e6444f
📒 Files selected for processing (10)
client/clients/metastorage/client.goclient/meta_storage_client.goclient/resource_group/controller/global_controller.goclient/resource_group/controller/global_controller_test.goclient/resource_manager_client.goclient/resource_manager_client_test.goclient/servicediscovery/resource_manager_service_discovery_test.gopkg/mcs/metastorage/server/grpc_service.gotests/integrations/client/client_test.gotests/integrations/mcs/resourcemanager/resource_manager_test.go
rleungx
approved these changes
Jun 11, 2026
ti-chi-bot
Bot
added
needs-1-more-lgtm
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: tongjian <1045931706@qq.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: tongjian <1045931706@qq.com>
…o bugfix/too_many_watch
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: tongjian <1045931706@qq.com>
okJiang
approved these changes
Jun 12, 2026
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: okJiang, rleungx The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
Contributor
Contributor
Author
|
/retest |
Contributor
|
@bufferflies: The following test 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. |
This was referenced Jul 8, 2026
Merged
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
3 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 problem does this PR solve?
Issue Number: Close #10882, Close #9895
The resource group controller can retry controller config watches from a stale revision after the watch stream is closed because the requested revision has been compacted. This can cause repeated failed watch attempts / noisy watch requests and delay applying updated controller config.
What is changed and how does it work?
Check List
Tests
Side effects
Related changes
Release note
Summary by CodeRabbit
New Features
Bug Fixes
Tests