Add cleanup when connection state is RESELECT_REQUESTED (using begin server) #1471

d-uzlov · 2023-06-19T09:21:44Z

Description

Modify begin to call Close when reselect is requested for already existing connection.

Issue link

Simplify healing - determinate reselect state for whole chain #1454

How Has This Been Tested?

Added unit testing to cover
Tested manually
Tested by integration testing
Have not tested

Types of changes

Signed-off-by: Danil Uzlov <[email protected]>

codecov · 2023-06-21T08:20:36Z

Codecov Report

❗ No coverage uploaded for pull request base (main@277758b). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1471   +/-   ##
=======================================
  Coverage        ?   70.31%           
=======================================
  Files           ?      248           
  Lines           ?    11196           
  Branches        ?        0           
=======================================
  Hits            ?     7873           
  Misses          ?     2821           
  Partials        ?      502

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Signed-off-by: Danil Uzlov <[email protected]>

…in-cleanup

denis-tingaikin

Changes looks fine.
Added questions related to change for existing tests.

denis-tingaikin · 2023-06-29T09:28:27Z

pkg/networkservice/chains/nsmgr/heal_test.go

@@ -459,7 +459,7 @@ func testNSMGRHealNSMgr(t *testing.T, nodeNum int, restored bool) {

 	if restored {
 		require.Equal(t, 3, counter.Requests())
-		require.Equal(t, 1, counter.Closes())
+		require.Equal(t, 2, counter.Closes())


Why do we change this?

Previously Close never reached NSE because nsmgr restarted, but now Close call is automatically re-issued when reselect request reaches forwarder.

denis-tingaikin · 2023-06-29T09:28:54Z

pkg/networkservice/chains/nsmgr/select_forwarder_test.go

@@ -217,15 +217,15 @@ func Test_DiscoverForwarder_ChangeForwarderOnDeath_LostHeal(t *testing.T) {
 	require.Eventually(t, checkSecondRequestsReceived(counter.Requests), timeout, tick)
 	require.Equal(t, 1, counter.UniqueRequests())
 	require.Equal(t, 2, counter.Requests())
-	require.Equal(t, 0, counter.Closes())
+	require.Equal(t, 1, counter.Closes())


Why do we change existing tests?

This test is designed to test what happens if there were network issues during Close.
Network issues don't matter for Close anymore, as long as we go through the same apps.
Maybe we should just delete this test.

denis-tingaikin · 2023-06-29T09:30:14Z

pkg/networkservice/common/monitor/passthrough_test.go

@@ -249,6 +249,7 @@ func (m *MonitorPassThroughSuite) TestServerUpdate() {
 	expectedConn := endpointConn.Clone()
 	expectedConn.GetPath().Index = m.conn.GetPath().GetIndex()
 	expectedConn.Id = expectedConn.GetCurrentPathSegment().GetId()
+	expectedConn.State = networkservice.State_DOWN


Why do we need to change this?

We modified begin to change the connection state to UP.
In this test we didn't do connection.Clone() when calling Request, so we ended up with a modified Connection object, which I tried to fix by manually reverting the connection to expected state.

I have now changed it to use Clone instead of manually modifying the object.

Signed-off-by: Danil Uzlov <[email protected]>

…k@main PR link: networkservicemesh/sdk#1471 Commit: 0ba94ea Author: Danil Uzlov Date: 2023-07-03 16:26:23 +0700 Message: - Add cleanup when connection state is RESELECT_REQUESTED (using begin server) (#1471) * use modified api dependency Signed-off-by: Danil Uzlov <[email protected]> * fix dialClient.Close Signed-off-by: Danil Uzlov <[email protected]> * fix discoverCandidatesServer.Close Signed-off-by: Danil Uzlov <[email protected]> * fix discoverForwarderServer.Close Signed-off-by: Danil Uzlov <[email protected]> * add reselect test, fix old tests for reselect Signed-off-by: Danil Uzlov <[email protected]> * use reselect flag on requests in event factory Signed-off-by: Danil Uzlov <[email protected]> * add reselectcleanup logic into begin Signed-off-by: Danil Uzlov <[email protected]> * fix repeated closes Signed-off-by: Danil Uzlov <[email protected]> * fix line endings Signed-off-by: Danil Uzlov <[email protected]> * fix Test_DiscoverForwarder_ChangeForwarderOnDeath_LostHeal Signed-off-by: Danil Uzlov <[email protected]> * fix Test_DiscoverForwarder_ChangeRemoteForwarderOnDeath Signed-off-by: Danil Uzlov <[email protected]> * adjust checks in the begin element Signed-off-by: Danil Uzlov <[email protected]> * add more tests for nsmgr restart Signed-off-by: Danil Uzlov <[email protected]> * bump ci Signed-off-by: Danil Uzlov <[email protected]> * revert api replacement Signed-off-by: Danil Uzlov <[email protected]> * fix MonitorPassThroughSuite.TestServerUpdate Signed-off-by: Danil Uzlov <[email protected]> --------- Signed-off-by: NSMBot <[email protected]>

d-uzlov force-pushed the 1454-begin-cleanup branch 2 times, most recently from f07537f to 9b2284c Compare June 20, 2023 12:05

d-uzlov added 8 commits June 21, 2023 13:42

use modified api dependency

cd44a94

Signed-off-by: Danil Uzlov <[email protected]>

fix dialClient.Close

e1a9ddb

Signed-off-by: Danil Uzlov <[email protected]>

fix discoverCandidatesServer.Close

073cc78

Signed-off-by: Danil Uzlov <[email protected]>

fix discoverForwarderServer.Close

35481c5

Signed-off-by: Danil Uzlov <[email protected]>

add reselect test, fix old tests for reselect

d80fb5a

Signed-off-by: Danil Uzlov <[email protected]>

use reselect flag on requests in event factory

b331833

Signed-off-by: Danil Uzlov <[email protected]>

add reselectcleanup logic into begin

4870ab1

Signed-off-by: Danil Uzlov <[email protected]>

fix repeated closes

5405eb5

Signed-off-by: Danil Uzlov <[email protected]>

d-uzlov force-pushed the 1454-begin-cleanup branch from 9b2284c to 5405eb5 Compare June 21, 2023 07:25

d-uzlov added 3 commits June 21, 2023 14:31

fix line endings

c7d769e

Signed-off-by: Danil Uzlov <[email protected]>

fix Test_DiscoverForwarder_ChangeForwarderOnDeath_LostHeal

a70747d

Signed-off-by: Danil Uzlov <[email protected]>

fix Test_DiscoverForwarder_ChangeRemoteForwarderOnDeath

0f57f88

Signed-off-by: Danil Uzlov <[email protected]>

d-uzlov added 3 commits June 22, 2023 15:53

adjust checks in the begin element

c328cc8

Signed-off-by: Danil Uzlov <[email protected]>

add more tests for nsmgr restart

8a4b0a3

Signed-off-by: Danil Uzlov <[email protected]>

bump ci

04b03b2

Signed-off-by: Danil Uzlov <[email protected]>

d-uzlov marked this pull request as ready for review June 22, 2023 09:29

glazychev-art requested review from denis-tingaikin, NikitaSkrynnik and glazychev-art June 22, 2023 09:39

d-uzlov mentioned this pull request Jun 23, 2023

Test Reselect flag networkservicemesh/integration-k8s-kind#836

Closed

d-uzlov added 2 commits June 28, 2023 13:47

revert api replacement

e3913c0

Signed-off-by: Danil Uzlov <[email protected]>

Merge commit '277758ba86168972a8bbbd52b94cdf0d54b37a81' into 1454-beg…

06183b2

…in-cleanup

denis-tingaikin reviewed Jun 29, 2023

View reviewed changes

fix MonitorPassThroughSuite.TestServerUpdate

8102231

Signed-off-by: Danil Uzlov <[email protected]>

d-uzlov force-pushed the 1454-begin-cleanup branch from 5c69aa7 to 8102231 Compare June 30, 2023 08:06

glazychev-art mentioned this pull request Jun 30, 2023

TestVl3_dns is not stable networkservicemesh/deployments-k8s#9063

Closed

glazychev-art approved these changes Jul 3, 2023

View reviewed changes

denis-tingaikin merged commit 0ba94ea into networkservicemesh:main Jul 3, 2023

denis-tingaikin mentioned this pull request Jul 18, 2023

How configuration change should be handled during healing? networkservicemesh/deployments-k8s#9219

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cleanup when connection state is RESELECT_REQUESTED (using begin server) #1471

Add cleanup when connection state is RESELECT_REQUESTED (using begin server) #1471

d-uzlov commented Jun 19, 2023 •

edited

Loading

codecov bot commented Jun 21, 2023 •

edited

Loading

denis-tingaikin left a comment

denis-tingaikin Jun 29, 2023

d-uzlov Jun 30, 2023

denis-tingaikin Jun 29, 2023

d-uzlov Jun 30, 2023

denis-tingaikin Jun 29, 2023

d-uzlov Jun 30, 2023

Add cleanup when connection state is RESELECT_REQUESTED (using begin server) #1471

Add cleanup when connection state is RESELECT_REQUESTED (using begin server) #1471

Conversation

d-uzlov commented Jun 19, 2023 • edited Loading

Description

Issue link

How Has This Been Tested?

Types of changes

codecov bot commented Jun 21, 2023 • edited Loading

Codecov Report

denis-tingaikin left a comment

Choose a reason for hiding this comment

denis-tingaikin Jun 29, 2023

Choose a reason for hiding this comment

d-uzlov Jun 30, 2023

Choose a reason for hiding this comment

denis-tingaikin Jun 29, 2023

Choose a reason for hiding this comment

d-uzlov Jun 30, 2023

Choose a reason for hiding this comment

denis-tingaikin Jun 29, 2023

Choose a reason for hiding this comment

d-uzlov Jun 30, 2023

Choose a reason for hiding this comment

d-uzlov commented Jun 19, 2023 •

edited

Loading

codecov bot commented Jun 21, 2023 •

edited

Loading