From 35b79daf4b9275ac13ab3eaf873afd661f097fff Mon Sep 17 00:00:00 2001 From: Yanks Yoon <37652070+yanksyoon@users.noreply.github.com> Date: Tue, 9 Apr 2024 11:50:33 +0900 Subject: [PATCH] chore: flake8 fixes (#248) * chore: reorder config alphabetically * chore: add openstack-cloud-yaml config option * chore: reorder charm state alphabetically * chore: reorder charm state alphabetically * chore: reorder requirements.txt alphabetically * feat: initialize openstack connection * ensure cloud config dir is present * test microstack installation on gh hosted ones * test microstack installation on gh hosted ones * use two-xlarge * use one step * checkin current workflow * Add integration test * Specify microk8s-addons (disable rbac) * bootstrap lxd controller * give a try with xlarge * Refactor and add unit tests * Add call to test connection * Move openstack check to __init__ * Revert "Move openstack check to __init__" This reverts commit 103123d45e35098869f2a97d98ca9b72a330512c. * Remove status in wait_for_idle * Rename config name * Cleanup * update script * update script with shellcheck directive * Fix requirements * Use clouds.yaml for integration test * Generate clouds.yaml using sunbeam * Fix openstack connect test in setup * chore: ignore testing clouds.yaml * chore: validate input type * chore: move type validation to charm state module * feat: initial openstack build image * feat: reduce image size * feat: create instance w/ cloud-init userdata script * chore: define literal types for runner application * chore: cloudimg multiarch support * chore: factor out get runner application func * chore: type hint openstack clouds yaml dict * chore: build on test mode only * chore: refactor too inject github client than instantiate from token * test: test for openstack servers * test: openstack integration test * test: use only openstack * fix: helpers import * fix:connect cloud * debug * chore: remove unattended upgrades * fix: use vm w/ machine constraints * fix: use units in mem/disk size * fix: translate to gigabytes * fix: increase mem * fix: increase timeout * chore remove tmate debug * chore: don't use lxd profile for openstack test * chore: run modeprobe br_netfilter before lxc launch * chore: revert image brnetfilter changes * feat: on_install for openstack mode * test: add additional charm config * chore: move openstack handle to on_install hook * chore: block all hooks on openstack config * fix: add back e2e test run workflow * chore: proxy configs * chore: wait for server creation * fix: create flavor * chore: test flavour to small * debug * fix: add retry * fix: use bootstrapped microstack for image buildtesting * chore: add comment to reemove cluster bootstrap * fix: sunbeam user show (user list permission denied) * fix: stringify proxy vars * test: unit tests for openstack manager * add retry * Revert "test: use only openstack" This reverts commit cd8f391adfdae24df05a2e9fb7f29e6ad1535c14. * remove tmate debug * add back license * remove extra line * lint & test fixes * refactor openstack_manager to openstack_cloud module * refactor block_on_openstack_config * update keyerror message * test: fix test for updated error message * fix: pass state to block_on_openstack_config * refactor get_runner_applications * add pipefail for build images * add cloudconfig type * refactor modules for unit testing * fix style issues * rename build image to build lxd image * check for different subprocess error * conditional /etc/environment update * use already defined constant * charm config to constants * flake8 setup * use field validator instead of root validator (cryptic error message _-root__) * use factory dict instead of config instance method * update pyproject * fix docstring issues * fix docstring issues(complete) * docstrings-complete issues * fs mock exc call issues * flake8 decorator issues * refactor too-complex charm state * merge conflict fixes * openstack build_image too complex refactor * runner remove too complex refactor * runner manager reconcile too complex refactor * shared_fs delete too complex refactor * type annotations * no immediate re-raise * lower coverage * ignore whitelist * false lint alarm * remove merge conflicts * typo fixes * import from errors * pylint ignore direct raise * chore: remove duplicate logs * replace generic type to actual type * update docstring ignores desription --------- Co-authored-by: Christopher Bartz --- .flake8 | 10 - .woke.yaml | 6 +- pyproject.toml | 24 +- src-docs/charm.py.md | 41 +- src-docs/charm_state.py.md | 207 ++++++++-- src-docs/event_timer.py.md | 10 +- src-docs/firewall.py.md | 5 +- src-docs/github_client.py.md | 47 ++- src-docs/github_metrics.py.md | 6 + src-docs/github_type.py.md | 31 +- src-docs/lxd.py.md | 92 +++-- src-docs/lxd_type.py.md | 47 +++ src-docs/metrics.py.md | 47 +-- src-docs/openstack_manager.md | 32 +- src-docs/repo_policy_compliance_client.py.md | 8 +- src-docs/runner.py.md | 31 +- src-docs/runner_manager.py.md | 39 +- src-docs/runner_manager_type.py.md | 2 + src-docs/runner_metrics.py.md | 24 +- src-docs/runner_type.py.md | 16 + src-docs/shared_fs.py.md | 26 +- src-docs/utilities.py.md | 24 +- src/charm.py | 168 ++++---- src/charm_state.py | 378 ++++++++++++------ src/event_timer.py | 10 +- src/firewall.py | 5 +- src/github_client.py | 54 ++- src/github_metrics.py | 9 +- src/github_type.py | 44 +- src/lxd.py | 40 +- src/lxd_type.py | 50 ++- src/metrics.py | 32 +- src/openstack_cloud/__init__.py | 21 +- src/openstack_cloud/openstack_manager.py | 81 ++-- src/repo_policy_compliance_client.py | 4 + src/runner.py | 182 ++++++--- src/runner_manager.py | 129 ++++-- src/runner_manager_type.py | 2 + src/runner_metrics.py | 96 +++-- src/runner_type.py | 16 +- src/shared_fs.py | 62 ++- src/utilities.py | 29 +- tests/conftest.py | 6 +- tests/integration/conftest.py | 26 +- tests/integration/helpers.py | 55 ++- tests/integration/test_charm_fork_repo.py | 7 +- .../integration/test_charm_metrics_failure.py | 11 +- .../integration/test_charm_metrics_success.py | 10 +- tests/integration/test_charm_no_runner.py | 5 +- tests/integration/test_charm_one_runner.py | 24 +- .../test_charm_with_juju_storage.py | 3 +- tests/integration/test_charm_with_proxy.py | 28 +- tests/integration/test_debug_ssh.py | 5 +- tests/integration/test_self_hosted_runner.py | 17 +- tests/unit/conftest.py | 10 +- tests/unit/factories.py | 80 ++-- tests/unit/mock.py | 295 +++++++++++++- tests/unit/test_charm.py | 142 +++++-- tests/unit/test_charm_state.py | 9 +- tests/unit/test_event_timer.py | 12 +- tests/unit/test_github_client.py | 17 +- tests/unit/test_github_metrics.py | 7 +- tests/unit/test_metrics.py | 40 +- tests/unit/test_openstack_cloud.py | 1 - tests/unit/test_openstack_manager.py | 4 +- tests/unit/test_runner.py | 11 +- tests/unit/test_runner_logs.py | 24 +- tests/unit/test_runner_manager.py | 49 ++- tests/unit/test_runner_metrics.py | 48 ++- tests/unit/test_shared_fs.py | 71 ++-- tests/unit/test_utilities.py | 11 +- 71 files changed, 2354 insertions(+), 861 deletions(-) delete mode 100644 .flake8 diff --git a/.flake8 b/.flake8 deleted file mode 100644 index fecb88550..000000000 --- a/.flake8 +++ /dev/null @@ -1,10 +0,0 @@ -[flake8] -ignore = W503 # not compatible with black -max-line-length = 99 -select: E,W,F,C,N -exclude: - venv - .git - build - dist - *.egg_info diff --git a/.woke.yaml b/.woke.yaml index 18ecc91b4..9c0931151 100644 --- a/.woke.yaml +++ b/.woke.yaml @@ -1,3 +1,3 @@ -ignore_files: - # Ignore pylintrc as it uses non compliant terminology: whitelist - - .pylintrc +rules: + # Ignore whitelist - we are using it to ignore pydantic in pyproject.toml and pylintrc + - name: whitelist diff --git a/pyproject.toml b/pyproject.toml index 9e5e5a94b..6b48bd0cc 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ [tool.bandit] exclude_dirs = ["/venv/"] [tool.bandit.assert_used] -skips = ["*/*test.py", "*/test_*.py"] +skips = ["*/*test.py", "*/test_*.py", "*tests/*.py"] # Testing tools configuration [tool.coverage.run] @@ -12,22 +12,26 @@ branch = true omit = [ # Contains interface for calling LXD. Tested in integration tests and end to end tests. "src/lxd.py", - # Contains interface for calling repo policy compliance service. Tested in integration test and end to end tests. + # Contains interface for calling repo policy compliance service. Tested in integration test + # and end to end tests. "src/repo_policy_compliance_client.py", ] [tool.coverage.report] -fail_under = 38 +fail_under = 83 show_missing = true - [tool.pytest.ini_options] minversion = "6.0" log_cli_level = "INFO" +[tool.pylint.'MESSAGES CONTROL'] +extension-pkg-whitelist = "pydantic" + # Formatting tools configuration [tool.black] line-length = 99 +target-version = ["py310"] [tool.isort] line_length = 99 @@ -43,8 +47,8 @@ select = ["E", "W", "F", "C", "N", "R", "D", "H"] # Ignore W503, E501 because using black creates errors with this # Ignore D107 Missing docstring in __init__ ignore = ["W503", "E501", "D107"] -# D100, D101, D102, D103: Ignore missing docstrings in tests -per-file-ignores = ["tests/*:D100,D101,D102,D103,D104"] +# D100, D101, D102, D103, D104: Ignore docstring style issues in tests +per-file-ignores = ["tests/*:D100,D101,D102,D103,D104,D205,D212"] docstring-convention = "google" # Check for properly formatted copyright header in each file copyright-check = "True" @@ -52,6 +56,12 @@ copyright-author = "Canonical Ltd." copyright-regexp = "Copyright\\s\\d{4}([-,]\\d{4})*\\s+%(author)s" [tool.mypy] -ignore_missing_imports = true +check_untyped_defs = true +disallow_untyped_defs = true explicit_package_bases = true +ignore_missing_imports = true namespace_packages = true + +[[tool.mypy.overrides]] +module = "tests.*" +disallow_untyped_defs = false diff --git a/src-docs/charm.py.md b/src-docs/charm.py.md index ffa94edac..53fec787b 100644 --- a/src-docs/charm.py.md +++ b/src-docs/charm.py.md @@ -8,19 +8,24 @@ Charm for creating and managing GitHub self-hosted runner instances. **Global Variables** --------------- - **DEBUG_SSH_INTEGRATION_NAME** +- **GROUP_CONFIG_NAME** - **LABELS_CONFIG_NAME** +- **PATH_CONFIG_NAME** +- **RECONCILE_INTERVAL_CONFIG_NAME** +- **TEST_MODE_CONFIG_NAME** +- **TOKEN_CONFIG_NAME** - **RECONCILE_RUNNERS_EVENT** --- - + ## function `catch_charm_errors` ```python catch_charm_errors( - func: Callable[[~CharmT, ~EventT], NoneType] -) → Callable[[~CharmT, ~EventT], NoneType] + func: Callable[[ForwardRef('GithubRunnerCharm'), ~EventT], NoneType] +) → Callable[[ForwardRef('GithubRunnerCharm'), ~EventT], NoneType] ``` Catch common errors in charm. @@ -39,14 +44,14 @@ Catch common errors in charm. --- - + ## function `catch_action_errors` ```python catch_action_errors( - func: Callable[[~CharmT, ActionEvent], NoneType] -) → Callable[[~CharmT, ActionEvent], NoneType] + func: Callable[[ForwardRef('GithubRunnerCharm'), ActionEvent], NoneType] +) → Callable[[ForwardRef('GithubRunnerCharm'), ActionEvent], NoneType] ``` Catch common errors in actions. @@ -68,12 +73,24 @@ Catch common errors in actions. ## class `GithubRunnerCharm` Charm for managing GitHub self-hosted runners. - + + +**Attributes:** + + - `service_token_path`: The path to token to access local services. + - `repo_check_web_service_path`: The path to repo-policy-compliance service directory. + - `repo_check_web_service_script`: The path to repo-policy-compliance web service script. + - `repo_check_systemd_service`: The path to repo-policy-compliance unit file. + - `juju_storage_path`: The path to juju storage. + - `ram_pool_path`: The path to memdisk storage. + - `kernel_module_path`: The path to kernel modules. + + ### function `__init__` ```python -__init__(*args, **kargs) → None +__init__(*args: Any, **kwargs: Any) → None ``` Construct the charm. @@ -83,7 +100,13 @@ Construct the charm. **Args:** - `args`: List of arguments to be passed to the `CharmBase` class. - - `kargs`: List of keyword arguments to be passed to the `CharmBase` class. + - `kwargs`: List of keyword arguments to be passed to the `CharmBase` class. + + + +**Raises:** + + - `RuntimeError`: If invalid test configuration was detected. --- diff --git a/src-docs/charm_state.py.md b/src-docs/charm_state.py.md index 84577b6b4..42a6ef056 100644 --- a/src-docs/charm_state.py.md +++ b/src-docs/charm_state.py.md @@ -9,14 +9,27 @@ State of the Charm. --------------- - **ARCHITECTURES_ARM64** - **ARCHITECTURES_X86** +- **DENYLIST_CONFIG_NAME** +- **DOCKERHUB_MIRROR_CONFIG_NAME** +- **GROUP_CONFIG_NAME** - **OPENSTACK_CLOUDS_YAML_CONFIG_NAME** +- **PATH_CONFIG_NAME** +- **RECONCILE_INTERVAL_CONFIG_NAME** +- **RUNNER_STORAGE_CONFIG_NAME** +- **TEST_MODE_CONFIG_NAME** +- **TOKEN_CONFIG_NAME** +- **USE_APROXY_CONFIG_NAME** +- **VIRTUAL_MACHINES_CONFIG_NAME** +- **VM_CPU_CONFIG_NAME** +- **VM_MEMORY_CONFIG_NAME** +- **VM_DISK_CONFIG_NAME** - **LABELS_CONFIG_NAME** - **COS_AGENT_INTEGRATION_NAME** - **DEBUG_SSH_INTEGRATION_NAME** --- - + ## function `parse_github_path` @@ -33,6 +46,14 @@ Parse GitHub path. - `path_str`: GitHub path in string format. - `runner_group`: Runner group name for GitHub organization. If the path is a repository this argument is ignored. + + +**Raises:** + + - `CharmConfigInvalidError`: if an invalid path string was given. + + + **Returns:** GithubPath object representing the GitHub repository, or the GitHub organization with runner group information. @@ -44,6 +65,13 @@ Supported system architectures. +**Attributes:** + + - `ARM64`: Represents an ARM64 system architecture. + - `X64`: Represents an X64/AMD64 system architecture. + + + --- @@ -62,7 +90,7 @@ Some charm configurations are grouped into other configuration models. - `labels`: Additional runner labels to append to default (i.e. os, flavor, architecture). - `openstack_clouds_yaml`: The openstack clouds.yaml configuration. - `path`: GitHub repository path in the format '/', or the GitHub organization name. - - `reconcile_interval`: Time between each reconciliation of runners. + - `reconcile_interval`: Time between each reconciliation of runners in minutes. - `token`: GitHub personal access token for GitHub API. @@ -70,12 +98,12 @@ Some charm configurations are grouped into other configuration models. --- - + -### classmethod `check_fields` +### classmethod `check_reconcile_interval` ```python -check_fields(values: dict) → dict +check_reconcile_interval(reconcile_interval: int) → int ``` Validate the general charm configuration. @@ -84,16 +112,22 @@ Validate the general charm configuration. **Args:** - - `values`: Values in the pydantic model. + - `reconcile_interval`: The value of reconcile_interval passed to class instantiation. + + + +**Raises:** + + - `ValueError`: if an invalid reconcile_interval value of less than 2 has been passed. **Returns:** - Modified values in the pydantic model. + The validated reconcile_interval value. --- - + ### classmethod `from_charm` @@ -111,6 +145,12 @@ Initialize the config from charm. +**Raises:** + + - `CharmConfigInvalidError`: If any invalid configuration has been set on the charm. + + + **Returns:** Current config of the charm. @@ -126,7 +166,7 @@ Raised when charm config is invalid. - `msg`: Explanation of the error. - + ### function `__init__` @@ -159,6 +199,7 @@ The charm state. - `charm_config`: Configuration of the juju charm. - `is_metrics_logging_available`: Whether the charm is able to issue metrics. - `proxy_config`: Proxy-related configuration. + - `runner_config`: The charm configuration related to runner VM configuration. - `ssh_debug_connections`: SSH debug connections configuration information. @@ -166,7 +207,7 @@ The charm state. --- - + ### classmethod `from_charm` @@ -184,10 +225,61 @@ Initialize the state from charm. +**Raises:** + + - `CharmConfigInvalidError`: If an invalid configuration was set. + + + **Returns:** Current state of the charm. +--- + +## class `GithubConfig` +Charm configuration related to GitHub. + + + +**Attributes:** + + - `token`: The Github API access token (PAT). + - `path`: The Github org/repo path. + + + + +--- + + + +### classmethod `from_charm` + +```python +from_charm(charm: CharmBase) → GithubConfig +``` + +Get github related charm configuration values from charm. + + + +**Args:** + + - `charm`: The charm instance. + + + +**Raises:** + + - `CharmConfigInvalidError`: If an invalid configuration value was set. + + + +**Returns:** + The parsed GitHub configuration values. + + --- ## class `GithubOrg` @@ -205,7 +297,7 @@ Represent GitHub organization. --- - + ### function `path` @@ -238,7 +330,7 @@ Represent GitHub repository. --- - + ### function `path` @@ -263,6 +355,7 @@ Proxy configuration. **Attributes:** + - `aproxy_address`: The address of aproxy snap instance if use_aproxy is enabled. - `http`: HTTP proxy address. - `https`: HTTPS proxy address. - `no_proxy`: Comma-separated list of hosts that should not be proxied. @@ -279,12 +372,12 @@ Return the aproxy address. --- - + -### classmethod `check_fields` +### classmethod `check_use_aproxy` ```python -check_fields(values: dict) → dict +check_use_aproxy(use_aproxy: bool, values: dict) → bool ``` Validate the proxy configuration. @@ -293,16 +386,23 @@ Validate the proxy configuration. **Args:** + - `use_aproxy`: Value of use_aproxy variable. - `values`: Values in the pydantic model. +**Raises:** + + - `ValueError`: if use_aproxy was set but no http/https was passed. + + + **Returns:** - Modified values in the pydantic model. + Validated use_aproxy value. --- - + ### classmethod `from_charm` @@ -342,30 +442,67 @@ Runner configurations for the charm. --- - + -### classmethod `check_fields` +### classmethod `check_virtual_machine_resources` ```python -check_fields(values: dict) → dict +check_virtual_machine_resources( + vm_resources: VirtualMachineResources +) → VirtualMachineResources ``` -Validate the runner configuration. +Validate the virtual_machine_resources field values. **Args:** - - `values`: Values in the pydantic model. + - `vm_resources`: the virtual_machine_resources value to validate. + + + +**Raises:** + + - `ValueError`: if an invalid number of cpu was given or invalid memory/disk size was given. **Returns:** - Modified values in the pydantic model. + The validated virtual_machine_resources value. --- - + + +### classmethod `check_virtual_machines` + +```python +check_virtual_machines(virtual_machines: int) → int +``` + +Validate the virtual machines configuration value. + + + +**Args:** + + - `virtual_machines`: The virtual machines value to validate. + + + +**Raises:** + + - `ValueError`: if a negative integer was passed. + + + +**Returns:** + Validated virtual_machines value. + +--- + + ### classmethod `from_charm` @@ -383,6 +520,12 @@ Initialize the config from charm. +**Raises:** + + - `CharmConfigInvalidError`: if an invalid runner charm config has been set on the charm. + + + **Returns:** Current config of the charm. @@ -394,6 +537,13 @@ Supported storage as runner disk. +**Attributes:** + + - `JUJU_STORAGE`: Represents runner storage from Juju storage. + - `MEMORY`: Represents tempfs storage (ramdisk). + + + --- @@ -415,7 +565,7 @@ SSH connection information for debug workflow. --- - + ### classmethod `from_charm` @@ -432,6 +582,11 @@ Initialize the SSHDebugInfo from charm relation data. - `charm`: The charm instance. + +**Returns:** + List of connection information for ssh debug access. + + --- ## class `UnsupportedArchitectureError` @@ -443,7 +598,7 @@ Raised when given machine charm architecture is unsupported. - `arch`: The current machine architecture. - + ### function `__init__` diff --git a/src-docs/event_timer.py.md b/src-docs/event_timer.py.md index 9532bd04f..b29f90455 100644 --- a/src-docs/event_timer.py.md +++ b/src-docs/event_timer.py.md @@ -61,12 +61,12 @@ Construct the timer manager. --- - + ### function `disable_event_timer` ```python -disable_event_timer(event_name: str) +disable_event_timer(event_name: str) → None ``` Disable the systemd timer for the given event. @@ -85,7 +85,7 @@ Disable the systemd timer for the given event. --- - + ### function `ensure_event_timer` @@ -94,7 +94,7 @@ ensure_event_timer( event_name: str, interval: int, timeout: Optional[int] = None -) +) → None ``` Ensure that a systemd service and timer are registered to dispatch the given event. @@ -119,7 +119,7 @@ The timeout is the number of seconds before an event is timed out. If not set or --- - + ### function `is_active` diff --git a/src-docs/firewall.py.md b/src-docs/firewall.py.md index b5c30dc37..486c03290 100644 --- a/src-docs/firewall.py.md +++ b/src-docs/firewall.py.md @@ -58,7 +58,7 @@ Get the host IP address for the corresponding LXD network. refresh_firewall( denylist: Iterable[FirewallEntry], allowlist: Optional[Iterable[FirewallEntry]] = None -) +) → None ``` Refresh the firewall configuration. @@ -67,7 +67,8 @@ Refresh the firewall configuration. **Args:** - - `denylist`: The list of FirewallEntry objects to allow. + - `denylist`: The list of FirewallEntry rules to allow. + - `allowlist`: The list of FirewallEntry rules to allow. --- diff --git a/src-docs/github_client.py.md b/src-docs/github_client.py.md index af58b2311..9ea4b2374 100644 --- a/src-docs/github_client.py.md +++ b/src-docs/github_client.py.md @@ -10,23 +10,36 @@ Migrate to PyGithub in the future. PyGithub is still lacking some API such as re --- - + ## function `catch_http_errors` ```python -catch_http_errors(func) +catch_http_errors( + func: Callable[~ParamT, ~ReturnT] +) → Callable[~ParamT, ~ReturnT] ``` Catch HTTP errors and raise custom exceptions. + +**Args:** + + - `func`: The target function to catch common errors for. + + + +**Returns:** + The decorated function. + + --- ## class `GithubClient` GitHub API client. - + ### function `__init__` @@ -41,14 +54,13 @@ Instantiate the GiHub API client. **Args:** - `token`: GitHub personal token for API requests. - - `request_session`: Requests session for HTTP requests. --- - + ### function `delete_runner` @@ -67,7 +79,7 @@ Delete the self-hosted runner from GitHub. --- - + ### function `get_job_info` @@ -91,12 +103,19 @@ Get information about a job for a specific workflow run. +**Raises:** + + - `TokenError`: if there was an error with the Github token crdential provided. + - `JobNotFoundError`: If no jobs were found. + + + **Returns:** Job information. --- - + ### function `get_runner_application` @@ -131,7 +150,7 @@ Get runner application available for download for given arch. --- - + ### function `get_runner_github_info` @@ -154,7 +173,7 @@ Get runner information on GitHub under a repo or org. --- - + ### function `get_runner_registration_token` @@ -177,7 +196,7 @@ Get token from GitHub used for registering runners. --- - + ### function `get_runner_remove_token` @@ -189,7 +208,13 @@ Get token from GitHub used for removing runners. +**Args:** + + - `path`: The Github org/repo path. + + + **Returns:** - The removing token. + The removing token. diff --git a/src-docs/github_metrics.py.md b/src-docs/github_metrics.py.md index e6174445b..b782a2d93 100644 --- a/src-docs/github_metrics.py.md +++ b/src-docs/github_metrics.py.md @@ -34,6 +34,12 @@ The Github API is accessed to retrieve the job data for the runner. +**Raises:** + + - `GithubMetricsError`: If the job for given workflow run is not found. + + + **Returns:** The job metrics. diff --git a/src-docs/github_type.py.md b/src-docs/github_type.py.md index 49c31b468..a77fccade 100644 --- a/src-docs/github_type.py.md +++ b/src-docs/github_type.py.md @@ -14,6 +14,13 @@ Status of runner on GitHub. +**Attributes:** + + - `ONLINE`: Represents an online runner status. + - `OFFLINE`: Represents an offline runner status. + + + --- @@ -21,6 +28,20 @@ Status of runner on GitHub. ## class `JobConclusion` Conclusion of a job on GitHub. +See :https://docs.github.com/en/rest/actions/workflow-runs?apiVersion=2022-11-28#list-workflow-runs-for-a-repository + + + +**Attributes:** + + - `ACTION_REQUIRED`: Represents additional action required on the job. + - `CANCELLED`: Represents a cancelled job status. + - `FAILURE`: Represents a failed job status. + - `NEUTRAL`: Represents a job status that can optionally succeed or fail. + - `SKIPPED`: Represents a skipped job status. + - `SUCCESS`: Represents a successful job status. + - `TIMED_OUT`: Represents a job that has timed out. + @@ -36,6 +57,7 @@ Stats for a job on GitHub. - `created_at`: The time the job was created. - `started_at`: The time the job was started. + - `conclusion`: The end result of a job. @@ -87,7 +109,7 @@ Information on the runner application. - `download_url`: URL to download the runner application. - `filename`: Filename of the runner application. - `temp_download_token`: A short lived bearer token used to download the runner, if needed. - - `sha256_check_sum`: SHA256 Checksum of the runner application. + - `sha256_checksum`: SHA256 Checksum of the runner application. @@ -102,11 +124,12 @@ Information on a single self-hosted runner. **Attributes:** - - `id`: Unique identifier of the runner. - - `name`: Name of the runner. - - `os`: Operation system of the runner. - `busy`: Whether the runner is executing a job. + - `id`: Unique identifier of the runner. - `labels`: Labels of the runner. + - `os`: Operation system of the runner. + - `name`: Name of the runner. + - `status`: The Github runner status. diff --git a/src-docs/lxd.py.md b/src-docs/lxd.py.md index cca9f0abf..b51bc3aac 100644 --- a/src-docs/lxd.py.md +++ b/src-docs/lxd.py.md @@ -17,12 +17,12 @@ The LxdClient class offers a low-level interface to isolate the underlying imple ## class `LxdClient` LXD client. - + ### function `__init__` ```python -__init__() +__init__() → None ``` Instantiate the LXD client. @@ -36,7 +36,7 @@ Instantiate the LXD client. ## class `LxdImageManager` LXD image manager. - + ### function `__init__` @@ -57,12 +57,12 @@ Instantiate the LXD image manager. --- - + ### function `create` ```python -create(name: 'str', path: 'Path') +create(name: 'str', path: 'Path') → None ``` Import a LXD image. @@ -94,7 +94,7 @@ An LXD instance. - `files` (LxdInstanceFiles): Manager for the files on the LXD instance. - `status` (str): Status of the LXD instance. - + ### function `__init__` @@ -126,7 +126,7 @@ Status of the LXD instance. --- - + ### function `delete` @@ -150,7 +150,7 @@ Delete the LXD instance. --- - + ### function `execute` @@ -159,7 +159,7 @@ execute( cmd: 'list[str]', cwd: 'Optional[str]' = None, hide_cmd: 'bool' = False, - **kwargs + **kwargs: 'Any' ) → Tuple[int, IO, IO] ``` @@ -187,7 +187,7 @@ The command is executed with `subprocess.run`, additional arguments can be passe --- - + ### function `start` @@ -213,7 +213,7 @@ Start the LXD instance. --- - + ### function `stop` @@ -284,16 +284,16 @@ Create a directory in the LXD instance. **Args:** - - `dir`: Name of the directory to create. + - `dir_name`: Name of the directory to create. --- - + ### function `pull_file` ```python -pull_file(source: 'str', destination: 'str', is_dir=False) → None +pull_file(source: 'str', destination: 'str', is_dir: 'bool' = False) → None ``` Pull a file from the LXD instance to the local machine. @@ -344,7 +344,7 @@ Push a file to the LXD instance. --- - + ### function `read_file` @@ -407,7 +407,7 @@ Write a file with the given content into the LXD instance. ## class `LxdInstanceManager` LXD instance manager. - + ### function `__init__` @@ -428,7 +428,7 @@ Instantiate the LXD instance manager. --- - + ### function `all` @@ -451,7 +451,7 @@ Get list of LXD instances. --- - + ### function `create` @@ -485,7 +485,7 @@ Create an LXD instance. ## class `LxdNetworkManager` LXD network manager. - + ### function `__init__` @@ -506,7 +506,7 @@ Instantiate the LXD profile manager. --- - + ### function `get` @@ -533,7 +533,7 @@ Get the LXD network information. ## class `LxdProfile` LXD profile. - + ### function `__init__` @@ -554,24 +554,24 @@ Instantiate the LXD profile. --- - + ### function `delete` ```python -delete() +delete() → None ``` Delete the profile. --- - + ### function `save` ```python -save() +save() → None ``` Save the current configuration of profile. @@ -582,7 +582,7 @@ Save the current configuration of profile. ## class `LxdProfileManager` LXD profile manager. - + ### function `__init__` @@ -603,7 +603,7 @@ Instantiate the LXD profile manager. --- - + ### function `create` @@ -622,7 +622,8 @@ Create an LXD profile. **Args:** - `name`: Name of the LXD profile to create. - - `config`: Configuration of the LXD profile. devices Devices configuration of the LXD profile. + - `config`: Configuration of the LXD profile. + - `devices`: Devices configuration of the LXD profile. @@ -632,7 +633,7 @@ Create an LXD profile. --- - + ### function `exists` @@ -661,7 +662,7 @@ Check whether an LXD profile of a given name exists. --- - + ### function `get` @@ -684,6 +685,11 @@ Get an LXD profile. - `LxdError`: Unable to get the LXD profile with the name. + +**Returns:** + LXDProfile with given name. + + --- ## class `LxdStoragePool` @@ -699,7 +705,7 @@ An LXD storage pool. - `config` (dict[str, any]): Dictionary of the configuration of the storage pool. - `managed` (bool): Whether LXD manages the storage pool. - + ### function `__init__` @@ -720,24 +726,24 @@ Instantiate the LXD storage pool. --- - + ### function `delete` ```python -delete() +delete() → None ``` Delete the storage pool. --- - + ### function `save` ```python -save() +save() → None ``` Save the current configuration of storage pool. @@ -748,7 +754,7 @@ Save the current configuration of storage pool. ## class `LxdStoragePoolManager` LXD storage pool manager. - + ### function `__init__` @@ -769,7 +775,7 @@ Instantiate the LXD storage pool manager. --- - + ### function `all` @@ -786,7 +792,7 @@ Get all LXD storage pool. --- - + ### function `create` @@ -809,7 +815,7 @@ Create an LXD storage pool. --- - + ### function `exists` @@ -832,7 +838,7 @@ Check if an LXD storage pool exists. --- - + ### function `get` @@ -850,6 +856,12 @@ Get an LXD storage pool. +**Raises:** + + - `LxdError`: If the storage pool with given name was not found. + + + **Returns:** The LXD storage pool. diff --git a/src-docs/lxd_type.py.md b/src-docs/lxd_type.py.md index 22d68951a..0b4ec54bb 100644 --- a/src-docs/lxd_type.py.md +++ b/src-docs/lxd_type.py.md @@ -18,6 +18,18 @@ The unit of storage and network limits can be found here: https://linuxcontainer ## class `LxdInstanceConfig` Configuration for the LXD instance. +See https://documentation.ubuntu.com/lxd/en/latest/howto/instances_create/ + + + +**Attributes:** + + - `name`: Name of the instance. + - `type`: Instance type, i.e. "container" or "virtual-machine". + - `source`: Instance creation source configuration. + - `ephemeral`: Whether the container should be deleted after a single run. + - `profiles`: List of LXD profiles applied to the instance. + @@ -29,6 +41,15 @@ Configuration for source image in the LXD instance. +**Attributes:** + + - `type`: Type of source configuration, e.g. image, disk + - `server`: The source server URL, e.g. https://cloud-images.ubuntu.com/releases + - `protocol`: Protocol of the configuration, e.g. simplestreams + - `alias`: Alias for configuration. + + + --- @@ -38,6 +59,17 @@ LXD network information. +**Attributes:** + + - `name`: The name of LXD network. + - `description`: LXD network descriptor. + - `type`: Network type, i.e. "bridge", "physical" + - `config`: The LXD network configuration values. + - `managed`: Whether the network is being managed by lxd. + - `used_by`: Number of instances using the network. + + + --- @@ -74,6 +106,13 @@ Configuration of the storage pool. +**Attributes:** + + - `source`: The storage pool configuration source image. + - `size`: The size of the storage pool, e.g. 30GiB + + + --- @@ -83,5 +122,13 @@ Configuration for LXD storage pool. +**Attributes:** + + - `name`: The storage pool name. + - `driver`: The storage driver being used, i.e. "dir", "btrfs", ... . See https://documentation.ubuntu.com/lxd/en/stable-5.0/reference/storage_drivers/ for more information. + - `config`: The storage pool configuration. + + + diff --git a/src-docs/metrics.py.md b/src-docs/metrics.py.md index eb35b7f84..7b74c161b 100644 --- a/src-docs/metrics.py.md +++ b/src-docs/metrics.py.md @@ -12,7 +12,7 @@ Models and functions for the metric events. --- - + ## function `issue_event` @@ -39,12 +39,12 @@ The metric event is logged to the metrics log. --- - + ## function `setup_logrotate` ```python -setup_logrotate() +setup_logrotate() → None ``` Configure logrotate for the metrics log. @@ -85,12 +85,12 @@ Base class for metric events. - `timestamp`: The UNIX time stamp of the time at which the event was originally issued. - `event`: The name of the event. Will be set to the class name in snake case if not provided. - + ### function `__init__` ```python -__init__(*args, **kwargs) +__init__(*args: Any, **kwargs: Any) ``` Initialize the event. @@ -99,8 +99,8 @@ Initialize the event. **Args:** - - `*args`: The positional arguments to pass to the base class. - - `**kwargs`: The keyword arguments to pass to the base class. These are used to set the specific fields. E.g. timestamp=12345 will set the timestamp field to 12345. + - `args`: The positional arguments to pass to the base class. + - `kwargs`: The keyword arguments to pass to the base class. These are used to set the specific fields. E.g. timestamp=12345 will set the timestamp field to 12345. @@ -120,12 +120,12 @@ Metric event for when the charm has finished reconciliation. - `idle_runners`: The number of idle runners. - `duration`: The duration of the reconciliation in seconds. - + ### function `__init__` ```python -__init__(*args, **kwargs) +__init__(*args: Any, **kwargs: Any) ``` Initialize the event. @@ -134,8 +134,8 @@ Initialize the event. **Args:** - - `*args`: The positional arguments to pass to the base class. - - `**kwargs`: The keyword arguments to pass to the base class. These are used to set the specific fields. E.g. timestamp=12345 will set the timestamp field to 12345. + - `args`: The positional arguments to pass to the base class. + - `kwargs`: The keyword arguments to pass to the base class. These are used to set the specific fields. E.g. timestamp=12345 will set the timestamp field to 12345. @@ -153,12 +153,12 @@ Metric event for when a runner is installed. - `flavor`: Describes the characteristics of the runner. The flavor could be for example "small". - `duration`: The duration of the installation in seconds. - + ### function `__init__` ```python -__init__(*args, **kwargs) +__init__(*args: Any, **kwargs: Any) ``` Initialize the event. @@ -167,8 +167,8 @@ Initialize the event. **Args:** - - `*args`: The positional arguments to pass to the base class. - - `**kwargs`: The keyword arguments to pass to the base class. These are used to set the specific fields. E.g. timestamp=12345 will set the timestamp field to 12345. + - `args`: The positional arguments to pass to the base class. + - `kwargs`: The keyword arguments to pass to the base class. These are used to set the specific fields. E.g. timestamp=12345 will set the timestamp field to 12345. @@ -190,12 +190,12 @@ Metric event for when a runner is started. - `idle`: The idle time in seconds. - `queue_duration`: The time in seconds it took before the runner picked up the job. This is optional as we rely on the Github API and there may be problems retrieving the data. - + ### function `__init__` ```python -__init__(*args, **kwargs) +__init__(*args: Any, **kwargs: Any) ``` Initialize the event. @@ -204,8 +204,8 @@ Initialize the event. **Args:** - - `*args`: The positional arguments to pass to the base class. - - `**kwargs`: The keyword arguments to pass to the base class. These are used to set the specific fields. E.g. timestamp=12345 will set the timestamp field to 12345. + - `args`: The positional arguments to pass to the base class. + - `kwargs`: The keyword arguments to pass to the base class. These are used to set the specific fields. E.g. timestamp=12345 will set the timestamp field to 12345. @@ -227,13 +227,14 @@ Metric event for when a runner is stopped. - `status`: A string describing the reason for stopping the runner. - `status_info`: More information about the status. - `job_duration`: The duration of the job in seconds. + - `job_conclusion`: The job conclusion, e.g. "success", "failure", ... - + ### function `__init__` ```python -__init__(*args, **kwargs) +__init__(*args: Any, **kwargs: Any) ``` Initialize the event. @@ -242,8 +243,8 @@ Initialize the event. **Args:** - - `*args`: The positional arguments to pass to the base class. - - `**kwargs`: The keyword arguments to pass to the base class. These are used to set the specific fields. E.g. timestamp=12345 will set the timestamp field to 12345. + - `args`: The positional arguments to pass to the base class. + - `kwargs`: The keyword arguments to pass to the base class. These are used to set the specific fields. E.g. timestamp=12345 will set the timestamp field to 12345. diff --git a/src-docs/openstack_manager.md b/src-docs/openstack_manager.md index ef228b256..2e36391c0 100644 --- a/src-docs/openstack_manager.md +++ b/src-docs/openstack_manager.md @@ -13,7 +13,7 @@ Module for handling interactions with OpenStack. --- - + ## function `build_image` @@ -33,6 +33,7 @@ Build and upload an image to OpenStack. **Args:** + - `arch`: The system architecture to build the image for. - `cloud_config`: The cloud configuration to connect OpenStack with. - `github_client`: The Github client to interact with Github API. - `path`: Github organisation or repository path. @@ -42,7 +43,7 @@ Build and upload an image to OpenStack. **Raises:** - - `ImageBuildError`: If there were errors building/creating the image. + - `OpenstackImageBuildError`: If there were errors building/creating the image. @@ -52,7 +53,7 @@ Build and upload an image to OpenStack. --- - + ## function `create_instance_config` @@ -72,14 +73,19 @@ Create an instance config from charm data. **Args:** - `unit_name`: The charm unit name. - - `image`: Ubuntu image flavor. + - `openstack_image`: The openstack image object to create the instance with. - `path`: Github organisation or repository path. - `github_client`: The Github client to interact with Github API. + +**Returns:** + Instance configuration created. + + --- - + ## function `create_instance` @@ -101,6 +107,7 @@ Create an OpenStack instance. - `cloud_config`: The cloud configuration to connect Openstack with. - `instance_config`: The configuration values for Openstack instance to launch. + - `proxies`: HTTP proxy settings. dockerhub_mirror: ssh_debug_connections: @@ -130,14 +137,14 @@ Wrapper class to proxy values to string. --- - + ## class `InstanceConfig` The configuration values for creating a single runner instance. -**Args:** +**Attributes:** - `name`: Name of the image to launch the GitHub runner instance with. - `labels`: The runner instance labels. @@ -167,3 +174,14 @@ __init__( +--- + + + +## class `ImageDeleteError` +Represents an error while deleting existing openstack image. + + + + + diff --git a/src-docs/repo_policy_compliance_client.py.md b/src-docs/repo_policy_compliance_client.py.md index 30ee1c57d..ccf7dbf1a 100644 --- a/src-docs/repo_policy_compliance_client.py.md +++ b/src-docs/repo_policy_compliance_client.py.md @@ -54,7 +54,13 @@ Get a single-use token for repo policy compliance check. +**Raises:** + + - `HTTPError`: If there was an error getting one-time token from repo-policy-compliance service. + + + **Returns:** - The one-time token to be used in a single request of repo policy compliance check. + The one-time token to be used in a single request of repo policy compliance check. diff --git a/src-docs/runner.py.md b/src-docs/runner.py.md index fa65652f0..2ab0f3b08 100644 --- a/src-docs/runner.py.md +++ b/src-docs/runner.py.md @@ -22,7 +22,7 @@ The configuration values for creating a single runner instance. -**Args:** +**Attributes:** - `image`: Name of the image to launch the LXD instance with. - `resources`: Resource setting for the LXD instance. @@ -39,7 +39,17 @@ The configuration values for creating a single runner instance. ## class `Runner` Single instance of GitHub self-hosted runner. - + + +**Attributes:** + + - `runner_application`: The runner application directory path + - `env_file`: The runner environment source .env file path. + - `config_script`: The runner configuration script file path. + - `runner_script`: The runner start script file path. + - `pre_job_script`: The runner pre_job script file path. This is referenced in the env_file in the ACTIONS_RUNNER_HOOK_JOB_STARTED environment variable. + + ### function `__init__` @@ -60,6 +70,7 @@ Construct the runner instance. - `clients`: Clients to access various services. - `runner_config`: Configuration of the runner instance. + - `runner_status`: Status info of the given runner. - `instance`: LXD instance of the runner if already created. @@ -67,12 +78,12 @@ Construct the runner instance. --- - + ### function `create` ```python -create(config: CreateRunnerConfig) +create(config: CreateRunnerConfig) → None ``` Create the runner instance on LXD and register it on GitHub. @@ -91,7 +102,7 @@ Create the runner instance on LXD and register it on GitHub. --- - + ### function `remove` @@ -121,6 +132,14 @@ This class represents a snap installation. +**Attributes:** + + - `name`: The snap application name. + - `channel`: The channel to install the snap from. + - `revision`: The revision number of the snap installation. + + + --- @@ -130,7 +149,7 @@ The executable to be installed through wget. -**Args:** +**Attributes:** - `url`: The URL of the executable binary. - `cmd`: Executable command name. E.g. yq_linux_amd64 -> yq diff --git a/src-docs/runner_manager.py.md b/src-docs/runner_manager.py.md index 1b4c074bd..6c9e5cd76 100644 --- a/src-docs/runner_manager.py.md +++ b/src-docs/runner_manager.py.md @@ -16,7 +16,14 @@ Runner Manager manages the runners on LXD and GitHub. ## class `RunnerManager` Manage a group of runners according to configuration. - + + +**Attributes:** + + - `runner_bin_path`: The github runner app scripts path. + - `cron_path`: The path to runner build image cron job. + + ### function `__init__` @@ -43,7 +50,7 @@ Construct RunnerManager object for creating and managing runners. --- - + ### function `build_runner_image` @@ -59,11 +66,11 @@ Build container image in test mode, else virtual machine image. **Raises:** - - `LxdError`: Unable to build the LXD image. + - `SubprocessError`: Unable to build the LXD image. --- - + ### function `check_runner_bin` @@ -80,7 +87,7 @@ Check if runner binary exists. --- - + ### function `flush` @@ -98,12 +105,18 @@ Remove existing runners. +**Raises:** + + - `GithubClientError`: If there was an error getting remove-token to unregister runners from GitHub. + + + **Returns:** Number of runners removed. --- - + ### function `get_github_info` @@ -120,7 +133,7 @@ Get information on the runners from GitHub. --- - + ### function `get_latest_runner_bin_url` @@ -151,7 +164,7 @@ The runner binary URL changes when a new version is available. --- - + ### function `reconcile` @@ -175,7 +188,7 @@ Bring runners in line with target. --- - + ### function `schedule_build_runner_image` @@ -187,7 +200,7 @@ Install cron job for building runner image. --- - + ### function `update_runner_bin` @@ -206,3 +219,9 @@ Remove the existing runner binary to prevent it from being used. This is done to - `binary`: Information on the runner binary to download. + +**Raises:** + + - `RunnerBinaryError`: If there was an error updating runner binary info. + + diff --git a/src-docs/runner_manager_type.py.md b/src-docs/runner_manager_type.py.md index 51e647439..d529b49df 100644 --- a/src-docs/runner_manager_type.py.md +++ b/src-docs/runner_manager_type.py.md @@ -57,6 +57,7 @@ Clients for accessing various services. - `github`: Used to query GitHub API. - `jinja`: Used for templating. - `lxd`: Used to interact with LXD API. + - `repo`: Used to interact with repo-policy-compliance API. @@ -71,6 +72,7 @@ Configuration of runner manager. **Attributes:** + - `are_metrics_enabled`: Whether metrics for the runners should be collected. - `charm_state`: The state of the charm. - `image`: Name of the image for creating LXD instance. - `lxd_storage_path`: Path to be used as LXD storage. diff --git a/src-docs/runner_metrics.py.md b/src-docs/runner_metrics.py.md index bdcd0556c..3d116e801 100644 --- a/src-docs/runner_metrics.py.md +++ b/src-docs/runner_metrics.py.md @@ -14,12 +14,12 @@ Classes and function to extract the metrics from a shared filesystem. --- - + ## function `extract` ```python -extract(ignore_runners: set[str]) → Iterator[RunnerMetrics] +extract(ignore_runners: set[str]) → Generator[RunnerMetrics, NoneType, NoneType] ``` Extract metrics from runners. @@ -38,13 +38,13 @@ In order to avoid DoS attacks, the file size is also checked. -**Returns:** - An iterator over the extracted metrics. +**Yields:** + Extracted runner metrics of a particular runner. --- - + ## function `issue_events` @@ -94,7 +94,7 @@ Metrics for the post-job phase of a runner. -**Args:** +**Attributes:** - `timestamp`: The UNIX time stamp of the time at which the event was originally issued. - `status`: The status of the job. @@ -111,6 +111,14 @@ The status of the post-job phase of a runner. +**Attributes:** + + - `NORMAL`: Represents a normal post-job. + - `ABNORMAL`: Represents an error with post-job. + - `REPO_POLICY_CHECK_FAILURE`: Represents an error with repo-policy-compliance check. + + + --- @@ -120,7 +128,7 @@ Metrics for the pre-job phase of a runner. -**Args:** +**Attributes:** - `timestamp`: The UNIX time stamp of the time at which the event was originally issued. - `workflow`: The workflow name. @@ -139,7 +147,7 @@ Metrics for a runner. -**Args:** +**Attributes:** - `installed_timestamp`: The UNIX time stamp of the time at which the runner was installed. - `pre_job`: The metrics for the pre-job phase. diff --git a/src-docs/runner_type.py.md b/src-docs/runner_type.py.md index 5bb649b03..8a97cc0ce 100644 --- a/src-docs/runner_type.py.md +++ b/src-docs/runner_type.py.md @@ -14,6 +14,15 @@ Represent HTTP-related proxy settings. +**Attributes:** + + - `no_proxy`: The comma separated URLs to not go through proxy. + - `http`: HTTP proxy URL. + - `https`: HTTPS proxy URL. + - `aproxy_address`: Aproxy URL. + + + --- @@ -23,6 +32,13 @@ Set of runners LXD instance by health state. +**Attributes:** + + - `healthy`: Runners that are correctly running runner script. + - `unhealthy`: Runners that are not running runner script. + + + --- diff --git a/src-docs/shared_fs.py.md b/src-docs/shared_fs.py.md index 9caf440ce..59add9fa4 100644 --- a/src-docs/shared_fs.py.md +++ b/src-docs/shared_fs.py.md @@ -13,7 +13,7 @@ Classes and functions to operate on the shared filesystem between the charm and --- - + ## function `create` @@ -45,25 +45,25 @@ The method is not idempotent and will raise an exception if the shared filesyste --- - + ## function `list_all` ```python -list_all() → Iterator[SharedFilesystem] +list_all() → Generator[SharedFilesystem, NoneType, NoneType] ``` List the shared filesystems. -**Returns:** - An iterator over shared filesystems. +**Yields:** + A shared filesystem instance. --- - + ## function `get` @@ -95,7 +95,7 @@ Mounts the filesystem if it is not currently mounted. --- - + ## function `delete` @@ -120,7 +120,7 @@ Delete the shared filesystem for the runner. --- - + ## function `move_to_quarantine` @@ -156,8 +156,14 @@ Shared filesystem between the charm and the runners. - `path`: The path of the shared filesystem inside the charm. - `runner_name`: The name of the associated runner. -**Returns:** - The shared filesystem. + + + + +--- + +## class `UnmountSharedFilesystemError` +Represents an error unmounting a shared filesystem. diff --git a/src-docs/utilities.py.md b/src-docs/utilities.py.md index 044433766..bebbc9347 100644 --- a/src-docs/utilities.py.md +++ b/src-docs/utilities.py.md @@ -34,7 +34,7 @@ Parameterize the decorator for adding retry to functions. - `delay`: Time in seconds to wait between retry. - `max_delay`: Max time in seconds to wait between retry. - `backoff`: Factor to increase the delay by each retry. - - `logger`: Logger for logging. + - `local_logger`: Logger for logging. @@ -44,7 +44,7 @@ Parameterize the decorator for adding retry to functions. --- - + ## function `secure_run_subprocess` @@ -52,7 +52,7 @@ Parameterize the decorator for adding retry to functions. secure_run_subprocess( cmd: Sequence[str], hide_cmd: bool = False, - **kwargs + **kwargs: dict[str, Any] ) → CompletedProcess[bytes] ``` @@ -78,7 +78,7 @@ The command is executed with `subprocess.run`, additional arguments can be passe --- - + ## function `execute_command` @@ -86,7 +86,7 @@ The command is executed with `subprocess.run`, additional arguments can be passe execute_command( cmd: Sequence[str], check_exit: bool = True, - **kwargs + **kwargs: Any ) → tuple[str, int] ``` @@ -118,7 +118,7 @@ The output is logged if the log level of the logger is set to debug. --- - + ## function `get_env_var` @@ -144,7 +144,7 @@ Looks for all upper-case and all low-case of the `env_var`. --- - + ## function `set_env_var` @@ -166,7 +166,7 @@ Set the all upper case and all low case of the `env_var`. --- - + ## function `bytes_with_unit_to_kib` @@ -182,6 +182,14 @@ Convert a positive integer followed by a unit to number of kibibytes. - `num_bytes`: A positive integer followed by one of the following unit: KiB, MiB, GiB, TiB, PiB, EiB. + + +**Raises:** + + - `ValueError`: If invalid unit was detected. + + + **Returns:** Number of kilobytes. diff --git a/src/charm.py b/src/charm.py index e595d195a..aab0cdbeb 100755 --- a/src/charm.py +++ b/src/charm.py @@ -24,20 +24,26 @@ ActionEvent, CharmBase, ConfigChangedEvent, + EventBase, InstallEvent, StartEvent, StopEvent, UpdateStatusEvent, UpgradeCharmEvent, ) -from ops.framework import EventBase, StoredState +from ops.framework import StoredState from ops.main import main from ops.model import ActiveStatus, BlockedStatus, MaintenanceStatus import metrics from charm_state import ( DEBUG_SSH_INTEGRATION_NAME, + GROUP_CONFIG_NAME, LABELS_CONFIG_NAME, + PATH_CONFIG_NAME, + RECONCILE_INTERVAL_CONFIG_NAME, + TEST_MODE_CONFIG_NAME, + TOKEN_CONFIG_NAME, CharmConfigInvalidError, CharmState, GithubPath, @@ -75,11 +81,12 @@ class ReconcileRunnersEvent(EventBase): """Event representing a periodic check to ensure runners are ok.""" -CharmT = TypeVar("CharmT") EventT = TypeVar("EventT") -def catch_charm_errors(func: Callable[[CharmT, EventT], None]) -> Callable[[CharmT, EventT], None]: +def catch_charm_errors( + func: Callable[["GithubRunnerCharm", EventT], None] +) -> Callable[["GithubRunnerCharm", EventT], None]: """Catch common errors in charm. Args: @@ -90,7 +97,13 @@ def catch_charm_errors(func: Callable[[CharmT, EventT], None]) -> Callable[[Char """ @functools.wraps(func) - def func_with_catch_errors(self, event: EventT) -> None: + # flake8 thinks the event argument description is missing in the docstring. + def func_with_catch_errors(self: "GithubRunnerCharm", event: EventT) -> None: + """Handle errors raised while handling charm events. + + Args: + event: The charm event to handle. + """ # noqa: D417 try: func(self, event) except ConfigurationError as err: @@ -115,8 +128,8 @@ def func_with_catch_errors(self, event: EventT) -> None: def catch_action_errors( - func: Callable[[CharmT, ActionEvent], None] -) -> Callable[[CharmT, ActionEvent], None]: + func: Callable[["GithubRunnerCharm", ActionEvent], None] +) -> Callable[["GithubRunnerCharm", ActionEvent], None]: """Catch common errors in actions. Args: @@ -127,7 +140,13 @@ def catch_action_errors( """ @functools.wraps(func) - def func_with_catch_errors(self, event: ActionEvent) -> None: + # flake8 thinks the event argument description is missing in the docstring. + def func_with_catch_errors(self: "GithubRunnerCharm", event: ActionEvent) -> None: + """Handle errors raised while handling events. + + Args: + event: The action event to catch for errors. + """ # noqa: D417 try: func(self, event) except ConfigurationError as err: @@ -147,7 +166,17 @@ def func_with_catch_errors(self, event: ActionEvent) -> None: class GithubRunnerCharm(CharmBase): - """Charm for managing GitHub self-hosted runners.""" + """Charm for managing GitHub self-hosted runners. + + Attributes: + service_token_path: The path to token to access local services. + repo_check_web_service_path: The path to repo-policy-compliance service directory. + repo_check_web_service_script: The path to repo-policy-compliance web service script. + repo_check_systemd_service: The path to repo-policy-compliance unit file. + juju_storage_path: The path to juju storage. + ram_pool_path: The path to memdisk storage. + kernel_module_path: The path to kernel modules. + """ _stored = StoredState() @@ -159,28 +188,31 @@ class GithubRunnerCharm(CharmBase): ram_pool_path = Path("/storage/ram") kernel_module_path = Path("/etc/modules") - def __init__(self, *args, **kargs) -> None: + def __init__(self, *args: Any, **kwargs: Any) -> None: """Construct the charm. Args: args: List of arguments to be passed to the `CharmBase` class. - kargs: List of keyword arguments to be passed to the `CharmBase` + kwargs: List of keyword arguments to be passed to the `CharmBase` class. + + Raises: + RuntimeError: If invalid test configuration was detected. """ - super().__init__(*args, **kargs) + super().__init__(*args, **kwargs) self._grafana_agent = COSAgentProvider(self) self.service_token: str | None = None self._event_timer = EventTimer(self.unit.name) if LXD_PROFILE_YAML.exists(): - if self.config.get("test-mode") != "insecure": + if self.config.get(TEST_MODE_CONFIG_NAME) != "insecure": raise RuntimeError("lxd-profile.yaml detected outside test mode") logger.critical("test mode is enabled") self._stored.set_default( - path=self.config["path"], # for detecting changes - token=self.config["token"], # for detecting changes + path=self.config[PATH_CONFIG_NAME], # for detecting changes + token=self.config[TOKEN_CONFIG_NAME], # for detecting changes labels=self.config[LABELS_CONFIG_NAME], # for detecting changes runner_bin_url=None, ) @@ -206,7 +238,14 @@ def __init__(self, *args, **kargs) -> None: self.framework.observe(self.on.update_status, self._on_update_status) def _setup_state(self) -> CharmState: - """Set up the charm state.""" + """Set up the charm state. + + Raises: + ConfigurationError: If an invalid charm state has set. + + Returns: + The charm state. + """ try: return CharmState.from_charm(self) except CharmConfigInvalidError as exc: @@ -252,7 +291,10 @@ def _ensure_runner_storage(self, size: int, runner_storage: RunnerStorage) -> Pa runner_storage: Type of storage to use for virtual machine hosting the runners. Raises: - RunnerError: Unable to setup storage for runner. + ConfigurationError: If there was an error with runner stoarge configuration. + + Returns: + Runner storage path. """ match runner_storage: case RunnerStorage.MEMORY: @@ -281,7 +323,10 @@ def _ensure_service_health(self) -> None: """Ensure services managed by the charm is healthy. Services managed include: - * repo-policy-compliance + * repo-policy-compliance + + Raises: + SubprocessError: if there was an error starting repo-policy-compliance service. """ logger.info("Checking health of repo-policy-compliance service") try: @@ -344,6 +389,9 @@ def _get_runner_manager( def _block_on_openstack_config(self, state: CharmState) -> bool: """Set unit to blocked status on openstack configuration set. + Args: + state: The charm state instance. + Returns: Whether openstack configuration is enabled. """ @@ -357,17 +405,13 @@ def _block_on_openstack_config(self, state: CharmState) -> bool: return False @catch_charm_errors - def _on_install(self, _event: InstallEvent) -> None: - """Handle the installation of charm. - - Args: - event: Event of installing the charm. - """ + def _on_install(self, _: InstallEvent) -> None: + """Handle the installation of charm.""" state = self._setup_state() if state.charm_config.openstack_clouds_yaml: # Only build it in test mode since it may interfere with users systems. - if self.config.get("test-mode") == "insecure": + if self.config.get(TEST_MODE_CONFIG_NAME) == "insecure": self.unit.status = MaintenanceStatus("Building Openstack image") github = GithubClient(token=state.charm_config.token) image = openstack_manager.build_image( @@ -436,12 +480,8 @@ def _on_install(self, _event: InstallEvent) -> None: self.unit.status = ActiveStatus() @catch_charm_errors - def _on_start(self, _event: StartEvent) -> None: - """Handle the start of the charm. - - Args: - event: Event of starting the charm. - """ + def _on_start(self, _: StartEvent) -> None: + """Handle the start of the charm.""" state = self._setup_state() if self._block_on_openstack_config(state): @@ -490,8 +530,8 @@ def _set_reconcile_timer(self) -> None: """Set the timer for regular reconciliation checks.""" self._event_timer.ensure_event_timer( event_name="reconcile-runners", - interval=int(self.config["reconcile-interval"]), - timeout=int(self.config["reconcile-interval"]) - 1, + interval=int(self.config[RECONCILE_INTERVAL_CONFIG_NAME]), + timeout=int(self.config[RECONCILE_INTERVAL_CONFIG_NAME]) - 1, ) def _ensure_reconcile_timer_is_active(self) -> None: @@ -506,12 +546,8 @@ def _ensure_reconcile_timer_is_active(self) -> None: self._set_reconcile_timer() @catch_charm_errors - def _on_upgrade_charm(self, _event: UpgradeCharmEvent) -> None: - """Handle the update of charm. - - Args: - event: Event of charm upgrade. - """ + def _on_upgrade_charm(self, _: UpgradeCharmEvent) -> None: + """Handle the update of charm.""" state = self._setup_state() logger.info("Reinstalling dependencies...") @@ -545,12 +581,8 @@ def _on_upgrade_charm(self, _event: UpgradeCharmEvent) -> None: ) @catch_charm_errors - def _on_config_changed(self, _event: ConfigChangedEvent) -> None: - """Handle the configuration change. - - Args: - event: Event of configuration change. - """ + def _on_config_changed(self, _: ConfigChangedEvent) -> None: + """Handle the configuration change.""" state = self._setup_state() self._set_reconcile_timer() @@ -560,14 +592,14 @@ def _on_config_changed(self, _event: ConfigChangedEvent) -> None: prev_config_for_flush: dict[str, str] = {} should_flush_runners = False if state.charm_config.token != self._stored.token: - prev_config_for_flush["token"] = str(self._stored.token) + prev_config_for_flush[TOKEN_CONFIG_NAME] = str(self._stored.token) self._start_services(state.charm_config.token, state.proxy_config) self._stored.token = None - if self.config["path"] != self._stored.path: - prev_config_for_flush["path"] = parse_github_path( - self._stored.path, self.config["group"] + if self.config[PATH_CONFIG_NAME] != self._stored.path: + prev_config_for_flush[PATH_CONFIG_NAME] = parse_github_path( + self._stored.path, self.config[GROUP_CONFIG_NAME] ) - self._stored.path = self.config["path"] + self._stored.path = self.config[PATH_CONFIG_NAME] if self.config[LABELS_CONFIG_NAME] != self._stored.labels: should_flush_runners = True self._stored.labels = self.config[LABELS_CONFIG_NAME] @@ -652,12 +684,8 @@ def _check_and_update_dependencies( return service_updated or runner_bin_updated @catch_charm_errors - def _on_reconcile_runners(self, _event: ReconcileRunnersEvent) -> None: - """Handle the reconciliation of runners. - - Args: - event: Event of reconciling the runner state. - """ + def _on_reconcile_runners(self, _: ReconcileRunnersEvent) -> None: + """Handle the reconciliation of runners.""" state = self._setup_state() if self._block_on_openstack_config(state): @@ -683,7 +711,11 @@ def _on_reconcile_runners(self, _event: ReconcileRunnersEvent) -> None: @catch_action_errors def _on_check_runners_action(self, event: ActionEvent) -> None: - """Handle the action of checking of runner state.""" + """Handle the action of checking of runner state. + + Args: + event: The event fired on check_runners action. + """ state = self._setup_state() runner_manager = self._get_runner_manager(state) if runner_manager.runner_bin_path is None: @@ -771,20 +803,12 @@ def _on_update_dependencies_action(self, event: ActionEvent) -> None: @catch_charm_errors def _on_update_status(self, _: UpdateStatusEvent) -> None: - """Handle the update of charm status. - - Args: - event: Event of updating the charm status. - """ + """Handle the update of charm status.""" self._ensure_reconcile_timer_is_active() @catch_charm_errors def _on_stop(self, _: StopEvent) -> None: - """Handle the stopping of the charm. - - Args: - event: Event of stopping the charm. - """ + """Handle the stopping of the charm.""" self._event_timer.disable_event_timer("reconcile-runners") state = self._setup_state() @@ -802,14 +826,17 @@ def _reconcile_runners( Args: runner_manager: For querying and managing the runner state. num: Target number of virtual machines. - resource: Target resource for each virtual machine. + resources: Target resource for each virtual machine. + + Raises: + MissingRunnerBinaryError: If the runner binary is not found. Returns: Changes in runner number due to reconciling runners. """ if not RunnerManager.runner_bin_path.is_file(): logger.warning("Unable to reconcile due to missing runner binary") - raise MissingRunnerBinaryError() + raise MissingRunnerBinaryError("Runner binary not found.") self.unit.status = MaintenanceStatus("Reconciling runners") delta_virtual_machines = runner_manager.reconcile( @@ -1009,6 +1036,11 @@ def _refresh_firewall(self, state: CharmState) -> None: ) def _apt_install(self, packages: Sequence[str]) -> None: + """Execute apt install command. + + Args: + packages: The names of apt packages to install. + """ execute_command(["/usr/bin/apt-get", "update"]) _, exit_code = execute_command( diff --git a/src/charm_state.py b/src/charm_state.py index 65aa3f2b7..f4091cccf 100644 --- a/src/charm_state.py +++ b/src/charm_state.py @@ -15,7 +15,7 @@ import yaml from ops import CharmBase -from pydantic import AnyHttpUrl, BaseModel, Field, ValidationError, root_validator +from pydantic import AnyHttpUrl, BaseModel, Field, ValidationError, validator from pydantic.networks import IPvAnyAddress import openstack_cloud @@ -30,7 +30,21 @@ CHARM_STATE_PATH = Path("charm_state.json") +DENYLIST_CONFIG_NAME = "denylist" +DOCKERHUB_MIRROR_CONFIG_NAME = "dockerhub-mirror" +GROUP_CONFIG_NAME = "group" OPENSTACK_CLOUDS_YAML_CONFIG_NAME = "experimental-openstack-clouds-yaml" +PATH_CONFIG_NAME = "path" +RECONCILE_INTERVAL_CONFIG_NAME = "reconcile-interval" +RUNNER_STORAGE_CONFIG_NAME = "runner-storage" +TEST_MODE_CONFIG_NAME = "test-mode" +# bandit thinks this is a hardcoded password. +TOKEN_CONFIG_NAME = "token" # nosec +USE_APROXY_CONFIG_NAME = "experimental-use-aproxy" +VIRTUAL_MACHINES_CONFIG_NAME = "virtual-machines" +VM_CPU_CONFIG_NAME = "vm-cpu" +VM_MEMORY_CONFIG_NAME = "vm-memory" +VM_DISK_CONFIG_NAME = "vm-disk" LABELS_CONFIG_NAME = "labels" @@ -90,6 +104,10 @@ def parse_github_path(path_str: str, runner_group: str) -> GithubPath: path_str: GitHub path in string format. runner_group: Runner group name for GitHub organization. If the path is a repository this argument is ignored. + + Raises: + CharmConfigInvalidError: if an invalid path string was given. + Returns: GithubPath object representing the GitHub repository, or the GitHub organization with runner group information. @@ -103,6 +121,45 @@ def parse_github_path(path_str: str, runner_group: str) -> GithubPath: return GithubOrg(org=path_str, group=runner_group) +@dataclasses.dataclass +class GithubConfig: + """Charm configuration related to GitHub. + + Attributes: + token: The Github API access token (PAT). + path: The Github org/repo path. + """ + + token: str + path: GithubPath + + @classmethod + def from_charm(cls, charm: CharmBase) -> "GithubConfig": + """Get github related charm configuration values from charm. + + Args: + charm: The charm instance. + + Raises: + CharmConfigInvalidError: If an invalid configuration value was set. + + Returns: + The parsed GitHub configuration values. + """ + runner_group = charm.config.get(GROUP_CONFIG_NAME, "default") + + path_str = charm.config.get(PATH_CONFIG_NAME, "") + if not path_str: + raise CharmConfigInvalidError(f"Missing {PATH_CONFIG_NAME} configuration") + path = parse_github_path(path_str, runner_group) + + token = charm.config.get(TOKEN_CONFIG_NAME) + if not token: + raise CharmConfigInvalidError(f"Missing {TOKEN_CONFIG_NAME} configuration") + + return cls(token=token, path=path) + + class VirtualMachineResources(NamedTuple): """Virtual machine resource configuration. @@ -118,7 +175,12 @@ class VirtualMachineResources(NamedTuple): class Arch(str, Enum): - """Supported system architectures.""" + """Supported system architectures. + + Attributes: + ARM64: Represents an ARM64 system architecture. + X64: Represents an X64/AMD64 system architecture. + """ ARM64 = "arm64" X64 = "x64" @@ -129,7 +191,12 @@ class Arch(str, Enum): class RunnerStorage(str, Enum): - """Supported storage as runner disk.""" + """Supported storage as runner disk. + + Attributes: + JUJU_STORAGE: Represents runner storage from Juju storage. + MEMORY: Represents tempfs storage (ramdisk). + """ JUJU_STORAGE = "juju-storage" MEMORY = "memory" @@ -173,7 +240,7 @@ def _parse_labels(labels: str) -> tuple[str, ...]: """Return valid labels. Args: - label: Comma separated labels string. + labels: Comma separated labels string. Raises: ValueError: if any invalid label was found. @@ -209,7 +276,7 @@ class CharmConfig(BaseModel): openstack_clouds_yaml: The openstack clouds.yaml configuration. path: GitHub repository path in the format '/', or the GitHub organization name. - reconcile_interval: Time between each reconciliation of runners. + reconcile_interval: Time between each reconciliation of runners in minutes. token: GitHub personal access token for GitHub API. """ @@ -222,8 +289,16 @@ class CharmConfig(BaseModel): token: str @classmethod - def _parse_denylist(cls, charm: CharmBase) -> list[str]: - denylist_str = charm.config.get("denylist", "") + def _parse_denylist(cls, charm: CharmBase) -> list[FirewallEntry]: + """Read charm denylist configuration and parse it into firewall deny entries. + + Args: + charm: The charm instance. + + Returns: + The firewall deny entries. + """ + denylist_str = charm.config.get(DENYLIST_CONFIG_NAME, "") entry_list = [entry.strip() for entry in denylist_str.split(",")] denylist = [FirewallEntry.decode(entry) for entry in entry_list if entry] @@ -233,24 +308,69 @@ def _parse_denylist(cls, charm: CharmBase) -> list[str]: def _parse_dockerhub_mirror(cls, charm: CharmBase) -> str | None: """Parse and validate dockerhub mirror URL. - args: + Args: charm: The charm instance. + Raises: + CharmConfigInvalidError: if insecure scheme is passed for dockerhub mirror. + Returns: The URL of dockerhub mirror. """ - dockerhub_mirror = charm.config.get("dockerhub-mirror") or None + dockerhub_mirror = charm.config.get(DOCKERHUB_MIRROR_CONFIG_NAME) or None + + if not dockerhub_mirror: + return None dockerhub_mirror_url = urlsplit(dockerhub_mirror) if dockerhub_mirror is not None and dockerhub_mirror_url.scheme != "https": raise CharmConfigInvalidError( ( - "Only secured registry supported for dockerhub-mirror configuration, the " - "scheme should be https" + f"Only secured registry supported for {DOCKERHUB_MIRROR_CONFIG_NAME} " + "configuration, the scheme should be https" ) ) + return dockerhub_mirror + @classmethod + def _parse_openstack_clouds_config(cls, charm: CharmBase) -> dict | None: + """Parse and validate openstack clouds yaml config value. + + Args: + charm: The charm instance. + + Raises: + CharmConfigInvalidError: if an invalid Openstack config value was set. + + Returns: + The openstack clouds yaml. + """ + openstack_clouds_yaml_str = charm.config.get(OPENSTACK_CLOUDS_YAML_CONFIG_NAME) + if not openstack_clouds_yaml_str: + return None + + try: + openstack_clouds_yaml = yaml.safe_load(openstack_clouds_yaml_str) + except yaml.YAMLError as exc: + logger.error(f"Invalid {OPENSTACK_CLOUDS_YAML_CONFIG_NAME} config: %s.", exc) + raise CharmConfigInvalidError( + f"Invalid {OPENSTACK_CLOUDS_YAML_CONFIG_NAME} config. Invalid yaml." + ) from exc + if (config_type := type(openstack_clouds_yaml)) is not dict: + raise CharmConfigInvalidError( + f"Invalid openstack config format, expected dict, got {config_type}" + ) + try: + openstack_cloud.initialize(openstack_clouds_yaml) + except OpenStackInvalidConfigError as exc: + logger.error("Invalid openstack config, %s.", exc) + raise CharmConfigInvalidError( + "Invalid openstack config. Not able to initialize openstack integration." + ) from exc + + return cast(dict, openstack_clouds_yaml) + @classmethod def from_charm(cls, charm: CharmBase) -> "CharmConfig": """Initialize the config from charm. @@ -258,49 +378,27 @@ def from_charm(cls, charm: CharmBase) -> "CharmConfig": Args: charm: The charm instance. + Raises: + CharmConfigInvalidError: If any invalid configuration has been set on the charm. + Returns: Current config of the charm. """ - path_str = charm.config.get("path", "") - if not path_str: - raise CharmConfigInvalidError("Missing path configuration") - runner_group = charm.config.get("group", "default") - path = parse_github_path(path_str, runner_group) - - token = charm.config.get("token") - if not token: - raise CharmConfigInvalidError("Missing token configuration") + try: + github_config = GithubConfig.from_charm(charm) + except CharmConfigInvalidError as exc: + raise CharmConfigInvalidError(f"Invalid Github config, {str(exc)}") from exc try: - reconcile_interval = int(charm.config["reconcile-interval"]) + reconcile_interval = int(charm.config[RECONCILE_INTERVAL_CONFIG_NAME]) except ValueError as err: - raise CharmConfigInvalidError("The reconcile-interval config must be int") from err + raise CharmConfigInvalidError( + f"The {RECONCILE_INTERVAL_CONFIG_NAME} config must be int" + ) from err denylist = cls._parse_denylist(charm) dockerhub_mirror = cls._parse_dockerhub_mirror(charm) - - openstack_clouds_yaml_str = charm.config.get(OPENSTACK_CLOUDS_YAML_CONFIG_NAME) - if openstack_clouds_yaml_str: - try: - openstack_clouds_yaml = yaml.safe_load(openstack_clouds_yaml_str) - except yaml.YAMLError as exc: - logger.error("Invalid experimental-openstack-clouds-yaml config: %s.", exc) - raise CharmConfigInvalidError( - "Invalid experimental-openstack-clouds-yaml config. Invalid yaml." - ) from exc - if (config_type := type(openstack_clouds_yaml)) is not dict: - raise CharmConfigInvalidError( - f"Invalid openstack config format, expected dict, got {config_type}" - ) - try: - openstack_cloud.initialize(openstack_clouds_yaml) - except OpenStackInvalidConfigError as exc: - logger.error("Invalid openstack config, %s.", exc) - raise CharmConfigInvalidError( - "Invalid openstack config. Not able to initialize openstack integration." - ) from exc - else: - openstack_clouds_yaml = None + openstack_clouds_yaml = cls._parse_openstack_clouds_config(charm) try: labels = _parse_labels(charm.config.get(LABELS_CONFIG_NAME, "")) @@ -312,33 +410,37 @@ def from_charm(cls, charm: CharmBase) -> "CharmConfig": dockerhub_mirror=dockerhub_mirror, labels=labels, openstack_clouds_yaml=openstack_clouds_yaml, - path=path, + path=github_config.path, reconcile_interval=reconcile_interval, - token=token, + token=github_config.token, ) - @root_validator + @validator("reconcile_interval") @classmethod - def check_fields(cls, values: dict) -> dict: + def check_reconcile_interval(cls, reconcile_interval: int) -> int: """Validate the general charm configuration. Args: - values: Values in the pydantic model. + reconcile_interval: The value of reconcile_interval passed to class instantiation. + + Raises: + ValueError: if an invalid reconcile_interval value of less than 2 has been passed. Returns: - Modified values in the pydantic model. + The validated reconcile_interval value. """ - reconcile_interval = cast(int, values.get("reconcile_interval")) - # The EventTimer class sets a timeout of `reconcile_interval` - 1. # Therefore the `reconcile_interval` must be at least 2. if reconcile_interval < 2: - logger.exception("The virtual-machines configuration must be int") + logger.exception( + "The %s configuration must be greater than 1", RECONCILE_INTERVAL_CONFIG_NAME + ) raise ValueError( - "The reconcile_interval configuration needs to be greater or equal to 2" + f"The {RECONCILE_INTERVAL_CONFIG_NAME} configuration needs to be \ + greater or equal to 2" ) - return values + return reconcile_interval class RunnerCharmConfig(BaseModel): @@ -354,6 +456,36 @@ class RunnerCharmConfig(BaseModel): virtual_machine_resources: VirtualMachineResources runner_storage: RunnerStorage + @classmethod + def _check_storage_change(cls, runner_storage: str) -> None: + """Check whether the storage configuration has changed. + + Args: + runner_storage: The current runner_storage config value. + + Raises: + CharmConfigInvalidError: If the runner-storage config value has changed after initial + deployment. + """ + prev_state = None + if CHARM_STATE_PATH.exists(): + json_data = CHARM_STATE_PATH.read_text(encoding="utf-8") + prev_state = json.loads(json_data) + logger.info("Previous charm state: %s", prev_state) + + if ( + prev_state is not None + and prev_state["runner_config"]["runner_storage"] != runner_storage + ): + logger.warning( + "Storage option changed from %s to %s, blocking the charm", + prev_state["runner_config"]["runner_storage"], + runner_storage, + ) + raise CharmConfigInvalidError( + "runner-storage config cannot be changed after deployment, redeploy if needed" + ) + @classmethod def from_charm(cls, charm: CharmBase) -> "RunnerCharmConfig": """Initialize the config from charm. @@ -361,28 +493,36 @@ def from_charm(cls, charm: CharmBase) -> "RunnerCharmConfig": Args: charm: The charm instance. + Raises: + CharmConfigInvalidError: if an invalid runner charm config has been set on the charm. + Returns: Current config of the charm. """ try: - runner_storage = RunnerStorage(charm.config["runner-storage"]) + runner_storage = RunnerStorage(charm.config[RUNNER_STORAGE_CONFIG_NAME]) + cls._check_storage_change(runner_storage=runner_storage) except ValueError as err: - raise CharmConfigInvalidError("Invalid runner-storage configuration") from err + raise CharmConfigInvalidError( + f"Invalid {RUNNER_STORAGE_CONFIG_NAME} configuration" + ) from err + except CharmConfigInvalidError as exc: + raise CharmConfigInvalidError(f"Invalid runner storage config, {str(exc)}") from exc try: - virtual_machines = int(charm.config["virtual-machines"]) + virtual_machines = int(charm.config[VIRTUAL_MACHINES_CONFIG_NAME]) except ValueError as err: raise CharmConfigInvalidError( - "The virtual-machines configuration must be int" + f"The {VIRTUAL_MACHINES_CONFIG_NAME} configuration must be int" ) from err try: - cpu = int(charm.config["vm-cpu"]) + cpu = int(charm.config[VM_CPU_CONFIG_NAME]) except ValueError as err: - raise CharmConfigInvalidError("Invalid vm-cpu configuration") from err + raise CharmConfigInvalidError(f"Invalid {VM_CPU_CONFIG_NAME} configuration") from err virtual_machine_resources = VirtualMachineResources( - cpu, charm.config["vm-memory"], charm.config["vm-disk"] + cpu, charm.config[VM_MEMORY_CONFIG_NAME], charm.config[VM_DISK_CONFIG_NAME] ) return cls( @@ -391,43 +531,66 @@ def from_charm(cls, charm: CharmBase) -> "RunnerCharmConfig": runner_storage=runner_storage, ) - @root_validator + @validator("virtual_machines") @classmethod - def check_fields(cls, values: dict) -> dict: - """Validate the runner configuration. + def check_virtual_machines(cls, virtual_machines: int) -> int: + """Validate the virtual machines configuration value. Args: - values: Values in the pydantic model. + virtual_machines: The virtual machines value to validate. + + Raises: + ValueError: if a negative integer was passed. Returns: - Modified values in the pydantic model. + Validated virtual_machines value. """ - virtual_machines = cast(int, values.get("virtual_machines")) - resources = cast(VirtualMachineResources, values.get("virtual_machine_resources")) - if virtual_machines < 0: raise ValueError( - "The virtual-machines configuration needs to be greater or equal to 0" + f"The {VIRTUAL_MACHINES_CONFIG_NAME} configuration needs to be greater or equal " + "to 0" ) - if resources.cpu < 1: - raise ValueError("The vm-cpu configuration needs to be greater than 0") - if not _valid_storage_size_str(resources.memory): + return virtual_machines + + @validator("virtual_machine_resources") + @classmethod + def check_virtual_machine_resources( + cls, vm_resources: VirtualMachineResources + ) -> VirtualMachineResources: + """Validate the virtual_machine_resources field values. + + Args: + vm_resources: the virtual_machine_resources value to validate. + + Raises: + ValueError: if an invalid number of cpu was given or invalid memory/disk size was + given. + + Returns: + The validated virtual_machine_resources value. + """ + if vm_resources.cpu < 1: + raise ValueError(f"The {VM_CPU_CONFIG_NAME} configuration needs to be greater than 0") + if not _valid_storage_size_str(vm_resources.memory): raise ValueError( - "Invalid format for vm-memory configuration, must be int with unit (e.g. MiB, GiB)" + f"Invalid format for {VM_MEMORY_CONFIG_NAME} configuration, must be int with unit " + "(e.g. MiB, GiB)" ) - if not _valid_storage_size_str(resources.disk): + if not _valid_storage_size_str(vm_resources.disk): raise ValueError( - "Invalid format for vm-disk configuration, must be int with unit (e.g., MiB, GiB)" + f"Invalid format for {VM_DISK_CONFIG_NAME} configuration, must be int with unit " + "(e.g., MiB, GiB)" ) - return values + return vm_resources class ProxyConfig(BaseModel): """Proxy configuration. Attributes: + aproxy_address: The address of aproxy snap instance if use_aproxy is enabled. http: HTTP proxy address. https: HTTPS proxy address. no_proxy: Comma-separated list of hosts that should not be proxied. @@ -449,7 +612,7 @@ def from_charm(cls, charm: CharmBase) -> "ProxyConfig": Returns: Current proxy config of the charm. """ - use_aproxy = bool(charm.config.get("experimental-use-aproxy")) + use_aproxy = bool(charm.config.get(USE_APROXY_CONFIG_NAME)) http_proxy = get_env_var("JUJU_CHARM_HTTP_PROXY") or None https_proxy = get_env_var("JUJU_CHARM_HTTPS_PROXY") or None no_proxy = get_env_var("JUJU_CHARM_NO_PROXY") or None @@ -477,24 +640,32 @@ def aproxy_address(self) -> Optional[str]: aproxy_address = None return aproxy_address - @root_validator + @validator("use_aproxy") @classmethod - def check_fields(cls, values: dict) -> dict: + def check_use_aproxy(cls, use_aproxy: bool, values: dict) -> bool: """Validate the proxy configuration. Args: + use_aproxy: Value of use_aproxy variable. values: Values in the pydantic model. + Raises: + ValueError: if use_aproxy was set but no http/https was passed. + Returns: - Modified values in the pydantic model. + Validated use_aproxy value. """ - if values.get("use_aproxy") and not (values.get("http") or values.get("https")): + if use_aproxy and not (values.get("http") or values.get("https")): raise ValueError("aproxy requires http or https to be set") - return values + return use_aproxy + + def __bool__(self) -> bool: + """Return whether the proxy config is set. - def __bool__(self): - """Return whether we have a proxy config.""" + Returns: + Whether the proxy config is set. + """ return bool(self.http or self.https) class Config: # pylint: disable=too-few-public-methods @@ -563,6 +734,9 @@ def from_charm(cls, charm: CharmBase) -> list["SSHDebugConnection"]: Args: charm: The charm instance. + + Returns: + List of connection information for ssh debug access. """ ssh_debug_connections: list[SSHDebugConnection] = [] relations = charm.model.relations[DEBUG_SSH_INTEGRATION_NAME] @@ -600,6 +774,7 @@ class CharmState: charm_config: Configuration of the juju charm. is_metrics_logging_available: Whether the charm is able to issue metrics. proxy_config: Proxy-related configuration. + runner_config: The charm configuration related to runner VM configuration. ssh_debug_connections: SSH debug connections configuration information. """ @@ -617,46 +792,23 @@ def from_charm(cls, charm: CharmBase) -> "CharmState": Args: charm: The charm instance. + Raises: + CharmConfigInvalidError: If an invalid configuration was set. + Returns: Current state of the charm. """ - prev_state = None - if CHARM_STATE_PATH.exists(): - json_data = CHARM_STATE_PATH.read_text(encoding="utf-8") - prev_state = json.loads(json_data) - logger.info("Previous charm state: %s", prev_state) - try: proxy_config = ProxyConfig.from_charm(charm) except (ValidationError, ValueError) as exc: - logger.error("Invalid proxy config: %s", exc) raise CharmConfigInvalidError(f"Invalid proxy configuration: {str(exc)}") from exc try: charm_config = CharmConfig.from_charm(charm) - except (ValidationError, ValueError) as exc: - logger.error("Invalid charm config: %s", exc) - raise CharmConfigInvalidError(f"Invalid configuration: {str(exc)}") from exc - - try: runner_config = RunnerCharmConfig.from_charm(charm) except (ValidationError, ValueError) as exc: - logger.error("Invalid charm config: %s", exc) raise CharmConfigInvalidError(f"Invalid configuration: {str(exc)}") from exc - if ( - prev_state is not None - and prev_state["runner_config"]["runner_storage"] != runner_config.runner_storage - ): - logger.warning( - "Storage option changed from %s to %s, blocking the charm", - prev_state["runner_config"]["runner_storage"], - runner_config.runner_storage, - ) - raise CharmConfigInvalidError( - "runner-storage config cannot be changed after deployment, redeploy if needed" - ) - try: arch = _get_supported_arch() except UnsupportedArchitectureError as exc: diff --git a/src/event_timer.py b/src/event_timer.py index 5808b1e7c..da03150ff 100644 --- a/src/event_timer.py +++ b/src/event_timer.py @@ -70,7 +70,9 @@ def __init__(self, unit_name: str): loader=jinja2.FileSystemLoader("templates"), autoescape=True ) - def _render_event_template(self, template_type: str, event_name: str, context: EventConfig): + def _render_event_template( + self, template_type: str, event_name: str, context: EventConfig + ) -> None: """Write event configuration files to systemd path. Args: @@ -105,7 +107,9 @@ def is_active(self, event_name: str) -> bool: return ret_code == 0 - def ensure_event_timer(self, event_name: str, interval: int, timeout: Optional[int] = None): + def ensure_event_timer( + self, event_name: str, interval: int, timeout: Optional[int] = None + ) -> None: """Ensure that a systemd service and timer are registered to dispatch the given event. The interval is how frequently, in minutes, the event should be dispatched. @@ -144,7 +148,7 @@ def ensure_event_timer(self, event_name: str, interval: int, timeout: Optional[i except (subprocess.CalledProcessError, subprocess.TimeoutExpired) as ex: raise TimerEnableError(f"Unable to enable systemd timer {systemd_timer}") from ex - def disable_event_timer(self, event_name: str): + def disable_event_timer(self, event_name: str) -> None: """Disable the systemd timer for the given event. Args: diff --git a/src/firewall.py b/src/firewall.py index e1a1a1425..b4bf07c00 100644 --- a/src/firewall.py +++ b/src/firewall.py @@ -101,11 +101,12 @@ def refresh_firewall( self, denylist: typing.Iterable[FirewallEntry], allowlist: typing.Iterable[FirewallEntry] | None = None, - ): + ) -> None: """Refresh the firewall configuration. Args: - denylist: The list of FirewallEntry objects to allow. + denylist: The list of FirewallEntry rules to allow. + allowlist: The list of FirewallEntry rules to allow. """ current_acls = [ acl["name"] diff --git a/src/github_client.py b/src/github_client.py index d3a2896a8..f8057dcac 100644 --- a/src/github_client.py +++ b/src/github_client.py @@ -9,14 +9,15 @@ import functools import logging from datetime import datetime +from typing import Callable, ParamSpec, TypeVar from urllib.error import HTTPError from ghapi.all import GhApi, pages from ghapi.page import paged from typing_extensions import assert_never -import errors from charm_state import Arch, GithubOrg, GithubPath, GithubRepo +from errors import GithubApiError, JobNotFoundError, RunnerBinaryError, TokenError from github_type import ( JobStats, RegistrationToken, @@ -28,12 +29,37 @@ logger = logging.getLogger(__name__) +# Parameters of the function decorated with retry +ParamT = ParamSpec("ParamT") +# Return type of the function decorated with retry +ReturnT = TypeVar("ReturnT") -def catch_http_errors(func): - """Catch HTTP errors and raise custom exceptions.""" + +def catch_http_errors(func: Callable[ParamT, ReturnT]) -> Callable[ParamT, ReturnT]: + """Catch HTTP errors and raise custom exceptions. + + Args: + func: The target function to catch common errors for. + + Returns: + The decorated function. + """ @functools.wraps(func) - def wrapper(*args, **kwargs): + def wrapper(*args: ParamT.args, **kwargs: ParamT.kwargs) -> ReturnT: + """Catch common errors when using the GitHub API. + + Args: + args: Placeholder for positional arguments. + kwargs: Placeholder for keyword arguments. + + Raises: + TokenError: If there was an error with the provided token. + GithubApiError: If there was an unexpected error using the GitHub API. + + Returns: + The decorated function. + """ try: return func(*args, **kwargs) except HTTPError as exc: @@ -42,8 +68,8 @@ def wrapper(*args, **kwargs): msg = "Invalid token." else: msg = "Provided token has not enough permissions or has reached rate-limit." - raise errors.TokenError(msg) from exc - raise errors.GithubApiError from exc + raise TokenError(msg) from exc + raise GithubApiError from exc return wrapper @@ -56,7 +82,6 @@ def __init__(self, token: str): Args: token: GitHub personal token for API requests. - request_session: Requests session for HTTP requests. """ self._token = token self._client = GhApi(token=self._token) @@ -97,7 +122,7 @@ def get_runner_application( if bin["os"] == os and bin["architecture"] == arch ) except StopIteration as err: - raise errors.RunnerBinaryError( + raise RunnerBinaryError( f"Unable query GitHub runner binary information for {os} {arch}" ) from err @@ -153,6 +178,9 @@ def get_runner_github_info(self, path: GithubPath) -> list[SelfHostedRunner]: def get_runner_remove_token(self, path: GithubPath) -> str: """Get token from GitHub used for removing runners. + Args: + path: The Github org/repo path. + Returns: The removing token. """ @@ -220,6 +248,10 @@ def get_job_info(self, path: GithubRepo, workflow_run_id: str, runner_name: str) workflow_run_id: Id of the workflow run. runner_name: Name of the runner. + Raises: + TokenError: if there was an error with the Github token crdential provided. + JobNotFoundError: If no jobs were found. + Returns: Job information. """ @@ -254,10 +286,10 @@ def get_job_info(self, path: GithubRepo, workflow_run_id: str, runner_name: str) except HTTPError as exc: if exc.code in (401, 403): - raise errors.TokenError from exc - raise errors.JobNotFoundError( + raise TokenError from exc + raise JobNotFoundError( f"Could not find job for runner {runner_name}. " f"Could not list jobs for workflow run {workflow_run_id}" ) from exc - raise errors.JobNotFoundError(f"Could not find job for runner {runner_name}.") + raise JobNotFoundError(f"Could not find job for runner {runner_name}.") diff --git a/src/github_metrics.py b/src/github_metrics.py index 868b8d199..282a88948 100644 --- a/src/github_metrics.py +++ b/src/github_metrics.py @@ -3,8 +3,8 @@ """Functions to calculate metrics from data retrieved from GitHub.""" -import errors from charm_state import GithubRepo +from errors import GithubMetricsError, JobNotFoundError from github_client import GithubClient from metrics_type import GithubJobMetrics from runner_metrics import PreJobMetrics @@ -22,6 +22,9 @@ def job( pre_job_metrics: The pre-job metrics. runner_name: The name of the runner. + Raises: + GithubMetricsError: If the job for given workflow run is not found. + Returns: The job metrics. """ @@ -33,8 +36,8 @@ def job( workflow_run_id=pre_job_metrics.workflow_run_id, runner_name=runner_name, ) - except errors.JobNotFoundError as exc: - raise errors.GithubMetricsError from exc + except JobNotFoundError as exc: + raise GithubMetricsError from exc queue_duration = (job_info.started_at - job_info.created_at).total_seconds() return GithubJobMetrics(queue_duration=queue_duration, conclusion=job_info.conclusion) diff --git a/src/github_type.py b/src/github_type.py index 8f232b2ce..efaf365be 100644 --- a/src/github_type.py +++ b/src/github_type.py @@ -15,7 +15,12 @@ class GitHubRunnerStatus(Enum): - """Status of runner on GitHub.""" + """Status of runner on GitHub. + + Attributes: + ONLINE: Represents an online runner status. + OFFLINE: Represents an offline runner status. + """ ONLINE = "online" OFFLINE = "offline" @@ -33,7 +38,7 @@ class RunnerApplication(TypedDict, total=False): filename: Filename of the runner application. temp_download_token: A short lived bearer token used to download the runner, if needed. - sha256_check_sum: SHA256 Checksum of the runner application. + sha256_checksum: SHA256 Checksum of the runner application. """ os: Literal["linux", "win", "osx"] @@ -66,19 +71,20 @@ class SelfHostedRunner(TypedDict): """Information on a single self-hosted runner. Attributes: - id: Unique identifier of the runner. - name: Name of the runner. - os: Operation system of the runner. busy: Whether the runner is executing a job. + id: Unique identifier of the runner. labels: Labels of the runner. + os: Operation system of the runner. + name: Name of the runner. + status: The Github runner status. """ + busy: bool id: int - name: str + labels: list[SelfHostedRunnerLabel] os: str + name: str status: GitHubRunnerStatus - busy: bool - labels: list[SelfHostedRunnerLabel] class SelfHostedRunnerList(TypedDict): @@ -118,15 +124,28 @@ class RemoveToken(TypedDict): class JobConclusion(str, Enum): - """Conclusion of a job on GitHub.""" + """Conclusion of a job on GitHub. - SUCCESS = "success" + See :https://docs.github.com/en/rest/actions/workflow-runs?apiVersion=2022-11-28\ +#list-workflow-runs-for-a-repository + + Attributes: + ACTION_REQUIRED: Represents additional action required on the job. + CANCELLED: Represents a cancelled job status. + FAILURE: Represents a failed job status. + NEUTRAL: Represents a job status that can optionally succeed or fail. + SKIPPED: Represents a skipped job status. + SUCCESS: Represents a successful job status. + TIMED_OUT: Represents a job that has timed out. + """ + + ACTION_REQUIRED = "action_required" + CANCELLED = "cancelled" FAILURE = "failure" NEUTRAL = "neutral" - CANCELLED = "cancelled" SKIPPED = "skipped" + SUCCESS = "success" TIMED_OUT = "timed_out" - ACTION_REQUIRED = "action_required" class JobStats(BaseModel): @@ -135,6 +154,7 @@ class JobStats(BaseModel): Attributes: created_at: The time the job was created. started_at: The time the job was started. + conclusion: The end result of a job. """ created_at: datetime diff --git a/src/lxd.py b/src/lxd.py index bae53d6e4..d371c9aa6 100644 --- a/src/lxd.py +++ b/src/lxd.py @@ -12,7 +12,7 @@ import logging import tempfile from pathlib import Path -from typing import IO, Optional, Tuple, Union +from typing import IO, Any, Optional, Tuple, Union import pylxd.models @@ -50,7 +50,7 @@ def mk_dir(self, dir_name: str) -> None: """Create a directory in the LXD instance. Args: - dir: Name of the directory to create. + dir_name: Name of the directory to create. """ self.instance.execute(["/usr/bin/mkdir", "-p", dir_name]) @@ -105,9 +105,13 @@ def write_file( file.write(content) file.flush() - self.push_file(file.name, filepath, mode) + try: + self.push_file(file.name, filepath, mode) + # 2024/04/02 - We should define a new error, wrap it and re-raise it. + except LxdError: # pylint: disable=try-except-raise + raise - def pull_file(self, source: str, destination: str, is_dir=False) -> None: + def pull_file(self, source: str, destination: str, is_dir: bool = False) -> None: """Pull a file from the LXD instance to the local machine. Args: @@ -148,7 +152,11 @@ def read_file(self, filepath: str) -> str: The content of the file. """ with tempfile.NamedTemporaryFile() as file: - self.pull_file(filepath, file.name) + try: + self.pull_file(filepath, file.name) + # 2024/04/02 - We should define a new error, wrap it and re-raise it. + except LxdError: # pylint: disable=try-except-raise + raise return file.read().decode("utf-8") @@ -235,7 +243,7 @@ def delete(self, wait: bool = False) -> None: raise LxdError(f"Unable to delete the LXD instance {self.name}") from err def execute( - self, cmd: list[str], cwd: Optional[str] = None, hide_cmd: bool = False, **kwargs + self, cmd: list[str], cwd: Optional[str] = None, hide_cmd: bool = False, **kwargs: Any ) -> Tuple[int, IO, IO]: """Execute a command within the LXD instance. @@ -351,7 +359,7 @@ def create( Args: name: Name of the LXD profile to create. config: Configuration of the LXD profile. - devices Devices configuration of the LXD profile. + devices: Devices configuration of the LXD profile. Raises: LxdError: Unable to create the LXD profile. @@ -370,6 +378,9 @@ def get(self, name: str) -> LxdProfile: Raises: LxdError: Unable to get the LXD profile with the name. + + Returns: + LXDProfile with given name. """ try: return self._pylxd_client.profiles.get(name) @@ -398,12 +409,12 @@ def __init__( self.devices = self._pylxd_profile.devices self.used_by = self._pylxd_profile.used_by - def save(self): + def save(self) -> None: """Save the current configuration of profile.""" self._pylxd_profile.config = self.config self._pylxd_profile.save() - def delete(self): + def delete(self) -> None: """Delete the profile.""" self._pylxd_profile.delete() @@ -465,6 +476,9 @@ def get(self, name: str) -> LxdStoragePool: Args: name: Name of the storage pool. + Raises: + LxdError: If the storage pool with given name was not found. + Returns: The LXD storage pool. """ @@ -526,12 +540,12 @@ def __init__( self.config = self._pylxd_storage_pool.config self.managed = self._pylxd_storage_pool.managed - def save(self): + def save(self) -> None: """Save the current configuration of storage pool.""" self._pylxd_storage_pool.config = self.config self._pylxd_storage_pool.save() - def delete(self): + def delete(self) -> None: """Delete the storage pool.""" self._pylxd_storage_pool.delete() @@ -547,7 +561,7 @@ def __init__(self, pylxd_client: pylxd.Client): """ self._pylxd_client = pylxd_client - def create(self, name: str, path: Path): + def create(self, name: str, path: Path) -> None: """Import a LXD image. Args: @@ -566,7 +580,7 @@ def create(self, name: str, path: Path): class LxdClient: # pylint: disable=too-few-public-methods """LXD client.""" - def __init__(self): + def __init__(self) -> None: """Instantiate the LXD client.""" pylxd_client = pylxd.Client() self.instances = LxdInstanceManager(pylxd_client) diff --git a/src/lxd_type.py b/src/lxd_type.py index 169703292..77a3b3cb6 100644 --- a/src/lxd_type.py +++ b/src/lxd_type.py @@ -35,17 +35,33 @@ class LxdInstanceConfigSource(TypedDict): - """Configuration for source image in the LXD instance.""" + """Configuration for source image in the LXD instance. + + Attributes: + type: Type of source configuration, e.g. image, disk + server: The source server URL, e.g. https://cloud-images.ubuntu.com/releases + protocol: Protocol of the configuration, e.g. simplestreams + alias: Alias for configuration. + """ type: str - mode: str server: str protocol: str alias: str class LxdInstanceConfig(TypedDict): - """Configuration for the LXD instance.""" + """Configuration for the LXD instance. + + See https://documentation.ubuntu.com/lxd/en/latest/howto/instances_create/ + + Attributes: + name: Name of the instance. + type: Instance type, i.e. "container" or "virtual-machine". + source: Instance creation source configuration. + ephemeral: Whether the container should be deleted after a single run. + profiles: List of LXD profiles applied to the instance. + """ name: str type: str @@ -63,14 +79,27 @@ class LxdInstanceConfig(TypedDict): class LxdStoragePoolConfig(TypedDict): - """Configuration of the storage pool.""" + """Configuration of the storage pool. + + Attributes: + source: The storage pool configuration source image. + size: The size of the storage pool, e.g. 30GiB + """ source: str size: str class LxdStoragePoolConfiguration(TypedDict): - """Configuration for LXD storage pool.""" + """Configuration for LXD storage pool. + + Attributes: + name: The storage pool name. + driver: The storage driver being used, i.e. "dir", "btrfs", ... . See \ + https://documentation.ubuntu.com/lxd/en/stable-5.0/reference/storage_drivers/ \ + for more information. + config: The storage pool configuration. + """ name: str driver: str @@ -79,7 +108,16 @@ class LxdStoragePoolConfiguration(TypedDict): @dataclass class LxdNetwork: - """LXD network information.""" + """LXD network information. + + Attributes: + name: The name of LXD network. + description: LXD network descriptor. + type: Network type, i.e. "bridge", "physical" + config: The LXD network configuration values. + managed: Whether the network is being managed by lxd. + used_by: Number of instances using the network. + """ name: str description: str diff --git a/src/metrics.py b/src/metrics.py index 26a74138f..f1c51d452 100644 --- a/src/metrics.py +++ b/src/metrics.py @@ -4,7 +4,7 @@ """Models and functions for the metric events.""" import logging from pathlib import Path -from typing import Optional +from typing import Any, Optional from pydantic import BaseModel, NonNegativeFloat @@ -39,6 +39,7 @@ def _camel_to_snake(camel_case_string: str) -> str: Args: camel_case_string: The string to convert. + Returns: The converted string. """ @@ -50,12 +51,12 @@ def _camel_to_snake(camel_case_string: str) -> str: snake_case_string += char return snake_case_string - def __init__(self, *args, **kwargs): + def __init__(self, *args: Any, **kwargs: Any): """Initialize the event. Args: - *args: The positional arguments to pass to the base class. - **kwargs: The keyword arguments to pass to the base class. These are used to set the + args: The positional arguments to pass to the base class. + kwargs: The keyword arguments to pass to the base class. These are used to set the specific fields. E.g. timestamp=12345 will set the timestamp field to 12345. """ if "event" not in kwargs: @@ -124,6 +125,7 @@ class RunnerStop(Event): status: A string describing the reason for stopping the runner. status_info: More information about the status. job_duration: The duration of the job in seconds. + job_conclusion: The job conclusion, e.g. "success", "failure", ... """ flavor: str @@ -177,15 +179,21 @@ def _enable_logrotate() -> None: Raises: SubprocessError: If the logrotate.timer cannot be enabled and started. """ - execute_command([SYSTEMCTL_PATH, "enable", LOG_ROTATE_TIMER_SYSTEMD_SERVICE], check_exit=True) - - _, retcode = execute_command( - [SYSTEMCTL_PATH, "is-active", "--quiet", LOG_ROTATE_TIMER_SYSTEMD_SERVICE] - ) - if retcode != 0: + try: execute_command( - [SYSTEMCTL_PATH, "start", LOG_ROTATE_TIMER_SYSTEMD_SERVICE], check_exit=True + [SYSTEMCTL_PATH, "enable", LOG_ROTATE_TIMER_SYSTEMD_SERVICE], check_exit=True + ) + + _, retcode = execute_command( + [SYSTEMCTL_PATH, "is-active", "--quiet", LOG_ROTATE_TIMER_SYSTEMD_SERVICE] ) + if retcode != 0: + execute_command( + [SYSTEMCTL_PATH, "start", LOG_ROTATE_TIMER_SYSTEMD_SERVICE], check_exit=True + ) + # 2024/04/02 - We should define a new error, wrap it and re-raise it. + except SubprocessError: # pylint: disable=try-except-raise + raise def _configure_logrotate() -> None: @@ -204,7 +212,7 @@ def _configure_logrotate() -> None: ) -def setup_logrotate(): +def setup_logrotate() -> None: """Configure logrotate for the metrics log. Raises: diff --git a/src/openstack_cloud/__init__.py b/src/openstack_cloud/__init__.py index 3e87cf695..ebcda4e5d 100644 --- a/src/openstack_cloud/__init__.py +++ b/src/openstack_cloud/__init__.py @@ -3,6 +3,7 @@ """Module for managing Openstack cloud.""" +import logging from pathlib import Path from typing import TypedDict, cast @@ -10,6 +11,9 @@ from errors import OpenStackInvalidConfigError +logger = logging.getLogger(__name__) + + CLOUDS_YAML_PATH = Path(Path.home() / ".config/openstack/clouds.yaml") @@ -30,7 +34,10 @@ def _validate_cloud_config(cloud_config: dict) -> CloudConfig: cloud_config: The configuration in clouds.yaml format to validate. Raises: - InvalidConfigError: if the format of the config is invalid. + OpenStackInvalidConfigError: if the format of the config is invalid. + + Returns: + A typed cloud_config dictionary. """ # dict of format: {clouds: : } try: @@ -57,11 +64,15 @@ def initialize(cloud_config: dict) -> None: Validates config and writes it to disk. + Raises: + OpenStackInvalidConfigError: If there was an given cloud config. + Args: cloud_config: The configuration in clouds.yaml format to apply. - - Raises: - InvalidConfigError: if the format of the config is invalid. """ - valid_config = _validate_cloud_config(cloud_config) + try: + valid_config = _validate_cloud_config(cloud_config) + # 2024/04/02 - We should define a new error, wrap it and re-raise it. + except OpenStackInvalidConfigError: # pylint: disable=try-except-raise + raise _write_config_to_disk(valid_config) diff --git a/src/openstack_cloud/openstack_manager.py b/src/openstack_cloud/openstack_manager.py index 58bd30ad8..ff8b1cf38 100644 --- a/src/openstack_cloud/openstack_manager.py +++ b/src/openstack_cloud/openstack_manager.py @@ -53,7 +53,7 @@ def _create_connection( Raises: OpenStackUnauthorizedError: if the credentials provided is not authorized. - Returns: + Yields: An openstack.connection.Connection object. """ clouds = list(cloud_config["clouds"].keys()) @@ -119,7 +119,9 @@ def _generate_docker_proxy_unit_file(proxies: Optional[ProxyConfig] = None) -> s return environment.get_template("systemd-docker-proxy.j2").render(proxies=proxies) -def _generate_docker_client_proxy_config_json(http_proxy: str, https_proxy: str, no_proxy: str): +def _generate_docker_client_proxy_config_json( + http_proxy: str, https_proxy: str, no_proxy: str +) -> str: """Generate proxy config.json for docker client. Args: @@ -187,7 +189,7 @@ def _build_image_command( class InstanceConfig: """The configuration values for creating a single runner instance. - Args: + Attributes: name: Name of the image to launch the GitHub runner instance with. labels: The runner instance labels. registration_token: Token for registering the runner on GitHub. @@ -202,7 +204,10 @@ class InstanceConfig: openstack_image: openstack.image.v2.image.Image -def _get_supported_runner_arch(arch: str) -> Literal["amd64", "arm64"]: +SupportedCloudImageArch = Literal["amd64", "arm64"] + + +def _get_supported_runner_arch(arch: str) -> SupportedCloudImageArch: """Validate and return supported runner architecture. The supported runner architecture takes in arch value from Github supported architecture and @@ -229,6 +234,43 @@ def _get_supported_runner_arch(arch: str) -> Literal["amd64", "arm64"]: raise UnsupportedArchitectureError(arch) +class ImageDeleteError(Exception): + """Represents an error while deleting existing openstack image.""" + + +def _put_image(cloud_config: dict[str, dict], image_arch: SupportedCloudImageArch) -> str: + """Create or replace the image with existing name. + + Args: + cloud_config: The cloud configuration to connect OpenStack with. + image_arch: Ubuntu cloud image architecture. + + Raises: + ImageDeleteError: If there was an error deleting the image. + OpenStackCloudException: If there was an error communicating with the Openstack API. + + Returns: + The ID of the image created. + """ + try: + with _create_connection(cloud_config) as conn: + existing_image: openstack.image.v2.image.Image + for existing_image in conn.search_images(name_or_id=IMAGE_NAME): + # images with same name (different ID) can be created and will error during server + # instantiation. + if not conn.delete_image(name_or_id=existing_image.id, wait=True): + raise ImageDeleteError("Failed to delete duplicate image on Openstack.") + image: openstack.image.v2.image.Image = conn.create_image( + name=IMAGE_NAME, + filename=IMAGE_PATH_TMPL.format(architecture=image_arch), + wait=True, + ) + return image.id + # 2024/04/02 - We should define a new error, wrap it and re-raise it. + except OpenStackCloudException: # pylint: disable=try-except-raise + raise + + def build_image( arch: Arch, cloud_config: dict[str, dict], @@ -239,13 +281,14 @@ def build_image( """Build and upload an image to OpenStack. Args: + arch: The system architecture to build the image for. cloud_config: The cloud configuration to connect OpenStack with. github_client: The Github client to interact with Github API. path: Github organisation or repository path. proxies: HTTP proxy settings. Raises: - ImageBuildError: If there were errors building/creating the image. + OpenstackImageBuildError: If there were errors building/creating the image. Returns: The created OpenStack image id. @@ -267,23 +310,9 @@ def build_image( raise OpenstackImageBuildError(f"Unsupported architecture {runner_arch}") from exc try: - with _create_connection(cloud_config) as conn: - existing_image: openstack.image.v2.image.Image - for existing_image in conn.search_images(name_or_id=IMAGE_NAME): - # images with same name (different ID) can be created and will error during server - # instantiation. - if not conn.delete_image(name_or_id=existing_image.id, wait=True): - raise OpenstackImageBuildError( - "Failed to delete duplicate image on Openstack." - ) - image: openstack.image.v2.image.Image = conn.create_image( - name=IMAGE_NAME, - filename=IMAGE_PATH_TMPL.format(architecture=image_arch), - wait=True, - ) - return image.id - except OpenStackCloudException as exc: - raise OpenstackImageBuildError("Failed to upload image.") from exc + return _put_image(cloud_config=cloud_config, image_arch=image_arch) + except (ImageDeleteError, OpenStackCloudException) as exc: + raise OpenstackImageBuildError(f"Failed to upload image: {str(exc)}") from exc def create_instance_config( @@ -296,9 +325,12 @@ def create_instance_config( Args: unit_name: The charm unit name. - image: Ubuntu image flavor. + openstack_image: The openstack image object to create the instance with. path: Github organisation or repository path. github_client: The Github client to interact with Github API. + + Returns: + Instance configuration created. """ app_name, unit_num = unit_name.rsplit("/", 1) suffix = secrets.token_hex(12) @@ -372,6 +404,9 @@ def create_instance( Args: cloud_config: The cloud configuration to connect Openstack with. instance_config: The configuration values for Openstack instance to launch. + proxies: HTTP proxy settings. + dockerhub_mirror: + ssh_debug_connections: Raises: OpenstackInstanceLaunchError: if any errors occurred while launching Openstack instance. diff --git a/src/repo_policy_compliance_client.py b/src/repo_policy_compliance_client.py index 2c169a5ef..ab0ed398a 100644 --- a/src/repo_policy_compliance_client.py +++ b/src/repo_policy_compliance_client.py @@ -35,6 +35,10 @@ def __init__(self, session: requests.Session, url: str, charm_token: str) -> Non def get_one_time_token(self) -> str: """Get a single-use token for repo policy compliance check. + Raises: + HTTPError: If there was an error getting one-time token from repo-policy-compliance \ + service. + Returns: The one-time token to be used in a single request of repo policy compliance check. """ diff --git a/src/runner.py b/src/runner.py index 290527d79..a13fa2b88 100644 --- a/src/runner.py +++ b/src/runner.py @@ -52,7 +52,13 @@ class Snap(NamedTuple): - """This class represents a snap installation.""" + """This class represents a snap installation. + + Attributes: + name: The snap application name. + channel: The channel to install the snap from. + revision: The revision number of the snap installation. + """ name: str channel: str @@ -63,7 +69,7 @@ class Snap(NamedTuple): class WgetExecutable: """The executable to be installed through wget. - Args: + Attributes: url: The URL of the executable binary. cmd: Executable command name. E.g. yq_linux_amd64 -> yq """ @@ -76,7 +82,7 @@ class WgetExecutable: class CreateRunnerConfig: """The configuration values for creating a single runner instance. - Args: + Attributes: image: Name of the image to launch the LXD instance with. resources: Resource setting for the LXD instance. binary_path: Path to the runner binary. @@ -92,7 +98,16 @@ class CreateRunnerConfig: class Runner: - """Single instance of GitHub self-hosted runner.""" + """Single instance of GitHub self-hosted runner. + + Attributes: + runner_application: The runner application directory path + env_file: The runner environment source .env file path. + config_script: The runner configuration script file path. + runner_script: The runner start script file path. + pre_job_script: The runner pre_job script file path. This is referenced in the env_file in + the ACTIONS_RUNNER_HOOK_JOB_STARTED environment variable. + """ runner_application = Path("/home/ubuntu/github-runner") env_file = runner_application / ".env" @@ -112,6 +127,7 @@ def __init__( Args: clients: Clients to access various services. runner_config: Configuration of the runner instance. + runner_status: Status info of the given runner. instance: LXD instance of the runner if already created. """ # Dependency injection to share the instances across different `Runner` instance. @@ -122,7 +138,7 @@ def __init__( self._shared_fs: Optional[shared_fs.SharedFilesystem] = None - def create(self, config: CreateRunnerConfig): + def create(self, config: CreateRunnerConfig) -> None: """Create the runner instance on LXD and register it on GitHub. Args: @@ -159,6 +175,62 @@ def create(self, config: CreateRunnerConfig): except (RunnerError, LxdError) as err: raise RunnerCreateError(f"Unable to create runner {self.config.name}") from err + def _remove_lxd_runner(self, remove_token: Optional[str]) -> None: + """Remove running LXD runner instance. + + Args: + remove_token: The Github remove token to execute removal with config.sh script. + + Raises: + LxdError:If there was an error removing LXD runner instance. + """ + logger.info("Executing command to removal of runner and clean up...") + + if not self.instance: + return + + if remove_token: + self.instance.execute( + [ + "/usr/bin/sudo", + "-u", + "ubuntu", + str(self.config_script), + "remove", + "--token", + remove_token, + ], + hide_cmd=True, + ) + + if self.instance.status == "Running": + logger.info("Removing LXD instance of runner: %s", self.config.name) + try: + self.instance.stop(wait=True, timeout=60) + except LxdError: + logger.exception( + "Unable to gracefully stop runner %s within timeout.", self.config.name + ) + logger.info("Force stopping of runner %s", self.config.name) + try: + self.instance.stop(force=True) + except LxdError as exc: + logger.error("Error stopping instance, %s", exc) + raise + else: + # Delete ephemeral instances that have error or stopped status which LXD failed to + # clean up. + logger.warning( + "Found runner %s with status %s, forcing deletion", + self.config.name, + self.instance.status, + ) + try: + self.instance.delete(wait=True) + except LxdError as exc: + logger.error("Error deleting instance, %s", exc) + raise + def remove(self, remove_token: Optional[str]) -> None: """Remove this runner instance from LXD and GitHub. @@ -170,48 +242,10 @@ def remove(self, remove_token: Optional[str]) -> None: """ logger.info("Removing runner: %s", self.config.name) - if self.instance: - logger.info("Executing command to removal of runner and clean up...") - - if remove_token: - self.instance.execute( - [ - "/usr/bin/sudo", - "-u", - "ubuntu", - str(self.config_script), - "remove", - "--token", - remove_token, - ], - hide_cmd=True, - ) - - if self.instance.status == "Running": - logger.info("Removing LXD instance of runner: %s", self.config.name) - try: - self.instance.stop(wait=True, timeout=60) - except LxdError: - logger.exception( - "Unable to gracefully stop runner %s within timeout.", self.config.name - ) - logger.info("Force stopping of runner %s", self.config.name) - try: - self.instance.stop(force=True) - except LxdError as err: - raise RunnerRemoveError(f"Unable to remove {self.config.name}") from err - else: - # Delete ephemeral instances that have error or stopped status which LXD failed to - # clean up. - logger.warning( - "Found runner %s with status %s, forcing deletion", - self.config.name, - self.instance.status, - ) - try: - self.instance.delete(wait=True) - except LxdError as err: - raise RunnerRemoveError(f"Unable to remove {self.config.name}") from err + try: + self._remove_lxd_runner(remove_token=remove_token) + except LxdError as exc: + raise RunnerRemoveError(f"Unable to remove {self.config.name}") from exc if self.status.runner_id is None: return @@ -272,6 +306,9 @@ def _create_instance( resources: Configuration of the virtual machine resources. ephemeral: Whether the instance is ephemeral. + Raises: + LxdError: if there was an error creating an LXD instance. + Returns: LXD instance of the runner. """ @@ -343,7 +380,11 @@ def _ensure_runner_profile(self) -> None: @retry(tries=5, delay=10, local_logger=logger) def _ensure_runner_storage_pool(self) -> None: - """Ensure the runner storage pool exists.""" + """Ensure the runner storage pool exists. + + Raises: + RunnerError: If there was an error creating LXD storage pool. + """ if self._clients.lxd.storage_pools.exists("runner"): logger.info("Found existing runner LXD storage pool.") return @@ -447,8 +488,8 @@ def _get_resource_profile(self, resources: VirtualMachineResources) -> str: def _start_instance(self) -> None: """Start an instance and wait for it to boot. - Args: - reconcile_interval: Time in seconds of period between each reconciliation. + Raises: + RunnerError: If the runner has not instantiated before calling this operation. """ if self.instance is None: raise RunnerError("Runner operation called prior to runner creation.") @@ -460,6 +501,11 @@ def _start_instance(self) -> None: @retry(tries=20, delay=30, local_logger=logger) def _wait_boot_up(self) -> None: + """Wait for LXD instance to boot up. + + Raises: + RunnerError: If there was an error while waiting for the runner to boot up. + """ if self.instance is None: raise RunnerError("Runner operation called prior to runner creation.") @@ -477,9 +523,11 @@ def _install_binaries(self, runner_binary: Path, arch: Arch) -> None: Args: runner_binary: Path to the compressed runner binary. + arch: The runner system architecture. Raises: RunnerFileLoadError: Unable to load the runner binary into the runner instance. + RunnerError: If the runner has not instantiated before calling this operation. """ if self.instance is None: raise RunnerError("Runner operation called prior to runner creation.") @@ -534,6 +582,9 @@ def _should_render_templates_with_metrics(self) -> bool: def _get_default_ip(self) -> Optional[str]: """Get the default IP of the runner. + Raises: + RunnerError: If the runner has not instantiated before calling this operation. + Returns: The default IP of the runner or None if not found. """ @@ -560,6 +611,7 @@ def _configure_aproxy(self, proxy_address: str) -> None: Raises: RunnerAproxyError: If unable to configure aproxy. + RunnerError: If the runner has not instantiated before calling this operation. """ if self.instance is None: raise RunnerError("Runner operation called prior to runner creation.") @@ -602,8 +654,12 @@ def _configure_aproxy(self, proxy_address: str) -> None: ) self.instance.execute(["nft", "-f", "-"], input=nft_input.encode("utf-8")) - def _configure_docker_proxy(self): - """Configure docker proxy.""" + def _configure_docker_proxy(self) -> None: + """Configure docker proxy. + + Raises: + RunnerError: If the runner has not instantiated before calling this operation. + """ if self.instance is None: raise RunnerError("Runner operation called prior to runner creation.") @@ -645,6 +701,7 @@ def _configure_runner(self) -> None: Raises: RunnerFileLoadError: Unable to load configuration file on the runner. + RunnerError: If the runner has not instantiated before calling this operation. """ if self.instance is None: raise RunnerError("Runner operation called prior to runner creation.") @@ -653,7 +710,11 @@ def _configure_runner(self) -> None: startup_contents = self._clients.jinja.get_template("start.j2").render( issue_metrics=self._should_render_templates_with_metrics() ) - self._put_file(str(self.runner_script), startup_contents, mode="0755") + try: + self._put_file(str(self.runner_script), startup_contents, mode="0755") + # 2024/04/02 - We should define a new error, wrap it and re-raise it. + except RunnerFileLoadError: # pylint: disable=try-except-raise + raise self.instance.execute(["/usr/bin/sudo", "chown", "ubuntu:ubuntu", str(self.runner_script)]) self.instance.execute(["/usr/bin/sudo", "chmod", "u+x", str(self.runner_script)]) @@ -722,6 +783,9 @@ def _register_runner(self, registration_token: str, labels: Sequence[str]) -> No Args: registration_token: Registration token request from GitHub. labels: Labels to tag the runner with. + + Raises: + RunnerError: If the runner has not instantiated before calling this operation. """ if self.instance is None: raise RunnerError("Runner operation called prior to runner creation.") @@ -757,7 +821,11 @@ def _register_runner(self, registration_token: str, labels: Sequence[str]) -> No @retry(tries=5, delay=30, local_logger=logger) def _start_runner(self) -> None: - """Start the GitHub runner.""" + """Start the GitHub runner. + + Raises: + RunnerError: If the runner has not instantiated before calling this operation. + """ if self.instance is None: raise RunnerError("Runner operation called prior to runner creation.") @@ -780,9 +848,11 @@ def _put_file(self, filepath: str, content: str, mode: Optional[str] = None) -> Args: filepath: Path to load the file in the runner instance. content: Content of the file. + mode: File permission setting. Raises: RunnerFileLoadError: Failed to load the file into the runner instance. + RunnerError: If the runner has not instantiated before calling this operation. """ if self.instance is None: raise RunnerError("Runner operation called prior to runner creation.") @@ -812,6 +882,10 @@ def _snap_install(self, snaps: Iterable[Snap]) -> None: This is a temporary solution to provide tools not offered by the base ubuntu image. Custom images based on the GitHub action runner image will be used in the future. + Raises: + RunnerError: if the runner was not created before calling the method. + RunnerCreateError: If there was an error installing a snap. + Args: snaps: snaps to be installed. """ diff --git a/src/runner_manager.py b/src/runner_manager.py index 8a8992b1c..2693b21b6 100644 --- a/src/runner_manager.py +++ b/src/runner_manager.py @@ -11,21 +11,29 @@ import time from datetime import datetime, timedelta, timezone from pathlib import Path -from typing import Dict, Iterator, Optional, Type +from typing import Iterator, Optional, Type import jinja2 import requests import requests.adapters import urllib3 -import errors import github_metrics import metrics import runner_logs import runner_metrics import shared_fs from charm_state import VirtualMachineResources -from errors import IssueMetricEventError, RunnerBinaryError, RunnerCreateError +from errors import ( + GetSharedFilesystemError, + GithubClientError, + GithubMetricsError, + IssueMetricEventError, + RunnerBinaryError, + RunnerCreateError, + RunnerLogsError, + SubprocessError, +) from github_client import GithubClient from github_type import RunnerApplication, SelfHostedRunner from lxd import LxdClient, LxdInstance @@ -48,7 +56,12 @@ class RunnerManager: - """Manage a group of runners according to configuration.""" + """Manage a group of runners according to configuration. + + Attributes: + runner_bin_path: The github runner app scripts path. + cron_path: The path to runner build image cron job. + """ runner_bin_path = Path("/home/ubuntu/github-runner-app") cron_path = Path("/etc/cron.d") @@ -147,6 +160,9 @@ def update_runner_bin(self, binary: RunnerApplication) -> None: Args: binary: Information on the runner binary to download. + + Raises: + RunnerBinaryError: If there was an error updating runner binary info. """ logger.info("Downloading runner binary from: %s", binary["download_url"]) @@ -216,6 +232,11 @@ def get_github_info(self) -> Iterator[RunnerInfo]: ) def _get_runner_health_states(self) -> RunnerByHealth: + """Get all runners sorted into health groups. + + Returns: + All runners sorted by health statuses. + """ local_runners = [ instance # Pylint cannot find the `all` method. @@ -237,7 +258,7 @@ def _get_runner_health_states(self) -> RunnerByHealth: def _create_runner( self, registration_token: str, resources: VirtualMachineResources, runner: Runner - ): + ) -> None: """Create a runner. Issues RunnerInstalled metric if metrics_logging is enabled. @@ -272,7 +293,7 @@ def _create_runner( try: fs = shared_fs.get(runner.config.name) - except errors.GetSharedFilesystemError: + except GetSharedFilesystemError: logger.exception( "Failed to get shared filesystem for runner %s, " "will not be able to issue all metrics.", @@ -317,7 +338,7 @@ def _issue_runner_metrics(self) -> IssuedMetricEventsStats: pre_job_metrics=extracted_metrics.pre_job, runner_name=extracted_metrics.runner_name, ) - except errors.GithubMetricsError: + except GithubMetricsError: logger.exception("Failed to calculate job metrics") job_metrics = None @@ -335,7 +356,7 @@ def _issue_reconciliation_metric( metric_stats: IssuedMetricEventsStats, reconciliation_start_ts: float, reconciliation_end_ts: float, - ): + ) -> None: """Issue reconciliation metric. Args: @@ -421,12 +442,15 @@ def _get_runner_config(self, name: str) -> RunnerConfig: ssh_debug_connections=self.config.charm_state.ssh_debug_connections, ) - def _spawn_new_runners(self, count: int, resources: VirtualMachineResources): + def _spawn_new_runners(self, count: int, resources: VirtualMachineResources) -> None: """Spawn new runners. Args: count: Number of runners to spawn. resources: Configuration of the virtual machine resources. + + Raises: + RunnerCreateError: If there was an error spawning new runner. """ if not RunnerManager.runner_bin_path.exists(): raise RunnerCreateError("Unable to create runner due to missing runner binary.") @@ -471,6 +495,34 @@ def _remove_runners(self, count: int, runners: list[Runner]) -> None: else: logger.info("There are no idle runners to remove.") + def _cleanup_offline_runners( + self, runner_states: RunnerByHealth, all_runners: list[Runner] + ) -> None: + """Cleanup runners that are not running the github run.sh script. + + Args: + runner_states: Runner names grouped by health. + all_runners: All currently running runners. + """ + if not runner_states.unhealthy: + logger.info("No unhealthy runners.") + return + + logger.info("Cleaning up unhealthy runners.") + remove_token = self._clients.github.get_runner_remove_token(self.config.path) + unhealthy_runners = [ + runner for runner in all_runners if runner.config.name in set(runner_states.unhealthy) + ] + + for runner in unhealthy_runners: + if self.config.are_metrics_enabled: + try: + runner_logs.get_crashed(runner) + except RunnerLogsError: + logger.exception("Failed to get logs of crashed runner %s", runner.config.name) + runner.remove(remove_token) + logger.info(REMOVED_RUNNER_LOG_STR, runner.config.name) + def reconcile(self, quantity: int, resources: VirtualMachineResources) -> int: """Bring runners in line with target. @@ -481,18 +533,14 @@ def reconcile(self, quantity: int, resources: VirtualMachineResources) -> int: Returns: Difference between intended runners and actual runners. """ - if self.config.are_metrics_enabled: - start_ts = time.time() + start_ts = time.time() runners = self._get_runners() - # Add/Remove runners to match the target quantity online_runners = [ runner for runner in runners if runner.status.exist and runner.status.online ] - runner_states = self._get_runner_health_states() - logger.info( ( "Expected runner count: %i, Online count: %i, Offline count: %i, " @@ -509,26 +557,7 @@ def reconcile(self, quantity: int, resources: VirtualMachineResources) -> int: if self.config.are_metrics_enabled: metric_stats = self._issue_runner_metrics() - # Clean up offline runners - if runner_states.unhealthy: - logger.info("Cleaning up unhealthy runners.") - - remove_token = self._clients.github.get_runner_remove_token(self.config.path) - - unhealthy_runners = [ - runner for runner in runners if runner.config.name in set(runner_states.unhealthy) - ] - - for runner in unhealthy_runners: - if self.config.are_metrics_enabled: - try: - runner_logs.get_crashed(runner) - except errors.RunnerLogsError: - logger.exception( - "Failed to get logs of crashed runner %s", runner.config.name - ) - runner.remove(remove_token) - logger.info(REMOVED_RUNNER_LOG_STR, runner.config.name) + self._cleanup_offline_runners(runner_states=runner_states, all_runners=runners) delta = quantity - len(runner_states.healthy) # Spawn new runners @@ -582,12 +611,16 @@ def flush(self, mode: FlushMode = FlushMode.FLUSH_IDLE) -> int: Args: mode: Strategy for flushing runners. + Raises: + GithubClientError: If there was an error getting remove-token to unregister runners \ + from GitHub. + Returns: Number of runners removed. """ try: remove_token = self._clients.github.get_runner_remove_token(self.config.path) - except errors.GithubClientError: + except GithubClientError: logger.exception("Failed to get remove-token to unregister runners from GitHub.") if mode != FlushMode.FORCE_FLUSH_WAIT_REPO_CHECK: raise @@ -650,7 +683,12 @@ def _generate_runner_name(self) -> str: suffix = secrets.token_hex(12) return f"{self.instance_name}-{suffix}" - def _get_runner_github_info(self) -> Dict[str, SelfHostedRunner]: + def _get_runner_github_info(self) -> dict[str, SelfHostedRunner]: + """Get a mapping of runner name to GitHub self-hosted runner info. + + Returns: + A mapping of runner name to GitHub self-hosted runner info. + """ remote_runners_list: list[SelfHostedRunner] = self._clients.github.get_runner_github_info( self.config.path ) @@ -675,7 +713,16 @@ def create_runner_info( local_runner: Optional[LxdInstance], remote_runner: Optional[SelfHostedRunner], ) -> Runner: - """Create runner from information from GitHub and LXD.""" + """Create runner from information from GitHub and LXD. + + Args: + name: Name of the runner. + local_runner: The Lxd runner. + remote_runner: The Github self hosted runner. + + Returns: + Wrapped runner information. + """ logger.debug( ( "Found runner %s with GitHub info [status: %s, busy: %s, labels: %s] and LXD " @@ -744,9 +791,13 @@ def build_runner_image(self) -> None: Build container image in test mode, else virtual machine image. Raises: - LxdError: Unable to build the LXD image. + SubprocessError: Unable to build the LXD image. """ - execute_command(self._build_image_command()) + try: + execute_command(self._build_image_command()) + except SubprocessError as exc: + logger.error("Error executing build image command, %s", exc) + raise def schedule_build_runner_image(self) -> None: """Install cron job for building runner image.""" diff --git a/src/runner_manager_type.py b/src/runner_manager_type.py index 784884689..895017a33 100644 --- a/src/runner_manager_type.py +++ b/src/runner_manager_type.py @@ -46,6 +46,7 @@ class RunnerManagerClients: github: Used to query GitHub API. jinja: Used for templating. lxd: Used to interact with LXD API. + repo: Used to interact with repo-policy-compliance API. """ github: GithubClient @@ -60,6 +61,7 @@ class RunnerManagerConfig: # pylint: disable=too-many-instance-attributes """Configuration of runner manager. Attributes: + are_metrics_enabled: Whether metrics for the runners should be collected. charm_state: The state of the charm. image: Name of the image for creating LXD instance. lxd_storage_path: Path to be used as LXD storage. diff --git a/src/runner_metrics.py b/src/runner_metrics.py index 7b8807f79..e6c76c4f3 100644 --- a/src/runner_metrics.py +++ b/src/runner_metrics.py @@ -8,14 +8,13 @@ from enum import Enum from json import JSONDecodeError from pathlib import Path -from typing import Iterator, Optional, Type +from typing import Generator, Optional, Type from pydantic import BaseModel, Field, NonNegativeFloat, ValidationError -import errors import metrics import shared_fs -from errors import CorruptMetricDataError +from errors import CorruptMetricDataError, DeleteSharedFilesystemError, IssueMetricEventError from metrics_type import GithubJobMetrics logger = logging.getLogger(__name__) @@ -29,7 +28,7 @@ class PreJobMetrics(BaseModel): """Metrics for the pre-job phase of a runner. - Args: + Attributes: timestamp: The UNIX time stamp of the time at which the event was originally issued. workflow: The workflow name. workflow_run_id: The workflow run id. @@ -45,7 +44,13 @@ class PreJobMetrics(BaseModel): class PostJobStatus(str, Enum): - """The status of the post-job phase of a runner.""" + """The status of the post-job phase of a runner. + + Attributes: + NORMAL: Represents a normal post-job. + ABNORMAL: Represents an error with post-job. + REPO_POLICY_CHECK_FAILURE: Represents an error with repo-policy-compliance check. + """ NORMAL = "normal" ABNORMAL = "abnormal" @@ -65,7 +70,7 @@ class CodeInformation(BaseModel): class PostJobMetrics(BaseModel): """Metrics for the post-job phase of a runner. - Args: + Attributes: timestamp: The UNIX time stamp of the time at which the event was originally issued. status: The status of the job. status_info: More information about the status. @@ -79,7 +84,7 @@ class PostJobMetrics(BaseModel): class RunnerMetrics(BaseModel): """Metrics for a runner. - Args: + Attributes: installed_timestamp: The UNIX time stamp of the time at which the runner was installed. pre_job: The metrics for the pre-job phase. post_job: The metrics for the post-job phase. @@ -112,6 +117,36 @@ def _inspect_file_sizes(fs: shared_fs.SharedFilesystem) -> tuple[Path, ...]: ) +def _extract_metrics_from_fs_file( + fs: shared_fs.SharedFilesystem, runner_name: str, filename: str +) -> dict | None: + """Extract metrics from a shared filesystem. + + Args: + fs: The shared filesystem for a specific runner. + runner_name: The name of the lxd runner to extract metrics from. + filename: The metrics filename. + + Raises: + CorruptMetricDataError: If any errors have been found within the metric. + + Returns: + Metrics for the given runner if present. + """ + try: + job_metrics = json.loads(fs.path.joinpath(filename).read_text()) + except FileNotFoundError: + logger.warning("%s not found for runner %s.", filename, runner_name) + return None + except JSONDecodeError as exc: + raise CorruptMetricDataError(str(exc)) from exc + if not isinstance(job_metrics, dict): + raise CorruptMetricDataError( + f"{filename} metrics for runner {runner_name} is not a JSON object." + ) + return job_metrics + + def _extract_metrics_from_fs(fs: shared_fs.SharedFilesystem) -> Optional[RunnerMetrics]: """Extract metrics from a shared filesystem. @@ -139,32 +174,20 @@ def _extract_metrics_from_fs(fs: shared_fs.SharedFilesystem) -> Optional[RunnerM return None try: - pre_job_metrics = json.loads(fs.path.joinpath(PRE_JOB_METRICS_FILE_NAME).read_text()) - logger.debug("Pre-job metrics for runner %s: %s", runner_name, pre_job_metrics) - except FileNotFoundError: - logger.warning("%s not found for runner %s.", PRE_JOB_METRICS_FILE_NAME, runner_name) - return None - except JSONDecodeError as exc: - raise CorruptMetricDataError(str(exc)) from exc - - try: - post_job_metrics = json.loads(fs.path.joinpath(POST_JOB_METRICS_FILE_NAME).read_text()) - logger.debug("Post-job metrics for runner %s: %s", runner_name, post_job_metrics) - except FileNotFoundError: - logger.warning("%s not found for runner %s", POST_JOB_METRICS_FILE_NAME, runner_name) - post_job_metrics = None - except JSONDecodeError as exc: - raise CorruptMetricDataError(str(exc)) from exc - - if not isinstance(pre_job_metrics, dict): - raise CorruptMetricDataError( - f"Pre-job metrics for runner {runner_name} is not a JSON object." + pre_job_metrics = _extract_metrics_from_fs_file( + fs=fs, runner_name=runner_name, filename=PRE_JOB_METRICS_FILE_NAME ) + if not pre_job_metrics: + return None + logger.debug("Pre-job metrics for runner %s: %s", runner_name, pre_job_metrics) - if not isinstance(post_job_metrics, dict) and post_job_metrics is not None: - raise CorruptMetricDataError( - f"Post-job metrics for runner {runner_name} is not a JSON object." + post_job_metrics = _extract_metrics_from_fs_file( + fs=fs, runner_name=runner_name, filename=POST_JOB_METRICS_FILE_NAME ) + logger.debug("Post-job metrics for runner %s: %s", runner_name, post_job_metrics) + # 2024/04/02 - We should define a new error, wrap it and re-raise it. + except CorruptMetricDataError: # pylint: disable=try-except-raise + raise try: return RunnerMetrics( @@ -181,6 +204,7 @@ def _clean_up_shared_fs(fs: shared_fs.SharedFilesystem) -> None: """Clean up the shared filesystem. Remove all metric files and afterwards the shared filesystem. + Args: fs: The shared filesystem for a specific runner. """ @@ -197,7 +221,7 @@ def _clean_up_shared_fs(fs: shared_fs.SharedFilesystem) -> None: try: shared_fs.delete(fs.runner_name) - except errors.DeleteSharedFilesystemError: + except DeleteSharedFilesystemError: logger.exception("Could not delete shared filesystem for runner %s.", fs.runner_name) @@ -226,7 +250,7 @@ def _extract_fs( return metrics_from_fs -def extract(ignore_runners: set[str]) -> Iterator[RunnerMetrics]: +def extract(ignore_runners: set[str]) -> Generator[RunnerMetrics, None, None]: """Extract metrics from runners. The metrics are extracted from the shared filesystems of the runners. @@ -241,8 +265,8 @@ def extract(ignore_runners: set[str]) -> Iterator[RunnerMetrics]: Args: ignore_runners: The set of runners to ignore. - Returns: - An iterator over the extracted metrics. + Yields: + Extracted runner metrics of a particular runner. """ for fs in shared_fs.list_all(): if fs.runner_name not in ignore_runners: @@ -279,7 +303,7 @@ def issue_events( ) try: metrics.issue_event(runner_start_event) - except errors.IssueMetricEventError: + except IssueMetricEventError: logger.exception( "Not able to issue RunnerStart metric for runner %s. " "Will not issue RunnerStop metric.", @@ -304,7 +328,7 @@ def issue_events( ) try: metrics.issue_event(runner_stop_event) - except errors.IssueMetricEventError: + except IssueMetricEventError: logger.exception( "Not able to issue RunnerStop metric for runner %s.", runner_metrics.runner_name ) diff --git a/src/runner_type.py b/src/runner_type.py index 6294019d8..81cc18b31 100644 --- a/src/runner_type.py +++ b/src/runner_type.py @@ -13,7 +13,12 @@ @dataclass class RunnerByHealth: - """Set of runners LXD instance by health state.""" + """Set of runners LXD instance by health state. + + Attributes: + healthy: Runners that are correctly running runner script. + unhealthy: Runners that are not running runner script. + """ healthy: tuple[str] unhealthy: tuple[str] @@ -21,7 +26,14 @@ class RunnerByHealth: @dataclass class ProxySetting: - """Represent HTTP-related proxy settings.""" + """Represent HTTP-related proxy settings. + + Attributes: + no_proxy: The comma separated URLs to not go through proxy. + http: HTTP proxy URL. + https: HTTPS proxy URL. + aproxy_address: Aproxy URL. + """ no_proxy: Optional[str] http: Optional[str] diff --git a/src/shared_fs.py b/src/shared_fs.py index ca0aeef86..813557876 100644 --- a/src/shared_fs.py +++ b/src/shared_fs.py @@ -7,7 +7,7 @@ import tarfile from dataclasses import dataclass from pathlib import Path -from typing import Iterator +from typing import Generator from errors import ( CreateSharedFilesystemError, @@ -37,8 +37,6 @@ class SharedFilesystem: Attributes: path: The path of the shared filesystem inside the charm. runner_name: The name of the associated runner. - Returns: - The shared filesystem. """ path: Path @@ -161,11 +159,11 @@ def create(runner_name: str) -> SharedFilesystem: return SharedFilesystem(runner_fs_path, runner_name) -def list_all() -> Iterator[SharedFilesystem]: +def list_all() -> Generator[SharedFilesystem, None, None]: """List the shared filesystems. - Returns: - An iterator over shared filesystems. + Yields: + A shared filesystem instance. """ if not FILESYSTEM_BASE_PATH.exists(): return @@ -222,24 +220,31 @@ def get(runner_name: str) -> SharedFilesystem: return SharedFilesystem(runner_fs_path, runner_name) -def delete(runner_name: str) -> None: - """Delete the shared filesystem for the runner. +class UnmountSharedFilesystemError(Exception): + """Represents an error unmounting a shared filesystem.""" + + +def _unmount_runner_fs_path(runner_name: str) -> Path: + """Unmount shared filesystem for given runner. Args: - runner_name: The name of the runner. + runner_name: The name of the runner to unmount shared fs for. Raises: - DeleteSharedFilesystemError: If the shared filesystem could not be deleted. + UnmountSharedFilesystemError: If there was an error trying to unmount shared filesystem. + + Returns: + The runner shared filesystem path that was unmounted. """ runner_fs_path = _get_runner_fs_path(runner_name) if not runner_fs_path.exists(): - raise DeleteSharedFilesystemError(f"Shared filesystem for runner {runner_name} not found.") - runner_image_path = _get_runner_image_path(runner_name) - + raise UnmountSharedFilesystemError( + f"Shared filesystem for runner {runner_name} not found." + ) try: is_mounted = _is_mountpoint(runner_fs_path) except SharedFilesystemMountError as exc: - raise DeleteSharedFilesystemError( + raise UnmountSharedFilesystemError( f"Failed to determine if shared filesystem is mounted for runner {runner_name}" ) from exc @@ -252,9 +257,30 @@ def delete(runner_name: str) -> None: check_exit=True, ) except SubprocessError as exc: - raise DeleteSharedFilesystemError( + raise UnmountSharedFilesystemError( f"Failed to unmount shared filesystem for runner {runner_name}" ) from exc + + return runner_fs_path + + +def delete(runner_name: str) -> None: + """Delete the shared filesystem for the runner. + + Args: + runner_name: The name of the runner. + + Raises: + DeleteSharedFilesystemError: If the shared filesystem could not be deleted. + """ + try: + runner_fs_path = _unmount_runner_fs_path(runner_name=runner_name) + except UnmountSharedFilesystemError as exc: + raise DeleteSharedFilesystemError( + f"Unexpected error while deleting shared Filesystem: {str(exc)}" + ) from exc + + runner_image_path = _get_runner_image_path(runner_name) try: runner_image_path.unlink(missing_ok=True) except OSError as exc: @@ -294,4 +320,8 @@ def move_to_quarantine(runner_name: str) -> None: f"Failed to archive shared filesystem for runner {runner_name}" ) from exc - delete(runner_name) + try: + delete(runner_name) + # 2024/04/02 - We should define a new error, wrap it and re-raise it. + except DeleteSharedFilesystemError: # pylint: disable=try-except-raise + raise diff --git a/src/utilities.py b/src/utilities.py index 60910b575..fe51a7722 100644 --- a/src/utilities.py +++ b/src/utilities.py @@ -8,7 +8,7 @@ import os import subprocess # nosec B404 import time -from typing import Callable, Optional, Sequence, Type, TypeVar +from typing import Any, Callable, Optional, Sequence, Type, TypeVar from typing_extensions import ParamSpec @@ -40,7 +40,7 @@ def retry( # pylint: disable=too-many-arguments delay: Time in seconds to wait between retry. max_delay: Max time in seconds to wait between retry. backoff: Factor to increase the delay by each retry. - logger: Logger for logging. + local_logger: Logger for logging. Returns: The function decorator for retry. @@ -52,15 +52,26 @@ def retry_decorator( """Decorate function with retry. Args: - fn: The function to decorate. + func: The function to decorate. Returns: The resulting function with retry added. """ @functools.wraps(func) - def fn_with_retry(*args, **kwargs) -> ReturnT: - """Wrap the function with retries.""" + def fn_with_retry(*args: ParamT.args, **kwargs: ParamT.kwargs) -> ReturnT: + """Wrap the function with retries. + + Args: + args: The placeholder for decorated function's positional arguments. + kwargs: The placeholder for decorated function's key word arguments. + + Raises: + RuntimeError: Should be unreachable. + + Returns: + Original return type of the decorated function. + """ remain_tries, current_delay = tries, delay for _ in range(tries): @@ -96,7 +107,7 @@ def fn_with_retry(*args, **kwargs) -> ReturnT: def secure_run_subprocess( - cmd: Sequence[str], hide_cmd: bool = False, **kwargs + cmd: Sequence[str], hide_cmd: bool = False, **kwargs: dict[str, Any] ) -> subprocess.CompletedProcess[bytes]: """Run command in subprocess according to security recommendations. @@ -136,7 +147,7 @@ def secure_run_subprocess( return result -def execute_command(cmd: Sequence[str], check_exit: bool = True, **kwargs) -> tuple[str, int]: +def execute_command(cmd: Sequence[str], check_exit: bool = True, **kwargs: Any) -> tuple[str, int]: """Execute a command on a subprocess. The command is executed with `subprocess.run`, additional arguments can be passed to it as @@ -210,6 +221,10 @@ def bytes_with_unit_to_kib(num_bytes: str) -> int: Args: num_bytes: A positive integer followed by one of the following unit: KiB, MiB, GiB, TiB, PiB, EiB. + + Raises: + ValueError: If invalid unit was detected. + Returns: Number of kilobytes. """ diff --git a/tests/conftest.py b/tests/conftest.py index 92cf2c350..a51d3bf65 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -8,7 +8,11 @@ def pytest_addoption(parser: Parser): - """Add options to pytest parser.""" + """Add options to pytest parser. + + Args: + parser: The pytest argument parser. + """ parser.addoption( "--path", action="store", diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index e9f631c94..18a990fa7 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -24,7 +24,11 @@ from juju.model import Model from pytest_operator.plugin import OpsTest -from charm_state import OPENSTACK_CLOUDS_YAML_CONFIG_NAME +from charm_state import ( + OPENSTACK_CLOUDS_YAML_CONFIG_NAME, + PATH_CONFIG_NAME, + VIRTUAL_MACHINES_CONFIG_NAME, +) from github_client import GithubClient from tests.integration.helpers import ( deploy_github_runner_charm, @@ -295,7 +299,7 @@ async def app_scheduled_events( reconcile_interval=8, ) - await application.set_config({"virtual-machines": "1"}) + await application.set_config({VIRTUAL_MACHINES_CONFIG_NAME: "1"}) await reconcile(app=application, model=model) return application @@ -340,7 +344,7 @@ async def app_no_wait_fixture( https_proxy: str, no_proxy: str, ) -> AsyncIterator[Application]: - """GitHub runner charm application without waiting for active.""" + """Github runner charm application without waiting for active.""" app: Application = await deploy_github_runner_charm( model=model, charm_file=charm_file, @@ -354,7 +358,7 @@ async def app_no_wait_fixture( reconcile_interval=60, wait_idle=False, ) - await app.set_config({"virtual-machines": "1"}) + await app.set_config({VIRTUAL_MACHINES_CONFIG_NAME: "1"}) return app @@ -374,7 +378,7 @@ async def tmate_ssh_server_app_fixture( async def tmate_ssh_server_unit_ip_fixture( model: Model, tmate_ssh_server_app: Application, -) -> AsyncIterator[str]: +) -> bytes | str: """tmate-ssh-server charm unit ip.""" status: FullStatus = await model.get_status([tmate_ssh_server_app.name]) try: @@ -467,7 +471,7 @@ async def app_with_forked_repo( """ app = app_no_runner # alias for readability as the app will have a runner during the test - await app.set_config({"path": forked_github_repository.full_name}) + await app.set_config({PATH_CONFIG_NAME: forked_github_repository.full_name}) await ensure_charm_has_runner(app=app, model=model) return app @@ -510,7 +514,15 @@ async def test_github_branch_fixture(github_repository: Repository) -> AsyncIter ) def get_branch(): - """Get newly created branch.""" + """Get newly created branch. + + Raises: + GithubException: if unexpected GithubException has happened apart from repository not \ + found. + + Returns: + New branch if successful, False otherwise. + """ try: branch = github_repository.get_branch(test_branch) except GithubException as err: diff --git a/tests/integration/helpers.py b/tests/integration/helpers.py index aca334a72..b412f083d 100644 --- a/tests/integration/helpers.py +++ b/tests/integration/helpers.py @@ -26,6 +26,15 @@ from juju.model import Model from juju.unit import Unit +from charm_state import ( + DENYLIST_CONFIG_NAME, + PATH_CONFIG_NAME, + RECONCILE_INTERVAL_CONFIG_NAME, + RUNNER_STORAGE_CONFIG_NAME, + TEST_MODE_CONFIG_NAME, + TOKEN_CONFIG_NAME, + VIRTUAL_MACHINES_CONFIG_NAME, +) from runner import Runner from runner_manager import RunnerManager from tests.status_name import ACTIVE @@ -106,10 +115,6 @@ async def assert_resource_lxd_profile(unit: Unit, configs: dict[str, Any]) -> No Args: unit: Unit instance to check for the LXD profile. configs: Configs of the application. - - Raises: - AssertionError: Unable to find an LXD profile with matching resource - config. """ cpu = configs["vm-cpu"]["value"] mem = configs["vm-memory"]["value"] @@ -159,10 +164,6 @@ async def wait_till_num_of_runners(unit: Unit, num: int) -> None: Args: unit: Unit instance to check for the LXD profile. num: Number of runner instances to check for. - - Raises: - AssertionError: Correct number of runners is not found within timeout - limit. """ return_code, stdout = await run_in_unit(unit, "lxc list --format json") assert return_code == 0 @@ -247,6 +248,12 @@ async def run_in_lxd_instance( async def start_test_http_server(unit: Unit, port: int): + """Start test http server. + + Args: + unit: The unit to start the test server in. + port: Http server port. + """ await run_in_unit( unit, f"""cat <> /etc/systemd/system/test-http-server.service @@ -281,7 +288,7 @@ async def ensure_charm_has_runner(app: Application, model: Model) -> None: app: The GitHub Runner Charm app to create the runner for. model: The machine charm model. """ - await app.set_config({"virtual-machines": "1"}) + await app.set_config({VIRTUAL_MACHINES_CONFIG_NAME: "1"}) await reconcile(app=app, model=model) await wait_till_num_of_runners(unit=app.units[0], num=1) @@ -293,6 +300,9 @@ async def get_runner_name(unit: Unit) -> str: Args: unit: The GitHub Runner Charm unit to get the runner name for. + + Returns: + The Github runner name deployed in the given unit. """ runners = await get_runner_names(unit) assert len(runners) == 1 @@ -336,6 +346,7 @@ async def deploy_github_runner_charm( app_name: Application name for the deployment. path: Path representing the GitHub repo/org. token: GitHub Personal Token for the application to use. + runner_storage: Runner storage to use, i.e. "memory" or "juju_storage", http_proxy: HTTP proxy for the application to use. https_proxy: HTTPS proxy for the application to use. no_proxy: No proxy configuration for the application. @@ -344,6 +355,9 @@ async def deploy_github_runner_charm( otherwise. config: Additional custom config to use. wait_idle: wait for model to become idle. + + Returns: + The charm application that was deployed. """ subprocess.run(["sudo", "modprobe", "br_netfilter"]) @@ -361,13 +375,13 @@ async def deploy_github_runner_charm( storage["runner"] = {"pool": "rootfs", "size": 11} default_config = { - "path": path, - "token": token, - "virtual-machines": 0, - "denylist": "10.10.0.0/16", - "test-mode": "insecure", - "reconcile-interval": reconcile_interval, - "runner-storage": runner_storage, + PATH_CONFIG_NAME: path, + TOKEN_CONFIG_NAME: token, + VIRTUAL_MACHINES_CONFIG_NAME: 0, + DENYLIST_CONFIG_NAME: "10.10.0.0/16", + TEST_MODE_CONFIG_NAME: "insecure", + RECONCILE_INTERVAL_CONFIG_NAME: reconcile_interval, + RUNNER_STORAGE_CONFIG_NAME: runner_storage, } if config: @@ -412,6 +426,9 @@ def get_workflow_runs( workflow: The target workflow to get the run for. runner_name: The runner name the workflow job is assigned to. branch: The branch the workflow is run on. + + Yields: + The workflow run. """ if branch is None: branch = github.GithubObject.NotSet @@ -432,6 +449,7 @@ def _get_latest_run( Args: workflow: The workflow to get the latest run for. start_time: The minimum start time of the run. + branch: The branch in which the workflow belongs to. Returns: The latest workflow run if the workflow has started. None otherwise. @@ -449,6 +467,10 @@ def _is_workflow_run_complete(run: WorkflowRun) -> bool: Args: run: The workflow run to check status for. + + Returns: + Whether the run status is "completed". + """ if run.update(): return run.status == "completed" @@ -474,6 +496,7 @@ async def dispatch_workflow( conclusion: The expected workflow run conclusion. workflow_id_or_name: The workflow filename in .github/workflows in main branch to run or its id. + dispatch_input: Workflow input values. Returns: A completed workflow. diff --git a/tests/integration/test_charm_fork_repo.py b/tests/integration/test_charm_fork_repo.py index 4bd4ebec1..2cd1b12f9 100644 --- a/tests/integration/test_charm_fork_repo.py +++ b/tests/integration/test_charm_fork_repo.py @@ -16,6 +16,7 @@ from juju.application import Application from juju.model import Model +from charm_state import PATH_CONFIG_NAME from tests.integration.helpers import ( DISPATCH_FAILURE_TEST_WORKFLOW_FILENAME, get_runner_names, @@ -31,8 +32,8 @@ async def test_dispatch_workflow_failure( forked_github_branch: Branch, ) -> None: """ - arrange: - 1. A forked repository. + arrange: \ + 1. A forked repository. \ 2. A working application with one runner on the forked repository. act: Trigger a workflow dispatch that fails the repo policy check on a branch in the forked repository. @@ -95,7 +96,7 @@ async def test_path_config_change( """ unit = app_with_forked_repo.units[0] - await app_with_forked_repo.set_config({"path": path}) + await app_with_forked_repo.set_config({PATH_CONFIG_NAME: path}) await reconcile(app=app_with_forked_repo, model=model) diff --git a/tests/integration/test_charm_metrics_failure.py b/tests/integration/test_charm_metrics_failure.py index 032b29b1e..be961259d 100644 --- a/tests/integration/test_charm_metrics_failure.py +++ b/tests/integration/test_charm_metrics_failure.py @@ -12,6 +12,7 @@ from juju.model import Model import runner_logs +from charm_state import PATH_CONFIG_NAME, VIRTUAL_MACHINES_CONFIG_NAME from runner_metrics import PostJobStatus from tests.integration.charm_metrics_helpers import ( assert_events_after_reconciliation, @@ -61,7 +62,7 @@ async def test_charm_issues_metrics_for_failed_repo_policy( assert: The RunnerStart, RunnerStop and Reconciliation metric is logged. The Reconciliation metric has the post job status set to failure. """ - await app.set_config({"path": forked_github_repository.full_name}) + await app.set_config({PATH_CONFIG_NAME: forked_github_repository.full_name}) await ensure_charm_has_runner(app=app, model=model) # Clear metrics log to make reconciliation event more predictable @@ -76,7 +77,7 @@ async def test_charm_issues_metrics_for_failed_repo_policy( ) # Set the number of virtual machines to 0 to speedup reconciliation - await app.set_config({"virtual-machines": "0"}) + await app.set_config({VIRTUAL_MACHINES_CONFIG_NAME: "0"}) await reconcile(app=app, model=model) await assert_events_after_reconciliation( @@ -100,7 +101,7 @@ async def test_charm_issues_metrics_for_abnormal_termination( assert: The RunnerStart, RunnerStop and Reconciliation metric is logged. The Reconciliation metric has the post job status set to Abnormal. """ - await app.set_config({"path": forked_github_repository.full_name}) + await app.set_config({PATH_CONFIG_NAME: forked_github_repository.full_name}) await ensure_charm_has_runner(app=app, model=model) unit = app.units[0] @@ -124,7 +125,7 @@ async def test_charm_issues_metrics_for_abnormal_termination( await wait_for_runner_to_be_marked_offline(forked_github_repository, runner_name) # Set the number of virtual machines to 0 to speedup reconciliation - await app.set_config({"virtual-machines": "0"}) + await app.set_config({VIRTUAL_MACHINES_CONFIG_NAME: "0"}) await reconcile(app=app, model=model) await assert_events_after_reconciliation( @@ -155,7 +156,7 @@ async def test_charm_retrieves_logs_from_unhealthy_runners( assert ret_code == 0, "Failed to kill start.sh" # Set the number of virtual machines to 0 to avoid to speedup reconciliation. - await app.set_config({"virtual-machines": "0"}) + await app.set_config({VIRTUAL_MACHINES_CONFIG_NAME: "0"}) await reconcile(app=app, model=model) ret_code, stdout = await run_in_unit(unit, f"ls {runner_logs.CRASHED_RUNNER_LOGS_DIR_PATH}") diff --git a/tests/integration/test_charm_metrics_success.py b/tests/integration/test_charm_metrics_success.py index dcb26c542..bc0637662 100644 --- a/tests/integration/test_charm_metrics_success.py +++ b/tests/integration/test_charm_metrics_success.py @@ -13,6 +13,7 @@ from juju.application import Application from juju.model import Model +from charm_state import PATH_CONFIG_NAME, VIRTUAL_MACHINES_CONFIG_NAME from runner_metrics import PostJobStatus from tests.integration.charm_metrics_helpers import ( assert_events_after_reconciliation, @@ -52,7 +53,6 @@ async def test_charm_issues_runner_installed_metric(app: Application, model: Mod act: Config the charm to contain one runner. assert: The RunnerInstalled metric is logged. """ - await ensure_charm_has_runner(app=app, model=model) metrics_log = await get_metrics_log(app.units[0]) @@ -81,7 +81,7 @@ async def test_charm_issues_metrics_after_reconciliation( assert: The RunnerStart, RunnerStop and Reconciliation metric is logged. The Reconciliation metric has the post job status set to normal. """ - await app.set_config({"path": forked_github_repository.full_name}) + await app.set_config({PATH_CONFIG_NAME: forked_github_repository.full_name}) await ensure_charm_has_runner(app=app, model=model) # Clear metrics log to make reconciliation event more predictable @@ -96,7 +96,7 @@ async def test_charm_issues_metrics_after_reconciliation( ) # Set the number of virtual machines to 0 to speedup reconciliation - await app.set_config({"virtual-machines": "0"}) + await app.set_config({VIRTUAL_MACHINES_CONFIG_NAME: "0"}) await reconcile(app=app, model=model) await assert_events_after_reconciliation( @@ -117,7 +117,7 @@ async def test_charm_remounts_shared_fs( act: Dispatch a test workflow and afterwards unmount the shared fs. After that, reconcile. assert: The RunnerStart, RunnerStop and Reconciliation metric is logged. """ - await app.set_config({"path": forked_github_repository.full_name}) + await app.set_config({PATH_CONFIG_NAME: forked_github_repository.full_name}) await ensure_charm_has_runner(app=app, model=model) # Clear metrics log to make reconciliation event more predictable @@ -136,7 +136,7 @@ async def test_charm_remounts_shared_fs( await run_in_unit(unit, f"sudo umount /home/ubuntu/runner-fs/{runner_name}") # Set the number of virtual machines to 0 to speedup reconciliation - await app.set_config({"virtual-machines": "0"}) + await app.set_config({VIRTUAL_MACHINES_CONFIG_NAME: "0"}) await reconcile(app=app, model=model) await assert_events_after_reconciliation( diff --git a/tests/integration/test_charm_no_runner.py b/tests/integration/test_charm_no_runner.py index e7ca21c02..2b849c0ca 100644 --- a/tests/integration/test_charm_no_runner.py +++ b/tests/integration/test_charm_no_runner.py @@ -7,6 +7,7 @@ from juju.application import Application from juju.model import Model +from charm_state import VIRTUAL_MACHINES_CONFIG_NAME from tests.integration.helpers import ( check_runner_binary_exists, get_repo_policy_compliance_pip_info, @@ -179,14 +180,14 @@ async def test_reconcile_runners(model: Model, app_no_runner: Application) -> No unit = app.units[0] # 1. - await app.set_config({"virtual-machines": "1"}) + await app.set_config({VIRTUAL_MACHINES_CONFIG_NAME: "1"}) await reconcile(app=app, model=model) await wait_till_num_of_runners(unit, 1) # 2. - await app.set_config({"virtual-machines": "0"}) + await app.set_config({VIRTUAL_MACHINES_CONFIG_NAME: "0"}) await reconcile(app=app, model=model) diff --git a/tests/integration/test_charm_one_runner.py b/tests/integration/test_charm_one_runner.py index eb305b25d..e1f4ce012 100644 --- a/tests/integration/test_charm_one_runner.py +++ b/tests/integration/test_charm_one_runner.py @@ -11,6 +11,14 @@ from juju.model import Model from charm import GithubRunnerCharm +from charm_state import ( + RUNNER_STORAGE_CONFIG_NAME, + TOKEN_CONFIG_NAME, + VIRTUAL_MACHINES_CONFIG_NAME, + VM_CPU_CONFIG_NAME, + VM_DISK_CONFIG_NAME, + VM_MEMORY_CONFIG_NAME, +) from tests.integration.helpers import ( assert_resource_lxd_profile, ensure_charm_has_runner, @@ -106,7 +114,9 @@ async def test_flush_runner_and_resource_config(app: Application) -> None: await assert_resource_lxd_profile(unit, configs) # 3. - await app.set_config({"vm-cpu": "1", "vm-memory": "3GiB", "vm-disk": "8GiB"}) + await app.set_config( + {VM_CPU_CONFIG_NAME: "1", VM_MEMORY_CONFIG_NAME: "3GiB", VM_DISK_CONFIG_NAME: "8GiB"} + ) # 4. action = await app.units[0].run_action("flush-runners") @@ -156,7 +166,7 @@ async def test_token_config_changed(model: Model, app: Application, token_alt: s """ unit = app.units[0] - await app.set_config({"token": token_alt}) + await app.set_config({TOKEN_CONFIG_NAME: token_alt}) await model.wait_for_idle(status=ACTIVE, timeout=30 * 60) return_code, stdout = await run_in_unit( @@ -188,7 +198,7 @@ async def test_reconcile_runners_with_lxd_storage_pool_failure( unit = app.units[0] # 1. - await app.set_config({"virtual-machines": "0"}) + await app.set_config({VIRTUAL_MACHINES_CONFIG_NAME: "0"}) await reconcile(app=app, model=model) await wait_till_num_of_runners(unit, 0) @@ -197,7 +207,7 @@ async def test_reconcile_runners_with_lxd_storage_pool_failure( assert exit_code == 0 # 2. - await app.set_config({"virtual-machines": "1"}) + await app.set_config({VIRTUAL_MACHINES_CONFIG_NAME: "1"}) await reconcile(app=app, model=model) @@ -219,14 +229,14 @@ async def test_change_runner_storage(model: Model, app: Application) -> None: unit = app.units[0] # 1. - await app.set_config({"runner-storage": "juju-storage"}) + await app.set_config({RUNNER_STORAGE_CONFIG_NAME: "juju-storage"}) await model.wait_for_idle(status=BLOCKED, timeout=1 * 60) assert ( "runner-storage config cannot be changed after deployment" in unit.workload_status_message ) # 2. - await app.set_config({"runner-storage": "memory"}) + await app.set_config({RUNNER_STORAGE_CONFIG_NAME: "memory"}) await model.wait_for_idle(status=ACTIVE, timeout=1 * 60) @@ -287,7 +297,7 @@ async def test_token_config_changed_insufficient_perms( """ unit = app.units[0] - await app.set_config({"token": "invalid-token", "virtual-machines": "0"}) + await app.set_config({TOKEN_CONFIG_NAME: "invalid-token", VIRTUAL_MACHINES_CONFIG_NAME: "0"}) await model.wait_for_idle() await wait_till_num_of_runners(unit, num=0) diff --git a/tests/integration/test_charm_with_juju_storage.py b/tests/integration/test_charm_with_juju_storage.py index ec1b3150e..65faf4c49 100644 --- a/tests/integration/test_charm_with_juju_storage.py +++ b/tests/integration/test_charm_with_juju_storage.py @@ -7,6 +7,7 @@ from juju.application import Application from juju.model import Model +from charm_state import VIRTUAL_MACHINES_CONFIG_NAME from tests.integration.helpers import reconcile, wait_till_num_of_runners @@ -18,7 +19,7 @@ async def test_spawn_one_runner(model: Model, app_juju_storage: Application) -> act: Spawn one runner. assert: One runner should exist. """ - await app_juju_storage.set_config({"virtual-machines": "1"}) + await app_juju_storage.set_config({VIRTUAL_MACHINES_CONFIG_NAME: "1"}) await reconcile(app=app_juju_storage, model=model) await wait_till_num_of_runners(unit=app_juju_storage.units[0], num=1) diff --git a/tests/integration/test_charm_with_proxy.py b/tests/integration/test_charm_with_proxy.py index bef653c34..6e5024fbf 100644 --- a/tests/integration/test_charm_with_proxy.py +++ b/tests/integration/test_charm_with_proxy.py @@ -15,6 +15,15 @@ from juju.model import Model from juju.unit import Unit +from charm_state import ( + DENYLIST_CONFIG_NAME, + PATH_CONFIG_NAME, + RECONCILE_INTERVAL_CONFIG_NAME, + TEST_MODE_CONFIG_NAME, + TOKEN_CONFIG_NAME, + USE_APROXY_CONFIG_NAME, + VIRTUAL_MACHINES_CONFIG_NAME, +) from tests.integration.helpers import ( ensure_charm_has_runner, get_runner_names, @@ -85,7 +94,6 @@ async def app_with_prepared_machine_fixture( proxy: str, ) -> Application: """Application with proxy setup and firewall to block all other network access.""" - await model.set_config( { "apt-http-proxy": proxy, @@ -158,12 +166,12 @@ async def app_with_prepared_machine_fixture( application_name=app_name, series="jammy", config={ - "path": path, - "token": token, - "virtual-machines": 0, - "denylist": "10.10.0.0/16", - "test-mode": "insecure", - "reconcile-interval": 60, + PATH_CONFIG_NAME: path, + TOKEN_CONFIG_NAME: token, + VIRTUAL_MACHINES_CONFIG_NAME: 0, + DENYLIST_CONFIG_NAME: "10.10.0.0/16", + TEST_MODE_CONFIG_NAME: "insecure", + RECONCILE_INTERVAL_CONFIG_NAME: 60, }, constraints={"root-disk": 15}, to=machine.id, @@ -194,7 +202,7 @@ async def app_fixture( """ await app_with_prepared_machine.set_config( { - "virtual-machines": "0", + VIRTUAL_MACHINES_CONFIG_NAME: "0", } ) await reconcile(app=app_with_prepared_machine, model=model) @@ -337,7 +345,7 @@ async def test_usage_of_aproxy(model: Model, app: Application, proxy_logs_filepa """ await app.set_config( { - "experimental-use-aproxy": "true", + USE_APROXY_CONFIG_NAME: "true", } ) await ensure_charm_has_runner(app, model) @@ -392,7 +400,7 @@ async def test_use_proxy_without_aproxy( """ await app.set_config( { - "experimental-use-aproxy": "false", + USE_APROXY_CONFIG_NAME: "false", } ) await ensure_charm_has_runner(app, model) diff --git a/tests/integration/test_debug_ssh.py b/tests/integration/test_debug_ssh.py index 6d977a258..40f3290f9 100644 --- a/tests/integration/test_debug_ssh.py +++ b/tests/integration/test_debug_ssh.py @@ -12,6 +12,7 @@ from juju.application import Application from juju.model import Model +from charm_state import DENYLIST_CONFIG_NAME from tests.integration.helpers import dispatch_workflow, get_job_logs, get_workflow_runs from tests.status_name import ACTIVE @@ -28,14 +29,14 @@ async def test_ssh_debug( tmate_ssh_server_unit_ip: str, ): """ - arrange: given an integrated GitHub-Runner charm and tmate-ssh-server charm with a denylist + arrange: given an integrated GitHub-Runner charm and tmate-ssh-server charm with a denylist \ covering ip ranges of tmate-ssh-server. act: when canonical/action-tmate is triggered. assert: the ssh connection info from action-log and tmate-ssh-server matches. """ await app_no_wait.set_config( { - "denylist": ( + DENYLIST_CONFIG_NAME: ( "0.0.0.0/8,10.0.0.0/8,100.64.0.0/10,169.254.0.0/16," "172.16.0.0/12,192.0.0.0/24,192.0.2.0/24,192.88.99.0/24,192.168.0.0/16," "198.18.0.0/15,198.51.100.0/24,203.0.113.0/24,224.0.0.0/4,233.252.0.0/24," diff --git a/tests/integration/test_self_hosted_runner.py b/tests/integration/test_self_hosted_runner.py index bf76ac68d..34d6933c5 100644 --- a/tests/integration/test_self_hosted_runner.py +++ b/tests/integration/test_self_hosted_runner.py @@ -12,7 +12,12 @@ from juju.application import Application from juju.model import Model -from charm_state import GithubRepo +from charm_state import ( + DOCKERHUB_MIRROR_CONFIG_NAME, + PATH_CONFIG_NAME, + VIRTUAL_MACHINES_CONFIG_NAME, + GithubRepo, +) from github_client import GithubClient from tests.integration.helpers import ( DISPATCH_TEST_WORKFLOW_FILENAME, @@ -48,7 +53,9 @@ async def test_dispatch_workflow_with_dockerhub_mirror( fake_url = "https://example.com:5000" # 1. - await app_runner.set_config({"virtual-machines": "1", "dockerhub-mirror": fake_url}) + await app_runner.set_config( + {VIRTUAL_MACHINES_CONFIG_NAME: "1", DOCKERHUB_MIRROR_CONFIG_NAME: fake_url} + ) action = await unit.run_action("reconcile-runners") await action.wait() await model.wait_for_idle(status=ACTIVE) @@ -120,7 +127,7 @@ async def test_flush_busy_runner( config = await app_runner.get_config() await app_runner.set_config( - {"path": forked_github_repository.full_name, "virtual-machines": "1"} + {PATH_CONFIG_NAME: forked_github_repository.full_name, VIRTUAL_MACHINES_CONFIG_NAME: "1"} ) await reconcile(app=app_runner, model=model) await wait_till_num_of_runners(unit, 1) @@ -176,6 +183,8 @@ async def test_flush_busy_runner( assert runner_to_be_used not in names, "Found a runner that should be flushed" # Ensure the app_runner is back to 0 runners. - await app_runner.set_config({"virtual-machines": "0", "path": config["path"]}) + await app_runner.set_config( + {VIRTUAL_MACHINES_CONFIG_NAME: "0", PATH_CONFIG_NAME: config[PATH_CONFIG_NAME]} + ) await reconcile(app=app_runner, model=model) await wait_till_num_of_runners(unit, 0) diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index fb5a97efd..5b43917ca 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -27,7 +27,15 @@ def runner_binary_path_fixture(tmp_path): return tmp_path / "github-runner-app" -def disk_usage_mock(total_disk): +def disk_usage_mock(total_disk: int): + """Mock disk usage factory. + + Args: + total_disk: Total disk size in bytes. + + Returns: + A disk usage magic mock instance. + """ disk = unittest.mock.MagicMock() disk.total = total_disk disk_usage = unittest.mock.MagicMock(return_value=disk) diff --git a/tests/unit/factories.py b/tests/unit/factories.py index 826075b8a..e17ad5181 100644 --- a/tests/unit/factories.py +++ b/tests/unit/factories.py @@ -14,10 +14,32 @@ import factory.fuzzy from pydantic.networks import IPvAnyAddress -from charm_state import COS_AGENT_INTEGRATION_NAME, DEBUG_SSH_INTEGRATION_NAME, SSHDebugConnection +from charm_state import ( + COS_AGENT_INTEGRATION_NAME, + DEBUG_SSH_INTEGRATION_NAME, + DENYLIST_CONFIG_NAME, + DOCKERHUB_MIRROR_CONFIG_NAME, + GROUP_CONFIG_NAME, + LABELS_CONFIG_NAME, + OPENSTACK_CLOUDS_YAML_CONFIG_NAME, + PATH_CONFIG_NAME, + RECONCILE_INTERVAL_CONFIG_NAME, + RUNNER_STORAGE_CONFIG_NAME, + TEST_MODE_CONFIG_NAME, + TOKEN_CONFIG_NAME, + USE_APROXY_CONFIG_NAME, + VIRTUAL_MACHINES_CONFIG_NAME, + VM_CPU_CONFIG_NAME, + VM_DISK_CONFIG_NAME, + VM_MEMORY_CONFIG_NAME, + SSHDebugConnection, +) T = TypeVar("T") +# DC060: Docstrings have been abbreviated for factories, checking for docstrings on model +# attributes can be skipped. + class BaseMetaFactory(Generic[T], factory.base.FactoryMetaClass): """Used for type hints of factories.""" @@ -33,8 +55,6 @@ def __call__(cls, *args, **kwargs) -> T: # noqa: N805 class SSHDebugInfoFactory( factory.Factory, metaclass=BaseMetaFactory[SSHDebugConnection] # type: ignore ): - # Docstrings have been abbreviated for factories, checking for docstrings on model attributes - # can be skipped. """Generate PathInfos.""" # noqa: DCO060 class Meta: @@ -50,16 +70,22 @@ class Meta: class MockGithubRunnerCharmUnitFactory(factory.Factory): + """Mock github-runner charm unit.""" # noqa: DCO060 + class Meta: + """Configuration for factory.""" # noqa: DCO060 + model = MagicMock name = "github-runner/0" class MockGithubRunnerCharmAppFactory(factory.Factory): - """Mock github-runner charm app.""" + """Mock github-runner charm app.""" # noqa: DCO060 class Meta: + """Configuration for factory.""" # noqa: DCO060 + model = MagicMock planned_units = 1 @@ -67,41 +93,43 @@ class Meta: class MockGithubRunnerCharmModelFactory(factory.Factory): - """Mock github-runner charm model.""" + """Mock github-runner charm model.""" # noqa: DCO060 class Meta: + """Configuration for factory.""" # noqa: DCO060 + model = MagicMock relations: dict[str, list] = {COS_AGENT_INTEGRATION_NAME: [], DEBUG_SSH_INTEGRATION_NAME: []} class MockGithubRunnerCharmFactory(factory.Factory): - """Mock GithubRunnerCharm.""" + """Mock GithubRunnerCharm.""" # noqa: DCO060 class Meta: + """Configuration for factory.""" # noqa: DCO060 + model = MagicMock unit = factory.SubFactory(MockGithubRunnerCharmUnitFactory) app = factory.SubFactory(MockGithubRunnerCharmAppFactory) model = factory.SubFactory(MockGithubRunnerCharmModelFactory) - - # Ignore N805 as the first param is not self for Factory Boy sequences. - # See: https://factoryboy.readthedocs.io/en/stable/introduction.html#sequences - @factory.sequence - def config(n): # noqa: N805 - return { - "path": f"mock_path_{n}", - "token": f"mock_token_{n}", - "group": "default", - "virtual-machines": 1, - "vm-cpu": 2, - "vm-memory": "7GiB", - "vm-disk": "10GiB", - "reconcile-interval": 10, - "test-mode": "", - "denylist": "", - "dockerhub-mirror": "", - "runner-storage": "juju-storage", - "experimental-use-aproxy": False, - "labels": "", + config = factory.Dict( + { + DENYLIST_CONFIG_NAME: "", + DOCKERHUB_MIRROR_CONFIG_NAME: "", + GROUP_CONFIG_NAME: "default", + LABELS_CONFIG_NAME: "", + OPENSTACK_CLOUDS_YAML_CONFIG_NAME: "", + PATH_CONFIG_NAME: factory.Sequence(lambda n: f"mock_path_{n}"), + RECONCILE_INTERVAL_CONFIG_NAME: 10, + RUNNER_STORAGE_CONFIG_NAME: "juju-storage", + TEST_MODE_CONFIG_NAME: "", + TOKEN_CONFIG_NAME: factory.Sequence(lambda n: f"mock_token_{n}"), + USE_APROXY_CONFIG_NAME: False, + VIRTUAL_MACHINES_CONFIG_NAME: 1, + VM_CPU_CONFIG_NAME: 2, + VM_MEMORY_CONFIG_NAME: "7GiB", + VM_DISK_CONFIG_NAME: "10GiB", } + ) diff --git a/tests/unit/mock.py b/tests/unit/mock.py index 873a31573..b9320f8a4 100644 --- a/tests/unit/mock.py +++ b/tests/unit/mock.py @@ -34,6 +34,7 @@ class MockLxdClient: """Mock the behavior of the LXD client.""" def __init__(self): + """Fake init implementation for LxdClient.""" self.instances = MockLxdInstanceManager() self.profiles = MockLxdProfileManager() self.networks = MockLxdNetworkManager() @@ -44,16 +45,39 @@ class MockLxdInstanceManager: """Mock the behavior of the LXD Instances.""" def __init__(self): + """Fake init implementation for LxdInstanceManager.""" self.instances = {} def create(self, config: LxdInstanceConfig, wait: bool = False) -> MockLxdInstance: + """Create an instance with given config. + + Args: + config: The instance configuration to create the instance with. + wait: Placeholder for wait argument. + + Returns: + Mock instance that was created. + """ self.instances[config["name"]] = MockLxdInstance(config["name"]) return self.instances[config["name"]] def get(self, name: str): + """Get an instance with given name. + + Args: + name: The name of the instance to get. + + Returns: + Instance with given name. + """ return self.instances[name] def all(self): + """Return all instances that have not been deleted. + + Returns: + All Lxd fake instances that have not been deleted. + """ return [i for i in self.instances.values() if not i.deleted] @@ -61,22 +85,47 @@ class MockLxdProfileManager: """Mock the behavior of the LXD Profiles.""" def __init__(self): + """Initialization method for LxdProfileManager fake.""" self.profiles = set() def create(self, name: str, config: dict[str, str], devices: dict[str, str]): + """Fake implementation of create method of LxdProfile manager. + + Args: + name: The name of LXD profile. + config: The config of LXD profile to create. + devices: The devices mapping of LXD profile to create with. + """ self.profiles.add(name) - def exists(self, name) -> bool: + def exists(self, name: str) -> bool: + """Fake implementation of exists method of LxdProfile manager. + + Args: + name: The name of LXD profile. + + Returns: + Whether given LXD profile exists. + """ return name in self.profiles class MockLxdNetworkManager: - """Mock the behavior of the LXD networks""" + """Mock the behavior of the LXD networks.""" def __init__(self): + """Placeholder for initialization method for LxdInstance stub.""" pass def get(self, name: str) -> LxdNetwork: + """Stub method get for LxdNetworkManager. + + Args: + name: the name of the LxdNetwork to get. + + Returns: + LxdNetwork stub. + """ return LxdNetwork( "lxdbr0", "", "bridge", {"ipv4.address": "10.1.1.1/24"}, True, ("default") ) @@ -86,6 +135,11 @@ class MockLxdInstance: """Mock the behavior of an LXD Instance.""" def __init__(self, name: str): + """Fake implementation of initialization method for LxdInstance fake. + + Args: + name: The mock instance name to create. + """ self.name = name self.status = "Stopped" self.deleted = False @@ -93,19 +147,46 @@ def __init__(self, name: str): self.files = MockLxdInstanceFileManager() def start(self, wait: bool = True, timeout: int = 60): + """Fake implementation of start method for LxdInstance fake. + + Args: + wait: Placeholder for wait argument. + timeout: Placeholder for timeout argument. + """ self.status = "Running" def stop(self, wait: bool = True, timeout: int = 60): + """Fake implementation of stop method for LxdInstance fake. + + Args: + wait: Placeholder for wait argument. + timeout: Placeholder for timeout argument. + """ self.status = "Stopped" # Ephemeral virtual machine should be deleted on stop. self.deleted = True def delete(self, wait: bool = True): + """Fake implementation of delete method for LxdInstance fake. + + Args: + wait: Placeholder for wait argument. + """ self.deleted = True def execute( self, cmd: Sequence[str], cwd: Optional[str] = None, hide_cmd: bool = False ) -> tuple[int, IO, IO]: + """Implementation for execute for LxdInstance fake. + + Args: + cmd: Placeholder for command to execute. + cwd: Placeholder for working directory to execute command. + hide_cmd: Placeholder for to hide command that is being executed. + + Returns: + Empty tuples values that represent a successful command execution. + """ return 0, io.BytesIO(b""), io.BytesIO(b"") @@ -113,18 +194,46 @@ class MockLxdInstanceFileManager: """Mock the behavior of an LXD Instance's files.""" def __init__(self): + """Initializer for fake instance of LxdInstanceFileManager.""" self.files = {} def mk_dir(self, path): + """Placeholder for mk_dir implementation of LxdInstanceFileManager. + + Args: + path: The path to create. + """ pass def push_file(self, source: str, destination: str, mode: Optional[str] = None): + """Fake push_file implementation of LxdInstanceFileManager. + + Args: + source: Placeholder argument for source file path copy from. + destination: File path to write to. + mode: Placeholder for file write mode. + """ self.files[destination] = "mock_content" def write_file(self, filepath: str, data: Union[bytes, str], mode: Optional[str] = None): + """Fake write_file implementation of LxdInstanceFileManager. + + Args: + filepath: The file path to read. + data: File contents to write + mode: Placeholder for file write mode. + """ self.files[filepath] = data def read_file(self, filepath: str): + """Fake read_file implementation of LxdInstanceFileManager. + + Args: + filepath: The file path to read. + + Returns: + Contents of file. + """ return self.files.get(str(filepath), None) @@ -132,21 +241,51 @@ class MockLxdStoragePoolManager: """Mock the behavior of LXD storage pools.""" def __init__(self): + """Initialize fake storage pools.""" self.pools = {} def all(self): - return [pool for pool in self.pools.values() if not pool.delete] + """Get all non-deleted fake lxd storage pools. + + Returns: + List of all non deleted fake LXD storages. + """ + return [pool for pool in self.pools.values() if not pool.deleted] def get(self, name): + """Get a fake storage pool of given name. + + Args: + name: Name of the storage pool to get. + + Returns: + Fake storage pool of given name. + """ return self.pools[name] def exists(self, name): + """Check if given storage exists in the fake LxdStoragePool. + + Args: + name: Fake storage pool name to check for existence. + + Returns: + If storage pool of given name exists. + """ if name in self.pools: - return not self.pools[name].delete + return not self.pools[name].deleted else: return False def create(self, config): + """Fake LxdStoragePoolManager create function. + + Args: + config: The LXD storage pool config. + + Returns: + Created LXDStoragePool fake. + """ self.pools[config["name"]] = MockLxdStoragePool() return self.pools[config["name"]] @@ -155,30 +294,57 @@ class MockLxdStoragePool: """Mock the behavior of an LXD storage pool.""" def __init__(self): - self.delete = False + """LXD storage pool fake initialization method.""" + self.deleted = False def save(self): + """LXD storage pool fake save method placeholder.""" pass def delete(self): - self.delete = True + """LXD storage pool fake delete method.""" + self.deleted = True class MockErrorResponse: """Mock of an error response for request library.""" def __init__(self): + """Successful error response initialization method.""" self.status_code = 200 def json(self): + """A stub method that always returns error response. + + Returns: + Test error response. + """ return {"metadata": {"err": "test error"}} -def mock_lxd_error_func(*arg, **kargs): +def mock_lxd_error_func(*args, **kwargs): + """A stub function that always raises LxdError. + + Args: + args: Placeholder for positional arguments. + kwargs: Placeholder for key word arguments. + + Raises: + LxdError: always. + """ raise LxdError(MockErrorResponse()) -def mock_runner_error_func(*arg, **kargs): +def mock_runner_error_func(*args, **kwargs): + """A stub function that always raises RunnerError. + + Args: + args: Placeholder for positional arguments. + kwargs: Placeholder for key word arguments. + + Raises: + RunnerError: always. + """ raise RunnerError("test error") @@ -186,10 +352,20 @@ class MockGhapiClient: """Mock for Ghapi client.""" def __init__(self, token: str): + """Initialization method for GhapiClient fake. + + Args: + token: The client token value. + """ self.token = token self.actions = MockGhapiActions() def last_page(self) -> int: + """Last page number stub. + + Returns: + Always zero. + """ return 0 @@ -197,6 +373,7 @@ class MockGhapiActions: """Mock for actions in Ghapi client.""" def __init__(self): + """A placeholder method for test stub/fakes initializtion.""" hash = hashlib.sha256() hash.update(TEST_BINARY) self.test_hash = hash.hexdigest() @@ -206,6 +383,11 @@ def __init__(self): self.remove_token_org = secrets.token_hex() def _list_runner_applications(self): + """A placeholder method for test fake. + + Returns: + A fake runner applications list. + """ runners = [] runners.append( RunnerApplication( @@ -219,27 +401,78 @@ def _list_runner_applications(self): return runners def list_runner_applications_for_repo(self, owner: str, repo: str): + """A placeholder method for test stub. + + Args: + owner: Placeholder for repository owner. + repo: Placeholder for repository name. + + Returns: + A fake runner applications list. + """ return self._list_runner_applications() def list_runner_applications_for_org(self, org: str): + """A placeholder method for test stub. + + Args: + org: Placeholder for repository owner. + + Returns: + A fake runner applications list. + """ return self._list_runner_applications() def create_registration_token_for_repo(self, owner: str, repo: str): + """A placeholder method for test stub. + + Args: + owner: Placeholder for repository owner. + repo: Placeholder for repository name. + + Returns: + Registration token stub. + """ return RegistrationToken( {"token": self.registration_token_repo, "expires_at": "2020-01-22T12:13:35.123-08:00"} ) def create_registration_token_for_org(self, org: str): + """A placeholder method for test stub. + + Args: + org: Placeholder for repository owner. + + Returns: + Registration token stub. + """ return RegistrationToken( {"token": self.registration_token_org, "expires_at": "2020-01-22T12:13:35.123-08:00"} ) def create_remove_token_for_repo(self, owner: str, repo: str): + """A placeholder method for test stub. + + Args: + owner: Placeholder for repository owner. + repo: Placeholder for repository name. + + Returns: + Remove token stub. + """ return RemoveToken( {"token": self.remove_token_repo, "expires_at": "2020-01-22T12:13:35.123-08:00"} ) def create_remove_token_for_org(self, org: str): + """A placeholder method for test stub. + + Args: + org: Placeholder for repository owner. + + Returns: + Remove token stub. + """ return RemoveToken( {"token": self.remove_token_org, "expires_at": "2020-01-22T12:13:35.123-08:00"} ) @@ -247,15 +480,49 @@ def create_remove_token_for_org(self, org: str): def list_self_hosted_runners_for_repo( self, owner: str, repo: str, per_page: int, page: int = 0 ): + """A placeholder method for test stub. + + Args: + owner: Placeholder for repository owner. + repo: Placeholder for repository name. + per_page: Placeholder for responses per page. + page: Placeholder for response page number. + + Returns: + Empty runners stub. + """ return {"runners": []} def list_self_hosted_runners_for_org(self, org: str, per_page: int, page: int = 0): + """A placeholder method for test stub. + + Args: + org: Placeholder for repository owner. + per_page: Placeholder for responses per page. + page: Placeholder for response page number. + + Returns: + Empty runners stub. + """ return {"runners": []} def delete_self_hosted_runner_from_repo(self, owner: str, repo: str, runner_id: str): + """A placeholder method for test stub. + + Args: + owner: Placeholder for repository owner. + repo: Placeholder for repository name. + runner_id: Placeholder for runenr_id. + """ pass def delete_self_hosted_runner_from_org(self, org: str, runner_id: str): + """A placeholder method for test stub. + + Args: + org: Placeholder for organisation. + runner_id: Placeholder for runner id. + """ pass @@ -263,7 +530,19 @@ class MockRepoPolicyComplianceClient: """Mock for RepoPolicyComplianceClient.""" def __init__(self, session=None, url=None, charm_token=None): + """Placeholder method for initialization. + + Args: + session: Placeholder for requests session. + url: Placeholder for repo policy compliance url. + charm_token: Placeholder for charm token. + """ pass def get_one_time_token(self) -> str: + """Generate a test token value. + + Returns: + A test token value. + """ return "MOCK_TOKEN_" + secrets.token_hex(8) diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index bde467313..2033d14ae 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -16,7 +16,15 @@ from charm import GithubRunnerCharm from charm_state import ( + GROUP_CONFIG_NAME, OPENSTACK_CLOUDS_YAML_CONFIG_NAME, + PATH_CONFIG_NAME, + RECONCILE_INTERVAL_CONFIG_NAME, + TOKEN_CONFIG_NAME, + USE_APROXY_CONFIG_NAME, + VIRTUAL_MACHINES_CONFIG_NAME, + VM_CPU_CONFIG_NAME, + VM_DISK_CONFIG_NAME, Arch, GithubOrg, GithubRepo, @@ -32,29 +40,78 @@ TEST_PROXY_SERVER_URL = "http://proxy.server:1234" -def raise_runner_error(*args, **kargs): +def raise_runner_error(*args, **kwargs): + """Stub function to raise RunnerError. + + Args: + args: Positional argument placeholder. + kwargs: Keyword argument placeholder. + + Raises: + RunnerError: Always. + """ raise RunnerError("mock error") -def raise_subprocess_error(*args, **kargs): +def raise_subprocess_error(*args, **kwargs): + """Stub function to raise SubprocessError. + + Args: + args: Positional argument placeholder. + kwargs: Keyword argument placeholder. + + Raises: + SubprocessError: Always. + """ raise SubprocessError(cmd=["mock"], return_code=1, stdout="mock stdout", stderr="mock stderr") -def raise_url_error(*args, **kargs): +def raise_url_error(*args, **kwargs): + """Stub function to raise URLError. + + Args: + args: Positional argument placeholder. + kwargs: Keyword argument placeholder. + + Raises: + URLError: Always. + """ raise urllib.error.URLError("mock error") def mock_get_latest_runner_bin_url(os_name: str = "linux", arch: Arch = Arch.X64): + """Stub function to return test runner_bin_url data. + + Args: + os_name: OS name placeholder argument. + arch: Architecture placeholder argument. + + Returns: + MagicMock runner application. + """ mock = MagicMock() mock.download_url = "www.example.com" return mock def mock_download_latest_runner_image(*args): + """A stub function to download runner latest image. + + Args: + args: Placeholder for positional arguments. + + Returns: + Latest runner image test URL. + """ return "www.example.com" def mock_get_github_info(): + """A stub function that returns mock Github runner information. + + Returns: + RunnerInfo with different name, statuses, busy values. + """ return [ RunnerInfo("test runner 0", GitHubRunnerStatus.ONLINE.value, True), RunnerInfo("test runner 1", GitHubRunnerStatus.ONLINE.value, False), @@ -64,12 +121,28 @@ def mock_get_github_info(): ] -def setup_charm_harness(monkeypatch, runner_bin_path: Path) -> Harness: - def stub_update_runner_bin(self, binary) -> None: +def setup_charm_harness(monkeypatch: pytest.MonkeyPatch, runner_bin_path: Path) -> Harness: + """Setup harness with patched runner manager methods. + + Args: + monkeypatch: Instance of pytest monkeypatch for patching RunnerManager methods. + runner_bin_path: Runner binary temporary path fixture. + + Returns: + Harness with patched RunnerManager instance. + """ + + def stub_update_runner_bin(*args, **kwargs) -> None: + """Update runner bin stub function. + + Args: + args: Placeholder for positional argument values. + kwargs: Placeholder for keyword argument values. + """ runner_bin_path.touch() harness = Harness(GithubRunnerCharm) - harness.update_config({"path": "mock/repo", "token": "mocktoken"}) + harness.update_config({PATH_CONFIG_NAME: "mock/repo", TOKEN_CONFIG_NAME: "mocktoken"}) harness.begin() monkeypatch.setattr("runner_manager.RunnerManager.update_runner_bin", stub_update_runner_bin) monkeypatch.setattr("runner_manager.RunnerManager._runners_in_pre_job", lambda self: False) @@ -121,7 +194,7 @@ def test_on_config_changed_failure(harness: Harness): act: Fire config changed event to use aproxy without configured http proxy. assert: Charm is in blocked state. """ - harness.update_config({"experimental-use-aproxy": True}) + harness.update_config({USE_APROXY_CONFIG_NAME: True}) assert isinstance(harness.charm.unit.status, BlockedStatus) assert "Invalid proxy configuration" in harness.charm.unit.status.message @@ -199,7 +272,6 @@ def test__refresh_firewall(monkeypatch, harness: Harness, runner_binary_path: Pa act: when refresh_firewall is called. assert: the unit ip addresses are included in allowlist. """ - runner_binary_path.touch() relation_id = harness.add_relation("debug-ssh", "tmate-ssh-server") @@ -253,6 +325,8 @@ def test__refresh_firewall(monkeypatch, harness: Harness, runner_binary_path: Pa # New test should be written with pytest, similar to the above tests. # Consider to rewrite test with pytest if the tests below needs to be changed. class TestCharm(unittest.TestCase): + """Test the GithubRunner charm.""" + @patch("charm.RunnerManager") @patch("pathlib.Path.mkdir") @patch("pathlib.Path.write_text") @@ -261,10 +335,10 @@ def test_org_register(self, run, wt, mkdir, rm): harness = Harness(GithubRunnerCharm) harness.update_config( { - "path": "mockorg", - "token": "mocktoken", - "group": "mockgroup", - "reconcile-interval": 5, + PATH_CONFIG_NAME: "mockorg", + TOKEN_CONFIG_NAME: "mocktoken", + GROUP_CONFIG_NAME: "mockgroup", + RECONCILE_INTERVAL_CONFIG_NAME: 5, } ) harness.begin() @@ -291,7 +365,11 @@ def test_org_register(self, run, wt, mkdir, rm): def test_repo_register(self, run, wt, mkdir, rm): harness = Harness(GithubRunnerCharm) harness.update_config( - {"path": "mockorg/repo", "token": "mocktoken", "reconcile-interval": 5} + { + PATH_CONFIG_NAME: "mockorg/repo", + TOKEN_CONFIG_NAME: "mocktoken", + RECONCILE_INTERVAL_CONFIG_NAME: 5, + } ) harness.begin() harness.charm.on.config_changed.emit() @@ -325,10 +403,10 @@ def test_exceed_free_disk_size(self, run, wt, mkdir, rm): mock_rm.download_latest_runner_image = mock_download_latest_runner_image harness = Harness(GithubRunnerCharm) - harness.update_config({"path": "mockorg/repo", "token": "mocktoken"}) + harness.update_config({PATH_CONFIG_NAME: "mockorg/repo", TOKEN_CONFIG_NAME: "mocktoken"}) harness.begin() - harness.update_config({"virtual-machines": 10}) + harness.update_config({VIRTUAL_MACHINES_CONFIG_NAME: 10}) harness.charm.on.reconcile_runners.emit() assert harness.charm.unit.status == BlockedStatus( ( @@ -347,11 +425,11 @@ def test_update_config(self, run, wt, mkdir, rm): mock_rm.download_latest_runner_image = mock_download_latest_runner_image harness = Harness(GithubRunnerCharm) - harness.update_config({"path": "mockorg/repo", "token": "mocktoken"}) + harness.update_config({PATH_CONFIG_NAME: "mockorg/repo", TOKEN_CONFIG_NAME: "mocktoken"}) harness.begin() # update to 0 virtual machines - harness.update_config({"virtual-machines": 0}) + harness.update_config({VIRTUAL_MACHINES_CONFIG_NAME: 0}) harness.charm.on.reconcile_runners.emit() token = harness.charm.service_token state = harness.charm._setup_state() @@ -371,7 +449,9 @@ def test_update_config(self, run, wt, mkdir, rm): mock_rm.reset_mock() # update to 10 VMs with 4 cpu and 7GiB memory - harness.update_config({"virtual-machines": 5, "vm-cpu": 4, "vm-disk": "6GiB"}) + harness.update_config( + {VIRTUAL_MACHINES_CONFIG_NAME: 5, VM_CPU_CONFIG_NAME: 4, VM_DISK_CONFIG_NAME: "6GiB"} + ) harness.charm.on.reconcile_runners.emit() token = harness.charm.service_token state = harness.charm._setup_state() @@ -398,10 +478,10 @@ def test_update_config(self, run, wt, mkdir, rm): @patch("subprocess.run") def test_on_update_status(self, run, wt, mkdir, rm): """ - arrange: reconciliation event timer mocked to be - 1. active - 2. inactive - 3. inactive with error thrown for ensure_event_timer + arrange: reconciliation event timer mocked to be \ + 1. active. \ + 2. inactive. \ + 3. inactive with error thrown for ensure_event_timer. act: Emit update_status assert: 1. ensure_event_timer is not called. @@ -414,7 +494,7 @@ def test_on_update_status(self, run, wt, mkdir, rm): harness = Harness(GithubRunnerCharm) - harness.update_config({"path": "mockorg/repo", "token": "mocktoken"}) + harness.update_config({PATH_CONFIG_NAME: "mockorg/repo", TOKEN_CONFIG_NAME: "mocktoken"}) harness.begin() event_timer_mock = MagicMock(spec=EventTimer) @@ -444,7 +524,7 @@ def test_on_update_status(self, run, wt, mkdir, rm): def test_on_stop(self, run, wt, mkdir, rm): rm.return_value = mock_rm = MagicMock() harness = Harness(GithubRunnerCharm) - harness.update_config({"path": "mockorg/repo", "token": "mocktoken"}) + harness.update_config({PATH_CONFIG_NAME: "mockorg/repo", TOKEN_CONFIG_NAME: "mocktoken"}) harness.begin() harness.charm.on.stop.emit() mock_rm.flush.assert_called() @@ -459,7 +539,7 @@ def test_on_start_failure(self, run, wt, mkdir, rm): mock_rm.get_latest_runner_bin_url = mock_get_latest_runner_bin_url harness = Harness(GithubRunnerCharm) - harness.update_config({"path": "mockorg/repo", "token": "mocktoken"}) + harness.update_config({PATH_CONFIG_NAME: "mockorg/repo", TOKEN_CONFIG_NAME: "mocktoken"}) harness.begin() harness.charm._reconcile_runners = raise_runner_error @@ -495,8 +575,8 @@ def test_on_config_changed_openstack_clouds_yaml(self, run, wt, mkdir, rm): } harness.update_config( { - "path": "mockorg/repo", - "token": "mocktoken", + PATH_CONFIG_NAME: "mockorg/repo", + TOKEN_CONFIG_NAME: "mocktoken", OPENSTACK_CLOUDS_YAML_CONFIG_NAME: yaml.safe_dump(cloud_yaml), } ) @@ -521,7 +601,7 @@ def test_check_runners_action(self, run, wt, mkdir, rm): mock_rm.get_github_info = mock_get_github_info harness = Harness(GithubRunnerCharm) - harness.update_config({"path": "mockorg/repo", "token": "mocktoken"}) + harness.update_config({PATH_CONFIG_NAME: "mockorg/repo", TOKEN_CONFIG_NAME: "mocktoken"}) harness.begin() harness.charm._on_check_runners_action(mock_event) @@ -541,7 +621,7 @@ def test_check_runners_action_with_errors(self, run, wt, mkdir, rm): # No config harness.charm._on_check_runners_action(mock_event) - mock_event.fail.assert_called_with("Missing path configuration") + mock_event.fail.assert_called_with("Invalid Github config, Missing path configuration") @patch("charm.RunnerManager") @patch("pathlib.Path.mkdir") @@ -554,10 +634,10 @@ def test_on_flush_runners_action(self, run, wt, mkdir, rm): harness.begin() harness.charm._on_flush_runners_action(mock_event) - mock_event.fail.assert_called_with("Missing path configuration") + mock_event.fail.assert_called_with("Invalid Github config, Missing path configuration") mock_event.reset_mock() - harness.update_config({"path": "mockorg/repo", "token": "mocktoken"}) + harness.update_config({PATH_CONFIG_NAME: "mockorg/repo", TOKEN_CONFIG_NAME: "mocktoken"}) harness.charm._on_flush_runners_action(mock_event) mock_event.set_results.assert_called() mock_event.reset_mock() diff --git a/tests/unit/test_charm_state.py b/tests/unit/test_charm_state.py index 77807366c..c94280202 100644 --- a/tests/unit/test_charm_state.py +++ b/tests/unit/test_charm_state.py @@ -14,6 +14,7 @@ from charm_state import ( COS_AGENT_INTEGRATION_NAME, DEBUG_SSH_INTEGRATION_NAME, + USE_APROXY_CONFIG_NAME, Arch, CharmConfigInvalidError, CharmState, @@ -83,7 +84,7 @@ def test_aproxy_proxy_missing(): assert: CharmConfigInvalidError is raised. """ mock_charm = MockGithubRunnerCharmFactory() - mock_charm.config["experimental-use-aproxy"] = "true" + mock_charm.config[USE_APROXY_CONFIG_NAME] = "true" with pytest.raises(CharmConfigInvalidError) as exc: CharmState.from_charm(mock_charm) @@ -107,9 +108,9 @@ def test_proxy_invalid_format(): def test_proxy_config_bool(): """ - arrange: Various combinations for ProxyConfig - act: Create ProxyConfig object - assert: Expected boolean value + arrange: Various combinations for ProxyConfig. + act: Create ProxyConfig object. + assert: Expected boolean value. """ proxy_url = "http://proxy.example.com:8080" diff --git a/tests/unit/test_event_timer.py b/tests/unit/test_event_timer.py index 386dabcab..818d64973 100644 --- a/tests/unit/test_event_timer.py +++ b/tests/unit/test_event_timer.py @@ -7,9 +7,9 @@ def test_is_active_true(): """ - arrange: create an EventTimer object and mock exec command to return zero exit code - act: call is_active - assert: return True + arrange: create an EventTimer object and mock exec command to return zero exit code. + act: call is_active. + assert: return True. """ event = EventTimer(unit_name=secrets.token_hex(16)) assert event.is_active(event_name=secrets.token_hex(16)) @@ -17,9 +17,9 @@ def test_is_active_true(): def test_is_active_false(exec_command): """ - arrange: create an EventTimer object and mock exec command to return non-zero exit code - act: call is_active - assert: return False + arrange: create an EventTimer object and mock exec command to return non-zero exit code. + act: call is_active. + assert: return False. """ exec_command.return_value = ("", 1) diff --git a/tests/unit/test_github_client.py b/tests/unit/test_github_client.py index e0d0ec7d6..a1ce538f5 100644 --- a/tests/unit/test_github_client.py +++ b/tests/unit/test_github_client.py @@ -24,7 +24,6 @@ @pytest.fixture(name="job_stats_raw") def job_stats_fixture() -> JobStatsRawData: """Create a JobStats object.""" - runner_name = secrets.token_hex(16) return JobStatsRawData( created_at="2021-10-01T00:00:00Z", @@ -88,8 +87,8 @@ def _mock_multiple_pages_for_job_response( def test_get_job_info(github_client: GithubClient, job_stats_raw: JobStatsRawData): """ - arrange: A mocked Github Client that returns one page of jobs containing one job - with the runner. + arrange: A mocked Github Client that returns one page of jobs containing one job \ + with the runner. act: Call get_job_info. assert: The correct JobStats object is returned. """ @@ -109,8 +108,8 @@ def test_get_job_info(github_client: GithubClient, job_stats_raw: JobStatsRawDat def test_get_job_info_no_conclusion(github_client: GithubClient, job_stats_raw: JobStatsRawData): """ - arrange: A mocked Github Client that returns one page of jobs containing one job - with the runner with conclusion set to None. + arrange: A mocked Github Client that returns one page of jobs containing one job \ + with the runner with conclusion set to None. act: Call get_job_info. assert: JobStats object with conclusion set to None is returned. """ @@ -142,8 +141,8 @@ def test_github_api_pagination_multiple_pages( github_client: GithubClient, job_stats_raw: JobStatsRawData ): """ - arrange: A mocked Github Client that returns multiple pages of jobs containing - one job with the runner. + arrange: A mocked Github Client that returns multiple pages of jobs containing \ + one job with the runner. act: Call get_job_info. assert: The correct JobStats object is returned. """ @@ -169,8 +168,8 @@ def test_github_api_pagination_job_not_found( github_client: GithubClient, job_stats_raw: JobStatsRawData ): """ - arrange: A mocked Github Client that returns multiple pages of jobs containing - no job with the runner. + arrange: A mocked Github Client that returns multiple pages of jobs containing \ + no job with the runner. act: Call get_job_info. assert: An exception is raised. """ diff --git a/tests/unit/test_github_metrics.py b/tests/unit/test_github_metrics.py index f79c940c0..4b6f2cd81 100644 --- a/tests/unit/test_github_metrics.py +++ b/tests/unit/test_github_metrics.py @@ -6,8 +6,8 @@ import pytest -import errors import github_metrics +from errors import GithubMetricsError, JobNotFoundError from github_client import GithubClient from github_type import JobConclusion, JobStats from runner_metrics import PreJobMetrics @@ -33,7 +33,6 @@ def test_job(pre_job_metrics: PreJobMetrics): act: Call job. assert: the job metrics are returned. """ - github_client = MagicMock(spec=GithubClient) runner_name = secrets.token_hex(16) created_at = datetime(2021, 10, 1, 0, 0, 0, tzinfo=timezone.utc) @@ -61,9 +60,9 @@ def test_job_job_not_found(pre_job_metrics: PreJobMetrics): """ github_client = MagicMock(spec=GithubClient) runner_name = secrets.token_hex(16) - github_client.get_job_info.side_effect = errors.JobNotFoundError("Job not found") + github_client.get_job_info.side_effect = JobNotFoundError("Job not found") - with pytest.raises(errors.GithubMetricsError): + with pytest.raises(GithubMetricsError): github_metrics.job( github_client=github_client, pre_job_metrics=pre_job_metrics, runner_name=runner_name ) diff --git a/tests/unit/test_metrics.py b/tests/unit/test_metrics.py index ebe23433b..561507437 100644 --- a/tests/unit/test_metrics.py +++ b/tests/unit/test_metrics.py @@ -14,9 +14,9 @@ def test_issue_metrics_logs_events(tmp_path: Path): """ - arrange: Change path of the metrics log - act: Issue a metric event - assert: The expected metric log is created + arrange: Change path of the metrics log. + act: Issue a metric event. + assert: The expected metric log is created. """ event = metrics.RunnerInstalled(timestamp=123, flavor="small", duration=456) @@ -32,9 +32,9 @@ def test_issue_metrics_logs_events(tmp_path: Path): def test_issue_metrics_exclude_none_values(tmp_path: Path): """ - arrange: Change path of the metrics log - act: Issue a metric event with a None value - assert: The expected metric log without the None value is created + arrange: Change path of the metrics log. + act: Issue a metric event with a None value. + assert: The expected metric log without the None value is created. """ event = metrics.RunnerStop( timestamp=123, @@ -63,11 +63,10 @@ def test_issue_metrics_exclude_none_values(tmp_path: Path): def test_setup_logrotate(tmp_path: Path): """ - arrange: Change paths for the logrotate config and the log file - act: Setup logrotate - assert: The expected logrotate config is created + arrange: Change paths for the logrotate config and the log file. + act: Setup logrotate. + assert: The expected logrotate config is created. """ - metrics.setup_logrotate() expected_logrotate_config = f"""{metrics.METRICS_LOG_PATH} {{ @@ -82,13 +81,22 @@ def test_setup_logrotate(tmp_path: Path): def test_setup_logrotate_enables_logrotate_timer(lxd_exec_command: MagicMock): """ - arrange: Mock execute command to return error for the is-active call and - non-error for the remaining calls. - act: Setup logrotate - assert: The commands to enable and start the logrotate timer are called + arrange: Mock execute command to return error for the is-active call and \ + non-error for the remaining calls. + act: Setup logrotate. + assert: The commands to enable and start the logrotate timer are called. """ def side_effect(*args, **kwargs): + """Mock side effect function that returns non-zero exit code. + + Args: + args: Placeholder for positional arguments for lxd exec command. + kwargs: Placeholder for keyword arguments for lxd exec command. + + Returns: + A tuple of return value and exit code. + """ if "is-active" in args[0]: return "", 1 return "", 0 @@ -109,8 +117,8 @@ def side_effect(*args, **kwargs): def test_setup_logrotate_raises_error(lxd_exec_command: MagicMock): """ - arrange: Mock execute command to raise a SubprocessError - act: Setup logrotate + arrange: Mock execute command to raise a SubprocessError. + act: Setup logrotate. assert: The expected error is raised. """ lxd_exec_command.side_effect = SubprocessError( diff --git a/tests/unit/test_openstack_cloud.py b/tests/unit/test_openstack_cloud.py index e3b9870ca..4f599e914 100644 --- a/tests/unit/test_openstack_cloud.py +++ b/tests/unit/test_openstack_cloud.py @@ -36,7 +36,6 @@ def test_initialize_validation_error(invalid_yaml: dict, expected_err_msg): act: Call initialize. assert: InvalidConfigError is raised. """ - with pytest.raises(OpenStackInvalidConfigError) as exc: openstack_cloud.initialize(invalid_yaml) assert expected_err_msg in str(exc) diff --git a/tests/unit/test_openstack_manager.py b/tests/unit/test_openstack_manager.py index 88f26b436..2a36e1f3d 100644 --- a/tests/unit/test_openstack_manager.py +++ b/tests/unit/test_openstack_manager.py @@ -417,7 +417,7 @@ def test_build_image_delete_image_error( mock_github_client: MagicMock, patched_create_connection_context: MagicMock ): """ - arrange: given a mocked openstack connection that returns existing images and delete_image + arrange: given a mocked openstack connection that returns existing images and delete_image \ that returns False (failed to delete image). act: when build_image is called. assert: ImageBuildError is raised. @@ -460,7 +460,7 @@ def test_build_image_create_image_error( proxies=None, ) - assert "Failed to upload image." in str(exc) + assert "Failed to upload image" in str(exc) @pytest.mark.usefixtures("patch_execute_command") diff --git a/tests/unit/test_runner.py b/tests/unit/test_runner.py index 2c96bc8e8..96b2b507a 100644 --- a/tests/unit/test_runner.py +++ b/tests/unit/test_runner.py @@ -164,7 +164,6 @@ def test_create( act: Create a runner. assert: An lxd instance for the runner is created. """ - runner.create( config=CreateRunnerConfig( image="test_image", @@ -195,6 +194,7 @@ def test_create_lxd_fail( token: str, binary_path: Path, lxd: MockLxdClient, + monkeypatch: pytest.MonkeyPatch, ): """ arrange: Setup the create runner to fail with lxd error. @@ -202,7 +202,7 @@ def test_create_lxd_fail( assert: Correct exception should be thrown. Any created instance should be cleanup. """ - lxd.profiles.exists = mock_lxd_error_func + monkeypatch.setattr(lxd.profiles, "exists", mock_lxd_error_func) with pytest.raises(RunnerCreateError): runner.create( @@ -259,7 +259,6 @@ def test_create_with_metrics( assert: The command for adding a device has been executed and the templates are rendered to issue metrics. """ - runner.config.issue_metrics = True shared_fs.create.return_value = SharedFilesystem( path=Path("/home/ubuntu/shared_fs"), runner_name="test_runner" @@ -303,8 +302,8 @@ def test_create_with_metrics_and_shared_fs_error( shared_fs: MagicMock, ): """ - arrange: Config the runner to issue metrics and mock the shared filesystem module - to throw an expected error. + arrange: Config the runner to issue metrics and mock the shared filesystem module\ + to throw an expected error. act: Create a runner. assert: The runner is created despite the error on the shared filesystem. """ @@ -336,7 +335,6 @@ def test_remove( act: Remove the runner. assert: The lxd instance for the runner is removed. """ - runner.create( config=CreateRunnerConfig( image="test_image", @@ -386,7 +384,6 @@ def test_remove_none( act: Remove the runner. assert: The lxd instance for the runner is removed. """ - runner.remove(token) assert len(lxd.instances.all()) == 0 diff --git a/tests/unit/test_runner_logs.py b/tests/unit/test_runner_logs.py index ce87d0de4..1a815fdbf 100644 --- a/tests/unit/test_runner_logs.py +++ b/tests/unit/test_runner_logs.py @@ -20,9 +20,9 @@ def log_dir_base_path_fixture(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) - def test_get_crashed(log_dir_base_path: Path): """ - arrange: Mock the Runner instance and the base log directory path - act: Get the logs of the crashed runner - assert: The expected log directory is created and logs are pulled + arrange: Mock the Runner instance and the base log directory path. + act: Get the logs of the crashed runner. + assert: The expected log directory is created and logs are pulled. """ runner = MagicMock() runner.config.name = "test-runner" @@ -45,9 +45,9 @@ def test_get_crashed(log_dir_base_path: Path): def test_get_crashed_no_instance(log_dir_base_path: Path): """ - arrange: Mock the Runner instance to be None - act: Get the logs of the crashed runner - assert: A RunnerLogsError is raised + arrange: Mock the Runner instance to be None. + act: Get the logs of the crashed runner. + assert: A RunnerLogsError is raised. """ runner = MagicMock() runner.config.name = "test-runner" @@ -63,9 +63,9 @@ def test_get_crashed_no_instance(log_dir_base_path: Path): def test_get_crashed_lxd_error(log_dir_base_path: Path): """ - arrange: Mock the Runner instance to raise an LxdError - act: Get the logs of the crashed runner - assert: A RunnerLogsError is raised + arrange: Mock the Runner instance to raise an LxdError. + act: Get the logs of the crashed runner. + assert: A RunnerLogsError is raised. """ runner = MagicMock() runner.config.name = "test-runner" @@ -80,9 +80,9 @@ def test_get_crashed_lxd_error(log_dir_base_path: Path): def test_remove_outdated_crashed(log_dir_base_path: Path, monkeypatch: pytest.MonkeyPatch): """ - arrange: Mock the base log directory path - act: Remove the logs of the crashed runner - assert: The expected logs are removed + arrange: Mock the base log directory path. + act: Remove the logs of the crashed runner. + assert: The expected logs are removed. """ monkeypatch.setattr(runner_logs, "OUTDATED_LOGS_IN_SECONDS", 0) diff --git a/tests/unit/test_runner_manager.py b/tests/unit/test_runner_manager.py index ea6620ae6..d51a4e995 100644 --- a/tests/unit/test_runner_manager.py +++ b/tests/unit/test_runner_manager.py @@ -174,10 +174,27 @@ def test_update_runner_bin(runner_manager: RunnerManager): """ class MockRequestLibResponse: - def __init__(self, *arg, **kargs): + """A mock requests library response.""" + + def __init__(self, *args, **kwargs): + """Initialize successful requests library response. + + Args: + args: Placeholder for positional arguments. + kwargs: Placeholder for keyword arguments. + """ self.status_code = 200 - def iter_content(self, *arg, **kargs): + def iter_content(self, *args, **kwargs): + """Mock content iterator returning an iterator over a single test runner binary. + + Args: + args: Placeholder positional arguments. + kwargs: Placeholder keyword arguments. + + Returns: + An iterator over a single test runner binary. + """ return iter([TEST_BINARY]) runner_manager.runner_bin_path.unlink(missing_ok=True) @@ -222,7 +239,11 @@ def test_reconcile_remove_runner(runner_manager: RunnerManager): """ def mock_get_runners(): - """Create three mock runners.""" + """Create three mock runners. + + Returns: + Three mock runners. + """ runners = [] for _ in range(3): # 0 is a mock runner id. @@ -346,11 +367,11 @@ def test_reconcile_issues_reconciliation_metric_event( charm_state: MagicMock, ): """ - arrange: - - Enable issuing of metrics - - Mock timestamps - - Mock the result of runner_metrics.issue_event to contain 2 RunnerStart and 1 RunnerStop - events, meaning one runner was active and one crashed. + arrange: \ + - Enable issuing of metrics \ + - Mock timestamps \ + - Mock the result of runner_metrics.issue_event to contain 2 RunnerStart and 1 RunnerStop \ + events, meaning one runner was active and one crashed. \ - Create two online runners , one active and one idle. act: Reconcile. assert: The expected event is issued. We expect two idle runners and one crashed runner @@ -367,7 +388,11 @@ def test_reconcile_issues_reconciliation_metric_event( active_runner_name = f"{runner_manager.instance_name}-2" def mock_get_runners(): - """Create three mock runners where one is busy.""" + """Create three mock runners where one is busy. + + Returns: + Mock runners with one busy runner. + """ runners = [] online_idle_runner = RunnerStatus(runner_id=0, exist=True, online=True, busy=False) @@ -425,8 +450,8 @@ def test_reconcile_places_timestamp_in_newly_created_runner( charm_state: MagicMock, ): """ - arrange: Enable issuing of metrics, mock timestamps and - create the directory for the shared filesystem. + arrange: Enable issuing of metrics, mock timestamps and create the directory for the shared\ + filesystem. act: Reconcile to create a runner. assert: The expected timestamp is placed in the shared filesystem. """ @@ -448,7 +473,7 @@ def test_reconcile_error_on_placing_timestamp_is_ignored( runner_manager: RunnerManager, shared_fs: MagicMock, tmp_path: Path, charm_state: MagicMock ): """ - arrange: Enable issuing of metrics and do not create the directory for the shared filesystem + arrange: Enable issuing of metrics and do not create the directory for the shared filesystem\ in order to let a FileNotFoundError to be raised inside the RunnerManager. act: Reconcile to create a runner. assert: No exception is raised. diff --git a/tests/unit/test_runner_metrics.py b/tests/unit/test_runner_metrics.py index bb71eab8e..4e906ca13 100644 --- a/tests/unit/test_runner_metrics.py +++ b/tests/unit/test_runner_metrics.py @@ -7,11 +7,11 @@ import pytest -import errors import metrics import metrics_type import runner_metrics import shared_fs +from errors import DeleteSharedFilesystemError, IssueMetricEventError from github_type import JobConclusion from metrics import RunnerStart, RunnerStop from runner_metrics import ( @@ -47,7 +47,14 @@ def runner_fs_base_fixture(tmp_path: Path) -> Path: def _create_metrics_data(runner_name: str) -> RunnerMetrics: - """Create a RunnerMetrics object that is suitable for most tests.""" + """Create a RunnerMetrics object that is suitable for most tests. + + Args: + runner_name: The test runner name. + + Returns: + Test metrics data. + """ return RunnerMetrics( installed_timestamp=1, pre_job=PreJobMetrics( @@ -63,7 +70,14 @@ def _create_metrics_data(runner_name: str) -> RunnerMetrics: def _create_runner_fs_base(tmp_path: Path): - """Create a runner filesystem base.""" + """Create a runner filesystem base. + + Args: + tmp_path: The temporary path to create test runner filesystem under. + + Returns: + The runner filesystem temporary path. + """ runner_fs_base = tmp_path / "runner-fs" runner_fs_base.mkdir(exist_ok=True) return runner_fs_base @@ -83,9 +97,13 @@ def _create_runner_files( Args: runner_fs_base: The base path of the shared fs. + runner_name: The runner name. pre_job_data: The pre-job metrics data. post_job_data: The post-job metrics data. installed_timestamp: The installed timestamp. + + Returns: + A SharedFilesystem instance. """ runner_fs = runner_fs_base / runner_name runner_fs.mkdir() @@ -119,10 +137,10 @@ def _create_runner_files( def test_extract(shared_fs_mock: MagicMock, runner_fs_base: Path): """ - arrange: - 1. A runner with all metrics inside shared fs - 2. A runner with only pre-job metrics inside shared fs - 3. A runner with no metrics except installed_timestamp inside shared fs + arrange: \ + 1. A runner with all metrics inside shared fs. \ + 2. A runner with only pre-job metrics inside shared fs. \ + 3. A runner with no metrics except installed_timestamp inside shared fs. act: Call extract assert: All shared filesystems are removed and for runners 1. + 2. metrics are extracted @@ -207,12 +225,12 @@ def test_extract_ignores_runners(shared_fs_mock: MagicMock, runner_fs_base: Path def test_extract_corrupt_data(runner_fs_base: Path, shared_fs_mock: MagicMock): """ - arrange: - 1. A runner with non-compliant pre-job metrics inside shared fs - 2. A runner with non-json post-job metrics inside shared fs - 3. A runner with json array post-job metrics inside shared fs - 4. A runner with no real timestamp in installed_timestamp file inside shared fs - act: Call extract + arrange: \ + 1. A runner with non-compliant pre-job metrics inside shared fs. \ + 2. A runner with non-json post-job metrics inside shared fs. \ + 3. A runner with json array post-job metrics inside shared fs. \ + 4. A runner with no real timestamp in installed_timestamp file inside shared fs. + act: Call extract. assert: No metrics are extracted is issued and shared filesystems are quarantined in all cases. """ runner_name = secrets.token_hex(16) @@ -411,7 +429,7 @@ def test_extract_ignores_failure_on_shared_fs_cleanup( ) shared_fs_mock.list_all.return_value = [runner_fs] - shared_fs_mock.delete.side_effect = errors.DeleteSharedFilesystemError( + shared_fs_mock.delete.side_effect = DeleteSharedFilesystemError( "Failed to delete shared filesystem" ) @@ -513,7 +531,7 @@ def test_issue_events_returns_empty_set_on_issue_event_failure( runner_name = secrets.token_hex(16) runner_metrics_data = _create_metrics_data(runner_name) - issue_event_mock.side_effect = [errors.IssueMetricEventError("Failed to issue metric"), None] + issue_event_mock.side_effect = [IssueMetricEventError("Failed to issue metric"), None] flavor = secrets.token_hex(16) job_metrics = metrics_type.GithubJobMetrics( diff --git a/tests/unit/test_shared_fs.py b/tests/unit/test_shared_fs.py index 13124b79d..7f8c2edb8 100644 --- a/tests/unit/test_shared_fs.py +++ b/tests/unit/test_shared_fs.py @@ -8,18 +8,21 @@ import pytest from _pytest.monkeypatch import MonkeyPatch -import errors import shared_fs -from errors import SubprocessError +from errors import ( + CreateSharedFilesystemError, + DeleteSharedFilesystemError, + GetSharedFilesystemError, + QuarantineSharedFilesystemError, + SubprocessError, +) MOUNTPOINT_FAILURE_EXIT_CODE = 1 @pytest.fixture(autouse=True, name="filesystem_paths") def filesystem_paths_fixture(monkeypatch: MonkeyPatch, tmp_path: Path) -> dict[str, Path]: - """ - Mock the hardcoded filesystem paths. - """ + """Mock the hardcoded filesystem paths.""" fs_path = tmp_path / "runner-fs" fs_images_path = tmp_path / "images" fs_quarantine_path = tmp_path / "quarantine" @@ -37,6 +40,20 @@ def exc_command_fixture(monkeypatch: MonkeyPatch) -> Mock: return exc_cmd_mock +def exc_cmd_side_effect(*args, **_): + """Mock command to return NOT_A_MOUNTPOINT exit code. + + Args: + args: Positional argument placeholder. + + Returns: + Fake exc_cmd return values. + """ + if args[0][0] == "mountpoint": + return "", shared_fs.DIR_NO_MOUNTPOINT_EXIT_CODE + return "", 0 + + def test_create_creates_directory(): """ arrange: Given a runner name and a path for the filesystems. @@ -62,7 +79,7 @@ def test_create_raises_exception(exc_cmd_mock: MagicMock): cmd=["mock"], return_code=1, stdout="mock stdout", stderr="mock stderr" ) - with pytest.raises(errors.CreateSharedFilesystemError): + with pytest.raises(CreateSharedFilesystemError): shared_fs.create(runner_name) @@ -75,7 +92,7 @@ def test_create_raises_exception_if_already_exists(): runner_name = secrets.token_hex(16) shared_fs.create(runner_name) - with pytest.raises(errors.CreateSharedFilesystemError): + with pytest.raises(CreateSharedFilesystemError): shared_fs.create(runner_name) @@ -110,8 +127,8 @@ def test_list_shared_filesystems_empty(): def test_list_shared_filesystems_ignore_unmounted_fs(exc_cmd_mock: MagicMock): """ - arrange: Create shared filesystems for multiple runners and mock mountpoint cmd - to return NOT_A_MOUNTPOINT exit code for a dedicated runner. + arrange: Create shared filesystems for multiple runners and mock mountpoint cmd \ + to return NOT_A_MOUNTPOINT exit code for a dedicated runner. act: Call list. assert: A generator listing all the shared filesystems except the one of the dedicated runner is returned. @@ -123,6 +140,14 @@ def test_list_shared_filesystems_ignore_unmounted_fs(exc_cmd_mock: MagicMock): runner_with_mount_failure = runner_names[0] def exc_cmd_side_effect(*args, **_): + """Mock command to return NOT_A_MOUNTPOINT exit code. + + Args: + args: Positional argument placeholder. + + Returns: + Fake exc_cmd return values. + """ if args[0][0] == "mountpoint" and runner_with_mount_failure in args[0][2]: return "", MOUNTPOINT_FAILURE_EXIT_CODE return "", 0 @@ -146,7 +171,7 @@ def test_delete_filesystem(): shared_fs.delete(runner_name) - with pytest.raises(errors.GetSharedFilesystemError): + with pytest.raises(GetSharedFilesystemError): shared_fs.get(runner_name) @@ -158,30 +183,25 @@ def test_delete_raises_error(): """ runner_name = secrets.token_hex(16) - with pytest.raises(errors.DeleteSharedFilesystemError): + with pytest.raises(DeleteSharedFilesystemError): shared_fs.delete(runner_name) def test_delete_filesystem_ignores_unmounted_filesystem(exc_cmd_mock: MagicMock): """ - arrange: Create a shared filesystem for a runner and mock mountpoint cmd - to return NOT_A_MOUNTPOINT exit code. + arrange: Create a shared filesystem for a runner and mock mountpoint cmd \ + to return NOT_A_MOUNTPOINT exit code. act: Call delete. assert: The shared filesystem is deleted. """ runner_name = secrets.token_hex(16) shared_fs.create(runner_name) - def exc_cmd_side_effect(*args, **_): - if args[0][0] == "mountpoint": - return "", shared_fs.DIR_NO_MOUNTPOINT_EXIT_CODE - return "", 0 - exc_cmd_mock.side_effect = exc_cmd_side_effect shared_fs.delete(runner_name) - with pytest.raises(errors.GetSharedFilesystemError): + with pytest.raises(GetSharedFilesystemError): shared_fs.get(runner_name) @@ -208,25 +228,20 @@ def test_get_raises_error_if_not_found(): """ runner_name = secrets.token_hex(16) - with pytest.raises(errors.GetSharedFilesystemError): + with pytest.raises(GetSharedFilesystemError): shared_fs.get(runner_name) def test_get_mounts_if_unmounted(exc_cmd_mock: MagicMock): """ - arrange: Given a runner name and a mock mountpoint cmd - which returns NOT_A_MOUNTPOINT exit code. + arrange: Given a runner name and a mock mountpoint cmd which returns NOT_A_MOUNTPOINT \ + exit code. act: Call create and get. assert: The shared filesystem is mounted. """ runner_name = secrets.token_hex(16) shared_fs.create(runner_name) - def exc_cmd_side_effect(*args, **_): - if args[0][0] == "mountpoint": - return "", shared_fs.DIR_NO_MOUNTPOINT_EXIT_CODE - return "", 0 - exc_cmd_mock.side_effect = exc_cmd_side_effect shared_fs.get(runner_name) @@ -272,5 +287,5 @@ def test_quarantine_raises_error(): """ runner_name = secrets.token_hex(16) - with pytest.raises(errors.QuarantineSharedFilesystemError): + with pytest.raises(QuarantineSharedFilesystemError): shared_fs.move_to_quarantine(runner_name) diff --git a/tests/unit/test_utilities.py b/tests/unit/test_utilities.py index 900a5ee68..d0bc8bf57 100644 --- a/tests/unit/test_utilities.py +++ b/tests/unit/test_utilities.py @@ -19,7 +19,16 @@ def test_execute_command_with_error(monkeypatch): assert: Throw related to subprocess thrown. """ - def raise_called_process_error(*args, **kargs): + def raise_called_process_error(*args, **kwargs): + """Raise CalledProcessError exception. + + Args: + args: Any positional arguments. + kwargs: Any keyword arguments. + + Raises: + CalledProcessError: when called. + """ raise CalledProcessError(returncode=1, cmd="mock cmd", stderr="mock stderr") mock_run = MagicMock()