From 697c1be723f22b1f0adcf6fdf3cf3880bcd6c275 Mon Sep 17 00:00:00 2001 From: David Cheung Date: Thu, 21 Nov 2024 00:24:45 +0000 Subject: [PATCH 1/4] Expand NEG GC with isPartialDeletion method. * Existing NEG GC will always delete all GCE NEGs in NEG CR. With multi-subnet cluster enabled, we could do partial deletions, only on to-be-deleted NEGs. --- pkg/neg/manager.go | 43 ++++++++++++++++++++++++++++++++----------- 1 file changed, 32 insertions(+), 11 deletions(-) diff --git a/pkg/neg/manager.go b/pkg/neg/manager.go index 2cf72c8f98..99c432f55e 100644 --- a/pkg/neg/manager.go +++ b/pkg/neg/manager.go @@ -548,16 +548,21 @@ func (manager *syncerManager) garbageCollectNEG() error { return nil } +type negDeletionCandidate struct { + svcNegCR *negv1beta1.ServiceNetworkEndpointGroup + isPartialDelete bool +} + // garbageCollectNEGWithCRD uses the NEG CRs and the svcPortMap to determine which NEGs // need to be garbage collected. Neg CRs that do not have a configuration in the svcPortMap will deleted // along with all corresponding NEGs in the CR's list of NetworkEndpointGroups. If NEG deletion fails in // the cloud, the corresponding Neg CR will not be deleted func (manager *syncerManager) garbageCollectNEGWithCRD() error { - deletionCandidates := map[string]*negv1beta1.ServiceNetworkEndpointGroup{} + deletionCandidates := make(map[string]negDeletionCandidate) negCRs := manager.svcNegLister.List() for _, obj := range negCRs { neg := obj.(*negv1beta1.ServiceNetworkEndpointGroup) - deletionCandidates[neg.Name] = neg + deletionCandidates[neg.Name] = negDeletionCandidate{svcNegCR: neg, isPartialDelete: false} } func() { @@ -595,7 +600,7 @@ func (manager *syncerManager) garbageCollectNEGWithCRD() error { errList = append(errList, fmt.Errorf("failed to get zones during garbage collection: %w", err)) } - deletionCandidatesChan := make(chan *negv1beta1.ServiceNetworkEndpointGroup, len(deletionCandidates)) + deletionCandidatesChan := make(chan negDeletionCandidate, len(deletionCandidates)) for _, dc := range deletionCandidates { deletionCandidatesChan <- dc } @@ -605,8 +610,8 @@ func (manager *syncerManager) garbageCollectNEGWithCRD() error { wg.Add(len(deletionCandidates)) for i := 0; i < manager.numGCWorkers; i++ { go func() { - for svcNegCR := range deletionCandidatesChan { - errs := manager.processNEGDeletionCandidate(svcNegCR, zones) + for negToDelete := range deletionCandidatesChan { + errs := manager.processNEGDeletionCandidate(negToDelete, zones) errListMutex.Lock() errList = append(errList, errs...) @@ -625,7 +630,9 @@ func (manager *syncerManager) garbageCollectNEGWithCRD() error { // associated with it. In case when `svcNegCR` does not have ample information // about the zones associated with this NEG, it will attempt to delete the NEG // from all zones specified through the `zones` slice. -func (manager *syncerManager) processNEGDeletionCandidate(svcNegCR *negv1beta1.ServiceNetworkEndpointGroup, zones []string) []error { +func (manager *syncerManager) processNEGDeletionCandidate(negToDelete negDeletionCandidate, zones []string) []error { + svcNegCR := negToDelete.svcNegCR + manager.logger.V(2).Info("Count of NEGs referenced by SvcNegCR", "svcneg", klog.KObj(svcNegCR), "count", len(svcNegCR.Status.NetworkEndpointGroups)) var errList []error shouldDeleteNegCR := true @@ -635,6 +642,11 @@ func (manager *syncerManager) processNEGDeletionCandidate(svcNegCR *negv1beta1.S deletedNegs := make(map[negtypes.NegInfo]struct{}) for _, negRef := range svcNegCR.Status.NetworkEndpointGroups { + if flags.F.EnableMultiSubnetClusterPhase1 { + if negToDelete.isPartialDelete && negRef.State != negv1beta1.ToBeDeletedState { + continue + } + } resourceID, err := cloud.ParseResourceURL(negRef.SelfLink) if err != nil { errList = append(errList, fmt.Errorf("failed to parse selflink for neg cr %s/%s: %s", svcNegCR.Namespace, svcNegCR.Name, err)) @@ -648,14 +660,23 @@ func (manager *syncerManager) processNEGDeletionCandidate(svcNegCR *negv1beta1.S shouldDeleteNegCR = shouldDeleteNegCR && negDeleted } + // If there is no NEG ref in the NEG CR, our best attempt is to get all existing subnets + // and delete NEGs in these subnets in a full service deletion. + // We will skip in the case of partial deletion. We cannot reconstruct the name of + // the NEG that needs to be deleted since that subnet is no longer available in + // Node Topology CR. if deleteByZone { manager.logger.V(2).Info("Deletion candidate has 0 NEG reference", "svcneg", klog.KObj(svcNegCR), "svcNegCR", svcNegCR) - for _, zone := range zones { - negDeleted := manager.deleteNegOrReportErr(svcNegCR.Name, zone, svcNegCR, &errList) - if negDeleted { - deletedNegs[negtypes.NegInfo{Name: svcNegCR.Name, Zone: zone}] = struct{}{} + // When EnableMultiSubnetClusterPhase1=false, we should always process deletion. + // Otherwise, we should only process full deletion. + if !flags.F.EnableMultiSubnetClusterPhase1 || !negToDelete.isPartialDelete { + for _, zone := range zones { + negDeleted := manager.deleteNegOrReportErr(svcNegCR.Name, zone, svcNegCR, &errList) + if negDeleted { + deletedNegs[negtypes.NegInfo{Name: svcNegCR.Name, Zone: zone}] = struct{}{} + } + shouldDeleteNegCR = shouldDeleteNegCR && negDeleted } - shouldDeleteNegCR = shouldDeleteNegCR && negDeleted } } // Since no more NEG deletion will be happening at this point, and NEG From 376c7890d9e2ef3c0478df6d962641c34ae13b29 Mon Sep 17 00:00:00 2001 From: David Cheung Date: Thu, 21 Nov 2024 02:11:05 +0000 Subject: [PATCH 2/4] NEG GC should consider a SvcNeg as deletion candidate if it contains tbd NEGs. --- pkg/neg/manager.go | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/pkg/neg/manager.go b/pkg/neg/manager.go index 99c432f55e..fc18dd7f1c 100644 --- a/pkg/neg/manager.go +++ b/pkg/neg/manager.go @@ -577,6 +577,13 @@ func (manager *syncerManager) garbageCollectNEGWithCRD() error { // neither the neg nor CR will not be deleted. In the situation a neg config is not in the svcPortMap, // but the CR does not have a deletion timestamp, both CR and neg will be deleted. if _, ok := deletionCandidates[portInfo.NegName]; ok { + // When the service still exists, we won't do GC unless it contains + // to-be-deleted NEGs. + svcNegCR := deletionCandidates[portInfo.NegName].svcNegCR + if flags.F.EnableMultiSubnetClusterPhase1 && containsTBDNeg(svcNegCR) { + deletionCandidates[portInfo.NegName] = negDeletionCandidate{svcNegCR: svcNegCR, isPartialDelete: true} + continue + } delete(deletionCandidates, portInfo.NegName) } } @@ -626,6 +633,16 @@ func (manager *syncerManager) garbageCollectNEGWithCRD() error { return utilerrors.NewAggregate(errList) } +// containsTBDNeg returns if the given NEG CR contains NEG refs in +// to-be-deleted state. +func containsTBDNeg(svcNegCR *negv1beta1.ServiceNetworkEndpointGroup) bool { + contains := false + for _, negRef := range svcNegCR.Status.NetworkEndpointGroups { + contains = contains || negRef.State == negv1beta1.ToBeDeletedState + } + return contains +} + // processNEGDeletionCandidate attempts to delete `svcNegCR` and all NEGs // associated with it. In case when `svcNegCR` does not have ample information // about the zones associated with this NEG, it will attempt to delete the NEG From ed1037f4936e1caa09bb763b4503152ea44a776e Mon Sep 17 00:00:00 2001 From: David Cheung Date: Mon, 25 Nov 2024 19:48:28 +0000 Subject: [PATCH 3/4] Refactor TestGarbageCollectionNegCrdEnabled --- pkg/neg/manager_test.go | 32 +++++++++++++------------------- 1 file changed, 13 insertions(+), 19 deletions(-) diff --git a/pkg/neg/manager_test.go b/pkg/neg/manager_test.go index 1cdd3d0d06..dcee437a44 100644 --- a/pkg/neg/manager_test.go +++ b/pkg/neg/manager_test.go @@ -29,6 +29,7 @@ import ( "google.golang.org/api/googleapi" "k8s.io/cloud-provider-gcp/providers/gce" "k8s.io/ingress-gce/pkg/composite" + "k8s.io/ingress-gce/pkg/flags" "k8s.io/ingress-gce/pkg/utils" "k8s.io/klog/v2" @@ -1315,6 +1316,7 @@ func TestGarbageCollectionNegCrdEnabled(t *testing.T) { desiredConfig: true, expectNegGC: false, expectCrGC: false, + negDesc: matchingDesc.String(), expectedNegCount: 2, }, {desc: "negs don't exist, config not in svcPortMap", @@ -1415,7 +1417,7 @@ func TestGarbageCollectionNegCrdEnabled(t *testing.T) { if !tc.emptyNegRefList { if !tc.malformedNegSelflink { - cr.Status.NetworkEndpointGroups = getNegObjectRefs(t, fakeNegCloud, zones, negName, version) + cr.Status.NetworkEndpointGroups = getNegObjectRefs(t, fakeNegCloud, zones, negName, version, negv1beta1.ActiveState) } else { cr.Status.NetworkEndpointGroups = []negv1beta1.NegObjectReference{{SelfLink: ""}} } @@ -1425,7 +1427,6 @@ func TestGarbageCollectionNegCrdEnabled(t *testing.T) { t.Fatalf("failed to create neg cr: %v", err) } - crs := getNegCRs(t, svcNegClient, testServiceNamespace) populateSvcNegCache(t, manager, svcNegClient, testServiceNamespace) if tc.desiredConfig { @@ -1465,7 +1466,7 @@ func TestGarbageCollectionNegCrdEnabled(t *testing.T) { t.Errorf("failed getting negs from cloud: %s", err) } - numExistingNegs := checkForNegDeletions(negs, negName) + numExistingNegs := len(negs) expectNegGC := tc.expectNegGC || (tc.expectGenNamedNegGC && !customName) if tc.negsExist && expectNegGC && numExistingNegs != 0 { @@ -1475,7 +1476,7 @@ func TestGarbageCollectionNegCrdEnabled(t *testing.T) { t.Errorf("expected %d negs in the cloud, but found %d", tc.expectedNegCount, numExistingNegs) } - crs = getNegCRs(t, svcNegClient, testServiceNamespace) + crs := getNegCRs(t, svcNegClient, testServiceNamespace) crDeleted := checkForNegCRDeletion(crs, negName) if tc.expectCrGC && !crDeleted { @@ -1754,7 +1755,7 @@ func (s *fakeSyncer) IsStopped() bool { return s.isStopped } func (s *fakeSyncer) IsShuttingDown() bool { return false } // getNegObjectRefs generates the NegObjectReference list of all negs with the specified negName in the specified zones -func getNegObjectRefs(t *testing.T, cloud negtypes.NetworkEndpointGroupCloud, zones []string, negName string, version meta.Version) []negv1beta1.NegObjectReference { +func getNegObjectRefs(t *testing.T, cloud negtypes.NetworkEndpointGroupCloud, zones []string, negName string, version meta.Version, negState negv1beta1.NegState) []negv1beta1.NegObjectReference { var negRefs []negv1beta1.NegObjectReference for _, zone := range zones { neg, err := cloud.GetNetworkEndpointGroup(negName, zone, version, klog.TODO()) @@ -1762,25 +1763,18 @@ func getNegObjectRefs(t *testing.T, cloud negtypes.NetworkEndpointGroupCloud, zo t.Errorf("failed to get neg %s, from zone %s", negName, zone) continue } - negRefs = append(negRefs, negv1beta1.NegObjectReference{ + negRef := negv1beta1.NegObjectReference{ Id: fmt.Sprint(neg.Id), SelfLink: neg.SelfLink, NetworkEndpointType: negv1beta1.NetworkEndpointType(neg.NetworkEndpointType), - }) - } - return negRefs -} - -// checkForNegDeletions gets the count of neg objects in negs that has the provided negName. -func checkForNegDeletions(negs map[*meta.Key]*composite.NetworkEndpointGroup, negName string) int { - foundNegs := 0 - for _, neg := range negs { - if neg.Name == negName { - foundNegs += 1 } + if flags.F.EnableMultiSubnetClusterPhase1 { + negRef.State = negState + negRef.SubnetURL = neg.Subnetwork + } + negRefs = append(negRefs, negRef) } - - return foundNegs + return negRefs } // checkForNegCRDeletion verifies that either no cr with name `negName` exists or a cr withe name `negName` has its deletion timestamp set From 7d86b8c7975e6eb6fabd77fd0b6efcfa27f08aa5 Mon Sep 17 00:00:00 2001 From: David Cheung Date: Tue, 26 Nov 2024 19:27:19 +0000 Subject: [PATCH 4/4] 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) + } } } }