Source to image #208

dhodovsk · 2017-11-16T11:29:00Z

No description provided.

pkubatrh · 2017-11-21T08:13:06Z

latest/root/usr/bin/run-postgresql

@@ -22,12 +22,17 @@ else
  try_pgupgrade
 fi

+process_extending_files ${APP_DATA}/src/pre-init ${CONTAINER_SCRIPTS_PATH}/pre-init


This call should also be added to run-postgresql-slave

pkubatrh · 2017-11-21T08:19:18Z

latest/root/usr/share/container-scripts/postgresql/README.md

+
+##### `postgresql-config/`
+
+when running `run-service` command contained


I would restate a bit differently since right now it seems a bit confusing. Something along the lines of:

The postgresql.conf configuration file contained in this folder is used instead of the default configuration when postgresql is started

This sounds bad to me. Shouldn't we again rather only "include" (additional?) configuration file into the default postgresql.conf?

pkubatrh · 2017-11-21T08:20:29Z

@praiskup Please take a look as well

pkubatrh · 2017-11-21T08:20:47Z

I wonder... Would this work for the Fedora version out of the box as well?

praiskup · 2017-11-21T13:49:02Z

latest/root/usr/share/container-scripts/postgresql/README.md

+
+
+```
+$ s2i build --assemble-user root ~/image-configuration/ postgresql new-postgresql


Is --asemble-user root realistically useable in openshift? Shouldn't we rather avoid root user at all?

User root is in this solution is used only for building the image using s2i. When the built container is run, it is under postgre user who is only in group postgre.

So is this really commonly enabled feature in OpenShift, so we can make it a "general" requirement? It seems to be pretty dangerous to get root access, even during the build..

Other possibility is to add postgres user to root group (as is done e.g in mysql https://github.com/sclorg/mysql-container/blob/master/root-common/usr/libexec/container-setup#L58) but I can't help, it doesn't feel right. I think this issue deserves more discussion. @praiskup @hhorak what do you think?

To better understand, what exactly is the 'root' needed for?

Actually, that is really good question! The problem with missing write permission (that caused failed s2i builds) on /opt/app-root/src is now managed in Dockerfiles with https://github.com/sclorg/postgresql-container/pull/208/files#diff-4040b6e79b7069db62e62fee9e5c3532R72. fix-permissions might fail, but the build finishes successfully. Thank you for making this comment and standing your ground! :)

praiskup · 2017-11-21T14:04:00Z

examples/extending-image/postgresql-config/postgresql.conf

+# -----------------------------
+# PostgreSQL configuration file
+# -----------------------------
+#


This example configuration file should be few-lines long only (this file is actually generated at postgresql build-time, and as such it would get staled very soon) and do something real.. and be included into the maint postgresql.conf file, or possibly transitively included through the configuration generated by our scripts...

dhodovsk · 2017-11-24T08:54:45Z

@pkubatrh yes, it should also work with fedora, but I ran into a problem with python2 package:

Steps to reproduce:

$ docker run -it registry.fedoraproject.org/f25/s2i-base:latest bash
bash-4.3# dnf install -y python2
Fedora 25 - x86_64                                                        80 MB/s |  50 MB     00:00    
Fedora 25 - x86_64 - Updates                                              73 MB/s |  24 MB     00:00    
Last metadata expiration check: 0:00:06 ago on Fri Nov 24 08:51:08 2017.
Package python-2.7.13-2.fc25.x86_64 is already installed, skipping.
Dependencies resolved.
Nothing to do.
Complete!
bash-4.3# rpm -V python2
package python2 is not installed

Do you know something about this? Could you, please, help me resolve this so I can rebase and apply s2i also to fedora image?

pkubatrh · 2017-11-24T09:10:19Z

@dhodovsk What exactly is the issue? Python 2.7 is installed, only the package is not named python2, just python (old style naming).

In any case the postgresql image should not be based on s2i-base at all. It is an oversight that is already fixed in master. It should be based on s2i-core, which is unfortunately not yet built. (sclorg/s2i-base-container#144)

dhodovsk · 2017-11-24T09:36:02Z

@pkubatrh the issue occurred while verifying INSTALED_PKGS - https://github.com/sclorg/postgresql-container/blob/master/latest/Dockerfile.fedora#L51. It works with registry.fedoraproject.org/fedora:26 but not with registry.fedoraproject.org/f25/s2i-base:latest bash. I don't know the python background in fedora but it seemed interesting. Anyway I built fedora s2i-core locally and used it in fedora image with s2i here and it went ok.

praiskup · 2017-11-28T08:22:44Z

latest/s2i/bin/run

@@ -0,0 +1,3 @@
+#!/bin/bash
+
+/usr/bin/run-postgresql


Can this be symlink to /usr/bin/run-postgresql?

dhodovsk · 2017-11-29T16:41:12Z

@praiskup please review discussed changes, there is another example of handling permissions for ${APP_DATA} here: https://github.com/sclorg/varnish-container/blob/master/4/Dockerfile#L46

praiskup · 2017-11-30T06:26:07Z

examples/extending-image/start-hook/set_passwords.sh

+#!/bin/bash
+
+# postgresql image encrypts user passwords at service start
+# the functionality can be disabled by providing this file (start-hook/set_passwords.sh) in s2i build


Missing newline.

praiskup · 2017-11-30T06:30:49Z

latest/root/usr/bin/run-postgresql

 pg_ctl -w start -o "-h ''"
 if $PG_INITIALIZED ; then
+    process_extending_files ${APP_DATA}/src/init-hook ${CONTAINER_SCRIPTS_PATH}/init-hook


Could we name it init only, or init-hooks?

praiskup · 2017-11-30T06:37:34Z

latest/Dockerfile

@@ -67,6 +69,8 @@ ENV BASH_ENV=${CONTAINER_SCRIPTS_PATH}/scl_enable \

 VOLUME ["/var/lib/pgsql/data"]

+# {APP_DATA} needs to be accessed by postgres user while s2i assembling
+RUN chown -R 26:0 ${APP_DATA}


chmod -R og+rwx should be probably used too

I meant chmod -R ug+rwx.

praiskup · 2017-11-30T06:38:30Z

latest/Dockerfile.rhel7

@@ -1,4 +1,4 @@
-FROM rhel7
+FROM rhscl/s2i-core-rhel7:1


typo :1 suffix

praiskup · 2017-11-30T06:47:33Z

latest/root/usr/bin/run-postgresql-slave

 wait_for_postgresql_master
 export MASTER_FQDN=$(postgresql_master_addr)
 initialize_replica

+process_extending_files ${APP_DATA}/src/init ${CONTAINER_SCRIPTS_PATH}/init


Should slave call these hooks?

I asked for it to be added because I thought even slaves want to have their configuration files extendible. But looking at pg_basebackup documentation it seems that the slaves will get the master's configurations files along with everything else, so it might not be needed after all.

More it seems to me that the password change method is newly called for slave, too.

praiskup · 2017-11-30T06:55:40Z

latest/root/usr/share/container-scripts/postgresql/start-hook/set_passwords.sh

+
+if [ -v POSTGRESQL_ADMIN_PASSWORD ]; then
+psql --command "ALTER USER \"postgres\" WITH ENCRYPTED PASSWORD '${POSTGRESQL_ADMIN_PASSWORD}';"
+fi


Probably editor mis-config (missing newline).

praiskup · 2017-11-30T06:56:54Z

latest/root/usr/share/container-scripts/postgresql/init-hook/set-config.sh

+
+extending_cfg_dir="${APP_DATA}/src/postgresql-config"
+
+for conf in $(ls ${extending_cfg_dir}); do


for conf in "$extending_cfg_dir"/*.conf; do

praiskup · 2017-11-30T06:57:45Z

latest/root/usr/share/container-scripts/postgresql/init-hook/set-config.sh

+for conf in $(ls ${extending_cfg_dir}); do
+    cat >> "$PGDATA/postgresql.conf" <<EOF
+include '$extending_cfg_dir/${conf}'
+EOF


Nit, but echo would be also OK and a bit more readable.

praiskup · 2017-11-30T07:02:40Z

Thanks, it is close now :-) could we please add s2i test-cases? Mariadb can be used for inspiration.

omron93 · 2017-12-01T09:16:51Z

latest/root/usr/bin/run-postgresql-slave

@@ -28,9 +28,13 @@ check_env_vars
 generate_passwd_file
 generate_postgresql_config

+process_extending_files ${APP_DATA}/src/pre-init ${CONTAINER_SCRIPTS_PATH}/pre-init


This file needs to be updated to use init-hook and start-hook directories.

dhodovsk · 2017-12-06T09:06:47Z

The requested changes has been applied; now also to Dockerfile.template and versions 9.5 and 9.4.

pkubatrh · 2017-12-12T11:53:14Z

do we need write permissions actually?

For the source files under /opt/app-root/src? One use case that comes to mind is easier debugging of the built image without the need to have it rebuilt with new sources

omron93 · 2017-12-12T13:08:14Z

do we really need to run fix-permission during assemble run?

We need to add/ensure g+r for root group. To be able to run this image under random UID (~group is 0)... So I think yes.

dhodovsk · 2017-12-13T08:32:51Z

So based on comments above, fix-permission wasn't changed in latest rebase and is called during assemble. Postgres user is added to root group in Dockerfile to be able to perform the operations.

praiskup · 2017-12-13T08:59:23Z

9.4/Dockerfile.rhel7

+# {APP_DATA} needs to be accessed by postgres user while s2i assembling
+# postgres user changes permissiond of files in APP_DATA during assemblng
+RUN chown -R 26:0 ${APP_DATA} && \
+    useradd -G root 26


This doesn't work on my system. You probably want usermod -a -G root postgres.
I don't think removal of chmod was correct, because default umask should be 0022 -> which means that group get's only r-x permissions.

pkubatrh · 2017-12-14T08:21:56Z

[test-openshift]

praiskup · 2017-12-14T08:43:24Z

9.4/root/usr/share/container-scripts/postgresql/README.md

+
+contained shell scripts (`*.sh`) are sourced once, when database is initialized
+
+##### `start-hook/`


Still no comment about s/start-hook/postgresql-start-hook/. Mariadb has mysql-cfg, mysql-init and mysql-pre-init. Shouldn't we have the namespaced hook directory, too?

praiskup · 2017-12-14T08:55:21Z

9.5/Dockerfile.rhel7

@@ -70,6 +72,12 @@ ENV BASH_ENV=${CONTAINER_SCRIPTS_PATH}/scl_enable \

 VOLUME ["/var/lib/pgsql/data"]

+# {APP_DATA} needs to be accessed by postgres user while s2i assembling
+# postgres user changes permissiond of files in APP_DATA during assemblng


What about:
Being also in supplemental group 0, postgres user can by default change the group
ownership of files in $APP_DATA to postgres:root (by fix-permissions script in assemble
script). This group ownership change is needed because the s2i-built image is run under
arbitrary user uid (but gid=0).

praiskup · 2017-12-14T08:56:01Z

9.5/Dockerfile.rhel7

+# postgres user changes permissiond of files in APP_DATA during assemblng
+RUN chown -R postgres:0 ${APP_DATA} && \
+    chmod -R ug+rwx ${APP_DATA} && \
+    usermod -a -G root postgres


I still have the two questions:

is the write permissions needed in case of postgresql?

could we just drop chown/chmod and use fix-permissions above (where the data ownership is changed)?

I think fix-permissions should be enough, at least it looks like it's enough for mysql: https://github.com/sclorg/mysql-container/blob/master/root-common/usr/libexec/container-setup#L57

hhorak · 2017-12-18T09:57:19Z

9.5/Dockerfile.rhel7

@@ -70,6 +72,12 @@ ENV BASH_ENV=${CONTAINER_SCRIPTS_PATH}/scl_enable \

 VOLUME ["/var/lib/pgsql/data"]

+# {APP_DATA} needs to be accessed by postgres user while s2i assembling
+# postgres user changes permissiond of files in APP_DATA during assemblng


two typos: "permissiond" and "assemblng"

hhorak · 2018-01-04T10:47:59Z

@dhodovsk @praiskup It's hard to see whether there is still anything to be fixed. From a quick look, it seems things like 's/init/postgresql-init/ands/start-hook/postgresql-start-hook/` are still to be addressed...

