apply self code review

Signed-off-by: Denis Tingaikin <[email protected]>

Author: denis-tingaikin

pkg/networkservice/chains/nsmgr/single_test.go

@@ -1,5 +1,7 @@
 // Copyright (c) 2020-2022 Doc.ai and/or its affiliates.
 //
+// Copyright (c) 2023 Cisco and/or its affiliates.
+//
 // SPDX-License-Identifier: Apache-2.0
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
@@ -52,7 +54,6 @@ import (
 	"github.com/networkservicemesh/sdk/pkg/networkservice/utils/checks/checkrequest"
 	"github.com/networkservicemesh/sdk/pkg/networkservice/utils/checks/checkresponse"
 	"github.com/networkservicemesh/sdk/pkg/networkservice/utils/count"
-	countutils "github.com/networkservicemesh/sdk/pkg/networkservice/utils/count"
 	"github.com/networkservicemesh/sdk/pkg/networkservice/utils/inject/injecterror"
 	"github.com/networkservicemesh/sdk/pkg/registry"
 	registryclient "github.com/networkservicemesh/sdk/pkg/registry/chains/client"
@@ -83,10 +84,10 @@ func Test_AwareNSEs(t *testing.T) {
 	_, ipNet, err := net.ParseCIDR("172.16.0.96/29")
 	require.NoError(t, err)
 
-	const count = 3
-	var nseRegs [count]*registryapi.NetworkServiceEndpoint
-	var nses [count]*sandbox.EndpointEntry
-	var requests [count]*networkservice.NetworkServiceRequest
+	const nseCount = 3
+	var nseRegs [nseCount]*registryapi.NetworkServiceEndpoint
+	var nses [nseCount]*sandbox.EndpointEntry
+	var requests [nseCount]*networkservice.NetworkServiceRequest
 
 	ns1 := defaultRegistryService("my-ns-1")
 	ns2 := defaultRegistryService("my-ns-2")
@@ -97,7 +98,7 @@ func Test_AwareNSEs(t *testing.T) {
 	nsurl2, err := url.Parse(fmt.Sprintf("kernel://%s?%s=%s", ns2.Name, "color", "red"))
 	require.NoError(t, err)
 
-	nsInfo := [count]struct {
+	nsInfo := [nseCount]struct {
 		ns         *registryapi.NetworkService
 		labelKey   string
 		labelValue string
@@ -119,7 +120,7 @@ func Test_AwareNSEs(t *testing.T) {
 		},
 	}
 
-	for i := 0; i < count; i++ {
+	for i := 0; i < nseCount; i++ {
 		nseRegs[i] = &registryapi.NetworkServiceEndpoint{
 			Name:                fmt.Sprintf("nse-%s", uuid.New().String()),
 			NetworkServiceNames: []string{nsInfo[i].ns.Name},
@@ -170,8 +171,8 @@ func Test_AwareNSEs(t *testing.T) {
 			},
 		))))
 
-	var conns [count]*networkservice.Connection
-	for i := 0; i < count; i++ {
+	var conns [nseCount]*networkservice.Connection
+	for i := 0; i < nseCount; i++ {
 		conns[i], err = nsc.Request(ctx, requests[i])
 		require.NoError(t, err)
 		require.Equal(t, conns[0].NetworkServiceEndpointName, nses[0].Name)
@@ -185,12 +186,12 @@ func Test_AwareNSEs(t *testing.T) {
 	require.NotEqual(t, srcIP1[0], srcIP3[0])
 	require.NotEqual(t, srcIP2[0], srcIP3[0])
 
-	for i := 0; i < count; i++ {
+	for i := 0; i < nseCount; i++ {
 		_, err = nsc.Close(ctx, conns[i])
 		require.NoError(t, err)
 	}
 
-	for i := 0; i < count; i++ {
+	for i := 0; i < nseCount; i++ {
 		_, err = nses[i].Unregister(ctx, nseRegs[i])
 		require.NoError(t, err)
 	}
@@ -643,7 +644,7 @@ func Test_RestartDuringRefresh(t *testing.T) {
 		}
 	}))
 
-	var client = domain.Nodes[0].NewClient(ctx, sandbox.GenerateTestToken, client.WithAdditionalFunctionality(
+	var nsc = domain.Nodes[0].NewClient(ctx, sandbox.GenerateTestToken, client.WithAdditionalFunctionality(
 		&countClint,
 		checkcontext.NewClient(t, func(t *testing.T, ctx context.Context) {
 			m.Do(func() {
@@ -657,20 +658,19 @@ func Test_RestartDuringRefresh(t *testing.T) {
 						fwd.Restart()
 					}
 				})
-
 			}
 		}),
 		heal.NewClient(ctx),
 	))
 
-	_, err = client.Request(ctx, &networkservice.NetworkServiceRequest{
+	_, err = nsc.Request(ctx, &networkservice.NetworkServiceRequest{
 		Connection: &networkservice.Connection{
 			Id:             uuid.NewString(),
 			NetworkService: "ns",
 		},
 	})
 	require.NoError(t, err)
-	_ = <-clientFactory.Request()
+	<-clientFactory.Request()
 	require.Equal(t, 2, countServer.Requests())
 	require.Never(t, func() bool { return countServer.Requests() > 2 }, time.Second/2, time.Second/20)
 	destroyFwd.Store(true)
@@ -718,7 +718,7 @@ func Test_Timeout(t *testing.T) {
 	nsReg, err := nsRegistryClient.Register(chainCtx, ns)
 	require.NoError(t, err)
 
-	counter := new(countutils.Server)
+	counter := new(count.Server)
 
 	createAuthorizedEndpoint(chainCtx, t, ns.Name, domain.Nodes[0].NSMgr.URL, counter)
 

pkg/networkservice/common/heal/options.go

@@ -41,6 +41,7 @@ type options struct {
 // Option - option for heal.NewClient() chain element
 type Option func(o *options)
 
+// WithoutRetry disables retry on failed refreshes
 func WithoutRetry() Option {
 	return func(o *options) {
 		o.retryOnRequestFail = false

pkg/networkservice/common/heal/retry/client.go

@@ -14,6 +14,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+// Package retry provies a chain elemen that manages retries for failed requests
 package retry
 
 import (
@@ -38,7 +39,7 @@ type retryClient struct {
 	contextMap genericsync.Map[string, *cancelableContext]
 }
 
-// NewClient: returns a new client that retries the request in case the previous attempt failed.
+// NewClient returns a new client that retries the request in case the previous attempt failed.
 func NewClient(ctx context.Context) networkservice.NetworkServiceClient {
 	return &retryClient{
 		chainCtx: ctx,
@@ -54,20 +55,24 @@ func (n *retryClient) Request(ctx context.Context, request *networkservice.Netwo
 	if err != nil {
 		var opts []begin.Option
 		opts = append(opts, begin.CancelContext(cancelableCtx.Context))
-		if request.GetConnection().GetNetworkServiceEndpointName() != "" {
+		if request.GetConnection().GetNetworkServiceEndpointName() != "" && request.GetConnection().GetState() != networkservice.State_RESELECT_REQUESTED {
 			opts = append(opts, begin.WithReselect())
 		}
 		factory.Request(opts...)
 	} else {
-		n.contextMap.Delete(request.GetConnection().GetId())
+		if v, ok := n.contextMap.LoadAndDelete(request.GetConnection().GetId()); ok {
+			v.cancel()
+		}
 	}
 	return resp, err
 }
 
 func (n *retryClient) Close(ctx context.Context, conn *networkservice.Connection, opts ...grpc.CallOption) (*empty.Empty, error) {
 	resp, err := next.Client(ctx).Close(ctx, conn, opts...)
 	if err == nil {
-		n.contextMap.Delete(conn.GetId())
+		if v, ok := n.contextMap.LoadAndDelete(conn.GetId()); ok {
+			v.cancel()
+		}
 	}
 	return resp, err
 }