operator: set smp based on resource request #1416

dimitriscruz · 2021-05-18T17:59:32Z

Cover letter

Currently, the operator reads the CPU limit from the Cluster CR but the only way to affect the smp redpanda argument is through developerMode: if set to true smp is set to 1, otherwise it's empty (consuming all cores).

This change

sets the smp depending on the resources requested in the CR.
uses the requested memory instead of limit to set --memory, such that the provided amount is guaranteed on the node
continues passing the resources/limits to the StatefulSet resource configuration (cgroup resources) as provided in the Cluster CR

Developer mode : true
As before, but smp is computed based on the requested cores (instead of always being set to 1). If the CPU request is empty, smp remains empty, and Redpanda uses all cores.

Developer mode : false

Given the requested memory, we compute the maximum number of cores - we want at least 2GB per core. The smp is then set to the minimum of that and the requested number of cores.
We use request instead of limit because we have to guarantee the resources are available when providing the arguments to Redpanda.

Fixes #1378

Example

  resources:
    requests:
      cpu: 100m
      memory: 2Gi [4Gi]
...
    developerMode: false

Gives

    Args:
      redpanda
      start
      --check=false
      --advertise-rpc-addr=..
      --default-log-level=info
      --reserve-memory 0M
      --smp=1
      --memory=2147483648 [4294967296]
    Requests:
      cpu:     100m
      memory:  2Gi [4Gi]

Setting cpu to 2 (or 2000m), sets smp to 2.

Release notes

Provided resource requests in CR determine Redpanda's smp value

BenPope

This is great. Lot's of questions, though.

src/go/k8s/pkg/resources/statefulset.go

BenPope · 2021-05-18T18:20:36Z

src/go/k8s/pkg/resources/statefulset.go

