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

Allow the user to disable vm.overcommit_memory #254

Closed
wants to merge 1 commit into from

Conversation

Migsi
Copy link

@Migsi Migsi commented Oct 29, 2020

This feature is useful, when the user wants to install redis inside a container, where this particular systemctl cannot be set, due to beeing read only. Simply not defining the config key redis_inside_container by default, nothing should break and backward compatibility should be maintained. Also this way it should never happend to be toggled by accident. Description is to be added in README.md

This feature is useful, when the user wants to install redis inside a container, where this particular systemctl cannot be set, due to beeing read only. Simply not defining the config key `redis_inside_container` by default, nothing should break and backward compatibility should be maintained. Also this way it should never happend to be toggled by accident. Description is to be added in README.md
Migsi added a commit to Migsi/ansible-redis that referenced this pull request Oct 29, 2020
This change refers to [this](DavidWittman#254) PR and should be merged together.
rndmh3ro added a commit to telekom-mms/ansible-redis that referenced this pull request Nov 27, 2020
Update README.md to match proposed changes in DavidWittman#254
rndmh3ro pushed a commit to telekom-mms/ansible-redis that referenced this pull request Nov 27, 2020
This change refers to [this](DavidWittman#254) PR and should be merged together.
@825i
Copy link

825i commented Apr 27, 2021

Doesn't the var redis_inside_container need to also be added to ansible-redis/blob/master/defaults/main.yml?

@Migsi
Copy link
Author

Migsi commented May 18, 2021

Doesn't the var redis_inside_container need to also be added to ansible-redis/blob/master/defaults/main.yml?

The idea behind this change was to keep maximum compatibility with existing playbooks unsing this role as-is. Due to the variable not beeing defined at all in default cases, vm.overcommit_memory will be enabled as usual.

Allthough I agree that it might be way cleaner to add the variable to the defaults, simply set to false. I'd have proposed that change already, but I'm facing some issues with it. As soon as I add it to the defaults, my (otherwise unchanged) playbook stops running through due to a confusing error message, claiming the variable is not a member of the given dict. As the role should have complete access to all its variables, it is unclear to me if this is actually caused by a bug in ansible or if I'm completely misunderstanding variable scopes in this role/roles in general.

tl;dr: Your request is valid, but issues arise when realized that way. Help appreciated.

EDIT: I'd consider reverting this change and applying #259 instead, which should autodetect unpriviledged containers and set some required settings accordingly.

@Migsi Migsi closed this Jun 24, 2022
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.

2 participants