SREP-413: Prevent false UpgradeConfigValidationFailedSRE alert after …

[Alcamech]

SREP-413: Prevent false UpgradeConfigValidationFailedSRE alert after upgrade completes

What type of PR is this?

Bug

What this PR does / why we need it?

When an UpgradeConfig is recreated by the UpgradeConfigManager (due to spec changes from OCM after an upgrade completes), the status/history is lost. The controller would then create a new history entry with Phase: New, run validation, and fail because the desired version matches the current version - triggering a false UpgradeConfigValidationFailedSRE alert.

This PR adds a check for HasUpgradeCompleted() before defaulting to Phase: New. If CVO confirms the upgrade already completed, we create the history entry with Phase: Upgraded, bypassing validation entirely and preventing the false alert.

Which Jira/Github issue(s) this PR fixes?

SREP-413

Special notes for your reviewer:

If StartTime or CompleteTime cannot be extracted from CVO history (e.g., CVO's CompletionTime is nil), the synthetic history entry may have nil timestamps. This could cause a panic in reportUpgradeMetrics() at line 310. Should we add nil checks or default to time.Now() as a fallback ?

[openshift-ci-robot]

@Alcamech: This pull request references SREP-413 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.22.0" version, but no target version was set.

Details

In response to this:

SREP-413: Prevent false UpgradeConfigValidationFailedSRE alert after upgrade completes

What type of PR is this?

Bug

What this PR does / why we need it?

When an UpgradeConfig is recreated by the UpgradeConfigManager (due to spec changes from OCM after an upgrade completes), the status/history is lost. The controller would then create a new history entry with Phase: New, run validation, and fail because the desired version matches the current version - triggering a false UpgradeConfigValidationFailedSRE alert.

This PR adds a check for HasUpgradeCompleted() before defaulting to Phase: New. If CVO confirms the upgrade already completed, we create the history entry with Phase: Upgraded, bypassing validation entirely and preventing the false alert.

Which Jira/Github issue(s) this PR fixes?

SREP-413

Special notes for your reviewer:

If StartTime or CompleteTime cannot be extracted from CVO history (e.g., CVO's CompletionTime is nil), the synthetic history entry may have nil timestamps. This could cause a panic in reportUpgradeMetrics() at line 310. Should we add nil checks or default to time.Now() as a fallback ?

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 openshift-eng/jira-lifecycle-plugin repository.

[openshift-ci-robot]

@Alcamech: This pull request references SREP-413 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.22.0" version, but no target version was set.

Details

In response to this:

SREP-413: Prevent false UpgradeConfigValidationFailedSRE alert after upgrade completes

What type of PR is this?

Bug

What this PR does / why we need it?

When an UpgradeConfig is recreated by the UpgradeConfigManager (due to spec changes from OCM after an upgrade completes), the status/history is lost. The controller would then create a new history entry with Phase: New, run validation, and fail because the desired version matches the current version - triggering a false UpgradeConfigValidationFailedSRE alert.

This PR adds a check for HasUpgradeCompleted() before defaulting to Phase: New. If CVO confirms the upgrade already completed, we create the history entry with Phase: Upgraded, bypassing validation entirely and preventing the false alert.

Which Jira/Github issue(s) this PR fixes?

SREP-413

Special notes for your reviewer:

If StartTime or CompleteTime cannot be extracted from CVO history (e.g., CVO's CompletionTime is nil), the synthetic history entry may have nil timestamps. This could cause a panic in reportUpgradeMetrics() at line 310. Should we add nil checks or default to time.Now() as a fallback ?

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 openshift-eng/jira-lifecycle-plugin repository.

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Alcamech
Once this PR has been reviewed and has the lgtm label, please assign tkong-redhat for approval. For more information see the 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:

• OWNERS

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

[codecov-commenter]

Codecov Report

❌ Patch coverage is 92.30769% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 54.47%. Comparing base (b43d6ec) to head (13b927d).
⚠️ Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
...trollers/upgradeconfig/upgradeconfig_controller.go	92.30%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #573      +/-   ##
==========================================
+ Coverage   54.35%   54.47%   +0.11%     
==========================================
  Files         123      123              
  Lines        6123     6139      +16     
==========================================
+ Hits         3328     3344      +16     
  Misses       2592     2592              
  Partials      203      203              

Files with missing lines	Coverage Δ
...trollers/upgradeconfig/upgradeconfig_controller.go	81.60% <92.30%> (+1.50%)	⬆️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[openshift-ci]

@Alcamech: 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
ci/prow/validate	13b927d	link	true	/test validate

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.