+) []string {
+	var smp int64 = 1
+	if !limits.Cpu().IsZero() {
+		limits.Cpu().RoundUp(0)


I wonder if the CPU request is non-integer, or, at least, below 1, we should run overprovisioned?

We do that for developer mode. Are you thinking of doing so for both modes?

E.g., if CPU request is set to 200m in non-developerMode, then overprovisioned might help it play nicely with other processes on the machine. It's not a recommended config. I don't think this is a showstopper.

BenPope · 2021-05-18T18:22:18Z

src/go/k8s/pkg/resources/statefulset.go

@@ -477,6 +489,7 @@ func overprovisioned(developerMode bool, limits corev1.ResourceList) []string {
 		"--default-log-level=info",
 		"--reserve-memory 0M",


I thought not having reserve-memory meant TLS didn't work? Or has memory already accounted for that? It's different to developerMode.

Yeah, they have been different. I know reserve-memory is set when memory isn't, to reserve memory for OS use. For the case where memory is set I don't specify reserve-memory, but I'm not sure what happens if memory is set to the full memory capacity of the machine.

("reserve-memory", bpo::value<std::string>(), "memory reserved to OS (if --memory not specified)")

https://github.com/vectorizedio/seastar/blob/master/src/core/reactor.cc#L3431-L3432

I don't know. If non-developerMode works fine with TLS then all good. I never got to the bottom of it, but when I was testing I recall that we added --reserve-memory when rpk was taught to pass in TLS. My guess what that rpk consumes some memory before forking Redpanda. I think we tested with 1 core and 1GiB - 1GiB being the minimum per core (2GiB is recommended), and Redpanda failed to start.

As far as I remember, seastar have some hiccup in our CI where we are constraint to only 100Mb in resource.request.memory and DeveloperMode on. In such low constraint the seastar initialization failed.

src/go/k8s/pkg/resources/statefulset.go

emaxerrno · 2021-05-18T19:06:11Z

@dimitriscruz this should take into account 2GB/core and side down. So the logic should be

Do all the CPU adjustment
Figure out memory
downscale CPU if > 2GB
if < 2GB; drop to developer mode

or smth like that.

emaxerrno · 2021-05-18T19:06:51Z

src/go/k8s/pkg/resources/statefulset.go

@@ -468,7 +480,7 @@ func overprovisioned(developerMode bool, limits corev1.ResourceList) []string {
 			"--overprovisioned",
 			// sometimes a little bit of memory is consumed by other processes than seastar
 			"--reserve-memory " + redpandav1alpha1.ReserveMemoryString,
-			"--smp=1",
+			"--smp=" + strconv.FormatInt(smp, 10),


should we make sure is at least 1?

In the updated version when in developer mode, smp is either positive or not passed as an argument.

emaxerrno · 2021-05-18T19:07:22Z

src/go/k8s/pkg/resources/statefulset.go

+		requests.Cpu().RoundUp(0)
+		smp = requests.Cpu().Value()
+	}
+


smp should be at least 1 here. some assertion or normalization is needed.

further cpu tunning based on available Memory is needed too.

src/go/k8s/pkg/resources/statefulset.go

src/go/k8s/apis/redpanda/v1alpha1/cluster_webhook.go

BenPope · 2021-05-19T16:38:06Z

src/go/k8s/apis/redpanda/v1alpha1/cluster_webhook.go

+	requests := r.Spec.Resources.Requests.DeepCopy()
+	requests.Cpu().RoundUp(0)
+	requestedCores := requests.Cpu().Value()
+	if !r.Spec.Configuration.DeveloperMode && r.Spec.Resources.Requests.Memory().Value() < requestedCores*MinimumMemoryPerCore {


@senior7515 it's possible to run with 1GiB/core - should this allow 1GiB/core or require 2GiB/core?

It is possible. we can downgrade to 1G/core, but not recommended for prod. It's very little memory - around 500MB free for request flow.

I'm flexible i think we need to change the 'check' in syschecks.h

The memory check I see is for a minimum of 1GiB (https://github.com/vectorizedio/redpanda/blob/dev/src/v/syschecks/syschecks.cc#L40)

Do we want to change that to 2GiB and then require the same through the operator, or the other way round (1GiB everywhere)?

i think 2G/core is recommended. We'd need to test the alternative to have a different recommendation.

We're bumping up against what a good default is vs. allowing users to override specific things. If we had more escape hatches for allowing the user to override a specific setting, this would be less of a problem.

We have developerMode that ignores the memory/cpu ratio and now converts the cpu request to an smp value.

We could introduce minimumMemoryCPURatio: 2 (default) and if one really wants production mode with a non-recommended ratio they could adjust it.

I'm not convinced that shadowing the actual configuration params with slightly different implications is useful. Can we support this: #1388 in the operator?

I'm happy with this PR as it stands (possibly weaken the webhook to 1GiB/core), and to make more direct configuration possible in another PR.

Can we support this: #1388 in the operator?

I just sent a PR with a simpler version of 1388: #1491

RafalKorepta · 2021-06-04T11:09:05Z

src/go/k8s/pkg/resources/statefulset.go

+
+	requests.Cpu().RoundUp(0)
+	requestedCores := requests.Cpu().Value()
+	requestedMemory := requests.Memory().Value()


As this PR needs some fixes to pass CI, could you subtract from requestedMemory around 10% of the requestedMemory?

I think you're asking to subtract 10% so the --memory doesn't end up taking the whole machine. Instead, the caller can ensure the passed requested memory is 90% (or other percent they prefer) of the machine capacity.

I'm asking for add another layer of cushion. Because in the container there are more processes than Redpanda under heavy lead Redpanda will be oom killed. If we set request and limit for the stateful to 10 Gi I would like to have Redpanda memory argument to be 10% than 10Gi. I'm ok with doing this in the next PR.

If we set request and limit for the stateful to 10 Gi

With this PR we're not setting a Limit anymore, only Request and then --memory

I would like to have Redpanda memory argument to be 10% than 10Gi.

It's hard to decide on the value of "10%", why not "15%". In that case we'd need to not hardcode it and expose a parameter.

Most importantly, we already have "requests", so the user can enter a request that is 90% (or similar) of their capacity. We already do this when using the operator.

It's hard to decide on the value of "10%", why not "15%". In that case we'd need to not hardcode it and expose a parameter.

The values was calculated by me when the OOM Killed event was reported via dmesg. User should not provide this value, because this is Redpanda container problem.

Most importantly, we already have "requests", so the user can enter a request that is 90% (or similar) of their capacity. We already do this when using the operator.

The requests on the cgroup level is different knob than Redpanda --memory.

This PR passes the requested memory as the --memory

The smp is set such that each core has at least 2GB of memory. Use request instead of limit, so setting a limit is not required.

In redpanda-data#1416 some changes were made, but I don't think it was intended to remove `--memory` flag in `developerMode`. This PR changes the behaviour: * When `requests.memory` or `redpanda.memory` is set, `--memory` is passed regardless of `developerMode` * When `--memory` is set, also set `--reserve-memory=0M`. There is already a 10% buffer calculated in `requests.RedpandaMemory()` Setting both `--memory` and `--reserve-memory` is undocumented, and shouldn't be used. However, there is insufficient information to correctly calculate what `--reserve-memory` should be. By setting just `--memory`, seastar will reserve at least 1.5Gi for the OS, which doesn't make sense in a container.

In redpanda-data#1416 some changes were made, but I don't think it was intended to remove `--memory` flag in `developerMode`. This PR changes the behaviour: * When `requests.memory` or `redpanda.memory` is set, `--memory` is passed regardless of `developerMode` * When `--memory` is set, also set `--reserve-memory=0M`. There is already a 10% buffer calculated in `requests.RedpandaMemory()` Setting both `--memory` and `--reserve-memory` is undocumented, and shouldn't be used. However, there is insufficient information to correctly calculate what `--reserve-memory` should be. By setting just `--memory`, seastar will reserve at least 1.5Gi for the OS, which doesn't make sense in a container. (cherry picked from commit 25f1055)

In redpanda-data#1416 some changes were made, but I don't think it was intended to remove `--memory` flag in `developerMode`. This PR changes the behaviour: * When `requests.memory` or `redpanda.memory` is set, `--memory` is passed regardless of `developerMode` * When `--memory` is set, also set `--reserve-memory=0M`. There is already a 10% buffer calculated in `requests.RedpandaMemory()` Setting both `--memory` and `--reserve-memory` is undocumented, and shouldn't be used. However, there is insufficient information to correctly calculate what `--reserve-memory` should be. By setting just `--memory`, seastar will reserve at least 1.5Gi for the OS, which doesn't make sense in a container.

…esource-to-smp operator: set smp based on resource request

In redpanda-data#1416 some changes were made, but I don't think it was intended to remove `--memory` flag in `developerMode`. This PR changes the behaviour: * When `requests.memory` or `redpanda.memory` is set, `--memory` is passed regardless of `developerMode` * When `--memory` is set, also set `--reserve-memory=0M`. There is already a 10% buffer calculated in `requests.RedpandaMemory()` Setting both `--memory` and `--reserve-memory` is undocumented, and shouldn't be used. However, there is insufficient information to correctly calculate what `--reserve-memory` should be. By setting just `--memory`, seastar will reserve at least 1.5Gi for the OS, which doesn't make sense in a container.

…-resource-to-smp operator: set smp based on resource request

In redpanda-data#1416 some changes were made, but I don't think it was intended to remove `--memory` flag in `developerMode`. This PR changes the behaviour: * When `requests.memory` or `redpanda.memory` is set, `--memory` is passed regardless of `developerMode` * When `--memory` is set, also set `--reserve-memory=0M`. There is already a 10% buffer calculated in `requests.RedpandaMemory()` Setting both `--memory` and `--reserve-memory` is undocumented, and shouldn't be used. However, there is insufficient information to correctly calculate what `--reserve-memory` should be. By setting just `--memory`, seastar will reserve at least 1.5Gi for the OS, which doesn't make sense in a container.

github-actions bot added the area/k8s label May 18, 2021

BenPope reviewed May 18, 2021

View reviewed changes

emaxerrno reviewed May 18, 2021

View reviewed changes

dimitriscruz force-pushed the cpu-resource-to-smp branch 2 times, most recently from 47c6316 to ea1b29e Compare May 19, 2021 02:15

dimitriscruz changed the title ~~operator: set smp depending on resource request/limit~~ operator: set smp depending on resource request May 19, 2021

dimitriscruz changed the title ~~operator: set smp depending on resource request~~ operator: set smp based on resource request May 19, 2021

dimitriscruz force-pushed the cpu-resource-to-smp branch 2 times, most recently from 92e1d00 to 9de74b7 Compare May 19, 2021 03:06

emaxerrno reviewed May 19, 2021

View reviewed changes

src/go/k8s/pkg/resources/statefulset.go Show resolved Hide resolved

emaxerrno reviewed May 19, 2021

View reviewed changes

src/go/k8s/pkg/resources/statefulset.go Outdated Show resolved Hide resolved

dimitriscruz force-pushed the cpu-resource-to-smp branch 2 times, most recently from ed922cb to 8b7a65b Compare May 19, 2021 04:17

dimitriscruz marked this pull request as ready for review May 19, 2021 06:14

dimitriscruz requested a review from a team as a code owner May 19, 2021 06:14

RafalKorepta reviewed May 19, 2021

View reviewed changes

src/go/k8s/apis/redpanda/v1alpha1/cluster_webhook.go Outdated Show resolved Hide resolved

BenPope reviewed May 19, 2021

View reviewed changes

dimitriscruz force-pushed the cpu-resource-to-smp branch from 8b7a65b to 64e3711 Compare May 26, 2021 04:12

RafalKorepta reviewed Jun 4, 2021

View reviewed changes

dimitriscruz added 2 commits June 4, 2021 08:07

operator/sts: set smp depending on resource requests

b3fcb39

The smp is set such that each core has at least 2GB of memory. Use request instead of limit, so setting a limit is not required.

operator/webhook: adjust webhook to prevent insufficient or low memory

b4204f1

dimitriscruz force-pushed the cpu-resource-to-smp branch from 64e3711 to b4204f1 Compare June 4, 2021 15:09

RafalKorepta approved these changes Jun 4, 2021

View reviewed changes

dimitriscruz merged commit edf8031 into redpanda-data:dev Jun 7, 2021

dimitriscruz deleted the cpu-resource-to-smp branch June 7, 2021 19:17

joejulian pushed a commit to joejulian/redpanda that referenced this pull request Mar 10, 2023

(split) Merge pull request redpanda-data#1416 from dimitriscruz/cpu-r…

51694b4

…esource-to-smp operator: set smp based on resource request

joejulian pushed a commit to joejulian/redpanda that referenced this pull request Mar 24, 2023

operator: Merge pull request redpanda-data#1416 from dimitriscruz/cpu…

fa10baf

…-resource-to-smp operator: set smp based on resource request

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

operator: set smp based on resource request #1416

operator: set smp based on resource request #1416

dimitriscruz commented May 18, 2021 •

edited

Loading

BenPope left a comment

BenPope May 18, 2021

dimitriscruz May 19, 2021

BenPope May 19, 2021

BenPope May 18, 2021 •

edited

Loading

dimitriscruz May 19, 2021 •

edited

Loading

BenPope May 19, 2021 •

edited

Loading

RafalKorepta May 19, 2021

emaxerrno commented May 18, 2021

emaxerrno May 18, 2021

dimitriscruz May 19, 2021

emaxerrno May 18, 2021

emaxerrno May 18, 2021

BenPope May 19, 2021

emaxerrno May 19, 2021

dimitriscruz May 26, 2021

emaxerrno May 26, 2021

BenPope May 26, 2021

dimitriscruz May 27, 2021 •

edited

Loading

BenPope May 28, 2021 •

edited

Loading

dimitriscruz May 28, 2021

RafalKorepta Jun 4, 2021

dimitriscruz Jun 4, 2021

RafalKorepta Jun 4, 2021

dimitriscruz Jun 4, 2021 •

edited

Loading

RafalKorepta Jun 4, 2021

dimitriscruz Jun 4, 2021

		@@ -477,6 +489,7 @@ func overprovisioned(developerMode bool, limits corev1.ResourceList) []string {
		"--default-log-level=info",
		"--reserve-memory 0M",

operator: set smp based on resource request #1416

operator: set smp based on resource request #1416

Conversation

dimitriscruz commented May 18, 2021 • edited Loading

Cover letter

Release notes

BenPope 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

BenPope May 18, 2021 • edited Loading

Choose a reason for hiding this comment

dimitriscruz May 19, 2021 • edited Loading

Choose a reason for hiding this comment

BenPope May 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emaxerrno commented May 18, 2021

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

dimitriscruz May 27, 2021 • edited Loading

Choose a reason for hiding this comment

BenPope May 28, 2021 • edited Loading

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

dimitriscruz Jun 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimitriscruz commented May 18, 2021 •

edited

Loading

BenPope May 18, 2021 •

edited

Loading

dimitriscruz May 19, 2021 •

edited

Loading

BenPope May 19, 2021 •

edited

Loading

dimitriscruz May 27, 2021 •

edited

Loading

BenPope May 28, 2021 •

edited

Loading

dimitriscruz Jun 4, 2021 •

edited

Loading