From 502f77d1ec1f87251a5a52fc16447403a8526683 Mon Sep 17 00:00:00 2001 From: Carl Csaposs Date: Tue, 17 Oct 2023 09:37:56 +0000 Subject: [PATCH 01/24] Sync files with K8s --- actions.yaml | 10 + charm_version | 1 + charmcraft.yaml | 3 + metadata.yaml | 5 + poetry.lock | 33 +- pyproject.toml | 1 + src/abstract_charm.py | 88 +++++- src/lifecycle.py | 10 + src/relations/database_provides.py | 15 +- src/relations/database_requires.py | 20 +- src/upgrade.py | 281 ++++++++++++++++++ src/workload.py | 8 +- tests/unit/conftest.py | 5 +- .../test_database_relations.py | 6 +- .../test_database_relations_breaking.py | 6 +- tests/unit/scenario_/test_start.py | 5 +- tox.ini | 8 + workload_version | 1 + 18 files changed, 452 insertions(+), 54 deletions(-) create mode 100644 actions.yaml create mode 100644 charm_version create mode 100644 src/upgrade.py create mode 100644 workload_version diff --git a/actions.yaml b/actions.yaml new file mode 100644 index 00000000..10dfc6c9 --- /dev/null +++ b/actions.yaml @@ -0,0 +1,10 @@ +# Copyright 2023 Canonical Ltd. +# See LICENSE file for licensing details. + +resume-upgrade: + description: Upgrade remaining units (after you manually verified that upgraded units are healthy). + params: + force: + type: boolean + description: Force upgrade of next unit if an upgraded unit has non-active status. + required: [] diff --git a/charm_version b/charm_version new file mode 100644 index 00000000..d00491fd --- /dev/null +++ b/charm_version @@ -0,0 +1 @@ +1 diff --git a/charmcraft.yaml b/charmcraft.yaml index 6a5346ee..33dd2864 100644 --- a/charmcraft.yaml +++ b/charmcraft.yaml @@ -28,3 +28,6 @@ parts: exit 1 fi charm-entrypoint: src/machine_charm.py + prime: + - charm_version + - workload_version diff --git a/metadata.yaml b/metadata.yaml index a484b388..1f8b8049 100644 --- a/metadata.yaml +++ b/metadata.yaml @@ -36,6 +36,11 @@ requires: interface: juju-info scope: container peers: + upgrade-version-a: + # Relation versioning scheme: + # DA056 - Upgrading in-place upgrade protocol + # https://docs.google.com/document/d/1H7qy5SAwLiCOKO9xMQJbbQP5_-jGV6Lhi-mJOk4gZ08/edit + interface: upgrade # TODO TLS VM: re-enable peer relation # mysql-router-peers: # interface: mysql_router_peers diff --git a/poetry.lock b/poetry.lock index 7ada43ea..91909497 100644 --- a/poetry.lock +++ b/poetry.lock @@ -835,6 +835,16 @@ files = [ {file = "MarkupSafe-2.1.3-cp311-cp311-musllinux_1_1_x86_64.whl", hash = "sha256:5bbe06f8eeafd38e5d0a4894ffec89378b6c6a625ff57e3028921f8ff59318ac"}, {file = "MarkupSafe-2.1.3-cp311-cp311-win32.whl", hash = "sha256:dd15ff04ffd7e05ffcb7fe79f1b98041b8ea30ae9234aed2a9168b5797c3effb"}, {file = "MarkupSafe-2.1.3-cp311-cp311-win_amd64.whl", hash = "sha256:134da1eca9ec0ae528110ccc9e48041e0828d79f24121a1a146161103c76e686"}, + {file = "MarkupSafe-2.1.3-cp312-cp312-macosx_10_9_universal2.whl", hash = "sha256:f698de3fd0c4e6972b92290a45bd9b1536bffe8c6759c62471efaa8acb4c37bc"}, + {file = "MarkupSafe-2.1.3-cp312-cp312-macosx_10_9_x86_64.whl", hash = "sha256:aa57bd9cf8ae831a362185ee444e15a93ecb2e344c8e52e4d721ea3ab6ef1823"}, + {file = "MarkupSafe-2.1.3-cp312-cp312-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:ffcc3f7c66b5f5b7931a5aa68fc9cecc51e685ef90282f4a82f0f5e9b704ad11"}, + {file = "MarkupSafe-2.1.3-cp312-cp312-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:47d4f1c5f80fc62fdd7777d0d40a2e9dda0a05883ab11374334f6c4de38adffd"}, + {file = "MarkupSafe-2.1.3-cp312-cp312-manylinux_2_5_i686.manylinux1_i686.manylinux_2_17_i686.manylinux2014_i686.whl", hash = "sha256:1f67c7038d560d92149c060157d623c542173016c4babc0c1913cca0564b9939"}, + {file = "MarkupSafe-2.1.3-cp312-cp312-musllinux_1_1_aarch64.whl", hash = "sha256:9aad3c1755095ce347e26488214ef77e0485a3c34a50c5a5e2471dff60b9dd9c"}, + {file = "MarkupSafe-2.1.3-cp312-cp312-musllinux_1_1_i686.whl", hash = "sha256:14ff806850827afd6b07a5f32bd917fb7f45b046ba40c57abdb636674a8b559c"}, + {file = "MarkupSafe-2.1.3-cp312-cp312-musllinux_1_1_x86_64.whl", hash = "sha256:8f9293864fe09b8149f0cc42ce56e3f0e54de883a9de90cd427f191c346eb2e1"}, + {file = "MarkupSafe-2.1.3-cp312-cp312-win32.whl", hash = "sha256:715d3562f79d540f251b99ebd6d8baa547118974341db04f5ad06d5ea3eb8007"}, + {file = "MarkupSafe-2.1.3-cp312-cp312-win_amd64.whl", hash = "sha256:1b8dd8c3fd14349433c79fa8abeb573a55fc0fdd769133baac1f5e07abf54aeb"}, {file = "MarkupSafe-2.1.3-cp37-cp37m-macosx_10_9_x86_64.whl", hash = "sha256:8e254ae696c88d98da6555f5ace2279cf7cd5b3f52be2b5cf97feafe883b58d2"}, {file = "MarkupSafe-2.1.3-cp37-cp37m-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:cb0932dc158471523c9637e807d9bfb93e06a95cbf010f1a38b98623b929ef2b"}, {file = "MarkupSafe-2.1.3-cp37-cp37m-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:9402b03f1a1b4dc4c19845e5c749e3ab82d5078d16a2a4c2cd2df62d57bb0707"}, @@ -1121,6 +1131,17 @@ files = [ dev = ["pre-commit", "tox"] testing = ["pytest", "pytest-benchmark"] +[[package]] +name = "poetry-core" +version = "1.7.0" +description = "Poetry PEP 517 Build Backend" +optional = false +python-versions = ">=3.8,<4.0" +files = [ + {file = "poetry_core-1.7.0-py3-none-any.whl", hash = "sha256:38e174cdb00a84ee4a1cab66a378b435747f72414f5573bc18cfc3850a94df38"}, + {file = "poetry_core-1.7.0.tar.gz", hash = "sha256:8f679b83bd9c820082637beca1204124d5d2a786e4818da47ec8acefd0353b74"}, +] + [[package]] name = "prompt-toolkit" version = "3.0.39" @@ -1522,6 +1543,7 @@ files = [ {file = "PyYAML-6.0.1-cp310-cp310-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:69b023b2b4daa7548bcfbd4aa3da05b3a74b772db9e23b982788168117739938"}, {file = "PyYAML-6.0.1-cp310-cp310-manylinux_2_17_s390x.manylinux2014_s390x.whl", hash = "sha256:81e0b275a9ecc9c0c0c07b4b90ba548307583c125f54d5b6946cfee6360c733d"}, {file = "PyYAML-6.0.1-cp310-cp310-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:ba336e390cd8e4d1739f42dfe9bb83a3cc2e80f567d8805e11b46f4a943f5515"}, + {file = "PyYAML-6.0.1-cp310-cp310-musllinux_1_1_x86_64.whl", hash = "sha256:326c013efe8048858a6d312ddd31d56e468118ad4cdeda36c719bf5bb6192290"}, {file = "PyYAML-6.0.1-cp310-cp310-win32.whl", hash = "sha256:bd4af7373a854424dabd882decdc5579653d7868b8fb26dc7d0e99f823aa5924"}, {file = "PyYAML-6.0.1-cp310-cp310-win_amd64.whl", hash = "sha256:fd1592b3fdf65fff2ad0004b5e363300ef59ced41c2e6b3a99d4089fa8c5435d"}, {file = "PyYAML-6.0.1-cp311-cp311-macosx_10_9_x86_64.whl", hash = "sha256:6965a7bc3cf88e5a1c3bd2e0b5c22f8d677dc88a455344035f03399034eb3007"}, @@ -1529,8 +1551,15 @@ files = [ {file = "PyYAML-6.0.1-cp311-cp311-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:42f8152b8dbc4fe7d96729ec2b99c7097d656dc1213a3229ca5383f973a5ed6d"}, {file = "PyYAML-6.0.1-cp311-cp311-manylinux_2_17_s390x.manylinux2014_s390x.whl", hash = "sha256:062582fca9fabdd2c8b54a3ef1c978d786e0f6b3a1510e0ac93ef59e0ddae2bc"}, {file = "PyYAML-6.0.1-cp311-cp311-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:d2b04aac4d386b172d5b9692e2d2da8de7bfb6c387fa4f801fbf6fb2e6ba4673"}, + {file = "PyYAML-6.0.1-cp311-cp311-musllinux_1_1_x86_64.whl", hash = "sha256:e7d73685e87afe9f3b36c799222440d6cf362062f78be1013661b00c5c6f678b"}, {file = "PyYAML-6.0.1-cp311-cp311-win32.whl", hash = "sha256:1635fd110e8d85d55237ab316b5b011de701ea0f29d07611174a1b42f1444741"}, {file = "PyYAML-6.0.1-cp311-cp311-win_amd64.whl", hash = "sha256:bf07ee2fef7014951eeb99f56f39c9bb4af143d8aa3c21b1677805985307da34"}, + {file = "PyYAML-6.0.1-cp312-cp312-macosx_10_9_x86_64.whl", hash = "sha256:855fb52b0dc35af121542a76b9a84f8d1cd886ea97c84703eaa6d88e37a2ad28"}, + {file = "PyYAML-6.0.1-cp312-cp312-macosx_11_0_arm64.whl", hash = "sha256:40df9b996c2b73138957fe23a16a4f0ba614f4c0efce1e9406a184b6d07fa3a9"}, + {file = "PyYAML-6.0.1-cp312-cp312-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:6c22bec3fbe2524cde73d7ada88f6566758a8f7227bfbf93a408a9d86bcc12a0"}, + {file = "PyYAML-6.0.1-cp312-cp312-musllinux_1_1_x86_64.whl", hash = "sha256:8d4e9c88387b0f5c7d5f281e55304de64cf7f9c0021a3525bd3b1c542da3b0e4"}, + {file = "PyYAML-6.0.1-cp312-cp312-win32.whl", hash = "sha256:d483d2cdf104e7c9fa60c544d92981f12ad66a457afae824d146093b8c294c54"}, + {file = "PyYAML-6.0.1-cp312-cp312-win_amd64.whl", hash = "sha256:0d3304d8c0adc42be59c5f8a4d9e3d7379e6955ad754aa9d6ab7a398b59dd1df"}, {file = "PyYAML-6.0.1-cp36-cp36m-macosx_10_9_x86_64.whl", hash = "sha256:50550eb667afee136e9a77d6dc71ae76a44df8b3e51e41b77f6de2932bfe0f47"}, {file = "PyYAML-6.0.1-cp36-cp36m-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:1fe35611261b29bd1de0070f0b2f47cb6ff71fa6595c077e42bd0c419fa27b98"}, {file = "PyYAML-6.0.1-cp36-cp36m-manylinux_2_17_s390x.manylinux2014_s390x.whl", hash = "sha256:704219a11b772aea0d8ecd7058d0082713c3562b4e271b849ad7dc4a5c90c13c"}, @@ -1547,6 +1576,7 @@ files = [ {file = "PyYAML-6.0.1-cp38-cp38-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:a0cd17c15d3bb3fa06978b4e8958dcdc6e0174ccea823003a106c7d4d7899ac5"}, {file = "PyYAML-6.0.1-cp38-cp38-manylinux_2_17_s390x.manylinux2014_s390x.whl", hash = "sha256:28c119d996beec18c05208a8bd78cbe4007878c6dd15091efb73a30e90539696"}, {file = "PyYAML-6.0.1-cp38-cp38-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:7e07cbde391ba96ab58e532ff4803f79c4129397514e1413a7dc761ccd755735"}, + {file = "PyYAML-6.0.1-cp38-cp38-musllinux_1_1_x86_64.whl", hash = "sha256:49a183be227561de579b4a36efbb21b3eab9651dd81b1858589f796549873dd6"}, {file = "PyYAML-6.0.1-cp38-cp38-win32.whl", hash = "sha256:184c5108a2aca3c5b3d3bf9395d50893a7ab82a38004c8f61c258d4428e80206"}, {file = "PyYAML-6.0.1-cp38-cp38-win_amd64.whl", hash = "sha256:1e2722cc9fbb45d9b87631ac70924c11d3a401b2d7f410cc0e3bbf249f2dca62"}, {file = "PyYAML-6.0.1-cp39-cp39-macosx_10_9_x86_64.whl", hash = "sha256:9eb6caa9a297fc2c2fb8862bc5370d0303ddba53ba97e71f08023b6cd73d16a8"}, @@ -1554,6 +1584,7 @@ files = [ {file = "PyYAML-6.0.1-cp39-cp39-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:5773183b6446b2c99bb77e77595dd486303b4faab2b086e7b17bc6bef28865f6"}, {file = "PyYAML-6.0.1-cp39-cp39-manylinux_2_17_s390x.manylinux2014_s390x.whl", hash = "sha256:b786eecbdf8499b9ca1d697215862083bd6d2a99965554781d0d8d1ad31e13a0"}, {file = "PyYAML-6.0.1-cp39-cp39-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:bc1bf2925a1ecd43da378f4db9e4f799775d6367bdb94671027b73b393a7c42c"}, + {file = "PyYAML-6.0.1-cp39-cp39-musllinux_1_1_x86_64.whl", hash = "sha256:04ac92ad1925b2cff1db0cfebffb6ffc43457495c9b3c39d3fcae417d7125dc5"}, {file = "PyYAML-6.0.1-cp39-cp39-win32.whl", hash = "sha256:faca3bdcf85b2fc05d06ff3fbc1f83e1391b3e724afa3feba7d13eeab355484c"}, {file = "PyYAML-6.0.1-cp39-cp39-win_amd64.whl", hash = "sha256:510c9deebc5c0225e8c96813043e62b680ba2f9c50a08d3724c7f28a747d1486"}, {file = "PyYAML-6.0.1.tar.gz", hash = "sha256:bfdf460b1736c775f2ba9f6a92bca30bc2095067b8a9d77876d1fad6cc3b4a43"}, @@ -1967,4 +1998,4 @@ files = [ [metadata] lock-version = "2.0" python-versions = "^3.8.1" -content-hash = "8b28fa9ac25a886f6c1f0f1b346635e418f409b74801921a5872cd771f04c62f" +content-hash = "39a6518d54ff3b1d0fceeeb4b878f9f294973d7e0cb43bd2c4b9a1d057ee9920" diff --git a/pyproject.toml b/pyproject.toml index fb87b26d..46dc6013 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -12,6 +12,7 @@ authors = [] python = "^3.8.1" # ^3.8.1 required by flake8 ops = "^2.6.0" tenacity = "^8.2.3" +poetry-core = "^1.7.0" jinja2 = "^3.1.2" [tool.poetry.group.charm-libs.dependencies] diff --git a/src/abstract_charm.py b/src/abstract_charm.py index 30796171..6c8720da 100644 --- a/src/abstract_charm.py +++ b/src/abstract_charm.py @@ -16,6 +16,7 @@ import logrotate import relations.database_provides import relations.database_requires +import upgrade import workload logger = logging.getLogger(__name__) @@ -35,11 +36,21 @@ def __init__(self, *args) -> None: self._authenticated_workload_type = workload.AuthenticatedWorkload self._database_requires = relations.database_requires.RelationEndpoint(self) self._database_provides = relations.database_provides.RelationEndpoint(self) - self.framework.observe(self.on.update_status, self.reconcile_database_relations) - # Set status on first start if no relations active - self.framework.observe(self.on.start, self.reconcile_database_relations) + self.framework.observe(self.on.update_status, self.reconcile) + self.framework.observe( + self.on[upgrade.PEER_RELATION_ENDPOINT_NAME].relation_changed, self.reconcile + ) + self.framework.observe( + self.on[upgrade.RESUME_ACTION_NAME].action, self._on_resume_upgrade_action + ) + # (For Kubernetes) Reset partition after scale down + self.framework.observe( + self.on[upgrade.PEER_RELATION_ENDPOINT_NAME].relation_departed, self.reconcile + ) + # Handle upgrade & set status on first start if no relations active + self.framework.observe(self.on.start, self.reconcile) # Update app status - self.framework.observe(self.on.leader_elected, self.reconcile_database_relations) + self.framework.observe(self.on.leader_elected, self.reconcile) @property @abc.abstractmethod @@ -60,6 +71,11 @@ def _tls_certificate_saved(self) -> bool: def _container(self) -> container.Container: """Workload container (snap or ROCK)""" + @property + @abc.abstractmethod + def _upgrade(self) -> typing.Optional[upgrade.Upgrade]: + pass + @property @abc.abstractmethod def _logrotate(self) -> logrotate.LogRotate: @@ -95,8 +111,8 @@ def _prioritize_statuses(statuses: typing.List[ops.StatusBase]) -> ops.StatusBas """ status_priority = ( ops.BlockedStatus, - ops.WaitingStatus, ops.MaintenanceStatus, + ops.WaitingStatus, # Catch any unknown status type ops.StatusBase, ) @@ -108,6 +124,11 @@ def _prioritize_statuses(statuses: typing.List[ops.StatusBase]) -> ops.StatusBas def _determine_app_status(self, *, event) -> ops.StatusBase: """Report app status.""" + if self._upgrade and (upgrade_status := self._upgrade.app_status): + # Upgrade status should take priority over relation status—even if the status level is + # normally lower priority. + # (Relations should not be modified during upgrade.) + return upgrade_status statuses = [] for endpoint in (self._database_requires, self._database_provides): if status := endpoint.get_status(event): @@ -118,16 +139,19 @@ def _determine_unit_status(self, *, event) -> ops.StatusBase: """Report unit status.""" statuses = [] workload_ = self.get_workload(event=event) - statuses.append(workload_.get_status(event)) + statuses.append(workload_.status) + if self._upgrade: + statuses.append(self._upgrade.unit_juju_status) return self._prioritize_statuses(statuses) - def set_status(self, *, event) -> None: + def set_status(self, *, event, app=True, unit=True) -> None: """Set charm status.""" - if self._unit_lifecycle.authorized_leader: + if app and self._unit_lifecycle.authorized_leader: self.app.status = self._determine_app_status(event=event) logger.debug(f"Set app status to {self.app.status}") - self.unit.status = self._determine_unit_status(event=event) - logger.debug(f"Set unit status to {self.unit.status}") + if unit: + self.unit.status = self._determine_unit_status(event=event) + logger.debug(f"Set unit status to {self.unit.status}") def wait_until_mysql_router_ready(self) -> None: """Wait until a connection to MySQL Router is possible. @@ -156,21 +180,38 @@ def wait_until_mysql_router_ready(self) -> None: # Handlers # ======================= - def reconcile_database_relations(self, event=None) -> None: - """Handle database requires/provides events.""" + def reconcile(self, event=None) -> None: # noqa: C901 + """Handle most events.""" + if not self._upgrade: + logger.debug("Peer relation not available") + return + if self._upgrade.unit_state == "restarting": + if not self._upgrade.is_compatible: + self.unit.status = ops.BlockedStatus( + "Upgrade incompatible. Rollback to previous revision with `juju refresh`" + ) + self.set_status(event=event, unit=False) + return workload_ = self.get_workload(event=event) logger.debug( "State of reconcile " f"{self._unit_lifecycle.authorized_leader=}, " f"{isinstance(workload_, workload.AuthenticatedWorkload)=}, " f"{workload_.container_ready=}, " - f"{self._database_requires.is_relation_breaking(event)=}" + f"{self._database_requires.is_relation_breaking(event)=}, " + f"{self._upgrade.in_progress=}" ) if self._unit_lifecycle.authorized_leader: if self._database_requires.is_relation_breaking(event): + if self._upgrade.in_progress: + logger.warning( + "Modifying relations during an upgrade is not supported. The charm may be in a broken, unrecoverable state. Re-deploy the charm" + ) self._database_provides.delete_all_databags() elif ( - isinstance(workload_, workload.AuthenticatedWorkload) and workload_.container_ready + not self._upgrade.in_progress + and isinstance(workload_, workload.AuthenticatedWorkload) + and workload_.container_ready ): self._database_provides.reconcile_users( event=event, @@ -182,4 +223,23 @@ def reconcile_database_relations(self, event=None) -> None: workload_.enable(tls=self._tls_certificate_saved, unit_name=self.unit.name) elif workload_.container_ready: workload_.disable() + if not workload_.status: + self._upgrade.unit_state = "healthy" + if self._unit_lifecycle.authorized_leader: + self._upgrade.reconcile_partition() + if not self._upgrade.in_progress: + self._upgrade.set_versions_in_app_databag() self.set_status(event=event) + + def _on_resume_upgrade_action(self, event: ops.ActionEvent) -> None: + if not self._unit_lifecycle.authorized_leader: + message = f"Must run action on leader unit. (e.g. `juju run {self.app.name}/leader {upgrade.RESUME_ACTION_NAME}`)" + logger.debug(f"Resume upgrade event failed: {message}") + event.fail(message) + return + if not self._upgrade or not self._upgrade.in_progress: + message = "No upgrade in progress" + logger.debug(f"Resume upgrade event failed: {message}") + event.fail(message) + return + self._upgrade.reconcile_partition(action_event=event) diff --git a/src/lifecycle.py b/src/lifecycle.py index 83f00ef5..c8ec9da4 100644 --- a/src/lifecycle.py +++ b/src/lifecycle.py @@ -52,6 +52,7 @@ def __init__( super().__init__(charm, str(type(self))) if subordinated_relation_endpoint_names is None: subordinated_relation_endpoint_names = () + self._subordinate = bool(subordinated_relation_endpoint_names) self._charm = charm for relation_endpoint in self.model.relations: if relation_endpoint in subordinated_relation_endpoint_names: @@ -127,6 +128,15 @@ def _on_subordinate_relation_broken(self, event: ops.RelationBrokenEvent) -> Non # Situation #1, #2, or #3 self._unit_tearing_down_and_app_active = _UnitTearingDownAndAppActive.FALSE + @property + def tearing_down_and_app_active(self) -> bool: + """Whether unit is tearing down and 1+ other units are NOT tearing down + + Cannot be called on subordinate charms + """ + assert not self._subordinate + return self._unit_tearing_down_and_app_active is not _UnitTearingDownAndAppActive.FALSE + @property def authorized_leader(self) -> bool: """Whether unit is authorized to act as leader diff --git a/src/relations/database_provides.py b/src/relations/database_provides.py index aae80508..a186f3da 100644 --- a/src/relations/database_provides.py +++ b/src/relations/database_provides.py @@ -150,18 +150,9 @@ class RelationEndpoint: def __init__(self, charm_: "abstract_charm.MySQLRouterCharm") -> None: self._interface = data_interfaces.DatabaseProvides(charm_, relation_name=self._NAME) - charm_.framework.observe( - charm_.on[self._NAME].relation_created, - charm_.reconcile_database_relations, - ) - charm_.framework.observe( - self._interface.on.database_requested, - charm_.reconcile_database_relations, - ) - charm_.framework.observe( - charm_.on[self._NAME].relation_broken, - charm_.reconcile_database_relations, - ) + charm_.framework.observe(charm_.on[self._NAME].relation_created, charm_.reconcile) + charm_.framework.observe(self._interface.on.database_requested, charm_.reconcile) + charm_.framework.observe(charm_.on[self._NAME].relation_broken, charm_.reconcile) @property # TODO python3.10 min version: Use `list` instead of `typing.List` diff --git a/src/relations/database_requires.py b/src/relations/database_requires.py index c8017bba..088f4d94 100644 --- a/src/relations/database_requires.py +++ b/src/relations/database_requires.py @@ -80,22 +80,10 @@ def __init__(self, charm_: "abstract_charm.MySQLRouterCharm") -> None: database_name="mysql_innodb_cluster_metadata", extra_user_roles="mysqlrouter", ) - charm_.framework.observe( - charm_.on[self._NAME].relation_created, - charm_.reconcile_database_relations, - ) - charm_.framework.observe( - self._interface.on.database_created, - charm_.reconcile_database_relations, - ) - charm_.framework.observe( - self._interface.on.endpoints_changed, - charm_.reconcile_database_relations, - ) - charm_.framework.observe( - charm_.on[self._NAME].relation_broken, - charm_.reconcile_database_relations, - ) + charm_.framework.observe(charm_.on[self._NAME].relation_created, charm_.reconcile) + charm_.framework.observe(self._interface.on.database_created, charm_.reconcile) + charm_.framework.observe(self._interface.on.endpoints_changed, charm_.reconcile) + charm_.framework.observe(charm_.on[self._NAME].relation_broken, charm_.reconcile) def get_connection_info(self, *, event) -> typing.Optional[ConnectionInformation]: """Information for connection to MySQL cluster""" diff --git a/src/upgrade.py b/src/upgrade.py new file mode 100644 index 00000000..eb702ff1 --- /dev/null +++ b/src/upgrade.py @@ -0,0 +1,281 @@ +# Copyright 2023 Canonical Ltd. +# See LICENSE file for licensing details. + +"""In-place upgrades + +Based off specification: DA058 - In-Place Upgrades - Kubernetes v2 +(https://docs.google.com/document/d/1tLjknwHudjcHs42nzPVBNkHs98XxAOT2BXGGpP7NyEU/) +""" + +import abc +import json +import logging +import pathlib +import typing + +import ops +import poetry.core.constraints.version as poetry_version + +logger = logging.getLogger(__name__) + +PEER_RELATION_ENDPOINT_NAME = "upgrade-version-a" +RESUME_ACTION_NAME = "resume-upgrade" + + +def _unit_number(unit_: ops.Unit) -> int: + return int(unit_.name.split("/")[-1]) + + +class PeerRelationNotReady(Exception): + """Upgrade peer relation not available (to this unit)""" + + +class Upgrade(abc.ABC): + """In-place upgrades""" + + def __init__(self, charm_: ops.CharmBase) -> None: + relations = charm_.model.relations[PEER_RELATION_ENDPOINT_NAME] + if not relations: + raise PeerRelationNotReady + assert len(relations) == 1 + self._peer_relation = relations[0] + self._unit: ops.Unit = charm_.unit + self._unit_databag = self._peer_relation.data[self._unit] + self._app_databag = self._peer_relation.data[charm_.app] + self._app_name = charm_.app.name + self._current_versions = {} # For this unit + for version, file_name in { + "charm": "charm_version", + "workload": "workload_version", + }.items(): + self._current_versions[version] = pathlib.Path(file_name).read_text().strip() + + @property + def unit_state(self) -> typing.Optional[str]: + """Unit upgrade state""" + return self._unit_databag.get("state") + + @unit_state.setter + def unit_state(self, value: str) -> None: + self._unit_databag["state"] = value + + @property + def is_compatible(self) -> bool: + """Whether upgrade is supported from previous versions""" + try: + previous_version_strs: dict[str, str] = json.loads(self._app_databag["versions"]) + except KeyError as exception: + logger.debug("`versions` missing from peer relation", exc_info=exception) + return False + previous_versions: dict[str, poetry_version.Version] = { + key: poetry_version.Version.parse(value) + for key, value in previous_version_strs.items() + } + current_versions = { + key: poetry_version.Version.parse(value) + for key, value in self._current_versions.items() + } + try: + if ( + # TODO charm versioning: Un-comment when charm versioning specification is + # implemented. Charm versions with git hash (temporary implementation) cannot be + # compared with `<` + # previous_versions["charm"] > current_versions["charm"] or + previous_versions["charm"].major + != current_versions["charm"].major + ): + logger.debug( + f'{previous_versions["charm"]=} incompatible with {current_versions["charm"]=}' + ) + return False + if ( + previous_versions["workload"] > current_versions["workload"] + or previous_versions["workload"].major != current_versions["workload"].major + or previous_versions["workload"].minor != current_versions["workload"].minor + ): + logger.debug( + f'{previous_versions["workload"]=} incompatible with {current_versions["workload"]=}' + ) + return False + logger.debug( + f"Versions before upgrade compatible with versions after upgrade {previous_version_strs=} {self._current_versions=}" + ) + return True + except KeyError as exception: + logger.debug(f"Version missing from {previous_versions=}", exc_info=exception) + return False + + @property + def in_progress(self) -> bool: + logger.debug(f"{self._app_workload_version=} {self._unit_workload_versions=}") + return any( + version != self._app_workload_version + for version in self._unit_workload_versions.values() + ) + + @property + def _sorted_units(self) -> list[ops.Unit]: + """Units sorted from highest to lowest unit number""" + return sorted((self._unit, *self._peer_relation.units), key=_unit_number, reverse=True) + + @property + def _unit_active_status(self) -> ops.ActiveStatus: + """Status shown during upgrade if unit is healthy""" + return ops.ActiveStatus(self._current_versions["charm"]) + + @property + def unit_juju_status(self) -> typing.Optional[ops.StatusBase]: + if self.in_progress: + return self._unit_active_status + + @property + def app_status(self) -> typing.Optional[ops.StatusBase]: + if self.in_progress: + if len(self._sorted_units) >= 2 and self._partition > _unit_number( + self._sorted_units[1] + ): + # User confirmation needed to resume upgrade (i.e. upgrade second unit) + # Statuses over 120 characters are truncated in `juju status` as of juju 3.1.6 and + # 2.9.45 + return ops.BlockedStatus( + f"Upgrading. Verify highest unit is healthy & run `{RESUME_ACTION_NAME}` action. To rollback, `juju refresh` to last revision" + ) + else: + return ops.MaintenanceStatus( + "Upgrading. To rollback, `juju refresh` to the previous revision" + ) + + def set_versions_in_app_databag(self) -> None: + """Save current versions in app databag + + Used after next upgrade to check compatibility (i.e. whether that upgrade should be + allowed) + """ + assert not self.in_progress + logger.debug(f"Setting {self._current_versions=} in upgrade peer relation app databag") + self._app_databag["versions"] = json.dumps(self._current_versions) + logger.debug(f"Set {self._current_versions=} in upgrade peer relation app databag") + + @property + @abc.abstractmethod + def _partition(self) -> int: + """Specifies which units should upgrade + + Unit numbers >= partition should upgrade + Unit numbers < partition should not upgrade + + Based on Kubernetes StatefulSet partition + (https://kubernetes.io/docs/concepts/workloads/controllers/statefulset/#partitions) + + For Kubernetes, unit numbers are guaranteed to be sequential + For machines, unit numbers are not guaranteed to be sequential + """ + + @_partition.setter + @abc.abstractmethod + def _partition(self, value: int) -> None: + pass + + @property + @abc.abstractmethod + def _unit_workload_versions(self) -> dict[str, str]: + """{Unit name: unique identifier for unit's workload version} + + If and only if this version changes, the workload will restart (during upgrade or + rollback). + + On Kubernetes, the workload & charm are upgraded together + On machines, the charm is upgraded before the workload + + This identifier should be comparable to `_app_workload_version` to determine if the unit & + app are the same workload version. + """ + + @property + @abc.abstractmethod + def _app_workload_version(self) -> str: + """Unique identifier for the app's workload version + + This should match the workload version in the current Juju app charm version. + + This identifier should be comparable to `_get_unit_workload_version` to determine if the + app & unit are the same workload version. + """ + + def reconcile_partition(self, *, action_event: ops.ActionEvent = None) -> None: + """If ready, lower partition to upgrade next unit. + + If upgrade is not in progress, set partition to 0. (On Kubernetes, if a unit receives a + stop event, it may raise the partition even if an upgrade is not in progress.) + + Automatically upgrades next unit if all upgraded units are healthy—except if only one unit + has upgraded (need manual user confirmation [via Juju action] to upgrade next unit) + + Handle Juju action to: + - confirm first upgraded unit is healthy and resume upgrade + - force upgrade of next unit if 1 or more upgraded units are unhealthy + """ + force = action_event and action_event.params.get("force") is True + + units = self._sorted_units + + def determine_partition() -> int: + if not self.in_progress: + return 0 + logger.debug(f"{self._peer_relation.data=}") + for upgrade_order_index, unit in enumerate(units): + # Note: upgrade_order_index != unit number + if ( + force is False and self._peer_relation.data[unit].get("state") != "healthy" + ) or self._unit_workload_versions[unit.name] != self._app_workload_version: + if not action_event and upgrade_order_index == 1: + # User confirmation needed to resume upgrade (i.e. upgrade second unit) + return _unit_number(units[0]) + return _unit_number(unit) + return 0 + + partition = determine_partition() + logger.debug(f"{self._partition=}, {partition=}") + # Only lower the partition—do not raise it. + # If this method is called during the action event and then called during another event a + # few seconds later, `determine_partition()` could return a lower number during the action + # and then a higher number a few seconds later. + + # On machines, this could cause a unit to not upgrade & require that the action is run + # again. (e.g. if an update-status event fires immediately after the action and before the + # would-be upgrading unit receives a relation-changed event.) + + # On Kubernetes, this can cause the unit to hang. + # Example: If partition is lowered to 1, unit 1 begins to upgrade, and partition is set to + # 2 right away, the unit/Juju agent will hang + # Details: https://chat.charmhub.io/charmhub/pl/on8rd538ufn4idgod139skkbfr + # This does not address the situation where another unit > 1 restarts and sets the + # partition during the `stop` event, but that is unlikely to occur in the small time window + # that causes the unit to hang. + if partition < self._partition: + self._partition = partition + logger.debug( + f"Lowered partition to {partition} {action_event=} {force=} {self.in_progress=}" + ) + if action_event: + assert len(units) >= 2 + if self._partition > _unit_number(units[1]): + message = "Highest number unit is unhealthy. Upgrade will not resume." + logger.debug(f"Resume upgrade event failed: {message}") + action_event.fail(message) + return + if force: + # If a unit was unhealthy and the upgrade was forced, only the next unit will + # upgrade. As long as 1 or more units are unhealthy, the upgrade will need to be + # forced for each unit. + + # Include "Attempting to" because (on Kubernetes) we only control the partition, + # not which units upgrade. Kubernetes may not upgrade a unit even if the partition + # allows it (e.g. if the charm container of a higher unit is not ready). This is + # also applicable `if not force`, but is unlikely to happen since all units are + # "healthy" `if not force`. + message = f"Attempting to upgrade unit {self._partition}" + else: + message = f"Upgrade resumed. Unit {self._partition} is upgrading next" + action_event.set_results({"result": message}) + logger.debug(f"Resume upgrade event succeeded: {message}") diff --git a/src/workload.py b/src/workload.py index 46b3062c..828f2762 100644 --- a/src/workload.py +++ b/src/workload.py @@ -99,7 +99,8 @@ def disable_tls(self) -> None: file.unlink(missing_ok=True) logger.debug("Disabled TLS") - def get_status(self, event) -> typing.Optional[ops.StatusBase]: + @property + def status(self) -> typing.Optional[ops.StatusBase]: """Report non-active status.""" if not self.container_ready: return ops.MaintenanceStatus("Waiting for container") @@ -250,9 +251,10 @@ def disable_tls(self) -> None: if self._container.mysql_router_service_enabled: self._restart(tls=False) - def get_status(self, event) -> typing.Optional[ops.StatusBase]: + @property + def status(self) -> typing.Optional[ops.StatusBase]: """Report non-active status.""" - if status := super().get_status(event): + if status := super().status: return status if not self.shell.is_router_in_cluster_set(self._router_id): # Router should not be removed from ClusterSet after bootstrap (except by MySQL charm diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index 940f4228..7cc51e38 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -32,10 +32,9 @@ def patch(monkeypatch): "abstract_charm.MySQLRouterCharm.wait_until_mysql_router_ready", lambda *args, **kwargs: None, ) - monkeypatch.setattr( - "workload.AuthenticatedWorkload._router_username", lambda *args, **kwargs: "" - ) + monkeypatch.setattr("workload.AuthenticatedWorkload._router_username", "") monkeypatch.setattr("mysql_shell.Shell.is_router_in_cluster_set", lambda *args, **kwargs: True) + monkeypatch.setattr("upgrade.Upgrade.in_progress", False) @pytest.fixture(autouse=True) diff --git a/tests/unit/scenario_/database_relations/test_database_relations.py b/tests/unit/scenario_/database_relations/test_database_relations.py index 8dbb25fa..4737b5c5 100644 --- a/tests/unit/scenario_/database_relations/test_database_relations.py +++ b/tests/unit/scenario_/database_relations/test_database_relations.py @@ -23,7 +23,7 @@ def output_states(*, relations: list[scenario.Relation]) -> typing.Iterable[scen """ context = scenario.Context(machine_charm.MachineSubordinateRouterCharm) input_state = scenario.State( - relations=relations, + relations=[*relations, scenario.PeerRelation(endpoint="upgrade-version-a")], leader=True, ) events = [] @@ -36,7 +36,9 @@ def output_states(*, relations: list[scenario.Relation]) -> typing.Iterable[scen ) ) for event in events: - yield context.run(event, input_state) + output = context.run(event, input_state) + output.relations.pop() # Remove PeerRelation + yield output # Tests are ordered by status priority. diff --git a/tests/unit/scenario_/database_relations/test_database_relations_breaking.py b/tests/unit/scenario_/database_relations/test_database_relations_breaking.py index f8eba6a1..6b83a792 100644 --- a/tests/unit/scenario_/database_relations/test_database_relations_breaking.py +++ b/tests/unit/scenario_/database_relations/test_database_relations_breaking.py @@ -15,10 +15,12 @@ def output_state(*, relations: list[scenario.Relation], event: scenario.Event) -> scenario.State: context = scenario.Context(machine_charm.MachineSubordinateRouterCharm) input_state = scenario.State( - relations=relations, + relations=[*relations, scenario.PeerRelation(endpoint="upgrade-version-a")], leader=True, ) - return context.run(event, input_state) + output = context.run(event, input_state) + output.relations.pop() # Remove PeerRelation + return output @pytest.mark.parametrize("complete_provides_s", combinations.complete_provides(1, 3)) diff --git a/tests/unit/scenario_/test_start.py b/tests/unit/scenario_/test_start.py index 5e013b28..bb8a3aae 100644 --- a/tests/unit/scenario_/test_start.py +++ b/tests/unit/scenario_/test_start.py @@ -11,7 +11,10 @@ @pytest.mark.parametrize("leader", [False, True]) def test_start_sets_status_if_no_relations(leader): context = scenario.Context(machine_charm.MachineSubordinateRouterCharm) - input_state = scenario.State(leader=leader) + input_state = scenario.State( + leader=leader, + relations=[scenario.PeerRelation(endpoint="upgrade-version-a")], + ) output_state = context.run("start", input_state) if leader: assert output_state.app_status == ops.BlockedStatus("Missing relation: backend-database") diff --git a/tox.ini b/tox.ini index a2424a83..8492c8d7 100644 --- a/tox.ini +++ b/tox.ini @@ -24,12 +24,20 @@ allowlist_externals = charmcraft mv commands_pre = + # TODO charm versioning: Remove + # Workaround to add unique identifier (git hash) to charm version while specification + # DA053 - Charm versioning + # (https://docs.google.com/document/d/1Jv1jhWLl8ejK3iJn7Q3VbCIM9GIhp8926bgXpdtx-Sg/edit?pli=1) + # is pending review. + python -c 'import pathlib; import shutil; import subprocess; git_hash=subprocess.run(["git", "describe", "--always", "--dirty"], capture_output=True, check=True, encoding="utf-8").stdout; file = pathlib.Path("charm_version"); shutil.copy(file, pathlib.Path("charm_version.backup")); version = file.read_text().strip(); file.write_text(f"{version}+{git_hash}")' + # `--without-hashes` workaround for https://github.com/canonical/charmcraft/issues/1179 poetry export --only main,charm-libs --output requirements.txt --without-hashes commands = build: charmcraft pack {posargs} commands_post = mv requirements.txt requirements-last-build.txt + mv charm_version.backup charm_version [testenv:format] description = Apply coding style standards to code diff --git a/workload_version b/workload_version new file mode 100644 index 00000000..ddfffe83 --- /dev/null +++ b/workload_version @@ -0,0 +1 @@ +8.0.34 From d233f6827fb2f10739d1d618ebf451a5d0b0f500 Mon Sep 17 00:00:00 2001 From: Carl Csaposs Date: Tue, 17 Oct 2023 09:39:16 +0000 Subject: [PATCH 02/24] Update deprecated_shared_db_database_provides.py --- .../deprecated_shared_db_database_provides.py | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/relations/deprecated_shared_db_database_provides.py b/src/relations/deprecated_shared_db_database_provides.py index b97385a6..a71a334c 100644 --- a/src/relations/deprecated_shared_db_database_provides.py +++ b/src/relations/deprecated_shared_db_database_provides.py @@ -194,14 +194,8 @@ def __init__(self, charm_: "abstract_charm.MySQLRouterCharm") -> None: logger.warning( "'mysql-shared' relation interface is DEPRECATED and will be removed in a future release. Use 'mysql_client' interface instead." ) - charm_.framework.observe( - charm_.on[self._NAME].relation_changed, - charm_.reconcile_database_relations, - ) - charm_.framework.observe( - charm_.on[self._NAME].relation_broken, - charm_.reconcile_database_relations, - ) + charm_.framework.observe(charm_.on[self._NAME].relation_changed, charm_.reconcile) + charm_.framework.observe(charm_.on[self._NAME].relation_broken, charm_.reconcile) self._charm = charm_ self.framework.observe( self._charm.on[self._CREDENTIALS_PEER_RELATION_ENDPOINT_NAME].relation_changed, From 3570ef5c7190578466827f812639704f0f09e320 Mon Sep 17 00:00:00 2001 From: Carl Csaposs Date: Mon, 23 Oct 2023 08:13:14 +0000 Subject: [PATCH 03/24] Sync files with K8s --- src/upgrade.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/upgrade.py b/src/upgrade.py index eb702ff1..12cc1d0f 100644 --- a/src/upgrade.py +++ b/src/upgrade.py @@ -215,7 +215,7 @@ def reconcile_partition(self, *, action_event: ops.ActionEvent = None) -> None: - confirm first upgraded unit is healthy and resume upgrade - force upgrade of next unit if 1 or more upgraded units are unhealthy """ - force = action_event and action_event.params.get("force") is True + force = bool(action_event and action_event.params.get("force") is True) units = self._sorted_units @@ -226,7 +226,7 @@ def determine_partition() -> int: for upgrade_order_index, unit in enumerate(units): # Note: upgrade_order_index != unit number if ( - force is False and self._peer_relation.data[unit].get("state") != "healthy" + not force and self._peer_relation.data[unit].get("state") != "healthy" ) or self._unit_workload_versions[unit.name] != self._app_workload_version: if not action_event and upgrade_order_index == 1: # User confirmation needed to resume upgrade (i.e. upgrade second unit) From 9a95a17d18b625a003669b6aebdf0ea6f2c907bc Mon Sep 17 00:00:00 2001 From: Carl Csaposs Date: Thu, 26 Oct 2023 13:01:13 +0000 Subject: [PATCH 04/24] Add in-place upgrades --- actions.yaml | 7 +- src/abstract_charm.py | 42 ++++++++++-- src/container.py | 9 +++ src/machine_charm.py | 44 +++++++++++++ src/machine_upgrade.py | 137 +++++++++++++++++++++++++++++++++++++++ src/snap.py | 36 +++++++---- src/upgrade.py | 144 ++++++++++++----------------------------- src/workload.py | 37 ++++++++--- 8 files changed, 323 insertions(+), 133 deletions(-) create mode 100644 src/machine_upgrade.py diff --git a/actions.yaml b/actions.yaml index 10dfc6c9..2f00cb65 100644 --- a/actions.yaml +++ b/actions.yaml @@ -3,8 +3,5 @@ resume-upgrade: description: Upgrade remaining units (after you manually verified that upgraded units are healthy). - params: - force: - type: boolean - description: Force upgrade of next unit if an upgraded unit has non-active status. - required: [] +force-upgrade: + description: Force upgrade of this unit. diff --git a/src/abstract_charm.py b/src/abstract_charm.py index 6c8720da..c64386e0 100644 --- a/src/abstract_charm.py +++ b/src/abstract_charm.py @@ -14,6 +14,7 @@ import container import lifecycle import logrotate +import machine_upgrade import relations.database_provides import relations.database_requires import upgrade @@ -51,6 +52,11 @@ def __init__(self, *args) -> None: self.framework.observe(self.on.start, self.reconcile) # Update app status self.framework.observe(self.on.leader_elected, self.reconcile) + # Set versions in upgrade peer relation app databag + self.framework.observe( + self.on[upgrade.PEER_RELATION_ENDPOINT_NAME].relation_created, + self._upgrade_relation_created, + ) @property @abc.abstractmethod @@ -138,10 +144,10 @@ def _determine_app_status(self, *, event) -> ops.StatusBase: def _determine_unit_status(self, *, event) -> ops.StatusBase: """Report unit status.""" statuses = [] - workload_ = self.get_workload(event=event) - statuses.append(workload_.status) + workload_status = self.get_workload(event=event).status if self._upgrade: - statuses.append(self._upgrade.unit_juju_status) + statuses.append(self._upgrade.get_unit_juju_status(workload_status=workload_status)) + statuses.append(workload_status) return self._prioritize_statuses(statuses) def set_status(self, *, event, app=True, unit=True) -> None: @@ -180,19 +186,41 @@ def wait_until_mysql_router_ready(self) -> None: # Handlers # ======================= + def _upgrade_relation_created(self, _) -> None: + if self._unit_lifecycle.authorized_leader: + # `self._upgrade.is_compatible` should return `True` during first charm + # installation/setup + self._upgrade.set_versions_in_app_databag() + def reconcile(self, event=None) -> None: # noqa: C901 """Handle most events.""" if not self._upgrade: logger.debug("Peer relation not available") return - if self._upgrade.unit_state == "restarting": + if not self._upgrade.versions_set: + logger.debug("Peer relation not ready") + return + workload_ = self.get_workload(event=event) + if self._upgrade.unit_state == "restarting": # Kubernetes only if not self._upgrade.is_compatible: self.unit.status = ops.BlockedStatus( "Upgrade incompatible. Rollback to previous revision with `juju refresh`" ) self.set_status(event=event, unit=False) return - workload_ = self.get_workload(event=event) + elif isinstance(self._upgrade, machine_upgrade.Upgrade): # Machines only + if not self._upgrade.is_compatible: + self.set_status(event=event) + return + if self._upgrade.unit_state == "outdated": + if self._upgrade.authorized: + self._upgrade.upgrade_unit( + workload_=workload_, tls=self._tls_certificate_saved + ) + else: + self.set_status(event=event) + logger.debug("Waiting to upgrade") + return logger.debug( "State of reconcile " f"{self._unit_lifecycle.authorized_leader=}, " @@ -223,7 +251,9 @@ def reconcile(self, event=None) -> None: # noqa: C901 workload_.enable(tls=self._tls_certificate_saved, unit_name=self.unit.name) elif workload_.container_ready: workload_.disable() - if not workload_.status: + # Empty waiting status means we're waiting for database requires relation before starting + # workload + if not workload_.status or workload_.status == ops.WaitingStatus(): self._upgrade.unit_state = "healthy" if self._unit_lifecycle.authorized_leader: self._upgrade.reconcile_partition() diff --git a/src/container.py b/src/container.py index 44f10445..3a144d7c 100644 --- a/src/container.py +++ b/src/container.py @@ -8,6 +8,8 @@ import subprocess import typing +import ops + class Path(pathlib.PurePosixPath, abc.ABC): """Workload container (snap or ROCK) filesystem path""" @@ -100,6 +102,13 @@ def update_mysql_router_service(self, *, enabled: bool, tls: bool = None) -> Non if enabled: assert tls is not None, "`tls` argument required when enabled=True" + @abc.abstractmethod + def upgrade(self, unit: ops.Unit) -> None: + """Upgrade container version + + Only applies to machine charm + """ + @abc.abstractmethod # TODO python3.10 min version: Use `list` instead of `typing.List` def _run_command(self, command: typing.List[str], *, timeout: typing.Optional[int]) -> str: diff --git a/src/machine_charm.py b/src/machine_charm.py index 4871053e..a7038ffb 100755 --- a/src/machine_charm.py +++ b/src/machine_charm.py @@ -13,9 +13,11 @@ import abstract_charm import machine_logrotate +import machine_upgrade import relations.database_providers_wrapper import snap import socket_workload +import upgrade logger = logging.getLogger(__name__) # TODO VM TLS: open ports for `juju expose` @@ -33,6 +35,8 @@ def __init__(self, *args) -> None: self._authenticated_workload_type = socket_workload.AuthenticatedSocketWorkload self.framework.observe(self.on.install, self._on_install) self.framework.observe(self.on.remove, self._on_remove) + self.framework.observe(self.on.upgrade_charm, self._on_upgrade_charm) + self.framework.observe(self.on["force-upgrade"].action, self._on_force_upgrade_action) @property def _subordinate_relation_endpoint_names(self) -> typing.Optional[typing.Iterable[str]]: @@ -45,6 +49,13 @@ def _subordinate_relation_endpoint_names(self) -> typing.Optional[typing.Iterabl def _container(self) -> snap.Snap: return snap.Snap() + @property + def _upgrade(self) -> typing.Optional[machine_upgrade.Upgrade]: + try: + return machine_upgrade.Upgrade(self) + except upgrade.PeerRelationNotReady: + pass + @property def _logrotate(self) -> machine_logrotate.LogRotate: return machine_logrotate.LogRotate(container_=self._container) @@ -68,6 +79,39 @@ def _on_install(self, _) -> None: def _on_remove(self, _) -> None: snap.uninstall() + def _on_upgrade_charm(self, _) -> None: + if self._unit_lifecycle.authorized_leader: + if not self._upgrade.in_progress: + logger.info("Charm upgraded. MySQL Router version unchanged") + self._upgrade.upgrade_resumed = False + # Only call `reconcile` on leader unit to avoid race conditions with `upgrade_resumed` + self.reconcile() + + def _on_force_upgrade_action(self, event: ops.ActionEvent) -> None: + if not self._upgrade or not self._upgrade.in_progress: + message = "No upgrade in progress" + logger.debug(f"Force upgrade event failed: {message}") + event.fail(message) + return + if not self._upgrade.upgrade_resumed: + message = f"Run `juju run {self.app.name}/leader resume-upgrade` before trying to force upgrade" + logger.debug(f"Force upgrade event failed: {message}") + event.fail(message) + return + if self._upgrade.unit_state != "outdated": + message = "Unit already upgraded" + logger.debug(f"Force upgrade event failed: {message}") + event.fail(message) + return + logger.debug("Forcing upgrade") + event.log(f"Forcefully upgrading {self.unit.name}") + self._upgrade.upgrade_unit( + workload_=self.get_workload(event=None), tls=self._tls_certificate_saved + ) + self.reconcile() + event.set_results({"result": f"Forcefully upgraded {self.unit.name}"}) + logger.debug("Forced upgrade") + if __name__ == "__main__": ops.main.main(MachineSubordinateRouterCharm) diff --git a/src/machine_upgrade.py b/src/machine_upgrade.py new file mode 100644 index 00000000..df89d17f --- /dev/null +++ b/src/machine_upgrade.py @@ -0,0 +1,137 @@ +# Copyright 2023 Canonical Ltd. +# See LICENSE file for licensing details. + +"""In-place upgrades on machines + +Derived from specification: DA058 - In-Place Upgrades - Kubernetes v2 +(https://docs.google.com/document/d/1tLjknwHudjcHs42nzPVBNkHs98XxAOT2BXGGpP7NyEU/) +""" +import json +import logging +import time +import typing + +import ops + +import snap +import upgrade +import workload + +logger = logging.getLogger(__name__) + + +class Upgrade(upgrade.Upgrade): + """In-place upgrades on machines""" + + @property + def unit_state(self) -> typing.Optional[str]: + if self._unit_workload_version != self._app_workload_version: + logger.debug("Unit upgrade state: outdated") + return "outdated" + return super().unit_state + + @unit_state.setter + def unit_state(self, value: str) -> None: + if value == "healthy": + # Set snap revision on first install + self._unit_databag["snap_revision"] = snap.REVISION + logger.debug(f"Saved {snap.REVISION=} in unit databag while setting state healthy") + # Super call + upgrade.Upgrade.unit_state.fset(self, value) + + def _get_unit_healthy_status( + self, *, workload_status: typing.Optional[ops.StatusBase] + ) -> ops.StatusBase: + if self._unit_workload_version == self._app_workload_version: + if isinstance(workload_status, ops.WaitingStatus): + return ops.WaitingStatus( + f'Router {self._current_versions["workload"]} rev {self._unit_workload_version}' + ) + return ops.ActiveStatus( + f'Router {self._current_versions["workload"]} rev {self._unit_workload_version} running' + ) + if isinstance(workload_status, ops.WaitingStatus): + return ops.WaitingStatus( + f'Charmed operator upgraded. Router {self._current_versions["workload"]} rev {self._unit_workload_version}' + ) + return ops.WaitingStatus( + f'Charmed operator upgraded. Router {self._current_versions["workload"]} rev {self._unit_workload_version} running' + ) + + @property + def app_status(self) -> typing.Optional[ops.StatusBase]: + if not self.is_compatible: + return ops.BlockedStatus( + "Upgrade incompatible. Rollback to previous revision with `juju refresh`" + ) + return super().app_status + + @property + def _unit_workload_versions(self) -> dict[str, str]: + """{Unit name: installed snap revision}""" + versions = {} + for unit in self._sorted_units: + if version := (self._peer_relation.data[unit].get("snap_revision")): + versions[unit.name] = version + return versions + + @property + def _unit_workload_version(self) -> typing.Optional[str]: + """Installed snap revision for this unit""" + return self._unit_databag.get("snap_revision") + + @property + def _app_workload_version(self) -> str: + """Snap revision for current charm code""" + return snap.REVISION + + def reconcile_partition(self, *, action_event: ops.ActionEvent = None) -> None: + """Handle Juju action to confirm first upgraded unit is healthy and resume upgrade.""" + if action_event: + self.upgrade_resumed = True + message = "Upgrade resumed." + action_event.set_results({"result": message}) + logger.debug(f"Resume upgrade event succeeded: {message}") + + @property + def upgrade_resumed(self) -> bool: + """Whether user has resumed upgrade with Juju action + + Reset to `False` after each `juju refresh` + """ + return json.loads(self._app_databag.get("upgrade-resumed", "false")) + + @upgrade_resumed.setter + def upgrade_resumed(self, value: bool): + # Trigger peer relation_changed event even if value does not change + # (Needed when leader sets value to False during `ops.UpgradeCharmEvent`) + self._app_databag["-unused-timestamp-upgrade-resume-last-updated"] = str(time.time()) + + self._app_databag["upgrade-resumed"] = json.dumps(value) + logger.debug(f"Set upgrade-resumed to {value=}") + + @property + def authorized(self) -> bool: + assert self._unit_workload_version != self._app_workload_version + for index, unit in enumerate(self._sorted_units): + if unit.name == self._unit.name: + # Higher number units have already upgraded + if index == 1: + # User confirmation needed to resume upgrade (i.e. upgrade second unit) + logger.debug(f"Second unit authorized to upgrade if {self.upgrade_resumed=}") + return self.upgrade_resumed + return True + if ( + self._unit_workload_versions.get(unit.name) != self._app_workload_version + or self._peer_relation.data[unit].get("state") != "healthy" + ): + # Waiting for higher number units to upgrade + return False + return False + + def upgrade_unit(self, *, workload_: workload.Workload, tls: bool) -> None: + logger.debug(f"Upgrading {self.authorized=}") + self.unit_state = "upgrading" + workload_.upgrade(unit=self._unit, tls=tls) + self._unit_databag["snap_revision"] = snap.REVISION + logger.debug(f"Saved {snap.REVISION=} in unit databag after upgrade") diff --git a/src/snap.py b/src/snap.py index 4e544c93..357e72c8 100644 --- a/src/snap.py +++ b/src/snap.py @@ -3,6 +3,7 @@ """Workload snap container & installer""" +import enum import logging import pathlib import shutil @@ -18,21 +19,22 @@ logger = logging.getLogger(__name__) _SNAP_NAME = "charmed-mysql" -_REVISION = "69" # v8.0.34 +REVISION = "69" # Keep in sync with `workload_version` file _snap = snap_lib.SnapCache()[_SNAP_NAME] _UNIX_USERNAME = "snap_daemon" -def install(*, unit: ops.Unit): - """Install snap.""" - if _snap.present: - logger.error(f"{_SNAP_NAME} snap already installed on machine. Installation aborted") - raise Exception(f"Multiple {_SNAP_NAME} snap installs not supported on one machine") - logger.debug(f"Installing {_SNAP_NAME=}, {_REVISION=}") - unit.status = ops.MaintenanceStatus("Installing snap") +class _RefreshVerb(str, enum.Enum): + INSTALL = "install" + UPGRADE = "upgrade" + + +def _refresh(*, unit: ops.Unit, verb: _RefreshVerb) -> None: + logger.debug(f'{verb.capitalize().removesuffix("e")}ing {_SNAP_NAME=}, {REVISION=}') + unit.status = ops.MaintenanceStatus(f'{verb.capitalize().removesuffix("e")}ing snap') def _set_retry_status(_) -> None: - message = "Snap install failed. Retrying..." + message = f"Snap {verb} failed. Retrying..." unit.status = ops.MaintenanceStatus(message) logger.debug(message) @@ -44,9 +46,17 @@ def _set_retry_status(_) -> None: reraise=True, ): with attempt: - _snap.ensure(state=snap_lib.SnapState.Present, revision=_REVISION) + _snap.ensure(state=snap_lib.SnapState.Present, revision=REVISION) _snap.hold() - logger.debug(f"Installed {_SNAP_NAME=}, {_REVISION=}") + logger.debug(f'{verb.capitalize().removesuffix("e")}ed {_SNAP_NAME=}, {REVISION=}') + + +def install(*, unit: ops.Unit): + """Install snap.""" + if _snap.present: + logger.error(f"{_SNAP_NAME} snap already installed on machine. Installation aborted") + raise Exception(f"Multiple {_SNAP_NAME} snap installs not supported on one machine") + _refresh(unit=unit, verb=_RefreshVerb.INSTALL) def uninstall(): @@ -144,6 +154,10 @@ def update_mysql_router_service(self, *, enabled: bool, tls: bool = None) -> Non else: _snap.stop([self._SERVICE_NAME], disable=True) + def upgrade(self, unit: ops.Unit) -> None: + """Upgrade snap.""" + _refresh(unit=unit, verb=_RefreshVerb.UPGRADE) + # TODO python3.10 min version: Use `list` instead of `typing.List` def _run_command(self, command: typing.List[str], *, timeout: typing.Optional[int]) -> str: try: diff --git a/src/upgrade.py b/src/upgrade.py index 12cc1d0f..ace87b67 100644 --- a/src/upgrade.py +++ b/src/upgrade.py @@ -16,6 +16,8 @@ import ops import poetry.core.constraints.version as poetry_version +import workload + logger = logging.getLogger(__name__) PEER_RELATION_ENDPOINT_NAME = "upgrade-version-a" @@ -23,6 +25,7 @@ def _unit_number(unit_: ops.Unit) -> int: + """Get unit number""" return int(unit_.name.split("/")[-1]) @@ -62,6 +65,7 @@ def unit_state(self, value: str) -> None: @property def is_compatible(self) -> bool: """Whether upgrade is supported from previous versions""" + assert self.versions_set try: previous_version_strs: dict[str, str] = json.loads(self._app_databag["versions"]) except KeyError as exception: @@ -118,32 +122,42 @@ def _sorted_units(self) -> list[ops.Unit]: """Units sorted from highest to lowest unit number""" return sorted((self._unit, *self._peer_relation.units), key=_unit_number, reverse=True) - @property - def _unit_active_status(self) -> ops.ActiveStatus: + @abc.abstractmethod + def _get_unit_healthy_status( + self, *, workload_status: typing.Optional[ops.StatusBase] + ) -> ops.StatusBase: """Status shown during upgrade if unit is healthy""" - return ops.ActiveStatus(self._current_versions["charm"]) - @property - def unit_juju_status(self) -> typing.Optional[ops.StatusBase]: + def get_unit_juju_status( + self, *, workload_status: typing.Optional[ops.StatusBase] + ) -> typing.Optional[ops.StatusBase]: if self.in_progress: - return self._unit_active_status + return self._get_unit_healthy_status(workload_status=workload_status) @property def app_status(self) -> typing.Optional[ops.StatusBase]: if self.in_progress: - if len(self._sorted_units) >= 2 and self._partition > _unit_number( - self._sorted_units[1] - ): + if self.upgrade_resumed: + return ops.MaintenanceStatus( + "Upgrading. To rollback, `juju refresh` to the previous revision" + ) + else: # User confirmation needed to resume upgrade (i.e. upgrade second unit) # Statuses over 120 characters are truncated in `juju status` as of juju 3.1.6 and # 2.9.45 return ops.BlockedStatus( f"Upgrading. Verify highest unit is healthy & run `{RESUME_ACTION_NAME}` action. To rollback, `juju refresh` to last revision" ) - else: - return ops.MaintenanceStatus( - "Upgrading. To rollback, `juju refresh` to the previous revision" - ) + + @property + def versions_set(self) -> bool: + """Whether versions have been saved in app databag + + Should only be `False` during first charm install + + If a user upgrades from a charm that does not set versions, this charm will get stuck. + """ + return self._app_databag.get("versions") is not None def set_versions_in_app_databag(self) -> None: """Save current versions in app databag @@ -158,23 +172,8 @@ def set_versions_in_app_databag(self) -> None: @property @abc.abstractmethod - def _partition(self) -> int: - """Specifies which units should upgrade - - Unit numbers >= partition should upgrade - Unit numbers < partition should not upgrade - - Based on Kubernetes StatefulSet partition - (https://kubernetes.io/docs/concepts/workloads/controllers/statefulset/#partitions) - - For Kubernetes, unit numbers are guaranteed to be sequential - For machines, unit numbers are not guaranteed to be sequential - """ - - @_partition.setter - @abc.abstractmethod - def _partition(self, value: int) -> None: - pass + def upgrade_resumed(self) -> bool: + """Whether user has resumed upgrade with Juju action""" @property @abc.abstractmethod @@ -202,80 +201,21 @@ def _app_workload_version(self) -> str: app & unit are the same workload version. """ + @abc.abstractmethod def reconcile_partition(self, *, action_event: ops.ActionEvent = None) -> None: - """If ready, lower partition to upgrade next unit. + """If ready, allow next unit to upgrade.""" - If upgrade is not in progress, set partition to 0. (On Kubernetes, if a unit receives a - stop event, it may raise the partition even if an upgrade is not in progress.) + @property + @abc.abstractmethod + def authorized(self) -> bool: + """Whether this unit is authorized to upgrade + + Only applies to machine charm + """ - Automatically upgrades next unit if all upgraded units are healthy—except if only one unit - has upgraded (need manual user confirmation [via Juju action] to upgrade next unit) + @abc.abstractmethod + def upgrade_unit(self, *, workload_: workload.Workload, tls: bool) -> None: + """Upgrade this unit. - Handle Juju action to: - - confirm first upgraded unit is healthy and resume upgrade - - force upgrade of next unit if 1 or more upgraded units are unhealthy + Only applies to machine charm """ - force = bool(action_event and action_event.params.get("force") is True) - - units = self._sorted_units - - def determine_partition() -> int: - if not self.in_progress: - return 0 - logger.debug(f"{self._peer_relation.data=}") - for upgrade_order_index, unit in enumerate(units): - # Note: upgrade_order_index != unit number - if ( - not force and self._peer_relation.data[unit].get("state") != "healthy" - ) or self._unit_workload_versions[unit.name] != self._app_workload_version: - if not action_event and upgrade_order_index == 1: - # User confirmation needed to resume upgrade (i.e. upgrade second unit) - return _unit_number(units[0]) - return _unit_number(unit) - return 0 - - partition = determine_partition() - logger.debug(f"{self._partition=}, {partition=}") - # Only lower the partition—do not raise it. - # If this method is called during the action event and then called during another event a - # few seconds later, `determine_partition()` could return a lower number during the action - # and then a higher number a few seconds later. - - # On machines, this could cause a unit to not upgrade & require that the action is run - # again. (e.g. if an update-status event fires immediately after the action and before the - # would-be upgrading unit receives a relation-changed event.) - - # On Kubernetes, this can cause the unit to hang. - # Example: If partition is lowered to 1, unit 1 begins to upgrade, and partition is set to - # 2 right away, the unit/Juju agent will hang - # Details: https://chat.charmhub.io/charmhub/pl/on8rd538ufn4idgod139skkbfr - # This does not address the situation where another unit > 1 restarts and sets the - # partition during the `stop` event, but that is unlikely to occur in the small time window - # that causes the unit to hang. - if partition < self._partition: - self._partition = partition - logger.debug( - f"Lowered partition to {partition} {action_event=} {force=} {self.in_progress=}" - ) - if action_event: - assert len(units) >= 2 - if self._partition > _unit_number(units[1]): - message = "Highest number unit is unhealthy. Upgrade will not resume." - logger.debug(f"Resume upgrade event failed: {message}") - action_event.fail(message) - return - if force: - # If a unit was unhealthy and the upgrade was forced, only the next unit will - # upgrade. As long as 1 or more units are unhealthy, the upgrade will need to be - # forced for each unit. - - # Include "Attempting to" because (on Kubernetes) we only control the partition, - # not which units upgrade. Kubernetes may not upgrade a unit even if the partition - # allows it (e.g. if the charm container of a higher unit is not ready). This is - # also applicable `if not force`, but is unlikely to happen since all units are - # "healthy" `if not force`. - message = f"Attempting to upgrade unit {self._partition}" - else: - message = f"Upgrade resumed. Unit {self._partition} is upgrading next" - action_event.set_results({"result": message}) - logger.debug(f"Resume upgrade event succeeded: {message}") diff --git a/src/workload.py b/src/workload.py index 828f2762..3bdb1913 100644 --- a/src/workload.py +++ b/src/workload.py @@ -67,6 +67,15 @@ def disable(self) -> None: self._router_data_directory.mkdir() logger.debug("Disabled MySQL Router service") + def upgrade(self, *, unit: ops.Unit, tls: bool) -> None: + """Upgrade MySQL Router. + + Only applies to machine charm + """ + logger.debug("Upgrading MySQL Router") + self._container.upgrade(unit=unit) + logger.debug("Upgraded MySQL Router") + @property def _tls_config_file_data(self) -> str: """Render config file template to string. @@ -104,6 +113,8 @@ def status(self) -> typing.Optional[ops.StatusBase]: """Report non-active status.""" if not self.container_ready: return ops.MaintenanceStatus("Waiting for container") + if not self._container.mysql_router_service_enabled: + return ops.WaitingStatus() class AuthenticatedWorkload(Workload): @@ -141,19 +152,17 @@ def _router_id(self) -> str: # MySQL Router is bootstrapped without `--directory`—there is one system-wide instance. return f"{socket.getfqdn()}::system" - def _cleanup_after_potential_container_restart(self) -> None: - """Remove MySQL Router cluster metadata & user after (potential) container restart. + def _cleanup_after_upgrade_or_potential_container_restart(self) -> None: + """Remove Router cluster metadata & user after upgrade or (potential) container restart. - Only applies to Kubernetes charm - - (Storage is not persisted on container restart—MySQL Router's config file is deleted. - Therefore, MySQL Router needs to be bootstrapped again.) + (On Kubernetes, storage is not persisted on container restart—MySQL Router's config file is + deleted. Therefore, MySQL Router needs to be bootstrapped again.) """ if user_info := self.shell.get_mysql_router_user_for_unit(self._charm.unit.name): - logger.debug("Cleaning up after container restart") + logger.debug("Cleaning up after upgrade or container restart") self.shell.remove_router_from_cluster_metadata(user_info.router_id) self.shell.delete_user(user_info.username) - logger.debug("Cleaned up after container restart") + logger.debug("Cleaned up after upgrade or container restart") # TODO python3.10 min version: Use `list` instead of `typing.List` def _get_bootstrap_command(self, password: str) -> typing.List[str]: @@ -218,7 +227,7 @@ def enable(self, *, tls: bool, unit_name: str) -> None: # Therefore, if the host or port changes, we do not need to restart MySQL Router. return logger.debug("Enabling MySQL Router service") - self._cleanup_after_potential_container_restart() + self._cleanup_after_upgrade_or_potential_container_restart() self._bootstrap_router(tls=tls) self.shell.add_attributes_to_mysql_router_user( username=self._router_username, router_id=self._router_id, unit_name=unit_name @@ -264,3 +273,13 @@ def status(self) -> typing.Optional[ops.StatusBase]: return ops.BlockedStatus( "Router was manually removed from MySQL ClusterSet. Remove & re-deploy unit" ) + + def upgrade(self, *, unit: ops.Unit, tls: bool) -> None: + enabled = self._container.mysql_router_service_enabled + if enabled: + logger.debug("Disabling MySQL Router service before upgrade") + self.disable() + super().upgrade(unit=unit, tls=tls) + if enabled: + logger.debug("Re-enabling MySQL Router service after upgrade") + self.enable(tls=tls, unit_name=unit.name) From 4b62b741dcba2995e5048172f449ba46da7cbb7b Mon Sep 17 00:00:00 2001 From: Carl Csaposs Date: Mon, 30 Oct 2023 07:56:38 +0000 Subject: [PATCH 05/24] Fix unit tests --- tests/unit/conftest.py | 5 +++++ tests/unit/scenario_/test_start.py | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index 7cc51e38..6895d8aa 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -35,6 +35,8 @@ def patch(monkeypatch): monkeypatch.setattr("workload.AuthenticatedWorkload._router_username", "") monkeypatch.setattr("mysql_shell.Shell.is_router_in_cluster_set", lambda *args, **kwargs: True) monkeypatch.setattr("upgrade.Upgrade.in_progress", False) + monkeypatch.setattr("upgrade.Upgrade.versions_set", True) + monkeypatch.setattr("upgrade.Upgrade.is_compatible", True) @pytest.fixture(autouse=True) @@ -50,6 +52,9 @@ def __init__(self): def ensure(self, *_, **__): return + def hold(self, *_, **__): + return + def start(self, services: list[str] = None, *_, **__): assert services == ["mysqlrouter-service"] self.services["mysqlrouter-service"]["active"] = True diff --git a/tests/unit/scenario_/test_start.py b/tests/unit/scenario_/test_start.py index bb8a3aae..2aaafadb 100644 --- a/tests/unit/scenario_/test_start.py +++ b/tests/unit/scenario_/test_start.py @@ -18,4 +18,4 @@ def test_start_sets_status_if_no_relations(leader): output_state = context.run("start", input_state) if leader: assert output_state.app_status == ops.BlockedStatus("Missing relation: backend-database") - assert output_state.unit_status == ops.ActiveStatus() + assert output_state.unit_status == ops.WaitingStatus() From 7d5b2a3d105e450274dc8e84fec5c994953c35aa Mon Sep 17 00:00:00 2001 From: Carl Csaposs Date: Mon, 30 Oct 2023 08:12:12 +0000 Subject: [PATCH 06/24] remove extraneous docstring --- src/upgrade.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/upgrade.py b/src/upgrade.py index ace87b67..1995abeb 100644 --- a/src/upgrade.py +++ b/src/upgrade.py @@ -25,7 +25,6 @@ def _unit_number(unit_: ops.Unit) -> int: - """Get unit number""" return int(unit_.name.split("/")[-1]) From 4e3708aafdf4011328e6ac64050ba7c28a745295 Mon Sep 17 00:00:00 2001 From: Carl Csaposs Date: Mon, 30 Oct 2023 08:53:31 +0000 Subject: [PATCH 07/24] sync with changes for k8s --- src/upgrade.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/upgrade.py b/src/upgrade.py index 1995abeb..b7381aea 100644 --- a/src/upgrade.py +++ b/src/upgrade.py @@ -24,7 +24,8 @@ RESUME_ACTION_NAME = "resume-upgrade" -def _unit_number(unit_: ops.Unit) -> int: +def unit_number(unit_: ops.Unit) -> int: + """Get unit number""" return int(unit_.name.split("/")[-1]) @@ -119,7 +120,7 @@ def in_progress(self) -> bool: @property def _sorted_units(self) -> list[ops.Unit]: """Units sorted from highest to lowest unit number""" - return sorted((self._unit, *self._peer_relation.units), key=_unit_number, reverse=True) + return sorted((self._unit, *self._peer_relation.units), key=unit_number, reverse=True) @abc.abstractmethod def _get_unit_healthy_status( From 590e21ed66398f331b844840190d921db85c0138 Mon Sep 17 00:00:00 2001 From: Carl Csaposs Date: Mon, 30 Oct 2023 10:47:51 +0000 Subject: [PATCH 08/24] Maintenance status ported from https://github.com/canonical/mysql-router-k8s-operator/pull/147 --- src/abstract_charm.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/abstract_charm.py b/src/abstract_charm.py index c64386e0..84ba06fd 100644 --- a/src/abstract_charm.py +++ b/src/abstract_charm.py @@ -165,7 +165,7 @@ def wait_until_mysql_router_ready(self) -> None: Retry every 5 seconds for up to 30 seconds. """ logger.debug("Waiting until MySQL Router is ready") - self.unit.status = ops.WaitingStatus("MySQL Router starting") + self.unit.status = ops.MaintenanceStatus("MySQL Router starting") try: for attempt in tenacity.Retrying( reraise=True, From 7e5335cff0eefca8c4c8a7bd69df37b693f8e9dc Mon Sep 17 00:00:00 2001 From: Carl Csaposs Date: Mon, 30 Oct 2023 13:06:40 +0000 Subject: [PATCH 09/24] Reduce nested if --- src/upgrade.py | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/upgrade.py b/src/upgrade.py index b7381aea..165f86e5 100644 --- a/src/upgrade.py +++ b/src/upgrade.py @@ -136,18 +136,18 @@ def get_unit_juju_status( @property def app_status(self) -> typing.Optional[ops.StatusBase]: - if self.in_progress: - if self.upgrade_resumed: - return ops.MaintenanceStatus( - "Upgrading. To rollback, `juju refresh` to the previous revision" - ) - else: - # User confirmation needed to resume upgrade (i.e. upgrade second unit) - # Statuses over 120 characters are truncated in `juju status` as of juju 3.1.6 and - # 2.9.45 - return ops.BlockedStatus( - f"Upgrading. Verify highest unit is healthy & run `{RESUME_ACTION_NAME}` action. To rollback, `juju refresh` to last revision" - ) + if not self.in_progress: + return + if not self.upgrade_resumed: + # User confirmation needed to resume upgrade (i.e. upgrade second unit) + # Statuses over 120 characters are truncated in `juju status` as of juju 3.1.6 and + # 2.9.45 + return ops.BlockedStatus( + f"Upgrading. Verify highest unit is healthy & run `{RESUME_ACTION_NAME}` action. To rollback, `juju refresh` to last revision" + ) + return ops.MaintenanceStatus( + "Upgrading. To rollback, `juju refresh` to the previous revision" + ) @property def versions_set(self) -> bool: From cacf3b50bc9bd63de90b25e3f4c4dc98518d7cf8 Mon Sep 17 00:00:00 2001 From: Carl Csaposs Date: Mon, 30 Oct 2023 13:39:43 +0000 Subject: [PATCH 10/24] python3.8 fix --- src/upgrade.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/upgrade.py b/src/upgrade.py index 165f86e5..2781eff8 100644 --- a/src/upgrade.py +++ b/src/upgrade.py @@ -118,7 +118,7 @@ def in_progress(self) -> bool: ) @property - def _sorted_units(self) -> list[ops.Unit]: + def _sorted_units(self) -> typing.List[ops.Unit]: """Units sorted from highest to lowest unit number""" return sorted((self._unit, *self._peer_relation.units), key=unit_number, reverse=True) From 6766eedd0c58210b4131c5cdab18e68b8d063602 Mon Sep 17 00:00:00 2001 From: Carl Csaposs Date: Tue, 31 Oct 2023 12:42:04 +0000 Subject: [PATCH 11/24] python3.8 fix --- poetry.lock | 20 -------------------- src/machine_upgrade.py | 2 +- src/upgrade.py | 8 +++++--- 3 files changed, 6 insertions(+), 24 deletions(-) diff --git a/poetry.lock b/poetry.lock index 91909497..a1bb1947 100644 --- a/poetry.lock +++ b/poetry.lock @@ -835,16 +835,6 @@ files = [ {file = "MarkupSafe-2.1.3-cp311-cp311-musllinux_1_1_x86_64.whl", hash = "sha256:5bbe06f8eeafd38e5d0a4894ffec89378b6c6a625ff57e3028921f8ff59318ac"}, {file = "MarkupSafe-2.1.3-cp311-cp311-win32.whl", hash = "sha256:dd15ff04ffd7e05ffcb7fe79f1b98041b8ea30ae9234aed2a9168b5797c3effb"}, {file = "MarkupSafe-2.1.3-cp311-cp311-win_amd64.whl", hash = "sha256:134da1eca9ec0ae528110ccc9e48041e0828d79f24121a1a146161103c76e686"}, - {file = "MarkupSafe-2.1.3-cp312-cp312-macosx_10_9_universal2.whl", hash = "sha256:f698de3fd0c4e6972b92290a45bd9b1536bffe8c6759c62471efaa8acb4c37bc"}, - {file = "MarkupSafe-2.1.3-cp312-cp312-macosx_10_9_x86_64.whl", hash = "sha256:aa57bd9cf8ae831a362185ee444e15a93ecb2e344c8e52e4d721ea3ab6ef1823"}, - {file = "MarkupSafe-2.1.3-cp312-cp312-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:ffcc3f7c66b5f5b7931a5aa68fc9cecc51e685ef90282f4a82f0f5e9b704ad11"}, - {file = "MarkupSafe-2.1.3-cp312-cp312-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:47d4f1c5f80fc62fdd7777d0d40a2e9dda0a05883ab11374334f6c4de38adffd"}, - {file = "MarkupSafe-2.1.3-cp312-cp312-manylinux_2_5_i686.manylinux1_i686.manylinux_2_17_i686.manylinux2014_i686.whl", hash = "sha256:1f67c7038d560d92149c060157d623c542173016c4babc0c1913cca0564b9939"}, - {file = "MarkupSafe-2.1.3-cp312-cp312-musllinux_1_1_aarch64.whl", hash = "sha256:9aad3c1755095ce347e26488214ef77e0485a3c34a50c5a5e2471dff60b9dd9c"}, - {file = "MarkupSafe-2.1.3-cp312-cp312-musllinux_1_1_i686.whl", hash = "sha256:14ff806850827afd6b07a5f32bd917fb7f45b046ba40c57abdb636674a8b559c"}, - {file = "MarkupSafe-2.1.3-cp312-cp312-musllinux_1_1_x86_64.whl", hash = "sha256:8f9293864fe09b8149f0cc42ce56e3f0e54de883a9de90cd427f191c346eb2e1"}, - {file = "MarkupSafe-2.1.3-cp312-cp312-win32.whl", hash = "sha256:715d3562f79d540f251b99ebd6d8baa547118974341db04f5ad06d5ea3eb8007"}, - {file = "MarkupSafe-2.1.3-cp312-cp312-win_amd64.whl", hash = "sha256:1b8dd8c3fd14349433c79fa8abeb573a55fc0fdd769133baac1f5e07abf54aeb"}, {file = "MarkupSafe-2.1.3-cp37-cp37m-macosx_10_9_x86_64.whl", hash = "sha256:8e254ae696c88d98da6555f5ace2279cf7cd5b3f52be2b5cf97feafe883b58d2"}, {file = "MarkupSafe-2.1.3-cp37-cp37m-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:cb0932dc158471523c9637e807d9bfb93e06a95cbf010f1a38b98623b929ef2b"}, {file = "MarkupSafe-2.1.3-cp37-cp37m-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:9402b03f1a1b4dc4c19845e5c749e3ab82d5078d16a2a4c2cd2df62d57bb0707"}, @@ -1543,7 +1533,6 @@ files = [ {file = "PyYAML-6.0.1-cp310-cp310-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:69b023b2b4daa7548bcfbd4aa3da05b3a74b772db9e23b982788168117739938"}, {file = "PyYAML-6.0.1-cp310-cp310-manylinux_2_17_s390x.manylinux2014_s390x.whl", hash = "sha256:81e0b275a9ecc9c0c0c07b4b90ba548307583c125f54d5b6946cfee6360c733d"}, {file = "PyYAML-6.0.1-cp310-cp310-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:ba336e390cd8e4d1739f42dfe9bb83a3cc2e80f567d8805e11b46f4a943f5515"}, - {file = "PyYAML-6.0.1-cp310-cp310-musllinux_1_1_x86_64.whl", hash = "sha256:326c013efe8048858a6d312ddd31d56e468118ad4cdeda36c719bf5bb6192290"}, {file = "PyYAML-6.0.1-cp310-cp310-win32.whl", hash = "sha256:bd4af7373a854424dabd882decdc5579653d7868b8fb26dc7d0e99f823aa5924"}, {file = "PyYAML-6.0.1-cp310-cp310-win_amd64.whl", hash = "sha256:fd1592b3fdf65fff2ad0004b5e363300ef59ced41c2e6b3a99d4089fa8c5435d"}, {file = "PyYAML-6.0.1-cp311-cp311-macosx_10_9_x86_64.whl", hash = "sha256:6965a7bc3cf88e5a1c3bd2e0b5c22f8d677dc88a455344035f03399034eb3007"}, @@ -1551,15 +1540,8 @@ files = [ {file = "PyYAML-6.0.1-cp311-cp311-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:42f8152b8dbc4fe7d96729ec2b99c7097d656dc1213a3229ca5383f973a5ed6d"}, {file = "PyYAML-6.0.1-cp311-cp311-manylinux_2_17_s390x.manylinux2014_s390x.whl", hash = "sha256:062582fca9fabdd2c8b54a3ef1c978d786e0f6b3a1510e0ac93ef59e0ddae2bc"}, {file = "PyYAML-6.0.1-cp311-cp311-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:d2b04aac4d386b172d5b9692e2d2da8de7bfb6c387fa4f801fbf6fb2e6ba4673"}, - {file = "PyYAML-6.0.1-cp311-cp311-musllinux_1_1_x86_64.whl", hash = "sha256:e7d73685e87afe9f3b36c799222440d6cf362062f78be1013661b00c5c6f678b"}, {file = "PyYAML-6.0.1-cp311-cp311-win32.whl", hash = "sha256:1635fd110e8d85d55237ab316b5b011de701ea0f29d07611174a1b42f1444741"}, {file = "PyYAML-6.0.1-cp311-cp311-win_amd64.whl", hash = "sha256:bf07ee2fef7014951eeb99f56f39c9bb4af143d8aa3c21b1677805985307da34"}, - {file = "PyYAML-6.0.1-cp312-cp312-macosx_10_9_x86_64.whl", hash = "sha256:855fb52b0dc35af121542a76b9a84f8d1cd886ea97c84703eaa6d88e37a2ad28"}, - {file = "PyYAML-6.0.1-cp312-cp312-macosx_11_0_arm64.whl", hash = "sha256:40df9b996c2b73138957fe23a16a4f0ba614f4c0efce1e9406a184b6d07fa3a9"}, - {file = "PyYAML-6.0.1-cp312-cp312-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:6c22bec3fbe2524cde73d7ada88f6566758a8f7227bfbf93a408a9d86bcc12a0"}, - {file = "PyYAML-6.0.1-cp312-cp312-musllinux_1_1_x86_64.whl", hash = "sha256:8d4e9c88387b0f5c7d5f281e55304de64cf7f9c0021a3525bd3b1c542da3b0e4"}, - {file = "PyYAML-6.0.1-cp312-cp312-win32.whl", hash = "sha256:d483d2cdf104e7c9fa60c544d92981f12ad66a457afae824d146093b8c294c54"}, - {file = "PyYAML-6.0.1-cp312-cp312-win_amd64.whl", hash = "sha256:0d3304d8c0adc42be59c5f8a4d9e3d7379e6955ad754aa9d6ab7a398b59dd1df"}, {file = "PyYAML-6.0.1-cp36-cp36m-macosx_10_9_x86_64.whl", hash = "sha256:50550eb667afee136e9a77d6dc71ae76a44df8b3e51e41b77f6de2932bfe0f47"}, {file = "PyYAML-6.0.1-cp36-cp36m-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:1fe35611261b29bd1de0070f0b2f47cb6ff71fa6595c077e42bd0c419fa27b98"}, {file = "PyYAML-6.0.1-cp36-cp36m-manylinux_2_17_s390x.manylinux2014_s390x.whl", hash = "sha256:704219a11b772aea0d8ecd7058d0082713c3562b4e271b849ad7dc4a5c90c13c"}, @@ -1576,7 +1558,6 @@ files = [ {file = "PyYAML-6.0.1-cp38-cp38-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:a0cd17c15d3bb3fa06978b4e8958dcdc6e0174ccea823003a106c7d4d7899ac5"}, {file = "PyYAML-6.0.1-cp38-cp38-manylinux_2_17_s390x.manylinux2014_s390x.whl", hash = "sha256:28c119d996beec18c05208a8bd78cbe4007878c6dd15091efb73a30e90539696"}, {file = "PyYAML-6.0.1-cp38-cp38-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:7e07cbde391ba96ab58e532ff4803f79c4129397514e1413a7dc761ccd755735"}, - {file = "PyYAML-6.0.1-cp38-cp38-musllinux_1_1_x86_64.whl", hash = "sha256:49a183be227561de579b4a36efbb21b3eab9651dd81b1858589f796549873dd6"}, {file = "PyYAML-6.0.1-cp38-cp38-win32.whl", hash = "sha256:184c5108a2aca3c5b3d3bf9395d50893a7ab82a38004c8f61c258d4428e80206"}, {file = "PyYAML-6.0.1-cp38-cp38-win_amd64.whl", hash = "sha256:1e2722cc9fbb45d9b87631ac70924c11d3a401b2d7f410cc0e3bbf249f2dca62"}, {file = "PyYAML-6.0.1-cp39-cp39-macosx_10_9_x86_64.whl", hash = "sha256:9eb6caa9a297fc2c2fb8862bc5370d0303ddba53ba97e71f08023b6cd73d16a8"}, @@ -1584,7 +1565,6 @@ files = [ {file = "PyYAML-6.0.1-cp39-cp39-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:5773183b6446b2c99bb77e77595dd486303b4faab2b086e7b17bc6bef28865f6"}, {file = "PyYAML-6.0.1-cp39-cp39-manylinux_2_17_s390x.manylinux2014_s390x.whl", hash = "sha256:b786eecbdf8499b9ca1d697215862083bd6d2a99965554781d0d8d1ad31e13a0"}, {file = "PyYAML-6.0.1-cp39-cp39-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:bc1bf2925a1ecd43da378f4db9e4f799775d6367bdb94671027b73b393a7c42c"}, - {file = "PyYAML-6.0.1-cp39-cp39-musllinux_1_1_x86_64.whl", hash = "sha256:04ac92ad1925b2cff1db0cfebffb6ffc43457495c9b3c39d3fcae417d7125dc5"}, {file = "PyYAML-6.0.1-cp39-cp39-win32.whl", hash = "sha256:faca3bdcf85b2fc05d06ff3fbc1f83e1391b3e724afa3feba7d13eeab355484c"}, {file = "PyYAML-6.0.1-cp39-cp39-win_amd64.whl", hash = "sha256:510c9deebc5c0225e8c96813043e62b680ba2f9c50a08d3724c7f28a747d1486"}, {file = "PyYAML-6.0.1.tar.gz", hash = "sha256:bfdf460b1736c775f2ba9f6a92bca30bc2095067b8a9d77876d1fad6cc3b4a43"}, diff --git a/src/machine_upgrade.py b/src/machine_upgrade.py index df89d17f..0a3cb872 100644 --- a/src/machine_upgrade.py +++ b/src/machine_upgrade.py @@ -67,7 +67,7 @@ def app_status(self) -> typing.Optional[ops.StatusBase]: return super().app_status @property - def _unit_workload_versions(self) -> dict[str, str]: + def _unit_workload_versions(self) -> typing.Dict[str, str]: """{Unit name: installed snap revision}""" versions = {} for unit in self._sorted_units: diff --git a/src/upgrade.py b/src/upgrade.py index 2781eff8..d5afb573 100644 --- a/src/upgrade.py +++ b/src/upgrade.py @@ -67,11 +67,13 @@ def is_compatible(self) -> bool: """Whether upgrade is supported from previous versions""" assert self.versions_set try: - previous_version_strs: dict[str, str] = json.loads(self._app_databag["versions"]) + previous_version_strs: typing.Dict[str, str] = json.loads( + self._app_databag["versions"] + ) except KeyError as exception: logger.debug("`versions` missing from peer relation", exc_info=exception) return False - previous_versions: dict[str, poetry_version.Version] = { + previous_versions: typing.Dict[str, poetry_version.Version] = { key: poetry_version.Version.parse(value) for key, value in previous_version_strs.items() } @@ -177,7 +179,7 @@ def upgrade_resumed(self) -> bool: @property @abc.abstractmethod - def _unit_workload_versions(self) -> dict[str, str]: + def _unit_workload_versions(self) -> typing.Dict[str, str]: """{Unit name: unique identifier for unit's workload version} If and only if this version changes, the workload will restart (during upgrade or From fb9464f5dd22ef92b040d35dd027f4fccd607a75 Mon Sep 17 00:00:00 2001 From: Carl Csaposs Date: Tue, 31 Oct 2023 13:55:01 +0000 Subject: [PATCH 12/24] python3.8 fix --- src/snap.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/snap.py b/src/snap.py index 357e72c8..5a9cc1c5 100644 --- a/src/snap.py +++ b/src/snap.py @@ -30,8 +30,9 @@ class _RefreshVerb(str, enum.Enum): def _refresh(*, unit: ops.Unit, verb: _RefreshVerb) -> None: - logger.debug(f'{verb.capitalize().removesuffix("e")}ing {_SNAP_NAME=}, {REVISION=}') - unit.status = ops.MaintenanceStatus(f'{verb.capitalize().removesuffix("e")}ing snap') + # TODO python3.10 min version: use `removesuffix` instead of `rstrip` + logger.debug(f'{verb.capitalize().rstrip("e")}ing {_SNAP_NAME=}, {REVISION=}') + unit.status = ops.MaintenanceStatus(f'{verb.capitalize().rstrip("e")}ing snap') def _set_retry_status(_) -> None: message = f"Snap {verb} failed. Retrying..." @@ -48,7 +49,7 @@ def _set_retry_status(_) -> None: with attempt: _snap.ensure(state=snap_lib.SnapState.Present, revision=REVISION) _snap.hold() - logger.debug(f'{verb.capitalize().removesuffix("e")}ed {_SNAP_NAME=}, {REVISION=}') + logger.debug(f'{verb.capitalize().rstrip("e")}ed {_SNAP_NAME=}, {REVISION=}') def install(*, unit: ops.Unit): From 6ae281a6369af84624aadaabe82a8cbbb5ced015 Mon Sep 17 00:00:00 2001 From: Judit Novak Date: Tue, 31 Oct 2023 11:13:29 +0100 Subject: [PATCH 13/24] Additional logs on pipeline for debugging --- .github/workflows/ci.yaml | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 6f6c52dd..5bbdc95e 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -142,5 +142,25 @@ jobs: echo Skipping unstable tests echo "mark_expression=not unstable" >> "$GITHUB_OUTPUT" fi + - run: juju add-model test - name: Run integration tests - run: tox run -e integration -- "${{ matrix.groups.path_to_test_file }}" --group="${{ matrix.groups.group_number }}" -m '${{ steps.select-test-stability.outputs.mark_expression }}' --mysql-router-charm-series=${{ matrix.ubuntu-versions.series }} --mysql-router-charm-bases-index=${{ matrix.ubuntu-versions.bases-index }} + id: tests + run: tox run -e integration -- "${{ matrix.groups.path_to_test_file }}" --group="${{ matrix.groups.group_number }}" -m '${{ steps.select-test-stability.outputs.mark_expression }}' --mysql-router-charm-series=${{ matrix.ubuntu-versions.series }} --mysql-router-charm-bases-index=${{ matrix.ubuntu-versions.bases-index }} --model test + - name: Select model + if: ${{ success() || (failure() && steps.tests.outcome == 'failure') }} + run: | + juju switch test + mkdir -p ~/logs/'${{ matrix.groups.log_artifact_path }}' + - name: juju status + if: ${{ success() || (failure() && steps.tests.outcome == 'failure') }} + run: juju status --color --relations | tee ~/logs/'${{ matrix.groups.log_artifact_path }}'/juju-status.txt + - name: juju debug-log + if: ${{ success() || (failure() && steps.tests.outcome == 'failure') }} + run: juju debug-log --color --replay --no-tail | tee ~/logs/'${{ matrix.groups.log_artifact_path }}'/juju-debug-log.txt + - name: Upload logs + if: ${{ success() || (failure() && steps.tests.outcome == 'failure') }} + uses: actions/upload-artifact@v3 + with: + name: intergration-test-charm-logs-${{ inputs.cloud }}-juju-${{ inputs.juju-agent-version || inputs.juju-snap-channel }} + path: ~/logs/ + if-no-files-found: error From 90f14df00aed5118e21bd1051c735c0ae11921af Mon Sep 17 00:00:00 2001 From: Judit Novak Date: Wed, 18 Oct 2023 20:40:48 +0200 Subject: [PATCH 14/24] pyproject changes: pytest-mock, ops, etc. for tests --- poetry.lock | 19 ++++++++++++++++++- pyproject.toml | 4 ++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/poetry.lock b/poetry.lock index a1bb1947..e5f8e525 100644 --- a/poetry.lock +++ b/poetry.lock @@ -1419,6 +1419,23 @@ pytest = ">=4.6" [package.extras] testing = ["fields", "hunter", "process-tests", "pytest-xdist", "six", "virtualenv"] +[[package]] +name = "pytest-mock" +version = "3.11.1" +description = "Thin-wrapper around the mock package for easier use with pytest" +optional = false +python-versions = ">=3.7" +files = [ + {file = "pytest-mock-3.11.1.tar.gz", hash = "sha256:7f6b125602ac6d743e523ae0bfa71e1a697a2f5534064528c6ff84c2f7c2fc7f"}, + {file = "pytest_mock-3.11.1-py3-none-any.whl", hash = "sha256:21c279fff83d70763b05f8874cc9cfb3fcacd6d354247a976f9529d19f9acf39"}, +] + +[package.dependencies] +pytest = ">=5.0" + +[package.extras] +dev = ["pre-commit", "pytest-asyncio", "tox"] + [[package]] name = "pytest-operator" version = "0.28.0" @@ -1978,4 +1995,4 @@ files = [ [metadata] lock-version = "2.0" python-versions = "^3.8.1" -content-hash = "39a6518d54ff3b1d0fceeeb4b878f9f294973d7e0cb43bd2c4b9a1d057ee9920" +content-hash = "0c4eee2836fef9a7f0124d7fb7823e2c60fb4edf29f94a542c06bc8927433752" diff --git a/pyproject.toml b/pyproject.toml index 46dc6013..b0cffe3c 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -45,6 +45,8 @@ pytest = "^7.4.0" pytest-xdist = "^3.3.1" pytest-cov = "^4.1.0" ops-scenario = "^5.1.2" +ops = ">=2.0.0" +pytest-mock = "^3.11.1" [tool.poetry.group.integration.dependencies] pytest = "^7.4.0" @@ -54,6 +56,8 @@ pytest-operator-groups = {git = "https://github.com/canonical/data-platform-work juju = "^2.9.44.0" mysql-connector-python = "~8.0.33" tenacity = "^8.2.2" +ops = ">=2.0.0" +pytest-mock = "^3.11.1" [tool.coverage.run] From 90edf0bacd5b69488f24707ce40c0d69a5613aa9 Mon Sep 17 00:00:00 2001 From: Judit Novak Date: Wed, 18 Oct 2023 20:42:14 +0200 Subject: [PATCH 15/24] Test execution both for Juju2 and Juju3 --- .github/workflows/ci.yaml | 10 +++++++++- tox.ini | 4 ++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 5bbdc95e..8be55c9c 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -108,7 +108,15 @@ jobs: bases-index: 0 - series: jammy bases-index: 1 - name: (GH hosted) ${{ matrix.groups.job_name }} | ${{ matrix.ubuntu-versions.series }} + juju-snap-channel: ["2.9/stable", "3.1/stable"] + include: + - juju-snap-channel: "3.1/stable" + agent-version: "3.1.6" + libjuju-version: "3.2.0.1" + - juju-snap-channel: "2.9/stable" + agent-version: "2.9.45" + libjuju-version: "2.9.44.1" + name: ${{ matrix.juju-snap-channel }} - (GH hosted) ${{ matrix.groups.job_name }} | ${{ matrix.ubuntu-versions.series }} needs: - lint - unit-test diff --git a/tox.ini b/tox.ini index 8492c8d7..18de8445 100644 --- a/tox.ini +++ b/tox.ini @@ -64,6 +64,9 @@ commands = description = Run unit tests commands_pre = poetry install --only main,charm-libs,unit +set_env = + {[testenv]set_env} + LIBJUJU_VERSION_SPECIFIER = {env:LIBJUJU_VERSION_SPECIFIER:3.2.2} commands = poetry run pytest --numprocesses=auto --cov=src --ignore={[vars]tests_path}/integration/ {posargs} @@ -73,6 +76,7 @@ set_env = {[testenv]set_env} # Workaround for https://github.com/python-poetry/poetry/issues/6958 POETRY_INSTALLER_PARALLEL = false + LIBJUJU_VERSION_SPECIFIER = {env:LIBJUJU_VERSION_SPECIFIER:3.2.2} pass_env = CI GITHUB_OUTPUT From 91afe4e7dd229cd297b74f35db8c5662725052e1 Mon Sep 17 00:00:00 2001 From: Judit Novak Date: Fri, 20 Oct 2023 01:38:40 +0200 Subject: [PATCH 16/24] data_platform_libs v22 (delete_relation_data(), support for rolling upgrades) --- .../data_platform_libs/v0/data_interfaces.py | 1151 +++++++++++++++-- 1 file changed, 1055 insertions(+), 96 deletions(-) diff --git a/lib/charms/data_platform_libs/v0/data_interfaces.py b/lib/charms/data_platform_libs/v0/data_interfaces.py index d894130e..df59585d 100644 --- a/lib/charms/data_platform_libs/v0/data_interfaces.py +++ b/lib/charms/data_platform_libs/v0/data_interfaces.py @@ -291,19 +291,23 @@ def _on_topic_requested(self, event: TopicRequestedEvent): exchanged in the relation databag. """ +import copy import json import logging from abc import ABC, abstractmethod from collections import namedtuple from datetime import datetime -from typing import List, Optional, Union +from enum import Enum +from typing import Callable, Dict, List, Optional, Set, Tuple, Union +from ops import JujuVersion, Secret, SecretInfo, SecretNotFoundError from ops.charm import ( CharmBase, CharmEvents, RelationChangedEvent, RelationCreatedEvent, RelationEvent, + SecretChangedEvent, ) from ops.framework import EventSource, Object from ops.model import Application, ModelError, Relation, Unit @@ -316,7 +320,7 @@ def _on_topic_requested(self, event: TopicRequestedEvent): # Increment this PATCH version before using `charmcraft publish-lib` or reset # to 0 if you are raising the major API version -LIBPATCH = 17 +LIBPATCH = 22 PYDEPS = ["ops>=2.0.0"] @@ -331,6 +335,78 @@ def _on_topic_requested(self, event: TopicRequestedEvent): deleted - key that were deleted""" +PROV_SECRET_PREFIX = "secret-" +REQ_SECRET_FIELDS = "requested-secrets" + + +class SecretGroup(Enum): + """Secret groups as constants.""" + + USER = "user" + TLS = "tls" + EXTRA = "extra" + + +# Local map to associate mappings with secrets potentially as a group +SECRET_LABEL_MAP = { + "username": SecretGroup.USER, + "password": SecretGroup.USER, + "uris": SecretGroup.USER, + "tls": SecretGroup.TLS, + "tls-ca": SecretGroup.TLS, +} + + +class DataInterfacesError(Exception): + """Common ancestor for DataInterfaces related exceptions.""" + + +class SecretError(Exception): + """Common ancestor for Secrets related exceptions.""" + + +class SecretAlreadyExistsError(SecretError): + """A secret that was to be added already exists.""" + + +class SecretsUnavailableError(SecretError): + """Secrets aren't yet available for Juju version used.""" + + +class SecretsIllegalUpdateError(SecretError): + """Secrets aren't yet available for Juju version used.""" + + +def get_encoded_dict( + relation: Relation, member: Union[Unit, Application], field: str +) -> Optional[Dict[str, str]]: + """Retrieve and decode an encoded field from relation data.""" + data = json.loads(relation.data[member].get(field, "{}")) + if isinstance(data, dict): + return data + logger.error("Unexpected datatype for %s instead of dict.", str(data)) + + +def get_encoded_list( + relation: Relation, member: Union[Unit, Application], field: str +) -> Optional[List[str]]: + """Retrieve and decode an encoded field from relation data.""" + data = json.loads(relation.data[member].get(field, "[]")) + if isinstance(data, list): + return data + logger.error("Unexpected datatype for %s instead of list.", str(data)) + + +def set_encoded_field( + relation: Relation, + member: Union[Unit, Application], + field: str, + value: Union[str, list, Dict[str, str]], +) -> None: + """Set an encoded field from relation data.""" + relation.data[member].update({field: json.dumps(value)}) + + def diff(event: RelationChangedEvent, bucket: Union[Unit, Application]) -> Diff: """Retrieves the diff of the data in the relation changed databag. @@ -343,7 +419,11 @@ def diff(event: RelationChangedEvent, bucket: Union[Unit, Application]) -> Diff: keys from the event relation databag. """ # Retrieve the old data from the data key in the application relation databag. - old_data = json.loads(event.relation.data[bucket].get("data", "{}")) + old_data = get_encoded_dict(event.relation, bucket, "data") + + if not old_data: + old_data = {} + # Retrieve the new data from the event relation databag. new_data = ( {key: value for key, value in event.relation.data[event.app].items() if key != "data"} @@ -352,24 +432,151 @@ def diff(event: RelationChangedEvent, bucket: Union[Unit, Application]) -> Diff: ) # These are the keys that were added to the databag and triggered this event. - added = new_data.keys() - old_data.keys() + added = new_data.keys() - old_data.keys() # pyright: ignore [reportGeneralTypeIssues] # These are the keys that were removed from the databag and triggered this event. - deleted = old_data.keys() - new_data.keys() + deleted = old_data.keys() - new_data.keys() # pyright: ignore [reportGeneralTypeIssues] # These are the keys that already existed in the databag, # but had their values changed. - changed = {key for key in old_data.keys() & new_data.keys() if old_data[key] != new_data[key]} + changed = { + key + for key in old_data.keys() & new_data.keys() # pyright: ignore [reportGeneralTypeIssues] + if old_data[key] != new_data[key] # pyright: ignore [reportGeneralTypeIssues] + } # Convert the new_data to a serializable format and save it for a next diff check. - event.relation.data[bucket].update({"data": json.dumps(new_data)}) + set_encoded_field(event.relation, bucket, "data", new_data) # Return the diff with all possible changes. return Diff(added, changed, deleted) +def leader_only(f): + """Decorator to ensure that only leader can perform given operation.""" + + def wrapper(self, *args, **kwargs): + if not self.local_unit.is_leader(): + logger.error( + "This operation (%s()) can only be performed by the leader unit", f.__name__ + ) + return + return f(self, *args, **kwargs) + + return wrapper + + +def juju_secrets_only(f): + """Decorator to ensure that certain operations would be only executed on Juju3.""" + + def wrapper(self, *args, **kwargs): + if not self.secrets_enabled: + raise SecretsUnavailableError("Secrets unavailable on current Juju version") + return f(self, *args, **kwargs) + + return wrapper + + +class Scope(Enum): + """Peer relations scope.""" + + APP = "app" + UNIT = "unit" + + +class CachedSecret: + """Locally cache a secret. + + The data structure is precisely re-using/simulating as in the actual Secret Storage + """ + + def __init__(self, charm: CharmBase, label: str, secret_uri: Optional[str] = None): + self._secret_meta = None + self._secret_content = {} + self._secret_uri = secret_uri + self.label = label + self.charm = charm + + def add_secret(self, content: Dict[str, str], relation: Relation) -> Secret: + """Create a new secret.""" + if self._secret_uri: + raise SecretAlreadyExistsError( + "Secret is already defined with uri %s", self._secret_uri + ) + + secret = self.charm.app.add_secret(content, label=self.label) + secret.grant(relation) + self._secret_uri = secret.id + self._secret_meta = secret + return self._secret_meta + + @property + def meta(self) -> Optional[Secret]: + """Getting cached secret meta-information.""" + if not self._secret_meta: + if not (self._secret_uri or self.label): + return + try: + self._secret_meta = self.charm.model.get_secret(label=self.label) + except SecretNotFoundError: + if self._secret_uri: + self._secret_meta = self.charm.model.get_secret( + id=self._secret_uri, label=self.label + ) + return self._secret_meta + + def get_content(self) -> Dict[str, str]: + """Getting cached secret content.""" + if not self._secret_content: + if self.meta: + self._secret_content = self.meta.get_content() + return self._secret_content + + def set_content(self, content: Dict[str, str]) -> None: + """Setting cached secret content.""" + if not self.meta: + return + + if content: + self.meta.set_content(content) + self._secret_content = content + else: + self.meta.remove_all_revisions() + + def get_info(self) -> Optional[SecretInfo]: + """Wrapper function to apply the corresponding call on the Secret object within CachedSecret if any.""" + if self.meta: + return self.meta.get_info() + + +class SecretCache: + """A data structure storing CachedSecret objects.""" + + def __init__(self, charm): + self.charm = charm + self._secrets: Dict[str, CachedSecret] = {} + + def get(self, label: str, uri: Optional[str] = None) -> Optional[CachedSecret]: + """Getting a secret from Juju Secret store or cache.""" + if not self._secrets.get(label): + secret = CachedSecret(self.charm, label, uri) + if secret.meta: + self._secrets[label] = secret + return self._secrets.get(label) + + def add(self, label: str, content: Dict[str, str], relation: Relation) -> CachedSecret: + """Adding a secret to Juju Secret.""" + if self._secrets.get(label): + raise SecretAlreadyExistsError(f"Secret {label} already exists") + + secret = CachedSecret(self.charm, label) + secret.add_secret(content, relation) + self._secrets[label] = secret + return self._secrets[label] + + # Base DataRelation class DataRelation(Object, ABC): - """Base relation data mainpulation class.""" + """Base relation data mainpulation (abstract) class.""" def __init__(self, charm: CharmBase, relation_name: str) -> None: super().__init__(charm, relation_name) @@ -381,13 +588,309 @@ def __init__(self, charm: CharmBase, relation_name: str) -> None: charm.on[relation_name].relation_changed, self._on_relation_changed_event, ) + self._jujuversion = None + self.secrets = SecretCache(self.charm) + + @property + def relations(self) -> List[Relation]: + """The list of Relation instances associated with this relation_name.""" + return [ + relation + for relation in self.charm.model.relations[self.relation_name] + if self._is_relation_active(relation) + ] + + @property + def secrets_enabled(self): + """Is this Juju version allowing for Secrets usage?""" + if not self._jujuversion: + self._jujuversion = JujuVersion.from_environ() + return self._jujuversion.has_secrets + + # Mandatory overrides for internal/helper methods @abstractmethod def _on_relation_changed_event(self, event: RelationChangedEvent) -> None: """Event emitted when the relation data has changed.""" raise NotImplementedError - def fetch_relation_data(self) -> dict: + @abstractmethod + def _get_relation_secret( + self, relation_id: int, group_mapping: SecretGroup, relation_name: Optional[str] = None + ) -> Optional[CachedSecret]: + """Retrieve a Juju Secret that's been stored in the relation databag.""" + raise NotImplementedError + + @abstractmethod + def _fetch_specific_relation_data( + self, relation: Relation, fields: Optional[List[str]] + ) -> Dict[str, str]: + """Fetch data available (directily or indirectly -- i.e. secrets) from the relation.""" + raise NotImplementedError + + @abstractmethod + def _fetch_my_specific_relation_data( + self, relation: Relation, fields: Optional[List[str]] + ) -> Dict[str, str]: + """Fetch data available (directily or indirectly -- i.e. secrets) from the relation for owner/this_app.""" + raise NotImplementedError + + @abstractmethod + def _update_relation_data(self, relation: Relation, data: Dict[str, str]) -> None: + """Update data available (directily or indirectly -- i.e. secrets) from the relation for owner/this_app.""" + raise NotImplementedError + + @abstractmethod + def _delete_relation_data(self, relation: Relation, fields: List[str]) -> None: + """Delete data available (directily or indirectly -- i.e. secrets) from the relation for owner/this_app.""" + raise NotImplementedError + + # Internal helper methods + + @staticmethod + def _is_relation_active(relation: Relation): + """Whether the relation is active based on contained data.""" + try: + _ = repr(relation.data) + return True + except (RuntimeError, ModelError): + return False + + @staticmethod + def _is_secret_field(field: str) -> bool: + """Is the field in question a secret reference (URI) field or not?""" + return field.startswith(PROV_SECRET_PREFIX) + + @staticmethod + def _generate_secret_label( + relation_name: str, relation_id: int, group_mapping: SecretGroup + ) -> str: + """Generate unique group_mappings for secrets within a relation context.""" + return f"{relation_name}.{relation_id}.{group_mapping.value}.secret" + + @staticmethod + def _generate_secret_field_name(group_mapping: SecretGroup) -> str: + """Generate unique group_mappings for secrets within a relation context.""" + return f"{PROV_SECRET_PREFIX}{group_mapping.value}" + + def _relation_from_secret_label(self, secret_label: str) -> Optional[Relation]: + """Retrieve the relation that belongs to a secret label.""" + contents = secret_label.split(".") + + if not (contents and len(contents) >= 3): + return + + contents.pop() # ".secret" at the end + contents.pop() # Group mapping + relation_id = contents.pop() + try: + relation_id = int(relation_id) + except ValueError: + return + + # In case '.' character appeared in relation name + relation_name = ".".join(contents) + + try: + return self.get_relation(relation_name, relation_id) + except ModelError: + return + + @staticmethod + def _group_secret_fields(secret_fields: List[str]) -> Dict[SecretGroup, List[str]]: + """Helper function to arrange secret mappings under their group. + + NOTE: All unrecognized items end up in the 'extra' secret bucket. + Make sure only secret fields are passed! + """ + secret_fieldnames_grouped = {} + for key in secret_fields: + if group := SECRET_LABEL_MAP.get(key): + secret_fieldnames_grouped.setdefault(group, []).append(key) + else: + secret_fieldnames_grouped.setdefault(SecretGroup.EXTRA, []).append(key) + return secret_fieldnames_grouped + + def _get_group_secret_contents( + self, + relation: Relation, + group: SecretGroup, + secret_fields: Optional[Union[Set[str], List[str]]] = None, + ) -> Dict[str, str]: + """Helper function to retrieve collective, requested contents of a secret.""" + if not secret_fields: + secret_fields = [] + + if (secret := self._get_relation_secret(relation.id, group)) and ( + secret_data := secret.get_content() + ): + return {k: v for k, v in secret_data.items() if k in secret_fields} + return {} + + @staticmethod + def _content_for_secret_group( + content: Dict[str, str], secret_fields: Set[str], group_mapping: SecretGroup + ) -> Dict[str, str]: + """Select : pairs from input, that belong to this particular Secret group.""" + if group_mapping == SecretGroup.EXTRA: + return { + k: v + for k, v in content.items() + if k in secret_fields and k not in SECRET_LABEL_MAP.keys() + } + + return { + k: v + for k, v in content.items() + if k in secret_fields and SECRET_LABEL_MAP.get(k) == group_mapping + } + + @juju_secrets_only + def _get_relation_secret_data( + self, relation_id: int, group_mapping: SecretGroup, relation_name: Optional[str] = None + ) -> Optional[Dict[str, str]]: + """Retrieve contents of a Juju Secret that's been stored in the relation databag.""" + secret = self._get_relation_secret(relation_id, group_mapping, relation_name) + if secret: + return secret.get_content() + + # Core operations on Relation Fields manipulations (regardless whether the field is in the databag or in a secret) + # Internal functions to be called directly from transparent public interface functions (+closely related helpers) + + def _process_secret_fields( + self, + relation: Relation, + req_secret_fields: Optional[List[str]], + impacted_rel_fields: List[str], + operation: Callable, + *args, + **kwargs, + ) -> Tuple[Dict[str, str], Set[str]]: + """Isolate target secret fields of manipulation, and execute requested operation by Secret Group.""" + result = {} + + # If the relation started on a databag, we just stay on the databag + # (Rolling upgrades may result in a relation starting on databag, getting secrets enabled on-the-fly) + # self.local_app is sufficient to check (ignored if Requires, never has secrets -- works if Provides) + fallback_to_databag = ( + req_secret_fields + and self.local_unit.is_leader() + and set(req_secret_fields) & set(relation.data[self.local_app]) + ) + + normal_fields = set(impacted_rel_fields) + if req_secret_fields and self.secrets_enabled and not fallback_to_databag: + normal_fields = normal_fields - set(req_secret_fields) + secret_fields = set(impacted_rel_fields) - set(normal_fields) + + secret_fieldnames_grouped = self._group_secret_fields(list(secret_fields)) + + for group in secret_fieldnames_grouped: + # operation() should return nothing when all goes well + if group_result := operation(relation, group, secret_fields, *args, **kwargs): + # If "meaningful" data was returned, we take it. (Some 'operation'-s only return success/failure.) + if isinstance(group_result, dict): + result.update(group_result) + else: + # If it wasn't found as a secret, let's give it a 2nd chance as "normal" field + # Needed when Juju3 Requires meets Juju2 Provider + normal_fields |= set(secret_fieldnames_grouped[group]) + return (result, normal_fields) + + def _fetch_relation_data_without_secrets( + self, app: Application, relation: Relation, fields: Optional[List[str]] + ) -> Dict[str, str]: + """Fetching databag contents when no secrets are involved. + + Since the Provider's databag is the only one holding secrest, we can apply + a simplified workflow to read the Require's side's databag. + This is used typically when the Provides side wants to read the Requires side's data, + or when the Requires side may want to read its own data. + """ + if fields: + return {k: relation.data[app][k] for k in fields if k in relation.data[app]} + else: + return dict(relation.data[app]) + + def _fetch_relation_data_with_secrets( + self, + app: Application, + req_secret_fields: Optional[List[str]], + relation: Relation, + fields: Optional[List[str]] = None, + ) -> Dict[str, str]: + """Fetching databag contents when secrets may be involved. + + This function has internal logic to resolve if a requested field may be "hidden" + within a Relation Secret, or directly available as a databag field. Typically + used to read the Provides side's databag (eigher by the Requires side, or by + Provides side itself). + """ + result = {} + normal_fields = [] + + if not fields: + all_fields = list(relation.data[app].keys()) + normal_fields = [field for field in all_fields if not self._is_secret_field(field)] + + # There must have been secrets there + if all_fields != normal_fields and req_secret_fields: + # So we assemble the full fields list (without 'secret-' fields) + fields = normal_fields + req_secret_fields + + if fields: + result, normal_fields = self._process_secret_fields( + relation, req_secret_fields, fields, self._get_group_secret_contents + ) + + # Processing "normal" fields. May include leftover from what we couldn't retrieve as a secret. + # (Typically when Juju3 Requires meets Juju2 Provides) + if normal_fields: + result.update( + self._fetch_relation_data_without_secrets(app, relation, list(normal_fields)) + ) + return result + + def _update_relation_data_without_secrets( + self, app: Application, relation: Relation, data: Dict[str, str] + ): + """Updating databag contents when no secrets are involved.""" + if any(self._is_secret_field(key) for key in data.keys()): + raise SecretsIllegalUpdateError("Can't update secret {key}.") + + if relation: + relation.data[app].update(data) + + def _delete_relation_data_without_secrets( + self, app: Application, relation: Relation, fields: List[str] + ) -> None: + """Remove databag fields 'fields' from Relation.""" + for field in fields: + relation.data[app].pop(field) + + # Public interface methods + # Handling Relation Fields seamlessly, regardless if in databag or a Juju Secret + + def get_relation(self, relation_name, relation_id) -> Relation: + """Safe way of retrieving a relation.""" + relation = self.charm.model.get_relation(relation_name, relation_id) + + if not relation: + raise DataInterfacesError( + "Relation %s %s couldn't be retrieved", relation_name, relation_id + ) + + if not relation.app: + raise DataInterfacesError("Relation's application missing") + + return relation + + def fetch_relation_data( + self, + relation_ids: Optional[List[int]] = None, + fields: Optional[List[str]] = None, + relation_name: Optional[str] = None, + ) -> Dict[int, Dict[str, str]]: """Retrieves data from relation. This function can be used to retrieve data from a relation @@ -398,48 +901,87 @@ def fetch_relation_data(self) -> dict: a dict of the values stored in the relation data bag for all relation instances (indexed by the relation ID). """ + if not relation_name: + relation_name = self.relation_name + + relations = [] + if relation_ids: + relations = [ + self.get_relation(relation_name, relation_id) for relation_id in relation_ids + ] + else: + relations = self.relations + data = {} - for relation in self.relations: - data[relation.id] = ( - {key: value for key, value in relation.data[relation.app].items() if key != "data"} - if relation.app - else {} - ) + for relation in relations: + if not relation_ids or (relation_ids and relation.id in relation_ids): + data[relation.id] = self._fetch_specific_relation_data(relation, fields) return data - def _update_relation_data(self, relation_id: int, data: dict) -> None: - """Updates a set of key-value pairs in the relation. + def fetch_relation_field( + self, relation_id: int, field: str, relation_name: Optional[str] = None + ) -> Optional[str]: + """Get a single field from the relation data.""" + return ( + self.fetch_relation_data([relation_id], [field], relation_name) + .get(relation_id, {}) + .get(field) + ) - This function writes in the application data bag, therefore, - only the leader unit can call it. + @leader_only + def fetch_my_relation_data( + self, + relation_ids: Optional[List[int]] = None, + fields: Optional[List[str]] = None, + relation_name: Optional[str] = None, + ) -> Optional[Dict[int, Dict[str, str]]]: + """Fetch data of the 'owner' (or 'this app') side of the relation. + + NOTE: Since only the leader can read the relation's 'this_app'-side + Application databag, the functionality is limited to leaders + """ + if not relation_name: + relation_name = self.relation_name + + relations = [] + if relation_ids: + relations = [ + self.get_relation(relation_name, relation_id) for relation_id in relation_ids + ] + else: + relations = self.relations - Args: - relation_id: the identifier for a particular relation. - data: dict containing the key-value pairs - that should be updated in the relation. + data = {} + for relation in relations: + if not relation_ids or relation.id in relation_ids: + data[relation.id] = self._fetch_my_specific_relation_data(relation, fields) + return data + + @leader_only + def fetch_my_relation_field( + self, relation_id: int, field: str, relation_name: Optional[str] = None + ) -> Optional[str]: + """Get a single field from the relation data -- owner side. + + NOTE: Since only the leader can read the relation's 'this_app'-side + Application databag, the functionality is limited to leaders """ - if self.local_unit.is_leader(): - relation = self.charm.model.get_relation(self.relation_name, relation_id) - if relation: - relation.data[self.local_app].update(data) + if relation_data := self.fetch_my_relation_data([relation_id], [field], relation_name): + return relation_data.get(relation_id, {}).get(field) - @staticmethod - def _is_relation_active(relation: Relation): - """Whether the relation is active based on contained data.""" - try: - _ = repr(relation.data) - return True - except (RuntimeError, ModelError): - return False + @leader_only + def update_relation_data(self, relation_id: int, data: dict) -> None: + """Update the data within the relation.""" + relation_name = self.relation_name + relation = self.get_relation(relation_name, relation_id) + return self._update_relation_data(relation, data) - @property - def relations(self) -> List[Relation]: - """The list of Relation instances associated with this relation_name.""" - return [ - relation - for relation in self.charm.model.relations[self.relation_name] - if self._is_relation_active(relation) - ] + @leader_only + def delete_relation_data(self, relation_id: int, fields: List[str]) -> None: + """Remove field from the relation.""" + relation_name = self.relation_name + relation = self.get_relation(relation_name, relation_id) + return self._delete_relation_data(relation, fields) # Base DataProvides and DataRequires @@ -463,6 +1005,174 @@ def _diff(self, event: RelationChangedEvent) -> Diff: """ return diff(event, self.local_app) + # Private methods handling secrets + + @juju_secrets_only + def _add_relation_secret( + self, relation: Relation, content: Dict[str, str], group_mapping: SecretGroup + ) -> bool: + """Add a new Juju Secret that will be registered in the relation databag.""" + secret_field = self._generate_secret_field_name(group_mapping) + if relation.data[self.local_app].get(secret_field): + logging.error("Secret for relation %s already exists, not adding again", relation.id) + return False + + label = self._generate_secret_label(self.relation_name, relation.id, group_mapping) + secret = self.secrets.add(label, content, relation) + + # According to lint we may not have a Secret ID + if secret.meta and secret.meta.id: + relation.data[self.local_app][secret_field] = secret.meta.id + + # Return the content that was added + return True + + @juju_secrets_only + def _update_relation_secret( + self, relation: Relation, content: Dict[str, str], group_mapping: SecretGroup + ) -> bool: + """Update the contents of an existing Juju Secret, referred in the relation databag.""" + secret = self._get_relation_secret(relation.id, group_mapping) + + if not secret: + logging.error("Can't update secret for relation %s", relation.id) + return False + + old_content = secret.get_content() + full_content = copy.deepcopy(old_content) + full_content.update(content) + secret.set_content(full_content) + + # Return True on success + return True + + def _add_or_update_relation_secrets( + self, + relation: Relation, + group: SecretGroup, + secret_fields: Set[str], + data: Dict[str, str], + ) -> bool: + """Update contents for Secret group. If the Secret doesn't exist, create it.""" + secret_content = self._content_for_secret_group(data, secret_fields, group) + if self._get_relation_secret(relation.id, group): + return self._update_relation_secret(relation, secret_content, group) + else: + return self._add_relation_secret(relation, secret_content, group) + + @juju_secrets_only + def _delete_relation_secret( + self, relation: Relation, group: SecretGroup, secret_fields: List[str], fields: List[str] + ) -> bool: + """Update the contents of an existing Juju Secret, referred in the relation databag.""" + secret = self._get_relation_secret(relation.id, group) + + if not secret: + logging.error("Can't update secret for relation %s", str(relation.id)) + return False + + old_content = secret.get_content() + new_content = copy.deepcopy(old_content) + for field in fields: + try: + new_content.pop(field) + except KeyError: + logging.error( + "Non-existing secret was attempted to be removed %s, %s", + str(relation.id), + str(field), + ) + return False + + secret.set_content(new_content) + + # Remove secret from the relation if it's fully gone + if not new_content: + field = self._generate_secret_field_name(group) + relation.data[self.local_app].pop(field) + + # Return the content that was removed + return True + + # Mandatory internal overrides + + @juju_secrets_only + def _get_relation_secret( + self, relation_id: int, group_mapping: SecretGroup, relation_name: Optional[str] = None + ) -> Optional[CachedSecret]: + """Retrieve a Juju Secret that's been stored in the relation databag.""" + if not relation_name: + relation_name = self.relation_name + + label = self._generate_secret_label(relation_name, relation_id, group_mapping) + if secret := self.secrets.get(label): + return secret + + relation = self.charm.model.get_relation(relation_name, relation_id) + if not relation: + return + + secret_field = self._generate_secret_field_name(group_mapping) + if secret_uri := relation.data[self.local_app].get(secret_field): + return self.secrets.get(label, secret_uri) + + def _fetch_specific_relation_data( + self, relation: Relation, fields: Optional[List[str]] + ) -> Dict[str, str]: + """Fetching relation data for Provides. + + NOTE: Since all secret fields are in the Provides side of the databag, we don't need to worry about that + """ + if not relation.app: + return {} + + return self._fetch_relation_data_without_secrets(relation.app, relation, fields) + + def _fetch_my_specific_relation_data( + self, relation: Relation, fields: Optional[List[str]] + ) -> dict: + """Fetching our own relation data.""" + secret_fields = None + if relation.app: + secret_fields = get_encoded_list(relation, relation.app, REQ_SECRET_FIELDS) + + return self._fetch_relation_data_with_secrets( + self.local_app, + secret_fields, + relation, + fields, + ) + + def _update_relation_data(self, relation: Relation, data: Dict[str, str]) -> None: + """Set values for fields not caring whether it's a secret or not.""" + req_secret_fields = [] + if relation.app: + req_secret_fields = get_encoded_list(relation, relation.app, REQ_SECRET_FIELDS) + + _, normal_fields = self._process_secret_fields( + relation, + req_secret_fields, + list(data), + self._add_or_update_relation_secrets, + data=data, + ) + + normal_content = {k: v for k, v in data.items() if k in normal_fields} + self._update_relation_data_without_secrets(self.local_app, relation, normal_content) + + def _delete_relation_data(self, relation: Relation, fields: List[str]) -> None: + """Delete fields from the Relation not caring whether it's a secret or not.""" + req_secret_fields = [] + if relation.app: + req_secret_fields = get_encoded_list(relation, relation.app, REQ_SECRET_FIELDS) + + _, normal_fields = self._process_secret_fields( + relation, req_secret_fields, fields, self._delete_relation_secret, fields=fields + ) + self._delete_relation_data_without_secrets(self.local_app, relation, list(normal_fields)) + + # Public methods - "native" + def set_credentials(self, relation_id: int, username: str, password: str) -> None: """Set credentials. @@ -474,13 +1184,7 @@ def set_credentials(self, relation_id: int, username: str, password: str) -> Non username: user that was created. password: password of the created user. """ - self._update_relation_data( - relation_id, - { - "username": username, - "password": password, - }, - ) + self.update_relation_data(relation_id, {"username": username, "password": password}) def set_tls(self, relation_id: int, tls: str) -> None: """Set whether TLS is enabled. @@ -489,7 +1193,7 @@ def set_tls(self, relation_id: int, tls: str) -> None: relation_id: the identifier for a particular relation. tls: whether tls is enabled (True or False). """ - self._update_relation_data(relation_id, {"tls": tls}) + self.update_relation_data(relation_id, {"tls": tls}) def set_tls_ca(self, relation_id: int, tls_ca: str) -> None: """Set the TLS CA in the application relation databag. @@ -498,29 +1202,41 @@ def set_tls_ca(self, relation_id: int, tls_ca: str) -> None: relation_id: the identifier for a particular relation. tls_ca: TLS certification authority. """ - self._update_relation_data(relation_id, {"tls-ca": tls_ca}) + self.update_relation_data(relation_id, {"tls-ca": tls_ca}) class DataRequires(DataRelation): """Requires-side of the relation.""" + SECRET_FIELDS = ["username", "password", "tls", "tls-ca", "uris"] + def __init__( self, charm, relation_name: str, extra_user_roles: Optional[str] = None, + additional_secret_fields: Optional[List[str]] = [], ): """Manager of base client relations.""" super().__init__(charm, relation_name) self.extra_user_roles = extra_user_roles + self._secret_fields = list(self.SECRET_FIELDS) + if additional_secret_fields: + self._secret_fields += additional_secret_fields + self.framework.observe( self.charm.on[relation_name].relation_created, self._on_relation_created_event ) + self.framework.observe( + charm.on.secret_changed, + self._on_secret_changed_event, + ) - @abstractmethod - def _on_relation_created_event(self, event: RelationCreatedEvent) -> None: - """Event emitted when the relation is created.""" - raise NotImplementedError + @property + def secret_fields(self) -> Optional[List[str]]: + """Local access to secrets field, in case they are being used.""" + if self.secrets_enabled: + return self._secret_fields def _diff(self, event: RelationChangedEvent) -> Diff: """Retrieves the diff of the data in the relation changed databag. @@ -534,14 +1250,50 @@ def _diff(self, event: RelationChangedEvent) -> Diff: """ return diff(event, self.local_unit) - @staticmethod - def _is_resource_created_for_relation(relation: Relation) -> bool: + # Internal helper functions + + def _register_secret_to_relation( + self, relation_name: str, relation_id: int, secret_id: str, group: SecretGroup + ): + """Fetch secrets and apply local label on them. + + [MAGIC HERE] + If we fetch a secret using get_secret(id=, label=), + then will be "stuck" on the Secret object, whenever it may + appear (i.e. as an event attribute, or fetched manually) on future occasions. + + This will allow us to uniquely identify the secret on Provides side (typically on + 'secret-changed' events), and map it to the corresponding relation. + """ + label = self._generate_secret_label(relation_name, relation_id, group) + + # Fetchin the Secret's meta information ensuring that it's locally getting registered with + CachedSecret(self.charm, label, secret_id).meta + + def _register_secrets_to_relation(self, relation: Relation, params_name_list: List[str]): + """Make sure that secrets of the provided list are locally 'registered' from the databag. + + More on 'locally registered' magic is described in _register_secret_to_relation() method + """ + if not relation.app: + return + + for group in SecretGroup: + secret_field = self._generate_secret_field_name(group) + if secret_field in params_name_list: + if secret_uri := relation.data[relation.app].get(secret_field): + self._register_secret_to_relation( + relation.name, relation.id, secret_uri, group + ) + + def _is_resource_created_for_relation(self, relation: Relation) -> bool: if not relation.app: return False - return ( - "username" in relation.data[relation.app] and "password" in relation.data[relation.app] + data = self.fetch_relation_data([relation.id], ["username", "password"]).get( + relation.id, {} ) + return bool(data.get("username")) and bool(data.get("password")) def is_resource_created(self, relation_id: Optional[int] = None) -> bool: """Check if the resource has been created. @@ -576,6 +1328,75 @@ def is_resource_created(self, relation_id: Optional[int] = None) -> bool: else False ) + # Event handlers + + def _on_relation_created_event(self, event: RelationCreatedEvent) -> None: + """Event emitted when the relation is created.""" + if not self.local_unit.is_leader(): + return + + if self.secret_fields: + set_encoded_field( + event.relation, self.charm.app, REQ_SECRET_FIELDS, self.secret_fields + ) + + @abstractmethod + def _on_secret_changed_event(self, event: RelationChangedEvent) -> None: + """Event emitted when the relation data has changed.""" + raise NotImplementedError + + # Mandatory internal overrides + + @juju_secrets_only + def _get_relation_secret( + self, relation_id: int, group: SecretGroup, relation_name: Optional[str] = None + ) -> Optional[CachedSecret]: + """Retrieve a Juju Secret that's been stored in the relation databag.""" + if not relation_name: + relation_name = self.relation_name + + label = self._generate_secret_label(relation_name, relation_id, group) + return self.secrets.get(label) + + def _fetch_specific_relation_data( + self, relation, fields: Optional[List[str]] = None + ) -> Dict[str, str]: + """Fetching Requires data -- that may include secrets.""" + if not relation.app: + return {} + return self._fetch_relation_data_with_secrets( + relation.app, self.secret_fields, relation, fields + ) + + def _fetch_my_specific_relation_data(self, relation, fields: Optional[List[str]]) -> dict: + """Fetching our own relation data.""" + return self._fetch_relation_data_without_secrets(self.local_app, relation, fields) + + def _update_relation_data(self, relation: Relation, data: dict) -> None: + """Updates a set of key-value pairs in the relation. + + This function writes in the application data bag, therefore, + only the leader unit can call it. + + Args: + relation: the particular relation. + data: dict containing the key-value pairs + that should be updated in the relation. + """ + return self._update_relation_data_without_secrets(self.local_app, relation, data) + + def _delete_relation_data(self, relation: Relation, fields: List[str]) -> None: + """Deletes a set of fields from the relation. + + This function writes in the application data bag, therefore, + only the leader unit can call it. + + Args: + relation: the particular relation. + fields: list containing the field names that should be removed from the relation. + """ + return self._delete_relation_data_without_secrets(self.local_app, relation, fields) + # General events @@ -593,7 +1414,50 @@ def extra_user_roles(self) -> Optional[str]: class AuthenticationEvent(RelationEvent): - """Base class for authentication fields for events.""" + """Base class for authentication fields for events. + + The amount of logic added here is not ideal -- but this was the only way to preserve + the interface when moving to Juju Secrets + """ + + @property + def _secrets(self) -> dict: + """Caching secrets to avoid fetching them each time a field is referrd. + + DON'T USE the encapsulated helper variable outside of this function + """ + if not hasattr(self, "_cached_secrets"): + self._cached_secrets = {} + return self._cached_secrets + + @property + def _jujuversion(self) -> JujuVersion: + """Caching jujuversion to avoid a Juju call on each field evaluation. + + DON'T USE the encapsulated helper variable outside of this function + """ + if not hasattr(self, "_cached_jujuversion"): + self._cached_jujuversion = None + if not self._cached_jujuversion: + self._cached_jujuversion = JujuVersion.from_environ() + return self._cached_jujuversion + + def _get_secret(self, group) -> Optional[Dict[str, str]]: + """Retrieveing secrets.""" + if not self.app: + return + if not self._secrets.get(group): + self._secrets[group] = None + secret_field = f"{PROV_SECRET_PREFIX}{group}" + if secret_uri := self.relation.data[self.app].get(secret_field): + secret = self.framework.model.get_secret(id=secret_uri) + self._secrets[group] = secret.get_content() + return self._secrets[group] + + @property + def secrets_enabled(self): + """Is this Juju version allowing for Secrets usage?""" + return self._jujuversion.has_secrets @property def username(self) -> Optional[str]: @@ -601,6 +1465,11 @@ def username(self) -> Optional[str]: if not self.relation.app: return None + if self.secrets_enabled: + secret = self._get_secret("user") + if secret: + return secret.get("username") + return self.relation.data[self.relation.app].get("username") @property @@ -609,6 +1478,11 @@ def password(self) -> Optional[str]: if not self.relation.app: return None + if self.secrets_enabled: + secret = self._get_secret("user") + if secret: + return secret.get("password") + return self.relation.data[self.relation.app].get("password") @property @@ -617,6 +1491,11 @@ def tls(self) -> Optional[str]: if not self.relation.app: return None + if self.secrets_enabled: + secret = self._get_secret("tls") + if secret: + return secret.get("tls") + return self.relation.data[self.relation.app].get("tls") @property @@ -625,6 +1504,11 @@ def tls_ca(self) -> Optional[str]: if not self.relation.app: return None + if self.secrets_enabled: + secret = self._get_secret("tls") + if secret: + return secret.get("tls-ca") + return self.relation.data[self.relation.app].get("tls-ca") @@ -761,10 +1645,9 @@ def __init__(self, charm: CharmBase, relation_name: str) -> None: def _on_relation_changed_event(self, event: RelationChangedEvent) -> None: """Event emitted when the relation has changed.""" - # Only the leader should handle this event. + # Leader only if not self.local_unit.is_leader(): return - # Check which data has changed to emit customs events. diff = self._diff(event) @@ -785,7 +1668,7 @@ def set_database(self, relation_id: int, database_name: str) -> None: relation_id: the identifier for a particular relation. database_name: database name. """ - self._update_relation_data(relation_id, {"database": database_name}) + self.update_relation_data(relation_id, {"database": database_name}) def set_endpoints(self, relation_id: int, connection_strings: str) -> None: """Set database primary connections. @@ -801,7 +1684,7 @@ def set_endpoints(self, relation_id: int, connection_strings: str) -> None: relation_id: the identifier for a particular relation. connection_strings: database hosts and ports comma separated list. """ - self._update_relation_data(relation_id, {"endpoints": connection_strings}) + self.update_relation_data(relation_id, {"endpoints": connection_strings}) def set_read_only_endpoints(self, relation_id: int, connection_strings: str) -> None: """Set database replicas connection strings. @@ -813,7 +1696,7 @@ def set_read_only_endpoints(self, relation_id: int, connection_strings: str) -> relation_id: the identifier for a particular relation. connection_strings: database hosts and ports comma separated list. """ - self._update_relation_data(relation_id, {"read-only-endpoints": connection_strings}) + self.update_relation_data(relation_id, {"read-only-endpoints": connection_strings}) def set_replset(self, relation_id: int, replset: str) -> None: """Set replica set name in the application relation databag. @@ -824,7 +1707,7 @@ def set_replset(self, relation_id: int, replset: str) -> None: relation_id: the identifier for a particular relation. replset: replica set name. """ - self._update_relation_data(relation_id, {"replset": replset}) + self.update_relation_data(relation_id, {"replset": replset}) def set_uris(self, relation_id: int, uris: str) -> None: """Set the database connection URIs in the application relation databag. @@ -835,7 +1718,7 @@ def set_uris(self, relation_id: int, uris: str) -> None: relation_id: the identifier for a particular relation. uris: connection URIs. """ - self._update_relation_data(relation_id, {"uris": uris}) + self.update_relation_data(relation_id, {"uris": uris}) def set_version(self, relation_id: int, version: str) -> None: """Set the database version in the application relation databag. @@ -844,7 +1727,7 @@ def set_version(self, relation_id: int, version: str) -> None: relation_id: the identifier for a particular relation. version: database version. """ - self._update_relation_data(relation_id, {"version": version}) + self.update_relation_data(relation_id, {"version": version}) class DatabaseRequires(DataRequires): @@ -859,9 +1742,10 @@ def __init__( database_name: str, extra_user_roles: Optional[str] = None, relations_aliases: Optional[List[str]] = None, + additional_secret_fields: Optional[List[str]] = [], ): """Manager of database client relations.""" - super().__init__(charm, relation_name, extra_user_roles) + super().__init__(charm, relation_name, extra_user_roles, additional_secret_fields) self.database = database_name self.relations_aliases = relations_aliases @@ -886,6 +1770,10 @@ def __init__( DatabaseReadOnlyEndpointsChangedEvent, ) + def _on_secret_changed_event(self, event: SecretChangedEvent): + """Event notifying about a new value of a secret.""" + pass + def _assign_relation_alias(self, relation_id: int) -> None: """Assigns an alias to a relation. @@ -917,6 +1805,10 @@ def _assign_relation_alias(self, relation_id: int) -> None: if relation: relation.data[self.local_unit].update({"alias": available_aliases[0]}) + # We need to set relation alias also on the application level so, + # it will be accessible in show-unit juju command, executed for a consumer application unit + self.update_relation_data(relation_id, {"alias": available_aliases[0]}) + def _emit_aliased_event(self, event: RelationChangedEvent, event_name: str) -> None: """Emit an aliased event to a particular relation if it has an alias. @@ -962,16 +1854,21 @@ def is_postgresql_plugin_enabled(self, plugin: str, relation_index: int = 0) -> if len(self.relations) == 0: return False - relation_data = self.fetch_relation_data()[self.relations[relation_index].id] - host = relation_data.get("endpoints") + relation_id = self.relations[relation_index].id + host = self.fetch_relation_field(relation_id, "endpoints") # Return False if there is no endpoint available. if host is None: return False host = host.split(":")[0] - user = relation_data.get("username") - password = relation_data.get("password") + + content = self.fetch_relation_data([relation_id], ["username", "password"]).get( + relation_id, {} + ) + user = content.get("username") + password = content.get("password") + connection_string = ( f"host='{host}' dbname='{self.database}' user='{user}' password='{password}'" ) @@ -990,13 +1887,15 @@ def is_postgresql_plugin_enabled(self, plugin: str, relation_index: int = 0) -> def _on_relation_created_event(self, event: RelationCreatedEvent) -> None: """Event emitted when the database relation is created.""" + super()._on_relation_created_event(event) + # If relations aliases were provided, assign one to the relation. self._assign_relation_alias(event.relation.id) # Sets both database and extra user roles in the relation # if the roles are provided. Otherwise, sets only the database. if self.extra_user_roles: - self._update_relation_data( + self.update_relation_data( event.relation.id, { "database": self.database, @@ -1004,16 +1903,23 @@ def _on_relation_created_event(self, event: RelationCreatedEvent) -> None: }, ) else: - self._update_relation_data(event.relation.id, {"database": self.database}) + self.update_relation_data(event.relation.id, {"database": self.database}) def _on_relation_changed_event(self, event: RelationChangedEvent) -> None: """Event emitted when the database relation has changed.""" # Check which data has changed to emit customs events. diff = self._diff(event) + # Register all new secrets with their labels + if any(newval for newval in diff.added if self._is_secret_field(newval)): + self._register_secrets_to_relation(event.relation, diff.added) + # Check if the database is created # (the database charm shared the credentials). - if "username" in diff.added and "password" in diff.added: + secret_field_user = self._generate_secret_field_name(SecretGroup.USER) + if ( + "username" in diff.added and "password" in diff.added + ) or secret_field_user in diff.added: # Emit the default event (the one without an alias). logger.info("database created at %s", datetime.now()) getattr(self.on, "database_created").emit( @@ -1159,7 +2065,7 @@ def __init__(self, charm: CharmBase, relation_name: str) -> None: def _on_relation_changed_event(self, event: RelationChangedEvent) -> None: """Event emitted when the relation has changed.""" - # Only the leader should handle this event. + # Leader only if not self.local_unit.is_leader(): return @@ -1180,7 +2086,7 @@ def set_topic(self, relation_id: int, topic: str) -> None: relation_id: the identifier for a particular relation. topic: the topic name. """ - self._update_relation_data(relation_id, {"topic": topic}) + self.update_relation_data(relation_id, {"topic": topic}) def set_bootstrap_server(self, relation_id: int, bootstrap_server: str) -> None: """Set the bootstrap server in the application relation databag. @@ -1189,7 +2095,7 @@ def set_bootstrap_server(self, relation_id: int, bootstrap_server: str) -> None: relation_id: the identifier for a particular relation. bootstrap_server: the bootstrap server address. """ - self._update_relation_data(relation_id, {"endpoints": bootstrap_server}) + self.update_relation_data(relation_id, {"endpoints": bootstrap_server}) def set_consumer_group_prefix(self, relation_id: int, consumer_group_prefix: str) -> None: """Set the consumer group prefix in the application relation databag. @@ -1198,7 +2104,7 @@ def set_consumer_group_prefix(self, relation_id: int, consumer_group_prefix: str relation_id: the identifier for a particular relation. consumer_group_prefix: the consumer group prefix string. """ - self._update_relation_data(relation_id, {"consumer-group-prefix": consumer_group_prefix}) + self.update_relation_data(relation_id, {"consumer-group-prefix": consumer_group_prefix}) def set_zookeeper_uris(self, relation_id: int, zookeeper_uris: str) -> None: """Set the zookeeper uris in the application relation databag. @@ -1207,7 +2113,7 @@ def set_zookeeper_uris(self, relation_id: int, zookeeper_uris: str) -> None: relation_id: the identifier for a particular relation. zookeeper_uris: comma-separated list of ZooKeeper server uris. """ - self._update_relation_data(relation_id, {"zookeeper-uris": zookeeper_uris}) + self.update_relation_data(relation_id, {"zookeeper-uris": zookeeper_uris}) class KafkaRequires(DataRequires): @@ -1222,10 +2128,11 @@ def __init__( topic: str, extra_user_roles: Optional[str] = None, consumer_group_prefix: Optional[str] = None, + additional_secret_fields: Optional[List[str]] = [], ): """Manager of Kafka client relations.""" # super().__init__(charm, relation_name) - super().__init__(charm, relation_name, extra_user_roles) + super().__init__(charm, relation_name, extra_user_roles, additional_secret_fields) self.charm = charm self.topic = topic self.consumer_group_prefix = consumer_group_prefix or "" @@ -1244,13 +2151,19 @@ def topic(self, value): def _on_relation_created_event(self, event: RelationCreatedEvent) -> None: """Event emitted when the Kafka relation is created.""" + super()._on_relation_created_event(event) + # Sets topic, extra user roles, and "consumer-group-prefix" in the relation relation_data = { f: getattr(self, f.replace("-", "_"), "") for f in ["consumer-group-prefix", "extra-user-roles", "topic"] } - self._update_relation_data(event.relation.id, relation_data) + self.update_relation_data(event.relation.id, relation_data) + + def _on_secret_changed_event(self, event: SecretChangedEvent): + """Event notifying about a new value of a secret.""" + pass def _on_relation_changed_event(self, event: RelationChangedEvent) -> None: """Event emitted when the Kafka relation has changed.""" @@ -1259,7 +2172,15 @@ def _on_relation_changed_event(self, event: RelationChangedEvent) -> None: # Check if the topic is created # (the Kafka charm shared the credentials). - if "username" in diff.added and "password" in diff.added: + + # Register all new secrets with their labels + if any(newval for newval in diff.added if self._is_secret_field(newval)): + self._register_secrets_to_relation(event.relation, diff.added) + + secret_field_user = self._generate_secret_field_name(SecretGroup.USER) + if ( + "username" in diff.added and "password" in diff.added + ) or secret_field_user in diff.added: # Emit the default event (the one without an alias). logger.info("topic created at %s", datetime.now()) getattr(self.on, "topic_created").emit(event.relation, app=event.app, unit=event.unit) @@ -1339,10 +2260,9 @@ def __init__(self, charm: CharmBase, relation_name: str) -> None: def _on_relation_changed_event(self, event: RelationChangedEvent) -> None: """Event emitted when the relation has changed.""" - # Only the leader should handle this event. + # Leader only if not self.local_unit.is_leader(): return - # Check which data has changed to emit customs events. diff = self._diff(event) @@ -1362,7 +2282,7 @@ def set_index(self, relation_id: int, index: str) -> None: requested index, and can be used to present a different index name if, for example, the requested index is invalid. """ - self._update_relation_data(relation_id, {"index": index}) + self.update_relation_data(relation_id, {"index": index}) def set_endpoints(self, relation_id: int, endpoints: str) -> None: """Set the endpoints in the application relation databag. @@ -1371,7 +2291,7 @@ def set_endpoints(self, relation_id: int, endpoints: str) -> None: relation_id: the identifier for a particular relation. endpoints: the endpoint addresses for opensearch nodes. """ - self._update_relation_data(relation_id, {"endpoints": endpoints}) + self.update_relation_data(relation_id, {"endpoints": endpoints}) def set_version(self, relation_id: int, version: str) -> None: """Set the opensearch version in the application relation databag. @@ -1380,7 +2300,7 @@ def set_version(self, relation_id: int, version: str) -> None: relation_id: the identifier for a particular relation. version: database version. """ - self._update_relation_data(relation_id, {"version": version}) + self.update_relation_data(relation_id, {"version": version}) class OpenSearchRequires(DataRequires): @@ -1389,22 +2309,54 @@ class OpenSearchRequires(DataRequires): on = OpenSearchRequiresEvents() # pyright: ignore[reportGeneralTypeIssues] def __init__( - self, charm, relation_name: str, index: str, extra_user_roles: Optional[str] = None + self, + charm, + relation_name: str, + index: str, + extra_user_roles: Optional[str] = None, + additional_secret_fields: Optional[List[str]] = [], ): """Manager of OpenSearch client relations.""" - super().__init__(charm, relation_name, extra_user_roles) + super().__init__(charm, relation_name, extra_user_roles, additional_secret_fields) self.charm = charm self.index = index def _on_relation_created_event(self, event: RelationCreatedEvent) -> None: """Event emitted when the OpenSearch relation is created.""" + super()._on_relation_created_event(event) + # Sets both index and extra user roles in the relation if the roles are provided. # Otherwise, sets only the index. data = {"index": self.index} if self.extra_user_roles: data["extra-user-roles"] = self.extra_user_roles - self._update_relation_data(event.relation.id, data) + self.update_relation_data(event.relation.id, data) + + def _on_secret_changed_event(self, event: SecretChangedEvent): + """Event notifying about a new value of a secret.""" + if not event.secret.label: + return + + relation = self._relation_from_secret_label(event.secret.label) + if not relation: + logging.info( + f"Received secret {event.secret.label} but couldn't parse, seems irrelevant" + ) + return + + if relation.app == self.charm.app: + logging.info("Secret changed event ignored for Secret Owner") + + remote_unit = None + for unit in relation.units: + if unit.app != self.charm.app: + remote_unit = unit + + logger.info("authentication updated") + getattr(self.on, "authentication_updated").emit( + relation, app=relation.app, unit=remote_unit + ) def _on_relation_changed_event(self, event: RelationChangedEvent) -> None: """Event emitted when the OpenSearch relation has changed. @@ -1414,8 +2366,13 @@ def _on_relation_changed_event(self, event: RelationChangedEvent) -> None: # Check which data has changed to emit customs events. diff = self._diff(event) - # Check if authentication has updated, emit event if so - updates = {"username", "password", "tls", "tls-ca"} + # Register all new secrets with their labels + if any(newval for newval in diff.added if self._is_secret_field(newval)): + self._register_secrets_to_relation(event.relation, diff.added) + + secret_field_user = self._generate_secret_field_name(SecretGroup.USER) + secret_field_tls = self._generate_secret_field_name(SecretGroup.TLS) + updates = {"username", "password", "tls", "tls-ca", secret_field_user, secret_field_tls} if len(set(diff._asdict().keys()) - updates) < len(diff): logger.info("authentication updated at: %s", datetime.now()) getattr(self.on, "authentication_updated").emit( @@ -1424,7 +2381,9 @@ def _on_relation_changed_event(self, event: RelationChangedEvent) -> None: # Check if the index is created # (the OpenSearch charm shares the credentials). - if "username" in diff.added and "password" in diff.added: + if ( + "username" in diff.added and "password" in diff.added + ) or secret_field_user in diff.added: # Emit the default event (the one without an alias). logger.info("index created at: %s", datetime.now()) getattr(self.on, "index_created").emit(event.relation, app=event.app, unit=event.unit) From 13eead15c0ea4c19487aded98de1be99e9d4552e Mon Sep 17 00:00:00 2001 From: Judit Novak Date: Thu, 19 Oct 2023 01:35:55 +0200 Subject: [PATCH 17/24] Test changes --- tests/conftest.py | 19 ++ tests/integration/conftest.py | 24 +++ tests/unit/conftest.py | 23 +++ .../database_relations/combinations.py | 115 +++++++++++- .../test_database_relations.py | 174 +++++++++++++++++- .../test_database_relations_breaking.py | 81 +++++++- 6 files changed, 426 insertions(+), 10 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index e112e922..33a64ffb 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -3,6 +3,8 @@ import argparse +import pytest + def pytest_addoption(parser): parser.addoption( @@ -28,3 +30,20 @@ def pytest_configure(config): config.option.mysql_router_charm_series = "jammy" if config.option.mysql_router_charm_bases_index is None: config.option.mysql_router_charm_bases_index = 1 + + +@pytest.fixture +def only_with_juju_secrets(juju_has_secrets): + """Pretty way to skip Juju 3 tests.""" + if not juju_has_secrets: + pytest.skip("Secrets test only applies on Juju 3.x") + + +@pytest.fixture +def only_without_juju_secrets(juju_has_secrets): + """Pretty way to skip Juju 2-specific tests. + + Typically: to save CI time, when the same check were executed in a Juju 3-specific way already + """ + if juju_has_secrets: + pytest.skip("Skipping legacy secrets tests") diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index 45585042..1677a02d 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -1,10 +1,14 @@ # Copyright 2022 Canonical Ltd. # See LICENSE file for licensing details. +import os import pathlib +from unittest.mock import PropertyMock import pytest import pytest_operator.plugin +from ops import JujuVersion +from pytest_mock import MockerFixture @pytest.fixture(scope="session") @@ -30,3 +34,23 @@ async def build_charm(charm_path) -> pathlib.Path: ops_test.build_charm = build_charm return ops_test + + +@pytest.fixture(autouse=True) +def juju_has_secrets(mocker: MockerFixture, request): + """This fixture will force the usage of secrets whenever run on Juju 3.x. + + NOTE: This is needed, as normally JujuVersion is set to 0.0.0 in tests + (i.e. not the real juju version) + """ + juju_version = os.environ["LIBJUJU_VERSION_SPECIFIER"].split("/")[0] + if juju_version < "3": + mocker.patch.object( + JujuVersion, "has_secrets", new_callable=PropertyMock + ).return_value = False + return False + else: + mocker.patch.object( + JujuVersion, "has_secrets", new_callable=PropertyMock + ).return_value = True + return True diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index 6895d8aa..7a4c11b5 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -1,7 +1,11 @@ # Copyright 2023 Canonical Ltd. # See LICENSE file for licensing details. +from unittest.mock import PropertyMock + import pytest +from ops import JujuVersion +from pytest_mock import MockerFixture import snap @@ -73,3 +77,22 @@ def stop(self, services: list[str] = None, *_, **__): monkeypatch.setattr("snap._Path.unlink", lambda *args, **kwargs: None) monkeypatch.setattr("snap._Path.mkdir", lambda *args, **kwargs: None) monkeypatch.setattr("snap._Path.rmtree", lambda *args, **kwargs: None) + + +@pytest.fixture(autouse=True, params=["juju2", "juju3"]) +def juju_has_secrets(mocker: MockerFixture, request): + """This fixture will force the usage of secrets whenever run on Juju 3.x. + + NOTE: This is needed, as normally JujuVersion is set to 0.0.0 in tests + (i.e. not the real juju version) + """ + if request.param == "juju3": + mocker.patch.object( + JujuVersion, "has_secrets", new_callable=PropertyMock + ).return_value = False + return False + else: + mocker.patch.object( + JujuVersion, "has_secrets", new_callable=PropertyMock + ).return_value = True + return True diff --git a/tests/unit/scenario_/database_relations/combinations.py b/tests/unit/scenario_/database_relations/combinations.py index 06fe6547..b9b06160 100644 --- a/tests/unit/scenario_/database_relations/combinations.py +++ b/tests/unit/scenario_/database_relations/combinations.py @@ -5,11 +5,20 @@ import itertools import typing +from uuid import uuid4 import scenario +APP_NAMES = ["remote", "mysql-k8s"] +SECRET_USER = scenario.Secret( + id="myXID", # Must be defined for obj instantiation, but we override it later + owner="mysql-router", + contents={0: {"username": "relation-68", "password": "Dy0k2UTfyNt2B13cfe412K7YGs07S4U7"}}, + remote_grants="myappB", +) -def _relation_combinations( + +def _relation_provides_combinations( *, relation_amounts: typing.Iterable[int], relations: list[scenario.SubordinateRelation] ) -> list[list[scenario.SubordinateRelation]]: """Get all combinations of `relations` for each length in `relation_amounts`.""" @@ -26,12 +35,49 @@ def _relation_combinations( return combinations +def _relation_requires_combinations( + *, + relation_amounts: typing.Iterable[int], + relations: list[scenario.Relation], + secrets: list[scenario.Secret], +) -> list[list[scenario.Relation]]: + """Get all combinations of `relations` for each length in `relation_amounts`.""" + combinations = [] + for number_of_relations in relation_amounts: + for combination in itertools.combinations_with_replacement(relations, number_of_relations): + combination: tuple[scenario.Relation] + + for relation in combination: + secret_id = uuid4().hex + relation_id = scenario.state.next_relation_id() + + secrets_new = [ + secret.replace( + id=secret_id, label=f"{relation.endpoint}.{relation_id}.user.secret" + ) + for secret in secrets + ] + + remote_app_data_new = relation.remote_app_data + remote_app_data_new["secret-user"] = secret_id + combinations += [ + ( + relation.replace( + relation_id=relation_id, remote_app_data=remote_app_data_new + ), + secret_new, + ) + for secret_new in secrets_new + ] + return combinations + + def incomplete_provides(*relation_amounts: int) -> list[list[scenario.SubordinateRelation]]: databags = [{}] relations = [] for remote_app_name in ["remote", "mysql-test-app"]: relations.extend( - _relation_combinations( + _relation_provides_combinations( relation_amounts=relation_amounts, relations=[ scenario.SubordinateRelation( @@ -53,7 +99,7 @@ def unsupported_extra_user_role_provides( {"database": "myappA", "extra-user-roles": "admin"}, {"database": "myappB", "extra-user-roles": "mysqlrouter"}, ] - return _relation_combinations( + return _relation_provides_combinations( relation_amounts=relation_amounts, relations=[ scenario.SubordinateRelation(endpoint="database", remote_app_data=databag) @@ -64,10 +110,71 @@ def unsupported_extra_user_role_provides( def complete_provides(*relation_amounts: int) -> list[list[scenario.SubordinateRelation]]: databags = [{"database": "myappA"}, {"database": "foo"}] - return _relation_combinations( + return _relation_provides_combinations( relation_amounts=relation_amounts, relations=[ scenario.SubordinateRelation(endpoint="database", remote_app_data=databag) for databag in databags ], ) + + +def complete_requires_secret(*relation_amounts: int) -> list[list[scenario.Relation]]: + relation = scenario.Relation( + endpoint="backend-database", + remote_app_data={ + "endpoints": "mysql-k8s-primary:5432" + }, # Will be extended with "secret-user" field + ) + return _relation_requires_combinations( + relation_amounts=relation_amounts, relations=[relation], secrets=[SECRET_USER] + ) + + +def incomplete_requires_secret(*relation_amounts: int) -> list[list[scenario.Relation]]: + params = [ + {}, + { + "database": "myappB", + "read-only-endpoints": "mysql-k8s-replicas:5432", + }, + { + "database": "myappB", + "endpoints": "mysql-k8s-primary:5432", + "read-only-endpoints": "mysql-k8s-replicas:5432", + }, + ] + + # Missing fields in the databag + relations = [ + scenario.Relation( + endpoint="backend-database", + remote_app_name=app_name, + remote_app_data=param, # Will be extended with "secret-user" field + ) + for app_name in APP_NAMES + for param in params[:2] + ] + combinations_full_secret = _relation_requires_combinations( + relation_amounts=relation_amounts, relations=relations, secrets=[SECRET_USER] + ) + + # Missing fields in the secret + secret_user_pw_missing = SECRET_USER.replace(contents={0: {"username": "relation-68"}}) + + relations_broken_secret = [ + scenario.Relation( + endpoint="backend-database", + remote_app_name=app_name, + remote_app_data=params[2], # Will be extended with "secret-user" field + ) + for app_name in APP_NAMES + ] + + combinations_broken_secret = _relation_requires_combinations( + relation_amounts=relation_amounts, + relations=relations_broken_secret, + secrets=[secret_user_pw_missing], + ) + + return combinations_full_secret + combinations_broken_secret diff --git a/tests/unit/scenario_/database_relations/test_database_relations.py b/tests/unit/scenario_/database_relations/test_database_relations.py index 4737b5c5..bf5dd2f0 100644 --- a/tests/unit/scenario_/database_relations/test_database_relations.py +++ b/tests/unit/scenario_/database_relations/test_database_relations.py @@ -14,7 +14,9 @@ from . import combinations -def output_states(*, relations: list[scenario.Relation]) -> typing.Iterable[scenario.State]: +def output_states( + *, relations: list[scenario.Relation], secrets: list[scenario.Secret] = [] +) -> typing.Iterable[scenario.State]: """Run scenario test for each `abstract_charm.reconcile_database_relations` event. Excludes *-relation-breaking events @@ -22,8 +24,8 @@ def output_states(*, relations: list[scenario.Relation]) -> typing.Iterable[scen The output state of each test should be identical for all events. """ context = scenario.Context(machine_charm.MachineSubordinateRouterCharm) - input_state = scenario.State( relations=[*relations, scenario.PeerRelation(endpoint="upgrade-version-a")], + secrets=secrets, leader=True, ) events = [] @@ -54,11 +56,22 @@ def test_missing_requires(): assert state.app_status == ops.BlockedStatus("Missing relation: backend-database") +@pytest.mark.usefixtures("only_without_juju_secrets") def test_missing_provides(incomplete_requires): for state in output_states(relations=[incomplete_requires]): assert state.app_status == ops.BlockedStatus("Missing relation: database") +@pytest.mark.usefixtures("only_with_juju_secrets") +@pytest.mark.parametrize( + "incomplete_requires_s, secret", combinations.incomplete_requires_secret(1) +) +def test_missing_provides_secrets(incomplete_requires_s, secret): + for state in output_states(relations=[incomplete_requires_s], secrets=[secret]): + assert state.app_status == ops.BlockedStatus("Missing relation: database") + + +@pytest.mark.usefixtures("only_without_juju_secrets") @pytest.mark.parametrize( "unsupported_extra_user_role_provides_s", combinations.unsupported_extra_user_role_provides(1, 3), @@ -75,10 +88,38 @@ def test_provides_unsupported_extra_user_role( ] ): assert state.app_status == ops.BlockedStatus( - f"{unsupported_extra_user_role_provides_s[0].remote_app_name} app requested unsupported extra user role on database endpoint" + f"{unsupported_extra_user_role_provides_s[0].remote_app_name} app " + "requested unsupported extra user role on database endpoint" ) +@pytest.mark.usefixtures("only_with_juju_secrets") +@pytest.mark.parametrize( + "unsupported_extra_user_role_provides_s", + combinations.unsupported_extra_user_role_provides(1, 3), +) +@pytest.mark.parametrize("complete_provides_s", combinations.complete_provides(0, 2)) +@pytest.mark.parametrize( + "incomplete_requires_s, secret", combinations.incomplete_requires_secret(1) +) +def test_provides_unsupported_extra_user_role_secrets( + incomplete_requires_s, secret, complete_provides_s, unsupported_extra_user_role_provides_s +): + for state in output_states( + relations=[ + incomplete_requires_s, + *complete_provides_s, + *unsupported_extra_user_role_provides_s, + ], + secrets=[secret], + ): + assert state.app_status == ops.BlockedStatus( + f"{unsupported_extra_user_role_provides_s[0].remote_app_name} app " + "requested unsupported extra user role on database endpoint" + ) + + +@pytest.mark.usefixtures("only_without_juju_secrets") @pytest.mark.parametrize("complete_provides_s", combinations.complete_provides(1, 2)) def test_incomplete_requires(incomplete_requires, complete_provides_s): for state in output_states(relations=[incomplete_requires, *complete_provides_s]): @@ -89,6 +130,23 @@ def test_incomplete_requires(incomplete_requires, complete_provides_s): assert state.relations[index].local_app_data == {} +@pytest.mark.usefixtures("only_with_juju_secrets") +@pytest.mark.parametrize("complete_provides_s", combinations.complete_provides(1, 2)) +@pytest.mark.parametrize( + "incomplete_requires_s, secret", combinations.incomplete_requires_secret(1) +) +def test_incomplete_requires_secrets(incomplete_requires_s, secret, complete_provides_s): + for state in output_states( + relations=[incomplete_requires_s, *complete_provides_s], secrets=[secret] + ): + assert state.app_status == ops.WaitingStatus( + f"Waiting for {incomplete_requires_s.remote_app_name} app on backend-database endpoint" + ) + for index, provides in enumerate(complete_provides_s, 1): + assert state.relations[index].local_app_data == {} + + +@pytest.mark.usefixtures("only_without_juju_secrets") @pytest.mark.parametrize( "unsupported_extra_user_role_provides_s", combinations.unsupported_extra_user_role_provides(1, 3), @@ -107,7 +165,8 @@ def test_complete_requires_and_provides_unsupported_extra_user_role( ] ): assert state.app_status == ops.BlockedStatus( - f"{unsupported_extra_user_role_provides_s[0].remote_app_name} app requested unsupported extra user role on database endpoint" + f"{unsupported_extra_user_role_provides_s[0].remote_app_name} app " + "requested unsupported extra user role on database endpoint" ) for index, provides in enumerate(complete_provides_s, 1): local_app_data = state.relations[index].local_app_data @@ -124,6 +183,47 @@ def test_complete_requires_and_provides_unsupported_extra_user_role( assert state.relations[index].local_app_data == {} +@pytest.mark.usefixtures("only_with_juju_secrets") +@pytest.mark.parametrize( + "unsupported_extra_user_role_provides_s", + combinations.unsupported_extra_user_role_provides(1, 3), +) +@pytest.mark.parametrize("complete_provides_s", combinations.complete_provides(0, 2)) +@pytest.mark.parametrize("complete_requires_s, secret", combinations.complete_requires_secret(1)) +def test_complete_requires_and_provides_unsupported_extra_user_role_secret( + complete_requires_s, + secret, + complete_provides_s, + unsupported_extra_user_role_provides_s, +): + for state in output_states( + relations=[ + complete_requires_s, + *complete_provides_s, + *unsupported_extra_user_role_provides_s, + ], + secrets=[secret], + ): + assert state.app_status == ops.BlockedStatus( + f"{unsupported_extra_user_role_provides_s[0].remote_app_name} app " + "requested unsupported extra user role on database endpoint" + ) + for index, provides in enumerate(complete_provides_s, 1): + local_app_data = state.relations[index].local_app_data + assert len(local_app_data.pop("password")) > 0 + assert local_app_data == { + "database": provides.remote_app_data["database"], + "endpoints": "file:///var/snap/charmed-mysql/common/run/mysqlrouter/mysql.sock", + "read-only-endpoints": "file:///var/snap/charmed-mysql/common/run/mysqlrouter/mysqlro.sock", + "username": f'{secret.contents[0]["username"]}-{provides.relation_id}', + } + for index, provides in enumerate( + unsupported_extra_user_role_provides_s, 1 + len(complete_provides_s) + ): + assert state.relations[index].local_app_data == {} + + +@pytest.mark.usefixtures("only_without_juju_secrets") @pytest.mark.parametrize("incomplete_provides_s", combinations.incomplete_provides(1, 3)) def test_incomplete_provides(complete_requires, incomplete_provides_s): for state in output_states(relations=[complete_requires, *incomplete_provides_s]): @@ -134,6 +234,21 @@ def test_incomplete_provides(complete_requires, incomplete_provides_s): assert state.relations[index].local_app_data == {} +@pytest.mark.usefixtures("only_with_juju_secrets") +@pytest.mark.parametrize("incomplete_provides_s", combinations.incomplete_provides(1, 3)) +@pytest.mark.parametrize("complete_requires_s, secret", combinations.complete_requires_secret(1)) +def test_incomplete_provides_secrets(complete_requires_s, secret, incomplete_provides_s): + for state in output_states( + relations=[complete_requires_s, *incomplete_provides_s], secrets=[secret] + ): + assert state.app_status == ops.WaitingStatus( + f"Waiting for {incomplete_provides_s[0].remote_app_name} app on database endpoint" + ) + for index, provides in enumerate(incomplete_provides_s, 1): + assert state.relations[index].local_app_data == {} + + +@pytest.mark.usefixtures("only_without_juju_secrets") @pytest.mark.parametrize("complete_provides_s", combinations.complete_provides(1, 2, 4)) def test_complete_provides(complete_requires, complete_provides_s): for state in output_states(relations=[complete_requires, *complete_provides_s]): @@ -149,8 +264,28 @@ def test_complete_provides(complete_requires, complete_provides_s): } +@pytest.mark.usefixtures("only_with_juju_secrets") +@pytest.mark.parametrize("complete_provides_s", combinations.complete_provides(1, 2, 4)) +@pytest.mark.parametrize("complete_requires_s, secret", combinations.complete_requires_secret(1)) +def test_complete_provides_secret(complete_requires_s, secret, complete_provides_s): + for state in output_states( + relations=[complete_requires_s, *complete_provides_s], secrets=[secret] + ): + assert state.app_status == ops.ActiveStatus() + for index, provides in enumerate(complete_provides_s, 1): + local_app_data = state.relations[index].local_app_data + assert len(local_app_data.pop("password")) > 0 + assert local_app_data == { + "database": provides.remote_app_data["database"], + "endpoints": "file:///var/snap/charmed-mysql/common/run/mysqlrouter/mysql.sock", + "read-only-endpoints": "file:///var/snap/charmed-mysql/common/run/mysqlrouter/mysqlro.sock", + "username": f'{secret.contents[0]["username"]}-{provides.relation_id}', + } + + +@pytest.mark.usefixtures("only_without_juju_secrets") @pytest.mark.parametrize("incomplete_provides_s", combinations.incomplete_provides(1, 3)) -@pytest.mark.parametrize("complete_provides_s", combinations.complete_provides(1, 3)) +@pytest.mark.parametrize("complete_provides_s", combinations.complete_provides(1)) def test_complete_provides_and_incomplete_provides( complete_requires, complete_provides_s, incomplete_provides_s ): @@ -171,3 +306,32 @@ def test_complete_provides_and_incomplete_provides( } for index, provides in enumerate(incomplete_provides_s, 1 + len(complete_provides_s)): assert state.relations[index].local_app_data == {} + + +@pytest.mark.usefixtures("only_with_juju_secrets") +@pytest.mark.parametrize("incomplete_provides_s", combinations.incomplete_provides(1, 3)) +@pytest.mark.parametrize("complete_provides_s", combinations.complete_provides(1)) +@pytest.mark.parametrize( + "complete_requires_s, secret", combinations.complete_requires_secret(1, 2, 4) +) +def test_complete_provides_and_incomplete_provides_secret( + complete_requires_s, secret, complete_provides_s, incomplete_provides_s +): + for state in output_states( + relations=[complete_requires_s, *complete_provides_s, *incomplete_provides_s], + secrets=[secret], + ): + assert state.app_status == ops.WaitingStatus( + f"Waiting for {incomplete_provides_s[0].remote_app_name} app on database endpoint" + ) + for index, provides in enumerate(complete_provides_s, 1): + local_app_data = state.relations[index].local_app_data + assert len(local_app_data.pop("password")) > 0 + assert local_app_data == { + "database": provides.remote_app_data["database"], + "endpoints": "file:///var/snap/charmed-mysql/common/run/mysqlrouter/mysql.sock", + "read-only-endpoints": "file:///var/snap/charmed-mysql/common/run/mysqlrouter/mysqlro.sock", + "username": f'{secret.contents[0]["username"]}-{provides.relation_id}', + } + for index, provides in enumerate(incomplete_provides_s, 1 + len(complete_provides_s)): + assert state.relations[index].local_app_data == {} diff --git a/tests/unit/scenario_/database_relations/test_database_relations_breaking.py b/tests/unit/scenario_/database_relations/test_database_relations_breaking.py index 6b83a792..4b716736 100644 --- a/tests/unit/scenario_/database_relations/test_database_relations_breaking.py +++ b/tests/unit/scenario_/database_relations/test_database_relations_breaking.py @@ -12,10 +12,16 @@ from . import combinations -def output_state(*, relations: list[scenario.Relation], event: scenario.Event) -> scenario.State: +def output_state( + *, + relations: list[scenario.Relation], + event: scenario.Event, + secrets: list[scenario.Secret] = [], +) -> scenario.State: context = scenario.Context(machine_charm.MachineSubordinateRouterCharm) input_state = scenario.State( relations=[*relations, scenario.PeerRelation(endpoint="upgrade-version-a")], + secrets=secrets, leader=True, ) output = context.run(event, input_state) @@ -23,6 +29,7 @@ def output_state(*, relations: list[scenario.Relation], event: scenario.Event) - return output +@pytest.mark.usefixtures("only_without_juju_secrets") @pytest.mark.parametrize("complete_provides_s", combinations.complete_provides(1, 3)) def test_breaking_requires_and_complete_provides(complete_requires, complete_provides_s): complete_provides_s = [ @@ -45,6 +52,37 @@ def test_breaking_requires_and_complete_provides(complete_requires, complete_pro assert state.relations[index].local_app_data == {} +@pytest.mark.usefixtures("only_with_juju_secrets") +@pytest.mark.parametrize("complete_provides_s", combinations.complete_provides(1, 3)) +@pytest.mark.parametrize( + "complete_requires_s, secret", combinations.complete_requires_secret(1, 2, 4) +) +def test_breaking_requires_and_complete_provides_secret( + complete_requires_s, secret, complete_provides_s +): + complete_provides_s = [ + relation.replace( + local_app_data={ + "database": "foobar", + "endpoints": "file:///var/snap/charmed-mysql/common/run/mysqlrouter/mysql.sock", + "read-only-endpoints": "file:///var/snap/charmed-mysql/common/run/mysqlrouter/mysqlro.sock", + "username": "foouser", + "password": "foobar", + } + ) + for relation in complete_provides_s + ] + state = output_state( + relations=[complete_requires_s, *complete_provides_s], + event=complete_requires_s.broken_event, + secrets=[secret], + ) + assert state.app_status == ops.BlockedStatus("Missing relation: backend-database") + for index, provides in enumerate(complete_provides_s, 1): + assert state.relations[index].local_app_data == {} + + +@pytest.mark.usefixtures("only_without_juju_secrets") @pytest.mark.parametrize("complete_provides_s", combinations.complete_provides(1, 3)) def test_complete_requires_and_breaking_provides(complete_requires, complete_provides_s): complete_provides_s = [ @@ -77,3 +115,44 @@ def test_complete_requires_and_breaking_provides(complete_requires, complete_pro "username": "foouser", "password": "foobar", } + + +@pytest.mark.usefixtures("only_with_juju_secrets") +@pytest.mark.parametrize("complete_provides_s", combinations.complete_provides(1, 3)) +@pytest.mark.parametrize( + "complete_requires_s, secret", combinations.complete_requires_secret(1, 2, 4) +) +def test_complete_requires_and_breaking_provides_secret( + complete_requires_s, secret, complete_provides_s +): + complete_provides_s = [ + relation.replace( + local_app_data={ + "database": "foobar", + "endpoints": "file:///var/snap/charmed-mysql/common/run/mysqlrouter/mysql.sock", + "read-only-endpoints": "file:///var/snap/charmed-mysql/common/run/mysqlrouter/mysqlro.sock", + "username": "foouser", + "password": "foobar", + } + ) + for relation in complete_provides_s + ] + state = output_state( + relations=[complete_requires_s, *complete_provides_s], + event=complete_provides_s[-1].broken_event, + secrets=[secret], + ) + if len(complete_provides_s) == 1: + assert state.app_status == ops.BlockedStatus("Missing relation: database") + else: + assert state.app_status == ops.ActiveStatus() + assert state.relations[-1].local_app_data == {} + complete_provides_s.pop() + for index, provides in enumerate(complete_provides_s, 1): + assert state.relations[index].local_app_data == { + "database": "foobar", + "endpoints": "file:///var/snap/charmed-mysql/common/run/mysqlrouter/mysql.sock", + "read-only-endpoints": "file:///var/snap/charmed-mysql/common/run/mysqlrouter/mysqlro.sock", + "username": "foouser", + "password": "foobar", + } From fca15b3a19fe3cebd930409451a80d8a16008377 Mon Sep 17 00:00:00 2001 From: Judit Novak Date: Thu, 19 Oct 2023 02:02:22 +0200 Subject: [PATCH 18/24] Updated pipelines to include Juju3 --- .github/workflows/ci.yaml | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 8be55c9c..d43d3958 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -112,7 +112,7 @@ jobs: include: - juju-snap-channel: "3.1/stable" agent-version: "3.1.6" - libjuju-version: "3.2.0.1" + libjuju-version: "3.2.2" - juju-snap-channel: "2.9/stable" agent-version: "2.9.45" libjuju-version: "2.9.44.1" @@ -135,6 +135,11 @@ jobs: uses: charmed-kubernetes/actions-operator@main with: provider: lxd + bootstrap-options: "--agent-version ${{ matrix.agent-version }}" + juju-channel: ${{ matrix.juju-snap-channel }} + - name: Update python-libjuju version + if: ${{ matrix.libjuju-version == '2.9.44.1' }} + run: poetry add --lock --group integration juju@'${{ matrix.libjuju-version }}' - name: Download packed charm(s) uses: actions/download-artifact@v3 with: @@ -154,6 +159,8 @@ jobs: - name: Run integration tests id: tests run: tox run -e integration -- "${{ matrix.groups.path_to_test_file }}" --group="${{ matrix.groups.group_number }}" -m '${{ steps.select-test-stability.outputs.mark_expression }}' --mysql-router-charm-series=${{ matrix.ubuntu-versions.series }} --mysql-router-charm-bases-index=${{ matrix.ubuntu-versions.bases-index }} --model test + env: + LIBJUJU_VERSION_SPECIFIER: ${{ matrix.libjuju-version }} - name: Select model if: ${{ success() || (failure() && steps.tests.outcome == 'failure') }} run: | From 981387263f61e945e4447952e70703869fef0d91 Mon Sep 17 00:00:00 2001 From: Judit Novak Date: Fri, 20 Oct 2023 19:56:58 +0200 Subject: [PATCH 19/24] Removing databag reference from MySQL Router Provides --- src/relations/database_provides.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/relations/database_provides.py b/src/relations/database_provides.py index a186f3da..db8630b0 100644 --- a/src/relations/database_provides.py +++ b/src/relations/database_provides.py @@ -126,7 +126,10 @@ def __init__( self, *, relation: ops.Relation, interface: data_interfaces.DatabaseProvides ) -> None: super().__init__(relation=relation) - self._local_databag = relation.data[interface.local_app] + self._interface = interface + self._local_databag = self._interface.fetch_my_relation_data([relation.id]).get( + relation.id + ) for key in ("database", "username", "password", "endpoints", "read-only-endpoints"): if key not in self._local_databag: raise _UserNotCreated @@ -134,7 +137,7 @@ def __init__( def delete_databag(self) -> None: """Remove connection information from databag.""" logger.debug(f"Deleting databag {self._id=}") - self._local_databag.clear() + self._interface.delete_relation_data(self._id, list(self._local_databag)) logger.debug(f"Deleted databag {self._id=}") def delete_user(self, *, shell: mysql_shell.Shell) -> None: From 580bd8fff575bf2c6e9c40cb6a6d77dee42b977b Mon Sep 17 00:00:00 2001 From: Judit Novak Date: Mon, 23 Oct 2023 13:04:11 +0200 Subject: [PATCH 20/24] Juju3 as default + ops-scenario fixed (https://github.com/canonical/ops-scenario/issues/71) --- poetry.lock | 12 ++++++------ pyproject.toml | 4 ++-- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/poetry.lock b/poetry.lock index e5f8e525..eb5b583c 100644 --- a/poetry.lock +++ b/poetry.lock @@ -727,12 +727,12 @@ i18n = ["Babel (>=2.7)"] [[package]] name = "juju" -version = "2.9.44.0" +version = "3.2.0.1" description = "Python library for Juju" optional = false python-versions = "*" files = [ - {file = "juju-2.9.44.0.tar.gz", hash = "sha256:bc71fe0c8fd59ee00f0c3b03066682cd2273f299c36135451abb1a81289e68f9"}, + {file = "juju-3.2.0.1.tar.gz", hash = "sha256:7b167e6750002cf0a3888acbb9081f6eaf492c5d34500717784ec2aef5214545"}, ] [package.dependencies] @@ -978,13 +978,13 @@ websocket-client = "==1.*" [[package]] name = "ops-scenario" -version = "5.1.2" +version = "5.4.1" description = "Python library providing a state-transition testing API for Operator Framework charms." optional = false python-versions = ">=3.8" files = [ - {file = "ops-scenario-5.1.2.tar.gz", hash = "sha256:48d3caa6cae3e91b7ce65437c79413e3facd198ac941c7d12a322b27a786e743"}, - {file = "ops_scenario-5.1.2-py3-none-any.whl", hash = "sha256:6a08a9586e9dac173703433e19f2437a8c7f34afa5dd4b1b27415b7089e0a270"}, + {file = "ops-scenario-5.4.1.tar.gz", hash = "sha256:7d9e303038d62deac5497e29c3addb074e8a7c49c567e531924f39e4848ec9df"}, + {file = "ops_scenario-5.4.1-py3-none-any.whl", hash = "sha256:2de8d1e831939aac295e170fc5c338d09aaf3b454ef6b2a83c7793babcffa139"}, ] [package.dependencies] @@ -1995,4 +1995,4 @@ files = [ [metadata] lock-version = "2.0" python-versions = "^3.8.1" -content-hash = "0c4eee2836fef9a7f0124d7fb7823e2c60fb4edf29f94a542c06bc8927433752" +content-hash = "e3a0757f29249a152bc6f6322ba942baa3c2d71c9fc2096868203e43d153fdc9" diff --git a/pyproject.toml b/pyproject.toml index b0cffe3c..d444c3c5 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -44,7 +44,7 @@ codespell = "^2.2.5" pytest = "^7.4.0" pytest-xdist = "^3.3.1" pytest-cov = "^4.1.0" -ops-scenario = "^5.1.2" +ops-scenario = "^5.4.1" ops = ">=2.0.0" pytest-mock = "^3.11.1" @@ -53,7 +53,7 @@ pytest = "^7.4.0" pytest-operator = "^0.28.0" pytest-operator-cache = {git = "https://github.com/canonical/data-platform-workflows", tag = "v5.0.1", subdirectory = "python/pytest_plugins/pytest_operator_cache"} pytest-operator-groups = {git = "https://github.com/canonical/data-platform-workflows", tag = "v5.0.1", subdirectory = "python/pytest_plugins/pytest_operator_groups"} -juju = "^2.9.44.0" +juju = "3.2.0.1" mysql-connector-python = "~8.0.33" tenacity = "^8.2.2" ops = ">=2.0.0" From de8a49e716c2ecfef0554ab2f7d1b36db0f52ef4 Mon Sep 17 00:00:00 2001 From: Judit Novak Date: Mon, 30 Oct 2023 17:07:55 +0100 Subject: [PATCH 21/24] 12 runners for unittests to progerss --- tox.ini | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tox.ini b/tox.ini index 18de8445..6f075229 100644 --- a/tox.ini +++ b/tox.ini @@ -68,7 +68,7 @@ set_env = {[testenv]set_env} LIBJUJU_VERSION_SPECIFIER = {env:LIBJUJU_VERSION_SPECIFIER:3.2.2} commands = - poetry run pytest --numprocesses=auto --cov=src --ignore={[vars]tests_path}/integration/ {posargs} + poetry run pytest --numprocesses=12 --cov=src --ignore={[vars]tests_path}/integration/ {posargs} [testenv:integration] description = Run integration tests From caba07dd8fbaf1a57f727085ec50a6b834fcec04 Mon Sep 17 00:00:00 2001 From: Judit Novak Date: Mon, 30 Oct 2023 20:31:24 +0100 Subject: [PATCH 22/24] poetry.lock and other fixes after rebase --- poetry.lock | 2 +- .../scenario_/database_relations/test_database_relations.py | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/poetry.lock b/poetry.lock index eb5b583c..462d2485 100644 --- a/poetry.lock +++ b/poetry.lock @@ -1995,4 +1995,4 @@ files = [ [metadata] lock-version = "2.0" python-versions = "^3.8.1" -content-hash = "e3a0757f29249a152bc6f6322ba942baa3c2d71c9fc2096868203e43d153fdc9" +content-hash = "42e6735c9add82a8adfb20216cc7f773caa93c1f901a34346f72559d49526c9a" diff --git a/tests/unit/scenario_/database_relations/test_database_relations.py b/tests/unit/scenario_/database_relations/test_database_relations.py index bf5dd2f0..dfb01fe4 100644 --- a/tests/unit/scenario_/database_relations/test_database_relations.py +++ b/tests/unit/scenario_/database_relations/test_database_relations.py @@ -24,6 +24,7 @@ def output_states( The output state of each test should be identical for all events. """ context = scenario.Context(machine_charm.MachineSubordinateRouterCharm) + input_state = scenario.State( relations=[*relations, scenario.PeerRelation(endpoint="upgrade-version-a")], secrets=secrets, leader=True, From 65302e24165ddf8364742ba6837790adc7f91681 Mon Sep 17 00:00:00 2001 From: Judit Novak Date: Tue, 31 Oct 2023 13:39:20 +0100 Subject: [PATCH 23/24] Adding requested change --- src/relations/database_provides.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/relations/database_provides.py b/src/relations/database_provides.py index db8630b0..8d5752e9 100644 --- a/src/relations/database_provides.py +++ b/src/relations/database_provides.py @@ -127,9 +127,7 @@ def __init__( ) -> None: super().__init__(relation=relation) self._interface = interface - self._local_databag = self._interface.fetch_my_relation_data([relation.id]).get( - relation.id - ) + self._local_databag = self._interface.fetch_my_relation_data([relation.id])[relation.id] for key in ("database", "username", "password", "endpoints", "read-only-endpoints"): if key not in self._local_databag: raise _UserNotCreated From ec2213d8f303067b4f02fb25d5091eba5e9d20d3 Mon Sep 17 00:00:00 2001 From: Judit Novak Date: Tue, 31 Oct 2023 21:48:44 +0100 Subject: [PATCH 24/24] data_platform_libs/data_interfaces v23 --- .../data_platform_libs/v0/data_interfaces.py | 34 +++++++++++++++---- 1 file changed, 27 insertions(+), 7 deletions(-) diff --git a/lib/charms/data_platform_libs/v0/data_interfaces.py b/lib/charms/data_platform_libs/v0/data_interfaces.py index df59585d..5d1691d9 100644 --- a/lib/charms/data_platform_libs/v0/data_interfaces.py +++ b/lib/charms/data_platform_libs/v0/data_interfaces.py @@ -320,7 +320,7 @@ def _on_topic_requested(self, event: TopicRequestedEvent): # Increment this PATCH version before using `charmcraft publish-lib` or reset # to 0 if you are raising the major API version -LIBPATCH = 22 +LIBPATCH = 23 PYDEPS = ["ops>=2.0.0"] @@ -807,6 +807,9 @@ def _fetch_relation_data_without_secrets( This is used typically when the Provides side wants to read the Requires side's data, or when the Requires side may want to read its own data. """ + if app not in relation.data or not relation.data[app]: + return {} + if fields: return {k: relation.data[app][k] for k in fields if k in relation.data[app]} else: @@ -830,6 +833,9 @@ def _fetch_relation_data_with_secrets( normal_fields = [] if not fields: + if app not in relation.data or not relation.data[app]: + return {} + all_fields = list(relation.data[app].keys()) normal_fields = [field for field in all_fields if not self._is_secret_field(field)] @@ -853,8 +859,11 @@ def _fetch_relation_data_with_secrets( def _update_relation_data_without_secrets( self, app: Application, relation: Relation, data: Dict[str, str] - ): + ) -> None: """Updating databag contents when no secrets are involved.""" + if app not in relation.data or relation.data[app] is None: + return + if any(self._is_secret_field(key) for key in data.keys()): raise SecretsIllegalUpdateError("Can't update secret {key}.") @@ -865,8 +874,19 @@ def _delete_relation_data_without_secrets( self, app: Application, relation: Relation, fields: List[str] ) -> None: """Remove databag fields 'fields' from Relation.""" + if app not in relation.data or not relation.data[app]: + return + for field in fields: - relation.data[app].pop(field) + try: + relation.data[app].pop(field) + except KeyError: + logger.debug( + "Non-existing field was attempted to be removed from the databag %s, %s", + str(relation.id), + str(field), + ) + pass # Public interface methods # Handling Relation Fields seamlessly, regardless if in databag or a Juju Secret @@ -880,9 +900,6 @@ def get_relation(self, relation_name, relation_id) -> Relation: "Relation %s %s couldn't be retrieved", relation_name, relation_id ) - if not relation.app: - raise DataInterfacesError("Relation's application missing") - return relation def fetch_relation_data( @@ -1089,7 +1106,10 @@ def _delete_relation_secret( # Remove secret from the relation if it's fully gone if not new_content: field = self._generate_secret_field_name(group) - relation.data[self.local_app].pop(field) + try: + relation.data[self.local_app].pop(field) + except KeyError: + pass # Return the content that was removed return True