Skip to content

✨ Use Helm List operator to determine Deployed status #1379

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

Merged
merged 1 commit into from
Oct 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ require (
github.com/opencontainers/go-digest v1.0.0
github.com/operator-framework/api v0.27.0
github.com/operator-framework/catalogd v0.35.0
github.com/operator-framework/helm-operator-plugins v0.5.0
github.com/operator-framework/helm-operator-plugins v0.7.0
github.com/operator-framework/operator-registry v1.47.0
github.com/spf13/pflag v1.0.5
github.com/stretchr/testify v1.9.0
Expand Down
4 changes: 2 additions & 2 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -537,8 +537,8 @@ github.com/operator-framework/api v0.27.0 h1:OrVaGKZJvbZo58HTv2guz7aURkhVKYhFqZ/
github.com/operator-framework/api v0.27.0/go.mod h1:lg2Xx+S8NQWGYlEOvFwQvH46E5EK5IrAIL7HWfAhciM=
github.com/operator-framework/catalogd v0.35.0 h1:2lQPyHIzEfcciUjQ/fo4i/GE/sX2LBzd8S+nYxlvEHU=
github.com/operator-framework/catalogd v0.35.0/go.mod h1:anZurjcFMBvbkuyqlJ98v9z+yjniPKqmhlyitk9DuBQ=
github.com/operator-framework/helm-operator-plugins v0.5.0 h1:qph2OoECcI9mpuUBtOsWOMgvpx52mPTTSvzVxICsT04=
github.com/operator-framework/helm-operator-plugins v0.5.0/go.mod h1:yVncrZ/FJNqedMil+055fk6sw8aMKRrget/AqGM0ig0=
github.com/operator-framework/helm-operator-plugins v0.7.0 h1:YmtIWFc9BaNaDc5mk/dkG0P2BqPZOqpDvjWih5Fczuk=
github.com/operator-framework/helm-operator-plugins v0.7.0/go.mod h1:fUUCJR3bWtMBZ1qdDhbwjacsBHi9uT576tF4u/DwOgQ=
github.com/operator-framework/operator-lib v0.15.0 h1:0QeRM4PMtThqINpcFGCEBnIV3Z8u7/8fYLEx6mUtdcM=
github.com/operator-framework/operator-lib v0.15.0/go.mod h1:ZxLvFuQ7bRWiTNBOqodbuNvcsy/Iq0kOygdxhlbNdI0=
github.com/operator-framework/operator-registry v1.47.0 h1:Imr7X/W6FmXczwpIOXfnX8d6Snr1dzwWxkMG+lLAfhg=
Expand Down
6 changes: 6 additions & 0 deletions internal/action/helm.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,12 @@ func (a ActionClient) Get(name string, opts ...actionclient.GetOption) (*release
return resp, err
}

func (a ActionClient) History(name string, opts ...actionclient.HistoryOption) ([]*release.Release, error) {
resp, err := a.ActionInterface.History(name, opts...)
err = a.actionClientErrorTranslator(err)
return resp, err
}

func (a ActionClient) Reconcile(rel *release.Release) error {
return a.actionClientErrorTranslator(a.ActionInterface.Reconcile(rel))
}
16 changes: 16 additions & 0 deletions internal/action/helm_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,17 @@ func (m *mockActionClient) Get(name string, opts ...actionclient.GetOption) (*re
return args.Get(0).(*release.Release), args.Error(1)
}

func (m *mockActionClient) History(name string, opts ...actionclient.HistoryOption) ([]*release.Release, error) {
args := m.Called(name, opts)
if args.Get(0) == nil {
return nil, args.Error(1)
}
rel := []*release.Release{
args.Get(0).(*release.Release),
}
return rel, args.Error(1)
}

func (m *mockActionClient) Install(name, namespace string, chrt *chart.Chart, vals map[string]interface{}, opts ...actionclient.InstallOption) (*release.Release, error) {
args := m.Called(name, namespace, chrt, vals, opts)
if args.Get(0) == nil {
Expand Down Expand Up @@ -82,6 +93,7 @@ func TestActionClientErrorTranslation(t *testing.T) {

ac := new(mockActionClient)
ac.On("Get", mock.Anything, mock.Anything).Return(nil, originalError)
ac.On("History", mock.Anything, mock.Anything).Return(nil, originalError)
ac.On("Install", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, originalError)
ac.On("Uninstall", mock.Anything, mock.Anything).Return(nil, originalError)
ac.On("Upgrade", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, originalError)
Expand All @@ -93,6 +105,10 @@ func TestActionClientErrorTranslation(t *testing.T) {
_, err := wrappedAc.Get("something")
assert.Equal(t, expectedErr, err, "expected Get() to return translated error")

// History
_, err = wrappedAc.History("something")
assert.Equal(t, expectedErr, err, "expected History() to return translated error")

// Install
_, err = wrappedAc.Install("something", "somethingelse", nil, nil)
assert.Equal(t, expectedErr, err, "expected Install() to return translated error")
Expand Down
83 changes: 50 additions & 33 deletions internal/controllers/clusterextension_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ type Applier interface {
}

type InstalledBundleGetter interface {
GetInstalledBundle(ctx context.Context, ext *ocv1alpha1.ClusterExtension) (*ocv1alpha1.BundleMetadata, error)
GetInstalledBundle(ctx context.Context, ext *ocv1alpha1.ClusterExtension) (*InstalledBundle, error)
}

//+kubebuilder:rbac:groups=olm.operatorframework.io,resources=clusterextensions,verbs=get;list;watch;update;patch
Expand Down Expand Up @@ -206,19 +206,22 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp
installedBundle, err := r.InstalledBundleGetter.GetInstalledBundle(ctx, ext)
if err != nil {
setInstallStatus(ext, nil)
// TODO: use Installed=Unknown
setInstalledStatusConditionFailed(ext, err.Error())
setStatusProgressing(ext, err)
setInstalledStatusConditionUnknown(ext, err.Error())
setStatusProgressing(ext, errors.New("retrying to get installed bundle"))
return ctrl.Result{}, err
}

// run resolution
l.Info("resolving bundle")
resolvedBundle, resolvedBundleVersion, resolvedDeprecation, err := r.Resolver.Resolve(ctx, ext, installedBundle)
var bm *ocv1alpha1.BundleMetadata
if installedBundle != nil {
bm = &installedBundle.BundleMetadata
}
resolvedBundle, resolvedBundleVersion, resolvedDeprecation, err := r.Resolver.Resolve(ctx, ext, bm)
if err != nil {
// Note: We don't distinguish between resolution-specific errors and generic errors
setInstallStatus(ext, nil)
setStatusProgressing(ext, err)
setInstalledStatusFromBundle(ext, installedBundle)
ensureAllConditionsWithReason(ext, ocv1alpha1.ReasonFailed, err.Error())
return ctrl.Result{}, err
}
Expand Down Expand Up @@ -255,6 +258,7 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp
// installed since we intend for the progressing condition to replace the resolved condition
// and will be removing the .status.resolution field from the ClusterExtension status API
setStatusProgressing(ext, wrapErrorWithResolutionInfo(resolvedBundleMetadata, err))
setInstalledStatusFromBundle(ext, installedBundle)
return ctrl.Result{}, err
}

Expand All @@ -268,9 +272,10 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp
}

storeLbls := map[string]string{
labels.BundleNameKey: resolvedBundle.Name,
labels.PackageNameKey: resolvedBundle.Package,
labels.BundleVersionKey: resolvedBundleVersion.String(),
labels.BundleNameKey: resolvedBundle.Name,
labels.PackageNameKey: resolvedBundle.Package,
labels.BundleVersionKey: resolvedBundleVersion.String(),
labels.BundleReferenceKey: resolvedBundle.Image,
}

l.Info("applying bundle contents")
Expand All @@ -286,18 +291,17 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp
managedObjs, _, err := r.Applier.Apply(ctx, unpackResult.Bundle, ext, objLbls, storeLbls)
if err != nil {
setStatusProgressing(ext, wrapErrorWithResolutionInfo(resolvedBundleMetadata, err))
// If bundle is not already installed, set Installed status condition to False
if installedBundle == nil {
setInstalledStatusConditionFailed(ext, err.Error())
}
// Now that we're actually trying to install, use the error
setInstalledStatusFromBundle(ext, installedBundle)
return ctrl.Result{}, err
}

installStatus := &ocv1alpha1.ClusterExtensionInstallStatus{
Bundle: resolvedBundleMetadata,
newInstalledBundle := &InstalledBundle{
BundleMetadata: resolvedBundleMetadata,
Image: resolvedBundle.Image,
}
setInstallStatus(ext, installStatus)
setInstalledStatusConditionSuccess(ext, fmt.Sprintf("Installed bundle %s successfully", resolvedBundle.Image))
// Successful install
setInstalledStatusFromBundle(ext, newInstalledBundle)

l.Info("watching managed objects")
cache, err := r.Manager.Get(ctx, ext)
Expand Down Expand Up @@ -466,32 +470,45 @@ type DefaultInstalledBundleGetter struct {
helmclient.ActionClientGetter
}

func (d *DefaultInstalledBundleGetter) GetInstalledBundle(ctx context.Context, ext *ocv1alpha1.ClusterExtension) (*ocv1alpha1.BundleMetadata, error) {
type InstalledBundle struct {
ocv1alpha1.BundleMetadata
Image string
}

func (d *DefaultInstalledBundleGetter) GetInstalledBundle(ctx context.Context, ext *ocv1alpha1.ClusterExtension) (*InstalledBundle, error) {
cl, err := d.ActionClientFor(ctx, ext)
if err != nil {
return nil, err
}

rel, err := cl.Get(ext.GetName())
relhis, err := cl.History(ext.GetName())
if err != nil && !errors.Is(err, driver.ErrReleaseNotFound) {
return nil, err
}
if rel == nil {
if len(relhis) == 0 {
return nil, nil
}

switch rel.Info.Status {
case release.StatusUnknown:
return nil, fmt.Errorf("installation status is unknown")
case release.StatusDeployed, release.StatusUninstalled, release.StatusSuperseded, release.StatusFailed:
case release.StatusUninstalling, release.StatusPendingInstall, release.StatusPendingRollback, release.StatusPendingUpgrade:
return nil, fmt.Errorf("installation is still pending: %s", rel.Info.Status)
default:
return nil, fmt.Errorf("unknown installation status: %s", rel.Info.Status)
// relhis[0].Info.Status is the status of the most recent install attempt.
// But we need to look for the most-recent _Deployed_ release
for _, rel := range relhis {
if rel.Info != nil && rel.Info.Status == release.StatusDeployed {
// 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
}
Comment on lines +496 to +502
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. 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.
  2. Something deleted our labels.
  3. We introduced a bug somehow where we didn't set the labels.

Comment on lines +496 to +502
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@tmshort tmshort Oct 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// Not checking BundleReferenceKey, as it's new; upgrade test would fail
return &InstalledBundle{
BundleMetadata: ocv1alpha1.BundleMetadata{
Name: rel.Labels[labels.BundleNameKey],
Version: rel.Labels[labels.BundleVersionKey],
},
Image: rel.Labels[labels.BundleReferenceKey],
}, nil
}
}

return &ocv1alpha1.BundleMetadata{
Name: rel.Labels[labels.BundleNameKey],
Version: rel.Labels[labels.BundleVersionKey],
}, nil
return nil, nil
}
145 changes: 143 additions & 2 deletions internal/controllers/clusterextension_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ import (
"github.com/google/go-cmp/cmp/cmpopts"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"helm.sh/helm/v3/pkg/chart"
"helm.sh/helm/v3/pkg/release"
"helm.sh/helm/v3/pkg/storage/driver"
apimeta "k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
Expand All @@ -21,12 +24,14 @@ import (
crfinalizer "sigs.k8s.io/controller-runtime/pkg/finalizer"
"sigs.k8s.io/controller-runtime/pkg/reconcile"

helmclient "github.com/operator-framework/helm-operator-plugins/pkg/client"
"github.com/operator-framework/operator-registry/alpha/declcfg"

ocv1alpha1 "github.com/operator-framework/operator-controller/api/v1alpha1"
"github.com/operator-framework/operator-controller/internal/conditionsets"
"github.com/operator-framework/operator-controller/internal/controllers"
"github.com/operator-framework/operator-controller/internal/finalizers"
"github.com/operator-framework/operator-controller/internal/labels"
"github.com/operator-framework/operator-controller/internal/resolve"
"github.com/operator-framework/operator-controller/internal/rukpak/source"
)
Expand Down Expand Up @@ -392,7 +397,10 @@ func TestClusterExtensionApplierFailsWithBundleInstalled(t *testing.T) {
cache: &MockManagedContentCache{},
}
reconciler.InstalledBundleGetter = &MockInstalledBundleGetter{
bundle: &ocv1alpha1.BundleMetadata{Name: "prometheus.v1.0.0", Version: "1.0.0"},
bundle: &controllers.InstalledBundle{
BundleMetadata: ocv1alpha1.BundleMetadata{Name: "prometheus.v1.0.0", Version: "1.0.0"},
Image: "quay.io/operatorhubio/[email protected]",
},
}
reconciler.Applier = &MockApplier{
objs: []client.Object{},
Expand Down Expand Up @@ -745,7 +753,10 @@ func TestClusterExtensionDeleteFinalizerFails(t *testing.T) {
cache: &MockManagedContentCache{},
}
reconciler.InstalledBundleGetter = &MockInstalledBundleGetter{
bundle: &ocv1alpha1.BundleMetadata{Name: "prometheus.v1.0.0", Version: "1.0.0"},
bundle: &controllers.InstalledBundle{
BundleMetadata: ocv1alpha1.BundleMetadata{Name: "prometheus.v1.0.0", Version: "1.0.0"},
Image: "quay.io/operatorhubio/[email protected]",
},
}
err = reconciler.Finalizers.Register(fakeFinalizer, finalizers.FinalizerFunc(func(ctx context.Context, obj client.Object) (crfinalizer.Result, error) {
return crfinalizer.Result{}, errors.New(finalizersMessage)
Expand Down Expand Up @@ -1400,3 +1411,133 @@ func TestSetDeprecationStatus(t *testing.T) {
})
}
}

type MockActionGetter struct {
description string
rels []*release.Release
err error
expectedBundle *controllers.InstalledBundle
expectedError error
}

func (mag *MockActionGetter) ActionClientFor(ctx context.Context, obj client.Object) (helmclient.ActionInterface, error) {
return mag, nil
}

func (mag *MockActionGetter) Get(name string, opts ...helmclient.GetOption) (*release.Release, error) {
return nil, nil
}

// This is the function we are really testing
func (mag *MockActionGetter) History(name string, opts ...helmclient.HistoryOption) ([]*release.Release, error) {
return mag.rels, mag.err
}

func (mag *MockActionGetter) Install(name, namespace string, chrt *chart.Chart, vals map[string]interface{}, opts ...helmclient.InstallOption) (*release.Release, error) {
return nil, nil
}

func (mag *MockActionGetter) Upgrade(name, namespace string, chrt *chart.Chart, vals map[string]interface{}, opts ...helmclient.UpgradeOption) (*release.Release, error) {
return nil, nil
}

func (mag *MockActionGetter) Uninstall(name string, opts ...helmclient.UninstallOption) (*release.UninstallReleaseResponse, error) {
return nil, nil
}

func (mag *MockActionGetter) Reconcile(rel *release.Release) error {
return nil
}

func TestGetInstalledBundleHistory(t *testing.T) {
getter := controllers.DefaultInstalledBundleGetter{}

ext := ocv1alpha1.ClusterExtension{
ObjectMeta: metav1.ObjectMeta{
Name: "test-ext",
},
}

mag := []MockActionGetter{
{
"No return",
nil, nil,
nil, nil,
},
{
"ErrReleaseNotFound (special case)",
nil, driver.ErrReleaseNotFound,
nil, nil,
},
{
"Error from History",
nil, fmt.Errorf("generic error"),
nil, fmt.Errorf("generic error"),
},
{
"One item in history",
[]*release.Release{
{
Name: "test-ext",
Info: &release.Info{
Status: release.StatusDeployed,
},
Labels: map[string]string{
labels.BundleNameKey: "test-ext",
labels.BundleVersionKey: "1.0",
labels.BundleReferenceKey: "bundle-ref",
},
},
}, nil,
&controllers.InstalledBundle{
BundleMetadata: ocv1alpha1.BundleMetadata{
Name: "test-ext",
Version: "1.0",
},
Image: "bundle-ref",
}, nil,
},
{
"Two items in history",
[]*release.Release{
{
Name: "test-ext",
Info: &release.Info{
Status: release.StatusFailed,
},
Labels: map[string]string{
labels.BundleNameKey: "test-ext",
labels.BundleVersionKey: "2.0",
labels.BundleReferenceKey: "bundle-ref-2",
},
},
{
Name: "test-ext",
Info: &release.Info{
Status: release.StatusDeployed,
},
Labels: map[string]string{
labels.BundleNameKey: "test-ext",
labels.BundleVersionKey: "1.0",
labels.BundleReferenceKey: "bundle-ref-1",
},
},
}, nil,
&controllers.InstalledBundle{
BundleMetadata: ocv1alpha1.BundleMetadata{
Name: "test-ext",
Version: "1.0",
},
Image: "bundle-ref-1",
}, nil,
},
}

for _, tst := range mag {
t.Log(tst.description)
getter.ActionClientGetter = &tst
md, err := getter.GetInstalledBundle(context.Background(), &ext)
require.Equal(t, tst.expectedError, err)
require.Equal(t, tst.expectedBundle, md)
}
}
Loading
Loading