fix: (helm) - in some rare helm install cases, status is missing InstallSuccessful #3735

mikeshng · 2020-08-13T15:44:52Z

Signed-off-by: Mike Ng [email protected]

Description of the change:
In some rare cases, the InstallSuccessful reason is not stamped to the helm operator based CR after a successful helm install.
This fix is to make sure that it will keep trying to put the InstallSuccessful into the status.

Motivation for the change:
Closes #3728

Checklist

If the pull request includes user-facing changes, extra documentation is required:

[Not applicable] Add a new changelog fragment in changelog/fragments (see changelog/fragments/00-template.yaml)
[Not applicable] Add or update relevant sections of the docs website in website/content/en/docs

joelanford · 2020-08-13T16:02:40Z

internal/helm/controller/reconcile.go

+		status.SetCondition(types.HelmAppCondition{
+			Type:    types.ConditionDeployed,
+			Status:  types.StatusTrue,
+			Reason:  types.ReasonInstallSuccessful,
+			Message: manager.ReleaseMessage(),
+		})


Can you move this to line ~334? We should just always set the condition.

At that point, we can also look at expectedRelease, so I don't think we need the extra manager functions.

Lastly, we should look at the release version to know whether to set ReasonInstallSuccessful or ReasonUpgradeSuccessful.

WDYT? Will the problem be still be solved if those tweaks are made?

joelanford · 2020-08-13T16:15:50Z

Also, I think this would be a bug fix that should have a changelog fragment. Would you mind adding that as well?

camilamacedo86 · 2020-08-13T17:55:14Z

changelog/fragments/helm-install-status-fix.yaml

@@ -0,0 +1,4 @@
+entries:
+  - description: Fixed a bug that caused the Helm operator not to set the `InstallSuccessful` status.


Suggested change

- description: Fixed a bug that caused the Helm operator not to set the `InstallSuccessful` status.

- description: Ensures that the `InstallSuccessful` status will be set always by checking if the release info.

I think @mikeshng's version reads more clearly with one tweak though.

Suggested change

- description: Fixed a bug that caused the Helm operator not to set the `InstallSuccessful` status.

- description: Fixed a bug that caused the Helm operator not to set the `InstallSuccessful` and `UpgradeSuccessful` status reasons when the status update fails during installation and upgrade.

…allSuccessful Signed-off-by: Mike Ng <[email protected]>

camilamacedo86 · 2020-08-13T18:07:19Z

internal/helm/controller/reconcile.go

+	reason := types.ReasonUpgradeSuccessful
+	if expectedRelease.Version == 1 {
+		reason = types.ReasonInstallSuccessful
+	}


Now it makes more sense for me 👍

When I ran the helm ls command it showed:

NAME NAMESPACE REVISION UPDATED STATUS CHART APP VERSION nginx-sample default 1 2020-08-13 14:09:57.443165995 -0400 EDT deployed nginx-0.1.0 1.16.0

So I thought Version is APP VERSION which means its not going to be 1 for fresh install case. Then I double checked and it seems like Version is infact the REVISION from the output above so I changed it back to what Joe suggested earlier.

Signed-off-by: Mike Ng <[email protected]>

camilamacedo86 · 2020-08-13T18:11:28Z

internal/helm/controller/reconcile.go

+		Type:    types.ConditionDeployed,
+		Status:  types.StatusTrue,
+		Reason:  reason,
+		Message: expectedRelease.Info.Notes,


we need to check if the info is not nil. Such as :

operator-sdk/internal/helm/controller/reconcile.go

Lines 212 to 215 in ed95c5a

message := ""

if installedRelease.Info != nil {

message = installedRelease.Info.Notes

}

Thanks for the good catch.

…ing the Notes Signed-off-by: Mike Ng <[email protected]>

joelanford · 2020-08-13T19:00:51Z

internal/helm/internal/types/types.go

+// ContainsCondition checks if the condition with the passed condition type from
+// the status object is already present. If present then return true.
+// Otherwise, return false.
+func (s *HelmAppStatus) ContainsCondition(conditionType HelmAppConditionType) bool {
+	for i := range s.Conditions {
+		if s.Conditions[i].Type == conditionType {
+			return true
+		}
+	}
+	return false
+}
+


This function is no longer necessary, right?

…ion because it's no longer necessary Signed-off-by: Mike Ng <[email protected]>

camilamacedo86

Now it shows ok for me 👍 Great work btw 🥇
But I did not test it locally yet.

Let's see what @joelanford think about.

.

joelanford

/lgtm

Thanks for the contribution! Great find and fix!

camilamacedo86

/lgtm

…allSuccessful (operator-framework#3735) Fixed a bug that caused the Helm operator not to set the `InstallSuccessful` and `UpgradeSuccessful` status reasons when the status update fails during installation and upgrade. Closes operator-framework#3728

openshift-ci-robot requested review from fabianvf and varshaprasad96 August 13, 2020 15:44

joelanford requested changes Aug 13, 2020

View reviewed changes

camilamacedo86 reviewed Aug 13, 2020

View reviewed changes

fix: (helm) - in some rare helm install cases, status is missing Inst…

dfc33e2

…allSuccessful Signed-off-by: Mike Ng <[email protected]>

camilamacedo86 reviewed Aug 13, 2020

View reviewed changes

added more details to the changelog description

ed95c5a

Signed-off-by: Mike Ng <[email protected]>

camilamacedo86 reviewed Aug 13, 2020

View reviewed changes

avoid a nil pointer by checking to see if Info is present before gett…

b5014cf

…ing the Notes Signed-off-by: Mike Ng <[email protected]>

joelanford reviewed Aug 13, 2020

View reviewed changes

removed previously added ContainsCondition helm operator status funct…

2a4b1ef

…ion because it's no longer necessary Signed-off-by: Mike Ng <[email protected]>

camilamacedo86 previously approved these changes Aug 13, 2020

View reviewed changes

openshift-ci-robot assigned camilamacedo86 Aug 13, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 13, 2020

camilamacedo86 removed the lgtm Indicates that a PR is ready to be merged. label Aug 13, 2020

camilamacedo86 self-requested a review August 13, 2020 19:47

joelanford approved these changes Aug 13, 2020

View reviewed changes

openshift-ci-robot assigned joelanford Aug 13, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 13, 2020

camilamacedo86 approved these changes Aug 14, 2020

View reviewed changes

camilamacedo86 merged commit e0b27a9 into operator-framework:master Aug 14, 2020

mikeshng deleted the helm-install-status branch October 19, 2021 13:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: (helm) - in some rare helm install cases, status is missing InstallSuccessful #3735

fix: (helm) - in some rare helm install cases, status is missing InstallSuccessful #3735

mikeshng commented Aug 13, 2020

joelanford Aug 13, 2020

joelanford commented Aug 13, 2020

camilamacedo86 Aug 13, 2020

camilamacedo86 Aug 13, 2020

joelanford Aug 13, 2020

camilamacedo86 Aug 13, 2020

mikeshng Aug 13, 2020

camilamacedo86 Aug 13, 2020

mikeshng Aug 13, 2020

joelanford Aug 13, 2020

camilamacedo86 left a comment •

edited

Loading

joelanford left a comment

camilamacedo86 left a comment

		@@ -0,0 +1,4 @@
		entries:
		- description: Fixed a bug that caused the Helm operator not to set the `InstallSuccessful` status.

	- description: Fixed a bug that caused the Helm operator not to set the `InstallSuccessful` status.
	- description: Ensures that the `InstallSuccessful` status will be set always by checking if the release info.

	message := ""
	if installedRelease.Info != nil {
	message = installedRelease.Info.Notes
	}

fix: (helm) - in some rare helm install cases, status is missing InstallSuccessful #3735

fix: (helm) - in some rare helm install cases, status is missing InstallSuccessful #3735

Conversation

mikeshng commented Aug 13, 2020

Choose a reason for hiding this comment

joelanford commented Aug 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilamacedo86 left a comment • edited Loading

Choose a reason for hiding this comment

joelanford left a comment

Choose a reason for hiding this comment

camilamacedo86 left a comment

Choose a reason for hiding this comment

camilamacedo86 left a comment •

edited

Loading