From 87139570e9da95d2b285f0405d040f8436a062b5 Mon Sep 17 00:00:00 2001 From: Todd Short Date: Tue, 15 Oct 2024 16:34:46 -0400 Subject: [PATCH] Use Helm List operator to determine Deployed status Signed-off-by: Todd Short --- go.mod | 4 +- go.sum | 8 +- internal/action/helm.go | 6 + internal/action/helm_test.go | 16 +++ .../clusterextension_controller.go | 39 +++---- .../clusterextension_controller_test.go | 108 ++++++++++++++++++ internal/controllers/common_controller.go | 29 +++++ 7 files changed, 182 insertions(+), 28 deletions(-) diff --git a/go.mod b/go.mod index f43a9e2a3..46cf85745 100644 --- a/go.mod +++ b/go.mod @@ -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.32.0 - github.com/operator-framework/helm-operator-plugins v0.5.0 + github.com/operator-framework/helm-operator-plugins v0.6.0 github.com/operator-framework/operator-registry v1.47.0 github.com/spf13/pflag v1.0.5 github.com/stretchr/testify v1.9.0 @@ -179,7 +179,7 @@ require ( github.com/pkg/errors v0.9.1 // indirect github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 // indirect github.com/proglottis/gpgme v0.1.3 // indirect - github.com/prometheus/client_golang v1.20.4 // indirect + github.com/prometheus/client_golang v1.20.5 // indirect github.com/prometheus/client_model v0.6.1 // indirect github.com/prometheus/common v0.55.0 // indirect github.com/prometheus/procfs v0.15.1 // indirect diff --git a/go.sum b/go.sum index bbc6afea6..55b84079d 100644 --- a/go.sum +++ b/go.sum @@ -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.32.0 h1:VKD+7wfEF6CnJgR4aUYyT85KP2Te7zjhaPvgvWy25Uw= github.com/operator-framework/catalogd v0.32.0/go.mod h1:FrFSCwRXr4aPslcXIv48dan5AdM37k/B9tK/RpdvZCU= -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.6.0 h1:MAb1oKsqlsb2gVTFJkytIySK+mzT5sdICLM9HfYF49A= +github.com/operator-framework/helm-operator-plugins v0.6.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= @@ -569,8 +569,8 @@ github.com/prometheus/client_golang v0.9.1/go.mod h1:7SWBe2y4D6OKWSNQJUaRYU/AaXP github.com/prometheus/client_golang v0.9.3/go.mod h1:/TN21ttK/J9q6uSwhBd54HahCDft0ttaMvbicHlPoso= github.com/prometheus/client_golang v1.0.0/go.mod h1:db9x61etRT2tGnBNRi70OPL5FsnadC4Ky3P0J6CfImo= github.com/prometheus/client_golang v1.1.0/go.mod h1:I1FGZT9+L76gKKOs5djB6ezCbFQP1xR9D75/vuwEF3g= -github.com/prometheus/client_golang v1.20.4 h1:Tgh3Yr67PaOv/uTqloMsCEdeuFTatm5zIq5+qNN23vI= -github.com/prometheus/client_golang v1.20.4/go.mod h1:PIEt8X02hGcP8JWbeHyeZ53Y/jReSnHgO035n//V5WE= +github.com/prometheus/client_golang v1.20.5 h1:cxppBPuYhUnsO6yo/aoRol4L7q7UFfdm+bR9r+8l63Y= +github.com/prometheus/client_golang v1.20.5/go.mod h1:PIEt8X02hGcP8JWbeHyeZ53Y/jReSnHgO035n//V5WE= github.com/prometheus/client_model v0.0.0-20180712105110-5c3871d89910/go.mod h1:MbSGuTsp3dbXC40dX6PRTWyKYBIrTGTE9sqQNg2J8bo= github.com/prometheus/client_model v0.0.0-20190129233127-fd36f4220a90/go.mod h1:xMI15A0UPsDsEKsMN9yxemIoYk6Tm2C1GtYGdfGttqA= github.com/prometheus/client_model v0.0.0-20190812154241-14fe0d1b01d4/go.mod h1:xMI15A0UPsDsEKsMN9yxemIoYk6Tm2C1GtYGdfGttqA= diff --git a/internal/action/helm.go b/internal/action/helm.go index 0ac18cf79..d54c469da 100644 --- a/internal/action/helm.go +++ b/internal/action/helm.go @@ -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)) } diff --git a/internal/action/helm_test.go b/internal/action/helm_test.go index d07dc53bc..3f1f02f9d 100644 --- a/internal/action/helm_test.go +++ b/internal/action/helm_test.go @@ -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 { @@ -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) @@ -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") diff --git a/internal/controllers/clusterextension_controller.go b/internal/controllers/clusterextension_controller.go index bb3a1494b..a4e84c145 100644 --- a/internal/controllers/clusterextension_controller.go +++ b/internal/controllers/clusterextension_controller.go @@ -206,8 +206,7 @@ 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()) + setInstalledStatusConditionUnknown(ext, "retrying get installed bundle") setStatusProgressing(ext, err) return ctrl.Result{}, err } @@ -217,7 +216,7 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp resolvedBundle, resolvedBundleVersion, resolvedDeprecation, err := r.Resolver.Resolve(ctx, ext, installedBundle) if err != nil { // Note: We don't distinguish between resolution-specific errors and generic errors - setInstallStatus(ext, nil) + setInstalledStatusFromBundle(ext, installedBundle) setStatusProgressing(ext, err) ensureAllConditionsWithReason(ext, ocv1alpha1.ReasonFailed, err.Error()) return ctrl.Result{}, err @@ -254,6 +253,7 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp // Wrap the error passed to this with the resolution information until we have successfully // 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 + setInstalledStatusFromBundle(ext, installedBundle) setStatusProgressing(ext, wrapErrorWithResolutionInfo(resolvedBundleMetadata, err)) return ctrl.Result{}, err } @@ -286,10 +286,7 @@ 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()) - } + setInstalledStatusFromBundle(ext, installedBundle) return ctrl.Result{}, err } @@ -472,26 +469,24 @@ func (d *DefaultInstalledBundleGetter) GetInstalledBundle(ctx context.Context, e 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) + // TODO: relhis[0].Info.Status is the status of the highest semver install attempt. + // This might be useful informaton if it's not release.StatusDeployed, in telling us + // the status of an upgrade attempt. + for _, rel := range relhis { + if rel.Info != nil && rel.Info.Status == release.StatusDeployed { + return &ocv1alpha1.BundleMetadata{ + Name: rel.Labels[labels.BundleNameKey], + Version: rel.Labels[labels.BundleVersionKey], + }, nil + } } - - return &ocv1alpha1.BundleMetadata{ - Name: rel.Labels[labels.BundleNameKey], - Version: rel.Labels[labels.BundleVersionKey], - }, nil + return nil, nil } diff --git a/internal/controllers/clusterextension_controller_test.go b/internal/controllers/clusterextension_controller_test.go index 8f7331384..c4c32a7be 100644 --- a/internal/controllers/clusterextension_controller_test.go +++ b/internal/controllers/clusterextension_controller_test.go @@ -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" @@ -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" ) @@ -1400,3 +1405,106 @@ func TestSetDeprecationStatus(t *testing.T) { }) } } + +type MockActionGetter struct { + rels []*release.Release + err 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) { + mag := MockActionGetter{} + + getter := controllers.DefaultInstalledBundleGetter{ + ActionClientGetter: &mag, + } + + ext := ocv1alpha1.ClusterExtension{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-ext", + }, + } + + md, err := getter.GetInstalledBundle(context.Background(), &ext) + require.NoError(t, err) + require.Nil(t, md) + + mag.err = driver.ErrReleaseNotFound + md, err = getter.GetInstalledBundle(context.Background(), &ext) + require.NoError(t, err) + require.Nil(t, md) + + mag.err = nil + mag.rels = []*release.Release{ + { + Name: "test-ext", + Info: &release.Info{ + Status: release.StatusDeployed, + }, + Labels: map[string]string{ + labels.BundleNameKey: "text-ext", + labels.BundleVersionKey: "1.0", + }, + }, + } + + md, err = getter.GetInstalledBundle(context.Background(), &ext) + require.NoError(t, err) + require.NotNil(t, md) + require.Equal(t, &ocv1alpha1.BundleMetadata{Name: "text-ext", Version: "1.0"}, md) + + mag.rels = []*release.Release{ + { + Name: "test-ext", + Info: &release.Info{ + Status: release.StatusFailed, + }, + Labels: map[string]string{ + labels.BundleNameKey: "text-ext", + labels.BundleVersionKey: "2.0", + }, + }, + { + Name: "test-ext", + Info: &release.Info{ + Status: release.StatusDeployed, + }, + Labels: map[string]string{ + labels.BundleNameKey: "text-ext", + labels.BundleVersionKey: "1.0", + }, + }, + } + md, err = getter.GetInstalledBundle(context.Background(), &ext) + require.NoError(t, err) + require.NotNil(t, md) + require.Equal(t, &ocv1alpha1.BundleMetadata{Name: "text-ext", Version: "1.0"}, md) +} diff --git a/internal/controllers/common_controller.go b/internal/controllers/common_controller.go index d04cd0a01..9d99acacd 100644 --- a/internal/controllers/common_controller.go +++ b/internal/controllers/common_controller.go @@ -26,6 +26,24 @@ import ( ocv1alpha1 "github.com/operator-framework/operator-controller/api/v1alpha1" ) +// setInstalledStatusFromBundle sets the installed status based on the given installedBundle. +func setInstalledStatusFromBundle(ext *ocv1alpha1.ClusterExtension, installedBundle *ocv1alpha1.BundleMetadata) { + if installedBundle != nil { + installStatus := &ocv1alpha1.ClusterExtensionInstallStatus{ + Bundle: *installedBundle, + } + setInstallStatus(ext, installStatus) + // We don't want to change the Transition time if we don't have to + if !apimeta.IsStatusConditionTrue(ext.Status.Conditions, ocv1alpha1.TypeInstalled) { + setInstalledStatusConditionSuccess(ext, "Installed extension successfully") + } + return + } + // If bundle is not already installed, set Installed status condition to False + setInstallStatus(ext, nil) + setInstalledStatusConditionFailed(ext, "no installed bundle found") +} + // setInstalledStatusConditionSuccess sets the installed status condition to success. func setInstalledStatusConditionSuccess(ext *ocv1alpha1.ClusterExtension, message string) { apimeta.SetStatusCondition(&ext.Status.Conditions, metav1.Condition{ @@ -48,6 +66,17 @@ func setInstalledStatusConditionFailed(ext *ocv1alpha1.ClusterExtension, message }) } +// setInstalledStatusConditionUnknown sets the installed status condition to unknown. +func setInstalledStatusConditionUnknown(ext *ocv1alpha1.ClusterExtension, message string) { + apimeta.SetStatusCondition(&ext.Status.Conditions, metav1.Condition{ + Type: ocv1alpha1.TypeInstalled, + Status: metav1.ConditionUnknown, + Reason: ocv1alpha1.ReasonFailed, + Message: message, + ObservedGeneration: ext.GetGeneration(), + }) +} + func setInstallStatus(ext *ocv1alpha1.ClusterExtension, installStatus *ocv1alpha1.ClusterExtensionInstallStatus) { ext.Status.Install = installStatus }