Skip to content

Commit 5cbe352

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #1081 from shysank/refactor-1047/part2
Refactor controllers to decouple service instantiation
2 parents e8f327d + 5aa90f6 commit 5cbe352

12 files changed

+147
-109
lines changed

‎controllers/azurecluster_controller.go‎

Lines changed: 22 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -51,9 +51,26 @@ import (
5151
// AzureClusterReconciler reconciles a AzureCluster object
5252
type AzureClusterReconciler struct {
5353
client.Client
54-
Log logr.Logger
55-
Recorder record.EventRecorder
56-
ReconcileTimeout time.Duration
54+
Log logr.Logger
55+
Recorder record.EventRecorder
56+
ReconcileTimeout time.Duration
57+
createAzureClusterService azureClusterServiceCreator
58+
}
59+
60+
type azureClusterServiceCreator func(clusterScope *scope.ClusterScope) (*azureClusterService, error)
61+
62+
// NewAzureClusterReconciler returns a new AzureClusterReconciler instance
63+
func NewAzureClusterReconciler(client client.Client, log logr.Logger, recorder record.EventRecorder, reconcileTimeout time.Duration) *AzureClusterReconciler {
64+
acr := &AzureClusterReconciler{
65+
Client: client,
66+
Log: log,
67+
Recorder: recorder,
68+
ReconcileTimeout: reconcileTimeout,
69+
}
70+
71+
acr.createAzureClusterService = newAzureClusterService
72+
73+
return acr
5774
}
5875

5976
// SetupWithManager initializes this controller with a manager.
@@ -225,7 +242,7 @@ func (r *AzureClusterReconciler) reconcileNormal(ctx context.Context, clusterSco
225242
}
226243
}
227244

228-
acr, err := newAzureClusterReconciler(clusterScope)
245+
acr, err := r.createAzureClusterService(clusterScope)
229246
if err != nil {
230247
return reconcile.Result{}, errors.Wrap(err, "failed to create a new AzureClusterReconciler")
231248
}
@@ -261,7 +278,7 @@ func (r *AzureClusterReconciler) reconcileDelete(ctx context.Context, clusterSco
261278
return reconcile.Result{}, err
262279
}
263280

264-
acr, err := newAzureClusterReconciler(clusterScope)
281+
acr, err := r.createAzureClusterService(clusterScope)
265282
if err != nil {
266283
return reconcile.Result{}, errors.Wrap(err, "failed to create a new AzureClusterReconciler")
267284
}

