Allow PG MIQ configuration overrides via configmap #185

fbladilo · 2017-07-21T15:43:08Z

Configmap object created with MIQ default overrides settings
Configmap is injected as a volume into /var/lib/pgsql/conf.d by default
POSTGRESQL_CONFIG_DIR supplied to on template to allow flexibility on dir location
Related to PR : Allow MIQ PG config overrides to be included container-postgresql#2
PG image will pickup and do an include_dir if the directory is found

The most important piece is that the configuration is externalized and parametized. A user for instance can oc edit configmap , change parameter, oc rollout latest postgresql and be done. Configs can be stacked and multiple *.confs can be placed in directory , PG will process them all.

- Configmap object created with MIQ default overrides settings - Configmap is injected as a volume into /etc/manageiq/postgresql.conf.d by default - POSTGRESQL_CONFIG_DIR supplied to on template to allow flexibility on location - Related to PR : ManageIQ/container-postgresql#2 - PG image will pickup and do an include_dir if the directory is found The most important piece is that the configuration is externalized and parametized.

carbonin

I like this approach. This keeps things easily configurable and if/when the SCL base image supports this sort of thing we just need to change the location of the configmap mount.

What do you think @bdunne ?

carbonin · 2017-07-21T17:26:13Z

templates/miq-template.yaml

+- name: POSTGRESQL_CONFIG_DIR
+  displayName: PostgreSQL Configuration Overrides
+  description: Directory used to store PostgreSQL configuration overrides.
+  value: '/etc/manageiq/postgresql.conf.d'


If these are changed to double quotes I think the specs will pass.

I'll test on openshift see it the parser likes it, I recall issues with double quotes.

carbonin · 2017-07-24T13:43:51Z

templates/miq-template.yaml

+- name: POSTGRESQL_CONFIG_DIR
+  displayName: PostgreSQL Configuration Overrides
+  description: Directory used to store PostgreSQL configuration overrides.
+  value: "/etc/manageiq/postgresql.conf.d"


If this directory doesn't exist, will the config map mount create the parent directory?

@carbonin Yep, the configmap volume plugin creates the dir structure if it does not exist.

carbonin · 2017-07-24T13:49:54Z

Almost forgot about these settings:

From https://github.com/ManageIQ/manageiq-gems-pending/blob/6bdd64449485857526c968a119d55a7edba6e015/lib/gems/pending/appliance_console/internal_database_configuration.rb#L168-L176

def apply_initial_configuration
  shared_buffers = run_as_evm_server ? SHARED_DB_SHARED_BUFFERS : DEDICATED_DB_SHARED_BUFFERS
  with_pg_connection do |conn|
    conn.exec("ALTER SYSTEM SET ssl TO on") if ssl
    conn.exec("ALTER SYSTEM SET shared_buffers TO #{shared_buffers}")
  end

  restart_postgres
end

I don't think we're doing anything with ssl, but when an appliance runs as a database only (not being also used as a ManageIQ application server), we set shared_buffers to 1GB. Do we also want to set that as one of the environment variables in the container?

It's already configurable in the template

fbladilo · 2017-07-24T14:03:21Z

@carbonin If we want to emulate what appliance_console does in these cases, we probably need to set SHARED_BUFFERS on template to 1GB by default, this will be the conservative option(IMHO the default 256MB is already pretty low). Another alternative is alllow the user to increase SHARED_BUFFERS via configmap or environment variable whichever way suits best their case. oc set env or oc edit configmap can be used respectively, it requires rolling out PG afterwards to take the changes.

bdunne

👍 LGTM

bdunne · 2017-07-24T20:14:04Z

templates/miq-template.yaml

@@ -446,6 +524,8 @@ objects:
            value: "${POSTGRESQL_MAX_CONNECTIONS}"
          - name: POSTGRESQL_SHARED_BUFFERS
            value: "${POSTGRESQL_SHARED_BUFFERS}"
+          - name: POSTGRESQL_CONFIG_DIR


Is this env variable needed?

