Support for upstream postgresql.org zypp repo #176
Conversation
|
Hi @vutny Can you merge this if you are happy please? |
|
Is anyone able to look at this? |
There was a problem hiding this comment.
Thanks @noelmcloughlin !
This looks pretty good. I have few suggestions to make it little bit more robust, so will post in-line comments by one on a time. Feel free to discuss. This should be merged eventually!
pillar.example
Outdated
| pkg: 'postgresql-9.3' | ||
| pkg_client: 'postgresql-client-9.3' | ||
| pkg: 'postgresql-9.6' | ||
| pkg_client: 'postgresql-client-9.6' |
There was a problem hiding this comment.
As the comment above said, these values are specific to Ubuntu, 14.04 to be precise.
This is because the pillar.example is used in test-kitchen within the formula.
That's kind of legacy, but still serves as smoke test for regressions.
Since now we have good support for package names mapping for various distros, I think it could be safely commented out.
There was a problem hiding this comment.
Can we go further and remove all package names from pillar.example?
Executing the formula with pillars use_upstream_repo: True with pkg_extras: ['postgresql-contrib', 'postgresql-plpython'] corrupts the postgres installation for example. Package names as pillars are not needed imho.
There was a problem hiding this comment.
And removeservice pillar too. And pkgs_extra example pillar implies postgres-contrib is optional "extra package", but the postgres.prepare_cluster.command fails (cannot find initdb) if the contrib package is not installed, implying its needed.
There was a problem hiding this comment.
You're right, the package and service definitions in Pillar are mostly unneeded.
But they could be useful when trying to install something on a latest-greatest and still unsupported distro.
I think it would be better to add a comment that would mention they are unnecessary in most cases.
There was a problem hiding this comment.
I'll think about this, thanks.
There was a problem hiding this comment.
Sure, just for now I think we should comment them out to make test-kitchen work, but add a proper description for these parameter later.
Or use generic ones like postgresql and postgresql-client. It will make transition to newer distro version much smoother as well.
pillar.example
Outdated
| - postgresql-contrib | ||
| - postgresql-plpython | ||
| #pkgs_extra: | ||
| # - postgresql-contrib |
There was a problem hiding this comment.
Regarding your comment above. If we're talking about Ubuntu, postgresql-contrib is definitely optional package, it does not contain initdb binary.
https://packages.ubuntu.com/trusty/amd64/postgresql-contrib-9.3/filelist
https://packages.ubuntu.com/xenial/amd64/postgresql-contrib-9.5/filelist
I think the issue with missing initdb lies somewhere else, maybe in our beloved alternatives?
So commenting out these extras is a good thing.
There was a problem hiding this comment.
Yeah, postgres-contrib creates alternatives. The formula requires initdb in PATH (see below) or state failures occur.
./postgres/defaults.yaml: command: initdb --pgdata=/var/lib/pgsql/data
./postgres/osmap.yaml: command: initdb -D /var/lib/postgres/data
./postgres/osmap.yaml: command: initdb --pgdata='{{ data_dir }}'
./postgres/osmap.yaml: - initdb
./postgres/osmap.yaml: command: initdb --pgdata='{{ lib_dir }}'
./postgres/osmap.yaml: command: initdb -D /Users/{{ repo.user }}/Library/AppSupport/postgres_{{ repo.use_upstream_repo }}
Do you know how initdb gets added to PATH without postgres-contrib package in this formula? The redhat os mapping invokes saltstack to create alternatives, so the package must have been missing when that feature was added.
Back to the specific comment - I would prefer to remove pkgs_extra pillar example, because it corrupted my installation once or twice, this is bad onboarding experience for new users. Otherwise commenting is definitely a good thing - I agree and will implement either solution.
There was a problem hiding this comment.
This is because of the postgres-contrib is just a metapackage that also installs a full PostgreSQL server + client of particular version available in the distro by dependencies. And there we have all the alternatives installation code in the downstream.
So if you have a wrong or empty value in the pkg Pillar value, the postgres-contrib just does the right thing.
This is just a very handy coincidence, that does not make postgres-contrib package mandatory.
There was a problem hiding this comment.
Okay. In this PR I explicitly fix the INITDB problem (#160) via Full Path. As documented in #160, upstream packages do not add initdb to PATH by design.
Here is my verified solution - I am unaware of any postgres-defined solution.
...
+ prepare_cluster:
+ #Full path needed as initdb is NOT 'cross version compatible' binary
+ command: /usr/pgsql-{{ release }}/bin/initdb --pgdata='{{ lib_dir }}'
+ test: test -f '{{ lib_dir }}/PG_VERSION'
+
+ # Handles etc alternatives for 'cross version compatible' binaries
+ pkgs_extra: ['postgresql{{ release }}-contrib',]
+
+{% else %}
If this solution is acceptable to you, let me know.
Note: For upstream repos, pillars values which override verified OS mapping defaults usually breaks things. Setting pkgs_extra: postgresql-contrib is particularly dangerous because it pulls distro dependencies unto upstream packages resulting in a mess.
There was a problem hiding this comment.
Yeah. That is a separate issue appearing exclusively when installing from upstream.
We will handle this. Thank you.
postgres/client.sls
Outdated
| @@ -23,11 +23,14 @@ postgresql-client-libs: | |||
| - refresh: True | |||
| - require: | |||
| - pkgrepo: postgresql-repo | |||
| {% if grains.os_family == 'Suse' %} | |||
| - cmd: postgresql-repo | |||
| {% endif %} | |||
There was a problem hiding this comment.
I think here would be better to depend on the whole postgres.upstream SLS to reduce verbose Jinja statements:
...
- require:
- sls: postgres.upstream
postgres/client.sls
Outdated
| {%- endif %} | ||
|
|
||
| {%- if 'bin_dir' in postgres %} | ||
|
|
||
| # Make client binaries available in $PATH | ||
| # Make client binaries available in $PATH |
There was a problem hiding this comment.
No need for indenting these comments, since they're part of rendered YAML, not Jinja.
It would look better in debug log when they are aligned with states they describe.
postgres/client.sls
Outdated
| @@ -23,6 +23,9 @@ postgresql-client-libs: | |||
| - refresh: True | |||
| - require: | |||
| - pkgrepo: postgresql-repo | |||
| {% if grains.os_family == 'Suse' %} | |||
There was a problem hiding this comment.
I meant, no need for {% if grains... %} statements and pkgrepo requisite above as well 😃
There was a problem hiding this comment.
no problem. check now.
postgres/upstream.sls
Outdated
| postgresql-repo: | ||
| pkgrepo.managed: | ||
| {{- format_kwargs(postgres.pkg_repo) }} | ||
| {% if grains.os_family == 'Suse' %} | ||
| cmd.run: | ||
| - name: zypper --gpg-auto-import-keys refresh --force |
There was a problem hiding this comment.
This is in connection with what we've discussed about requisites for the pkg state.
I know it is not obvious from the Salt docs, but pkgrepo.managed state supports special option gpgautoimport when target package manager is Zypper.
No need for messing with grains here. I'll show how simple it could be done above at osmap.yaml file.
There was a problem hiding this comment.
How did you know that ;-) I did check the docs.
| #Using sles-12 upstream repo for opensuse | ||
| baseurl: 'https://download.postgresql.org/pub/repos/zypp/{{ repo.version }}/suse/sles-12-$basearch' | ||
| key_url: 'https://download.postgresql.org/pub/repos/zypp/{{ repo.version }}/suse/sles-12-$basearch/repodata/repomd.xml.key' | ||
| gpgcheck: 1 |
There was a problem hiding this comment.
Just add
gpgautoimport: Truebellow, and importing GPG key from the upstream will start to magically work. No need for additional cmd.run state in the upstream.sls.
There was a problem hiding this comment.
Thanks - done - let me know when your PR comments are done and I'll do some retesting on Suse to make sure there is no regression.
postgres/osmap.yaml
Outdated
| test: test -f '{{ lib_dir }}/PG_VERSION' | ||
|
|
||
| # Handles etc alternatives for 'cross version compatible' binaries | ||
| pkgs_extra: ['postgresql{{ release }}-contrib',] |
There was a problem hiding this comment.
I've checked out the postgresql10-contrib-10.1-1PGDG.sles12.x86_64.rpm and appears it has nothing to do with alternatives at all.
I think we should disable extra packages installation by default, but may add a comment to pillar.example file mentioning how typically they called if having use_upstream_repo enabled.
There was a problem hiding this comment.
Thank you for checking. I fully agree extra package installation can be disabled by default for general OS mapping/defaults/pillar.example.
I will retest with Zypp upstream and if not needed will also ensure its disabled for Suse.
postgres/osmap.yaml
Outdated
| pkg_client: postgresql{{ release }} | ||
| conf_dir: {{ lib_dir }} | ||
| {% if release|int == 10 %} | ||
| service: postgresql-{{ release }} |
There was a problem hiding this comment.
I've checked out postgresql96-server-9.6.6-1PGDG.sles12.x86_64.rpm package, and there the service name follows the same pattern. I think we should omit release version check above.
|
Hi @vutny, I have done testing and few updates were needed to consistently pass verification. Please review again when you have time. Thanks. |
|
Hi @vutny can I merge this? |
There was a problem hiding this comment.
@noelmcloughlin This PR looks better and better. Good job!
But I doubt about introducing postgres.pkg_contrib property...
postgres/server/init.sls
Outdated
| @@ -15,7 +15,11 @@ include: | |||
|
|
|||
| {%- endif %} | |||
|
|
|||
| {%- set pkgs = [postgres.pkg] + postgres.pkgs_extra %} | |||
| {%- if postgres.extensions %} | |||
| {%- set pkgs = [postgres.pkg] + [postgres.pkg_contrib] + postgres.pkgs_extra %} | |||
There was a problem hiding this comment.
This statement looks very verbose and unnecessary...
It declares strait dependency between extensions setting in Pillar and newly introduced pkg_contrib value, which in turn seems to be applicable only on SUSE distros.
- What if desired extensions are not shipped with package defined in
pkg_contrib? - I see it was set empty (evaluates to
None) for other platforms. Will it work if some extensions have been set?
It looks little bit confusing, and is not described anywhere.
I suggest to keep the things as is: if user needs some extension, he or she should provide extra packages via pkgs_extra or install them another way.
|
I've removed the if postgres.extensions check. |
postgres/osmap.yaml
Outdated
| pkg_libpq_dev: libpqxx | ||
| pkg_dev: postgresql{{ release }}-devel | ||
| pkg_libs: postgresql{{ release }}-libs | ||
| pkg_contrib: postgresql{{ release }}-contrib |
There was a problem hiding this comment.
Here and below, the pkg_contrib parameter becomes unused. So no need to keep it.
postgres/osmap.yaml
Outdated
| pkg_libpq_dev: postgresql | ||
| pkg_libpq_dev: libpq5 | ||
| pkg_libs: postgresql-libs | ||
| pkg_contrib: postgresql-contrib |
There was a problem hiding this comment.
You may put the postgresql-contrib back into the proper section in pillar.example file with comment saying this package could be a requirement for installing some extensions.
There was a problem hiding this comment.
I don't want to. Pillar handling exposes production users to unacceptable risk.
- Handling packages via osmap.yaml pattern is good.
- Handling packages only via pillars (i.e. postgresql_contrib) is bad pattern.
-- Missing Postgresql-contrib package causes bug State postgres_extension-uuid-ossp fails #172.
-- Specifying wrong contrib_pkg pillar value corrupts package manager, PG installation, and worse, risks data loss. The PG schema may get upgraded. - And trying to handle State postgres_extension-uuid-ossp fails #172 via comments is wrong approach.
I think these risk need discussion.
There was a problem hiding this comment.
I mostly agree with all your points.
My main point is that pkg_contrib have not been used within current state of your changes, so having this in defaults and OS mapping files is meaningless right now. It just does nothing.
And this does not really help to solve issue #172 taking into an account broad scope of distributions supported by the formula.
I will try to put my thoughts there. Let's concentrate on bringing in initial basic support for SUSE systems (maybe less functional), but without breaking things which already work.
There was a problem hiding this comment.
I'll remove these entries from osmap.yaml but don't want pkg_contrib in pillar.example. Apart from the reasons above, I've done lots of verification on this formula past few weeks across many scenarios and I just don't agree with packages ONLY in pillars anymore, should be hidden from users. But, I do want the PR merged, so tell me if you want pillar.example restored.
okay committed.
postgres/osmap.yaml
Outdated
| pkg: postgresql-server | ||
| pkg_client: postgresql | ||
| pkg_libpq_dev: postgresql | ||
| pkg_libpq_dev: libpq5 |
There was a problem hiding this comment.
I found that OpenSuse also has C++ bindings package named libpqxx similar to the upstream.
There was a problem hiding this comment.
Thanks for checking. Should libpqxx value replace 'libpq5'?
There was a problem hiding this comment.
Yeah, exactly as in the upstream case to have everything consistent.
There was a problem hiding this comment.
Where did libpq5 come from? Cut and paste? maybe by me? ;-(
Committed
postgres/defaults.yaml
Outdated
| @@ -5,6 +5,7 @@ postgres: | |||
| version: '9.5' | |||
| pkg: postgresql | |||
| pkgs_extra: [] | |||
| pkg_contrib: | |||
There was a problem hiding this comment.
This one has been forgotten.
There was a problem hiding this comment.
Got it!! Sorry.
|
|
||
| prepare_cluster: | ||
| #Full path needed as initdb is NOT 'cross version compatible' binary | ||
| command: /usr/pgsql-{{ repo.version }}/bin/initdb --pgdata='{{ lib_dir }}' |
There was a problem hiding this comment.
This is generally OK. But I would recommend you to consider providing bin_dir and *_bins parameters similar to RedHat section for postgres.manage state to work on Suse out of the box as well.
But that could be added later.
There was a problem hiding this comment.
Also #190 captures problem regarding package names in pillar data.
pillar.example
Outdated
| # pkg: 'postgresql' | ||
| # pkg_client: 'postgresql-client' | ||
| # service: postgresql | ||
| #pkgs_extra: |
There was a problem hiding this comment.
This pkg_extra parameter still concerns me.
I thought we reached agreement that pillar.example file could not be used as valid source for any possible platform which the formula supports. Hard-coding some package names is bad thing too.
But now this breaks the test kitchen run.
We have two options:
- Uncomment
pkgs_extraand addpostgresql-contribthere mentioned this could be used only withuse_upstream_repo: False. - ...or replace
uuid-osspextension withplpgsql, that comes with PG server. The schema name also should be changed to somecustom_schemafor consistency.
There was a problem hiding this comment.
I have reinstated pkgs_extra as per original pillar.example.
Yeah, I'm unconvinced about pillar.example. The README and pillar.example are both delivered artifacts, responsible for onboarding new formula users (perhaps in a 1 week agile sprint), and guiding adoption for happy and unhappy paths. test kitchen is a platform too. On the other hand, I agree the pillar.example must have limited scope, which aligns with your stated view.
|
Excellent work reviewing this, thanks. |
|
@noelmcloughlin and @vutny thanks for all the hard work. |
This PR is an enhancement to allow the upstream repo to be used for SLES and OpenSUSE.
This rebased commit replaces PR #169
Tested successfully on version '9.6' and '10'. Failed with 9.5 due to upstream repo problem.