Reduce HAProxy reloads - adds support to use the haproxy dynamic config api by ramr · Pull Request #19073 · openshift/origin

ramr · 2018-03-23T02:06:55Z

Add support to the template router to dynamically configure haproxy. This reduces the reloads
needed when the router configuration changes due to routes/endpoints changing.

Note that HAProxy doesn't have support to dynamically change certificates, so we need to pre-allocate pools of "blueprint" routes to dynamically bring new routes online.

We now add/remove servers from a backend on endpoint changes (for scale up/down, service addition/removal/update, node drain/evictions etc).
Each haproxy backend (for a route) has a set of dynamic servers added named
_dynamic-pod-[1-5] - which are used when endpoints change (example for scale up).
Scaling down endpoints just sets the existing servers in a haproxy backend to "maint" or
non-serving state.

The infra router options - command line flags or environment variables named
ROUTER_DYNAMIC_SERVER_PREFIX and ROUTER_MAX_DYNAMIC_SERVERS (default: 5)
controls these options.
We dynamically add and remove routes if a route matches a pre-allocated/configured blueprint route.
The default config has blueprint routes for insecure/http, edge terminated and passthrough routes.
Re-encrypt routes are an issue as they could have custom certificates so is not currently in that
default blueprint set (commented out).
Route removal just removes the map entries, marks the servers in maint state but leaves the
backend config in haproxy intact. The backend is not referenced from anywhere, so is unused.

Route addition checks if it can bring the route online without a router reload - it searches for a
matching blueprint and then a free slot from the pre-configured pool of routes for that blueprint.
If the new route can't be dynamically configured, then the behavior is as today and a reload occurs.

A route might not match a blueprint if it has information that causes a custom haproxy config.
Examples of that could be a route that has custom certificate information or custom CA information
to validate re-encrypted routes or it could even be for custom config that is generated for the
route annotations. Those would fail to find a matching blueprint (and hence cause a reload).

However that said, the template router does also provide the ability to add custom blueprints.
These are basically just routes in a namespace which serve as templates or blueprints - similar
to the default blueprints (for insecure/http, edge terminated and passthrough routes).
So one can easily add a re-encrypt route blueprint if needed.
Also, the add route code would search the custom blueprints (in the same fashion that it does
the default blueprints - custom ones are just added to the list).

The infra router options
ROUTER_BLUEPRINT_ROUTE_NAMESPACE and
ROUTER_BLUEPRINT_ROUTE_POOL_SIZE (default: 10)
provide some knobs for these custom blueprints.

Tests will automatically use this new functionality (extended ones needed a config change) but the
others use the router image, so automatically check with the haproxy router with the dynamic config
manager enabled.

Again, a point to note here is that if we exhaust the pool of dynamic servers or blueprint routes or
don't find a matching blueprint, the template router will need/do a reload.

Associated trello card: https://trello.com/c/W21FD0v0/585-13-implement-dynamic-changes-to-the-router-routerscaledemo

/cc @knobunc @zhaozhanqi fyi

openshift-ci-robot · 2018-03-23T02:06:57Z

@ramr: GitHub didn't allow me to request PR reviews from the following users: fyi.

Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

Add support to the template router to dynamically configure haproxy. This reduces the reloads
needed when the router configuration changes due to routes/endpoints changing.

Note that HAProxy doesn't have support to dynamically change certificates, so we need to pre-allocate pools of "blueprint" routes to dynamically bring new routes online.

We now add/remove servers from a backend on endpoint changes (for scale up/down, service addition/removal/update, node drain/evictions etc).
Each haproxy backend (for a route) has a set of dynamic servers added named
_dynamic-pod-[1-5] - which are used when endpoints change (example for scale up).
Scaling down endpoints just sets the existing servers in a haproxy backend to "maint" or
non-serving state.

The infra router options - command line flags or environment variables named
ROUTER_DYNAMIC_SERVER_PREFIX and ROUTER_MAX_DYNAMIC_SERVERS (default: 5)
controls these options.

We dynamically add and remove routes if a route matches a pre-allocated/configured blueprint route.
The default config has blueprint routes for insecure/http, edge terminated and passthrough routes.
Re-encrypt routes are an issue as they could have custom certificates so is not currently in that
default blueprint set (commented out).
Route removal just removes the map entries, marks the servers in maint state but leaves the
backend config in haproxy intact. The backend is not referenced from anywhere, so is unused.

