Add metrics for sync result #1911

sawsa307 · 2023-01-24T20:04:39Z

Created sync metrics collector to collect sync related metrics. Added metrics to collect the cumulative count of sync results. Specified metrics emit interval in flags.go.

k8s-ci-robot · 2023-01-24T20:04:48Z

Hi @sawsa307. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

sawsa307 · 2023-01-24T20:04:58Z

/assign @swetharepakula

swetharepakula · 2023-01-28T02:02:17Z

/ok-to-test

pkg/neg/metrics/metrics.go

pkg/neg/metrics/neg_metrics_collector.go

swetharepakula · 2023-01-30T21:41:19Z

pkg/neg/metrics/neg_metrics_collector.go

+}
+
+// UpdateSyncer updates the count of sync results based on the result/error of sync
+func (im *SyncerMetrics) UpdateSyncer(key negtypes.NegSyncerKey, err error) {


nit: maybe sm or just m?

swetharepakula · 2023-01-30T21:45:21Z

pkg/neg/metrics/neg_metrics_collector.go

+	if err == nil {
+		im.countSinceLastExport[Success] += 1
+	} else {
+		syncErr := errors.Unwrap(err).(syncError)


should we verify that the error is of type syncError?

Or can we guarantee that this is the type of error always?

swetharepakula · 2023-01-30T21:46:27Z

pkg/neg/metrics/neg_metrics_collector.go

+	im.mu.Lock()
+	defer im.mu.Unlock()
+	if im.countSinceLastExport == nil {
+		klog.Fatalf("Syncer Metrics failed to initialize correctly, countSinceLastExport: %v", im.countSinceLastExport)


fatal will cause the binary to exit. should we just log the error and initialize here?

swetharepakula · 2023-01-30T21:48:10Z

pkg/neg/metrics/sync_errors.go

@@ -0,0 +1,86 @@
+/*


these errors should be in the syncer code that will return this errors.

swetharepakula · 2023-01-30T21:48:18Z

pkg/neg/metrics/sync_errors.go

@@ -0,0 +1,86 @@
+/*
+Copyright 2020 The Kubernetes Authors.


swetharepakula · 2023-01-30T22:27:57Z

pkg/flags/flags.go

@@ -256,7 +257,8 @@ L7 load balancing. CSV values accepted. Example: -node-port-ranges=80,8080,400-5
 	flag.BoolVar(&F.EnableL4ILBDualStack, "enable-l4ilb-dual-stack", false, "Enable Dual-Stack handling for L4 Internal Load Balancers")
 	flag.BoolVar(&F.EnableMultipleIGs, "enable-multiple-igs", false, "Enable using multiple unmanaged instance groups")
 	flag.IntVar(&F.MaxIGSize, "max-ig-size", 1000, "Max number of instances in Instance Group")
-	flag.DurationVar(&F.MetricsExportInterval, "metrics-export-interval", 10*time.Minute, `Period for calculating and exporting metrics related to state of managed objects.`)
+	flag.DurationVar(&F.UsageMetricsExportInterval, "usage-metrics-export-interval", 10*time.Minute, `Period for calculating and exporting metrics related to state of managed objects.`)
+	flag.DurationVar(&F.SyncMetricsExportInterval, "sync-metrics-export-interval", 5*time.Second, `Period for calculating and exporting metrics related to state of syncer.`)


maybe neg-metrics-export-interval

In the description, add that these are neg controller internal metrics, not usage.

pkg/neg/metrics/neg_metrics_collector.go

swetharepakula · 2023-01-30T22:41:12Z

cmd/glbc/main.go

@@ -329,6 +329,7 @@ func runControllers(ctx *ingctx.ControllerContext) {
 		ctx.SvcNegInformer,
 		ctx.HasSynced,
 		ctx.ControllerMetrics,
+		ctx.SyncerMetrics,


since moving to the controller, we don't nee to add to context.

swetharepakula · 2023-01-30T22:50:17Z

pkg/neg/syncers/transaction.go

@@ -217,7 +217,7 @@ func (s *transactionSyncer) syncInternalImpl() error {

 	currentMap, err := retrieveExistingZoneNetworkEndpointMap(s.NegSyncerKey.NegName, s.zoneGetter, s.cloud, s.NegSyncerKey.GetAPIVersion(), s.endpointsCalculator.Mode())
 	if err != nil {
-		return err
+		return fmt.Errorf("%w, reason: %v", metrics.ErrCurrentEPNotFound, err)


This slightly obscures what is the error we are returning. Typically we wrap the error we received instead of wrapping the context we are returning.

Implement a result instead where you can provide the reason. Look at what was done in L4lb:

ingress-gce/pkg/loadbalancers/l4.go

Line 62 in 0f312a7

type L4ILBSyncResult struct {

Updated. Thanks!

swetharepakula · 2023-01-30T23:07:52Z

pkg/neg/metrics/neg_metrics_collector.go

+// NewNEGMetricsCollector initializes SyncerMetrics and starts a go routine to compute and export metrics periodically.
+func NewNegMetricsCollector(exportInterval time.Duration) *SyncerMetrics {
+	return &SyncerMetrics{
+		countSinceLastExport: map[syncError]int{


Since this is a counter, we don't need to store this state. This will be necessary only when we have gauge metrics.

Updated. Thanks!

swetharepakula · 2023-02-02T21:54:59Z

pkg/neg/syncers/transaction.go

 	}
 	s.logEndpoints(addEndpoints, "adding endpoint")
 	s.logEndpoints(removeEndpoints, "removing endpoint")

-	return s.syncNetworkEndpoints(addEndpoints, removeEndpoints)
+	err = s.syncNetworkEndpoints(addEndpoints, removeEndpoints)


should we just make syncNetworkEndpoints return the sync result?

pkg/neg/syncers/transaction.go

swetharepakula · 2023-02-24T22:13:53Z

pkg/neg/syncers/transaction.go

 //
 //  1. The endpoint count from endpointData doesn't equal to the one from endpointPodMap:
 //     endpiontPodMap removes the duplicated endpoints, and dupCount stores the number of duplicated it removed
 //     and we compare the endpoint counts with duplicates
 //  2. The endpoint count from endpointData or the one from endpointPodMap is 0
-func (s *transactionSyncer) isValidEndpointInfo(eds []negtypes.EndpointsData, endpointPodMap negtypes.EndpointPodMap, dupCount int) (bool, string) {
+func (s *transactionSyncer) checkValidEndpointInfo(eds []negtypes.EndpointsData, endpointPodMap negtypes.EndpointPodMap, dupCount int) (bool, *negtypes.NegSyncResult) {


nitL checkEndpointInfo or validateEndpointInfo

swetharepakula · 2023-02-24T22:18:49Z

pkg/neg/syncers/transaction.go

-	if errors.Is(err, ErrEPMissingNodeName) {
+// checkValidEPField checks if endpoints have valid field
+// It return the result boolean with the corresponding reason
+func (s *transactionSyncer) checkValidEPField(err error) (bool, *negtypes.NegSyncResult) {


nit: checkEPField or validateEPField

swetharepakula · 2023-02-24T22:26:17Z

pkg/neg/syncers/transaction.go

+		errorState := s.errorState
+		if errorState == negtypes.ResultInvalidAPIResponse {
+			syncResult = negtypes.NewNegSyncResult(negtypes.ErrInvalidAPIResponse, errorState)
+		}
+		if errorState == negtypes.ResultInvalidEPAttach {
+			syncResult = negtypes.NewNegSyncResult(negtypes.ErrInvalidEPAttach, errorState)
+		}
+		if errorState == negtypes.ResultInvalidEPDetach {
+			syncResult = negtypes.NewNegSyncResult(negtypes.ErrInvalidEPDetach, errorState)
+		}


you can use a switch and make the default case do the last one

swetharepakula · 2023-02-24T22:58:26Z

pkg/neg/syncers/transaction.go

+	defer s.syncLock.Unlock()
+
+	var syncResult *negtypes.NegSyncResult
+	if s.inErrorState() {


Do you need to check for error state here? We just need the syncResult, so there should be a nil case and the detach case added to the switch.

swetharepakula · 2023-02-24T23:26:15Z

pkg/neg/syncers/transaction_test.go

-			if got, reason := transactionSyncer.isValidEndpointInfo(tc.endpointsData, tc.endpointPodMap, tc.dupCount); got != tc.expect && reason != tc.expectedReason {
-				t.Errorf("invalidEndpointInfo() = %t,  expected %t", got, tc.expect)
+			if got, result := transactionSyncer.CheckEndpointInfo(tc.endpointsData, tc.endpointPodMap, tc.dupCount); got != tc.expect || result.Result != tc.expectedResult.Result || !errors.Is(result.Error, tc.expectedResult.Error) {
+				t.Errorf("CheckEndpointInfo() = %t|%s|%t, expected %t|%s|%t", got, result.Result, result.Error, tc.expect, tc.expectedResult.Result, tc.expectedResult.Error)


We should do got then wanted or expected based on go style guide.

Break it up a little more so that it is easier to understand the test and debug in the future:

got, result := transactionSyncer.CheckEndpointInfo(tc.endpointsData, tc.endpointPodMap, tc.dupCount) if got != tc.expect { t.Errorf(...) } if result.Result != tc.expectedResult { t.Errorf(...) }

same for the other test.

Added metrics to collect the cumulative count of sync results.

swetharepakula

/lgtm
/approve

k8s-ci-robot · 2023-02-25T00:20:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sawsa307, swetharepakula

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [swetharepakula]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Revert kubernetes#1911

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 24, 2023

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 24, 2023

k8s-ci-robot requested review from aojea and bowei January 24, 2023 20:04

k8s-ci-robot assigned swetharepakula Jan 24, 2023

sawsa307 force-pushed the neg-sync-result-metric branch 7 times, most recently from 2b4b322 to 16db295 Compare January 26, 2023 20:02

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 28, 2023

swetharepakula reviewed Jan 28, 2023

View reviewed changes

pkg/neg/metrics/metrics.go Outdated Show resolved Hide resolved

sawsa307 force-pushed the neg-sync-result-metric branch 3 times, most recently from d2af05f to b36169a Compare January 30, 2023 17:51

sawsa307 mentioned this pull request Jan 30, 2023

Add metrics for endpointslice state #1923

Closed

sawsa307 commented Jan 30, 2023

View reviewed changes

pkg/neg/metrics/neg_metrics_collector.go Outdated Show resolved Hide resolved

swetharepakula reviewed Jan 30, 2023

View reviewed changes

sawsa307 force-pushed the neg-sync-result-metric branch from b36169a to bb394ad Compare January 31, 2023 18:29

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 31, 2023

sawsa307 force-pushed the neg-sync-result-metric branch from bb394ad to 3b2b1c7 Compare January 31, 2023 18:36

k8s-ci-robot requested a review from thockin February 14, 2023 01:06

sawsa307 force-pushed the neg-sync-result-metric branch 2 times, most recently from b14be83 to 4d3b024 Compare February 16, 2023 05:37

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 17, 2023

sawsa307 added 2 commits February 17, 2023 08:22

Create sync result metrics

a6056ab

update syncResult

81c79a6

sawsa307 force-pushed the neg-sync-result-metric branch from 4d3b024 to f5b9857 Compare February 17, 2023 09:29

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 17, 2023

swetharepakula reviewed Feb 23, 2023

View reviewed changes

sawsa307 force-pushed the neg-sync-result-metric branch 2 times, most recently from 48bc2e8 to b64514b Compare February 24, 2023 00:51

swetharepakula reviewed Feb 24, 2023

View reviewed changes

sawsa307 force-pushed the neg-sync-result-metric branch from b64514b to 72c4296 Compare February 24, 2023 22:36

swetharepakula reviewed Feb 24, 2023

View reviewed changes

sawsa307 force-pushed the neg-sync-result-metric branch from 72c4296 to 9933f59 Compare February 24, 2023 23:12

swetharepakula reviewed Feb 24, 2023

View reviewed changes

sawsa307 force-pushed the neg-sync-result-metric branch from 9933f59 to dd0242e Compare February 24, 2023 23:32

Add metrics for sync result

ff4751f

Added metrics to collect the cumulative count of sync results.

sawsa307 force-pushed the neg-sync-result-metric branch from dd0242e to ff4751f Compare February 24, 2023 23:58

swetharepakula reviewed Feb 25, 2023

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 25, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 25, 2023

k8s-ci-robot merged commit 88f329d into kubernetes:master Feb 25, 2023

sawsa307 added a commit to sawsa307/ingress-gce that referenced this pull request Feb 28, 2023

Revert "Add metrics for sync result"

1855adc

Revert kubernetes#1911

sawsa307 mentioned this pull request Feb 28, 2023

Revert "Add metrics for sync result" #1982

Merged

sawsa307 added a commit to sawsa307/ingress-gce that referenced this pull request Feb 28, 2023

Revert "Add metrics for sync result"

451c47f

Revert kubernetes#1911

sawsa307 added a commit to sawsa307/ingress-gce that referenced this pull request Feb 28, 2023

Revert "Add metrics for sync result"

0265b92

Revert kubernetes#1911

sawsa307 deleted the neg-sync-result-metric branch April 3, 2023 16:48

code-elinka pushed a commit to akhmanova/ingress-gce that referenced this pull request Apr 18, 2023

Revert "Add metrics for sync result"

8083de2

Revert kubernetes#1911

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add metrics for sync result #1911

Add metrics for sync result #1911

sawsa307 commented Jan 24, 2023

k8s-ci-robot commented Jan 24, 2023

sawsa307 commented Jan 24, 2023

swetharepakula commented Jan 28, 2023

swetharepakula Jan 30, 2023

swetharepakula Jan 30, 2023

swetharepakula Jan 30, 2023

swetharepakula Jan 30, 2023

swetharepakula Jan 30, 2023

swetharepakula Jan 30, 2023

swetharepakula Jan 30, 2023

swetharepakula Jan 30, 2023

sawsa307 Jan 30, 2023

swetharepakula Jan 30, 2023

sawsa307 Jan 30, 2023

swetharepakula Feb 2, 2023

swetharepakula Feb 24, 2023

swetharepakula Feb 24, 2023

swetharepakula Feb 24, 2023

swetharepakula Feb 24, 2023

swetharepakula Feb 24, 2023

swetharepakula left a comment

k8s-ci-robot commented Feb 25, 2023

Add metrics for sync result #1911

Add metrics for sync result #1911

Conversation

sawsa307 commented Jan 24, 2023

k8s-ci-robot commented Jan 24, 2023

sawsa307 commented Jan 24, 2023

swetharepakula commented Jan 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swetharepakula left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 25, 2023