Add EnableUnidling flag to openshift-sdn #214

squeed · 2019-06-26T13:31:23Z

This PR has 2 changes:

Improved handling of the Applied configuration for upgrades

Call ApplyDefaults on the previously applied configuration
A test case that makes sure we can adopt 4.1 clusters

The EnableUnidling flag, and its corresponding API change

If this all looks good, I'll split off an api PR.

danwinship · 2019-06-26T19:42:13Z

Call ApplyDefaults on the previously applied configuration

So how does that help?

Also, there's a typo in the commit message: "preivously"

danwinship · 2019-06-26T19:48:03Z

So how does that help?

Ah, duh, never mind, it helps by filling in the default value of EnableUnidling.

vendor/github.com/openshift/api/operator/v1/types_network.go

pkg/network/openshift_sdn.go

danwinship

Looks good. Go ahead and submit the API changes.

pkg/network/openshift_sdn.go

squeed · 2019-07-18T14:26:59Z

API changes merged, rebased, revendored, etc. etc. @danwinship, PTAL

danwinship

Typo in the call ApplyDefaults on the previously-applied configuration commit message: "preivously"

danwinship · 2019-07-18T14:54:12Z

README.md

@@ -114,6 +114,7 @@ OpenShiftSDN supports the following configuration options, all of which are opti
 * `vxlanPort`: The port to use for the VXLAN overlay. The default is 4789
 * `MTU`: The MTU to use for the VXLAN overlay. The default is the MTU of the node that the cluster-network-operator is first run on, minus 50 bytes for overhead. If the nodes in your cluster don't all have the same MTU then you will need to set this explicitly.
 * `useExternalOpenvswitch`: boolean. If the nodes are already running openvswitch, and OpenShiftSDN should not install its own, set this to true. This only needed for certain advanced installations with DPDK or OpenStack.
+* `enableUnidling`: boolean. Whether or not the service proxy supports idling and unidling of services.


"Whether the service proxy should allow idling and unidling of services"

danwinship · 2019-07-18T15:03:53Z

pkg/network/openshift_sdn.go

@@ -48,9 +48,13 @@ func renderOpenShiftSDN(conf *operv1.NetworkSpec, manifestDir string) ([]*uns.Un
 		"metrics-bind-address":    {"0.0.0.0"},
 		"metrics-port":            {"9101"},
 		"healthz-port":            {"10256"},
-		"proxy-mode":              {"iptables"},
+		"proxy-mode":              {"unidling+iptables"},


This is wrong: we have to apply the value of c.EnableUnidling after merging conf and kpcDefaults. If a 4.1 user redundantly specified proxy-mode: iptables, then in 4.1 they would have gotten unidling, but the way this code is currently written here, in 4.2 they would get no unidling. (There should be a test case for this.)

(The simplest way to do this might be to pass c.EnableUnidling to kubeProxyConfiguration().)

Argh, good point. The "unidling+" is an implementation detail.

This is to make it more likely that we don't accidentally reject a safe upgrade as unsafe. Also, add a test that includes known deployed previously-applied configurations, and ensures that they will always upgrade safely.

squeed · 2019-07-18T17:50:06Z

OK, fixed. Thanks a ton for the review. Solved the proxy-mode thing a slightly different way. PTAL.

danwinship · 2019-07-18T20:42:36Z

pkg/network/openshift_sdn.go

+	kpcOverrides := map[string]operv1.ProxyArgumentList{}
+	if *c.EnableUnidling && len(conf.KubeProxyConfig.ProxyArguments["proxy-mode"]) > 0 {
+		pma := conf.KubeProxyConfig.ProxyArguments["proxy-mode"]
+		if !strings.HasPrefix(pma[0], "unidling+") {


FWIW, validation has already confirmed that if *c.EnableUnidling is set (after defaulting), then conf.KubeProxyConfig.ProxyArguments["proxy-mode"] is either unset or ["iptables"]. So you can simplify this to just:

kpcOverrides := map[string]operv1.ProxyArgumentList{} if *c.EnableUnidling { kpcOverrides["proxy-mode"] = operv1.ProxyArgumentList{"unidling+iptables"}` }

Ah yeah, good point.

danwinship · 2019-07-18T20:43:19Z

pkg/network/openshift_sdn.go

+
+	}
+
+	kpc, err := kubeProxyConfiguration(conf, kpcDefaults, kpcOverrides)


maybe rearrange to defaults, config, overrides to match the order they're merged in? or not

yeah, thought about that. might as well be nice to future maintainers.

This also carries forward any possible 4.1 installations with --proxy-mode=iptables.

squeed · 2019-07-18T21:00:23Z

Updated, PTAL.

danwinship · 2019-07-19T16:37:02Z

/lgtm
/retest

openshift-ci-robot · 2019-07-19T16:37:14Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [danwinship,squeed]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

squeed · 2019-07-19T19:16:17Z

/retest

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 26, 2019

openshift-ci-robot requested review from JacobTanenbaum and pecameron June 26, 2019 13:31

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 26, 2019

squeed requested review from danwinship and removed request for JacobTanenbaum and pecameron June 26, 2019 13:31

danwinship reviewed Jun 26, 2019

View reviewed changes

vendor/github.com/openshift/api/operator/v1/types_network.go Outdated Show resolved Hide resolved

pkg/network/openshift_sdn.go Outdated Show resolved Hide resolved

pkg/network/openshift_sdn.go Show resolved Hide resolved

danwinship reviewed Jun 27, 2019

View reviewed changes

pkg/network/openshift_sdn.go Show resolved Hide resolved

squeed mentioned this pull request Jun 28, 2019

operator/network: add EnableUnidling flag. openshift/api#357

Merged

squeed changed the title ~~[wip] Add EnableUnidling flag to openshift-sdn~~ Add EnableUnidling flag to openshift-sdn Jul 18, 2019

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 18, 2019

danwinship requested changes Jul 18, 2019

View reviewed changes

squeed added 2 commits July 18, 2019 13:48

call ApplyDefaults on the previously-applied configuration.

c630f94

This is to make it more likely that we don't accidentally reject a safe upgrade as unsafe. Also, add a test that includes known deployed previously-applied configurations, and ensures that they will always upgrade safely.

dep ensure -update github.com/openshift/api

362836d

danwinship reviewed Jul 18, 2019

View reviewed changes

openshift-sdn: add ability to disable unidling

65bafa0

This also carries forward any possible 4.1 installations with --proxy-mode=iptables.

openshift-ci-robot assigned danwinship Jul 19, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 19, 2019

openshift-merge-robot merged commit e1453ac into openshift:master Jul 19, 2019

squeed deleted the sdn-unidling-flag branch August 19, 2019 13:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add EnableUnidling flag to openshift-sdn #214

Add EnableUnidling flag to openshift-sdn #214

squeed commented Jun 26, 2019 •

edited

danwinship commented Jun 26, 2019

danwinship commented Jun 26, 2019

danwinship left a comment

squeed commented Jul 18, 2019

danwinship left a comment

danwinship Jul 18, 2019

danwinship Jul 18, 2019

squeed Jul 18, 2019

squeed commented Jul 18, 2019

danwinship Jul 18, 2019

squeed Jul 18, 2019

danwinship Jul 18, 2019

squeed Jul 18, 2019

squeed commented Jul 18, 2019

danwinship commented Jul 19, 2019

openshift-ci-robot commented Jul 19, 2019

squeed commented Jul 19, 2019


		}

		kpc, err := kubeProxyConfiguration(conf, kpcDefaults, kpcOverrides)

Add EnableUnidling flag to openshift-sdn #214

Add EnableUnidling flag to openshift-sdn #214

Conversation

squeed commented Jun 26, 2019 • edited

danwinship commented Jun 26, 2019

danwinship commented Jun 26, 2019

danwinship left a comment

Choose a reason for hiding this comment

squeed commented Jul 18, 2019

danwinship left a comment

Choose a reason for hiding this comment

danwinship Jul 18, 2019

Choose a reason for hiding this comment

danwinship Jul 18, 2019

Choose a reason for hiding this comment

squeed Jul 18, 2019

Choose a reason for hiding this comment

squeed commented Jul 18, 2019

danwinship Jul 18, 2019

Choose a reason for hiding this comment

squeed Jul 18, 2019

Choose a reason for hiding this comment

danwinship Jul 18, 2019

Choose a reason for hiding this comment

squeed Jul 18, 2019

Choose a reason for hiding this comment

squeed commented Jul 18, 2019

danwinship commented Jul 19, 2019

openshift-ci-robot commented Jul 19, 2019

squeed commented Jul 19, 2019

squeed commented Jun 26, 2019 •

edited