openshift / openshift-sdn

Apache License 2.0
69 stars 63 forks source link

Reorganize and clean SDN code #159

Closed pravisankar closed 9 years ago

pravisankar commented 9 years ago

closes: #130

pravisankar commented 9 years ago

@danwinship @dcbw @rajatchopra PTAL

danwinship commented 9 years ago

You broke travis-ci. :-)

The divisions between the commits are inconsistent. Eg, in "Remove standalone openshift-sdn binary", you remove both the source files and the build system references (Makefile, hack/*, etc), but in "cleanup lbr" (which really ought to be "remove lbr"), you just remove the sources, and leave dangling references elsewhere that get cleaned up by "Fix Makefile", etc...

You should rename the "ovssubnet" directory while you're moving it. "sdn"? Also, rename the "kube" subdirectory...

The commit that brings in the origin code should probably mention that that's where it came from?

I don't think there's any point to keeping Makefile at all; you're not going to want to test changes to the scripts without also getting the new .go code, so install-dev isn't really useful.

danwinship commented 9 years ago

At a meta level...

I don't see the point of keeping this separate from origin at this point. You can't build anything in the openshift-sdn module--not even test programs, since it all depends on kubernetes and origin code that isn't godep'ed in. (So if we want test programs for the openshift-sdn code, we'd have to put them in origin somewhere...) We're all going to end up doing all of our development in origin/Godeps/.../openshift-sdn/, and then once it works, we'll copy it over to the "real" openshift-sdn checkout, submit a PR, possibly go back and make and test changes in origin again for the review comments and then copy them back over to openshift-sdn, eventually commit to openshift-sdn, and then file another PR against origin to update the Godep dependency. As opposed to if we moved all the code the other way, we'd just develop in origin and commit to origin and be done with it.

I had thought that part of the point of keeping openshift-sdn separate from origin was to ensure that it was possible to develop out-of-tree network plugins (contrail, nuage, etc), but this doesn't really seem to do that. (OTOH, go doesn't have dlopen so I guess you can't have out-of-tree network plugins anyway?)

Where do we expect the flannel integration to end up? Is the idea that the flannel code will be in origin (and not reuse any of the old openshift-sdn code), and the old openshift-sdn stuff will be kept out until eventually everyone has migrated away and we can forget it ever existed?

dcbw commented 9 years ago

Where do we expect the flannel integration to end up? Is the idea that the flannel code will be in origin (and not reuse any of the old openshift-sdn code), and the old openshift-sdn stuff will be kept out until eventually everyone has migrated away and we can forget it ever existed?

@danwinship the flannel backend itself will likely end up in flannel git. But that leaves two other parts that are not flannel related: (a) the CNI plugin that sets up each pod, and (b) the bits that handle creating/removing the OpenShift projects/namespaces in etcd (that flannel reads). Those bits are clearly kubernetes/openshift specific, and those still have to live somewhere. However, they'll be a lot lighter than openshift-sdn, so maybe it's OK to have those directly in origin instead of the openshift-sdn repo.

danwinship commented 9 years ago

(b) the bits that handle creating/removing the OpenShift projects/namespaces in etcd (that flannel reads). Those bits are clearly kubernetes/openshift specific, and those still have to live somewhere. However, they'll be a lot lighter than openshift-sdn, so maybe it's OK to have those directly in origin instead of the openshift-sdn repo.

So that implies that the code in osdn.go that is being moved to openshift-sdn in this patch would eventually get moved back into origin...

danwinship commented 9 years ago

So anyway, despite my arguments above about this making no sense, I don't object to it going in (other than the comments above. At a minimum you have to remove .travis.yml.). Because having it all merged together anywhere is better than having it be split between the two modules. (And if we decide later it should go in origin, it's easy to move it there.)

And other things are blocking on this, so we should do it soon.

pravisankar commented 9 years ago

@danwinship Removed Makefile, sync-to-origin.sh will display which openshift-sdn revision is synced to origin repo, fixed hack/test.sh and travis.yml. If we decide to move all sdn changes to origin repo then we can create a separate PR. Merge this PR if you are okay with the changes, at least that will unblock other pull requests waiting on this.

rajatchopra commented 9 years ago

@dcbw For the flannel based plugin, we should probably not use anything from openshift-sdn then. We can include both the CNI+master_write_to_etcd logic in origin itself. The final aim being that as the flannel based plugin becomes ready to ship, we can kill openshift-sdn repo entirely. This would mean that we build a separate executable out of origin. In agreement with what you imagine?

dcbw commented 9 years ago

For the flannel based plugin, we should probably not use anything from openshift-sdn then. We can include both the CNI+master_write_to_etcd logic in origin itself. The final aim being that as the flannel based plugin becomes ready to ship, we can kill openshift-sdn repo entirely. This would mean that we build a separate executable out of origin. In agreement with what you imagine?

@rajatchopra Yeah, I agree. It's trivial to make flannel listen to the Origin etcd service by something like:

/usr/bin/flanneld -etcd-cafile="/vagrant/openshift.local.config/master/ca.crt" -etcd-certfile="/vagrant/openshift.local.config/master/master.etcd-client.crt" -etcd-keyfile="/vagrant/openshift.local.config/master/master.etcd-client.key" -etcd-endpoints="https://10.245.2.2:4001"

So that part is easy. But we still need some additional API in origin itself (and move it eventually to Kubernetes) to watch various resources like namespaces, services, etc so we can poke etcd when things change, and for now that should live in Origin. But eventually we need to move that to a Kubernetes plugin when they finally figure out how to expose an API for plugins to do things more than just pod setup/teardown.