-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
pkg/helm: use status conditions, update status for failures #814
Changes from 6 commits
7140ec6
4b2a6e2
c9431bc
5580c03
b31e603
acf2202
ca6dc20
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -80,14 +80,29 @@ func (r HelmOperatorReconciler) Reconcile(request reconcile.Request) (reconcile. | |
log.V(1).Info("Adding finalizer", "finalizer", finalizer) | ||
finalizers := append(pendingFinalizers, finalizer) | ||
o.SetFinalizers(finalizers) | ||
err := r.Client.Update(context.TODO(), o) | ||
return reconcile.Result{}, err | ||
err = r.updateResource(o) | ||
|
||
// Need to requeue because finalizer update does not change metadata.generation | ||
return reconcile.Result{Requeue: true}, err | ||
} | ||
|
||
status.SetCondition(types.HelmAppCondition{ | ||
Type: types.ConditionInitialized, | ||
Status: types.StatusTrue, | ||
}) | ||
|
||
if err := manager.Sync(context.TODO()); err != nil { | ||
log.Error(err, "failed to sync release") | ||
status.SetCondition(types.HelmAppCondition{ | ||
Type: types.ConditionIrreconcilable, | ||
Status: types.StatusTrue, | ||
Reason: types.ReasonReconcileError, | ||
Message: err.Error(), | ||
}) | ||
_ = r.updateResourceStatus(o, status) | ||
return reconcile.Result{}, err | ||
} | ||
status.RemoveCondition(types.ConditionIrreconcilable) | ||
|
||
if deleted { | ||
if !contains(pendingFinalizers, finalizer) { | ||
|
@@ -98,41 +113,75 @@ func (r HelmOperatorReconciler) Reconcile(request reconcile.Request) (reconcile. | |
uninstalledRelease, err := manager.UninstallRelease(context.TODO()) | ||
if err != nil && err != release.ErrNotFound { | ||
log.Error(err, "failed to uninstall release") | ||
status.SetCondition(types.HelmAppCondition{ | ||
Type: types.ConditionReleaseFailed, | ||
Status: types.StatusTrue, | ||
Reason: types.ReasonUninstallError, | ||
Message: err.Error(), | ||
}) | ||
_ = r.updateResourceStatus(o, status) | ||
return reconcile.Result{}, err | ||
} | ||
status.RemoveCondition(types.ConditionReleaseFailed) | ||
|
||
if err == release.ErrNotFound { | ||
log.Info("Release not found, removing finalizer") | ||
} else { | ||
log.Info("Uninstalled release") | ||
if log.Enabled() { | ||
fmt.Println(diffutil.Diff(uninstalledRelease.GetManifest(), "")) | ||
} | ||
status.SetCondition(types.HelmAppCondition{ | ||
Type: types.ConditionDeployed, | ||
Status: types.StatusFalse, | ||
Reason: types.ReasonUninstallSuccessful, | ||
}) | ||
} | ||
if err := r.updateResourceStatus(o, status); err != nil { | ||
return reconcile.Result{}, err | ||
} | ||
|
||
finalizers := []string{} | ||
for _, pendingFinalizer := range pendingFinalizers { | ||
if pendingFinalizer != finalizer { | ||
finalizers = append(finalizers, pendingFinalizer) | ||
} | ||
} | ||
o.SetFinalizers(finalizers) | ||
err = r.Client.Update(context.TODO(), o) | ||
return reconcile.Result{}, err | ||
err = r.updateResource(o) | ||
|
||
// Need to requeue because finalizer update does not change metadata.generation | ||
return reconcile.Result{Requeue: true}, err | ||
} | ||
|
||
if !manager.IsInstalled() { | ||
installedRelease, err := manager.InstallRelease(context.TODO()) | ||
if err != nil { | ||
log.Error(err, "failed to install release") | ||
status.SetCondition(types.HelmAppCondition{ | ||
Type: types.ConditionReleaseFailed, | ||
Status: types.StatusTrue, | ||
Reason: types.ReasonInstallError, | ||
Message: err.Error(), | ||
Release: installedRelease, | ||
}) | ||
_ = r.updateResourceStatus(o, status) | ||
return reconcile.Result{}, err | ||
} | ||
status.RemoveCondition(types.ConditionReleaseFailed) | ||
|
||
log.Info("Installed release") | ||
if log.Enabled() { | ||
fmt.Println(diffutil.Diff("", installedRelease.GetManifest())) | ||
} | ||
log.V(1).Info("Config values", "values", installedRelease.GetConfig()) | ||
status.SetRelease(installedRelease) | ||
status.SetPhase(types.PhaseApplied, types.ReasonApplySuccessful, installedRelease.GetInfo().GetStatus().GetNotes()) | ||
status.SetCondition(types.HelmAppCondition{ | ||
Type: types.ConditionDeployed, | ||
Status: types.StatusTrue, | ||
Reason: types.ReasonInstallSuccessful, | ||
Message: installedRelease.GetInfo().GetStatus().GetNotes(), | ||
Release: installedRelease, | ||
}) | ||
err = r.updateResourceStatus(o, status) | ||
return reconcile.Result{RequeueAfter: r.ResyncPeriod}, err | ||
} | ||
|
@@ -141,27 +190,55 @@ func (r HelmOperatorReconciler) Reconcile(request reconcile.Request) (reconcile. | |
previousRelease, updatedRelease, err := manager.UpdateRelease(context.TODO()) | ||
if err != nil { | ||
log.Error(err, "failed to update release") | ||
status.SetCondition(types.HelmAppCondition{ | ||
Type: types.ConditionReleaseFailed, | ||
Status: types.StatusTrue, | ||
Reason: types.ReasonUpdateError, | ||
Message: err.Error(), | ||
Release: updatedRelease, | ||
}) | ||
_ = r.updateResourceStatus(o, status) | ||
return reconcile.Result{}, err | ||
} | ||
status.RemoveCondition(types.ConditionReleaseFailed) | ||
|
||
log.Info("Updated release") | ||
if log.Enabled() { | ||
fmt.Println(diffutil.Diff(previousRelease.GetManifest(), updatedRelease.GetManifest())) | ||
} | ||
log.V(1).Info("Config values", "values", updatedRelease.GetConfig()) | ||
status.SetRelease(updatedRelease) | ||
status.SetPhase(types.PhaseApplied, types.ReasonApplySuccessful, updatedRelease.GetInfo().GetStatus().GetNotes()) | ||
status.SetCondition(types.HelmAppCondition{ | ||
Type: types.ConditionDeployed, | ||
Status: types.StatusTrue, | ||
Reason: types.ReasonUpdateSuccessful, | ||
Message: updatedRelease.GetInfo().GetStatus().GetNotes(), | ||
Release: updatedRelease, | ||
}) | ||
err = r.updateResourceStatus(o, status) | ||
return reconcile.Result{RequeueAfter: r.ResyncPeriod}, err | ||
} | ||
|
||
_, err = manager.ReconcileRelease(context.TODO()) | ||
if err != nil { | ||
log.Error(err, "failed to reconcile release") | ||
status.SetCondition(types.HelmAppCondition{ | ||
Type: types.ConditionIrreconcilable, | ||
Status: types.StatusTrue, | ||
Reason: types.ReasonReconcileError, | ||
Message: err.Error(), | ||
}) | ||
_ = r.updateResourceStatus(o, status) | ||
return reconcile.Result{}, err | ||
} | ||
status.RemoveCondition(types.ConditionIrreconcilable) | ||
|
||
log.Info("Reconciled release") | ||
return reconcile.Result{RequeueAfter: r.ResyncPeriod}, nil | ||
err = r.updateResourceStatus(o, status) | ||
return reconcile.Result{RequeueAfter: r.ResyncPeriod}, err | ||
} | ||
|
||
func (r HelmOperatorReconciler) updateResource(o *unstructured.Unstructured) error { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is just my sensibilities, but why do we have this function if it is not abstracting anything but a different function call? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair question. I was on the fence about this, but I went with it to emphasize the distinction between updating the non-subresources (e.g. metadata, spec, etc.) and the status subresource. So with this we'd have |
||
return r.Client.Update(context.TODO(), o) | ||
} | ||
|
||
func (r HelmOperatorReconciler) updateResourceStatus(o *unstructured.Unstructured, status *types.HelmAppStatus) error { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized this is causing immediate reconciliations even when nothing has changed, so I'll need to come up with a way to prevent this.
Are we updating the CRD scaffold to use the
status
subresource when we move to 1.12?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joelanford We're trying to do it before 1.12 actually, as it seems CRD subresources are already beta in 1.11 and on by default.
#787 (comment)
https://github.com/operator-framework/operator-sdk/pull/787/files#diff-f44ebe3a96e65181844d62f38ed5a148R60
We can use the status client to only update the status sub resource and have a predicate to filter out status updates by checking
metadata.generation
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is getting added here: #787