From 459f1f6cfbb6c1d8b480dec04367623e59b65d3b Mon Sep 17 00:00:00 2001 From: Dragomir Penev <6687393+dragomirp@users.noreply.github.com> Date: Thu, 27 Jun 2024 00:53:18 +0300 Subject: [PATCH] [MISC] Increase coverage (#505) * Bump libs * Tweak asserts * Bump topology coverage * Increase charm coverage * Increase coverage on charm * Bump libs --- lib/charms/postgresql_k8s/v0/postgresql.py | 11 +- .../postgresql_k8s/v0/postgresql_tls.py | 5 +- lib/charms/tempo_k8s/v1/charm_tracing.py | 301 ++++++++++++------ lib/charms/tempo_k8s/v2/tracing.py | 69 +++- tests/integration/test_charm.py | 4 +- tests/unit/test_charm.py | 55 +++- tests/unit/test_cluster_topology_observer.py | 30 ++ tests/unit/test_db.py | 144 +++++---- tests/unit/test_postgresql_provider.py | 104 +++--- tests/unit/test_upgrade.py | 24 +- tests/unit/test_utils.py | 12 +- 11 files changed, 498 insertions(+), 261 deletions(-) diff --git a/lib/charms/postgresql_k8s/v0/postgresql.py b/lib/charms/postgresql_k8s/v0/postgresql.py index 792d92b8f0..08b559649c 100644 --- a/lib/charms/postgresql_k8s/v0/postgresql.py +++ b/lib/charms/postgresql_k8s/v0/postgresql.py @@ -36,7 +36,7 @@ # Increment this PATCH version before using `charmcraft publish-lib` or reset # to 0 if you are raising the major API version -LIBPATCH = 28 +LIBPATCH = 29 INVALID_EXTRA_USER_ROLE_BLOCKING_MESSAGE = "invalid role(s) for extra user roles" @@ -604,12 +604,11 @@ def build_postgresql_parameters( # Use 25% of the available memory for shared_buffers. # and the remaining as cache memory. shared_buffers = int(available_memory * 0.25) + parameters["shared_buffers"] = f"{int(shared_buffers * 128 / 10**6)}" effective_cache_size = int(available_memory - shared_buffers) - parameters.setdefault("shared_buffers", f"{int(shared_buffers / 10**6)}MB") - parameters.update({"effective_cache_size": f"{int(effective_cache_size / 10**6)}MB"}) - else: - # Return default - parameters.setdefault("shared_buffers", "128MB") + parameters.update({ + "effective_cache_size": f"{int(effective_cache_size / 10**6) * 128}" + }) return parameters def validate_date_style(self, date_style: str) -> bool: diff --git a/lib/charms/postgresql_k8s/v0/postgresql_tls.py b/lib/charms/postgresql_k8s/v0/postgresql_tls.py index 450752be98..740a607fbc 100644 --- a/lib/charms/postgresql_k8s/v0/postgresql_tls.py +++ b/lib/charms/postgresql_k8s/v0/postgresql_tls.py @@ -34,6 +34,7 @@ from ops.charm import ActionEvent, RelationBrokenEvent from ops.framework import Object from ops.pebble import ConnectionError, PathError, ProtocolError +from tenacity import RetryError # The unique Charmhub library identifier, never change it LIBID = "c27af44a92df4ef38d7ae06418b2800f" @@ -43,7 +44,7 @@ # Increment this PATCH version before using `charmcraft publish-lib` or reset # to 0 if you are raising the major API version. -LIBPATCH = 8 +LIBPATCH = 9 logger = logging.getLogger(__name__) SCOPE = "unit" @@ -142,7 +143,7 @@ def _on_certificate_available(self, event: CertificateAvailableEvent) -> None: logger.debug("Cannot push TLS certificates at this moment") event.defer() return - except (ConnectionError, PathError, ProtocolError) as e: + except (ConnectionError, PathError, ProtocolError, RetryError) as e: logger.error("Cannot push TLS certificates: %r", e) event.defer() return diff --git a/lib/charms/tempo_k8s/v1/charm_tracing.py b/lib/charms/tempo_k8s/v1/charm_tracing.py index 000e0cb578..ebe022e00d 100644 --- a/lib/charms/tempo_k8s/v1/charm_tracing.py +++ b/lib/charms/tempo_k8s/v1/charm_tracing.py @@ -9,21 +9,57 @@ This means that, if your charm is related to, for example, COS' Tempo charm, you will be able to inspect in real time from the Grafana dashboard the execution flow of your charm. -To start using this library, you need to do two things: +# Quickstart +Fetch the following charm libs (and ensure the minimum version/revision numbers are satisfied): + + charmcraft fetch-lib charms.tempo_k8s.v2.tracing # >= 1.10 + charmcraft fetch-lib charms.tempo_k8s.v1.charm_tracing # >= 2.7 + +Then edit your charm code to include: + +```python +# import the necessary charm libs +from charms.tempo_k8s.v2.tracing import TracingEndpointRequirer, charm_tracing_config +from charms.tempo_k8s.v1.charm_tracing import charm_tracing + +# decorate your charm class with charm_tracing: +@charm_tracing( + # forward-declare the instance attributes that the instrumentor will look up to obtain the + # tempo endpoint and server certificate + tracing_endpoint="tracing_endpoint", + server_cert="server_cert" +) +class MyCharm(CharmBase): + _path_to_cert = "/path/to/cert.crt" + # path to cert file **in the charm container**. Its presence will be used to determine whether + # the charm is ready to use tls for encrypting charm traces. If your charm does not support tls, + # you can ignore this and pass None to charm_tracing_config. + # If you do support TLS, you'll need to make sure that the server cert is copied to this location + # and kept up to date so the instrumentor can use it. + + def __init__(self, ...): + ... + self.tracing = TracingEndpointRequirer(self, ...) + self.tracing_endpoint, self.server_cert = charm_tracing_config(self.tracing, self._path_to_cert) +``` + +# Detailed usage +To use this library, you need to do two things: 1) decorate your charm class with `@trace_charm(tracing_endpoint="my_tracing_endpoint")` -2) add to your charm a "my_tracing_endpoint" (you can name this attribute whatever you like) **property** -that returns an otlp http/https endpoint url. If you are using the `TracingEndpointProvider` as -`self.tracing = TracingEndpointProvider(self)`, the implementation could be: +2) add to your charm a "my_tracing_endpoint" (you can name this attribute whatever you like) +**property**, **method** or **instance attribute** that returns an otlp http/https endpoint url. +If you are using the ``charms.tempo_k8s.v2.tracing.TracingEndpointRequirer`` as +``self.tracing = TracingEndpointRequirer(self)``, the implementation could be: ``` @property def my_tracing_endpoint(self) -> Optional[str]: '''Tempo endpoint for charm tracing''' if self.tracing.is_ready(): - return self.tracing.otlp_http_endpoint() + return self.tracing.get_endpoint("otlp_http") else: return None ``` @@ -33,19 +69,52 @@ def my_tracing_endpoint(self) -> Optional[str]: - every event as a span (including custom events) - every charm method call (except dunders) as a span -if you wish to add more fine-grained information to the trace, you can do so by getting a hold of the tracer like so: + +## TLS support +If your charm integrates with a TLS provider which is also trusted by the tracing provider (the Tempo charm), +you can configure ``charm_tracing`` to use TLS by passing a ``server_cert`` parameter to the decorator. + +If your charm is not trusting the same CA as the Tempo endpoint it is sending traces to, +you'll need to implement a cert-transfer relation to obtain the CA certificate from the same +CA that Tempo is using. + +For example: +``` +from charms.tempo_k8s.v1.charm_tracing import trace_charm +@trace_charm( + tracing_endpoint="my_tracing_endpoint", + server_cert="_server_cert" +) +class MyCharm(CharmBase): + self._server_cert = "/path/to/server.crt" + ... + + def on_tls_changed(self, e) -> Optional[str]: + # update the server cert on the charm container for charm tracing + Path(self._server_cert).write_text(self.get_server_cert()) + + def on_tls_broken(self, e) -> Optional[str]: + # remove the server cert so charm_tracing won't try to use tls anymore + Path(self._server_cert).unlink() +``` + + +## More fine-grained manual instrumentation +if you wish to add more spans to the trace, you can do so by getting a hold of the tracer like so: ``` import opentelemetry ... - @property - def tracer(self) -> opentelemetry.trace.Tracer: - return opentelemetry.trace.get_tracer(type(self).__name__) +def get_tracer(self) -> opentelemetry.trace.Tracer: + return opentelemetry.trace.get_tracer(type(self).__name__) ``` By default, the tracer is named after the charm type. If you wish to override that, you can pass -a different `service_name` argument to `trace_charm`. +a different ``service_name`` argument to ``trace_charm``. + +See the official opentelemetry Python SDK documentation for usage: +https://opentelemetry-python.readthedocs.io/en/latest/ -*Upgrading from `v0`:* +## Upgrading from `v0` If you are upgrading from `charm_tracing` v0, you need to take the following steps (assuming you already have the newest version of the library in your charm): @@ -55,8 +124,9 @@ def tracer(self) -> opentelemetry.trace.Tracer: `opentelemetry-exporter-otlp-proto-http>=1.21.0`. -2) Update the charm method referenced to from `@trace` and `@trace_charm`, -to return from `TracingEndpointRequirer.otlp_http_endpoint()` instead of `grpc_http`. For example: +2) Update the charm method referenced to from ``@trace`` and ``@trace_charm``, +to return from ``TracingEndpointRequirer.get_endpoint("otlp_http")`` instead of ``grpc_http``. +For example: ``` from charms.tempo_k8s.v0.charm_tracing import trace_charm @@ -72,7 +142,7 @@ class MyCharm(CharmBase): def my_tracing_endpoint(self) -> Optional[str]: '''Tempo endpoint for charm tracing''' if self.tracing.is_ready(): - return self.tracing.otlp_grpc_endpoint() + return self.tracing.otlp_grpc_endpoint() # OLD API, DEPRECATED. else: return None ``` @@ -93,13 +163,13 @@ class MyCharm(CharmBase): def my_tracing_endpoint(self) -> Optional[str]: '''Tempo endpoint for charm tracing''' if self.tracing.is_ready(): - return self.tracing.otlp_http_endpoint() + return self.tracing.get_endpoint("otlp_http") # NEW API, use this. else: return None ``` -3) If you were passing a certificate using `server_cert`, you need to change it to provide an *absolute* path to -the certificate file. +3) If you were passing a certificate (str) using `server_cert`, you need to change it to +provide an *absolute* path to the certificate file instead. """ import functools @@ -122,19 +192,19 @@ def my_tracing_endpoint(self) -> Optional[str]: ) import opentelemetry +import ops from opentelemetry.exporter.otlp.proto.http.trace_exporter import OTLPSpanExporter from opentelemetry.sdk.resources import Resource from opentelemetry.sdk.trace import Span, TracerProvider from opentelemetry.sdk.trace.export import BatchSpanProcessor +from opentelemetry.trace import INVALID_SPAN, Tracer +from opentelemetry.trace import get_current_span as otlp_get_current_span from opentelemetry.trace import ( - INVALID_SPAN, - Tracer, get_tracer, get_tracer_provider, set_span_in_context, set_tracer_provider, ) -from opentelemetry.trace import get_current_span as otlp_get_current_span from ops.charm import CharmBase from ops.framework import Framework @@ -147,14 +217,23 @@ def my_tracing_endpoint(self) -> Optional[str]: # Increment this PATCH version before using `charmcraft publish-lib` or reset # to 0 if you are raising the major API version -LIBPATCH = 8 +LIBPATCH = 11 PYDEPS = ["opentelemetry-exporter-otlp-proto-http==1.21.0"] logger = logging.getLogger("tracing") +dev_logger = logging.getLogger("tracing-dev") + +# set this to 0 if you are debugging/developing this library source +dev_logger.setLevel(logging.CRITICAL) + +_CharmType = Type[CharmBase] # the type CharmBase and any subclass thereof +_C = TypeVar("_C", bound=_CharmType) +_T = TypeVar("_T", bound=type) +_F = TypeVar("_F", bound=Type[Callable]) tracer: ContextVar[Tracer] = ContextVar("tracer") -_GetterType = Union[Callable[[CharmBase], Optional[str]], property] +_GetterType = Union[Callable[[_CharmType], Optional[str]], property] CHARM_TRACING_ENABLED = "CHARM_TRACING_ENABLED" @@ -220,11 +299,6 @@ def _span(name: str) -> Generator[Optional[Span], Any, Any]: yield None -_C = TypeVar("_C", bound=Type[CharmBase]) -_T = TypeVar("_T", bound=type) -_F = TypeVar("_F", bound=Type[Callable]) - - class TracingError(RuntimeError): """Base class for errors raised by this module.""" @@ -233,60 +307,78 @@ class UntraceableObjectError(TracingError): """Raised when an object you're attempting to instrument cannot be autoinstrumented.""" -def _get_tracing_endpoint(tracing_endpoint_getter, self, charm): - if isinstance(tracing_endpoint_getter, property): - tracing_endpoint = tracing_endpoint_getter.__get__(self) - else: # method or callable - tracing_endpoint = tracing_endpoint_getter(self) +class TLSError(TracingError): + """Raised when the tracing endpoint is https but we don't have a cert yet.""" + + +def _get_tracing_endpoint( + tracing_endpoint_attr: str, + charm_instance: object, + charm_type: type, +): + _tracing_endpoint = getattr(charm_instance, tracing_endpoint_attr) + if callable(_tracing_endpoint): + tracing_endpoint = _tracing_endpoint() + else: + tracing_endpoint = _tracing_endpoint if tracing_endpoint is None: - logger.debug( - f"{charm}.{tracing_endpoint_getter} returned None; quietly disabling " - f"charm_tracing for the run." - ) return + elif not isinstance(tracing_endpoint, str): raise TypeError( - f"{charm}.{tracing_endpoint_getter} should return a tempo endpoint (string); " + f"{charm_type.__name__}.{tracing_endpoint_attr} should resolve to a tempo endpoint (string); " f"got {tracing_endpoint} instead." ) - else: - logger.debug(f"Setting up span exporter to endpoint: {tracing_endpoint}/v1/traces") + + dev_logger.debug(f"Setting up span exporter to endpoint: {tracing_endpoint}/v1/traces") return f"{tracing_endpoint}/v1/traces" -def _get_server_cert(server_cert_getter, self, charm): - if isinstance(server_cert_getter, property): - server_cert = server_cert_getter.__get__(self) - else: # method or callable - server_cert = server_cert_getter(self) +def _get_server_cert( + server_cert_attr: str, + charm_instance: ops.CharmBase, + charm_type: Type[ops.CharmBase], +): + _server_cert = getattr(charm_instance, server_cert_attr) + if callable(_server_cert): + server_cert = _server_cert() + else: + server_cert = _server_cert if server_cert is None: logger.warning( - f"{charm}.{server_cert_getter} returned None; sending traces over INSECURE connection." + f"{charm_type}.{server_cert_attr} is None; sending traces over INSECURE connection." ) return elif not Path(server_cert).is_absolute(): raise ValueError( - f"{charm}.{server_cert_getter} should return a valid tls cert absolute path (string | Path)); " + f"{charm_type}.{server_cert_attr} should resolve to a valid tls cert absolute path (string | Path)); " f"got {server_cert} instead." ) return server_cert def _setup_root_span_initializer( - charm: Type[CharmBase], - tracing_endpoint_getter: _GetterType, - server_cert_getter: Optional[_GetterType], + charm_type: _CharmType, + tracing_endpoint_attr: str, + server_cert_attr: Optional[str], service_name: Optional[str] = None, ): """Patch the charm's initializer.""" - original_init = charm.__init__ + original_init = charm_type.__init__ @functools.wraps(original_init) def wrap_init(self: CharmBase, framework: Framework, *args, **kwargs): + # we're using 'self' here because this is charm init code, makes sense to read what's below + # from the perspective of the charm. Self.unit.name... + original_init(self, framework, *args, **kwargs) + # we call this from inside the init context instead of, say, _autoinstrument, because we want it to + # be checked on a per-charm-instantiation basis, not on a per-type-declaration one. if not is_enabled(): + # this will only happen during unittesting, hopefully, so it's fine to log a + # bit more verbosely logger.info("Tracing DISABLED: skipping root span initialization") return @@ -295,41 +387,41 @@ def wrap_init(self: CharmBase, framework: Framework, *args, **kwargs): # self.handle = Handle(None, self.handle_kind, None) original_event_context = framework._event_context + # default service name isn't just app name because it could conflict with the workload service name + _service_name = service_name or f"{self.app.name}-charm" - _service_name = service_name or self.app.name - + unit_name = self.unit.name resource = Resource.create( attributes={ "service.name": _service_name, "compose_service": _service_name, "charm_type": type(self).__name__, # juju topology - "juju_unit": self.unit.name, + "juju_unit": unit_name, "juju_application": self.app.name, "juju_model": self.model.name, "juju_model_uuid": self.model.uuid, } ) provider = TracerProvider(resource=resource) - try: - tracing_endpoint = _get_tracing_endpoint(tracing_endpoint_getter, self, charm) - except Exception: - # if anything goes wrong with retrieving the endpoint, we go on with tracing disabled. - # better than breaking the charm. - logger.exception( - f"exception retrieving the tracing " - f"endpoint from {charm}.{tracing_endpoint_getter}; " - f"proceeding with charm_tracing DISABLED. " - ) - return + + # if anything goes wrong with retrieving the endpoint, we let the exception bubble up. + tracing_endpoint = _get_tracing_endpoint(tracing_endpoint_attr, self, charm_type) if not tracing_endpoint: + # tracing is off if tracing_endpoint is None return server_cert: Optional[Union[str, Path]] = ( - _get_server_cert(server_cert_getter, self, charm) if server_cert_getter else None + _get_server_cert(server_cert_attr, self, charm_type) if server_cert_attr else None ) + if tracing_endpoint.startswith("https://") and not server_cert: + raise TLSError( + "Tracing endpoint is https, but no server_cert has been passed." + "Please point @trace_charm to a `server_cert` attr." + ) + exporter = OTLPSpanExporter( endpoint=tracing_endpoint, certificate_file=str(Path(server_cert).absolute()) if server_cert else None, @@ -342,16 +434,18 @@ def wrap_init(self: CharmBase, framework: Framework, *args, **kwargs): _tracer = get_tracer(_service_name) # type: ignore _tracer_token = tracer.set(_tracer) - dispatch_path = os.getenv("JUJU_DISPATCH_PATH", "") + dispatch_path = os.getenv("JUJU_DISPATCH_PATH", "") # something like hooks/install + event_name = dispatch_path.split("/")[1] if "/" in dispatch_path else dispatch_path + root_span_name = f"{unit_name}: {event_name} event" + span = _tracer.start_span(root_span_name, attributes={"juju.dispatch_path": dispatch_path}) # all these shenanigans are to work around the fact that the opentelemetry tracing API is built # on the assumption that spans will be used as contextmanagers. # Since we don't (as we need to close the span on framework.commit), # we need to manually set the root span as current. - span = _tracer.start_span("charm exec", attributes={"juju.dispatch_path": dispatch_path}) ctx = set_span_in_context(span) - # log a trace id so we can look it up in tempo. + # log a trace id, so we can pick it up from the logs (and jhack) to look it up in tempo. root_trace_id = hex(span.get_span_context().trace_id)[2:] # strip 0x prefix logger.debug(f"Starting root trace with id={root_trace_id!r}.") @@ -359,6 +453,7 @@ def wrap_init(self: CharmBase, framework: Framework, *args, **kwargs): @contextmanager def wrap_event_context(event_name: str): + dev_logger.info(f"entering event context: {event_name}") # when the framework enters an event context, we create a span. with _span("event: " + event_name) as event_context_span: if event_context_span: @@ -372,6 +467,7 @@ def wrap_event_context(event_name: str): @functools.wraps(original_close) def wrap_close(): + dev_logger.info("tearing down tracer and flushing traces") span.end() opentelemetry.context.detach(span_token) # type: ignore tracer.reset(_tracer_token) @@ -383,7 +479,7 @@ def wrap_close(): framework.close = wrap_close return - charm.__init__ = wrap_init + charm_type.__init__ = wrap_init # type: ignore def trace_charm( @@ -391,7 +487,7 @@ def trace_charm( server_cert: Optional[str] = None, service_name: Optional[str] = None, extra_types: Sequence[type] = (), -): +) -> Callable[[_T], _T]: """Autoinstrument the decorated charm with tracing telemetry. Use this function to get out-of-the-box traces for all events emitted on this charm and all @@ -399,7 +495,7 @@ def trace_charm( Usage: >>> from charms.tempo_k8s.v1.charm_tracing import trace_charm - >>> from charms.tempo_k8s.v1.tracing import TracingEndpointProvider + >>> from charms.tempo_k8s.v1.tracing import TracingEndpointRequirer >>> from ops import CharmBase >>> >>> @trace_charm( @@ -409,7 +505,7 @@ def trace_charm( >>> >>> def __init__(self, framework: Framework): >>> ... - >>> self.tracing = TracingEndpointProvider(self) + >>> self.tracing = TracingEndpointRequirer(self) >>> >>> @property >>> def tempo_otlp_http_endpoint(self) -> Optional[str]: @@ -418,24 +514,28 @@ def trace_charm( >>> else: >>> return None >>> - :param server_cert: method or property on the charm type that returns an - optional absolute path to a tls certificate to be used when sending traces to a remote server. - If it returns None, an _insecure_ connection will be used. - :param tracing_endpoint: name of a property on the charm type that returns an - optional (fully resolvable) tempo url. If None, tracing will be effectively disabled. Else, traces will be - pushed to that endpoint. + + :param tracing_endpoint: name of a method, property or attribute on the charm type that returns an + optional (fully resolvable) tempo url to which the charm traces will be pushed. + If None, tracing will be effectively disabled. + :param server_cert: name of a method, property or attribute on the charm type that returns an + optional absolute path to a CA certificate file to be used when sending traces to a remote server. + If it returns None, an _insecure_ connection will be used. To avoid errors in transient + situations where the endpoint is already https but there is no certificate on disk yet, it + is recommended to disable tracing (by returning None from the tracing_endpoint) altogether + until the cert has been written to disk. :param service_name: service name tag to attach to all traces generated by this charm. Defaults to the juju application name this charm is deployed under. :param extra_types: pass any number of types that you also wish to autoinstrument. For example, charm libs, relation endpoint wrappers, workload abstractions, ... """ - def _decorator(charm_type: Type[CharmBase]): + def _decorator(charm_type: _T) -> _T: """Autoinstrument the wrapped charmbase type.""" _autoinstrument( charm_type, - tracing_endpoint_getter=getattr(charm_type, tracing_endpoint), - server_cert_getter=getattr(charm_type, server_cert) if server_cert else None, + tracing_endpoint_attr=tracing_endpoint, + server_cert_attr=server_cert, service_name=service_name, extra_types=extra_types, ) @@ -445,12 +545,12 @@ def _decorator(charm_type: Type[CharmBase]): def _autoinstrument( - charm_type: Type[CharmBase], - tracing_endpoint_getter: _GetterType, - server_cert_getter: Optional[_GetterType] = None, + charm_type: _T, + tracing_endpoint_attr: str, + server_cert_attr: Optional[str] = None, service_name: Optional[str] = None, extra_types: Sequence[type] = (), -) -> Type[CharmBase]: +) -> _T: """Set up tracing on this charm class. Use this function to get out-of-the-box traces for all events emitted on this charm and all @@ -462,29 +562,32 @@ def _autoinstrument( >>> from ops.main import main >>> _autoinstrument( >>> MyCharm, - >>> tracing_endpoint_getter=MyCharm.tempo_otlp_http_endpoint, + >>> tracing_endpoint_attr="tempo_otlp_http_endpoint", >>> service_name="MyCharm", >>> extra_types=(Foo, Bar) >>> ) >>> main(MyCharm) :param charm_type: the CharmBase subclass to autoinstrument. - :param server_cert_getter: method or property on the charm type that returns an - optional absolute path to a tls certificate to be used when sending traces to a remote server. - This needs to be a valid path to a certificate. - :param tracing_endpoint_getter: method or property on the charm type that returns an - optional tempo url. If None, tracing will be effectively disabled. Else, traces will be - pushed to that endpoint. + :param tracing_endpoint_attr: name of a method, property or attribute on the charm type that returns an + optional (fully resolvable) tempo url to which the charm traces will be pushed. + If None, tracing will be effectively disabled. + :param server_cert_attr: name of a method, property or attribute on the charm type that returns an + optional absolute path to a CA certificate file to be used when sending traces to a remote server. + If it returns None, an _insecure_ connection will be used. To avoid errors in transient + situations where the endpoint is already https but there is no certificate on disk yet, it + is recommended to disable tracing (by returning None from the tracing_endpoint) altogether + until the cert has been written to disk. :param service_name: service name tag to attach to all traces generated by this charm. Defaults to the juju application name this charm is deployed under. :param extra_types: pass any number of types that you also wish to autoinstrument. For example, charm libs, relation endpoint wrappers, workload abstractions, ... """ - logger.info(f"instrumenting {charm_type}") + dev_logger.info(f"instrumenting {charm_type}") _setup_root_span_initializer( charm_type, - tracing_endpoint_getter, - server_cert_getter=server_cert_getter, + tracing_endpoint_attr, + server_cert_attr=server_cert_attr, service_name=service_name, ) trace_type(charm_type) @@ -501,12 +604,12 @@ def trace_type(cls: _T) -> _T: It assumes that this class is only instantiated after a charm type decorated with `@trace_charm` has been instantiated. """ - logger.info(f"instrumenting {cls}") + dev_logger.info(f"instrumenting {cls}") for name, method in inspect.getmembers(cls, predicate=inspect.isfunction): - logger.info(f"discovered {method}") + dev_logger.info(f"discovered {method}") if method.__name__.startswith("__"): - logger.info(f"skipping {method} (dunder)") + dev_logger.info(f"skipping {method} (dunder)") continue new_method = trace_method(method) @@ -534,7 +637,7 @@ def trace_function(function: _F) -> _F: def _trace_callable(callable: _F, qualifier: str) -> _F: - logger.info(f"instrumenting {callable}") + dev_logger.info(f"instrumenting {callable}") # sig = inspect.signature(callable) @functools.wraps(callable) diff --git a/lib/charms/tempo_k8s/v2/tracing.py b/lib/charms/tempo_k8s/v2/tracing.py index b4e341c349..8b9fb4f32d 100644 --- a/lib/charms/tempo_k8s/v2/tracing.py +++ b/lib/charms/tempo_k8s/v2/tracing.py @@ -72,6 +72,7 @@ def __init__(self, *args): import enum import json import logging +from pathlib import Path from typing import ( TYPE_CHECKING, Any, @@ -82,6 +83,7 @@ def __init__(self, *args): Optional, Sequence, Tuple, + Union, cast, ) @@ -105,7 +107,7 @@ def __init__(self, *args): # Increment this PATCH version before using `charmcraft publish-lib` or reset # to 0 if you are raising the major API version -LIBPATCH = 6 +LIBPATCH = 7 PYDEPS = ["pydantic"] @@ -921,3 +923,68 @@ def get_endpoint( return None return endpoint + + +def charm_tracing_config( + endpoint_requirer: TracingEndpointRequirer, cert_path: Optional[Union[Path, str]] +) -> Tuple[Optional[str], Optional[str]]: + """Utility function to determine the charm_tracing config you will likely want. + + If no endpoint is provided: + disable charm tracing. + If https endpoint is provided but cert_path is not found on disk: + disable charm tracing. + If https endpoint is provided and cert_path is None: + ERROR + Else: + proceed with charm tracing (with or without tls, as appropriate) + + Usage: + If you are using charm_tracing >= v1.9: + >>> from lib.charms.tempo_k8s.v1.charm_tracing import trace_charm + >>> from lib.charms.tempo_k8s.v2.tracing import charm_tracing_config + >>> @trace_charm(tracing_endpoint="my_endpoint", cert_path="cert_path") + >>> class MyCharm(...): + >>> _cert_path = "/path/to/cert/on/charm/container.crt" + >>> def __init__(self, ...): + >>> self.tracing = TracingEndpointRequirer(...) + >>> self.my_endpoint, self.cert_path = charm_tracing_config( + ... self.tracing, self._cert_path) + + If you are using charm_tracing < v1.9: + >>> from lib.charms.tempo_k8s.v1.charm_tracing import trace_charm + >>> from lib.charms.tempo_k8s.v2.tracing import charm_tracing_config + >>> @trace_charm(tracing_endpoint="my_endpoint", cert_path="cert_path") + >>> class MyCharm(...): + >>> _cert_path = "/path/to/cert/on/charm/container.crt" + >>> def __init__(self, ...): + >>> self.tracing = TracingEndpointRequirer(...) + >>> self._my_endpoint, self._cert_path = charm_tracing_config( + ... self.tracing, self._cert_path) + >>> @property + >>> def my_endpoint(self): + >>> return self._my_endpoint + >>> @property + >>> def cert_path(self): + >>> return self._cert_path + + """ + if not endpoint_requirer.is_ready(): + return None, None + + endpoint = endpoint_requirer.get_endpoint("otlp_http") + if not endpoint: + return None, None + + is_https = endpoint.startswith("https://") + + if is_https: + if cert_path is None: + raise TracingError("Cannot send traces to an https endpoint without a certificate.") + elif not Path(cert_path).exists(): + # if endpoint is https BUT we don't have a server_cert yet: + # disable charm tracing until we do to prevent tls errors + return None, None + return endpoint, str(cert_path) + else: + return endpoint, None diff --git a/tests/integration/test_charm.py b/tests/integration/test_charm.py index 12ac5c5a46..b2c24ef068 100644 --- a/tests/integration/test_charm.py +++ b/tests/integration/test_charm.py @@ -171,7 +171,7 @@ async def test_postgresql_parameters_change(ops_test: OpsTest) -> None: """Test that's possible to change PostgreSQL parameters.""" await ops_test.model.applications[DATABASE_APP_NAME].set_config({ "memory_max_prepared_transactions": "100", - "memory_shared_buffers": "128", + "memory_shared_buffers": "32768", # 2 * 128MB. Patroni may refuse the config if < 128MB "response_lc_monetary": "en_GB.utf8", "experimental_max_connections": "200", }) @@ -204,7 +204,7 @@ async def test_postgresql_parameters_change(ops_test: OpsTest) -> None: # Validate each configuration set by Patroni on PostgreSQL. assert settings["max_prepared_transactions"] == "100" - assert settings["shared_buffers"] == "128" + assert settings["shared_buffers"] == "32768" assert settings["lc_monetary"] == "en_GB.utf8" assert settings["max_connections"] == "200" finally: diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index e69dad14fd..1e0cc81d02 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -32,7 +32,7 @@ PRIMARY_NOT_REACHABLE_MESSAGE, PostgresqlOperatorCharm, ) -from cluster import RemoveRaftMemberFailedError +from cluster import NotReadyError, RemoveRaftMemberFailedError from constants import PEER, POSTGRESQL_SNAP_NAME, SECRET_INTERNAL_LABEL, SNAP_PACKAGES from tests.helpers import patch_network_get @@ -1738,6 +1738,59 @@ def test_client_relations(harness): assert harness.charm.client_relations == [database_relation, db_relation, db_admin_relation] +def test_on_pgdata_storage_detaching(harness): + with ( + patch( + "charm.PostgresqlOperatorCharm._update_relation_endpoints" + ) as _update_relation_endpoints, + patch("charm.PostgresqlOperatorCharm.primary_endpoint", new_callable=PropertyMock), + patch("charm.Patroni.are_all_members_ready") as _are_all_members_ready, + patch("charm.Patroni.get_primary", return_value="primary") as _get_primary, + patch("charm.Patroni.switchover") as _switchover, + patch("charm.Patroni.primary_changed") as _primary_changed, + ): + # Early exit if not primary + event = Mock() + harness.charm._on_pgdata_storage_detaching(event) + assert not _are_all_members_ready.called + + _get_primary.side_effect = [harness.charm.unit.name, "primary"] + harness.charm._on_pgdata_storage_detaching(event) + _switchover.assert_called_once_with() + _primary_changed.assert_called_once_with("primary") + _update_relation_endpoints.assert_called_once_with() + + +def test_add_cluster_member(harness): + with ( + patch("charm.PostgresqlOperatorCharm.update_config") as _update_config, + patch("charm.PostgresqlOperatorCharm._get_unit_ip", return_value="1.1.1.1"), + patch("charm.PostgresqlOperatorCharm._add_to_members_ips") as _add_to_members_ips, + patch("charm.Patroni.are_all_members_ready") as _are_all_members_ready, + ): + harness.charm.add_cluster_member("postgresql/0") + + _add_to_members_ips.assert_called_once_with("1.1.1.1") + _update_config.assert_called_once_with() + _update_config.reset_mock() + + # Charm blocks when update_config fails + _update_config.side_effect = RetryError(last_attempt=None) + harness.charm.add_cluster_member("postgresql/0") + _update_config.assert_called_once_with() + assert isinstance(harness.charm.unit.status, BlockedStatus) + assert harness.charm.unit.status.message == "failed to update cluster members on member" + _update_config.reset_mock() + + # Not ready error if not all members are ready + _are_all_members_ready.return_value = False + try: + harness.charm.add_cluster_member("postgresql/0") + assert False + except NotReadyError: + pass + + # # Secrets # diff --git a/tests/unit/test_cluster_topology_observer.py b/tests/unit/test_cluster_topology_observer.py index 507db774b9..3a40668854 100644 --- a/tests/unit/test_cluster_topology_observer.py +++ b/tests/unit/test_cluster_topology_observer.py @@ -94,6 +94,28 @@ def test_start_observer(harness): _popen.assert_called_once() +def test_start_observer_already_running(harness): + with ( + patch("builtins.open") as _open, + patch("subprocess.Popen") as _popen, + patch("os.kill") as _kill, + patch.object(MockCharm, "_peers", new_callable=PropertyMock) as _peers, + ): + harness.charm.unit.status = ActiveStatus() + _peers.return_value = Mock(data={harness.charm.unit: {"observer-pid": "1234"}}) + harness.charm.observer.start_observer() + _kill.assert_called_once_with(1234, 0) + assert not _popen.called + _kill.reset_mock() + + # If process is already dead, it should restart + _kill.side_effect = OSError + harness.charm.observer.start_observer() + _kill.assert_called_once_with(1234, 0) + _popen.assert_called_once() + _kill.reset_mock() + + def test_stop_observer(harness): with ( patch("os.kill") as _kill, @@ -111,6 +133,14 @@ def test_stop_observer(harness): _peers.return_value = Mock(data={harness.charm.unit: {"observer-pid": "1"}}) harness.charm.observer.stop_observer() _kill.assert_called_once_with(1, signal.SIGINT) + _kill.reset_mock() + + # Dead process doesn't break the script + _peers.return_value = Mock(data={harness.charm.unit: {"observer-pid": "1"}}) + _kill.side_effect = OSError + harness.charm.observer.stop_observer() + _kill.assert_called_once_with(1, signal.SIGINT) + _kill.reset_mock() def test_dispatch(harness): diff --git a/tests/unit/test_db.py b/tests/unit/test_db.py index e241adab99..b518757567 100644 --- a/tests/unit/test_db.py +++ b/tests/unit/test_db.py @@ -1,7 +1,6 @@ # Copyright 2022 Canonical Ltd. # See LICENSE file for licensing details. -from unittest import TestCase from unittest.mock import Mock, PropertyMock, patch import pytest @@ -22,9 +21,6 @@ RELATION_NAME = "db" POSTGRESQL_VERSION = "12" -# used for assert functions -tc = TestCase() - @pytest.fixture(autouse=True) def harness(): @@ -121,7 +117,7 @@ def test_on_relation_changed(harness): # Request a database before primary endpoint is available. request_database(harness) - tc.assertEqual(_defer.call_count, 2) + assert _defer.call_count == 2 _set_up_relation.assert_not_called() # Request it again when the database is ready. @@ -136,7 +132,7 @@ def test_get_extensions(harness): # Test when there are no extensions in the relation databags. rel_id = harness.model.get_relation(RELATION_NAME).id relation = harness.model.get_relation(RELATION_NAME, rel_id) - tc.assertEqual(harness.charm.legacy_db_relation._get_extensions(relation), ([], set())) + assert harness.charm.legacy_db_relation._get_extensions(relation) == ([], set()) # Test when there are extensions in the application relation databag. extensions = ["", "citext:public", "debversion"] @@ -146,9 +142,9 @@ def test_get_extensions(harness): "application", {"extensions": ",".join(extensions)}, ) - tc.assertEqual( - harness.charm.legacy_db_relation._get_extensions(relation), - ([extensions[1], extensions[2]], {extensions[1].split(":")[0], extensions[2]}), + assert harness.charm.legacy_db_relation._get_extensions(relation) == ( + [extensions[1], extensions[2]], + {extensions[1].split(":")[0], extensions[2]}, ) # Test when there are extensions in the unit relation databag. @@ -163,9 +159,9 @@ def test_get_extensions(harness): "application/0", {"extensions": ",".join(extensions)}, ) - tc.assertEqual( - harness.charm.legacy_db_relation._get_extensions(relation), - ([extensions[1], extensions[2]], {extensions[1].split(":")[0], extensions[2]}), + assert harness.charm.legacy_db_relation._get_extensions(relation) == ( + [extensions[1], extensions[2]], + {extensions[1].split(":")[0], extensions[2]}, ) # Test when one of the plugins/extensions is enabled. @@ -179,9 +175,9 @@ def test_get_extensions(harness): harness = Harness(PostgresqlOperatorCharm, config=config) harness.cleanup() harness.begin() - tc.assertEqual( - harness.charm.legacy_db_relation._get_extensions(relation), - ([extensions[1], extensions[2]], {extensions[2]}), + assert harness.charm.legacy_db_relation._get_extensions(relation) == ( + [extensions[1], extensions[2]], + {extensions[2]}, ) @@ -217,7 +213,7 @@ def test_set_up_relation(harness): # Assert no operation is done when at least one of the requested extensions # is disabled. relation = harness.model.get_relation(RELATION_NAME, rel_id) - tc.assertFalse(harness.charm.legacy_db_relation.set_up_relation(relation)) + assert not harness.charm.legacy_db_relation.set_up_relation(relation) postgresql_mock.create_user.assert_not_called() postgresql_mock.create_database.assert_not_called() postgresql_mock.get_postgresql_version.assert_not_called() @@ -232,7 +228,7 @@ def test_set_up_relation(harness): "application", {"database": DATABASE}, ) - tc.assertTrue(harness.charm.legacy_db_relation.set_up_relation(relation)) + assert harness.charm.legacy_db_relation.set_up_relation(relation) user = f"relation-{rel_id}" postgresql_mock.create_user.assert_called_once_with(user, "test-password", False) postgresql_mock.create_database.assert_called_once_with( @@ -240,7 +236,7 @@ def test_set_up_relation(harness): ) _update_endpoints.assert_called_once() _update_unit_status.assert_called_once() - tc.assertNotIsInstance(harness.model.unit.status, BlockedStatus) + assert not isinstance(harness.model.unit.status, BlockedStatus) # Assert that the correct calls were made when the database name is not # provided in both application and unit databags. @@ -260,14 +256,14 @@ def test_set_up_relation(harness): "application/0", {"database": DATABASE}, ) - tc.assertTrue(harness.charm.legacy_db_relation.set_up_relation(relation)) + assert harness.charm.legacy_db_relation.set_up_relation(relation) postgresql_mock.create_user.assert_called_once_with(user, "test-password", False) postgresql_mock.create_database.assert_called_once_with( DATABASE, user, plugins=[], client_relations=[relation] ) _update_endpoints.assert_called_once() _update_unit_status.assert_called_once() - tc.assertNotIsInstance(harness.model.unit.status, BlockedStatus) + assert not isinstance(harness.model.unit.status, BlockedStatus) # Assert that the correct calls were made when the database name is not provided. postgresql_mock.create_user.reset_mock() @@ -281,32 +277,32 @@ def test_set_up_relation(harness): "application/0", {"database": ""}, ) - tc.assertTrue(harness.charm.legacy_db_relation.set_up_relation(relation)) + assert harness.charm.legacy_db_relation.set_up_relation(relation) postgresql_mock.create_user.assert_called_once_with(user, "test-password", False) postgresql_mock.create_database.assert_called_once_with( "application", user, plugins=[], client_relations=[relation] ) _update_endpoints.assert_called_once() _update_unit_status.assert_called_once() - tc.assertNotIsInstance(harness.model.unit.status, BlockedStatus) + assert not isinstance(harness.model.unit.status, BlockedStatus) # BlockedStatus due to a PostgreSQLCreateUserError. postgresql_mock.create_database.reset_mock() postgresql_mock.get_postgresql_version.reset_mock() _update_endpoints.reset_mock() _update_unit_status.reset_mock() - tc.assertFalse(harness.charm.legacy_db_relation.set_up_relation(relation)) + assert not harness.charm.legacy_db_relation.set_up_relation(relation) postgresql_mock.create_database.assert_not_called() _update_endpoints.assert_not_called() _update_unit_status.assert_not_called() - tc.assertIsInstance(harness.model.unit.status, BlockedStatus) + assert isinstance(harness.model.unit.status, BlockedStatus) # BlockedStatus due to a PostgreSQLCreateDatabaseError. harness.charm.unit.status = ActiveStatus() - tc.assertFalse(harness.charm.legacy_db_relation.set_up_relation(relation)) + assert not harness.charm.legacy_db_relation.set_up_relation(relation) _update_endpoints.assert_not_called() _update_unit_status.assert_not_called() - tc.assertIsInstance(harness.model.unit.status, BlockedStatus) + assert isinstance(harness.model.unit.status, BlockedStatus) @patch_network_get(private_address="1.1.1.1") @@ -325,14 +321,14 @@ def test_update_unit_status(harness): _is_blocked.return_value = False harness.charm.legacy_db_relation._update_unit_status(relation) _check_for_blocking_relations.assert_not_called() - tc.assertNotIsInstance(harness.charm.unit.status, ActiveStatus) + assert not isinstance(harness.charm.unit.status, ActiveStatus) # Test when the charm is blocked but not due to extensions request. _is_blocked.return_value = True harness.charm.unit.status = BlockedStatus("fake message") harness.charm.legacy_db_relation._update_unit_status(relation) _check_for_blocking_relations.assert_not_called() - tc.assertNotIsInstance(harness.charm.unit.status, ActiveStatus) + assert not isinstance(harness.charm.unit.status, ActiveStatus) # Test when there are relations causing the blocked status. harness.charm.unit.status = BlockedStatus( @@ -341,14 +337,14 @@ def test_update_unit_status(harness): _check_for_blocking_relations.return_value = True harness.charm.legacy_db_relation._update_unit_status(relation) _check_for_blocking_relations.assert_called_once_with(relation.id) - tc.assertNotIsInstance(harness.charm.unit.status, ActiveStatus) + assert not isinstance(harness.charm.unit.status, ActiveStatus) # Test when there are no relations causing the blocked status anymore. _check_for_blocking_relations.reset_mock() _check_for_blocking_relations.return_value = False harness.charm.legacy_db_relation._update_unit_status(relation) _check_for_blocking_relations.assert_called_once_with(relation.id) - tc.assertIsInstance(harness.charm.unit.status, ActiveStatus) + assert isinstance(harness.charm.unit.status, ActiveStatus) @patch_network_get(private_address="1.1.1.1") @@ -381,7 +377,7 @@ def test_on_relation_broken_extensions_unblock(harness): # Break the relation that blocked the charm. harness.remove_relation(rel_id) - tc.assertTrue(isinstance(harness.model.unit.status, ActiveStatus)) + assert isinstance(harness.model.unit.status, ActiveStatus) @patch_network_get(private_address="1.1.1.1") @@ -422,7 +418,7 @@ def test_on_relation_broken_extensions_keep_block(harness): event.relation.id = first_rel_id # Break one of the relations that block the charm. harness.charm.legacy_db_relation._on_relation_broken(event) - tc.assertTrue(isinstance(harness.model.unit.status, BlockedStatus)) + assert isinstance(harness.model.unit.status, BlockedStatus) @patch_network_get(private_address="1.1.1.1") @@ -481,8 +477,8 @@ def test_update_endpoints_with_relation(harness): # BlockedStatus due to a PostgreSQLGetPostgreSQLVersionError. harness.charm.legacy_db_relation.update_endpoints(relation) - tc.assertIsInstance(harness.model.unit.status, BlockedStatus) - tc.assertEqual(harness.get_relation_data(rel_id, harness.charm.unit.name), {}) + assert isinstance(harness.model.unit.status, BlockedStatus) + assert harness.get_relation_data(rel_id, harness.charm.unit.name) == {} # Test with both a primary and a replica. # Update the endpoints with the event and check that it updated only @@ -492,18 +488,26 @@ def test_update_endpoints_with_relation(harness): # Set the expected username based on the relation id. user = f"relation-{rel}" - # Set the assert function based on each relation (whether it should have data). - assert_based_on_relation = tc.assertTrue if rel == rel_id else tc.assertFalse - # Check that the unit relation databag contains (or not) the endpoints. unit_relation_data = harness.get_relation_data(rel, harness.charm.unit.name) - assert_based_on_relation( - "master" in unit_relation_data and master + user == unit_relation_data["master"] - ) - assert_based_on_relation( - "standbys" in unit_relation_data - and standbys + user == unit_relation_data["standbys"] - ) + if rel == rel_id: + assert ( + "master" in unit_relation_data + and master + user == unit_relation_data["master"] + ) + assert ( + "standbys" in unit_relation_data + and standbys + user == unit_relation_data["standbys"] + ) + else: + assert not ( + "master" in unit_relation_data + and master + user == unit_relation_data["master"] + ) + assert not ( + "standbys" in unit_relation_data + and standbys + user == unit_relation_data["standbys"] + ) # Also test with only a primary instance. harness.charm.legacy_db_relation.update_endpoints(relation) @@ -511,18 +515,26 @@ def test_update_endpoints_with_relation(harness): # Set the expected username based on the relation id. user = f"relation-{rel}" - # Set the assert function based on each relation (whether it should have data). - assert_based_on_relation = tc.assertTrue if rel == rel_id else tc.assertFalse - # Check that the unit relation databag contains the endpoints. unit_relation_data = harness.get_relation_data(rel, harness.charm.unit.name) - assert_based_on_relation( - "master" in unit_relation_data and master + user == unit_relation_data["master"] - ) - assert_based_on_relation( - "standbys" in unit_relation_data - and standbys + user == unit_relation_data["standbys"] - ) + if rel == rel_id: + assert ( + "master" in unit_relation_data + and master + user == unit_relation_data["master"] + ) + assert ( + "standbys" in unit_relation_data + and standbys + user == unit_relation_data["standbys"] + ) + else: + assert not ( + "master" in unit_relation_data + and master + user == unit_relation_data["master"] + ) + assert not ( + "standbys" in unit_relation_data + and standbys + user == unit_relation_data["standbys"] + ) @patch_network_get(private_address="1.1.1.1") @@ -578,8 +590,8 @@ def test_update_endpoints_without_relation(harness): # BlockedStatus due to a PostgreSQLGetPostgreSQLVersionError. harness.charm.legacy_db_relation.update_endpoints() - tc.assertIsInstance(harness.model.unit.status, BlockedStatus) - tc.assertEqual(harness.get_relation_data(rel_id, harness.charm.unit.name), {}) + assert isinstance(harness.model.unit.status, BlockedStatus) + assert harness.get_relation_data(rel_id, harness.charm.unit.name) == {} # Test with both a primary and a replica. # Update the endpoints and check that all relations' databags are updated. @@ -590,10 +602,8 @@ def test_update_endpoints_without_relation(harness): # Check that the unit relation databag contains the endpoints. unit_relation_data = harness.get_relation_data(rel, harness.charm.unit.name) - tc.assertTrue( - "master" in unit_relation_data and master + user == unit_relation_data["master"] - ) - tc.assertTrue( + assert "master" in unit_relation_data and master + user == unit_relation_data["master"] + assert ( "standbys" in unit_relation_data and standbys + user == unit_relation_data["standbys"] ) @@ -606,10 +616,8 @@ def test_update_endpoints_without_relation(harness): # Check that the unit relation databag contains the endpoints. unit_relation_data = harness.get_relation_data(rel, harness.charm.unit.name) - tc.assertTrue( - "master" in unit_relation_data and master + user == unit_relation_data["master"] - ) - tc.assertTrue( + assert "master" in unit_relation_data and master + user == unit_relation_data["master"] + assert ( "standbys" in unit_relation_data and standbys + user == unit_relation_data["standbys"] ) @@ -621,12 +629,12 @@ def test_get_allowed_units(harness): peer_rel_id = harness.model.get_relation(PEER).id rel_id = harness.model.get_relation(RELATION_NAME).id peer_relation = harness.model.get_relation(PEER, peer_rel_id) - tc.assertEqual(harness.charm.legacy_db_relation._get_allowed_units(peer_relation), "") + assert harness.charm.legacy_db_relation._get_allowed_units(peer_relation) == "" # List of space separated allowed units from the other application. harness.add_relation_unit(rel_id, "application/1") db_relation = harness.model.get_relation(RELATION_NAME, rel_id) - tc.assertEqual( - harness.charm.legacy_db_relation._get_allowed_units(db_relation), - "application/0 application/1", + assert ( + harness.charm.legacy_db_relation._get_allowed_units(db_relation) + == "application/0 application/1" ) diff --git a/tests/unit/test_postgresql_provider.py b/tests/unit/test_postgresql_provider.py index bd8fcec787..cb8e010943 100644 --- a/tests/unit/test_postgresql_provider.py +++ b/tests/unit/test_postgresql_provider.py @@ -1,7 +1,6 @@ # Copyright 2022 Canonical Ltd. # See LICENSE file for licensing details. -from unittest import TestCase from unittest.mock import Mock, PropertyMock, patch import pytest @@ -24,9 +23,6 @@ RELATION_NAME = "database" POSTGRESQL_VERSION = "12" -# used for assert functions -tc = TestCase() - @pytest.fixture(autouse=True) def harness(): @@ -122,7 +118,7 @@ def test_on_database_requested(harness): # Request a database before primary endpoint is available. request_database(harness) - tc.assertEqual(_defer.call_count, 2) + assert _defer.call_count == 2 # Request it again when the database is ready. request_database(harness) @@ -141,45 +137,36 @@ def test_on_database_requested(harness): _update_endpoints.assert_called_once() # Assert that the relation data was updated correctly. - tc.assertEqual( - harness.get_relation_data(rel_id, harness.charm.app.name), - { - "data": f'{{"database": "{DATABASE}", "extra-user-roles": "{EXTRA_USER_ROLES}"}}', - "username": user, - "password": "test-password", - "version": POSTGRESQL_VERSION, - "database": f"{DATABASE}", - }, - ) + assert harness.get_relation_data(rel_id, harness.charm.app.name) == { + "data": f'{{"database": "{DATABASE}", "extra-user-roles": "{EXTRA_USER_ROLES}"}}', + "username": user, + "password": "test-password", + "version": POSTGRESQL_VERSION, + "database": f"{DATABASE}", + } # Assert no BlockedStatus was set. - tc.assertFalse(isinstance(harness.model.unit.status, BlockedStatus)) + assert not isinstance(harness.model.unit.status, BlockedStatus) # BlockedStatus due to a PostgreSQLCreateUserError. request_database(harness) - tc.assertTrue(isinstance(harness.model.unit.status, BlockedStatus)) + assert isinstance(harness.model.unit.status, BlockedStatus) # No data is set in the databag by the database. - tc.assertEqual( - harness.get_relation_data(rel_id, harness.charm.app.name), - { - "data": f'{{"database": "{DATABASE}", "extra-user-roles": "{EXTRA_USER_ROLES}"}}', - }, - ) + assert harness.get_relation_data(rel_id, harness.charm.app.name) == { + "data": f'{{"database": "{DATABASE}", "extra-user-roles": "{EXTRA_USER_ROLES}"}}', + } # BlockedStatus due to a PostgreSQLCreateDatabaseError. request_database(harness) - tc.assertTrue(isinstance(harness.model.unit.status, BlockedStatus)) + assert isinstance(harness.model.unit.status, BlockedStatus) # No data is set in the databag by the database. - tc.assertEqual( - harness.get_relation_data(rel_id, harness.charm.app.name), - { - "data": f'{{"database": "{DATABASE}", "extra-user-roles": "{EXTRA_USER_ROLES}"}}', - }, - ) + assert harness.get_relation_data(rel_id, harness.charm.app.name) == { + "data": f'{{"database": "{DATABASE}", "extra-user-roles": "{EXTRA_USER_ROLES}"}}', + } # BlockedStatus due to a PostgreSQLGetPostgreSQLVersionError. request_database(harness) - tc.assertTrue(isinstance(harness.model.unit.status, BlockedStatus)) + assert isinstance(harness.model.unit.status, BlockedStatus) @patch_network_get(private_address="1.1.1.1") @@ -254,25 +241,18 @@ def test_update_endpoints_with_event(harness): # Update the endpoints with the event and check that it updated # only the right relation databag (the one from the event). harness.charm.postgresql_client_relation.update_endpoints(mock_event) - tc.assertEqual( - harness.get_relation_data(rel_id, harness.charm.app.name), - {"endpoints": "1.1.1.1:5432", "read-only-endpoints": "2.2.2.2:5432"}, - ) - tc.assertEqual( - harness.get_relation_data(another_rel_id, harness.charm.app.name), - {}, - ) + assert harness.get_relation_data(rel_id, harness.charm.app.name) == { + "endpoints": "1.1.1.1:5432", + "read-only-endpoints": "2.2.2.2:5432", + } + assert harness.get_relation_data(another_rel_id, harness.charm.app.name) == {} # Also test with only a primary instance. harness.charm.postgresql_client_relation.update_endpoints(mock_event) - tc.assertEqual( - harness.get_relation_data(rel_id, harness.charm.app.name), - {"endpoints": "1.1.1.1:5432"}, - ) - tc.assertEqual( - harness.get_relation_data(another_rel_id, harness.charm.app.name), - {}, - ) + assert harness.get_relation_data(rel_id, harness.charm.app.name) == { + "endpoints": "1.1.1.1:5432" + } + assert harness.get_relation_data(another_rel_id, harness.charm.app.name) == {} @patch_network_get(private_address="1.1.1.1") @@ -300,22 +280,20 @@ def test_update_endpoints_without_event(harness): # Test with both a primary and a replica. # Update the endpoints and check that all relations' databags are updated. harness.charm.postgresql_client_relation.update_endpoints() - tc.assertEqual( - harness.get_relation_data(rel_id, harness.charm.app.name), - {"endpoints": "1.1.1.1:5432", "read-only-endpoints": "2.2.2.2:5432"}, - ) - tc.assertEqual( - harness.get_relation_data(another_rel_id, harness.charm.app.name), - {"endpoints": "1.1.1.1:5432", "read-only-endpoints": "2.2.2.2:5432"}, - ) + assert harness.get_relation_data(rel_id, harness.charm.app.name) == { + "endpoints": "1.1.1.1:5432", + "read-only-endpoints": "2.2.2.2:5432", + } + assert harness.get_relation_data(another_rel_id, harness.charm.app.name) == { + "endpoints": "1.1.1.1:5432", + "read-only-endpoints": "2.2.2.2:5432", + } # Also test with only a primary instance. harness.charm.postgresql_client_relation.update_endpoints() - tc.assertEqual( - harness.get_relation_data(rel_id, harness.charm.app.name), - {"endpoints": "1.1.1.1:5432"}, - ) - tc.assertEqual( - harness.get_relation_data(another_rel_id, harness.charm.app.name), - {"endpoints": "1.1.1.1:5432"}, - ) + assert harness.get_relation_data(rel_id, harness.charm.app.name) == { + "endpoints": "1.1.1.1:5432" + } + assert harness.get_relation_data(another_rel_id, harness.charm.app.name) == { + "endpoints": "1.1.1.1:5432" + } diff --git a/tests/unit/test_upgrade.py b/tests/unit/test_upgrade.py index daad8a7044..b9112119e1 100644 --- a/tests/unit/test_upgrade.py +++ b/tests/unit/test_upgrade.py @@ -1,6 +1,5 @@ # Copyright 2023 Canonical Ltd. # See LICENSE file for licensing details. -from unittest import TestCase from unittest.mock import MagicMock, PropertyMock, patch import pytest @@ -12,9 +11,6 @@ from constants import SNAP_PACKAGES from tests.helpers import patch_network_get -# used for assert functions -tc = TestCase() - @pytest.fixture(autouse=True) def harness(): @@ -45,8 +41,8 @@ def test_build_upgrade_stack(harness): for rel_id in (upgrade_relation_id, peer_relation_id): harness.add_relation_unit(rel_id, "postgresql/2") - tc.assertEqual(harness.charm.upgrade.build_upgrade_stack(), [0, 1, 2]) - tc.assertEqual(harness.charm.upgrade.build_upgrade_stack(), [1, 2, 0]) + assert harness.charm.upgrade.build_upgrade_stack() == [0, 1, 2] + assert harness.charm.upgrade.build_upgrade_stack() == [1, 2, 0] def test_log_rollback(harness): @@ -133,7 +129,7 @@ def test_on_upgrade_granted(harness): _start_patroni.return_value = True _member_started.return_value = False harness.charm.upgrade._on_upgrade_granted(mock_event) - tc.assertEqual(_member_started.call_count, 6) + assert _member_started.call_count == 6 _cluster_members.assert_not_called() mock_event.defer.assert_called_once() _set_unit_completed.assert_not_called() @@ -146,8 +142,8 @@ def test_on_upgrade_granted(harness): _member_started.return_value = True _cluster_members.return_value = ["postgresql-1"] harness.charm.upgrade._on_upgrade_granted(mock_event) - tc.assertEqual(_member_started.call_count, 6) - tc.assertEqual(_cluster_members.call_count, 6) + assert _member_started.call_count == 6 + assert _cluster_members.call_count == 6 mock_event.defer.assert_called_once() _set_unit_completed.assert_not_called() _set_unit_failed.assert_not_called() @@ -211,12 +207,18 @@ def test_pre_upgrade_check(harness): _is_creating_backup.side_effect = [True, False, False] # Test when not all members are ready. - with tc.assertRaises(ClusterNotReadyError): + try: harness.charm.upgrade.pre_upgrade_check() + assert False + except ClusterNotReadyError: + pass # Test when a backup is being created. - with tc.assertRaises(ClusterNotReadyError): + try: harness.charm.upgrade.pre_upgrade_check() + assert False + except ClusterNotReadyError: + pass # Test when everything is ok to start the upgrade. harness.charm.upgrade.pre_upgrade_check() diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py index 1080e659e6..6da8995d02 100644 --- a/tests/unit/test_utils.py +++ b/tests/unit/test_utils.py @@ -2,21 +2,17 @@ # See LICENSE file for licensing details. import re -from unittest import TestCase from utils import new_password -# used for assert functions -tc = TestCase() - def test_new_password(): # Test the password generation twice in order to check if we get different passwords and # that they meet the required criteria. first_password = new_password() - tc.assertEqual(len(first_password), 16) - tc.assertIsNotNone(re.fullmatch("[a-zA-Z0-9\b]{16}$", first_password)) + assert len(first_password) == 16 + assert re.fullmatch("[a-zA-Z0-9\b]{16}$", first_password) is not None second_password = new_password() - tc.assertIsNotNone(re.fullmatch("[a-zA-Z0-9\b]{16}$", second_password)) - tc.assertNotEqual(second_password, first_password) + assert re.fullmatch("[a-zA-Z0-9\b]{16}$", second_password) is not None + assert second_password != first_password