Add postgres init container by fosterseth · Pull Request #1799 · ansible/awx-operator

fosterseth · 2024-03-28T20:20:29Z

SUMMARY

Potentially resolves:

Postgres 15 pod: cannot create directory '/var/lib/pgsql/data/userdata': Permission denied" #1770
Upgrade from 2.12.2 to 2.13.1 failed - mkdir: cannot create directory '/var/lib/pgsql/data/userdata': Permission denied #1775

Create a postgres init container that will run user-provided init_postgres_extra_commands

for example, in the awx spec file:

  init_postgres_extra_commands: |
    sudo touch /var/lib/pgsql/data/foo
    sudo touch /var/lib/pgsql/data/bar
    chown 26:26 /var/lib/pgsql/data/foo
    chown root:root /var/lib/pgsql/data/bar

ISSUE TYPE

New or Enhanced Feature

kurokobo

Hi, thanks for working on this.

If we don't implement any default value for init_postgres_extra_commands, I think there should be a complete example that can use as it is instead of foo nor bar, in the docs, something like;

  init_postgres_extra_commands: |
    chown 26:0 /var/lib/pgsql/data
    chmod 700 /var/lib/pgsql/data

roles/installer/defaults/main.yml

fosterseth · 2024-04-01T21:14:12Z

@kurokobo thanks, added a snippet to the docs

config/crd/bases/awx.ansible.com_awxs.yaml

hhk7734 · 2024-04-02T02:04:28Z

roles/installer/templates/statefulsets/postgres.yaml.j2

+{% endif %}
+{% if init_container_extra_volume_mounts -%}
+            {{ init_container_extra_volume_mounts | indent(width=12, first=True) }}


This is not in volumes. I think this needs to be removed.

@hhk7734
I think this is intended, since any other *_extra_volume_mounts are designed to use with single extra_volumes: https://ansible.readthedocs.io/projects/awx-operator/en/latest/user-guide/advanced-configuration/custom-volume-and-volume-mount-options.html

If init_container_extra_volume_mounts is designed to use with extra_volumes, it need to add extra_volumes to volumes in this template.

Oh, sorry, it looks like postgres_extra_volumes is used for the additional PSQL volumes instead of extra_volumes.
I don't see it mentioned in the documentation. This is a different issue: https://github.com/ansible/awx-operator/pull/1799/files#diff-57afb141659730e9bae27cd3ea91bfe5b1709a2569d98759221305423d544c03L128-L131

I don't think that init_container_extra_volume_mounts block belongs here. The postgres_extra_volume_mounts parameter is sufficient.

kurokobo

Hi, I'm testing this PR and found some issues. Could you please check my comments?
Also there are some updates on devel branch so if you can please rebase your branch.

roles/installer/templates/statefulsets/postgres.yaml.j2

kurokobo · 2024-04-02T05:14:52Z

roles/installer/templates/statefulsets/postgres.yaml.j2

+              mountPath: '{{ postgres_data_path | dirname }}'
+              subPath: '{{ postgres_data_path | dirname | basename }}'


postgres_data_path is removed by #1798. Using this causing error:

