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

plugins/main/ptp: support for creating tap devices #271

Closed
wants to merge 4 commits into from

Conversation

lukasredynk
Copy link
Contributor

It's prototype code to move forward with discussion at #251

If non-empty CNI_VIRT is passed in env variable then instead of creating veth pair there's created tap interface and passed via iface field. Most of code changes here is related to updating netlink version, also in vendor clone I have to add additional codepath for specifying custom flags for tap creation.

Related PR: vishvananda/netlink#154

return "", fmt.Errorf("failed to generate random veth name: %v", err)
}

// NetworkManager (recent versions) will ignore veth devices that start with "veth"
Copy link
Member

Choose a reason for hiding this comment

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

This comment seems misleading in this place.

@jellonek
Copy link
Member

IMO this PR should be divided into 2 separate PRs - one with netlink update, second with changes for tap devices.

Farther - with small comment correction - LGTM.

@lukasredynk
Copy link
Contributor Author

Yeah, I agree, the netlink update is quite big and makes it more difficult to review code.

* Moved tap creatin to ip/link package
* Tap name is now generated similar to veth
* Enabled libcni to create tap devices
@antoni
Copy link

antoni commented Aug 24, 2016

This PR has been rebased after the merge of #274.

@jjlakis
Copy link

jjlakis commented Aug 24, 2016

PTAL @steveej @jellonek

@jellonek
Copy link
Member

LGTM

@pskrzyns
Copy link

@feiskyer @containernetworking/cni-maintainers PTAL

@feiskyer
Copy link
Contributor

Is it possible to add a test for this?

@lukasredynk lukasredynk changed the title [WIP] plugins/main/ptp: experimental support for creating tap devices plugins/main/ptp: support for creating tap devices Aug 29, 2016
@antoni
Copy link

antoni commented Aug 29, 2016

@feiskyer I have added TAP device test for the ptp plugin.

@jellonek
Copy link
Member

LGTM

/cc @steveej

@rosenhouse
Copy link
Contributor

This addition of the Iface field to the result data looks a lot like #145. In particular, it requires a spec change, which would require a version bump.

@lukasredynk
Copy link
Contributor Author

lukasredynk commented Sep 7, 2016

Yeah, but changes to input/output are less intrusive. Ok, I'll prepare changes to spec. As for version: 0.2.0 or 0.1.1? Given, that all fields added are purely optional I was thinking about 0.1.1.

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.

None yet

8 participants