Use kubernetes shared informers in OpenShift network plugins #14030

pravisankar · 2017-05-03T18:00:19Z

Changed network policy plugin to do pod watch for all namespaces (one go routine for the cluster) instead of pod watch for each namespace (go routine for each namespace).

pravisankar · 2017-05-03T18:00:56Z

@openshift/networking PTAL

ncdc · 2017-05-03T18:04:52Z

pkg/sdn/plugin/common.go

+	var expectedObjType interface{}
+
+	switch resourceName {
+	case Nodes:


You call this func with 4 different ResourceName values (Namespaces, Services, Pods, Nodes) but only support Nodes?

oops, missed updating this method for the other resource names. Updated

pravisankar · 2017-05-03T18:31:32Z

[testextended][extended:networking]

pravisankar · 2017-05-04T18:20:50Z

[test]

dcbw

Generally LGTM.

dcbw · 2017-05-06T03:57:04Z

pkg/sdn/plugin/networkpolicy.go

+	// threads modify this map.
+	oldPod, podExisted := np.pods[pod.UID]
+	if (podExisted && oldPod.Status.PodIP == pod.Status.PodIP && reflect.DeepEqual(oldPod.Labels, pod.Labels)) ||
+		(pod.Status.PodIP == "") {


Could there ever be a pod Update event where PodIP = ""? Likely not probably, but maybe we should handle that as a pod deletion?

I realized deleting pod in this case is not a good idea. Pod network plugin setup is only called for pods with HostNetwork=false but watch pods do get pods that has HostNetwork=true and podIP is not set in this case. Since HostNetwork check was missing in watch pods, we could have deleted a valid pod. I will add HostNetwork check in watch pods and will log a warning if podIP is not set.

dcbw · 2017-05-06T04:08:14Z

pkg/sdn/plugin/subnets.go

+		log.Errorf("Error creating subnet for node %s, ip %s: %v", node.Name, nodeIP, err)
+		return
+	}
+	master.hostSubnetNodeIPs[node.UID] = usedNodeIP


Does this map need locking at all? I guess we're guaranteed that the shared informer will call add/del in the same goroutine, so we probably don't. Right?

Yes, add/update/del events are serially processed from the shared informer queue. So no additional locking required.

danwinship

also mostly lgtm

danwinship · 2017-05-09T12:48:59Z

pkg/sdn/plugin/networkpolicy.go

@@ -364,7 +278,7 @@ func (np *networkPolicyPlugin) selectPods(npns *npNamespace, lsel *metav1.LabelS
 		glog.Errorf("ValidateNetworkPolicy() failure! Invalid PodSelector: %v", err)
 		return ips
 	}
-	for _, pod := range npns.pods {
+	for _, pod := range np.pods {


Since you're looping over all pods now you need to check pod.Namespace explicitly

danwinship · 2017-05-09T12:53:38Z

pkg/sdn/plugin/subnets.go

-		}
+	nodeInformer := master.informers.InternalKubernetesInformers().Core().InternalVersion().Nodes()
+	nodeInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{
+		AddFunc: func(obj interface{}) {


maybe add a helper function that takes a shared.InformerFactory, a "handleAddOrUpdateFunc" and a "handleDeleteFunc", and creates a cache.ResourceEventHandlerFuncs (doing the DeletedFinalStateUnknown handling) and calls AddEventHandler on it?

pravisankar · 2017-05-11T01:40:09Z

@openshift/networking Updated, PTAL

This will help sdn watch resources to reuse the existing shared informers instead of creating a new watch routine for the resource.

- Create pod watch for all namespaces instead of pod watch for each namespace - Use shared informers for pod watch

…EventQueue() These resources can use shared informers if needed.

If we get a pod with out any IP set then log warning and bail out.

openshift-bot · 2017-05-11T18:53:49Z

Evaluated for origin testextended up to abc81c6

openshift-bot · 2017-05-11T20:21:46Z

continuous-integration/openshift-jenkins/testextended SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_extended/375/) (Base Commit: 2628c77) (Extended Tests: networking)

dcbw · 2017-05-15T04:24:19Z

@openshift-bot test this issue #13977

dcbw · 2017-05-15T04:28:26Z

Latest changes LGTM too.

dcbw · 2017-05-15T04:28:52Z

[test] issue #13977

openshift-bot · 2017-05-15T04:29:21Z

Evaluated for origin test up to abc81c6

openshift-bot · 2017-05-15T06:28:12Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/1432/) (Base Commit: b38274c)

knobunc

LGTM

knobunc · 2017-05-16T12:23:33Z

[merge]

danwinship · 2017-05-17T15:06:28Z

flake #14229 [merge]

danwinship · 2017-05-17T19:55:31Z

flake #14197, [merge]

smarterclayton · 2017-05-18T04:52:56Z

[merge] flake on #14236

…

On Wed, May 17, 2017 at 6:53 PM, OpenShift Bot ***@***.***> wrote: continuous-integration/openshift-jenkins/merge FAILURE ( https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/682/) (Base Commit: 1c9427c <1c9427c> ) — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#14030 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p3oyZwS4BcgWtOJxNHl2G3OJnBM8ks5r63pygaJpZM4NPv3Y> .

openshift-bot · 2017-05-18T04:53:31Z

Evaluated for origin merge up to abc81c6

openshift-bot · 2017-05-18T05:45:39Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/687/) (Base Commit: 2ea9f5a) (Image: devenv-rhel7_6239)

ncdc reviewed May 3, 2017

View reviewed changes

pravisankar force-pushed the sdn-use-shared-informers branch from 3630f4e to 60dfe15 Compare May 3, 2017 18:16

pravisankar force-pushed the sdn-use-shared-informers branch 3 times, most recently from 9e50cb2 to 4089a7a Compare May 4, 2017 19:51

pravisankar added the component/networking label May 5, 2017

dcbw reviewed May 6, 2017

View reviewed changes

danwinship suggested changes May 9, 2017

View reviewed changes

pravisankar force-pushed the sdn-use-shared-informers branch 3 times, most recently from b927bde to 6afa0ff Compare May 11, 2017 01:36

danwinship approved these changes May 11, 2017

View reviewed changes

Ravi Sankar Penta added 9 commits May 11, 2017 11:45

Pass shared informers to master SDN controller

15ff9d4

This will help sdn watch resources to reuse the existing shared informers instead of creating a new watch routine for the resource.

Change SDN watch nodes to reuse existing shared informer

b08dcaa

Change SDN watch namespaces on master to reuse existing shared informer

c9ffa67

Pass shared informers to node SDN controller

08400f9

This will help sdn watch resources to reuse the existing shared informers instead of creating a new watch routine for the resource.

Change network policy watch namespaces to use shared informers

b022a4f

Change SDN watch services on node to reuse existing shared informer

d6dacde

Network policy pod watch changes

ac7bb7e

- Create pod watch for all namespaces instead of pod watch for each namespace - Use shared informers for pod watch

Remove Nodes,Namespaces,Pods and Services resource names from SDN Run…

8194830

…EventQueue() These resources can use shared informers if needed.

Network policy pod watch should ignore pods with HostNetwork set to true

abc81c6

If we get a pod with out any IP set then log warning and bail out.

pravisankar force-pushed the sdn-use-shared-informers branch from 6afa0ff to abc81c6 Compare May 11, 2017 18:45

knobunc approved these changes May 15, 2017

View reviewed changes

knobunc requested a review from smarterclayton May 16, 2017 14:36

openshift-bot merged commit fb92178 into openshift:master May 18, 2017

knobunc mentioned this pull request May 23, 2017

Add then resync then Delete cause Pop() panic #14232

Merged

Use kubernetes shared informers in OpenShift network plugins #14030

Use kubernetes shared informers in OpenShift network plugins #14030

Uh oh!

Conversation

pravisankar commented May 3, 2017

Uh oh!

pravisankar commented May 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pravisankar commented May 3, 2017

Uh oh!

pravisankar commented May 4, 2017

Uh oh!

dcbw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danwinship left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pravisankar commented May 11, 2017

Uh oh!

openshift-bot commented May 11, 2017

Uh oh!

openshift-bot commented May 11, 2017

Uh oh!

dcbw commented May 15, 2017

Uh oh!

dcbw commented May 15, 2017

Uh oh!

dcbw commented May 15, 2017

Uh oh!

openshift-bot commented May 15, 2017

Uh oh!

openshift-bot commented May 15, 2017

Uh oh!

knobunc left a comment

Choose a reason for hiding this comment

Uh oh!

knobunc commented May 16, 2017

Uh oh!

danwinship commented May 17, 2017

Uh oh!

danwinship commented May 17, 2017

Uh oh!

smarterclayton commented May 18, 2017 via email

Uh oh!

openshift-bot commented May 18, 2017

Uh oh!

openshift-bot commented May 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

openshift-bot commented May 18, 2017 •

edited

Loading