praiskup · 2018-01-04T11:01:19Z

I would prefer to have that hooks namespaced, yes.

dhodovsk · 2018-01-09T11:39:55Z

@praiskup Is it now ok?

praiskup · 2018-01-09T14:20:37Z

Seems to be, small nit with the docs.

praiskup · 2018-01-09T14:22:21Z

latest/root/usr/share/container-scripts/postgresql/README.md

+
+##### `postgresql-config/`
+
+contained files will be included at the end of image postgresql.conf file during database initialization


Meh, github fights against me. This should be either dropped, or at least you should mention that only *.conf files are sourced.

praiskup · 2018-01-09T16:42:02Z

[test-openshift]

praiskup · 2018-01-10T09:00:22Z

lgtm

pkubatrh · 2018-01-10T11:17:55Z

Merging, thanks!

dhodovsk force-pushed the source-to-image branch from f57438e to 1b2f764 Compare November 20, 2017 14:03

pkubatrh reviewed Nov 21, 2017

View reviewed changes

praiskup reviewed Nov 21, 2017

View reviewed changes

dhodovsk force-pushed the source-to-image branch 4 times, most recently from bd05c52 to f58b86e Compare November 24, 2017 15:15

praiskup reviewed Nov 28, 2017

View reviewed changes

praiskup reviewed Nov 30, 2017

View reviewed changes

latest/Dockerfile.rhel7 Outdated

@@ -1,4 +1,4 @@

FROM rhel7

FROM rhscl/s2i-core-rhel7:1

Copy link

Contributor

praiskup Nov 30, 2017

Choose a reason for hiding this comment

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

typo :1 suffix

praiskup reviewed Nov 30, 2017

View reviewed changes

pkubatrh mentioned this pull request Nov 30, 2017

Allow remote connections for pg_basebackup #157

Closed

omron93 reviewed Dec 1, 2017

View reviewed changes

dhodovsk force-pushed the source-to-image branch from 23189b7 to 55617a6 Compare December 4, 2017 15:19

praiskup mentioned this pull request Dec 7, 2017

NFS mount for data directory of postgresql 9.5 container #213

Open

dhodovsk force-pushed the source-to-image branch from ec63a26 to f230590 Compare December 13, 2017 07:44

praiskup reviewed Dec 13, 2017

View reviewed changes

dhodovsk force-pushed the source-to-image branch from f230590 to 381227a Compare December 13, 2017 09:53

praiskup reviewed Dec 14, 2017

View reviewed changes

hhorak mentioned this pull request Dec 16, 2017

Slightly changed S2i extensible from kosciCZ sclorg/nginx-container#41

Merged

hhorak reviewed Dec 18, 2017

View reviewed changes

dhodovsk force-pushed the source-to-image branch from 381227a to 1c199b2 Compare January 2, 2018 09:03

This was referenced Jan 4, 2018

faster s2i container start idea #220

Closed

Distgen - support for image generation sclorg/container-common-scripts#38

Merged

dhodovsk force-pushed the source-to-image branch from 1c199b2 to 8aa193c Compare January 8, 2018 13:07

praiskup reviewed Jan 9, 2018

View reviewed changes

Enable s2i

0c9da02

dhodovsk force-pushed the source-to-image branch from 8aa193c to 0c9da02 Compare January 9, 2018 14:59

pkubatrh merged commit 6443165 into sclorg:master Jan 10, 2018

This was referenced Feb 1, 2018

Add support for extending image and enabling SSL connections sclorg/s2i-perl-container#133

Open

Add support for extending image sclorg/redis-container#33

Closed

Add support for extending image and enabling SSL connections sclorg/varnish-container#34

Open


		##### `postgresql-config/`

		when running `run-service` command contained



		```
		$ s2i build --assemble-user root ~/image-configuration/ postgresql new-postgresql


		extending_cfg_dir="${APP_DATA}/src/postgresql-config"

		for conf in $(ls ${extending_cfg_dir}); do


		contained shell scripts (`*.sh`) are sourced once, when database is initialized

		##### `start-hook/`


		##### `postgresql-config/`

		contained files will be included at the end of image postgresql.conf file during database initialization

		@@ -0,0 +1,3 @@
		#!/bin/bash

		/usr/bin/run-postgresql

Source to image #208

Source to image #208

Conversation

dhodovsk commented Nov 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkubatrh commented Nov 21, 2017

pkubatrh 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

praiskup Nov 21, 2017 • edited Loading

Choose a reason for hiding this comment

dhodovsk commented Nov 24, 2017

pkubatrh commented Nov 24, 2017

dhodovsk commented Nov 24, 2017

Choose a reason for hiding this comment

dhodovsk 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

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

praiskup commented Nov 30, 2017

Choose a reason for hiding this comment

dhodovsk commented Dec 6, 2017

pkubatrh commented Dec 12, 2017

omron93 commented Dec 12, 2017

dhodovsk commented Dec 13, 2017

Choose a reason for hiding this comment

pkubatrh commented Dec 14, 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

hhorak commented Jan 4, 2018

praiskup commented Jan 4, 2018

dhodovsk commented Jan 9, 2018

praiskup commented Jan 9, 2018

praiskup Jan 9, 2018 • edited Loading

Choose a reason for hiding this comment

praiskup commented Jan 9, 2018

praiskup commented Jan 10, 2018

pkubatrh commented Jan 10, 2018

praiskup Nov 21, 2017 •

edited

Loading

praiskup Jan 9, 2018 •

edited

Loading