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 users to define custom config for Redis Container #1427

Open
wants to merge 19 commits into
base: devel
Choose a base branch
from

Conversation

rakesh561
Copy link
Contributor

@rakesh561 rakesh561 commented May 22, 2023

Allow users to define custom config for Redis Container

ISSUE TYPE
  • New or Enhanced Feature
ADDITIONAL INFORMATION

This will address the following issue #1171

Allow users to define custom config for Redis Container
@@ -286,6 +286,9 @@ web_command: []
rsyslog_args:
- /usr/bin/launch_awx_rsyslog.sh
rsyslog_command: []
redis_args:
- /usr/bin/launch_awx_redis.sh
Copy link
Member

Choose a reason for hiding this comment

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

this file doesn't exist, so I think redis_args can be empty by default

@fosterseth
Copy link
Member

thanks for this PR, this sounds like the right approach

Does this allow users to drop in a redis.conf file? then the supplied redis command/args would start the redis instance from that config?

can you provide some steps as comment on the PR on what that would look like?

@rakesh561
Copy link
Contributor Author

rakesh561 commented May 24, 2023

@fosterseth I will provide some steps

@rakesh561
Copy link
Contributor Author

@fosterseth any one who wants to customize redis container can try out following steps.It would be nice if we can run these tests through github actions if possible

apiVersion: v1
kind: ConfigMap
metadata:
name: redis-config
namespace: awx
data:
redis.conf: |
daemonize yes
unixsocket /var/run/redis/redis.sock
unixsocketperm 777
port 0
bind 127.0.0.1
maxmemory 2mb
maxmemory-policy allkeys-lru


spec:
...
  extra_volumes: |
    - name: ansible-cfg
      configMap:
        defaultMode: 420
        items:
          - key: redis.conf
            path: redis.cong
        name: redis-config


  redis_extra_volume_mounts: |
    - name: redis.cfg
      mountPath: /etc/redis.conf
      subPath: redis.conf

  redis_command:
   chmod 755 /etc/redis.conf
   redis-cli start 

 redis_extra_env:
      - name: foo
         value: bar 

@rakesh561
Copy link
Contributor Author

@fosterseth let me know what you think of above inputs or commands.

@rakesh561
Copy link
Contributor Author

@fosterseth @rooftopcellist just checking to see if you can take a look at it

@djyasin
Copy link
Member

djyasin commented Jun 28, 2023

@rakesh561 Thank you so much for your contribution! It looks like we have some merge conflicts. Would you mind rebasing this PR for us?

@rakesh561
Copy link
Contributor Author

@djyasin i have done the rebasing.Please let me know how it looks from your end.

@@ -382,6 +387,8 @@ spec:
- name: awx-devel
mountPath: "/awx_devel"
{% endif %}
{% if redis_extra_volume_mounts -%}
{{ redis_extra_volume_mounts | indent(width=12, first=True) }}
{% if termination_grace_period_seconds is defined %}
Copy link
Member

Choose a reason for hiding this comment

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

It looks like the redis_extra_env is missing here:

Suggested change
{% if termination_grace_period_seconds is defined %}
{% if redis_extra_env -%}
{{ redis_extra_env | indent(width=12, first=True) }}
{% endif %}
{% if termination_grace_period_seconds is defined %}

Copy link
Member

Choose a reason for hiding this comment

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

merge conflict needs to be resolved here

Copy link
Member

Choose a reason for hiding this comment

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

@rakesh561 This is still needed

command: {{ redis_command }}
{% endif %}
{% if redis_args %}
args: {{ redis_args }}
Copy link
Member

Choose a reason for hiding this comment

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

This line already exists:

You'll probably want to set the default to

["redis-server", "/etc/redis.conf"]

I think that should be the value for redis_command, not redis_args, but the only way to know for sure will be to deploy it and try it out once those changes are made.

command: {{ redis_command }}
{% endif %}
{% if redis_args %}
args: {{ redis_args }}
Copy link
Member

Choose a reason for hiding this comment

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

Same here, this already exists, currently, your templaated redis_args wouldn't be used at all, the entry further down on line 140 would be used instead (here).

@rakesh561
Copy link
Contributor Author

@rooftopcellist Updated README with example and also applied suggested changes

README.md Outdated Show resolved Hide resolved
Co-authored-by: Christian Adams <rooftopcellist@gmail.com>
@@ -1566,6 +1574,9 @@ spec:
web_extra_volume_mounts:
description: Specify volume mounts to be added to the Web container
type: string
redis_extra_volume_mounts:
description: Specify volume mounts to be added to the Redis container
type: string
redis_image:
description: Registry path to the redis container to use
Copy link
Member

Choose a reason for hiding this comment

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

It looks like redis_extra_env is missing here.

Copy link
Member

Choose a reason for hiding this comment

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

@rakesh561 This is still needed.

Co-authored-by: Christian Adams <rooftopcellist@gmail.com>
Comment on lines +392 to +395
<<<<<<< HEAD
=======
{% endif %}
>>>>>>> f32bd4e (Updated requested changes)
Copy link
Member

Choose a reason for hiding this comment

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

can you address the merge conflict

@TheRealHaoLiu
Copy link
Member

https://github.com/ansible/awx-operator/pull/1695/files
try this out and let me know if this implementation allow you to ovverride the things you want

@Denney-tech
Copy link

Denney-tech commented Jan 29, 2024

@TheRealHaoLiu and @kurokobo , neither of your PR's #1695 or #1697 appear to address modifying the redis.conf file that is mounted to the Redis container. Not everything can be defined as an environment variable, so being able to customize the container specs doesn't necessarily help here.

What we need is a way to modify or replace the settings in redis.conf in the configmap created by the operator, perhaps by adding something like redis_extra_settings: to the awx CRD. Then it could do a dynaconf merge similar to how extra_settings: works.

@kurokobo
Copy link
Contributor

@Denney-tech
Yes, you are correct. My and Hao's PRs are focused on pod spec customization, not configmap customization.
As you mentiond changing conf file requires a different way (not sure whether to continue that in this PR or create it as a separate PR).

@Denney-tech
Copy link

Whatever you brilliant devs think would work the best. :)

Just wanted to reply to Hao's comment that his PR, and in turn yours, didn't satisfy the objective of this PR.

@TheRealHaoLiu
Copy link
Member

@Denney-tech thanks for checking our our PR and provide valuable feedback

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants