More SDN code reorg #11137

danwinship · 2016-09-28T14:56:43Z

Split EgressNetworkPolicy monitoring into its own file
Split "VNID tracking" from "multitenant policy" (since the networkpolicy plugin will want the former but not the latter), so now vnids_node.go is just NetNamespace monitoring
Simplify SDN setup, which still had vestiges of the old "FlowController" split in it, so now subnets.go is just HostSubnet monitoring/maintaining.

@openshift/networking PTAL

pravisankar · 2016-09-28T18:51:25Z

pkg/sdn/plugin/controller.go

+	var err error
+	var subnet *osapi.HostSubnet
+	// Try every retryInterval and bail-out if it exceeds max retries
+	for i := 0; i < retries; i++ {


We could use existing wait.ExponentialBackoff() here.

Yeah, clayton pushed back on that for the CNI stuff too. See https://github.com/openshift/origin/pull/9981/files#diff-6357e2d44bec4f49542401c788e87f51R426 for an example.

pravisankar · 2016-09-28T19:12:37Z

pkg/sdn/plugin/node.go

+		return err
+	}
+
+	err = node.SubnetStartNode()
 	if err != nil {
 		return err
 	}


https://github.com/danwinship/origin/blob/9df71e32e6c65fb5eed4177bfa36d14b8b84890e/pkg/sdn/plugin/node.go#L125
What happens to the pods when UpadePod() fails in case of network change? Networking for these pods will be broken. May be we can retry couple of times and log error if it didn't succeed?

I don't think retrying is likely to help; if we can't update the pod networking, then something is just broken. (Eg, OVS isn't running.) But I think if things are broken enough that UpdatePod() fails, then startup is going to fail for other reasons anyway

dcbw · 2016-09-28T19:16:40Z

pkg/sdn/plugin/controller.go

+	var err error
+	var subnet *osapi.HostSubnet
+	// Try every retryInterval and bail-out if it exceeds max retries
+	for i := 0; i < retries; i++ {


Yeah, clayton pushed back on that for the CNI stuff too. See https://github.com/openshift/origin/pull/9981/files#diff-6357e2d44bec4f49542401c788e87f51R426 for an example.

dcbw · 2016-09-28T19:23:09Z

pkg/sdn/plugin/vnids_node.go

-		ids:        make(map[string]uint32),
-		namespaces: make(map[uint32]sets.String),
-	}
+	return &nodeVNIDMap{}


This just saving memory or someting?

No, it's part of a set of changes to get rid of plugin.go:getVNID(); the fields get initialized by VnidStartNode() now, which only gets called for multitenant, so if GetVNID() sees that they haven't been initialized later, it can just return 0 for the VNID, rather than plugin.go having to make that assumption itself.

No, This enables nil check (https://github.com/openshift/origin/pull/11137/files#diff-eabef4ae7ed24f933a3d3ac531af3814R65)
and GetVNID (https://github.com/openshift/origin/pull/11137/files#diff-5f233cf7267651424a495cc6b900f8d6R230) will return correct value both for subnet and multitenant plugins.

dcbw · 2016-09-28T19:26:01Z

pkg/sdn/plugin/multitenant.go

+
+func (node *OsdnNode) watchServices() {
+	services := make(map[string]*kapi.Service)
+	RunEventQueue(node.kClient, Services, func(delta cache.Delta) error {


Should probably update this to use eventqueue.NewEventQueueForStore() and make 'services' a cache.Store, but that could be done after.

pravisankar · 2016-09-28T19:39:04Z

pkg/sdn/plugin/multitenant.go

+	return nil
+}
+
+func (node *OsdnNode) updatePodNetwork(namespace string, oldNetID, netID uint32) error {


updatePodNetwork() is only used by watchNetNamespaces() in vnids_node.go
I was expecting MultitenantStartNode() to watch for both NetNamespaces and Services to patch vnid when needed. I didn't understand the split between vnids_node.go and multitenant.go

Hm... did I explain that in the commit message? Maybe not... The idea is that there's going to be an openshift-ovs-networkpolicy network plugin soon as well, and it will use the VNID-tracking code, but it won't react to it in the same way. So vnids_node.go = NetNamespace watching, and multitenant.go = openshift-ovs-multitenant-specific policy, and there will later be networkpolicy.go as well.

The fact that updatePodNetwork() gets called directly from vnids_node.go is wrong, yeah, and that's going to change later. (Initially I was planning to just move code around in these commits, and not refactor anything, which is why this is like that. Although then I did end up making some code changes too, so maybe I should have fixed this...)

danwinship · 2016-09-29T15:42:51Z

OK, repushed without the vnids_node/multitenant split (I'll rework that a bit and do it with the rest of the networkpolicy branch), but with a new patch to use utilwait.ExponentialBackoff() where we should.

dcbw · 2016-09-30T22:13:01Z

LGTM

dcbw · 2016-10-04T13:23:23Z

@pravisankar PTAL?

dcbw · 2016-10-04T18:04:44Z

[merge]

openshift-bot · 2016-10-04T18:07:17Z

[Test]ing while waiting on the merge queue

pravisankar · 2016-10-04T18:21:57Z

LGTM

dcbw · 2016-10-06T02:49:16Z

flake is #11240 [merge]

The SDN initialization was being called from subnets.go for historical reasons; have node.go call it directly instead. Also, don't bother passing data to SetupSDN() that it could get from the OsdnNode structure itself (another historical artifact).

danwinship · 2016-10-07T20:14:37Z

flake is #11240, [test]

openshift-bot · 2016-10-07T20:17:31Z

Evaluated for origin test up to 54b856a

openshift-bot · 2016-10-07T22:30:55Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/9761/)

openshift-bot · 2016-10-09T10:42:12Z

Evaluated for origin merge up to 54b856a

openshift-bot · 2016-10-09T10:42:20Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/9787/) (Image: devenv-rhel7_5157)

dcbw added the component/networking label Sep 28, 2016

pravisankar reviewed Sep 28, 2016

View reviewed changes

dcbw suggested changes Sep 28, 2016

View reviewed changes

pravisankar reviewed Sep 28, 2016

View reviewed changes

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 29, 2016

danwinship force-pushed the more-sdn-cleanup branch from e6593e9 to 3a2ec09 Compare September 29, 2016 15:37

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 29, 2016

danwinship force-pushed the more-sdn-cleanup branch from 3a2ec09 to 239a236 Compare September 30, 2016 15:04

dcbw approved these changes Sep 30, 2016

View reviewed changes

danwinship force-pushed the more-sdn-cleanup branch 2 times, most recently from 2ea77a3 to 6cd347a Compare October 5, 2016 21:18

danwinship added 3 commits October 7, 2016 13:50

Split out EgressNetworkPolicy-monitoring code into its own file

e2b2895

Use utilwait.ExponentialBackoff instead of looping

54b856a

danwinship force-pushed the more-sdn-cleanup branch from 6cd347a to 54b856a Compare October 7, 2016 17:50

openshift-bot merged commit 1bcada1 into openshift:master Oct 9, 2016

danwinship deleted the more-sdn-cleanup branch October 10, 2016 13:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More SDN code reorg #11137

More SDN code reorg #11137

danwinship commented Sep 28, 2016

pravisankar Sep 28, 2016

dcbw Sep 28, 2016

pravisankar Sep 28, 2016

danwinship Sep 29, 2016

dcbw Sep 28, 2016

dcbw Sep 28, 2016

danwinship Sep 28, 2016

pravisankar Sep 28, 2016

dcbw Sep 28, 2016

pravisankar Sep 28, 2016

danwinship Sep 28, 2016

danwinship commented Sep 29, 2016

dcbw commented Sep 30, 2016

dcbw commented Oct 4, 2016

dcbw commented Oct 4, 2016

openshift-bot commented Oct 4, 2016

pravisankar commented Oct 4, 2016

dcbw commented Oct 6, 2016

danwinship commented Oct 7, 2016

openshift-bot commented Oct 7, 2016

openshift-bot commented Oct 7, 2016

openshift-bot commented Oct 9, 2016

openshift-bot commented Oct 9, 2016 •

edited

More SDN code reorg #11137

More SDN code reorg #11137

Conversation

danwinship commented Sep 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danwinship commented Sep 29, 2016

dcbw commented Sep 30, 2016

dcbw commented Oct 4, 2016

dcbw commented Oct 4, 2016

openshift-bot commented Oct 4, 2016

pravisankar commented Oct 4, 2016

dcbw commented Oct 6, 2016

danwinship commented Oct 7, 2016

openshift-bot commented Oct 7, 2016

openshift-bot commented Oct 7, 2016

openshift-bot commented Oct 9, 2016

openshift-bot commented Oct 9, 2016 • edited

openshift-bot commented Oct 9, 2016 •

edited