funtion to get pgsql version, exposed version, cluster and fixed session #1758

aaronlippold · 2017-05-04T00:28:52Z

Signed-off-by: Aaron Lippold [email protected]

adamleff

This seems like a reasonable approach. I left some comments with ways we can clean up the code and reduce some complexity.

This sort of behavior change where we trust psql's output before the actual data dir path is something we should document both in the resource itself (in the desc section so it shows up in the help postgres output) but also in the website docs. If someone has an old client in their path but a later-version data directory, they would get unexpected results.

Also, we need to add some tests. It looks like we don't have any for the postgres resource today, so starting with the new method you've created is a fine start.

adamleff · 2017-05-04T11:31:45Z

lib/resources/postgres.rb

-        cluster = cluster_from_dir("/etc/postgresql/#{version}")
-        @conf_dir = "/etc/postgresql/#{version}/#{cluster}"
-        @data_dir = "/var/lib/postgresql/#{version}/#{cluster}"
+        @version = version_from_psql.to_s


Why is to_s necessary here?

I suppose not, just being pedantic perhaps, ok we can remove it - I know it will return a string but just making sure. :)

adamleff · 2017-05-04T11:32:12Z

lib/resources/postgres.rb

-        @conf_dir = "/etc/postgresql/#{version}/#{cluster}"
-        @data_dir = "/var/lib/postgresql/#{version}/#{cluster}"
+        @version = version_from_psql.to_s
+        @version = version_from_dir('/etc/postgresql') if @version.nil?


This line and the line above it can be shortened to:

@version = version_from_psql || version_from_dir('/etc/postgresql')

adamleff · 2017-05-04T11:33:56Z

lib/resources/postgres.rb

-                  else
-                    version_from_dir('/var/lib/pgsql/')
-                  end
+        @version = version_from_psql.to_s


It looks like we can shorten this whole section to:

@version = version_from_psql || version_from_dir('/var/lib/pgsql/data') || version_from_dir('/var/lib/pgsql')

well we wouldn't run version_from_dir on /var/lib/pgsql/data, it is just checking if that dir exists. If it does, it was assumed that means the version structure isn't the standard of /var/lib/pgsql/'version'/data thus we could not find the version number via the directories so it was just let unset.

Using the new function we will always get a version so this logic really don't need to be there any more, so we can shorten it. The else in this cases was to say if 'var/lib/pgsql/data' doesn't exist - we assume that we can run version from dir on '/var/lib/pgsql' and assume to find the ver dir waiting for us. I will see about cleaning up the logic.

Oh yes, I remember why I left this. I updated the 'warn' statement to clarify why the logic is here.

If psql was not installed, the check for version.nil? also ensure that we still try to use the filesystem assuming we don't have an unversioned directory structure.

aaronlippold · 2017-05-11T13:11:35Z

@adamleff I completed the requested changes per your review. Are we good to go on this one?

aaronlippold · 2017-05-11T13:28:29Z

@note: this will also close #1551

aaronlippold · 2017-05-11T20:32:51Z

@adamleaf we good to go on this one as well?

…

On May 11, 2017 09:28, "Aaron Lippold" ***@***.***> wrote: @note <https://github.com/note>: this will also close #1551 <#1551> — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1758 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABauaMvwVKGTo9FS-YF4RrSBLsJ1GwVCks5r4wz-gaJpZM4NQG7D> .

adamleff

Real close, @aaronlippold - just a couple more things I think we need to address.

adamleff · 2017-05-16T02:42:29Z

lib/resources/postgres.rb

+        @version = version_from_psql
+        if @version.nil?
+          if inspec.directory('/var/lib/pgsql/data').exist?
+            warn 'PSQL did not return a version number and found /var/lib/pgsql/data.


I'd personally get rid of this warning statement. I'm not sure it adds any value to the user.

If we choose to leave it, we should change the wording because an end-user shouldn't need to worry about what a "nil" is, etc.

Suggested wording:

Unable to determine PostgreSQL version: psql did not return a version number and unversioned data directories were found.

