Merge pull request #1420 from swetharepakula/neg-err-count-metrics

Add success and error neg counts

pkg/metrics/features.go

@@ -99,6 +99,10 @@ const (
 	vmIpNegLocal   = feature("VmIpNegLocal")
 	vmIpNegCluster = feature("VmIpNegCluster")
 	customNamedNeg = feature("CustomNamedNEG")
+	// negInSuccess feature specifies that syncers were created for the Neg
+	negInSuccess = feature("NegInSuccess")
+	// negInError feature specifies that an error occuring in ensuring Neg Syncer
+	negInError = feature("NegInError")
 
 	l4ILBService      = feature("L4ILBService")
 	l4ILBGlobalAccess = feature("L4ILBGlobalAccess")

pkg/metrics/metrics.go

@@ -307,6 +307,8 @@ func (im *ControllerMetrics) computeNegMetrics() map[feature]int {
 		vmIpNegLocal:   0,
 		vmIpNegCluster: 0,
 		customNamedNeg: 0,
+		negInSuccess:   0,
+		negInError:     0,
 	}
 
 	for key, negState := range im.negMap {
@@ -317,6 +319,8 @@ func (im *ControllerMetrics) computeNegMetrics() map[feature]int {
 		counts[asmNeg] += negState.AsmNeg
 		counts[neg] += negState.AsmNeg + negState.StandaloneNeg + negState.IngressNeg
 		counts[customNamedNeg] += negState.CustomNamedNeg
+		counts[negInSuccess] += negState.SuccessfulNeg
+		counts[negInError] += negState.ErrorNeg
 		if negState.VmIpNeg != nil {
 			counts[neg] += 1
 			counts[vmIpNeg] += 1

pkg/metrics/metrics_test.go

@@ -22,7 +22,7 @@ import (
 
 	"github.com/google/go-cmp/cmp"
 	"k8s.io/api/networking/v1beta1"
-	"k8s.io/apimachinery/pkg/apis/meta/v1"
+	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/ingress-gce/pkg/annotations"
@@ -965,12 +965,14 @@ func TestComputeNegMetrics(t *testing.T) {
 				vmIpNegLocal:   0,
 				vmIpNegCluster: 0,
 				customNamedNeg: 0,
+				negInSuccess:   0,
+				negInError:     0,
 			},
 		},
 		{
 			"one neg service",
 			[]NegServiceState{
-				newNegState(0, 0, 1, 0, nil),
+				newNegState(0, 0, 1, 0, 1, 0, nil),
 			},
 			map[feature]int{
 				standaloneNeg:  0,
@@ -981,12 +983,14 @@ func TestComputeNegMetrics(t *testing.T) {
 				vmIpNegLocal:   0,
 				vmIpNegCluster: 0,
 				customNamedNeg: 0,
+				negInSuccess:   1,
+				negInError:     0,
 			},
 		},
 		{
 			"vm primary ip neg in traffic policy cluster mode",
 			[]NegServiceState{
-				newNegState(0, 0, 1, 0, &VmIpNegType{trafficPolicyLocal: false}),
+				newNegState(0, 0, 1, 0, 1, 0, &VmIpNegType{trafficPolicyLocal: false}),
 			},
 			map[feature]int{
 				standaloneNeg:  0,
@@ -997,12 +1001,14 @@ func TestComputeNegMetrics(t *testing.T) {
 				vmIpNegLocal:   0,
 				vmIpNegCluster: 1,
 				customNamedNeg: 0,
+				negInSuccess:   1,
+				negInError:     0,
 			},
 		},
 		{
 			"custom named neg",
 			[]NegServiceState{
-				newNegState(1, 0, 0, 1, nil),
+				newNegState(1, 0, 0, 1, 1, 0, nil),
 			},
 			map[feature]int{
 				standaloneNeg:  1,
@@ -1013,15 +1019,17 @@ func TestComputeNegMetrics(t *testing.T) {
 				vmIpNegLocal:   0,
 				vmIpNegCluster: 0,
 				customNamedNeg: 1,
+				negInSuccess:   1,
+				negInError:     0,
 			},
 		},
 		{
 			"many neg services",
 			[]NegServiceState{
-				newNegState(0, 0, 1, 0, nil),
-				newNegState(0, 1, 0, 0, &VmIpNegType{trafficPolicyLocal: false}),
-				newNegState(5, 0, 0, 0, &VmIpNegType{trafficPolicyLocal: true}),
-				newNegState(5, 3, 2, 0, nil),
+				newNegState(0, 0, 1, 0, 1, 0, nil),
+				newNegState(0, 1, 0, 0, 0, 1, &VmIpNegType{trafficPolicyLocal: false}),
+				newNegState(5, 0, 0, 0, 3, 2, &VmIpNegType{trafficPolicyLocal: true}),
+				newNegState(5, 3, 2, 0, 2, 3, nil),
 			},
 			map[feature]int{
 				standaloneNeg:  10,
@@ -1032,6 +1040,8 @@ func TestComputeNegMetrics(t *testing.T) {
 				vmIpNegLocal:   1,
 				vmIpNegCluster: 1,
 				customNamedNeg: 0,
+				negInSuccess:   6,
+				negInError:     6,
 			},
 		},
 	} {
@@ -1051,13 +1061,15 @@ func TestComputeNegMetrics(t *testing.T) {
 	}
 }
 
-func newNegState(standalone, ingress, asm, customNamed int, negType *VmIpNegType) NegServiceState {
+func newNegState(standalone, ingress, asm, customNamed, success, err int, negType *VmIpNegType) NegServiceState {
 	return NegServiceState{
 		IngressNeg:     ingress,
 		StandaloneNeg:  standalone,
 		AsmNeg:         asm,
 		VmIpNeg:        negType,
 		CustomNamedNeg: customNamed,
+		SuccessfulNeg:  success,
+		ErrorNeg:       err,
 	}
 }
 

pkg/metrics/types.go

@@ -41,6 +41,10 @@ type NegServiceState struct {
 	VmIpNeg *VmIpNegType
 	// CustomNamedNeg is the count of standalone negs with custom names
 	CustomNamedNeg int
+	// SuccessfulNeg is the count of successful NEG syncer creations
+	SuccessfulNeg int
+	// SuccessfulNeg is the count of errors in NEG syncer creations
+	ErrorNeg int
 }
 
 // VmIpNegType contains whether a GCE_VM_IP NEG is requesting for

pkg/neg/controller.go

@@ -464,8 +464,10 @@ func (c *Controller) processService(key string) error {
 		if err := svcPortInfoMap.Merge(destinationRulesPortInfoMap); err != nil {
 			return fmt.Errorf("failed to merge service ports referenced by Istio:DestinationRule (%v): %v", destinationRulesPortInfoMap, err)
 		}
+
+		negUsage.SuccessfulNeg, negUsage.ErrorNeg, err = c.manager.EnsureSyncers(namespace, name, svcPortInfoMap)
 		c.collector.SetNegService(key, negUsage)
-		return c.manager.EnsureSyncers(namespace, name, svcPortInfoMap)
+		return err
 	}
 
 	// do not need Neg

pkg/neg/manager.go

@@ -123,8 +123,9 @@ func newSyncerManager(namer negtypes.NetworkEndpointGroupNamer,
 	}
 }
 
-// EnsureSyncer starts and stops syncers based on the input service ports.
-func (manager *syncerManager) EnsureSyncers(namespace, name string, newPorts negtypes.PortInfoMap) error {
+// EnsureSyncer starts and stops syncers based on the input service ports. Returns the number of
+// syncers that were successfully created and the number of syncers that failed to be created
+func (manager *syncerManager) EnsureSyncers(namespace, name string, newPorts negtypes.PortInfoMap) (int, int, error) {
 	manager.mu.Lock()
 	defer manager.mu.Unlock()
 	start := time.Now()
@@ -147,6 +148,8 @@ func (manager *syncerManager) EnsureSyncers(namespace, name string, newPorts neg
 	klog.V(3).Infof("EnsureSyncer %v/%v: syncing %v ports, removing %v ports, adding %v ports", namespace, name, newPorts, removes, adds)
 
 	errList := []error{}
+	successfulSyncers := 0
+	errorSyncers := 0
 	for svcPort, portInfo := range removes {
 		syncer, ok := manager.syncerMap[manager.getSyncerKey(namespace, name, svcPort, portInfo)]
 		if ok {
@@ -164,6 +167,9 @@ func (manager *syncerManager) EnsureSyncers(namespace, name string, newPorts neg
 		// desired port.
 		if err := manager.ensureSvcNegCR(key, portInfo); err != nil {
 			errList = append(errList, fmt.Errorf("failed to ensure svc neg cr %s/%s/%d for existing port: %s", namespace, portInfo.NegName, portInfo.PortTuple.Port, err))
+			errorSyncers += 1
+		} else {
+			successfulSyncers += 1
 		}
 	}
 
@@ -178,6 +184,7 @@ func (manager *syncerManager) EnsureSyncers(namespace, name string, newPorts neg
 			// possibility of invalid states and reduces complexity of garbage collection
 			if err := manager.ensureSvcNegCR(key, portInfo); err != nil {
 				errList = append(errList, fmt.Errorf("failed to ensure svc neg cr %s/%s/%d for new port: %s ", namespace, portInfo.NegName, svcPort.ServicePort, err))
+				errorSyncers += 1
 				continue
 			}
 
@@ -206,12 +213,16 @@ func (manager *syncerManager) EnsureSyncers(namespace, name string, newPorts neg
 		if syncer.IsStopped() {
 			if err := syncer.Start(); err != nil {
 				errList = append(errList, err)
+				errorSyncers += 1
+				continue
 			}
 		}
+		successfulSyncers += 1
 	}
 	err := utilerrors.NewAggregate(errList)
 	metrics.PublishNegManagerProcessMetrics(metrics.SyncProcess, err, start)
-	return err
+
+	return successfulSyncers, errorSyncers, err
 }
 
 // StopSyncer stops all syncers for the input service.

pkg/neg/manager_test.go

@@ -244,7 +244,7 @@ func TestEnsureAndStopSyncer(t *testing.T) {
 		} else {
 			if tc.expectEnsureError {
 				if err := wait.Poll(time.Second, 10*time.Second, func() (bool, error) {
-					if err := manager.EnsureSyncers(tc.namespace, tc.name, tc.portInfoMap); err != nil {
+					if _, _, err := manager.EnsureSyncers(tc.namespace, tc.name, tc.portInfoMap); err != nil {
 						return false, nil
 					}
 					return true, nil
@@ -253,7 +253,7 @@ func TestEnsureAndStopSyncer(t *testing.T) {
 				}
 			} else {
 				// Expect EnsureSyncers returns successfully immediately
-				if err := manager.EnsureSyncers(tc.namespace, tc.name, tc.portInfoMap); err != nil {
+				if _, _, err := manager.EnsureSyncers(tc.namespace, tc.name, tc.portInfoMap); err != nil {
 					t.Errorf("For case %q, failed to ensure syncer %s/%s-%v: %v", tc.desc, tc.namespace, tc.name, tc.portInfoMap, err)
 				}
 			}
@@ -338,7 +338,7 @@ func TestGarbageCollectionSyncer(t *testing.T) {
 
 	manager.serviceLister.Add(&v1.Service{ObjectMeta: metav1.ObjectMeta{Namespace: namespace, Name: name}})
 
-	if err := manager.EnsureSyncers(namespace, name, portMap); err != nil {
+	if _, _, err := manager.EnsureSyncers(namespace, name, portMap); err != nil {
 		t.Fatalf("Failed to ensure syncer: %v", err)
 	}
 	manager.StopSyncer(namespace, name)
@@ -372,7 +372,7 @@ func TestGarbageCollectionNEG(t *testing.T) {
 	ports := make(types.PortInfoMap)
 	manager.serviceLister.Add(&v1.Service{ObjectMeta: metav1.ObjectMeta{Namespace: testServiceNamespace, Name: testServiceName}})
 	ports[negtypes.PortInfoMapKey{ServicePort: svcPort, Subset: ""}] = types.PortInfo{PortTuple: negtypes.SvcPortTuple{TargetPort: "namedport"}, NegName: manager.namer.NEG(testServiceNamespace, testServiceName, svcPort)}
-	if err := manager.EnsureSyncers(testServiceNamespace, testServiceName, ports); err != nil {
+	if _, _, err := manager.EnsureSyncers(testServiceNamespace, testServiceName, ports); err != nil {
 		t.Fatalf("Failed to ensure syncer: %v", err)
 	}
 
@@ -741,7 +741,7 @@ func TestNegCRCreations(t *testing.T) {
 		namer, false,
 		map[negtypes.SvcPortTuple]string{{Port: port1, TargetPort: targetPort1}: customNegName})
 
-	if err := manager.EnsureSyncers(svcNamespace, svcName, expectedPortInfoMap); err != nil {
+	if _, _, err := manager.EnsureSyncers(svcNamespace, svcName, expectedPortInfoMap); err != nil {
 		t.Errorf("failed to ensure syncer %s/%s-%v: %v", svcNamespace, svcName, expectedPortInfoMap, err)
 	}
 
@@ -777,7 +777,7 @@ func TestNegCRCreations(t *testing.T) {
 	}
 
 	// Second call of EnsureSyncers shouldn't cause any changes or errors
-	if err := manager.EnsureSyncers(svcNamespace, svcName, expectedPortInfoMap); err != nil {
+	if _, _, err := manager.EnsureSyncers(svcNamespace, svcName, expectedPortInfoMap); err != nil {
 		t.Errorf("failed to ensure syncer after creating %s/%s-%v: %v", svcNamespace, svcName, expectedPortInfoMap, err)
 	}
 
@@ -800,7 +800,7 @@ func TestNegCRCreations(t *testing.T) {
 	}
 
 	// EnsureSyncers should recreate the deleted CRs
-	if err := manager.EnsureSyncers(svcNamespace, svcName, expectedPortInfoMap); err != nil {
+	if _, _, err := manager.EnsureSyncers(svcNamespace, svcName, expectedPortInfoMap); err != nil {
 		t.Errorf("failed to ensure syncer after creating %s/%s-%v: %v", svcNamespace, svcName, expectedPortInfoMap, err)
 	}
 
@@ -985,7 +985,7 @@ func TestNegCRDuplicateCreations(t *testing.T) {
 				map[negtypes.SvcPortTuple]string{svcTuple1: customNegName},
 			)
 
-			err := manager.EnsureSyncers(namespace, svc1.Name, portInfoMap)
+			_, _, err := manager.EnsureSyncers(namespace, svc1.Name, portInfoMap)
 			if tc.expectErr && err == nil {
 				t.Errorf("expected error when ensuring syncer %s/%s %+v", namespace, svc1.Name, portInfoMap)
 			} else if !tc.expectErr && err != nil {
@@ -1105,7 +1105,7 @@ func TestNegCRDeletions(t *testing.T) {
 				}
 			}
 
-			if err := manager.EnsureSyncers(svcNamespace, svcName, nil); err != nil {
+			if _, _, err := manager.EnsureSyncers(svcNamespace, svcName, nil); err != nil {
 
 				t.Errorf("unexpected error when deleting negs: %s", err)
 			}

pkg/neg/types/interfaces.go

@@ -67,8 +67,9 @@ type NegSyncer interface {
 // NegSyncerManager is an interface for controllers to manage syncer
 type NegSyncerManager interface {
 	// EnsureSyncer ensures corresponding syncers are started and stops any unnecessary syncer
-	// portMap is a map of ServicePort Port to TargetPort
-	EnsureSyncers(namespace, name string, portMap PortInfoMap) error
+	// portMap is a map of ServicePort Port to TargetPort. Returns counts of successful Neg syncers
+	// and failed Neg sycner creations
+	EnsureSyncers(namespace, name string, portMap PortInfoMap) (int, int, error)
 	// StopSyncer stops all syncers related to the service. This call is asynchronous. It will not wait for all syncers to stop.
 	StopSyncer(namespace, name string)
 	// Sync signals all syncers related to the service to sync. This call is asynchronous.