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

[Wip] Kubernetes talks to flannel-vxlan #13877

Closed
wants to merge 11 commits into from

Conversation

bprashanth
Copy link
Contributor

Requires flannel as a godep with flannel-io/flannel#308
Invoked via USE_OVERLAY="true" ./hack/local-up-cluster.sh or e2e.go -pushup

Only uploading so I can reference in networking discussions. Won't build because the godep isn't included, and including it will make the pr less referencable because of diff size. Godep commit hash 8b260bc.

@bprashanth bprashanth self-assigned this Sep 11, 2015
@k8s-bot
Copy link

k8s-bot commented Sep 11, 2015

GCE e2e build/test failed for commit b75d42ee3eeb438b690d427ba3fa9e224bf4df72.

@k8s-github-robot k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 13, 2015
@k8s-github-robot
Copy link

Labelling this PR as size/XL

@derekwaynecarr
Copy link
Member

Will this ensure that the proper firewall ports are open on the hosts when it configures flannel for you?

@bprashanth
Copy link
Contributor Author

Hmm.. firewall for the vxlan port? For external access or inside the cluster?

@derekwaynecarr
Copy link
Member

@bprashanth - by default the port used by vxlan and flannel was blocked on my host and blocked pod-to-pod communication, so if kube is configuring things, it may want to open the firewall port on the host by doing the proper iptables rule.

@derekwaynecarr
Copy link
Member

cc @kubernetes/rh-cluster-infra

@eparis
Copy link
Contributor

eparis commented Sep 15, 2015

@kubernetes/rh-networking

@@ -98,16 +98,13 @@ readonly KUBE_CLIENT_PLATFORMS=(
linux/amd64
linux/386
linux/arm
darwin/amd64
darwin/386
windows/amd64
Copy link
Contributor

Choose a reason for hiding this comment

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

can't build on non-linux any more?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, heh, that's just a hack. One of the packages flannel includes (netlink i think) had documented problems building on non-linux. Punted on it for a while.

Copy link
Member

Choose a reason for hiding this comment

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

ah, heh, that's just a hack. One of the packages flannel includes (netlink i think) had documented problems building on non-linux. Punted on it for a while.

netlink is Linux specific. And flannel uses netlink to create the vxlan interfaces at this point. So yeah, anything using flannel + the VXLAN backend would be Linux-only.

@dcbw
Copy link
Member

dcbw commented Sep 17, 2015

So is the point of this PR to implement the flannel server API in kubernetes so that the cluster doesn't have to run a master flanneld alongside the apiserver on the master?

@bprashanth
Copy link
Contributor Author

Roughly, yes. Flannel doesn't have access to etcd, which is the important part. Kube pushed subnets down to the flannel daemon, not the other way aroud. No new resources are created to manage subnets etc (normal flanneld will create new kvs under --etcd-prefix subdir).

This pr is just a proof of concept at the moment.

@bprashanth
Copy link
Contributor Author

@wojtek-t rebased, latest patch set includes patched flannel godep

@k8s-bot
Copy link

k8s-bot commented Oct 8, 2015

GCE e2e build/test failed for commit 667b873.

@k8s-github-robot k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 8, 2015
@k8s-github-robot
Copy link

Labelling this PR as size/XS

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 10, 2015
@k8s-bot
Copy link

k8s-bot commented Oct 28, 2015

GCE e2e build/test failed for commit 2429be4741018ca90612138b6299670a0620c574.

@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 28, 2015
@dcbw
Copy link
Member

dcbw commented Oct 28, 2015

IMO adding flannel bits into Kubernetes would be the wrong solution to the issue. Instead, the solution would be more along the lines of:

  1. Finish Kubernetes network plugin hooks to allow network providers (see Proposal: multi-tenant networking #15465) which would add global hooks (instead of per-pod hooks like exist now with the CNI and 'exec' plugins)
  2. Use those new hooks to implement your own plugin outside of Kubernetes that vendors flannel in and exposes the flannel API in the plugin, not in kubernetes
  3. Profit!

We're doing almost exactly the same thing for openshift-sdn, except we'll be running a flannel master that talks to the kubernetes etcd instance, rather than stuffing all of flannel into the Kubernetes process itself.

@bprashanth
Copy link
Contributor Author

Only reason it is the way it is, is because nothing else is ready yet. Even getting this for 1.2 and churning a little after that won't be too bad, it's isolated enough to split out eitherway. Exposing etcd to flannel probably won't fly, though.

@k8s-bot
Copy link

k8s-bot commented Oct 28, 2015

GCE e2e build/test failed for commit 4bf6f07b20b42fd1a95353ea99cd2e7edf15f90f.

@k8s-bot
Copy link

k8s-bot commented Nov 2, 2015

GCE e2e build/test failed for commit 8b92564.

@dcbw
Copy link
Member

dcbw commented Nov 3, 2015

Exposing etcd to flannel probably won't fly, though.

@bprashanth Why is that, especially if using flannel's listen/remote mode so that only the master flannel instance can talk to etcd?

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 4, 2015
@bprashanth
Copy link
Contributor Author

@dcbw directly accessing etcd is verboten for a few reasons (api server checks schema of a database, the db is pluggable and can be mysql, we shard/replicate the apiserver not the db etc). It certainly is possible, and listen/remote makes it less egregious, but it will take some convincing before everyone just agress to writing to etcd.

i.e we have a higher chance of getting stuff done by going through the apiserver, and an even higher chance if we just let the master assign the subnets like it does today (though I'll concede there are advantages to letting flannel do the latter, and that's an easier sell than the former).

@dcbw
Copy link
Member

dcbw commented Nov 11, 2015

i.e we have a higher chance of getting stuff done by going through the apiserver, and an even higher chance if we just let the master assign the subnets like it does today (though I'll concede there are advantages to letting flannel do the latter, and that's an easier sell than the former).

@bprashanth I see some of the arguments here; though personally (and I'm not authoritative on the matter) I really think anything like this, especially adding a whole new API surface, should be done in a network provider once the network provider proposals land. That way the API is contained in your external network provider, and not added to Kubernetes itself.

@bprashanth
Copy link
Contributor Author

We already hacked in flannel integration in for 1.2, hopefully it will get cleaned up through CNI plugins for 1.3

@bprashanth bprashanth closed this Mar 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants