Fix healing after failed refresh #1465

d-uzlov · 2023-06-06T09:10:08Z

Description

Restart connection monitoring after a failed refresh.

Issue link

Healing after failed refresh #1457

Should also at least partially fix this:

DiscoverForwarder tests are unstable #1459

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 <DanilUzlov@yandex.ru>

codecov · 2023-06-06T09:17:02Z

Codecov Report

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

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1465   +/-   ##
=======================================
  Coverage        ?   70.39%           
=======================================
  Files           ?      248           
  Lines           ?    11166           
  Branches        ?        0           
=======================================
  Hits            ?     7860           
  Misses          ?     2806           
  Partials        ?      500

☔ 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 <DanilUzlov@yandex.ru>

denis-tingaikin · 2023-06-19T14:20:42Z

pkg/networkservice/chains/client/options.go

+func WithRefresh(refreshClient networkservice.NetworkServiceClient) Option {
+	if refreshClient == nil {
+		panic("refreshClient cannot be nil")
+	}
+	return Option(func(c *clientOptions) {
+		c.refreshClient = refreshClient
+	})
+}


Do not add a new api for testing goals.

denis-tingaikin · 2023-06-19T14:23:15Z

pkg/networkservice/chains/nsmgr/select_forwarder_test.go

-	require.Equal(t, 2, counter.Requests())
-	require.Equal(t, 0, counter.Closes())


Could you explain the change?

I modified it because this test was unstable, and yesterday the CI was constantly failing.
There was somewhat of a race between the test code and healing.
I figured out that this check isn't really important for the purpose of this test.
Now I reverted this and pushed a better solution which makes the test more deterministic.

pkg/networkservice/chains/nsmgr/select_forwarder_test.go

pkg/networkservice/common/heal/eventloop.go

denis-tingaikin · 2023-06-19T14:38:18Z

pkg/networkservice/common/heal/eventloop.go


