Skip to content

Commit 9e7afa7

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #11972 from sbueringer/pr-fix-pause-double-reconcile
🐛 Avoid redundant reconciles if only generation of Paused condition changed
2 parents 6558328 + 3e6bc6a commit 9e7afa7

File tree

23 files changed

+211
-143
lines changed

23 files changed

+211
-143
lines changed

‎bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller.go

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -207,7 +207,13 @@ func (r *KubeadmConfigReconciler) Reconcile(ctx context.Context, req ctrl.Reques
207207
return ctrl.Result{}, err
208208
}
209209

210-
if isPaused, conditionChanged, err := paused.EnsurePausedCondition(ctx, r.Client, cluster, config); err != nil || isPaused || conditionChanged {
210+
// Initialize the patch helper.
211+
patchHelper, err := patch.NewHelper(config, r.Client)
212+
if err != nil {
213+
return ctrl.Result{}, err
214+
}
215+
216+
if isPaused, requeue, err := paused.EnsurePausedCondition(ctx, r.Client, cluster, config); err != nil || isPaused || requeue {
211217
return ctrl.Result{}, err
212218
}
213219

@@ -218,12 +224,6 @@ func (r *KubeadmConfigReconciler) Reconcile(ctx context.Context, req ctrl.Reques
218224
Cluster: cluster,
219225
}
220226

221-
// Initialize the patch helper.
222-
patchHelper, err := patch.NewHelper(config, r.Client)
223-
if err != nil {
224-
return ctrl.Result{}, err
225-
}
226-
227227
// Attempt to Patch the KubeadmConfig object and status after each reconciliation if no error occurs.
228228
defer func() {
229229
// always update the readyCondition; the summary is represented using the "1 of x completed" notation.
@@ -261,6 +261,7 @@ func (r *KubeadmConfigReconciler) Reconcile(ctx context.Context, req ctrl.Reques
261261
bootstrapv1.CertificatesAvailableCondition,
262262
}},
263263
patch.WithOwnedV1Beta2Conditions{Conditions: []string{
264+
clusterv1.PausedV1Beta2Condition,
264265
bootstrapv1.KubeadmConfigReadyV1Beta2Condition,
265266
bootstrapv1.KubeadmConfigDataSecretAvailableV1Beta2Condition,
266267
bootstrapv1.KubeadmConfigCertificatesAvailableV1Beta2Condition,

‎controlplane/kubeadm/internal/controllers/controller.go

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -189,17 +189,17 @@ func (r *KubeadmControlPlaneReconciler) Reconcile(ctx context.Context, req ctrl.
189189
log = log.WithValues("Cluster", klog.KObj(cluster))
190190
ctx = ctrl.LoggerInto(ctx, log)
191191

192-
if isPaused, conditionChanged, err := paused.EnsurePausedCondition(ctx, r.Client, cluster, kcp); err != nil || isPaused || conditionChanged {
193-
return ctrl.Result{}, err
194-
}
195-
196192
// Initialize the patch helper.
197193
patchHelper, err := patch.NewHelper(kcp, r.Client)
198194
if err != nil {
199195
log.Error(err, "Failed to configure the patch helper")
200196
return ctrl.Result{Requeue: true}, nil
201197
}
202198

199+
if isPaused, requeue, err := paused.EnsurePausedCondition(ctx, r.Client, cluster, kcp); err != nil || isPaused || requeue {
200+
return ctrl.Result{}, err
201+
}
202+
203203
// Initialize the control plane scope; this includes also checking for orphan machines and
204204
// adopt them if necessary.
205205
controlPlane, adoptableMachineFound, err := r.initControlPlaneScope(ctx, cluster, kcp)
@@ -337,6 +337,7 @@ func patchKubeadmControlPlane(ctx context.Context, patchHelper *patch.Helper, kc
337337
controlplanev1.CertificatesAvailableCondition,
338338
}},
339339
patch.WithOwnedV1Beta2Conditions{Conditions: []string{
340+
clusterv1.PausedV1Beta2Condition,
340341
controlplanev1.KubeadmControlPlaneAvailableV1Beta2Condition,
341342
controlplanev1.KubeadmControlPlaneInitializedV1Beta2Condition,
342343
controlplanev1.KubeadmControlPlaneCertificatesAvailableV1Beta2Condition,

‎exp/internal/controllers/machinepool_controller.go

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -179,16 +179,16 @@ func (r *MachinePoolReconciler) Reconcile(ctx context.Context, req ctrl.Request)
179179
mp.Spec.ClusterName, mp.Name, mp.Namespace)
180180
}
181181

182-
if isPaused, conditionChanged, err := paused.EnsurePausedCondition(ctx, r.Client, cluster, mp); err != nil || isPaused || conditionChanged {
183-
return ctrl.Result{}, err
184-
}
185-
186182
// Initialize the patch helper.
187183
patchHelper, err := patch.NewHelper(mp, r.Client)
188184
if err != nil {
189185
return ctrl.Result{}, err
190186
}
191187

188+
if isPaused, requeue, err := paused.EnsurePausedCondition(ctx, r.Client, cluster, mp); err != nil || isPaused || requeue {
189+
return ctrl.Result{}, err
190+
}
191+
192192
defer func() {
193193
r.reconcilePhase(mp)
194194
// TODO(jpang): add support for metrics.
@@ -211,6 +211,9 @@ func (r *MachinePoolReconciler) Reconcile(ctx context.Context, req ctrl.Request)
211211
clusterv1.InfrastructureReadyCondition,
212212
expv1.ReplicasReadyCondition,
213213
}},
214+
patch.WithOwnedV1Beta2Conditions{Conditions: []string{
215+
clusterv1.PausedV1Beta2Condition,
216+
}},
214217
}
215218
if reterr == nil {
216219
patchOpts = append(patchOpts, patch.WithStatusObservedGeneration{})

‎exp/runtime/internal/controllers/extensionconfig_controller.go

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -132,7 +132,10 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
132132
return ctrl.Result{}, err
133133
}
134134

135-
if isPaused, conditionChanged, err := paused.EnsurePausedCondition(ctx, r.Client, nil, extensionConfig); err != nil || isPaused || conditionChanged {
135+
// Copy to avoid modifying the original extensionConfig.
136+
original := extensionConfig.DeepCopy()
137+
138+
if isPaused, requeue, err := paused.EnsurePausedCondition(ctx, r.Client, nil, extensionConfig); err != nil || isPaused || requeue {
136139
return ctrl.Result{}, err
137140
}
138141

@@ -141,9 +144,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
141144
return r.reconcileDelete(ctx, extensionConfig)
142145
}
143146

144-
// Copy to avoid modifying the original extensionConfig.
145-
original := extensionConfig.DeepCopy()
146-
147147
// Inject CABundle from secret if annotation is set. Otherwise https calls may fail.
148148
if err := reconcileCABundle(ctx, r.Client, extensionConfig); err != nil {
149149
return ctrl.Result{}, err
@@ -183,6 +183,7 @@ func patchExtensionConfig(ctx context.Context, client client.Client, original, m
183183
runtimev1.RuntimeExtensionDiscoveredCondition,
184184
}},
185185
patch.WithOwnedV1Beta2Conditions{Conditions: []string{
186+
clusterv1.PausedV1Beta2Condition,
186187
runtimev1.ExtensionConfigDiscoveredV1Beta2Condition,
187188
}},
188189
)

‎exp/runtime/internal/controllers/extensionconfig_controller_test.go

Lines changed: 1 addition & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -180,19 +180,7 @@ func TestExtensionReconciler_Reconcile(t *testing.T) {
180180
return nil
181181
}, 30*time.Second, 100*time.Millisecond).Should(Succeed())
182182

183-
// Reconcile the extension and assert discovery has succeeded (the first reconcile updates observedGeneration in the Paused condition).
184-
_, err = r.Reconcile(ctx, ctrl.Request{NamespacedName: util.ObjectKey(extensionConfig)})
185-
g.Expect(err).ToNot(HaveOccurred())
186-
187-
// Wait until the ExtensionConfig in the cache has the up-to-date Paused condition so the next Reconcile can do discovery.
188-
g.Eventually(func(g Gomega) {
189-
conf := &runtimev1.ExtensionConfig{}
190-
g.Expect(env.Get(ctx, util.ObjectKey(extensionConfig), conf)).To(Succeed())
191-
pausedCondition := v1beta2conditions.Get(conf, clusterv1.PausedV1Beta2Condition)
192-
g.Expect(pausedCondition).ToNot(BeNil())
193-
g.Expect(pausedCondition.ObservedGeneration).To(Equal(conf.Generation))
194-
}).WithTimeout(10 * time.Second).WithPolling(100 * time.Millisecond).Should(Succeed())
195-
183+
// Reconcile the extension and assert discovery has succeeded.
196184
_, err = r.Reconcile(ctx, ctrl.Request{NamespacedName: util.ObjectKey(extensionConfig)})
197185
g.Expect(err).ToNot(HaveOccurred())
198186

‎internal/controllers/cluster/cluster_controller.go

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -159,7 +159,13 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (retRes ct
159159
return ctrl.Result{}, err
160160
}
161161

162-
if isPaused, conditionChanged, err := paused.EnsurePausedCondition(ctx, r.Client, cluster, cluster); err != nil || isPaused || conditionChanged {
162+
// Initialize the patch helper.
163+
patchHelper, err := patch.NewHelper(cluster, r.Client)
164+
if err != nil {
165+
return ctrl.Result{}, err
166+
}
167+
168+
if isPaused, requeue, err := paused.EnsurePausedCondition(ctx, r.Client, cluster, cluster); err != nil || isPaused || requeue {
163169
return ctrl.Result{}, err
164170
}
165171

@@ -173,12 +179,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (retRes ct
173179
}
174180
}
175181

176-
// Initialize the patch helper.
177-
patchHelper, err := patch.NewHelper(cluster, r.Client)
178-
if err != nil {
179-
return ctrl.Result{}, err
180-
}
181-
182182
defer func() {
183183
// Always reconcile the Status.
184184
r.updateStatus(ctx, s)
@@ -272,6 +272,7 @@ func patchCluster(ctx context.Context, patchHelper *patch.Helper, cluster *clust
272272
clusterv1.InfrastructureReadyCondition,
273273
}},
274274
patch.WithOwnedV1Beta2Conditions{Conditions: []string{
275+
clusterv1.PausedV1Beta2Condition,
275276
clusterv1.ClusterInfrastructureReadyV1Beta2Condition,
276277
clusterv1.ClusterControlPlaneAvailableV1Beta2Condition,
277278
clusterv1.ClusterControlPlaneInitializedV1Beta2Condition,

‎internal/controllers/clusterclass/clusterclass_controller.go

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,12 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (retres ct
114114
return ctrl.Result{}, err
115115
}
116116

117-
if isPaused, conditionChanged, err := paused.EnsurePausedCondition(ctx, r.Client, nil, clusterClass); err != nil || isPaused || conditionChanged {
117+
patchHelper, err := patch.NewHelper(clusterClass, r.Client)
118+
if err != nil {
119+
return ctrl.Result{}, err
120+
}
121+
122+
if isPaused, requeue, err := paused.EnsurePausedCondition(ctx, r.Client, nil, clusterClass); err != nil || isPaused || requeue {
118123
return ctrl.Result{}, err
119124
}
120125

@@ -126,11 +131,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (retres ct
126131
clusterClass: clusterClass,
127132
}
128133

129-
patchHelper, err := patch.NewHelper(clusterClass, r.Client)
130-
if err != nil {
131-
return ctrl.Result{}, err
132-
}
133-
134134
defer func() {
135135
updateStatus(ctx, s)
136136

@@ -140,6 +140,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (retres ct
140140
clusterv1.ClusterClassVariablesReconciledCondition,
141141
}},
142142
patch.WithOwnedV1Beta2Conditions{Conditions: []string{
143+
clusterv1.PausedV1Beta2Condition,
143144
clusterv1.ClusterClassRefVersionsUpToDateV1Beta2Condition,
144145
clusterv1.ClusterClassVariablesReadyV1Beta2Condition,
145146
}},

‎internal/controllers/clusterresourceset/clusterresourceset_controller.go

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -142,20 +142,25 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
142142
return ctrl.Result{}, err
143143
}
144144

145-
if isPaused, conditionChanged, err := paused.EnsurePausedCondition(ctx, r.Client, nil, clusterResourceSet); err != nil || isPaused || conditionChanged {
146-
return ctrl.Result{}, err
147-
}
148-
149145
// Initialize the patch helper.
150146
patchHelper, err := patch.NewHelper(clusterResourceSet, r.Client)
151147
if err != nil {
152148
return ctrl.Result{}, err
153149
}
154150

151+
if isPaused, requeue, err := paused.EnsurePausedCondition(ctx, r.Client, nil, clusterResourceSet); err != nil || isPaused || requeue {
152+
return ctrl.Result{}, err
153+
}
154+
155155
defer func() {
156156
// Always attempt to patch the object and status after each reconciliation.
157157
// Patch ObservedGeneration only if the reconciliation completed successfully.
158-
patchOpts := []patch.Option{}
158+
patchOpts := []patch.Option{
159+
patch.WithOwnedV1Beta2Conditions{Conditions: []string{
160+
clusterv1.PausedV1Beta2Condition,
161+
addonsv1.ResourcesAppliedV1Beta2Condition,
162+
}},
163+
}
159164
if reterr == nil {
160165
patchOpts = append(patchOpts, patch.WithStatusObservedGeneration{})
161166
}

‎internal/controllers/machine/machine_controller.go

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -221,7 +221,13 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
221221
m.Spec.ClusterName, m.Name, m.Namespace)
222222
}
223223

224-
if isPaused, conditionChanged, err := paused.EnsurePausedCondition(ctx, r.Client, cluster, m); err != nil || isPaused || conditionChanged {
224+
// Initialize the patch helper
225+
patchHelper, err := patch.NewHelper(m, r.Client)
226+
if err != nil {
227+
return ctrl.Result{}, err
228+
}
229+
230+
if isPaused, requeue, err := paused.EnsurePausedCondition(ctx, r.Client, cluster, m); err != nil || isPaused || requeue {
225231
return ctrl.Result{}, err
226232
}
227233

@@ -240,12 +246,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
240246
machine: m,
241247
}
242248

243-
// Initialize the patch helper
244-
patchHelper, err := patch.NewHelper(m, r.Client)
245-
if err != nil {
246-
return ctrl.Result{}, err
247-
}
248-
249249
defer func() {
250250
r.updateStatus(ctx, s)
251251

@@ -315,6 +315,7 @@ func patchMachine(ctx context.Context, patchHelper *patch.Helper, machine *clust
315315
clusterv1.DrainingSucceededCondition,
316316
}},
317317
patch.WithOwnedV1Beta2Conditions{Conditions: []string{
318+
clusterv1.PausedV1Beta2Condition,
318319
clusterv1.MachineAvailableV1Beta2Condition,
319320
clusterv1.MachineReadyV1Beta2Condition,
320321
clusterv1.MachineBootstrapConfigReadyV1Beta2Condition,

‎internal/controllers/machinedeployment/machinedeployment_controller.go

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -146,16 +146,16 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (retres ct
146146
return ctrl.Result{}, err
147147
}
148148

149-
if isPaused, conditionChanged, err := paused.EnsurePausedCondition(ctx, r.Client, cluster, deployment); err != nil || isPaused || conditionChanged {
150-
return ctrl.Result{}, err
151-
}
152-
153149
// Initialize the patch helper
154150
patchHelper, err := patch.NewHelper(deployment, r.Client)
155151
if err != nil {
156152
return ctrl.Result{}, err
157153
}
158154

155+
if isPaused, requeue, err := paused.EnsurePausedCondition(ctx, r.Client, cluster, deployment); err != nil || isPaused || requeue {
156+
return ctrl.Result{}, err
157+
}
158+
159159
s := &scope{
160160
machineDeployment: deployment,
161161
cluster: cluster,
@@ -215,6 +215,7 @@ func patchMachineDeployment(ctx context.Context, patchHelper *patch.Helper, md *
215215
clusterv1.MachineDeploymentAvailableCondition,
216216
}},
217217
patch.WithOwnedV1Beta2Conditions{Conditions: []string{
218+
clusterv1.PausedV1Beta2Condition,
218219
clusterv1.MachineDeploymentAvailableV1Beta2Condition,
219220
clusterv1.MachineDeploymentMachinesReadyV1Beta2Condition,
220221
clusterv1.MachineDeploymentMachinesUpToDateV1Beta2Condition,

‎internal/controllers/machinehealthcheck/machinehealthcheck_controller.go

Lines changed: 14 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -132,8 +132,8 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
132132
log := ctrl.LoggerFrom(ctx)
133133

134134
// Fetch the MachineHealthCheck instance
135-
m := &clusterv1.MachineHealthCheck{}
136-
if err := r.Client.Get(ctx, req.NamespacedName, m); err != nil {
135+
mhc := &clusterv1.MachineHealthCheck{}
136+
if err := r.Client.Get(ctx, req.NamespacedName, mhc); err != nil {
137137
if apierrors.IsNotFound(err) {
138138
// Object not found, return. Created objects are automatically garbage collected.
139139
// For additional cleanup logic use finalizers.
@@ -144,22 +144,22 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
144144
return ctrl.Result{}, err
145145
}
146146

147-
log = log.WithValues("Cluster", klog.KRef(m.Namespace, m.Spec.ClusterName))
147+
log = log.WithValues("Cluster", klog.KRef(mhc.Namespace, mhc.Spec.ClusterName))
148148
ctx = ctrl.LoggerInto(ctx, log)
149149

150-
cluster, err := util.GetClusterByName(ctx, r.Client, m.Namespace, m.Spec.ClusterName)
150+
cluster, err := util.GetClusterByName(ctx, r.Client, mhc.Namespace, mhc.Spec.ClusterName)
151151
if err != nil {
152152
log.Error(err, "Failed to fetch Cluster for MachineHealthCheck")
153153
return ctrl.Result{}, err
154154
}
155155

156-
if isPaused, conditionChanged, err := paused.EnsurePausedCondition(ctx, r.Client, cluster, m); err != nil || isPaused || conditionChanged {
156+
// Initialize the patch helper
157+
patchHelper, err := patch.NewHelper(mhc, r.Client)
158+
if err != nil {
157159
return ctrl.Result{}, err
158160
}
159161

160-
// Initialize the patch helper
161-
patchHelper, err := patch.NewHelper(m, r.Client)
162-
if err != nil {
162+
if isPaused, requeue, err := paused.EnsurePausedCondition(ctx, r.Client, cluster, mhc); err != nil || isPaused || requeue {
163163
return ctrl.Result{}, err
164164
}
165165

@@ -171,24 +171,25 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
171171
clusterv1.RemediationAllowedCondition,
172172
}},
173173
patch.WithOwnedV1Beta2Conditions{Conditions: []string{
174+
clusterv1.PausedV1Beta2Condition,
174175
clusterv1.MachineHealthCheckRemediationAllowedV1Beta2Condition,
175176
}},
176177
}
177178
if reterr == nil {
178179
patchOpts = append(patchOpts, patch.WithStatusObservedGeneration{})
179180
}
180-
if err := patchHelper.Patch(ctx, m, patchOpts...); err != nil {
181+
if err := patchHelper.Patch(ctx, mhc, patchOpts...); err != nil {
181182
reterr = kerrors.NewAggregate([]error{reterr, err})
182183
}
183184
}()
184185

185186
// Reconcile labels.
186-
if m.Labels == nil {
187-
m.Labels = make(map[string]string)
187+
if mhc.Labels == nil {
188+
mhc.Labels = make(map[string]string)
188189
}
189-
m.Labels[clusterv1.ClusterNameLabel] = m.Spec.ClusterName
190+
mhc.Labels[clusterv1.ClusterNameLabel] = mhc.Spec.ClusterName
190191

191-
return r.reconcile(ctx, log, cluster, m)
192+
return r.reconcile(ctx, log, cluster, mhc)
192193
}
193194

194195
func (r *Reconciler) reconcile(ctx context.Context, logger logr.Logger, cluster *clusterv1.Cluster, m *clusterv1.MachineHealthCheck) (ctrl.Result, error) {

0 commit comments

Comments
 (0)