But my recommendation is to eliminate it. See below for another commend on exposing the detection mechanism to the users if that's the problem we're trying to solve.

Ok. Agreed, just trying to not modify what I did not create but given that finding the version is much more stable using this method the warns may be unneeded.

Always leave something better than when you found it. :) If that means modifying someone else's code, that's just fine... in fact, you're doing that with this PR anyways. :)

adamleff · 2017-05-16T02:43:56Z

lib/resources/postgres.rb

@@ -59,7 +55,7 @@ def initialize
        @data_dir = '/var/lib/pgsql/data'
      end

-      @service = 'postgresql'
+      @service = 'postgresql-' + @version if @version.to_f >= 9.4


What if @version is < 9.4? Then @service will never get set.

This probably needs to be:

@service = 'postgresql' @service += "-#{@version}" if @version.to_f >= 9.4

Ok. I think this works as well. I can update that.

adamleff · 2017-05-16T02:47:39Z

lib/resources/postgres.rb

@@ -81,6 +77,16 @@ def verify_dirs
      end
    end

+    def version_from_psql


I don't think we need the warn here. I'd recommend we simplify this to:

return unless inspec.command('psql').exist? inspec.command("psql --version | awk '{ print $NF }' | awk -F. '{ print $1\".\"$2 }'").stdout.strip

Ultimately if we need users to know how we determined how we found the version, we can create a method they can assert a value on... i.e.:

describe postgres do its('detection_mechanism') { should eq 'psql' } its('detection_mechanism') { should eq 'versioned_data_directory' } its('detection_mechanism') { should eq 'unversioned_data_directory' } end

I think the warnings clutter up the output too much, and for users using a structured formatter (i.e. junit, json, etc.), they have to worry about redirecting stderr out of the way.

Ok. I was just following the previous convention from the code before I modified it. If you think the warn statements are no longer needed then I am fine with removing them. I just left them because @arlimus had them there before.

These changes have been pushed.

adamleff · 2017-05-18T17:59:45Z

I think the logic for Redhat is broken. I just spun up a new CentOS 7.3 box and yum install postgresql-server, initdb and started the service.

[root@localhost ~]# tree -L 2 /var/lib/pgsql
/var/lib/pgsql
├── backups
├── data
│   ├── base
│   ├── global
│   ├── pg_clog
│   ├── pg_hba.conf
│   ├── pg_ident.conf
│   ├── pg_log
│   ├── pg_multixact
│   ├── pg_notify
│   ├── pg_serial
│   ├── pg_snapshots
│   ├── pg_stat_tmp
│   ├── pg_subtrans
│   ├── pg_tblspc
│   ├── pg_twophase
│   ├── PG_VERSION
│   ├── pg_xlog
│   ├── postgresql.conf
│   ├── postmaster.opts
│   └── postmaster.pid
└── initdb.log

But we assume the version is part of the data directory path:

inspec> postgres.data_dir
Default postgresql configuration directory: /var/lib/pgsql/9.2/data does not exist. Postgresql may not be installed or we've misidentified the configuration directory.
Default postgresql data directory: /var/lib/pgsql/9.2/data does not exist. Postgresql may not be installed or we've misidentified the data directory.
=> "/var/lib/pgsql/9.2/data"
inspec> file(postgres.data_dir).exist?
Default postgresql configuration directory: /var/lib/pgsql/9.2/data does not exist. Postgresql may not be installed or we've misidentified the configuration directory.
Default postgresql data directory: /var/lib/pgsql/9.2/data does not exist. Postgresql may not be installed or we've misidentified the data directory.
=> false

Looks like this needs some rework.

adamleff · 2017-05-18T21:15:41Z

@aaronlippold per our conversation, you're going to go back and refactor this to try to determine the correct directory but only return it if it actually exists, avoiding the situation where we get the version but the directory isn't named with the version.

 - does /var/lib/pgsql/9.2/data exist? if so, return it. Else, continue.
 - does /var/lib/pgsql/data exist? if so, return it. Else, continue.
 - return nil because we don't know what other directory could be in use without running a psql query

