Skip to content

Commit 9c8dc66

Browse files
yash97yash97
committed
Revert "Node termination cleaner refactor and adding node id from node object (#567)"
This reverts commit e5eb800.
1 parent 14c9f62 commit 9c8dc66

37 files changed

+310
-680
lines changed

‎controllers/crds/cninode_controller.go‎

Lines changed: 49 additions & 80 deletions
Original file line numberDiff line numberDiff line change
@@ -25,9 +25,8 @@ import (
2525
"github.com/aws/amazon-vpc-resource-controller-k8s/pkg/utils"
2626
"github.com/go-logr/logr"
2727
"github.com/prometheus/client_golang/prometheus"
28-
"golang.org/x/sync/semaphore"
2928
v1 "k8s.io/api/core/v1"
30-
apierrors "k8s.io/apimachinery/pkg/api/errors"
29+
"k8s.io/apimachinery/pkg/api/errors"
3130
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
3231
"k8s.io/apimachinery/pkg/runtime"
3332
"k8s.io/apimachinery/pkg/types"
@@ -36,7 +35,6 @@ import (
3635
ctrl "sigs.k8s.io/controller-runtime"
3736
"sigs.k8s.io/controller-runtime/pkg/client"
3837
"sigs.k8s.io/controller-runtime/pkg/controller"
39-
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
4038
"sigs.k8s.io/controller-runtime/pkg/metrics"
4139
)
4240

@@ -77,8 +75,7 @@ type CNINodeReconciler struct {
7775
clusterName string
7876
vpcId string
7977
finalizerManager k8s.FinalizerManager
80-
deletePool *semaphore.Weighted
81-
newResourceCleaner func(nodeID string, eC2Wrapper ec2API.EC2Wrapper, vpcID string, log logr.Logger) cleanup.ResourceCleaner
78+
newResourceCleaner func(nodeID string, eC2Wrapper ec2API.EC2Wrapper, vpcID string) cleanup.ResourceCleaner
8279
}
8380

8481
func NewCNINodeReconciler(
@@ -91,8 +88,7 @@ func NewCNINodeReconciler(
9188
clusterName string,
9289
vpcId string,
9390
finalizerManager k8s.FinalizerManager,
94-
maxConcurrentWorkers int,
95-
newResourceCleaner func(nodeID string, eC2Wrapper ec2API.EC2Wrapper, vpcID string, log logr.Logger) cleanup.ResourceCleaner,
91+
newResourceCleaner func(nodeID string, eC2Wrapper ec2API.EC2Wrapper, vpcID string) cleanup.ResourceCleaner,
9692
) *CNINodeReconciler {
9793
return &CNINodeReconciler{
9894
Client: client,
@@ -104,7 +100,6 @@ func NewCNINodeReconciler(
104100
clusterName: clusterName,
105101
vpcId: vpcId,
106102
finalizerManager: finalizerManager,
107-
deletePool: semaphore.NewWeighted(int64(maxConcurrentWorkers)),
108103
newResourceCleaner: newResourceCleaner,
109104
}
110105
}
@@ -123,7 +118,7 @@ func (r *CNINodeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
123118
nodeFound := true
124119
node := &v1.Node{}
125120
if err := r.Client.Get(ctx, req.NamespacedName, node); err != nil {
126-
if apierrors.IsNotFound(err) {
121+
if errors.IsNotFound(err) {
127122
nodeFound = false
128123
} else {
129124
r.log.Error(err, "failed to get the node object in CNINode reconciliation, will retry")
@@ -133,50 +128,66 @@ func (r *CNINodeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
133128
}
134129

135130
if cniNode.GetDeletionTimestamp().IsZero() {
131+
shouldPatch := false
136132
cniNodeCopy := cniNode.DeepCopy()
137-
shouldPatch, err := r.ensureTagsAndLabels(cniNodeCopy, node)
138-
shouldPatch = controllerutil.AddFinalizer(cniNodeCopy, config.NodeTerminationFinalizer) || shouldPatch
139-
140-
if shouldPatch {
141-
r.log.Info("patching CNINode to add fields Tags, Labels and finalizer", "cninode", cniNode.Name)
142-
if err := r.Client.Patch(ctx, cniNodeCopy, client.MergeFromWithOptions(cniNode, client.MergeFromWithOptimisticLock{})); err != nil {
143-
if apierrors.IsConflict(err) {
144-
r.log.Info("failed to update cninode", "cninode", cniNode.Name, "error", err)
145-
return ctrl.Result{Requeue: true}, nil
133+
// Add cluster name tag if it does not exist
134+
val, ok := cniNode.Spec.Tags[config.VPCCNIClusterNameKey]
135+
if !ok || val != r.clusterName {
136+
if len(cniNodeCopy.Spec.Tags) != 0 {
137+
cniNodeCopy.Spec.Tags[config.VPCCNIClusterNameKey] = r.clusterName
138+
} else {
139+
cniNodeCopy.Spec.Tags = map[string]string{
140+
config.VPCCNIClusterNameKey: r.clusterName,
146141
}
147-
return ctrl.Result{}, err
148142
}
143+
shouldPatch = true
149144
}
150-
return ctrl.Result{}, err
145+
// if node exists, get & add OS label if it does not exist on CNINode
146+
if nodeFound {
147+
nodeLabelOS := node.ObjectMeta.Labels[config.NodeLabelOS]
148+
val, ok = cniNode.ObjectMeta.Labels[config.NodeLabelOS]
149+
if !ok || val != nodeLabelOS {
150+
if len(cniNodeCopy.ObjectMeta.Labels) != 0 {
151+
cniNodeCopy.ObjectMeta.Labels[config.NodeLabelOS] = nodeLabelOS
152+
} else {
153+
cniNodeCopy.ObjectMeta.Labels = map[string]string{
154+
config.NodeLabelOS: nodeLabelOS,
155+
}
156+
}
157+
shouldPatch = true
158+
}
159+
}
160+
161+
if shouldPatch {
162+
r.log.Info("patching CNINode to add required fields Tags and Labels", "cninode", cniNode.Name)
163+
return ctrl.Result{}, r.Client.Patch(ctx, cniNodeCopy, client.MergeFromWithOptions(cniNode, client.MergeFromWithOptimisticLock{}))
164+
}
165+
166+
// Add finalizer if it does not exist
167+
if err := r.finalizerManager.AddFinalizers(ctx, cniNode, config.NodeTerminationFinalizer); err != nil {
168+
r.log.Error(err, "failed to add finalizer on CNINode, will retry", "cniNode", cniNode.Name, "finalizer", config.NodeTerminationFinalizer)
169+
return ctrl.Result{}, err
170+
}
171+
return ctrl.Result{}, nil
172+
151173
} else { // CNINode is marked for deletion
152174
if !nodeFound {
153175
// node is also deleted, proceed with running the cleanup routine and remove the finalizer
176+
154177
// run cleanup for Linux nodes only
155178
if val, ok := cniNode.ObjectMeta.Labels[config.NodeLabelOS]; ok && val == config.OSLinux {
156179
r.log.Info("running the finalizer routine on cniNode", "cniNode", cniNode.Name)
157180
// run cleanup when node id is present
158181
if nodeID, ok := cniNode.Spec.Tags[config.NetworkInterfaceNodeIDKey]; ok && nodeID != "" {
159-
if !r.deletePool.TryAcquire(1) {
160-
r.log.Info("d, will requeue request")
161-
return ctrl.Result{RequeueAfter: 10 * time.Second}, nil
182+
if err := r.newResourceCleaner(nodeID, r.eC2Wrapper, r.vpcId).DeleteLeakedResources(); err != nil {
183+
r.log.Error(err, "failed to cleanup resources during node termination")
184+
ec2API.NodeTerminationENICleanupFailure.Inc()
162185
}
163-
go func(nodeID string) {
164-
defer r.deletePool.Release(1)
165-
childCtx, cancel := context.WithTimeout(ctx, config.NodeTerminationTimeout)
166-
defer cancel()
167-
if err := r.newResourceCleaner(nodeID, r.eC2Wrapper, r.vpcId, r.log).DeleteLeakedResources(childCtx); err != nil {
168-
r.log.Error(err, "failed to cleanup resources during node termination")
169-
ec2API.NodeTerminationENICleanupFailure.Inc()
170-
}
171-
}(nodeID)
172186
}
173187
}
174188

175-
if err := r.removeFinalizer(ctx, cniNode, config.NodeTerminationFinalizer); err != nil {
189+
if err := r.finalizerManager.RemoveFinalizers(ctx, cniNode, config.NodeTerminationFinalizer); err != nil {
176190
r.log.Error(err, "failed to remove finalizer on CNINode, will retry", "cniNode", cniNode.Name, "finalizer", config.NodeTerminationFinalizer)
177-
if apierrors.IsConflict(err) {
178-
return ctrl.Result{Requeue: true}, nil
179-
}
180191
return ctrl.Result{}, err
181192
}
182193
return ctrl.Result{}, nil
@@ -196,7 +207,7 @@ func (r *CNINodeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
196207
Spec: cniNode.Spec,
197208
}
198209

199-
if err := r.removeFinalizer(ctx, cniNode, config.NodeTerminationFinalizer); err != nil {
210+
if err := r.finalizerManager.RemoveFinalizers(ctx, cniNode, config.NodeTerminationFinalizer); err != nil {
200211
r.log.Error(err, "failed to remove finalizer on CNINode, will retry")
201212
return ctrl.Result{}, err
202213
}
@@ -241,7 +252,7 @@ func (r *CNINodeReconciler) waitTillCNINodeDeleted(nameSpacedCNINode types.Names
241252
oldCNINode := &v1alpha1.CNINode{}
242253

243254
return wait.PollUntilContextTimeout(context.TODO(), 500*time.Millisecond, time.Second*3, true, func(ctx context.Context) (bool, error) {
244-
if err := r.Client.Get(ctx, nameSpacedCNINode, oldCNINode); err != nil && apierrors.IsNotFound(err) {
255+
if err := r.Client.Get(ctx, nameSpacedCNINode, oldCNINode); err != nil && errors.IsNotFound(err) {
245256
return true, nil
246257
}
247258
return false, nil
@@ -255,45 +266,3 @@ func (r *CNINodeReconciler) createCNINodeFromObj(ctx context.Context, newCNINode
255266
return r.Client.Create(ctx, newCNINode)
256267
})
257268
}
258-
259-
func (r *CNINodeReconciler) ensureTagsAndLabels(cniNode *v1alpha1.CNINode, node *v1.Node) (bool, error) {
260-
shouldPatch := false
261-
var err error
262-
if cniNode.Spec.Tags == nil {
263-
cniNode.Spec.Tags = make(map[string]string)
264-
}
265-
// add cluster name tag if it does not exist
266-
if cniNode.Spec.Tags[config.VPCCNIClusterNameKey] != r.clusterName {
267-
cniNode.Spec.Tags[config.VPCCNIClusterNameKey] = r.clusterName
268-
shouldPatch = true
269-
}
270-
if node != nil {
271-
var nodeID string
272-
nodeID, err = utils.GetNodeID(node)
273-
274-
if nodeID != "" && cniNode.Spec.Tags[config.NetworkInterfaceNodeIDKey] != nodeID {
275-
cniNode.Spec.Tags[config.NetworkInterfaceNodeIDKey] = nodeID
276-
shouldPatch = true
277-
}
278-
279-
// add node label if it does not exist
280-
if cniNode.ObjectMeta.Labels == nil {
281-
cniNode.ObjectMeta.Labels = make(map[string]string)
282-
}
283-
if cniNode.ObjectMeta.Labels[config.NodeLabelOS] != node.ObjectMeta.Labels[config.NodeLabelOS] {
284-
cniNode.ObjectMeta.Labels[config.NodeLabelOS] = node.ObjectMeta.Labels[config.NodeLabelOS]
285-
shouldPatch = true
286-
}
287-
}
288-
return shouldPatch, err
289-
}
290-
291-
func (r *CNINodeReconciler) removeFinalizer(ctx context.Context, cniNode *v1alpha1.CNINode, finalizer string) error {
292-
cniNodeCopy := cniNode.DeepCopy()
293-
294-
if controllerutil.RemoveFinalizer(cniNodeCopy, finalizer) {
295-
r.log.Info("removing finalizer for cninode", "name", cniNode.GetName(), "finalizer", finalizer)
296-
return r.Client.Patch(ctx, cniNodeCopy, client.MergeFromWithOptions(cniNode, client.MergeFromWithOptimisticLock{}))
297-
}
298-
return nil
299-
}

‎controllers/crds/cninode_controller_test.go‎

Lines changed: 17 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -11,10 +11,8 @@ import (
1111
ec2API "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/aws/ec2/api"
1212
"github.com/aws/amazon-vpc-resource-controller-k8s/pkg/aws/ec2/api/cleanup"
1313
"github.com/aws/amazon-vpc-resource-controller-k8s/pkg/config"
14-
"github.com/go-logr/logr"
1514
"github.com/golang/mock/gomock"
1615
"github.com/stretchr/testify/assert"
17-
"golang.org/x/sync/semaphore"
1816
corev1 "k8s.io/api/core/v1"
1917
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2018
"k8s.io/apimachinery/pkg/runtime"
@@ -39,9 +37,6 @@ var (
3937
config.NodeLabelOS: "linux",
4038
},
4139
},
42-
Spec: corev1.NodeSpec{
43-
ProviderID: "aws:///us-west-2a/i-0123456789abcdef0",
44-
},
4540
}
4641
reconcileRequest = reconcile.Request{
4742
NamespacedName: types.NamespacedName{
@@ -62,7 +57,6 @@ func NewCNINodeMock(ctrl *gomock.Controller, mockObjects ...client.Object) *CNIN
6257
log: zap.New(),
6358
clusterName: mockClusterName,
6459
vpcId: "vpc-000000000000",
65-
deletePool: semaphore.NewWeighted(10),
6660
},
6761
}
6862
}
@@ -86,7 +80,7 @@ func TestCNINodeReconcile(t *testing.T) {
8680
asserts func(reconcile.Result, error, *v1alpha1.CNINode)
8781
}{
8882
{
89-
name: "verify clusterName, instanceID, os label are added if missing",
83+
name: "verify clusterName tag and labels are added if missing",
9084
args: args{
9185
mockNode: mockNodeWithLabel,
9286
mockCNINode: &v1alpha1.CNINode{
@@ -100,7 +94,7 @@ func TestCNINodeReconcile(t *testing.T) {
10094
assert.NoError(t, err)
10195
assert.Equal(t, res, reconcile.Result{})
10296
assert.Equal(t, cniNode.Labels, map[string]string{config.NodeLabelOS: "linux"})
103-
assert.Equal(t, cniNode.Spec.Tags, map[string]string{config.VPCCNIClusterNameKey: mockClusterName, config.NetworkInterfaceNodeIDKey: "i-0123456789abcdef0"})
97+
assert.Equal(t, cniNode.Spec.Tags, map[string]string{config.VPCCNIClusterNameKey: mockClusterName})
10498
},
10599
},
106100
{
@@ -119,10 +113,14 @@ func TestCNINodeReconcile(t *testing.T) {
119113
},
120114
},
121115
prepare: func(f *fields) {
122-
f.mockCNINode.Reconciler.newResourceCleaner = func(nodeID string, eC2Wrapper ec2API.EC2Wrapper, vpcID string, log logr.Logger) cleanup.ResourceCleaner {
116+
f.mockCNINode.Reconciler.newResourceCleaner = func(nodeID string, eC2Wrapper ec2API.EC2Wrapper, vpcID string) cleanup.ResourceCleaner {
123117
return f.mockResourceCleaner
124118
}
125-
f.mockResourceCleaner.EXPECT().DeleteLeakedResources(gomock.Any()).Times(0)
119+
f.mockResourceCleaner.EXPECT().DeleteLeakedResources().Times(0)
120+
121+
f.mockFinalizerManager.EXPECT().
122+
RemoveFinalizers(gomock.Any(), gomock.Any(), config.NodeTerminationFinalizer).
123+
Return(nil)
126124
},
127125
asserts: func(res reconcile.Result, err error, cniNode *v1alpha1.CNINode) {
128126
assert.NoError(t, err)
@@ -144,50 +142,27 @@ func TestCNINodeReconcile(t *testing.T) {
144142
},
145143
Spec: v1alpha1.CNINodeSpec{
146144
Tags: map[string]string{
147-
config.NetworkInterfaceNodeIDKey: "i-0123456789abcdef0",
145+
config.NetworkInterfaceNodeIDKey: "i-1234567890",
148146
},
149147
},
150148
},
151149
},
152150
prepare: func(f *fields) {
153-
f.mockCNINode.Reconciler.newResourceCleaner = func(nodeID string, eC2Wrapper ec2API.EC2Wrapper, vpcID string, log logr.Logger) cleanup.ResourceCleaner {
154-
assert.Equal(t, "i-0123456789abcdef0", nodeID)
151+
f.mockCNINode.Reconciler.newResourceCleaner = func(nodeID string, eC2Wrapper ec2API.EC2Wrapper, vpcID string) cleanup.ResourceCleaner {
152+
assert.Equal(t, "i-1234567890", nodeID)
155153
return f.mockResourceCleaner
156154
}
157-
f.mockResourceCleaner.EXPECT().DeleteLeakedResources(gomock.Any()).Times(1).Return(nil)
155+
f.mockResourceCleaner.EXPECT().DeleteLeakedResources().Times(1).Return(nil)
156+
f.mockFinalizerManager.EXPECT().
157+
RemoveFinalizers(gomock.Any(), gomock.Any(), config.NodeTerminationFinalizer).
158+
Return(nil)
158159

159160
},
160161
asserts: func(res reconcile.Result, err error, cniNode *v1alpha1.CNINode) {
161162
assert.NoError(t, err)
162163
assert.Equal(t, res, reconcile.Result{})
163164
},
164165
},
165-
{
166-
name: "verify finalizer is added when labels and tags are present",
167-
args: args{
168-
mockNode: mockNodeWithLabel,
169-
mockCNINode: &v1alpha1.CNINode{
170-
ObjectMeta: metav1.ObjectMeta{
171-
Name: mockName,
172-
Labels: map[string]string{
173-
config.NodeLabelOS: "linux",
174-
},
175-
},
176-
Spec: v1alpha1.CNINodeSpec{
177-
Tags: map[string]string{
178-
config.VPCCNIClusterNameKey: mockClusterName,
179-
config.NetworkInterfaceNodeIDKey: "i-0123456789abcdef0",
180-
},
181-
},
182-
},
183-
},
184-
prepare: nil,
185-
asserts: func(res reconcile.Result, err error, cniNode *v1alpha1.CNINode) {
186-
assert.NoError(t, err)
187-
assert.Equal(t, res, reconcile.Result{})
188-
assert.Contains(t, cniNode.Finalizers, config.NodeTerminationFinalizer)
189-
},
190-
},
191166
}
192167
for _, tt := range tests {
193168
t.Run(tt.name, func(t *testing.T) {
@@ -213,10 +188,8 @@ func TestCNINodeReconcile(t *testing.T) {
213188
res, err := mock.Reconciler.Reconcile(context.Background(), reconcileRequest)
214189

215190
cniNode := &v1alpha1.CNINode{}
216-
if tt.args.mockCNINode.GetDeletionTimestamp() == nil {
217-
getErr := mock.Reconciler.Client.Get(context.Background(), reconcileRequest.NamespacedName, cniNode)
218-
assert.NoError(t, getErr)
219-
}
191+
getErr := mock.Reconciler.Client.Get(context.Background(), reconcileRequest.NamespacedName, cniNode)
192+
assert.NoError(t, getErr)
220193

221194
if tt.asserts != nil {
222195
tt.asserts(res, err, cniNode)

‎go.mod‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@ require (
2424
github.com/prometheus/common v0.62.0
2525
github.com/stretchr/testify v1.10.0
2626
go.uber.org/zap v1.27.0
27-
golang.org/x/sync v0.13.0
2827
golang.org/x/time v0.11.0
2928
gomodules.xyz/jsonpatch/v2 v2.5.0
3029
k8s.io/api v0.33.0
@@ -48,6 +47,7 @@ require (
4847
github.com/gorilla/websocket v1.5.4-0.20250319132907-e064f32e3674 // indirect
4948
github.com/mxk/go-flowrate v0.0.0-20140419014527-cca7078d478f // indirect
5049
github.com/x448/float16 v0.8.4 // indirect
50+
golang.org/x/sync v0.13.0 // indirect
5151
gopkg.in/evanphx/json-patch.v4 v4.12.0 // indirect
5252
sigs.k8s.io/randfill v1.0.0 // indirect
5353
)

‎main.go‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -439,7 +439,6 @@ func main() {
439439
clusterName,
440440
vpcID,
441441
finalizerManager,
442-
maxNodeConcurrentReconciles,
443442
cleanup.NewNodeResourceCleaner,
444443
).SetupWithManager(mgr, maxNodeConcurrentReconciles)); err != nil {
445444
setupLog.Error(err, "unable to create controller", "controller", "CNINode")

0 commit comments

Comments
 (0)