Skip to content

Conversation

@anjoshi24
Copy link
Contributor

@anjoshi24 anjoshi24 commented Nov 2, 2021

What type of PR is this?

Refactor

What this PR does / why we need it?

MUO needs to check CVO's ack gate condition before the upgrade and make decision based on the status.

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

https://issues.redhat.com/browse/OSD-8404
Fixes #

Special notes for your reviewer:

Pre-checks (if applicable):

  • Tested latest changes against a cluster
  • Ran make generate command locally to validate code changes
  • Included documentation changes with PR

Aniket Joshi added 2 commits November 1, 2021 12:34
Signed-off-by: Aniket Joshi <anjoshi@redhat.com>
Signed-off-by: Aniket Joshi <anjoshi@redhat.com>
@openshift-ci openshift-ci bot requested review from bmeng and dofinn November 2, 2021 05:17
@mrbarge
Copy link
Contributor

mrbarge commented Nov 2, 2021

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 2, 2021
@mrbarge
Copy link
Contributor

mrbarge commented Nov 2, 2021

cc @Tessg22 FYI, above is my initial review on the code so far

@mrbarge
Copy link
Contributor

mrbarge commented Nov 8, 2021

Leaving one more comment on this PR. I think it will be relatively straightforward to get specific service log messaging going that can inform the cluster owner that the upgrade failed specifically due to the upgrade gate issue.

These changes will mainly need to occur in the event manager code (https://github.com/openshift/managed-upgrade-operator/blob/master/pkg/eventmanager/eventmanager.go).

Specifically:

  • A new description const for the 'failed' event states. (a 'delayed' one is not necessary IMO)
	// UPGRADE_UPGRADEABLE_FAILED_DESC describes the upgradeable check failure
	UPGRADE_UPGRADEABLE_FAILED_DESC = "Cluster upgrade to version %s was cancelled because manual acknowledgement is required concerning the removal of deprecated APIs. Please review the Knowledge Base article at https://access.redhat.com/articles/6329921 and perform the acknowledgement procedure if you are not using workloads that require the deprecated APIs and wish to proceed with the upgrade. Automated upgrades will be retried on their next scheduling cycle. If you have manually scheduled an upgrade instead, it must be rescheduled."
  • Update the createFailureDescription function, in particular this switch condition, to use the aforementioned UPGRADE_UPGRADEABLE_FAILED_DESC const if the failedCondition is IsClusterUpgradable.

@codecov-commenter
Copy link

Codecov Report

Merging #289 (84e65cf) into master (3885848) will increase coverage by 0.05%.
The diff coverage is 36.36%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #289      +/-   ##
==========================================
+ Coverage   50.61%   50.67%   +0.05%     
==========================================
  Files         110      111       +1     
  Lines        4453     4460       +7     
==========================================
+ Hits         2254     2260       +6     
+ Misses       2027     2023       -4     
- Partials      172      177       +5     
Impacted Files Coverage Δ
pkg/apis/upgrade/v1alpha1/upgradeconfig_types.go 52.85% <ø> (ø)
pkg/eventmanager/eventmanager.go 53.46% <0.00%> (-1.09%) ⬇️
pkg/upgraders/osdupgrader.go 0.00% <0.00%> (ø)
pkg/upgraders/upgradeable.go 40.00% <40.00%> (ø)
pkg/pod/pod.go 96.15% <0.00%> (-3.85%) ⬇️
pkg/drain/strategy.go 0.00% <0.00%> (ø)
...ntroller/upgradeconfig/upgradeconfig_controller.go 59.14% <0.00%> (+1.06%) ⬆️
pkg/clusterversion/cv.go 61.65% <0.00%> (+6.76%) ⬆️
pkg/drain/podDeleteStrategy.go 100.00% <0.00%> (+6.89%) ⬆️
pkg/drain/stuckTerminatingStrategy.go 100.00% <0.00%> (+6.89%) ⬆️
... and 1 more

@mrbarge
Copy link
Contributor

mrbarge commented Nov 16, 2021

One final comment in terms of testing. First off, thanks for adding tests, that's awesome!

If they could be expanded to cover some additional scenarios that would be great and is the last thing this PR really needs.

  • If the cluster is performing a z-stream upgrade: It should perform upgrade
  • If the cluster is performing a y-stream upgrade:
    • If the cluster has no Upgradeable condition: It should perform upgrade
    • If the cluster is Upgradeable: it should perform upgradeable
    • If the cluster is not Upgradeable: it should not perform upgrade (it has this test already)

exist
Co-authored-by: Aniket Joshi <anjoshi@redhat.com>
@mrbarge
Copy link
Contributor

mrbarge commented Nov 26, 2021

/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 26, 2021
@mrbarge
Copy link
Contributor

mrbarge commented Nov 26, 2021

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 26, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 26, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: anjoshi24, mrbarge

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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 26, 2021
@openshift-merge-robot openshift-merge-robot merged commit b03430a into openshift:master Nov 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants