Split networking out from node initialization into its own package #16269

smarterclayton · 2017-09-10T01:14:41Z

Node and networking are now completely independent initialization paths.

Builds off of #16268, only second commit is new

@deads2k second step towards removing the need for node to be tied together

deads2k · 2017-09-11T12:14:27Z

pkg/cmd/server/kubernetes/network/network.go

+}
+
+// RunDNS starts the DNS server as soon as services are loaded.
+func (c *NetworkConfig) RunDNS() {


Don't we start DNS inside the master today?

DNS runs on nodes and masters. Nodes are the primary, masters are only for backcompat.

deads2k · 2017-09-11T12:17:45Z

pkg/cmd/server/kubernetes/network/network.go

+		c.ProxyConfig.ConfigSyncPeriod.Duration,
+	)
+
+	if c.EnableUnidling {


There are definitely follow-up questions about the unidler, but this probably isn't making it worse.

Side question, where does the idler fall in the update order with controllers. Will we have to have a version where we have a controller running on its own resource and events?

deads2k · 2017-09-11T12:18:50Z

pkg/cmd/server/kubernetes/network/network_config.go

+	InternalKubeInformers kinternalinformers.SharedInformerFactory
+
+	// ProxyConfig is the configuration for the kube-proxy, fully initialized
+	ProxyConfig *componentconfig.KubeProxyConfiguration


// TODO, this should wire through on options if at all possible.

deads2k · 2017-09-11T12:20:40Z

pkg/cmd/server/kubernetes/node/node.go

@@ -549,7 +327,7 @@ func firstEndpointIPWithNamedPort(endpoints *kapi.Endpoints, port int, portName
 }

 // TODO: more generic location
-func includesNamedEndpointPort(ports []kapi.EndpointPort, port int, portName string) bool {
+func includesNamedEndpointPort(ports []kapiv1.EndpointPort, port int, portName string) bool {


Not wrong, but why bother switching?

Allows us to not load an internal client for kubelet (completely). So the kubelet code path has no internal client logic at all. Given the desire to split, reduces by one the list of dependencies in NodeConfig (which is net good)

deads2k · 2017-09-11T12:21:02Z

pkg/cmd/server/kubernetes/node/node_config.go

 	// DockerClient is a client to connect to Docker
 	DockerClient dockertools.Interface
 	// KubeletServer contains the KubeletServer configuration
 	KubeletServer *kubeletoptions.KubeletServer
 	// KubeletDeps are the injected code dependencies for the kubelet, fully initialized
 	KubeletDeps *kubelet.KubeletDeps
-	// ProxyConfig is the configuration for the kube-proxy, fully initialized
-	ProxyConfig *componentconfig.KubeProxyConfiguration


happy to see this go.

deads2k · 2017-09-11T12:22:00Z

pkg/cmd/server/kubernetes/node/node_config.go

-		return nil, err
-	}
-
+func New(options configapi.NodeConfig, server *kubeletoptions.KubeletServer) (*NodeConfig, error) {


Eventually, this should wire through on options like the scheduler and the upstream controllers, right?

Yeah, I suspect this method should be split into two parts as we get there - node config to kubelet and proxy Options, then further refine node config with other defaults. I think there are a few things that can't be teased apart yet without duplicating a lot of subtle backwards compatibility on options.

deads2k · 2017-09-11T12:22:49Z

pkg/cmd/server/kubernetes/node/options/options.go

+
+// Build creates the core Kubernetes component configs for a given NodeConfig, or returns
+// an error
+func Build(options configapi.NodeConfig) (*kubeletoptions.KubeletServer, *componentconfig.KubeProxyConfiguration, error) {


I'd prefer to build these separately. Any reason that they have to be built together?

Yeah, there is data calculated for kubelet server that is an input to proxy config. I'd prefer not to separate that in this step (we can do that once kubelet config moves to beta).

deads2k · 2017-09-11T12:23:26Z

pkg/cmd/server/kubernetes/node/options/options.go

+		return nil, nil, kerrors.NewAggregate(err)
+	}
+
+	proxyconfig, err := buildKubeProxyConfig(options)


doesn't look like it.

deads2k · 2017-09-11T12:24:48Z

minor questions/comments about direction, I'm glad to see this moving forward.

deads2k · 2017-09-11T17:44:26Z

lgtm. I'll leave tagging to you if there's an order or concern about dcut.

smarterclayton · 2017-09-11T20:31:47Z

Going to wait till next sprint. On Sep 11, 2017, at 2:12 PM, David Eads <[email protected]> wrote: lgtm. I'll leave tagging to you if there's an order or concern about dcut. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#16269 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_pytoKz5VCjKd2ukpAhglb62Lgcnpks5shXGCgaJpZM4PSK4N> .

smarterclayton · 2017-09-12T02:19:58Z

@openshift/sig-networking should also take a look.

danwinship · 2017-09-12T14:02:49Z

pkg/cmd/server/kubernetes/node/options/options.go

+		return nil, nil, err
+	}
+
+	// Initialize SDN before building kubelet config so it can modify option


that comment doesn't really make sense here any more

danwinship · 2017-09-12T14:03:52Z

pkg/cmd/server/kubernetes/node/options/options.go

+
+	if network.IsOpenShiftNetworkPlugin(server.NetworkPluginName) {
+		// set defaults for openshift-sdn
+		server.HairpinMode = componentconfig.HairpinNone


This can be dropped; it gets set in the IsOpenShiftNetworkPlugin() block below as well. (It looks like it was redundant in the old code too, we just hadn't noticed.)

danwinship · 2017-09-12T14:15:06Z

pkg/cmd/server/start/start_node.go

-		// TODO: SDN plugin depends on the Kubelet registering as a Node and doesn't retry cleanly,
-		// and Kubelet also can't start the PodSync loop until the SDN plugin has loaded.
-		if components.Enabled(ComponentKubelet) != components.Enabled(ComponentPlugins) {
-			return fmt.Errorf("the SDN plugin must be run in the same process as the kubelet")


Hm... so this check is going away, not just being moved somewhere else, right?

I think that's correct... the proxy filtering code doesn't depend on any of the node code these days. We should get QE to add a test for this case though. (The existing polarion test case OCP-12167 "The endpoints in EgressNetworkPolicy denying cidrSelector will be ignored", but with the node configured to run separate node and proxy processes.)

Yeah, I double checked locally and can't find any crossover.

deads2k · 2017-09-15T16:54:33Z

Going to wait till next sprint.

We've branched.

/assign danwinship

First refactoring towards separating out the code that launches networking from the code that launches the kubelet. Ensures that node options (the part that converts nodeconfig to kubelet config) is completely separate from how processes are started.

Node and networking are now completely independent initialization paths.

smarterclayton · 2017-09-15T17:52:13Z

Updated

danwinship · 2017-09-15T18:13:03Z

/lgtm
Filed https://trello.com/c/bTEOBhpx to get the new multitenant+separate node/proxy test written

openshift-merge-robot · 2017-09-15T18:13:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danwinship, smarterclayton

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~pkg/cmd/OWNERS~~ [smarterclayton]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

smarterclayton · 2017-09-15T20:17:52Z

/retest

termination message log not found

openshift-merge-robot · 2017-09-16T12:42:03Z

/test all [submit-queue is verifying that this PR is safe to merge]

openshift-ci-robot · 2017-09-16T13:01:18Z

@smarterclayton: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/openshift-jenkins/cmd	`9fc4265`	link	`/test cmd`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-merge-robot · 2017-09-16T14:38:36Z

Automatic merge from submit-queue (batch tested with PRs 16269, 13282, 16386)

Automatic merge from submit-queue (batch tested with PRs 16480, 16486, 16270, 16128, 16489) Direct exec() the kubelet instead of launching in proc If only the kubelet is launched by the node process, execve(2) instead of launching in process. Requires some corrections to the upstream flags to support round tripping. Support `OPENSHIFT_ALLOW_UNSUPPORTED_KUBELET=<path>` to allow a kubelet binary that is not exactly equivalent (symlink or hardlink) to the current file. If the kubelet binary cannot be found, print a warning and continue with the in-proc flow (so we don't break older users without the kubelet symlink). To start: ``` $ openshift start node --config=... --enable=kubelet --loglevel=3 <will log, then exec kubelet> ... kubelet logs ``` Networking can be run separately with: ``` $ openshift start network --config=... ``` Did a quick sanity test against this, didn't hit any obvious issues. Builds off #16269

openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 10, 2017

openshift-merge-robot assigned enj and csrwng Sep 10, 2017

openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 10, 2017

smarterclayton force-pushed the snip_network branch 2 times, most recently from a460007 to d97345b Compare September 10, 2017 01:53

smarterclayton mentioned this pull request Sep 10, 2017

Direct exec() the kubelet instead of launching in proc #16270

Merged

deads2k reviewed Sep 11, 2017

View reviewed changes

smarterclayton mentioned this pull request Sep 12, 2017

Separate config and options specific code for node from start #16268

Closed

danwinship reviewed Sep 12, 2017

View reviewed changes

openshift-ci-robot assigned danwinship Sep 15, 2017

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 15, 2017

smarterclayton added 2 commits September 15, 2017 13:52

Split networking out from node initialization into its own package

9fc4265

Node and networking are now completely independent initialization paths.

smarterclayton force-pushed the snip_network branch from d97345b to 9fc4265 Compare September 15, 2017 17:52

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 15, 2017

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 15, 2017

openshift-merge-robot merged commit 0a33b4c into openshift:master Sep 16, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split networking out from node initialization into its own package #16269

Split networking out from node initialization into its own package #16269

smarterclayton commented Sep 10, 2017 •

edited

Loading

deads2k Sep 11, 2017

smarterclayton Sep 11, 2017

deads2k Sep 11, 2017

deads2k Sep 11, 2017

deads2k Sep 11, 2017

smarterclayton Sep 11, 2017

deads2k Sep 11, 2017

deads2k Sep 11, 2017

smarterclayton Sep 11, 2017

deads2k Sep 11, 2017

smarterclayton Sep 11, 2017

deads2k Sep 11, 2017

deads2k commented Sep 11, 2017

deads2k commented Sep 11, 2017

smarterclayton commented Sep 11, 2017 via email

smarterclayton commented Sep 12, 2017

danwinship Sep 12, 2017

danwinship Sep 12, 2017

smarterclayton Sep 15, 2017

danwinship Sep 12, 2017

smarterclayton Sep 15, 2017

deads2k commented Sep 15, 2017

smarterclayton commented Sep 15, 2017

danwinship commented Sep 15, 2017

openshift-merge-robot commented Sep 15, 2017

smarterclayton commented Sep 15, 2017

openshift-merge-robot commented Sep 16, 2017

openshift-ci-robot commented Sep 16, 2017 •

edited

Loading

openshift-merge-robot commented Sep 16, 2017

Split networking out from node initialization into its own package #16269

Split networking out from node initialization into its own package #16269

Conversation

smarterclayton commented Sep 10, 2017 • edited Loading

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

Choose a reason for hiding this comment

deads2k commented Sep 11, 2017

deads2k commented Sep 11, 2017

smarterclayton commented Sep 11, 2017 via email

smarterclayton commented Sep 12, 2017

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

deads2k commented Sep 15, 2017

smarterclayton commented Sep 15, 2017

danwinship commented Sep 15, 2017

openshift-merge-robot commented Sep 15, 2017

smarterclayton commented Sep 15, 2017

openshift-merge-robot commented Sep 16, 2017

openshift-ci-robot commented Sep 16, 2017 • edited Loading

openshift-merge-robot commented Sep 16, 2017

smarterclayton commented Sep 10, 2017 •

edited

Loading

openshift-ci-robot commented Sep 16, 2017 •

edited

Loading