Improve the osmapping defaults in osmap.yaml #159

noelmcloughlin · 2017-09-14T00:38:31Z

This PR is to resolve #156 by instead populating osmap.yaml with more accurate OS defaults. The goal is to improve formula onboarding and working defaults. Three files are updated:

repo.yaml

fix typo

osmap.yaml

update Debian, RedHat and SUSE mappings

pillar.example

comment out pkg, pkg_client, pkg_extras stanza.

Verified on SUSE (no issues seen, all states passed)
Verified on Fedora (see #157, #158)
Todo: Ubuntu

noelmcloughlin · 2017-09-14T00:49:06Z

Please consider #125 during PR of osmap.yaml

vutny

@noelmcloughlin Thanks for the PR.

The cleanup of pillar.example and fixing Suse is definitely a valuable contribution.

However, did you have any real issues installing on Debians and RedHats using only stock defaults already in place? As for me, there is just a duplication of previous efforts.
More over, the formula handles pretty complex logic on determine the proper parameters for any possible packaged version of PostgreSQL distribution.

And again, please try to separate concerns and don't attempt to fix everything within single change :-) That's too hard, believe the experience of other contributors.
Although, you did a great job, thank you for being interested in making this formula even better.

vutny · 2017-09-14T06:33:44Z

pillar.example

+  ## pkg_client: 'postgresql-client-9.3'
+  ## pkgs_extra:
+    ## - postgresql-contrib
+    ## - postgresql-plpython


This is good, I think these items are no longer necessary. AFAIR they are legacy defaults for the test kitchen with Ubuntu 14. That should work smoothly now.

Just wondering, why double hashes everythere? (## )
I usually see the following convention then commenting out YAML or code blocks:

Real comments, i.e. developer notes are starting from # (hash followed by single space) or ## (two hashes with space).

Commented code or key-value pairs starts from #: hash sign only without following space, the space added only in case of required indentation.

This is easy to read and automatically strip off comments.

These values are Debian specific but Salt supports many OS. I'd prefer to remove but compromise is commenting out with 'one' hash ;-) I used two hashes to try to attract comment during PR review to be honest. I've noted your comments regarding commenting conventions. thanks

However, did you have any real issues installing on Debians and RedHats using only stock defaults already in place? As for me, there is just a duplication of previous efforts. More over, the formula handles pretty complex logic on determine the proper parameters for any possible packaged version of PostgreSQL distribution.

As mentioned in #156 the problems started when using pillars and involved heavy customization of pillar.example with jinja to get this working. Requiring end-users to do heavy customisation is unnecessary - the pillar.example should just work!

This is how I see the ideal formula use case:

Check it out and run with stock defaults with no Pillars set at all.

Find out that something wrong or not applicable to my environment and look at pillar.example.

See every value which could be customizable and adjust to my case.

Apply again and get profit.

So, the formula must just work without pillar.example, but pillar.example must provide working reference (or source for some test cases) without hard coded OS and implementation specific details whenever possible.

vutny · 2017-09-14T06:35:41Z

pillar.example

@@ -46,6 +43,8 @@ postgres:
  # Set ``False`` to stop creation of backups when config files change.
  {%- if salt['status.time']|default(none) is callable %}
  config_backup: ".backup@{{ salt['status.time']('%y-%m-%d_%H:%M:%S') }}"
+  {% else %}
+  config_backup: '.bak'


This is the default going to be merged from defaults.yaml. I don't think it is really necessary, if you would need to change the default, you'd need to remember to edit it in two places.

OK can remove. Does "if" always evaluate to true? Is false possible?

I guess on modern Salt versions it always be true.

vutny · 2017-09-14T06:36:13Z

pillar.example

@@ -58,7 +57,7 @@ postgres:
  # Use ``bake_image`` setting to control how PostgreSQL will be started: if set
  # to ``True`` the raw ``pg_ctl`` will be utilized instead of packaged init
  # script, job or unit run with Salt ``service`` state.
-  bake_image: True
+  bake_image: False


I thinks it is even better to comment this out and leave just for the reference. Good spot.

Commented out; but true?

By default it is false, there is an explanation above why you might want to uncomment the feature.

vutny · 2017-09-14T06:44:47Z

postgres/osmap.yaml

@@ -19,6 +19,17 @@ Debian:
  pkg_dev: postgresql-server-dev-all
  pkg_libpq_dev: libpq-dev

+  pkg: 'postgresql-{{ repo.version }}'
+  pkg_client: 'postgresql-client-{{ repo.version }}'


Please don't do it here. We have special file codenamemap.yaml to take care of this. The package names are really depending on use_upstream_repo setting.

Agree - The dilemma arises because these are defined in pillar.example for Debian, and osmap.yaml for RedHat and I don't like the inconsistency. I moved the Debian variables to osmap.yaml for consistency; but open to suggestions here. Looking at codenamemap.yaml Debian and Fedora are defined today.

vutny · 2017-09-14T06:46:59Z

postgres/osmap.yaml

@@ -19,6 +19,17 @@ Debian:
  pkg_dev: postgresql-server-dev-all
  pkg_libpq_dev: libpq-dev

+  pkg: 'postgresql-{{ repo.version }}'
+  pkg_client: 'postgresql-client-{{ repo.version }}'
+  pkgs_extra: [ postgresql-contrib, postgresql-plpython-{{ repo.version }}, libpostgresql-jdbc-java]


Are you sure everybody wants all the optional stuff, additional Python and Java dependencies by default?
This makes the formula run longer (read: it's longer to test).

I agree - we should support pkgs_extra without forcing it.

But I think "pkgs_extra: []" should be default in pillar.example uncommented. Then it should be easy to toggle this by removing "pkgs_extra: []" pillar. This makes regression test and end-user support seamless.

I added pkgs_extra: [] to pillar.example.

My point is that the formula by default should provide minimum working installation. And in many use cases a user just don't need to wait and download a half of the distribution just to get a separate DB for WordPress backend or something.
I understand that those packages are system-specific, but still I think it is up to the user to figure out and fill in any additional stuff. I suggest to keep pkgs_extra: [] as default and mention it in pillar.example without exposing values for some particular distro.

vutny · 2017-09-14T06:52:15Z

postgres/osmap.yaml

+  {% set data_dir = '/var/lib/postgresql/data' %}
+  conf_dir: {{ data_dir }}
+  prepare_cluster:
+    command: /usr/lib/postgresql/9.6/bin/initdb --pgdata={{ data_dir }}


You definitely should not do it here. Especially with hard-coded values like 9.6,

Agree. The default postgres.server state did not work properly. Fixing in pillar.example is considered wrong (see discussion at #156) so where should this be handled?

Yes, This should read {{ repo.version }} not 9.6.

vutny · 2017-09-14T07:00:35Z

postgres/osmap.yaml

+  prepare_cluster:
+    command: initdb --pgdata='{{ data_dir }}'
+    test: test -f '{{ data_dir }}/PG_VERSION'
+    user: postgres


All these lines are unnecessary, please see defaults.yaml.

Yes. But initdb and data_dir paths can differ between OS in theory.
How can this be addressed? See: #160

Here you just moved the section that should be defined for upstream PostgreSQL distribution.
If these parameters are not defined, the corresponding ones from defaults.yaml would be merged in.

vutny · 2017-09-14T07:06:40Z

postgres/osmap.yaml


+  {% set data_dir = '/var/lib/pgsql/data' %}
+  prepare_cluster:
+    command: /usr/lib/postgresql/bin/initdb --pgdata={{ data_dir }}


I guess this is the only required change for Suse. Are you sure that initdb is not in $PATH?

The command failed on SuSE so that is path.
The command failed on Ubuntu too - path is /usr/lib/postgresql/9.6/bin/initdb #160

vutny · 2017-09-14T07:11:49Z

postgres/osmap.yaml


+  {% set data_dir = '/var/lib/pgsql/data' %}


If you're going to set global variable, better to put it on top of the file. It would be redefined in if statement when necessary. But you don't need it at all.

vutny · 2017-09-14T07:15:25Z

postgres/osmap.yaml

-
-  prepare_cluster:
-    command: initdb --pgdata='{{ data_dir }}'
-    test: test -f '{{ data_dir }}/PG_VERSION'


This whole section is here for the purpose: the values should be set like this only when use_upstream_repo has been set to True.

You are correct. Note however that initdb, data_dir and conf_dir path can differ between OS.

vutny · 2017-09-15T07:36:07Z

@noelmcloughlin This PR and conversation threads are getting too complex to follow and grasp.
Please would you mind to close this one and prepare 3 separate changesets doing clear things:

Fix defaults for SUSE
Clean-up distro-specific and special case values from pillar.example
Deal with pkgs_extra if that you think the right way or something else

Then we will be able to do more cool stuff. And we would get clean history of changes for free!
I saw you've created issues for Fedora and Ubuntu. This is a good start. Thank you very much.

noelmcloughlin · 2017-09-15T10:26:37Z

@vutny I agree with your suggestion, but I will do the pillar.example PR only for now.

For the problems with Fedora and Ubuntu I have raised issues. I will do same for SuSE.

In saltstack-formulas I detect two main interfaces ...

External interfaces: Pillars are the 'common information model' (CIM) for SaltStack, the external exposed interface; and integration point (https://docs.saltstack.com/en/latest/topics/pillar/)
Internal interfaces: often YAML files to handle OS/platform differences & defaults.

The aggregated Pillar model (https://docs.saltstack.com/en/latest/topics/pillar/) is the Saltstack version of a CIM (http://www.dmtf.org/standards/cim). Any type of CIM must be constructed from element model fragments - aka pillar.example in this scenario - but using postgres pillar.example as a reference model does not work: #156, #157, #158, #160, #161, ....

Tomcat formula has same problem and I shared similar opinion there with @whiteinge
(saltstack-formulas/tomcat-formula#65)

I personally prefer each formula to have a good pillar.example for more pleasant onboarding experience. This formula installed ok on multiple OS without pillars. Once I introduced pillar.example it was disconcerting to see installation fail on every OS due to undefined tomcat "business logic" variables. The risk is new adopters walk away from saltstack due to sloppy user experience.

I believe a non-working pillar.example is unnecessary. It can force me (the adopter) to spend time building an appreciation of software "business logic" at very time when I want to focus on deployment UC using saltstack. Once deployed my end-user community (the "techies") can provide feedback on defaults and my role can be facilitator to drive formula evolution/backlog.

I'll close this PR shortly and follow up.
Thanks for your comments and patience!!!!

EvaSDK · 2017-09-20T14:43:46Z

From my experience, pillar.example is broken by design. Its purpose is to show some (most) of the actionable controls of a formula, it does not tell you what you should put in your configuration as only you do know. My view on the matter of formula expected behavior is:

it should apply out of the box, even if that means doing nothing (and I agree it can be disturbing at first) as the point is to not break existing installations
it should be easy to figure out actionable items (pillar.example here) as you want to quickly figure out if the formula can do what you need without reading all the states. You eventually need to do it to verify it won't break your system but you need a quick way to evaluate if it will do the job or if you need to contribute or even create your own.

I get that pillar.example then is misnamed but that's the current state of formula in the community afaics and it is a (bad) shortcut for documentation, although I still wonder how formula documentation could look like given it just is a transposition of upstream settings.

noelmcloughlin · 2017-09-20T18:51:37Z

That pillar.example is broken by design is my experience too. I am familiar with orchestration and technical solutions and this community satisfies my expectations. But pillar.example remains a puzzle. Sure its an optional artefact and, in my experience formulas tend to work without it within certain boundaries, but its a system model fragment.

And if I want a systems description language (SDL) then I'm keenly interested in pillars as the information model. Here formulas are simply model/namespace fragments to be resused, applied in isolation, merged and orchestrated, etc. Again the pillar information model seems to be the native system description language for integrating saltstack into system solutions - the interface.

We model user cases using some SDL (saltstack uses pillars, many modelling technologies exist) to build interesting solutions (formula yaml files are internal interfaces). Seeking a reference model the only reference model is pillar.example - the saltstack integration point for MANO - refer to https://wiki.opnfv.org/display/mano/MANO+Group+Home. WRT Mano, robust modelling is important.

You explanation of pillar.example is the best one I have seen yet & I need to think about it.

The bugs I raised on postgres recently - whilst maybe avoidable if I accept pillar.example as un-guaranteed artifact - must remain bugs because if I cannot model stuff using a SDL (pillars) then saltstack-formulas and saltstack could be bad fit for MANO and orchestartion user cases. We need to figure out how to describe and position the pillar interface to build confidence of system modellers in this technology & community.

Thanks @vutny

noelmcloughlin · 2017-09-20T19:24:43Z

Different information models and data models are being used amongst SDOs and open source communities resulting in fragmentation and complexity for implementation hence increased cost and delayed time to market [reference: https://wiki.opnfv.org/display/COM/Multi-SDO-Open+Source+Information+Modeling+Collaboration ]

myoung34 · 2017-09-21T00:52:21Z

I think if these are going to keep off topic this PR should be closed.
PR's become impossible to sift through when they get this long and off topic

noelmcloughlin · 2017-09-23T01:49:29Z

Thanks @vutny for collaborating on this pull request and I think we agreed the best plan is to split into some smaller tasks although I want to do further research first but some ideas were:

Fix defaults for SUSE
Clean-up distro-specific and special case values from pillar.example
Deal with pkgs_extra if that you think the right way or something else

In summary the community consensus for data/pillar model seems to be as follows:

cross-platform defaults, (i.e. defaults.yaml)
OS family default overrides, (i.e. osmap yaml)
OS codename overrides (i.e codemap yaml)
[ pillars ]

I'll look further at https://docs.saltstack.com/en/latest/topics/pillar/. All four of these layers are important and I'll need to find the best balance and that will come with experience.

Cheers.

Noel McLoughlin added 2 commits September 13, 2017 23:46

Resolve issue #156; better osmapping

97b82cd

Resolve issue #156; better osmapping

0fe31a6

This was referenced Sep 14, 2017

postgres_tablespace.present fails for [x] if x already present #158

Closed

Fedora: Unexpected issue with postgres-python-pkg state #157

Closed

vutny requested changes Sep 14, 2017

View reviewed changes

Noel McLoughlin added 5 commits September 14, 2017 20:47

Adopt PR comments

12dab82

Adopt PR comments

3ba9c85

Adopt PR comments

cb16d8c

Adopt PR comments

78f6f74

Adopt PR comments

e79bb6b

noelmcloughlin closed this Sep 23, 2017

noelmcloughlin deleted the better_osmapping branch September 23, 2017 22:20

Improve the osmapping defaults in osmap.yaml #159

Improve the osmapping defaults in osmap.yaml #159

Conversation

noelmcloughlin commented Sep 14, 2017 • edited Loading

noelmcloughlin commented Sep 14, 2017

vutny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noelmcloughlin Sep 14, 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

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

noelmcloughlin Sep 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noelmcloughlin Sep 14, 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

noelmcloughlin Sep 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noelmcloughlin Sep 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noelmcloughlin Sep 14, 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

noelmcloughlin Sep 14, 2017 • edited Loading

Choose a reason for hiding this comment

vutny commented Sep 15, 2017

noelmcloughlin commented Sep 15, 2017 • edited Loading

EvaSDK commented Sep 20, 2017 • edited Loading

noelmcloughlin commented Sep 20, 2017 • edited Loading

noelmcloughlin commented Sep 20, 2017 • edited Loading

myoung34 commented Sep 21, 2017

noelmcloughlin commented Sep 23, 2017 • edited Loading

noelmcloughlin commented Sep 14, 2017 •

edited

Loading

noelmcloughlin Sep 14, 2017 •

edited

Loading

noelmcloughlin Sep 14, 2017 •

edited

Loading

noelmcloughlin Sep 14, 2017 •

edited

Loading

noelmcloughlin Sep 14, 2017 •

edited

Loading

noelmcloughlin Sep 14, 2017 •

edited

Loading

noelmcloughlin Sep 14, 2017 •

edited

Loading

noelmcloughlin Sep 14, 2017 •

edited

Loading

noelmcloughlin commented Sep 15, 2017 •

edited

Loading

EvaSDK commented Sep 20, 2017 •

edited

Loading

noelmcloughlin commented Sep 20, 2017 •

edited

Loading

noelmcloughlin commented Sep 20, 2017 •

edited

Loading

noelmcloughlin commented Sep 23, 2017 •

edited

Loading