aaronlippold · 2017-05-28T15:39:55Z

@adamleff added a function that will have a list of '@data_dir' locations. At the moment this is just used by the 'redhat' case but we could move this up so that all platforms could use it in a more general way.

i.e. we could just make the verify_data_dir_location take an Array and:

return nil if the array is empty and warn
return the first valid directory it finds
else warn we could not find the directory and return nil

aaronlippold · 2017-05-28T16:19:54Z

@adamleff something like this:

    def verify_data_dir_location(ver = @version)
      @data_dir = nil
      dir_list = [
        "/var/lib/pgsql/#{ver}/data",
        '/var/lib/pgsql/data',
        '/var/lib/postgres/data',
      ]
      dir_list.each do |data_dir_loc|
        @data_dir = data_dir_loc if inspec.directory(data_dir_loc).exists?
        break
      end
      if @data_dir.nil?
        warn 'Unable to find the PostgreSQL data_dir in expected location(s), please
        execute "psql -t -A -p <port> -h <host> -c "show hba_file";" as the PostgreSQL
        DBA to find the non-starndard data_dir location.'
      end
      @data_dir
    end

This would also make it so we can drop all the platform family if, else statements as we would check them all anyway. The only thing that could be a problem with this approach is I am not sure what would happen if two installations of postgres were on the system.

adamleff

@aaronlippold I think we're a lot closer. See my comments on things I think we need to tidy up for cleanliness and consistency, then I think we're good.

adamleff · 2017-05-31T18:30:57Z

lib/resources/postgres.rb

+             a version number and unversioned data directories were found.'
+            nil
+          else
+            version_from_dir('/var/lib/pgsql/')


Shouldn't this be:

@version = version_from_dir('/var/lib/pgsql/')

... #version_from_dir doesn't actually set @version

Yes most likely but we most likely missed it because we don't usually get to that branch of the tree in our tests. I updated it after ensuring that each case would return a number.

adamleff · 2017-05-31T18:33:16Z

lib/resources/postgres.rb

+            version_from_dir('/var/lib/pgsql/')
+          end
+        end
+        verify_data_dir_location(@version)


I'd like to see us remain consistent within a resource whether helper functions manipulate instance variables, or whether they return data to the caller who then decides whether to update an instance variable.

I prefer that methods do NOT manipulate instance variables because it makes it harder to test them and also could unknowingly pollute other tests.

I'd like to see this be:

@data_dir = verify_data_dir_locaton(@version)

... and have #verify_data_dir_location changed to #locate_data_dir_location_by_version and have it return a string containing the validated data directory.

Ok. I switched it so that all the helper functions just set an instance var externally rather than internally and updated the function name.

adamleff · 2017-05-31T18:34:19Z

lib/resources/postgres.rb

+      inspec.command("psql --version | awk '{ print $NF }' | awk -F. '{ print $1\".\"$2 }'").stdout.strip
+    end
+
+    def verify_data_dir_location(ver = @version)


See comment above re: keeping this helper method isolated rather than having it modify instance variables. That will make it easier to write tests and will remain consistent with the other functions in this resource, such as version_from_psql

the @Version instance var is not modified it is just used in the loop of the dir list, this function now just returns the instance var is it supporting rather than modifying it directly.

included requested /var/lib/postgresql/data to cover inspec#1673 as well Signed-off-by: Aaron Lippold <[email protected]>

arlimus

Amazing work and great to have this detection in here. Thank you Aaron!!

Also thank you @adamleff for helping and improving this PR :)

adamhjk added the in progress label May 4, 2017

adamleff suggested changes May 4, 2017

View reviewed changes

aaronlippold force-pushed the al/postgres-version-fix branch from 026be87 to 4d46847 Compare May 9, 2017 15:50

adamleff suggested changes May 16, 2017

View reviewed changes

aaronlippold force-pushed the al/postgres-version-fix branch from 4d46847 to eeef5d1 Compare May 16, 2017 21:56

