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

[sdk-vpp#314] Use token ID key from api #227

Merged
merged 1 commit into from
Sep 13, 2021
Merged

[sdk-vpp#314] Use token ID key from api #227

merged 1 commit into from
Sep 13, 2021

Conversation

Bolodya1997
Copy link

@Bolodya1997 Bolodya1997 commented Aug 2, 2021 •
edited
Loading

Description

Starts SR-IOV token ID mechanism parameter from api.

Depends on networkservicemesh/api#110.

Issue link

Needed for networkservicemesh/sdk-vpp#314.

How Has This Been Tested?

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

Types of changes

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

@Bolodya1997 Bolodya1997 mentioned this pull request Aug 20, 2021
Closed
@Bolodya1997 Bolodya1997 force-pushed the sdk-vpp#314/token-id-key branch from 577204f to 21873d0 Compare September 3, 2021 08:02
@Bolodya1997 Bolodya1997 mentioned this pull request Sep 3, 2021
Closed
9 tasks
Bolodya1997
Bolodya1997 commented Sep 3, 2021
View reviewed changes
pkg/networkservice/common/resourcepool/server.go
if !ok {
logger.Infof("no token id present for client connection %v", conn)
return next.Server(ctx).Request(ctx, request)
return nil, errors.New("no token ID provided")
Copy link
Author

Choose a reason for hiding this comment

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

@pperiyasamy
What do you think of returning an error in such case and so using it with a switchcase server for the complex chains?
Example of usage with switchcase - https://github.com/networkservicemesh/sdk-vpp/blob/70d7b5353d50c9c81b5de0978d4d040cc22a4d18/pkg/networkservice/chains/xconnectns/server.go#L119-L124.

Copy link
Member

Choose a reason for hiding this comment

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

yes @Bolodya1997, this is a perfect case for using switch case chain element, I will change this and adapt sdk-ovs to use switch case chain element to align for this change.

Copy link
Member

Choose a reason for hiding this comment

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

@Bolodya1997 raised a review in sdk-ovs to address this with switchcase networkservicemesh/sdk-ovs#16, so please go ahead with returning an error here.

@Bolodya1997 Bolodya1997 force-pushed the sdk-vpp#314/token-id-key branch from 21873d0 to 61efa2d Compare September 8, 2021 10:55
@Bolodya1997 Bolodya1997 marked this pull request as ready for review September 8, 2021 10:56
@denis-tingaikin
Copy link
Member

@Bolodya1997 Could you resolve conflicts?

@edwarnicke
Copy link
Member

@Bolodya1997 How are things going looking at just doing the endpoint Switch approach in cmd-forwarder-vpp ?

Switch to using common.DeviceTokenID and special SR-IOV tokens from r…
6d36aa7 
…esourcepool.TokenID

Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>
@Bolodya1997 Bolodya1997 force-pushed the sdk-vpp#314/token-id-key branch from 61efa2d to 6d36aa7 Compare September 13, 2021 08:34
@Bolodya1997
Copy link
Author

@Bolodya1997 How are things going looking at just doing the endpoint Switch approach in cmd-forwarder-vpp ?

Everything is great:

  1. PR introducing endpoint.Combine - [sdk-vpp#314] Add endpoint.Combine sdk#1077
  2. endpoint.Combine usage in cmd-forwarder-vpp - https://github.com/networkservicemesh/cmd-forwarder-vpp/blob/d50f7aab7dde0350e91509eb182784dc3b6a29fb/internal/xconnectns/server.go

We still need this PR to sdk-sriov to make everything work with common.DeviceTokenIDKey from api.

@edwarnicke
Copy link
Member

We still need this PR to sdk-sriov to make everything work with common.DeviceTokenIDKey from api.

@Bolodya1997 Why does sdk-vpp need to know anything about DeviceTokenID?

@Bolodya1997
Copy link
Author

We still need this PR to sdk-sriov to make everything work with common.DeviceTokenIDKey from api.

@Bolodya1997 Why does sdk-vpp need to know anything about DeviceTokenID?

It is a PR to sdk-sriov, sdk-vpp#314 is an issue ID.

@edwarnicke
Copy link
Member

Doh... apologies :)

@edwarnicke edwarnicke merged commit 05bb114 into networkservicemesh:main Sep 13, 2021
nsmbot pushed a commit to networkservicemesh/cmd-nsc that referenced this pull request Sep 13, 2021
Update go.mod and go.sum to latest version from networkservicemesh/sd…
98bb879 
…k-sriov@main

PR link: networkservicemesh/sdk-sriov#227

Commit: 05bb114
Author: Ed Warnicke
Date: 2021-09-13 10:19:09 -0500
Message:
  - Merge pull request #227 from Bolodya1997/sdk-vpp#314/token-id-key
Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-nsc-init that referenced this pull request Sep 13, 2021
Update go.mod and go.sum to latest version from networkservicemesh/sd…
374168b 
…k-sriov@main

PR link: networkservicemesh/sdk-sriov#227

Commit: 05bb114
Author: Ed Warnicke
Date: 2021-09-13 10:19:09 -0500
Message:
  - Merge pull request #227 from Bolodya1997/sdk-vpp#314/token-id-key
Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/sdk-ovs that referenced this pull request Sep 13, 2021
Update go.mod and go.sum to latest version from networkservicemesh/sd…
7237bfb 
…k-sriov@main

PR link: networkservicemesh/sdk-sriov#227

Commit: 05bb114
Author: Ed Warnicke
Date: 2021-09-13 10:19:09 -0500
Message:
  - Merge pull request #227 from Bolodya1997/sdk-vpp#314/token-id-key
Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-forwarder-sriov that referenced this pull request Sep 13, 2021
Update go.mod and go.sum to latest version from networkservicemesh/sd…
3ef287d 
…k-sriov@main

PR link: networkservicemesh/sdk-sriov#227

Commit: 05bb114
Author: Ed Warnicke
Date: 2021-09-13 10:19:09 -0500
Message:
  - Merge pull request #227 from Bolodya1997/sdk-vpp#314/token-id-key
Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
@nsmbot nsmbot mentioned this pull request Sep 13, 2021
Closed
@nsmbot nsmbot mentioned this pull request Sep 13, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pperiyasamy pperiyasamy pperiyasamy approved these changes

@Mixaster995 Mixaster995 Mixaster995 approved these changes

@glazychev-art glazychev-art glazychev-art approved these changes

@denis-tingaikin denis-tingaikin Awaiting requested review from denis-tingaikin

@edwarnicke edwarnicke Awaiting requested review from edwarnicke

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

Successfully merging this pull request may close these issues.
6 participants
@Bolodya1997 @denis-tingaikin @edwarnicke @glazychev-art @Mixaster995 @pperiyasamy