From a3dd0a8b8f97ab910abc8a1d2a54401681ba4ecd Mon Sep 17 00:00:00 2001 From: David Cheung Date: Wed, 7 Dec 2022 08:26:10 +0000 Subject: [PATCH] Clean up error state checking Consolidate EndpointSlice nodeName and zone check, and flip checking logic for better readability. Return error if nodeName or zone is missing when CalculateEndpoints, and check for error state based on the returned error. Add check for case when nodeName is set to be empty string. Flip invalid check to valid check. --- pkg/neg/syncers/transaction.go | 11 +- pkg/neg/syncers/transaction_test.go | 285 +++++++++++++++++++++++----- pkg/neg/syncers/utils.go | 13 +- 3 files changed, 250 insertions(+), 59 deletions(-) diff --git a/pkg/neg/syncers/transaction.go b/pkg/neg/syncers/transaction.go index 219230a8d6..17b0e36468 100644 --- a/pkg/neg/syncers/transaction.go +++ b/pkg/neg/syncers/transaction.go @@ -18,6 +18,7 @@ package syncers import ( "context" + "errors" "fmt" "net/http" "strings" @@ -244,7 +245,7 @@ func (s *transactionSyncer) syncInternalImpl() error { } endpointsData := negtypes.EndpointsDataFromEndpointSlices(endpointSlices) targetMap, endpointPodMap, dupCount, err = s.endpointsCalculator.CalculateEndpoints(endpointsData, currentMap) - if s.invalidEndpointInfo(endpointsData, endpointPodMap, dupCount) || s.isZoneMissing(targetMap) { + if s.isZoneMissing(err) || s.invalidEndpointInfo(endpointsData, endpointPodMap, dupCount) { s.setErrorState() } if err != nil { @@ -398,10 +399,10 @@ func (s *transactionSyncer) invalidEndpointInfo(eds []negtypes.EndpointsData, en return false } -// isZoneMissing returns true if there is invalid(empty) zone in zoneNetworkEndpointMap -func (s *transactionSyncer) isZoneMissing(zoneNetworkEndpointMap map[string]negtypes.NetworkEndpointSet) bool { - if _, isPresent := zoneNetworkEndpointMap[""]; isPresent { - s.logger.Info("Detected error when checking missing zone", "zoneNetworkEndpointMap", zoneNetworkEndpointMap) +// isZoneMissing returns true if the error returned from CalculateEndpoint is ErrEPMissingZone +func (s *transactionSyncer) isZoneMissing(err error) bool { + if errors.Is(err, ErrEPMissingZone) { + s.logger.Info("Detected unexpected error when checking missing zone", "error", err) return true } return false diff --git a/pkg/neg/syncers/transaction_test.go b/pkg/neg/syncers/transaction_test.go index 47b686182b..12dc022a7d 100644 --- a/pkg/neg/syncers/transaction_test.go +++ b/pkg/neg/syncers/transaction_test.go @@ -1419,7 +1419,7 @@ func TestInvalidEndpointInfo(t *testing.T) { instance1 := testInstance1 instance2 := testInstance2 instance4 := testInstance4 - testPortName := "port1" + testPortName := "" testServiceNamespace := "namespace" port80 := int32(80) port81 := int32(81) @@ -1431,6 +1431,7 @@ func TestInvalidEndpointInfo(t *testing.T) { testPodName2 := "pod2" testPodName3 := "pod3" testPodName4 := "pod4" + emptyNodeName := "" testEndpointPodMap := map[negtypes.NetworkEndpoint]types.NamespacedName{ { @@ -1835,7 +1836,7 @@ func TestInvalidEndpointInfo(t *testing.T) { expect: false, }, { - desc: "at least one endpoint is missing a nodeName", + desc: "at least one endpoint has nil nodeName ", endpointsData: []negtypes.EndpointsData{ { Meta: &metav1.ObjectMeta{ @@ -1906,6 +1907,78 @@ func TestInvalidEndpointInfo(t *testing.T) { dupCount: 0, expect: true, }, + { + desc: "at least one endpoint has empty nodeName", + endpointsData: []negtypes.EndpointsData{ + { + Meta: &metav1.ObjectMeta{ + Name: testServiceName + "-1", + Namespace: testServiceNamespace, + }, + Ports: []negtypes.PortData{ + { + Name: testPortName, + Port: port80, + }, + }, + Addresses: []negtypes.AddressData{ + { + TargetRef: &corev1.ObjectReference{ + Namespace: testServiceNamespace, + Name: testPodName1, + }, + NodeName: &emptyNodeName, + Addresses: []string{testIP1}, + Ready: true, + }, + { + TargetRef: &corev1.ObjectReference{ + Namespace: testServiceNamespace, + Name: testPodName2, + }, + NodeName: &instance1, + Addresses: []string{testIP2}, + Ready: true, + }, + }, + }, + { + Meta: &metav1.ObjectMeta{ + Name: testServiceName + "-2", + Namespace: testServiceNamespace, + }, + Ports: []negtypes.PortData{ + { + Name: testPortName, + Port: port81, + }, + }, + Addresses: []negtypes.AddressData{ + { + TargetRef: &corev1.ObjectReference{ + Namespace: testServiceNamespace, + Name: testPodName3, + }, + NodeName: &instance2, + Addresses: []string{testIP3}, + Ready: true, + }, + { + TargetRef: &corev1.ObjectReference{ + Namespace: testServiceNamespace, + Name: testPodName4, + }, + NodeName: &instance4, + Addresses: []string{testIP4}, + Ready: true, + }, + }, + }, + }, + endpointPodMap: testEndpointPodMap, + dupCount: 0, + expect: false, + }, { desc: "endpointData has zero endpoint", endpointsData: []negtypes.EndpointsData{ @@ -2132,72 +2205,179 @@ func TestInvalidEndpointInfo(t *testing.T) { func TestIsZoneMissing(t *testing.T) { t.Parallel() _, transactionSyncer := newTestTransactionSyncer(negtypes.NewAdapter(gce.NewFakeGCECloud(gce.DefaultTestClusterValues())), negtypes.VmIpPortEndpointType, false, true) + instance1 := testInstance1 instance2 := testInstance2 - - zone1 := "us-central1-a" - zone2 := "us-central1-b" + instance4 := testInstance4 + testPortName := "" + testServiceNamespace := "namespace" + port80 := int32(80) + port81 := int32(81) + testIP1 := "10.100.1.1" + testIP2 := "10.100.1.2" + testIP3 := "10.100.2.2" + testIP4 := "10.100.4.1" + testPodName1 := "pod1" + testPodName2 := "pod2" + testPodName3 := "pod3" + testPodName4 := "pod4" + emptyZoneNodeName := "empty-zone-node" + fakeZoneGetter := transactionSyncer.zoneGetter.(*negtypes.FakeZoneGetter) + if err := fakeZoneGetter.AddZone("", emptyZoneNodeName); err != nil { + t.Errorf("error when adding zone, expected nil, err: %v", err) + } testCases := []struct { - desc string - zoneNetworkEndpointMap map[string]negtypes.NetworkEndpointSet - expect bool + desc string + endpointsData []negtypes.EndpointsData + expect bool }{ { desc: "no missing zones", - zoneNetworkEndpointMap: map[string]negtypes.NetworkEndpointSet{ - zone1: negtypes.NewNetworkEndpointSet( - negtypes.NetworkEndpoint{ - IP: "10.100.1.2", - Port: "80", - Node: instance1, - }, - negtypes.NetworkEndpoint{ - IP: "10.100.1.3", - Port: "80", - Node: instance1, - }, - ), - zone2: negtypes.NewNetworkEndpointSet( - negtypes.NetworkEndpoint{ - IP: "10.100.2.2", - Port: "81", - Node: instance2, - }, - ), + endpointsData: []negtypes.EndpointsData{ + { + Meta: &metav1.ObjectMeta{ + Name: testServiceName + "-1", + Namespace: testServiceNamespace, + }, + Ports: []negtypes.PortData{ + { + Name: testPortName, + Port: port80, + }, + }, + Addresses: []negtypes.AddressData{ + { + TargetRef: &corev1.ObjectReference{ + Namespace: testServiceNamespace, + Name: testPodName1, + }, + NodeName: &instance1, + Addresses: []string{testIP1}, + Ready: true, + }, + { + TargetRef: &corev1.ObjectReference{ + Namespace: testServiceNamespace, + Name: testPodName2, + }, + NodeName: &instance1, + Addresses: []string{testIP2}, + Ready: true, + }, + }, + }, + { + Meta: &metav1.ObjectMeta{ + Name: testServiceName + "-2", + Namespace: testServiceNamespace, + }, + Ports: []negtypes.PortData{ + { + Name: testPortName, + Port: port81, + }, + }, + Addresses: []negtypes.AddressData{ + { + TargetRef: &corev1.ObjectReference{ + Namespace: testServiceNamespace, + Name: testPodName3, + }, + NodeName: &instance2, + Addresses: []string{testIP3}, + Ready: true, + }, + { + TargetRef: &corev1.ObjectReference{ + Namespace: testServiceNamespace, + Name: testPodName4, + }, + NodeName: &instance4, + Addresses: []string{testIP4}, + Ready: true, + }, + }, + }, }, expect: false, }, { desc: "contains one missing zone", - zoneNetworkEndpointMap: map[string]negtypes.NetworkEndpointSet{ - zone1: negtypes.NewNetworkEndpointSet( - negtypes.NetworkEndpoint{ - IP: "10.100.1.2", - Port: "80", - Node: instance1, - }, - negtypes.NetworkEndpoint{ - IP: "10.100.1.3", - Port: "80", - Node: instance1, - }, - ), - "": negtypes.NewNetworkEndpointSet( - negtypes.NetworkEndpoint{ - IP: "10.100.2.2", - Port: "81", - Node: instance2, - }, - ), + endpointsData: []negtypes.EndpointsData{ + { + Meta: &metav1.ObjectMeta{ + Name: testServiceName + "-1", + Namespace: testServiceNamespace, + }, + Ports: []negtypes.PortData{ + { + Name: testPortName, + Port: port80, + }, + }, + Addresses: []negtypes.AddressData{ + { + TargetRef: &corev1.ObjectReference{ + Namespace: testServiceNamespace, + Name: testPodName1, + }, + NodeName: &emptyZoneNodeName, + Addresses: []string{testIP1}, + Ready: true, + }, + { + TargetRef: &corev1.ObjectReference{ + Namespace: testServiceNamespace, + Name: testPodName2, + }, + NodeName: &instance1, + Addresses: []string{testIP2}, + Ready: true, + }, + }, + }, + { + Meta: &metav1.ObjectMeta{ + Name: testServiceName + "-2", + Namespace: testServiceNamespace, + }, + Ports: []negtypes.PortData{ + { + Name: testPortName, + Port: port81, + }, + }, + Addresses: []negtypes.AddressData{ + { + TargetRef: &corev1.ObjectReference{ + Namespace: testServiceNamespace, + Name: testPodName3, + }, + NodeName: &instance2, + Addresses: []string{testIP3}, + Ready: true, + }, + { + TargetRef: &corev1.ObjectReference{ + Namespace: testServiceNamespace, + Name: testPodName4, + }, + NodeName: &instance4, + Addresses: []string{testIP4}, + Ready: true, + }, + }, + }, }, expect: true, }, } for _, tc := range testCases { t.Run(tc.desc, func(t *testing.T) { - if got := transactionSyncer.isZoneMissing(tc.zoneNetworkEndpointMap); got != tc.expect { - t.Errorf("isZoneMissing() = %t, expected %t", got, tc.expect) + _, _, _, err := transactionSyncer.endpointsCalculator.CalculateEndpoints(tc.endpointsData, nil) + if got := transactionSyncer.isZoneMissing(err); got != tc.expect { + t.Errorf("isZoneMissing() = %t, expected %t, err: %v, ", got, tc.expect, err) } }) } @@ -2275,11 +2455,12 @@ func newTestTransactionSyncer(fakeGCE negtypes.NetworkEndpointGroupCloud, negTyp // TODO(freehan): use real readiness reflector reflector := &readiness.NoopReflector{} + fakeZoneGetter := negtypes.NewFakeZoneGetter() negsyncer := NewTransactionSyncer(svcPort, record.NewFakeRecorder(100), fakeGCE, - negtypes.NewFakeZoneGetter(), + fakeZoneGetter, testContext.PodInformer.GetIndexer(), testContext.ServiceInformer.GetIndexer(), testContext.EndpointInformer.GetIndexer(), @@ -2287,7 +2468,7 @@ func newTestTransactionSyncer(fakeGCE negtypes.NetworkEndpointGroupCloud, negTyp testContext.NodeInformer.GetIndexer(), testContext.SvcNegInformer.GetIndexer(), reflector, - GetEndpointsCalculator(testContext.NodeInformer.GetIndexer(), testContext.PodInformer.GetIndexer(), negtypes.NewFakeZoneGetter(), + GetEndpointsCalculator(testContext.NodeInformer.GetIndexer(), testContext.PodInformer.GetIndexer(), fakeZoneGetter, svcPort, mode, klog.TODO()), string(kubeSystemUID), testContext.SvcNegClient, diff --git a/pkg/neg/syncers/utils.go b/pkg/neg/syncers/utils.go index a077a26c27..5c3783b3dd 100644 --- a/pkg/neg/syncers/utils.go +++ b/pkg/neg/syncers/utils.go @@ -17,6 +17,7 @@ limitations under the License. package syncers import ( + "errors" "fmt" "strconv" "strings" @@ -47,6 +48,11 @@ const ( separator = "||" ) +var ( + ErrNodeNotFound = errors.New("failed to retrieve associated zone of node") + ErrEPMissingZone = errors.New("endpoint has empty zone field") +) + // encodeEndpoint encodes ip and instance into a single string func encodeEndpoint(ip, instance, port string) string { return strings.Join([]string{ip, instance, port}, separator) @@ -257,7 +263,7 @@ func toZoneNetworkEndpointMap(eds []negtypes.EndpointsData, zoneGetter negtypes. continue } } - if endpointAddress.NodeName == nil { + if endpointAddress.NodeName == nil || len(*endpointAddress.NodeName) == 0 { klog.V(2).Infof("Endpoint %q in Endpoints %s/%s does not have an associated node. Skipping", endpointAddress.Addresses, ed.Meta.Namespace, ed.Meta.Name) continue } @@ -267,7 +273,10 @@ func toZoneNetworkEndpointMap(eds []negtypes.EndpointsData, zoneGetter negtypes. } zone, err := zoneGetter.GetZoneForNode(*endpointAddress.NodeName) if err != nil { - return nil, nil, dupCount, fmt.Errorf("failed to retrieve associated zone of node %q: %w", *endpointAddress.NodeName, err) + return nil, nil, dupCount, ErrNodeNotFound + } + if zone == "" { + return nil, nil, dupCount, ErrEPMissingZone } if zoneNetworkEndpointMap[zone] == nil { zoneNetworkEndpointMap[zone] = negtypes.NewNetworkEndpointSet()