add RetryOnConflict to modelbooster by FAUST-BENCHOU · Pull Request #1039 · volcano-sh/kthena

FAUST-BENCHOU · 2026-05-13T15:38:03Z

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:
Fixes #1038

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Copilot

Pull request overview

This PR addresses flakiness in the controller-manager e2e suite (issue #1038) by increasing the Go test timeout so longer-running tests (e.g., rolling update scenarios) are less likely to hit the global test deadline.

Changes:

Increased the go test timeout for test-e2e-controller-manager from 10m to 15m.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

FAUST-BENCHOU · 2026-05-13T15:39:31Z

e2e test are becoming more and more these days.need to increase the timeout.
I only increase controller-manager e2e for now since others seems working well except itself

gemini-code-assist

Code Review

This pull request increases the timeout for controller-manager E2E tests from 10 to 15 minutes in the Makefile. The reviewer suggests adding the -p 1 flag to the test command to ensure tests run sequentially and prevent potential flakiness in the CI environment.

gemini-code-assist · 2026-05-13T15:43:17Z

 	@command -v kind >/dev/null 2>&1 || { echo "Kind is not installed."; exit 1; }
 	@TEST_CATEGORY=controller-manager ./test/e2e/setup.sh
-	@KUBECONFIG=/tmp/kubeconfig-e2e go test -v -timeout=10m ./test/e2e/controller-manager/...
+	@KUBECONFIG=/tmp/kubeconfig-e2e go test -v -timeout=15m ./test/e2e/controller-manager/...


To ensure that E2E tests run sequentially and do not interfere with each other while sharing the same Kind cluster, consider adding the -p 1 flag. This maintains consistency with the main test-e2e target and helps prevent flakiness in the CI environment.

@KUBECONFIG=/tmp/kubeconfig-e2e go test -p 1 -v -timeout=15m ./test/e2e/controller-manager/...

pm-ju · 2026-05-13T15:58:39Z

I second this. Faced this problem in many prs.

pm-ju · 2026-05-13T16:03:32Z

I faced this issue in router and gateway-api too. @FAUST-BENCHOU

FAUST-BENCHOU · 2026-05-14T00:25:45Z

increase the time sucess but

=== RUN   TestModelCR
    model_booster_test.go:43: Created Model CR: kthena-e2e-controller-ay0vx/test-model
    model_booster_test.go:45: 
        	Error Trace:	/home/runner/work/kthena/kthena/test/e2e/controller-manager/model_booster_test.go:45
        	Error:      	Condition never satisfied
        	Test:       	TestModelCR
        	Messages:   	Model did not become Active
--- FAIL: TestModelCR (300.03s)

i remember this test also fails before.no idea why it comes back again

FAUST-BENCHOU · 2026-05-14T00:26:36Z

I faced this issue in router and gateway-api too. @FAUST-BENCHOU

could u link the relative failed PR or ci? maybe we can increase all of these ci test time in one time

pm-ju · 2026-05-14T02:04:35Z

I faced this issue in router and gateway-api too. @FAUST-BENCHOU

could u link the relative failed PR or ci? maybe we can increase all of these ci test time in one time

#1025
#1021
#1030
#1028

FAUST-BENCHOU · 2026-05-14T02:09:38Z

I faced this issue in router and gateway-api too. @FAUST-BENCHOU

could u link the relative failed PR or ci? maybe we can increase all of these ci test time in one time

#1025 #1021 #1030 #1028

the failing reason are not the same as controller-manager,they fail because of some specific test

FAUST-BENCHOU · 2026-05-14T02:49:55Z

/hold
@pm-ju lets disscuss it in issue, i think its not the time issue.I'm checking the controller-manager's log

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (2)

pkg/model-booster-controller/controller/model_booster_controller.go:269

Inside the RetryOnConflict closure, ObservedGeneration is now set to updated.Generation (the generation from the latest object in the lister) rather than the generation of the modelBooster actually being reconciled. If the lister has already observed a newer spec generation than the one whose conditions are being written, this will prematurely advance ObservedGeneration to a generation that has not actually been reconciled. That, in turn, interacts badly with the gating in updateModelBooster (if oldModel.Status.ObservedGeneration != newModel.Generation) and the new early‑return in setModelProcessingCondition (model.Status.ObservedGeneration == model.Generation), both of which can cause the controller to skip reconciling the newer generation. It is safer to set updated.Status.ObservedGeneration = modelBooster.Generation so that the value reflects the generation whose state was actually computed.

		updated.Status.ObservedGeneration = updated.Generation

pkg/model-booster-controller/controller/condition.go:58

The early-return short-circuit is only applied to setModelProcessingCondition, but setModelActiveCondition (called every reconcile after success) and setModelInitCondition are still unconditionally invoking updateModelBoosterStatus on every reconcile. That keeps generating unnecessary UpdateStatus calls (and corresponding conflicts/retries) once the ModelBooster is already Active with matching ObservedGeneration. For consistency and to actually eliminate the update churn that motivated this PR, the same "no-op if already in target state" guard should be applied to the other condition setters as well.

func (mc *ModelBoosterController) setModelProcessingCondition(ctx context.Context, model *workloadv1alpha1.ModelBooster) error {
	if meta.IsStatusConditionPresentAndEqual(model.Status.Conditions, string(workloadv1alpha1.ModelStatusConditionTypeActive), metav1.ConditionTrue) &&
		model.Status.ObservedGeneration == model.Generation {
		return nil
	}
	meta.SetStatusCondition(&model.Status.Conditions, newCondition(string(workloadv1alpha1.ModelStatusConditionTypeActive),
		metav1.ConditionFalse, ModelProcessingReason, "ModelBooster not ready yet"))
	if err := mc.updateModelBoosterStatus(ctx, model); err != nil {
		klog.Errorf("update ModelBooster status failed: %v", err)
		return err
	}
	return nil

+		latest, err := mc.modelBoosterLister.ModelBoosters(modelBooster.Namespace).Get(modelBooster.Name)
+		if err != nil {
+			return err
+		}


 	@command -v kind >/dev/null 2>&1 || { echo "Kind is not installed."; exit 1; }
 	@TEST_CATEGORY=controller-manager ./test/e2e/setup.sh
-	@KUBECONFIG=/tmp/kubeconfig-e2e go test -v -timeout=10m ./test/e2e/controller-manager/...
+	@KUBECONFIG=/tmp/kubeconfig-e2e go test -v -timeout=15m ./test/e2e/controller-manager/...


FAUST-BENCHOU · 2026-05-14T04:24:41Z

/unhold

hzxuzhonghu · 2026-05-14T07:09:35Z

 	@command -v kind >/dev/null 2>&1 || { echo "Kind is not installed."; exit 1; }
 	@TEST_CATEGORY=controller-manager ./test/e2e/setup.sh
-	@KUBECONFIG=/tmp/kubeconfig-e2e go test -v -timeout=10m ./test/e2e/controller-manager/...
+	@KUBECONFIG=/tmp/kubeconfig-e2e go test -v -timeout=15m ./test/e2e/controller-manager/...


the Makefile timeout bump (10m→15m) seems unrelated to the RetryOnConflict change — worth splitting into a separate commit so the history stays clean and the CI change is easy to revert independently if needed.

it is a seperate commit in fact...

hzxuzhonghu · 2026-05-14T07:09:36Z

 func (mc *ModelBoosterController) setModelProcessingCondition(ctx context.Context, model *workloadv1alpha1.ModelBooster) error {
+	if meta.IsStatusConditionPresentAndEqual(model.Status.Conditions, string(workloadv1alpha1.ModelStatusConditionTypeActive), metav1.ConditionTrue) &&
+		model.Status.ObservedGeneration == model.Generation {
+		return nil


this guard feels off to me — setModelProcessingCondition is supposed to flip Active to False, but here we bail early if it's already True with a current generation. What if the model genuinely transitions back into processing (e.g. a pod was replaced)? We'd silently skip the status update and leave it looking Active when it isn't. Unless the call site already filters that out?

IC,my bad.This change need to be reverted.

Signed-off-by: zhoujinyu <2319109590@qq.com>

FAUST-BENCHOU · 2026-05-14T15:22:32Z

/retest

volcano-sh-bot · 2026-05-14T15:22:55Z

@FAUST-BENCHOU: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

Details

In response to this:

/retest

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/test-infra repository.

pm-ju · 2026-05-17T10:37:13Z

hey @FAUST-BENCHOU did the falling test get resolved with this pr?

FAUST-BENCHOU · 2026-05-17T10:51:00Z

/gemini review

FAUST-BENCHOU · 2026-05-17T10:52:00Z

hey @FAUST-BENCHOU did the falling test get resolved with this pr?

maybe wait till tomorrow for maintainer's feedback.Today is weekend

gemini-code-assist

Code Review

This pull request increases the e2e test timeout for the controller manager and implements a retry mechanism for updating ModelBooster status using RetryOnConflict. Feedback suggests improving the retry logic by fetching the latest object version directly from the API client instead of the lister to avoid stale cache issues, correctly assigning the ObservedGeneration to reflect the reconciled version, and ensuring object metadata is synchronized after the update.

hzxuzhonghu · 2026-05-18T03:23:20Z

/lgtm
/approve

volcano-sh-bot · 2026-05-18T03:23:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hzxuzhonghu

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [hzxuzhonghu]

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

Copilot AI review requested due to automatic review settings May 13, 2026 15:38

volcano-sh-bot added the do-not-merge/work-in-progress label May 13, 2026

volcano-sh-bot requested a review from hzxuzhonghu May 13, 2026 15:38

FAUST-BENCHOU marked this pull request as ready for review May 13, 2026 15:38

volcano-sh-bot added size/XS and removed do-not-merge/work-in-progress labels May 13, 2026

Copilot started reviewing on behalf of FAUST-BENCHOU May 13, 2026 15:38 View session

Copilot AI reviewed May 13, 2026

View reviewed changes

gemini-code-assist Bot reviewed May 13, 2026

View reviewed changes

volcano-sh-bot added the do-not-merge/hold label May 14, 2026

FAUST-BENCHOU force-pushed the test/controller-time-more branch from d896890 to 9bcf04f Compare May 14, 2026 03:18

volcano-sh-bot added size/S and removed size/XS labels May 14, 2026

FAUST-BENCHOU changed the title ~~increase timeout in controller-manager e2e~~ add RetryOnConflict to modelbooster May 14, 2026

Copilot AI review requested due to automatic review settings May 14, 2026 04:00

Copilot started reviewing on behalf of FAUST-BENCHOU May 14, 2026 04:00 View session

Copilot AI reviewed May 14, 2026

View reviewed changes

volcano-sh-bot removed the do-not-merge/hold label May 14, 2026

FAUST-BENCHOU mentioned this pull request May 14, 2026

flaky e2e controller-manager #1038

Closed

hzxuzhonghu reviewed May 14, 2026

View reviewed changes

FAUST-BENCHOU added 2 commits May 14, 2026 19:06

increase timeout

3196f13

Signed-off-by: zhoujinyu <2319109590@qq.com>

add RetryOnConflict

3c22e25

Signed-off-by: zhoujinyu <2319109590@qq.com>

FAUST-BENCHOU force-pushed the test/controller-time-more branch from c408395 to 3c22e25 Compare May 14, 2026 11:13

gemini-code-assist Bot reviewed May 17, 2026

View reviewed changes

Comment thread pkg/model-booster-controller/controller/model_booster_controller.go

volcano-sh-bot assigned hzxuzhonghu May 18, 2026

volcano-sh-bot added the lgtm label May 18, 2026

volcano-sh-bot added the approved label May 18, 2026

volcano-sh-bot merged commit 1acf175 into volcano-sh:main May 18, 2026
20 of 21 checks passed

Conversation

FAUST-BENCHOU commented May 13, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

FAUST-BENCHOU commented May 13, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot May 13, 2026

Choose a reason for hiding this comment

Uh oh!

pm-ju commented May 13, 2026

Uh oh!

pm-ju commented May 13, 2026

Uh oh!

FAUST-BENCHOU commented May 14, 2026

Uh oh!

FAUST-BENCHOU commented May 14, 2026

Uh oh!

pm-ju commented May 14, 2026

Uh oh!

FAUST-BENCHOU commented May 14, 2026

Uh oh!

FAUST-BENCHOU commented May 14, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

FAUST-BENCHOU commented May 14, 2026

Uh oh!

hzxuzhonghu May 14, 2026

Choose a reason for hiding this comment

Uh oh!

FAUST-BENCHOU May 14, 2026

Choose a reason for hiding this comment

Uh oh!

hzxuzhonghu May 14, 2026

Choose a reason for hiding this comment

Uh oh!

FAUST-BENCHOU May 14, 2026

Choose a reason for hiding this comment

Uh oh!

FAUST-BENCHOU commented May 14, 2026

Uh oh!

volcano-sh-bot commented May 14, 2026

Uh oh!

pm-ju commented May 17, 2026

Uh oh!

FAUST-BENCHOU commented May 17, 2026

Uh oh!

FAUST-BENCHOU commented May 17, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

hzxuzhonghu commented May 18, 2026

Uh oh!

volcano-sh-bot commented May 18, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants