NE-1790: Enable Dynamic Configuration Manager feature gate #1159
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -84,6 +84,11 @@ const ( | |||||
|
|
||||||
| RouterHAProxyConfigManager = "ROUTER_HAPROXY_CONFIG_MANAGER" | ||||||
|
|
||||||
| RouterHAProxyMaxDynamicServers = "ROUTER_MAX_DYNAMIC_SERVERS" | ||||||
| RouterHAProxyMaxDynamicServersDefaultValue = 1 | ||||||
|
|
||||||
| RouterHAProxyBlueprintRoutePoolSize = "ROUTER_BLUEPRINT_ROUTE_POOL_SIZE" | ||||||
|
Comment on lines
+87
to
+90
There was a problem hiding this comment. These two env variables (ROUTER_MAX_DYNAMIC_SERVERS and ROUTER_BLUEPRINT_ROUTE_POOL_SIZE) are not found in our haproxy.config template yet and I don't see a PR that adds them. Does that matter? There was a problem hiding this comment. They are not used in the config template, they are only for the configuration of the router's DCM behavior. There was a problem hiding this comment. Ok, but it's strange that they'd need to be env variables in that case. There was a problem hiding this comment. The operator usually uses environment variables rather than command-line switches for openshift-router, for which reason I asked Andrey to use environment variables in this case too (#1159 (comment)). |
||||||
|
|
||||||
| RouterHAProxyContstats = "ROUTER_HAPROXY_CONTSTATS" | ||||||
|
|
||||||
| RouterHAProxyThreadsEnvName = "ROUTER_THREADS" | ||||||
|
|
@@ -121,7 +126,7 @@ func (r *reconciler) ensureRouterDeployment(ci *operatorv1.IngressController, in | |||||
| if err != nil { | ||||||
| return false, nil, fmt.Errorf("failed to determine if proxy protocol is needed for ingresscontroller %s/%s: %v", ci.Namespace, ci.Name, err) | ||||||
| } | ||||||
| desired, err := desiredRouterDeployment(ci, r.config.IngressControllerImage, ingressConfig, infraConfig, apiConfig, networkConfig, proxyNeeded, haveClientCAConfigmap, clientCAConfigmap, clusterProxyConfig, r.config.RouteExternalCertificateEnabled, r.config.IngressControllerDCMEnabled) | ||||||
|
There was a problem hiding this comment. nit: if there is anything else to change, I would suggest refactoring the There was a problem hiding this comment. Yes, this can be done in a separate PR. |
||||||
| if err != nil { | ||||||
| return haveDepl, current, fmt.Errorf("failed to build router deployment: %v", err) | ||||||
| } | ||||||
|
|
@@ -243,7 +248,7 @@ func headerValues(values []operatorv1.IngressControllerHTTPHeader) string { | |||||
| } | ||||||
|
|
||||||
| // desiredRouterDeployment returns the desired router deployment. | ||||||
| func desiredRouterDeployment(ci *operatorv1.IngressController, ingressControllerImage string, ingressConfig *configv1.Ingress, infraConfig *configv1.Infrastructure, apiConfig *configv1.APIServer, networkConfig *configv1.Network, proxyNeeded bool, haveClientCAConfigmap bool, clientCAConfigmap *corev1.ConfigMap, clusterProxyConfig *configv1.Proxy, routeExternalCertificateEnabled, ingressControllerDCMEnabled bool) (*appsv1.Deployment, error) { | ||||||
| deployment := manifests.RouterDeployment() | ||||||
| name := controller.RouterDeploymentName(ci) | ||||||
| deployment.Name = name.Name | ||||||
|
|
@@ -523,6 +528,15 @@ func desiredRouterDeployment(ci *operatorv1.IngressController, ingressController | |||||
| LoadBalancingAlgorithm string `json:"loadBalancingAlgorithm"` | ||||||
| DynamicConfigManager string `json:"dynamicConfigManager"` | ||||||
| ContStats string `json:"contStats"` | ||||||
| // maxDynamicServers specifies the number of dynamic server slots that will be added to each route. | ||||||
| // This setting can only be configured if dynamicConfigManager is `true`. | ||||||
| // The default value is 1, with a maximum of 10. | ||||||
|
There was a problem hiding this comment.
Suggested change
|
||||||
| // Dynamic server slots help to avoid reloads during endpoint scale-ups. The more dynamic servers | ||||||
| // added, the fewer reloads required when scaling up. | ||||||
| // Note, however, that dynamic servers consume memory even when not enabled. | ||||||
| // Use this analysis of the memory usage to assess the impact of different numbers of servers: | ||||||
| // https://gist.github.com/frobware/2b527ce3f040797909eff482a4776e0b | ||||||
|
There was a problem hiding this comment. nit: When this becomes suppported, we should add a link to an OpenShift repo. |
||||||
| MaxDynamicServers string `json:"maxDynamicServers"` | ||||||
|
There was a problem hiding this comment. This should probably be an int, not a string. Users may be confused that they'd have to specify 10000 as "10000" here. There was a problem hiding this comment. Well, I just followed the same pattern as for the other overrides. Two of them ( There was a problem hiding this comment. True, but I think we have precedent for string-based bools. Choosing the string type for numbers allows malformatting, like "100K", which will fail later. |
||||||
| } | ||||||
| if len(ci.Spec.UnsupportedConfigOverrides.Raw) > 0 { | ||||||
| if err := json.Unmarshal(ci.Spec.UnsupportedConfigOverrides.Raw, &unsupportedConfigOverrides); err != nil { | ||||||
|
|
@@ -567,13 +581,36 @@ func desiredRouterDeployment(ci *operatorv1.IngressController, ingressController | |||||
| }) | ||||||
| } | ||||||
|
|
||||||
| enableDCM := false | ||||||
| if ingressControllerDCMEnabled { | ||||||
| enableDCM = true | ||||||
| } | ||||||
| dynamicConfigOverride := unsupportedConfigOverrides.DynamicConfigManager | ||||||
| if v, err := strconv.ParseBool(dynamicConfigOverride); err == nil { | ||||||
| // Config override can still be used to opt out from DCM. | ||||||
| enableDCM = v | ||||||
| } | ||||||
|
|
||||||
| if enableDCM { | ||||||
| env = append(env, corev1.EnvVar{ | ||||||
| Name: RouterHAProxyConfigManager, | ||||||
| Value: "true", | ||||||
| }, corev1.EnvVar{ | ||||||
| Name: RouterHAProxyBlueprintRoutePoolSize, | ||||||
| Value: "0", | ||||||
| }) | ||||||
|
|
||||||
| // Max dynamic servers override is only taken into account when DCM is enabled. | ||||||
| routerHAProxyMaxDynamicServersValue := RouterHAProxyMaxDynamicServersDefaultValue | ||||||
| if v, err := strconv.Atoi(unsupportedConfigOverrides.MaxDynamicServers); err == nil && v > 0 { | ||||||
|
There was a problem hiding this comment. It looks like this allows users to set it to a value > 1, which we don't yet support. Should there be a validation to make sure it doesn't exceed 1? There was a problem hiding this comment.
There was a problem hiding this comment. It turns out we allow value > 1 in the unsupportedConfigOverride. There was a problem hiding this comment. Regarding converting string to int here, see https://github.com/openshift/cluster-ingress-operator/pull/1159/files#r1852667015 And then I guess there is the edge case where someone might try to specify a number greater than int size or greater than the HAProxy size allowed (if any). |
||||||
| routerHAProxyMaxDynamicServersValue = v | ||||||
| } | ||||||
| env = append(env, corev1.EnvVar{ | ||||||
| Name: RouterHAProxyMaxDynamicServers, | ||||||
| Value: strconv.Itoa(routerHAProxyMaxDynamicServersValue), | ||||||
| }) | ||||||
| } | ||||||
|
|
||||||
| contStats := unsupportedConfigOverrides.ContStats | ||||||
| if v, err := strconv.ParseBool(contStats); err == nil && v { | ||||||
| env = append(env, corev1.EnvVar{ | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does there need to be any change for this in the flags https://github.com/openshift/router/blob/master/pkg/cmd/infra/router/template.go#L185?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. Why? This flag accepts an environment varible as an alternative way of using it. This one is limited to 1 as minimal value and defaults to 5. I set the envvar explicitly to 1 so no default value is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that you changed the minimum for BlueprintRoutePoolSize, and wondered why the default didn't need to change for MaxDynamicServers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to deliver an MVP in 4.18, we decided to turn off blueprint routes and pre-allocate 1 dynamic server per backend. openshift/enhancements#1687 explains the rationale.