Replace bitnami postgresql with integrated one#389
Conversation
deee30a to
ee07b64
Compare
taylorsilva
left a comment
There was a problem hiding this comment.
Looks good overall! Only nit-type of comments and things to change. Ran our k8s integration test suite against it, it only failed on one test related to the externalUrl. Once that change, and the other minor things, are reverted/fixed, this will be good to merge.
|
Updated with all your comments with fixups, if approve I will rebase and let you merge :) |
a3fbe19 to
a344f25
Compare
| | `postgresql.service.clusterIPs` | Hardcode services IPs | `[]` | | ||
| | `postgresql.service.extraSpec` | Add extra `spec` attributes to the postgresql service. | `{}` | | ||
| | `postgresql.image` | Set the image repository | `postgres` | | ||
| | `postgresql.imageTag` | Set the image tag, exclusive with imageDigest. | `17-postgres` | |
There was a problem hiding this comment.
Doesn't match the default in values.yaml. I think the default should be the major tag 17, not a variant tag.
| - secretName: concourse-web-tls | ||
| hosts: | ||
| - concourse.l.bahl.fr |
There was a problem hiding this comment.
Why is this the default value of tls here?
| ## Ref: https://artifacthub.io/packages/helm/bitnami/postgresql/11.9.8 | ||
| ## |
There was a problem hiding this comment.
Remove reference to bitnami chart
| extraSpec: | ||
|
|
||
| image: "postgres" | ||
| imageTag: "17-bookworm" |
There was a problem hiding this comment.
doesn't match default in README.
ce1e631 to
45a2bce
Compare
5192806 to
4950179
Compare
|
@taylorsilva I updated with your last nits I didn't saw. We did some more tests on our side and added 3 settings:
We updated the pvc name to match the one that was in the bitnami chart, so migrating via the PVC should work, I would advise to dump the database anyway to be sure. ;) |
fhacloid
left a comment
There was a problem hiding this comment.
I get an error on _helpers.tpl at line 81:
helm lint --values values.yaml
==> Linting .
[ERROR] templates/: template: concourse/templates/_helpers.tpl:102:16: executing "concourse.statefulSet.apiVersion" at <include "concourse.kubeVersion" .>: error calling include: template: concourse/templates/_helpers.tpl:81:59: executing "concourse.kubeVersion" at <"">: invalid value; expected string
Error: 1 chart(s) linted, 1 chart(s) failed
error: Recipe lint failed on line 4 with exit code 1
Does that ring a bell on your side ?
|
Oh, dumb issue, pushing a fix |
ae9862a to
2b4d061
Compare
|
We did some thorough testing for the migration, and it works. Here are the migration steps if you want to re-use the same pvc:
If you remount the same pvc, the data dir will not be detected by the entrypoint. So you must move the content of the Also, the config for both For that override the command: postgresql:
commandOverride:
- /bin/bash
- -ec
- |
if [ -d "/var/lib/postgresql/data/data" ]; then
echo "Migrating existing data directory..."
mv /var/lib/postgresql/data/data/* /var/lib/postgresql/data/ || true
rmdir /var/lib/postgresql/data/data || true
# We recreate the default config file, but you can also override
# the postgres config using the values provided in the new chart.
echo "listen_addresses = '*'" > /var/lib/postgresql/data/postgresql.conf
echo "max_connections = 100" >> /var/lib/postgresql/data/postgresql.conf
echo "shared_buffers = 128MB" >> /var/lib/postgresql/data/postgresql.conf
echo "dynamic_shared_memory_type = posix" >> /var/lib/postgresql/data/postgresql.conf
echo "max_wal_size = 1GB" >> /var/lib/postgresql/data/postgresql.conf
echo "min_wal_size = 80MB" >> /var/lib/postgresql/data/postgresql.conf
echo "log_timezone = 'Etc/UTC'" >> /var/lib/postgresql/data/postgresql.conf
echo "datestyle = 'iso, mdy'" >> /var/lib/postgresql/data/postgresql.conf
echo "timezone = 'Etc/UTC'" >> /var/lib/postgresql/data/postgresql.conf
echo "lc_messages = 'en_US.utf8'" >> /var/lib/postgresql/data/postgresql.conf
echo "lc_monetary = 'en_US.utf8'" >> /var/lib/postgresql/data/postgresql.conf
echo "lc_numeric = 'en_US.utf8'" >> /var/lib/postgresql/data/postgresql.conf
echo "lc_time = 'en_US.utf8'" >> /var/lib/postgresql/data/postgresql.conf
echo "default_text_search_config = 'pg_catalog.english'" >> /var/lib/postgresql/data/postgresql.conf
echo "local all all trust" > /var/lib/postgresql/data/pg_hba.conf
echo "host all all 127.0.0.1/32 trust" >> /var/lib/postgresql/data/pg_hba.conf
echo "host all all ::1/128 trust" >> /var/lib/postgresql/data/pg_hba.conf
echo "local replication all trust" >> /var/lib/postgresql/data/pg_hba.conf
echo "host replication all 127.0.0.1/32 trust" >> /var/lib/postgresql/data/pg_hba.conf
echo "host replication all ::1/128 trust" >> /var/lib/postgresql/data/pg_hba.conf
echo "host all all all scram-sha-256" >> /var/lib/postgresql/data/pg_hba.conf
fi
exec docker-entrypoint.sh postgres
You will need to alter the DB, you can do it by adding a lifecycle script like so: postgresql:
lifecycle:
postStart:
exec:
command:
- /bin/sh
- -ec
- |
for i in $(seq 1 30); do
pg_isready -U ${POSTGRES_USER:-{{ .Values.postgresql.auth.user }}} \
-d "dbname=${POSTGRES_DB:-{{ .Values.postgresql.auth.database }}}" \
-h 127.0.0.1 -p 5432 && \
psql -U ${POSTGRES_USER:-{{ .Values.postgresql.auth.user }}} \
-d ${POSTGRES_DB:-{{ .Values.postgresql.auth.database }}} \
-c "ALTER DATABASE atc REFRESH COLLATION VERSION;" && break
echo "Database not ready yet, retrying in 2s..."
sleep 2
done
Warning In our tests, we weren't able to find the logs of the postStart script also: This is the migration setup we tried on our side, please be very careful, back up your db and test the migration steps on a staging env before making it in production. |
|
@taylorsilva Waiting for review :) |
Signed-off-by: Frédéric BARRAS HAMPEL <frederic.hampel@cycloid.io>
ec6330d to
45f472a
Compare
Signed-off-by: Frédéric BARRAS HAMPEL <frederic.hampel@cycloid.io>
Signed-off-by: Frédéric BARRAS HAMPEL <frederic.hampel@cycloid.io>
Signed-off-by: Frédéric BARRAS HAMPEL <frederic.hampel@cycloid.io>
Signed-off-by: Frédéric BARRAS HAMPEL <frederic.hampel@cycloid.io>
Signed-off-by: Frédéric BARRAS HAMPEL <frederic.hampel@cycloid.io>
Signed-off-by: Frédéric BARRAS HAMPEL <frederic.hampel@cycloid.io>
Signed-off-by: Frédéric BARRAS HAMPEL <frederic.hampel@cycloid.io>
Signed-off-by: Frédéric BARRAS HAMPEL <frederic.hampel@cycloid.io>
Signed-off-by: Frédéric BARRAS HAMPEL <frederic.hampel@cycloid.io>
Signed-off-by: Frédéric BARRAS HAMPEL <frederic.hampel@cycloid.io>
45f472a to
8cb04e3
Compare
|
@taylorsilva still up to review, whenever you get the time ;) |
taylorsilva
left a comment
There was a problem hiding this comment.
LGTM - Running this PR against our E2E test suite and then will merge.
|
I've published v19 of the chart. Thanks for all your work here! |
|
You're welcome, feel free to ping if you need help on a related subject ! It was a pleasure to contribute ;) |
Existing Issue
Fixes #387.
Why do we need this PR?
This PR will remove the dependency to bitnami chart and replace the postgresql with a simple statefulSet with one master database.
Changes proposed in this pull request
postgresql.enabled == trueContributor Checklist
README.mdmasteris for changes related to the current release of theconcourse/concourse:latestimage and should be good to publish immediatelyReviewer Checklist
masterordev)