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

[chart/redis-ha][REQUEST] - Extend redis.config and sentinel.config #86

Closed
SarjakSShah opened this issue Dec 3, 2020 · 5 comments
Closed
Labels
enhancement New feature or request

Comments

@SarjakSShah
Copy link
Contributor

Is your feature request related to a problem? Please describe.
Right now we cannot configure multiple configuration lines through redis.config. I know redis.customConfig can be used to solve this problem but it will be nice to have redis.config take care of this as well. Furtermore, sentinel.config values adds "sentinel {{key}} {{value}}" for all values, it will be nice to support other config values which doesn't need to start with "sentinel" keyword in sentinel.conf file

Describe the solution you'd like
We can have a key called "redis.config.multipleLineConfig" where multipleLineConfig is just a blob of data like:

redis:
  config:
    multipleLineConfig: |
        save 100 60
        save 200 30

In _configs.tpl we can check for this key to just copy this whole blob of data to redis.conf

For sentinel:
We can have "sentinel.config.otherConfig" where otherConfig can have values which doesn't need to be pre appended by "sentinel {{key}} ..."

Describe alternatives you've considered
Alternative is to use redis.customConfig and sentinel.customConfig

@SarjakSShah SarjakSShah added the enhancement New feature or request label Dec 3, 2020
SarjakSShah added a commit to SarjakSShah/charts that referenced this issue Dec 10, 2020
Signed-off-by: Sarjak Shah <sshah@afilias.info>
@DandyDeveloper
Copy link
Owner

DandyDeveloper commented Jan 4, 2021

I've added some comments in your PR to confirm some things. Get back to me when you can!

@fveerden
Copy link

When will this be merged, as it is now impossible to add more than one SAVE instruction in the redis.config

@DandyDeveloper
Copy link
Owner

@fveerden Sorry, I completely missed @SarjakSShah response to my original suggestions.

Right now, I'm torn between what options we offer here without bringing more unnecessary complexity into the chart.

I'd like to give users the option of:

  1. Use the default configmap
  2. Provide your own existing configmap to replace the default

@SarjakSShah is effectively suggesting we instead add an append option, which I'm not against, but I'm worried will bring me more work to resolve in the future when people expect existing config options to be replaced / duplicates start appearing and I have to offer option 2 anyway to resolve those kinds of issues.

@fveerden
Copy link

fveerden commented Mar 22, 2021

Well, the nice thing about redis.config.* settings is that you can easily override a setting in a secondary values.yaml. We now use it to override the maxmemory setting for the production environment. Like this:

redis:
  config:
    maxmemory: "12gb"

The drawback of redis.customConfig seems that is overrules the redis.config.* settings and you have to copy all config if you want to override just one.
Or is there another way to have a default redis config and override the maxmemory if necessary?

@DandyDeveloper
Copy link
Owner

Closing for housekeeping as there's not a lot of movement on this right now. I'm happy to discuss this further if the conversation still needs to happen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants