sts-sponsors team mailing list archive

[Adam Collard <mp+440179@xxxxxxxxxxxxxxxxxx>]

Diff comments:

> diff --git a/systemtests/ansible.py b/systemtests/ansible.py
> index 7c671c8..b69b96b 100644
> --- a/systemtests/ansible.py
> +++ b/systemtests/ansible.py
> @@ -300,6 +305,8 @@ class AnsibleMain:
>          self.ansible_repo_path = "/home/ubuntu/ansible_repo"
>          self.default_debug = debug or ""
>  
> +        self.runtimes: dict[str, list[float]] = {}

if you make this a defaultdict(list), then `log_runtime` can be simplified to `self.runtimes[function].append(runtime)` and drop the if

> +
>      def setup(self) -> None:
>          self.logger.info("Installing python3-pip")
>          apt_update(self.instance, environment=self._proxy_env)
> @@ -331,6 +338,27 @@ class AnsibleMain:
>      def logger(self, logger: Logger) -> None:
>          self.instance.logger = logger
>  
> +    def log_runtime(self, function: str, runtime: float) -> None:
> +        if function in self.runtimes:
> +            self.runtimes[function].append(runtime)
> +        else:
> +            self.runtimes[function] = [runtime]
> +
> +    def show_runtimes(self) -> None:
> +        def log_rt(func: str, runtime: float) -> None:
> +            self.logger.info(f"| {func}".ljust(wid) + f": {round(runtime,2)}s")

use the built in formatting

self.logger.info(f"| {func:<wid}: {runtime:.2}s")

> +
> +        if self.runtimes:
> +            wid = max([len(func) for func in self.runtimes.keys()]) + 3

1) let's just call it `width`
2) pass it through to the log_rt function so you're not reliant on the variable scoping

> +            self.logger.info("┌ Playbook execution time:")
> +            for func, runtime in self.runtimes.items():
> +                if len(runtime) == 1:
> +                    log_rt(func, runtime[0])
> +                    continue
> +                for idx, rt in enumerate(runtime):
> +                    log_rt(f"{func}-{idx}", rt)
> +            self.logger.info("└ ✔")
> +
>      @cached_property
>      def public_ssh_key(self) -> str:
>          ssh_key = self.instance.files[self.ssh_key_file]
> @@ -515,28 +543,54 @@ class AnsibleMain:
>  
>      def create_config_file(self) -> None:
>          cfg_dict = {
> -            "host_key_checking": "False",
> -            "remote_user": "ubuntu",
> -            "deprecation_warnings": "False",
> -            "callback_result_format": "yaml",
> -            "timeout": "60",
> -            "display_skipped_hosts": "False",
> -            "display_ok_hosts": "False",
> +            "defaults": {
> +                "callback_result_format": "yaml",
> +                "deprecation_warnings": "False",
> +                "display_ok_hosts": "False",
> +                "display_skipped_hosts": "False",
> +                "host_key_checking": "False",
> +                "pipelining": "True",
> +                "remote_user": "ubuntu",
> +                "timeout": "60",
> +            },
> +            "ssh_connection": {
> +                "ssh_args": "-o ControlMaster=auto -o ControlPersist=60s"
> +            },
>          }
> +        if self.use_timeout:
> +            cfg_dict["defaults"][
> +                "task_timeout"
> +            ] = f"{int(self.timeout.total_seconds())}"

= f"{self.timeout.total_seconds():.0f}

>  
>          path = f"{self.ansible_repo_path}/ansible.cfg"
>          ansible_cfg = self.instance.files[path]
> -        if not ansible_cfg.exists():
> -            ansible_cfg.write("[defaults]\ninventory = hosts\n")
> -        ansible_cfg_content = ansible_cfg.read().split("\n")
> -
> -        if self.use_timeout:
> -            cfg_dict["task_timeout"] = f"{int(self.timeout.total_seconds())}"
> -        for k, v in cfg_dict.items():
> -            if k not in ansible_cfg_content:
> -                ansible_cfg_content.append(f"{k} = {v}")
> -
> -        ansible_cfg.write("\n".join(ansible_cfg_content))
> +        _found_cfg_dict = {"defaults": {"inventory": "hosts"}}
> +        if ansible_cfg.exists():
> +            ansible_cfg_content = ansible_cfg.read()
> +            # find all sections denoted by "[<title>]"
> +            if cfg_heads := re.findall(MATCH_HEADERS, ansible_cfg_content):
> +                head_pos = [ansible_cfg_content.find(heading) for heading in cfg_heads] + [-1]

i think you can use heading.start here? see https://docs.python.org/3/library/re.html#re.Match.start

> +                # fetch all of the key, values under each column
> +                for key, start, end in zip(cfg_heads, head_pos[:-1], head_pos[1:]):
> +                    if key not in _found_cfg_dict.keys():
> +                        _found_cfg_dict[key] = {}
> +                    for entries in ansible_cfg_content[start:end].split("\n")[1:]:
> +                        k, v = entries.split("=")
> +                        _found_cfg_dict[key][k.strip()] = v.strip()
> +        for key in cfg_dict.keys():
> +            if key in _found_cfg_dict:
> +                _found_cfg_dict[key].update(cfg_dict[key])
> +            else:
> +                _found_cfg_dict[key] = cfg_dict[key]
> +
> +        # reconstruct the file format from the dictionary
> +        new_cfg_content = []
> +        for new_head, new_entries in _found_cfg_dict.items():
> +            new_cfg_content.append(f"[{new_head}]")
> +            new_cfg_content.extend([f"{new_key} = {new_value}" for new_key, new_value in new_entries.items()])
> +
> +        # and write back to the cfg file
> +        ansible_cfg.write("\n".join(new_cfg_content))
>          self.instance.execute(["mkdir", "-p", "/etc/ansible"])
>          etc_ansible_cfg = self.instance.files["/etc/ansible/ansible.cfg"]
>          etc_ansible_cfg.write(ansible_cfg.read())
> @@ -554,4 +608,8 @@ class AnsibleMain:
>          ]
>          if _debug := re.match(r"-(v)+", debug or self.default_debug or ""):
>              cmd.append(str(_debug.group()))
> -        self.instance.execute(cmd, environment=self._proxy_env)
> +        runtime = Timer(
> +            partial(self.instance.execute, command=cmd, environment=self._proxy_env)
> +        ).timeit(number=1)
> +        self.log_runtime(inspect.stack()[1].function, runtime)

eek, what are you trying to do here?

> +        self.logger.info(f"Playbook execution took {timedelta(seconds=runtime)}")


-- 
https://code.launchpad.net/~maas-committers/maas-ci/+git/system-tests/+merge/440179
Your team MAAS Committers is requested to review the proposed merge of ~maas-committers/maas-ci/+git/system-tests:improve-ansible-performance into ~maas-committers/maas-ci/+git/system-tests:master.