@bdunne This allows more flexibility, as user could potentially change the default location on template and we will process accordingly via image scripts.

I feel like this is a lot less likely to change now that we are specifying it at build time of our PG image.

@fbladilo Do you think people are likely to rebuild the image to change this?

@carbonin I could see some cases where you could want to supply your own location for your stacked configs (perhaps during troubleshoot/debug), also allow us to dynamically move to whatever directory SCL eventually decide for drop in config-customs. I don't think it hurts. My 2 cents.

@carbonin They do not need to rebuild the image to change this, kubernetes will allow the configmap volume plugin to be placed anywhere on the filesystem, it has special privileges. The image baked default directory is handy when things go wrong with the configmap, when is either missing or deleted, or also older builds such as euwe which they could begin using newer PG images that support conf.d but their templates are old and do not supply the configmap yet.

They do not need to rebuild the image to change this

I think they would.
They would need to create the new directory and set the permissions on the location like you did in the dependent PR right?

@carbonin Consider this custom deployment :

oc new-app --template=manageiq -p POSTGRESQL_IMG_NAME=docker.io/fbladilo/miq-postgresql -p POSTGRESQL_CONFIG_DIR=/etc/blah oc describe pods postgresql-1-fszjq | grep -A4 miq-pg-config /etc/blah from miq-pg-configs (rw) /var/lib/pgsql/data from miq-pgdb-volume (rw) /var/run/secrets/kubernetes.io/serviceaccount from default-token-zlxmm (ro) sh-4.2$ ls -la /etc/blah total 8 drwxrwsrwx. 3 root 1000060000 85 Jul 25 16:01 . drwxr-xr-x. 50 root root 4096 Jul 25 16:01 .. drwxr-sr-x. 2 root 1000060000 34 Jul 25 16:01 ..7987_25_07_12_01_10.985121115 lrwxrwxrwx. 1 root root 31 Jul 25 16:01 ..data -> ..7987_25_07_12_01_10.985121115 lrwxrwxrwx. 1 root root 28 Jul 25 16:01 01_miq_overrides.conf -> ..data/01_miq_overrides.conf sh-4.2$ grep include_dir /var/lib/pgsql/data/userdata/postgresql.conf #include_dir = 'conf.d' # include files ending in '.conf' from include_dir '/etc/blah'

Kubernetes grants the configmap volume plugin elevated privileges to do this (assuming the configmap is in place and added into the POD spec). Our default directory creation on image allows older deployments to take advantage of the dependent PR, as they are likely to not have any of the required variables set or configmaps created. It is also a safe default for us to have it created under most circumstances, things can go wrong as I mentioned with configmaps missing or troubled.

Okay, if that deployment works then I misunderstood the reasoning for changing the location of the conf.d directory.

I'm okay with this as is then 👍

- PG SCL image fixes permissions for OpenShift on /var/lib/pgsql

miq-bot · 2017-07-24T21:03:53Z

Checked commits fbladilo/manageiq-pods@56360c5~...7f49c49 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
0 files checked, 0 offenses detected
Everything looks fine. 🍪

carbonin

I think now that we have a sensible default in the base image, we probably don't need to make the location of the config map configurable at all.

In fact I'm hoping the SCL team will roll something like this into their base image so I would imagine they would just pick a spot eventually.

carbonin · 2017-07-25T13:47:42Z

templates/miq-template.yaml

@@ -446,6 +524,8 @@ objects:
            value: "${POSTGRESQL_MAX_CONNECTIONS}"
          - name: POSTGRESQL_SHARED_BUFFERS
            value: "${POSTGRESQL_SHARED_BUFFERS}"
+          - name: POSTGRESQL_CONFIG_DIR


I feel like this is a lot less likely to change now that we are specifying it at build time of our PG image.

@fbladilo Do you think people are likely to rebuild the image to change this?

carbonin · 2017-07-25T13:48:19Z

templates/miq-template.yaml

+- name: POSTGRESQL_CONFIG_DIR
+  displayName: PostgreSQL Configuration Overrides
+  description: Directory used to store PostgreSQL configuration overrides.
+  value: "/var/lib/pgsql/conf.d"


