Skip to content

Commit f7d67d8

Browse files
pedjakclaude
andcommitted
refactor: replace contentmanager with shared TrackingCache
Replace the per-ClusterExtension contentmanager cache with a single shared TrackingCache from boxcutter/managedcache. The contentmanager created per-CE dynamic clients and informer factories for drift detection; now that all operations use the same controller identity, a single global cache is simpler and more efficient. - Create TrackingCache at shared scope before controller setup - Wire TrackingCache.Source to CE controller via WithWatchesRawSource - Replace Manager+Watcher fields in Helm applier with trackingCache interface (Watch + Free) - Suppress Create events on the source predicate to match old behavior - Update finalizer to call TrackingCache.Free instead of cm.Delete - Remove ClientRestConfigMapper from Helm action config getter - Update helm_test.go mocks for new interface - Delete contentmanager/ package (~1,240 lines) OPRUN-4633 Co-Authored-By: Claude <noreply@anthropic.com>
1 parent 10e7337 commit f7d67d8

12 files changed

Lines changed: 85 additions & 1346 deletions

File tree

.claude/TODO-OCPSTRAT-3040.md

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -69,12 +69,12 @@ Anyone: Stream 6 (docs) ← after 4
6969
7070
### Stream 5 — Global shared cache refactor (after stream 4)
7171

72-
- [ ] Refactor `contentmanager/` from per-CE caches to single global cache — **OPRUN-4633**
73-
- [ ] Remove `createScopedClient` from `revision_engine_factory.go`**OPRUN-4633**
74-
- [ ] Simplify `RestConfigMapper` pattern — **OPRUN-4633**
75-
- [ ] Refactor Boxcutter to shared client (if applicable) — **OPRUN-4633**
76-
- [ ] Verify no regression in drift detection / managed content watching — **OPRUN-4633**
77-
- [ ] Update unit + e2e tests — **OPRUN-4633**
72+
- [X] Refactor `contentmanager/` from per-CE caches to single global cache — **OPRUN-4633**
73+
- [X] Remove `createScopedClient` from `revision_engine_factory.go`**OPRUN-4633** `61b62e78` (Daniel)
74+
- [X] Simplify `RestConfigMapper` pattern — **OPRUN-4633**
75+
- [X] Refactor Boxcutter to shared client (if applicable) — **OPRUN-4633** `61b62e78` (Daniel)
76+
- [X] Verify no regression in drift detection / managed content watching — **OPRUN-4633**
77+
- [X] Update unit + e2e tests — **OPRUN-4633**
7878

7979
> **Ticket close gate:** OPRUN-4633 closable when all stream 5 items are done
8080

cmd/operator-controller/main.go

