test: stabilize flaky resource manager client tests

[rleungx]

What problem does this PR solve?

Issue Number: Close #10650, Close #8512, Close #8739, Close #7400, Ref #6810

Several tests in TestResourceManagerClientTestSuite assume that asynchronous
resource-manager state is visible immediately or include unrelated wall-clock
overhead in timing assertions. These assumptions make the suite flaky under CI
load, especially with standalone resource-manager discovery.

What is changed and how does it work?

Stabilize resource-manager client integration tests without changing the tested
behavior.

Wait until standalone resource-manager metadata and RU statistics are observable
before checking their values. Measure token-bucket wait time using the duration
reported by OnRequestWait so scheduler and logging overhead do not affect the
throttling assertion.

The existing final assertions remain in place. The changes only replace
immediate reads and fixed sleeps with condition-based waits and use the
controller-reported wait duration for the timing check.

Check List

Tests

• Integration test
  • Affected tests in both deployment modes, 3 repetitions: PASS (194.898s)
  • Complete TestResourceManagerClientTestSuite in both deployment modes: PASS (127.256s)
  • git diff --check: PASS

Release note

None.

Summary by CodeRabbit

• Tests
  • Improved integration tests to reliably handle asynchronous resource-group metadata and RU stat propagation.
  • Added eventual-state polling to verify resource visibility, RU stats, fill-rate changes, and behavior after failover/leader resignation.
  • Updated timing assertions to accumulate controller-reported wait durations instead of measuring elapsed time directly.

[ti-chi-bot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign andremouche for approval. For more information see the Code Review Process.
Please ensure that each of them provides their approval before proceeding.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[coderabbitai]

📝 Walkthrough

Walkthrough

Integration tests now account for asynchronous resource-group propagation by polling for visibility, controller state, RU statistics, and fill-rate updates. Controller timing assertions use durations returned by request handling instead of measuring elapsed wall-clock time.

Changes

Resource manager integration test stability

Layer / File(s)	Summary
Watch state synchronization tests/integrations/mcs/resourcemanager/resource_manager_test.go	Watch tests wait for resource-group visibility, controller activation, and updated fill rates before asserting state.
Controller wait accounting tests/integrations/mcs/resourcemanager/resource_manager_test.go	Request timing checks accumulate durations returned by OnRequestWait for read and write requests.
Consumption and failover assertions tests/integrations/mcs/resourcemanager/resource_manager_test.go	RU-statistics and failover checks use eventual polling until resource-group objects and fill rates match expected values.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

• tikv/pd#10873: Updates resource-manager integration tests for asynchronous resource-group loading and visibility.
• tikv/pd#10883: Updates resource-group watch recovery behavior related to eventual controller state visibility.

Suggested labels: ok-to-test, lgtm, approved

Suggested reviewers: lhy1024, ystaticy, bufferflies

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly matches the main change: stabilizing flaky resource manager client tests.
Description check	✅ Passed	The description follows the template with problem, changes, checklist, and release note sections filled in.
Linked Issues check	✅ Passed	The code changes address the listed flaky tests by waiting for metadata, using controller wait duration, and polling until state is visible.
Out of Scope Changes check	✅ Passed	The diff is confined to test stabilization changes in the targeted integration test file and shows no unrelated scope creep.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

tests/integrations/mcs/resourcemanager/resource_manager_test.go (1)

804-807: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Scope the error variable inside the closure.

Since getErr is not used outside of the testutil.Eventually closure, it can be declared locally within the function to minimize its scope.

♻️ Proposed refactor

-	var getErr error
-	testutil.Eventually(re, func() bool {
-		_, getErr = cli.GetResourceGroup(suite.ctx, group.Name)
-		return getErr == nil
+	testutil.Eventually(re, func() bool {
+		_, err := cli.GetResourceGroup(suite.ctx, group.Name)
+		return err == nil
	}, testutil.WithTickInterval(50*time.Millisecond))

🤖 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
804 - 807, Remove the outer getErr declaration and declare the error variable
within the testutil.Eventually callback alongside the GetResourceGroup call,
preserving the callback’s boolean success check.

🤖 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 `@tests/integrations/mcs/resourcemanager/resource_manager_test.go`:
- Around line 804-807: Remove the outer getErr declaration and declare the error
variable within the testutil.Eventually callback alongside the GetResourceGroup
call, preserving the callback’s boolean success check.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 76627cfe-5758-44fb-b807-cbc1419f802c

📥 Commits

Reviewing files that changed from the base of the PR and between 45bccd7 and bc22c64.

📒 Files selected for processing (1)

• tests/integrations/mcs/resourcemanager/resource_manager_test.go

[ti-chi-bot]

@rleungx: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-unit-test-next-gen-2	bc22c64	link	true	/test pull-unit-test-next-gen-2

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.