Hello Sandro Bonazzola,
I'd like you to do a code review. Please visit
http://gerrit.ovirt.org/13470
to review the following change.
Change subject: tools: fixed command line parser setup
......................................................................
tools: fixed command line parser setup
The previous command line parser setup caused the rewrite of the
database connection parameters read from .pgpass with the hardcoded
defaults each time the command line was parsed.
This caused a failure while collecting PostgreSQL database
configuration when using a remote setup
Now the script check if we have root permission, then read .pgpass
setting the default values for the command line parser.
In order to figure out what was happening, I added some logging
to the __main__ module and to the posgresql sos plugin.
Change-Id: I284554888143ad27bca0630cdbc26e91cf735ab9
Bug-Url: https://bugzilla.redhat.com/885421
Signed-off-by: Sandro Bonazzola <[email protected]>
---
M src/__main__.py
M src/sos/plugins/postgresql.py
2 files changed, 98 insertions(+), 70 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/ovirt-log-collector
refs/changes/70/13470/1
diff --git a/src/__main__.py b/src/__main__.py
index 634e61d..c716fcd 100755
--- a/src/__main__.py
+++ b/src/__main__.py
@@ -57,6 +57,62 @@
t = gettext.translation('logcollector', fallback=True)
_ = t.ugettext
+
+def get_pg_var(dbconf_param, user=None):
+ '''
+ Provides a mechanism to extract information from .pgpass.
+ '''
+ field = {'pass': 4, 'admin': 3, 'host': 0, 'port': 1}
+ if dbconf_param not in field.keys():
+ raise ValueError("Error: unknown value type '%s' was requested" % \
+ dbconf_param)
+ inDbAdminSection = False
+ if (os.path.exists(FILE_PG_PASS)):
+ logging.debug("Found existing pgpass file, fetching DB %s value" % \
+ dbconf_param)
+ with open(FILE_PG_PASS) as pgPassFile:
+ for line in pgPassFile:
+
+ # find the line with "DB ADMIN"
+ if PGPASS_FILE_ADMIN_LINE in line:
+ inDbAdminSection = True
+ continue
+
+ if inDbAdminSection and not line.startswith("#"):
+ # Means we're on DB ADMIN line, as it's for all DBs
+ dbcreds = line.split(":", 4)
+ return str(dbcreds[field[dbconf_param]]).strip()
+
+ # Fetch the password if needed
+ if dbconf_param == "pass" and user \
+ and not line.startswith("#"):
+ dbcreds = line.split(":", 4)
+ if dbcreds[3] == user:
+ return dbcreds[field[dbconf_param]]
+ return None
+
+
+def setup_pg_defaults():
+ """
+ Set defaults value to those read from .pgpass
+ """
+ global pg_user
+ global pg_pass
+ global pg_dbhost
+ global pg_dbport
+ try:
+ pg_user = get_pg_var('admin') or pg_user
+ pg_pass = get_pg_var('pass', pg_user) or pg_pass
+ pg_dbhost = get_pg_var('host') or pg_dbhost
+ pg_dbport = get_pg_var('port') or pg_dbport
+ except ValueError as ve:
+ print "Programming error in get_pg_var invocation: %s" % str(ve)
+ sys.exit(ExitCodes.CRITICAL)
+ except EnvironmentError as e:
+ print "Warning: error while reading .pgpass configuration: %s" % str(e)
+ ExitCodes.exit_code = ExitCodes.WARN
+
+
def multilog(logger, msg):
for line in str(msg).splitlines():
logger(line)
@@ -96,6 +152,7 @@
def call(self, cmds):
"""Uses the configuration to fork a subprocess and run cmds."""
_cmds = self.prep(cmds)
+ logging.debug("calling(%s)" % _cmds)
proc = subprocess.Popen(_cmds,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE)
@@ -137,9 +194,6 @@
raise Exception("Configuration requires a parser")
self.options, self.args = self.parser.parse_args()
- if os.geteuid() != 0:
- raise Exception("This tool requires root permissions to run.")
-
# At this point we know enough about the command line options
# to test for verbose and if it is set we should re-initialize
# the logger to DEBUG. This will have the effect of printing
@@ -147,7 +201,6 @@
if getattr(self.options, "verbose"):
self.__initLogger(logging.DEBUG)
- self.set_pg_db_attrs()
self.load_config_file()
if self.options:
@@ -172,58 +225,6 @@
self.__initLogger(level, self.options.quiet, self.options.log_file)
def __missing__(self, key):
- return None
-
- def set_pg_db_attrs(self):
- '''
- This method will check for /etc/ovirt-engin/.pgpass and set the
- values for the admin user, admin user pw, pg host, and pg port
- in the configuration dictionary.
-
- If values not found, use defaults.
- '''
-
- try:
- self['pg_user'] = self._get_pg_var('admin') or pg_user
- self['pg_pass'] = self._get_pg_var('pass', self['pg_user']) or
pg_pass
- self['pg_dbhost'] = self._get_pg_var('host') or pg_dbhost
- self['pg_dbport'] = self._get_pg_var('port') or pg_dbport
- except Exception, e:
- logging.debug(str(e))
-
- def _get_pg_var(self, dbconf_param, user=None):
- '''
- Provides a mechanism to extract information from .pgpass.
- '''
- field = {'pass': 4, 'admin': 3, 'host': 0, 'port': 1}
- if dbconf_param not in field.keys():
- raise Exception("Error: unknown value type '%s' was requested" % \
- dbconf_param)
-
- inDbAdminSection = False
- if (os.path.exists(FILE_PG_PASS)):
- logging.debug("found existing pgpass file, fetching DB %s value" %
\
- dbconf_param)
- with open(FILE_PG_PASS) as pgPassFile:
- for line in pgPassFile:
-
- # find the line with "DB ADMIN"
- if PGPASS_FILE_ADMIN_LINE in line:
- inDbAdminSection = True
- continue
-
- if inDbAdminSection and not line.startswith("#"):
- # Means we're on DB ADMIN line, as it's for all DBs
- dbcreds = line.split(":", 4)
- return str(dbcreds[field[dbconf_param]]).strip()
-
- # Fetch the password if needed
- if dbconf_param == "pass" and user \
- and not line.startswith("#"):
- dbcreds = line.split(":", 4)
- if dbcreds[3] == user:
- return dbcreds[field[dbconf_param]]
-
return None
def load_config_file(self):
@@ -641,7 +642,7 @@
def sosreport(self):
opt = ""
- if self.hostname == "localhost":
+ if self.configuration.get("pg_dbhost") == "localhost":
if self.configuration.get("pg_pass"):
opt = '-k postgresql.dbname=%(pg_dbname)s -k
postgresql.username=%(pg_user)s -k postgresql.password=%(pg_pass)s'
@@ -890,6 +891,11 @@
setattr(parser.values, option.dest, value)
if __name__ == '__main__':
+ if os.geteuid() != 0:
+ print "This tool requires root permissions to run."
+ sys.exit(ExitCodes.CRITICAL)
+
+ setup_pg_defaults()
def comma_separated_list(option, opt_str, value, parser):
setattr(parser.values, option.dest, [v.strip() for v in
value.split(",")])
diff --git a/src/sos/plugins/postgresql.py b/src/sos/plugins/postgresql.py
index 1088aaf..7befbb3 100644
--- a/src/sos/plugins/postgresql.py
+++ b/src/sos/plugins/postgresql.py
@@ -44,28 +44,35 @@
old_env_pgpassword = os.environ.get("PGPASSWORD")
os.environ["PGPASSWORD"] = "%s" % (self.getOption("password"))
if self.getOption("dbhost"):
- (status, output, rtime) = self.callExtProg("pg_dump -U %s -h %s -p
%s -w -f %s -F t %s" %
- (self.__username,
- self.getOption("dbhost"),
- self.__dbport,
- dest_file,
- self.getOption("dbname")))
+ cmd = "pg_dump -U %s -h %s -p %s -w -f %s -F t %s" % (
+ self.__username,
+ self.getOption("dbhost"),
+ self.__dbport,
+ dest_file,
+ self.getOption("dbname")
+ )
else:
- (status, output, rtime) = self.callExtProg("pg_dump -C -U %s -w -f
%s -F t %s " %
- (self.__username,
- dest_file,
-
self.getOption("dbname")))
-
+ cmd = "pg_dump -C -U %s -w -f %s -F t %s " % (
+ self.__username,
+ dest_file,
+ self.getOption("dbname")
+ )
+ self.soslog.debug("calling %s" % cmd)
+ (status, output, rtime) = self.callExtProg(cmd)
if old_env_pgpassword is not None:
os.environ["PGPASSWORD"] = str(old_env_pgpassword)
if (status == 0):
self.addCopySpec(dest_file)
else:
+ self.soslog.error(
+ "Unable to execute pg_dump. Error(%s)" % (output)
+ )
self.addAlert("ERROR: Unable to execute pg_dump. Error(%s)" %
(output))
def setup(self):
if self.getOption("pghome"):
self.__pghome = self.getOption("pghome")
+ self.soslog.debug("using pghome=%s" % self.__pghome)
if self.getOption("dbname"):
if self.getOption("password"):
@@ -76,7 +83,20 @@
self.tmp_dir = tempfile.mkdtemp()
self.pg_dump()
else:
- self.addAlert("WARN: password must be supplied to dump a
database.")
+ self.soslog.warning(
+ "password must be supplied to dump a database."
+ )
+ self.addAlert(
+ "WARN: password must be supplied to dump a database."
+ )
+ else:
+ self.soslog.warning(
+ "dbname must be supplied to dump a database."
+ )
+ self.addAlert(
+ "WARN: dbname must be supplied to dump a database."
+ )
+
# Copy PostgreSQL log files.
for file in find("*.log", self.__pghome):
@@ -88,10 +108,12 @@
self.addCopySpec(os.path.join(self.__pghome, "data" , "PG_VERSION"))
self.addCopySpec(os.path.join(self.__pghome, "data" ,
"postmaster.opts"))
-
def postproc(self):
import shutil
try:
shutil.rmtree(self.tmp_dir)
except:
+ self.soslog.exception(
+ "Unable to remove %s." % (self.tmp_dir)
+ )
self.addAlert("ERROR: Unable to remove %s." % (self.tmp_dir))
--
To view, visit http://gerrit.ovirt.org/13470
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I284554888143ad27bca0630cdbc26e91cf735ab9
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-log-collector
Gerrit-Branch: ovirt-log-collector-3.2
Gerrit-Owner: Kiril Nesenko <[email protected]>
Gerrit-Reviewer: Sandro Bonazzola <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
