From aef8c57d91d81229b86f3b67eaafecb0dbb1782b Mon Sep 17 00:00:00 2001 From: Caner Derici Date: Thu, 25 Jan 2024 08:51:49 -0700 Subject: [PATCH 01/10] Avoid adding signal handlers to the event loop at connection 1. Because if someone runs pylibjuju in an off-main thread, then add_signal_handler will (rightfully) complain (see issue #1010). 2. Pylibjuju as a library shouldn't enforce a certain way of handling signals. Clients should install their handlers however they wanna handle them. Fixes #1010 --- juju/client/connection.py | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/juju/client/connection.py b/juju/client/connection.py index ff6d8ee6..0e3d9ef0 100644 --- a/juju/client/connection.py +++ b/juju/client/connection.py @@ -5,7 +5,6 @@ import json import logging import ssl -import signal import urllib.request import weakref from http.client import HTTPSConnection @@ -425,10 +424,6 @@ def _exit_tasks(): for task in jasyncio.all_tasks(): task.cancel() - loop = jasyncio.get_running_loop() - for sig in (signal.SIGINT, signal.SIGTERM): - loop.add_signal_handler(sig, _exit_tasks) - return (await websockets.connect( url, ssl=self._get_ssl(cacert), @@ -473,11 +468,6 @@ async def close(self, to_reconnect=False): if self.proxy is not None: self.proxy.close() - # Remove signal handlers - loop = jasyncio.get_running_loop() - for sig in (signal.SIGINT, signal.SIGTERM): - loop.remove_signal_handler(sig) - async def _recv(self, request_id): if not self.is_open: raise websockets.exceptions.ConnectionClosed(0, 'websocket closed') From c6af97a0dc74120333d0ae033bb0103fe3134476 Mon Sep 17 00:00:00 2001 From: Caner Derici Date: Thu, 25 Jan 2024 08:54:08 -0700 Subject: [PATCH 02/10] Fix typo in the makefile --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index e3d141e1..c2c25a30 100644 --- a/Makefile +++ b/Makefile @@ -24,7 +24,7 @@ run-unit-tests: lint .tox tox -e py3 .PHONY: run-integration-tests -run-unit-tests: lint .tox +run-integration-tests: lint .tox tox -e integration .PHONY: run-all-tests From 06c0e7aff1c2f2c453140565a86b47eb503389ae Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Sat, 27 Jan 2024 22:58:06 +0800 Subject: [PATCH 03/10] feat: remove app block until done --- juju/application.py | 32 ++++++++++++++++++--------- tests/integration/test_application.py | 21 ++++++++++++++---- 2 files changed, 39 insertions(+), 14 deletions(-) diff --git a/juju/application.py b/juju/application.py index 5f824e3a..2134fa9e 100644 --- a/juju/application.py +++ b/juju/application.py @@ -4,17 +4,20 @@ import hashlib import json import logging +import typing from pathlib import Path -from . import model, tag, utils, jasyncio -from .url import URL -from .status import derive_status +from . import jasyncio, model, tag, utils from .annotationhelper import _get_annotations, _set_annotations -from .client import client -from .errors import JujuError, JujuApplicationConfigError from .bundle import get_charm_series, is_local_charm -from .placement import parse as parse_placement +from .client import client +from .errors import JujuApplicationConfigError, JujuError from .origin import Channel, Source +from .placement import parse as parse_placement +from .relation import Relation +from .status import derive_status +from .url import URL +from .utils import block_until log = logging.getLogger(__name__) @@ -59,14 +62,14 @@ def subordinate_units(self): return [u for u in self.units if u.is_subordinate] @property - def relations(self): + def relations(self) -> typing.List[Relation]: return [rel for rel in self.model.relations if rel.matches(self.name)] def related_applications(self, endpoint_name=None): apps = {} for rel in self.relations: if rel.is_peer: - local_ep, remote_ep = rel.endpoints[0] + local_ep, remote_ep = rel.endpoints else: def is_us(ep): return ep.application.name == self.name @@ -191,12 +194,13 @@ async def scale(self, scale=None, scale_change=None): scale_change=scale_change) ]) - async def destroy_relation(self, local_relation, remote_relation): + async def destroy_relation(self, local_relation, remote_relation, block_until_done: bool = False): """Remove a relation to another application. :param str local_relation: Name of relation on this application :param str remote_relation: Name of relation on the other application in the form '[:]' + :param bool block_until_done: Wait until the relation is completely removed. """ if ':' not in local_relation: @@ -207,8 +211,16 @@ async def destroy_relation(self, local_relation, remote_relation): log.debug( 'Destroying relation %s <-> %s', local_relation, remote_relation) - return await app_facade.DestroyRelation(endpoints=[ + await app_facade.DestroyRelation(endpoints=[ local_relation, remote_relation]) + if block_until_done: + await block_until( + not any( + relation.matches(local_relation, remote_relation) + for relation in self.relations + ) + ) + remove_relation = destroy_relation async def destroy_unit(self, *unit_names): diff --git a/tests/integration/test_application.py b/tests/integration/test_application.py index 05252d46..86a21edb 100644 --- a/tests/integration/test_application.py +++ b/tests/integration/test_application.py @@ -1,15 +1,17 @@ # Copyright 2023 Canonical Ltd. # Licensed under the Apache V2, see LICENCE file for details. +import logging from pathlib import Path import pytest -import logging -from .. import base -from juju import jasyncio, errors -from juju.url import URL, Schema +from juju import errors, jasyncio +from juju.application import Application from juju.client import client +from juju.url import URL, Schema + +from .. import base MB = 1 @@ -331,3 +333,14 @@ async def test_app_charm_name(): await model.wait_for_idle(status="active") assert 'ubuntu' in app.charm_url assert 'ubuntu' == app.charm_name + + +@base.bootstrapped +async def test_app_relation_destroy_block_until_done(): + async with base.CleanModel() as model: + app: Application = await model.deploy('docker-registry') + rsa: Application = await model.deploy("easyrsa") + await app.relate("cert-provider", rsa.name) + await model.wait_for_idle(status="active") + await app.destroy_relation("cert-provider", rsa.name, block_until_done=True) + await app.relate("cert-provider", rsa.name) From 6a2a7ab0fe91c8ce8b7f4cf56733e1c99cb14920 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Sat, 27 Jan 2024 23:09:36 +0800 Subject: [PATCH 04/10] feat: remove app timeout --- juju/model.py | 14 ++++++++++++-- tests/integration/test_application.py | 11 +++++++++++ 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/juju/model.py b/juju/model.py index 56af31e7..59637b74 100644 --- a/juju/model.py +++ b/juju/model.py @@ -1000,7 +1000,15 @@ async def remove_storage(self, *storage_ids, force=False, destroy_storage=False) if ret.results[0].error: raise JujuError(ret.results[0].error.message) - async def remove_application(self, app_name, block_until_done=False, force=False, destroy_storage=False, no_wait=False): + async def remove_application( + self, + app_name, + block_until_done=False, + force=False, + destroy_storage=False, + no_wait=False, + timeout=None + ): """Removes the given application from the model. :param str app_name: Name of the application @@ -1009,6 +1017,8 @@ async def remove_application(self, app_name, block_until_done=False, force=False :param bool no_wait: Rush through application removal without waiting for each individual step to complete (=false) :param bool block_until_done: Ensure the app is removed from the model when returned + :param int timeout: Raise asyncio.exceptions.TimeoutError if the application is not removed + within the timeout period. """ if app_name not in self.applications: raise JujuError("Given application doesn't seem to appear in the\ @@ -1019,7 +1029,7 @@ async def remove_application(self, app_name, block_until_done=False, force=False no_wait=no_wait, ) if block_until_done: - await self.block_until(lambda: app_name not in self.applications) + await self.block_until(lambda: app_name not in self.applications, timeout=timeout) async def block_until(self, *conditions, timeout=None, wait_period=0.5): """Return only after all conditions are true. diff --git a/tests/integration/test_application.py b/tests/integration/test_application.py index 86a21edb..b00f9dd2 100644 --- a/tests/integration/test_application.py +++ b/tests/integration/test_application.py @@ -3,6 +3,7 @@ import logging from pathlib import Path +import asyncio import pytest @@ -326,6 +327,16 @@ async def test_app_remove_wait_flag(): assert a_name not in model.applications +@base.bootstrapped +async def test_app_remove_timeout(): + async with base.CleanModel() as model: + app = await model.deploy('ubuntu') + await model.wait_for_idle(status="active") + + with pytest.raises(asyncio.TimeoutError): + await model.remove_application(app.name, block_until_done=True, timeout=1) + + @base.bootstrapped async def test_app_charm_name(): async with base.CleanModel() as model: From 8df075e558f2d5d60f8e7d779ba4f7792499e273 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Sat, 27 Jan 2024 23:09:54 +0800 Subject: [PATCH 05/10] Revert "feat: remove app timeout" This reverts commit 6a2a7ab0fe91c8ce8b7f4cf56733e1c99cb14920. --- juju/model.py | 14 ++------------ tests/integration/test_application.py | 11 ----------- 2 files changed, 2 insertions(+), 23 deletions(-) diff --git a/juju/model.py b/juju/model.py index 59637b74..56af31e7 100644 --- a/juju/model.py +++ b/juju/model.py @@ -1000,15 +1000,7 @@ async def remove_storage(self, *storage_ids, force=False, destroy_storage=False) if ret.results[0].error: raise JujuError(ret.results[0].error.message) - async def remove_application( - self, - app_name, - block_until_done=False, - force=False, - destroy_storage=False, - no_wait=False, - timeout=None - ): + async def remove_application(self, app_name, block_until_done=False, force=False, destroy_storage=False, no_wait=False): """Removes the given application from the model. :param str app_name: Name of the application @@ -1017,8 +1009,6 @@ async def remove_application( :param bool no_wait: Rush through application removal without waiting for each individual step to complete (=false) :param bool block_until_done: Ensure the app is removed from the model when returned - :param int timeout: Raise asyncio.exceptions.TimeoutError if the application is not removed - within the timeout period. """ if app_name not in self.applications: raise JujuError("Given application doesn't seem to appear in the\ @@ -1029,7 +1019,7 @@ async def remove_application( no_wait=no_wait, ) if block_until_done: - await self.block_until(lambda: app_name not in self.applications, timeout=timeout) + await self.block_until(lambda: app_name not in self.applications) async def block_until(self, *conditions, timeout=None, wait_period=0.5): """Return only after all conditions are true. diff --git a/tests/integration/test_application.py b/tests/integration/test_application.py index b00f9dd2..86a21edb 100644 --- a/tests/integration/test_application.py +++ b/tests/integration/test_application.py @@ -3,7 +3,6 @@ import logging from pathlib import Path -import asyncio import pytest @@ -327,16 +326,6 @@ async def test_app_remove_wait_flag(): assert a_name not in model.applications -@base.bootstrapped -async def test_app_remove_timeout(): - async with base.CleanModel() as model: - app = await model.deploy('ubuntu') - await model.wait_for_idle(status="active") - - with pytest.raises(asyncio.TimeoutError): - await model.remove_application(app.name, block_until_done=True, timeout=1) - - @base.bootstrapped async def test_app_charm_name(): async with base.CleanModel() as model: From f28390cf884944fd78727a6813710e76cafcf8e3 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Sat, 27 Jan 2024 23:12:26 +0800 Subject: [PATCH 06/10] feat: remove app timeout --- juju/model.py | 14 ++++++++++++-- tests/integration/test_application.py | 11 +++++++++++ 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/juju/model.py b/juju/model.py index 56af31e7..59637b74 100644 --- a/juju/model.py +++ b/juju/model.py @@ -1000,7 +1000,15 @@ async def remove_storage(self, *storage_ids, force=False, destroy_storage=False) if ret.results[0].error: raise JujuError(ret.results[0].error.message) - async def remove_application(self, app_name, block_until_done=False, force=False, destroy_storage=False, no_wait=False): + async def remove_application( + self, + app_name, + block_until_done=False, + force=False, + destroy_storage=False, + no_wait=False, + timeout=None + ): """Removes the given application from the model. :param str app_name: Name of the application @@ -1009,6 +1017,8 @@ async def remove_application(self, app_name, block_until_done=False, force=False :param bool no_wait: Rush through application removal without waiting for each individual step to complete (=false) :param bool block_until_done: Ensure the app is removed from the model when returned + :param int timeout: Raise asyncio.exceptions.TimeoutError if the application is not removed + within the timeout period. """ if app_name not in self.applications: raise JujuError("Given application doesn't seem to appear in the\ @@ -1019,7 +1029,7 @@ async def remove_application(self, app_name, block_until_done=False, force=False no_wait=no_wait, ) if block_until_done: - await self.block_until(lambda: app_name not in self.applications) + await self.block_until(lambda: app_name not in self.applications, timeout=timeout) async def block_until(self, *conditions, timeout=None, wait_period=0.5): """Return only after all conditions are true. diff --git a/tests/integration/test_application.py b/tests/integration/test_application.py index 05252d46..dfbcd2d4 100644 --- a/tests/integration/test_application.py +++ b/tests/integration/test_application.py @@ -2,6 +2,7 @@ # Licensed under the Apache V2, see LICENCE file for details. from pathlib import Path +import asyncio import pytest import logging @@ -324,6 +325,16 @@ async def test_app_remove_wait_flag(): assert a_name not in model.applications +@base.bootstrapped +async def test_app_remove_timeout(): + async with base.CleanModel() as model: + app = await model.deploy('ubuntu') + await model.wait_for_idle(status="active") + + with pytest.raises(asyncio.TimeoutError): + await model.remove_application(app.name, block_until_done=True, timeout=1) + + @base.bootstrapped async def test_app_charm_name(): async with base.CleanModel() as model: From 2f4813c1b3e2f7794994b5feb91e8f8127f1f10f Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Sun, 28 Jan 2024 12:54:22 +0800 Subject: [PATCH 07/10] fix: callable --- juju/application.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/juju/application.py b/juju/application.py index 2134fa9e..e1e08ef2 100644 --- a/juju/application.py +++ b/juju/application.py @@ -215,7 +215,7 @@ async def destroy_relation(self, local_relation, remote_relation, block_until_do local_relation, remote_relation]) if block_until_done: await block_until( - not any( + lambda: not any( relation.matches(local_relation, remote_relation) for relation in self.relations ) From 0288f6c3c6ce0623e9afc662a97aae0d8281725d Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Sun, 28 Jan 2024 12:55:12 +0800 Subject: [PATCH 08/10] fix: lint --- juju/model.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/juju/model.py b/juju/model.py index 59637b74..ccfd7858 100644 --- a/juju/model.py +++ b/juju/model.py @@ -1008,7 +1008,7 @@ async def remove_application( destroy_storage=False, no_wait=False, timeout=None - ): + ): """Removes the given application from the model. :param str app_name: Name of the application From 5b87acb76cece77c9ea9fdd123cd423f765e5601 Mon Sep 17 00:00:00 2001 From: Yanks Yoon <37652070+yanksyoon@users.noreply.github.com> Date: Wed, 31 Jan 2024 08:57:33 +0800 Subject: [PATCH 09/10] Deploy juju-qa-test Co-authored-by: Caner Derici --- tests/integration/test_application.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/test_application.py b/tests/integration/test_application.py index dfbcd2d4..abbb8be9 100644 --- a/tests/integration/test_application.py +++ b/tests/integration/test_application.py @@ -328,7 +328,7 @@ async def test_app_remove_wait_flag(): @base.bootstrapped async def test_app_remove_timeout(): async with base.CleanModel() as model: - app = await model.deploy('ubuntu') + app = await model.deploy('juju-qa-test') await model.wait_for_idle(status="active") with pytest.raises(asyncio.TimeoutError): From 27cae57d53a79f317abcdfd53fee233eced3187a Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Wed, 31 Jan 2024 18:21:01 +0800 Subject: [PATCH 10/10] chore: use relations assertion --- tests/integration/test_application.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/integration/test_application.py b/tests/integration/test_application.py index 86a21edb..b396f902 100644 --- a/tests/integration/test_application.py +++ b/tests/integration/test_application.py @@ -340,7 +340,7 @@ async def test_app_relation_destroy_block_until_done(): async with base.CleanModel() as model: app: Application = await model.deploy('docker-registry') rsa: Application = await model.deploy("easyrsa") - await app.relate("cert-provider", rsa.name) + relation = await app.relate('cert-provider', rsa.name) await model.wait_for_idle(status="active") - await app.destroy_relation("cert-provider", rsa.name, block_until_done=True) - await app.relate("cert-provider", rsa.name) + await app.destroy_relation('cert-provider', rsa.name, block_until_done=True) + assert relation not in app.relations