✨ Use Helm List operator to determine Deployed status #1379

tmshort · 2024-10-15T20:35:35Z

Latest fixes for #1296

Description

Reviewer Checklist

API Go Documentation
Tests: Unit Tests (and E2E Tests, if appropriate)
Comprehensive Commit Messages
Links to related GitHub Issue(s)

netlify · 2024-10-15T20:35:52Z

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	`b557ff6`
🔍 Latest deploy log	https://app.netlify.com/sites/olmv1/deploys/6719438237399b00083293e7
😎 Deploy Preview	https://deploy-preview-1379--olmv1.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

codecov · 2024-10-15T20:45:41Z

Codecov Report

Attention: Patch coverage is 73.43750% with 17 lines in your changes missing coverage. Please review.

Project coverage is 73.40%. Comparing base (6f42274) to head (b557ff6).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...nternal/controllers/clusterextension_controller.go	77.50%	7 Missing and 2 partials ⚠️
internal/controllers/common_controller.go	60.00%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1379      +/-   ##
==========================================
+ Coverage   73.31%   73.40%   +0.09%     
==========================================
  Files          42       42              
  Lines        3166     3200      +34     
==========================================
+ Hits         2321     2349      +28     
- Misses        659      664       +5     
- Partials      186      187       +1

Flag	Coverage Δ
e2e	`55.03% <71.87%> (+0.19%)`	⬆️
unit	`53.25% <73.43%> (+0.69%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

internal/controllers/clusterextension_controller.go

internal/controllers/common_controller.go

internal/controllers/clusterextension_controller.go

internal/controllers/clusterextension_controller_test.go

internal/controllers/clusterextension_controller.go

everettraven · 2024-10-21T18:10:33Z

internal/controllers/clusterextension_controller.go

+			// If there are blank values, we should consider this as not installed
+			if n, ok := rel.Labels[labels.BundleNameKey]; !ok || n == "" {
+				return nil, nil
+			}
+			if v, ok := rel.Labels[labels.BundleVersionKey]; !ok || v == "" {
+				return nil, nil
+			}


Should we return early in these cases or continue looking for the next successfully deployed release that contains these labels?

Is there any case where we wouldn't have set the labels on a release or is the absence of these labels indicative of a much bigger issue?

These checks should be able to be removed once we do a release, as they're only necessary for the upgrade.

We didn't previously check for for empty, but we still might want to consider doing it.

Is there any case where we wouldn't have set the labels on a release or is the absence of these labels indicative of a much bigger issue?

I was a bit concerned about this possibility. If the labels are empty, we don't know the bundle or version, so something is wrong.

After v1, it seems there are three possibilities for missing labels on the release:

We make a new release that adds a new label (say in 1.2), but we just upgraded from 1.1, so any releases that exist prior to 1.2 would not have all of 1.2's expected labels.

Something deleted our labels.

We introduced a bug somehow where we didn't set the labels.

internal/controllers/clusterextension_controller_test.go

everettraven · 2024-10-21T18:17:02Z

internal/controllers/common_controller.go

+		// If both conditions occur, this is a failed Apply when a bundle was already installed
+		if err != nil {
+			setInstalledStatusConditionFailed(ext, err.Error())


Wouldn't we want to preserve the historical installation information on a failed "upgrade" attempt, which is what I interpret a "failed Apply when a bundle was already installed" to mean.

I was concerned about this, but the problem is that we lose the fact that we failed an upgrade. I'm not sure which is better. But this is what @joelanford suggested.

Another way to look at it: if we tried an install, we store the bundle information. Should we not have the same information on an upgrade?
I almost think we need to have an upgrade field...

There's additional discussion here: #1379 (comment)

Also note, that this change (no longer preserving Installed after the Apply()) is a separate fixup commit, so it can be looked at in isolation.

If a ClusterExtension has been installed successfully in the past, we should keep the installed condition as is and include the upgrade failure information in the progressing condition. The resolution information can be included in the error message so that a user can better connect the dots that an upgrade failed and what bundles were involved.

joelanford · 2024-10-22T19:03:05Z

internal/controllers/clusterextension_controller.go

+			// If there are blank values, we should consider this as not installed
+			if n, ok := rel.Labels[labels.BundleNameKey]; !ok || n == "" {
+				return nil, nil
+			}
+			if v, ok := rel.Labels[labels.BundleVersionKey]; !ok || v == "" {
+				return nil, nil
+			}


I personally think we should return an error if the expected labels are missing. Or the resolver should return an error if the installedBundle is non-nil, but then expected struct fields are unset.

I don't think it makes sense to assume "not installed" because clearly something is installed.

This occurs in the e2e-upgrade; seemingly. But we can't tell if it's a temporary property of the lack of a helm-operater-plugin update, or something else.

I plan to clean this up after a release and a clean upgrade-e2e.

internal/controllers/common_controller.go

internal/controllers/clusterextension_controller.go

joelanford · 2024-10-23T16:51:00Z

internal/controllers/clusterextension_controller.go

+		// Now that we're actually trying to install, use the error
+		setInstalledStatusFromBundle(ext, installedBundle, err)


Yesterday we said:

Failed install

status.conditions[Installed] = false

Failed upgrade

status.conditions[Installed] = true, message indicates successfully installed bundle ref

So should this be:

Suggested change

// Now that we're actually trying to install, use the error

setInstalledStatusFromBundle(ext, installedBundle, err)

// Now that we're actually trying to install, use the error

setInstalledStatusFromBundle(ext, installedBundle, nil)

No, we want the err in there, the code is correct.

A failed (initial) install means that installedBundle is nil, and err is not nil, so the following happens:

setInstallStatus(ext, nil) setInstalledStatusConditionFailed(ext, err.Error())

A failed upgrade means that installedBundle is not nil, and err is not nil, so the following happens:

setInstallStatus(ext, installStatus) setInstalledStatusConditionSuccess(ext, fmt.Sprintf("Installed bundle %s successfully", installedBundle.Image))

If err is nil in here, then the result will be a message of "No install bundle found", rather than the installation error from err.

Signed-off-by: Todd Short <[email protected]>

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 15, 2024

joelanford reviewed Oct 15, 2024

View reviewed changes

internal/controllers/clusterextension_controller.go Outdated Show resolved Hide resolved

joelanford reviewed Oct 15, 2024

View reviewed changes

internal/controllers/clusterextension_controller.go Outdated Show resolved Hide resolved

joelanford reviewed Oct 15, 2024

View reviewed changes

internal/controllers/clusterextension_controller.go Show resolved Hide resolved

tmshort force-pushed the helm-status branch from 95190ba to 6911043 Compare October 16, 2024 16:02

joelanford reviewed Oct 16, 2024

View reviewed changes

internal/controllers/clusterextension_controller.go Outdated Show resolved Hide resolved

joelanford reviewed Oct 16, 2024

View reviewed changes

internal/controllers/clusterextension_controller.go Outdated Show resolved Hide resolved

tmshort force-pushed the helm-status branch from 6911043 to a7a1680 Compare October 16, 2024 17:26

joelanford reviewed Oct 16, 2024

View reviewed changes

internal/controllers/clusterextension_controller.go Outdated Show resolved Hide resolved

tmshort force-pushed the helm-status branch from a7a1680 to ce898ec Compare October 16, 2024 18:22

joelanford reviewed Oct 16, 2024

View reviewed changes

internal/controllers/common_controller.go Outdated Show resolved Hide resolved

tmshort force-pushed the helm-status branch from ce898ec to 8713957 Compare October 16, 2024 18:45

joelanford reviewed Oct 17, 2024

View reviewed changes

internal/controllers/clusterextension_controller.go Outdated Show resolved Hide resolved

tmshort force-pushed the helm-status branch from 8713957 to 0f21db4 Compare October 17, 2024 21:22

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 17, 2024

tmshort force-pushed the helm-status branch from 0f21db4 to 21f4d94 Compare October 17, 2024 21:26

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 17, 2024

tmshort marked this pull request as ready for review October 17, 2024 21:26

tmshort requested a review from a team as a code owner October 17, 2024 21:26

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 17, 2024

tmshort force-pushed the helm-status branch from 21f4d94 to 8859eb4 Compare October 17, 2024 21:28

joelanford reviewed Oct 18, 2024

View reviewed changes

internal/controllers/clusterextension_controller.go Outdated Show resolved Hide resolved

joelanford reviewed Oct 18, 2024

View reviewed changes

internal/controllers/clusterextension_controller_test.go Show resolved Hide resolved

tmshort force-pushed the helm-status branch 2 times, most recently from 0601479 to 170974b Compare October 18, 2024 16:18

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 19, 2024

tmshort force-pushed the helm-status branch from 170974b to d29117c Compare October 21, 2024 04:08

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 21, 2024

everettraven reviewed Oct 21, 2024

View reviewed changes

internal/controllers/clusterextension_controller.go Show resolved Hide resolved

everettraven reviewed Oct 21, 2024

View reviewed changes

internal/controllers/clusterextension_controller.go Show resolved Hide resolved

everettraven reviewed Oct 21, 2024

View reviewed changes

internal/controllers/clusterextension_controller_test.go Outdated Show resolved Hide resolved

everettraven reviewed Oct 21, 2024

View reviewed changes

internal/controllers/clusterextension_controller_test.go Outdated Show resolved Hide resolved

everettraven reviewed Oct 21, 2024

View reviewed changes

tmshort force-pushed the helm-status branch 3 times, most recently from 7d8a1ae to 1b55cde Compare October 22, 2024 14:18

LalatenduMohanty mentioned this pull request Oct 22, 2024

Ensure rollouts are completed before starting a new upgrade. #1296

Closed

joelanford reviewed Oct 22, 2024

View reviewed changes

tmshort force-pushed the helm-status branch from 1b55cde to e403e45 Compare October 22, 2024 19:49

everettraven reviewed Oct 22, 2024

View reviewed changes

internal/controllers/common_controller.go Outdated Show resolved Hide resolved

tmshort force-pushed the helm-status branch from e403e45 to 481a142 Compare October 22, 2024 20:46

LalatenduMohanty reviewed Oct 23, 2024

View reviewed changes

internal/controllers/clusterextension_controller.go Outdated Show resolved Hide resolved

tmshort force-pushed the helm-status branch from 481a142 to f8b11bd Compare October 23, 2024 15:17

joelanford reviewed Oct 23, 2024

View reviewed changes

tmshort force-pushed the helm-status branch 2 times, most recently from 8e97ae0 to 1b74d9f Compare October 23, 2024 18:03

joelanford previously approved these changes Oct 23, 2024

View reviewed changes

Use Helm List operator to determine Deployed status

b557ff6

Signed-off-by: Todd Short <[email protected]>

tmshort dismissed joelanford’s stale review via b557ff6 October 23, 2024 18:42

tmshort force-pushed the helm-status branch from 1b74d9f to b557ff6 Compare October 23, 2024 18:42

tmshort enabled auto-merge October 23, 2024 19:00

joelanford approved these changes Oct 23, 2024

View reviewed changes

tmshort added this pull request to the merge queue Oct 23, 2024

Merged via the queue into operator-framework:main with commit 1cb254c Oct 23, 2024
19 checks passed

tmshort deleted the helm-status branch October 23, 2024 20:23

everettraven mentioned this pull request Oct 24, 2024

🐛 don't set installed status unless it is successful #1368

Closed

4 tasks

		// Now that we're actually trying to install, use the error
		setInstalledStatusFromBundle(ext, installedBundle, err)

✨ Use Helm List operator to determine Deployed status #1379

✨ Use Helm List operator to determine Deployed status #1379

Uh oh!

Conversation

tmshort commented Oct 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Reviewer Checklist

Uh oh!

netlify bot commented Oct 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for olmv1 ready!

Uh oh!

codecov bot commented Oct 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmshort Oct 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

joelanford Oct 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tmshort commented Oct 15, 2024 •

edited

Loading

netlify bot commented Oct 15, 2024 •

edited

Loading

codecov bot commented Oct 15, 2024 •

edited

Loading

tmshort Oct 22, 2024 •

edited

Loading

joelanford Oct 23, 2024 •

edited

Loading