9:42 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 (ACKed)
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
Since v1:
- uncommented the mandatory options in the default template YAML config so that
we know about all the supported keys which is useful for validating the user
config
- removed guests/group_vars/all/install.yaml in patch 11 (which I forgot in v1)
- added checks for value types we get from the config
- use yaml.safe_load instead of yaml.load
- added code snippet to delete keys we don't recognize so as not to introduce a
security issue, because we essentially just take the config and pass it to
ansible, we don't want users to use to re-define some of Ansible's variables
- added the last patch just to demonstrate a number of test cases I used as a
proof for correctness of this revision (feel free to add more cases), but
this is not the right series to add pytest support into lcitool
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.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.yaml
DO NOT MERGE: Demonstrate functionality with pytest unit tests
guests/README.markdown | 18 +-
guests/config.yaml | 42 +++++
guests/group_vars/all/install.yml | 11 --
guests/{lcitool => lcitool.py} | 209 +++++++++++-----------
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 | 4 +
guests/test_config.py | 165 +++++++++++++++++
11 files changed, 353 insertions(+), 152 deletions(-)
create mode 100644 guests/config.yaml
delete mode 100644 guests/group_vars/all/install.yml
rename guests/{lcitool => lcitool.py} (88%)
create mode 100644 guests/requirements.txt
create mode 100644 guests/test_config.py
--
2.25.3
9:42 a.m.
New subject: [libvirt-ci PATCH v2 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>
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
9:42 a.m.
New subject: [libvirt-ci PATCH v2 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>
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 88eb248..dcc54ae 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -610,11 +610,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:42 a.m.
New subject: [libvirt-ci PATCH v2 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>
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 dcc54ae..56eb543 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -24,7 +24,6 @@ import json
import os
import platform
import random
-import shutil
import string
import subprocess
import sys
@@ -610,8 +609,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)
@@ -665,8 +664,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 v2 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>
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
---
guests/lcitool | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/guests/lcitool b/guests/lcitool
index 56eb543..5b44582 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -506,11 +506,14 @@ 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({
+ extra_vars = {
"base": base,
"playbook_base": playbook_base,
"root_password_file": root_pass_file,
@@ -520,7 +523,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:
@@ -529,7 +535,7 @@ class Application:
cmd = [
ansible_playbook,
"--limit", ansible_hosts,
- "--extra-vars", extra_vars,
+ "--extra-vars", "@" + extra_vars_path,
]
# Provide the vault password if available
@@ -548,6 +554,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:42 a.m.
New subject: [libvirt-ci PATCH v2 05/13] 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 | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
create mode 100644 guests/config.yaml
diff --git a/guests/config.yaml b/guests/config.yaml
new file mode 100644
index 0000000..05f1e8a
--- /dev/null
+++ b/guests/config.yaml
@@ -0,0 +1,25 @@
+---
+# Configuration file for lcitool
+
+install:
+
+ # Installation flavor determining the target environment for the VM:
+ #
+ # test - VMs suitable for local testing (the 'test' user has sudo privileges)
+ # jenkins - VMs ready to be plugged into a Jenkins environment
+ # gitlab - VMs ready to be plugged into a GitLab environment
+ flavor: test
+
+ # 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-related options (only apply to the 'gitlab' flavor)
+gitlab:
+
+ # GitLab connection URL
+ url: https://gitlab.com
+
+ # GitLab runner registration token. (Mandatory)
+ runner_secret:
--
2.25.3
11:22 a.m.
New subject: [libvirt-ci PATCH v2 05/13] config: Introduce a new global config.yaml configuration file
On Tue, 2020-05-12 at 16:42 +0200, Erik Skultety wrote:
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 | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
create mode 100644 guests/config.yaml
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
--
Andrea Bolognani / Red Hat / Virtualization
9:42 a.m.
New subject: [libvirt-ci PATCH v2 06/13] 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 | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 82 insertions(+)
diff --git a/guests/lcitool b/guests/lcitool
index 5b44582..577e9d2 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -128,6 +128,30 @@ 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.safe_load(fp)
+
+ try:
+ with open(self._get_config_file("config.yaml"), "r") as
fp:
+ user_config = yaml.safe_load(fp)
+ except Exception as e:
+ raise Exception("Missing or invalid config.yaml file:
{}".format(e))
+
+ if user_config is None:
+ raise Exception("Missing or invalid config.yaml file")
+
+ # 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:
@@ -149,6 +173,64 @@ class Config:
return os.path.join(config_dir, name)
+ @staticmethod
+ def _remove_unknown_keys(_dict, known_keys):
+ keys = list(_dict.keys())
+
+ for k in keys:
+ if k not in known_keys:
+ del _dict[k]
+
+ def _validate_section(self, config, section, mandatory_keys):
+ # remove keys we don't recognize
+ self._remove_unknown_keys(config[section], self.values[section].keys())
+
+ # check that the mandatory keys are present and non-empty
+ for key in mandatory_keys:
+ if config.get(section).get(key, None) is None:
+ raise Exception(("Missing or empty value for mandatory key"
+ "'{}.{}'").format(section, key))
+
+ # check that all keys have values assigned and of the right type
+ for key in config[section].keys():
+
+ # mandatory keys were already checked, so this covers optional keys
+ if config[section][key] is None:
+ raise Exception(
+ "Missing value for '{}.{}'".format(section, key)
+ )
+
+ if not isinstance(config[section][key], (str, int)):
+ raise Exception(
+ "Invalid type for key '{}.{}'".format(section,
key)
+ )
+
+ def _validate(self, config):
+ # delete sections we don't recognize
+ self._remove_unknown_keys(config, self.values.keys())
+
+ if "install" not in config:
+ raise Exception("Missing mandatory section 'install'")
+
+ self._validate_section(config, "install", ["root_password"])
+
+ # we only need this for the gitlab check below, if 'flavor' is missing
+ # that's okay, we'll provide a default later
+ flavor = config["install"].get("flavor", None)
+ if flavor is not None and flavor not in ["test", "jenkins",
"gitlab"]:
+ raise Exception(
+ "Invalid value '{}' for
'install.flavor'".format(flavor)
+ )
+
+ if flavor == "gitlab":
+ self._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:48 a.m.
New subject: [libvirt-ci PATCH v2 06/13] lcitool: Introduce methods to load and validate the YAML config
On Tue, 2020-05-12 at 16:42 +0200, Erik Skultety wrote:
+ def _validate_section(self, config, section, mandatory_keys):
+ # remove keys we don't recognize
+ self._remove_unknown_keys(config[section], self.values[section].keys())
+
+ # check that the mandatory keys are present and non-empty
+ for key in mandatory_keys:
+ if config.get(section).get(key, None) is None:
Isn't
foo.get("bar")
defined as returning None if the "bar" key is not defined for the
dictionary foo? Basically, I think you could write this as
if config.get(section).get(key) is None:
+ def _validate(self, config):
+ # delete sections we don't recognize
+ self._remove_unknown_keys(config, self.values.keys())
+
+ if "install" not in config:
+ raise Exception("Missing mandatory section 'install'")
+
+ self._validate_section(config, "install",
["root_password"])
+
+ # we only need this for the gitlab check below, if 'flavor' is missing
+ # that's okay, we'll provide a default later
+ flavor = config["install"].get("flavor", None)
Same here...
+ def _update(self, values):
+ self.values["install"].update(values["install"])
+
+ if values.get("gitlab", None):
... and here. In this specific case, I think the pythonic way to
write this check would be
if values.get("gitlab") is not None:
With these nits addressed,
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
--
Andrea Bolognani / Red Hat / Virtualization
2:32 a.m.
New subject: [libvirt-ci PATCH v2 06/13] lcitool: Introduce methods to load and validate the YAML config
On Wed, May 13, 2020 at 06:48:26PM +0200, Andrea Bolognani wrote:
On Tue, 2020-05-12 at 16:42 +0200, Erik Skultety wrote:
> + def _validate_section(self, config, section, mandatory_keys):
> + # remove keys we don't recognize
> + self._remove_unknown_keys(config[section], self.values[section].keys())
> +
> + # check that the mandatory keys are present and non-empty
> + for key in mandatory_keys:
> + if config.get(section).get(key, None) is None:
Isn't
foo.get("bar")
defined as returning None if the "bar" key is not defined for the
dictionary foo? Basically, I think you could write this as
if config.get(section).get(key) is None:
Correct, I was just being explicit, it might not be clear at first glance that
None is the default return value, but I agree that what you suggest is the
Pythonic way, so I'll happily change it.
--
Erik Skultety
9:42 a.m.
New subject: [libvirt-ci PATCH v2 07/13] 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 577e9d2..3270fca 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -595,20 +595,16 @@ class Application:
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:57 a.m.
New subject: [libvirt-ci PATCH v2 07/13] lcitool: Update the config values with internal playbook settings
On Tue, 2020-05-12 at 16:42 +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 still think this would look better as
extra_vars = self._config.values
extra_vars.update({
...
}
but either way my
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
still stands.
--
Andrea Bolognani / Red Hat / Virtualization
9:42 a.m.
New subject: [libvirt-ci PATCH v2 08/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>
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
---
guests/lcitool | 36 ++++--------------------------------
1 file changed, 4 insertions(+), 32 deletions(-)
diff --git a/guests/lcitool b/guests/lcitool
index 3270fca..fa60218 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -231,37 +231,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:
@@ -294,7 +269,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")
@@ -313,7 +288,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")
@@ -567,7 +542,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()
@@ -646,8 +620,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)
@@ -743,7 +715,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
9:42 a.m.
New subject: [libvirt-ci PATCH v2 09/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>
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
---
guests/lcitool | 17 -----------------
1 file changed, 17 deletions(-)
diff --git a/guests/lcitool b/guests/lcitool
index fa60218..9dd4aea 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -252,22 +252,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
@@ -543,7 +527,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
9:42 a.m.
New subject: [libvirt-ci PATCH v2 10/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>
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
---
guests/lcitool | 40 ----------------------------------------
1 file changed, 40 deletions(-)
diff --git a/guests/lcitool b/guests/lcitool
index 9dd4aea..a93b56e 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -252,44 +252,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:
@@ -527,8 +489,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
9:42 a.m.
New subject: [libvirt-ci PATCH v2 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>
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
---
guests/config.yaml | 17 +++++++++++++++++
guests/group_vars/all/install.yml | 11 -----------
guests/lcitool | 21 +++++++++++----------
3 files changed, 28 insertions(+), 21 deletions(-)
delete mode 100644 guests/group_vars/all/install.yml
diff --git a/guests/config.yaml b/guests/config.yaml
index 05f1e8a..0b0b79c 100644
--- a/guests/config.yaml
+++ b/guests/config.yaml
@@ -15,6 +15,23 @@ install:
# cases, SSH key authentication will be used 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-related options (only apply to the 'gitlab' flavor)
gitlab:
diff --git a/guests/group_vars/all/install.yml b/guests/group_vars/all/install.yml
deleted file mode 100644
index 94b752f..0000000
--- a/guests/group_vars/all/install.yml
+++ /dev/null
@@ -1,11 +0,0 @@
----
-# Sizes are in GiB
-install_virt_type: kvm
-install_arch: x86_64
-install_machine: pc
-install_cpu_model: host-passthrough
-install_vcpus: 2
-install_memory_size: 2
-install_disk_size: 15
-install_storage_pool: default
-install_network: default
diff --git a/guests/lcitool b/guests/lcitool
index a93b56e..378c937 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -562,6 +562,7 @@ 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)
@@ -569,16 +570,16 @@ class Application:
# 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)
+ memory_arg = str(config.values["install"]["memory_size"]
* 1024)
- vcpus_arg = str(facts["install_vcpus"])
+ vcpus_arg = str(config.values["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
@@ -638,10 +639,10 @@ 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"],
+ "--virt-type",
config.values["install"]["virt_type"],
+ "--arch",
config.values["install"]["arch"],
+ "--machine",
config.values["install"]["machine"],
+ "--cpu",
config.values["install"]["cpu_model"],
"--vcpus", vcpus_arg,
"--memory", memory_arg,
"--disk", disk_arg,
@@ -658,7 +659,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
9:42 a.m.
New subject: [libvirt-ci PATCH v2 12/13] guests: README: Document the existence and usage of config.yaml
Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
Reviewed-by: Andrea Bolognani <abologna(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..0e70f5f 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 `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
+~/.config/lcitool/config.yaml, ideally by copying the `config.yaml` template,
+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 `install.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
9:42 a.m.
New subject: [libvirt-ci PATCH v2 13/13] DO NOT MERGE: Demonstrate functionality with pytest unit tests
This patch exercises the functionality added to the Config class by
defining a couple of pytest unit tests. The pytest script imports
lcitool as a local module (which is far from ideal), but in order to do
that, lcitool has to adopt the .py extension otherwise python refuses
to import it, but then again, this patch is just to showcase the
functionality.
---
guests/{lcitool => lcitool.py} | 0
guests/requirements.txt | 2 +
guests/test_config.py | 165 +++++++++++++++++++++++++++++++++
3 files changed, 167 insertions(+)
rename guests/{lcitool => lcitool.py} (100%)
create mode 100644 guests/test_config.py
diff --git a/guests/lcitool b/guests/lcitool.py
similarity index 100%
rename from guests/lcitool
rename to guests/lcitool.py
diff --git a/guests/requirements.txt b/guests/requirements.txt
index 900cfd6..c2b8f2c 100644
--- a/guests/requirements.txt
+++ b/guests/requirements.txt
@@ -1,2 +1,4 @@
ansible
jenkins-job-builder
+pytest
+mock
diff --git a/guests/test_config.py b/guests/test_config.py
new file mode 100644
index 0000000..09343cc
--- /dev/null
+++ b/guests/test_config.py
@@ -0,0 +1,165 @@
+import os
+import pytest
+import yaml
+from mock import patch
+import lcitool
+
+empty_config = ""
+
+missing_mandatory_key = \
+ """
+ install:
+ flavor: test
+ """
+
+empty_mandatory_key = \
+ """
+ install:
+ flavor: test
+ root_password:
+ """
+
+empty_optional_key = \
+ """
+ install:
+ flavor:
+ root_password: foo
+ """
+
+extra_key = \
+ """
+ install:
+ root_password: foo
+ bar: baz
+ """
+
+empty_flavor = \
+ """
+ install:
+ root_password: foo
+ """
+
+invalid_flavor = \
+ """
+ install:
+ flavor: bar
+ root_password: foo
+ """
+
+invalid_value_type = \
+ """
+ install:
+ flavor: bar
+ root_password:
+ - let
+ - me
+ - in
+ """
+
+
+def Config_init_mock(self):
+ # load only the default config template here
+ base = lcitool.Util.get_base()
+ with open(os.path.join(base, "config.yaml"), "r") as fp:
+ self.values = yaml.safe_load(fp)
+
+
+(a)pytest.fixture
+def config():
+ with patch.object(config, "__init__", Config_init_mock):
+ return lcitool.Config()
+
+
+(a)pytest.mark.parametrize("yamlstr,_pass", [
+ (empty_config, False),
+ (missing_mandatory_key, False),
+ (empty_mandatory_key, False),
+ (empty_optional_key, False),
+ (invalid_flavor, False),
+ (invalid_value_type, False),
+ (extra_key, True),
+ (empty_flavor, True),
+])
+def test_config_validate(config, yamlstr, _pass):
+ user_config = yaml.safe_load(yamlstr)
+
+ if _pass:
+ config._validate(user_config)
+ else:
+ with pytest.raises(Exception):
+ config._validate(user_config)
+
+
+yaml_in1 = \
+ """
+ install:
+ root_password: foo
+ vcpus: 4
+ """
+
+dict_out1 = {
+ "install": {
+ "flavor": "test",
+ "root_password": "foo",
+ "virt_type": "kvm",
+ "arch": "x86_64",
+ "machine": "pc",
+ "cpu_model": "host-passthrough",
+ "vcpus": 4,
+ "memory_size": 2,
+ "disk_size": 15,
+ "storage_pool": "default",
+ "network": "default"},
+ "gitlab": {
+ "url": "https://gitlab.com",
+ "runner_secret": None}
+}
+
+yaml_in2 = \
+ """
+ install:
+ flavor: gitlab
+ root_password: foo
+ virt_type: qemu
+ arch: aarch64
+ machine: q35
+ cpu_model: host-passthrough
+ vcpus: 4
+ memory_size: 4
+ disk_size: 8
+ storage_pool: foo
+ network: bar
+ gitlab:
+ url: https://example.com
+ runner_secret: foobar
+ """
+
+dict_out2 = {
+ "install": {
+ "flavor": "gitlab",
+ "root_password": "foo",
+ "virt_type": "qemu",
+ "arch": "aarch64",
+ "machine": "q35",
+ "cpu_model": "host-passthrough",
+ "vcpus": 4,
+ "memory_size": 4,
+ "disk_size": 8,
+ "storage_pool": "foo",
+ "network": "bar"},
+ "gitlab": {
+ "url": "https://example.com",
+ "runner_secret": "foobar"}
+}
+
+
+(a)pytest.mark.parametrize("yaml_in, dict_out", [
+ (yaml_in1, dict_out1),
+ (yaml_in2, dict_out2)
+])
+def test_compare_config_contents(config, yaml_in, dict_out):
+ parsed = yaml.safe_load(yaml_in)
+
+ # fill in the default values
+ config._update(parsed)
+ assert config.values == dict_out
--
2.25.3
noon
On Tue, 2020-05-12 at 16:42 +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 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 (ACKed)
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
Since v1:
- uncommented the mandatory options in the default template YAML config so that
we know about all the supported keys which is useful for validating the user
config
- removed guests/group_vars/all/install.yaml in patch 11 (which I forgot in v1)
- added checks for value types we get from the config
- use yaml.safe_load instead of yaml.load
- added code snippet to delete keys we don't recognize so as not to introduce a
security issue, because we essentially just take the config and pass it to
ansible, we don't want users to use to re-define some of Ansible's variables
- added the last patch just to demonstrate a number of test cases I used as a
proof for correctness of this revision (feel free to add more cases), but
this is not the right series to add pytest support into lcitool
You should have all the ACKs you need now.
Thanks for being patient during review, and for taking care of this
in the first place :)
--
Andrea Bolognani / Red Hat / Virtualization
3:06 a.m.
On Wed, May 13, 2020 at 07:00:49PM +0200, Andrea Bolognani wrote:
On Tue, 2020-05-12 at 16:42 +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 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 (ACKed)
>
> 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
>
> Since v1:
> - uncommented the mandatory options in the default template YAML config so that
> we know about all the supported keys which is useful for validating the user
> config
> - removed guests/group_vars/all/install.yaml in patch 11 (which I forgot in v1)
> - added checks for value types we get from the config
> - use yaml.safe_load instead of yaml.load
> - added code snippet to delete keys we don't recognize so as not to introduce a
> security issue, because we essentially just take the config and pass it to
> ansible, we don't want users to use to re-define some of Ansible's
variables
> - added the last patch just to demonstrate a number of test cases I used as a
> proof for correctness of this revision (feel free to add more cases), but
> this is not the right series to add pytest support into lcitool
You should have all the ACKs you need now.
Thanks for being patient during review, and for taking care of this
in the first place :)
Thanks for the review, changes are now merged.
--
Erik Skultety
1684
days inactive
1686
days old
19 comments
2 participants
participants (2)
-
Andrea Bolognani -
Erik Skultety