Review: Needs Fixing
Diff comments:
> diff --git a/systemtests/ansible.py b/systemtests/ansible.py
> new file mode 100644
> index 0000000..ff2db89
> --- /dev/null
> +++ b/systemtests/ansible.py
> @@ -0,0 +1,437 @@
> +from __future__ import annotations
> +
> +import re
> +import warnings
> +from contextlib import contextmanager
> +from logging import getLogger
> +from typing import Generator
> +
> +from .lxd import get_lxd
> +from .region import MAASRegion
> +
> +NAME = "systemtests.ansible"
> +LOG = getLogger(NAME)
> +empty_dict = {"None": "None"}
> +
> +
> +class MissingRoleOnHost(Exception):
> + """Raised when a host is missing a role they were expected to have."""
> +
> + pass
> +
> +
> +class HostWithoutRole(Warning):
> + """Raised when a host does not have any assigned roles."""
> +
> + pass
> +
> +
> +class HostWithoutRegion(MissingRoleOnHost):
> + """Raised when a host attempts to access a region when
> + it does not yet have one installed."""
> +
> + pass
> +
> +
> +class CommandExecutaionFailed(Exception):
> + """Raised when the try_execute failed"""
> +
> + pass
> +
> +
> +class AnsibleShared:
hmm, this is smelly - doesn't seem to have much that's specific to ansible
> + config: dict[str, str] = {}
> + lxd = get_lxd(LOG)
> + name: str = "ansible-shared"
> + roles: dict[str, dict[str, str]] = {}
> +
> + def __init__(self, user: str = "ubuntu") -> None:
> + self.base_filepath = f"/home/{user}"
> + self.host_file = f"{self.base_filepath}/hosts"
> + self.user = user
> +
> + def __repr__(self) -> str:
> + return f"<{self.__class__.__name__} in container {self.name}"
> +
> + def apt_install(self, module: str) -> None:
> + if self.has_container:
> + self.execute(["apt-get", "update", "-y"])
> + self.execute(["dpkg", "--configure", "-a"])
> + self.execute(["apt", "install", module, "-y"])
> +
> + def module_exists(self, module: str) -> bool:
> + if self.has_container:
> + return bool(self.quietly_execute(["pip3", "list", "|", "grep",
> module]))
> + return False
> +
> + def pip_install(self, module: str) -> None:
> + if self.has_container:
> + if not self.module_exists(module):
> + self.execute(["pip3", "install", module, "-y"])
> + else:
> + self.execute(["pip3", "install", module, "--upgrade"])
> +
> + @property
> + def has_container(self) -> bool:
> + return self.lxd.container_exists(self.name)
> +
> + @property
> + def ip(self) -> str:
> + if self.has_container:
> + return self.lxd.get_ip_address(self.name)
> + return "127.0.0.1"
> +
> + def restart(self) -> None:
> + self.lxd.restart(self.name, True)
> +
> + def execute(self, command: list[str]) -> str:
> + if isinstance(command, str):
> + command = command.split(" ")
no need, let mypy verify that command is always a list[str]
> + return self.lxd.execute(self.name, command).stdout
> +
> + def try_execute(self, command: list[str]) -> str:
> + try:
> + return self.execute(command)
> + except Exception as exception:
> + LOG.warning(exception)
> + return str(exception)
this feels bad, why would we either want the stdout of a command or the str()
of an Exception? Seems like if the command failed, we could end up with some
very strange behaviour assuming that it succeeded
> +
> + def quietly_execute(self, command: list[str]) -> str:
> + return self.lxd.quietly_execute(self.name, command).stdout
> +
> + def absolute_file_path(self, file_path: str) -> str:
> + if file_path[0] == "~":
> + file_path = f"{self.base_filepath}{file_path[1:]}"
> + return file_path
> +
> + def abs_fp(self, file_path: str) -> str:
> + return self.absolute_file_path(file_path)
huh, why is this needed?
> +
> + def file_exists(self, file_path: str) -> bool:
> + return self.lxd.file_exists(self.name,
> self.absolute_file_path(file_path))
> +
> + def get_file_contents(self, file_path: str) -> str:
> + if file_path[0] == "~":
> + file_path = f"{self.base_filepath}{file_path[1:]}"
> + return self.lxd.get_file_contents(self.name,
> self.absolute_file_path(file_path))
this is redundant given absolute_file_path?
> +
> + def push_text_file(
> + self,
> + content: str,
> + target_file: str,
> + uid: int = 0,
> + gid: int = 0,
> + ) -> None:
> + self.lxd.push_text_file(
> + self.name, content, self.absolute_file_path(target_file), uid,
> gid
> + )
> +
> + def append_to_file(self, content: str, file_path: str) -> None:
> + file_content = (
> + self.get_file_contents(file_path).split("\n")
> + if self.file_exists(file_path)
> + else []
> + )
> + if file_content[-1] == "\n":
> + file_content = file_content[:-1]
> + file_content.extend(
> + content.split("\n") if isinstance(content, str) else content
> + )
> + self.push_text_file("\n".join(file_content), file_path)
> +
> + def update_config(self, config: dict[str, str], role: str = "") ->
> dict[str, str]:
> + if role:
> + if role not in self.roles:
> + raise MissingRoleOnHost()
> + self.roles[role] |= config
> + return self.roles[role]
> + self.config |= config
> + return self.config
> +
> + def fetch_config(self, config_key: str, role: str = "") -> str:
> + if role:
> + if role not in self.roles.keys():
> + raise MissingRoleOnHost()
> + role_conf = self.roles.get(role, empty_dict)
if you're already checking for role being in self.roles, you shouldn't need to
then use .get()
how about:
role_conf = self.roles.get(role)
if not role_conf:
raise MissingRoleOnHost(role)
return role_conf.get(config_key, "")
> + if config_key in role_conf.keys():
> + return role_conf.get(config_key, "")
> + return self.config.get(config_key, "")
> +
> +
> +class AnsibleHost(AnsibleShared):
> + def __init__(
> + self,
> + name: str,
> + image: str,
> + user_data: dict[str, str],
> + profile: str,
> + ) -> None:
> + self.name = name
> + self.image = image
> + self.user_data = user_data
> + self.profile = profile
> + super().__init__(image.split(":")[0])
> + self.config["ansible_user"] = str(self.user)
> +
> + def host_setup(self, role: str = "") -> str:
> + cfg = [self.ip]
> + cfg_dict: dict[str, str] = {}
> + if role:
> + cfg_dict |= self.roles.get(role, empty_dict)
why do you want None, None in this dict? Can we get rid of `empty_dict` (which
is not empty)
> + cfg_dict |= self.config
> + cfg.extend([f"{k}={v}" for k, v in cfg_dict.items()])
> + return " ".join(cfg)
> +
> + def add_test_db(self) -> AnsibleHost:
> + self.update_config({"maas_postgres_uri": "maas-test-db:///"})
> + if self.has_container:
> + self.execute(["snap", "install", "maas-test-db"])
> + return self
> +
> + def _add_role_(self, role: str, config: dict[str, str]) -> None:
strange naming, why _x_ ? _x is preferred for private methods...
> + self.roles[role] = config | {"ansible_user": str(self.user)}
> +
> + def _remove_role_(self, role: str) -> None:
> + self.roles.pop(role)
> +
> + def has_role(self, role: str) -> bool:
> + return self.has_container and role in self.roles
> +
> + @property
> + def region(self) -> MAASRegion:
> + if self.has_region:
> + url = self.fetch_config("maas_url", "maas_region_controller")
> + return MAASRegion(
> + url=url,
> + http_url=url,
> + host=str(self.name),
> + maas_container=str(self.name),
> + installed_from_snap=bool(
> + self.fetch_config(
> + "maas_installation_type", "maas_region_controller"
> + )
> + == "snap"
> + ),
> + )
> + raise HostWithoutRegion()
> +
> + @property
> + def has_rack(self) -> bool:
> + return self.has_role("maas_rack_controller")
> +
> + @property
> + def has_region(self) -> bool:
> + return self.has_role("maas_region_controller")
> +
> + @property
> + def has_region_rack(self) -> bool:
> + return self.has_role("maas_region_controller") and self.has_role(
> + "maas_rack_controller"
> + )
> +
> + def add_rack(self, config: dict[str, str] = {}) -> AnsibleHost:
> + self._add_role_("maas_rack_controller", config)
> + return self
> +
> + def add_region(self, config: dict[str, str] = {}) -> AnsibleHost:
> + self._add_role_("maas_region_controller", config)
> + return self
> +
> + def add_region_rack(self, config: dict[str, str] = {}) -> AnsibleHost:
> + self._add_role_("maas_rack_controller", config)
> + self._add_role_("maas_region_controller", config)
> + return self
> +
> + def remove_rack(self) -> None:
> + self._remove_role_("maas_rack_controller")
> +
> + def remove_region(self) -> None:
> + self._remove_role_("maas_region_controller")
> +
> + def remove_region_rack(self) -> None:
> + self._remove_role_("maas_rack_controller")
> + self._remove_role_("maas_region_controller")
> +
> +
> +class AnsibleMain(AnsibleShared):
> + _inventory_: set[AnsibleHost] = set()
> + ssh_key_file = "/home/ubuntu/.ssh/id_rsa.pub"
> +
> + def __init__(self, name: str) -> None:
> + self.name = name
> + super().__init__()
> + self.apt_install("python3")
> + self.apt_install("python3-pip")
> + self.pip_install("ansible")
> + self.clone_repo("maas", "maas-ansible-playbook")
> + assert self.public_ssh_key
> +
> + def clone_repo(self, user: str, repo: str) -> None:
this can be extracted into a stand alone function
> + repo_path = f"{self.base_filepath}/{repo}"
> + if not self.lxd.file_exists(self.name, repo_path):
> + self.execute(
> + [
> + "git",
> + "clone",
> + f"https://github.com/{user}/{repo}.git";,
> + f"{repo_path}",
> + ]
> + )
> +
> + @property
> + def public_ssh_key(self) -> str:
> + if not self.file_exists(self.ssh_key_file):
> + self.execute(["ssh-keygen", "-t", "rsa", "-f",
> self.ssh_key_file[:-4]])
we already have code in fixtures.ssh_key() to generate a key and add it to MAAS
- might be worth reworking that to have a ssh_key distinct from ssh_key_in_maas
?
> + key = self.execute(["cat", self.ssh_key_file])
> + self.execute(["chmod", "600", self.ssh_key_file.replace(".pub", "")])
> + self.execute(["chmod", "600", self.ssh_key_file])
> + return " ".join(key.split()[:2])
> +
> + def add_key_to_host(self, host: AnsibleHost) -> None:
> + self.try_execute(
> + ["ssh-keygen", "-f", self.abs_fp("~/.ssh/known_hosts"), "-R",
> host.ip]
> + )
> + host.append_to_file(
> + self.public_ssh_key, f"{host.abs_fp('~/.ssh/authorized_keys')}"
> + )
> + self.try_execute(
> + [
> + "ssh",
> + "-i",
> + self.ssh_key_file,
> + "-o",
> + "StrictHostKeyChecking=no",
> + f"{host.user}@{host.ip}",
> + ]
> + )
> + self.execute(
> + [
> + "ssh-copy-id",
> + "-i",
> + self.ssh_key_file,
> + f"{host.user}@{host.ip}",
> + ]
> + )
> +
> + @contextmanager
> + def collect_inventory(self) -> Generator[set[AnsibleHost], None, None]:
> + hosts = set()
> + for host in self._inventory_:
> + if not host.roles:
> + warnings.warn(
> + f"Empty host: {host.name} has not been assigned a role.",
> + HostWithoutRole,
> + )
> + self.lxd.create_container(
> + host.name,
> + host.image,
> + f"ssh_authorized_keys:\n- {self.public_ssh_key}",
> + )
> + if host.config.get("maas_postgres_uri", "") ==
> "maas-test-db:///":
> + host.add_test_db()
> + self.add_key_to_host(host)
> + self._inventory_.add(host)
> + hosts.add(host)
> + yield hosts
> + for host in hosts:
> + self.remove_host(host, delete=True)
> +
> + def _make_host_name_(self) -> str:
> + """Determine the smallest number not yet used as a name"""
> + name_scheme = "ansible-host"
> + nums: set[int] = set()
> + for host in self._inventory_:
> + if re.match(rf"{name_scheme}-\d+", host.name):
> + val = re.search(r"\d+", host.name)
> + if val:
> + nums.add(int(val.group()))
> + if nums:
> + return f"{name_scheme}-{min(set(range(1, max(nums)+2)) - nums)}"
> + return f"{name_scheme}-1"
> +
> + def add_host(
> + self,
> + name: str = "",
> + image: str = "ubuntu:focal",
> + user_data: dict[str, str] = empty_dict,
> + profile: str = "",
> + ) -> AnsibleHost:
> + host = AnsibleHost(
> + f"{name if name else self._make_host_name_()}",
> + image,
> + user_data,
> + profile,
> + )
> + self._inventory_.add(host)
> + return host
> +
> + def remove_host(self, host: AnsibleHost, delete: bool = False) -> None:
> + self._inventory_.discard(host)
> + if delete:
> + self.lxd.delete(host.name)
> +
> + def create_hosts_file(self) -> None:
> + inventory: dict[str, list[AnsibleHost]] = {}
> + inv: list[str] = []
> + for host in self._inventory_:
> + if not host.roles:
> + warnings.warn(
> + f"Empty host: {host.name} has not been assigned a role.",
> + HostWithoutRole,
> + )
> + continue
> + for role in host.roles:
> + if role not in inventory.keys():
> + inventory[role] = []
> + inventory[role].append(host)
> + for role, hosts in inventory.items():
> + inv.append(f"[{role}]")
> + inv.extend([host.host_setup(role) for host in hosts])
> + inv.append("")
> + if [True for key in inventory.keys() if "postgres" in key]:
> + inv.extend(
> + [
> + "[maas_postgres:children]",
> + "maas_postgres_primary",
> + "maas_postgres_secondary",
> + "",
> + ]
> + )
> + self.push_text_file("\n".join(inv), self.host_file)
> +
> + def create_config_file(self) -> None:
> + def append_if_not_found(
> + content: str, path: str, loose_check: bool = True
> + ) -> None:
> + search = content if not loose_check else content.split()[0]
> + if search not in self.get_file_contents(path):
> + self.append_to_file(content, path)
> +
> + path = "~/maas-ansible-playbook/ansible.cfg"
> + if not self.file_exists(path):
> + self.push_text_file("[defaults]\ninventory = hosts", path)
> + append_if_not_found("host_key_checking = False", path)
> + append_if_not_found("remote_user = ubuntu", path)
> + append_if_not_found("deprecation_warnings = False", path)
> + self.execute(["mkdir", "-p", "/etc/ansible"])
> + self.push_text_file(self.get_file_contents(path),
> "/etc/ansible/ansible.cfg")
> +
> + def run_playbook(self, playbook: str = "site.yaml", debug: str = "-v")
> -> None:
> + self.create_hosts_file()
> + self.create_config_file()
> + cmd = [
> + "ansible-playbook",
> + f"{self.base_filepath}/maas-ansible-playbook/{playbook}",
> + "-i",
> + self.host_file,
> + "--private-key",
> + self.ssh_key_file[:-4],
> + ]
> + _debug = re.match(r"-(v)+", str(debug))
> + if _debug:
> + cmd.append(_debug.group())
> + if self.config:
> + cfg = " ".join([f"{k}={v}" for k, v in self.config.items()])
> + cmd.append("--extra-vars")
> + cmd.append(f'"{cfg}"')
> + self.execute(cmd)
> diff --git a/systemtests/ansible_tests/__init__.py
> b/systemtests/ansible_tests/__init__.py
> new file mode 100644
> index 0000000..7c571b3
> --- /dev/null
> +++ b/systemtests/ansible_tests/__init__.py
> @@ -0,0 +1,5 @@
> +"""
> +Prepares a container running ansible, and clones maas/maas-ansible-playbooks,
> +to test their MAAS topolgy is configurable with Ansible, and that the MAAS
typo: topology
> +install behaves as expected under testing.
> +"""
> diff --git a/systemtests/ansible_tests/test_ansible.py
> b/systemtests/ansible_tests/test_ansible.py
> new file mode 100644
> index 0000000..7e6196e
> --- /dev/null
> +++ b/systemtests/ansible_tests/test_ansible.py
> @@ -0,0 +1,67 @@
> +import pytest
> +
> +from systemtests.ansible import AnsibleMain
> +
> +
> +@pytest.mark.usefixtures("ansible_main")
this seems unnecessary - ansible_main will be used automatically
> +class TestAnsibleConfig:
> + def test_setup_ansible_main(self, ansible_main: AnsibleMain) -> None:
> + assert ansible_main.module_exists("ansible")
> + assert ansible_main.file_exists(
> + f"/home/{ansible_main.user}/maas-ansible-playbook/README.md",
> + )
> +
> + def test_setup_maas_region(self, ansible_main: AnsibleMain) -> None:
> + host = ansible_main.add_host().add_region(
> + {
> + "maas_version": "3.2",
> + }
> + )
> + with ansible_main.collect_inventory() as inv:
> + assert inv == {host}
> + assert host.ip
> + host.update_config(
> + {"maas_url": f"http://{host.ip}:5240/MAAS"},
> "maas_region_controller"
> + )
> +
> + config = host.roles.get("maas_region_controller")
> + assert config == {
> + "ansible_user": "ubuntu",
> + "maas_version": "3.2",
> + "maas_url": f"http://{host.ip}:5240/MAAS";,
> + }
> + assert not ansible_main._inventory_
> + assert not host.has_container
> +
> +
> +@pytest.mark.usefixtures("ansible_main")
> +class TestAnsibleMAAS:
> + def test_maas_region_installed(self, ansible_main: AnsibleMain) -> None:
> + host = (
> + ansible_main.add_host()
> + .add_test_db()
> + .add_region_rack({"maas_version": "3.2",
> "maas_installation_type": "snap"})
> + )
> + with ansible_main.collect_inventory():
> + ansible_main.update_config({"maas_url":
> f"http://{host.ip}:5240/MAAS"})
> + ansible_main.run_playbook("site.yaml")
> + assert host.region.get_version()
> +
> + def test_maas_region_updated(self, ansible_main: AnsibleMain) -> None:
> + start_version = "3.0"
> + upgrade_version = "3.2"
> + host = (
> + ansible_main.add_host()
> + .add_test_db()
> + .add_region_rack({"maas_installation_type": "snap"})
> + )
> + ansible_main.update_config({"maas_version": start_version})
> + with ansible_main.collect_inventory():
> + ansible_main.update_config({"maas_url":
> f"http://{host.ip}:5240/MAAS"})
> + ansible_main.run_playbook("site.yaml")
> + host.region.login_user("admin")
> + assert host.region.get_version()[:3] == start_version
> +
> + ansible_main.update_config({"maas_version": upgrade_version})
> + ansible_main.run_playbook("site.yaml")
> + assert host.region.get_version()[:3] == upgrade_version
> diff --git a/systemtests/region.py b/systemtests/region.py
> index a6ef1bf..c17a6b2 100644
> --- a/systemtests/region.py
> +++ b/systemtests/region.py
> @@ -64,6 +66,27 @@ class MAASRegion:
> ]
> )
>
> + def login_user(self, user: str) -> None:
hmm, you shouldn't need this - we already have
api.UnauthenticatedMAASAPIClient.login() which returns you an
AuthenticatedAPIClient.
It's important that MAASRegion can be used with many different actors (e.g.
API, UI, ...)
> + self.execute(
> + [
> + "maas",
> + "login",
> + user,
> + f"{self.url}/api/2.0/",
> + self.get_api_token(user),
> + ]
> + )
> +
> + def get_version(self) -> str:
> + if not self.user_exists(ADMIN_USER):
> + self.create_user(ADMIN_USER, ADMIN_PASSWORD, ADMIN_EMAIL)
this is a bit magical, why should `get_version` do all these things?
Note we already have AuthenticatedAPIClient.read_version_information()
> + self.login_user(ADMIN_USER)
> + v_info = json.loads(
> + self.execute(["maas", ADMIN_USER, "version", "read"]).stdout
> + )
> + LOG.info(v_info)
> + return str(v_info.get("version", "0.0.0"))
> +
> def get_proxy_settings(self, config: dict[str, Any]) -> dict[str,
> Union[bool, str]]:
> proxy_config = config.get("proxy", {})
> http_proxy = proxy_config.get("http", "")
> diff --git a/tox.ini b/tox.ini
> index aa171e3..cb72bf4 100644
> --- a/tox.ini
> +++ b/tox.ini
> @@ -55,6 +55,11 @@ commands=
> # passenv = {{[base]passenv}}
> # """)
> #]]]
> +
be sure to clean these before pushing
> +[testenv:{opelt,stunky,vm1}]
> +description=Per-machine tests for {envname}
> +passenv = {[base]passenv}
> +
> #[[[end]]]
>
> [testenv:format]
--
https://code.launchpad.net/~maas-committers/maas-ci/+git/system-tests/+merge/433880
Your team MAAS Committers is subscribed to branch
~maas-committers/maas-ci/+git/system-tests:master.
--
Mailing list: https://launchpad.net/~sts-sponsors
Post to : sts-sponsors@lists.launchpad.net
Unsubscribe : https://launchpad.net/~sts-sponsors
More help : https://help.launchpad.net/ListHelp
