From 251344b01dc3eadb7fe616f54d5d463283aef36b Mon Sep 17 00:00:00 2001 From: Paul Miller Date: Wed, 9 Mar 2022 22:10:53 +0000 Subject: [PATCH 1/5] wrap a histogram around syncHostNCVersion --- cns/restserver/internalapi.go | 22 +++++++++++++++------- cns/restserver/metrics.go | 11 +++++++++++ 2 files changed, 26 insertions(+), 7 deletions(-) diff --git a/cns/restserver/internalapi.go b/cns/restserver/internalapi.go index 65930ec061..bb6975aa37 100644 --- a/cns/restserver/internalapi.go +++ b/cns/restserver/internalapi.go @@ -12,6 +12,7 @@ import ( "net/http/httptest" "reflect" "strconv" + "time" "github.com/Azure/azure-container-networking/cns" "github.com/Azure/azure-container-networking/cns/logger" @@ -152,6 +153,15 @@ func (service *HTTPRestService) SyncNodeStatus( func (service *HTTPRestService) SyncHostNCVersion(ctx context.Context, channelMode string) { service.Lock() defer service.Unlock() + start := time.Now() + err := service.syncHostNCVersion(ctx, channelMode) + if err != nil { + logger.Errorf("%v", err) + } + syncHostNcVersion.WithLabelValues(strconv.FormatBool(err != nil)).Observe(time.Since(start).Seconds()) +} + +func (service *HTTPRestService) syncHostNCVersion(ctx context.Context, channelMode string) error { var hostVersionNeedsUpdateContainers []string for idx := range service.state.ContainerStatus { // Will open a separate PR to convert all the NC version related variable to int. Change from string to int is a pain. @@ -173,12 +183,11 @@ func (service *HTTPRestService) SyncHostNCVersion(ctx context.Context, channelMo } } if len(hostVersionNeedsUpdateContainers) == 0 { - return + return nil } ncList, err := service.nmagentClient.GetNCVersionList(ctx) if err != nil { - logger.Errorf("%v", err) - return + return err } newHostNCVersionList := map[string]string{} @@ -192,15 +201,13 @@ func (service *HTTPRestService) SyncHostNCVersion(ctx context.Context, channelMo } version, err := strconv.Atoi(versionStr) if err != nil { - logger.Errorf("failed to parse %s to int", versionStr) - continue + return err } // Check whether it exist in service state and get the related nc info ncInfo, exist := service.state.ContainerStatus[ncID] if !exist { - logger.Errorf("Can't find NC with ID %s in service state, stop updating this host NC version", ncID) - continue + return fmt.Errorf("Can't find NC with ID %s in service state, stop updating this host NC version", ncID) } if channelMode == cns.CRD { service.MarkIpsAsAvailableUntransacted(ncInfo.ID, version) @@ -210,6 +217,7 @@ func (service *HTTPRestService) SyncHostNCVersion(ctx context.Context, channelMo service.state.ContainerStatus[ncID] = ncInfo logger.Printf("Updated NC %s host version from %s to %s", ncID, oldHostNCVersion, ncInfo.HostVersion) } + return nil } // This API will be called by CNS RequestController on CRD update. diff --git a/cns/restserver/metrics.go b/cns/restserver/metrics.go index 87bead0af1..41233f9567 100644 --- a/cns/restserver/metrics.go +++ b/cns/restserver/metrics.go @@ -39,11 +39,22 @@ var ipConfigStatusStateTransitionTime = prometheus.NewHistogramVec( []string{"previous_state", "next_state"}, ) +var syncHostNcVersion = prometheus.NewHistogramVec( + prometheus.HistogramOpts{ + Name: "sync_host_nc_version", + Help: "Time spent by the IP Configuration Status in each state transition", + //nolint:gomnd // default bucket consts + Buckets: prometheus.ExponentialBuckets(0.001, 2, 15), // 1 ms to ~16 seconds + }, + []string{}, +) + func init() { metrics.Registry.MustRegister( httpRequestLatency, ipAssignmentLatency, ipConfigStatusStateTransitionTime, + syncHostNcVersion, ) } From 8e55000d73a65be1bcb4d51d7934f331d4e92707 Mon Sep 17 00:00:00 2001 From: Paul Miller Date: Fri, 11 Mar 2022 19:35:07 +0000 Subject: [PATCH 2/5] try and fix linter errros though dubious about one --- cns/restserver/internalapi.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/cns/restserver/internalapi.go b/cns/restserver/internalapi.go index bb6975aa37..f48e31ecde 100644 --- a/cns/restserver/internalapi.go +++ b/cns/restserver/internalapi.go @@ -156,11 +156,14 @@ func (service *HTTPRestService) SyncHostNCVersion(ctx context.Context, channelMo start := time.Now() err := service.syncHostNCVersion(ctx, channelMode) if err != nil { - logger.Errorf("%v", err) + logger.Errorf("sync host error %v", err) } syncHostNcVersion.WithLabelValues(strconv.FormatBool(err != nil)).Observe(time.Since(start).Seconds()) } +//https://stackoverflow.com/questions/62291709/linter-err113-do-not-define-dynamic-errors-use-wrapped-static-errors-instead +var nonexistContainerStatusError = errors.New("nonExistantContainerstatus") + func (service *HTTPRestService) syncHostNCVersion(ctx context.Context, channelMode string) error { var hostVersionNeedsUpdateContainers []string for idx := range service.state.ContainerStatus { @@ -187,7 +190,7 @@ func (service *HTTPRestService) syncHostNCVersion(ctx context.Context, channelMo } ncList, err := service.nmagentClient.GetNCVersionList(ctx) if err != nil { - return err + return fmt.Errorf("failed to get nc version list from nmagent: %w", err) } newHostNCVersionList := map[string]string{} @@ -201,13 +204,13 @@ func (service *HTTPRestService) syncHostNCVersion(ctx context.Context, channelMo } version, err := strconv.Atoi(versionStr) if err != nil { - return err + return fmt.Errorf("failed to parse container version of %s: %w", ncID, err) } // Check whether it exist in service state and get the related nc info ncInfo, exist := service.state.ContainerStatus[ncID] if !exist { - return fmt.Errorf("Can't find NC with ID %s in service state, stop updating this host NC version", ncID) + return fmt.Errorf("%w, Can't find NC with ID %s in service state, stop updating this host NC version", nonexistContainerStatusError, ncID) } if channelMode == cns.CRD { service.MarkIpsAsAvailableUntransacted(ncInfo.ID, version) From b42ec4e446a9eaed88aebba445082cd83e4f2ac0 Mon Sep 17 00:00:00 2001 From: Evan Baker Date: Fri, 11 Mar 2022 19:55:33 +0000 Subject: [PATCH 3/5] clean up Signed-off-by: Evan Baker --- cns/restserver/internalapi.go | 10 +++++----- cns/restserver/metrics.go | 4 ++-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/cns/restserver/internalapi.go b/cns/restserver/internalapi.go index f48e31ecde..6676220b03 100644 --- a/cns/restserver/internalapi.go +++ b/cns/restserver/internalapi.go @@ -161,8 +161,8 @@ func (service *HTTPRestService) SyncHostNCVersion(ctx context.Context, channelMo syncHostNcVersion.WithLabelValues(strconv.FormatBool(err != nil)).Observe(time.Since(start).Seconds()) } -//https://stackoverflow.com/questions/62291709/linter-err113-do-not-define-dynamic-errors-use-wrapped-static-errors-instead -var nonexistContainerStatusError = errors.New("nonExistantContainerstatus") +// https://stackoverflow.com/questions/62291709/linter-err113-do-not-define-dynamic-errors-use-wrapped-static-errors-instead +var errNonExistentContainerStatus = errors.New("nonExistantContainerstatus") func (service *HTTPRestService) syncHostNCVersion(ctx context.Context, channelMode string) error { var hostVersionNeedsUpdateContainers []string @@ -190,7 +190,7 @@ func (service *HTTPRestService) syncHostNCVersion(ctx context.Context, channelMo } ncList, err := service.nmagentClient.GetNCVersionList(ctx) if err != nil { - return fmt.Errorf("failed to get nc version list from nmagent: %w", err) + return errors.Wrap(err, "failed to get nc version list from nmagent") } newHostNCVersionList := map[string]string{} @@ -204,13 +204,13 @@ func (service *HTTPRestService) syncHostNCVersion(ctx context.Context, channelMo } version, err := strconv.Atoi(versionStr) if err != nil { - return fmt.Errorf("failed to parse container version of %s: %w", ncID, err) + return errors.Wrapf(err, "failed to parse container version of %s", ncID) } // Check whether it exist in service state and get the related nc info ncInfo, exist := service.state.ContainerStatus[ncID] if !exist { - return fmt.Errorf("%w, Can't find NC with ID %s in service state, stop updating this host NC version", nonexistContainerStatusError, ncID) + return errors.Wrapf(errNonExistentContainerStatus, "can't find NC with ID %s in service state, stop updating this host NC version", ncID) } if channelMode == cns.CRD { service.MarkIpsAsAvailableUntransacted(ncInfo.ID, version) diff --git a/cns/restserver/metrics.go b/cns/restserver/metrics.go index 41233f9567..ae2004e0fe 100644 --- a/cns/restserver/metrics.go +++ b/cns/restserver/metrics.go @@ -42,11 +42,11 @@ var ipConfigStatusStateTransitionTime = prometheus.NewHistogramVec( var syncHostNcVersion = prometheus.NewHistogramVec( prometheus.HistogramOpts{ Name: "sync_host_nc_version", - Help: "Time spent by the IP Configuration Status in each state transition", + Help: "Sync Host NC Latency", //nolint:gomnd // default bucket consts Buckets: prometheus.ExponentialBuckets(0.001, 2, 15), // 1 ms to ~16 seconds }, - []string{}, + []string{"error"}, ) func init() { From c6c969315e7653b80786b40caa36aeb9785ec24f Mon Sep 17 00:00:00 2001 From: Paul Miller Date: Fri, 11 Mar 2022 22:54:49 +0000 Subject: [PATCH 4/5] kick pr? --- cns/restserver/internalapi.go | 1 - 1 file changed, 1 deletion(-) diff --git a/cns/restserver/internalapi.go b/cns/restserver/internalapi.go index 6676220b03..ca6aff8962 100644 --- a/cns/restserver/internalapi.go +++ b/cns/restserver/internalapi.go @@ -161,7 +161,6 @@ func (service *HTTPRestService) SyncHostNCVersion(ctx context.Context, channelMo syncHostNcVersion.WithLabelValues(strconv.FormatBool(err != nil)).Observe(time.Since(start).Seconds()) } -// https://stackoverflow.com/questions/62291709/linter-err113-do-not-define-dynamic-errors-use-wrapped-static-errors-instead var errNonExistentContainerStatus = errors.New("nonExistantContainerstatus") func (service *HTTPRestService) syncHostNCVersion(ctx context.Context, channelMode string) error { From e136a6ac56cccdae138c026fe5a3db3c46ea4e3e Mon Sep 17 00:00:00 2001 From: Paul Miller Date: Fri, 11 Mar 2022 23:02:50 +0000 Subject: [PATCH 5/5] use success to be consistent with dnc --- cns/restserver/internalapi.go | 2 +- cns/restserver/metrics.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/cns/restserver/internalapi.go b/cns/restserver/internalapi.go index ca6aff8962..c8bd9acb3a 100644 --- a/cns/restserver/internalapi.go +++ b/cns/restserver/internalapi.go @@ -158,7 +158,7 @@ func (service *HTTPRestService) SyncHostNCVersion(ctx context.Context, channelMo if err != nil { logger.Errorf("sync host error %v", err) } - syncHostNcVersion.WithLabelValues(strconv.FormatBool(err != nil)).Observe(time.Since(start).Seconds()) + syncHostNcVersion.WithLabelValues(strconv.FormatBool(err == nil)).Observe(time.Since(start).Seconds()) } var errNonExistentContainerStatus = errors.New("nonExistantContainerstatus") diff --git a/cns/restserver/metrics.go b/cns/restserver/metrics.go index ae2004e0fe..f696657d9e 100644 --- a/cns/restserver/metrics.go +++ b/cns/restserver/metrics.go @@ -46,7 +46,7 @@ var syncHostNcVersion = prometheus.NewHistogramVec( //nolint:gomnd // default bucket consts Buckets: prometheus.ExponentialBuckets(0.001, 2, 15), // 1 ms to ~16 seconds }, - []string{"error"}, + []string{"success"}, ) func init() {