Centralized leaked ENI cleanup- refactor periodic cleanup & add node termination cleaner (aws#505)

* Centralized leaked ENI cleanup- refactor periodic cleanup & add node termination cleaner

* defer updating metrics

* WIP commit no.3

* instance id as tag, not requeuing on node cleanup failure

* not requeing on node termination cleaner failure

* minor log change

* adding node termination failure metric

* minor test change

* using paginated calls to describe network interface while eni cleanup

* refactor based on comments

---------

Co-authored-by: Yash Thakkar <[email protected]>

Author: sushrk

PROJECT

@@ -19,6 +19,7 @@ resources:
   version: v1beta1
 - api:
     crdVersion: v1
+  controller: true
   domain: k8s.aws
   group: vpcresources
   kind: CNINode

config/rbac/role.yaml

@@ -60,6 +60,8 @@ rules:
   - create
   - get
   - list
+  - patch
+  - update
   - watch
 - apiGroups:
   - vpcresources.k8s.aws

controllers/core/node_controller.go

@@ -15,7 +15,6 @@ package controllers
 
 import (
 	"context"
-	"fmt"
 	"net/http"
 	"time"
 
@@ -25,8 +24,6 @@ import (
 	"github.com/aws/amazon-vpc-resource-controller-k8s/pkg/k8s"
 	"github.com/aws/amazon-vpc-resource-controller-k8s/pkg/node/manager"
 	"github.com/google/uuid"
-	"github.com/prometheus/client_golang/prometheus"
-	"sigs.k8s.io/controller-runtime/pkg/metrics"
 
 	"github.com/go-logr/logr"
 	corev1 "k8s.io/api/core/v1"
@@ -36,21 +33,9 @@ import (
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller"
-	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/healthz"
 )
 
-var (
-	leakedCNINodeResourceCount = prometheus.NewCounter(
-		prometheus.CounterOpts{
-			Name: "orphaned_cninode_objects",
-			Help: "The number of leaked cninode resources",
-		},
-	)
-
-	prometheusRegistered = false
-)
-
 // MaxNodeConcurrentReconciles is the number of go routines that can invoke
 // Reconcile in parallel. Since Node Reconciler, performs local operation
 // on cache only a single go routine should be sufficient. Using more than
@@ -93,23 +78,6 @@ func (r *NodeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.
 
 	if nodeErr := r.Client.Get(ctx, req.NamespacedName, node); nodeErr != nil {
 		if errors.IsNotFound(nodeErr) {
-			// clean up CNINode finalizer
-			cniNode := &v1alpha1.CNINode{}
-			if cninodeErr := r.Client.Get(ctx, req.NamespacedName, cniNode); cninodeErr == nil {
-				if yes := controllerutil.ContainsFinalizer(cniNode, NodeTerminationFinalizer); yes {
-					updated := cniNode.DeepCopy()
-					if yes = controllerutil.RemoveFinalizer(updated, NodeTerminationFinalizer); yes {
-						if err := r.Client.Patch(ctx, updated, client.MergeFrom(cniNode)); err != nil {
-							return ctrl.Result{}, err
-						}
-						r.Log.Info("removed leaked CNINode resource's finalizer", "cninode", cniNode.Name)
-					}
-					leakedCNINodeResourceCount.Inc()
-				}
-			} else if !errors.IsNotFound(cninodeErr) {
-				return ctrl.Result{}, fmt.Errorf("failed getting CNINode %s from cached client, %w", cniNode.Name, cninodeErr)
-			}
-
 			// clean up local cached nodes
 			_, found := r.Manager.GetNode(req.Name)
 			if found {
@@ -148,8 +116,6 @@ func (r *NodeReconciler) SetupWithManager(mgr ctrl.Manager, maxConcurrentReconci
 		map[string]healthz.Checker{"health-node-controller": r.Check()},
 	)
 
-	prometheusRegister()
-
 	return ctrl.NewControllerManagedBy(mgr).
 		For(&corev1.Node{}).
 		WithOptions(controller.Options{MaxConcurrentReconciles: maxConcurrentReconciles}).
@@ -193,11 +159,3 @@ func (r *NodeReconciler) Check() healthz.Checker {
 		return err
 	}
 }
-
-func prometheusRegister() {
-	if !prometheusRegistered {
-		metrics.Registry.MustRegister(leakedCNINodeResourceCount)
-
-		prometheusRegistered = true
-	}
-}

controllers/core/node_controller_test.go

@@ -32,7 +32,6 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	fakeClient "sigs.k8s.io/controller-runtime/pkg/client/fake"
-	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/log/zap"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 )
@@ -143,43 +142,6 @@ func TestNodeReconciler_Reconcile_DeleteNonExistentNode(t *testing.T) {
 	assert.Equal(t, res, reconcile.Result{})
 }
 
-func TestNodeReconciler_Reconcile_DeleteNonExistentNodesCNINode(t *testing.T) {
-	ctrl := gomock.NewController(t)
-	defer ctrl.Finish()
-
-	mock := NewNodeMock(ctrl)
-	cniNode := &v1alpha1.CNINode{
-		ObjectMeta: v1.ObjectMeta{
-			Name:       mockNodeName,
-			Finalizers: []string{NodeTerminationFinalizer},
-		},
-	}
-	mock.Reconciler.Client = fakeClient.NewClientBuilder().WithScheme(mock.Reconciler.Scheme).WithObjects(cniNode).Build()
-
-	mock.Conditions.EXPECT().GetPodDataStoreSyncStatus().Return(true)
-	mock.Manager.EXPECT().GetNode(mockNodeName).Return(mock.MockNode, false)
-
-	original := &v1alpha1.CNINode{}
-	err := mock.Reconciler.Client.Get(context.TODO(), types.NamespacedName{Name: cniNode.Name}, original)
-	assert.NoError(t, err)
-	assert.True(t, controllerutil.ContainsFinalizer(original, NodeTerminationFinalizer), "the CNINode has finalizer")
-
-	res, err := mock.Reconciler.Reconcile(context.TODO(), reconcileRequest)
-	assert.NoError(t, err)
-	assert.Equal(t, res, reconcile.Result{})
-
-	node := &corev1.Node{}
-	updated := &v1alpha1.CNINode{}
-	err = mock.Reconciler.Client.Get(context.TODO(), types.NamespacedName{Name: cniNode.Name}, node)
-	assert.Error(t, err, "the node shouldn't existing")
-	assert.True(t, errors.IsNotFound(err))
-
-	err = mock.Reconciler.Client.Get(context.TODO(), types.NamespacedName{Name: cniNode.Name}, updated)
-	assert.NoError(t, err)
-	assert.True(t, updated.Name == mockNodeName, "the CNINode should existing and waiting for finalizer removal")
-	assert.False(t, controllerutil.ContainsFinalizer(updated, NodeTerminationFinalizer), "CNINode finalizer should be removed when the node is gone")
-}
-
 func TestNodeReconciler_Reconcile_DeleteNonExistentUnmanagedNode(t *testing.T) {
 	ctrl := gomock.NewController(t)
 	defer ctrl.Finish()