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 SR-IOV CNI plugin #259

Closed
wants to merge 5 commits into from
Closed

Conversation

hustcat
Copy link

@hustcat hustcat commented Jun 29, 2016

Add SR-IOV CNI plugin. #netlink needed to be bump to the latest.

@@ -0,0 +1,54 @@
# SR-IOV CNI plugin

Copy link
Member

Choose a reason for hiding this comment

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

It seems a bit empty here! Could you add an introduction to this plugin that explains what it does and what it's used for?

Copy link
Author

Choose a reason for hiding this comment

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

Introduction added:)


```
# vi /etc/modprobe.conf
options ixgbe max_vfs=8,8
Copy link
Member

@steveej steveej Jul 11, 2016

Choose a reason for hiding this comment

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

is this a requirement for the plugin to work or just an example?

Copy link
Author

Choose a reason for hiding this comment

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

It's requirement. SR-IOV plugin need NIC enable SR-IOV capability, which not enabled by default.

@steveej
Copy link
Member

steveej commented Jul 11, 2016

In general this plugin looks fine and I'm in favor of merging this PR. A couple of notes:

@hustcat
Copy link
Author

hustcat commented Jul 21, 2016

@steveej I add some tests, but didn't enable it by default. You can run it with SR-IOV NIC. Also, you can run example in docment. Any problems let me know please:)

@rosenhouse
Copy link
Contributor

@steveej @hustcat I'm not sure how I feel about us having plugins with tests that we can't actually run in CI. I think that us maintaining the plugin in the repo implies a certain level of support and endorsement, which I expect to be backed-up by test coverage running in our CI.

Perhaps such plugins may be better maintained by 3rd parties.

One way to address this could be to maintain a list in our readme, pointing to 3rd party plugins living in other repos. So there can be some level of information sharing and social capital, without us taking on the burden of maintaining those plugins

Thoughts? @containernetworking/cni-maintainers

@hustcat
Copy link
Author

hustcat commented Jul 22, 2016

Created a standalone repo here.

@rosenhouse
Copy link
Contributor

Thanks @hustcat I've opened a separate PR #272 that adds your standalone repo to the list of 3rd party CNI plugins.

@rosenhouse
Copy link
Contributor

@hustcat That README PR has been merged -- so your plugin repo is now linked to from the main page.

I'm going to close this PR now. Feel free to re-open if you'd like us to re-consider your plugin for merging into this repo, but then I'd ask that we figure out how to run the test suite in our CI.

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

Successfully merging this pull request may close these issues.

3 participants