SDN cleanup; kill "Registry" type #10924

danwinship · 2016-09-15T12:22:27Z

As discussed elsewhere recently, pkg/sdn/plugin/registry.go is a relic of the old standalone openshift-sdn (and not even in a way that would be useful for a future standalone openshift-sdn), so this gets rid of it (and manages to have less total code in the end).

@openshift/networking PTAL

dcbw · 2016-09-15T17:41:51Z

pkg/sdn/plugin/node.go

+	node.networkInfo, err = parseNetworkInfo(cn.Network, cn.ServiceNetwork)
+	if err != nil {
+		return err
+	}


This all could be a helper function that returns cn, netinfo, and err. It's also used in proxy.go.

dcbw · 2016-09-15T17:44:59Z

Generally LGTM

pravisankar · 2016-09-15T21:05:59Z

pkg/sdn/plugin/node.go

@@ -124,7 +133,24 @@ func (node *OsdnNode) Start() error {
 }

 func (node *OsdnNode) GetLocalPods(namespace string) ([]kapi.Pod, error) {
-	return node.registry.GetRunningPods(node.hostName, namespace)
+	fieldSelector := fields.Set{"spec.host": node.hostName}.AsSelector()


"spec.nodeName"

hm... actually, it's referring to a deprecated v1.PodSpec field which was replaced with "nodeName", not "hostname", which is apparently something slightly different.

that's the same field (deprecated version and current version), though the assumption that it is a hostname has always been tenuous and caused problems in some cloud providers. there is work in progress upstream to record the actual node hostname in the node status, and make the master honor that when connecting to the node

pravisankar · 2016-09-15T21:07:31Z

pkg/sdn/plugin/plugin.go

@@ -153,13 +155,32 @@ func getScriptError(output []byte) string {
 	return string(output)
 }

+func (plugin *OsdnNode) getPod(nodeName, namespace, podName string) (*kapi.Pod, error) {
+	fieldSelector := fields.Set{"spec.host": nodeName}.AsSelector()


"spec.nodeName"

pravisankar · 2016-09-15T21:12:22Z

pkg/sdn/plugin/proxy.go

+	proxy.networkInfo, err = parseNetworkInfo(cn.Network, cn.ServiceNetwork)
+	if err != nil {
+		return err
+	}


As @dcbw suggested, we could add a helper method to fetch and parse networkInfo.

pravisankar · 2016-09-15T21:21:41Z

Overall LGTM

danwinship · 2016-09-16T00:02:23Z

added back the removed getNetworkInfo() function and added a new commit fixing the deprecated field name.
[test]

smarterclayton · 2016-09-19T00:21:47Z

With the networking tests disabled, do you have a clean local run of the extended suite?

danwinship · 2016-09-19T18:25:16Z

With the networking tests disabled, do you have a clean local run of the extended suite?

test/extended/conformance.sh doesn't pass locally even in master

danwinship · 2016-09-19T18:30:36Z

[test][extended:all]

danwinship · 2016-09-19T21:42:48Z

https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/8995/ flakes #9490 and #10951
[test]

marun · 2016-09-20T04:05:59Z

It should be possible to get a clean local run with #10972. A clean ci run won't be possible until I figure out the cert issue.

danwinship · 2016-09-21T16:10:29Z

after (locally) rebasing on top of #11039, extended networking tests pass

dcbw · 2016-09-22T15:07:08Z

Changes for my comments LGTM and this is tested. [merge]

dcbw · 2016-09-22T17:24:14Z

flakes all around:
#9548
#9490
#10765
#11008

dcbw · 2016-09-22T17:24:26Z

[merge]

dcbw · 2016-09-22T22:11:14Z

flake city [merge]

dcbw · 2016-09-23T15:56:49Z

Deploymentconfig flake #11016 again

danwinship · 2016-09-23T15:59:51Z

you have to say [merge] again when you identify the flake #11016 or openshift-bot won't realize you're talking to it

dcbw · 2016-09-23T16:13:09Z

@danwinship yeah I know, but I was going to hold off until something is done about #11016 since it's killing a lot of our PRs right now. Until that gets fixed I don't think another merge will be useful.

danwinship · 2016-09-23T16:35:57Z

some merges have made it through... but anyway, it looks like we didn't need a re-tag anyway because it's still in the merge queue

openshift-bot · 2016-09-25T13:07:27Z

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

Rather than implicitly caching NetworkInfo in the Registry, explicitly cache it in the node/master/proxy objects.

Nothing needs to have the other info cached

This used to be an abstraction between kclient-based access and direct etcd access, back when openshift-sdn could be compiled standalone, but now it's just cruft.

openshift-bot · 2016-09-27T18:33:41Z

Evaluated for origin test up to eca63da

openshift-bot · 2016-09-27T18:33:51Z

Evaluated for origin merge up to eca63da

openshift-bot · 2016-09-27T20:39:46Z

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

danwinship force-pushed the kill-registry branch from 18adec7 to dcc80f8 Compare September 15, 2016 12:36

dcbw reviewed Sep 15, 2016

View reviewed changes

dcbw mentioned this pull request Sep 15, 2016

The main purpose of validateClusterNetwork is not validate #10889

Closed

pravisankar reviewed Sep 15, 2016

View reviewed changes

danwinship force-pushed the kill-registry branch from dcc80f8 to 86905ea Compare September 16, 2016 00:01

danwinship force-pushed the kill-registry branch from 86905ea to a6df5b6 Compare September 16, 2016 14:49

smarterclayton added the component/networking label Sep 19, 2016

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

danwinship force-pushed the kill-registry branch from a6df5b6 to 18a4d0b Compare September 19, 2016 16:01

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

danwinship force-pushed the kill-registry branch from 18a4d0b to 7d12cde Compare September 19, 2016 18:06

danwinship force-pushed the kill-registry branch from 7d12cde to 0b722ca Compare September 23, 2016 13:37

danwinship added 5 commits September 27, 2016 14:29

Move NetworkInfo from Registry to OsdnNode/OsdnMaster/ovsProxyPlugin

789fcf5

Rather than implicitly caching NetworkInfo in the Registry, explicitly cache it in the node/master/proxy objects.

Move remaining NetworkInfo-related code out of registry.go

a4d9f50

Make NetworkInfo just cache cluster/service CIDRs

da69710

Nothing needs to have the other info cached

Kill off SDN "Registry" type

b94e424

This used to be an abstraction between kclient-based access and direct etcd access, back when openshift-sdn could be compiled standalone, but now it's just cruft.

Fix use of deprecated PodSpec field name

eca63da

danwinship force-pushed the kill-registry branch from 0b722ca to eca63da Compare September 27, 2016 18:29

openshift-bot merged commit 4750429 into openshift:master Sep 27, 2016

danwinship deleted the kill-registry branch September 27, 2016 22:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SDN cleanup; kill "Registry" type #10924

SDN cleanup; kill "Registry" type #10924

danwinship commented Sep 15, 2016

dcbw Sep 15, 2016

dcbw commented Sep 15, 2016

pravisankar Sep 15, 2016 •

edited

Loading

danwinship Sep 15, 2016

liggitt Sep 16, 2016 •

edited

Loading

pravisankar Sep 15, 2016 •

edited

Loading

pravisankar Sep 15, 2016

pravisankar commented Sep 15, 2016

danwinship commented Sep 16, 2016

smarterclayton commented Sep 19, 2016

danwinship commented Sep 19, 2016

danwinship commented Sep 19, 2016 •

edited

Loading

danwinship commented Sep 19, 2016

marun commented Sep 20, 2016

danwinship commented Sep 21, 2016

dcbw commented Sep 22, 2016

dcbw commented Sep 22, 2016

dcbw commented Sep 22, 2016

dcbw commented Sep 22, 2016

dcbw commented Sep 23, 2016

danwinship commented Sep 23, 2016

dcbw commented Sep 23, 2016

danwinship commented Sep 23, 2016

openshift-bot commented Sep 25, 2016 •

edited

Loading

openshift-bot commented Sep 27, 2016

openshift-bot commented Sep 27, 2016

openshift-bot commented Sep 27, 2016

SDN cleanup; kill "Registry" type #10924

SDN cleanup; kill "Registry" type #10924

Conversation

danwinship commented Sep 15, 2016

dcbw Sep 15, 2016

Choose a reason for hiding this comment

dcbw commented Sep 15, 2016

pravisankar Sep 15, 2016 • edited Loading

Choose a reason for hiding this comment

danwinship Sep 15, 2016

Choose a reason for hiding this comment

liggitt Sep 16, 2016 • edited Loading

Choose a reason for hiding this comment

pravisankar Sep 15, 2016 • edited Loading

Choose a reason for hiding this comment

pravisankar Sep 15, 2016

Choose a reason for hiding this comment

pravisankar commented Sep 15, 2016

danwinship commented Sep 16, 2016

smarterclayton commented Sep 19, 2016

danwinship commented Sep 19, 2016

danwinship commented Sep 19, 2016 • edited Loading

danwinship commented Sep 19, 2016

marun commented Sep 20, 2016

danwinship commented Sep 21, 2016

dcbw commented Sep 22, 2016

dcbw commented Sep 22, 2016

dcbw commented Sep 22, 2016

dcbw commented Sep 22, 2016

dcbw commented Sep 23, 2016

danwinship commented Sep 23, 2016

dcbw commented Sep 23, 2016

danwinship commented Sep 23, 2016

openshift-bot commented Sep 25, 2016 • edited Loading

openshift-bot commented Sep 27, 2016

openshift-bot commented Sep 27, 2016

openshift-bot commented Sep 27, 2016

pravisankar Sep 15, 2016 •

edited

Loading

liggitt Sep 16, 2016 •

edited

Loading

pravisankar Sep 15, 2016 •

edited

Loading

danwinship commented Sep 19, 2016 •

edited

Loading

openshift-bot commented Sep 25, 2016 •

edited

Loading