To my point about this not changing, this probably doesn't even need to be a parameter.

carbonin · 2017-07-25T13:48:43Z

templates/miq-template.yaml

 - name: POSTGRESQL_MAX_CONNECTIONS
  displayName: PostgreSQL Max Connections
  description: PostgreSQL maximum number of database connections allowed.
-  value: '100'
+  value: '1000'


carbonin · 2017-07-25T13:49:00Z

templates/miq-template.yaml

 - name: POSTGRESQL_MAX_CONNECTIONS
  displayName: PostgreSQL Max Connections
  description: PostgreSQL maximum number of database connections allowed.
-  value: '100'
+  value: '1000'
 - name: POSTGRESQL_SHARED_BUFFERS


Did we want to change this in this PR also @fbladilo ?

@carbonin I noticed our default MIQ overrides had it at 1000 and went ahead for the change. Let me know if you prefer a different PR for this setting and I'll revert it.

This comment was for the shared buffers, not the connections.

@carbonin 😄 Sure we can go ahead with the 1GB SHARED_BUFFERS default change for this one, I'll commit in a moment.

carbonin

I'm good with this as is. We can change the shared buffers in a follow up as we will probably need to also change some of the other PG memory requirements and limits.

fbladilo requested review from bdunne and carbonin July 21, 2017 15:58

carbonin approved these changes Jul 21, 2017

View reviewed changes

Use double quotes to pass specs and be happy

dbd52e3

carbonin reviewed Jul 24, 2017

View reviewed changes

bdunne approved these changes Jul 24, 2017

View reviewed changes

Change default conf.d directory location to known writable dir

7f49c49

- PG SCL image fixes permissions for OpenShift on /var/lib/pgsql

carbonin reviewed Jul 25, 2017

View reviewed changes

carbonin self-assigned this Jul 25, 2017

carbonin added the enhancement label Jul 25, 2017

carbonin added this to the Sprint 66 Ending Aug 7, 2017 milestone Jul 25, 2017

carbonin approved these changes Jul 25, 2017

View reviewed changes

carbonin merged commit 4a24abc into ManageIQ:master Jul 25, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow PG MIQ configuration overrides via configmap #185

Allow PG MIQ configuration overrides via configmap #185

fbladilo commented Jul 21, 2017 •

edited

Loading

carbonin left a comment

carbonin Jul 21, 2017

fbladilo Jul 21, 2017

carbonin Jul 24, 2017

fbladilo Jul 24, 2017

carbonin commented Jul 24, 2017

fbladilo commented Jul 24, 2017

bdunne left a comment

bdunne Jul 24, 2017

fbladilo Jul 24, 2017 •

edited

Loading

carbonin Jul 25, 2017

fbladilo Jul 25, 2017

fbladilo Jul 25, 2017 •

edited

Loading

carbonin Jul 25, 2017

fbladilo Jul 25, 2017

carbonin Jul 25, 2017

miq-bot commented Jul 24, 2017

carbonin left a comment

carbonin Jul 25, 2017

carbonin Jul 25, 2017

carbonin Jul 25, 2017

carbonin Jul 25, 2017

fbladilo Jul 25, 2017

carbonin Jul 25, 2017

fbladilo Jul 25, 2017

carbonin left a comment

Allow PG MIQ configuration overrides via configmap #185

Allow PG MIQ configuration overrides via configmap #185

Conversation

fbladilo commented Jul 21, 2017 • edited Loading

carbonin left a 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

carbonin commented Jul 24, 2017

fbladilo commented Jul 24, 2017

bdunne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbladilo Jul 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbladilo Jul 25, 2017 • edited Loading

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

miq-bot commented Jul 24, 2017

carbonin left a 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

carbonin left a comment

Choose a reason for hiding this comment

fbladilo commented Jul 21, 2017 •

edited

Loading

fbladilo Jul 24, 2017 •

edited

Loading

fbladilo Jul 25, 2017 •

edited

Loading