Skip to content
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.

Sign up for GitHub

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

Jump to bottom

pkg/reconciler: force upgrade if release is failed or superseded #81

Merged
merged 2 commits into from
Jan 25, 2021
Merged

pkg/reconciler: force upgrade if release is failed or superseded #81

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6865997
pkg/reconciler: do upgrade if last release isn't deployed
joelanford Jan 25, 2021
891f081
pkg/reconciler: add tests for upgrading Failed or Superseded releases
joelanford Jan 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,10 @@ all: test lint build
# Run tests
.PHONY: test
export KUBEBUILDER_ASSETS := $(TOOLS_BIN_DIR)
TESTPKG ?= ./...
CR_VERSION=$(shell go list -m sigs.k8s.io/controller-runtime | cut -d" " -f2 | sed 's/^v//')
test:
fetch envtest $(CR_VERSION) && go test -race -covermode atomic -coverprofile cover.out ./...
fetch envtest $(CR_VERSION) && go test -race -covermode atomic -coverprofile cover.out $(TESTPKG)

# Build manager binary
.PHONY: build
Expand Down
12 changes: 7 additions & 5 deletions pkg/reconciler/reconciler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -582,7 +582,7 @@ func (r *Reconciler) handleDeletion(ctx context.Context, actionClient helmclient
}

func (r *Reconciler) getReleaseState(client helmclient.ActionInterface, obj metav1.Object, vals map[string]interface{}) (*release.Release, helmReleaseState, error) {
deployedRelease, err := client.Get(obj.GetName())
currentRelease, err := client.Get(obj.GetName())
if err != nil && !errors.Is(err, driver.ErrReleaseNotFound) {
return nil, stateError, err
}
Expand All @@ -603,12 +603,14 @@ func (r *Reconciler) getReleaseState(client helmclient.ActionInterface, obj meta
})
specRelease, err := client.Upgrade(obj.GetName(), obj.GetNamespace(), r.chrt, vals, opts...)
if err != nil {
return deployedRelease, stateError, err
return currentRelease, stateError, err
}
if specRelease.Manifest != deployedRelease.Manifest {
return deployedRelease, stateNeedsUpgrade, nil
if specRelease.Manifest != currentRelease.Manifest ||
currentRelease.Info.Status == release.StatusFailed ||
currentRelease.Info.Status == release.StatusSuperseded {
return currentRelease, stateNeedsUpgrade, nil
}
return deployedRelease, stateUnchanged, nil
return currentRelease, stateUnchanged, nil
}

