conflict resolve

.go-version

@@ -1 +1 @@
-1.22.5
+1.22

PROJECT

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

apis/vpcresources/v1alpha1/cninode_types.go

@@ -35,8 +35,6 @@ type Feature struct {
 // CNINodeSpec defines the desired state of CNINode
 type CNINodeSpec struct {
 	Features []Feature `json:"features,omitempty"`
-	// Additional tag key/value added to all network interfaces provisioned by the vpc-resource-controller and VPC-CNI
-	Tags map[string]string `json:"tags,omitempty"`
 }
 
 // CNINodeStatus defines the managed VPC resources.

config/crd/bases/vpcresources.k8s.aws_cninodes.yaml

@@ -61,12 +61,6 @@ spec:
                       type: string
                   type: object
                 type: array
-              tags:
-                additionalProperties:
-                  type: string
-                description: Additional tag key/value added to all network interfaces
-                  provisioned by the vpc-resource-controller and VPC-CNI
-                type: object
             type: object
           status:
             description: CNINodeStatus defines the managed VPC resources.

config/rbac/role.yaml

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

controllers/core/node_controller.go

@@ -15,16 +15,18 @@ package controllers
 
 import (
 	"context"
+	"fmt"
 	"net/http"
 	"time"
 
 	"github.com/aws/amazon-vpc-resource-controller-k8s/apis/vpcresources/v1alpha1"
 	"github.com/aws/amazon-vpc-resource-controller-k8s/pkg/condition"
-	"github.com/aws/amazon-vpc-resource-controller-k8s/pkg/config"
 	rcHealthz "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/healthz"
 	"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"
@@ -34,9 +36,31 @@ 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
+// one routines to help high rate churn and larger nodes groups restarting
+// when the controller has to be restarted for various reasons.
+const (
+	MaxNodeConcurrentReconciles = 10
+	NodeTerminationFinalizer    = "networking.k8s.aws/resource-cleanup"
+)
+
 // NodeReconciler reconciles a Node object
 type NodeReconciler struct {
 	client.Client
@@ -65,27 +89,45 @@ func (r *NodeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.
 	}
 
 	node := &corev1.Node{}
-	var err error
 
 	logger := r.Log.WithValues("node", req.NamespacedName)
 
-	if err := r.Client.Get(ctx, req.NamespacedName, node); err != nil {
-		if errors.IsNotFound(err) {
-			r.Log.V(1).Info("the requested node couldn't be found by k8s client", "Node", req.NamespacedName)
+	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 {
-				err := r.Manager.DeleteNode(req.Name)
-				if err != nil {
+				cacheErr := r.Manager.DeleteNode(req.Name)
+				if cacheErr != nil {
 					// The request is not retryable so not returning the error
-					logger.Error(err, "failed to delete node from manager")
+					logger.Error(cacheErr, "failed to delete node from manager")
 					return ctrl.Result{}, nil
 				}
 				logger.V(1).Info("deleted the node from manager")
 			}
 		}
-		return ctrl.Result{}, client.IgnoreNotFound(err)
+		return ctrl.Result{}, client.IgnoreNotFound(nodeErr)
 	}
 
+	var err error
+
 	_, found := r.Manager.GetNode(req.Name)
 	if found {
 		logger.V(1).Info("updating node")
@@ -107,9 +149,11 @@ func (r *NodeReconciler) SetupWithManager(mgr ctrl.Manager, healthzHandler *rcHe
 		map[string]healthz.Checker{"health-node-controller": r.Check()},
 	)
 
+	prometheusRegister()
+
 	return ctrl.NewControllerManagedBy(mgr).
 		For(&corev1.Node{}).
-		WithOptions(controller.Options{MaxConcurrentReconciles: config.MaxNodeConcurrentReconciles}).
+		WithOptions(controller.Options{MaxConcurrentReconciles: MaxNodeConcurrentReconciles}).
 		Owns(&v1alpha1.CNINode{}).
 		Complete(r)
 }
@@ -124,6 +168,12 @@ func (r *NodeReconciler) Check() healthz.Checker {
 			return nil
 		}
 
+		if r.Manager.SkipHealthCheck() {
+			// node manager observes EC2 error on processing node, pausing reconciler check to avoid stressing the system
+			r.Log.Info("due to EC2 error, node controller skips node reconciler health check for now")
+			return nil
+		}
+
 		err := rcHealthz.PingWithTimeout(func(c chan<- error) {
 			// when the reconciler is ready, testing the reconciler with a fake node request
 			pingRequest := &ctrl.Request{
@@ -144,3 +194,11 @@ func (r *NodeReconciler) Check() healthz.Checker {
 		return err
 	}
 }
+
+func prometheusRegister() {
+	if !prometheusRegistered {
+		metrics.Registry.MustRegister(leakedCNINodeResourceCount)
+
+		prometheusRegistered = true
+	}
+}

controllers/core/node_controller_test.go

@@ -18,18 +18,21 @@ import (
 	"testing"
 	"time"
 
+	"github.com/aws/amazon-vpc-resource-controller-k8s/apis/vpcresources/v1alpha1"
 	mock_condition "github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/condition"
 	mock_node "github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/node"
 	mock_manager "github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/node/manager"
 
 	"github.com/golang/mock/gomock"
 	"github.com/stretchr/testify/assert"
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/errors"
 	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"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"
 )
@@ -61,6 +64,7 @@ func NewNodeMock(ctrl *gomock.Controller, mockObjects ...client.Object) NodeMock
 
 	scheme := runtime.NewScheme()
 	_ = corev1.AddToScheme(scheme)
+	_ = v1alpha1.AddToScheme(scheme)
 	client := fakeClient.NewClientBuilder().WithScheme(scheme).WithObjects(mockObjects...).Build()
 
 	return NodeMock{
@@ -139,6 +143,43 @@ 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()