sts-sponsors team mailing list archive

[Alberto Donato <mp+437336@xxxxxxxxxxxxxxxxxx>]

Review: Approve

+1

a couple of commands/questions inline, but otherwise looks good

Diff comments:

> diff --git a/systemtests/lxd.py b/systemtests/lxd.py
> index a264e10..3de83b4 100644
> --- a/systemtests/lxd.py
> +++ b/systemtests/lxd.py
> @@ -37,21 +41,37 @@ class BadWebSocketHandshakeError(Exception):
>  
>  
>  class _FileWrapper:
> -    def __init__(self, lxd: CLILXD, instance_name: str, path: str):
> -        self._lxd = lxd
> -        self._instance_name = instance_name
> +    def __init__(self, instance: Instance, path: str):
> +        self._instance = instance
>          self.path = path
>  
>      def __repr__(self) -> str:
> -        return f"<FileWrapper for {self.path} on {self._instance_name}>"
> +        return f"<FileWrapper for {self.path} on {self._instance}>"
>  
>      def read(self) -> str:
> -        return self._lxd.get_file_contents(self._instance_name, self.path)
> +        filename = os.path.basename(self.path)
> +        with tempfile.TemporaryDirectory() as tempdir:
> +            self._instance._run(

is the plan to rework this further? It's a bit weird that the (private) method is called on the instance, but it's actually a local command (same for other uses of self._instance._run below)

> +                [
> +                    "lxc",
> +                    "file",
> +                    "--quiet",
> +                    "pull",
> +                    f"{self._instance.name}{self.path}",
> +                    f"{tempdir}/",
> +                ],
> +            )
> +            with open(os.path.join(tempdir, filename), "r") as f:
> +                return f.read()
>  
>      def write(self, content: str, uid: int = 0, gid: int = 0) -> None:
> -        return self._lxd.push_text_file(
> -            self._instance_name, content, self.path, uid=uid, gid=gid
> -        )
> +        with tempfile.NamedTemporaryFile() as source_file:
> +            source_file.write(content.encode())
> +            self.push(
> +                Path(source_file.name),
> +                uid=uid,
> +                gid=gid,
> +            )
>  
>      def append(self, content: str) -> None:
>          file_content = self.read().splitlines() if self.exists() else []
> @@ -112,67 +151,175 @@ class Instance:
>      def name(self) -> str:
>          return self._instance_name
>  
> -    @property
> -    def logger(self) -> logging.Logger:
> -        return self._lxd.logger
> -
> -    @logger.setter
> -    def logger(self, logger: logging.Logger) -> None:
> -        self._lxd.logger = logger
> +    def _run(
> +        self,
> +        cmd: list[str],
> +        prefix: Optional[list[str]] = None,
> +        logger: Optional[logging.Logger] = None,
> +    ) -> subprocess.CompletedProcess[str]:
> +        __tracebackhide__ = True
> +        if logger is None:
> +            logger = self.logger
> +        return run_with_logging(cmd, logger, prefix=prefix)
>  
>      def exists(self) -> bool:
> -        return self._lxd.container_exists(self._instance_name)
> +        try:
> +            self._run(["lxc", "info", self._instance_name])
> +        except subprocess.CalledProcessError:
> +            return False
> +        else:
> +            return True
>  
>      def create_container(
>          self, image: str, user_data: Optional[str] = None, profile: Optional[str] = None
>      ) -> Instance:
>          return self._lxd.create_container(self.name, image, user_data, profile)
>  
> +    def _get_lxc_command(self, environment: Optional[dict[str, str]]) -> list[str]:
> +        lxc_command = ["lxc", "exec", "--force-noninteractive", self._instance_name]
> +        if environment is not None:
> +            for key, value in environment.items():
> +                lxc_command.extend(["--env", f"{key}={value}"])
> +        lxc_command.append("--")
> +        return lxc_command
> +
> +    def _run_with_logger(
> +        self,
> +        executor: Callable[[], subprocess.CompletedProcess[str]],
> +        logger: Optional[logging.Logger],
> +    ) -> subprocess.CompletedProcess[str]:
> +        __tracebackhide__ = True
> +
> +        # Retry the run, excepting websocket: bad handshake errors
> +        @retry(exceptions=BadWebSocketHandshakeError, tries=3, logger=logger)
> +        def _retry_bad_handshake() -> subprocess.CompletedProcess[str]:
> +            __tracebackhide__ = True
> +            try:
> +                result = executor()
> +            except subprocess.CalledProcessError as e:
> +                if e.stderr.strip().endswith("websocket: bad handshake"):
> +                    raise BadWebSocketHandshakeError()
> +                else:
> +                    raise
> +            else:
> +                return result
> +
> +        return _retry_bad_handshake()
> +
>      def execute(
>          self, command: list[str], environment: Optional[dict[str, str]] = None
>      ) -> subprocess.CompletedProcess[str]:
> -        return self._lxd.execute(self._instance_name, command, environment=environment)
> +        __tracebackhide__ = True
> +        lxc_command = self._get_lxc_command(environment)
> +
> +        # Suppress logging of the lxc wrapper for clearer logs
> +        executor = partial(self._run, command, prefix=lxc_command)
> +        return self._run_with_logger(executor, self.logger)
>  
>      def quietly_execute(
>          self, command: list[str], environment: Optional[dict[str, str]] = None
>      ) -> subprocess.CompletedProcess[str]:
> -        return self._lxd.quietly_execute(
> -            self._instance_name, command, environment=environment
> +        """Execute a command without logging it."""
> +        __tracebackhide__ = True
> +        lxc_command = self._get_lxc_command(environment)
> +
> +        executor = partial(
> +            subprocess.run,
> +            lxc_command + command,
> +            capture_output=True,
> +            check=True,
> +            encoding="utf-8",
> +            errors="backslashreplace",
>          )
> +        return self._run_with_logger(executor, None)
>  
>      @property
>      def files(self) -> _FilesWrapper:
> -        return _FilesWrapper(self._lxd, self._instance_name)
> +        return _FilesWrapper(self)
>  
>      def get_ip_address(self) -> str:
> -        return self._lxd.get_ip_address(self._instance_name)
> +        # quieten mypy due to https://github.com/python/mypy/issues/9689

make this an XXX comment so we remember to remove it once it's fixed? (same below)

> +        @retry(
> +            exceptions=ValueError, tries=30, delay=2, backoff=1.1, logger=self.logger
> +        )  # type:ignore
> +        def _get_ip_address() -> str:
> +            result = self._run(
> +                ["lxc", "list", "--format", "csv", "-c4", f"name={self._instance_name}"]
> +            )
> +            ip, *rest = result.stdout.split()
> +            return ip

you can just return result.stdout.split()[0]

> +
> +        return _get_ip_address()
>  
>      def list_devices(self) -> list[str]:
> -        return self._lxd.list_instance_devices(self._instance_name)
> +        result = self._run(["lxc", "config", "device", "list", self._instance_name])
> +        return result.stdout.splitlines()
>  
>      def add_device(self, name: str, device_config: DeviceConfig) -> None:
> -        return self._lxd.add_instance_device(self._instance_name, name, device_config)
> +        self._run(
> +            [
> +                "lxc",
> +                "config",
> +                "device",
> +                "add",
> +                self._instance_name,
> +                name,
> +                device_config["type"],
> +            ]
> +            + fmt_lxd_options(device_config),
> +        )
>  
>      def remove_device(self, name: str) -> None:
> -        return self._lxd.remove_instance_device(self._instance_name, name)
> +        """Remove a device from this instance."""
> +
> +        # quieten mypy due to https://github.com/python/mypy/issues/9689
> +        @retry(
> +            exceptions=subprocess.CalledProcessError,
> +            tries=5,
> +            delay=0.5,
> +            logger=self.logger,
> +        )  # type:ignore
> +        def _remove_device() -> subprocess.CompletedProcess[str]:
> +            return self._run(
> +                ["lxc", "config", "device", "remove", self._instance_name, name],
> +            )
> +
> +        _remove_device()
>  
>      def start(self) -> subprocess.CompletedProcess[str]:
> -        return self._lxd.start(self._instance_name)
> +        return self._run(["lxc", "start", self._instance_name])
>  
>      def stop(self, force: bool = False) -> subprocess.CompletedProcess[str]:
> -        return self._lxd.stop(self._instance_name, force=force)
> +        argv = ["lxc", "stop", self._instance_name]
> +        if force:
> +            argv.append("--force")
> +        return self._run(argv)
>  
> -    def restart(self) -> subprocess.CompletedProcess[str]:
> -        return self._lxd.restart(self._instance_name)
> +    def restart(self, force: bool = False) -> subprocess.CompletedProcess[str]:
> +        argv = ["lxc", "restart", self._instance_name]
> +        if force:
> +            argv.append("--force")
> +        return self._run(argv)
>  
>      def delete(self) -> None:
> -        return self._lxd.delete(self._instance_name)
> -
> -    def is_running(self) -> bool:
> -        return self._lxd.is_running(self._instance_name)
> +        self._run(["lxc", "delete", "--force", self._instance_name])
>  
>      def status(self) -> str:
> -        return self._lxd.instance_status(self._instance_name)
> +        result = self._run(["lxc", "info", self._instance_name])
> +        for line in result.stdout.splitlines():
> +            key, value = line.split(": ", 1)
> +            if key == "Status":
> +                return value
> +        else:
> +            raise ValueError(f"Unable to find Status of {self._instance_name}")
> +
> +    def is_running(self) -> bool:
> +        try:
> +            status = self.status()
> +        except ValueError:
> +            return False
> +        else:
> +            return status == "RUNNING"
>  
>  
>  class CLILXD:


-- 
https://code.launchpad.net/~adam-collard/maas-ci/+git/system-tests/+merge/437336
Your team MAAS Committers is subscribed to branch ~maas-committers/maas-ci/+git/system-tests:master.