Skip to content

Commit 8776a64

Browse files
metal3-io-botmetal3-io-bot
authored
Merge pull request #2007 from metal3-io-bot/cherry-pick-2000-to-release-1.8
[release-1.8] 🐛 Fix unterminating DataTemplate deletion
2 parents 1f2a669 + f725c94 commit 8776a64

6 files changed

+48
-42
lines changed

‎baremetal/metal3datatemplate_manager.go

+16-17
Original file line numberDiff line numberDiff line change
@@ -40,8 +40,8 @@ type DataTemplateManagerInterface interface {
4040
UnsetFinalizer()
4141
SetClusterOwnerRef(*clusterv1.Cluster) error
4242
// UpdateDatas handles the Metal3DataClaims and creates or deletes Metal3Data accordingly.
43-
// It returns the number of current allocations.
44-
UpdateDatas(context.Context) (int, error)
43+
// It returns if there are still Data object and undeleted DataClaims objects.
44+
UpdateDatas(context.Context) (bool, bool, error)
4545
}
4646

4747
// DataTemplateManager is responsible for performing machine reconciliation.
@@ -135,12 +135,7 @@ func (m *DataTemplateManager) getIndexes(ctx context.Context) (map[int]string, e
135135
continue
136136
}
137137

138-
// Get the claim Name, if unset use empty string, to still record the
139-
// index being used, to avoid conflicts
140-
claimName := ""
141-
if dataObject.Spec.Claim.Name != "" {
142-
claimName = dataObject.Spec.Claim.Name
143-
}
138+
claimName := dataObject.Spec.Claim.Name
144139
m.DataTemplate.Status.Indexes[claimName] = dataObject.Spec.Index
145140
indexes[dataObject.Spec.Index] = claimName
146141
}
@@ -170,12 +165,13 @@ func (m *DataTemplateManager) updateStatusTimestamp() {
170165
}
171166

172167
// UpdateDatas handles the Metal3DataClaims and creates or deletes Metal3Data accordingly.
173-
// It returns the number of current allocations.
174-
func (m *DataTemplateManager) UpdateDatas(ctx context.Context) (int, error) {
168+
// It returns if there are still Data object and undeleted DataClaims objects.
169+
func (m *DataTemplateManager) UpdateDatas(ctx context.Context) (bool, bool, error) {
175170
indexes, err := m.getIndexes(ctx)
176171
if err != nil {
177-
return 0, err
172+
return false, false, err
178173
}
174+
hasData := len(indexes) > 0
179175

180176
// get list of Metal3DataClaim objects
181177
dataClaimObjects := infrav1.Metal3DataClaimList{}
@@ -186,28 +182,31 @@ func (m *DataTemplateManager) UpdateDatas(ctx context.Context) (int, error) {
186182

187183
err = m.client.List(ctx, &dataClaimObjects, opts)
188184
if err != nil {
189-
return 0, err
185+
return false, false, err
190186
}
191187

188+
hasClaims := false
192189
// Iterate over the Metal3Data objects to find all indexes and objects
193190
for _, dataClaim := range dataClaimObjects.Items {
194191
dataClaim := dataClaim
195192
// If DataTemplate does not point to this object, discard
196193
if dataClaim.Spec.Template.Name != m.DataTemplate.Name {
197194
continue
198195
}
199-
200-
if dataClaim.Status.RenderedData != nil && dataClaim.DeletionTimestamp.IsZero() {
201-
continue
196+
if dataClaim.DeletionTimestamp.IsZero() {
197+
hasClaims = true
198+
if dataClaim.Status.RenderedData != nil {
199+
continue
200+
}
202201
}
203202

204203
indexes, err = m.updateData(ctx, &dataClaim, indexes)
205204
if err != nil {
206-
return 0, err
205+
return false, false, err
207206
}
208207
}
209208
m.updateStatusTimestamp()
210-
return len(indexes), nil
209+
return hasData, hasClaims, nil
211210
}
212211

213212
func (m *DataTemplateManager) updateData(ctx context.Context,

‎baremetal/metal3datatemplate_manager_test.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -256,7 +256,7 @@ var _ = Describe("Metal3DataTemplate manager", func() {
256256
)
257257
Expect(err).NotTo(HaveOccurred())
258258

259-
nbIndexes, err := templateMgr.UpdateDatas(context.TODO())
259+
hasData, _, err := templateMgr.UpdateDatas(context.TODO())
260260
if tc.expectRequeue || tc.expectError {
261261
Expect(err).To(HaveOccurred())
262262
if tc.expectRequeue {
@@ -267,7 +267,7 @@ var _ = Describe("Metal3DataTemplate manager", func() {
267267
} else {
268268
Expect(err).NotTo(HaveOccurred())
269269
}
270-
Expect(nbIndexes).To(Equal(tc.expectedNbIndexes))
270+
Expect(hasData).To(Equal(tc.expectedNbIndexes > 0))
271271
Expect(tc.template.Status.LastUpdated.IsZero()).To(BeFalse())
272272
Expect(tc.template.Status.Indexes).To(Equal(tc.expectedIndexes))
273273

‎baremetal/mocks/zz_generated.metal3datatemplate_manager.go

+5-4
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎controllers/metal3data_controller.go

+7-4
Original file line numberDiff line numberDiff line change
@@ -113,11 +113,14 @@ func (r *Metal3DataReconciler) Reconcile(ctx context.Context, req ctrl.Request)
113113
if !metal3Data.ObjectMeta.DeletionTimestamp.IsZero() {
114114
// Check if the Metal3DataClaim is gone. We cannot clean up until it is.
115115
err := r.Client.Get(ctx, types.NamespacedName{Name: metal3Data.Spec.Claim.Name, Namespace: metal3Data.Spec.Claim.Namespace}, &infrav1.Metal3DataClaim{})
116-
if err != nil && apierrors.IsNotFound(err) {
117-
return r.reconcileDelete(ctx, metadataMgr)
116+
if err != nil {
117+
if apierrors.IsNotFound(err) {
118+
return r.reconcileDelete(ctx, metadataMgr)
119+
}
120+
return ctrl.Result{}, err
118121
}
119-
// We were unable to determine if the Metal3DataClaim is gone
120-
return ctrl.Result{}, err
122+
// Requeue until Metal3DataClaim is gone.
123+
return ctrl.Result{Requeue: true, RequeueAfter: requeueAfter}, nil
121124
}
122125

123126
// Handle non-deleted machines

‎controllers/metal3datatemplate_controller.go

+9-6
Original file line numberDiff line numberDiff line change
@@ -140,7 +140,7 @@ func (r *Metal3DataTemplateReconciler) reconcileNormal(ctx context.Context,
140140
// If the Metal3DataTemplate doesn't have finalizer, add it.
141141
dataTemplateMgr.SetFinalizer()
142142

143-
_, err := dataTemplateMgr.UpdateDatas(ctx)
143+
_, _, err := dataTemplateMgr.UpdateDatas(ctx)
144144
if err != nil {
145145
return checkReconcileError(err, "Failed to recreate the status")
146146
}
@@ -150,17 +150,20 @@ func (r *Metal3DataTemplateReconciler) reconcileNormal(ctx context.Context,
150150
func (r *Metal3DataTemplateReconciler) reconcileDelete(ctx context.Context,
151151
dataTemplateMgr baremetal.DataTemplateManagerInterface,
152152
) (ctrl.Result, error) {
153-
allocationsCount, err := dataTemplateMgr.UpdateDatas(ctx)
153+
hasData, hasClaims, err := dataTemplateMgr.UpdateDatas(ctx)
154154
if err != nil {
155155
return checkReconcileError(err, "Failed to recreate the status")
156156
}
157157

158-
if allocationsCount == 0 {
159-
// metal3datatemplate is marked for deletion and ready to be deleted,
160-
// so remove the finalizer.
161-
dataTemplateMgr.UnsetFinalizer()
158+
if hasClaims {
159+
return ctrl.Result{}, nil
160+
}
161+
if hasData {
162+
return ctrl.Result{Requeue: true, RequeueAfter: requeueAfter}, nil
162163
}
163164

165+
dataTemplateMgr.UnsetFinalizer()
166+
164167
return ctrl.Result{}, nil
165168
}
166169

‎controllers/metal3datatemplate_controller_test.go

+9-9
Original file line numberDiff line numberDiff line change
@@ -83,19 +83,19 @@ var _ = Describe("Metal3DataTemplate manager", func() {
8383
}
8484
}
8585
if tc.m3dt != nil && !tc.m3dt.DeletionTimestamp.IsZero() && tc.reconcileDeleteError {
86-
m.EXPECT().UpdateDatas(context.Background()).Return(0, errors.New(""))
86+
m.EXPECT().UpdateDatas(context.Background()).Return(false, false, errors.New(""))
8787
} else if tc.m3dt != nil && !tc.m3dt.DeletionTimestamp.IsZero() {
88-
m.EXPECT().UpdateDatas(context.Background()).Return(0, nil)
88+
m.EXPECT().UpdateDatas(context.Background()).Return(false, false, nil)
8989
m.EXPECT().UnsetFinalizer()
9090
}
9191

9292
if tc.m3dt != nil && tc.m3dt.DeletionTimestamp.IsZero() &&
9393
tc.reconcileNormal {
9494
m.EXPECT().SetFinalizer()
9595
if tc.reconcileNormalError {
96-
m.EXPECT().UpdateDatas(context.Background()).Return(0, errors.New(""))
96+
m.EXPECT().UpdateDatas(context.Background()).Return(false, false, errors.New(""))
9797
} else {
98-
m.EXPECT().UpdateDatas(context.Background()).Return(1, nil)
98+
m.EXPECT().UpdateDatas(context.Background()).Return(true, true, nil)
9999
}
100100
}
101101

@@ -232,9 +232,9 @@ var _ = Describe("Metal3DataTemplate manager", func() {
232232
m.EXPECT().SetFinalizer()
233233

234234
if !tc.UpdateError {
235-
m.EXPECT().UpdateDatas(context.TODO()).Return(1, nil)
235+
m.EXPECT().UpdateDatas(context.TODO()).Return(true, true, nil)
236236
} else {
237-
m.EXPECT().UpdateDatas(context.TODO()).Return(0, errors.New(""))
237+
m.EXPECT().UpdateDatas(context.TODO()).Return(false, false, errors.New(""))
238238
}
239239

240240
res, err := r.reconcileNormal(context.TODO(), m)
@@ -284,12 +284,12 @@ var _ = Describe("Metal3DataTemplate manager", func() {
284284
m := baremetal_mocks.NewMockDataTemplateManagerInterface(gomockCtrl)
285285

286286
if !tc.DeleteError && tc.DeleteReady {
287-
m.EXPECT().UpdateDatas(context.TODO()).Return(0, nil)
287+
m.EXPECT().UpdateDatas(context.TODO()).Return(false, false, nil)
288288
m.EXPECT().UnsetFinalizer()
289289
} else if !tc.DeleteError {
290-
m.EXPECT().UpdateDatas(context.TODO()).Return(1, nil)
290+
m.EXPECT().UpdateDatas(context.TODO()).Return(true, true, nil)
291291
} else {
292-
m.EXPECT().UpdateDatas(context.TODO()).Return(0, errors.New(""))
292+
m.EXPECT().UpdateDatas(context.TODO()).Return(false, false, errors.New(""))
293293
}
294294

295295
res, err := r.reconcileDelete(context.TODO(), m)

0 commit comments

Comments
 (0)