Implement nbthreads for haproxy router #19975

imcsk8 · 2018-06-12T08:09:54Z

Add threading support for router.

Trello: https://trello.com/c/0thyZwBw/25-3-implement-nbthreads-for-haproxy-router-routerperformanceoperations

imcsk8 · 2018-06-12T08:10:53Z

PTAL @openshift/sig-networking

knobunc · 2018-06-12T13:53:14Z

/hold
This is a 3.11 feature. Holding until the branch opens.

knobunc · 2018-06-12T13:54:51Z

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

@@ -33,6 +33,11 @@

 global
  maxconn {{env "ROUTER_MAX_CONNECTIONS" "20000"}}
+{{- if isTrue (env "ROUTER_ENABLE_TRHEADS") }}
+  nbthread {{env "ROUTER_THREADS" "4"}}


Let's just do it based on whether ROUTER_THREADS is defined and > 1, and then default to 1 thread.

+1, ROUTER_ENABLE_TRHEADS seems redundant.

pravisankar · 2018-06-13T15:23:22Z

pkg/oc/admin/router/router.go

@@ -308,6 +311,7 @@ func NewCmdRouter(f *clientcmd.Factory, parentName, name string, out, errout io.
 	cmd.Flags().StringVar(&cfg.Ciphers, "ciphers", cfg.Ciphers, "Specifies the cipher suites to use. You can choose a predefined cipher set ('modern', 'intermediate', or 'old') or specify exact cipher suites by passing a : separated list. Not supported for F5.")
 	cmd.Flags().BoolVar(&cfg.StrictSNI, "strict-sni", cfg.StrictSNI, "Use strict-sni bind processing (do not use default cert). Not supported for F5.")
 	cmd.Flags().BoolVar(&cfg.Local, "local", cfg.Local, "If true, do not contact the apiserver")
+	cmd.Flags().StringVar(&cfg.Threads, "threads", cfg.Threads, "Specifies the amount of threads for the haproxy router.")


Specifies the number of threads... (number instead of amount)

Use cmd.Flags().Int32Var() instead of cmd.Flags().StringVar() then you don't need to do integer validations for this param.

Is there a max thread limit? (very high thread count does not seem practical)

i haven't tested the thread limit, i'll check that

ironcladlou · 2018-06-13T15:48:19Z

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

+{{- if ne (env "ROUTER_THREADS") "" }}
+  nbthread {{env "ROUTER_THREADS"}}
+{{-  end }}
+


Could be simplified by setting the default threads to 1 and removing the conditional entirely, assuming the controller code is the only consumer of this template

I'd rather not have the nbthreads word in the config file if it is not being used... the feature is beta in haproxy, so while it should be equivalent, who knows for sure?

But why not use 1 as the default for env and say gt 1?
{{- if gt (env "ROUTER_THREADS" 1) 1 }}

Agree re: the beta status, good point. Regarding the conditional, the way you suggest here makes more sense once the data type is corrected to be an int.

ironcladlou · 2018-06-13T15:48:40Z

pkg/oc/admin/router/router.go

@@ -228,6 +228,9 @@ type RouterConfig struct {
 	// Strict SNI (do not use default cert)
 	StrictSNI bool

+	// Number of threads to start per process
+	Threads string


Should be an int

knobunc · 2018-06-13T16:08:25Z

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

+{{- if ne (env "ROUTER_THREADS") "" }}
+  nbthread {{env "ROUTER_THREADS"}}
+{{-  end }}
+


I'd rather not have the nbthreads word in the config file if it is not being used... the feature is beta in haproxy, so while it should be equivalent, who knows for sure?

But why not use 1 as the default for env and say gt 1?
{{- if gt (env "ROUTER_THREADS" 1) 1 }}

pravisankar

Also needs corresponding docs pr

pravisankar · 2018-06-13T15:27:58Z

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

@@ -33,6 +33,11 @@

 global
  maxconn {{env "ROUTER_MAX_CONNECTIONS" "20000"}}
+{{- if ne (env "ROUTER_THREADS") "" }}
+  nbthread {{env "ROUTER_THREADS"}}


Don't we want this for ROUTER_THREADS > 1?

Should we be configuring cpu-map here to make sure threads are mapped to cpu sets (ensuring the workload is distributed)? If we detect the number of cores and set nbthread to that there is an auto setting.

ref

cpu-map needs nbproc, this could be more complex , but it might be possible since we're already reading processor information.
we could have something like this

nbproc {{env "ROUTER_NUM_CPUS"}}
cpu-map auto:/all 0-{{env "ROUTER_NUM_CPUS" - 1}} this operation is not valid on the template is just as an example
nbthread {{env "ROUTER_THREADS"}}

pravisankar · 2018-06-13T16:46:27Z

contrib/completions/zsh/oc

@@ -5949,6 +5949,8 @@ _oc_adm_router()
    local_nonpersistent_flags+=("--disable-namespace-ownership-check")
    flags+=("--dry-run")
    local_nonpersistent_flags+=("--dry-run")
+    flags+=("--enable-threads")
+    local_nonpersistent_flags+=("--enable-threads")


You need to re-generate bash completions

oh yeah i forgot to run the completions again

pravisankar · 2018-06-13T16:48:16Z

pkg/oc/admin/router/router.go

@@ -308,6 +311,7 @@ func NewCmdRouter(f *clientcmd.Factory, parentName, name string, out, errout io.
 	cmd.Flags().StringVar(&cfg.Ciphers, "ciphers", cfg.Ciphers, "Specifies the cipher suites to use. You can choose a predefined cipher set ('modern', 'intermediate', or 'old') or specify exact cipher suites by passing a : separated list. Not supported for F5.")
 	cmd.Flags().BoolVar(&cfg.StrictSNI, "strict-sni", cfg.StrictSNI, "Use strict-sni bind processing (do not use default cert). Not supported for F5.")
 	cmd.Flags().BoolVar(&cfg.Local, "local", cfg.Local, "If true, do not contact the apiserver")
+	cmd.Flags().StringVar(&cfg.Threads, "threads", cfg.Threads, "Specifies the amount of threads for the haproxy router.")


Use cmd.Flags().Int32Var() instead of cmd.Flags().StringVar() then you don't need to do integer validations for this param.

pravisankar · 2018-06-13T16:50:56Z

pkg/oc/admin/router/router.go

@@ -308,6 +311,7 @@ func NewCmdRouter(f *clientcmd.Factory, parentName, name string, out, errout io.
 	cmd.Flags().StringVar(&cfg.Ciphers, "ciphers", cfg.Ciphers, "Specifies the cipher suites to use. You can choose a predefined cipher set ('modern', 'intermediate', or 'old') or specify exact cipher suites by passing a : separated list. Not supported for F5.")
 	cmd.Flags().BoolVar(&cfg.StrictSNI, "strict-sni", cfg.StrictSNI, "Use strict-sni bind processing (do not use default cert). Not supported for F5.")
 	cmd.Flags().BoolVar(&cfg.Local, "local", cfg.Local, "If true, do not contact the apiserver")
+	cmd.Flags().StringVar(&cfg.Threads, "threads", cfg.Threads, "Specifies the amount of threads for the haproxy router.")


Is there a max thread limit? (very high thread count does not seem practical)

pravisankar · 2018-06-14T17:12:41Z

pkg/oc/admin/router/router.go

@@ -228,6 +228,9 @@ type RouterConfig struct {
 	// Strict SNI (do not use default cert)
 	StrictSNI bool

+	// Number of threads to start per process
+	Threads int


Didn't we wanted to use fixed width integers even for internal types, i.e. int32/64 over int?
Refer: #9366 and 865faaf
@knobunc @ironcladlou

Yes, although I don't think we're dealing with formal versioned API here... no objection to uint32 regardless

smarterclayton · 2018-06-15T00:36:31Z

pkg/oc/admin/router/router.go

@@ -308,6 +311,7 @@ func NewCmdRouter(f *clientcmd.Factory, parentName, name string, out, errout io.
 	cmd.Flags().StringVar(&cfg.Ciphers, "ciphers", cfg.Ciphers, "Specifies the cipher suites to use. You can choose a predefined cipher set ('modern', 'intermediate', or 'old') or specify exact cipher suites by passing a : separated list. Not supported for F5.")
 	cmd.Flags().BoolVar(&cfg.StrictSNI, "strict-sni", cfg.StrictSNI, "Use strict-sni bind processing (do not use default cert). Not supported for F5.")
 	cmd.Flags().BoolVar(&cfg.Local, "local", cfg.Local, "If true, do not contact the apiserver")
+	cmd.Flags().IntVar(&cfg.Threads, "threads", cfg.Threads, "Specifies the amount of threads for the haproxy router.")


I don't know why we would configure this - I think we want it to be autodetected by default. Probably based on the number of cores configured for this pod.

If there is a generally correct value we should be choosing, then we should have it autodetect.

We could set as default half or a quarter of the max number of threads per process (/proc/sys/kernel/threads-max) but i think that we should allow the sysadmin to tune this setting to suit his needs.

Auto-detecting a default sounds great, but what I wonder is if we need to also provide a way to override the default; if so, then the defaulting logic could be a followup. If not an explicit value configuration, would we at least want an overall feature flag?

We can add the --enable-threads flag, if no --threads flag is present a default is used for the nbthreads setting.
If the --threads flag is set just check that it's value is not higher than the one in /proc/sys/kernel/threads-max

Another option could be to only have the --threads flag, if no value is set use the default but i think this could be confusing

smarterclayton · 2018-06-15T00:37:03Z

pkg/oc/admin/router/router.go

@@ -655,6 +659,7 @@ func RunCmdRouter(f *clientcmd.Factory, cmd *cobra.Command, out, errout io.Write
 		"STATS_PORT":                            strconv.Itoa(cfg.StatsPort),
 		"STATS_USERNAME":                        cfg.StatsUsername,
 		"STATS_PASSWORD":                        cfg.StatsPassword,
+		"ROUTER_THREADS":                        strconv.Itoa(cfg.Threads),


Would prefer we not set this here. I don't see why a user would set this flag, and not just expect us to do the right thing.

What is the right number for the threads? How do we know what else they want to use the machine for? Or are you suggesting we pick it up from the container limits? The other problem there is that the number of processes that are running will vary if they have long-running connections and have router reloads.

I really don't want to enable this by default yet, so we at least need a way to disable it. I'm fine with an "auto" setting for num threads, but I suspect we will need a way to tune it.

+1 on auto setting the default num threads based on pod cpu limits and allow this value to be overridden via env var or some other mechanism.

Without knowing how effective our auto-detected number is in practice, does it make any sense to add an explicitly unsupported parameter to give an escape hatch and allow us to test tunings without a new build?

When we have higher confidence our auto-detected algo is solid we could talk about removing the override. There is a bit of UX complexity there in explaining the mutual exclusivity or precedence order of the auto/explicit options, but if the override is considered unsupported/debugging-only maybe it isn't as big an issue?

JacobTanenbaum · 2018-06-25T20:15:28Z

pkg/oc/admin/router/router.go

+		"ROUTER_ENABLE_THREADS":                 strconv.FormatBool(cfg.EnableThreads),
+	}
+
+	if cfg.EnableThreads {


Should an error be returned if a number of threads is specified but threads are not enabled?

Yes, an error or at least a warning that the --threads is ignored when --enable-threads is not set.

pravisankar · 2018-06-25T20:48:20Z

pkg/oc/admin/router/router.go

+		"ROUTER_ENABLE_THREADS":                 strconv.FormatBool(cfg.EnableThreads),
+	}
+
+	if cfg.EnableThreads {


Yes, an error or at least a warning that the --threads is ignored when --enable-threads is not set.

pravisankar · 2018-06-25T20:51:46Z

pkg/oc/admin/router/router.go

+	}
+
+	if cfg.EnableThreads {
+		if cfg.Threads >= 1 {


Add validations for cfg.Threads (check >= minThreads and <= maxThreads)?

pravisankar · 2018-06-25T21:36:13Z

pkg/oc/admin/router/router.go

+	cpuinfo := readProcFile("/proc/cpuinfo")
+	siblings, _ := strconv.Atoi(cpuinfo["siblings"])
+	cores, _ := strconv.Atoi(cpuinfo["cpucores"])
+	threads := siblings * cores


Don't we want to consider CPU requests/limits specified by the router pod/container?
https://kubernetes.io/docs/tasks/configure-pod-container/assign-cpu-resource/#specify-a-cpu-request-and-a-cpu-limit

If the router is running as a privileged container, /proc/cpuinfo will return container or host limits?

If the router is running as a privileged container, /proc/cpuinfo will return container or host limits?

Worse than that, it will be /proc/cpuinfo on the host where the administrator runs oc adm router, which is probably not the node host. Calculating the default number of threads really needs to be done in pkg/router/template/router.go, not pkg/oc/admin/router/router.go.

Miciah · 2018-07-03T19:32:14Z

pkg/router/template/template_helper.go

@@ -257,6 +265,7 @@ var helperFunctions = template.FuncMap{
 	"env":          env,          //tries to get an environment variable, returns the first non-empty default value or "" on failure
 	"matchPattern": matchPattern, //anchors provided regular expression and evaluates against given string
 	"isInteger":    isInteger,    //determines if a given variable is an integer
+	"toInt":        toInt,        //Convert string to integer


Can we call it toInteger in the map for consistency with isInteger?

Miciah · 2018-07-03T19:43:57Z

pkg/router/template/template_helper.go

+func toInt(s string) int {
+	ret, err := strconv.Atoi(s)
+	if err != nil {
+		return 0


Since this is becoming a part of the informal API for router templates, we should consider how it might be used or expected to work in the general case. Is returning 0 for invalid input a sensible default? Or should we return nil? For example, we could do the following:

func toInt(s string) *int { ret, err := strconv.Atoi(s) if err != nil { return nil } return &ret }

Which could be used as follows:

{{- with $threads := toInt (env "ROUTER_THREADS") }} {{- if gt $threads 0 }} nbthread {{ $threads }} {{- end }} {{- end }}

The part inside the with block will get skipped entirely if strconv.Atoi returns an error.

Maybe document it as private (FWIW)?

I think this is another example of where a new field on an object passed to the template helps. There's no reason to do conversion inside the template itself: why would a user ever want to treat ROUTER_THREADS as anything but an int? If I were making a custom template and had to deal with this conversion manually I'd be annoyed

knobunc · 2018-07-03T19:53:00Z

I'm not sure that we want to add all this complexity... I was good with the ENV as long as we changed gt to ne... @ironcladlou your call though.

ironcladlou · 2018-07-03T20:04:11Z

I'm fine with the ne solution in #19975 (comment) given our reluctance to modify the data struct or add functions

imcsk8 · 2018-07-03T22:34:01Z

I find the ne solution more complex than adding a conversion function which simplifies the logic on the template

ironcladlou · 2018-07-04T02:06:33Z

@imcsk8

I find the ne solution more complex than adding a conversion function which simplifies the logic on the template

I agree, however using the ne solution reduces the amount of new "implicit API surface area" by 1, so that gets my vote (although I would really like to have a separate convo about the API going forward)

imcsk8 · 2018-07-04T14:13:14Z

Ok, i'll change the code for the ne solution

ironcladlou · 2018-07-05T13:26:08Z

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

@@ -33,6 +33,12 @@

 global
  maxconn {{env "ROUTER_MAX_CONNECTIONS" "20000"}}
+{{- $threads := env "ROUTER_THREADS"
+{{- if ne "" (firstMatch "[1-9][0-9]*" $threads) }}


Did the clearer alternative not work?

if ne "0" (env "ROUTER_THREADS" "0")

And then adding range validation in the Go code

Yes, ^^ this looks clean to me.

Miciah · 2018-07-05T17:51:04Z

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

@@ -33,6 +33,12 @@

 global
  maxconn {{env "ROUTER_MAX_CONNECTIONS" "20000"}}
+{{- $threads := env "ROUTER_THREADS"


Missing }}.

Hopefully that was caught by an automated test...

knobunc

/lgtm
/approve

knobunc · 2018-07-09T20:38:43Z

@juanvallejo @enj @soltysh please approve the completions changes.

knobunc · 2018-07-09T20:38:57Z

/retest

juanvallejo · 2018-07-09T21:35:59Z

completion changes lgtm

pravisankar

/hold
You need to squash your commits.

pravisankar · 2018-07-09T22:37:32Z

@imcsk8 You preferred regex match over #19975 (comment) because once router dc is generated, user could edit the dc and can fiddle with 'ROUTER_THREADS' env which needs revalidation?

knobunc · 2018-07-10T14:42:05Z

@juanvallejo Can you say /approve please so the bot is happy.

juanvallejo · 2018-07-10T14:43:37Z

/approve

Add threading support for router. Trello: https://trello.com/c/0thyZwBw/25-3-implement-nbthreads-for-haproxy-router-routerperformanceoperations Add completions Fix typo Simplify the threading options Change argument type Add enable threads flag Fix template condition Remove defaut threads setting Add proper integer condition for threads Change thread check condition Fix typo

knobunc

/hold cancel
/lgtm

openshift-ci-robot · 2018-07-10T19:29:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: imcsk8, juanvallejo, knobunc

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:

~~contrib/completions/OWNERS~~ [juanvallejo]
~~images/router/OWNERS~~ [knobunc]
~~pkg/oc/admin/router/OWNERS~~ [knobunc]

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

pravisankar

LGTM

openshift-ci-robot requested review from ironcladlou and pweil- June 12, 2018 08:09

imcsk8 requested review from knobunc and removed request for ironcladlou and pweil- June 12, 2018 08:09

openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jun 12, 2018

imcsk8 requested a review from rajatchopra June 12, 2018 08:10

imcsk8 added component/networking sig/networking labels Jun 12, 2018

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 12, 2018

knobunc requested changes Jun 12, 2018

View reviewed changes

knobunc assigned ironcladlou Jun 12, 2018

knobunc requested a review from ironcladlou June 12, 2018 13:56

imcsk8 force-pushed the router-threads branch from e14ee3a to 6fe0172 Compare June 13, 2018 14:51

pravisankar reviewed Jun 13, 2018

View reviewed changes

ironcladlou reviewed Jun 13, 2018

View reviewed changes

knobunc requested changes Jun 13, 2018

View reviewed changes

pravisankar suggested changes Jun 13, 2018

View reviewed changes

imcsk8 force-pushed the router-threads branch from 6fe0172 to 790098c Compare June 14, 2018 01:44

pravisankar reviewed Jun 14, 2018

View reviewed changes

smarterclayton reviewed Jun 15, 2018

View reviewed changes

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 22, 2018

JacobTanenbaum reviewed Jun 25, 2018

View reviewed changes

pravisankar reviewed Jun 25, 2018

View reviewed changes

imcsk8 force-pushed the router-threads branch from 12ca02d to 36ff58d Compare June 26, 2018 03:46

Miciah reviewed Jul 3, 2018

View reviewed changes

ironcladlou reviewed Jul 5, 2018

View reviewed changes

Miciah reviewed Jul 5, 2018

View reviewed changes

openshift-ci-robot assigned knobunc Jul 9, 2018

knobunc approved these changes Jul 9, 2018

View reviewed changes

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

pravisankar reviewed Jul 9, 2018

View reviewed changes

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 9, 2018

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 10, 2018

imcsk8 force-pushed the router-threads branch from 24a4ba2 to f5638e4 Compare July 10, 2018 19:27

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 10, 2018

knobunc approved these changes Jul 10, 2018

View reviewed changes

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jul 10, 2018

pravisankar approved these changes Jul 10, 2018

View reviewed changes

openshift-merge-robot merged commit 6a372db into openshift:master Jul 10, 2018

imcsk8 mentioned this pull request Jul 18, 2018

Add threads option documentation openshift/openshift-docs#10888

Closed

Implement nbthreads for haproxy router #19975

Implement nbthreads for haproxy router #19975

Conversation

imcsk8 commented Jun 12, 2018

imcsk8 commented Jun 12, 2018

knobunc commented Jun 12, 2018

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

pravisankar left a 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

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

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

Miciah Jul 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knobunc commented Jul 3, 2018

ironcladlou commented Jul 3, 2018

imcsk8 commented Jul 3, 2018

ironcladlou commented Jul 4, 2018

imcsk8 commented Jul 4, 2018

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 left a comment

Choose a reason for hiding this comment

knobunc commented Jul 9, 2018

knobunc commented Jul 9, 2018

juanvallejo commented Jul 9, 2018

pravisankar left a comment

Choose a reason for hiding this comment

pravisankar commented Jul 9, 2018

knobunc commented Jul 10, 2018

juanvallejo commented Jul 10, 2018

knobunc left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Jul 10, 2018

pravisankar left a comment

Choose a reason for hiding this comment

Miciah Jul 3, 2018 •

edited

Loading