Skip to content

Commit

Permalink
Update Installed status condition handling
Browse files Browse the repository at this point in the history 
Signed-off-by: Tayler Geiger <tayler@redhat.com>
  • Loading branch information
@trgeiger
trgeiger committed Sep 30, 2024
1 parent f169414 commit 7500b15
Show file tree
Hide file tree
Showing 4 changed files with 221 additions and 16 deletions.
11 changes: 3 additions & 8 deletions cmd/manager/main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ import (
"github.com/operator-framework/operator-controller/internal/contentmanager"
"github.com/operator-framework/operator-controller/internal/controllers"
"github.com/operator-framework/operator-controller/internal/features"
"github.com/operator-framework/operator-controller/internal/finalizers"
"github.com/operator-framework/operator-controller/internal/httputil"
"github.com/operator-framework/operator-controller/internal/labels"
"github.com/operator-framework/operator-controller/internal/resolve"
Expand Down Expand Up @@ -207,7 +208,7 @@ func main() {
}

clusterExtensionFinalizers := crfinalizer.NewFinalizers()
if err := clusterExtensionFinalizers.Register(controllers.ClusterExtensionCleanupUnpackCacheFinalizer, finalizerFunc(func(ctx context.Context, obj client.Object) (crfinalizer.Result, error) {
if err := clusterExtensionFinalizers.Register(controllers.ClusterExtensionCleanupUnpackCacheFinalizer, finalizers.FinalizerFunc(func(ctx context.Context, obj client.Object) (crfinalizer.Result, error) {
return crfinalizer.Result{}, unpacker.Cleanup(ctx, &source.BundleSource{Name: obj.GetName()})
})); err != nil {
setupLog.Error(err, "unable to register finalizer", "finalizerKey", controllers.ClusterExtensionCleanupUnpackCacheFinalizer)
Expand Down Expand Up @@ -258,7 +259,7 @@ func main() {
}

cm := contentmanager.NewManager(clientRestConfigMapper, mgr.GetConfig(), mgr.GetRESTMapper())
err = clusterExtensionFinalizers.Register(controllers.ClusterExtensionCleanupContentManagerCacheFinalizer, finalizerFunc(func(ctx context.Context, obj client.Object) (crfinalizer.Result, error) {
err = clusterExtensionFinalizers.Register(controllers.ClusterExtensionCleanupContentManagerCacheFinalizer, finalizers.FinalizerFunc(func(ctx context.Context, obj client.Object) (crfinalizer.Result, error) {
ext := obj.(*ocv1alpha1.ClusterExtension)
err := cm.Delete(ext)
return crfinalizer.Result{}, err
Expand Down Expand Up @@ -308,12 +309,6 @@ func main() {
}
}

type finalizerFunc func(ctx context.Context, obj client.Object) (crfinalizer.Result, error)

func (f finalizerFunc) Finalize(ctx context.Context, obj client.Object) (crfinalizer.Result, error) {
return f(ctx, obj)
}

func authFilePathIfPresent(logger logr.Logger) string {
_, err := os.Stat(authFilePath)
if os.IsNotExist(err) {
Expand Down
20 changes: 12 additions & 8 deletions internal/controllers/clusterextension_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -192,15 +192,16 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp
l.Info("handling finalizers")
finalizeResult, err := r.Finalizers.Finalize(ctx, ext)
if err != nil {
// TODO: For now, this error handling follows the pattern of other error handling.
// Namely: zero just about everything out, throw our hands up, and return an error.
// This is not ideal, and we should consider a more nuanced approach that resolves
// as much status as possible before returning, or at least keeps previous state if
// it is properly labeled with its observed generation.
setInstallStatus(ext, nil)
setResolutionStatus(ext, nil)
setStatusProgressing(ext, err)
ensureAllConditionsWithReason(ext, ocv1alpha1.ReasonFailed, err.Error())

// Here we check if the bundle is already installed.
// We only set Installed status to False if the bundle is actually no longer on cluster.
installedBundle, _ := r.InstalledBundleGetter.GetInstalledBundle(ctx, ext)
if installedBundle == nil {
setInstallStatus(ext, nil)
setInstalledStatusConditionFailed(ext, err.Error())
}
return ctrl.Result{}, err
}
if finalizeResult.Updated || finalizeResult.StatusUpdated {
Expand Down Expand Up @@ -297,8 +298,11 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp
// The only way to eventually recover from permission errors is to keep retrying).
managedObjs, _, err := r.Applier.Apply(ctx, unpackResult.Bundle, ext, objLbls, storeLbls)
if err != nil {
setInstalledStatusConditionFailed(ext, err.Error())
setStatusProgressing(ext, wrapErrorWithResolutionInfo(resolvedBundleMetadata, err))
// If bundle is not already installed, set Installed status condition to False
if installedBundle == nil {
setInstalledStatusConditionFailed(ext, err.Error())
}
return ctrl.Result{}, err
}

Expand Down
192 changes: 192 additions & 0 deletions internal/controllers/clusterextension_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,14 @@ import (
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/reconcile"

crfinalizer "sigs.k8s.io/controller-runtime/pkg/finalizer"

"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/resolve"
"github.com/operator-framework/operator-controller/internal/rukpak/source"
)
Expand Down Expand Up @@ -336,6 +339,105 @@ func TestClusterExtensionResolutionAndUnpackSuccessfulApplierFails(t *testing.T)
require.NoError(t, cl.DeleteAllOf(ctx, &ocv1alpha1.ClusterExtension{}))
}

func TestClusterExtensionApplierFailsWithBundleInstalled(t *testing.T) {
cl, reconciler := newClientAndReconciler(t)
reconciler.Unpacker = &MockUnpacker{
result: &source.Result{
State: source.StateUnpacked,
Bundle: fstest.MapFS{},
},
}

ctx := context.Background()
extKey := types.NamespacedName{Name: fmt.Sprintf("cluster-extension-test-%s", rand.String(8))}

t.Log("When the cluster extension specifies a channel with version that exist")
t.Log("By initializing cluster state")
pkgName := "prometheus"
pkgVer := "1.0.0"
pkgChan := "beta"
namespace := fmt.Sprintf("test-ns-%s", rand.String(8))
serviceAccount := fmt.Sprintf("test-sa-%s", rand.String(8))

clusterExtension := &ocv1alpha1.ClusterExtension{
ObjectMeta: metav1.ObjectMeta{Name: extKey.Name},
Spec: ocv1alpha1.ClusterExtensionSpec{
Source: ocv1alpha1.SourceConfig{
SourceType: "Catalog",
Catalog: &ocv1alpha1.CatalogSource{
PackageName: pkgName,
Version: pkgVer,
Channels: []string{pkgChan},
},
},
Install: ocv1alpha1.ClusterExtensionInstallConfig{
Namespace: namespace,
ServiceAccount: ocv1alpha1.ServiceAccountReference{
Name: serviceAccount,
},
},
},
}
err := cl.Create(ctx, clusterExtension)
require.NoError(t, err)

t.Log("It sets resolution success status")
t.Log("By running reconcile")
reconciler.Resolver = resolve.Func(func(_ context.Context, _ *ocv1alpha1.ClusterExtension, _ *ocv1alpha1.BundleMetadata) (*declcfg.Bundle, *bsemver.Version, *declcfg.Deprecation, error) {
v := bsemver.MustParse("1.0.0")
return &declcfg.Bundle{
Name: "prometheus.v1.0.0",
Package: "prometheus",
Image: "quay.io/operatorhubio/prometheus@fake1.0.0",
}, &v, nil, nil
})

reconciler.Manager = &MockManagedContentCacheManager{
cache: &MockManagedContentCache{},
}
reconciler.InstalledBundleGetter = &MockInstalledBundleGetter{
bundle: &ocv1alpha1.BundleMetadata{Name: "prometheus.v1.0.0", Version: "1.0.0"},
}
reconciler.Applier = &MockApplier{
objs: []client.Object{},
}

res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: extKey})
require.Equal(t, ctrl.Result{}, res)
require.NoError(t, err)

reconciler.Applier = &MockApplier{
err: errors.New("apply failure"),
}

res, err = reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: extKey})
require.Equal(t, ctrl.Result{}, res)
require.Error(t, err)

t.Log("By fetching updated cluster extension after reconcile")
require.NoError(t, cl.Get(ctx, extKey, clusterExtension))

t.Log("By checking the status fields")
expectedBundleMetadata := ocv1alpha1.BundleMetadata{Name: "prometheus.v1.0.0", Version: "1.0.0"}
require.Equal(t, expectedBundleMetadata, clusterExtension.Status.Resolution.Bundle)
require.Equal(t, expectedBundleMetadata, clusterExtension.Status.Install.Bundle)

t.Log("By checking the expected installed conditions")
installedCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeInstalled)
require.NotNil(t, installedCond)
require.Equal(t, metav1.ConditionTrue, installedCond.Status)
require.Equal(t, ocv1alpha1.ReasonSuccess, installedCond.Reason)

t.Log("By checking the expected progressing conditions")
progressingCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeProgressing)
require.NotNil(t, progressingCond)
require.Equal(t, metav1.ConditionTrue, progressingCond.Status)
require.Equal(t, ocv1alpha1.ReasonRetrying, progressingCond.Reason)
require.Contains(t, progressingCond.Message, fmt.Sprintf("for resolved bundle %q with version %q", expectedBundleMetadata.Name, expectedBundleMetadata.Version))

require.NoError(t, cl.DeleteAllOf(ctx, &ocv1alpha1.ClusterExtension{}))
}

func TestClusterExtensionManagerFailed(t *testing.T) {
cl, reconciler := newClientAndReconciler(t)
reconciler.Unpacker = &MockUnpacker{
Expand Down Expand Up @@ -591,6 +693,96 @@ func TestClusterExtensionInstallationSucceeds(t *testing.T) {
require.NoError(t, cl.DeleteAllOf(ctx, &ocv1alpha1.ClusterExtension{}))
}

func TestClusterExtensionDeleteFinalizerFails(t *testing.T) {
cl, reconciler := newClientAndReconciler(t)
reconciler.Unpacker = &MockUnpacker{
result: &source.Result{
State: source.StateUnpacked,
Bundle: fstest.MapFS{},
},
}

ctx := context.Background()
extKey := types.NamespacedName{Name: fmt.Sprintf("cluster-extension-test-%s", rand.String(8))}

t.Log("When the cluster extension specifies a channel with version that exist")
t.Log("By initializing cluster state")
pkgName := "prometheus"
pkgVer := "1.0.0"
pkgChan := "beta"
namespace := fmt.Sprintf("test-ns-%s", rand.String(8))
serviceAccount := fmt.Sprintf("test-sa-%s", rand.String(8))

clusterExtension := &ocv1alpha1.ClusterExtension{
ObjectMeta: metav1.ObjectMeta{Name: extKey.Name},
Spec: ocv1alpha1.ClusterExtensionSpec{
Source: ocv1alpha1.SourceConfig{
SourceType: "Catalog",
Catalog: &ocv1alpha1.CatalogSource{
PackageName: pkgName,
Version: pkgVer,
Channels: []string{pkgChan},
},
},
Install: ocv1alpha1.ClusterExtensionInstallConfig{
Namespace: namespace,
ServiceAccount: ocv1alpha1.ServiceAccountReference{
Name: serviceAccount,
},
},
},
}
err := cl.Create(ctx, clusterExtension)
require.NoError(t, err)
t.Log("It sets resolution success status")
t.Log("By running reconcile")
reconciler.Resolver = resolve.Func(func(_ context.Context, _ *ocv1alpha1.ClusterExtension, _ *ocv1alpha1.BundleMetadata) (*declcfg.Bundle, *bsemver.Version, *declcfg.Deprecation, error) {
v := bsemver.MustParse("1.0.0")
return &declcfg.Bundle{
Name: "prometheus.v1.0.0",
Package: "prometheus",
Image: "quay.io/operatorhubio/prometheus@fake1.0.0",
}, &v, nil, nil
})
reconciler.Applier = &MockApplier{
objs: []client.Object{},
}
reconciler.Manager = &MockManagedContentCacheManager{
cache: &MockManagedContentCache{},
}
reconciler.InstalledBundleGetter = &MockInstalledBundleGetter{
bundle: &ocv1alpha1.BundleMetadata{Name: "prometheus.v1.0.0", Version: "1.0.0"},
}
err = reconciler.Finalizers.Register("fake.testfinalizer.io", finalizers.FinalizerFunc(func(ctx context.Context, obj client.Object) (crfinalizer.Result, error) {
return crfinalizer.Result{}, errors.New("still have finalizers")
}))
require.NoError(t, err)

// Reconcile twice to simulate installing the ClusterExtension and loading in the finalizers
res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: extKey})
require.Equal(t, ctrl.Result{}, res)
require.NoError(t, err)
res, err = reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: extKey})
require.Equal(t, ctrl.Result{}, res)
require.NoError(t, err)

t.Log("By fetching updated cluster extension after reconcile")
require.NoError(t, cl.Get(ctx, extKey, clusterExtension))
cond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeInstalled)
require.NotNil(t, cond)
require.Equal(t, metav1.ConditionTrue, cond.Status)

require.NoError(t, cl.DeleteAllOf(ctx, &ocv1alpha1.ClusterExtension{}))
res, err = reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: extKey})
require.Error(t, err, res)

t.Log("By fetching updated cluster extension after reconcile")
require.NoError(t, cl.Get(ctx, extKey, clusterExtension))
cond = apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeInstalled)
require.NotNil(t, cond)
require.Equal(t, metav1.ConditionTrue, cond.Status)
}

func verifyInvariants(ctx context.Context, t *testing.T, c client.Client, ext *ocv1alpha1.ClusterExtension) {
key := client.ObjectKeyFromObject(ext)
require.NoError(t, c.Get(ctx, key, ext))
Expand Down
14 changes: 14 additions & 0 deletions internal/finalizers/finalizers.go
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
package finalizers

import (
"context"

"sigs.k8s.io/controller-runtime/pkg/client"
crfinalizer "sigs.k8s.io/controller-runtime/pkg/finalizer"
)

type FinalizerFunc func(ctx context.Context, obj client.Object) (crfinalizer.Result, error)

func (f FinalizerFunc) Finalize(ctx context.Context, obj client.Object) (crfinalizer.Result, error) {
return f(ctx, obj)
}

0 comments on commit 7500b15

Please sign in to comment.