Skip to content
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

🌱 Cleanup separate unstructuredCachingClient #10692

Merged
merged 1 commit into from
May 30, 2024
+193 −371
Merged

🌱 Cleanup separate unstructuredCachingClient #10692

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
2 changes: 2 additions & 0 deletions bootstrap/kubeadm/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,8 @@ func main() {
&corev1.ConfigMap{},
&corev1.Secret{},
},
// Use the cache for all Unstructured get/list calls.
Unstructured: true,
},
},
WebhookServer: webhook.NewServer(
Expand Down
8 changes: 3 additions & 5 deletions cmd/clusterctl/client/cluster/topology.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,9 +200,8 @@ func (t *topologyClient) Plan(ctx context.Context, in *TopologyPlanInput) (*Topo

res.ReconciledCluster = targetCluster
reconciler := &clustertopologycontroller.Reconciler{
Client: dryRunClient,
APIReader: dryRunClient,
UnstructuredCachingClient: dryRunClient,
Client: dryRunClient,
APIReader: dryRunClient,
}
reconciler.SetupForDryRun(&noOpRecorder{})
request := reconcile.Request{NamespacedName: *targetCluster}
Expand Down Expand Up @@ -515,8 +514,7 @@ func reconcileClusterClass(ctx context.Context, apiReader client.Reader, class c
reconcilerClient := dryrun.NewClient(apiReader, reconciliationObjects)

clusterClassReconciler := &clusterclasscontroller.Reconciler{
Client: reconcilerClient,
UnstructuredCachingClient: reconcilerClient,
Client: reconcilerClient,
}

if _, err := clusterClassReconciler.Reconcile(ctx, reconcile.Request{NamespacedName: targetClusterClass}); err != nil {
Expand Down
76 changes: 29 additions & 47 deletions controllers/alias.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,29 +42,26 @@ import (

// ClusterReconciler reconciles a Cluster object.
type ClusterReconciler struct {
Client client.Client
Copy link
Member

Choose a reason for hiding this comment

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

night need to handle deprecation https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api/10692/pull-cluster-api-apidiff-main/1795517221561176064

Copy link
Member Author

Choose a reason for hiding this comment

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

I can add the fields here again (with //Deprecated and without using them) if you want.

This would allow folks one more minor release to not set these fields. Not sure if it makes much difference to be honest

UnstructuredCachingClient client.Client
APIReader client.Reader
Client client.Client
APIReader client.Reader

// WatchFilterValue is the label value used to filter events prior to reconciliation.
WatchFilterValue string
}

func (r *ClusterReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, options controller.Options) error {
return (&clustercontroller.Reconciler{
Client: r.Client,
UnstructuredCachingClient: r.UnstructuredCachingClient,
APIReader: r.APIReader,
WatchFilterValue: r.WatchFilterValue,
Client: r.Client,
APIReader: r.APIReader,
WatchFilterValue: r.WatchFilterValue,
}).SetupWithManager(ctx, mgr, options)
}

// MachineReconciler reconciles a Machine object.
type MachineReconciler struct {
Client client.Client
UnstructuredCachingClient client.Client
APIReader client.Reader
Tracker *remote.ClusterCacheTracker
Client client.Client
APIReader client.Reader
Tracker *remote.ClusterCacheTracker

// WatchFilterValue is the label value used to filter events prior to reconciliation.
WatchFilterValue string
Expand All @@ -75,21 +72,19 @@ type MachineReconciler struct {

func (r *MachineReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, options controller.Options) error {
return (&machinecontroller.Reconciler{
Client: r.Client,
UnstructuredCachingClient: r.UnstructuredCachingClient,
APIReader: r.APIReader,
Tracker: r.Tracker,
WatchFilterValue: r.WatchFilterValue,
NodeDrainClientTimeout: r.NodeDrainClientTimeout,
Client: r.Client,
APIReader: r.APIReader,
Tracker: r.Tracker,
WatchFilterValue: r.WatchFilterValue,
NodeDrainClientTimeout: r.NodeDrainClientTimeout,
}).SetupWithManager(ctx, mgr, options)
}

// MachineSetReconciler reconciles a MachineSet object.
type MachineSetReconciler struct {
Client client.Client
UnstructuredCachingClient client.Client
APIReader client.Reader
Tracker *remote.ClusterCacheTracker
Client client.Client
APIReader client.Reader
Tracker *remote.ClusterCacheTracker

// WatchFilterValue is the label value used to filter events prior to reconciliation.
WatchFilterValue string
Expand All @@ -101,7 +96,6 @@ type MachineSetReconciler struct {
func (r *MachineSetReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, options controller.Options) error {
return (&machinesetcontroller.Reconciler{
Client: r.Client,
UnstructuredCachingClient: r.UnstructuredCachingClient,
APIReader: r.APIReader,
Tracker: r.Tracker,
WatchFilterValue: r.WatchFilterValue,
Expand All @@ -111,20 +105,18 @@ func (r *MachineSetReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Ma

// MachineDeploymentReconciler reconciles a MachineDeployment object.
type MachineDeploymentReconciler struct {
Client client.Client
UnstructuredCachingClient client.Client
APIReader client.Reader
Client client.Client
APIReader client.Reader

// WatchFilterValue is the label value used to filter events prior to reconciliation.
WatchFilterValue string
}

func (r *MachineDeploymentReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, options controller.Options) error {
return (&machinedeploymentcontroller.Reconciler{
Client: r.Client,
UnstructuredCachingClient: r.UnstructuredCachingClient,
APIReader: r.APIReader,
WatchFilterValue: r.WatchFilterValue,
Client: r.Client,
APIReader: r.APIReader,
WatchFilterValue: r.WatchFilterValue,
}).SetupWithManager(ctx, mgr, options)
}

Expand Down Expand Up @@ -157,20 +149,15 @@ type ClusterTopologyReconciler struct {

// WatchFilterValue is the label value used to filter events prior to reconciliation.
WatchFilterValue string

// UnstructuredCachingClient provides a client that forces caching of unstructured objects,
// thus allowing to optimize reads for templates or provider specific objects in a managed topology.
UnstructuredCachingClient client.Client
}

func (r *ClusterTopologyReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, options controller.Options) error {
return (&clustertopologycontroller.Reconciler{
Client: r.Client,
APIReader: r.APIReader,
Tracker: r.Tracker,
RuntimeClient: r.RuntimeClient,
UnstructuredCachingClient: r.UnstructuredCachingClient,
WatchFilterValue: r.WatchFilterValue,
Client: r.Client,
APIReader: r.APIReader,
Tracker: r.Tracker,
RuntimeClient: r.RuntimeClient,
WatchFilterValue: r.WatchFilterValue,
}).SetupWithManager(ctx, mgr, options)
}

Expand Down Expand Up @@ -227,18 +214,13 @@ type ClusterClassReconciler struct {

// WatchFilterValue is the label value used to filter events prior to reconciliation.
WatchFilterValue string

// UnstructuredCachingClient provides a client that forces caching of unstructured objects,
// thus allowing to optimize reads for templates or provider specific objects.
UnstructuredCachingClient client.Client
}

func (r *ClusterClassReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, options controller.Options) error {
r.internalReconciler = &clusterclasscontroller.Reconciler{
Client: r.Client,
RuntimeClient: r.RuntimeClient,
UnstructuredCachingClient: r.UnstructuredCachingClient,
WatchFilterValue: r.WatchFilterValue,
Client: r.Client,
RuntimeClient: r.RuntimeClient,
WatchFilterValue: r.WatchFilterValue,
}
return r.internalReconciler.SetupWithManager(ctx, mgr, options)
}
Expand Down
2 changes: 1 addition & 1 deletion controlplane/kubeadm/internal/controllers/helpers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,7 +338,7 @@ func TestCloneConfigsAndGenerateMachine(t *testing.T) {
},
},
}
g.Expect(env.Create(ctx, genericInfrastructureMachineTemplate)).To(Succeed())
g.Expect(env.CreateAndWait(ctx, genericInfrastructureMachineTemplate)).To(Succeed())
Copy link
Member Author

Choose a reason for hiding this comment

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

I think I had to change this because now the tests are also using cached unstructured get/list calls (production code in KCP always did)


kcp := &controlplanev1.KubeadmControlPlane{
ObjectMeta: metav1.ObjectMeta{
Expand Down
2 changes: 1 addition & 1 deletion controlplane/kubeadm/internal/controllers/upgrade_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ func TestKubeadmControlPlaneReconciler_RolloutStrategy_ScaleUp(t *testing.T) {
timeout := 30 * time.Second

cluster, kcp, genericInfrastructureMachineTemplate := createClusterWithControlPlane(namespace.Name)
g.Expect(env.Create(ctx, genericInfrastructureMachineTemplate, client.FieldOwner("manager"))).To(Succeed())
g.Expect(env.CreateAndWait(ctx, genericInfrastructureMachineTemplate, client.FieldOwner("manager"))).To(Succeed())
cluster.UID = types.UID(util.RandomString(10))
cluster.Spec.ControlPlaneEndpoint.Host = Host
cluster.Spec.ControlPlaneEndpoint.Port = 6443
Expand Down
2 changes: 1 addition & 1 deletion controlplane/kubeadm/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,7 @@ func main() {
&corev1.ConfigMap{},
&corev1.Secret{},
},
// This config ensures that the default client caches Unstructured objects.
// This config ensures that the default client uses the cache for all Unstructured get/list calls.
// KCP is only using Unstructured to retrieve InfraMachines and InfraMachineTemplates.
// As the cache should be used in those cases, caching is configured globally instead of
// creating a separate client that caches Unstructured.
Expand Down
1 change: 0 additions & 1 deletion docs/proposals/20220221-runtime-SDK.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -576,7 +576,6 @@ func setupReconcilers(ctx context.Context, mgr ctrl.Manager) {
Client: mgr.GetClient(),
APIReader: mgr.GetAPIReader(),
RuntimeClient: runtimeClient,
UnstructuredCachingClient: unstructuredCachingClient,
WatchFilterValue: watchFilterValue,
}).SetupWithManager(ctx, mgr, concurrency(clusterTopologyConcurrency)); err != nil {
setupLog.Error(err, "Unable to create controller", "controller", "ClusterTopology")
Expand Down
9 changes: 4 additions & 5 deletions internal/controllers/cluster/cluster_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,8 @@ const (

// Reconciler reconciles a Cluster object.
type Reconciler struct {
Client client.Client
UnstructuredCachingClient client.Client
APIReader client.Reader
Client client.Client
APIReader client.Reader

// WatchFilterValue is the label value used to filter events prior to reconciliation.
WatchFilterValue string
Expand Down Expand Up @@ -279,7 +278,7 @@ func (r *Reconciler) reconcileDelete(ctx context.Context, cluster *clusterv1.Clu
}

if cluster.Spec.ControlPlaneRef != nil {
obj, err := external.Get(ctx, r.UnstructuredCachingClient, cluster.Spec.ControlPlaneRef, cluster.Namespace)
obj, err := external.Get(ctx, r.Client, cluster.Spec.ControlPlaneRef, cluster.Namespace)
switch {
case apierrors.IsNotFound(errors.Cause(err)):
// All good - the control plane resource has been deleted
Expand Down Expand Up @@ -310,7 +309,7 @@ func (r *Reconciler) reconcileDelete(ctx context.Context, cluster *clusterv1.Clu
}

if cluster.Spec.InfrastructureRef != nil {
obj, err := external.Get(ctx, r.UnstructuredCachingClient, cluster.Spec.InfrastructureRef, cluster.Namespace)
obj, err := external.Get(ctx, r.Client, cluster.Spec.InfrastructureRef, cluster.Namespace)
switch {
case apierrors.IsNotFound(errors.Cause(err)):
// All good - the infra resource has been deleted
Expand Down
2 changes: 1 addition & 1 deletion internal/controllers/cluster/cluster_controller_phases.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ func (r *Reconciler) reconcileExternal(ctx context.Context, cluster *clusterv1.C
return external.ReconcileOutput{}, err
}

obj, err := external.Get(ctx, r.UnstructuredCachingClient, ref, cluster.Namespace)
obj, err := external.Get(ctx, r.Client, ref, cluster.Namespace)
if err != nil {
if apierrors.IsNotFound(errors.Cause(err)) {
log.Info("Could not find external object for cluster, requeuing", "refGroupVersionKind", ref.GroupVersionKind(), "refName", ref.Name)
Expand Down
20 changes: 8 additions & 12 deletions internal/controllers/cluster/cluster_controller_phases_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,9 +137,8 @@ func TestClusterReconcilePhases(t *testing.T) {
Build()
}
r := &Reconciler{
Client: c,
UnstructuredCachingClient: c,
recorder: record.NewFakeRecorder(32),
Client: c,
recorder: record.NewFakeRecorder(32),
}

res, err := r.reconcileInfrastructure(ctx, tt.cluster)
Expand Down Expand Up @@ -218,9 +217,8 @@ func TestClusterReconcilePhases(t *testing.T) {
Build()
}
r := &Reconciler{
Client: c,
UnstructuredCachingClient: c,
recorder: record.NewFakeRecorder(32),
Client: c,
recorder: record.NewFakeRecorder(32),
}
res, err := r.reconcileKubeconfig(ctx, tt.cluster)
if tt.wantErr {
Expand Down Expand Up @@ -370,9 +368,8 @@ func TestClusterReconciler_reconcilePhase(t *testing.T) {
Build()

r := &Reconciler{
Client: c,
UnstructuredCachingClient: c,
recorder: record.NewFakeRecorder(32),
Client: c,
recorder: record.NewFakeRecorder(32),
}
r.reconcilePhase(ctx, tt.cluster)
g.Expect(tt.cluster.Status.GetTypedPhase()).To(Equal(tt.wantPhase))
Expand Down Expand Up @@ -488,9 +485,8 @@ func TestClusterReconcilePhases_reconcileFailureDomains(t *testing.T) {

c := fake.NewClientBuilder().WithObjects(objs...).Build()
r := &Reconciler{
Client: c,
UnstructuredCachingClient: c,
recorder: record.NewFakeRecorder(32),
Client: c,
recorder: record.NewFakeRecorder(32),
}

_, err := r.reconcileInfrastructure(ctx, tt.cluster)
Expand Down
8 changes: 3 additions & 5 deletions internal/controllers/cluster/cluster_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -389,9 +389,8 @@ func TestClusterReconciler_reconcileDelete(t *testing.T) {
g := NewWithT(t)
fakeClient := fake.NewClientBuilder().WithObjects(fakeInfraCluster, tt.cluster).Build()
r := &Reconciler{
Client: fakeClient,
UnstructuredCachingClient: fakeClient,
APIReader: fakeClient,
Client: fakeClient,
APIReader: fakeClient,
}

_, _ = r.reconcileDelete(ctx, tt.cluster)
Expand Down Expand Up @@ -527,8 +526,7 @@ func TestClusterReconcilerNodeRef(t *testing.T) {

c := fake.NewClientBuilder().WithObjects(cluster, controlPlaneWithNoderef, controlPlaneWithoutNoderef, nonControlPlaneWithNoderef, nonControlPlaneWithoutNoderef).Build()
r := &Reconciler{
Client: c,
UnstructuredCachingClient: c,
Client: c,
}
requests := r.controlPlaneMachineToCluster(ctx, tt.o)
g.Expect(requests).To(BeComparableTo(tt.want))
Expand Down
24 changes: 5 additions & 19 deletions internal/controllers/cluster/suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ import (
"k8s.io/apimachinery/pkg/runtime"
clientgoscheme "k8s.io/client-go/kubernetes/scheme"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/controller"

clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
Expand Down Expand Up @@ -81,29 +80,16 @@ func TestMain(m *testing.M) {
panic(fmt.Sprintf("Failed to start ClusterCacheReconciler: %v", err))
}

unstructuredCachingClient, err := client.New(mgr.GetConfig(), client.Options{
HTTPClient: mgr.GetHTTPClient(),
Cache: &client.CacheOptions{
Reader: mgr.GetCache(),
Unstructured: true,
},
})
if err != nil {
panic(fmt.Sprintf("unable to create unstructuredCachineClient: %v", err))
}

if err := (&Reconciler{
Client: mgr.GetClient(),
UnstructuredCachingClient: unstructuredCachingClient,
APIReader: mgr.GetClient(),
Client: mgr.GetClient(),
APIReader: mgr.GetClient(),
}).SetupWithManager(ctx, mgr, controller.Options{MaxConcurrentReconciles: 1}); err != nil {
panic(fmt.Sprintf("Failed to start ClusterReconciler: %v", err))
}
if err := (&machinecontroller.Reconciler{
Client: mgr.GetClient(),
UnstructuredCachingClient: unstructuredCachingClient,
APIReader: mgr.GetAPIReader(),
Tracker: tracker,
Client: mgr.GetClient(),
APIReader: mgr.GetAPIReader(),
Tracker: tracker,
}).SetupWithManager(ctx, mgr, controller.Options{MaxConcurrentReconciles: 1}); err != nil {
panic(fmt.Sprintf("Failed to start MachineReconciler: %v", err))
}
Expand Down
6 changes: 1 addition & 5 deletions internal/controllers/clusterclass/clusterclass_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,6 @@ type Reconciler struct {

// RuntimeClient is a client for calling runtime extensions.
RuntimeClient runtimeclient.Client

// UnstructuredCachingClient provides a client that forces caching of unstructured objects,
// thus allowing to optimize reads for templates or provider specific objects.
UnstructuredCachingClient client.Client
}

func (r *Reconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, options controller.Options) error {
Expand Down Expand Up @@ -358,7 +354,7 @@ func refString(ref *corev1.ObjectReference) string {
func (r *Reconciler) reconcileExternal(ctx context.Context, clusterClass *clusterv1.ClusterClass, ref *corev1.ObjectReference) error {
log := ctrl.LoggerFrom(ctx)

obj, err := external.Get(ctx, r.UnstructuredCachingClient, ref, clusterClass.Namespace)
obj, err := external.Get(ctx, r.Client, ref, clusterClass.Namespace)
if err != nil {
if apierrors.IsNotFound(errors.Cause(err)) {
return errors.Wrapf(err, "Could not find external object for the ClusterClass. refGroupVersionKind: %s, refName: %s", ref.GroupVersionKind(), ref.Name)
Expand Down
Loading
Loading