Skip to content

Commit 1d4ed7f

Browse files
DyannggDyanngg
committed
Fix leak in resourceimport_controller
The ResourceImport reconciler only adds/updates installed ResourceImports to the installedResImports cache, but never removes them when they are deleted from the K8s API and all corresponding resources are deleted successfully. This PR fixes the above issue. Signed-off-by: Dyanngg <dingyang@vmware.com>
1 parent e7486d5 commit 1d4ed7f

6 files changed

Lines changed: 82 additions & 46 deletions

File tree

‎multicluster/controllers/multicluster/commonarea/acnp_resourceimport_controller.go‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -120,10 +120,14 @@ func (r *ResourceImportReconciler) handleResImpDeleteForClusterNetworkPolicy(ctx
120120
Name: acnpName,
121121
},
122122
}
123-
if err := r.localClusterClient.Delete(ctx, acnp, &client.DeleteOptions{}); err != nil {
124-
return ctrl.Result{}, client.IgnoreNotFound(err)
123+
err := client.IgnoreNotFound(r.localClusterClient.Delete(ctx, acnp, &client.DeleteOptions{}))
124+
if err != nil {
125+
klog.ErrorS(err, "Failed to delete imported ACNP", "acnp", acnpName)
126+
return ctrl.Result{}, err
127+
} else {
128+
r.installedResImports.Delete(*resImp)
129+
return ctrl.Result{}, nil
125130
}
126-
return ctrl.Result{}, nil
127131
}
128132

