Cleaned up exception handling

Added exception chanes

Also improved docstrings

creation/lib/cWDictFile.py

@@ -1,16 +1,12 @@
 # SPDX-FileCopyrightText: 2009 Fermi Research Alliance, LLC
 # SPDX-License-Identifier: Apache-2.0
 
-#
-# Project:
-#   glideinWMS
-#
-# File Version:
-#
 # Description:
 #   Classes needed to handle dictionary files
 #   And other support functions
-#
+
+# TODO: in this file there are several directory creation functions. Use what's available in
+#  Python 3 and reduce custom code
 
 import copy
 import io
@@ -209,7 +205,7 @@ def save(
             with open(filepath, "wb") as fd:
                 self.save_into_fd(fd, sort_keys, set_readonly, reset_changed, want_comments)
         except OSError as e:
-            raise DictFileError(f"Error creating or writing to {filepath}: {e}")
+            raise DictFileError(f"Error creating or writing to {filepath}: {e}") from e
 
         # ensure that the file permissions are 644
         # This is to minimize a security risk where we load python code from
@@ -257,7 +253,7 @@ def save_into_fd(self, fd, sort_keys=None, set_readonly=True, reset_changed=True
                 fd.write(b"%s\n" % footer.encode(BINARY_ENCODING))
         except AttributeError as e:
             # .encode() attribute may be missing because bytes are passed
-            raise DictFileError(f"str received while writing {self.fname} ({type(self).__name__}): {e}")
+            raise DictFileError(f"str received while writing {self.fname} ({type(self).__name__}): {e}") from e
 
         if set_readonly:
             self.set_readonly(True)
@@ -336,7 +332,7 @@ def load(self, dir=None, fname=None, change_self=True, erase_first=True, set_not
             with fd:
                 self.load_from_fd(fd, erase_first, set_not_changed)
         except RuntimeError as e:
-            raise DictFileError(f"File {filepath}: {str(e)}")
+            raise DictFileError(f"File {filepath}: {str(e)}") from e
 
         if change_self:
             self.dir = dir
@@ -372,7 +368,7 @@ def load_from_fd(self, fd, erase_first=True, set_not_changed=True):
             try:
                 self.parse_val(line.decode(BINARY_ENCODING))
             except RuntimeError as e:
-                raise DictFileError("Line %i: %s" % (idx, str(e)))
+                raise DictFileError("Line %i: %s" % (idx, str(e))) from e
 
         if set_not_changed:
             self.changed = False  # the memory copy is now same as the one on disk
@@ -394,7 +390,7 @@ def load_from_str(self, data, erase_first=True, set_not_changed=True):
             try:
                 self.load_from_fd(fd, erase_first, set_not_changed)
             except RuntimeError as e:
-                raise DictFileError("Memory buffer: %s" % (str(e)))
+                raise DictFileError("Memory buffer: %s" % (str(e))) from e
         return
 
     def is_equal(self, other, compare_dir=False, compare_fname=False, compare_keys=None):
@@ -913,7 +909,7 @@ def add_from_file(self, key, val, filepath, allow_overwrite=False):
             with open(filepath, "rb") as fd:
                 self.add_from_fd(key, val, fd, allow_overwrite)
         except OSError as e:
-            raise DictFileError("Could not open file or read from it: %s" % filepath)
+            raise DictFileError("Could not open file or read from it: %s" % filepath) from e
 
     def format_val(self, key, want_comments):
         """Print lines: only the file name (key) the first item of the value tuple if not None
@@ -988,12 +984,12 @@ def save_files(self, allow_overwrite=False):
             try:
                 fd = open(filepath, "wb")
             except OSError as e:
-                raise DictFileError("Could not create file %s" % filepath)
+                raise DictFileError("Could not create file %s" % filepath) from None
             try:
                 with fd:
                     fd.write(fdata)
             except OSError as e:
-                raise DictFileError("Error writing into file %s" % filepath)
+                raise DictFileError("Error writing into file %s" % filepath) from None
 
 
 class FileDictFile(SimpleFileDictFile):
@@ -1125,8 +1121,10 @@ def add(
         # TODO: check parameters!!
         try:
             int(val[2])  # to check if is integer. Period must be int or convertible to int
-        except (ValueError, IndexError):
-            raise DictFileError("Values '%s' not (real_fname,cache/exec,period,prefix,cond_download,config_out)" % val)
+        except (ValueError, IndexError) as e:
+            raise DictFileError(
+                "Values '%s' not (real_fname,cache/exec,period,prefix,cond_download,config_out)" % val
+            ) from e
 
         if len(val) == self.DATA_LENGTH:
             # Alt: return self.add_from_str(key, val[:self.DATA_LENGTH-1], val[self.DATA_LENGTH-1], allow_overwrite)
@@ -1411,6 +1409,7 @@ def add_extended(
         elif user_name == True:
             user_name = "+"
 
+        # TODO: check .add end set allow_overwrite=False above instead allow_overwrite=0
         self.add(key, (type_str, val_default, condor_name, req_str, export_condor_str, user_name), allow_overwrite)
 
     def format_val(self, key, want_comments):
@@ -1551,7 +1550,7 @@ def save(
             with open(filepath, "wb") as fd:
                 self.save_into_fd(fd, sort_keys, set_readonly, reset_changed, want_comments)
         except OSError as e:
-            raise RuntimeError(f"Error creating or writing to {filepath}: {e}")
+            raise RuntimeError(f"Error creating or writing to {filepath}: {e}") from e
         chmod(filepath, 0o755)
 
         return
@@ -1597,17 +1596,17 @@ def __init__(self, dir, dir_name):
         self.dir = dir
         self.dir_name = dir_name
 
+    # TODO: there is os.mkdirs with fail_if_exists
     def create_dir(self, fail_if_exists=True):
         if os.path.isdir(self.dir):
             if fail_if_exists:
                 raise RuntimeError(f"Cannot create {self.dir_name} dir {self.dir}, already exists.")
             else:
                 return False  # already exists, nothing to do
-
         try:
             os.mkdir(self.dir)
         except OSError as e:
-            raise RuntimeError(f"Failed to create {self.dir_name} dir: {e}")
+            raise RuntimeError(f"Failed to create {self.dir_name} dir: {e}") from None
         return True
 
     def delete_dir(self):
@@ -1619,6 +1618,7 @@ def __init__(self, dir, chmod, dir_name):
         simpleDirSupport.__init__(self, dir, dir_name)
         self.chmod = chmod
 
+    # TODO: there is os.mkdirs with fail_if_exists
     def create_dir(self, fail_if_exists=True):
         if os.path.isdir(self.dir):
             if fail_if_exists:
@@ -1629,7 +1629,7 @@ def create_dir(self, fail_if_exists=True):
         try:
             os.mkdir(self.dir, self.chmod)
         except OSError as e:
-            raise RuntimeError(f"Failed to create {self.dir_name} dir: {e}")
+            raise RuntimeError(f"Failed to create {self.dir_name} dir: {e}") from None
         return True
 
 
@@ -1641,6 +1641,7 @@ def __init__(self, target_dir, symlink, dir_name):
         self.symlink = symlink
         self.dir_name = dir_name
 
+    # TODO: there is os.mkdirs with fail_if_exists, check if something similar for symlink
     def create_dir(self, fail_if_exists=True):
         if os.path.islink(self.symlink):
             if fail_if_exists:
@@ -1651,7 +1652,7 @@ def create_dir(self, fail_if_exists=True):
         try:
             os.symlink(self.target_dir, self.symlink)
         except OSError as e:
-            raise RuntimeError(f"Failed to create {self.dir_name} symlink: {e}")
+            raise RuntimeError(f"Failed to create {self.dir_name} symlink: {e}") from None
         return True
 
     def delete_dir(self):
@@ -1667,6 +1668,7 @@ def __init__(self):
     def add_dir_obj(self, dir_obj):
         self.dir_list.append(dir_obj)
 
+    # TODO: there is os.mkdirs with fail_if_exists
     def create_dirs(self, fail_if_exists=True):
         created_dirs = []
         try:

creation/lib/cWParams.py

@@ -267,7 +267,7 @@ def __init__(self, usage_prefix, src_dir, argv):
             # create derived values
             self.derive()
         except RuntimeError as e:
-            raise RuntimeError("Unexpected error occurred loading the configuration file.\n\n%s" % e)
+            raise RuntimeError("Unexpected error occurred loading the configuration file.\n\n%s" % e) from e
 
     def derive(self):
         return  # by default nothing... children should overwrite this
@@ -308,9 +308,9 @@ def load_file(self, fname):
         try:
             self.data = xmlParse.xmlfile2dict(fname, use_ord_dict=True)
         except xml.parsers.expat.ExpatError as e:
-            raise RuntimeError("XML error parsing config file: %s" % e)
+            raise RuntimeError("XML error parsing config file: %s" % e) from e
         except OSError as e:
-            raise RuntimeError("Config file error: %s" % e)
+            raise RuntimeError("Config file error: %s" % e) from e
         self.subparams = self.get_subparams_class()(self.data)
         return
 

creation/lib/cvWParamDict.py

@@ -56,7 +56,7 @@ def translate_match_attrs(loc_str, match_attrs_name, match_attrs):
         try:
             translated_attrs[attr_name] = translations[attr_type]
         except KeyError as e:
-            raise RuntimeError(f"Invalid {loc_str} {match_attrs_name} attr type '{attr_type}'")
+            raise RuntimeError(f"Invalid {loc_str} {match_attrs_name} attr type '{attr_type}'") from e
 
     return translated_attrs
 
@@ -106,9 +106,9 @@ def validate_match(loc_str, match_str, factory_attrs, job_attrs, attr_dict, poli
         match_obj = compile(match_str, "<string>", "exec")
         eval(match_obj, env)
     except KeyError as e:
-        raise RuntimeError(f"Invalid {loc_str} match_expr '{match_str}': Missing attribute {e}")
+        raise RuntimeError(f"Invalid {loc_str} match_expr '{match_str}': Missing attribute {e}") from e
     except Exception as e:
-        raise RuntimeError(f"Invalid {loc_str} match_expr '{match_str}': {e}")
+        raise RuntimeError(f"Invalid {loc_str} match_expr '{match_str}': {e}") from e
 
     # Validate the match(job, glidein) from the policy modules
     for pmodule in policy_modules:
@@ -118,9 +118,9 @@ def validate_match(loc_str, match_str, factory_attrs, job_attrs, attr_dict, poli
         except KeyError as e:
             raise RuntimeError(
                 f"Error in {loc_str} policy module's {pmodule.name}.match(job, glidein): Missing attribute {e}"
-            )
+            ) from e
         except Exception as e:
-            raise RuntimeError(f"Error in {loc_str} policy module's {pmodule.name}.match(job, glidein): {e}")
+            raise RuntimeError(f"Error in {loc_str} policy module's {pmodule.name}.match(job, glidein): {e}") from e
 
     return
 
@@ -770,7 +770,7 @@ def add_attr_unparsed(attr_name, params, dicts, description):
     try:
         add_attr_unparsed_real(attr_name, params, dicts)
     except RuntimeError as e:
-        raise RuntimeError(f"Error parsing attr {description}[{attr_name}]: {str(e)}")
+        raise RuntimeError(f"Error parsing attr {description}[{attr_name}]: {str(e)}") from e
 
 
 def validate_attribute(attr_name, attr_val):

creation/lib/xmlConfig.py

@@ -217,7 +217,7 @@ def check_sort_key(self):
             try:
                 LIST_TAGS[self.tag](child)
             except KeyError as e:
-                raise RuntimeError(child.err_str('missing "%s" attribute' % e))
+                raise RuntimeError(child.err_str('missing "%s" attribute' % e)) from None
 
     # this creates references into other rather than deep copies for efficiency
     def merge(self, other):
@@ -293,7 +293,7 @@ def validate(self):
         try:
             period = int(self["period"])
         except ValueError:
-            raise RuntimeError(self.err_str("period must be an int"))
+            raise RuntimeError(self.err_str("period must be an int")) from None
 
         if is_exec + is_wrapper + is_tar > 1:
             raise RuntimeError(self.err_str('must be exactly one of type "executable", "wrapper", or "untar"'))

creation/reconfig_glidein

@@ -292,7 +292,7 @@ if __name__ == "__main__":
             main(conf, update_scripts, update_def_cfg, comment=comment)
 
         except RuntimeError as e:
-            raise ReconfigError(str(e))
+            raise ReconfigError(str(e)) from e
 
     except ReconfigError as re:
         print2(re)

factory/glideFactory.py

@@ -3,21 +3,11 @@
 # SPDX-FileCopyrightText: 2009 Fermi Research Alliance, LLC
 # SPDX-License-Identifier: Apache-2.0
 
-#
-# Project:
-#   glideinWMS
-#
-# File Version:
-#
 # Description:
 #   This is the main of the glideinFactory
 #
 # Arguments:
 #   $1 = glidein submit_dir
-#
-# Author:
-#   Igor Sfiligoi (Apr 9th 2007 - moved old glideFactory to glideFactoryEntry)
-#
 
 import copy
 import fcntl
@@ -168,12 +158,11 @@ def write_descript(glideinDescript, frontendDescript, monitor_dir):
 
 
 def generate_log_tokens(startup_dir, glideinDescript):
-    """
-    Generate the JSON Web Tokens used to authenticate with the remote HTTP log server.
+    """Generate the JSON Web Tokens used to authenticate with the remote HTTP log server.
     Note: tokens are generated for disabled entries too
 
     Args:
-        startup_dir: Path to the glideinsubmit directory
+        startup_dir (str|Path): Path to the glideinsubmit directory
         glideinDescript: Factory config's glidein description object
 
     Returns:
@@ -805,7 +794,6 @@ def increase_process_limit(new_limit=10000):
             logSupport.log.info("Raised RLIMIT_NPROC from %d to %d" % (soft, new_limit))
         except ValueError:
             logSupport.log.info("Warning: could not raise RLIMIT_NPROC " "from %d to %d" % (soft, new_limit))
-
     else:
         logSupport.log.info("RLIMIT_NPROC already %d, not changing to %d" % (soft, new_limit))
 
@@ -969,8 +957,10 @@ def main(startup_dir):
             % (pid_obj.mypid, err)
         )
         raise
+    # TODO: use a single try.. except.. finally when moving to Python 3.8 or above (dropping 3.6)
     try:
         try:
+            # Spawn the EntryGroup processes handling the work
             spawn(
                 sleep_time,
                 advertize_rate,
@@ -981,11 +971,13 @@ def main(startup_dir):
                 restart_attempts,
                 restart_interval,
             )
-        except KeyboardInterrupt as e:
-            raise e
+        # No need for special handling of KeyboardInterrupt
+        # It is not in Exception so it will remain un-handled
+        # except KeyboardInterrupt as e:
+        #   raise e  # raise e is re-raising a different exceptoin from here? Use raise instead?
         except HUPException as e:
             # inside spawn(), outermost try will catch HUPException,
-            # then the code within the finally will run
+            # then the code within the finally clouse of spawn() will run
             # which will terminate glideFactoryEntryGroup children processes
             # and then the following 3 lines will be executed.
             logSupport.log.info("Received SIGHUP, reload config uid = %d" % os.getuid())
@@ -1004,8 +996,12 @@ def main(startup_dir):
                     "/etc/gwms-factory/glideinWMS.xml",
                 ],
             )
-        except:
-            logSupport.log.exception("Exception occurred spawning the factory: ")
+            # TODO: verify. This is invoking reconfig but how is the Factory/EntryGroups re-started?
+            #    Should there be an infinite loop around spawn?
+        except Exception as e:
+            # Exception excludes SystemExit, KeyboardInterrupt, GeneratorExit
+            # Log the exception and exit
+            logSupport.log.exception("Exception occurred spawning the factory: ", e)
     finally:
         pid_obj.relinquish()
 
@@ -1016,14 +1012,18 @@ def main(startup_dir):
 #
 ############################################################
 class HUPException(Exception):
+    """Used to catch SIGHUP and trigger a reconfig"""
+
     pass
 
 
 def termsignal(signr, frame):
+    """Signal handler. Raise KeyboardInterrupt when receiving SIGTERN or SIGQUIT"""
     raise KeyboardInterrupt("Received signal %s" % signr)
 
 
 def hupsignal(signr, frame):
+    """Signal handler. Raise HUPException when receiving SIGHUP. Used to trigger a reconfig and restart."""
     signal.signal(signal.SIGHUP, signal.SIG_IGN)
     raise HUPException("Received signal %s" % signr)