Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[qfix] Add WithEndpointChange heal option #1082

Merged
merged 1 commit into from
Sep 23, 2021
Merged

[qfix] Add WithEndpointChange heal option #1082

merged 1 commit into from
Sep 23, 2021

Conversation

Bolodya1997
Copy link

Description

Adds WithEndpointChange heal option to be enabled only on NSC.

How Has This Been Tested?

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

Types of changes

  • Bug fix
  • New functionallity
  • Documentation
  • Refactoring
  • CI

Add WithEndpointChange heal option
fda8076 
Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>
@Bolodya1997 Bolodya1997 added this to Review in progress in Issue/PR tracking Sep 20, 2021
denis-tingaikin
denis-tingaikin reviewed Sep 20, 2021
View reviewed changes
pkg/networkservice/common/heal/option.go
@@ -67,8 +67,16 @@ func WithRestoreTimeout(restoreTimeout time.Duration) Option {
}
}

// WithEndpointChange sets if Connection.EndpointName can be changed with monitor updates
func WithEndpointChange() Option {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Bolodya1997 Could you please explain in what cases we plan do not use this option?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is NSC-only option. Do you mean adding such comment in code?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite follow why do we need the option only for nscs.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We currently have the following on the Request:

  1. NSC -> NSMgr : endpointName == ""
  2. NSMgr -> Forwarder -> NSMgr -> Passthrough : endpointName == "Passthrough"
  3. Passthrough -> NSMgr : endpointName == ""
  4. NSMgr -> Forwarder -> NSMgr -> NSE : endpointName == "NSE"
  5. Passthrough <- NSMgr <- Forwarder <- NSMgr <- NSE : endpointName == "NSE"
  6. NSC <- NSMgr <- Forwarder <- NSMgr <- Passthrough : endpointName == "Passthrough"

On refresh Request we should have the following:

  1. NSC -> NSMgr -> Forwarder -> NSMgr -> Passthrough : endpointName == "Passthrough"
  2. Passthrough -> NSMgr : endpointName == ""
  3. NSMgr -> Forwarder -> NSMgr -> NSE : endpointName == "NSE"
  4. Passthrough <- NSMgr <- Forwarder <- NSMgr <- NSE : endpointName == "NSE"
  5. NSC <- NSMgr <- Forwarder <- NSMgr <- Passthrough : endpointName == "Passthrough"

But because of heal client standing in the chain before the replacelabels client after refresh happen in NSMgr (Passthrough -> NSMgr) we will have the following:

  1. NSC -> NSMgr -> Forwarder -> NSMgr -> Passthrough : endpointName == "Passthrough"
  2. Passthrough -> NSMgr -> Forwarder -> NSMgr -> NSE : endpointName == "NSE"
  3. NSC <- NSMgr <- Forwarder <- NSMgr <- Passthrough <- NSMgr <- Forwarder <- NSMgr <- NSE : endpointName == "NSE"

And so subsequent refresh Request will fail because of invalid Endpoint name.

We cannot move heal client after additionalFunctionality clients, because it will lead to problems with mechanismtranslation client. So we can just disable endpointName update for all applications except NSC.

@denis-tingaikin
Copy link
Member

I don't like adding more complexity into heal, but we are planning to remove and replace exist heal logic.

Motviation

  1. [qfix] Add WithEndpointChange heal option #1082 (comment)
  2. This PR is required for Issue running nse-composition example deployments-k8s#2381

So merging this as a part of solution for networkservicemesh/deployments-k8s#2381

@denis-tingaikin denis-tingaikin merged commit 55adc08 into networkservicemesh:main Sep 23, 2021
Issue/PR tracking automation moved this from Review in progress to Done Sep 23, 2021
nsmbot pushed a commit to networkservicemesh/sdk-k8s that referenced this pull request Sep 23, 2021
Update go.mod and go.sum to latest version from networkservicemesh/sd…
e8358d8 
…k@main

PR link: networkservicemesh/sdk#1082

Commit: 55adc08
Author: Vladimir Popov
Date: 2021-09-23 16:54:20 +0700
Message:
  - Add WithEndpointChange heal option (#1082)
Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/sdk-kernel that referenced this pull request Sep 23, 2021
Update go.mod and go.sum to latest version from networkservicemesh/sd…
10104af 
…k@main

PR link: networkservicemesh/sdk#1082

Commit: 55adc08
Author: Vladimir Popov
Date: 2021-09-23 16:54:20 +0700
Message:
  - Add WithEndpointChange heal option (#1082)
Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-nse-vfio that referenced this pull request Sep 23, 2021
Update go.mod and go.sum to latest version from networkservicemesh/sd…
0ac42d8 
…k@main

PR link: networkservicemesh/sdk#1082

Commit: 55adc08
Author: Vladimir Popov
Date: 2021-09-23 16:54:20 +0700
Message:
  - Add WithEndpointChange heal option (#1082)
Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-registry-memory that referenced this pull request Sep 23, 2021
Update go.mod and go.sum to latest version from networkservicemesh/sd…
6b05c86 
…k@main

PR link: networkservicemesh/sdk#1082

Commit: 55adc08
Author: Vladimir Popov
Date: 2021-09-23 16:54:20 +0700
Message:
  - Add WithEndpointChange heal option (#1082)
Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-registry-proxy-dns that referenced this pull request Sep 23, 2021
Update go.mod and go.sum to latest version from networkservicemesh/sd…
5c41cc2 
…k@main

PR link: networkservicemesh/sdk#1082

Commit: 55adc08
Author: Vladimir Popov
Date: 2021-09-23 16:54:20 +0700
Message:
  - Add WithEndpointChange heal option (#1082)
Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-nsc-init that referenced this pull request Sep 23, 2021
Update go.mod and go.sum to latest version from networkservicemesh/sd…
a2cfaaf 
…k@main

PR link: networkservicemesh/sdk#1082

Commit: 55adc08
Author: Vladimir Popov
Date: 2021-09-23 16:54:20 +0700
Message:
  - Add WithEndpointChange heal option (#1082)
Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-nsmgr-proxy that referenced this pull request Sep 23, 2021
Update go.mod and go.sum to latest version from networkservicemesh/sd…
86d6fbb 
…k@main

PR link: networkservicemesh/sdk#1082

Commit: 55adc08
Author: Vladimir Popov
Date: 2021-09-23 16:54:20 +0700
Message:
  - Add WithEndpointChange heal option (#1082)
Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-nsmgr that referenced this pull request Sep 23, 2021
Update go.mod and go.sum to latest version from networkservicemesh/sd…
0775d34 
…k@main

PR link: networkservicemesh/sdk#1082

Commit: 55adc08
Author: Vladimir Popov
Date: 2021-09-23 16:54:20 +0700
Message:
  - Add WithEndpointChange heal option (#1082)
Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-nse-icmp-responder that referenced this pull request Sep 23, 2021
Update go.mod and go.sum to latest version from networkservicemesh/sd…
ccdeb38 
…k@main

PR link: networkservicemesh/sdk#1082

Commit: 55adc08
Author: Vladimir Popov
Date: 2021-09-23 16:54:20 +0700
Message:
  - Add WithEndpointChange heal option (#1082)
Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
This was referenced Sep 23, 2021
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
@denis-tingaikin denis-tingaikin moved this from Done to Recently merged PRs in Issue/PR tracking Sep 23, 2021
@denis-tingaikin denis-tingaikin moved this from Recently merged PRs to Done in Issue/PR tracking Oct 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@denis-tingaikin denis-tingaikin denis-tingaikin left review comments

@Mixaster995 Mixaster995 Awaiting requested review from Mixaster995

@glazychev-art glazychev-art Awaiting requested review from glazychev-art

Assignees
No one assigned
Labels
None yet
Projects
Issue/PR tracking
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Bolodya1997 @denis-tingaikin