sts-sponsors team mailing list archive

[Diego Mascialino <mp+433880@xxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing



Diff comments:

> diff --git a/README.md b/README.md
> index 2a998fc..52e2d75 100644
> --- a/README.md
> +++ b/README.md
> @@ -50,7 +50,11 @@ for init in inits:
>  	docstring = textwrap.fill(module.__doc__, 80)
>  	cog.outl(f" - `{package}`: {docstring}\n")
>  ]]] -->
> -We have 4 test suites:
> +We have 5 test suites:
> + - `ansible_tests`:  Prepares a container running ansible, and clones maas/maas-ansible-playbooks,

I am not sure if we want a different test suite for Ansible.
I prefer to use the same "env_builder" test suite, and if Ansible is configured for this test execution then build environment with ansible (similar about how we currently install MAAS from snap or deb).
Because if we have a different test suite we will need also different Jenkins job definitions and not only a different configuration.

> +to test their MAAS topolgy is configurable with Ansible, and that the MAAS
> +install behaves as expected under testing.
> +
>   - `collect_sos_report`: Collect an SOS report from the test run.
>  
>   - `env_builder`:  Prepares a container with a running MAAS ready to run the tests, and writes a
> 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:
> +    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(" ")
> +        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)
> +
> +    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)
> +
> +    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))
> +
> +    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 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)
> +        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:
> +        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:
> +        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]])

Here the test execution is blocked, because this ssh-keygn execution is interactive.
One way for fixing it is:
 self.execute(["ssh-keygen", "-t", "rsa", "-f", self.ssh_key_file[:-4], '-q', '-N', ''])


Regarding to reuse "fixtures.ssh_key()":
 Our fixture is intended to have a key pair for be able to connect via ssh to MAAS deployed machines. 
 But in this case a key is needed to allow Ansible container to connect to host containers to deploy MAAS. I found not useful to modify and reuse the existing ssh_key fixture.

> +        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)


-- 
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.