adamleff added the Awaiting Contributor label May 18, 2017

adamleff mentioned this pull request May 18, 2017

updated the resource to first try to use the postgres process itself … #1699

Closed

aaronlippold force-pushed the al/postgres-version-fix branch 5 times, most recently from 2044937 to e09476d Compare May 28, 2017 15:26

aaronlippold mentioned this pull request May 28, 2017

WIP: support postgres 9.4 service on rhel #1551

Closed

aaronlippold force-pushed the al/postgres-version-fix branch 3 times, most recently from 8798866 to d4c88ae Compare May 30, 2017 15:11

adamleff suggested changes May 31, 2017

View reviewed changes

aaronlippold force-pushed the al/postgres-version-fix branch from d4c88ae to 7ca8339 Compare June 3, 2017 21:29

funtion to get pgsql version, exposed version, cluster and fixed session

3c02de9

included requested /var/lib/postgresql/data to cover inspec#1673 as well Signed-off-by: Aaron Lippold <[email protected]>

aaronlippold force-pushed the al/postgres-version-fix branch from 7ca8339 to 3c02de9 Compare June 3, 2017 22:28

aaronlippold mentioned this pull request Jun 3, 2017

Update postgres.rb #1673

Closed

adamleff approved these changes Jun 5, 2017

View reviewed changes

adamleff requested a review from chris-rock June 5, 2017 11:37

adamleff requested a review from arlimus June 5, 2017 11:37

adamleff added Type: Enhancement Improves an existing feature and removed Awaiting Contributor labels Jun 6, 2017

arlimus approved these changes Jun 6, 2017

View reviewed changes

arlimus merged commit 1c6fa01 into inspec:master Jun 6, 2017

arlimus removed the in progress label Jun 6, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

funtion to get pgsql version, exposed version, cluster and fixed session #1758

funtion to get pgsql version, exposed version, cluster and fixed session #1758

aaronlippold commented May 4, 2017

adamleff left a comment

adamleff May 4, 2017

aaronlippold May 5, 2017

adamleff May 4, 2017

adamleff May 4, 2017

aaronlippold May 5, 2017 •

edited

Loading

aaronlippold May 5, 2017 •

edited

Loading

aaronlippold commented May 11, 2017

aaronlippold commented May 11, 2017

aaronlippold commented May 11, 2017 via email

adamleff left a comment

adamleff May 16, 2017

aaronlippold May 16, 2017

adamleff May 16, 2017

adamleff May 16, 2017

aaronlippold May 16, 2017

adamleff May 16, 2017

aaronlippold May 16, 2017

aaronlippold May 17, 2017

adamleff commented May 18, 2017

adamleff commented May 18, 2017

aaronlippold commented May 28, 2017

aaronlippold commented May 28, 2017 •

edited

Loading

adamleff left a comment

adamleff May 31, 2017

aaronlippold Jun 3, 2017

adamleff May 31, 2017

aaronlippold Jun 3, 2017

adamleff May 31, 2017

aaronlippold Jun 3, 2017

arlimus left a comment

funtion to get pgsql version, exposed version, cluster and fixed session #1758

funtion to get pgsql version, exposed version, cluster and fixed session #1758

Conversation

aaronlippold commented May 4, 2017

adamleff 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

aaronlippold May 5, 2017 • edited Loading

Choose a reason for hiding this comment

aaronlippold May 5, 2017 • edited Loading

Choose a reason for hiding this comment

aaronlippold commented May 11, 2017

aaronlippold commented May 11, 2017

aaronlippold commented May 11, 2017 via email

adamleff 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

Choose a reason for hiding this comment

adamleff commented May 18, 2017

adamleff commented May 18, 2017

aaronlippold commented May 28, 2017

aaronlippold commented May 28, 2017 • edited Loading

adamleff 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

arlimus left a comment

Choose a reason for hiding this comment

aaronlippold May 5, 2017 •

edited

Loading

aaronlippold May 5, 2017 •

edited

Loading

aaronlippold commented May 28, 2017 •

edited

Loading