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.

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

Rework timeout chain element #520

Merged
merged 19 commits into from
Nov 3, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/ci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ jobs:
go-version: 1.15
- name: Install proto-gen-go
run: go get -u github.com/golang/protobuf/[email protected]
- name: Install proto-gen-go
- name: Install go-syncmap
run: go get github.com/searKing/golang/tools/cmd/go-syncmap
- name: Generate files
run: go generate ./...
Expand Down
6 changes: 4 additions & 2 deletions pkg/networkservice/chains/endpoint/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,13 +61,15 @@ type endpoint struct {
// - additionalFunctionality - any additional NetworkServiceServer chain elements to be included in the chain
func NewServer(ctx context.Context, name string, authzServer networkservice.NetworkServiceServer, tokenGenerator token.GeneratorFunc, additionalFunctionality ...networkservice.NetworkServiceServer) Endpoint {
rv := &endpoint{}
var ns networkservice.NetworkServiceServer = rv
rv.NetworkServiceServer = chain.NewNetworkServiceServer(
append([]networkservice.NetworkServiceServer{
authzServer,
updatepath.NewServer(name),
// `timeout` uses ctx as a context for the timeout Close and it closes only the subsequent chain, so
// chain elements before the `timeout` in chain shouldn't make any updates to the Close context and
// shouldn't be closed on Connection Close.
timeout.NewServer(ctx),
Bolodya1997 marked this conversation as resolved.
Show resolved Hide resolved
monitor.NewServer(ctx, &rv.MonitorConnectionServer),
timeout.NewServer(&ns),
updatetoken.NewServer(tokenGenerator),
}, additionalFunctionality...)...)
return rv
Expand Down
46 changes: 38 additions & 8 deletions pkg/networkservice/common/externalips/string_map.gen.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

46 changes: 38 additions & 8 deletions pkg/networkservice/common/interpose/connection_info_map.gen.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

47 changes: 47 additions & 0 deletions pkg/networkservice/common/timeout/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
# Functional requirements

If authentication token of the previous connection path element expires, it does mean that we lost connection with the
previous NSM-based application. For such case connection should be closed.

# Implementation

## timeoutServer

timeoutServer keeps timers [timeout.timerMap](https://github.com/networkservicemesh/sdk/blob/master/pkg/networkservice/common/timeout/gen.go#L26)
mapping incoming request Connection.ID to a timeout timer firing Close on the subsequent chain after the connection previous
path element expires. To prevent simultaneous execution of multiple Request, Close event for the same Connection.ID in parallel
it also keeps executors [timeout.executorMap](https://github.com/networkservicemesh/sdk/blob/master/pkg/networkservice/common/timeout/gen.go#L27)
mapping request Connection.ID to an executor for serializing all Request, Close event for the mapped Connection.ID.

timeoutServer closes only subsequent chain elements and uses base context for the Close. So all the chain elements in
the chain before the timeoutServer shouldn't be closed with Close and shouldn't set any required data to the Close context.

Example of correct timeoutServer usage in [endpoint.NewServer](https://github.com/networkservicemesh/sdk/blob/master/pkg/networkservice/chains/endpoint/server.go#L62):
```go
rv.NetworkServiceServer = chain.NewNetworkServiceServer(
append([]networkservice.NetworkServiceServer{
authzServer, // <-- shouldn't be closed, don't set anything to the context
updatepath.NewServer(name), // <-- same
timeout.NewServer(ctx), // <-- timeoutServer
monitor.NewServer(ctx, &rv.MonitorConnectionServer), // <-- should be closed
updatetoken.NewServer(tokenGenerator), // <-- should be closed
}, additionalFunctionality...)...) // <-- should be closed, sets data to context
```

## Comments on concurrency characteristics

Concurrency is managed through type specific wrappers of [sync.Map](https://golang.org/pkg/sync/#Map) and with
per-connection [serialize.Executor](https://github.com/edwarnicke/serialize/blob/master/serialize.go) which are created on
Request and deleted on Close.

Since we are deleting the per-connection executor on connection Close, there possibly can be a race condition:
```
1. -> timeout close : locking executor
2. -> request-1 : waiting on executor
3. timeout close -> : unlocking executor, removing it from executors
4. -> request-2 : creating exec, storing into executors, locking exec
5. -request-1-> : locking executor, trying to store it into executors
```
at 5. we get request-1 locking executor, request-2 locking exec and only exec stored in executors. It means that
request-2 and all subsequent events will be executed in parallel with request-1.
So we check for this [here](https://github.com/networkservicemesh/sdk/blob/master/pkg/networkservice/common/timeout/server.go#L68).
75 changes: 75 additions & 0 deletions pkg/networkservice/common/timeout/executor_map.gen.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading