Skip to content

Commit 30d9841

Browse files
yash97yash97
authored
skipping node cleaner if node id is not present (#553)
* skipping node cleaner if node id is not present * changing var name
1 parent ae2384c commit 30d9841

File tree

7 files changed

+140
-50
lines changed

7 files changed

+140
-50
lines changed

‎controllers/crds/cninode_controller.go‎

Lines changed: 26 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -67,14 +67,15 @@ func prometheusRegister() {
6767
// CNINodeReconciler reconciles a CNINode object
6868
type CNINodeReconciler struct {
6969
client.Client
70-
scheme *runtime.Scheme
71-
context context.Context
72-
log logr.Logger
73-
eC2Wrapper ec2API.EC2Wrapper
74-
k8sAPI k8s.K8sWrapper
75-
clusterName string
76-
vpcId string
77-
finalizerManager k8s.FinalizerManager
70+
scheme *runtime.Scheme
71+
context context.Context
72+
log logr.Logger
73+
eC2Wrapper ec2API.EC2Wrapper
74+
k8sAPI k8s.K8sWrapper
75+
clusterName string
76+
vpcId string
77+
finalizerManager k8s.FinalizerManager
78+
newResourceCleaner func(nodeID string, eC2Wrapper ec2API.EC2Wrapper, vpcID string) cleanup.ResourceCleaner
7879
}
7980

8081
func NewCNINodeReconciler(
@@ -87,17 +88,19 @@ func NewCNINodeReconciler(
8788
clusterName string,
8889
vpcId string,
8990
finalizerManager k8s.FinalizerManager,
91+
newResourceCleaner func(nodeID string, eC2Wrapper ec2API.EC2Wrapper, vpcID string) cleanup.ResourceCleaner,
9092
) *CNINodeReconciler {
9193
return &CNINodeReconciler{
92-
Client: client,
93-
scheme: scheme,
94-
context: ctx,
95-
log: logger,
96-
eC2Wrapper: ec2Wrapper,
97-
k8sAPI: k8sWrapper,
98-
clusterName: clusterName,
99-
vpcId: vpcId,
100-
finalizerManager: finalizerManager,
94+
Client: client,
95+
scheme: scheme,
96+
context: ctx,
97+
log: logger,
98+
eC2Wrapper: ec2Wrapper,
99+
k8sAPI: k8sWrapper,
100+
clusterName: clusterName,
101+
vpcId: vpcId,
102+
finalizerManager: finalizerManager,
103+
newResourceCleaner: newResourceCleaner,
101104
}
102105
}
103106

@@ -174,19 +177,12 @@ func (r *CNINodeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
174177
// run cleanup for Linux nodes only
175178
if val, ok := cniNode.ObjectMeta.Labels[config.NodeLabelOS]; ok && val == config.OSLinux {
176179
r.log.Info("running the finalizer routine on cniNode", "cniNode", cniNode.Name)
177-
cleaner := &cleanup.NodeTerminationCleaner{
178-
NodeID: cniNode.Spec.Tags[config.NetworkInterfaceNodeIDKey],
179-
}
180-
cleaner.ENICleaner = &cleanup.ENICleaner{
181-
EC2Wrapper: r.eC2Wrapper,
182-
Manager: cleaner,
183-
VpcId: r.vpcId,
184-
Log: ctrl.Log.WithName("eniCleaner").WithName("node"),
185-
}
186-
187-
if err := cleaner.DeleteLeakedResources(); err != nil {
188-
r.log.Error(err, "failed to cleanup resources during node termination")
189-
ec2API.NodeTerminationENICleanupFailure.Inc()
180+
// run cleanup when node id is present
181+
if nodeID, ok := cniNode.Spec.Tags[config.NetworkInterfaceNodeIDKey]; ok && nodeID != "" {
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()
185+
}
190186
}
191187
}
192188

‎controllers/crds/cninode_controller_test.go‎

Lines changed: 82 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,11 @@ import (
55
"testing"
66

77
"github.com/aws/amazon-vpc-resource-controller-k8s/apis/vpcresources/v1alpha1"
8+
mock_api "github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/aws/ec2/api"
89
mock_cleanup "github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/aws/ec2/api/cleanup"
910
mock_k8s "github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/k8s"
11+
ec2API "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/aws/ec2/api"
12+
"github.com/aws/amazon-vpc-resource-controller-k8s/pkg/aws/ec2/api/cleanup"
1013
"github.com/aws/amazon-vpc-resource-controller-k8s/pkg/config"
1114
"github.com/golang/mock/gomock"
1215
"github.com/stretchr/testify/assert"
@@ -42,12 +45,12 @@ var (
4245
}
4346
)
4447

45-
func NewCNINodeMock(ctrl *gomock.Controller, mockObjects ...client.Object) CNINodeMock {
48+
func NewCNINodeMock(ctrl *gomock.Controller, mockObjects ...client.Object) *CNINodeMock {
4649
scheme := runtime.NewScheme()
4750
_ = corev1.AddToScheme(scheme)
4851
_ = v1alpha1.AddToScheme(scheme)
4952
client := fakeClient.NewClientBuilder().WithScheme(scheme).WithObjects(mockObjects...).Build()
50-
return CNINodeMock{
53+
return &CNINodeMock{
5154
Reconciler: CNINodeReconciler{
5255
Client: client,
5356
scheme: scheme,
@@ -67,6 +70,8 @@ func TestCNINodeReconcile(t *testing.T) {
6770
mockResourceCleaner *mock_cleanup.MockResourceCleaner
6871
mockK8sApi *mock_k8s.MockK8sWrapper
6972
mockFinalizerManager *mock_k8s.MockFinalizerManager
73+
mockEC2API *mock_api.MockEC2APIHelper
74+
mockCNINode *CNINodeMock
7075
}
7176
tests := []struct {
7277
name string
@@ -92,18 +97,91 @@ func TestCNINodeReconcile(t *testing.T) {
9297
assert.Equal(t, cniNode.Spec.Tags, map[string]string{config.VPCCNIClusterNameKey: mockClusterName})
9398
},
9499
},
100+
{
101+
name: "verify cleaner was not called if node id is not present given that node is being finalized",
102+
args: args{
103+
mockNode: nil,
104+
mockCNINode: &v1alpha1.CNINode{
105+
ObjectMeta: metav1.ObjectMeta{
106+
Name: mockName,
107+
Labels: map[string]string{
108+
config.NodeLabelOS: config.OSLinux,
109+
},
110+
Finalizers: []string{config.NodeTerminationFinalizer},
111+
DeletionTimestamp: &metav1.Time{Time: metav1.Now().Time},
112+
},
113+
},
114+
},
115+
prepare: func(f *fields) {
116+
f.mockCNINode.Reconciler.newResourceCleaner = func(nodeID string, eC2Wrapper ec2API.EC2Wrapper, vpcID string) cleanup.ResourceCleaner {
117+
return f.mockResourceCleaner
118+
}
119+
f.mockResourceCleaner.EXPECT().DeleteLeakedResources().Times(0)
120+
121+
f.mockFinalizerManager.EXPECT().
122+
RemoveFinalizers(gomock.Any(), gomock.Any(), config.NodeTerminationFinalizer).
123+
Return(nil)
124+
},
125+
asserts: func(res reconcile.Result, err error, cniNode *v1alpha1.CNINode) {
126+
assert.NoError(t, err)
127+
assert.Equal(t, res, reconcile.Result{})
128+
},
129+
},
130+
{
131+
name: "verify cleaner was called if node id is not empty when node is being finalized",
132+
args: args{
133+
mockNode: nil,
134+
mockCNINode: &v1alpha1.CNINode{
135+
ObjectMeta: metav1.ObjectMeta{
136+
Name: mockName,
137+
Labels: map[string]string{
138+
config.NodeLabelOS: config.OSLinux,
139+
},
140+
Finalizers: []string{config.NodeTerminationFinalizer},
141+
DeletionTimestamp: &metav1.Time{Time: metav1.Now().Time},
142+
},
143+
Spec: v1alpha1.CNINodeSpec{
144+
Tags: map[string]string{
145+
config.NetworkInterfaceNodeIDKey: "i-1234567890",
146+
},
147+
},
148+
},
149+
},
150+
prepare: func(f *fields) {
151+
f.mockCNINode.Reconciler.newResourceCleaner = func(nodeID string, eC2Wrapper ec2API.EC2Wrapper, vpcID string) cleanup.ResourceCleaner {
152+
assert.Equal(t, "i-1234567890", nodeID)
153+
return f.mockResourceCleaner
154+
}
155+
f.mockResourceCleaner.EXPECT().DeleteLeakedResources().Times(1).Return(nil)
156+
f.mockFinalizerManager.EXPECT().
157+
RemoveFinalizers(gomock.Any(), gomock.Any(), config.NodeTerminationFinalizer).
158+
Return(nil)
159+
160+
},
161+
asserts: func(res reconcile.Result, err error, cniNode *v1alpha1.CNINode) {
162+
assert.NoError(t, err)
163+
assert.Equal(t, res, reconcile.Result{})
164+
},
165+
},
95166
}
96167
for _, tt := range tests {
97168
t.Run(tt.name, func(t *testing.T) {
98169
ctrl := gomock.NewController(t)
99170
defer ctrl.Finish()
100-
101-
mock := NewCNINodeMock(ctrl, tt.args.mockNode, tt.args.mockCNINode)
171+
objs := []client.Object{tt.args.mockCNINode}
172+
if tt.args.mockNode != nil {
173+
objs = append(objs, tt.args.mockNode)
174+
}
175+
mock := NewCNINodeMock(ctrl, objs...)
102176
f := fields{
103177
mockResourceCleaner: mock_cleanup.NewMockResourceCleaner(ctrl),
104178
mockK8sApi: mock_k8s.NewMockK8sWrapper(ctrl),
105179
mockFinalizerManager: mock_k8s.NewMockFinalizerManager(ctrl),
180+
mockEC2API: mock_api.NewMockEC2APIHelper(ctrl),
181+
mockCNINode: mock,
106182
}
183+
mock.Reconciler.finalizerManager = f.mockFinalizerManager
184+
mock.Reconciler.k8sAPI = f.mockK8sApi
107185
if tt.prepare != nil {
108186
tt.prepare(&f)
109187
}
@@ -118,5 +196,4 @@ func TestCNINodeReconcile(t *testing.T) {
118196
}
119197
})
120198
}
121-
122199
}

‎main.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ import (
2929
crdcontroller "github.com/aws/amazon-vpc-resource-controller-k8s/controllers/crds"
3030
"github.com/aws/amazon-vpc-resource-controller-k8s/pkg/api"
3131
ec2API "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/aws/ec2/api"
32+
"github.com/aws/amazon-vpc-resource-controller-k8s/pkg/aws/ec2/api/cleanup"
3233
eniCleaner "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/aws/ec2/api/cleanup"
3334
"github.com/aws/amazon-vpc-resource-controller-k8s/pkg/condition"
3435
"github.com/aws/amazon-vpc-resource-controller-k8s/pkg/config"
@@ -438,6 +439,7 @@ func main() {
438439
clusterName,
439440
vpcID,
440441
finalizerManager,
442+
cleanup.NewNodeResourceCleaner,
441443
).SetupWithManager(mgr, maxNodeConcurrentReconciles)); err != nil {
442444
setupLog.Error(err, "unable to create controller", "controller", "CNINode")
443445
os.Exit(1)

‎pkg/aws/ec2/api/cleanup/node_cleanup.go‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,9 +14,11 @@
1414
package cleanup
1515

1616
import (
17+
ec2API "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/aws/ec2/api"
1718
"github.com/aws/amazon-vpc-resource-controller-k8s/pkg/config"
1819
"github.com/aws/aws-sdk-go-v2/aws"
1920
ec2types "github.com/aws/aws-sdk-go-v2/service/ec2/types"
21+
ctrl "sigs.k8s.io/controller-runtime"
2022
)
2123

2224
// NodeTerminationCleanerto handle resource cleanup at node termination
@@ -48,3 +50,16 @@ func (n *NodeTerminationCleaner) UpdateAvailableENIsIfNeeded(eniMap *map[string]
4850
func (n *NodeTerminationCleaner) UpdateCleanupMetrics(vpcrcAvailableCount *int, vpccniAvailableCount *int, leakedENICount *int) {
4951
return
5052
}
53+
54+
func NewNodeResourceCleaner(nodeID string, eC2Wrapper ec2API.EC2Wrapper, vpcID string) ResourceCleaner {
55+
cleaner := &NodeTerminationCleaner{
56+
NodeID: nodeID,
57+
}
58+
cleaner.ENICleaner = &ENICleaner{
59+
EC2Wrapper: eC2Wrapper,
60+
Manager: cleaner,
61+
VpcId: vpcID,
62+
Log: ctrl.Log.WithName("eniCleaner").WithName("node"),
63+
}
64+
return cleaner.ENICleaner
65+
}

‎pkg/config/type.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,7 @@ var (
127127
// CoolDownPeriod is the time to let kube-proxy propagates IP tables rules before assigning the resource back to new pod
128128
CoolDownPeriod = time.Second * 30
129129
// ENICleanUpInterval is the time interval between each dangling ENI clean up task
130-
ENICleanUpInterval = time.Minute * 2
130+
ENICleanUpInterval = time.Minute * 30
131131
)
132132

133133
// ResourceConfig is the configuration for each resource type

‎pkg/provider/branch/trunk/trunk.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -124,7 +124,7 @@ type trunkENI struct {
124124
// deleteQueue is the queue of ENIs that are being cooled down before being deleted
125125
deleteQueue []*ENIDetails
126126
// nodeName tag is the tag added to trunk and branch ENIs created on the node
127-
nodeNameTag []ec2types.Tag
127+
nodeIDTag []ec2types.Tag
128128
}
129129

130130
// PodENI is a json convertible structure that stores the Branch ENI details that can be
@@ -176,7 +176,7 @@ func NewTrunkENI(logger logr.Logger, instance ec2.EC2Instance, helper api.EC2API
176176
ec2ApiHelper: helper,
177177
instance: instance,
178178
uidToBranchENIMap: make(map[string][]*ENIDetails),
179-
nodeNameTag: []ec2types.Tag{
179+
nodeIDTag: []ec2types.Tag{
180180
{
181181
Key: aws.String(config.NetworkInterfaceNodeIDKey),
182182
Value: aws.String(instance.InstanceID()),
@@ -237,7 +237,7 @@ func (t *trunkENI) InitTrunk(instance ec2.EC2Instance, podList []v1.Pod) error {
237237
}
238238

239239
trunk, err := t.ec2ApiHelper.CreateAndAttachNetworkInterface(&instanceID, aws.String(t.instance.SubnetID()),
240-
t.instance.CurrentInstanceSecurityGroups(), t.nodeNameTag, &freeIndex, &TrunkEniDescription, &InterfaceTypeTrunk, nil)
240+
t.instance.CurrentInstanceSecurityGroups(), t.nodeIDTag, &freeIndex, &TrunkEniDescription, &InterfaceTypeTrunk, nil)
241241
if err != nil {
242242
trunkENIOperationsErrCount.WithLabelValues("create_trunk_eni").Inc()
243243
return err
@@ -425,7 +425,7 @@ func (t *trunkENI) CreateAndAssociateBranchENIs(pod *v1.Pod, securityGroups []st
425425
},
426426
}
427427
// append the nodeName tag to add to branch ENIs
428-
tags = append(tags, t.nodeNameTag...)
428+
tags = append(tags, t.nodeIDTag...)
429429
// Create Branch ENI
430430
nwInterface, err = t.ec2ApiHelper.CreateNetworkInterface(&BranchEniDescription,
431431
aws.String(t.instance.SubnetID()), securityGroups, tags, nil, nil)

‎pkg/provider/branch/trunk/trunk_test.go‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -244,7 +244,7 @@ func getMockTrunk() trunkENI {
244244
log: log,
245245
usedVlanIds: make([]bool, MaxAllocatableVlanIds),
246246
uidToBranchENIMap: map[string][]*ENIDetails{},
247-
nodeNameTag: []awsEc2Types.Tag{
247+
nodeIDTag: []awsEc2Types.Tag{
248248
{
249249
Key: aws.String(config.NetworkInterfaceNodeIDKey),
250250
Value: aws.String(FakeInstance.InstanceID()),
@@ -695,7 +695,7 @@ func TestTrunkENI_InitTrunk(t *testing.T) {
695695
f.mockEC2APIHelper.EXPECT().GetInstanceNetworkInterface(&InstanceId).Return([]awsEc2Types.InstanceNetworkInterface{}, nil)
696696
f.mockInstance.EXPECT().GetHighestUnusedDeviceIndex().Return(freeIndex, nil)
697697
f.mockInstance.EXPECT().SubnetID().Return(SubnetId)
698-
f.mockEC2APIHelper.EXPECT().CreateAndAttachNetworkInterface(&InstanceId, &SubnetId, SecurityGroups, f.trunkENI.nodeNameTag,
698+
f.mockEC2APIHelper.EXPECT().CreateAndAttachNetworkInterface(&InstanceId, &SubnetId, SecurityGroups, f.trunkENI.nodeIDTag,
699699
&freeIndex, &TrunkEniDescription, &InterfaceTypeTrunk, nil).Return(trunkInterface, nil)
700700
},
701701
// Pass nil to set the instance to fields.mockInstance in the function later
@@ -842,9 +842,9 @@ func TestTrunkENI_CreateAndAssociateBranchENIs(t *testing.T) {
842842
mockInstance.EXPECT().SubnetV6CidrBlock().Return(SubnetV6CidrBlock).Times(2)
843843

844844
mockEC2APIHelper.EXPECT().CreateNetworkInterface(&BranchEniDescription, &SubnetId, SecurityGroups,
845-
append(vlan1Tag, trunkENI.nodeNameTag...), nil, nil).Return(BranchInterface1, nil)
845+
append(vlan1Tag, trunkENI.nodeIDTag...), nil, nil).Return(BranchInterface1, nil)
846846
mockEC2APIHelper.EXPECT().AssociateBranchToTrunk(&trunkId, &Branch1Id, VlanId1).Return(mockAssociationOutput1, nil)
847-
mockEC2APIHelper.EXPECT().CreateNetworkInterface(&BranchEniDescription, &SubnetId, SecurityGroups, append(vlan2Tag, trunkENI.nodeNameTag...),
847+
mockEC2APIHelper.EXPECT().CreateNetworkInterface(&BranchEniDescription, &SubnetId, SecurityGroups, append(vlan2Tag, trunkENI.nodeIDTag...),
848848
nil, nil).Return(BranchInterface2, nil)
849849
mockEC2APIHelper.EXPECT().AssociateBranchToTrunk(&trunkId, &Branch2Id, VlanId2).Return(mockAssociationOutput2, nil)
850850

@@ -876,10 +876,10 @@ func TestTrunkENI_CreateAndAssociateBranchENIs_InstanceSecurityGroup(t *testing.
876876
mockInstance.EXPECT().CurrentInstanceSecurityGroups().Return(InstanceSecurityGroup)
877877

878878
mockEC2APIHelper.EXPECT().CreateNetworkInterface(&BranchEniDescription, &SubnetId, InstanceSecurityGroup,
879-
append(vlan1Tag, trunkENI.nodeNameTag...), nil, nil).Return(BranchInterface1, nil)
879+
append(vlan1Tag, trunkENI.nodeIDTag...), nil, nil).Return(BranchInterface1, nil)
880880
mockEC2APIHelper.EXPECT().AssociateBranchToTrunk(&trunkId, &Branch1Id, VlanId1).Return(mockAssociationOutput1, nil)
881881
mockEC2APIHelper.EXPECT().CreateNetworkInterface(&BranchEniDescription, &SubnetId, InstanceSecurityGroup,
882-
append(vlan2Tag, trunkENI.nodeNameTag...), nil, nil).Return(BranchInterface2, nil)
882+
append(vlan2Tag, trunkENI.nodeIDTag...), nil, nil).Return(BranchInterface2, nil)
883883
mockEC2APIHelper.EXPECT().AssociateBranchToTrunk(&trunkId, &Branch2Id, VlanId2).Return(mockAssociationOutput2, nil)
884884

885885
eniDetails, err := trunkENI.CreateAndAssociateBranchENIs(MockPod2, []string{}, 2)
@@ -910,10 +910,10 @@ func TestTrunkENI_CreateAndAssociateBranchENIs_ErrorAssociate(t *testing.T) {
910910

911911
gomock.InOrder(
912912
mockEC2APIHelper.EXPECT().CreateNetworkInterface(&BranchEniDescription, &SubnetId, SecurityGroups,
913-
append(vlan1Tag, trunkENI.nodeNameTag...), nil, nil).Return(BranchInterface1, nil),
913+
append(vlan1Tag, trunkENI.nodeIDTag...), nil, nil).Return(BranchInterface1, nil),
914914
mockEC2APIHelper.EXPECT().AssociateBranchToTrunk(&trunkId, &Branch1Id, VlanId1).Return(mockAssociationOutput1, nil),
915915
mockEC2APIHelper.EXPECT().CreateNetworkInterface(&BranchEniDescription, &SubnetId, SecurityGroups,
916-
append(vlan2Tag, trunkENI.nodeNameTag...), nil, nil).Return(BranchInterface2, nil),
916+
append(vlan2Tag, trunkENI.nodeIDTag...), nil, nil).Return(BranchInterface2, nil),
917917
mockEC2APIHelper.EXPECT().AssociateBranchToTrunk(&trunkId, &Branch2Id, VlanId2).Return(nil, MockError),
918918
)
919919

@@ -937,10 +937,10 @@ func TestTrunkENI_CreateAndAssociateBranchENIs_ErrorCreate(t *testing.T) {
937937
mockInstance.EXPECT().SubnetV6CidrBlock().Return(SubnetV6CidrBlock).Times(1)
938938

939939
gomock.InOrder(
940-
mockEC2APIHelper.EXPECT().CreateNetworkInterface(&BranchEniDescription, &SubnetId, SecurityGroups, append(vlan1Tag, trunkENI.nodeNameTag...),
940+
mockEC2APIHelper.EXPECT().CreateNetworkInterface(&BranchEniDescription, &SubnetId, SecurityGroups, append(vlan1Tag, trunkENI.nodeIDTag...),
941941
nil, nil).Return(BranchInterface1, nil),
942942
mockEC2APIHelper.EXPECT().AssociateBranchToTrunk(&trunkId, &Branch1Id, VlanId1).Return(mockAssociationOutput1, nil),
943-
mockEC2APIHelper.EXPECT().CreateNetworkInterface(&BranchEniDescription, &SubnetId, SecurityGroups, append(vlan2Tag, trunkENI.nodeNameTag...),
943+
mockEC2APIHelper.EXPECT().CreateNetworkInterface(&BranchEniDescription, &SubnetId, SecurityGroups, append(vlan2Tag, trunkENI.nodeIDTag...),
944944
nil, nil).Return(nil, MockError),
945945
)
946946

0 commit comments

Comments
 (0)