From 7c83d331895bffa05c828b7625fde9c558eb2296 Mon Sep 17 00:00:00 2001 From: Ivan Elfimov Date: Sat, 14 Mar 2020 23:03:56 +0300 Subject: [PATCH 1/5] Add support for watcher include directive - configs and tests This is an attempt to implement #1119. For now only configs and tests are added and some drafts in the config loader. This is still work in progress. --- circus/config.py | 25 ++++++++++++++++++++--- circus/tests/config/issue1119.ini | 6 ++++++ circus/tests/config/issue1119_include.ini | 1 + circus/tests/test_config.py | 8 +++++++- 4 files changed, 36 insertions(+), 4 deletions(-) create mode 100644 circus/tests/config/issue1119.ini create mode 100644 circus/tests/config/issue1119_include.ini diff --git a/circus/config.py b/circus/config.py index af274c5c2..24cbfb23a 100644 --- a/circus/config.py +++ b/circus/config.py @@ -2,6 +2,7 @@ import os import signal import warnings +from configparser import RawConfigParser from fnmatch import fnmatch try: import resource @@ -11,7 +12,7 @@ import six from circus import logger -from circus.py3compat import sort_by_field +from circus.py3compat import sort_by_field, StringIO from circus.util import (DEFAULT_ENDPOINT_DEALER, DEFAULT_ENDPOINT_SUB, DEFAULT_ENDPOINT_MULTICAST, DEFAULT_ENDPOINT_STATS, StrictConfigParser, replace_gnu_args, to_signum, @@ -127,7 +128,25 @@ def _scan(filename, includes): _scan(os.path.join(include_dir, '*.ini'), includes) logger.debug('Reading config files: %s' % includes) - return cfg, [config_path] + cfg.read(includes) + cfg.read(includes) + + # after all sections are included we try to include + # watcher specific files + for section in cfg.sections(): + if not section.startswith("watcher:"): + continue + watcher_includes = [] + for include_file in cfg.dget(section, 'include', '').split(): + _scan(include_file, watcher_includes) + for p in watcher_includes: + ini_str = '[root]\n' + open(p, 'r').read() + ini_fp = StringIO(ini_str) + config = RawConfigParser() + config.readfp(ini_fp) + # replace watcher config option + # with ones from config + + return cfg def get_config(config_file): @@ -135,7 +154,7 @@ def get_config(config_file): raise IOError("the configuration file %r does not exist\n" % config_file) - cfg, cfg_files_read = read_config(config_file) + cfg = read_config(config_file) dget = cfg.dget config = {} diff --git a/circus/tests/config/issue1119.ini b/circus/tests/config/issue1119.ini new file mode 100644 index 000000000..b86c00621 --- /dev/null +++ b/circus/tests/config/issue1119.ini @@ -0,0 +1,6 @@ +[circus] +pidfile = pidfile + +[watcher:server] +cmd = echo +include = issue1119_include.ini diff --git a/circus/tests/config/issue1119_include.ini b/circus/tests/config/issue1119_include.ini new file mode 100644 index 000000000..18c2bfd35 --- /dev/null +++ b/circus/tests/config/issue1119_include.ini @@ -0,0 +1 @@ +numprocesses = 5 diff --git a/circus/tests/test_config.py b/circus/tests/test_config.py index 150783ad6..24f55f3e2 100644 --- a/circus/tests/test_config.py +++ b/circus/tests/test_config.py @@ -46,7 +46,8 @@ 'issue680': os.path.join(CONFIG_DIR, 'issue680.ini'), 'virtualenv': os.path.join(CONFIG_DIR, 'virtualenv.ini'), 'empty_section': os.path.join(CONFIG_DIR, 'empty_section.ini'), - 'issue1088': os.path.join(CONFIG_DIR, 'issue1088.ini') + 'issue1088': os.path.join(CONFIG_DIR, 'issue1088.ini'), + 'watcher_include': os.path.join(CONFIG_DIR, 'issue1119.ini') } @@ -197,6 +198,11 @@ def test_empty_include(self, mock_logger_warn): self.fail('Non-existent includes should not raise') self.assertTrue(mock_logger_warn.called) + def test_watcher_include(self): + conf = get_config(_CONF['watcher_include']) + watchers = conf['watchers'] + self.assertEqual(watchers[0]['numprocesses'], 5) + def test_watcher_graceful_timeout(self): conf = get_config(_CONF['issue210']) watcher = Watcher.load_from_config(conf['watchers'][0]) From 665cc1f0d2f803029f993648dfe77963e54c91a1 Mon Sep 17 00:00:00 2001 From: Ivan Elfimov Date: Sat, 14 Mar 2020 23:14:26 +0300 Subject: [PATCH 2/5] Add simple solution, which still needs some optimizations - rename config in tests - rename test case - add setting of config option from include --- circus/config.py | 9 +++++---- circus/tests/test_config.py | 13 +++++++------ 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/circus/config.py b/circus/config.py index 24cbfb23a..0cb0e97ae 100644 --- a/circus/config.py +++ b/circus/config.py @@ -138,13 +138,14 @@ def _scan(filename, includes): watcher_includes = [] for include_file in cfg.dget(section, 'include', '').split(): _scan(include_file, watcher_includes) - for p in watcher_includes: - ini_str = '[root]\n' + open(p, 'r').read() + for include_path in watcher_includes: + # add section header for ConfigParser to understand it + ini_str = '[tmp]\n' + open(include_path, 'r').read() ini_fp = StringIO(ini_str) config = RawConfigParser() config.readfp(ini_fp) - # replace watcher config option - # with ones from config + for name, value in config.items('tmp'): + cfg.set(section, name, value) return cfg diff --git a/circus/tests/test_config.py b/circus/tests/test_config.py index 24f55f3e2..f2d038ee9 100644 --- a/circus/tests/test_config.py +++ b/circus/tests/test_config.py @@ -47,7 +47,7 @@ 'virtualenv': os.path.join(CONFIG_DIR, 'virtualenv.ini'), 'empty_section': os.path.join(CONFIG_DIR, 'empty_section.ini'), 'issue1088': os.path.join(CONFIG_DIR, 'issue1088.ini'), - 'watcher_include': os.path.join(CONFIG_DIR, 'issue1119.ini') + 'issue1119': os.path.join(CONFIG_DIR, 'issue1119.ini') } @@ -198,11 +198,6 @@ def test_empty_include(self, mock_logger_warn): self.fail('Non-existent includes should not raise') self.assertTrue(mock_logger_warn.called) - def test_watcher_include(self): - conf = get_config(_CONF['watcher_include']) - watchers = conf['watchers'] - self.assertEqual(watchers[0]['numprocesses'], 5) - def test_watcher_graceful_timeout(self): conf = get_config(_CONF['issue210']) watcher = Watcher.load_from_config(conf['watchers'][0]) @@ -402,5 +397,11 @@ def test_issue1088(self): watcher = Watcher.load_from_config(conf['watchers'][0]) watcher.stop() + def test_watcher_issue_1119(self): + # #1119 - add support for include directive in watcher section + conf = get_config(_CONF['issue1119']) + watchers = conf['watchers'] + self.assertEqual(watchers[0]['numprocesses'], 5) + test_suite = EasyTestSuite(__name__) From 75684efdd6091f121726c9ccc0275375fa1c2180 Mon Sep 17 00:00:00 2001 From: Ivan Elfimov Date: Sat, 14 Mar 2020 23:22:51 +0300 Subject: [PATCH 3/5] Replace RawConfigParser with StricConfigParser (py27 compat) --- circus/config.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/circus/config.py b/circus/config.py index 0cb0e97ae..1f9d3bf51 100644 --- a/circus/config.py +++ b/circus/config.py @@ -2,7 +2,7 @@ import os import signal import warnings -from configparser import RawConfigParser + from fnmatch import fnmatch try: import resource @@ -142,7 +142,7 @@ def _scan(filename, includes): # add section header for ConfigParser to understand it ini_str = '[tmp]\n' + open(include_path, 'r').read() ini_fp = StringIO(ini_str) - config = RawConfigParser() + config = StrictConfigParser() config.readfp(ini_fp) for name, value in config.items('tmp'): cfg.set(section, name, value) From 6a5802b6ce22de9ab7ce68386ef42b4a0c4b5f51 Mon Sep 17 00:00:00 2001 From: Ivan Elfimov Date: Tue, 17 Mar 2020 23:33:22 +0300 Subject: [PATCH 4/5] Improve watcher include directive handling - Refactor read_config method with two new local methods This was necessary to avoid code duplication, because include directive in `[watcher]` is basically the same as in `[circus]`. This allowed the reuse of `_scan` method. I also added underscores to arguments names, because they were shadowed. - `_init_config` - takes config path, has a parser option with default value and return parsed config from the path - `_get_includes` - takes config object and section name and returns a list of paths to read options from - Add a required section name for watcher includes to avoid usage of a fake `[tmp]` section from previous commits according to [documentation](https://docs.python.org/3/library/configparser.html) sections are required and in my opinion should be used going forward - Expand test case to ensure included configs can be loaded from folders and using wildcards --- circus/config.py | 60 ++++++++++--------- circus/tests/config/issue1119.ini | 3 +- circus/tests/config/issue1119_include.ini | 1 - circus/tests/config/issue1119_included.ini | 10 ++++ .../config/issue1119_included/bar/baz.ini | 2 + .../tests/config/issue1119_included/foo.ini | 2 + circus/tests/test_config.py | 9 ++- 7 files changed, 55 insertions(+), 32 deletions(-) delete mode 100644 circus/tests/config/issue1119_include.ini create mode 100644 circus/tests/config/issue1119_included.ini create mode 100644 circus/tests/config/issue1119_included/bar/baz.ini create mode 100644 circus/tests/config/issue1119_included/foo.ini diff --git a/circus/config.py b/circus/config.py index 1f9d3bf51..c7174241c 100644 --- a/circus/config.py +++ b/circus/config.py @@ -12,7 +12,7 @@ import six from circus import logger -from circus.py3compat import sort_by_field, StringIO +from circus.py3compat import sort_by_field from circus.util import (DEFAULT_ENDPOINT_DEALER, DEFAULT_ENDPOINT_SUB, DEFAULT_ENDPOINT_MULTICAST, DEFAULT_ENDPOINT_STATS, StrictConfigParser, replace_gnu_args, to_signum, @@ -98,19 +98,17 @@ def rlimit_value(val): def read_config(config_path): - cfg = DefaultConfigParser() - with open(config_path) as f: - if hasattr(cfg, 'read_file'): - cfg.read_file(f) - else: - cfg.readfp(f) - current_dir = os.path.dirname(config_path) - - # load included config files - includes = [] + def _init_config(config_path_, parser=DefaultConfigParser): + config = parser() + with open(config_path_) as f: + if hasattr(config, 'read_file'): + config.read_file(f) + else: + config.readfp(f) + return config - def _scan(filename, includes): + def _scan(filename, includes_): if os.path.abspath(filename) != filename: filename = os.path.join(current_dir, filename) @@ -119,32 +117,36 @@ def _scan(filename, includes): logger.warn('%r does not lead to any config. Make sure ' 'include paths are relative to the main config ' 'file' % filename) - includes += paths + includes_ += paths + + def _get_includes(config, section_): + incl = [] - for include_file in cfg.dget('circus', 'include', '').split(): - _scan(include_file, includes) + for include_file in config.dget(section_, 'include', '').split(): + _scan(include_file, incl) + + for include_dir in config.dget(section_, 'include_dir', '').split(): + _scan(os.path.join(include_dir, '*.ini'), incl) + + return incl + + current_dir = os.path.dirname(config_path) + cfg = _init_config(config_path) - for include_dir in cfg.dget('circus', 'include_dir', '').split(): - _scan(os.path.join(include_dir, '*.ini'), includes) + # load included config files in circus section + includes = _get_includes(cfg, 'circus') logger.debug('Reading config files: %s' % includes) cfg.read(includes) - # after all sections are included we try to include - # watcher specific files + # load included config files in watcher sections for section in cfg.sections(): - if not section.startswith("watcher:"): + if not section.startswith('watcher:'): continue - watcher_includes = [] - for include_file in cfg.dget(section, 'include', '').split(): - _scan(include_file, watcher_includes) + watcher_includes = _get_includes(cfg, section) for include_path in watcher_includes: - # add section header for ConfigParser to understand it - ini_str = '[tmp]\n' + open(include_path, 'r').read() - ini_fp = StringIO(ini_str) - config = StrictConfigParser() - config.readfp(ini_fp) - for name, value in config.items('tmp'): + included_cfg = _init_config(include_path) + for name, value in included_cfg.items('included'): cfg.set(section, name, value) return cfg diff --git a/circus/tests/config/issue1119.ini b/circus/tests/config/issue1119.ini index b86c00621..233be9ae3 100644 --- a/circus/tests/config/issue1119.ini +++ b/circus/tests/config/issue1119.ini @@ -3,4 +3,5 @@ pidfile = pidfile [watcher:server] cmd = echo -include = issue1119_include.ini +include = issue1119_included.ini issue1119_included/*/*.ini +include_dir = issue1119_included diff --git a/circus/tests/config/issue1119_include.ini b/circus/tests/config/issue1119_include.ini deleted file mode 100644 index 18c2bfd35..000000000 --- a/circus/tests/config/issue1119_include.ini +++ /dev/null @@ -1 +0,0 @@ -numprocesses = 5 diff --git a/circus/tests/config/issue1119_included.ini b/circus/tests/config/issue1119_included.ini new file mode 100644 index 000000000..f68f4e8f5 --- /dev/null +++ b/circus/tests/config/issue1119_included.ini @@ -0,0 +1,10 @@ +[included] +numprocesses = 5 +stdout_stream.class = FileStream +stdout_stream.filename = /var/logs/$(circus.wid).log +stdout_stream.max_bytes = 10000000 +stdout_stream.backup_count = 10 +stderr_stream.class = FileStream +stderr_stream.filename = /var/logs/$(circus.wid).log +stderr_stream.max_bytes = 1000000 +stderr_stream.backup_count = 10 diff --git a/circus/tests/config/issue1119_included/bar/baz.ini b/circus/tests/config/issue1119_included/bar/baz.ini new file mode 100644 index 000000000..5ba65cbdf --- /dev/null +++ b/circus/tests/config/issue1119_included/bar/baz.ini @@ -0,0 +1,2 @@ +[included] +stderr_stream.backup_count = 13 diff --git a/circus/tests/config/issue1119_included/foo.ini b/circus/tests/config/issue1119_included/foo.ini new file mode 100644 index 000000000..ece868bd6 --- /dev/null +++ b/circus/tests/config/issue1119_included/foo.ini @@ -0,0 +1,2 @@ +[included] +stderr_stream.max_bytes = 1000001 diff --git a/circus/tests/test_config.py b/circus/tests/test_config.py index f2d038ee9..9fa01009c 100644 --- a/circus/tests/test_config.py +++ b/circus/tests/test_config.py @@ -397,11 +397,18 @@ def test_issue1088(self): watcher = Watcher.load_from_config(conf['watchers'][0]) watcher.stop() - def test_watcher_issue_1119(self): + def test_ssue_1119_include(self): # #1119 - add support for include directive in watcher section conf = get_config(_CONF['issue1119']) watchers = conf['watchers'] + # numprocesses is overriden in issue1119_included.ini self.assertEqual(watchers[0]['numprocesses'], 5) + # this is to make sure wid is properly populated + self.assertEqual(watchers[0]['stdout_stream']['filename'], '/var/logs/$(circus.wid).log') + # this option is in the included directory + self.assertEqual(watchers[0]['stderr_stream']['max_bytes'], '1000001') + # this option is in the included sub-directory + self.assertEqual(watchers[0]['stderr_stream']['backup_count'], '13') test_suite = EasyTestSuite(__name__) From 28ba9221a914851b8d3055b485c40d1e56409ae9 Mon Sep 17 00:00:00 2001 From: Ivan Elfimov Date: Tue, 17 Mar 2020 23:39:10 +0300 Subject: [PATCH 5/5] fix flake8 error (line length) --- circus/tests/test_config.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/circus/tests/test_config.py b/circus/tests/test_config.py index 9fa01009c..a9d0dc884 100644 --- a/circus/tests/test_config.py +++ b/circus/tests/test_config.py @@ -404,7 +404,10 @@ def test_ssue_1119_include(self): # numprocesses is overriden in issue1119_included.ini self.assertEqual(watchers[0]['numprocesses'], 5) # this is to make sure wid is properly populated - self.assertEqual(watchers[0]['stdout_stream']['filename'], '/var/logs/$(circus.wid).log') + self.assertEqual( + watchers[0]['stdout_stream']['filename'], + '/var/logs/$(circus.wid).log', + ) # this option is in the included directory self.assertEqual(watchers[0]['stderr_stream']['max_bytes'], '1000001') # this option is in the included sub-directory