Lines changed: 37 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -41,20 +41,22 @@ import (
4141
"k8s.io/client-go/discovery/cached/memory"
4242
corev1client "k8s.io/client-go/kubernetes/typed/core/v1"
4343
_ "k8s.io/client-go/plugin/pkg/client/auth"
44-
"k8s.io/client-go/rest"
4544
"k8s.io/klog/v2"
4645
"k8s.io/utils/ptr"
4746
"pkg.package-operator.run/boxcutter/managedcache"
4847
ctrl "sigs.k8s.io/controller-runtime"
4948
crcache "sigs.k8s.io/controller-runtime/pkg/cache"
5049
"sigs.k8s.io/controller-runtime/pkg/certwatcher"
5150
"sigs.k8s.io/controller-runtime/pkg/client"
51+
"sigs.k8s.io/controller-runtime/pkg/event"
5252
crfinalizer "sigs.k8s.io/controller-runtime/pkg/finalizer"
53+
crhandler "sigs.k8s.io/controller-runtime/pkg/handler"
5354
"sigs.k8s.io/controller-runtime/pkg/healthz"
5455
"sigs.k8s.io/controller-runtime/pkg/log"
5556
"sigs.k8s.io/controller-runtime/pkg/manager"
5657
"sigs.k8s.io/controller-runtime/pkg/metrics/filters"
5758
"sigs.k8s.io/controller-runtime/pkg/metrics/server"
59+
"sigs.k8s.io/controller-runtime/pkg/predicate"
5860

5961
helmclient "github.com/operator-framework/helm-operator-plugins/pkg/client"
6062

@@ -63,8 +65,6 @@ import (
6365
"github.com/operator-framework/operator-controller/internal/operator-controller/applier"
6466
"github.com/operator-framework/operator-controller/internal/operator-controller/catalogmetadata/cache"
6567
catalogclient "github.com/operator-framework/operator-controller/internal/operator-controller/catalogmetadata/client"
66-
"github.com/operator-framework/operator-controller/internal/operator-controller/contentmanager"
67-
cmcache "github.com/operator-framework/operator-controller/internal/operator-controller/contentmanager/cache"
6868
"github.com/operator-framework/operator-controller/internal/operator-controller/controllers"
6969
"github.com/operator-framework/operator-controller/internal/operator-controller/features"
7070
"github.com/operator-framework/operator-controller/internal/operator-controller/finalizers"
@@ -118,6 +118,7 @@ type boxcutterReconcilerConfigurator struct {
118118
imageCache imageutil.Cache
119119
imagePuller imageutil.Puller
120120
finalizers crfinalizer.Finalizers
121+
trackingCache managedcache.TrackingCache
121122
}
122123

123124
type helmReconcilerConfigurator struct {
@@ -128,7 +129,7 @@ type helmReconcilerConfigurator struct {
128129
imageCache imageutil.Cache
129130
imagePuller imageutil.Puller
130131
finalizers crfinalizer.Finalizers
131-
watcher cmcache.Watcher
132+
trackingCache managedcache.TrackingCache
132133
}
133134

134135
const (
@@ -459,15 +460,40 @@ func run() error {
459460
crdupgradesafety.NewPreflight(aeClient.CustomResourceDefinitions()),
460461
}
461462

463+
trackingCache, err := managedcache.NewTrackingCache(
464+
ctrl.Log.WithName("trackingCache"),
465+
mgr.GetConfig(),
466+
crcache.Options{
467+
Scheme: mgr.GetScheme(), Mapper: mgr.GetRESTMapper(),
468+
},
469+
)
470+
if err != nil {
471+
setupLog.Error(err, "unable to create tracking cache")
472+
return err
473+
}
474+
if err := mgr.Add(trackingCache); err != nil {
475+
setupLog.Error(err, "unable to add tracking cache to manager")
476+
return err
477+
}
478+
462479
var ctrlBuilderOpts []controllers.ControllerBuilderOption
463480
if features.OperatorControllerFeatureGate.Enabled(features.BoxcutterRuntime) {
464481
ctrlBuilderOpts = append(ctrlBuilderOpts, controllers.WithOwns(&ocv1.ClusterObjectSet{}))
465482
}
483+
ctrlBuilderOpts = append(ctrlBuilderOpts, controllers.WithWatchesRawSource(
484+
trackingCache.Source(
485+
crhandler.EnqueueRequestForOwner(mgr.GetScheme(), mgr.GetRESTMapper(), &ocv1.ClusterExtension{}),
486+
predicate.ResourceVersionChangedPredicate{},
487+
predicate.Funcs{
488+
CreateFunc: func(event.TypedCreateEvent[client.Object]) bool { return false },
489+
},
490+
),
491+
))
466492

467493
ceReconciler := &controllers.ClusterExtensionReconciler{
468494
Client: cl,
469495
}
470-
ceController, err := ceReconciler.SetupWithManager(mgr, ctrlBuilderOpts...)
496+
_, err = ceReconciler.SetupWithManager(mgr, ctrlBuilderOpts...)
471497
if err != nil {
472498
setupLog.Error(err, "unable to create controller", "controller", "ClusterExtension")
473499
return err
@@ -491,6 +517,7 @@ func run() error {
491517
imageCache: imageCache,
492518
imagePuller: imagePuller,
493519
finalizers: clusterExtensionFinalizers,
520+
trackingCache: trackingCache,
494521
}
495522
} else {
496523
cerCfg = &helmReconcilerConfigurator{
@@ -501,7 +528,7 @@ func run() error {
501528
imageCache: imageCache,
502529
imagePuller: imagePuller,
503530
finalizers: clusterExtensionFinalizers,
504-
watcher: ceController,
531+
trackingCache: trackingCache,
505532
}
506533
}
507534
if err := cerCfg.Configure(ceReconciler); err != nil {
@@ -642,23 +669,9 @@ func (c *boxcutterReconcilerConfigurator) Configure(ceReconciler *controllers.Cl
642669
// Wrap the discovery client with caching to reduce memory usage from repeated OpenAPI schema fetches
643670
discoveryClient := memory.NewMemCacheClient(baseDiscoveryClient)
644671

645-
trackingCache, err := managedcache.NewTrackingCache(
646-
ctrl.Log.WithName("trackingCache"),
647-
c.mgr.GetConfig(),
648-
crcache.Options{
649-
Scheme: c.mgr.GetScheme(), Mapper: c.mgr.GetRESTMapper(),
650-
},
651-
)
652-
if err != nil {
653-
return fmt.Errorf("unable to create boxcutter tracking cache: %v", err)
654-
}
655-
if err := c.mgr.Add(trackingCache); err != nil {
656-
return fmt.Errorf("unable to add tracking cache to manager: %v", err)
657-
}
658-
659672
revisionEngineFactory, err := controllers.NewDefaultRevisionEngineFactory(
660673
c.mgr.GetScheme(),
661-
trackingCache,
674+
c.trackingCache,
662675
discoveryClient,
663676
c.mgr.GetRESTMapper(),
664677
fieldOwnerPrefix,
@@ -676,7 +689,7 @@ func (c *boxcutterReconcilerConfigurator) Configure(ceReconciler *controllers.Cl
676689
if err = (&controllers.ClusterObjectSetReconciler{
677690
Client: cosClient,
678691
RevisionEngineFactory: revisionEngineFactory,
679-
TrackingCache: trackingCache,
692+
TrackingCache: c.trackingCache,
680693
}).SetupWithManager(c.mgr); err != nil {
681694
return fmt.Errorf("unable to setup ClusterObjectSet controller: %w", err)
682695
}
@@ -688,17 +701,12 @@ func (c *helmReconcilerConfigurator) Configure(ceReconciler *controllers.Cluster
688701
if err != nil {
689702
return fmt.Errorf("unable to create core client: %w", err)
690703
}
691-
clientRestConfigMapper := func(_ context.Context, _ client.Object, _ *rest.Config) (*rest.Config, error) {
692-
return rest.CopyConfig(c.mgr.GetConfig()), nil
693-
}
694-
695704
cfgGetter, err := helmclient.NewActionConfigGetter(c.mgr.GetConfig(), c.mgr.GetRESTMapper(),
696705
helmclient.StorageDriverMapper(action.ChunkedStorageDriverMapper(coreClient, c.mgr.GetAPIReader(), cfg.systemNamespace)),
697706
helmclient.ClientNamespaceMapper(func(obj client.Object) (string, error) {
698707
ext := obj.(*ocv1.ClusterExtension)
699708
return ext.Spec.Namespace, nil
700709
}),
701-
helmclient.ClientRestConfigMapper(clientRestConfigMapper),
702710
)
703711
if err != nil {
704712
return fmt.Errorf("unable to create helm action config getter: %w", err)
@@ -711,11 +719,8 @@ func (c *helmReconcilerConfigurator) Configure(ceReconciler *controllers.Cluster
711719
return fmt.Errorf("unable to create helm action client getter: %w", err)
712720
}
713721

714-
cm := contentmanager.NewManager(clientRestConfigMapper, c.mgr.GetConfig(), c.mgr.GetRESTMapper())
715722
err = c.finalizers.Register(controllers.ClusterExtensionCleanupContentManagerCacheFinalizer, finalizers.FinalizerFunc(func(ctx context.Context, obj client.Object) (crfinalizer.Result, error) {
716-
ext := obj.(*ocv1.ClusterExtension)
717-
err := cm.Delete(ext)
718-
return crfinalizer.Result{}, err
723+
return crfinalizer.Result{}, c.trackingCache.Free(ctx, obj)
719724
}))
720725
if err != nil {
721726
setupLog.Error(err, "unable to register content manager cleanup finalizer")
@@ -729,8 +734,7 @@ func (c *helmReconcilerConfigurator) Configure(ceReconciler *controllers.Cluster
729734
ManifestProvider: c.regv1ManifestProvider,
730735
},
731736
HelmReleaseToObjectsConverter: &applier.HelmReleaseToObjectsConverter{},
732-
Watcher: c.watcher,
733-
Manager: cm,
737+
TrackingCache: c.trackingCache,
734738
}
735739
revisionStatesGetter := &controllers.HelmRevisionStatesGetter{ActionClientGetter: acg}
736740
ceReconciler.ReconcileSteps = []controllers.ReconcileStepFunc{

internal/operator-controller/applier/helm.go

Lines changed: 22 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -16,20 +16,25 @@ import (
1616
"helm.sh/helm/v3/pkg/release"
1717
"helm.sh/helm/v3/pkg/storage/driver"
1818
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
19+
"k8s.io/apimachinery/pkg/runtime/schema"
20+
"k8s.io/apimachinery/pkg/util/sets"
1921
apimachyaml "k8s.io/apimachinery/pkg/util/yaml"
2022
"k8s.io/klog/v2"
2123
"sigs.k8s.io/controller-runtime/pkg/client"
2224

2325
helmclient "github.com/operator-framework/helm-operator-plugins/pkg/client"
2426

2527
ocv1 "github.com/operator-framework/operator-controller/api/v1"
26-
"github.com/operator-framework/operator-controller/internal/operator-controller/contentmanager"
27-
"github.com/operator-framework/operator-controller/internal/operator-controller/contentmanager/cache"
2828
"github.com/operator-framework/operator-controller/internal/operator-controller/features"
2929
"github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/util"
3030
imageutil "github.com/operator-framework/operator-controller/internal/shared/util/image"
3131
)
3232

33+
type trackingCache interface {
34+
Watch(ctx context.Context, user client.Object, gvks sets.Set[schema.GroupVersionKind]) error
35+
Free(ctx context.Context, user client.Object) error
36+
}
37+
3338
// HelmChartProvider provides helm charts from bundle sources and cluster extensions
3439
type HelmChartProvider interface {
3540
Get(bundle fs.FS, clusterExtension *ocv1.ClusterExtension) (*chart.Chart, error)
@@ -60,8 +65,7 @@ type Helm struct {
6065
HelmChartProvider HelmChartProvider
6166
HelmReleaseToObjectsConverter HelmReleaseToObjectsConverterInterface
6267

63-
Manager contentmanager.Manager
64-
Watcher cache.Watcher
68+
TrackingCache trackingCache
6569
}
6670

6771
func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExtension, objectLabels map[string]string, storageLabels map[string]string) (bool, string, error) {
@@ -149,12 +153,8 @@ func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExte
149153
return true, "", err
150154
}
151155
klog.FromContext(ctx).Info("watching managed objects")
152-
cache, err := h.Manager.Get(ctx, ext)
153-
if err != nil {
154-
return true, "", err
155-
}
156-
157-
if err := cache.Watch(ctx, h.Watcher, relObjects...); err != nil {
156+
gvks := gvksForObjects(relObjects)
157+
if err := h.TrackingCache.Watch(ctx, ext, gvks); err != nil {
158158
return true, "", err
159159
}
160160

@@ -194,22 +194,12 @@ func (h *Helm) reconcileExistingRelease(ctx context.Context, ac helmclient.Actio
194194

195195
logger.V(1).Info("setting up drift detection watches on managed objects")
196196

197-
// Defensive nil checks to prevent panics if Manager or Watcher not properly initialized
198-
if h.Manager == nil {
199-
logger.Error(fmt.Errorf("manager is nil"), "Manager not initialized, cannot set up drift detection watches (resources are applied but drift detection disabled)")
197+
if h.TrackingCache == nil {
198+
logger.Error(fmt.Errorf("tracking cache is nil"), "TrackingCache not initialized, cannot set up drift detection watches (resources are applied but drift detection disabled)")
200199
return true, "", nil
201200
}
202-
cache, err := h.Manager.Get(ctx, ext)
203-
if err != nil {
204-
logger.Error(err, "failed to get managed content cache, cannot set up drift detection watches (resources are applied but drift detection disabled)")
205-
return true, "", nil
206-
}
207-
208-
if h.Watcher == nil {
209-
logger.Error(fmt.Errorf("watcher is nil"), "Watcher not initialized, cannot set up drift detection watches (resources are applied but drift detection disabled)")
210-
return true, "", nil
211-
}
212-
if err := cache.Watch(ctx, h.Watcher, relObjects...); err != nil {
201+
gvks := gvksForObjects(relObjects)
202+
if err := h.TrackingCache.Watch(ctx, ext, gvks); err != nil {
213203
logger.Error(err, "failed to set up drift detection watches (resources are applied but drift detection disabled)")
214204
return true, "", nil
215205
}
@@ -298,3 +288,11 @@ func (p *postrenderer) Run(renderedManifests *bytes.Buffer) (*bytes.Buffer, erro
298288
}
299289
return &buf, nil
300290
}
291+
292+
func gvksForObjects(objs []client.Object) sets.Set[schema.GroupVersionKind] {
293+
gvks := sets.New[schema.GroupVersionKind]()
294+
for _, obj := range objs {
295+
gvks.Insert(obj.GetObjectKind().GroupVersionKind())
296+
}
297+
return gvks
298+
}

0 commit comments

Comments
 (0)