func (r *Reconciler) doInstall(actionClient helmclient.ActionInterface, u *updater.Updater, obj *unstructured.Unstructured, vals map[string]interface{}, log logr.Logger) (*release.Release, error) {
Expand Down
86 changes: 69 additions & 17 deletions pkg/reconciler/reconciler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
"github.com/go-logr/logr/testing"
. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"
"helm.sh/helm/v3/pkg/action"
"helm.sh/helm/v3/pkg/chart"
"helm.sh/helm/v3/pkg/chartutil"
"helm.sh/helm/v3/pkg/release"
Expand Down Expand Up @@ -450,7 +451,7 @@ var _ = Describe("Reconciler", func() {
Expect(mgr.GetClient().Create(ctx, obj)).To(Succeed())
})

When("requested CR release is not installed", func() {
When("requested CR release is not present", func() {
When("action client getter is not working", func() {
It("returns an error getting the action client", func() {
acgErr := errors.New("broken action client getter: error getting action client")
Expand Down Expand Up @@ -682,9 +683,9 @@ var _ = Describe("Reconciler", func() {
})
})
})
When("requested CR release is installed", func() {
When("requested CR release is present", func() {
var (
installedRelease *release.Release
currentRelease *release.Release
)
BeforeEach(func() {
// Reconcile once to get the release installed and finalizers added
Expand All @@ -693,7 +694,7 @@ var _ = Describe("Reconciler", func() {
Expect(res).To(Equal(reconcile.Result{}))
Expect(err).To(BeNil())

installedRelease, err = ac.Get(obj.GetName())
currentRelease, err = ac.Get(obj.GetName())
Expect(err).To(BeNil())
})
When("action client getter is not working", func() {
Expand Down Expand Up @@ -801,27 +802,78 @@ var _ = Describe("Reconciler", func() {
Expect(c.Reason).To(Equal(conditions.ReasonErrorGettingValues))
Expect(c.Message).To(ContainSubstring("error parsing index"))

Expect(objStat.Status.DeployedRelease.Name).To(Equal(installedRelease.Name))
Expect(objStat.Status.DeployedRelease.Manifest).To(Equal(installedRelease.Manifest))
Expect(objStat.Status.DeployedRelease.Name).To(Equal(currentRelease.Name))
Expect(objStat.Status.DeployedRelease.Manifest).To(Equal(currentRelease.Manifest))
})

By("verifying the uninstall finalizer is not present on the CR", func() {
Expect(controllerutil.ContainsFinalizer(obj, uninstallFinalizer)).To(BeTrue())
})
})
})
When("all preconditions are met", func() {
When("requested CR release is not deployed", func() {
var actionConf *action.Configuration
BeforeEach(func() {
By("getting the current release and config", func() {
var err error
acg := helmclient.NewActionConfigGetter(mgr.GetConfig(), mgr.GetRESTMapper(), nil)
actionConf, err = acg.ActionConfigFor(obj)
Expect(err).To(BeNil())
})
})
When("state is Failed", func() {
BeforeEach(func() {
currentRelease.Info.Status = release.StatusFailed
Expect(actionConf.Releases.Update(currentRelease)).To(Succeed())
})
It("upgrades the release", func() {
By("successfully reconciling a request", func() {
res, err := r.Reconcile(ctx, req)
Expect(res).To(Equal(reconcile.Result{}))
Expect(err).To(BeNil())
})
By("verifying the release", func() {
rel, err := ac.Get(obj.GetName())
Expect(err).To(BeNil())
Expect(rel).NotTo(BeNil())
Expect(rel.Version).To(Equal(2))
verifyRelease(ctx, mgr.GetAPIReader(), obj.GetNamespace(), rel)
})
})
})
When("state is Superseded", func() {
BeforeEach(func() {
currentRelease.Info.Status = release.StatusSuperseded
Expect(actionConf.Releases.Update(currentRelease)).To(Succeed())
})
It("upgrades the release", func() {
By("successfully reconciling a request", func() {
res, err := r.Reconcile(ctx, req)
Expect(res).To(Equal(reconcile.Result{}))
Expect(err).To(BeNil())
})
By("verifying the release", func() {
rel, err := ac.Get(obj.GetName())
Expect(err).To(BeNil())
Expect(rel).NotTo(BeNil())
Expect(rel.Version).To(Equal(2))
verifyRelease(ctx, mgr.GetAPIReader(), obj.GetNamespace(), rel)
})
})
})
})
When("state is Deployed", func() {
When("upgrade fails", func() {
BeforeEach(func() {
ac := helmfake.NewActionClient()
ac.HandleGet = func() (*release.Release, error) {
return &release.Release{Name: "test", Version: 1, Manifest: "version: 1"}, nil
return &release.Release{Name: "test", Version: 1, Manifest: "manifest: 1"}, nil
}
firstRun := true
ac.HandleUpgrade = func() (*release.Release, error) {
if firstRun {
firstRun = false
return &release.Release{Name: "test", Version: 1, Manifest: "version: 2"}, nil
return &release.Release{Name: "test", Version: 1, Manifest: "manifest: 2"}, nil
}
return nil, errors.New("upgrade failed: foobar")
}
Expand All @@ -846,7 +898,7 @@ var _ = Describe("Reconciler", func() {
Expect(objStat.Status.Conditions.IsTrueFor(conditions.TypeDeployed)).To(BeTrue())
Expect(objStat.Status.Conditions.IsTrueFor(conditions.TypeReleaseFailed)).To(BeTrue())
Expect(objStat.Status.DeployedRelease.Name).To(Equal("test"))
Expect(objStat.Status.DeployedRelease.Manifest).To(Equal("version: 1"))
Expect(objStat.Status.DeployedRelease.Manifest).To(Equal("manifest: 1"))

c := objStat.Status.Conditions.GetCondition(conditions.TypeReleaseFailed)
Expect(c).NotTo(BeNil())
Expand Down Expand Up @@ -921,10 +973,10 @@ var _ = Describe("Reconciler", func() {
BeforeEach(func() {
ac := helmfake.NewActionClient()
ac.HandleGet = func() (*release.Release, error) {
return &release.Release{Name: "test", Version: 1, Manifest: "version: 1"}, nil
return &release.Release{Name: "test", Version: 1, Manifest: "manifest: 1", Info: &release.Info{Status: release.StatusDeployed}}, nil
}
ac.HandleUpgrade = func() (*release.Release, error) {
return &release.Release{Name: "test", Version: 1, Manifest: "version: 1"}, nil
return &release.Release{Name: "test", Version: 2, Manifest: "manifest: 1", Info: &release.Info{Status: release.StatusDeployed}}, nil
}
ac.HandleReconcile = func() error {
return errors.New("reconciliation failed: foobar")
Expand All @@ -950,7 +1002,7 @@ var _ = Describe("Reconciler", func() {
Expect(objStat.Status.Conditions.IsTrueFor(conditions.TypeDeployed)).To(BeTrue())
Expect(objStat.Status.Conditions.IsFalseFor(conditions.TypeReleaseFailed)).To(BeTrue())
Expect(objStat.Status.DeployedRelease.Name).To(Equal("test"))
Expect(objStat.Status.DeployedRelease.Manifest).To(Equal("version: 1"))
Expect(objStat.Status.DeployedRelease.Manifest).To(Equal("manifest: 1"))

c := objStat.Status.Conditions.GetCondition(conditions.TypeIrreconcilable)
Expect(c).NotTo(BeNil())
Expand All @@ -970,7 +1022,7 @@ var _ = Describe("Reconciler", func() {
err error
)
By("changing the release resources", func() {
for _, resource := range manifestToObjects(installedRelease.Manifest) {
for _, resource := range manifestToObjects(currentRelease.Manifest) {
key := client.ObjectKeyFromObject(resource)

u := &unstructured.Unstructured{}
Expand Down Expand Up @@ -1032,7 +1084,7 @@ var _ = Describe("Reconciler", func() {
BeforeEach(func() {
ac := helmfake.NewActionClient()
ac.HandleGet = func() (*release.Release, error) {
return &release.Release{Name: "test", Version: 1, Manifest: "version: 1"}, nil
return &release.Release{Name: "test", Version: 1, Manifest: "manifest: 1"}, nil
}
ac.HandleUninstall = func() (*release.UninstallReleaseResponse, error) {
return nil, errors.New("uninstall failed: foobar")
Expand Down Expand Up @@ -1062,7 +1114,7 @@ var _ = Describe("Reconciler", func() {
Expect(objStat.Status.Conditions.IsTrueFor(conditions.TypeDeployed)).To(BeTrue())
Expect(objStat.Status.Conditions.IsTrueFor(conditions.TypeReleaseFailed)).To(BeTrue())
Expect(objStat.Status.DeployedRelease.Name).To(Equal("test"))
Expect(objStat.Status.DeployedRelease.Manifest).To(Equal("version: 1"))
Expect(objStat.Status.DeployedRelease.Manifest).To(Equal("manifest: 1"))

c := objStat.Status.Conditions.GetCondition(conditions.TypeReleaseFailed)
Expect(c).NotTo(BeNil())
Expand Down Expand Up @@ -1093,7 +1145,7 @@ var _ = Describe("Reconciler", func() {
})

By("verifying the release is uninstalled", func() {
verifyNoRelease(ctx, mgr.GetClient(), obj.GetNamespace(), obj.GetName(), installedRelease)
verifyNoRelease(ctx, mgr.GetClient(), obj.GetNamespace(), obj.GetName(), currentRelease)
})

By("ensuring the finalizer is removed and the CR is deleted", func() {
Expand Down