8:28 a.m.
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
8:28 a.m.
New subject: [libvirt-ci PATCH 01/13] requirements: Introduce a requirements.txt file
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(a)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
9:02 a.m.
New subject: [libvirt-ci PATCH 01/13] requirements: Introduce a requirements.txt file
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(a)redhat.com>
--
Andrea Bolognani / Red Hat / Virtualization
8:28 a.m.
New subject: [libvirt-ci PATCH 02/13] lcitool: Decrease the indent when creating a tempdir for initrd injection
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(a)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
9:27 a.m.
New subject: [libvirt-ci PATCH 02/13] lcitool: Decrease the indent when creating a tempdir for initrd injection
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(a)redhat.com>
---
guests/lcitool | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
--
Andrea Bolognani / Red Hat / Virtualization
8:28 a.m.
New subject: [libvirt-ci PATCH 03/13] lcitool: Prefer tempfile's native wrappers over low level primitives
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(a)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
9:42 a.m.
New subject: [libvirt-ci PATCH 03/13] lcitool: Prefer tempfile's native wrappers over low level primitives
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(a)redhat.com>
---
guests/lcitool | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
--
Andrea Bolognani / Red Hat / Virtualization
8:28 a.m.
New subject: [libvirt-ci PATCH 04/13] lcitool: Use a temporary JSON file to pass extra variables
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(a)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
11:25 a.m.
New subject: [libvirt-ci PATCH 04/13] lcitool: Use a temporary JSON file to pass extra variables
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(a)redhat.com>
--
Andrea Bolognani / Red Hat / Virtualization
8:28 a.m.
New subject: [libvirt-ci PATCH 05/13] config: Introduce a new global config.toml configuration file
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(a)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
11:31 a.m.
New subject: [libvirt-ci PATCH 05/13] config: Introduce a new global config.toml configuration file
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(a)redhat.com>
--
Andrea Bolognani / Red Hat / Virtualization
2:39 a.m.
New subject: [libvirt-ci PATCH 05/13] config: Introduce a new global config.toml configuration file
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
2:51 a.m.
New subject: [libvirt-ci PATCH 05/13] config: Introduce a new global config.toml configuration file
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
8:28 a.m.
New subject: [libvirt-ci PATCH 06/13] lcitool: Introduce methods to load and validate the TOML config
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(a)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
12:47 p.m.
New subject: [libvirt-ci PATCH 06/13] lcitool: Introduce methods to load and validate the TOML config
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
3:19 a.m.
New subject: [libvirt-ci PATCH 06/13] lcitool: Introduce methods to load and validate the TOML config
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
4:23 a.m.
New subject: [libvirt-ci PATCH 06/13] lcitool: Introduce methods to load and validate the TOML config
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
2:41 a.m.
New subject: [libvirt-ci PATCH 06/13] lcitool: Introduce methods to load and validate the TOML config
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
8:28 a.m.
New subject: [libvirt-ci PATCH 07/13] lcitool: Drop the get_flavor() method
We can now access this value directly in the config dictionary.
Signed-off-by: Erik Skultety <eskultet(a)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
2:34 a.m.
New subject: [libvirt-ci PATCH 07/13] lcitool: Drop the get_flavor() method
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
3:20 a.m.
New subject: [libvirt-ci PATCH 07/13] lcitool: Drop the get_flavor() method
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
4:25 a.m.
New subject: [libvirt-ci PATCH 07/13] lcitool: Drop the get_flavor() method
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
8:28 a.m.
New subject: [libvirt-ci PATCH 08/13] lcitool: Drop the get_root_password_file() method
We can now access this value directly in the config dictionary.
Signed-off-by: Erik Skultety <eskultet(a)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
2:36 a.m.
New subject: [libvirt-ci PATCH 08/13] lcitool: Drop the get_root_password_file() method
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(a)redhat.com>
---
guests/lcitool | 19 +------------------
guests/playbooks/update/tasks/users.yml | 2 +-
2 files changed, 2 insertions(+), 19 deletions(-)
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
--
Andrea Bolognani / Red Hat / Virtualization
8:28 a.m.
New subject: [libvirt-ci PATCH 09/13] lcitool: Drop the gitlab-related getter methods
We can now access the values directly in the config dictionary.
Signed-off-by: Erik Skultety <eskultet(a)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-r...
ansible_system|lower }}-amd64
gitlab_runner_config_dir: '/etc/gitlab-runner'
--
2.25.3
2:43 a.m.
New subject: [libvirt-ci PATCH 09/13] lcitool: Drop the gitlab-related getter methods
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(a)redhat.com>
--
Andrea Bolognani / Red Hat / Virtualization
8:28 a.m.
New subject: [libvirt-ci PATCH 10/13] lcitool: Use d.update() on extra_vars for options coming from the config
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(a)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
3:24 a.m.
New subject: [libvirt-ci PATCH 10/13] lcitool: Use d.update() on extra_vars for options coming from the config
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
8:28 a.m.
New subject: [libvirt-ci PATCH 11/13] config: Move the virt-install settings from install.yml to the config
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(a)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
3:30 a.m.
New subject: [libvirt-ci PATCH 11/13] config: Move the virt-install settings from install.yml to the config
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(a)redhat.com>
--
Andrea Bolognani / Red Hat / Virtualization
8:28 a.m.
New subject: [libvirt-ci PATCH 12/13] lcitool: Use the config install options rather than install facts
Now that we have the corresponding options available in the config,
let's start using them.
Signed-off-by: Erik Skultety <eskultet(a)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
3:34 a.m.
New subject: [libvirt-ci PATCH 12/13] lcitool: Use the config install options rather than install facts
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(a)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(a)redhat.com>
--
Andrea Bolognani / Red Hat / Virtualization
8:28 a.m.
New subject: [libvirt-ci PATCH 13/13] guests: README: Document the existence and usage of config.toml
Signed-off-by: Erik Skultety <eskultet(a)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
3:44 a.m.
New subject: [libvirt-ci PATCH 13/13] guests: README: Document the existence and usage of config.toml
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(a)redhat.com>
--
Andrea Bolognani / Red Hat / Virtualization
11:46 a.m.
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
1:47 a.m.
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
9 a.m.
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
1669
days inactive
1675
days old
36 comments
2 participants
participants (2)
-
Andrea Bolognani -
Erik Skultety