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