7:05 a.m.
This series is trying to consolidate the number of config files we currently
recognize under ~/.config/lcitool to a single global YAML 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
Patches 1-3 (ACKed) only cosmetical changes (like moving requirements.txt)
Since RFC:
- replaced TOML with YAML which simplified some aspects of the code, thanks
Andrea
- instead of hardcoding the default values, the config within the repo is used
as a template and overriden with user-selected options
Erik Skultety (12):
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.yaml configuration file
lcitool: Introduce methods to load and validate the YAML config
lcitool: Update the config values with internal playbook settings
lcitool: Drop the get_flavor() method
lcitool: Drop the get_root_password_file() method
lcitool: Drop the gitlab-related getter methods
config: Move the virt-install settings from install.yml to the config
guests: README: Document the existence and usage of config.toml
guests/README.markdown | 18 +-
guests/config.yaml | 40 +++++
guests/lcitool | 192 +++++++++-------------
guests/playbooks/build/main.yml | 2 +-
guests/playbooks/update/main.yml | 6 +-
guests/playbooks/update/tasks/gitlab.yml | 4 +-
guests/playbooks/update/tasks/kludges.yml | 2 +-
guests/playbooks/update/tasks/users.yml | 42 ++---
guests/requirements.txt | 2 +
9 files changed, 159 insertions(+), 149 deletions(-)
create mode 100644 guests/config.yaml
create mode 100644 guests/requirements.txt
--
2.25.3
7:05 a.m.
New subject: [libvirt-ci PATCH 01/12] 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>
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
---
guests/requirements.txt | 2 ++
1 file changed, 2 insertions(+)
create mode 100644 guests/requirements.txt
diff --git a/guests/requirements.txt b/guests/requirements.txt
new file mode 100644
index 0000000..900cfd6
--- /dev/null
+++ b/guests/requirements.txt
@@ -0,0 +1,2 @@
+ansible
+jenkins-job-builder
--
2.25.3
7:05 a.m.
New subject: [libvirt-ci PATCH 02/12] 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>
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
---
guests/lcitool | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/guests/lcitool b/guests/lcitool
index ab3b95f..c8d0d9a 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
7:05 a.m.
New subject: [libvirt-ci PATCH 03/12] 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>
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
---
guests/lcitool | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/guests/lcitool b/guests/lcitool
index c8d0d9a..759e604 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(prefix="lcitool")
+ 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
7:05 a.m.
New subject: [libvirt-ci PATCH 04/12] 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 | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/guests/lcitool b/guests/lcitool
index 759e604..2dca8b5 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -504,11 +504,13 @@ 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 = json.dumps({
+ extra_vars_path = os.path.join(tempdir.name, "extra_vars.json")
+ extra_vars = {
"base": base,
"playbook_base": playbook_base,
"root_password_file": root_pass_file,
@@ -518,7 +520,10 @@ class Application:
"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:
+ 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
9:25 a.m.
New subject: [libvirt-ci PATCH 04/12] lcitool: Use a temporary JSON file to pass extra variables
On Wed, 2020-05-06 at 14:05 +0200, Erik Skultety wrote:
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 = json.dumps({
+ extra_vars_path = os.path.join(tempdir.name, "extra_vars.json")
+ extra_vars = {
Leave the empty line between all the various paths and extra_vars.
With that changed,
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
--
Andrea Bolognani / Red Hat / Virtualization
7:05 a.m.
New subject: [libvirt-ci PATCH 05/12] config: Introduce a new global config.yaml 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 YAML format has been chosen simply because it's the native data
format in Ansible and thus plays very nicely when accessing variables
within Ansible playbooks.
Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
---
guests/config.yaml | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
create mode 100644 guests/config.yaml
diff --git a/guests/config.yaml b/guests/config.yaml
new file mode 100644
index 0000000..291fd57
--- /dev/null
+++ b/guests/config.yaml
@@ -0,0 +1,23 @@
+---
+# Configuration file for lcitool
+
+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'.
+ url: https://gitlab.com
+
+ # GitLab runner registration token. (Mandatory)
+ #runner_secret:
--
2.25.3
9:38 a.m.
New subject: [libvirt-ci PATCH 05/12] config: Introduce a new global config.yaml configuration file
On Wed, 2020-05-06 at 14:05 +0200, Erik Skultety wrote:
+++ b/guests/config.yaml
@@ -0,0 +1,23 @@
+---
+# Configuration file for lcitool
+
+install:
The file looks better if you leave an empty line between the section
name and the options.
+ # Installation flavor determining the target environment for the
VM:
+ #
+ # test - VMs suitable for local testing, 'test' has passwordless sudo
VMs suitable for local testing (the 'test' user has sudo privileges)
+ # jenkins - VMs pluggable to a jenkins environment
VMs ready to be plugged into a Jenkins environment
+ # gitlab - VMs ready to be plugged to a GitLab environment
VMs ready to be plugged into a GitLab environment
+ flavor: test
+
+ # Initial root password to be set by ansible on the appliance. This password
Root password for the VM. 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'.
+ url: https://gitlab.com
This comment really applies to the section, not to the specific
option, so it should look like
# GitLab-related options (only apply to the 'gitlab' flavor)
gitlab:
# GitLab connection URL
url: https://gitlab.com
...
--
Andrea Bolognani / Red Hat / Virtualization
7:05 a.m.
New subject: [libvirt-ci PATCH 06/12] lcitool: Introduce methods to load and validate the YAML config
This patch introduce a set of class Config helper methods in order to
parse and validate the new global YAML 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 | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 57 insertions(+)
diff --git a/guests/lcitool b/guests/lcitool
index 2dca8b5..07d0b3c 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -133,6 +133,29 @@ class Util:
class Config:
+ def __init__(self):
+
+ # Load the template config containing the defaults first, this must
+ # always succeed.
+ # NOTE: we should load this from /usr/share once we start packaging
+ # lcitool
+ base = Util.get_base()
+ with open(os.path.join(base, "config.yaml"), "r") as fp:
+ self.values = yaml.load(fp, Loader=yaml.Loader)
+
+ try:
+ with open(self._get_config_file("config.yaml"), "r") as
fp:
+ user_config = yaml.load(fp, Loader=yaml.Loader)
+ except Exception as e:
+ print("Missing or invalid config.yaml file: {}".format(e),
+ file=sys.stderr)
+ sys.exit(1)
+
+ # Validate the user provided config and use it to override the default
+ # settings
+ self._validate(user_config)
+ self._update(user_config)
+
@staticmethod
def _get_config_file(name):
try:
@@ -154,6 +177,40 @@ class Config:
return os.path.join(config_dir, name)
+ @staticmethod
+ def _validate_section(config, section, keys):
+
+ if config.get(section, None) is None:
+ raise KeyError("Section '{}' not found".format(section))
+
+ for key in keys:
+ if config.get(section).get(key, None) is None:
+ raise KeyError("Missing mandatory key
'{}.{}'".format(section,
+ key))
+
+ @staticmethod
+ def _validate(config):
+
+ # verify the [install] section and its mandatory options
+ Config._validate_section(config, "install",
["root_password"])
+
+ # we need to check flavor here, because later validations depend on it
+ flavor = config.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":
+ Config._validate_section(config, "gitlab",
["runner_secret"])
+
+ def _update(self, values):
+ self.values["install"].update(values["install"])
+
+ if values.get("gitlab", None):
+ self.values["gitlab"].update(values["gitlab"])
+
def get_flavor(self):
flavor_file = self._get_config_file("flavor")
--
2.25.3
11:20 a.m.
New subject: [libvirt-ci PATCH 06/12] lcitool: Introduce methods to load and validate the YAML config
On Wed, 2020-05-06 at 14:05 +0200, Erik Skultety wrote:
class Config:
+ def __init__(self):
+
+ # Load the template config containing the defaults first, this must
+ # always succeed.
+ # NOTE: we should load this from /usr/share once we start packaging
+ # lcitool
+ base = Util.get_base()
+ with open(os.path.join(base, "config.yaml"), "r") as fp:
+ self.values = yaml.load(fp, Loader=yaml.Loader)
We're use yaml.safe_load(fp) everywhere else. Any reason why we
should need the non-safe version? Why is it necessary to explicitly
provide it with a Loader?
+ try:
+ with open(self._get_config_file("config.yaml"), "r") as
fp:
+ user_config = yaml.load(fp, Loader=yaml.Loader)
+ except Exception as e:
+ print("Missing or invalid config.yaml file: {}".format(e),
+ file=sys.stderr)
+ sys.exit(1)
As mentioned last time, this is not the correct place or way to
handle this exception. I know you don't like how we process failures
right now :) but the way to address that is to post a patch that
eradicates the current implementation and replaces it with a better
one in one fell swoop, not to introduce alternative ways to do error
handling in a few select places.
I have posted a patch[1] that implements the suggestion I gave last
time, and that allows you to simply raise an Exception here.
+ # Validate the user provided config and use it to override
the default
+ # settings
+ self._validate(user_config)
+ self._update(user_config)
Incidentally, since these two functions can also fail, with your
approach you'd have reported a nice error message if the
configuration file is missing altogether but *not* if it exists but
is invalid.
+ @staticmethod
+ def _validate_section(config, section, keys):
+
+ if config.get(section, None) is None:
+ raise KeyError("Section '{}' not found".format(section))
This will raise the same error message both when the section is
actually missing and when it's present but doesn't contain anything,
so the error handling should be
raise Exception("Missing or empty section '{}'.format(section))
Note the use of Exception instead of KeyError - again, if you want
to change the way we do error handling by all means post patches
towards that goal, but until then please stick with the current
approach.
As for the check itself, I think I would like it to look like
if section not in config or config[section] is None:
I'm no Pythonista, but this it seems to follow the mantra "explicit
is better than implicit" more closely... You've written more Python
than me, though, so if you think using get() is superior I don't have
a problem leaving the code as is :)
+ for key in keys:
+ if config.get(section).get(key, None) is None:
+ raise KeyError("Missing mandatory key
'{}.{}'".format(section,
+ key))
The very same considerations apply here.
One additional thing: since YAML is quite flexible, I think it would
be sensible (if a little paranoid) to also have something like
if not (instanceof(config[section][key], str) or
instanceof(config[section][key], int)):
raise Exception(
"Invalid type for key '{}.{}'".format(section, key)
)
otherwise we'd happily accept something like
---
install:
root_password:
- let
- me
- in
as valid.
+ @staticmethod
+ def _validate(config):
The first thing you need to check here is whether config is not None,
otherwise an empty configuration file will result in
Traceback (most recent call last):
File "./lcitool", line 1090, in <module>
Application().run()
File "./lcitool", line 452, in __init__
self._config = Config()
File "./lcitool", line 156, in __init__
self._validate(user_config)
File "./lcitool", line 195, in _validate
Config._validate_section(config, "install", ["root_password"])
File "./lcitool", line 183, in _validate_section
if config.get(section, None) is None:
AttributeError: 'NoneType' object has no attribute 'get'
+ # verify the [install] section and its mandatory options
+ Config._validate_section(config, "install",
["root_password"])
+
+ # we need to check flavor here, because later validations depend on it
+ flavor = config.get("install").get("flavor",
"test")
This hardcodes the default value for flavor in the script instead of
reading it from the default configuration file. It should be
.get("flavor", self.values["install"]["flavor"])
[1] https://www.redhat.com/archives/libvir-list/2020-May/msg00315.html
--
Andrea Bolognani / Red Hat / Virtualization
5:50 a.m.
New subject: [libvirt-ci PATCH 06/12] lcitool: Introduce methods to load and validate the YAML config
On Thu, May 07, 2020 at 06:20:46PM +0200, Andrea Bolognani wrote:
On Wed, 2020-05-06 at 14:05 +0200, Erik Skultety wrote:
> class Config:
>
> + def __init__(self):
> +
> + # Load the template config containing the defaults first, this must
> + # always succeed.
> + # NOTE: we should load this from /usr/share once we start packaging
> + # lcitool
> + base = Util.get_base()
> + with open(os.path.join(base, "config.yaml"), "r") as
fp:
> + self.values = yaml.load(fp, Loader=yaml.Loader)
We're use yaml.safe_load(fp) everywhere else. Any reason why we
should need the non-safe version? Why is it necessary to explicitly
provide it with a Loader?
"YAMLLoadWarning: calling yaml.load() without Loader=... is deprecated"
...but this was a completely wrong choice of API on my end, since rather then
grepping the code base and copy-pasting, I read the documentation and missed
the safe_load API which also doesn't suffer from the deprecation warning.
> + try:
> + with open(self._get_config_file("config.yaml"),
"r") as fp:
> + user_config = yaml.load(fp, Loader=yaml.Loader)
> + except Exception as e:
> + print("Missing or invalid config.yaml file: {}".format(e),
> + file=sys.stderr)
> + sys.exit(1)
As mentioned last time, this is not the correct place or way to
handle this exception. I know you don't like how we process failures
right now :) but the way to address that is to post a patch that
eradicates the current implementation and replaces it with a better
one in one fell swoop, not to introduce alternative ways to do error
handling in a few select places.
I have posted a patch[1] that implements the suggestion I gave last
time, and that allows you to simply raise an Exception here.
> + # Validate the user provided config and use it to override the default
> + # settings
> + self._validate(user_config)
> + self._update(user_config)
Incidentally, since these two functions can also fail, with your
approach you'd have reported a nice error message if the
configuration file is missing altogether but *not* if it exists but
is invalid.
> + @staticmethod
> + def _validate_section(config, section, keys):
> +
> + if config.get(section, None) is None:
> + raise KeyError("Section '{}' not
found".format(section))
This will raise the same error message both when the section is
actually missing and when it's present but doesn't contain anything,
so the error handling should be
raise Exception("Missing or empty section '{}'.format(section))
Note the use of Exception instead of KeyError - again, if you want
to change the way we do error handling by all means post patches
towards that goal, but until then please stick with the current
approach.
As for the check itself, I think I would like it to look like
if section not in config or config[section] is None:
Well, if it was only about key existence, the first part of your boolean
predicate would be preferable, but once you need to check both existence and
value, .get() is the recommended way.
I'm no Pythonista, but this it seems to follow the mantra "explicit
As it turned out, I even was explicit :P since 'None' is the default value
returned on key non-existence
is better than implicit" more closely... You've written more
Python
than me, though, so if you think using get() is superior I don't have
a problem leaving the code as is :)
> + for key in keys:
> + if config.get(section).get(key, None) is None:
> + raise KeyError("Missing mandatory key
'{}.{}'".format(section,
> + key))
The very same considerations apply here.
One additional thing: since YAML is quite flexible, I think it would
be sensible (if a little paranoid) to also have something like
if not (instanceof(config[section][key], str) or
instanceof(config[section][key], int)):
raise Exception(
"Invalid type for key '{}.{}'".format(section, key)
)
Yes, this is a good idea, I'll incorporate that into the next revision,
although I'll do it a bit differently.
otherwise we'd happily accept something like
---
install:
root_password:
- let
- me
- in
as valid.
Yep, that's bad.
> + @staticmethod
> + def _validate(config):
The first thing you need to check here is whether config is not None,
otherwise an empty configuration file will result in
Traceback (most recent call last):
File "./lcitool", line 1090, in <module>
Application().run()
File "./lcitool", line 452, in __init__
self._config = Config()
File "./lcitool", line 156, in __init__
self._validate(user_config)
File "./lcitool", line 195, in _validate
Config._validate_section(config, "install", ["root_password"])
File "./lcitool", line 183, in _validate_section
if config.get(section, None) is None:
AttributeError: 'NoneType' object has no attribute 'get'
Damn, this is bad too, I'll fix it.
> + # verify the [install] section and its mandatory options
> + Config._validate_section(config, "install",
["root_password"])
> +
> + # we need to check flavor here, because later validations depend on it
> + flavor = config.get("install").get("flavor",
"test")
This hardcodes the default value for flavor in the script instead of
reading it from the default configuration file. It should be
.get("flavor", self.values["install"]["flavor"])
I'm not hardcoding anything. What's actually being done is we're only
verifying
the user-provided data, so we should not interact with the default values at
this point yet, so instead, either '' or None should be returned from the
.get() method.
--
Erik Skultety
7:27 a.m.
New subject: [libvirt-ci PATCH 06/12] lcitool: Introduce methods to load and validate the YAML config
On Mon, 2020-05-11 at 12:50 +0200, Erik Skultety wrote:
On Thu, May 07, 2020 at 06:20:46PM +0200, Andrea Bolognani wrote:
> On Wed, 2020-05-06 at 14:05 +0200, Erik Skultety wrote:
> > + self.values = yaml.load(fp, Loader=yaml.Loader)
>
> We're use yaml.safe_load(fp) everywhere else. Any reason why we
> should need the non-safe version? Why is it necessary to explicitly
> provide it with a Loader?
"YAMLLoadWarning: calling yaml.load() without Loader=... is deprecated"
...but this was a completely wrong choice of API on my end, since rather then
grepping the code base and copy-pasting, I read the documentation and missed
the safe_load API which also doesn't suffer from the deprecation warning.
Please don't take this specific situation to mean that copying and
pasting is better than reading documentation in the general case O:-)
> > + if config.get(section, None) is None:
> > + raise KeyError("Section '{}' not
found".format(section))
>
> This will raise the same error message both when the section is
> actually missing and when it's present but doesn't contain anything,
> so the error handling should be
>
> raise Exception("Missing or empty section '{}'.format(section))
>
> Note the use of Exception instead of KeyError - again, if you want
> to change the way we do error handling by all means post patches
> towards that goal, but until then please stick with the current
> approach.
>
> As for the check itself, I think I would like it to look like
>
> if section not in config or config[section] is None:
Well, if it was only about key existence, the first part of your boolean
predicate would be preferable, but once you need to check both existence and
value, .get() is the recommended way.
> I'm no Pythonista, but this it seems to follow the mantra "explicit
As it turned out, I even was explicit :P since 'None' is the default value
returned on key non-existence
Okay, I'm fine using your version as long as you update the error
message as described above.
> > + # we need to check flavor here, because later
validations depend on it
> > + flavor = config.get("install").get("flavor",
"test")
>
> This hardcodes the default value for flavor in the script instead of
> reading it from the default configuration file. It should be
>
> .get("flavor", self.values["install"]["flavor"])
I'm not hardcoding anything. What's actually being done is we're only
verifying
the user-provided data, so we should not interact with the default values at
this point yet, so instead, either '' or None should be returned from the
.get() method.
Maybe I'm missing something, but
flavor = config.get("install").get("flavor", "test")
if flavor not in ["test", "jenkins", "gitlab"]:
raise ValueError(
"Invalid value '{}' for
'install.flavor'".format(flavor)
)
looks to me like you're trying to fetch the flavor configured by
the user and validate it's one of the three accepted values; however,
the second argument of the get() function makes it so that, if the
value is missing in the user's configuration file, you'll get the
default flavor (test) instead.
This is why I say you're hardcoding the default value in the script:
if tomorrow we decided that "gitlab" should be the default flavor, we
would have to change not only the default configuration file but also
the script; with the approach I suggest, changing the former would be
enough.
--
Andrea Bolognani / Red Hat / Virtualization
8:27 a.m.
New subject: [libvirt-ci PATCH 06/12] lcitool: Introduce methods to load and validate the YAML config
On Mon, May 11, 2020 at 02:27:13PM +0200, Andrea Bolognani wrote:
On Mon, 2020-05-11 at 12:50 +0200, Erik Skultety wrote:
> On Thu, May 07, 2020 at 06:20:46PM +0200, Andrea Bolognani wrote:
> > On Wed, 2020-05-06 at 14:05 +0200, Erik Skultety wrote:
> > > + self.values = yaml.load(fp, Loader=yaml.Loader)
> >
> > We're use yaml.safe_load(fp) everywhere else. Any reason why we
> > should need the non-safe version? Why is it necessary to explicitly
> > provide it with a Loader?
>
> "YAMLLoadWarning: calling yaml.load() without Loader=... is deprecated"
> ...but this was a completely wrong choice of API on my end, since rather then
> grepping the code base and copy-pasting, I read the documentation and missed
> the safe_load API which also doesn't suffer from the deprecation warning.
Please don't take this specific situation to mean that copying and
pasting is better than reading documentation in the general case O:-)
Of course...now that I'm reading my response back, it does read differently
(and silly too) that what I actually meant, nevermind :).
...
>
> I'm not hardcoding anything. What's actually being done is we're only
verifying
> the user-provided data, so we should not interact with the default values at
> this point yet, so instead, either '' or None should be returned from the
> .get() method.
Maybe I'm missing something, but
flavor = config.get("install").get("flavor", "test")
if flavor not in ["test", "jenkins", "gitlab"]:
raise ValueError(
"Invalid value '{}' for
'install.flavor'".format(flavor)
)
looks to me like you're trying to fetch the flavor configured by
the user and validate it's one of the three accepted values; however,
the second argument of the get() function makes it so that, if the
value is missing in the user's configuration file, you'll get the
default flavor (test) instead.
This is why I say you're hardcoding the default value in the script:
if tomorrow we decided that "gitlab" should be the default flavor, we
would have to change not only the default configuration file but also
the script; with the approach I suggest, changing the former would be
enough.
That is a correct assumption, but at the same time this code should only verify
that we can recognize the user-provided value, so "test" was just a remnant
from the previous revision... not to be confused with actually selecting a
default, that is handled by loading the default config from the repo in a
different part of the code.
That's why I'm saying that in order to clear this confusion, the default from
the get() here should be None, because that way we know that the user didn't
provide any value for flavor (and we need to fill it in later). Therefore,
None should also be part of the list of recognized values, but serve merely as
a sentinel so we don't end up raising an Exception about an invalid config
value.
--
Erik Skultety
8:55 a.m.
New subject: [libvirt-ci PATCH 06/12] lcitool: Introduce methods to load and validate the YAML config
On Mon, May 11, 2020 at 03:27:37PM +0200, Erik Skultety wrote:
On Mon, May 11, 2020 at 02:27:13PM +0200, Andrea Bolognani wrote:
> On Mon, 2020-05-11 at 12:50 +0200, Erik Skultety wrote:
> > On Thu, May 07, 2020 at 06:20:46PM +0200, Andrea Bolognani wrote:
> > > On Wed, 2020-05-06 at 14:05 +0200, Erik Skultety wrote:
> > > > + self.values = yaml.load(fp, Loader=yaml.Loader)
> > >
> > > We're use yaml.safe_load(fp) everywhere else. Any reason why we
> > > should need the non-safe version? Why is it necessary to explicitly
> > > provide it with a Loader?
> >
> > "YAMLLoadWarning: calling yaml.load() without Loader=... is
deprecated"
> > ...but this was a completely wrong choice of API on my end, since rather then
> > grepping the code base and copy-pasting, I read the documentation and missed
> > the safe_load API which also doesn't suffer from the deprecation warning.
>
> Please don't take this specific situation to mean that copying and
> pasting is better than reading documentation in the general case O:-)
Of course...now that I'm reading my response back, it does read differently
(and silly too) that what I actually meant, nevermind :).
...
> >
> > I'm not hardcoding anything. What's actually being done is we're
only verifying
> > the user-provided data, so we should not interact with the default values at
> > this point yet, so instead, either '' or None should be returned from
the
> > .get() method.
>
> Maybe I'm missing something, but
>
> flavor = config.get("install").get("flavor",
"test")
>
> if flavor not in ["test", "jenkins", "gitlab"]:
> raise ValueError(
> "Invalid value '{}' for
'install.flavor'".format(flavor)
> )
>
> looks to me like you're trying to fetch the flavor configured by
> the user and validate it's one of the three accepted values; however,
> the second argument of the get() function makes it so that, if the
> value is missing in the user's configuration file, you'll get the
> default flavor (test) instead.
>
> This is why I say you're hardcoding the default value in the script:
> if tomorrow we decided that "gitlab" should be the default flavor, we
> would have to change not only the default configuration file but also
> the script; with the approach I suggest, changing the former would be
> enough.
That is a correct assumption, but at the same time this code should only verify
that we can recognize the user-provided value, so "test" was just a remnant
from the previous revision... not to be confused with actually selecting a
default, that is handled by loading the default config from the repo in a
different part of the code.
That's why I'm saying that in order to clear this confusion, the default from
the get() here should be None, because that way we know that the user didn't
provide any value for flavor (and we need to fill it in later). Therefore,
None should also be part of the list of recognized values, but serve merely as
Actually, None cannot be part of the list of recognized values, because it will
mess with the _update function and populate an invalid setting to the playbook.
So, I suggest we mandate that when an option is specified, it cannot be empty.
By doing that, the dictionary update() works like expected and we don't have
to mess with further dictionary comprehensions to filter out keys with empty
values for which we're able to provide a default.
--
Erik Skultety
11:36 a.m.
New subject: [libvirt-ci PATCH 06/12] lcitool: Introduce methods to load and validate the YAML config
On Mon, 2020-05-11 at 15:55 +0200, Erik Skultety wrote:
On Mon, May 11, 2020 at 03:27:37PM +0200, Erik Skultety wrote:
> On Mon, May 11, 2020 at 02:27:13PM +0200, Andrea Bolognani wrote:
> > Maybe I'm missing something, but
> >
> > flavor = config.get("install").get("flavor",
"test")
> >
> > if flavor not in ["test", "jenkins",
"gitlab"]:
> > raise ValueError(
> > "Invalid value '{}' for
'install.flavor'".format(flavor)
> > )
> >
> > looks to me like you're trying to fetch the flavor configured by
> > the user and validate it's one of the three accepted values; however,
> > the second argument of the get() function makes it so that, if the
> > value is missing in the user's configuration file, you'll get the
> > default flavor (test) instead.
> >
> > This is why I say you're hardcoding the default value in the script:
> > if tomorrow we decided that "gitlab" should be the default flavor,
we
> > would have to change not only the default configuration file but also
> > the script; with the approach I suggest, changing the former would be
> > enough.
>
> That is a correct assumption, but at the same time this code should only verify
> that we can recognize the user-provided value, so "test" was just a
remnant
> from the previous revision... not to be confused with actually selecting a
> default, that is handled by loading the default config from the repo in a
> different part of the code.
> That's why I'm saying that in order to clear this confusion, the default
from
> the get() here should be None, because that way we know that the user didn't
> provide any value for flavor (and we need to fill it in later). Therefore,
> None should also be part of the list of recognized values, but serve merely as
Actually, None cannot be part of the list of recognized values, because it will
mess with the _update function and populate an invalid setting to the playbook.
So, I suggest we mandate that when an option is specified, it cannot be empty.
By doing that, the dictionary update() works like expected and we don't have
to mess with further dictionary comprehensions to filter out keys with empty
values for which we're able to provide a default.
Sure, expecting the user to provide actual values in the
configuration file is an entirely acceptable constraint :)
I'm not entirely sure how you plan to reconcile that with the fact
that some keys are optional, and user_config.get("key") will return
None both when "key" was set to an empty value and when it was not
present at all... But I'm sure you have something in mind :)
--
Andrea Bolognani / Red Hat / Virtualization
7:05 a.m.
New subject: [libvirt-ci PATCH 07/12] lcitool: Update the config values with internal playbook settings
So, the idea is to pass our YAML config to the Ansible playbooks as
extra vars. However, not all variables we need to pass on to Ansible
are exposed in the config (and they shouldn't be). Update the config
values dictionary with these variables before passing on to Ansible.
Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
---
guests/lcitool | 10 ++----
guests/playbooks/build/main.yml | 2 +-
guests/playbooks/update/main.yml | 6 ++--
guests/playbooks/update/tasks/gitlab.yml | 4 +--
guests/playbooks/update/tasks/kludges.yml | 2 +-
guests/playbooks/update/tasks/users.yml | 42 +++++++++++------------
6 files changed, 30 insertions(+), 36 deletions(-)
diff --git a/guests/lcitool b/guests/lcitool
index 07d0b3c..818ae82 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -567,20 +567,16 @@ class Application:
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 = {
+ self._config.values.update({
"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:
- json.dump(extra_vars, fp)
+ json.dump(self._config.values, fp)
ansible_playbook = distutils.spawn.find_executable("ansible-playbook")
if ansible_playbook is None:
diff --git a/guests/playbooks/build/main.yml b/guests/playbooks/build/main.yml
index 8abda67..462764b 100644
--- a/guests/playbooks/build/main.yml
+++ b/guests/playbooks/build/main.yml
@@ -1,6 +1,6 @@
---
- hosts: all
- remote_user: '{{ flavor }}'
+ remote_user: '{{ install.flavor }}'
vars_files:
- '{{ playbook_base }}/jobs/defaults.yml'
diff --git a/guests/playbooks/update/main.yml b/guests/playbooks/update/main.yml
index 371e53d..1b97027 100644
--- a/guests/playbooks/update/main.yml
+++ b/guests/playbooks/update/main.yml
@@ -45,7 +45,7 @@
vars:
project: jenkins
when:
- - flavor == "jenkins"
+ - install.flavor == "jenkins"
# Configure environment. Needs to happen after installing packages
- include: '{{ playbook_base }}/tasks/kludges.yml'
@@ -57,9 +57,9 @@
# Configure the Jenkins agent
- include: '{{ playbook_base }}/tasks/jenkins.yml'
when:
- - flavor == 'jenkins'
+ - install.flavor == 'jenkins'
# Install the Gitlab runner agent
- include: '{{ playbook_base }}/tasks/gitlab.yml'
when:
- - flavor == 'gitlab'
+ - install.flavor == 'gitlab'
diff --git a/guests/playbooks/update/tasks/gitlab.yml
b/guests/playbooks/update/tasks/gitlab.yml
index f07279c..07a376c 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'
@@ -14,7 +12,7 @@
force: yes
- name: Register the gitlab-runner agent
- shell: 'gitlab-runner register --non-interactive --config "{{
gitlab_runner_config_dir }}/config.toml" --registration-token "{{
gitlab_runner_secret }}" --url "{{ gitlab_url }}" --executor shell
--tag-list "{{ os.name|lower }}-{{ os.version }}"'
+ shell: 'gitlab-runner register --non-interactive --config "{{
gitlab_runner_config_dir }}/config.toml" --registration-token "{{
gitlab.runner_secret }}" --url "{{ gitlab.url }}" --executor shell
--tag-list "{{ os.name|lower }}-{{ os.version }}"'
args:
creates: '{{ gitlab_runner_config_dir }}/config.toml'
diff --git a/guests/playbooks/update/tasks/kludges.yml
b/guests/playbooks/update/tasks/kludges.yml
index 96fe1a5..33c6532 100644
--- a/guests/playbooks/update/tasks/kludges.yml
+++ b/guests/playbooks/update/tasks/kludges.yml
@@ -12,7 +12,7 @@
group: wheel
when:
- os.name == 'FreeBSD'
- - flavor == "jenkins"
+ - install.flavor == "jenkins"
# FreeBSD compiles bash without defining SSH_SOURCE_BASHRC, which means
# it won't try to detect when it's spawned by ssh and source ~/.bashrc
diff --git a/guests/playbooks/update/tasks/users.yml
b/guests/playbooks/update/tasks/users.yml
index 5c6ce8f..bc3cc11 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: '{{ install.root_password|password_hash("sha512") }}'
- name: 'root: Configure ssh access'
authorized_key:
@@ -17,54 +17,54 @@
line: 'PermitRootLogin without-password'
state: present
-- name: '{{ flavor }}: Create group'
+- name: '{{ install.flavor }}: Create group'
group:
- name: '{{ flavor }}'
+ name: '{{ install.flavor }}'
state: present
-- name: '{{ flavor }}: Create user account'
+- name: '{{ install.flavor }}: Create user account'
user:
- name: '{{ flavor }}'
- group: '{{ flavor }}'
- comment: '{{ flavor }}'
+ name: '{{ install.flavor }}'
+ group: '{{ install.flavor }}'
+ comment: '{{ install.flavor }}'
password: '*'
shell: '{{ paths.bash }}'
-- name: '{{ flavor }}: Set password'
+- name: '{{ install.flavor }}: Set password'
user:
- name: '{{ flavor }}'
+ name: '{{ install.flavor }}'
password: '{{ "test"|password_hash("sha512") }}'
when:
- - flavor == 'test'
+ - install.flavor == 'test'
-- name: '{{ flavor }}: Configure ssh access'
+- name: '{{ install.flavor }}: Configure ssh access'
authorized_key:
- user: '{{ flavor }}'
+ user: '{{ install.flavor }}'
key: '{{ lookup("file", lookup("env", "HOME") +
"/.ssh/id_rsa.pub") }}'
state: present
-- name: '{{ flavor }}: Grant passwordless sudo access'
+- name: '{{ install.flavor }}: Grant passwordless sudo access'
lineinfile:
path: '{{ paths.sudoers }}'
- line: '{{ flavor }} ALL=(ALL) NOPASSWD: ALL'
+ line: '{{ install.flavor }} ALL=(ALL) NOPASSWD: ALL'
state: present
validate: 'visudo -cf %s'
when:
- - flavor == 'test'
+ - install.flavor == 'test'
-- name: '{{ flavor }}: Create shell profile'
+- name: '{{ install.flavor }}: Create shell profile'
template:
src: '{{ playbook_base }}/templates/{{ item }}.j2'
- dest: /home/{{ flavor }}/.{{ item }}
- owner: '{{ flavor }}'
- group: '{{ flavor }}'
+ dest: /home/{{ install.flavor }}/.{{ item }}
+ owner: '{{ install.flavor }}'
+ group: '{{ install.flavor }}'
with_items:
- bash_profile
- bashrc
-- name: '{{ flavor }}: Remove unnecessary home skeleton files'
+- name: '{{ install.flavor }}: Remove unnecessary home skeleton files'
file:
- path: /home/{{ flavor }}/.{{ item }}
+ path: /home/{{ install.flavor }}/.{{ item }}
state: absent
with_items:
- profile
--
2.25.3
11:35 a.m.
New subject: [libvirt-ci PATCH 07/12] lcitool: Update the config values with internal playbook settings
On Wed, 2020-05-06 at 14:05 +0200, Erik Skultety wrote:
- extra_vars = {
+ self._config.values.update({
"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,
- }
+ })
I think this would be nicer as
extra_vars = self._config.values
extra_vars.update({
...
})
but, either with your version or mine, the patch is still
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
--
Andrea Bolognani / Red Hat / Virtualization
7:05 a.m.
New subject: [libvirt-ci PATCH 08/12] 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 | 36 ++++--------------------------------
1 file changed, 4 insertions(+), 32 deletions(-)
diff --git a/guests/lcitool b/guests/lcitool
index 818ae82..9c1a722 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -211,37 +211,12 @@ class Config:
if values.get("gitlab", None):
self.values["gitlab"].update(values["gitlab"])
- 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.values["install"]["flavor"] == "jenkins":
vault_pass_file = self._get_config_file("vault-password")
try:
@@ -274,7 +249,7 @@ class Config:
return root_pass_file
def get_gitlab_runner_token_file(self):
- if self.get_flavor() != "gitlab":
+ if self.values["install"]["flavor"] != "gitlab":
return None
gitlab_runner_token_file =
self._get_config_file("gitlab-runner-token")
@@ -293,7 +268,7 @@ class Config:
return gitlab_runner_token_file
def get_gitlab_url_file(self):
- if self.get_flavor() != "gitlab":
+ if self.values["install"]["flavor"] != "gitlab":
return None
gitlab_url_file = self._get_config_file("gitlab-url")
@@ -540,7 +515,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()
@@ -618,8 +592,6 @@ class Application:
def _action_install(self, args):
base = Util.get_base()
- flavor = self._config.get_flavor()
-
for host in self._inventory.expand_pattern(args.hosts):
facts = self._inventory.get_facts(host)
@@ -715,7 +687,7 @@ class Application:
cmd.append("--noautoconsole")
# Only configure autostart for the guest for the jenkins flavor
- if flavor == "jenkins":
+ if self._config.values["install"]["flavor"] ==
"jenkins":
cmd += ["--autostart"]
try:
--
2.25.3
11:37 a.m.
New subject: [libvirt-ci PATCH 08/12] lcitool: Drop the get_flavor() method
On Wed, 2020-05-06 at 14:05 +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 | 36 ++++--------------------------------
1 file changed, 4 insertions(+), 32 deletions(-)
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
--
Andrea Bolognani / Red Hat / Virtualization
7:05 a.m.
New subject: [libvirt-ci PATCH 09/12] 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 | 17 -----------------
1 file changed, 17 deletions(-)
diff --git a/guests/lcitool b/guests/lcitool
index 9c1a722..0725be9 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -232,22 +232,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.values["install"]["flavor"] != "gitlab":
return None
@@ -516,7 +500,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()
--
2.25.3
11:37 a.m.
New subject: [libvirt-ci PATCH 09/12] lcitool: Drop the get_root_password_file() method
On Wed, 2020-05-06 at 14:05 +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 | 17 -----------------
1 file changed, 17 deletions(-)
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
--
Andrea Bolognani / Red Hat / Virtualization
7:06 a.m.
New subject: [libvirt-ci PATCH 10/12] 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 | 40 ----------------------------------------
1 file changed, 40 deletions(-)
diff --git a/guests/lcitool b/guests/lcitool
index 0725be9..6bbe314 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -232,44 +232,6 @@ class Config:
return vault_pass_file
- def get_gitlab_runner_token_file(self):
- if self.values["install"]["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.values["install"]["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:
@@ -500,8 +462,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)
--
2.25.3
11:38 a.m.
New subject: [libvirt-ci PATCH 10/12] lcitool: Drop the gitlab-related getter methods
On Wed, 2020-05-06 at 14:06 +0200, Erik Skultety wrote:
We can now access the values directly in the config dictionary.
Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
---
guests/lcitool | 40 ----------------------------------------
1 file changed, 40 deletions(-)
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
--
Andrea Bolognani / Red Hat / Virtualization
7:06 a.m.
New subject: [libvirt-ci PATCH 11/12] 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>
---
guests/config.yaml | 17 +++++++++++++++++
guests/lcitool | 28 +++++++++++-----------------
2 files changed, 28 insertions(+), 17 deletions(-)
diff --git a/guests/config.yaml b/guests/config.yaml
index 291fd57..5f750e7 100644
--- a/guests/config.yaml
+++ b/guests/config.yaml
@@ -15,6 +15,23 @@ install:
# 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'.
url: https://gitlab.com
diff --git a/guests/lcitool b/guests/lcitool
index 6bbe314..91582cb 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -534,23 +534,17 @@ class Application:
def _action_install(self, args):
base = Util.get_base()
+ config = self._config
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"],
+ config.values["install"]["disk_size"],
+ config.values["install"]["storage_pool"],
)
network_arg = "network={},model=virtio".format(
- facts["install_network"],
+ config.values["install"]["network"],
)
# Different operating systems require different configuration
@@ -610,12 +604,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",
config.values["install"]["virt_type"],
+ "--arch",
config.values["install"]["arch"],
+ "--machine",
config.values["install"]["machine"],
+ "--cpu",
config.values["install"]["cpu_model"],
+ "--vcpus",
str(config.values["install"]["vcpus"]),
+ "--memory",
str(config.values["install"]["memory_size"] * 1024),
"--disk", disk_arg,
"--network", network_arg,
"--graphics", "none",
@@ -630,7 +624,7 @@ class Application:
cmd.append("--noautoconsole")
# Only configure autostart for the guest for the jenkins flavor
- if self._config.values["install"]["flavor"] ==
"jenkins":
+ if config.values["install"]["flavor"] ==
"jenkins":
cmd += ["--autostart"]
try:
--
2.25.3
11:56 a.m.
New subject: [libvirt-ci PATCH 11/12] config: Move the virt-install settings from install.yml to the config
On Wed, 2020-05-06 at 14:06 +0200, Erik Skultety wrote:
+++ b/guests/config.yaml
@@ -15,6 +15,23 @@ install:
# 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
Now is a great time to delete group_vars/all/install.yml, since
you've just made it obsolete and we don't want to have the same
information stored in two places.
Note that there's one use of install_* variables that would be broken
by doing so in playbooks/update/templates/bashrc.j2, but I've already
posted a patch[1] that takes care of that, so as long as that goes in
before your series we don't have to worry about it :)
def _action_install(self, args):
base = Util.get_base()
+ config = self._config
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"])
Please leave these type conversion bits here where they can easily
be spotted, instead of hiding them in the jungle of creating the
virt-install command line.
With the changes mentioned above,
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
[1] https://www.redhat.com/archives/libvir-list/2020-May/msg00330.html
--
Andrea Bolognani / Red Hat / Virtualization
3:08 a.m.
New subject: [libvirt-ci PATCH 11/12] config: Move the virt-install settings from install.yml to the config
On Thu, May 07, 2020 at 06:56:44PM +0200, Andrea Bolognani wrote:
On Wed, 2020-05-06 at 14:06 +0200, Erik Skultety wrote:
> +++ b/guests/config.yaml
> @@ -15,6 +15,23 @@ install:
> # 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
Now is a great time to delete group_vars/all/install.yml, since
you've just made it obsolete and we don't want to have the same
information stored in two places.
Note that there's one use of install_* variables that would be broken
by doing so in playbooks/update/templates/bashrc.j2, but I've already
posted a patch[1] that takes care of that, so as long as that goes in
before your series we don't have to worry about it :)
Would it? I knew about the MAKEFLAGS thing, but I expected that to simply work
once I pass the extra variables, nevertheless the patch you posted makes
complete sense and is even in sync in what we do in gitlab CI.
> def _action_install(self, args):
> base = Util.get_base()
> + config = self._config
>
> 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"])
Please leave these type conversion bits here where they can easily
be spotted, instead of hiding them in the jungle of creating the
virt-install command line.
Okay.
With the changes mentioned above,
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
[1] https://www.redhat.com/archives/libvir-list/2020-May/msg00330.html
--
Andrea Bolognani / Red Hat / Virtualization
--
Erik Skultety
7:13 a.m.
New subject: [libvirt-ci PATCH 11/12] config: Move the virt-install settings from install.yml to the config
On Mon, 2020-05-11 at 10:08 +0200, Erik Skultety wrote:
On Thu, May 07, 2020 at 06:56:44PM +0200, Andrea Bolognani wrote:
> On Wed, 2020-05-06 at 14:06 +0200, Erik Skultety wrote:
> > + # 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
>
> Now is a great time to delete group_vars/all/install.yml, since
> you've just made it obsolete and we don't want to have the same
> information stored in two places.
>
> Note that there's one use of install_* variables that would be broken
> by doing so in playbooks/update/templates/bashrc.j2, but I've already
> posted a patch[1] that takes care of that, so as long as that goes in
> before your series we don't have to worry about it :)
Would it? I knew about the MAKEFLAGS thing, but I expected that to simply work
once I pass the extra variables,
Yeah, you're right, it *would* work: I was mistakenly thinking that
the additional variables would only be used when installing, but I
see now that any time Ansible is called it would have access to them
as well.
Anyway, I've already merged the other patch, so it no longer
matters :)
--
Andrea Bolognani / Red Hat / Virtualization
7:06 a.m.
New subject: [libvirt-ci PATCH 12/12] guests: README: Document the existence and usage of config.toml
Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
---
guests/README.markdown | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/guests/README.markdown b/guests/README.markdown
index 7dbefed..a58c5cb 100644
--- a/guests/README.markdown
+++ b/guests/README.markdown
@@ -64,13 +64,15 @@ branches out of those with
Host setup
----------
-Ansible and `virt-install` need to be available on the host.
+`ansible` and `virt-install` need to be available on the host, the former 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.yaml` template) ~/.config/lcitool/config.yaml 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 +125,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.yaml`.
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
12:01 p.m.
New subject: [libvirt-ci PATCH 12/12] guests: README: Document the existence and usage of config.toml
On Wed, 2020-05-06 at 14:06 +0200, Erik Skultety wrote:
-Ansible and `virt-install` need to be available on the host.
+`ansible` and `virt-install` need to be available on the host, the former can
+be either installed system-wide using your package manager or using by `pip`
s/using by/using/
+Before you can start bringing up guests, you need to create (ideally
by
+copying the `config.yaml` template) ~/.config/lcitool/config.yaml and set at
... you need to create `~/.config/lcitool/config.yaml`, ideally by
copying the provided `config.yaml` template, and set...
-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.yaml`.
... simply set `install.flavor` to `jenkins` in...
With the changes above,
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
--
Andrea Bolognani / Red Hat / Virtualization
1656
days inactive
1661
days old
28 comments
2 participants
participants (2)
-
Andrea Bolognani -
Erik Skultety