From 01bd849f024bcae57a1edab270772a32664fa79e Mon Sep 17 00:00:00 2001 From: Ali Kelkawi <81743070+kelkawi-a@users.noreply.github.com> Date: Fri, 19 Jul 2024 11:47:59 +0300 Subject: [PATCH] Address charm listing review comments (#16) * address charm listing review comments --- .github/workflows/integration_test.yaml | 6 +- CONTRIBUTING.md | 2 +- README.md | 9 ++- charmcraft.yaml | 10 +++- src/charm.py | 18 +++--- src/literals.py | 4 +- src/relations/minio.py | 3 +- tests/conftest.py | 2 +- tests/integration/conftest.py | 3 +- tests/integration/helpers.py | 39 +++++++++++-- .../integration/temporal_client/workflows.py | 2 - tests/integration/test_charm.py | 2 - tests/unit/test_charm.py | 57 ++++++++++++------- tests/unit/test_state.py | 4 +- tests/unit/test_structured_config.py | 8 +-- tox.ini | 9 ++- 16 files changed, 119 insertions(+), 59 deletions(-) diff --git a/.github/workflows/integration_test.yaml b/.github/workflows/integration_test.yaml index ba40eec..638bb97 100644 --- a/.github/workflows/integration_test.yaml +++ b/.github/workflows/integration_test.yaml @@ -1,5 +1,9 @@ name: Integration tests +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + on: pull_request: @@ -10,7 +14,7 @@ jobs: with: channel: 1.28-strict/stable modules: '["test_charm.py"]' - juju-channel: 3.1/stable + juju-channel: 3.4/stable self-hosted-runner: true self-hosted-runner-label: "xlarge" microk8s-addons: "dns ingress rbac storage metallb:10.15.119.2-10.15.119.4 registry" diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index b01ab27..919419d 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -41,7 +41,7 @@ workflows are as follows: library checks which run on every pull request. - `integration_test.yaml`: This runs the suite of integration tests included with the charm and runs on every pull request. -- `test_and_publish_charm.yaml`: This runs either by manual dispatch or on every +- `publish_charm.yaml`: This runs either by manual dispatch or on every push to the main branch or a special track/\*\* branch. Once a PR is merged with one of these branches, this workflow runs to ensure the tests have passed before building the charm and publishing the new version to the edge channel diff --git a/README.md b/README.md index 9ca35ca..56a9b41 100644 --- a/README.md +++ b/README.md @@ -1,3 +1,6 @@ +[![Charmhub Badge](https://charmhub.io/airbyte-k8s/badge.svg)](https://charmhub.io/airbyte-k8s) +[![Release Edge](https://github.com/canonical/airbyte-k8s-operator/actions/workflows/publish_charm.yaml/badge.svg)](https://github.com/canonical/airbyte-k8s-operator/actions/workflows/publish_charm.yaml) + # Airbyte K8s Operator This is the Kubernetes Python Operator for [Airbyte](https://airbyte.com/). @@ -39,6 +42,10 @@ juju deploy postgresql-k8s --channel 14/edge --trust juju relate airbyte-k8s postgresql-k8s ``` +Note: The `--trust` is required when deploying charmed Airbyte k8s to enable it +to create k8s pods for sync jobs. The charm contains a script which periodically +cleans up these resources once they complete their function. + ### Deploying Minio Airbyte uses Minio for storing state and relevant logs. The Airbyte and Minio @@ -57,7 +64,7 @@ The Temporal operators can be deployed and connected to each other using the Juju command line as follows: ```bash -juju deploy temporal-k8s +juju deploy temporal-k8s --config num-history-shards=512 juju deploy temporal-admin-k8s juju relate temporal-k8s:db postgresql-k8s:database juju relate temporal-k8s:visibility postgresql-k8s:database diff --git a/charmcraft.yaml b/charmcraft.yaml index 42d968c..e20d97e 100644 --- a/charmcraft.yaml +++ b/charmcraft.yaml @@ -15,6 +15,10 @@ description: | links: documentation: https://discourse.charmhub.io/t/charmed-airbyte-k8s-overview/14530 + source: + - https://github.com/canonical/airbyte-k8s-operator + issues: + - https://github.com/canonical/airbyte-k8s-operator/issues # (Required for 'charm' type) bases: @@ -25,10 +29,13 @@ bases: - name: ubuntu channel: "22.04" +assumes: + - juju >= 3.1 + - k8s-api # Metadata peers: - peer: + airbyte-peer: interface: airbyte requires: @@ -38,6 +45,7 @@ requires: object-storage: interface: object-storage + limit: 1 schema: v1: provides: diff --git a/src/charm.py b/src/charm.py index 075155a..80e2412 100755 --- a/src/charm.py +++ b/src/charm.py @@ -21,7 +21,7 @@ AIRBYTE_VERSION, BUCKET_CONFIGS, CONNECTOR_BUILDER_SERVER_API_PORT, - CONTAINERS, + CONTAINER_HEALTH_CHECK_MAP, INTERNAL_API_PORT, LOGS_BUCKET_CONFIG, REQUIRED_S3_PARAMETERS, @@ -63,7 +63,7 @@ def get_pebble_layer(application_name, context): }, } - application_info = CONTAINERS[application_name] + application_info = CONTAINER_HEALTH_CHECK_MAP[application_name] if application_info is not None: pebble_layer["services"][application_name].update( { @@ -104,10 +104,10 @@ def __init__(self, *args): args: Ignore. """ super().__init__(*args) - self._state = State(self.app, lambda: self.model.get_relation("peer")) + self._state = State(self.app, lambda: self.model.get_relation("airbyte-peer")) self.framework.observe(self.on.config_changed, self._on_config_changed) - self.framework.observe(self.on.peer_relation_changed, self._on_peer_relation_changed) + self.framework.observe(self.on.airbyte_peer_relation_changed, self._on_peer_relation_changed) self.framework.observe(self.on.update_status, self._on_update_status) # Handle postgresql relation. @@ -123,7 +123,7 @@ def __init__(self, *args): # Handle UI relation self.airbyte_ui = AirbyteServerProvider(self) - for container_name in list(CONTAINERS.keys()): + for container_name in CONTAINER_HEALTH_CHECK_MAP: self.framework.observe(self.on[container_name].pebble_ready, self._on_pebble_ready) @log_event_handler(logger) @@ -157,8 +157,8 @@ def _on_update_status(self, event): return all_valid_plans = True - for container_name in list(CONTAINERS.keys()): - if not CONTAINERS[container_name]: + for container_name, settings in CONTAINER_HEALTH_CHECK_MAP.items(): + if not settings: continue container = self.unit.get_container(container_name) @@ -167,7 +167,6 @@ def _on_update_status(self, event): if not valid_pebble_plan: logger.debug(f"failed to validate pebble plan for {container_name}, attempting creation again") all_valid_plans = False - self._update(event) continue logger.info(f"performing up check for {container_name}") @@ -178,6 +177,7 @@ def _on_update_status(self, event): return if not all_valid_plans: + self._update(event) return self.unit.set_workload_version(f"v{AIRBYTE_VERSION}") @@ -287,7 +287,7 @@ def _update(self, event): self.model.unit.set_ports(AIRBYTE_API_PORT, INTERNAL_API_PORT, CONNECTOR_BUILDER_SERVER_API_PORT) - for container_name in list(CONTAINERS.keys()): + for container_name in CONTAINER_HEALTH_CHECK_MAP: container = self.unit.get_container(container_name) if not container.can_connect(): event.defer() diff --git a/src/literals.py b/src/literals.py index b296835..c6fedf3 100644 --- a/src/literals.py +++ b/src/literals.py @@ -1,14 +1,14 @@ # Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. -"""Literals.""" +"""Charm literals.""" CONNECTOR_BUILDER_SERVER_API_PORT = 80 INTERNAL_API_PORT = 8001 AIRBYTE_API_PORT = 8006 WORKLOAD_API_PORT = 8007 -CONTAINERS = { +CONTAINER_HEALTH_CHECK_MAP = { "airbyte-api-server": { "port": AIRBYTE_API_PORT, "health_endpoint": "/health", diff --git a/src/relations/minio.py b/src/relations/minio.py index 7196619..e54f6db 100644 --- a/src/relations/minio.py +++ b/src/relations/minio.py @@ -101,7 +101,8 @@ def _get_interfaces(self): charm = self.charm # Hack: get_interfaces checks for peer relation which does not exist under # requires/provides list in charmcraft.yaml - del charm.meta.relations["peer"] + if "airbyte-peer" in charm.meta.relations: + del charm.meta.relations["airbyte-peer"] interfaces = get_interfaces(charm) except NoVersionsListed as err: raise ErrorWithStatus(err, WaitingStatus) from err diff --git a/tests/conftest.py b/tests/conftest.py index 0998abd..04df5c6 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,7 +1,7 @@ # Copyright 2024 Canonical Ltd. # See LICENSE file for licensing details. -"""Fixtures for jenkins-k8s charm tests.""" +"""Fixtures for charm tests.""" import pytest diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index 44d7d2d..cf09a4d 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -3,7 +3,6 @@ """Charm integration test config.""" -import asyncio import logging import pytest_asyncio @@ -37,7 +36,7 @@ async def deploy(ops_test: OpsTest): config={"num-history-shards": 4}, ) await ops_test.model.deploy(APP_NAME_TEMPORAL_ADMIN, channel="edge") - await ops_test.model.deploy("postgresql-k8s", channel="14/edge", trust=True) + await ops_test.model.deploy("postgresql-k8s", channel="14/stable", trust=True) await ops_test.model.deploy("minio", channel="edge") async with ops_test.fast_forward(): diff --git a/tests/integration/helpers.py b/tests/integration/helpers.py index 6ad9eac..5e39cf2 100644 --- a/tests/integration/helpers.py +++ b/tests/integration/helpers.py @@ -29,6 +29,11 @@ def get_airbyte_charm_resources(): + """Fetch charm resources from charmcraft.yaml. + + Returns: + Charm resources. + """ return { "airbyte-api-server": METADATA["resources"]["airbyte-api-server"]["upstream-source"], "airbyte-bootloader": METADATA["resources"]["airbyte-bootloader"]["upstream-source"], @@ -153,6 +158,9 @@ def get_airbyte_workspace_id(api_url): Args: api_url: Airbyte API base URL. + + Returns: + Airbyte workspace ID. """ url = f"{api_url}/v1/workspaces?includeDeleted=false&limit=20&offset=0" logger.info("fetching Airbyte workspace ID") @@ -168,6 +176,9 @@ def create_airbyte_source(api_url, workspace_id): Args: api_url: Airbyte API base URL. workspace_id: default workspace ID. + + Returns: + Created source ID. """ url = f"{api_url}/v1/sources" payload = { @@ -190,7 +201,10 @@ def create_airbyte_destination(api_url, model_name, workspace_id, db_password): api_url: Airbyte API base URL. model_name: name of the juju model. workspace_id: default workspace ID. - password: database password. + db_password: database password. + + Returns: + Created destination ID. """ url = f"{api_url}/v1/destinations" payload = { @@ -223,6 +237,9 @@ def create_airbyte_connection(api_url, source_id, destination_id): api_url: Airbyte API base URL. source_id: Airbyte source ID. destination_id: Airbyte destination ID. + + Returns: + Created connection ID. """ url = f"{api_url}/v1/connections" payload = { @@ -248,6 +265,9 @@ def trigger_airbyte_connection(api_url, connection_id): Args: api_url: Airbyte API base URL. connection_id: Airbyte connection ID. + + Returns: + Created job ID. """ url = f"{api_url}/v1/jobs" payload = {"jobType": "sync", "connectionId": connection_id} @@ -264,6 +284,9 @@ def check_airbyte_job_status(api_url, job_id): Args: api_url: Airbyte API base URL. job_id: Sync job ID. + + Returns: + Job status. """ url = f"{api_url}/v1/jobs/{job_id}" logger.info("fetching Airbyte job status") @@ -279,6 +302,9 @@ def cancel_airbyte_job(api_url, job_id): Args: api_url: Airbyte API base URL. job_id: Sync job ID. + + Returns: + Job status. """ url = f"{api_url}/v1/jobs/{job_id}" logger.info("cancelling Airbyte job") @@ -293,6 +319,9 @@ async def get_db_password(ops_test): Args: ops_test: PyTest object. + + Returns: + PostgreSQL DB admin password. """ postgresql_unit = ops_test.model.applications["postgresql-k8s"].units[0] for i in range(10): @@ -328,24 +357,24 @@ async def run_test_sync_job(ops_test): # Trigger sync job for i in range(2): - logger.info(f"attempt {i+1} to trigger new job") + logger.info(f"attempt {i + 1} to trigger new job") job_id = trigger_airbyte_connection(api_url, connection_id) # Wait until job is successful job_successful = False for j in range(15): - logger.info(f"job {i+1} attempt {j+1}: getting job status") + logger.info(f"job {i + 1} attempt {j + 1}: getting job status") status = check_airbyte_job_status(api_url, job_id) if status == "failed": break if status == "succeeded": - logger.info(f"job {i+1} attempt {j+1}: job successful!") + logger.info(f"job {i + 1} attempt {j + 1}: job successful!") job_successful = True break - logger.info(f"job {i+1} attempt {j+1}: job still running, retrying in 20 seconds") + logger.info(f"job {i + 1} attempt {j + 1}: job still running, retrying in 20 seconds") time.sleep(20) if job_successful: diff --git a/tests/integration/temporal_client/workflows.py b/tests/integration/temporal_client/workflows.py index 058682f..96f13ae 100644 --- a/tests/integration/temporal_client/workflows.py +++ b/tests/integration/temporal_client/workflows.py @@ -4,9 +4,7 @@ """Temporal client sample workflow.""" -import asyncio from datetime import timedelta -from typing import List from temporalio import workflow diff --git a/tests/integration/test_charm.py b/tests/integration/test_charm.py index 0abc93e..c768cfc 100644 --- a/tests/integration/test_charm.py +++ b/tests/integration/test_charm.py @@ -3,11 +3,9 @@ # See LICENSE file for licensing details. import logging -import time import pytest import requests -from conftest import deploy # noqa: F401, pylint: disable=W0611 from helpers import APP_NAME_AIRBYTE_SERVER, get_unit_url, run_test_sync_job from pytest_operator.plugin import OpsTest diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index ccd1837..9968c3f 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -16,7 +16,7 @@ from ops.testing import Harness from charm import AirbyteK8SOperatorCharm -from src.literals import BASE_ENV, CONTAINERS +from src.literals import BASE_ENV, CONTAINER_HEALTH_CHECK_MAP from src.structured_config import StorageType logging.basicConfig(level=logging.DEBUG) @@ -50,17 +50,17 @@ def setUp(self): """Set up for the unit tests.""" self.harness = Harness(AirbyteK8SOperatorCharm) self.addCleanup(self.harness.cleanup) - for container_name in list(CONTAINERS.keys()): + for container_name in CONTAINER_HEALTH_CHECK_MAP: self.harness.set_can_connect(container_name, True) self.harness.set_leader(True) self.harness.set_model_name("airbyte-model") - self.harness.add_network("10.0.0.10", endpoint="peer") + self.harness.add_network("10.0.0.10", endpoint="airbyte-peer") self.harness.begin() def test_initial_plan(self): """The initial pebble plan is empty.""" harness = self.harness - for container_name in list(CONTAINERS.keys()): + for container_name in CONTAINER_HEALTH_CHECK_MAP: initial_plan = harness.get_container_pebble_plan(container_name).to_dict() self.assertEqual(initial_plan, {}) @@ -78,7 +78,7 @@ def test_blocked_by_db(self): harness = self.harness # Simulate peer relation readiness. - harness.add_relation("peer", "airbyte") + harness.add_relation("airbyte-peer", "airbyte") simulate_pebble_readiness(harness) @@ -93,7 +93,7 @@ def test_blocked_by_minio(self): harness = self.harness # Simulate peer relation readiness. - harness.add_relation("peer", "airbyte") + harness.add_relation("airbyte-peer", "airbyte") simulate_pebble_readiness(harness) @@ -114,7 +114,7 @@ def test_blocked_by_s3(self): harness.update_config({"storage-type": "S3"}) # Simulate peer relation readiness. - harness.add_relation("peer", "airbyte") + harness.add_relation("airbyte-peer", "airbyte") simulate_pebble_readiness(harness) @@ -136,7 +136,7 @@ def test_ready_with_minio(self): simulate_lifecycle(harness) # The plan is generated after pebble is ready. - for container_name in list(CONTAINERS.keys()): + for container_name in CONTAINER_HEALTH_CHECK_MAP: want_plan = create_plan(container_name, "MINIO") got_plan = harness.get_container_pebble_plan(container_name).to_dict() @@ -156,7 +156,7 @@ def test_ready_with_s3(self): simulate_lifecycle(harness) # The plan is generated after pebble is ready. - for container_name in list(CONTAINERS.keys()): + for container_name in CONTAINER_HEALTH_CHECK_MAP: want_plan = create_plan(container_name, "S3") got_plan = harness.get_container_pebble_plan(container_name).to_dict() @@ -173,9 +173,9 @@ def test_update_status_up(self): harness = self.harness simulate_lifecycle(harness) - for container_name in list(CONTAINERS.keys()): + for container_name, settings in CONTAINER_HEALTH_CHECK_MAP.items(): container = harness.model.unit.get_container(container_name) - if CONTAINERS[container_name]: + if settings: container.get_check = mock.Mock(status="up") container.get_check.return_value.status = CheckStatus.UP @@ -188,9 +188,9 @@ def test_update_status_down(self): simulate_lifecycle(harness) - for container_name in list(CONTAINERS.keys()): + for container_name, settings in CONTAINER_HEALTH_CHECK_MAP.items(): container = harness.model.unit.get_container(container_name) - if CONTAINERS[container_name]: + if settings: container.get_check = mock.Mock(status="up") container.get_check.return_value.status = CheckStatus.DOWN @@ -202,10 +202,10 @@ def test_incomplete_pebble_plan(self): harness = self.harness simulate_lifecycle(harness) - for container_name in list(CONTAINERS.keys()): + for container_name, settings in CONTAINER_HEALTH_CHECK_MAP.items(): container = harness.model.unit.get_container(container_name) container.add_layer(container_name, mock_incomplete_pebble_plan, combine=True) - if CONTAINERS[container_name]: + if settings: container.get_check = mock.Mock(status="up") container.get_check.return_value.status = CheckStatus.UP @@ -256,9 +256,13 @@ def simulate_lifecycle( Args: harness: ops.testing.Harness object used to simulate charm lifecycle. + _get_interfaces: Mock of "_get_interfaces" method. + _get_object_storage_data: Mock of "_get_object_storage_data" method. + create_bucket_if_not_exists: Mock of "create_bucket_if_not_exists" method. + set_bucket_lifecycle_policy: Mock of "set_bucket_lifecycle_policy" method. """ # Simulate peer relation readiness. - harness.add_relation("peer", "airbyte") + harness.add_relation("airbyte-peer", "airbyte") simulate_pebble_readiness(harness) @@ -280,8 +284,12 @@ def simulate_lifecycle( def simulate_pebble_readiness(harness): - # Simulate pebble readiness on all containers. - for container_name in list(CONTAINERS.keys()): + """Simulate pebble readiness on all charm container. + + Args: + harness: ops.testing.Harness object used to simulate charm lifecycle. + """ + for container_name in CONTAINER_HEALTH_CHECK_MAP: container = harness.model.unit.get_container(container_name) harness.charm.on[container_name].pebble_ready.emit(container) @@ -327,6 +335,15 @@ def s3_provider_databag(): def create_plan(container_name, storage_type): + """Create container pebble plan. + + Args: + container_name: Name of Airbyte container. + storage_type: Type of storage in charm config. + + Returns: + Container pebble plan. + """ want_plan = { "services": { container_name: { @@ -399,7 +416,7 @@ def create_plan(container_name, storage_type): } if container_name == "airbyte-api-server": - want_plan["services"][container_name]["environment"].update({"INTERNAL_API_HOST": f"http://airbyte-k8s:8001"}) + want_plan["services"][container_name]["environment"].update({"INTERNAL_API_HOST": "http://airbyte-k8s:8001"}) if storage_type == StorageType.minio: want_plan["services"][container_name]["environment"].update( @@ -425,7 +442,7 @@ def create_plan(container_name, storage_type): } ) - application_info = CONTAINERS[container_name] + application_info = CONTAINER_HEALTH_CHECK_MAP[container_name] if application_info: want_plan["services"][container_name].update( { diff --git a/tests/unit/test_state.py b/tests/unit/test_state.py index 5dd9fb3..0796622 100644 --- a/tests/unit/test_state.py +++ b/tests/unit/test_state.py @@ -3,13 +3,13 @@ # # Learn more about testing at: https://juju.is/docs/sdk/testing +"""Charm state unit tests.""" + import json from unittest import TestCase from state import State -"""Charm state unit tests.""" - class TestState(TestCase): """Unit tests for state. diff --git a/tests/unit/test_structured_config.py b/tests/unit/test_structured_config.py index c0bda66..c5d7dd4 100644 --- a/tests/unit/test_structured_config.py +++ b/tests/unit/test_structured_config.py @@ -37,8 +37,8 @@ def test_config_parsing_parameters_integer_values(_harness) -> None: check_valid_values(_harness, field, valid_values) -def test_product_related_values(_harness) -> None: - """Test specific parameters for each field.""" +def test_application_related_values(_harness) -> None: + """Test specific parameters for application-related fields.""" erroneus_values = ["test-value", "foo", "bar"] # storage-type @@ -48,7 +48,7 @@ def test_product_related_values(_harness) -> None: def test_cpu_related_values(_harness) -> None: - """Test specific parameters for each field.""" + """Test specific parameters for cpu-related fields.""" erroneus_values = ["-123", "0", "100f"] check_invalid_values(_harness, "job-main-container-cpu-limit", erroneus_values) accepted_values = ["200m", "4"] @@ -56,7 +56,7 @@ def test_cpu_related_values(_harness) -> None: def test_memory_related_values(_harness) -> None: - """Test specific parameters for each field.""" + """Test specific parameters for memory-related fields.""" erroneus_values = ["-123", "0", "100f"] check_invalid_values(_harness, "job-main-container-memory-limit", erroneus_values) accepted_values = ["4Gi", "256Mi"] diff --git a/tox.ini b/tox.ini index 9854c36..8812d52 100644 --- a/tox.ini +++ b/tox.ini @@ -11,7 +11,6 @@ max-line-length=120 [vars] src_path = {tox_root}/src tests_path = {tox_root}/tests -;lib_path = {tox_root}/lib/charms/operator_name_with_underscores all_path = {[vars]src_path} {[vars]tests_path} [testenv] @@ -56,11 +55,11 @@ commands = codespell {toxinidir} --skip {toxinidir}/.git --skip {toxinidir}/.tox \ --skip {toxinidir}/build --skip {toxinidir}/lib --skip {toxinidir}/venv \ --skip {toxinidir}/.mypy_cache --skip {toxinidir}/icon.svg - pflake8 {[vars]src_path} + pflake8 {[vars]src_path} {[vars]tests_path} isort --check-only --diff {[vars]src_path} {[vars]tests_path} black --check --diff {[vars]src_path} {[vars]tests_path} mypy {[vars]all_path} --ignore-missing-imports --follow-imports=skip --install-types --non-interactive - pylint {[vars]src_path} --disable=E0401,W1203,W0613,W0718,R0903,W1514,C0103,R0913,C0301,W0212,R0902,C0104,W0640,R0801,W0511,R0914,R0912 + pylint {[vars]src_path} {[vars]tests_path} --disable=E0401,W1203,W0613,W0718,R0903,W1514,C0103,R0913,C0301,W0212,R0902,C0104,W0640,R0801,W0511,R0914,R0912,E1120 [testenv:unit] @@ -100,10 +99,10 @@ commands = description = Run integration tests deps = ipdb==0.13.9 - juju==3.1.0.1 + juju==3.5.2.0 pytest==7.1.3 pytest-operator==0.35.0 - temporalio==1.1.0 + temporalio==1.6.0 pytest-asyncio==0.21 -r{toxinidir}/requirements.txt commands =