-		for {
-			eventIn, err := cev.client.Recv()
+	if !needToHeal {


Probably name inversion could be more clear

Suggested change

if !needToHeal {

if healthy {

What if needToHead=false and reselct=true?

In the event monitoring there are three states:

Context was cancelled, needToHeal == false

Control plane is down, needToHeal == true, reselect == false

Data plane is down, needToHeal == true, reselect == true

Maybe we could add a special enum for these three states, but I think the code would only become more complex, because these 2 variables are checked and changed in different places.

Probably name inversion could be more clear

Fixed.
I modified it to use canceled.

denis-tingaikin · 2023-06-19T14:40:22Z

pkg/networkservice/common/heal/eventloop.go

+			// We need to force check the DataPlane if a down event was received from the ControlPlane
+			if !reselect {
+				deadlineCtx, deadlineCancel := context.WithDeadline(cev.chainCtx, time.Now().Add(cev.heal.livenessCheckTimeout))
+				if !cev.heal.livenessCheck(deadlineCtx, cev.conn) {
+					cev.logger.Warnf("Data plane is down")
+					reselect = true
 				}
-				// Otherwise - Start healing
-				return
+				deadlineCancel()
 			}

-			// Handle event. Start healing
-			if eventIn.GetConnections()[cev.conn.GetId()].GetState() == networkservice.State_DOWN {
+			var options []begin.Option
+			if reselect {
+				cev.logger.Debugf("Reconnect with reselect")


Suggested change

// We need to force check the DataPlane if a down event was received from the ControlPlane

if !reselect {

deadlineCtx, deadlineCancel := context.WithDeadline(cev.chainCtx, time.Now().Add(cev.heal.livenessCheckTimeout))

if !cev.heal.livenessCheck(deadlineCtx, cev.conn) {

cev.logger.Warnf("Data plane is down")

reselect = true

}

// Otherwise - Start healing

return

deadlineCancel()

}

// Handle event. Start healing

if eventIn.GetConnections()[cev.conn.GetId()].GetState() == networkservice.State_DOWN {

var options []begin.Option

if reselect {

cev.logger.Debugf("Reconnect with reselect")

var options []begin.Option

if reselect {

...

} else {

deadlineCtx, deadlineCancel := context.WithDeadline(cev.chainCtx, time.Now().Add(cev.heal.livenessCheckTimeout))

if !cev.heal.livenessCheck(deadlineCtx, cev.conn) {

cev.logger.Warnf("Data plane is down")

reselect = true

}

deadlineCancel()

}

This is also old code that was moved. Now I reverted this to make it clear that I didn't modify it.
Here reselect is not a constant flag. It is used as a state that could be switched, so it doesn't make sense to use if-else pattern.

pkg/networkservice/common/heal/eventloop.go

Signed-off-by: Danil Uzlov <DanilUzlov@yandex.ru>

denis-tingaikin · 2023-06-20T09:55:48Z

@glazychev-art Could you have a look?

…k@main PR link: networkservicemesh/sdk#1465 Commit: a8c394e Author: Danil Uzlov Date: 2023-06-20 21:27:40 +0700 Message: - Fix healing after failed refresh (#1465) * fix dialer Signed-off-by: Danil Uzlov <DanilUzlov@yandex.ru> * add tests for healing after refresh Signed-off-by: Danil Uzlov <DanilUzlov@yandex.ru> * refactor heal context for event loop Signed-off-by: Danil Uzlov <DanilUzlov@yandex.ru> * add heal started flag to heal event lopp Signed-off-by: Danil Uzlov <DanilUzlov@yandex.ru> * start monitoring after refresh error Signed-off-by: Danil Uzlov <DanilUzlov@yandex.ru> * remove test TestNSMGR_RefreshFailed_DataPlaneHealthy Signed-off-by: Danil Uzlov <DanilUzlov@yandex.ru> * fix coyright Signed-off-by: Danil Uzlov <DanilUzlov@yandex.ru> * fix linter Signed-off-by: Danil Uzlov <DanilUzlov@yandex.ru> * fix linter Signed-off-by: Danil Uzlov <DanilUzlov@yandex.ru> * bump ci Signed-off-by: Danil Uzlov <DanilUzlov@yandex.ru> * fix discover forwarder tests Signed-off-by: Danil Uzlov <DanilUzlov@yandex.ru> * improve heal monitor cleanup Signed-off-by: Danil Uzlov <DanilUzlov@yandex.ru> * remove withrefresh option, use clock in context Signed-off-by: Danil Uzlov <DanilUzlov@yandex.ru> * move monitorCtrlPlane function Signed-off-by: Danil Uzlov <DanilUzlov@yandex.ru> * use explicit returns in waitForEvents Signed-off-by: Danil Uzlov <DanilUzlov@yandex.ru> * fix typo Signed-off-by: Danil Uzlov <DanilUzlov@yandex.ru> * different fix for select forwarder tests Signed-off-by: Danil Uzlov <DanilUzlov@yandex.ru> * use better variable names in heal monitor Signed-off-by: Danil Uzlov <DanilUzlov@yandex.ru> --------- Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>

d-uzlov added 7 commits June 5, 2023 11:01

fix dialer

2e747db

Signed-off-by: Danil Uzlov <DanilUzlov@yandex.ru>

add tests for healing after refresh

dea554a

Signed-off-by: Danil Uzlov <DanilUzlov@yandex.ru>

refactor heal context for event loop

fbfe7db

Signed-off-by: Danil Uzlov <DanilUzlov@yandex.ru>

add heal started flag to heal event lopp

600c6f5

Signed-off-by: Danil Uzlov <DanilUzlov@yandex.ru>

start monitoring after refresh error

299364d

Signed-off-by: Danil Uzlov <DanilUzlov@yandex.ru>

remove test TestNSMGR_RefreshFailed_DataPlaneHealthy

27315df

Signed-off-by: Danil Uzlov <DanilUzlov@yandex.ru>

fix coyright

7fde756

Signed-off-by: Danil Uzlov <DanilUzlov@yandex.ru>

d-uzlov added 2 commits June 6, 2023 16:19

fix linter

5fe964b

Signed-off-by: Danil Uzlov <DanilUzlov@yandex.ru>

fix linter

d594673

Signed-off-by: Danil Uzlov <DanilUzlov@yandex.ru>

denis-tingaikin requested review from glazychev-art, NikitaSkrynnik, edwarnicke and denis-tingaikin June 14, 2023 09:41

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

d-uzlov force-pushed the 1457-restart-monitor branch 3 times, most recently from 5692592 to a38cab0 Compare June 19, 2023 05:21

bump ci

fcec8a9

Signed-off-by: Danil Uzlov <DanilUzlov@yandex.ru>

d-uzlov force-pushed the 1457-restart-monitor branch from a38cab0 to fcec8a9 Compare June 19, 2023 05:46

fix discover forwarder tests

0ff7602

Signed-off-by: Danil Uzlov <DanilUzlov@yandex.ru>

d-uzlov force-pushed the 1457-restart-monitor branch from 1916b2b to 0ff7602 Compare June 19, 2023 06:02

improve heal monitor cleanup

b0b1c0f

Signed-off-by: Danil Uzlov <DanilUzlov@yandex.ru>

d-uzlov force-pushed the 1457-restart-monitor branch from 945b583 to b0b1c0f Compare June 19, 2023 06:22

denis-tingaikin requested changes Jun 19, 2023

View reviewed changes

d-uzlov added 5 commits June 20, 2023 11:38

remove withrefresh option, use clock in context

ef2c537

Signed-off-by: Danil Uzlov <DanilUzlov@yandex.ru>

move monitorCtrlPlane function

a8d2015

Signed-off-by: Danil Uzlov <DanilUzlov@yandex.ru>

use explicit returns in waitForEvents

bcc63e5

Signed-off-by: Danil Uzlov <DanilUzlov@yandex.ru>

fix typo

e7b73fa

Signed-off-by: Danil Uzlov <DanilUzlov@yandex.ru>

different fix for select forwarder tests

917bde6

Signed-off-by: Danil Uzlov <DanilUzlov@yandex.ru>

denis-tingaikin approved these changes Jun 20, 2023

View reviewed changes

glazychev-art approved these changes Jun 20, 2023

View reviewed changes

denis-tingaikin merged commit a8c394e into networkservicemesh:main Jun 20, 2023

d-uzlov deleted the 1457-restart-monitor branch June 21, 2023 06:41

This was referenced Jun 21, 2023

Update from update/networkservicemesh/sdk networkservicemesh/cmd-cluster-info-k8s#72

Merged

Update from update/networkservicemesh/sdk networkservicemesh/cmd-nsc-init#631

Merged

Update from update/networkservicemesh/sdk networkservicemesh/sdk-k8s#440

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix healing after failed refresh #1465

Fix healing after failed refresh #1465

d-uzlov commented Jun 6, 2023 •

edited

Loading

codecov bot commented Jun 6, 2023 •

edited

Loading

denis-tingaikin Jun 19, 2023

d-uzlov Jun 20, 2023

denis-tingaikin Jun 19, 2023

d-uzlov Jun 20, 2023 •

edited

Loading

denis-tingaikin Jun 19, 2023

denis-tingaikin Jun 19, 2023

d-uzlov Jun 20, 2023

d-uzlov Jun 20, 2023

denis-tingaikin Jun 19, 2023

d-uzlov Jun 20, 2023

denis-tingaikin commented Jun 20, 2023

		require.Equal(t, 2, counter.Requests())
		require.Equal(t, 0, counter.Closes())

Fix healing after failed refresh #1465

Fix healing after failed refresh #1465

Conversation

d-uzlov commented Jun 6, 2023 • edited Loading

Description

Issue link

How Has This Been Tested?

Types of changes

codecov bot commented Jun 6, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d-uzlov Jun 20, 2023 • edited Loading

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

denis-tingaikin commented Jun 20, 2023

d-uzlov commented Jun 6, 2023 •

edited

Loading

codecov bot commented Jun 6, 2023 •

edited

Loading

d-uzlov Jun 20, 2023 •

edited

Loading