search

openshift / openshift-sdn

Apache License 2.0
69 stars 63 forks source link

[DO NOT MERGE] Convert OpenShift SDN to a CNI plugin #320

Closed dcbw closed 8 years ago

dcbw commented 8 years ago

Based off @danwinship's plugin reorg branch, so start with the "Use CNI for IPAM instead of docker" commit. Not quite ready for merge yet, but I'd love feedback on the approach, especially the pod info server parts.

cc @pravisankar @rajatchopra @smarterclayton

dcbw commented 8 years ago

@danwinship PTAL, thanks!

smarterclayton commented 8 years ago

Do you guys have a design doc / writeup / proposal to cover this work?

dcbw commented 8 years ago

Do you guys have a design doc / writeup / proposal to cover this work?

Yes, I'll mail you a link.

dcbw commented 8 years ago

@smarterclayton after prototyping using the kube/openshift clients in the CNI plugin and discussing with @danwinship, I've decided to drop the podinfo server for now. We may need it in the future if things become more complicated or contacting the apiserver becomes a performance issue.

I agree with your points (2) and (4) in https://github.com/openshift/openshift-sdn/pull/320#discussion_r68142557; the other points are moot with the latest commits. We'll work on making those changes.

In the mean time, PTAL and let me know what you think. Thanks!

dcbw commented 8 years ago

@smarterclayton any thoughts on this approach? One issue with not having communication between the CNI plugin and the openshift node process is that it somewhat complicates hostport/containerport redirect functionality (eg, like docker's "-p host:container" functionality) since the long-lived node process doesn't know when pods start/stop since the CNI plugin is now using its own k/os clients.

dcbw commented 8 years ago

@liggitt any thoughts on the podinfo server vs. hitting up the apiserver directly here?

dcbw commented 8 years ago

@eparis any thoughts here either?

dcbw commented 8 years ago

Relevant discussion about podinfoserver vs. apiserver is buried, so here they are:

https://github.com/openshift/openshift-sdn/pull/320#discussion_r67255635 https://github.com/openshift/openshift-sdn/pull/320#discussion_r68142557 https://github.com/openshift/openshift-sdn/pull/320#issuecomment-228480623

smarterclayton commented 8 years ago

So I like the general approach of having CNI do more. We may need to consider the perf impact, but it makes it truly modular rather than baked in. Since you have the node credentials it's no worse. If the hostPort stuff was rolled in (and it doesn't seem like there's a reason it can't be?) then it would remove the need for the node to do anything except maybe in the future be a caching proxy for the SDN tool.

dcbw commented 8 years ago

Why isn't hostPort managed by cni setup and tear down?

@smarterclayton There currently isn't any way to get two things from the CNI plugin: (a) whether it handles HostPort itself, and (b) what interface name to do the iptables SNAT/DNAT on.

We're working on some of this upstream in CNI (https://github.com/containernetworking/cni/pull/145 and https://github.com/containernetworking/cni/pull/255) and will use this functionality in the Kubernetes CNI plugin then too.

dcbw commented 8 years ago

So I like the general approach of having CNI do more. We may need to consider the perf impact, but it makes it truly modular rather than baked in. Since you have the node credentials it's no worse. If the hostPort stuff was rolled in (and it doesn't seem like there's a reason it can't be?) then it would remove the need for the node to do anything except maybe in the future be a caching proxy for the SDN tool.

@smarterclayton The HostPort is the only thing that's really icky about this right now. I'm working to get that rolled into the kubernetes CNI network plugin but it's taking more upstream work to update the CNI spec than I thought. But I expect that to be solved in the next month or two.

dcbw commented 8 years ago

openshift-sdn is now merged into origin, so see https://github.com/openshift/origin/issues/9981 for the continuation of this PR.