Images Postgres Alpine, optimization #207

andruwa13 · 2017-11-21T12:14:25Z

No description provided.

pichouk · 2017-11-21T21:26:52Z

db/Dockerfile

-RUN apt-get update \
-    && apt-get install -y \
-      build-essential \
+RUN apk add --no-cache --virtual .build-deps \
      curl \


I know I sound maniac, but can you order package list in alphabetical order please (like it was before) ? It is a good practice :)

pichouk · 2017-11-21T21:30:00Z

I just test it on my test server and my production server, it works perfect :)
But I'm not using Wal-e and I think it is important. If you can test it, that's great. If not we'll try to find out someone for this ^^

I also add @xcompass as reviewer. I like to have his wise feedback on change like this.

andruwa13 · 2017-11-21T21:46:48Z

@pichouk I also do not have the opportunity to check how Wal-e works (

pichouk · 2017-11-21T23:21:06Z

db/entrypoint.sh

@@ -20,6 +16,8 @@ function update_conf () {
        sed -i "s/wal_level =.*$/wal_level = $WAL_LEVEL/g" $PGDATA/postgresql.conf
        sed -i "s/archive_mode =.*$/archive_mode = $ARCHIVE_MODE/g" $PGDATA/postgresql.conf
        sed -i "s/archive_timeout =.*$/archive_timeout = $ARCHIVE_TIMEOUT/g" $PGDATA/postgresql.conf
+        sed -i "s/log_timezone =.*$/log_timezone = $DEFAULT_TIMEZONE/g" $PGDATA/postgresql.conf
+		sed -i "s/timezone =.*$/timezone = $DEFAULT_TIMEZONE/g" $PGDATA/postgresql.conf


Just an indentation mistake here :)

pichouk · 2017-11-21T23:26:05Z

db/entrypoint.sh

@@ -46,6 +48,9 @@ if [ "$1" = 'postgres'  ]; then

    WAL_LEVEL=archive
    ARCHIVE_MODE=on
+
+    echo "log_timezone = $DEFAULT_TIMEZONE" >> $PGDATA/postgresql.conf


I guess the $PGDATA folder should be a mounted volume, so data inside it are persistent. So it seems that everytime the container restart it will add two new lines inside the postgresql.conf file. Am I right ? If true this is not a correct behavior, but I don't know a correct workaround for this.

yes, right. this fixes errors:

invalid value for parameter "log_timezone": "localtime"
invalid value for parameter "TimeZone": "localtime"
configuration file "/var/lib/postgresql/data/postgresql.conf" contains errors

Ok, but this is not ok if our container just append lines to a persistent configuration file at each restart.
I tried to reproduce the errors you show, but it seems that it didn't happen if you mount /etc/localtime inside the container like we do on our docker-compose.yml file

Oups I just see that you can remove those two lines since you add this :

sed -i "s/log_timezone =.*$/log_timezone = $DEFAULT_TIMEZONE/g" $PGDATA/postgresql.conf sed -i "s/timezone =.*$/timezone = $DEFAULT_TIMEZONE/g" $PGDATA/postgresql.conf

On RancherOS if there is not this line there will be errors that I sent above. Delete echo "log_timezone = $DEFAULT_TIMEZONE" >> $PGDATA/postgresql.conf ?

I think the sed commands you add should fix it. Can you try without the two following echo instructions please ?

echo "log_timezone = $DEFAULT_TIMEZONE" >> $PGDATA/postgresql.conf echo "timezone = $DEFAULT_TIMEZONE" >> $PGDATA/postgresql.conf

If there is still errors, we'll have to investigate on another way to fix it.

RancherOS not work, if you delete :(

echo "log_timezone = $ DEFAULT_TIMEZONE" >> $ PGDATA / postgresql.conf
echo "timezone = $ DEFAULT_TIMEZONE" >> $ PGDATA / postgresql.conf

Log:

25.11.2017 19:25:38LOG: invalid value for parameter "log_timezone": "localtime" 25.11.2017 19:25:38LOG: invalid value for parameter "TimeZone": "localtime" 25.11.2017 19:25:38FATAL: configuration file "/var/lib/postgresql/data/postgresql.conf" contains errors```

delete ?
this

sed -i "s/log_timezone =.*$/log_timezone = $DEFAULT_TIMEZONE/g" $PGDATA/postgresql.conf
sed -i "s/timezone =.*$/timezone = $DEFAULT_TIMEZONE/g" $PGDATA/postgresql.conf

without it

echo "log_timezone = $ DEFAULT_TIMEZONE" >> $ PGDATA / postgresql.conf
echo "timezone = $ DEFAULT_TIMEZONE" >> $ PGDATA / postgresql.conf

does not work

I think the correct workaround can be:

check (using grep) if timezone = and log_timezone = are already present in the file

if yes, use sed to set the value

if not, append the configuration lines using echo >>

Or you could remove the timezone and log_timezone lines first and then append them:

sed -i "s/log_timezone =.*$//g" $PGDATA/postgresql.conf sed -i "s/timezone =.*$//g" $PGDATA/postgresql.conf echo "log_timezone = $ DEFAULT_TIMEZONE" >> $ PGDATA / postgresql.conf echo "timezone = $ DEFAULT_TIMEZONE" >> $ PGDATA / postgresql.conf

pichouk · 2017-11-25T16:17:25Z

TODO/reminder before validating this PR:

Remove lines mentioned in this discussion : Images Postgres Alpine, optimization #207 (comment)
Review from @xcompass
(Optionnal) Test Wal-e feature. Should work but if someone can do this extra test, could be interesting.

andruwa13 · 2017-11-25T17:59:49Z

test WALE_S3, not work

25.11.2017 19:59:10LOG:  archive command failed with exit code 127
25.11.2017 19:59:10DETAIL:  The failed archive command was: envdir /etc/wal-e.d/env /usr/local/bin/wal-e wal-push pg_xlog/00000001000000000000000C
25.11.2017 19:59:11sh: envdir: not found
25.11.2017 19:59:11LOG:  archive command failed with exit code 127
25.11.2017 19:59:11DETAIL:  The failed archive command was: envdir /etc/wal-e.d/env /usr/local/bin/wal-e wal-push pg_xlog/00000001000000000000000C
25.11.2017 19:59:12sh: envdir: not found
25.11.2017 19:59:12LOG:  archive command failed with exit code 127
25.11.2017 19:59:12DETAIL:  The failed archive command was: envdir /etc/wal-e.d/env /usr/local/bin/wal-e wal-push pg_xlog/00000001000000000000000C
25.11.2017 19:59:12WARNING:  archiving transaction log file "00000001000000000000000C" failed too many times, will try again later

pichouk · 2017-11-27T18:55:09Z

Hmm seems that it is an issue with envdir. Maybe it is just a missing command in Alpine.

andruwa13 · 2017-11-29T20:16:32Z

WAL-E not support Alpine :(

pichouk · 2017-11-29T21:28:22Z

Arf... What made you say that ?

xcompass · 2017-12-01T01:05:07Z

db/Dockerfile

-
+    && pip --no-cache-dir install 'wal-e<1.0.0' \
+    && rm -rf /var/cache/apk/* /tmp/* /var/tmp/* \
+    && apk del .build-deps


here you removed tzdata. Isn't it being used for timezone info for postgres later when user sets timezone? And I think the same for ca-certificates, pv, py-cffi and py-cryptography. They are not build dependencies but runtime dependencies.

fix, py-cryptography - without it does not work

xcompass · 2017-12-01T01:10:09Z

db/entrypoint.sh

@@ -46,6 +48,9 @@ if [ "$1" = 'postgres'  ]; then

    WAL_LEVEL=archive
    ARCHIVE_MODE=on
+
+    echo "log_timezone = $DEFAULT_TIMEZONE" >> $PGDATA/postgresql.conf


Or you could remove the timezone and log_timezone lines first and then append them:

sed -i "s/log_timezone =.*$//g" $PGDATA/postgresql.conf sed -i "s/timezone =.*$//g" $PGDATA/postgresql.conf echo "log_timezone = $ DEFAULT_TIMEZONE" >> $ PGDATA / postgresql.conf echo "timezone = $ DEFAULT_TIMEZONE" >> $ PGDATA / postgresql.conf

xcompass · 2017-12-01T01:13:42Z

db/Dockerfile

-    && apt-get install -y \
-      build-essential \
+RUN apk add --no-cache --virtual .build-deps \
+      bash \


I think we can remove this as we are aiming for smaller image.

👍 But then we need to use #/bin/sh for the entrypoint.sh script. Should test if it also works OK with sh.

not work
03.12.2017 1:24:49/entrypoint.sh: line 29: syntax error: unexpected "(" (expecting "fi")

The entrypoint script need some rewrite to work with sh. But I think we can achieve this.

andruwa13 · 2017-12-03T00:11:30Z

WAL-E, not work to alpine(
error:

03.12.2017 2:10:50LOG:  archive command failed with exit code 127
03.12.2017 2:10:50DETAIL:  The failed archive command was: envdir /etc/wal-e.d/env /usr/local/bin/wal-e wal-push pg_xlog/000000010000000000000002
03.12.2017 2:10:50WARNING:  archiving transaction log file "000000010000000000000002" failed too many times, will try again later

andruwa13 · 2017-12-03T00:24:10Z

@pichouk @xcompass can make a backup (WAL-E) a separate container?

pichouk · 2017-12-05T21:19:45Z

About the WAL-E support, I really don't know what to do. I don't use it and I don't really know why it didn't work on Alpine.

I would like to investigate on this, but I have no time for now. I'll see in few days/weeks if I can achieve to something with WAL-E and Alpine.

xcompass · 2017-12-07T00:43:29Z

I don't think we can use a separate container. I think postgres is actually calling WAL-E script directly. Re: WAL-E error, could you enable WAL-E logging: https://github.com/wal-e/wal-e#logging to see where it fails? Might be a configuration thing.

pichouk · 2018-01-22T16:30:36Z

Just push some changes which should fix wal-e support. But image is bigger (345MB).

andruwa13 · 2018-01-22T18:35:00Z

@pichouk not work(

pichouk · 2018-01-23T19:22:19Z

Ok I get it. In fact the postgresql configuration file ($PGDATA/postgresql.conf), which also configure Wal-e is setup by the setup-wale.sh script. This script is put on the /docker-entrypoint-initdb.d/ folder so it is executed once (and only once) at the database initialization. So my workaround works for new database but not existing ones.
I'll find something better.

pichouk · 2018-01-23T22:03:21Z

It should works now :)

andruwa13 · 2018-01-23T22:27:57Z

@pichouk Goog work! Work old DataBases and new

pichouk · 2018-01-24T22:50:56Z

Snap, just tried to deploy it on my existing server and I got this error

LOG:  invalid value for parameter "log_timezone": "localtime"
LOG:  invalid value for parameter "TimeZone": "localtime"
FATAL:  configuration file "/var/lib/postgresql/data/postgresql.conf" contains errors

Thought it was solved ?

andruwa13 · 2018-01-25T19:29:09Z

yes 7ed04e8

pichouk · 2018-01-25T19:44:45Z

Seems that you removed it on 321b162

pichouk · 2018-02-05T23:31:29Z

I squashed all commits and tested the image. Seems to works with existing and new databases :)
Looks good to me ! I'll like an approval from @xcompass too but apart from that I'm ok to merge.

IonCharge · 2018-02-13T22:33:32Z

Are you including Bash in the alpine package? Or just Sh? It seems Mattermost needs/prefers bash?
Maybe worth seeing if it fixes this issue: #234

Just add the following to the dockerfile for the app:
RUN apk add --no-cache bash and change the entrypoint to be bin\bash

@pichouk

pichouk · 2018-02-13T23:03:41Z

@ionbasa I don't understand, this PR is about PostgreSQL image, not App image. I guess you speak about #208 right ?

IonCharge · 2018-02-13T23:12:21Z

@pichouk Yes, I posted in the wrong PR, sorry.

pichouk · 2018-02-26T15:43:44Z

I guess we can merge now. Thanks a lot @andruwa13 :)

pichouk self-requested a review November 21, 2017 12:45

pichouk added Enhancement need tests in production null 2: Dev Review Requires review by a core committer labels Nov 21, 2017

pichouk self-assigned this Nov 21, 2017

andruwa13 closed this Nov 21, 2017

andruwa13 force-pushed the master branch from 9e014c9 to 48ff028 Compare November 21, 2017 16:32

andruwa13 reopened this Nov 21, 2017

pichouk mentioned this pull request Nov 21, 2017

Convert db and web containers to use alpine images #135

Closed

pichouk requested a review from xcompass November 21, 2017 21:04

pichouk reviewed Nov 21, 2017

View reviewed changes

pichouk suggested changes Nov 21, 2017

View reviewed changes

xcompass suggested changes Dec 1, 2017

View reviewed changes

pichouk added the Work In Progress Not yet ready for review label Dec 5, 2017

andruwa13 force-pushed the master branch from a9f0935 to 5370d7c Compare January 31, 2018 06:34

pichouk force-pushed the master branch 2 times, most recently from af26c88 to 6244f03 Compare February 5, 2018 23:03

Images Postgres Alpine, optimization

3e6c96c

pichouk force-pushed the master branch from 6244f03 to 3e6c96c Compare February 5, 2018 23:19

pichouk approved these changes Feb 5, 2018

View reviewed changes

pichouk removed need tests in production null Work In Progress Not yet ready for review labels Feb 26, 2018

pichouk merged commit 4e24257 into mattermost:master Feb 26, 2018

abarbare pushed a commit to abarbare/mattermost-docker that referenced this pull request Feb 26, 2018

Images Postgres Alpine, optimization (mattermost#207)

49c899b

Images Postgres Alpine, optimization #207

Images Postgres Alpine, optimization #207

Conversation

andruwa13 commented Nov 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pichouk commented Nov 21, 2017

andruwa13 commented Nov 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pichouk commented Nov 25, 2017

andruwa13 commented Nov 25, 2017 • edited Loading

pichouk commented Nov 27, 2017

andruwa13 commented Nov 29, 2017

pichouk commented Nov 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andruwa13 commented Dec 3, 2017

andruwa13 commented Dec 3, 2017 • edited Loading

pichouk commented Dec 5, 2017

xcompass commented Dec 7, 2017

pichouk commented Jan 22, 2018

andruwa13 commented Jan 22, 2018

pichouk commented Jan 23, 2018

pichouk commented Jan 23, 2018

andruwa13 commented Jan 23, 2018

pichouk commented Jan 24, 2018

andruwa13 commented Jan 25, 2018

pichouk commented Jan 25, 2018

pichouk commented Feb 5, 2018

IonCharge commented Feb 13, 2018 • edited Loading

pichouk commented Feb 13, 2018

IonCharge commented Feb 13, 2018

pichouk commented Feb 26, 2018

andruwa13 commented Nov 25, 2017 •

edited

Loading

andruwa13 commented Dec 3, 2017 •

edited

Loading

IonCharge commented Feb 13, 2018 •

edited

Loading