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 xconnectns, l2ovsconnect and mechanism chain elements #1

Merged
merged 14 commits into from
Aug 9, 2021

Conversation

pperiyasamy
Copy link
Member

This PR adds the relevant chain elements needed in sdk-ovs for basic l2 connectivity between client and endpoint.

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

@pperiyasamy pperiyasamy force-pushed the chain-elements branch 2 times, most recently from a3b4ced to 625b5cf Compare May 28, 2021 14:33
@denis-tingaikin
Copy link
Member

@pperiyasamy Could you copy ci files from SDK and provide them to this repo sepratly? Currently, this repo has not ci. I think it would be cool to run linters, builds, tests, and so on this PR :)

@edwarnicke
Copy link
Member

@pperiyasamy I'd highly recommend starting with robust CI and then moving files in. We tend to turn on a lot of golangci-lint checks for example. Its much much easier to start from there than to try to bring them in later.

@@ -0,0 +1,75 @@
// Copyright (c) 2021 Nordix Foundation.
Copy link
Member

Choose a reason for hiding this comment

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

In sdk-vpp I choose to omit the 'common' because, the chain elements were all about vpp.
So rather than pkg/networkservice/common/mechanisms/kernel/client.go I had pkg/networkservice/mechanisms/kernel/client.go etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

@edwarnicke done. the kernel mechanism with token id has to be revisited later (would need to realign those chain elements a bit) once resource pool and client pr is checked in.

Signed-off-by: Periyasamy Palanisamy <periyasamy.palanisamy@est.tech>
Signed-off-by: Periyasamy Palanisamy <periyasamy.palanisamy@est.tech>
Signed-off-by: Periyasamy Palanisamy <periyasamy.palanisamy@est.tech>
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.

@edwarnicke Can we merge this?

Signed-off-by: Periyasamy Palanisamy <periyasamy.palanisamy@est.tech>
Signed-off-by: Periyasamy Palanisamy <periyasamy.palanisamy@est.tech>
Signed-off-by: Periyasamy Palanisamy <periyasamy.palanisamy@est.tech>
…ements

Signed-off-by: Periyasamy Palanisamy <periyasamy.palanisamy@est.tech>
Signed-off-by: Periyasamy Palanisamy <periyasamy.palanisamy@est.tech>
having l2ovsconnect server chain element after connect server
breaks the vWire connection and invokes close handler after
connection establishment.

Signed-off-by: Periyasamy Palanisamy <periyasamy.palanisamy@est.tech>
@pperiyasamy pperiyasamy requested a review from Bolodya1997 August 5, 2021 07:55
@@ -18,7 +18,7 @@ jobs:
runs-on: ${{ matrix.os }}
strategy:
matrix:
os: [ubuntu-latest, macos-latest, windows-latest]
os: [ubuntu-latest]
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest keeping the macos-latest and windows-latest here. Not because your code will run on them (obviously it won't) but because it forces you to proper discipline about applying build tags to mark out code that doesn't run on them. That way if someone tries to pull your code for local development... they don't get weird build errors.

Copy link
Member Author

Choose a reason for hiding this comment

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

@edwarnicke I tried to downgrade of nettlink library into v1.1.0 version to make ci to succeed for macOS and windows as well, but Mellanox's sriovnet needs its latest version, because of this ci failing early in lint and ubuntu checks. this was a reason why I made ci to run only ubuntu platform ;) do you've suggestion to fix this ?

Copy link
Member

Choose a reason for hiding this comment

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

@pperiyasamy Don't fret about the netlink lib. Here's what I've done in sdk-vpp.

In places I am using the netllink lib (or other linux-ism), I just set the proper build tags:

// +build linux

(this will help a lot )

Then for the things that import the thing I marked with // build + linux, I also mark them.

Your issue isn't that you are using a version of netlink that doesn't work on Mac OS (no version of netlink will work on Mac OS). Its that you aren't marking things as only working on linux with build tags.

Does that help?

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks @edwarnicke, it helps. done.

Signed-off-by: Periyasamy Palanisamy <periyasamy.palanisamy@est.tech>
Signed-off-by: Periyasamy Palanisamy <periyasamy.palanisamy@est.tech>
this would stabilize vwire connection when connect server's Request
fails due to context cancel error.

Signed-off-by: Periyasamy Palanisamy <periyasamy.palanisamy@est.tech>
Signed-off-by: Periyasamy Palanisamy <periyasamy.palanisamy@est.tech>
Signed-off-by: Periyasamy Palanisamy <periyasamy.palanisamy@est.tech>
@edwarnicke edwarnicke merged commit 7ba6f46 into networkservicemesh:main Aug 9, 2021
@pperiyasamy pperiyasamy deleted the chain-elements branch August 26, 2021 06:53
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.

3 participants