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

[WIP][discuss][bug]psql needs to be installed manually before running formula bacause it's used wrong for table detection (causes formula to fail completely) #151

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ghormoon
Copy link

PR progress checklist (to be filled in by reviewers)

  • Changes to documentation are appropriate (or tick if not required)
  • Changes to tests are appropriate (or tick if not required)
  • Reviews completed

What type of PR is this?

Primary type

  • [fix] A bug fix

Does this PR introduce a BREAKING CHANGE?

No.

Related issues and/or pull requests

Splitt off from #148 so the rest of the changes can be merged separately

Describe the changes you're proposing

Ignore changes done in #148 and discuss them there please

zabbix/pgsql/schema.sls should be changed to NOT use salt.postgresql.psql_schema on COMPILE time in jinja. This causes it to fail in case the psql is not present on the host yet -> even though zabbix.pgsql.pkgs looks like it should install it, it will not pass that far, so it will never pass without manual intervention.

Also we don't need to use dbroot_user and dbroot_password for that, we're using dbuser and dbpassword for import, so we should have enough permissions to check if the tables exist, so in my proposal, schema doesn't use these variables at all (they are only needed for zabbix.pgsql.conf, which uses them to create DB/user).

Pillar / config required to test the proposed changes

Use pgsql, eg.

zabbix:
  lookup:
    server:
      pkgs:
        - zabbix-server-pgsql

zabbix-pgsql:
  dbhost: host.xxx
  dbname: zabbix
  dbuser: zabbix
  dbpassword: zabbixpass
  dbsocket: ""

Debug log showing how the proposed changes work

----------
          ID: check_db_pgsql
    Function: cmd.run
        Name: [[ $(psql -X -A -t -c "SELECT count(tablename) FROM pg_catalog.pg_tables WHERE schemaname != 'pg_catalog' AND schemaname != 'information_schema';" || echo "-1") -eq "0"  ]] && echo "changed=yes comment='DB needs schema import.'" || echo "changed=no comment='No DB import needed or possible.'"
      Result: True
     Comment: DB needs schema import.
     Started: 14:30:43.815100
    Duration: 134.654 ms
     Changes:   
              ----------
              changed:
                  yes
              pid:
                  3132
              retcode:
                  0
              stderr:
              stdout:
                  changed=yes comment='DB needs schema import.
----------
          ID: import_sql
    Function: cmd.run
        Name: zcat /usr/share/doc/packages/zabbix-server-pgsql/create.sql.gz | psql | { head -5; cat >/dev/null; }
      Result: True
     Comment: Command "zcat /usr/share/doc/packages/zabbix-server-pgsql/create.sql.gz | psql | { head -5; cat >/dev/null; }" run
     Started: 14:30:43.953672
    Duration: 23340.792 ms
     Changes:   
              ----------
              pid:
                  3158
              retcode:
                  0
              stderr:
              stdout:
                  CREATE TABLE
                  CREATE INDEX
                  CREATE TABLE
                  CREATE INDEX
                  CREATE TABLE

Documentation checklist

  • Updated the README (e.g. Available states).
  • Updated pillar.example.

Testing checklist

  • Included in Kitchen (i.e. under state_top).
  • Covered by new/existing tests (e.g. InSpec, Serverspec, etc.).
  • Updated the relevant test pillar.

Additional context

@hatifnatt didn;t like the approach with added shell to run the psql (even though the import state does the same).
I tried to find a way, but i have no idea how to do it in a nicer way.

most promising idea was this, but i have no idea how to nicely process the output to make it plain True/False and the test state can't take anything more complex.

check_db_pgsql:
  test.configurable_test_state:
    - name: Is there any tables in '{{ dbname }}' database?
    - changes: __slot__:salt:postgres.psql_query("{{ list_tables }}", maintenance_db="{{ dbname }}", host="{{ dbhost }}", user="{{ dbuser }}", password="{{ dbpassword }}")
    - result: True
    - comment: If changes is 'True' data import required.

@ghormoon ghormoon force-pushed the pgsql-table-detection branch from 15fc710 to a42d822 Compare May 13, 2021 11:47
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.

1 participant