Skip to content

Commit

Permalink
Clean up error state checking
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@sawsa307
sawsa307 committed Jan 23, 2023
1 parent 091ae8b commit a3dd0a8
Show file tree
Hide file tree
Showing 3 changed files with 250 additions and 59 deletions.
11 changes: 6 additions & 5 deletions pkg/neg/syncers/transaction.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package syncers

import (
"context"
"errors"
"fmt"
"net/http"
"strings"
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand Down
285 changes: 233 additions & 52 deletions pkg/neg/syncers/transaction_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -1431,6 +1431,7 @@ func TestInvalidEndpointInfo(t *testing.T) {
testPodName2 := "pod2"
testPodName3 := "pod3"
testPodName4 := "pod4"
emptyNodeName := ""

testEndpointPodMap := map[negtypes.NetworkEndpoint]types.NamespacedName{
{
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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)
}
})
}
Expand Down Expand Up @@ -2275,19 +2455,20 @@ 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(),
testContext.EndpointSliceInformer.GetIndexer(),
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,
Expand Down
Loading

0 comments on commit a3dd0a8

Please sign in to comment.