From b2bf3e0b38407862729b140ba2fd2def5858d45a Mon Sep 17 00:00:00 2001 From: David Cheung Date: Tue, 26 Nov 2024 19:27:19 +0000 Subject: [PATCH] Add tests for GC to-be-deleted NEGs. --- pkg/neg/manager_test.go | 230 ++++++++++++++++++++++++---------------- 1 file changed, 137 insertions(+), 93 deletions(-) diff --git a/pkg/neg/manager_test.go b/pkg/neg/manager_test.go index dcee437a44..c3b055f074 100644 --- a/pkg/neg/manager_test.go +++ b/pkg/neg/manager_test.go @@ -83,6 +83,7 @@ const ( negName1 = "neg1" defaultTestSubnet = "default" + additionalTestSubnet = "additional-subnet" defaultTestSubnetURL = "https://www.googleapis.com/compute/v1/projects/mock-project/regions/test-region/subnetworks/default" ) @@ -1186,7 +1187,10 @@ func TestNegCRDeletions(t *testing.T) { } func TestGarbageCollectionNegCrdEnabled(t *testing.T) { - t.Parallel() + prevFlag := flags.F.EnableMultiSubnetClusterPhase1 + defer func() { + flags.F.EnableMultiSubnetClusterPhase1 = prevFlag + }() svc := &v1.Service{ TypeMeta: metav1.TypeMeta{ @@ -1234,7 +1238,8 @@ func TestGarbageCollectionNegCrdEnabled(t *testing.T) { // expectGenNamedNegGC indicates that the Neg GC only occurs if using a generated name // expectNegGC will take precedence over this value - expectGenNamedNegGC bool + expectGenNamedNegGC bool + includeToBeDeletedNeg bool }{ {desc: "neg config not in svcPortMap, marked for deletion", negsExist: true, @@ -1319,6 +1324,16 @@ func TestGarbageCollectionNegCrdEnabled(t *testing.T) { negDesc: matchingDesc.String(), expectedNegCount: 2, }, + {desc: "neg config in svcPortMap, include to-be-deleted NEGs", + negsExist: true, + markedForDeletion: false, + desiredConfig: true, + includeToBeDeletedNeg: true, + expectNegGC: false, + expectCrGC: false, + expectedNegCount: 2, // We should expect NEG to be deleted but not GC NEG CR + negDesc: matchingDesc.String(), + }, {desc: "negs don't exist, config not in svcPortMap", negsExist: false, markedForDeletion: false, @@ -1371,119 +1386,148 @@ func TestGarbageCollectionNegCrdEnabled(t *testing.T) { for _, tc := range testCases { t.Run(tc.desc, func(t *testing.T) { - for _, customName := range []bool{true, false} { - for _, networkEndpointType := range []negtypes.NetworkEndpointType{negtypes.VmIpPortEndpointType, negtypes.NonGCPPrivateEndpointType, negtypes.VmIpEndpointType} { - - kubeClient := fake.NewSimpleClientset() - manager, testCloud, _ := NewTestSyncerManager(kubeClient) - svcNegClient := manager.svcNegClient - - if tc.negCrGCError != nil { - svcNegClientFake := svcNegClient.(*negfake.Clientset) - svcNegClientFake.PrependReactor("delete", "servicenetworkendpointgroups", func(action k8stesting.Action) (handled bool, ret runtime.Object, err error) { - return true, &negv1beta1.ServiceNetworkEndpointGroup{}, tc.negCrGCError - }) - } + for _, enableMultiSubnetClusterPhase1 := range []bool{true, false} { + if !enableMultiSubnetClusterPhase1 && tc.includeToBeDeletedNeg { + continue + } + flags.F.EnableMultiSubnetClusterPhase1 = enableMultiSubnetClusterPhase1 + for _, customName := range []bool{true, false} { + for _, networkEndpointType := range []negtypes.NetworkEndpointType{negtypes.VmIpPortEndpointType, negtypes.NonGCPPrivateEndpointType, negtypes.VmIpEndpointType} { + + kubeClient := fake.NewSimpleClientset() + manager, testCloud, _ := NewTestSyncerManager(kubeClient) + svcNegClient := manager.svcNegClient + + if tc.negCrGCError != nil { + svcNegClientFake := svcNegClient.(*negfake.Clientset) + svcNegClientFake.PrependReactor("delete", "servicenetworkendpointgroups", func(action k8stesting.Action) (handled bool, ret runtime.Object, err error) { + return true, &negv1beta1.ServiceNetworkEndpointGroup{}, tc.negCrGCError + }) + } - manager.serviceLister.Add(svc) - fakeNegCloud := manager.cloud + manager.serviceLister.Add(svc) + fakeNegCloud := manager.cloud - version := meta.VersionGA - if networkEndpointType == negtypes.VmIpEndpointType { - version = meta.VersionAlpha - } + version := meta.VersionGA + if networkEndpointType == negtypes.VmIpEndpointType { + version = meta.VersionAlpha + } - // Create NEG to be GC'ed - negName := manager.namer.NEG("test", "test", port80) - if customName { - negName = "test" - } + // Create NEG to be GC'ed + negName := manager.namer.NEG("test", "test", port80) + nonDefaultNegName := manager.namer.NonDefaultSubnetNEG("test", "test", additionalTestSubnet, port80) + + if customName { + negName = "test" + var err error + nonDefaultNegName, err = manager.namer.NonDefaultSubnetCustomNEG("test", additionalTestSubnet) + if err != nil { + t.Fatalf("Failed to generate custom name for non-default subnet NEG: %v", err) + } + } - for _, zone := range zones { - fakeNegCloud.CreateNetworkEndpointGroup(&composite.NetworkEndpointGroup{ - Version: version, - Name: negName, - NetworkEndpointType: string(networkEndpointType), - Description: tc.negDesc, - }, zone, klog.TODO()) - } + for _, zone := range zones { + if err := fakeNegCloud.CreateNetworkEndpointGroup(&composite.NetworkEndpointGroup{ + Version: version, + Name: negName, + NetworkEndpointType: string(networkEndpointType), + Description: tc.negDesc, + }, zone, klog.TODO()); err != nil { + t.Fatalf("Failed to create NEG %s in zone %s: %v", negName, zone, err) + } + + if tc.includeToBeDeletedNeg { + if err := fakeNegCloud.CreateNetworkEndpointGroup(&composite.NetworkEndpointGroup{ + Version: version, + Name: nonDefaultNegName, + NetworkEndpointType: string(networkEndpointType), + Description: tc.negDesc, + }, zone, klog.TODO()); err != nil { + t.Fatalf("Failed to create NEG %s in zone %s: %v", nonDefaultNegName, zone, err) + } + } + } - gcPortInfo := negtypes.PortInfo{PortTuple: negtypes.SvcPortTuple{Port: port80}, NegName: negName} - cr := createNegCR(svc, serviceKey{namespace: testServiceNamespace, name: testServiceName}, gcPortInfo) - if tc.markedForDeletion { - now := metav1.Now() - cr.SetDeletionTimestamp(&now) - } + gcPortInfo := negtypes.PortInfo{PortTuple: negtypes.SvcPortTuple{Port: port80}, NegName: negName} + cr := createNegCR(svc, serviceKey{namespace: testServiceNamespace, name: testServiceName}, gcPortInfo) + if tc.markedForDeletion { + now := metav1.Now() + cr.SetDeletionTimestamp(&now) + } - if !tc.emptyNegRefList { - if !tc.malformedNegSelflink { - cr.Status.NetworkEndpointGroups = getNegObjectRefs(t, fakeNegCloud, zones, negName, version, negv1beta1.ActiveState) - } else { - cr.Status.NetworkEndpointGroups = []negv1beta1.NegObjectReference{{SelfLink: ""}} + if !tc.emptyNegRefList { + if !tc.malformedNegSelflink { + cr.Status.NetworkEndpointGroups = getNegObjectRefs(t, fakeNegCloud, zones, negName, version, negv1beta1.ActiveState) + if tc.includeToBeDeletedNeg { + cr.Status.NetworkEndpointGroups = append(cr.Status.NetworkEndpointGroups, getNegObjectRefs(t, fakeNegCloud, zones, nonDefaultNegName, version, negv1beta1.ToBeDeletedState)...) + } + } else { + cr.Status.NetworkEndpointGroups = []negv1beta1.NegObjectReference{{SelfLink: ""}} + } } - } - if _, err := manager.svcNegClient.NetworkingV1beta1().ServiceNetworkEndpointGroups(cr.Namespace).Create(context2.TODO(), &cr, metav1.CreateOptions{}); err != nil { - t.Fatalf("failed to create neg cr: %v", err) - } + if _, err := manager.svcNegClient.NetworkingV1beta1().ServiceNetworkEndpointGroups(cr.Namespace).Create(context2.TODO(), &cr, metav1.CreateOptions{}); err != nil { + t.Fatalf("failed to create neg cr: %v", err) + } - populateSvcNegCache(t, manager, svcNegClient, testServiceNamespace) + populateSvcNegCache(t, manager, svcNegClient, testServiceNamespace) - if tc.desiredConfig { - // Add config to svc map - serviceKey := getServiceKey(testServiceNamespace, testServiceName) - ports := make(negtypes.PortInfoMap) - ports[negtypes.PortInfoMapKey{ServicePort: port80}] = gcPortInfo + if tc.desiredConfig { + // Add config to svc map + serviceKey := getServiceKey(testServiceNamespace, testServiceName) + ports := make(negtypes.PortInfoMap) + ports[negtypes.PortInfoMapKey{ServicePort: port80}] = gcPortInfo - manager.svcPortMap[serviceKey] = ports - } + manager.svcPortMap[serviceKey] = ports + } - if !tc.negsExist { - for _, zone := range []string{negtypes.TestZone1, negtypes.TestZone2} { - fakeNegCloud.DeleteNetworkEndpointGroup(negName, zone, version, klog.TODO()) + if !tc.negsExist { + for _, zone := range []string{negtypes.TestZone1, negtypes.TestZone2} { + fakeNegCloud.DeleteNetworkEndpointGroup(negName, zone, version, klog.TODO()) + } } - } - if tc.negGCError != nil { - mockCloud := testCloud.Compute().(*cloud.MockGCE) - mockNEG := mockCloud.NetworkEndpointGroups().(*cloud.MockNetworkEndpointGroups) + if tc.negGCError != nil { + mockCloud := testCloud.Compute().(*cloud.MockGCE) + mockNEG := mockCloud.NetworkEndpointGroups().(*cloud.MockNetworkEndpointGroups) - for _, zone := range tc.negGCErrorZone { - mockNEG.DeleteError[*meta.ZonalKey(negName, zone)] = tc.negGCError + for _, zone := range tc.negGCErrorZone { + mockNEG.DeleteError[*meta.ZonalKey(negName, zone)] = tc.negGCError + } } - } - err := manager.GC() - if !tc.expectErr && err != nil { - t.Fatalf("failed to GC: %v", err) - } - if tc.expectErr && err == nil { - t.Errorf("expected GC to error") - } + err := manager.GC() + if !tc.expectErr && err != nil { + t.Fatalf("failed to GC: %v", err) + } + if tc.expectErr && err == nil { + t.Errorf("expected GC to error") + } - negs, err := fakeNegCloud.AggregatedListNetworkEndpointGroup(version, klog.TODO()) - if err != nil { - t.Errorf("failed getting negs from cloud: %s", err) - } + negs, err := fakeNegCloud.AggregatedListNetworkEndpointGroup(version, klog.TODO()) + if err != nil { + t.Errorf("failed getting negs from cloud: %s", err) + } - numExistingNegs := len(negs) + numExistingNegs := len(negs) - expectNegGC := tc.expectNegGC || (tc.expectGenNamedNegGC && !customName) - if tc.negsExist && expectNegGC && numExistingNegs != 0 { - t.Errorf("expected negs to be GCed, but found %d", numExistingNegs) - } - if tc.negsExist && !expectNegGC && numExistingNegs != tc.expectedNegCount { - t.Errorf("expected %d negs in the cloud, but found %d", tc.expectedNegCount, numExistingNegs) - } + expectNegGC := tc.expectNegGC || (tc.expectGenNamedNegGC && !customName) + if tc.negsExist && expectNegGC && numExistingNegs != 0 { + t.Errorf("expected negs to be GCed, but found %d", numExistingNegs) + } + if tc.negsExist && !expectNegGC && numExistingNegs != tc.expectedNegCount { + t.Errorf("expected %d negs in the cloud, but found %d", tc.expectedNegCount, numExistingNegs) + } - crs := getNegCRs(t, svcNegClient, testServiceNamespace) - crDeleted := checkForNegCRDeletion(crs, negName) + crs := getNegCRs(t, svcNegClient, testServiceNamespace) + crDeleted := checkForNegCRDeletion(crs, negName) - if tc.expectCrGC && !crDeleted { - t.Errorf("expected neg %s to be deleted", negName) - } - if !tc.expectCrGC && crDeleted && !tc.markedForDeletion { - t.Errorf("expected neg %s to not be deleted", negName) + if tc.expectCrGC && !crDeleted { + t.Errorf("expected neg %s to be deleted", negName) + } + if !tc.expectCrGC && crDeleted && !tc.markedForDeletion { + t.Errorf("expected neg %s to not be deleted", negName) + } } } }