MGMT-19979: Propagate ClusterVersion upgrade status message #126
Conversation
|
@CrystalChun: This pull request references MGMT-19979 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 story to target the "4.19.0" version, but no target version was set. In 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
|
Skipping CI for Draft Pull Request. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CrystalChun The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
""" WalkthroughA new constant Changes
Sequence Diagram(s)sequenceDiagram
participant C as Controller
participant S as setUpgradeStatus
participant U as Upgrader
participant CM as ConditionManager
C->>S: setUpgradeStatus(oacp, upgradeInProgress, msg)
S->>U: GetUpgradeStatus(ctx)
U-->>S: (upgradeMessage, error)
alt Error encountered
S->>C: Return error
else
S->>CM: conditions.MarkFalse(oacp, reason, upgradeMessage)
end
Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)level=warning msg="The linter 'exportloopref' is deprecated (since v1.60.2) due to: Since Go1.22 (loopvar) this linter is no longer relevant. Replaced by copyloopvar." Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
openshift-ci
bot
added
approved
|
approach seems reasonable, please make sure tests where we define CVO status (that we will expect to be reported on our CR) are added 🙏 |
openshift-ci
bot
added
size/L
| @@ -506,8 +517,9 @@ var _ = Describe("Upgrade scenarios", func() { | |||
| Expect(k8sClient.Update(ctx, openshiftAssistedControlPlane)).To(Succeed()) | |||
|
|
|||
| mockUpgradeFactory.EXPECT().NewUpgrader(gomock.Any()).Return(mockUpgrader, nil) | |||
| mockUpgrader.EXPECT().IsUpgradeInProgress(gomock.Any()).Return(false, nil) | |||
| mockUpgrader.EXPECT().IsUpgradeInProgress(gomock.Any()).Return(true, nil) // should this be successfully starting upgrade? | |||
There was a problem hiding this comment.
Hey @rccrdpccl should this test should handle upgrade with repository override return true that upgrade is in progress? I wasn't sure
There was a problem hiding this comment.
According to the current behaviour, this should start an upgrade either way:
- if UpgradeInProgress, but DesiredVersion is not updated, we should trigger an update
- if Upgrade not in progress, and desiredVersion is not updated, we should still trigger an update.
I think the logic is fair, but would this work with CVO? Can we update the version if an upgrade is already running?
There was a problem hiding this comment.
Good question, I can verify that!
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
Thanks @rccrdpccl 🙏 added checks to existing tests for correct statuses. Let me know if more tests are needed though! |
|
@CrystalChun: This pull request references MGMT-19979 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 story to target the "4.19.0" version, but no target version was set. In 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 openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
controlplane/api/v1alpha2/condition_consts.go(1 hunks)controlplane/internal/controller/openshiftassistedcontrolplane_controller.go(3 hunks)controlplane/internal/controller/openshiftassistedcontrolplane_controller_test.go(9 hunks)controlplane/internal/upgrade/mock_upgrade.go(1 hunks)controlplane/internal/upgrade/upgrade.go(2 hunks)controlplane/internal/upgrade/upgrade_test.go(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
controlplane/internal/controller/openshiftassistedcontrolplane_controller_test.go (1)
controlplane/api/v1alpha2/condition_consts.go (2)
UpgradeCompletedCondition(13-13)UpgradeFailedReason(40-40)
controlplane/internal/controller/openshiftassistedcontrolplane_controller.go (2)
controlplane/api/v1alpha2/openshiftassistedcontrolplane_types.go (1)
OpenshiftAssistedControlPlane(239-245)controlplane/api/v1alpha2/condition_consts.go (3)
UpgradeInProgressReason(37-37)UpgradeFailedReason(40-40)UpgradeCompletedCondition(13-13)
🔇 Additional comments (15)
controlplane/api/v1alpha2/condition_consts.go (1)
39-40: Appropriate addition of the UpgradeFailedReason constantThe new constant for representing failed upgrades is well-defined and follows the existing pattern of other reason constants in the file. It has the correct comment format with severity information.
controlplane/internal/upgrade/upgrade_test.go (1)
179-185: Good test coverage for the new GetUpgradeStatus methodThe test correctly verifies the expected behavior of the new method, ensuring it returns the upgrade progress message.
controlplane/internal/controller/openshiftassistedcontrolplane_controller.go (4)
289-290: Good addition of upgrade condition message trackingThe new variable appropriately captures the upgrade status message that will be propagated to users.
293-294: Good refactoring to use the centralized setUpgradeStatus functionExtracting this logic into a dedicated function improves code organization and maintainability.
314-317: Proper error handling for upgrade status retrievalThe code now correctly checks for errors when retrieving the upgrade status message, which improves robustness.
356-374: Well-structured centralized upgrade status functionThe new
setUpgradeStatusfunction effectively centralizes the logic for upgrade status handling, determining the appropriate reason and severity based on the upgrade state. It also properly includes the upgrade condition message in the status, which achieves the PR's objective of propagating ClusterVersion messages to users.controlplane/internal/upgrade/mock_upgrade.go (1)
90-103: Appropriate mock implementation for the newGetUpgradeStatusmethod.The mock implementation follows the standard pattern for GoMock, correctly defining both the method implementation and the mock recorder. This allows tests to verify the expected behavior of the
GetUpgradeStatusmethod and ensures proper integration with the upgrade status reporting feature.controlplane/internal/controller/openshiftassistedcontrolplane_controller_test.go (8)
84-84: LGTM: Good default behavior for mock setup.Setting a default behavior for
GetUpgradeStatusto return an empty string with no error is appropriate for most test scenarios, as it represents the case when no upgrade is in progress.
388-389: Appropriate mock expectation for the 'no upgrade in progress' scenario.The mock is correctly set up to return an empty status message when no upgrade is in progress, maintaining consistency with the implementation in the controller.
408-409: Proper handling of upgrade status message in the in-progress scenario.The test correctly verifies that the upgrade status message from the cluster is incorporated into the condition message. This allows users to see both the general upgrade progress and any specific details from the underlying ClusterVersion operator.
Also applies to: 423-423
430-431: Appropriate mock expectation for completed upgrade scenario.When an upgrade is completed, the status message should be empty, which is correctly modeled in this test.
461-462: Correct implementation of failure status propagation.The test properly verifies that when an upgrade fails, both the failure reason and the status message from the cluster are properly reflected in the condition. This ensures users get detailed information about the failure.
Also applies to: 484-485
493-494: Appropriate handling of version retrieval failures.Even when there's an error getting the current version, the test correctly verifies that the upgrade status message is still propagated, ensuring users get as much information as possible about the issue.
Also applies to: 508-509
520-520: Clarify the expected upgrade state in this test.There's uncertainty about whether this test should return that an upgrade is in progress or successfully starting. Consider documenting the reasoning for choosing "in progress" to clarify the intended behavior.
This appears to be a comment related to a previous review conversation with rccrdpccl. Based on the test context, the
IsUpgradeInProgressreturningtrueseems correct since the test is verifying the behavior when an upgrade is in progress with a repository override.
522-523: Proper status propagation with repository override.The test correctly verifies that the upgrade status message is properly propagated when using a custom repository override, maintaining consistency with the other upgrade scenarios.
Also applies to: 543-543
| Conditions: []configv1.ClusterOperatorStatusCondition{ | ||
| { | ||
| Message: "upgrade is in progress", | ||
| Status: configv1.ConditionTrue, | ||
| }, | ||
| { | ||
| Message: "upgrade is not in progress", | ||
| Status: configv1.ConditionFalse, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Incomplete condition objects in test data
The added conditions are missing the Type field, which is normally required for ClusterOperatorStatusCondition objects. In the actual implementation, the condition Type is used to identify specific conditions.
Conditions: []configv1.ClusterOperatorStatusCondition{
{
+ Type: configv1.OperatorProgressing,
Message: "upgrade is in progress",
Status: configv1.ConditionTrue,
},
{
+ Type: configv1.OperatorProgressing,
Message: "upgrade is not in progress",
Status: configv1.ConditionFalse,
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Conditions: []configv1.ClusterOperatorStatusCondition{ | |
| { | |
| Message: "upgrade is in progress", | |
| Status: configv1.ConditionTrue, | |
| }, | |
| { | |
| Message: "upgrade is not in progress", | |
| Status: configv1.ConditionFalse, | |
| }, | |
| }, | |
| Conditions: []configv1.ClusterOperatorStatusCondition{ | |
| { | |
| Type: configv1.OperatorProgressing, | |
| Message: "upgrade is in progress", | |
| Status: configv1.ConditionTrue, | |
| }, | |
| { | |
| Type: configv1.OperatorProgressing, | |
| Message: "upgrade is not in progress", | |
| Status: configv1.ConditionFalse, | |
| }, | |
| }, |
| // Returns the cluster version's current upgrade condition message for the client's cluster. | ||
| // If any error occurs while performing this operation, it will be returned. | ||
| func (u *OpenshiftUpgrader) GetUpgradeStatus(ctx context.Context) (string, error) { | ||
| clusterVersion, err := u.getClusterVersion(ctx) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| for _, condition := range clusterVersion.Status.Conditions { | ||
| if condition.Status == configv1.ConditionTrue { | ||
| return condition.Message, nil | ||
| } | ||
| } | ||
| return "", nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider filtering conditions by Type in GetUpgradeStatus
The current implementation returns the message from the first condition with status ConditionTrue without checking its Type. This might return messages from conditions unrelated to upgrades. Consider filtering for specific condition types related to upgrade status, such as configv1.OperatorProgressing.
func (u *OpenshiftUpgrader) GetUpgradeStatus(ctx context.Context) (string, error) {
clusterVersion, err := u.getClusterVersion(ctx)
if err != nil {
return "", err
}
for _, condition := range clusterVersion.Status.Conditions {
- if condition.Status == configv1.ConditionTrue {
+ if condition.Type == configv1.OperatorProgressing && condition.Status == configv1.ConditionTrue {
return condition.Message, nil
}
}
return "", nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Returns the cluster version's current upgrade condition message for the client's cluster. | |
| // If any error occurs while performing this operation, it will be returned. | |
| func (u *OpenshiftUpgrader) GetUpgradeStatus(ctx context.Context) (string, error) { | |
| clusterVersion, err := u.getClusterVersion(ctx) | |
| if err != nil { | |
| return "", err | |
| } | |
| for _, condition := range clusterVersion.Status.Conditions { | |
| if condition.Status == configv1.ConditionTrue { | |
| return condition.Message, nil | |
| } | |
| } | |
| return "", nil | |
| } | |
| // Returns the cluster version's current upgrade condition message for the client's cluster. | |
| // If any error occurs while performing this operation, it will be returned. | |
| func (u *OpenshiftUpgrader) GetUpgradeStatus(ctx context.Context) (string, error) { | |
| clusterVersion, err := u.getClusterVersion(ctx) | |
| if err != nil { | |
| return "", err | |
| } | |
| for _, condition := range clusterVersion.Status.Conditions { | |
| if condition.Type == configv1.OperatorProgressing && condition.Status == configv1.ConditionTrue { | |
| return condition.Message, nil | |
| } | |
| } | |
| return "", nil | |
| } |
There was a problem hiding this comment.
@CrystalChun is the rabbit's concern real?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Conditions is only set to one of Available, Progressing, and Degraded for ClusterVersion from the documentation https://docs.redhat.com/en/documentation/openshift_container_platform/4.18/html/config_apis/clusterversion-config-openshift-io-v1#status-4
So I didn't think we would need to check as it's all related to upgrading no matter what. However, if you prefer, I can add the type check in case this happens to change in the future.
There was a problem hiding this comment.
So in this logic we're picking up the first message of any of those conditions and report that? Is that because they are sorted from most recent to least recent?
There was a problem hiding this comment.
It sounds like these are mutually exclusive, so the logic picks up any condition that's marked as true at the time.
There was a problem hiding this comment.
Can we please add a little comment about this so the next person reading this code wouldn't be confused about it? 🙏
There was a problem hiding this comment.
Added, thank you for the feedback!
| @@ -528,6 +540,7 @@ var _ = Describe("Upgrade scenarios", func() { | |||
| condition := conditions.Get(openshiftAssistedControlPlane, controlplanev1alpha2.UpgradeCompletedCondition) | |||
| Expect(condition).NotTo(BeNil()) | |||
| Expect(condition.Status).To(Equal(corev1.ConditionFalse)) | |||
| Expect(condition.Message).To(Equal("upgrade to version 4.15.0 in progress\nupgrading cluster")) | |||
There was a problem hiding this comment.
Do we need this message? I think it's superflous (the upgrading cluster part)
There was a problem hiding this comment.
The upgrading cluster message is just a stand-in for what CVO would report back and it's going to be appended to the final message we show in our status. Do you mean I should change the message to be more relevant?
There was a problem hiding this comment.
I mean we can omit it if everything is OK: as we already say "upgrade to version X in progress" I think there's no need to append "upgrading cluster"
In case of failure, I think adding further details is great
There was a problem hiding this comment.
Gotcha! That makes sense to show only relevant information and especially not repeating things. Thanks! Removed the upgrading cluster part of the message.
|
@CrystalChun: This pull request references MGMT-19979 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 story to target the "4.19.0" version, but no target version was set. In 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 openshift-eng/jira-lifecycle-plugin repository. |
Adds the upgrade message from the ClusterVersion in the workload cluster to the OACP's upgrade status so it's transparent to the user. Also adds a failure reason in case the upgrade fails.
|
/lgtm |
openshift-merge-bot
bot
merged commit 51a99db
into
openshift-assisted:master
Adds the upgrade message from the ClusterVersion in the workload cluster to the OACP's upgrade status so it's transparent to the user. Also adds a failure reason in case the upgrade fails.
Summary by CodeRabbit
New Features
Tests