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

Fix registry client #110

Merged
merged 11 commits into from
Feb 20, 2020
Merged

Fix registry client #110

merged 11 commits into from
Feb 20, 2020

Conversation

lvfxx
Copy link
Member

@lvfxx lvfxx commented Feb 17, 2020

Fixes to make clientInfo NetworkServiceRegistryClient tests work

edwarnicke
edwarnicke reviewed Feb 17, 2020
View reviewed changes
pkg/registry/core/next/registry_client.go Outdated
}

func (n *nextRegistryClient) BulkRegisterNSE(ctx context.Context, opts ...grpc.CallOption) (registry.NetworkServiceRegistry_BulkRegisterNSEClient, error) {
if n.index+1 < len(n.clients) {
return n.clients[n.index].BulkRegisterNSE(withNextRegistryClient(ctx, &nextRegistryClient{clients: n.clients, index: n.index + 1}), opts...)
}
return n.clients[n.index].BulkRegisterNSE(withNextRegistryClient(ctx, nil), opts...)
return n.clients[n.index].BulkRegisterNSE(withNextRegistryClient(ctx, &tailRegistryClient{}), opts...)
Copy link
Member

Choose a reason for hiding this comment

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

Move the &tailRegistryClient{} to context.go (see #106 for the example for chaining networkservice).

Copy link
Member

Choose a reason for hiding this comment

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

This makes it safe to run a chain element even its not in a chain.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, did it

edwarnicke
edwarnicke requested changes Feb 17, 2020
View reviewed changes
Copy link
Member

@edwarnicke edwarnicke left a comment

Choose a reason for hiding this comment

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

See comments about putting *tailRegistryClient{} in the context.go

denis-tingaikin
denis-tingaikin reviewed Feb 17, 2020
View reviewed changes
Copy link
Member

@denis-tingaikin denis-tingaikin left a comment

Choose a reason for hiding this comment

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

Let's add a unit test for this issue

denis-tingaikin
denis-tingaikin requested changes Feb 17, 2020
View reviewed changes
pkg/registry/core/next/tail_registry_client.go

// tailRegistryClient is a simple implementation of registry.NetworkServiceRegistryClient that is called at the end
// of a chain to ensure that we never call a method on a nil object
type tailRegistryClient struct{}
Copy link
Member

@denis-tingaikin denis-tingaikin Feb 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Do we need to add the same thing for next.RegistryServer?
I guess we need to check that pkg/registry/next for RegistryClient/RegistryServer is working as expected.
Let's cover pkg/registry/next by unit tests and fix problems.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we need. I've added it

@semenov-spirent
Fixed typo
394af04 
Signed-off-by: Sergey Semenov <sergey.semenov@xored.com>
@semenov-spirent
Added partially implemented tailRegistryClient
6cbe807 
Signed-off-by: Sergey Semenov <sergey.semenov@xored.com>
@semenov-spirent
Implemented nil stubs
cb90904 
Signed-off-by: Sergey Semenov <sergey.semenov@xored.com>
@semenov-spirent
Moved tailRegistryClient creation to registry context
15ecb38 
Signed-off-by: Sergey Semenov <sergey.semenov@xored.com>

Signed-off-by: Sergey Semenov <sergey.semenov@xored.com>
@semenov-spirent
Added rv nil case
8d4d643 
Signed-off-by: Sergey Semenov <sergey.semenov@xored.com>

Signed-off-by: Sergey Semenov <sergey.semenov@xored.com>
@semenov-spirent
Added tailRegistryServer
0954439 
Signed-off-by: Sergey Semenov <sergey.semenov@xored.com>
@semenov-spirent
Added both client and server keys checking for adapters usage case
bca6ed7 
Signed-off-by: Sergey Semenov <sergey.semenov@xored.com>
@semenov-spirent
Added empty/nil checks
c4d75cf 
Signed-off-by: Sergey Semenov <sergey.semenov@xored.com>
@semenov-spirent
Added tests for pkg/registry/core/next
31c6aca 
Signed-off-by: Sergey Semenov <sergey.semenov@xored.com>
@lvfxx lvfxx force-pushed the registry-client-fixes branch from 1195ace to 31c6aca Compare February 18, 2020 11:44
denis-tingaikin
denis-tingaikin approved these changes Feb 18, 2020
View reviewed changes
Copy link
Member

@denis-tingaikin denis-tingaikin left a comment

Choose a reason for hiding this comment

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

Looks good to me

edwarnicke
edwarnicke reviewed Feb 18, 2020
View reviewed changes
pkg/registry/core/next/registry_server.go
func NewRegistryServer(server []registry.NetworkServiceRegistryServer) registry.NetworkServiceRegistryServer {
return NewWrappedRegistryServer(nil, server...)
func NewRegistryServer(servers ...registry.NetworkServiceRegistryServer) registry.NetworkServiceRegistryServer {
if len(servers) == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

Nice catch!

edwarnicke
edwarnicke requested changes Feb 18, 2020
View reviewed changes
Copy link
Member

@edwarnicke edwarnicke left a comment

Choose a reason for hiding this comment

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

Can we move pkg/registry/core/next/tests/ up to pkg/registry/core/next/ using package 'next_test' in keeping with standard go idioms?

@semenov-spirent
Moved test to to pkg/registry/core/next/ using package 'next_test'
b34d6f8 
Signed-off-by: Sergey Semenov <sergey.semenov@xored.com>
@semenov-spirent
Fixed names
4eca762 
Signed-off-by: Sergey Semenov <sergey.semenov@xored.com>
@lvfxx lvfxx requested review from edwarnicke and haiodo February 19, 2020 04:34
@edwarnicke edwarnicke merged commit b8ce689 into networkservicemesh:master Feb 20, 2020
@lvfxx lvfxx deleted the registry-client-fixes branch February 21, 2020 05:17
nsmbot pushed a commit that referenced this pull request Sep 7, 2021
Update go.mod and go.sum to latest version from networkservicemesh/ap…
834f783 
…i@main

PR link: networkservicemesh/api#110

Commit: 9a36433
Author: Vladimir Popov
Date: 2021-09-08 02:48:27 +0700
Message:
  - [sdk-vpp#314] Add SR-IOV token ID mechanism parameter for kernel, VFIO mechanisms (#110)
* Add SR-IOV token ID mechanism parameter for kernel, VFIO mechanisms

Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>

* Renam SR-IOV token ID to just token ID

Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>

* Rename token ID to device token ID

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
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 approved these changes

@edwarnicke edwarnicke Awaiting requested review from edwarnicke

@haiodo haiodo Awaiting requested review from haiodo

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lvfxx @edwarnicke @denis-tingaikin @semenov-spirent