Skip to content

Commit 6c6813f

Browse files
theobarberbanytheobarberbany
committed
Updates InfraMachine watch_filters
1 parent 2a0af92 commit 6c6813f

File tree

4 files changed

+188
-81
lines changed

4 files changed

+188
-81
lines changed

‎pkg/controllers/machinesync/machine_sync_controller.go‎

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,8 @@ const (
8181
mapiNamespace string = "openshift-machine-api"
8282
capiInfraCommonFinalizerSuffix string = ".cluster.x-k8s.io"
8383

84+
requeueInfraProgress = 1 * time.Second
85+
8486
messageSuccessfullySynchronizedCAPItoMAPI = "Successfully synchronized CAPI Machine to MAPI"
8587
messageSuccessfullySynchronizedMAPItoCAPI = "Successfully synchronized MAPI Machine to CAPI"
8688
progressingToSynchronizeMAPItoCAPI = "Progressing to synchronize MAPI Machine to CAPI"
@@ -134,7 +136,7 @@ type MachineSyncReconciler struct {
134136
MAPINamespace string
135137
}
136138

137-
// SetupWithManager sets the CoreClusterReconciler controller up with the given manager.
139+
// SetupWithManager sets the MachineSyncReconciler controller up with the given manager.
138140
func (r *MachineSyncReconciler) SetupWithManager(mgr ctrl.Manager) error {
139141
infraMachine, _, err := controllers.InitInfraMachineAndInfraClusterFromProvider(r.Platform)
140142
if err != nil {
@@ -161,7 +163,7 @@ func (r *MachineSyncReconciler) SetupWithManager(mgr ctrl.Manager) error {
161163
).
162164
Watches(
163165
infraMachine,
164-
handler.EnqueueRequestsFromMapFunc(util.RewriteNamespace(r.MAPINamespace)),
166+
handler.EnqueueRequestsFromMapFunc(util.ResolveCAPIMachineFromInfraMachine(r.MAPINamespace)),
165167
builder.WithPredicates(util.FilterNamespace(r.CAPINamespace)),
166168
).
167169
Complete(r); err != nil {
@@ -182,8 +184,8 @@ func (r *MachineSyncReconciler) SetupWithManager(mgr ctrl.Manager) error {
182184
func (r *MachineSyncReconciler) Reconcile(ctx context.Context, req reconcile.Request) (ctrl.Result, error) {
183185
logger := log.FromContext(ctx, "namespace", req.Namespace, "name", req.Name)
184186

185-
logger.V(1).Info("Reconciling machine")
186-
defer logger.V(1).Info("Finished reconciling machine")
187+
logger.Info("Reconciling machine")
188+
defer logger.Info("Finished reconciling machine")
187189

188190
var mapiMachineNotFound, capiMachineNotFound bool
189191

@@ -540,13 +542,13 @@ func (r *MachineSyncReconciler) reconcileMAPIMachinetoCAPIMachine(ctx context.Co
540542
BlockOwnerDeletion: ptr.To(true),
541543
}})
542544

543-
result, syncronizationIsProgressing, err := r.createOrUpdateCAPIInfraMachine(ctx, mapiMachine, infraMachine, newCAPIInfraMachine)
545+
result, synchronizationIsProgressing, err := r.createOrUpdateCAPIInfraMachine(ctx, mapiMachine, infraMachine, newCAPIInfraMachine)
544546
if err != nil {
545547
return result, fmt.Errorf("unable to ensure Cluster API infra machine: %w", err)
546548
}
547549

548-
if syncronizationIsProgressing {
549-
return ctrl.Result{RequeueAfter: time.Second * 1}, r.applySynchronizedConditionWithPatch(ctx, mapiMachine, corev1.ConditionUnknown,
550+
if synchronizationIsProgressing {
551+
return ctrl.Result{RequeueAfter: requeueInfraProgress}, r.applySynchronizedConditionWithPatch(ctx, mapiMachine, corev1.ConditionUnknown,
550552
reasonProgressingToCreateCAPIInfraMachine, progressingToSynchronizeMAPItoCAPI, nil)
551553
}
552554

@@ -1011,7 +1013,9 @@ func (r *MachineSyncReconciler) fetchCAPIInfraResources(ctx context.Context, cap
10111013
}
10121014

10131015
//nolint:funlen,gocognit,cyclop
1014-
func (r *MachineSyncReconciler) reconcileMAPItoCAPIMachineDeletion(ctx context.Context, mapiMachine *machinev1beta1.Machine, capiMachine *clusterv1.Machine, infraMachine client.Object) (bool, error) {
1016+
func (r *MachineSyncReconciler) reconcileMAPItoCAPIMachineDeletion(ctx context.Context, mapiMachine *machinev1beta1.Machine, capiMachine *clusterv1.Machine, infraMachine client.Object) (shouldReqeue bool, err error) {
1017+
logger := log.FromContext(ctx)
1018+
10151019
if mapiMachine.DeletionTimestamp.IsZero() {
10161020
if capiMachine == nil || capiMachine.DeletionTimestamp.IsZero() {
10171021
// Neither MAPI authoritative machine nor its CAPI non-authoritative machine mirror
@@ -1029,8 +1033,6 @@ func (r *MachineSyncReconciler) reconcileMAPItoCAPIMachineDeletion(ctx context.C
10291033
return true, nil
10301034
}
10311035

1032-
logger := log.FromContext(ctx)
1033-
10341036
if capiMachine == nil && util.IsNilObject(infraMachine) {
10351037
logger.Info("Cluster API machine and infra machine do not exist, removing corresponding Machine API machine sync finalizer")
10361038
// We don't have a capi machine or infra resouorces to clean up we can
@@ -1228,9 +1230,7 @@ func (r *MachineSyncReconciler) reconcileCAPItoMAPIMachineDeletion(ctx context.C
12281230

12291231
// ensureSyncFinalizer ensures the sync finalizer is present across the mapi
12301232
// machine, capi machine and capi infra machine.
1231-
func (r *MachineSyncReconciler) ensureSyncFinalizer(ctx context.Context, mapiMachine *machinev1beta1.Machine, capiMachine *clusterv1.Machine, infraMachine client.Object) (bool, error) {
1232-
var shouldRequeue bool
1233-
1233+
func (r *MachineSyncReconciler) ensureSyncFinalizer(ctx context.Context, mapiMachine *machinev1beta1.Machine, capiMachine *clusterv1.Machine, infraMachine client.Object) (shouldRequeue bool, err error) {
12341234
var errors []error
12351235

12361236
if mapiMachine != nil {

‎pkg/controllers/machinesync/machine_sync_controller_test.go‎

Lines changed: 125 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -191,6 +191,7 @@ var _ = Describe("With a running MachineSync Reconciler", func() {
191191

192192
mgr, err = ctrl.NewManager(controllerCfg, ctrl.Options{
193193
Scheme: testScheme,
194+
Logger: GinkgoLogr,
194195
Controller: config.Controller{
195196
SkipNameValidation: ptr.To(true),
196197
},
@@ -235,11 +236,6 @@ var _ = Describe("With a running MachineSync Reconciler", func() {
235236
})
236237

237238
Context("when all the CAPI infra resources exist", func() {
238-
BeforeEach(func() {
239-
By("Creating the CAPI infra machine")
240-
Expect(k8sClient.Create(ctx, capaMachine)).To(Succeed(), "capa machine should be able to be created")
241-
})
242-
243239
Context("when the MAPI machine has MachineAuthority set to Machine API", func() {
244240
BeforeEach(func() {
245241
By("Creating the MAPI machine")
@@ -272,10 +268,27 @@ var _ = Describe("With a running MachineSync Reconciler", func() {
272268
})
273269
})
274270

275-
Context("when the CAPI machine does exist", func() {
271+
Context("when the CAPI machine and infra machine do exist", func() {
276272
BeforeEach(func() {
277273
capiMachine = capiMachineBuilder.Build()
278274
Expect(k8sClient.Create(ctx, capiMachine)).Should(Succeed())
275+
276+
By("Creating the CAPI infra machine")
277+
// we must set the capi machine as an owner of the capa machine
278+
// in order to ensure we reconcile capa changes in our sync controller.
279+
capaMachine = capaMachineBuilder.WithOwnerReferences([]metav1.OwnerReference{
280+
{
281+
Kind: machineKind,
282+
APIVersion: clusterv1.GroupVersion.String(),
283+
Name: capiMachine.Name,
284+
UID: capiMachine.UID,
285+
BlockOwnerDeletion: ptr.To(true),
286+
Controller: ptr.To(false),
287+
},
288+
}).Build()
289+
290+
Expect(k8sClient.Create(ctx, capaMachine)).To(Succeed(), "capa machine should be able to be created")
291+
279292
})
280293

281294
It("should update the synchronized condition on the MAPI machine to True", func() {
@@ -289,41 +302,41 @@ var _ = Describe("With a running MachineSync Reconciler", func() {
289302
))),
290303
)
291304
})
292-
})
293305

294-
Context("when the MAPI machine providerSpec gets updated", func() {
295-
BeforeEach(func() {
296-
By("Updating the MAPI machine providerSpec")
297-
modifiedMAPIMachineBuilder := machinev1resourcebuilder.Machine().
298-
WithNamespace(mapiNamespace.GetName()).
299-
WithName(mapiMachine.Name).
300-
WithProviderSpecBuilder(machinev1resourcebuilder.AWSProviderSpec().WithLoadBalancers(nil).WithInstanceType("m6i.2xlarge")).Build()
306+
Context("when the MAPI machine providerSpec gets updated", func() {
307+
BeforeEach(func() {
308+
By("Updating the MAPI machine providerSpec")
309+
modifiedMAPIMachineBuilder := machinev1resourcebuilder.Machine().
310+
WithNamespace(mapiNamespace.GetName()).
311+
WithName(mapiMachine.Name).
312+
WithProviderSpecBuilder(machinev1resourcebuilder.AWSProviderSpec().WithLoadBalancers(nil).WithInstanceType("m6i.2xlarge")).Build()
301313

302-
Eventually(k.Update(mapiMachine, func() {
303-
mapiMachine.Spec.ProviderSpec = modifiedMAPIMachineBuilder.Spec.ProviderSpec
304-
})).Should(Succeed())
305-
})
314+
Eventually(k.Update(mapiMachine, func() {
315+
mapiMachine.Spec.ProviderSpec = modifiedMAPIMachineBuilder.Spec.ProviderSpec
316+
})).Should(Succeed())
317+
})
306318

307-
It("should recreate the CAPI infra machine", func() {
308-
capaMachineBuilder = awsv1resourcebuilder.AWSMachine().
309-
WithNamespace(capiNamespace.GetName()).
310-
WithName(mapiMachine.Name)
319+
It("should recreate the CAPI infra machine", func() {
320+
capaMachineBuilder = awsv1resourcebuilder.AWSMachine().
321+
WithNamespace(capiNamespace.GetName()).
322+
WithName(mapiMachine.Name)
311323

312-
Eventually(k.Object(capaMachineBuilder.Build()), timeout).Should(
313-
HaveField("Spec.InstanceType", Equal("m6i.2xlarge")),
314-
)
315-
})
324+
Eventually(k.Object(capaMachineBuilder.Build()), timeout).Should(
325+
HaveField("Spec.InstanceType", Equal("m6i.2xlarge")),
326+
)
327+
})
316328

317-
It("should update the synchronized condition on the MAPI machine to True", func() {
318-
Eventually(k.Object(mapiMachine), timeout).Should(
319-
HaveField("Status.Conditions", ContainElement(
320-
SatisfyAll(
321-
HaveField("Type", Equal(consts.SynchronizedCondition)),
322-
HaveField("Status", Equal(corev1.ConditionTrue)),
323-
HaveField("Reason", Equal("ResourceSynchronized")),
324-
HaveField("Message", Equal("Successfully synchronized MAPI Machine to CAPI")),
325-
))),
326-
)
329+
It("should update the synchronized condition on the MAPI machine to True", func() {
330+
Eventually(k.Object(mapiMachine), timeout).Should(
331+
HaveField("Status.Conditions", ContainElement(
332+
SatisfyAll(
333+
HaveField("Type", Equal(consts.SynchronizedCondition)),
334+
HaveField("Status", Equal(corev1.ConditionTrue)),
335+
HaveField("Reason", Equal("ResourceSynchronized")),
336+
HaveField("Message", Equal("Successfully synchronized MAPI Machine to CAPI")),
337+
))),
338+
)
339+
})
327340
})
328341
})
329342
})
@@ -339,6 +352,26 @@ var _ = Describe("With a running MachineSync Reconciler", func() {
339352
capiMachine = capiMachineBuilder.WithName("test-machine").Build()
340353
Expect(k8sClient.Create(ctx, capiMachine)).Should(Succeed())
341354

355+
By("Creating the CAPI infra machine")
356+
// we must set the capi machine as an owner of the capa machine
357+
// in order to ensure we reconcile capa changes in our sync controller.
358+
359+
// Updates the capaMachineBuilder with the correct owner ref,
360+
// so when we use it later on, we don't need to repeat ourselves.
361+
capaMachineBuilder = capaMachineBuilder.WithOwnerReferences([]metav1.OwnerReference{
362+
{
363+
Kind: machineKind,
364+
APIVersion: clusterv1.GroupVersion.String(),
365+
Name: capiMachine.Name,
366+
UID: capiMachine.UID,
367+
BlockOwnerDeletion: ptr.To(true),
368+
Controller: ptr.To(false),
369+
},
370+
})
371+
372+
capaMachine = capaMachineBuilder.Build()
373+
Expect(k8sClient.Create(ctx, capaMachine)).To(Succeed(), "capa machine should be able to be created")
374+
342375
By("Setting the MAPI machine AuthoritativeAPI to Cluster API")
343376
Eventually(k.UpdateStatus(mapiMachine, func() {
344377
mapiMachine.Status.AuthoritativeAPI = machinev1beta1.MachineAuthorityClusterAPI
@@ -428,18 +461,39 @@ var _ = Describe("With a running MachineSync Reconciler", func() {
428461
Eventually(k.UpdateStatus(mapiMachine, func() {
429462
mapiMachine.Status.AuthoritativeAPI = machinev1beta1.MachineAuthorityMachineAPI
430463
})).Should(Succeed())
464+
465+
capiMachine = capiMachineBuilder.WithName(mapiMachine.Name).Build()
431466
})
432467

433468
It("should successfully create the CAPI machine", func() {
434-
Eventually(k.Get(
435-
clusterv1resourcebuilder.Machine().WithName(mapiMachine.Name).WithNamespace(capiNamespace.Name).Build(),
436-
)).Should(Succeed())
469+
Eventually(k.Get(capiMachine)).Should(Succeed())
437470
})
438471

439-
It("should successfully create the CAPA machine", func() {
440-
Eventually(k.Get(
441-
awsv1resourcebuilder.AWSMachine().WithName(mapiMachine.Name).WithNamespace(capiNamespace.Name).Build(),
442-
)).Should(Succeed())
472+
It("should successfully create the CAPA machine with correct owner references", func() {
473+
// Get the CAPI machine, so we know the UID in the api server
474+
Eventually(k.Get(capiMachine)).Should(Succeed())
475+
476+
capaMachine = capaMachineBuilder.WithOwnerReferences([]metav1.OwnerReference{
477+
{
478+
Kind: machineKind,
479+
APIVersion: clusterv1.GroupVersion.String(),
480+
Name: capiMachine.Name,
481+
UID: capiMachine.UID,
482+
BlockOwnerDeletion: ptr.To(true),
483+
Controller: ptr.To(false),
484+
},
485+
}).Build()
486+
487+
Eventually(k.Object(capaMachine), timeout).Should(
488+
HaveField("ObjectMeta.OwnerReferences", ContainElement(
489+
SatisfyAll(
490+
HaveField("Kind", Equal(machineKind)),
491+
HaveField("APIVersion", Equal(clusterv1.GroupVersion.String())),
492+
HaveField("Name", Equal(capiMachine.Name)),
493+
HaveField("UID", Equal(capiMachine.UID)),
494+
))),
495+
)
496+
443497
})
444498

445499
It("should update the synchronized condition on the MAPI machine to True", func() {
@@ -496,7 +550,7 @@ var _ = Describe("With a running MachineSync Reconciler", func() {
496550
Expect(k8sClient.Create(ctx, capiMachine)).Should(Succeed())
497551
Expect(k8sClient.Create(ctx, mapiMachine)).Should(Succeed())
498552

499-
By("Setting the status.authoritativeAPI to Migrating")
553+
By("Setting the status.authoritativeAPI to the empty string")
500554
Eventually(k.UpdateStatus(mapiMachine, func() {
501555
mapiMachine.Status.AuthoritativeAPI = ""
502556
})).Should(Succeed())
@@ -858,9 +912,6 @@ var _ = Describe("With a running MachineSync Reconciler", func() {
858912
}, timeout).Should(Succeed())
859913
}
860914

861-
By("Creating the CAPI infra machine")
862-
Eventually(k8sClient.Create(ctx, capaMachine), timeout).Should(Succeed(), "capa machine should be able to be created")
863-
864915
By("Creating the MAPI machine")
865916
mapiMachine = mapiMachineBuilder.WithName("test-machine").WithLabels(map[string]string{
866917
"machine.openshift.io/cluster-api-cluster": "ci-op-gs2k97d6-c9e33-2smph",
@@ -891,6 +942,21 @@ var _ = Describe("With a running MachineSync Reconciler", func() {
891942
Eventually(k.Get(capiMachine)).Should(Succeed())
892943
Eventually(k.Get(mapiMachine)).Should(Succeed())
893944

945+
By("Creating the CAPI infra machine")
946+
capaMachine = capaMachineBuilder.WithOwnerReferences([]metav1.OwnerReference{
947+
{
948+
Kind: machineKind,
949+
APIVersion: clusterv1.GroupVersion.String(),
950+
Name: capiMachine.Name,
951+
UID: capiMachine.UID,
952+
BlockOwnerDeletion: ptr.To(true),
953+
Controller: ptr.To(false),
954+
},
955+
}).Build()
956+
957+
capaMachine = capaMachineBuilder.Build()
958+
Eventually(k8sClient.Create(ctx, capaMachine), timeout).Should(Succeed(), "capa machine should be able to be created")
959+
894960
})
895961

896962
AfterEach(func() {
@@ -946,21 +1012,22 @@ var _ = Describe("With a running MachineSync Reconciler", func() {
9461012
)
9471013

9481014
By("Creating a throwaway MAPI machine")
949-
testMachine := mapiMachineBuilder.WithGenerateName("test-machine").Build()
950-
Eventually(k8sClient.Create(ctx, testMachine), timeout).Should(Succeed())
1015+
sentinelMachine := mapiMachineBuilder.WithGenerateName("sentinel-machine").Build()
1016+
Eventually(k8sClient.Create(ctx, sentinelMachine), timeout).Should(Succeed())
9511017

9521018
By("Setting the throwaway MAPI machine AuthoritativeAPI to Cluster API")
953-
Eventually(k.UpdateStatus(testMachine, func() {
954-
testMachine.Status.AuthoritativeAPI = machinev1beta1.MachineAuthorityClusterAPI
1019+
Eventually(k.UpdateStatus(sentinelMachine, func() {
1020+
sentinelMachine.Status.AuthoritativeAPI = machinev1beta1.MachineAuthorityClusterAPI
9551021
})).Should(Succeed())
9561022

957-
Eventually(k.Object(testMachine), timeout).Should(
1023+
Eventually(k.Object(sentinelMachine), timeout).Should(
9581024
HaveField("Status.AuthoritativeAPI", Equal(machinev1beta1.MachineAuthorityClusterAPI)))
9591025

960-
Eventually(k.Update(testMachine, func() {
961-
testMachine.ObjectMeta.Labels = map[string]string{"test-sentinel": "fubar"}
1026+
Eventually(k.Update(sentinelMachine, func() {
1027+
sentinelMachine.ObjectMeta.Labels = map[string]string{"test-sentinel": "fubar"}
9621028
}), timeout).Should(MatchError(ContainSubstring("policy in place")))
9631029
})
1030+
9641031
Context("with status.AuthoritativeAPI: Machine API", func() {
9651032
BeforeEach(func() {
9661033
By("Setting the MAPI machine AuthoritativeAPI to Machine API")
@@ -1145,12 +1212,12 @@ var _ = Describe("With a running MachineSync Reconciler", func() {
11451212
),
11461213
)
11471214

1148-
checkVAPMachine := clusterv1resourcebuilder.Machine().WithName("vap-checking-machine").WithNamespace(capiNamespace.Name).Build()
1149-
Expect(k8sClient.Create(ctx, checkVAPMachine)).To(Succeed())
1215+
sentinelMachine := clusterv1resourcebuilder.Machine().WithName("sentinel-machine").WithNamespace(capiNamespace.Name).Build()
1216+
Expect(k8sClient.Create(ctx, sentinelMachine)).To(Succeed())
11501217

11511218
// Continually try to update the capiMachine to a forbidden field until the VAP blocks it
1152-
Eventually(k.Update(checkVAPMachine, func() {
1153-
checkVAPMachine.ObjectMeta.Labels = map[string]string{"test-sentinel": "fubar"}
1219+
Eventually(k.Update(sentinelMachine, func() {
1220+
sentinelMachine.ObjectMeta.Labels = map[string]string{"test-sentinel": "fubar"}
11541221
}), timeout).Should(MatchError(ContainSubstring("policy in place")))
11551222
})
11561223

‎pkg/controllers/machinesync/suite_test.go‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ import (
3333
"k8s.io/client-go/kubernetes/scheme"
3434
"k8s.io/client-go/rest"
3535
"k8s.io/klog/v2"
36-
"k8s.io/klog/v2/textlogger"
36+
ctrl "sigs.k8s.io/controller-runtime"
3737

3838
"sigs.k8s.io/controller-runtime/pkg/client"
3939
"sigs.k8s.io/controller-runtime/pkg/client/apiutil"
@@ -64,7 +64,8 @@ func TestAPIs(t *testing.T) {
6464
var _ = BeforeSuite(func() {
6565
klog.SetOutput(GinkgoWriter)
6666

67-
logf.SetLogger(textlogger.NewLogger(textlogger.NewConfig()))
67+
logf.SetLogger(GinkgoLogr)
68+
ctrl.SetLogger(GinkgoLogr)
6869

6970
By("bootstrapping test environment")
7071
var err error

0 commit comments

Comments
 (0)