‎controllers/azurecluster_controller_test.go‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -80,11 +80,7 @@ var _ = Describe("AzureClusterReconciler", func() {
8080

8181
c, err := client.New(testEnv.Config, client.Options{Scheme: testEnv.GetScheme()})
8282
Expect(err).ToNot(HaveOccurred())
83-
reconciler := &AzureClusterReconciler{
84-
Client: c,
85-
Log: log,
86-
ReconcileTimeout: 1 * time.Second,
87-
}
83+
reconciler := NewAzureClusterReconciler(c, log, testEnv.GetEventRecorderFor("azurecluster-reconciler"), 1*time.Second)
8884

8985
instance := &infrav1.AzureCluster{ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"}}
9086
_, err = reconciler.Reconcile(ctrl.Request{

‎controllers/azurecluster_reconciler.go‎

Lines changed: 36 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -37,8 +37,8 @@ import (
3737
"sigs.k8s.io/cluster-api-provider-azure/util/tele"
3838
)
3939

40-
// azureClusterReconciler is the reconciler called by the AzureCluster controller
41-
type azureClusterReconciler struct {
40+
// azureClusterService is the reconciler called by the AzureCluster controller
41+
type azureClusterService struct {
4242
scope *scope.ClusterScope
4343
groupsSvc azure.Service
4444
vnetSvc azure.Service
@@ -52,14 +52,14 @@ type azureClusterReconciler struct {
5252
skuCache *resourceskus.Cache
5353
}
5454

55-
// newAzureClusterReconciler populates all the services based on input scope
56-
func newAzureClusterReconciler(scope *scope.ClusterScope) (*azureClusterReconciler, error) {
55+
// newAzureClusterService populates all the services based on input scope
56+
func newAzureClusterService(scope *scope.ClusterScope) (*azureClusterService, error) {
5757
skuCache, err := resourceskus.GetCache(scope, scope.Location())
5858
if err != nil {
5959
return nil, errors.Wrap(err, "failed creating a NewCache")
6060
}
6161

62-
return &azureClusterReconciler{
62+
return &azureClusterService{
6363
scope: scope,
6464
groupsSvc: groups.New(scope),
6565
vnetSvc: virtualnetworks.New(scope),
@@ -74,93 +74,95 @@ func newAzureClusterReconciler(scope *scope.ClusterScope) (*azureClusterReconcil
7474
}, nil
7575
}
7676

77+
var _ azure.Service = (*azureClusterService)(nil)
78+
7779
// Reconcile reconciles all the services in pre determined order
78-
func (r *azureClusterReconciler) Reconcile(ctx context.Context) error {
79-
ctx, span := tele.Tracer().Start(ctx, "controllers.azureClusterReconciler.Reconcile")
80+
func (s *azureClusterService) Reconcile(ctx context.Context) error {
81+
ctx, span := tele.Tracer().Start(ctx, "controllers.azureClusterService.Reconcile")
8082
defer span.End()
8183

82-
if err := r.setFailureDomainsForLocation(ctx); err != nil {
84+
if err := s.setFailureDomainsForLocation(ctx); err != nil {
8385
return errors.Wrapf(err, "failed to get availability zones")
8486
}
8587

86-
r.scope.SetDNSName()
87-
r.scope.SetControlPlaneIngressRules()
88+
s.scope.SetDNSName()
89+
s.scope.SetControlPlaneIngressRules()
8890

89-
if err := r.groupsSvc.Reconcile(ctx); err != nil {
91+
if err := s.groupsSvc.Reconcile(ctx); err != nil {
9092
return errors.Wrapf(err, "failed to reconcile resource group")
9193
}
9294

93-
if err := r.vnetSvc.Reconcile(ctx); err != nil {
95+
if err := s.vnetSvc.Reconcile(ctx); err != nil {
9496
return errors.Wrapf(err, "failed to reconcile virtual network")
9597
}
9698

97-
if err := r.securityGroupSvc.Reconcile(ctx); err != nil {
99+
if err := s.securityGroupSvc.Reconcile(ctx); err != nil {
98100
return errors.Wrapf(err, "failed to reconcile network security group")
99101
}
100102

101-
if err := r.routeTableSvc.Reconcile(ctx); err != nil {
103+
if err := s.routeTableSvc.Reconcile(ctx); err != nil {
102104
return errors.Wrapf(err, "failed to reconcile route table")
103105
}
104106

105-
if err := r.subnetsSvc.Reconcile(ctx); err != nil {
107+
if err := s.subnetsSvc.Reconcile(ctx); err != nil {
106108
return errors.Wrapf(err, "failed to reconcile subnet")
107109
}
108110

109-
if err := r.publicIPSvc.Reconcile(ctx); err != nil {
111+
if err := s.publicIPSvc.Reconcile(ctx); err != nil {
110112
return errors.Wrapf(err, "failed to reconcile public IP")
111113
}
112114

113-
if err := r.loadBalancerSvc.Reconcile(ctx); err != nil {
115+
if err := s.loadBalancerSvc.Reconcile(ctx); err != nil {
114116
return errors.Wrapf(err, "failed to reconcile load balancer")
115117
}
116118

117-
if err := r.privateDNSSvc.Reconcile(ctx); err != nil {
119+
if err := s.privateDNSSvc.Reconcile(ctx); err != nil {
118120
return errors.Wrapf(err, "failed to reconcile private dns")
119121
}
120122

121-
if err := r.availabilitySetsSvc.Reconcile(ctx); err != nil {
123+
if err := s.availabilitySetsSvc.Reconcile(ctx); err != nil {
122124
return errors.Wrapf(err, "failed to reconcile availability sets")
123125
}
124126

125127
return nil
126128
}
127129

128130
// Delete reconciles all the services in pre determined order
129-
func (r *azureClusterReconciler) Delete(ctx context.Context) error {
130-
ctx, span := tele.Tracer().Start(ctx, "controllers.azureClusterReconciler.Delete")
131+
func (s *azureClusterService) Delete(ctx context.Context) error {
132+
ctx, span := tele.Tracer().Start(ctx, "controllers.azureClusterService.Delete")
131133
defer span.End()
132134

133-
if err := r.groupsSvc.Delete(ctx); err != nil {
135+
if err := s.groupsSvc.Delete(ctx); err != nil {
134136
if errors.Is(err, azure.ErrNotOwned) {
135-
if err := r.privateDNSSvc.Delete(ctx); err != nil {
137+
if err := s.privateDNSSvc.Delete(ctx); err != nil {
136138
return errors.Wrapf(err, "failed to delete private dns")
137139
}
138140

139-
if err := r.loadBalancerSvc.Delete(ctx); err != nil {
141+
if err := s.loadBalancerSvc.Delete(ctx); err != nil {
140142
return errors.Wrapf(err, "failed to delete load balancer")
141143
}
142144

143-
if err := r.publicIPSvc.Delete(ctx); err != nil {
145+
if err := s.publicIPSvc.Delete(ctx); err != nil {
144146
return errors.Wrapf(err, "failed to delete public IP")
145147
}
146148

147-
if err := r.subnetsSvc.Delete(ctx); err != nil {
149+
if err := s.subnetsSvc.Delete(ctx); err != nil {
148150
return errors.Wrapf(err, "failed to delete subnet")
149151
}
150152

151-
if err := r.routeTableSvc.Delete(ctx); err != nil {
153+
if err := s.routeTableSvc.Delete(ctx); err != nil {
152154
return errors.Wrapf(err, "failed to delete route table")
153155
}
154156

155-
if err := r.securityGroupSvc.Delete(ctx); err != nil {
157+
if err := s.securityGroupSvc.Delete(ctx); err != nil {
156158
return errors.Wrapf(err, "failed to delete network security group")
157159
}
158160

159-
if err := r.vnetSvc.Delete(ctx); err != nil {
161+
if err := s.vnetSvc.Delete(ctx); err != nil {
160162
return errors.Wrapf(err, "failed to delete virtual network")
161163
}
162164

163-
if err := r.availabilitySetsSvc.Delete(ctx); err != nil {
165+
if err := s.availabilitySetsSvc.Delete(ctx); err != nil {
164166
return errors.Wrapf(err, "failed to delete availability sets")
165167
}
166168
} else {
@@ -171,14 +173,14 @@ func (r *azureClusterReconciler) Delete(ctx context.Context) error {
171173
return nil
172174
}
173175

174-
func (r *azureClusterReconciler) setFailureDomainsForLocation(ctx context.Context) error {
175-
zones, err := r.skuCache.GetZones(ctx, r.scope.Location())
176+
func (s *azureClusterService) setFailureDomainsForLocation(ctx context.Context) error {
177+
zones, err := s.skuCache.GetZones(ctx, s.scope.Location())
176178
if err != nil {
177-
return errors.Wrapf(err, "failed to get zones for location %s", r.scope.Location())
179+
return errors.Wrapf(err, "failed to get zones for location %s", s.scope.Location())
178180
}
179181

180182
for _, zone := range zones {
181-
r.scope.SetFailureDomain(zone, clusterv1.FailureDomainSpec{
183+
s.scope.SetFailureDomain(zone, clusterv1.FailureDomainSpec{
182184
ControlPlane: true,
183185
})
184186
}

‎controllers/azurecluster_reconciler_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -131,7 +131,7 @@ func TestAzureClusterReconcilerDelete(t *testing.T) {
131131

132132
tc.expect(groupsMock.EXPECT(), vnetMock.EXPECT(), sgMock.EXPECT(), rtMock.EXPECT(), subnetsMock.EXPECT(), publicIPMock.EXPECT(), lbMock.EXPECT(), dnsMock.EXPECT(), asMock.EXPECT())
133133

134-
r := &azureClusterReconciler{
134+
s := &azureClusterService{
135135
scope: &scope.ClusterScope{
136136
AzureCluster: &infrav1.AzureCluster{},
137137
},
@@ -147,7 +147,7 @@ func TestAzureClusterReconcilerDelete(t *testing.T) {
147147
availabilitySetsSvc: asMock,
148148
}
149149

150-
err := r.Delete(context.TODO())
150+
err := s.Delete(context.TODO())
151151
if tc.expectedError != "" {
152152
g.Expect(err).To(HaveOccurred())
153153
g.Expect(err).To(MatchError(tc.expectedError))

‎controllers/suite_test.go‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -50,11 +50,8 @@ var _ = BeforeSuite(func(done Done) {
5050
By("bootstrapping test environment")
5151
testEnv = env.NewTestEnvironment()
5252

53-
Expect((&AzureClusterReconciler{
54-
Client: testEnv,
55-
Log: testEnv.Log,
56-
Recorder: testEnv.GetEventRecorderFor("azurecluster-reconciler"),
57-
}).SetupWithManager(testEnv.Manager, controller.Options{MaxConcurrentReconciles: 1})).To(Succeed())
53+
Expect(NewAzureClusterReconciler(testEnv, testEnv.Log, testEnv.GetEventRecorderFor("azurecluster-reconciler"), reconciler.DefaultLoopTimeout).
54+
SetupWithManager(testEnv.Manager, controller.Options{MaxConcurrentReconciles: 1})).To(Succeed())
5855

5956
Expect(NewAzureMachineReconciler(testEnv, testEnv.Log, testEnv.GetEventRecorderFor("azuremachine-reconciler"), reconciler.DefaultLoopTimeout).
6057
SetupWithManager(testEnv.Manager, controller.Options{MaxConcurrentReconciles: 1})).To(Succeed())

‎exp/controllers/azuremachinepool_controller.go‎

Lines changed: 23 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -55,10 +55,11 @@ type (
5555
// AzureMachinePoolReconciler reconciles a AzureMachinePool object
5656
AzureMachinePoolReconciler struct {
5757
client.Client
58-
Log logr.Logger
59-
Scheme *runtime.Scheme
60-
Recorder record.EventRecorder
61-
ReconcileTimeout time.Duration
58+
Log logr.Logger
59+
Scheme *runtime.Scheme
60+
Recorder record.EventRecorder
61+
ReconcileTimeout time.Duration
62+
createAzureMachinePoolService azureMachinePoolServiceCreator
6263
}
6364

6465
// annotationReaderWriter provides an interface to read and write annotations
@@ -68,6 +69,22 @@ type (
6869
}
6970
)
7071

72+
type azureMachinePoolServiceCreator func(machinePoolScope *scope.MachinePoolScope) (*azureMachinePoolService, error)
73+
74+
// NewAzureMachinePoolReconciler returns a new AzureMachinePoolReconciler instance
75+
func NewAzureMachinePoolReconciler(client client.Client, log logr.Logger, recorder record.EventRecorder, reconcileTimeout time.Duration) *AzureMachinePoolReconciler {
76+
ampr := &AzureMachinePoolReconciler{
77+
Client: client,
78+
Log: log,
79+
Recorder: recorder,
80+
ReconcileTimeout: reconcileTimeout,
81+
}
82+
83+
ampr.createAzureMachinePoolService = newAzureMachinePoolService
84+
85+
return ampr
86+
}
87+
7188
// SetupWithManager initializes this controller with a manager.
7289
func (r *AzureMachinePoolReconciler) SetupWithManager(mgr ctrl.Manager, options controller.Options) error {
7390
log := r.Log.WithValues("controller", "AzureMachinePool")
@@ -255,7 +272,7 @@ func (r *AzureMachinePoolReconciler) reconcileNormal(ctx context.Context, machin
255272
return reconcile.Result{}, nil
256273
}
257274

258-
ams, err := newAzureMachinePoolService(machinePoolScope)
275+
ams, err := r.createAzureMachinePoolService(machinePoolScope)
259276
if err != nil {
260277
return reconcile.Result{}, errors.Wrap(err, "failed creating a newAzureMachinePoolService")
261278
}
@@ -325,7 +342,7 @@ func (r *AzureMachinePoolReconciler) reconcileDelete(ctx context.Context, machin
325342
machinePoolScope.Info("Handling deleted AzureMachinePool")
326343

327344
if infracontroller.ShouldDeleteIndividualResources(ctx, clusterScope) {
328-
amps, err := newAzureMachinePoolService(machinePoolScope)
345+
amps, err := r.createAzureMachinePoolService(machinePoolScope)
329346
if err != nil {
330347
return reconcile.Result{}, errors.Wrap(err, "failed creating a new AzureMachinePoolService")
331348
}

‎exp/controllers/azuremachinepool_controller_test.go‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import (
2020
. "github.com/onsi/ginkgo"
2121
. "github.com/onsi/gomega"
2222
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
23+
"sigs.k8s.io/cluster-api-provider-azure/util/reconciler"
2324
ctrl "sigs.k8s.io/controller-runtime"
2425
"sigs.k8s.io/controller-runtime/pkg/client"
2526
"sigs.k8s.io/controller-runtime/pkg/log"
@@ -33,10 +34,8 @@ var _ = Describe("AzureMachinePoolReconciler", func() {
3334

3435
Context("Reconcile an AzureMachinePool", func() {
3536
It("should not error with minimal set up", func() {
36-
reconciler := &AzureMachinePoolReconciler{
37-
Client: testEnv,
38-
Log: log.Log,
39-
}
37+
reconciler := NewAzureMachinePoolReconciler(testEnv, log.Log, testEnv.GetEventRecorderFor("azuremachinepool-reconciler"),
38+
reconciler.DefaultLoopTimeout)
4039
By("Calling reconcile")
4140
instance := &infrav1exp.AzureMachinePool{ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"}}
4241
result, err := reconciler.Reconcile(ctrl.Request{

‎exp/controllers/azuremachinepool_reconciler.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,8 @@ type azureMachinePoolService struct {
3939
vmssExtensionSvc azure.Service
4040
}
4141

42+
var _ azure.Service = (*azureMachinePoolService)(nil)
43+
4244
// newAzureMachinePoolService populates all the services based on input scope.
4345
func newAzureMachinePoolService(machinePoolScope *scope.MachinePoolScope) (*azureMachinePoolService, error) {
4446
cache, err := resourceskus.GetCache(machinePoolScope, machinePoolScope.Location())

‎exp/controllers/azuremanagedmachinepool_controller.go‎

Lines changed: 22 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -50,9 +50,26 @@ import (
5050
// AzureManagedMachinePoolReconciler reconciles a AzureManagedMachinePool object
5151
type AzureManagedMachinePoolReconciler struct {
5252
client.Client
53-
Log logr.Logger
54-
Recorder record.EventRecorder
55-
ReconcileTimeout time.Duration
53+
Log logr.Logger
54+
Recorder record.EventRecorder
55+
ReconcileTimeout time.Duration
56+
createAzureManagedMachinePoolService azureManagedMachinePoolServiceCreator
57+
}
58+
59+
type azureManagedMachinePoolServiceCreator func(managedControlPlaneScope *scope.ManagedControlPlaneScope) *azureManagedMachinePoolService
60+
61+
// NewAzureManagedMachinePoolReconciler returns a new AzureManagedMachinePoolReconciler instance
62+
func NewAzureManagedMachinePoolReconciler(client client.Client, log logr.Logger, recorder record.EventRecorder, reconcileTimeout time.Duration) *AzureManagedMachinePoolReconciler {
63+
ampr := &AzureManagedMachinePoolReconciler{
64+
Client: client,
65+
Log: log,
66+
Recorder: recorder,
67+
ReconcileTimeout: reconcileTimeout,
68+
}
69+
70+
ampr.createAzureManagedMachinePoolService = newAzureManagedMachinePoolService
71+
72+
return ampr
5673
}
5774

5875
// SetupWithManager initializes this controller with a manager.
@@ -212,7 +229,7 @@ func (r *AzureManagedMachinePoolReconciler) reconcileNormal(ctx context.Context,
212229
return reconcile.Result{}, err
213230
}
214231

215-
if err := newAzureManagedMachinePoolReconciler(scope).Reconcile(ctx, scope); err != nil {
232+
if err := r.createAzureManagedMachinePoolService(scope).Reconcile(ctx, scope); err != nil {
216233
if IsAgentPoolVMSSNotFoundError(err) {
217234
// if the underlying VMSS is not yet created, requeue for 30s in the future
218235
return reconcile.Result{
@@ -234,7 +251,7 @@ func (r *AzureManagedMachinePoolReconciler) reconcileDelete(ctx context.Context,
234251

235252
scope.Logger.Info("Reconciling AzureManagedMachinePool delete")
236253

237-
if err := newAzureManagedMachinePoolReconciler(scope).Delete(ctx, scope); err != nil {
254+
if err := r.createAzureManagedMachinePoolService(scope).Delete(ctx, scope); err != nil {
238255
return reconcile.Result{}, errors.Wrapf(err, "error deleting AzureManagedMachinePool %s/%s", scope.InfraMachinePool.Namespace, scope.InfraMachinePool.Name)
239256
}
240257

0 commit comments

Comments
 (0)