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

add sriov token server chain element #249

Merged

Conversation

pperiyasamy
Copy link
Member

this attempts to provide basis for NSE to inject dedicated VF
for every client connection.

Fixes #246

Signed-off-by: Periyasamy Palanisamy periyasamy.palanisamy@est.tech

this attempts to provide basis for NSE to inject dedicated VF
for every client connection.

Signed-off-by: Periyasamy Palanisamy <periyasamy.palanisamy@est.tech>
tokens[nameIDs[0]] = strings.Split(nameIDs[1], ",")
}
return tokens
}

// GetTokensFromEnv returns stored token ids from env for given tokenKey
func GetTokensFromEnv(envs []string, tokenKey string) map[string][]string {

Choose a reason for hiding this comment

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

Why does it return map[string][]string instead of just []string?
Do we actually need this method instead of using FromEnv(envs)[tokenKey]?

Copy link
Member Author

@pperiyasamy pperiyasamy Aug 19, 2021

Choose a reason for hiding this comment

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

in case of token server chain element, we should just retrieve tokens for only one token key and also to make compatible with tokenElement#tokens, I added this method. are you not liking it ?

Choose a reason for hiding this comment

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

Got it. What do you think about doing like this: #249 (comment).

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

}

func (s *tokenServer) Request(ctx context.Context, request *networkservice.NetworkServiceRequest) (*networkservice.Connection, error) {
if mechanism := kernel.ToMechanism(request.GetConnection().GetMechanism()); mechanism != nil || mechanism.GetPCIAddress() == "" {

Choose a reason for hiding this comment

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

I suppose that it would be better to check here for mechanism.Parameters[resourcepool.TokenIDKey] == "" to prevent allocating new tokens on refresh Requests.
It looks like mechanism.GetPCIAddress() == "" does the same, but probably checking for token ID would be more meaningful here.

Copy link
Member Author

Choose a reason for hiding this comment

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

sure, changed it to use token id check.

@Bolodya1997
Copy link

@pperiyasamy
I made a mistake in original client - it doesn't release token ID if initial Request fails. Can you please fix it in this PR?

func NewServer(tokenKey string) networkservice.NetworkServiceServer {
return &tokenServer{
tokenName: tokenKey,
config: createTokenElement(tokens.GetTokensFromEnv(os.Environ(), tokenKey)),

Choose a reason for hiding this comment

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

Suggested change
config: createTokenElement(tokens.GetTokensFromEnv(os.Environ(), tokenKey)),
config: createTokenElement(map[string][]string{
tokenKey: tokens.FromEnv(os.Environ())[tokenKey],
}),

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Signed-off-by: Periyasamy Palanisamy <periyasamy.palanisamy@est.tech>
@pperiyasamy
Copy link
Member Author

@pperiyasamy
I made a mistake in original client - it doesn't release token ID if initial Request fails. Can you please fix it in this PR?

yes @Bolodya1997 done for both client and server.

@pperiyasamy
Copy link
Member Author

@denis-tingaikin are we ok to merge this change ?

@denis-tingaikin
Copy link
Member

LGTM

@denis-tingaikin denis-tingaikin merged commit cc4115c into networkservicemesh:main Aug 31, 2021
nsmbot pushed a commit to networkservicemesh/cmd-nsc that referenced this pull request Aug 31, 2021
…k-sriov@main

PR link: networkservicemesh/sdk-sriov#249

Commit: cc4115c
Author: Denis Tingaikin
Date: 2021-08-31 18:06:40 +0300
Message:
  - Merge pull request #249 from Nordix/sriov-token-server-elem
Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-forwarder-sriov that referenced this pull request Aug 31, 2021
…k-sriov@main

PR link: networkservicemesh/sdk-sriov#249

Commit: cc4115c
Author: Denis Tingaikin
Date: 2021-08-31 18:06:40 +0300
Message:
  - Merge pull request #249 from Nordix/sriov-token-server-elem
Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
@pperiyasamy pperiyasamy deleted the sriov-token-server-elem branch September 1, 2021 07:20
@pperiyasamy
Copy link
Member Author

Thanks @denis-tingaikin !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

implement sriov token server chain element
3 participants