129133
func getMCAntreaClusterPolicy(resImp *multiclusterv1alpha1.ResourceImport) *v1alpha1.ClusterNetworkPolicy {

‎multicluster/controllers/multicluster/commonarea/acnp_resourceimport_controller_test.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -200,6 +200,9 @@ func TestResourceImportReconciler_handleCopySpanACNPDeleteEvent(t *testing.T) {
200200
if err := fakeClient.Get(ctx, types.NamespacedName{Namespace: "", Name: common.AntreaMCSPrefix + acnpImportName}, acnp); !apierrors.IsNotFound(err) {
201201
t.Errorf("ResourceImport Reconciler should delete ACNP successfully but got error = %v", err)
202202
}
203+
if _, exists, _ := r.installedResImports.Get(*acnpResImport); exists {
204+
t.Errorf("Reconciler should delete ResImport from installedResImports after successful resource deletion")
205+
}
203206
}
204207

205208
func TestResourceImportReconciler_handleCopySpanACNPUpdateEvent(t *testing.T) {

‎multicluster/controllers/multicluster/commonarea/clusterinfo_importer.go‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -75,8 +75,14 @@ func (r *ResourceImportReconciler) handleResImpUpdateForClusterInfo(ctx context.
7575
func (r *ResourceImportReconciler) handleResImpDeleteForClusterInfo(ctx context.Context, req ctrl.Request, resImp *mcsv1alpha1.ResourceImport) (ctrl.Result, error) {
7676
clusterInfoImport, clusterInfoImportName := newClusterInfoImport(req.Name, r.namespace)
7777
klog.InfoS("Deleting ClusterInfoImport", "clusterinfoimport", clusterInfoImportName.String())
78-
err := r.localClusterClient.Delete(ctx, clusterInfoImport, &client.DeleteOptions{})
79-
return ctrl.Result{}, client.IgnoreNotFound(err)
78+
err := client.IgnoreNotFound(r.localClusterClient.Delete(ctx, clusterInfoImport, &client.DeleteOptions{}))
79+
if err != nil {
80+
klog.ErrorS(err, "Failed to delete imported ClusterInfo", "clusterInfo", clusterInfoImportName)
81+
return ctrl.Result{}, err
82+
} else {
83+
r.installedResImports.Delete(*resImp)
84+
return ctrl.Result{}, nil
85+
}
8086
}
8187

8288
func newClusterInfoImport(name, namespace string) (*mcsv1alpha1.ClusterInfoImport, types.NamespacedName) {

‎multicluster/controllers/multicluster/commonarea/clusterinfo_importer_test.go‎

Lines changed: 43 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -149,26 +149,30 @@ func TestResourceImportReconciler_handleClusterInfo(t *testing.T) {
149149
}
150150

151151
tests := []struct {
152-
name string
153-
existingCIResImport *mcsv1alpha1.ResourceImport
154-
existingCIImport *mcsv1alpha1.ClusterInfoImport
155-
expectedCIImport *mcsv1alpha1.ClusterInfoImport
156-
isDelete bool
152+
name string
153+
existingCIResImport *mcsv1alpha1.ResourceImport
154+
existingCIImport *mcsv1alpha1.ClusterInfoImport
155+
expectedCIImport *mcsv1alpha1.ClusterInfoImport
156+
isDelete bool
157+
expNumInstalledResImp int
157158
}{
158159
{
159-
name: "create ClusterInfoImport successfully",
160-
existingCIResImport: ciResImportA,
161-
expectedCIImport: &ciImportA,
160+
name: "create ClusterInfoImport successfully",
161+
existingCIResImport: ciResImportA,
162+
expectedCIImport: &ciImportA,
163+
expNumInstalledResImp: 1,
162164
},
163165
{
164-
name: "skip import empty ResourceImport",
165-
existingCIResImport: ciResImportEmptySpec,
166-
expectedCIImport: nil,
166+
name: "skip import empty ResourceImport",
167+
existingCIResImport: ciResImportEmptySpec,
168+
expectedCIImport: nil,
169+
expNumInstalledResImp: 1,
167170
},
168171
{
169-
name: "skip import ResourceImport from local",
170-
existingCIResImport: ciResImportLocal,
171-
expectedCIImport: nil,
172+
name: "skip import ResourceImport from local",
173+
existingCIResImport: ciResImportLocal,
174+
expectedCIImport: nil,
175+
expNumInstalledResImp: 1,
172176
},
173177
{
174178
name: "update ClusterInfoImport successfully",
@@ -181,12 +185,14 @@ func TestResourceImportReconciler_handleClusterInfo(t *testing.T) {
181185
},
182186
Spec: clusterBInfoNew,
183187
},
188+
expNumInstalledResImp: 1,
184189
},
185190
{
186-
name: "delete ClusterInfoImport successfully",
187-
existingCIResImport: ciResImportC,
188-
existingCIImport: &ciImportC,
189-
isDelete: true,
191+
name: "delete ClusterInfoImport successfully",
192+
existingCIResImport: ciResImportC,
193+
existingCIImport: &ciImportC,
194+
isDelete: true,
195+
expNumInstalledResImp: 0,
190196
},
191197
}
192198

@@ -202,8 +208,8 @@ func TestResourceImportReconciler_handleClusterInfo(t *testing.T) {
202208
}
203209
remoteCluster := NewFakeRemoteCommonArea(fakeRemoteClient, "leader-cluster", "cluster-d", "default")
204210
r := NewResourceImportReconciler(fakeClient, scheme, fakeClient, "cluster-d", "default", remoteCluster)
205-
if tt.isDelete {
206-
r.installedResImports.Add(*ciResImportC)
211+
if tt.existingCIResImport != nil {
212+
r.installedResImports.Add(*tt.existingCIResImport)
207213
}
208214
ciResImportName := types.NamespacedName{
209215
Namespace: tt.existingCIResImport.Namespace,
@@ -213,22 +219,24 @@ func TestResourceImportReconciler_handleClusterInfo(t *testing.T) {
213219

214220
if _, err := r.Reconcile(ctx, req); err != nil {
215221
t.Errorf("ClusterInfo Importer should handle ResourceImport events successfully but got error = %v", err)
216-
} else {
217-
gotCIImp := &mcsv1alpha1.ClusterInfoImport{}
218-
err := fakeClient.Get(ctx, ciResImportName, gotCIImp)
219-
isNotFound := apierrors.IsNotFound(err)
220-
if err != nil {
221-
if tt.expectedCIImport == nil && !isNotFound {
222-
t.Errorf("Expected to get not found error but got error = %v", err)
223-
}
224-
if tt.expectedCIImport != nil && isNotFound {
225-
t.Errorf("Expected to get ClusterInfoImport %v but got not found error = %v", tt.expectedCIImport, err)
226-
}
227-
} else if tt.expectedCIImport != nil {
228-
if !reflect.DeepEqual(tt.expectedCIImport.Spec, gotCIImp.Spec) {
229-
t.Errorf("Expected ClusterInfoImport %v but got %v", tt.expectedCIImport.Spec, gotCIImp.Spec)
230-
}
222+
}
223+
gotCIImp := &mcsv1alpha1.ClusterInfoImport{}
224+
err := fakeClient.Get(ctx, ciResImportName, gotCIImp)
225+
isNotFound := apierrors.IsNotFound(err)
226+
if err != nil {
227+
if tt.expectedCIImport == nil && !isNotFound {
228+
t.Errorf("Expected to get not found error but got error = %v", err)
229+
}
230+
if tt.expectedCIImport != nil && isNotFound {
231+
t.Errorf("Expected to get ClusterInfoImport %v but got not found error = %v", tt.expectedCIImport, err)
231232
}
233+
} else if tt.expectedCIImport != nil {
234+
if !reflect.DeepEqual(tt.expectedCIImport.Spec, gotCIImp.Spec) {
235+
t.Errorf("Expected ClusterInfoImport %v but got %v", tt.expectedCIImport.Spec, gotCIImp.Spec)
236+
}
237+
}
238+
if tt.expNumInstalledResImp != len(r.installedResImports.List()) {
239+
t.Errorf("Unexpected number of installed ResImports after reconciliation")
232240
}
233241
})
234242
}

‎multicluster/controllers/multicluster/commonarea/resourceimport_controller.go‎

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -245,6 +245,7 @@ func (r *ResourceImportReconciler) handleResImpDeleteForService(ctx context.Cont
245245
}
246246
err := r.localClusterClient.Delete(ctx, svc, &client.DeleteOptions{})
247247
if err != nil && !apierrors.IsNotFound(err) {
248+
klog.ErrorS(err, "Failed to delete imported Service", "service", svcName)
248249
return ctrl.Result{}, err
249250
}
250251

@@ -254,8 +255,14 @@ func (r *ResourceImportReconciler) handleResImpDeleteForService(ctx context.Cont
254255
Name: resImp.Spec.Name,
255256
},
256257
}
257-
err = r.localClusterClient.Delete(ctx, svcImp, &client.DeleteOptions{})
258-
return ctrl.Result{}, client.IgnoreNotFound(err)
258+
err = client.IgnoreNotFound(r.localClusterClient.Delete(ctx, svcImp, &client.DeleteOptions{}))
259+
if err != nil {
260+
klog.ErrorS(err, "Failed to delete ServiceImport for ResourceImport", "serviceImport", svcImpName)
261+
return ctrl.Result{}, err
262+
} else {
263+
r.installedResImports.Delete(*resImp)
264+
return ctrl.Result{}, nil
265+
}
259266
}
260267

261268
func (r *ResourceImportReconciler) handleResImpUpdateForEndpoints(ctx context.Context, resImp *multiclusterv1alpha1.ResourceImport) (ctrl.Result, error) {
@@ -325,12 +332,14 @@ func (r *ResourceImportReconciler) handleResImpDeleteForEndpoints(ctx context.Co
325332
Namespace: resImp.Spec.Namespace,
326333
},
327334
}
328-
err := r.localClusterClient.Delete(ctx, ep, &client.DeleteOptions{})
335+
err := client.IgnoreNotFound(r.localClusterClient.Delete(ctx, ep, &client.DeleteOptions{}))
329336
if err != nil {
330-
klog.InfoS("Failed to delete imported Endpoints", "endpoints", epNamespacedName, "err", err)
331-
return ctrl.Result{}, client.IgnoreNotFound(err)
337+
klog.ErrorS(err, "Failed to delete imported Endpoint", "endpoint", epNamespacedName)
338+
return ctrl.Result{}, err
339+
} else {
340+
r.installedResImports.Delete(*resImp)
341+
return ctrl.Result{}, nil
332342
}
333-
return ctrl.Result{}, nil
334343
}
335344

336345
func getMCService(resImp *multiclusterv1alpha1.ResourceImport) *corev1.Service {

‎multicluster/controllers/multicluster/commonarea/resourceimport_controller_test.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -229,11 +229,17 @@ func TestResourceImportReconciler_handleDeleteEvent(t *testing.T) {
229229
if err := fakeClient.Get(ctx, types.NamespacedName{Namespace: "default", Name: "nginx"}, svcImp); !apierrors.IsNotFound(err) {
230230
t.Errorf("ResourceImport Reconciler should delete a ServiceImport successfully but got error = %v", err)
231231
}
232+
if _, exists, _ := r.installedResImports.Get(*svcResImport); exists {
233+
t.Errorf("Reconciler should delete ResImport from installedResImports after successful resource deletion")
234+
}
232235
case "Endpoints":
233236
ep := &corev1.Endpoints{}
234237
if err := fakeClient.Get(ctx, types.NamespacedName{Namespace: "default", Name: "antrea-mc-nginx"}, ep); !apierrors.IsNotFound(err) {
235238
t.Errorf("ResourceImport Reconciler should delete an Endpoint successfully but got error = %v", err)
236239
}
240+
if _, exists, _ := r.installedResImports.Get(*epResImport); exists {
241+
t.Errorf("Reconciler should delete ResImport from installedResImports after successful resource deletion")
242+
}
237243
}
238244
}
239245
})

0 commit comments

Comments
 (0)