resource_group client: narrow degraded fallback conditions for GetResourceGroup#11009
Conversation
Signed-off-by: ystaticy <y_static_y@sina.com>
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
dco-signoff: yes
|
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:
📝 WalkthroughWalkthroughThe change preserves underlying resource-group errors, maps gRPC cancellation and deadline statuses to context errors, and updates controller fallback decisions to classify errors before creating degraded resource groups. ChangesResource group error handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant GlobalController
participant ResourceManagerClient
participant DegradedResourceGroup
Caller->>GlobalController: request resource-group controller
GlobalController->>ResourceManagerClient: GetResourceGroup(context)
ResourceManagerClient-->>GlobalController: wrapped error
GlobalController->>GlobalController: normalize and classify error
GlobalController->>DegradedResourceGroup: create degraded group when eligible
GlobalController-->>Caller: degraded controller or normalized error
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![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
🤖 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/resource_group/controller/global_controller_test.go`:
- Around line 342-345: Update the newController helper to accept the subtest
testing.T and create a local require.New(t) within each invocation instead of
using the parent re. Update every t.Run call that invokes newController to pass
its subtest t, preserving the existing controller creation and assertion
behavior.
In `@client/resource_group/controller/global_controller.go`:
- Around line 579-580: Update the error handling around
normalizeGetResourceGroupError to check context.Canceled and
context.DeadlineExceeded on the original error before normalization. Then
classify the unwrapped gRPC status so codes.DeadlineExceeded reaches the
existing degraded-mode path, while preserving the current cancellation handling.
In `@client/resource_manager_client.go`:
- Around line 162-173: Update the error mapping in
client/resource_manager_client.go:162-173 around the resource-group client
method to convert codes.DeadlineExceeded to ctx.Err() only when the caller
context is done; otherwise preserve the original gRPC deadline error. Ensure
client/resource_group/controller/global_controller.go:579-588 continues routing
server-generated deadline errors through the degraded fallback, and add coverage
in client/resource_group/controller/global_controller_test.go:397-425 for a
server-returned codes.DeadlineExceeded case.
🪄 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: 80d51619-73e1-4ed0-9026-61aacf4a2b6e
📒 Files selected for processing (5)
client/errs/errno.goclient/resource_group/controller/global_controller.goclient/resource_group/controller/global_controller_test.goclient/resource_manager_client.goclient/resource_manager_client_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #11009 +/- ##
==========================================
+ Coverage 79.20% 79.26% +0.06%
==========================================
Files 541 541
Lines 75450 76035 +585
==========================================
+ Hits 59762 60272 +510
- Misses 11456 11518 +62
- Partials 4232 4245 +13
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
ystaticy
changed the title
Signed-off-by: ystaticy <y_static_y@sina.com>
Signed-off-by: ystaticy <y_static_y@sina.com>
Signed-off-by: ystaticy <y_static_y@sina.com>
|
/test pull-unit-test-next-gen-2 |
|
/retest-required |
1 similar comment
|
/retest-required |
|
/retest |
|
/retest |
|
/test pull-unit-test-next-gen-3 |
| switch ctxErr := ctx.Err(); ctxErr { | ||
| case context.Canceled, context.DeadlineExceeded: | ||
| causeErr = ctxErr | ||
| } |
There was a problem hiding this comment.
are you considering context cause err? when will ctx.Err() return other type of errors
There was a problem hiding this comment.
ctx.Err() only returns context.Canceled or context.DeadlineExceeded, so this branch is only used to normalize caller-side cancellation or timeout. I am intentionally not using context.Cause here to keep the change minimal and avoid changing the error chain.
| func unwrapGetResourceGroupError(err error) error { | ||
| for err != nil { | ||
| var groupErr *errs.ErrClientGetResourceGroup | ||
| if !goerrors.As(err, &groupErr) || groupErr.Err == nil || groupErr.Err == err { |
There was a problem hiding this comment.
Yes, groupErr.Err == nil is possible. For example, NewResourceGroupNotExistErr constructs ErrClientGetResourceGroup with only Cause set and no Err field, so there is no inner error to unwrap. The groupErr.Err == nil check handles this case by stopping unwrap and returning the wrapper as-is.
|
/retest |
Signed-off-by: ystaticy <y_static_y@sina.com>
ti-chi-bot
Bot
added
needs-1-more-lgtm
|
@lhy1024: Your lgtm message is repeated, so it is ignored. 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 kubernetes-sigs/prow repository. |
|
/retest |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lhy1024, 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 |
5 participants
What problem does this PR solve?
Issue Number: ref #11010
What is changed and how does it work?
Refine the degraded fallback behavior of
GetResourceGroupwhendegradedRUSettingsis configured.Currently, degraded fallback is too broad and may hide original errors from RM. We should only use degraded fallback for transient RM failures, while preserving original errors for non-retryable cases.
Expected behavior:
SwitchGrouptarget -> do not switchIt is also helpful to preserve the underlying error from
ErrClientGetResourceGroupso upper layers can correctly inspect errors such ascontext.Canceledandcontext.DeadlineExceeded.Check List
Tests
Code changes
Side effects
Related changes
pingcap/docs/pingcap/docs-cn:pingcap/tiup:Release note
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
CanceledandDeadlineExceedederrors, even when the server reports gRPC deadline failures.Tests