[libvirt-ci RFC PATCH 00/13] Introduce a new global TOML config file for lcitool
This series is trying to consolidate the number of config files we currently recognize under ~/.config/lcitool to a single global TOML config file. Thanks to this effort we can expose more seetings than we previously could which will come handy in terms of generating cloudinit images for OpenStack. Patches 1-4 patches are just a little extra - not heavily related to the series See patch 5/13 why TOML was selected. Erik Skultety (13): requirements: Introduce a requirements.txt file lcitool: Decrease the indent when creating a tempdir for initrd injection lcitool: Prefer tempfile's native wrappers over low level primitives lcitool: Use a temporary JSON file to pass extra variables config: Introduce a new global config.toml configuration file lcitool: Introduce methods to load and validate the TOML config lcitool: Drop the get_flavor() method lcitool: Drop the get_root_password_file() method lcitool: Drop the gitlab-related getter methods lcitool: Use d.update() on extra_vars for options coming from the config config: Move the virt-install settings from install.yml to the config lcitool: Use the config install options rather than install facts guests: README: Document the existence and usage of config.toml config.toml | 43 +++++ guests/README.markdown | 19 +- guests/lcitool | 235 +++++++++++------------ guests/playbooks/update/tasks/gitlab.yml | 2 - guests/playbooks/update/tasks/users.yml | 2 +- requirements.txt | 3 + 6 files changed, 175 insertions(+), 129 deletions(-) create mode 100644 config.toml create mode 100644 requirements.txt -- 2.25.3
Right now, lcitool's users have 2 options to satisfy the tool's dependencies: install them through the system package manager system-wide or install through pip manually. Since pip gives us the option to automate this process a tiny bit, let's ship a requirements file. This targets users who are used to work with Python virtual environments and install whatever it is necessary inside. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- requirements.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 requirements.txt diff --git a/requirements.txt b/requirements.txt new file mode 100644 index 0000000..dc416fa --- /dev/null +++ b/requirements.txt @@ -0,0 +1,3 @@ +ansible +jenkins-job-builder +toml -- 2.25.3
On Wed, 2020-04-22 at 15:28 +0200, Erik Skultety wrote:
Right now, lcitool's users have 2 options to satisfy the tool's dependencies: install them through the system package manager system-wide or install through pip manually. Since pip gives us the option to automate this process a tiny bit, let's ship a requirements file. This targets users who are used to work with Python virtual environments and install whatever it is necessary inside.
I remember Katerina suggesting that we add this, literally years ago O:-)
+++ b/requirements.txt
This is only relevat to lcitool, so it should go into the guests/ directory.
+ansible +jenkins-job-builder +toml
You don't actually use the toml module until 6/13, so leave this last line out until then. I haven't actually tested whether this work, but I trust you did and it certainly will not get in the way, so with the above fixed Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
The 'with' statement doesn't define a new code block [1], thus no local namespace is created. Therefore, we can still access the @content variable outside of the 'with' block. So, there's really no need to hold the @initrd_template file open longer than necessary (not that it would be a big deal anyway). [1] https://docs.python.org/3.7/reference/executionmodel.html Signed-off-by: Erik Skultety <eskultet@redhat.com> --- guests/lcitool | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/guests/lcitool b/guests/lcitool index dfb1ebc..9a9326d 100755 --- a/guests/lcitool +++ b/guests/lcitool @@ -608,11 +608,11 @@ class Application: facts[option] ) - tempdir = tempfile.mkdtemp() - initrd_inject = os.path.join(tempdir, install_config) + tempdir = tempfile.mkdtemp() + initrd_inject = os.path.join(tempdir, install_config) - with open(initrd_inject, "w") as inject: - inject.write(content) + with open(initrd_inject, "w") as inject: + inject.write(content) # preseed files must use a well-known name to be picked up by # d-i; for kickstart files, we can use whatever name we please -- 2.25.3
On Wed, 2020-04-22 at 15:28 +0200, Erik Skultety wrote:
The 'with' statement doesn't define a new code block [1], thus no local namespace is created. Therefore, we can still access the @content variable outside of the 'with' block. So, there's really no need to hold the @initrd_template file open longer than necessary (not that it would be a big deal anyway).
[1] https://docs.python.org/3.7/reference/executionmodel.html
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- guests/lcitool | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
Rather than requiring shutil module to get rid of the temporary directory we're creating for virt-install, let's use the TemporaryDirectory method instead which returns a file-like object which can be used to clean up the standard Python way. Although the internal exit handlers will take care of closing the temporary directories (and thus removing their contents) automatically, let's be explicit anyway and use the 'finally' clause to clean these up on both success and failure. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- guests/lcitool | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/guests/lcitool b/guests/lcitool index 9a9326d..51ee211 100755 --- a/guests/lcitool +++ b/guests/lcitool @@ -23,7 +23,6 @@ import json import os import platform import random -import shutil import string import subprocess import sys @@ -608,8 +607,8 @@ class Application: facts[option] ) - tempdir = tempfile.mkdtemp() - initrd_inject = os.path.join(tempdir, install_config) + tempdir = tempfile.TemporaryDirectory() + initrd_inject = os.path.join(tempdir.name, install_config) with open(initrd_inject, "w") as inject: inject.write(content) @@ -663,8 +662,8 @@ class Application: subprocess.check_call(cmd) except Exception as ex: raise Exception("Failed to install '{}': {}".format(host, ex)) - - shutil.rmtree(tempdir, ignore_errors=True) + finally: + tempdir.cleanup() def _action_update(self, args): self._execute_playbook("update", args.hosts, args.projects, -- 2.25.3
On Wed, 2020-04-22 at 15:28 +0200, Erik Skultety wrote:
Rather than requiring shutil module to get rid of the temporary directory we're creating for virt-install, let's use the TemporaryDirectory method instead which returns a file-like object which can be used to clean up the standard Python way. Although the internal exit handlers will take care of closing the temporary directories (and thus removing their contents) automatically, let's be explicit anyway and use the 'finally' clause to clean these up on both success and failure.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- guests/lcitool | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
This patch is a pre-requisite config file consolidation. Currently we've got a number of files which serve as a configuration either to the lcitool itself or to the ansible playbooks (majority). Once we replace these with a single global lcitool config, we'd end up passing tokens (potentially some passwords) as ansible extra variables bare naked on the cmdline. In order to prevent this security flaw use temporary JSON file holding all these extra variables and pass it as follows: $ ansible-playbook --extra-vars @extra_vars.json playbook.yml Signed-off-by: Erik Skultety <eskultet@redhat.com> --- guests/lcitool | 31 +++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/guests/lcitool b/guests/lcitool index 51ee211..138b5e2 100755 --- a/guests/lcitool +++ b/guests/lcitool @@ -504,21 +504,26 @@ class Application: git_remote = "default" git_branch = "master" + tempdir = tempfile.TemporaryDirectory(prefix='lcitool') + ansible_cfg_path = os.path.join(base, "ansible.cfg") playbook_base = os.path.join(base, "playbooks", playbook) playbook_path = os.path.join(playbook_base, "main.yml") + extra_vars_path = os.path.join(tempdir.name, 'extra_vars.json') - extra_vars = json.dumps({ - "base": base, - "playbook_base": playbook_base, - "root_password_file": root_pass_file, - "flavor": flavor, - "selected_projects": selected_projects, - "git_remote": git_remote, - "git_branch": git_branch, - "gitlab_url_file": gitlab_url_file, - "gitlab_runner_token_file": gitlab_runner_token_file, - }) + with open(extra_vars_path, 'w') as fp: + extra_vars = { + "base": base, + "playbook_base": playbook_base, + "root_password_file": root_pass_file, + "flavor": flavor, + "selected_projects": selected_projects, + "git_remote": git_remote, + "git_branch": git_branch, + "gitlab_url_file": gitlab_url_file, + "gitlab_runner_token_file": gitlab_runner_token_file, + } + json.dump(extra_vars, fp) ansible_playbook = distutils.spawn.find_executable("ansible-playbook") if ansible_playbook is None: @@ -527,7 +532,7 @@ class Application: cmd = [ ansible_playbook, "--limit", ansible_hosts, - "--extra-vars", extra_vars, + "--extra-vars", "@" + extra_vars_path, ] # Provide the vault password if available @@ -546,6 +551,8 @@ class Application: except Exception as ex: raise Exception( "Failed to run {} on '{}': {}".format(playbook, hosts, ex)) + finally: + tempdir.cleanup() def _action_hosts(self, args): for host in self._inventory.expand_pattern("all"): -- 2.25.3
On Wed, 2020-04-22 at 15:28 +0200, Erik Skultety wrote:
This patch is a pre-requisite config file consolidation. Currently we've got a number of files which serve as a configuration either to the lcitool itself or to the ansible playbooks (majority). Once we replace these with a single global lcitool config, we'd end up passing tokens (potentially some passwords) as ansible extra variables bare naked on the cmdline. In order to prevent this security flaw use temporary JSON file holding all these extra variables and pass it as follows:
$ ansible-playbook --extra-vars @extra_vars.json playbook.yml
I find it impossible not to point out that, if the configuration file was in YAML format, then we could pass its contents to Ansible without having to create a temporary file *or* risk exposing sensitive data O:-)
@@ -504,21 +504,26 @@ class Application: git_remote = "default" git_branch = "master"
+ tempdir = tempfile.TemporaryDirectory(prefix='lcitool')
If you want to pass prefix, do the same thing for the call introduced in the previous commit. Also, double quotes around strings please.
ansible_cfg_path = os.path.join(base, "ansible.cfg") playbook_base = os.path.join(base, "playbooks", playbook) playbook_path = os.path.join(playbook_base, "main.yml") + extra_vars_path = os.path.join(tempdir.name, 'extra_vars.json')
Double quotes.
- extra_vars = json.dumps({ - "base": base, - "playbook_base": playbook_base, - "root_password_file": root_pass_file, - "flavor": flavor, - "selected_projects": selected_projects, - "git_remote": git_remote, - "git_branch": git_branch, - "gitlab_url_file": gitlab_url_file, - "gitlab_runner_token_file": gitlab_runner_token_file, - }) + with open(extra_vars_path, 'w') as fp:
Double quotes.
+ extra_vars = { + "base": base, + "playbook_base": playbook_base, + "root_password_file": root_pass_file, + "flavor": flavor, + "selected_projects": selected_projects, + "git_remote": git_remote, + "git_branch": git_branch, + "gitlab_url_file": gitlab_url_file, + "gitlab_runner_token_file": gitlab_runner_token_file, + } + json.dump(extra_vars, fp)
Moving the definition of the dictionary is not needed: just do extra_vars = { ... } with open(...) as fp: json.dumps(extra_vars, fp) which keeps the with scope nice and small. With these few nits fixed, Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
Rather than having the configuration options split across multiple files (root-password, flavor, gitlab-url, gitlab-runner-token, ...), let's consolidate these settings into a global config file. The TOML format has been chosen simply because its syntax is very similar to the conventional INI format (thus easily human readable), but unlike INI it has a formal specification, it's typed and it also has some nice features over the plain INI should we find ourselves in need of any of such extended features in the future, e.g. table nesting. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- config.toml | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) create mode 100644 config.toml diff --git a/config.toml b/config.toml new file mode 100644 index 0000000..5bb3725 --- /dev/null +++ b/config.toml @@ -0,0 +1,26 @@ +# Configuration file for the lcitool -- https://gitlab.com/libvirt/libvirt-ci/ +# ============================================================================ + +[install] +# Installation flavor determining the target environment for the VM: +# +# test - VMs suitable for local testing, 'test' has passwordless sudo +# jenkins - VMs pluggable to a jenkins environment +# gitlab - VMs ready to be plugged to a GitLab environment +# +#flavor = "test" + +# Initial root password to be set by ansible on the appliance. This password +# will only be necessary for serial console access in case something goes +# horribly wrong, for all other use cases, SSH key authentication will be used +# instead. (mandatory) +#root_password = "" + +[gitlab] +# GitLab runner agent registration options, applies only if flavor == 'gitlab'. + +# GitLab server URL to register the runner. +#gitlab_url = "https://gitlab.com" + +# GitLab runner registration token. (mandatory) +#gitlab_runner_secret = "" -- 2.25.3
On Wed, 2020-04-22 at 15:28 +0200, Erik Skultety wrote:
and it also has some nice features over the plain INI should we find ourselves in need of any of such extended features in the future, e.g. table nesting.
Let's hope we never will! :)
diff --git a/config.toml b/config.toml
This is specific to lcitool, so it should go in guests/ rather than in the top-level directory.
+# Configuration file for the lcitool -- https://gitlab.com/libvirt/libvirt-ci/ +# ============================================================================
s/the //, and I would leave out the URL as well.
+# Initial root password to be set by ansible on the appliance. This password +# will only be necessary for serial console access in case something goes +# horribly wrong, for all other use cases, SSH key authentication will be used +# instead. (mandatory) +#root_password = ""
s/ansible/Ansible/ With these nits addressed, Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
On Wed, 2020-04-22 at 15:28 +0200, Erik Skultety wrote:
+[gitlab] +# GitLab runner agent registration options, applies only if flavor == 'gitlab'. + +# GitLab server URL to register the runner. +#gitlab_url = "https://gitlab.com" + +# GitLab runner registration token. (mandatory) +#gitlab_runner_secret = ""
Additional nit: since TOML is organized in sections, it doesn't make sense to include the section name in the key name, so this should look like [gitlab] url = "https://gitlab.com" runner_secret = "..." -- Andrea Bolognani / Red Hat / Virtualization
On Tue, Apr 28, 2020 at 09:39:31AM +0200, Andrea Bolognani wrote:
On Wed, 2020-04-22 at 15:28 +0200, Erik Skultety wrote:
+[gitlab] +# GitLab runner agent registration options, applies only if flavor == 'gitlab'. + +# GitLab server URL to register the runner. +#gitlab_url = "https://gitlab.com" + +# GitLab runner registration token. (mandatory) +#gitlab_runner_secret = ""
Additional nit: since TOML is organized in sections, it doesn't make sense to include the section name in the key name, so this should look like
[gitlab] url = "https://gitlab.com" runner_secret = "..."
Absolutely agreed. That was the initial approach, however it didn't pair with the naming used within the Ansible playbooks, so I changed it. If I'm not mistaken "url" will cause a collision with another variable, that's why I was explicit with naming in the config, although I agree that from the config POV it doesn't make sense to include the section prefix. -- Erik Skultety
This patch introduce a set of class Config helper methods in order to parse and validate the new global TOML config. Currently, only 'install' and 'gitlab' sections are recognized with the flavor setting defaulting to "test" (backwards compatibility) and gitlab runner registration url defaulting to "https://gitlab.com"; the rest of the options are currently mandatory. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- guests/lcitool | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/guests/lcitool b/guests/lcitool index 138b5e2..f08bd98 100755 --- a/guests/lcitool +++ b/guests/lcitool @@ -28,6 +28,7 @@ import subprocess import sys import tempfile import textwrap +import toml import yaml # This is necessary to maintain Python 2.7 compatibility @@ -133,6 +134,21 @@ class Util: class Config: + def __init__(self): + path = self._get_config_file("config.toml") + + try: + self.dict = toml.load(path) + self._validate() + + except Exception as ex: + raise Exception( + "Missing or invalid config.toml file: {}".format(ex) + ) + + # fill in default options + self._fill_default_options(self.dict) + @staticmethod def _get_config_file(name): try: @@ -154,6 +170,42 @@ class Config: return os.path.join(config_dir, name) + @staticmethod + def _fill_default_options(cfg): + flavor = cfg.get("install").get("flavor", "test") + cfg["install"]["flavor"] = flavor + + if flavor == "gitlab": + url = cfg.get("gitlab").get("gitlab_url", "https://gitlab.com") + cfg["gitlab"]["gitlab_url"] = url + + @staticmethod + def _validate_section(cfg, section, *args): + + if cfg.get(section, None) is None: + raise KeyError("Section '[{}]' not found".format(section)) + + for key in args: + if cfg.get(section).get(key, None) is None: + raise KeyError("Missing mandatory key '{}.{}'".format(section, + key)) + + def _validate(self): + + # verify the [install] section and its mandatory options + self._validate_section(self.dict, "install", "root_password") + + # we need to check flavor here, because later validations depend on it + flavor = self.dict.get("install").get("flavor", "test") + if flavor not in ["test", "jenkins", "gitlab"]: + raise ValueError( + "Invalid value '{}' for 'install.flavor'".format(flavor) + ) + + # verify the optional [gitlab] section and its mandatory options + if flavor == "gitlab": + self._validate_section(self.dict, "gitlab", "gitlab_runner_secret") + def get_flavor(self): flavor_file = self._get_config_file("flavor") -- 2.25.3
On Wed, 2020-04-22 at 15:28 +0200, Erik Skultety wrote:
+ def __init__(self): + path = self._get_config_file("config.toml") + + try: + self.dict = toml.load(path)
I'm not too keen on the name "dict". What about "values"?
+ self._validate() + + except Exception as ex: + raise Exception( + "Missing or invalid config.toml file: {}".format(ex) + )
This will cause lcitool to blow up pretty spectacularly if you haven't created a configuration file already: $ ./lcitool update all all Traceback (most recent call last): File "./lcitool", line 141, in __init__ self.dict = toml.load(path) File "/usr/lib/python3.6/site-packages/toml.py", line 87, in load with io.open(f, encoding='utf-8') as ffile: FileNotFoundError: [Errno 2] No such file or directory: '/home/abologna/.config/lcitool/config.toml' During handling of the above exception, another exception occurred: Traceback (most recent call last): File "./lcitool", line 1043, in <module> Application().run() File "./lcitool", line 395, in __init__ self._config = Config() File "./lcitool", line 146, in __init__ "Missing or invalid config.toml file: {}".format(ex) Exception: Missing or invalid config.toml file: [Errno 2] No such file or directory: '/home/abologna/.config/lcitool/config.toml' This could technically happen already, but that would have been as the result of invalid data being committed to the repository, so it never showed up in practice. You'll need to refactor the script so that exceptions can be caught and pretty-printed even when they occur outside of Application.run().
+ # fill in default options + self._fill_default_options(self.dict)
You don't need to pass the dictionary as argument - it's already part of self at this point.
+ def _fill_default_options(cfg): + flavor = cfg.get("install").get("flavor", "test") + cfg["install"]["flavor"] = flavor + + if flavor == "gitlab": + url = cfg.get("gitlab").get("gitlab_url", "https://gitlab.com") + cfg["gitlab"]["gitlab_url"] = url
It would be neat if, instead of hardcoding our defaults in the script, we did something like cfg.get("gitlab").get("gitlab_url", default_gitlab_url) where default_gitlab_url is obtained from the inventory. Alternatively, and this is probably a cleaner approach, we could make it so lcitool reads config.toml from the source directory, which would be uncommented, first, and then the one in the user's home directory, adding / overriding only those keys that are found: this would ensure, among other things, that the comments in the sample configuration file never get out of sync with the actual defaults, and also I think the amount of boring code necessary in patch 11.
+ @staticmethod + def _validate_section(cfg, section, *args):
I don't like *args very much, and would prefer if this was just a regular list.
+ if cfg.get(section, None) is None: + raise KeyError("Section '[{}]' not found".format(section))
No need for square brackets around the name, you already mentioned that it's a section we're talking about. -- Andrea Bolognani / Red Hat / Virtualization
On Mon, Apr 27, 2020 at 07:47:13PM +0200, Andrea Bolognani wrote:
On Wed, 2020-04-22 at 15:28 +0200, Erik Skultety wrote:
+ def __init__(self): + path = self._get_config_file("config.toml") + + try: + self.dict = toml.load(path)
I'm not too keen on the name "dict". What about "values"?
Sure, works for me :).
+ self._validate() + + except Exception as ex: + raise Exception( + "Missing or invalid config.toml file: {}".format(ex) + )
This will cause lcitool to blow up pretty spectacularly if you haven't created a configuration file already:
$ ./lcitool update all all Traceback (most recent call last): File "./lcitool", line 141, in __init__ self.dict = toml.load(path) File "/usr/lib/python3.6/site-packages/toml.py", line 87, in load with io.open(f, encoding='utf-8') as ffile: FileNotFoundError: [Errno 2] No such file or directory: '/home/abologna/.config/lcitool/config.toml'
During handling of the above exception, another exception occurred:
Traceback (most recent call last): File "./lcitool", line 1043, in <module> Application().run() File "./lcitool", line 395, in __init__ self._config = Config() File "./lcitool", line 146, in __init__ "Missing or invalid config.toml file: {}".format(ex) Exception: Missing or invalid config.toml file: [Errno 2] No such file or directory: '/home/abologna/.config/lcitool/config.toml'
This could technically happen already, but that would have been as the result of invalid data being committed to the repository, so it never showed up in practice.
You'll need to refactor the script so that exceptions can be caught and pretty-printed even when they occur outside of Application.run().
I guess that in this case if config is missing, it should print a nice error, in general, I don't like us re-raising exceptions and wrapping them with Exception - this is not the only occurrence where ^this kind of horrible backtrace is to be seen. In fact, I've never seen or heard of this practice, instead, depending on what type of project it is - if it's a standalone program, you let python fail with its standard exception mechanism as it's most likely a result of user input; if it's a library, you define your own exception class so that projects consuming the library can catch specific errors, e.g. config is invalid and cannot be parsed... I'll fix the issue pointed out, but the exception overhaul is a refactor for another day.
+ # fill in default options + self._fill_default_options(self.dict)
You don't need to pass the dictionary as argument - it's already part of self at this point.
I actually do because it's a static method, it doesn't get a reference to self, and I believe that's the correct design, since it's just a validation helper, it validates a combination of key-value pairs and as such does not modify the instance state and thus does not need nor should have access to the instance itself.
+ def _fill_default_options(cfg): + flavor = cfg.get("install").get("flavor", "test") + cfg["install"]["flavor"] = flavor + + if flavor == "gitlab": + url = cfg.get("gitlab").get("gitlab_url", "https://gitlab.com") + cfg["gitlab"]["gitlab_url"] = url
It would be neat if, instead of hardcoding our defaults in the script, we did something like
cfg.get("gitlab").get("gitlab_url", default_gitlab_url)
where default_gitlab_url is obtained from the inventory.
Alternatively, and this is probably a cleaner approach, we could make it so lcitool reads config.toml from the source directory, which would be uncommented, first, and then the one in the user's home directory, adding / overriding only those keys that are found: this would ensure, among other things, that the comments in the sample configuration file never get out of sync with the actual defaults, and also I think the amount of boring code necessary in patch 11.
Okay, makes sense, I can go with that change I like that.
+ @staticmethod + def _validate_section(cfg, section, *args):
I don't like *args very much, and would prefer if this was just a regular list.
Passing list has a side-effect that args could be accidentally modified, *args prevents that. Is there a specific reason why *args is not suitable as a parameter in this case?
+ if cfg.get(section, None) is None: + raise KeyError("Section '[{}]' not found".format(section))
No need for square brackets around the name, you already mentioned that it's a section we're talking about.
Noted. -- Erik Skultety
On Tue, 2020-04-28 at 10:19 +0200, Erik Skultety wrote:
On Mon, Apr 27, 2020 at 07:47:13PM +0200, Andrea Bolognani wrote:
On Wed, 2020-04-22 at 15:28 +0200, Erik Skultety wrote:
+ except Exception as ex: + raise Exception( + "Missing or invalid config.toml file: {}".format(ex) + )
This will cause lcitool to blow up pretty spectacularly if you haven't created a configuration file already:
$ ./lcitool update all all Traceback (most recent call last): File "./lcitool", line 141, in __init__ self.dict = toml.load(path) File "/usr/lib/python3.6/site-packages/toml.py", line 87, in load with io.open(f, encoding='utf-8') as ffile: FileNotFoundError: [Errno 2] No such file or directory: '/home/abologna/.config/lcitool/config.toml'
During handling of the above exception, another exception occurred:
Traceback (most recent call last): File "./lcitool", line 1043, in <module> Application().run() File "./lcitool", line 395, in __init__ self._config = Config() File "./lcitool", line 146, in __init__ "Missing or invalid config.toml file: {}".format(ex) Exception: Missing or invalid config.toml file: [Errno 2] No such file or directory: '/home/abologna/.config/lcitool/config.toml'
This could technically happen already, but that would have been as the result of invalid data being committed to the repository, so it never showed up in practice.
You'll need to refactor the script so that exceptions can be caught and pretty-printed even when they occur outside of Application.run().
I guess that in this case if config is missing, it should print a nice error, in general, I don't like us re-raising exceptions and wrapping them with Exception - this is not the only occurrence where ^this kind of horrible backtrace is to be seen. In fact, I've never seen or heard of this practice, instead, depending on what type of project it is - if it's a standalone program, you let python fail with its standard exception mechanism as it's most likely a result of user input; if it's a library, you define your own exception class so that projects consuming the library can catch specific errors, e.g. config is invalid and cannot be parsed... I'll fix the issue pointed out, but the exception overhaul is a refactor for another day.
I don't think it's reasonable to get a Python stack trace as a result of some failure scenario that can be easily anticipated, such as the configuration file not being present. In other words, the fact that we see two exceptions above is not the issue: if there was a single exception, I would still consider it a problem. If you have identified other code paths that can result in a Python stack trace instead of a nice, readable error message, then we should address those too.
+ # fill in default options + self._fill_default_options(self.dict)
You don't need to pass the dictionary as argument - it's already part of self at this point.
I actually do because it's a static method, it doesn't get a reference to self, and I believe that's the correct design, since it's just a validation helper, it validates a combination of key-value pairs and as such does not modify the instance state and thus does not need nor should have access to the instance itself.
I don't really see the point in making it static, because it's a method that's internal to the class and will only ever be used to validate a dictionary that's part of the object. The same consideration applies to _validate_section(), especially since _validate() - which is the only caller for it - actually uses self.dict instead of being static itself.
+ @staticmethod + def _validate_section(cfg, section, *args):
I don't like *args very much, and would prefer if this was just a regular list.
Passing list has a side-effect that args could be accidentally modified, *args prevents that. Is there a specific reason why *args is not suitable as a parameter in this case?
I just don't like it very much, because when looking at the call site I find something like do_something(with, these, values) not as clear as something like do_something(with, [these, values]) because in the latter it's obvious that the last two values are going to be used for the same purpose, whereas in the former they could be completely unrelated for all you know. Values being modified by mistake is kind of a moot point in Python anyway: in this specific case, I would be more concerned about the dictionary being modified by a function that is supposed to only validate it, but I don't think there is a good way to prevent that from happening, and with that off the table worrying about some other argument that's never even going to be used again feels a bit silly. -- Andrea Bolognani / Red Hat / Virtualization
On Wed, 2020-04-22 at 15:28 +0200, Erik Skultety wrote:
+ @staticmethod + def _fill_default_options(cfg): + flavor = cfg.get("install").get("flavor", "test") + cfg["install"]["flavor"] = flavor + + if flavor == "gitlab": + url = cfg.get("gitlab").get("gitlab_url", "https://gitlab.com") + cfg["gitlab"]["gitlab_url"] = url
The key should be "url" here...
+ def _validate(self): + + # verify the [install] section and its mandatory options + self._validate_section(self.dict, "install", "root_password") + + # we need to check flavor here, because later validations depend on it + flavor = self.dict.get("install").get("flavor", "test") + if flavor not in ["test", "jenkins", "gitlab"]: + raise ValueError( + "Invalid value '{}' for 'install.flavor'".format(flavor) + ) + + # verify the optional [gitlab] section and its mandatory options + if flavor == "gitlab": + self._validate_section(self.dict, "gitlab", "gitlab_runner_secret")
... and "runner_secret" here. -- Andrea Bolognani / Red Hat / Virtualization
We can now access this value directly in the config dictionary. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- guests/lcitool | 32 +++----------------------------- 1 file changed, 3 insertions(+), 29 deletions(-) diff --git a/guests/lcitool b/guests/lcitool index f08bd98..a385c15 100755 --- a/guests/lcitool +++ b/guests/lcitool @@ -206,37 +206,12 @@ class Config: if flavor == "gitlab": self._validate_section(self.dict, "gitlab", "gitlab_runner_secret") - def get_flavor(self): - flavor_file = self._get_config_file("flavor") - - try: - with open(flavor_file, "r") as infile: - flavor = infile.readline().strip() - except Exception: - # If the flavor has not been configured, we choose the default - # and store it on disk to ensure consistent behavior from now on - flavor = "test" - try: - with open(flavor_file, "w") as infile: - infile.write("{}\n".format(flavor)) - except Exception as ex: - raise Exception( - "Can't write flavor file ({}): {}".format( - flavor_file, ex - ) - ) - - if flavor not in ["test", "jenkins", "gitlab"]: - raise Exception("Invalid flavor '{}'".format(flavor)) - - return flavor - def get_vault_password_file(self): vault_pass_file = None # The vault password is only needed for the jenkins flavor, but in # that case we want to make sure there's *something* in there - if self.get_flavor() == "jenkins": + if self.dict["install"]["flavor"] == "jenkins": vault_pass_file = self._get_config_file("vault-password") try: @@ -535,7 +510,6 @@ class Application: def _execute_playbook(self, playbook, hosts, projects, git_revision): base = Util.get_base() - flavor = self._config.get_flavor() vault_pass_file = self._config.get_vault_password_file() root_pass_file = self._config.get_root_password_file() gitlab_url_file = self._config.get_gitlab_url_file() @@ -568,7 +542,7 @@ class Application: "base": base, "playbook_base": playbook_base, "root_password_file": root_pass_file, - "flavor": flavor, + "flavor": self._config.dict["install"]["flavor"], "selected_projects": selected_projects, "git_remote": git_remote, "git_branch": git_branch, @@ -617,7 +591,7 @@ class Application: def _action_install(self, args): base = Util.get_base() - flavor = self._config.get_flavor() + flavor = self._config.dict["install"]["flavor"] for host in self._inventory.expand_pattern(args.hosts): facts = self._inventory.get_facts(host) -- 2.25.3
On Wed, 2020-04-22 at 15:28 +0200, Erik Skultety wrote:
@@ -617,7 +591,7 @@ class Application: def _action_install(self, args): base = Util.get_base()
- flavor = self._config.get_flavor() + flavor = self._config.dict["install"]["flavor"]
You can remove this assignment and just use the value directly below, like you've done in _execute_playbook(). There are also two uses of get_flavor() remaining after this patch: get_gitlab_runner_token_file() get_gitlab_url_file() -- Andrea Bolognani / Red Hat / Virtualization
On Tue, Apr 28, 2020 at 09:34:54AM +0200, Andrea Bolognani wrote:
On Wed, 2020-04-22 at 15:28 +0200, Erik Skultety wrote:
@@ -617,7 +591,7 @@ class Application: def _action_install(self, args): base = Util.get_base()
- flavor = self._config.get_flavor() + flavor = self._config.dict["install"]["flavor"]
You can remove this assignment and just use the value directly below, like you've done in _execute_playbook().
Okay, missed that one.
There are also two uses of get_flavor() remaining after this patch:
get_gitlab_runner_token_file() get_gitlab_url_file()
Yeah, does it matter since I'm dropping those 2 patches later? -- Erik Skultety
On Tue, 2020-04-28 at 10:20 +0200, Erik Skultety wrote:
On Tue, Apr 28, 2020 at 09:34:54AM +0200, Andrea Bolognani wrote:
There are also two uses of get_flavor() remaining after this patch:
get_gitlab_runner_token_file() get_gitlab_url_file()
Yeah, does it matter since I'm dropping those 2 patches later?
In the grand scheme of things? No, not really :) But removing all calls to a function when you remove the function itself is just the right thing to do. -- Andrea Bolognani / Red Hat / Virtualization
We can now access this value directly in the config dictionary. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- guests/lcitool | 19 +------------------ guests/playbooks/update/tasks/users.yml | 2 +- 2 files changed, 2 insertions(+), 19 deletions(-) diff --git a/guests/lcitool b/guests/lcitool index a385c15..6c905c2 100755 --- a/guests/lcitool +++ b/guests/lcitool @@ -227,22 +227,6 @@ class Config: return vault_pass_file - def get_root_password_file(self): - root_pass_file = self._get_config_file("root-password") - - try: - with open(root_pass_file, "r") as infile: - if not infile.readline().strip(): - raise ValueError - except Exception as ex: - raise Exception( - "Missing or invalid root password file ({}): {}".format( - root_pass_file, ex - ) - ) - - return root_pass_file - def get_gitlab_runner_token_file(self): if self.get_flavor() != "gitlab": return None @@ -511,7 +495,6 @@ class Application: base = Util.get_base() vault_pass_file = self._config.get_vault_password_file() - root_pass_file = self._config.get_root_password_file() gitlab_url_file = self._config.get_gitlab_url_file() gitlab_runner_token_file = self._config.get_gitlab_runner_token_file() @@ -541,7 +524,7 @@ class Application: extra_vars = { "base": base, "playbook_base": playbook_base, - "root_password_file": root_pass_file, + "root_password": self._config.dict["install"]["root_password"], "flavor": self._config.dict["install"]["flavor"], "selected_projects": selected_projects, "git_remote": git_remote, diff --git a/guests/playbooks/update/tasks/users.yml b/guests/playbooks/update/tasks/users.yml index 28ee96a..8744290 100644 --- a/guests/playbooks/update/tasks/users.yml +++ b/guests/playbooks/update/tasks/users.yml @@ -2,7 +2,7 @@ - name: 'root: Set password' user: name: root - password: '{{ lookup("file", root_password_file)|password_hash("sha512") }}' + password: '{{ root_password|password_hash("sha512") }}' - name: 'root: Configure ssh access' authorized_key: -- 2.25.3
On Wed, 2020-04-22 at 15:28 +0200, Erik Skultety wrote:
We can now access this value directly in the config dictionary.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- guests/lcitool | 19 +------------------ guests/playbooks/update/tasks/users.yml | 2 +- 2 files changed, 2 insertions(+), 19 deletions(-)
Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
We can now access the values directly in the config dictionary. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- guests/lcitool | 44 ++---------------------- guests/playbooks/update/tasks/gitlab.yml | 2 -- 2 files changed, 2 insertions(+), 44 deletions(-) diff --git a/guests/lcitool b/guests/lcitool index 6c905c2..f2b4d44 100755 --- a/guests/lcitool +++ b/guests/lcitool @@ -227,44 +227,6 @@ class Config: return vault_pass_file - def get_gitlab_runner_token_file(self): - if self.get_flavor() != "gitlab": - return None - - gitlab_runner_token_file = self._get_config_file("gitlab-runner-token") - - try: - with open(gitlab_runner_token_file, "r") as infile: - if not infile.readline().strip(): - raise ValueError - except Exception as ex: - raise Exception( - "Missing or invalid GitLab runner token file ({}): {}".format( - gitlab_runner_token_file, ex - ) - ) - - return gitlab_runner_token_file - - def get_gitlab_url_file(self): - if self.get_flavor() != "gitlab": - return None - - gitlab_url_file = self._get_config_file("gitlab-url") - - try: - with open(gitlab_url_file, "r") as infile: - if not infile.readline().strip(): - raise ValueError - except Exception as ex: - raise Exception( - "Missing or invalid GitLab url file ({}): {}".format( - gitlab_url_file, ex - ) - ) - - return gitlab_url_file - class Inventory: @@ -495,8 +457,6 @@ class Application: base = Util.get_base() vault_pass_file = self._config.get_vault_password_file() - gitlab_url_file = self._config.get_gitlab_url_file() - gitlab_runner_token_file = self._config.get_gitlab_runner_token_file() ansible_hosts = ",".join(self._inventory.expand_pattern(hosts)) selected_projects = self._projects.expand_pattern(projects) @@ -529,8 +489,8 @@ class Application: "selected_projects": selected_projects, "git_remote": git_remote, "git_branch": git_branch, - "gitlab_url_file": gitlab_url_file, - "gitlab_runner_token_file": gitlab_runner_token_file, + "gitlab_url": self._config.dict["gitlab"]["url"], + "gitlab_runner_secret": self._config.dict["gitlab"]["token"], } json.dump(extra_vars, fp) diff --git a/guests/playbooks/update/tasks/gitlab.yml b/guests/playbooks/update/tasks/gitlab.yml index 7691442..c13ca22 100644 --- a/guests/playbooks/update/tasks/gitlab.yml +++ b/guests/playbooks/update/tasks/gitlab.yml @@ -1,8 +1,6 @@ --- - name: Define gitlab-related facts set_fact: - gitlab_url: '{{ lookup("file", gitlab_url_file) }}' - gitlab_runner_secret: '{{ lookup("file", gitlab_runner_token_file) }}' gitlab_runner_download_url: https://gitlab-runner-downloads.s3.amazonaws.com/latest/binaries/gitlab-runner-{{ ansible_system|lower }}-amd64 gitlab_runner_config_dir: '/etc/gitlab-runner' -- 2.25.3
On Wed, 2020-04-22 at 15:28 +0200, Erik Skultety wrote:
@@ -529,8 +489,8 @@ class Application: "selected_projects": selected_projects, "git_remote": git_remote, "git_branch": git_branch, - "gitlab_url_file": gitlab_url_file, - "gitlab_runner_token_file": gitlab_runner_token_file, + "gitlab_url": self._config.dict["gitlab"]["url"], + "gitlab_runner_secret": self._config.dict["gitlab"]["token"],
The second key is "runner_secret", not "token". With that fixed, Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
Rather than putting all the options to the extra_vars JSON, use the update method to extend the source dictionary with options coming from the lcitool config file. By doing this split, we know which options are hard-coded and which come from external sources. The main reason for this change though is that some sections/members in the config file are optional (thus may be missing in the config dictionary) and we'd risk the KeyError exception if we tried to access them directly when filling out the extra_vars JSON. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- guests/lcitool | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/guests/lcitool b/guests/lcitool index f2b4d44..4cb6e69 100755 --- a/guests/lcitool +++ b/guests/lcitool @@ -481,17 +481,22 @@ class Application: extra_vars_path = os.path.join(tempdir.name, 'extra_vars.json') with open(extra_vars_path, 'w') as fp: + + # start with generic items not coming from the config extra_vars = { "base": base, "playbook_base": playbook_base, - "root_password": self._config.dict["install"]["root_password"], - "flavor": self._config.dict["install"]["flavor"], "selected_projects": selected_projects, "git_remote": git_remote, "git_branch": git_branch, - "gitlab_url": self._config.dict["gitlab"]["url"], - "gitlab_runner_secret": self._config.dict["gitlab"]["token"], } + + # now add the config vars + extra_vars.update(self._config.dict["install"]) + + if extra_vars["flavor"] == "gitlab": + extra_vars.update(self._config.dict["gitlab"]) + json.dump(extra_vars, fp) ansible_playbook = distutils.spawn.find_executable("ansible-playbook") -- 2.25.3
On Wed, 2020-04-22 at 15:28 +0200, Erik Skultety wrote:
+ # start with generic items not coming from the config extra_vars = { "base": base, "playbook_base": playbook_base, - "root_password": self._config.dict["install"]["root_password"], - "flavor": self._config.dict["install"]["flavor"], "selected_projects": selected_projects, "git_remote": git_remote, "git_branch": git_branch, - "gitlab_url": self._config.dict["gitlab"]["url"], - "gitlab_runner_secret": self._config.dict["gitlab"]["token"], } + + # now add the config vars + extra_vars.update(self._config.dict["install"]) + + if extra_vars["flavor"] == "gitlab": + extra_vars.update(self._config.dict["gitlab"]) +
Mh, I don't think I like this. First of all, it forces you to replicate the "if gitlab" conditional in one additional place, and most importantly it completely flattens the resulting dictionary, eg. config.dict["gitlab"]["url"] -> extra_vars["url"] Both of these issues will disappear if you follow the suggestion I made in 6/13 and load the default config.toml first: at that point, you can unconditionally copy entire sections from the TOML into extra_vars, and later in the playbook you can simply refer to them along the lines of - shell: gitlab-runner {{ gitlab.url }} {{ gitlab.runner_secret }} when: - install.flavor == "gitlab" which is much nicer. In fact, we should use the same approach for values in the inventory, eg. os_name: "Debian" os_version: "10" should become os: name: "Debian" version: "10" Given that both Python and Ansible support proper dictionaries, we might very well use them instead of using prefix notation to emulate a worse version of them :) -- Andrea Bolognani / Red Hat / Virtualization
Looking into the future where we're able to generate cloudinit images, we'll need to configure some of the install options which is currently not possible without editing the install.yml group vars file within the repository. That is suboptimal, so let's move the install options to the global config under the '[install]' section so that further tweaking is possible (but explicitly discouraged at the same time). Signed-off-by: Erik Skultety <eskultet@redhat.com> --- config.toml | 17 +++++++++++++++++ guests/lcitool | 27 +++++++++++++++++++++++++++ 2 files changed, 44 insertions(+) diff --git a/config.toml b/config.toml index 5bb3725..9bb1b7f 100644 --- a/config.toml +++ b/config.toml @@ -16,6 +16,23 @@ # instead. (mandatory) #root_password = "" +# Settings mapping to the virt-install options - see virt-install(1). +# It is strongly recommended that you keep the following at their default +# values to produce machines which conform to the upstream libvirt standard, +# unless you have a reason to do otherwise. +# +# Sizes are expressed in GiB. +# +#virt_type = "kvm" +#arch = "x86_64" +#machine = "pc" +#cpu_model = "host-passthrough" +#vcpus = 2 +#memory_size = 2 +#disk_size = 15 +#storage_pool = "default" +#network = "default" + [gitlab] # GitLab runner agent registration options, applies only if flavor == 'gitlab'. diff --git a/guests/lcitool b/guests/lcitool index 4cb6e69..36320ab 100755 --- a/guests/lcitool +++ b/guests/lcitool @@ -175,6 +175,33 @@ class Config: flavor = cfg.get("install").get("flavor", "test") cfg["install"]["flavor"] = flavor + virt_type = cfg.get("install").get("virt_type", "kvm") + cfg["install"]["virt_type"] = virt_type + + arch = cfg.get("install").get("arch", "x86_64") + cfg["install"]["arch"] = arch + + machine = cfg.get("install").get("machine", "pc") + cfg["install"]["machine"] = machine + + cpu_model = cfg.get("install").get("cpu_model", "host-passthrough") + cfg["install"]["cpu_model"] = cpu_model + + vcpus = cfg.get("install").get("vcpus", 2) + cfg["install"]["vcpus"] = vcpus + + memory_size = cfg.get("install").get("memory_size", 2) + cfg["install"]["memory_size"] = memory_size + + disk_size = cfg.get("install").get("disk_size", 15) + cfg["install"]["disk_size"] = disk_size + + storage_pool = cfg.get("install").get("storage_pool", "default") + cfg["install"]["storage_pool"] = storage_pool + + network = cfg.get("install").get("network", "default") + cfg["install"]["network"] = network + if flavor == "gitlab": url = cfg.get("gitlab").get("gitlab_url", "https://gitlab.com") cfg["gitlab"]["gitlab_url"] = url -- 2.25.3
On Wed, 2020-04-22 at 15:28 +0200, Erik Skultety wrote:
+++ b/guests/lcitool @@ -175,6 +175,33 @@ class Config: flavor = cfg.get("install").get("flavor", "test") cfg["install"]["flavor"] = flavor
+ virt_type = cfg.get("install").get("virt_type", "kvm") + cfg["install"]["virt_type"] = virt_type + + arch = cfg.get("install").get("arch", "x86_64") + cfg["install"]["arch"] = arch + + machine = cfg.get("install").get("machine", "pc") + cfg["install"]["machine"] = machine + + cpu_model = cfg.get("install").get("cpu_model", "host-passthrough") + cfg["install"]["cpu_model"] = cpu_model + + vcpus = cfg.get("install").get("vcpus", 2) + cfg["install"]["vcpus"] = vcpus + + memory_size = cfg.get("install").get("memory_size", 2) + cfg["install"]["memory_size"] = memory_size + + disk_size = cfg.get("install").get("disk_size", 15) + cfg["install"]["disk_size"] = disk_size + + storage_pool = cfg.get("install").get("storage_pool", "default") + cfg["install"]["storage_pool"] = storage_pool + + network = cfg.get("install").get("network", "default") + cfg["install"]["network"] = network
All this code will go away once you start obtaining default from the in-repo config.toml. For the remaining changes, Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
Now that we have the corresponding options available in the config, let's start using them. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- guests/lcitool | 30 +++++++++++------------------- 1 file changed, 11 insertions(+), 19 deletions(-) diff --git a/guests/lcitool b/guests/lcitool index 36320ab..9a9256f 100755 --- a/guests/lcitool +++ b/guests/lcitool @@ -565,25 +565,17 @@ class Application: def _action_install(self, args): base = Util.get_base() - - flavor = self._config.dict["install"]["flavor"] + cfg = self._config.dict["install"] for host in self._inventory.expand_pattern(args.hosts): facts = self._inventory.get_facts(host) - # Both memory size and disk size are stored as GiB in the - # inventory, but virt-install expects the disk size in GiB - # and the memory size in *MiB*, so perform conversion here - memory_arg = str(int(facts["install_memory_size"]) * 1024) - - vcpus_arg = str(facts["install_vcpus"]) - disk_arg = "size={},pool={},bus=virtio".format( - facts["install_disk_size"], - facts["install_storage_pool"], + cfg["disk_size"], + cfg["storage_pool"], ) network_arg = "network={},model=virtio".format( - facts["install_network"], + cfg["network"], ) # Different operating systems require different configuration @@ -643,12 +635,12 @@ class Application: virt_install, "--name", host, "--location", facts["install_url"], - "--virt-type", facts["install_virt_type"], - "--arch", facts["install_arch"], - "--machine", facts["install_machine"], - "--cpu", facts["install_cpu_model"], - "--vcpus", vcpus_arg, - "--memory", memory_arg, + "--virt-type", cfg["virt_type"], + "--arch", cfg["arch"], + "--machine", cfg["machine"], + "--cpu", cfg["cpu_model"], + "--vcpus", str(cfg["vcpus"]), + "--memory", str(cfg["memory_size"] * 1024), "--disk", disk_arg, "--network", network_arg, "--graphics", "none", @@ -663,7 +655,7 @@ class Application: cmd.append("--noautoconsole") # Only configure autostart for the guest for the jenkins flavor - if flavor == "jenkins": + if cfg["flavor"] == "jenkins": cmd += ["--autostart"] try: -- 2.25.3
On Wed, 2020-04-22 at 15:28 +0200, Erik Skultety wrote:
Now that we have the corresponding options available in the config, let's start using them.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- guests/lcitool | 30 +++++++++++------------------- 1 file changed, 11 insertions(+), 19 deletions(-)
At this point the file group_vars/all/install.yml is no longer used and you should drop it. I also don't think it makes sense to split this changes from the ones in 11/13, so please squash the two patches together. With that done, Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- guests/README.markdown | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/guests/README.markdown b/guests/README.markdown index 7dbefed..d9afbd3 100644 --- a/guests/README.markdown +++ b/guests/README.markdown @@ -64,13 +64,16 @@ branches out of those with Host setup ---------- -Ansible and `virt-install` need to be available on the host. +`ansible`, `python3-toml` and `virt-install` need to be available on the host, +the former two can be either installed system-wide using your package manager +or using by `pip` (see the provided requirements.txt file). The latter can only +be installed with your package manager as `virt-install` is not distributed via +PyPi. -Before you can start bringing up guests, you'll have to store your -site-specific root password in the `~/.config/lcitool/root-password` file. -This password will only be necessary for serial console access in case -something goes horribly wrong; for day to day operations, SSH key -authentication will be used instead. +Before you can start bringing up guests, you need to create (ideally by +copying the `config.toml` template) ~/.config/lcitool/config.toml and set at +least the options marked as "(mandatory)" depending on the flavor (`test`, +`jenkins`, `gitlab`) you wish to use with your machines. Ansible expects to be able to connect to the guests by name: installing and enabling the [libvirt NSS plugin](https://wiki.libvirt.org/page/NSS_module) @@ -123,8 +126,8 @@ Jenkins CI use -------------- You'll need to configure `lcitool` to use the `jenkins` flavor for -guests: to do so, just write `jenkins` in the `~/.config/lcitool/flavor` -file. +guests. To do so, simply set the flavor to 'jenkins' in +`~/.config/lcitool/config.toml`. Once a guest has been prepared, you'll be able to log in as root either via SSH (your public key will have been authorized) or on the serial -- 2.25.3
On Wed, 2020-04-22 at 15:28 +0200, Erik Skultety wrote:
+`ansible`, `python3-toml` and `virt-install` need to be available on the host, +the former two can be either installed system-wide using your package manager +or using by `pip` (see the provided requirements.txt file).
s/using by/using/ You should also add backticks around requirements.txt.
The latter can only +be installed with your package manager as `virt-install` is not distributed via +PyPi.
s/PyPi/PyPI/
+Before you can start bringing up guests, you need to create (ideally by +copying the `config.toml` template) ~/.config/lcitool/config.toml and set at
Backticks around /.config/lcitool/config.toml. With these nits fixed, Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
On Wed, 2020-04-22 at 15:28 +0200, Erik Skultety wrote:
This series is trying to consolidate the number of config files we currently recognize under ~/.config/lcitool to a single global TOML config file. Thanks to this effort we can expose more seetings than we previously could which will come handy in terms of generating cloudinit images for OpenStack.
Patches 1-4 patches are just a little extra - not heavily related to the series See patch 5/13 why TOML was selected.
First of all, thanks for tackling this! It's something that we've sorely needed for a while now. Now, I know I'm the one who suggested TOML in the first place... But looking at the series now I can't help but think, why not YAML? O:-) Since we're using it extensively already due to Ansible, I think it would make sense to use it for the configuration file as well. It's easy enough to consume for a human, and we wouldn't need to drag in an additional dependency. I also believe, perhaps naively, that adapting your code to use YAML instead of TOML wouldn't require much work - from the Python point of view, you basically end up with a dictionary in both cases. Feel free to argue against this suggestion! For example, if you agree with it in principle but feel like it's unfair of me to ask you to rework your code, I'm happy to port it myself. As for the rest of the series - I've only skimmed it so far, but I did not spot anything horribly wrong with it at first glance. I'll provide an actual, detailed review next week. -- Andrea Bolognani / Red Hat / Virtualization
On Thu, Apr 23, 2020 at 06:46:09PM +0200, Andrea Bolognani wrote:
On Wed, 2020-04-22 at 15:28 +0200, Erik Skultety wrote:
This series is trying to consolidate the number of config files we currently recognize under ~/.config/lcitool to a single global TOML config file. Thanks to this effort we can expose more seetings than we previously could which will come handy in terms of generating cloudinit images for OpenStack.
Patches 1-4 patches are just a little extra - not heavily related to the series See patch 5/13 why TOML was selected.
First of all, thanks for tackling this! It's something that we've sorely needed for a while now.
Now, I know I'm the one who suggested TOML in the first place... But looking at the series now I can't help but think, why not YAML? O:-)
To be honest, even before you originally mentioned TOML, I myself had INI in mind, so then I thought, yeah, why not go with TOML then, it's similar and more powerful. I did some comparison of several formats, because like you say, with YAML we'd be more close to Ansible and I was on the cusp of choosing between YAML and TOML and I felt like TOML was still more readable and expressive in terms of simple configuration (and that's what Linux users are IMO used to from INI). I was never a big fan of YAML really and when the dictionaries and list happen to intertwine and nest a lot, YAML looses its readability quite quickly IMO, which I never felt with TOML, but it's fair to say that my TOML experience is very limited. That said, I don't expect us to have such a massive config, so that multiple levels of YAML nesting will be necessary :).
Since we're using it extensively already due to Ansible, I think it would make sense to use it for the configuration file as well. It's easy enough to consume for a human, and we wouldn't need to drag in an additional dependency. I also believe, perhaps naively, that adapting your code to use YAML instead of TOML wouldn't require much work - from the Python point of view, you basically end up with a dictionary in both cases.
Feel free to argue against this suggestion! For example, if you agree with it in principle but feel like it's unfair of me to ask you to rework your code, I'm happy to port it myself.
I'd still prefer TOML, but I don't really have a compelling reason to argue against YAML other than readability which I already admitted to be just a matter of taste. Now on a more serious note, I'll wait for your detailed review and then rework it in YAML in vX.
As for the rest of the series - I've only skimmed it so far, but I did not spot anything horribly wrong with it at first glance. I'll provide an actual, detailed review next week.
Okay, thanks. -- Erik Skultety
On Fri, 2020-04-24 at 08:47 +0200, Erik Skultety wrote:
On Thu, Apr 23, 2020 at 06:46:09PM +0200, Andrea Bolognani wrote:
Now, I know I'm the one who suggested TOML in the first place... But looking at the series now I can't help but think, why not YAML? O:-)
To be honest, even before you originally mentioned TOML, I myself had INI in mind, so then I thought, yeah, why not go with TOML then, it's similar and more powerful. I did some comparison of several formats, because like you say, with YAML we'd be more close to Ansible and I was on the cusp of choosing between YAML and TOML and I felt like TOML was still more readable and expressive in terms of simple configuration (and that's what Linux users are IMO used to from INI).
Are you sure you didn't mean s/Linux/Windows/ here? ;)
I'd still prefer TOML, but I don't really have a compelling reason to argue against YAML other than readability which I already admitted to be just a matter of taste. Now on a more serious note, I'll wait for your detailed review and then rework it in YAML in vX.
Eh, you know what, whatever. YAML is fine, TOML is fine, bringing in an additional Python module is not a big deal. Let's fix actual issues (if there are any) and skip at least some of the bikeshedding. -- Andrea Bolognani / Red Hat / Virtualization
participants (2)
-
Andrea Bolognani -
Erik Skultety