Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add system's PostgreSQL compatibility #129

Merged
merged 8 commits into from
May 30, 2019

Conversation

miguelsorianod
Copy link
Contributor

@miguelsorianod miguelsorianod commented May 28, 2019

This PR adds the ability to use PostgreSQL as the System's database when using the Operator.

It also contains implementation to support it in the templates but we still have to decide whether we are going to support it there. I tried to split the commits so we can more easily remove that part of the functionality in case we don't need this functionality into the templates.

Changes performed:

  • Create separate components for SystemMySQL image and SystemPostgreSQL image, removing them from the Ampimages component
  • Update the operator's APIManager CR and logic to be able to define PostgreSQL or MySQL to be used as the System's database via configuration
  • Renamed template parameter POSTGRESQL_IMAGE TO ZYNC_DATABASE_IMAGE . This is because now that there might potentially be two PostgreSQL databases (zync-database and system-postgresql) and we will control the images separately, having two different imagestreams for system-postgresql and zync-database (that both use postgresql), so we can control updates separately
  • Add SystemPostgreSQL component that includes the needed elements to deploy a postgresql for system. As a special remark, this component deploys its own ImageStream named system-postgresql, instead of using the existing one named 'postgresql' (that is used by zync-database). The motivation behind this is that we want to manage image changes/updates separately for system-postgresql and zync-database, even if they both use postgresql as the container image. The 'postgresql' ImageStream that is currently used by zync-database has not been renamed at this moment (I know it might be confusing to have a 'system-postgresql' imagestream and a 'postgresql' imagestream (used by zync)). Doing so would imply that an upgrade specific procedure would be needed for it for the next release. Let me know your thoughts in case you think we need to change that to make it more intuitive for the user.
  • In the templates, renamed MYSQL_USER, MYSQL_PASSWORD, MYSQL_DATABASE and MYSQL_ROOT_PASSWORD changing the 'MYSQL' prefix by the 'SYSTEM_DATABASE' prefix, making use of the same prefix than the parameters used when PostgreSQL is used
  • Updated templates contents, including the addition of the amp-postgresql template based on the 'standard' one with the potsgresql-specific changes

@miguelsorianod miguelsorianod force-pushed the add-postgresql-compatibility branch 5 times, most recently from bef5352 to 99e11df Compare May 28, 2019 19:40
@miguelsorianod miguelsorianod changed the title [WIP] Add system's PostgreSQL compatibility Add system's PostgreSQL compatibility May 29, 2019
@miguelsorianod
Copy link
Contributor Author

miguelsorianod commented May 29, 2019

I think this PR is done.

Just a remark that I already commented with @guicassolato, when running PostgreSQL the following errors and warnings have been observed in the system-app pre-hook pod:

PG::InsufficientPrivilege: ERROR:  permission denied to create database
: CREATE DATABASE "system" ENCODING = 'utf8' LC_COLLATE = 'utf8_bin'
...```

with a stacktrace.
This too:
```Couldn't create database for {"adapter"=>"postgresql", "variables"=>{"timezone"=>"UTC"}, "encoding"=>"utf8", "collation"=>"utf8_bin", "pool"=>5, "username"=>"system", "password"=>"xxxxx", "database"=>"system", "host"=>"system-postgresql"}```

and several warnings like this one
```Recreated 74 triggers
WARNING: Can't mass-assign protected attributes for CMS::Builtin::Section: parent_id
    app/lib/logic/cms.rb:63:in `create_cms_assets'
    lib/tasks/db.rake:5:in `block (2 levels) in <top (required)>'
    lib/tasks/openshift.rake:5:in `block (2 levels) in <top (required)>'

But the pod apparently is able to finish and the PostgreSQL database seems to be populated. After commenting it with @guicassolato it seems that those errors and warnings are not a problem apparently, it's just rails tat tries to perform some tests when starting for which don't has permissions. Can you confirm it? If not, the only concern here that we should take into account is that those messages might be confusing for the user that reads the logs.

@guicassolato
Copy link
Contributor

I think this PR is done.

Just a remark that I already commented with @guicassolato, when running PostgreSQL the following errors and warnings have been observed in the system-app pre-hook pod:

PG::InsufficientPrivilege: ERROR:  permission denied to create database
: CREATE DATABASE "system" ENCODING = 'utf8' LC_COLLATE = 'utf8_bin'
...```

with a stacktrace.
This too:
```Couldn't create database for {"adapter"=>"postgresql", "variables"=>{"timezone"=>"UTC"}, "encoding"=>"utf8", "collation"=>"utf8_bin", "pool"=>5, "username"=>"system", "password"=>"xxxxx", "database"=>"system", "host"=>"system-postgresql"}```

and several warnings like this one
```Recreated 74 triggers
WARNING: Can't mass-assign protected attributes for CMS::Builtin::Section: parent_id
    app/lib/logic/cms.rb:63:in `create_cms_assets'
    lib/tasks/db.rake:5:in `block (2 levels) in <top (required)>'
    lib/tasks/openshift.rake:5:in `block (2 levels) in <top (required)>'

But the pod apparently is able to finish and the PostgreSQL database seems to be populated. After commenting it with @guicassolato it seems that those errors and warnings are not a problem apparently, it's just rails tat tries to perform some tests when starting for which don't has permissions. Can you confirm it? If not, the only concern here that we should take into account is that those messages might be confusing for the user that reads the logs.

The reason why it works despite of the error is because the database already exists. We are setting POSTGRESQL_DATABASE in dc/system-postgresql and then using the same values set to POSTGRESQL_USER and POSTGRESQL_PASSWORD in secret/system-database::URL. That ensures that the DBMS is deployed with its default database dedicated to system and that system has the required credentials to access it. No need to use POSTGRESQL_ADMIN_PASSWORD in this scenario.

There's a special case where the customer wants to use a given PostgreSQL instance where taking the default db for system is not an option and/or for any other reason the Rails application needs to create system db in the first deploy. In these cases, the customer may need to modify secret/system-database::URL so connections are established using a more privileged user – i.e. postgres, whose password in a PostgreSQL image like rhscl/postgresql-10-rhel (when defined) is given by setting the env var POSTGRESQL_ADMIN_PASSWORD in the initialization of the DBMS, just like it happens as well in https://hub.docker.com/r/centos/postgresql-10-centos7. Instructions:

  1. Modify secret/system-database::URL with more privileged credentials (e.g. postgres: or postgres:${POSTGRESQL_ADMIN_PASSWORD})
  2. Deploy system-(app-sidekiq-sphinx)
  3. Create a less privileged user for system
  4. Grant permissions to the user created in (3) over the database created in (2) including for DDL execution
  5. Modify secret/system-database::URL with the credentials of the user created in (3)
  6. Re-deploy system-(app-sidekiq-sphinx)

Copy link
Contributor

@guicassolato guicassolato left a comment

Choose a reason for hiding this comment

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

I successfully tested the PostgreSQL template amp-postgresql.yml included here and a custom-generated version amp-postgresql-eval-s3.yml locally and onto our ocp/devel cluster respectively.

@miguelsorianod
Copy link
Contributor Author

Thank you Gui. I tested locally too but it is good to have another tester 👍

Because we now we potentially have two different PostgreSQL
databases we renamed the POSTGRESQL_IMAGE to ZYNC_DATABASE_IMAGE
to only set the PostgreSQL database for Zync Database
Because we now might have potentially two PostgreSQL databases
we specify a little bit more the description and displayName
of the parameter ZYNC_DATABASE_PASSWORD.
Move System's MySQL ImageStream object into the
SystemMySQLImage component. This allows us to
be able to instantiate only the MySQL ImageStream when desired.
This is needed to be able to optionally deploy it. For example,
when System's PostgreSQL is used we should not deploy the
System's MySQL ImageStream.
The MYSQL_IMAGE parameter has also been renamed
into SYSTEM_DATABASE_IMAGE
Renamed MYSQL_USER, MYSQL_PASSWORD, MYSQL_DATABASE and
MYSQL_ROOT_PASSWORD chaning the 'MYSQL' prefix by the
'SYSTEM_DATABASE' prefix, making use of the same prefix
than the parameters used when PostgreSQL is used
@miguelsorianod miguelsorianod force-pushed the add-postgresql-compatibility branch from 99e11df to d7e6f8b Compare May 30, 2019 15:22
This commit also contains the addition of the new amp-postgresql
template.
@miguelsorianod miguelsorianod force-pushed the add-postgresql-compatibility branch from d7e6f8b to 5340351 Compare May 30, 2019 15:34
@codeclimate
Copy link

codeclimate bot commented May 30, 2019

Code Climate has analyzed commit 5340351 and detected 90 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 5
Duplication 20
Style 65

View more on Code Climate.

@miguelsorianod miguelsorianod merged commit e73506e into master May 30, 2019
@eguzki eguzki deleted the add-postgresql-compatibility branch June 12, 2019 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants