sts-sponsors team mailing list archive

[Adam Collard <mp+433880@xxxxxxxxxxxxxxxxxx>]

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.