Skip to content

chore(api): Remove duplicate TrainJob status patch#3448

Open
robert-bell wants to merge 2 commits intokubeflow:masterfrom
robert-bell:remove-duplicate-code
Open

chore(api): Remove duplicate TrainJob status patch#3448
robert-bell wants to merge 2 commits intokubeflow:masterfrom
robert-bell:remove-duplicate-code

Conversation

@robert-bell
Copy link
Copy Markdown
Contributor

What this PR does / why we need it:

Small refactoring to tidy up the implementation from #3258.

  • Removes duplicate code for updating the TrainJob status in the reconciliation loop to simplify maintenance.
  • Changed reconcileDeadline to return ctrl.Result instead of (ctrl.Result, error) since it never returned non-nil errors.

Checklist:

  • Docs included if any changes are user facing

Copilot AI review requested due to automatic review settings April 24, 2026 07:43
@google-oss-prow
Copy link
Copy Markdown

[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 johnugeorge for approval. For more information see the Kubernetes Code Review Process.

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

Details Needs approval from an approver in each of these files:

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

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Refactors the TrainJob reconciliation loop to remove duplicated status-patching logic introduced in #3258, and simplifies reconcileDeadline since it never returns an error.

Changes:

  • Consolidates TrainJob status patching into a single location in Reconcile.
  • Changes reconcileDeadline signature to return only ctrl.Result.
  • Adjusts deadline-based requeue handling to reuse the unified status patch logic.

Comment thread pkg/controller/trainjob_controller.go Outdated
}
deadlineResult := r.reconcileDeadline(ctx, &trainJob)

if !equality.Semantic.DeepEqual(&trainJob.Status, prevTrainJob.Status) {
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The DeepEqual call compares &trainJob.Status (a pointer) with prevTrainJob.Status (a value), which will always evaluate as different and cause a status patch on every reconcile; compare like-for-like (either both values or both pointers) to avoid unnecessary API writes and update conflicts.

Suggested change
if !equality.Semantic.DeepEqual(&trainJob.Status, prevTrainJob.Status) {
if !equality.Semantic.DeepEqual(trainJob.Status, prevTrainJob.Status) {

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@robert-bell I think it would be great if you fix this in this PR as well.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

cc @astefanutti @andreyvelich - I think this came up before in #3227 but we reverted the change that time. Just flagging in case you wanted to comment.

I think we should make this change though - testing locally, the current logic is always returning false so we're making a status patch on every reconcile regardless of whether the status has changed.

@robert-bell
Copy link
Copy Markdown
Contributor Author

cc @XploY04 - this is a small tidy up from your PR #3258. ptal

Signed-off-by: Rob Bell <robell@redhat.com>
Signed-off-by: Rob Bell <robell@redhat.com>
@robert-bell robert-bell force-pushed the remove-duplicate-code branch from 03a6ed1 to 64bfc42 Compare April 27, 2026 08:01
@robert-bell
Copy link
Copy Markdown
Contributor Author

Thanks for the review @XploY04. PTAL.

I've also rebased which should hopefully fix the ci e2e tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants