router: Add option to bind ports only when ready #11768

marun · 2016-11-04T05:35:19Z

By default the router binds ports 80 and 443 even if no routing configuration is available. This may be desirable when haproxy is serving traffic directly to clients. However, the f5 loadbalancer will treat bound ports as indicating that a backend is ready to receive traffic. This means that router pods that are starting or restarting may be put into rotation before they are ready to serve the current route state and clients may see 503s for perfectly healthy endpoints. To avoid this possibility, this change adds an option (ROUTER_BIND_PORTS_AFTER_SYNC/--bind-ports-after-sync) to the router that ensures that ports are bound only when a router instance has synced with the api.

Reference: bug 1383663 link

cc: @openshift/networking

marun · 2016-11-04T05:35:24Z

[test]

knobunc · 2016-11-04T13:04:41Z

images/router/haproxy/conf/haproxy-config.template

@@ -85,6 +85,10 @@ listen stats :1936
    mode http
    # Health check monitoring uri.
    monitor-uri /healthz
+{{ if (and .State .ServiceUnits) }}
+    # This uri being configured indicates that routing configuration has been synchronized.
+    monitor-uri /readyz


What's with the z's? Should we really use that? I was being somewhat facetious on the call when I said it.

knobunc · 2016-11-04T13:08:01Z

pkg/cmd/infra/router/template.go

@@ -86,6 +87,7 @@ func (o *TemplateRouter) Bind(flag *pflag.FlagSet) {
 	flag.StringVar(&o.ReloadScript, "reload", util.Env("RELOAD_SCRIPT", ""), "The path to the reload script to use")
 	flag.DurationVar(&o.ReloadInterval, "interval", reloadInterval(), "Controls how often router reloads are invoked. Mutiple router reload requests are coalesced for the duration of this interval since the last reload time.")
 	flag.BoolVar(&o.ExtendedValidation, "extended-validation", util.Env("EXTENDED_VALIDATION", "true") == "true", "If set, then an additional extended validation step is performed on all routes admitted in by this router. Defaults to true and enables the extended validation checks.")
+	flag.BoolVar(&o.BindOnlyWhenReady, "bind-only-when-ready", util.Env("BIND_ONLY_WHEN_READY", "") == "true", "Only bind ports when route state has been synchronized")


The more recent trend has been to put ROUTER_ in the name (see openshift/openshift-docs#3090 for the complete list)

Should we do that here?

Naming of the flag - I wouldn't say "ready" because that has overlap with our existing readiness concepts. I would prefer "--bind-ports-after-sync"

knobunc · 2016-11-04T13:09:06Z

images/router/haproxy/conf/haproxy-config.template

@@ -85,6 +85,10 @@ listen stats :1936
    mode http
    # Health check monitoring uri.
    monitor-uri /healthz
+{{ if (and .State .ServiceUnits) }}


If there are no services in the system (or that match our shard params) will this still bind?

knobunc · 2016-11-04T13:18:41Z

@smarterclayton PTAL

knobunc · 2016-11-04T14:45:57Z

Flake #11775 [test]

knobunc · 2016-11-04T16:29:47Z

flake #10773 [test]

marun · 2016-11-04T18:26:08Z

As per an irc conversation with @knobunc readiness should denote that route state has been synchronized, even if there is no state available.

smarterclayton · 2016-11-04T21:34:18Z

pkg/router/controller/controller.go

+				return
+			}
+		}
+		time.Sleep(1)


This is a hot loop, don't do this. Sleep longer.

Minimum of 50ms

Good call, I keep forgetting that time.Sleep() takes a duration not an int. My intention was to sleep for 1s.

smarterclayton · 2016-11-04T21:34:30Z

pkg/router/controller/controller.go

+			(c.Namespaces == nil || c.filteredByNamespace)
+		if syncedAtLeastOnce {
+			if err := c.Plugin.SetSyncedAtLeastOnce(); err != nil {
+				utilruntime.HandleError(err)


invert this condition and return early.

marun · 2016-11-07T06:04:35Z

I'm still working on this, coordinating readiness and reload is more complicated than I originally thought.

marun · 2016-11-08T12:27:33Z

A router now becomes ready even when no items are queued for either of the watchers. Some of the integration tests are broken, though. Will investigate tomorrow.

marun · 2016-11-09T21:35:27Z

It's not possible to configure multiple monitor-uri paths on a given stats port, so adding a readiness check would require opening multiple stats ports. I'm assuming that is out of scope here, so this PR no longer includes the addition of a readiness check.

knobunc · 2016-11-09T21:39:25Z

@ramr @openshift/networking PTAL

By default the router binds ports 80 and 443 even if no routing configuration is available. This may be desirable when haproxy is serving traffic directly to clients. However, the f5 loadbalancer will treat bound ports as indicating that a backend is ready to receive traffic. This means that router pods that are starting or restarting may be put into rotation before they are ready to serve the current route state and clients may see 503s for perfectly healthy backend endpoints. To avoid this possibility, this change adds an option (BIND_ONLY_WHEN_READY/--bind-only-when-ready) to the router that ensures that ports are bound only when a router instance has route and endpoint state available. Reference: bz1383663

marun · 2016-11-09T23:22:30Z

flake #11685 [test]

marun · 2016-11-11T17:35:47Z

flake #11887 re-[test]

openshift-bot · 2016-11-11T18:13:49Z

Evaluated for origin test up to 7b3e0d3

openshift-bot · 2016-11-11T19:46:42Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11402/) (Base Commit: 74a2188)

ramr · 2016-11-30T01:07:48Z

pkg/router/controller/controller.go

+
+	synced := c.RoutesListSuccessfulAtLeastOnce() &&
+		c.EndpointsListSuccessfulAtLeastOnce() &&
+		(c.Namespaces == nil || c.filteredByNamespace)


Is the c.Namespaces == nil check needed? Wouldn't c.filteredByNamespace suffice? I see c.filteredByNamespace set in handle namespaces on no errors.

Yes, it is needed. If c.Namespaces == nil then c.filteredByNamespace will never be set to true.

If c.Namespaces == nil, then c.HandleNamespaces() will never be called:

https://github.com/openshift/origin/blob/master/pkg/router/controller/controller.go#L51

ramr · 2016-11-30T01:08:46Z

Changes LGTM - just a small question.

knobunc · 2016-12-01T15:10:52Z

[merge]

openshift-bot · 2016-12-01T15:13:17Z

Evaluated for origin merge up to 7b3e0d3

openshift-bot · 2016-12-01T17:37:35Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11402/) (Base Commit: 3bab31d) (Image: devenv-rhel7_5458)

marun added the component/networking label Nov 4, 2016

marun force-pushed the bz1383663 branch 2 times, most recently from ee87525 to 64ac08b Compare November 4, 2016 06:23

knobunc requested changes Nov 4, 2016

View reviewed changes

smarterclayton reviewed Nov 4, 2016

View reviewed changes

marun force-pushed the bz1383663 branch from 0da33d3 to 8723241 Compare November 8, 2016 12:26

marun force-pushed the bz1383663 branch from 8723241 to 76d419f Compare November 9, 2016 21:34

marun force-pushed the bz1383663 branch from 76d419f to 7b3e0d3 Compare November 9, 2016 23:07

knobunc approved these changes Nov 10, 2016

View reviewed changes

marun assigned ramr Nov 29, 2016

ramr reviewed Nov 30, 2016

View reviewed changes

openshift-bot merged commit e43ec15 into openshift:master Dec 1, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

router: Add option to bind ports only when ready #11768

router: Add option to bind ports only when ready #11768

marun commented Nov 4, 2016 •

edited

Loading

marun commented Nov 4, 2016

knobunc Nov 4, 2016

knobunc Nov 4, 2016

smarterclayton Nov 4, 2016

knobunc Nov 4, 2016

knobunc commented Nov 4, 2016

knobunc commented Nov 4, 2016

knobunc commented Nov 4, 2016

marun commented Nov 4, 2016

smarterclayton Nov 4, 2016

smarterclayton Nov 4, 2016

marun Nov 4, 2016

smarterclayton Nov 4, 2016

marun commented Nov 7, 2016

marun commented Nov 8, 2016

marun commented Nov 9, 2016

knobunc commented Nov 9, 2016

marun commented Nov 9, 2016

marun commented Nov 11, 2016

openshift-bot commented Nov 11, 2016

openshift-bot commented Nov 11, 2016

ramr Nov 30, 2016

marun Nov 30, 2016

marun Nov 30, 2016

ramr commented Nov 30, 2016

knobunc commented Dec 1, 2016

openshift-bot commented Dec 1, 2016

openshift-bot commented Dec 1, 2016 •

edited

Loading

router: Add option to bind ports only when ready #11768

router: Add option to bind ports only when ready #11768

Conversation

marun commented Nov 4, 2016 • edited Loading

marun commented Nov 4, 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

knobunc commented Nov 4, 2016

knobunc commented Nov 4, 2016

knobunc commented Nov 4, 2016

marun commented Nov 4, 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

marun commented Nov 7, 2016

marun commented Nov 8, 2016

marun commented Nov 9, 2016

knobunc commented Nov 9, 2016

marun commented Nov 9, 2016

marun commented Nov 11, 2016

openshift-bot commented Nov 11, 2016

openshift-bot commented Nov 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramr commented Nov 30, 2016

knobunc commented Dec 1, 2016

openshift-bot commented Dec 1, 2016

openshift-bot commented Dec 1, 2016 • edited Loading

marun commented Nov 4, 2016 •

edited

Loading

openshift-bot commented Dec 1, 2016 •

edited

Loading