Skip to content

🐛 Use SSA instead of Update for finalizer operations #2328

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

🐛 Use SSA instead of Update for finalizer operations #2328

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
48 changes: 17 additions & 31 deletions cmd/operator-controller/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ import (
"sigs.k8s.io/controller-runtime/pkg/certwatcher"
"sigs.k8s.io/controller-runtime/pkg/client"
crcontroller "sigs.k8s.io/controller-runtime/pkg/controller"
crfinalizer "sigs.k8s.io/controller-runtime/pkg/finalizer"
"sigs.k8s.io/controller-runtime/pkg/healthz"
"sigs.k8s.io/controller-runtime/pkg/log"
"sigs.k8s.io/controller-runtime/pkg/manager"
Expand All @@ -70,7 +69,6 @@ import (
"github.com/operator-framework/operator-controller/internal/operator-controller/contentmanager"
"github.com/operator-framework/operator-controller/internal/operator-controller/controllers"
"github.com/operator-framework/operator-controller/internal/operator-controller/features"
"github.com/operator-framework/operator-controller/internal/operator-controller/finalizers"
"github.com/operator-framework/operator-controller/internal/operator-controller/resolve"
"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/preflights/crdupgradesafety"
"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/render"
Expand Down Expand Up @@ -391,12 +389,11 @@ func run() error {
},
}

clusterExtensionFinalizers := crfinalizer.NewFinalizers()
if err := clusterExtensionFinalizers.Register(controllers.ClusterExtensionCleanupUnpackCacheFinalizer, finalizers.FinalizerFunc(func(ctx context.Context, obj client.Object) (crfinalizer.Result, error) {
return crfinalizer.Result{}, imageCache.Delete(ctx, obj.GetName())
})); err != nil {
setupLog.Error(err, "unable to register finalizer", "finalizerKey", controllers.ClusterExtensionCleanupUnpackCacheFinalizer)
return err
// Set up finalizer handlers for ClusterExtension
clusterExtensionFinalizerHandlers := map[string]controllers.FinalizerHandler{
controllers.ClusterExtensionCleanupUnpackCacheFinalizer: func(ctx context.Context, ext *ocv1.ClusterExtension) error {
return imageCache.Delete(ctx, ext.GetName())
},
}

cl := mgr.GetClient()
Expand Down Expand Up @@ -443,11 +440,11 @@ func run() error {
}

ceReconciler := &controllers.ClusterExtensionReconciler{
Client: cl,
Resolver: resolver,
ImageCache: imageCache,
ImagePuller: imagePuller,
Finalizers: clusterExtensionFinalizers,
Client: cl,
Resolver: resolver,
ImageCache: imageCache,
ImagePuller: imagePuller,
FinalizerHandlers: clusterExtensionFinalizerHandlers,
}
ceController, err := ceReconciler.SetupWithManager(mgr, ctrlBuilderOpts...)
if err != nil {
Expand All @@ -464,9 +461,9 @@ func run() error {
}

if features.OperatorControllerFeatureGate.Enabled(features.BoxcutterRuntime) {
err = setupBoxcutter(mgr, ceReconciler, preflights, clusterExtensionFinalizers, regv1ManifestProvider)
err = setupBoxcutter(mgr, ceReconciler, preflights, regv1ManifestProvider)
} else {
err = setupHelm(mgr, ceReconciler, preflights, ceController, clusterExtensionFinalizers, regv1ManifestProvider)
err = setupHelm(mgr, ceReconciler, preflights, ceController, regv1ManifestProvider)
}
if err != nil {
setupLog.Error(err, "unable to setup lifecycler")
Expand Down Expand Up @@ -531,7 +528,6 @@ func setupBoxcutter(
mgr manager.Manager,
ceReconciler *controllers.ClusterExtensionReconciler,
preflights []applier.Preflight,
clusterExtensionFinalizers crfinalizer.Registerer,
regv1ManifestProvider applier.ManifestProvider,
) error {
coreClient, err := corev1client.NewForConfig(mgr.GetConfig())
Expand Down Expand Up @@ -560,13 +556,9 @@ func setupBoxcutter(
// This finalizer was added by the Helm applier for ClusterExtensions created
// before BoxcutterRuntime was enabled. Boxcutter doesn't use contentmanager,
// so we just need to acknowledge the finalizer to allow deletion to proceed.
err = clusterExtensionFinalizers.Register(controllers.ClusterExtensionCleanupContentManagerCacheFinalizer, finalizers.FinalizerFunc(func(ctx context.Context, obj client.Object) (crfinalizer.Result, error) {
ceReconciler.FinalizerHandlers[controllers.ClusterExtensionCleanupContentManagerCacheFinalizer] = func(ctx context.Context, ext *ocv1.ClusterExtension) error {
// No-op: Boxcutter doesn't use contentmanager, so no cleanup is needed
return crfinalizer.Result{}, nil
}))
if err != nil {
setupLog.Error(err, "unable to register content manager cleanup finalizer for boxcutter")
return err
return nil
}

// TODO: add support for preflight checks
Expand Down Expand Up @@ -638,7 +630,6 @@ func setupHelm(
ceReconciler *controllers.ClusterExtensionReconciler,
preflights []applier.Preflight,
ceController crcontroller.Controller,
clusterExtensionFinalizers crfinalizer.Registerer,
regv1ManifestProvider applier.ManifestProvider,
) error {
coreClient, err := corev1client.NewForConfig(mgr.GetConfig())
Expand Down Expand Up @@ -677,14 +668,9 @@ func setupHelm(
}

cm := contentmanager.NewManager(clientRestConfigMapper, mgr.GetConfig(), mgr.GetRESTMapper())
err = clusterExtensionFinalizers.Register(controllers.ClusterExtensionCleanupContentManagerCacheFinalizer, finalizers.FinalizerFunc(func(ctx context.Context, obj client.Object) (crfinalizer.Result, error) {
ext := obj.(*ocv1.ClusterExtension)
err := cm.Delete(ext)
return crfinalizer.Result{}, err
}))
if err != nil {
setupLog.Error(err, "unable to register content manager cleanup finalizer")
return err
// Register the content manager cleanup finalizer handler
ceReconciler.FinalizerHandlers[controllers.ClusterExtensionCleanupContentManagerCacheFinalizer] = func(ctx context.Context, ext *ocv1.ClusterExtension) error {
return cm.Delete(ext)
}

// now initialize the helmApplier, assigning the potentially nil preAuth
Expand Down
104 changes: 42 additions & 62 deletions internal/catalogd/controllers/core/clustercatalog_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,12 @@ import (
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
crfinalizer "sigs.k8s.io/controller-runtime/pkg/finalizer"
"sigs.k8s.io/controller-runtime/pkg/log"
"sigs.k8s.io/controller-runtime/pkg/reconcile"

ocv1 "github.com/operator-framework/operator-controller/api/v1"
"github.com/operator-framework/operator-controller/internal/catalogd/storage"
finalizerutil "github.com/operator-framework/operator-controller/internal/shared/util/finalizer"
imageutil "github.com/operator-framework/operator-controller/internal/shared/util/image"
)

Expand All @@ -59,8 +59,6 @@ type ClusterCatalogReconciler struct {

Storage storage.Instance

finalizers crfinalizer.Finalizers

// TODO: The below storedCatalogs fields are used for a quick a hack that helps
// us correctly populate a ClusterCatalog's status. The fact that we need
// these is indicative of a larger problem with the design of one or both
Expand Down Expand Up @@ -106,33 +104,18 @@ func (r *ClusterCatalogReconciler) Reconcile(ctx context.Context, req ctrl.Reque

// Do checks before any Update()s, as Update() may modify the resource structure!
updateStatus := !equality.Semantic.DeepEqual(existingCatsrc.Status, reconciledCatsrc.Status)
updateFinalizers := !equality.Semantic.DeepEqual(existingCatsrc.Finalizers, reconciledCatsrc.Finalizers)
unexpectedFieldsChanged := checkForUnexpectedFieldChange(existingCatsrc, *reconciledCatsrc)

if unexpectedFieldsChanged {
panic("spec or metadata changed by reconciler")
}

// Save the finalizers off to the side. If we update the status, the reconciledCatsrc will be updated
// to contain the new state of the ClusterCatalog, which contains the status update, but (critically)
// does not contain the finalizers. After the status update, we need to re-add the finalizers to the
// reconciledCatsrc before updating the object.
finalizers := reconciledCatsrc.Finalizers

if updateStatus {
if err := r.Client.Status().Update(ctx, reconciledCatsrc); err != nil {
reconcileErr = errors.Join(reconcileErr, fmt.Errorf("error updating status: %v", err))
}
}

reconciledCatsrc.Finalizers = finalizers

if updateFinalizers {
if err := r.Update(ctx, reconciledCatsrc); err != nil {
reconcileErr = errors.Join(reconcileErr, fmt.Errorf("error updating finalizers: %v", err))
}
}

return res, reconcileErr
}

Expand All @@ -142,10 +125,6 @@ func (r *ClusterCatalogReconciler) SetupWithManager(mgr ctrl.Manager) error {
defer r.storedCatalogsMu.Unlock()
r.storedCatalogs = make(map[string]storedCatalogData)

if err := r.setupFinalizers(); err != nil {
return fmt.Errorf("failed to setup finalizers: %v", err)
}

return ctrl.NewControllerManagedBy(mgr).
For(&ocv1.ClusterCatalog{}).
Named("catalogd-clustercatalog-controller").
Expand All @@ -171,32 +150,47 @@ func (r *ClusterCatalogReconciler) reconcile(ctx context.Context, catalog *ocv1.
return ctrl.Result{}, err
}

// Set status.conditions[type=Progressing] to False as we are done with
// all that needs to be done with the catalog
updateStatusProgressingUserSpecifiedUnavailable(&catalog.Status, catalog.GetGeneration())

// Remove the fbcDeletionFinalizer as we do not want a finalizer attached to the catalog
// when it is disabled. Because the finalizer serves no purpose now.
controllerutil.RemoveFinalizer(catalog, fbcDeletionFinalizer)
if err := finalizerutil.RemoveFinalizer(ctx, r.Client, catalog, fbcDeletionFinalizer); err != nil {
return ctrl.Result{}, fmt.Errorf("error removing finalizer: %v", err)
}

// Set status.conditions[type=Progressing] to True as we are done with
// all that needs to be done with the catalog
updateStatusProgressingUserSpecifiedUnavailable(&catalog.Status, catalog.GetGeneration())
// Clear URLs, ResolvedSource, and LastUnpacked since catalog is unavailable
catalog.Status.ResolvedSource = nil
catalog.Status.URLs = nil
catalog.Status.LastUnpacked = nil

return ctrl.Result{}, nil
}

finalizeResult, err := r.finalizers.Finalize(ctx, catalog)
if err != nil {
return ctrl.Result{}, err
}
if finalizeResult.Updated || finalizeResult.StatusUpdated {
// On create: make sure the finalizer is applied before we do anything
// On delete: make sure we do nothing after the finalizer is removed
// Handle deletion
if catalog.GetDeletionTimestamp() != nil {
if !controllerutil.ContainsFinalizer(catalog, fbcDeletionFinalizer) {
// All finalizers removed, nothing more to do
return ctrl.Result{}, nil
}
if err := r.deleteCatalogCache(ctx, catalog); err != nil {
return ctrl.Result{}, fmt.Errorf("finalizer %q failed: %w", fbcDeletionFinalizer, err)
}
if err := finalizerutil.RemoveFinalizer(ctx, r.Client, catalog, fbcDeletionFinalizer); err != nil {
return ctrl.Result{}, fmt.Errorf("error removing finalizer: %v", err)
}
// Update status to reflect that catalog is no longer serving
updateStatusNotServing(&catalog.Status, catalog.GetGeneration())
return ctrl.Result{}, nil
}

if catalog.GetDeletionTimestamp() != nil {
// If we've gotten here, that means the cluster catalog is being deleted, we've handled all of
// _our_ finalizers (above), but the cluster catalog is still present in the cluster, likely
// because there are _other_ finalizers that other controllers need to handle, (e.g. the orphan
// deletion finalizer).
// Ensure finalizer is present
finalizerAdded, err := finalizerutil.EnsureFinalizer(ctx, r.Client, catalog, fbcDeletionFinalizer)
if err != nil {
return ctrl.Result{}, fmt.Errorf("error ensuring finalizer: %v", err)
}
// On create: make sure the finalizer is applied before we do anything else
if finalizerAdded {
return ctrl.Result{}, nil
}

Expand Down Expand Up @@ -419,30 +413,16 @@ func (r *ClusterCatalogReconciler) needsPoll(lastSuccessfulPoll time.Time, catal
func checkForUnexpectedFieldChange(a, b ocv1.ClusterCatalog) bool {
a.Status, b.Status = ocv1.ClusterCatalogStatus{}, ocv1.ClusterCatalogStatus{}
a.Finalizers, b.Finalizers = []string{}, []string{}
return !equality.Semantic.DeepEqual(a, b)
}

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 (r *ClusterCatalogReconciler) setupFinalizers() error {
f := crfinalizer.NewFinalizers()
err := f.Register(fbcDeletionFinalizer, finalizerFunc(func(ctx context.Context, obj client.Object) (crfinalizer.Result, error) {
catalog, ok := obj.(*ocv1.ClusterCatalog)
if !ok {
panic("could not convert object to clusterCatalog")
}
err := r.deleteCatalogCache(ctx, catalog)
return crfinalizer.Result{StatusUpdated: true}, err
}))
if err != nil {
return err
a.ManagedFields, b.ManagedFields = nil, nil
a.ResourceVersion, b.ResourceVersion = "", ""
// Remove kubectl's last-applied-configuration annotation which may be added by the API server
if a.Annotations != nil {
delete(a.Annotations, "kubectl.kubernetes.io/last-applied-configuration")
}
r.finalizers = f
return nil
if b.Annotations != nil {
delete(b.Annotations, "kubectl.kubernetes.io/last-applied-configuration")
}
return !equality.Semantic.DeepEqual(a, b)
}

func (r *ClusterCatalogReconciler) deleteStoredCatalog(catalogName string) {
Expand Down
27 changes: 21 additions & 6 deletions internal/catalogd/controllers/core/clustercatalog_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,10 @@ import (
"go.podman.io/image/v5/docker/reference"
"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/utils/ptr"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client/fake"
"sigs.k8s.io/controller-runtime/pkg/reconcile"

ocv1 "github.com/operator-framework/operator-controller/api/v1"
Expand Down Expand Up @@ -304,6 +306,7 @@ func TestCatalogdControllerReconcile(t *testing.T) {
name: "storage finalizer not set, storage finalizer gets set",
puller: &imageutil.MockPuller{
ImageFS: &fstest.MapFS{},
Ref: mustRef(t, "my.org/someimage@sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"),
},
store: &MockStore{},
catalog: &ocv1.ClusterCatalog{
Expand Down Expand Up @@ -332,6 +335,8 @@ func TestCatalogdControllerReconcile(t *testing.T) {
},
},
},
// Status remains empty because controller returns early after adding finalizer
Status: ocv1.ClusterCatalogStatus{},
},
},
{
Expand Down Expand Up @@ -802,8 +807,12 @@ func TestCatalogdControllerReconcile(t *testing.T) {
},
} {
t.Run(tt.name, func(t *testing.T) {
scheme := runtime.NewScheme()
require.NoError(t, ocv1.AddToScheme(scheme))
cl := fake.NewClientBuilder().WithScheme(scheme).WithObjects(tt.catalog).WithStatusSubresource(tt.catalog).Build()

reconciler := &ClusterCatalogReconciler{
Client: nil,
Client: cl,
ImagePuller: tt.puller,
ImageCache: tt.cache,
Storage: tt.store,
Expand All @@ -812,7 +821,6 @@ func TestCatalogdControllerReconcile(t *testing.T) {
if reconciler.ImageCache == nil {
reconciler.ImageCache = &imageutil.MockCache{}
}
require.NoError(t, reconciler.setupFinalizers())
ctx := context.Background()

res, err := reconciler.reconcile(ctx, tt.catalog)
Expand All @@ -826,6 +834,7 @@ func TestCatalogdControllerReconcile(t *testing.T) {
}
diff := cmp.Diff(tt.expectedCatalog, tt.catalog,
cmpopts.IgnoreFields(metav1.Condition{}, "Message", "LastTransitionTime"),
cmpopts.IgnoreFields(metav1.ObjectMeta{}, "ResourceVersion"),
cmpopts.SortSlices(func(a, b metav1.Condition) bool { return a.Type < b.Type }))
assert.Empty(t, diff, "comparing the expected Catalog")
})
Expand Down Expand Up @@ -909,8 +918,12 @@ func TestPollingRequeue(t *testing.T) {
URLs: &ocv1.ClusterCatalogURLs{Base: "URL"},
LastUnpacked: ptr.To(metav1.NewTime(time.Now().Truncate(time.Second))),
}
scheme := runtime.NewScheme()
require.NoError(t, ocv1.AddToScheme(scheme))
cl := fake.NewClientBuilder().WithScheme(scheme).WithObjects(tc.catalog).WithStatusSubresource(tc.catalog).Build()

reconciler := &ClusterCatalogReconciler{
Client: nil,
Client: cl,
ImagePuller: &imageutil.MockPuller{
ImageFS: &fstest.MapFS{},
Ref: ref,
Expand All @@ -924,7 +937,6 @@ func TestPollingRequeue(t *testing.T) {
},
},
}
require.NoError(t, reconciler.setupFinalizers())
res, _ := reconciler.reconcile(context.Background(), tc.catalog)
assert.InDelta(t, tc.expectedRequeueAfter, res.RequeueAfter, 2*requeueJitterMaxFactor*float64(tc.expectedRequeueAfter))
})
Expand Down Expand Up @@ -1136,13 +1148,16 @@ func TestPollingReconcilerUnpack(t *testing.T) {
if scd == nil {
scd = map[string]storedCatalogData{}
}
scheme := runtime.NewScheme()
require.NoError(t, ocv1.AddToScheme(scheme))
cl := fake.NewClientBuilder().WithScheme(scheme).WithObjects(tc.catalog).WithStatusSubresource(tc.catalog).Build()

reconciler := &ClusterCatalogReconciler{
Client: nil,
Client: cl,
ImagePuller: &imageutil.MockPuller{Error: errors.New("mockpuller error")},
Storage: &MockStore{},
storedCatalogs: scd,
}
require.NoError(t, reconciler.setupFinalizers())
_, err := reconciler.reconcile(context.Background(), tc.catalog)
if tc.expectedUnpackRun {
assert.Error(t, err)
Expand Down
Loading
Loading