Nir Soffer has posted comments on this change. Change subject: vdsm.conf: Add drop-in dir ......................................................................
Patch Set 13: (26 comments) https://gerrit.ovirt.org/#/c/58728/13//COMMIT_MSG Commit Message: Line 23: - /etc/vdsm/vdsm.conf - for user configuration. We install this Line 24: file if missing, and never touch this file during upgrade. Line 25: - /etc/vdsm/vdsm.conf.d/ - for admin drop-in conf files. Line 26: - /usr/lib/vdsm/vdsm.conf.d/ - for vendor drop-in configuration files. Line 27: - /var/run/vdsm/vdsm.conf.d/ - for admin temporary configuration. > by that order? what if I have /etc/vdsm/vdsm.conf.d/50-yaniv.conf and /var/ Order does not matter, we use same rules used by systemd for drop-ins. The only thing that matter is the file name. See https://www.freedesktop.org/software/systemd/man/journald.conf.html and other systemd docs. Using the same rules, we make it easier for people to configure the system, everything behave in the same way. If you use the same file name in 2 directories, the result is undefined, do not do that. You can name the configuration file as you like, but using number_name.conf make it easier for users to set the order. This is a very old conventions since sysvinit. This should be documented in the config module docstring, a commit message is not a place for documentation. Line 28: Line 29: Files with a .conf suffix can be placed into any of the Line 30: vdsm.conf.d drop-in directories. Line 31: https://gerrit.ovirt.org/#/c/58728/13/lib/vdsm/config.py.in File lib/vdsm/config.py.in: Line 25: import glob Line 26: Line 27: Line 28: _SYSCONFDIR = '@sysconfdir@' Line 29: _VDSMCONFDIR = 'vdsm/vdsm.conf.d/' This is not needed, the old version with a name argument was better, making the code more generic - can be used by other ovirt project to load configurations in standard way. Line 30: _DROPPIN_BASE = ("/usr/lib/", "/run/", _SYSCONFDIR) Line 31: Line 32: parameters = [ Line 33: # Section: [vars] Line 26: Line 27: Line 28: _SYSCONFDIR = '@sysconfdir@' Line 29: _VDSMCONFDIR = 'vdsm/vdsm.conf.d/' Line 30: _DROPPIN_BASE = ("/usr/lib/", "/run/", _SYSCONFDIR) This is not one base, so it should be named _DROPIN_BASES Line 31: Line 32: parameters = [ Line 33: # Section: [vars] Line 34: ('vars', [ Line 506: print(twp.fill(comment)) Line 507: print(twp.fill("%s = %s" % (key, value))) Line 508: print('') Line 509: Line 510: Modifying code in .in files is huge pain. Please move all the code this file to a regular python module like configloader.py. Then the code in this file will contain only: import . configloader parameters = [ ... ] config = configloader.load(name, parameters) if __name__ == "__main__": configloader.print_config(parameters) This is best done in another patch before this patch, so adding support for drop-ins will be easier. Line 511: def load(): Line 512: cfg = configparser.ConfigParser() Line 513: set_defaults(cfg) Line 514: read_configs(cfg) Line 507: print(twp.fill("%s = %s" % (key, value))) Line 508: print('') Line 509: Line 510: Line 511: def load(): Better add name argument here def load(name): Line 512: cfg = configparser.ConfigParser() Line 513: set_defaults(cfg) Line 514: read_configs(cfg) Line 515: return cfg Line 510: Line 511: def load(): Line 512: cfg = configparser.ConfigParser() Line 513: set_defaults(cfg) Line 514: read_configs(cfg) And here: read_configs(cfg, name) Line 515: return cfg Line 516: Line 517: Line 518: def read_configs(cfg): Line 514: read_configs(cfg) Line 515: return cfg Line 516: Line 517: Line 518: def read_configs(cfg): And here: read_configs(cfg, name) Line 519: """This function is reading config files in a specific scheme Line 520: Line 521: The function reads - for one component - config files from several Line 522: locations and in addition it is also reading configuration snippets Line 526: other packages which can then be put into the vendor drop-in dir, Line 527: or users can overwrite the defaults, by placing a complete or partial Line 528: configuration file into /etc or a drop-in directory. Line 529: """ Line 530: default_conf = os.path.join(_SYSCONFDIR, 'vdsm', 'vdsm.conf') > why don't you use the _VDSMCONFDIR? I don't see a reason for the default_co Your suggestion would try to read: /etc/vdsm/vdsm.conf.d/vdsm.conf instead of /etc/vdsm/vdsm.conf. Also the code is more clear with default_conf var, is explains what is the string you build. Please keep this variable. With name argument this would be: default_conf = os.path.join(_SYSCONFDIR, name, name + ".conf") This is generic way to read any standard config, not only vdsm config. Line 531: cfg.read(default_conf) Line 532: Line 533: dropins = [] Line 534: for path in _DROPPIN_BASE: Line 531: cfg.read(default_conf) Line 532: Line 533: dropins = [] Line 534: for path in _DROPPIN_BASE: Line 535: pattern = os.path.join(path, _VDSMCONFDIR, '[0-9]*.conf') > shouldn't it be 0-99? Do not assume any format of file names, we should read any conf file. Use: "*.conf" Also, using name argument make this generic: pattern = os.path.join(path, name, name + "conf.d", '*.conf') We do this exactly 3 times, using _VDSMCONFDIR does not save anything. Line 536: dropins.extend(glob.glob(pattern)) Line 537: Line 538: dropins.sort(key=os.path.basename) Line 539: cfg.read(dropins) Line 534: for path in _DROPPIN_BASE: Line 535: pattern = os.path.join(path, _VDSMCONFDIR, '[0-9]*.conf') Line 536: dropins.extend(glob.glob(pattern)) Line 537: Line 538: dropins.sort(key=os.path.basename) > the priority should be sorted with the definition of _DROPPIN_BASE var imo. There is no priority, the current code is fine. Line 539: cfg.read(dropins) Line 540: Line 541: Line 542: config = load() Line 538: dropins.sort(key=os.path.basename) Line 539: cfg.read(dropins) Line 540: Line 541: Line 542: config = load() > can we remove the load() call in import, or is it a pain to do so now? beca This practically impossible. Today we import config everywhere, and we expect that the config is already loaded. The only way to do this is to load the config on the first import. The main issue with the current code is not having any logging when config is imported, so we don't have any way to report errors. Would be nice to improve this for 4.1, but we want to backport this change to 4.0. Line 543: Line 544: if __name__ == '__main__': https://gerrit.ovirt.org/#/c/58728/13/tests/config_test.py File tests/config_test.py: Line 24: from testlib import make_config, namedTemporaryDir, VdsmTestCase Line 25: from vdsm import config Line 26: Line 27: Line 28: ADMIN_PATH = 'etc/' We don't need these constants, they are used only in create_conf_dirs. Line 29: CONF_PATH = 'vdsm/vdsm.conf.d' Line 30: Line 31: RUNTIME_PATH = 'run/' Line 32: Line 25: from vdsm import config Line 26: Line 27: Line 28: ADMIN_PATH = 'etc/' Line 29: CONF_PATH = 'vdsm/vdsm.conf.d' > you have this var twice now. use the one from config. We don't need this, use a helper to create dropin, it will be the only place that need to know about this. Line 30: Line 31: RUNTIME_PATH = 'run/' Line 32: Line 33: VENDOR_PATH = 'usr/lib/' Line 32: Line 33: VENDOR_PATH = 'usr/lib/' Line 34: Line 35: SSL_TRUE = '[vars]\nssl = true\n' Line 36: SSL_FALSE = '[vars]\nssl = false\n' We don't need these constants, you want to have specifc value for each dropin, so you can tell which dropin value was taken when testing multiple dropin. This should be different for each test, so there is no point to define constants. Line 37: Line 38: Line 39: @contextmanager Line 40: def create_conf_dirs(): Line 36: SSL_FALSE = '[vars]\nssl = false\n' Line 37: Line 38: Line 39: @contextmanager Line 40: def create_conf_dirs(): Nice! I would name it confdirs or fakedirs: with fakedirs() as (admin_dir, vendor_dir, runtime_dir): ... Line 41: with namedTemporaryDir() as path: Line 42: admin_path = os.path.join(path, ADMIN_PATH) Line 43: vendor_path = os.path.join(path, VENDOR_PATH) Line 44: runtime_path = os.path.join(path, RUNTIME_PATH) Line 40: def create_conf_dirs(): Line 41: with namedTemporaryDir() as path: Line 42: admin_path = os.path.join(path, ADMIN_PATH) Line 43: vendor_path = os.path.join(path, VENDOR_PATH) Line 44: runtime_path = os.path.join(path, RUNTIME_PATH) We don't need the *_PATH constants, they are use only here. Line 45: paths = (admin_path, vendor_path, runtime_path) Line 46: with MonkeyPatchScope( Line 47: [(config, '_SYSCONFDIR', admin_path), Line 48: (config, '_DROPPIN_BASE', paths)]): Line 44: runtime_path = os.path.join(path, RUNTIME_PATH) Line 45: paths = (admin_path, vendor_path, runtime_path) Line 46: with MonkeyPatchScope( Line 47: [(config, '_SYSCONFDIR', admin_path), Line 48: (config, '_DROPPIN_BASE', paths)]): We should also monkeypatch the defaults, otherwise changing the defaults would break the tests. This will also make the test easier, we can invent our own test section and options using strings, so if we add validation in the future, it will not break the tests. parameters = [ ("test", [ ("option", "default value", None), )], ] We can monkeypatch now config.parameters. Line 49: yield paths Line 50: Line 51: Line 52: def create_conf(base_path, directory, file_name, content): Line 54: if not os.path.exists(file_path): Line 55: os.makedirs(file_path) Line 56: abs_file_name = os.path.join(file_path, file_name) Line 57: with open(abs_file_name, 'w') as f: Line 58: f.write(content) This complicated and make it harde to write and read the tests. We need these helpers: def create_dropin(basedir, name, content): path = os.path.join(basedir, "vdsm", "vdsm.conf.d", name) write_file(path, content) def create_conf(basedir, content): path = os.path.join(basedir, "vdsm", "vdsm.conf") write_file(path, content) def write_file(path, content): dirname = os.path.dirname(path) if not os.path.exists(dirname): os.makedirs(dirname) with open(path, "w") as f: f.write(content) Now the tests can use one line for creating any item. For example, testing reading a conf file and one dropin: with fakedirs() as (admin_dir, vendor_dir, runtime_dir): create_conf(admin_dir, "[test]\option=default conf value\n") create_dropin(vendor_dir, "50_vendor.conf", "[test]\noption=vendor value\n") cfg = config.load() self.assertEqual(cfg.get("test", "option"), "vendor value") Line 59: Line 60: Line 61: class TestConfig(VdsmTestCase): Line 62: def test_default(self): Line 59: Line 60: Line 61: class TestConfig(VdsmTestCase): Line 62: def test_default(self): Line 63: with create_conf_dirs() as (admin_path, vendor_path, runtime_path): admin_path, ... is confusing, this is not a path but a directory. Plase use admin_dir, vendor_dir, runtime_dir Line 64: create_conf(admin_path, 'vdsm', 'vdsm.conf', SSL_FALSE) Line 65: cfg = config.load() Line 66: self.assertFalse(cfg.getboolean('vars', 'ssl')) Line 67: Line 64: create_conf(admin_path, 'vdsm', 'vdsm.conf', SSL_FALSE) Line 65: cfg = config.load() Line 66: self.assertFalse(cfg.getboolean('vars', 'ssl')) Line 67: Line 68: def test_confread_dirs_priority(self): There are 4 tests here - please create seprarate test for case. There should be only one call to config.load() per test. Line 69: with create_conf_dirs() as (admin_path, vendor_path, runtime_path): Line 70: create_conf( Line 71: admin_path, Line 72: 'vdsm', Line 70: create_conf( Line 71: admin_path, Line 72: 'vdsm', Line 73: 'vdsm.conf', Line 74: '[test]\nnum = 1\n') This style is needed only because create_conf is accept too many arguments, please make a version that accept only the content and the base dir. We don't have a [test] section, so this will not work. We must test existing section and existing value, to make sure we have a default value. Line 75: Line 76: cfg = config.load() Line 77: self.assertEqual(cfg.getint('test', 'num'), 1) Line 78: Line 79: create_conf( Line 80: admin_path, Line 81: CONF_PATH, Line 82: '51.conf', Line 83: '[test]\nnum = 51\n') This style is needed only because you use create_conf here, use create_dropin helper to make the test more readable. Line 84: Line 85: cfg = config.load() Line 86: self.assertEqual(cfg.getint('test', 'num'), 51) Line 87: Line 106: def test_conf_override(self): Line 107: with create_conf_dirs() as (admin_path, vendor_path, runtime_path): Line 108: create_conf(runtime_path, CONF_PATH, '51_runtime.conf', SSL_FALSE) Line 109: cfg = config.load() Line 110: self.assertFalse(cfg.getboolean('vars', 'ssl')) > and if the default will change to false? please set the ssl to True , verif If we want to control the defaults, we must monkey patch them for the test. Line 111: Line 112: def test_ignore_invalid_conf_prefix(self): Line 113: with create_conf_dirs() as (admin_path, vendor_path, runtime_path): Line 114: create_conf(admin_path, 'vdsm', 'vdsm.conf', SSL_TRUE) Line 111: Line 112: def test_ignore_invalid_conf_prefix(self): Line 113: with create_conf_dirs() as (admin_path, vendor_path, runtime_path): Line 114: create_conf(admin_path, 'vdsm', 'vdsm.conf', SSL_TRUE) Line 115: create_conf(runtime_path, CONF_PATH, 'invalid.conf', SSL_FALSE) This is not invalid and should not be ignored. This test is wrong and if it pass the code is also wrong. Prefixing dropin names with number is the recommended way, not a requirement. See https://www.freedesktop.org/software/systemd/man/journald.conf.html for example. We want vdsm to behave exactly like systemd in this case. We don't want to be "better", as these better solutions are usually worse (e.g. pthreading, cpopen) in the long term. Line 116: cfg = config.load() Line 117: self.assertTrue(cfg.getboolean('vars', 'ssl')) Line 118: Line 119: def test_ignore_invalid_conf_suffix(self): Line 120: with create_conf_dirs() as (admin_path, vendor_path, runtime_path): Line 121: create_conf(admin_path, 'vdsm', 'vdsm.conf', SSL_TRUE) Line 122: create_conf(runtime_path, CONF_PATH, '50_invalid_name', SSL_FALSE) Line 123: cfg = config.load() Line 124: self.assertTrue(cfg.getboolean('vars', 'ssl')) > check also that we can pass 1-something.conf and 00-a.conf and 99-13221.con We don't need any of these, we must accept any filename ending with .conf. Line 125: Line 126: def test_same_directory_read_proirity(self): Line 127: with create_conf_dirs() as (admin_path, vendor_path, runtime_path): Line 128: create_conf( Line 156: """ Line 157: make sure unrelated items were not modified by config files Line 158: """ Line 159: with create_conf_dirs() as (admin_path, vendor_path, runtime_path): Line 160: cfg = make_config([('vars', 'ssl', 'true')]) > you can use load as the rest of the tests.. I don't see any reason for this make_config is needed so tests do not depend on the defaults or on the particular configuration on a machine. Please check the tests using this helper. Here it ensures that changing the defaults would not break this test. But if we monkeypatch the defaults (see my comment in create_conf_dirs), we don't need it for this test. Line 161: create_conf(runtime_path, CONF_PATH, '51_runtime.conf', SSL_FALSE) Line 162: create_conf(admin_path, CONF_PATH, 'vdsm.conf', SSL_FALSE) Line 163: Line 164: config.read_configs(cfg) -- To view, visit https://gerrit.ovirt.org/58728 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3849829aca50b30742e9c860d7c19296d6361015 Gerrit-PatchSet: 13 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Irit Goihman <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Irit Goihman <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: gerrit-hooks <[email protected]> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/admin/lists/[email protected]
