This repository has been archived by the owner on Jan 25, 2019. It is now read-only.
Improve release name uniqueness and ownership #57
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -69,8 +69,10 @@ const ( | |
| // API_VERSION and KIND environment variables. | ||
| HelmChartEnvVar = "HELM_CHART" | ||
|
|
||
| defaultHelmChartWatchesFile = "/opt/helm/watches.yaml" | ||
|
|
||
| annotationReleaseName = "helm.operator-sdk/release-name" | ||
| annotationUseNameAsReleaseName = "helm.operator-sdk/use-name-as-release-name" | ||
| ) | ||
|
|
||
| // Installer can install and uninstall Helm releases given a custom resource | ||
|
|
@@ -235,8 +237,10 @@ func (c installer) ReconcileRelease(r *unstructured.Unstructured) (*unstructured | |
| return r, needsUpdate, fmt.Errorf("failed to sync release status: %s", err) | ||
| } | ||
|
|
||
| releaseName := getReleaseName(r) | ||
|
|
||
| // Get release history for this release name | ||
| releases, err := c.storageBackend.History(releaseName) | ||
| if err != nil && !notFoundErr(err) { | ||
| return r, needsUpdate, fmt.Errorf("failed to retrieve release history: %s", err) | ||
| } | ||
|
|
@@ -254,30 +258,36 @@ func (c installer) ReconcileRelease(r *unstructured.Unstructured) (*unstructured | |
| } | ||
|
|
||
| var updatedRelease *release.Release | ||
| latestRelease, err := c.storageBackend.Deployed(releaseName) | ||
| if err != nil || latestRelease == nil { | ||
| // If there's no deployed release, attempt a tiller install. | ||
| updatedRelease, err = c.installRelease(tiller, r.GetNamespace(), releaseName, chart, config) | ||
| if err != nil { | ||
| return r, needsUpdate, fmt.Errorf("install error: %s", err) | ||
| } | ||
| needsUpdate = true | ||
| logrus.Infof("Installed release for %s release=%s", ResourceString(r), updatedRelease.GetName()) | ||
|
|
||
| } else if status.Release == nil { | ||
| // If the object has no release status, it does not own the release, | ||
| // so return an error. | ||
| return r, needsUpdate, fmt.Errorf("install error: release \"%s\" already exists", releaseName) | ||
| } else { | ||
| candidateRelease, err := c.getCandidateRelease(tiller, releaseName, chart, config) | ||
| if err != nil { | ||
| return r, needsUpdate, fmt.Errorf("failed to generate candidate release: %s", err) | ||
| } | ||
|
|
||
| latestManifest := latestRelease.GetManifest() | ||
| if latestManifest == candidateRelease.GetManifest() { | ||
| err = c.reconcileRelease(r.GetNamespace(), latestManifest) | ||
| if err != nil { | ||
| return r, needsUpdate, fmt.Errorf("reconcile error: %s", err) | ||
| } | ||
| updatedRelease = latestRelease | ||
| logrus.Infof("Reconciled release for %s release=%s", ResourceString(r), updatedRelease.GetName()) | ||
| } else { | ||
| updatedRelease, err = c.updateRelease(tiller, releaseName, chart, config) | ||
| if err != nil { | ||
| return r, needsUpdate, fmt.Errorf("update error: %s", err) | ||
| } | ||
|
|
@@ -298,8 +308,17 @@ func (c installer) ReconcileRelease(r *unstructured.Unstructured) (*unstructured | |
| // UninstallRelease accepts a custom resource, uninstalls the existing Helm release | ||
| // using Tiller, and returns the custom resource with updated `status`. | ||
| func (c installer) UninstallRelease(r *unstructured.Unstructured) (*unstructured.Unstructured, error) { | ||
| // If the object has no release status, it does not own the release, | ||
| // so there's nothing to do. | ||
| status := v1alpha1.StatusFor(r) | ||
| if status.Release == nil { | ||
| return r, nil | ||
| } | ||
|
|
||
| releaseName := getReleaseName(r) | ||
|
|
||
| // Get history of this release | ||
| h, err := c.storageBackend.History(releaseName) | ||
| if err != nil { | ||
| return r, fmt.Errorf("failed to get release history: %s", err) | ||
| } | ||
|
|
@@ -312,13 +331,13 @@ func (c installer) UninstallRelease(r *unstructured.Unstructured) (*unstructured | |
|
|
||
| tiller := c.tillerRendererForCR(r) | ||
| _, err = tiller.UninstallRelease(context.TODO(), &services.UninstallReleaseRequest{ | ||
| Name: releaseName, | ||
| Purge: true, | ||
| }) | ||
| if err != nil { | ||
| return r, err | ||
| } | ||
| logrus.Infof("Uninstalled release for %s release=%s", ResourceString(r), releaseName) | ||
| return r, nil | ||
| } | ||
|
|
||
|
|
@@ -327,37 +346,55 @@ func ResourceString(r *unstructured.Unstructured) string { | |
| return fmt.Sprintf("apiVersion=%s kind=%s name=%s/%s", r.GetAPIVersion(), r.GetKind(), r.GetNamespace(), r.GetName()) | ||
| } | ||
|
|
||
| func (c installer) installRelease(tiller *tiller.ReleaseServer, namespace, name string, chart *cpb.Chart, config *cpb.Config) (*release.Release, error) { | ||
| installReq := &services.InstallReleaseRequest{ | ||
| Namespace: namespace, | ||
| Name: name, | ||
| Chart: chart, | ||
| Values: config, | ||
| } | ||
|
|
||
| releaseResponse, err := tiller.InstallRelease(context.TODO(), installReq) | ||
| if err != nil { | ||
| // Workaround for helm/helm#3338 | ||
| uninstallReq := &services.UninstallReleaseRequest{ | ||
| Name: releaseResponse.GetRelease().GetName(), | ||
| Purge: true, | ||
| } | ||
| _, uninstallErr := tiller.UninstallRelease(context.TODO(), uninstallReq) | ||
| if uninstallErr != nil { | ||
| return nil, fmt.Errorf("failed to roll back failed installation: %s: %s", uninstallErr, err) | ||
| } | ||
| return nil, err | ||
| } | ||
| return releaseResponse.GetRelease(), nil | ||
| } | ||
|
|
||
| func (c installer) updateRelease(tiller *tiller.ReleaseServer, name string, chart *cpb.Chart, config *cpb.Config) (*release.Release, error) { | ||
| updateReq := &services.UpdateReleaseRequest{ | ||
| Name: name, | ||
| Chart: chart, | ||
| Values: config, | ||
| } | ||
|
|
||
| releaseResponse, err := tiller.UpdateRelease(context.TODO(), updateReq) | ||
| if err != nil { | ||
| // Workaround for helm/helm#3338 | ||
| rollbackReq := &services.RollbackReleaseRequest{ | ||
| Name: name, | ||
| Force: true, | ||
| } | ||
| _, rollbackErr := tiller.RollbackRelease(context.TODO(), rollbackReq) | ||
| if rollbackErr != nil { | ||
| return nil, fmt.Errorf("failed to roll back failed update: %s: %s", rollbackErr, err) | ||
| } | ||
| return nil, err | ||
| } | ||
| return releaseResponse.GetRelease(), nil | ||
| } | ||
|
|
||
| func (c installer) reconcileRelease(namespace string, expectedManifest string) error { | ||
| expectedInfos, err := c.tillerKubeClient.BuildUnstructured(namespace, bytes.NewBufferString(expectedManifest)) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
@@ -387,9 +424,9 @@ func (c installer) reconcileRelease(r *unstructured.Unstructured, expectedManife | |
| }) | ||
| } | ||
|
|
||
| func (c installer) getCandidateRelease(tiller *tiller.ReleaseServer, name string, chart *cpb.Chart, config *cpb.Config) (*release.Release, error) { | ||
| dryRunReq := &services.UpdateReleaseRequest{ | ||
| Name: name, | ||
| Chart: chart, | ||
| Values: config, | ||
| DryRun: true, | ||
|
|
@@ -442,8 +479,21 @@ func (c installer) tillerRendererForCR(r *unstructured.Unstructured) *tiller.Rel | |
| return tiller.NewReleaseServer(env, internalClientSet, false) | ||
| } | ||
|
|
||
| func getReleaseName(r *unstructured.Unstructured) string { | ||
| status := v1alpha1.StatusFor(r) | ||
| if status.Release != nil { | ||
|
There was a problem hiding this comment. @joelanford The implementation seems fine to me but I'm thinking we should try our best to avoid relying on Status for our control logic. That seems to be the upstream convention as well. https://github.com/kubernetes/community/blob/master/contributors/devel/api-conventions.md#spec-and-status
The Right now with this implementation what happens if:
Ideally we could make the UUID part of the release name e.g There was a problem hiding this comment. Yeah, you're right. That would result in a separate new release. Thanks for linking the conventions and principles. That's good info! I did think about using UUIDs in the release name, but there are some downsides:
We could do something like If we want better human readability, another option could be that we add the release name to the CR (somewhere in |
||
| return status.Release.GetName() | ||
| } | ||
| if v, ok := r.GetAnnotations()[annotationReleaseName]; ok { | ||
| return v | ||
| } | ||
| if v, ok := r.GetAnnotations()[annotationUseNameAsReleaseName]; ok && v == "true" { | ||
| return r.GetName() | ||
| } | ||
|
|
||
| // An empty release name will be populated automatically by tiller | ||
| // during installation | ||
| return "" | ||
| } | ||
|
|
||
| func notFoundErr(err error) bool { | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 I'm wondering if this scenario is possible:
c.installRelease()status.Releasec.storageBackend.Deployed()returnslatestRelease, nil.status.Release==nilfrom our last failed status update and we return an error thinking this release is not owned by the CR(even though it actually is).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.
Yeah, that definitely sounds possible. Off the top of my head, I can think of a few things we can try:
status.Releasewas, or uninstall if it was nil. Depending on the error, the rollback may also fail.c.storageBackend.Deployed()as the latestRelease, usestatus.Release.c.storageBackend.Deployed()would "know" its owner.Each of these may cause other interesting failure scenarios though, so I'd have to experiment to know whether any of these are feasible.
Any other ideas?
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.
Turns out it's pretty easy to add an arbitrary config value to the release, which gets saved with the release in the storage backend. It seems like this is probably more robust than the other options. The latest commit changes the ownership checks to use this method instead.