TASK [Create Database if no database is specified] ******************************** fatal: [localhost]: FAILED! => {"msg": "The task includes an option with an undefined variable. The error was: Unable to look up a name or access an attribute in template string (# Postgres StatefulSet.\n...

We should use _postgres_data_path instead.

Suggested change

mountPath: '{{ postgres_data_path | dirname }}'

subPath: '{{ postgres_data_path | dirname | basename }}'

mountPath: '{{ _postgres_data_path | dirname }}'

subPath: '{{ _postgres_data_path | dirname | basename }}'

Add postgres init container if postgres_init_container_extra_commands is defined This allows users to run arbitrary commands This is aimed to solve the issue where users may need to chmod or chown the postgres data volume for user 26, which is the user that is running postgres in the sclorg image. For example, one can now set the follow on the AWX spec: spec: postgres_init_container_extra_commands: | chown 26:0 /var/lib/pgsql/data chmod 700 /var/lib/pgsql/data Signed-off-by: Seth Foster <[email protected]>

rooftopcellist · 2024-04-02T19:54:38Z

I tested this out with @kurokobo 's awx-on-k3s README.md and yaml snippets using this changes and it works now without having to run sudo chown 26:0 /data/postgres-15/data on the host directory that gets mounted.

First I stood up a k3s cluster:

curl -sfL https://get.k3s.io | INSTALL_K3S_VERSION=v1.28.7+k3s1 sh -s - --write-kubeconfig-mode 644

# Set KUBECONFIG
export KUBECONFIG=/etc/rancher/k3s/k3s.yaml

Instead of running kubectl apply -k operator, I ran the following so I would have the changes from this PR:

# Build & Push Operator Image
make docker-build docker-push IMG=quay.io/username/awx-operator:dev

# Deploy Operator
make deploy NAMESPACE=$NAMESPACE IMG=quay.io/username/awx-operator:dev

kubectl -n awx get pods

# Create SSL cert and key
AWX_HOST="awx.example.com"
openssl req -x509 -nodes -days 3650 -newkey rsa:2048 -out ./base/tls.crt -keyout ./base/tls.key -subj "/CN=${AWX_HOST}/O=${AWX_HOST}" -addext "subjectAltName = DNS:${AWX_HOST}"
cat base/awx.yaml 

# Pre-create data directory for database at /data
sudo mkdir -p /data/postgres-15/data
sudo mkdir -p /data/projects
# sudo chown 26:0 /data/postgres-15/data
sudo chown 1000:0 /data/projects
sudo chmod 700 /data/postgres-15/data

# Apply AWX custom resource
kubectl apply -k base

I observed the the migrations k8s job completed without any permissions issues. Similarly, there are no longer any permissions issues in the postgres pod.

Everything deployed successfully and I can reach the UI at awx.example.com locally.

rooftopcellist · 2024-04-02T21:56:41Z

@kurokobo @craph
@TheRealHaoLiu and I wanted to find a way to run these init commands by default, but could not figure out a way to do so that did not involve adding a lot of complexity. This is because in Openshift, we cannot easily run a container as root (uid=0) and should never do so by default. Furthermore, there may be k8s cluster/storageclass combinations that this might cause issues for.

We thought it might be easier for users to configure this as a flag instead of a set of commands. Let me know if you have feedback on the approach on #1805 versus the approach on this PR.

kurokobo · 2024-04-03T04:24:22Z

@rooftopcellist
Hi, thanks for working on this!
I vote for the flag PR #1805. It would provide a simple and better user experience.

craph · 2024-04-03T07:17:24Z

Hi @rooftopcellist,
Thank you very much for your work on this subject.
I haven't tested it on my side because I revert my production instance to version 2.12.2

May be the PR #1805 is a best solution to provide a better user experience with a good updated documentation to explain the spec.

Best regards,

rooftopcellist · 2024-04-03T16:56:46Z

@craph thanks for weighing in. I added in docs here to help make things clean for future users:

https://github.com/ansible/awx-operator/pull/1805/files#diff-e93c5e0f5a35e585b520af34a34e8a7ef808bf70cfe28d7e0fc9582142dadb41R102-R119

I will close this PR in favor of #1805

fosterseth requested a review from TheRealHaoLiu March 28, 2024 20:20

kurokobo reviewed Mar 29, 2024

View reviewed changes

roles/installer/defaults/main.yml Outdated Show resolved Hide resolved

fosterseth force-pushed the add_postgres_init_container branch 3 times, most recently from 5e44b6a to da69551 Compare April 1, 2024 21:13

fosterseth force-pushed the add_postgres_init_container branch from da69551 to de48ddc Compare April 1, 2024 21:15

hhk7734 reviewed Apr 2, 2024

View reviewed changes

kurokobo reviewed Apr 2, 2024

View reviewed changes

rooftopcellist force-pushed the add_postgres_init_container branch from de48ddc to 79497c7 Compare April 2, 2024 19:19

rooftopcellist mentioned this pull request Apr 2, 2024

Add postgres init container to resolve permissions for some k3s deployments #1805

Merged

rooftopcellist closed this Apr 3, 2024

		mountPath: '{{ postgres_data_path \| dirname }}'
		subPath: '{{ postgres_data_path \| dirname \| basename }}'

Conversation

fosterseth commented Mar 28, 2024 • edited by rooftopcellist Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

SUMMARY

ISSUE TYPE

Uh oh!

kurokobo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fosterseth commented Apr 1, 2024

Uh oh!

Uh oh!

hhk7734 Apr 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kurokobo Apr 2, 2024

Choose a reason for hiding this comment

Uh oh!

hhk7734 Apr 2, 2024

Choose a reason for hiding this comment

Uh oh!

kurokobo Apr 2, 2024

Choose a reason for hiding this comment

Uh oh!

rooftopcellist Apr 2, 2024

Choose a reason for hiding this comment

Uh oh!

kurokobo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kurokobo Apr 2, 2024

Choose a reason for hiding this comment

Uh oh!

rooftopcellist commented Apr 2, 2024

Uh oh!

rooftopcellist commented Apr 2, 2024

Uh oh!

kurokobo commented Apr 3, 2024

Uh oh!

craph commented Apr 3, 2024

Uh oh!

rooftopcellist commented Apr 3, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

fosterseth commented Mar 28, 2024 •

edited by rooftopcellist

Loading

hhk7734 Apr 2, 2024 •

edited

Loading