Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

redpanda: simplify setting resources #1622

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

chrisseto
Copy link
Contributor

1fff60a redpanda: refactor resource flag computations

This commit has no user facing changes. It refactors the various functions that
affect and define the redpanda container's resources and the resource related
CLI flags (--smp, --memory, --reserve-memory, and --overprovisioned)
into methods on the RedpandaResource struct.

9a266e5 redpanda: simplify setting redpanda resources

Prior to this commit it was not possible to "mix and match" the resource
requests and limits of the redpanda container. The redpanda chart also required
users to grok a new model of resources unique to the chart which made the chart
less user friendly.

This commit introduces a more user friendly and backwards compatible way of
directly controlling the requests and limits of the redpanda container. Rather
than directly exposing the --smp, --memory, --reserve-memory, and
--overprovisioned flags to the end user, this method will infer them from the
provided values with updated best practices.

Fixes #1494
K8S-325
K8S-434

@chrisseto chrisseto force-pushed the chris/p/redpanda-direct-resources branch from 9a266e5 to 98e5b7b Compare December 16, 2024 21:26
@chrisseto chrisseto changed the title Chris/p/redpanda direct resources redpanda: simplify setting resources Dec 16, 2024
@chrisseto chrisseto force-pushed the chris/p/redpanda-direct-resources branch from 98e5b7b to 47529d0 Compare December 16, 2024 21:48
Comment on lines 557 to 560
// The amount of memory to allocate to a container is determined by the sum of three values:
// 1. Redpanda (at least 2Gi per core, ~80% of the container's total memory)
// 2. Seastar subsystem (200Mi * 0.2% of the container's total memory, 200Mi < x < 1Gi)
// 3. Other container processes (whatever small amount remains)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be updated now, given we are now reserving 90% of the container limit for Redpanda.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated ever so slightly. The legacy mode still uses the 80% calculation so that part is left as is.

@chrisseto chrisseto force-pushed the chris/p/redpanda-direct-resources branch 2 times, most recently from 04b5615 to 3bbf085 Compare December 17, 2024 19:37
Base automatically changed from chris/p/use-operator-gotohelm to main December 18, 2024 15:57
An error occurred while trying to automatically change base from chris/p/use-operator-gotohelm to main December 18, 2024 15:57
This commit has no user facing changes. It refactors the various functions that
affect and define the redpanda container's resources and the resource related
CLI flags (`--smp`, `--memory`, `--reserve-memory`, and `--overprovisioned`)
into methods on the `RedpandaResource` struct.
@chrisseto
Copy link
Contributor Author

So all tests are failing due to rpk barking about --lock-memory already being set. It seems like there may be a bug floating around in RPK. I'll probably fallback to enable_memory_locking instead of specifying it as a flag.

@chrisseto chrisseto force-pushed the chris/p/redpanda-direct-resources branch from 3bbf085 to 3867f3a Compare December 18, 2024 22:19
@chrisseto
Copy link
Contributor Author

Ugh. So found the roadblock here. rpk doesn't permit --lock-memory or --overprovisioned to be specified via additional_start_flags due to a bug.

We'll need to implement some argument parsing to be able to filter both values out and insert them into the correct dedicated spots before this can get merged.

@travisdowns
Copy link
Member

travisdowns commented Jan 7, 2025

Ugh. So found the roadblock here. rpk doesn't permit --lock-memory or --overprovisioned to be specified via additional_start_flags due to a bug.

Is it the one where passing a command line arg without a parameter, like --foo will end up being passed as --foo=true (i.e., =true appended, which is invalid)?

There was only like that in the past but fixed: redpanda-data/redpanda#4778

@andrewstucki
Copy link
Contributor

gross workaround/fix, but if what @travisdowns is talking about is the bug you're running into @chrisseto, seems like adding --lock-memory to https://github.com/redpanda-data/redpanda/blob/dev/src/go/rpk/pkg/redpanda/launcher.go#L79 might do the trick?

@chrisseto
Copy link
Contributor Author

None of the above from what I can tell. Try this:

docker run -it --entrypoint bash docker.redpanda.com/redpandadata/redpanda:v23.3.1
redpanda@bf5a0eb9a68e:/$ rpk start # Create the default /etc/redpanda/redpanda.yaml
redpanda@bf5a0eb9a68e:/$  ^-C # stop redpanda
redpanda@bf5a0eb9a68e:/$ vi /etc/redpanda/redpanda.yaml # Add additional_start_flags: [--lock-memory] to the rpk stanza

redpanda@bf5a0eb9a68e:/$ rpk start
Command "start" is deprecated, use "rpk redpanda start" instead
WARNING: This is a setup for development purposes only; in this mode your clusters may run unrealistically fast and data can be corrupted any time your computer shuts down uncleanly.
Error: configuration conflict. Flag '--lock-memory' is also present in 'rpk.additional_start_flags' in configuration file '/etc/redpanda/redpanda.yaml'. Please remove it and pass '--lock-memory' directly to `rpk start`
Usage:
  rpk start [flags]

Flags:
      --advertise-kafka-addr strings        A comma-separated list of Kafka addresses to advertise (scheme://host:port|name)
      --advertise-pandaproxy-addr strings   A comma-separated list of Pandaproxy addresses to advertise (scheme://host:port|name)
      --advertise-rpc-addr string           The advertised RPC address (host:port)
      --check                               When set to false will disable system checking before starting redpanda (default true)
  -h, --help                                Help for start
      --install-dir string                  Directory where redpanda has been installed
      --kafka-addr strings                  A comma-separated list of Kafka listener addresses to bind to (scheme://host:port|name)
      --mode string                         Mode sets well-known configuration properties for development or test environments; use --mode help for more info
      --pandaproxy-addr strings             A comma-separated list of Pandaproxy listener addresses to bind to (scheme://host:port|name)
      --rpc-addr string                     The RPC address to bind to (host:port)
      --schema-registry-addr strings        A comma-separated list of Schema Registry listener addresses to bind to (scheme://host:port|name)
  -s, --seeds strings                       A comma-separated list of seed nodes to connect to (scheme://host:port|name)
      --timeout duration                    The maximum time to wait for the checks and tune processes to complete (e.g. 300ms, 1.5s, 2h45m) (default 10s)
      --tune                                When present will enable tuning before starting redpanda
      --well-known-io string                The cloud vendor and VM type, in the format <vendor>:<vm type>:<storage type>

Global Flags:
      --config string            Redpanda or rpk config file; default search paths are ~/.config/rpk/rpk.yaml, $PWD, and /etc/redpanda/redpanda.yaml
  -X, --config-opt stringArray   Override rpk configuration settings; '-X help' for detail or '-X list' for terser detail
      --profile string           rpk profile to use
  -v, --verbose                  Enable verbose logging

My guess is that it's due to RPK's config typing enable_memory_locking as bool instead of *bool. I'll file an issue once this PR is finished up. We'll have to work around it because every running instance is affected so there's no much benefit to fixing it right now.

Prior to this commit it was not possible to "mix and match" the resource
requests and limits of the redpanda container. The redpanda chart also required
users to grok a new model of resources unique to the chart which made the chart
less user friendly.

This commit introduces a more user friendly and backwards compatible way of
directly controlling the requests and limits of the redpanda container. Rather
than directly exposing the `--smp`, `--memory`, `--reserve-memory`, and
`--overprovisioned` flags to the end user, this method will infer them from the
provided values with updated best practices.

Fixes #1494
K8S-325
K8S-434
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CPU requests aren't set on redpanda statefulset unless min memory is specified.
5 participants