Route addition checks if it can bring the route online without a router reload - it searches for a
matching blueprint and then a free slot from the pre-configured pool of routes for that blueprint.
If the new route can't be dynamically configured, then the behavior is as today and a reload occurs.

A route might not match a blueprint if it has information that causes a custom haproxy config.
Examples of that could be a route that has custom certificate information or custom CA information
to validate re-encrypted routes or it could even be for custom config that is generated for the
route annotations. Those would fail to find a matching blueprint (and hence cause a reload).

However that said, the template router does also provide the ability to add custom blueprints.
These are basically just routes in a namespace which serve as templates or blueprints - similar
to the default blueprints (for insecure/http, edge terminated and passthrough routes).
So one can easily add a re-encrypt route blueprint if needed.
Also, the add route code would search the custom blueprints (in the same fashion that it does
the default blueprints - custom ones are just added to the list).

The infra router options
ROUTER_BLUEPRINT_ROUTE_NAMESPACE and
ROUTER_BLUEPRINT_ROUTE_POOL_SIZE (default: 10)
provide some knobs for these custom blueprints.

Tests will automatically use this new functionality (extended ones needed a config change) but the
others use the router image, so automatically check with the haproxy router with the dynamic config
manager enabled.

Again, a point to note here is that if we exhaust the pool of dynamic servers or blueprint routes or
don't find a matching blueprint, the template router will need/do a reload.

Associated trello card: https://trello.com/c/W21FD0v0/585-13-implement-dynamic-changes-to-the-router-routerscaledemo

/cc @knobunc @zhaozhanqi fyi

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.

smarterclayton · 2018-03-23T03:36:06Z

glide.yaml

 - package: k8s.io/apiserver
-  repo:    git@github.com:openshift/kubernetes-apiserver
  version: release-1.9.1
+  repo: git@github.com:openshift/kubernetes-apiserver


Please undo this reformatting.

That's all done by glide. I couldn't run glide on my local repo - it doesn't run for me, times out all the time, so @rajatchopra ran it for me and he didn't do anything other than glide get && ./hack/update-deps.sh. Is something that's glide version specific causing this?

I’ve never had glide time out - are you on an old version? Bad internet?

glide version is v0.13.1 and its a fast[ish] pipe - 200mb+ consistently (and also tried it on a T3 connection) - so i suspect its not the speed. Plus did try it on 3 different vms as well (all fedora 27 though), so something else. Anyway, will look at it at some other time.

smarterclayton · 2018-03-23T03:36:40Z

vendor/k8s.io/kubernetes/pkg/volume/configmap/configmap.go

 	if err := wrapped.SetUpAt(dir, fsGroup); err != nil {
 		return err
 	}
-	if err := volumeutil.MakeNestedMountpoints(b.volName, dir, b.pod); err != nil {


Please fix this, you reverted changes.

Hmm, maybe because we did the glide get and update-deps.sh a few weeks back - this might be outdated. Will do.

smarterclayton · 2018-03-23T03:37:21Z

test/end-to-end/router_test.go

 		fmt.Sprintf("NAMESPACE_LABELS=%s", namespaceLabels),
+		fmt.Sprintf("ROUTER_CONFIG_MANAGER=haproxy-manager"),
+		fmt.Sprintf("ROUTER_DYNAMIC_SERVER_PREFIX=_test-dynamic"),
+		fmt.Sprintf("ROUTER_MAX_DYNAMIC_SERVERS=3"),


These should have reasonable defaults. I'd prefer if your testing required only ROUTER_CONFIG_MANAGER unless there's a good reason not to.

ok - i'll set it to only the defaults.

smarterclayton · 2018-03-23T03:37:45Z

pkg/router/template/types.go

+	Initialize(router RouterInterface, certPath string)
+
+	// Register registers an id to be associated with a route.
+	Register(id string, route *routeapi.Route, wildcard bool)


Why are you passing wildcard here when it should be on the route?

We already had boolean deciphered based on the policy so I just passed the boolean but yeah we could recompute it in register/add.

smarterclayton · 2018-03-23T03:38:25Z

pkg/router/template/types.go

+
+// ConfigManager is used by the router to make configuration changes using
+// the template router's dynamic configuration API (if any).
+type ConfigManager interface {


Please specify the threading guarantees this code requires with the router controller here.

smarterclayton · 2018-03-23T03:39:38Z

pkg/router/template/configmanager/haproxy/map.go

@@ -0,0 +1,209 @@
+package haproxy


Give this its own unit tests.

there's a map_test.go in there. Or did you mean something else?

smarterclayton · 2018-03-23T03:40:04Z

pkg/router/template/configmanager/haproxy/converter.go

+}
+
+// NewCSVConverter returns a new CSVConverter.
+func NewCSVConverter(headers string, out interface{}, fn ByteConverterFunc) CSVConverter {


Should be a pointer unless you have a good reason not to.

Not really - ok will change.

smarterclayton · 2018-03-23T03:40:23Z

pkg/router/template/configmanager/haproxy/converter.go

+
+// Convert runs a haproxy dynamic config API command.
+func (c CSVConverter) Convert(data []byte) ([]byte, error) {
+	glog.V(4).Infof("CSV converter input data bytes: %s", string(data))


How much data is this? If it's going to regularly be more than a few hundred bytes it should be at v5.

yeah it might be more than a few hundred - will set to v5.

smarterclayton · 2018-03-23T03:40:43Z

pkg/router/template/configmanager/haproxy/converter.go

+	if c.converterFunc != nil {
+		convertedBytes, err := c.converterFunc(data)
+		if err != nil {
+			glog.Errorf("CSV converter error: %v", err)


Don't log errors like this, wrap or log when you eat it with utilruntime.HandleError

ok will fix.

I think this was a remnant from the logs I was generating during dev - will remove this as its not needed - error are anyways propagated up and handled by the caller.

smarterclayton · 2018-03-23T03:41:21Z

pkg/router/template/configmanager/haproxy/converter.go

+// fixupHeaders fixes up haproxy API responses that don't contain any CSV
+// header information. This allows us to easily parse the data and marshal
+// into an array of native golang structs.
+func (c CSVConverter) fixupHeaders(data []byte) ([]byte, error) {


don't make this a method, just a function that takes headers and data.

Give this file unit tests.

smarterclayton · 2018-03-23T03:41:33Z

pkg/router/template/configmanager/haproxy/client.go

+}
+
+// isRetryable checks if a haproxy command can be retried.
+func isRetryable(err error, cmd string) bool {


isRetriable

smarterclayton · 2018-03-23T03:42:09Z

pkg/router/template/configmanager/haproxy/client.go

+// Backends returns the list of configured haproxy backends.
+func (c *Client) Backends() ([]*Backend, error) {
+	if len(c.backends) == 0 {
+		if backends, err := GetHAProxyBackends(c); err != nil {


If these methods don't need to be public (GetHAProxyBackends) they shouldn't be public.

smarterclayton · 2018-03-23T03:43:01Z

pkg/router/template/configmanager/haproxy/client.go

+// if the error for the command is a retryable error.
+func (c *Client) runCommandWithRetries(cmd string, limit int) (*bytes.Buffer, error) {
+	retryAttempt := 0
+	for {


use util.ExponentialBackoff or util.PollImmediate instead of inventing your own code for it.

smarterclayton · 2018-03-23T03:43:56Z

pkg/oc/admin/router/router.go

-		env["ROUTER_METRICS_TLS_CERT_FILE"] = "/etc/pki/tls/metrics/tls.crt"
-		env["ROUTER_METRICS_TLS_KEY_FILE"] = "/etc/pki/tls/metrics/tls.key"
+	if cfg.Type == "haproxy-router" {
+		env["ROUTER_CONFIG_MANAGER"] = "haproxy-manager"


If everyone is going to get these just make these the new default on the command. Don't force people to change their config and make a pointless choice. Otherwise you'll have to add this to the upgrade scripts.

ok will do.

hmm, so because the template router is also used for nginx, this needs to be either set here since its haproxy specific or we need to pass down the type of router to the infra command (that's not done today). I can either pass down this new setting or then the type and set the defaults based on that in the infra command ... but in either case it still means upgrading existing haproxy-router deployments. So which do you prefer?
Edited

I think if the config manager works, there’s no reason we wouldn’t have it on by default for haproxy. However, we will likely default it off at first until we’re comfortable that we’ve caught all the edge cases and then default it on. So ideally we just have a Boolean that has an internal default and if the user specifies it we obey their rule. That avoids end users ending up with the flag in their config.

smarterclayton · 2018-03-23T03:45:08Z