Skip to content
This repository has been archived by the owner on Jul 25, 2019. It is now read-only.

plugin reorg: merge subdirs, split master/node #305

Merged
merged 8 commits into from Jun 23, 2016

Conversation

danwinship
Copy link
Contributor

The plugins/osdn vs plugins/osdn/ovs split was based on the idea that the code in plugins/osdn would eventually also be used by other plugins. Given that this is no longer expected to happen, there's no point in keeping the split.

OTOH, we do want to be able to split the master and node code apart more easily, and that's easier to do once everything is merged together, and that's what the second part of this branch does.

@openshift/networking PTAL. You'll probably want to review commit-by-commit.

@danwinship danwinship force-pushed the merge-and-split branch 2 times, most recently from 8b23a52 to 27948ad Compare May 4, 2016 21:30
case SingleTenantPluginName:
return true
case MultiTenantPluginName:
return true
Copy link

Choose a reason for hiding this comment

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

L68-L72 can be simplified:

case SingleTenantPluginName, MultiTenantPluginName:
     return true

@pravisankar
Copy link

We changed openshift-sdn-docker-setup.sh and openshift-sdn-ovs paths, need to update origin.spec and contrib/node/install-sdn.sh on the origin repo.

const (
SingleTenantPluginName string = "redhat/openshift-ovs-subnet"
MultiTenantPluginName string = "redhat/openshift-ovs-multitenant"
)

Choose a reason for hiding this comment

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

Move these constants under pkg/ so that CLI and plugins/osdn can reuse it.

@danwinship
Copy link
Contributor Author

Updated. I moved the constants into plugins/osdn/api/ rather than pkg/, since that seemed to be consistent with the rest of what we're doing.

Corresponding origin branch is "merge-and-split" in danwinship/origin

@dcbw
Copy link
Member

dcbw commented May 13, 2016

LGTM

@@ -9,6 +9,8 @@ import (
kcmdutil "k8s.io/kubernetes/pkg/kubectl/cmd/util"
kerrors "k8s.io/kubernetes/pkg/util/errors"

osdnapi "github.com/openshift/openshift-sdn/plugins/osdn/api"

Choose a reason for hiding this comment

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

I think we don't want pkg/cmd to depend on plugins/, please check #233

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK then, for now I'm going to just revert the pkg/cmd/ part of the change; we already have origin/pkg/sdn/api and openshift-sdn/plugins/osdn/api, and the latter already depends on random stuff from kubernetes that we presumably don't want "oc" depending on, so it can't be moved wholesale to openshift-sdn/pkg/. So that means we would need some third "api" directory in openshift-sdn/pkg/ with some slightly different name, which will just be confusing.

We can revisit this in a later cleanup.

@danwinship
Copy link
Contributor Author

Repushed with:

  • pkg/cmd/ no longer imports plugin names from plugins/osdn/api
  • common.go:GetNodeIP() is now subnets.go:getNodeIP()
  • a bunch of other functions are also renamed to initial-lowercase since they don't need to be exported now that there's only a single namespace

I guess this should probably wait until after #303 though

@pravisankar
Copy link

pravisankar commented May 18, 2016

Merge plugins/osdn/ovs into plugins/odsn

typo *osdn

@pravisankar
Copy link

Good cleanup/reorg, LGTM

@danwinship
Copy link
Contributor Author

@openshift/networking updated; I ran the extended networking tests locally, but you might want to wait for the origin PR to finish the other tests before merging this

@danwinship
Copy link
Contributor Author

btw @pravisankar my version of this rebased on top of #303 is danwinship:merge-and-split-303, though that's based on an earlier version of this branch, and also I never tried compiling it, so it probably has bugs

Also, move getNodeIP() from common.go to subnets.go since it's only
used from there.
@danwinship
Copy link
Contributor Author

dropped "Move plugin name constants into plugins/osdn/api/" since those constants are no longer used outside of openshift-sdn after Ravi's addition of IsOpenShiftNetworkPlugin() and IsOpenShiftMultiTenantNetworkPlugin().

@dcbw
Copy link
Member

dcbw commented Jun 23, 2016

LGTM

pluginName string
multitenant bool

Choose a reason for hiding this comment

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

We don't need multitenant anymore?
Now we got the pluginName, can use IsOpenShiftMultitenantNetworkPlugin(oc.pluginName) where ever required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Later in the branch, both pluginName and multitenant go away in OsdnMaster, and pluginName goes away in OsdnNode. (Keeping oc.multitenant rather than keeping oc.pluginName and using IsOpenShiftMultiTenantNetworkPlugin() involved less code change.)

@pravisankar
Copy link

LGTM

@pravisankar pravisankar merged commit 72ffb02 into openshift:master Jun 23, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants