From 0ba4c82861a27b2b2717816a25e2e65adac278f3 Mon Sep 17 00:00:00 2001 From: Paulo Machado Date: Thu, 28 Sep 2023 17:29:00 -0300 Subject: [PATCH 01/16] first pass with minimal unit test fixes --- config.yaml | 12 +- .../data_platform_libs/v0/data_models.py | 354 ++++++++++++++++ lib/charms/mysql/v0/mysql.py | 23 +- lib/charms/rolling_ops/v0/rollingops.py | 390 ++++++++++++++++++ metadata.yaml | 2 + src/charm.py | 82 +++- src/config.py | 104 +++++ src/constants.py | 1 + src/mysql_vm_helpers.py | 54 +-- src/relations/mysql.py | 26 +- src/utils.py | 15 + tests/unit/test_charm.py | 6 +- tests/unit/test_mysql.py | 2 - tests/unit/test_mysqlsh_helpers.py | 58 +-- 14 files changed, 1052 insertions(+), 77 deletions(-) create mode 100644 lib/charms/data_platform_libs/v0/data_models.py create mode 100644 lib/charms/rolling_ops/v0/rollingops.py create mode 100644 src/config.py diff --git a/config.yaml b/config.yaml index 5ab5e854f..864d4c3b9 100644 --- a/config.yaml +++ b/config.yaml @@ -6,12 +6,18 @@ options: description: "Optional - Name of the MySQL InnoDB cluster" type: "string" profile: - description: | + description: | profile representing the scope of deployment, and used to be able to enable high-level high-level customisation of sysconfigs, resource checks/allocation, warning levels, etc. Allowed values are: “production” and “testing”. - type: string - default: production + type: string + default: production + profile-limit-memory: + type: int + description: | + Amount of memory in Megabytes to limit MySQL and associated process to. + If unset, this will be decided according to the default memory limit in the selected profile. + Only comes into effect when a profile is selected. # Config options for the legacy 'mysql relation' mysql-interface-user: description: "The database username for the legacy 'mysql' relation" diff --git a/lib/charms/data_platform_libs/v0/data_models.py b/lib/charms/data_platform_libs/v0/data_models.py new file mode 100644 index 000000000..a1dbb8299 --- /dev/null +++ b/lib/charms/data_platform_libs/v0/data_models.py @@ -0,0 +1,354 @@ +# Copyright 2023 Canonical Ltd. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +r"""Library to provide simple API for promoting typed, validated and structured dataclass in charms. + +Dict-like data structure are often used in charms. They are used for config, action parameters +and databag. This library aims at providing simple API for using pydantic BaseModel-derived class +in charms, in order to enhance: +* Validation, by embedding custom business logic to validate single parameters or even have + validators that acts across different fields +* Parsing, by loading data into pydantic object we can both allow for other types (e.g. float) to + be used in configuration/parameters as well as specify even nested complex objects for databags +* Static typing checks, by moving from dict-like object to classes with typed-annotated properties, + that can be statically checked using mypy to ensure that the code is correct. + +Pydantic models can be used on: + +* Charm Configuration (as defined in config.yaml) +* Actions parameters (as defined in actions.yaml) +* Application/Unit Databag Information (thus making it more structured and encoded) + + +## Creating models + +Any data-structure can be modeled using dataclasses instead of dict-like objects (e.g. storing +config, action parameters and databags). Within pydantic, we can define dataclasses that provides +also parsing and validation on standard dataclass implementation: + +```python + +from charms.data_platform_libs.v0.data_models import BaseConfigModel + +class MyConfig(BaseConfigModel): + + my_key: int + + @validator("my_key") + def is_lower_than_100(cls, v: int): + if v > 100: + raise ValueError("Too high") + +``` + +This should allow to collapse both parsing and validation as the dataclass object is parsed and +created: + +```python +dataclass = MyConfig(my_key="1") + +dataclass.my_key # this returns 1 (int) +dataclass["my_key"] # this returns 1 (int) + +dataclass = MyConfig(my_key="102") # this returns a ValueError("Too High") +``` + +## Charm Configuration Model + +Using the class above, we can implement parsing and validation of configuration by simply +extending our charms using the `TypedCharmBase` class, as shown below. + +```python +class MyCharm(TypedCharmBase[MyConfig]): + config_type = MyConfig + + # everywhere in the code you will have config property already parsed and validate + def my_method(self): + self.config: MyConfig +``` + +## Action parameters + +In order to parse action parameters, we can use a decorator to be applied to action event +callbacks, as shown below. + +```python +@validate_params(PullActionModel) +def _pull_site_action( + self, event: ActionEvent, + params: Optional[Union[PullActionModel, ValidationError]] = None +): + if isinstance(params, ValidationError): + # handle errors + else: + # do stuff +``` + +Note that this changes the signature of the callbacks by adding an extra parameter with the parsed +counterpart of the `event.params` dict-like field. If validation fails, we return (not throw!) the +exception, to be handled (or raised) in the callback. + +## Databag + +In order to parse databag fields, we define a decorator to be applied to base relation event +callbacks. + +```python +@parse_relation_data(app_model=AppDataModel, unit_model=UnitDataModel) +def _on_cluster_relation_joined( + self, event: RelationEvent, + app_data: Optional[Union[AppDataModel, ValidationError]] = None, + unit_data: Optional[Union[UnitDataModel, ValidationError]] = None +) -> None: + ... +``` + +The parameters `app_data` and `unit_data` refers to the databag of the entity which fired the +RelationEvent. + +When we want to access to a relation databag outsides of an action, it can be useful also to +compact multiple databags into a single object (if there are no conflicting fields), e.g. + +```python + +class ProviderDataBag(BaseClass): + provider_key: str + +class RequirerDataBag(BaseClass): + requirer_key: str + +class MergedDataBag(ProviderDataBag, RequirerDataBag): + pass + +merged_data = get_relation_data_as( + MergedDataBag, relation.data[self.app], relation.data[relation.app] +) + +merged_data.requirer_key +merged_data.provider_key + +``` + +The above code can be generalized to other kinds of merged objects, e.g. application and unit, and +it can be extended to multiple sources beyond 2: + +```python +merged_data = get_relation_data_as( + MergedDataBag, relation.data[self.app], relation.data[relation.app], ... +) +``` + +""" + +import json +from functools import reduce, wraps +from typing import Callable, Generic, MutableMapping, Optional, Type, TypeVar, Union + +import pydantic +from ops.charm import ActionEvent, CharmBase, RelationEvent +from ops.model import RelationDataContent +from pydantic import BaseModel, ValidationError + +# The unique Charmhub library identifier, never change it +LIBID = "cb2094c5b07d47e1bf346aaee0fcfcfe" + +# Increment this major API version when introducing breaking changes +LIBAPI = 0 + +# Increment this PATCH version before using `charmcraft publish-lib` or reset +# to 0 if you are raising the major API version +LIBPATCH = 4 + +PYDEPS = ["ops>=2.0.0", "pydantic>=1.10,<2"] + +G = TypeVar("G") +T = TypeVar("T", bound=BaseModel) +AppModel = TypeVar("AppModel", bound=BaseModel) +UnitModel = TypeVar("UnitModel", bound=BaseModel) + +DataBagNativeTypes = (int, str, float) + + +class BaseConfigModel(BaseModel): + """Class to be used for defining the structured configuration options.""" + + def __getitem__(self, x): + """Return the item using the notation instance[key].""" + return getattr(self, x.replace("-", "_")) + + +class TypedCharmBase(CharmBase, Generic[T]): + """Class to be used for extending config-typed charms.""" + + config_type: Type[T] + + @property + def config(self) -> T: + """Return a config instance validated and parsed using the provided pydantic class.""" + translated_keys = {k.replace("-", "_"): v for k, v in self.model.config.items()} + return self.config_type(**translated_keys) + + +def validate_params(cls: Type[T]): + """Return a decorator to allow pydantic parsing of action parameters. + + Args: + cls: Pydantic class representing the model to be used for parsing the content of the + action parameter + """ + + def decorator( + f: Callable[[CharmBase, ActionEvent, Union[T, ValidationError]], G] + ) -> Callable[[CharmBase, ActionEvent], G]: + @wraps(f) + def event_wrapper(self: CharmBase, event: ActionEvent): + try: + params = cls( + **{key.replace("-", "_"): value for key, value in event.params.items()} + ) + except ValidationError as e: + params = e + return f(self, event, params) + + return event_wrapper + + return decorator + + +def write(relation_data: RelationDataContent, model: BaseModel): + """Write the data contained in a domain object to the relation databag. + + Args: + relation_data: pointer to the relation databag + model: instance of pydantic model to be written + """ + for key, value in model.dict(exclude_none=False).items(): + if value: + relation_data[key.replace("_", "-")] = ( + str(value) + if any(isinstance(value, _type) for _type in DataBagNativeTypes) + else json.dumps(value) + ) + else: + relation_data[key.replace("_", "-")] = "" + + +def read(relation_data: MutableMapping[str, str], obj: Type[T]) -> T: + """Read data from a relation databag and parse it into a domain object. + + Args: + relation_data: pointer to the relation databag + obj: pydantic class representing the model to be used for parsing + """ + return obj( + **{ + field_name: ( + relation_data[parsed_key] + if field.outer_type_ in DataBagNativeTypes + else json.loads(relation_data[parsed_key]) + ) + for field_name, field in obj.__fields__.items() + # pyright: ignore[reportGeneralTypeIssues] + if (parsed_key := field_name.replace("_", "-")) in relation_data + if relation_data[parsed_key] + } + ) + + +def parse_relation_data( + app_model: Optional[Type[AppModel]] = None, unit_model: Optional[Type[UnitModel]] = None +): + """Return a decorator to allow pydantic parsing of the app and unit databags. + + Args: + app_model: Pydantic class representing the model to be used for parsing the content of the + app databag. None if no parsing ought to be done. + unit_model: Pydantic class representing the model to be used for parsing the content of the + unit databag. None if no parsing ought to be done. + """ + + def decorator( + f: Callable[ + [ + CharmBase, + RelationEvent, + Optional[Union[AppModel, ValidationError]], + Optional[Union[UnitModel, ValidationError]], + ], + G, + ] + ) -> Callable[[CharmBase, RelationEvent], G]: + @wraps(f) + def event_wrapper(self: CharmBase, event: RelationEvent): + try: + app_data = ( + read(event.relation.data[event.app], app_model) + if app_model is not None and event.app + else None + ) + except pydantic.ValidationError as e: + app_data = e + + try: + unit_data = ( + read(event.relation.data[event.unit], unit_model) + if unit_model is not None and event.unit + else None + ) + except pydantic.ValidationError as e: + unit_data = e + + return f(self, event, app_data, unit_data) + + return event_wrapper + + return decorator + + +class RelationDataModel(BaseModel): + """Base class to be used for creating data models to be used for relation databags.""" + + def write(self, relation_data: RelationDataContent): + """Write data to a relation databag. + + Args: + relation_data: pointer to the relation databag + """ + return write(relation_data, self) + + @classmethod + def read(cls, relation_data: RelationDataContent) -> "RelationDataModel": + """Read data from a relation databag and parse it as an instance of the pydantic class. + + Args: + relation_data: pointer to the relation databag + """ + return read(relation_data, cls) + + +def get_relation_data_as( + model_type: Type[AppModel], + *relation_data: RelationDataContent, +) -> Union[AppModel, ValidationError]: + """Return a merged representation of the provider and requirer databag into a single object. + + Args: + model_type: pydantic class representing the merged databag + relation_data: list of RelationDataContent of provider/requirer/unit sides + """ + try: + app_data = read(reduce(lambda x, y: dict(x) | dict(y), relation_data, {}), model_type) + except pydantic.ValidationError as e: + app_data = e + return app_data diff --git a/lib/charms/mysql/v0/mysql.py b/lib/charms/mysql/v0/mysql.py index 1bbfc49ea..9395c4621 100644 --- a/lib/charms/mysql/v0/mysql.py +++ b/lib/charms/mysql/v0/mysql.py @@ -114,15 +114,17 @@ def wait_until_mysql_connection(self) -> None: # Increment this PATCH version before using `charmcraft publish-lib` or reset # to 0 if you are raising the major API version -LIBPATCH = 48 +LIBPATCH = 50 UNIT_TEARDOWN_LOCKNAME = "unit-teardown" UNIT_ADD_LOCKNAME = "unit-add" BYTES_1GiB = 1073741824 # 1 gibibyte BYTES_1GB = 1000000000 # 1 gigabyte +BYTES_1MB = 1000000 # 1 megabyte BYTES_1MiB = 1048576 # 1 mebibyte RECOVERY_CHECK_TIME = 10 # seconds +GET_MEMBER_STATE_TIME = 10 # seconds class Error(Exception): @@ -558,7 +560,7 @@ def _get_secret_from_juju(self, scope: str, key: str) -> Optional[str]: secret = self.model.get_secret(id=secret_id) content = secret.get_content() self.app_secrets = content - logger.debug(f"Retrieved secert {key} for app from juju") + logger.debug(f"Retrieved secret {key} for app from juju") return self.app_secrets.get(key) @@ -747,11 +749,13 @@ def render_myqld_configuration( self, *, profile: str, + memory_limit: Optional[int] = None, ) -> str: """Render mysqld ini configuration file. Args: profile: profile to use for the configuration (testing, production) + memory_limit: memory limit to use for the configuration in bytes Returns: mysqld ini file string content """ @@ -764,6 +768,10 @@ def render_myqld_configuration( disable_memory_instruments = "performance-schema-instrument = 'memory/%=OFF'" else: available_memory = self.get_available_memory() + if memory_limit: + # when memory limit is set, we need to use the minimum + # between the available memory and the limit + available_memory = min(available_memory, memory_limit) ( innodb_buffer_pool_size, innodb_buffer_pool_chunk_size, @@ -1747,6 +1755,15 @@ def get_primary_label(self) -> Optional[str]: if value["memberrole"] == "primary": return label + def is_unit_primary(self, unit_label: str) -> bool: + """Test if a given unit is the cluster primary. + + Args: + unit_label: The label of the unit to test + """ + primary_label = self.get_primary_label() + return primary_label == unit_label + def set_cluster_primary(self, new_primary_address: str) -> None: """Set the cluster primary. @@ -1907,7 +1924,7 @@ def update_user_password(self, username: str, new_password: str, host: str = "%" ) raise MySQLCheckUserExistenceError(e.message) - @retry(reraise=True, stop=stop_after_attempt(3), wait=wait_fixed(10)) + @retry(reraise=True, stop=stop_after_attempt(3), wait=wait_fixed(GET_MEMBER_STATE_TIME)) def get_member_state(self) -> Tuple[str, str]: """Get member status in cluster. diff --git a/lib/charms/rolling_ops/v0/rollingops.py b/lib/charms/rolling_ops/v0/rollingops.py new file mode 100644 index 000000000..7f4bd1b89 --- /dev/null +++ b/lib/charms/rolling_ops/v0/rollingops.py @@ -0,0 +1,390 @@ +# Copyright 2022 Canonical Ltd. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""This library enables "rolling" operations across units of a charmed Application. + +For example, a charm author might use this library to implement a "rolling restart", in +which all units in an application restart their workload, but no two units execute the +restart at the same time. + +To implement the rolling restart, a charm author would do the following: + +1. Add a peer relation called 'restart' to a charm's `metadata.yaml`: +```yaml +peers: + restart: + interface: rolling_op +``` + +Import this library into src/charm.py, and initialize a RollingOpsManager in the Charm's +`__init__`. The Charm should also define a callback routine, which will be executed when +a unit holds the distributed lock: + +src/charm.py +```python +# ... +from charms.rolling_ops.v0.rollingops import RollingOpsManager +# ... +class SomeCharm(...): + def __init__(...) + # ... + self.restart_manager = RollingOpsManager( + charm=self, relation="restart", callback=self._restart + ) + # ... + def _restart(self, event): + systemd.service_restart('foo') +``` + +To kick off the rolling restart, emit this library's AcquireLock event. The simplest way +to do so would be with an action, though it might make sense to acquire the lock in +response to another event. + +```python + def _on_trigger_restart(self, event): + self.charm.on[self.restart_manager.name].acquire_lock.emit() +``` + +In order to trigger the restart, a human operator would execute the following command on +the CLI: + +``` +juju run-action some-charm/0 some-charm/1 <... some-charm/n> restart +``` + +Note that all units that plan to restart must receive the action and emit the aquire +event. Any units that do not run their acquire handler will be left out of the rolling +restart. (An operator might take advantage of this fact to recover from a failed rolling +operation without restarting workloads that were able to successfully restart -- simply +omit the successful units from a subsequent run-action call.) + +""" +import logging +from enum import Enum +from typing import AnyStr, Callable + +from ops.charm import ActionEvent, CharmBase, RelationChangedEvent +from ops.framework import EventBase, Object +from ops.model import ActiveStatus, MaintenanceStatus, WaitingStatus + +logger = logging.getLogger(__name__) + +# The unique Charmhub library identifier, never change it +LIBID = "20b7777f58fe421e9a223aefc2b4d3a4" + +# Increment this major API version when introducing breaking changes +LIBAPI = 0 + +# Increment this PATCH version before using `charmcraft publish-lib` or reset +# to 0 if you are raising the major API version +LIBPATCH = 2 + + +class LockNoRelationError(Exception): + """Raised if we are trying to process a lock, but do not appear to have a relation yet.""" + + pass + + +class LockState(Enum): + """Possible states for our Distributed lock. + + Note that there are two states set on the unit, and two on the application. + + """ + + ACQUIRE = "acquire" + RELEASE = "release" + GRANTED = "granted" + IDLE = "idle" + + +class Lock: + """A class that keeps track of a single asynchronous lock. + + Warning: a Lock has permission to update relation data, which means that there are + side effects to invoking the .acquire, .release and .grant methods. Running any one of + them will trigger a RelationChanged event, once per transition from one internal + status to another. + + This class tracks state across the cloud by implementing a peer relation + interface. There are two parts to the interface: + + 1) The data on a unit's peer relation (defined in metadata.yaml.) Each unit can update + this data. The only meaningful values are "acquire", and "release", which represent + a request to acquire the lock, and a request to release the lock, respectively. + + 2) The application data in the relation. This tracks whether the lock has been + "granted", Or has been released (and reverted to idle). There are two valid states: + "granted" or None. If a lock is in the "granted" state, a unit should emit a + RunWithLocks event and then release the lock. + + If a lock is in "None", this means that a unit has not yet requested the lock, or + that the request has been completed. + + In more detail, here is the relation structure: + + relation.data: + : + status: 'acquire|release' + : + : 'granted|None' + + Note that this class makes no attempts to timestamp the locks and thus handle multiple + requests in a row. If a unit re-requests a lock before being granted the lock, the + lock will simply stay in the "acquire" state. If a unit wishes to clear its lock, it + simply needs to call lock.release(). + + """ + + def __init__(self, manager, unit=None): + + self.relation = manager.model.relations[manager.name][0] + if not self.relation: + # TODO: defer caller in this case (probably just fired too soon). + raise LockNoRelationError() + + self.unit = unit or manager.model.unit + self.app = manager.model.app + + @property + def _state(self) -> LockState: + """Return an appropriate state. + + Note that the state exists in the unit's relation data, and the application + relation data, so we have to be careful about what our states mean. + + Unit state can only be in "acquire", "release", "None" (None means unset) + Application state can only be in "granted" or "None" (None means unset or released) + + """ + unit_state = LockState(self.relation.data[self.unit].get("state", LockState.IDLE.value)) + app_state = LockState( + self.relation.data[self.app].get(str(self.unit), LockState.IDLE.value) + ) + + if app_state == LockState.GRANTED and unit_state == LockState.RELEASE: + # Active release request. + return LockState.RELEASE + + if app_state == LockState.IDLE and unit_state == LockState.ACQUIRE: + # Active acquire request. + return LockState.ACQUIRE + + return app_state # Granted or unset/released + + @_state.setter + def _state(self, state: LockState): + """Set the given state. + + Since we update the relation data, this may fire off a RelationChanged event. + """ + if state == LockState.ACQUIRE: + self.relation.data[self.unit].update({"state": state.value}) + + if state == LockState.RELEASE: + self.relation.data[self.unit].update({"state": state.value}) + + if state == LockState.GRANTED: + self.relation.data[self.app].update({str(self.unit): state.value}) + + if state is LockState.IDLE: + self.relation.data[self.app].update({str(self.unit): state.value}) + + def acquire(self): + """Request that a lock be acquired.""" + self._state = LockState.ACQUIRE + + def release(self): + """Request that a lock be released.""" + self._state = LockState.RELEASE + + def clear(self): + """Unset a lock.""" + self._state = LockState.IDLE + + def grant(self): + """Grant a lock to a unit.""" + self._state = LockState.GRANTED + + def is_held(self): + """This unit holds the lock.""" + return self._state == LockState.GRANTED + + def release_requested(self): + """A unit has reported that they are finished with the lock.""" + return self._state == LockState.RELEASE + + def is_pending(self): + """Is this unit waiting for a lock?""" + return self._state == LockState.ACQUIRE + + +class Locks: + """Generator that returns a list of locks.""" + + def __init__(self, manager): + self.manager = manager + + # Gather all the units. + relation = manager.model.relations[manager.name][0] + units = [unit for unit in relation.units] + + # Plus our unit ... + units.append(manager.model.unit) + + self.units = units + + def __iter__(self): + """Yields a lock for each unit we can find on the relation.""" + for unit in self.units: + yield Lock(self.manager, unit=unit) + + +class RunWithLock(EventBase): + """Event to signal that this unit should run the callback.""" + + pass + + +class AcquireLock(EventBase): + """Signals that this unit wants to acquire a lock.""" + + pass + + +class ProcessLocks(EventBase): + """Used to tell the leader to process all locks.""" + + pass + + +class RollingOpsManager(Object): + """Emitters and handlers for rolling ops.""" + + def __init__(self, charm: CharmBase, relation: AnyStr, callback: Callable): + """Register our custom events. + + params: + charm: the charm we are attaching this to. + relation: an identifier, by convention based on the name of the relation in the + metadata.yaml, which identifies this instance of RollingOperatorsFactory, + distinct from other instances that may be hanlding other events. + callback: a closure to run when we have a lock. (It must take a CharmBase object and + EventBase object as args.) + """ + # "Inherit" from the charm's class. This gives us access to the framework as + # self.framework, as well as the self.model shortcut. + super().__init__(charm, None) + + self.name = relation + self._callback = callback + self.charm = charm # Maintain a reference to charm, so we can emit events. + + charm.on.define_event("{}_run_with_lock".format(self.name), RunWithLock) + charm.on.define_event("{}_acquire_lock".format(self.name), AcquireLock) + charm.on.define_event("{}_process_locks".format(self.name), ProcessLocks) + + # Watch those events (plus the built in relation event). + self.framework.observe(charm.on[self.name].relation_changed, self._on_relation_changed) + self.framework.observe(charm.on[self.name].acquire_lock, self._on_acquire_lock) + self.framework.observe(charm.on[self.name].run_with_lock, self._on_run_with_lock) + self.framework.observe(charm.on[self.name].process_locks, self._on_process_locks) + + def _callback(self: CharmBase, event: EventBase) -> None: + """Placeholder for the function that actually runs our event. + + Usually overridden in the init. + """ + raise NotImplementedError + + def _on_relation_changed(self: CharmBase, event: RelationChangedEvent): + """Process relation changed. + + First, determine whether this unit has been granted a lock. If so, emit a RunWithLock + event. + + Then, if we are the leader, fire off a process locks event. + + """ + lock = Lock(self) + + if lock.is_pending(): + self.model.unit.status = WaitingStatus("Awaiting {} operation".format(self.name)) + + if lock.is_held(): + self.charm.on[self.name].run_with_lock.emit() + + if self.model.unit.is_leader(): + self.charm.on[self.name].process_locks.emit() + + def _on_process_locks(self: CharmBase, event: ProcessLocks): + """Process locks. + + Runs only on the leader. Updates the status of all locks. + + """ + if not self.model.unit.is_leader(): + return + + pending = [] + + for lock in Locks(self): + if lock.is_held(): + # One of our units has the lock -- return without further processing. + return + + if lock.release_requested(): + lock.clear() # Updates relation data + + if lock.is_pending(): + if lock.unit == self.model.unit: + # Always run on the leader last. + pending.insert(0, lock) + else: + pending.append(lock) + + # If we reach this point, and we have pending units, we want to grant a lock to + # one of them. + if pending: + self.model.app.status = MaintenanceStatus("Beginning rolling {}".format(self.name)) + lock = pending[-1] + lock.grant() + if lock.unit == self.model.unit: + # It's time for the leader to run with lock. + self.charm.on[self.name].run_with_lock.emit() + return + + self.model.app.status = ActiveStatus() + + def _on_acquire_lock(self: CharmBase, event: ActionEvent): + """Request a lock.""" + try: + Lock(self).acquire() # Updates relation data + # emit relation changed event in the edge case where aquire does not + relation = self.model.get_relation(self.name) + self.charm.on[self.name].relation_changed.emit(relation) + except LockNoRelationError: + logger.debug("No {} peer relation yet. Delaying rolling op.".format(self.name)) + event.defer() + + def _on_run_with_lock(self: CharmBase, event: RunWithLock): + lock = Lock(self) + self.model.unit.status = MaintenanceStatus("Executing {} operation".format(self.name)) + self._callback(event) + lock.release() # Updates relation data + if lock.unit == self.model.unit: + self.charm.on[self.name].process_locks.emit() + + self.model.unit.status = ActiveStatus() diff --git a/metadata.yaml b/metadata.yaml index 604ed514e..6f80e1241 100644 --- a/metadata.yaml +++ b/metadata.yaml @@ -26,6 +26,8 @@ peers: interface: mysql_peers upgrade: interface: upgrade + restart: + interface: rolling_op provides: database: diff --git a/src/charm.py b/src/charm.py index 6caf0a0b7..41e7091fd 100755 --- a/src/charm.py +++ b/src/charm.py @@ -9,6 +9,7 @@ import subprocess from typing import Optional +from charms.data_platform_libs.v0.data_models import TypedCharmBase from charms.data_platform_libs.v0.s3 import S3Requirer from charms.grafana_agent.v0.cos_agent import COSAgentProvider from charms.mysql.v0.backups import MySQLBackups @@ -29,6 +30,8 @@ MySQLSetClusterPrimaryError, ) from charms.mysql.v0.tls import MySQLTLS +from charms.rolling_ops.v0.rollingops import RollingOpsManager +from ops import EventBase from ops.charm import ( InstallEvent, RelationBrokenEvent, @@ -54,6 +57,7 @@ wait_fixed, ) +from config import STATIC_CONFIGS, CharmConfig, MySQLConfig from constants import ( BACKUPS_PASSWORD_KEY, BACKUPS_USERNAME, @@ -90,7 +94,7 @@ from relations.mysql_provider import MySQLProvider from relations.shared_db import SharedDBRelation from upgrade import MySQLVMUpgrade, get_mysql_dependencies_model -from utils import generate_random_hash, generate_random_password +from utils import compare_dictionaries, generate_random_hash, generate_random_password logger = logging.getLogger(__name__) @@ -99,9 +103,11 @@ class MySQLDNotRestartedError(Error): """Exception raised when MySQLD is not restarted after configuring instance.""" -class MySQLOperatorCharm(MySQLCharmBase): +class MySQLOperatorCharm(MySQLCharmBase, TypedCharmBase[CharmConfig]): """Operator framework charm for MySQL.""" + config_type = CharmConfig + def __init__(self, *args): super().__init__(*args) @@ -117,6 +123,7 @@ def __init__(self, *args): self.framework.observe(self.on[PEER].relation_changed, self._on_peer_relation_changed) + self.mysql_config = MySQLConfig() self.shared_db_relation = SharedDBRelation(self) self.db_router_relation = DBRouterRelation(self) self.database_relation = MySQLProvider(self) @@ -146,6 +153,7 @@ def __init__(self, *args): relation_name="upgrade", substrate="vm", ) + self.restart = RollingOpsManager(self, relation="restart", callback=self._restart) # ======================= # Charm Lifecycle Hooks @@ -182,22 +190,66 @@ def _on_leader_elected(self, _) -> None: ) self.unit_peer_data.update({"leader": "true"}) + # Create and set cluster and cluster-set names in the peer relation databag + common_hash = generate_random_hash() + self.app_peer_data.setdefault( + "cluster-name", self.config.cluster_name or f"cluster-{common_hash}" + ) + self.app_peer_data.setdefault("cluster-set-domain-name", f"cluster-set-{common_hash}") + def _on_leader_settings_changed(self, _) -> None: """Handle the leader settings changed event.""" self.unit_peer_data.update({"leader": "false"}) - def _on_config_changed(self, _) -> None: + def _on_config_changed(self, event: EventBase) -> None: """Handle the config changed event.""" - # Only execute on leader unit - if not self.unit.is_leader(): + if not self._is_peer_data_set: + # skip when not initialized return - # Create and set cluster and cluster-set names in the peer relation databag - common_hash = generate_random_hash() - self.app_peer_data.setdefault( - "cluster-name", self.config.get("cluster-name", f"cluster-{common_hash}") + if not self.upgrade.idle: + # Defer config-changed event until upgrade is complete + event.defer() + return + + previous_config = self.mysql_config.custom_config + if not previous_config: + # empty config means not initialized, skipping + return + + # always update config file + self._mysql.write_mysqld_config( + profile=self.config.profile, memory_limit=self.config.profile_limit_memory ) - self.app_peer_data.setdefault("cluster-set-domain-name", f"cluster-set-{common_hash}") + + # retrieve the new config + new_config = self.mysql_config.custom_config or dict() + + changed_config = compare_dictionaries(previous_config, new_config) + + if changed_config & STATIC_CONFIGS: + # there are static configurations in changed keys + logger.info("Configuration change requires restart") + + # if self._mysql.is_unit_primary(self.unit_label): + # restart_data = self.model.get_relation("restart").data + # restart_states = { + # restart_data[unit].get("state", "unset") for unit in self.peers.units + # } + # if restart_states != {"release"}: + # # other units restarting, Defer + # logger.debug("Primary is waiting for other units to restart") + # event.defer() + # return + + self.on[f"{self.restart.name}"].acquire_lock.emit() + return + + if dynamic_config := changed_config - STATIC_CONFIGS: + # if only dynamic config changed, apply it + logger.info("Configuration does not requires restart") + for config in dynamic_config: + self._mysql.set_dynamic_variable(config, new_config[config]) def _on_start(self, event: StartEvent) -> None: """Handle the start event. @@ -517,7 +569,9 @@ def _workload_initialise(self) -> None: Create users and configuration to setup instance as an Group Replication node. Raised errors must be treated on handlers. """ - self._mysql.write_mysqld_config(profile=self.config["profile"]) + self._mysql.write_mysqld_config( + profile=self.config.profile, memory_limit=self.config.profile_limit_memory + ) self._mysql.reset_root_password_and_start_mysqld() self._mysql.configure_mysql_users() @@ -697,6 +751,12 @@ def _join_unit_to_cluster(self) -> None: self.unit.status = WaitingStatus("waiting to join the cluster") logger.debug("Waiting to joing the cluster, failed to acquire lock.") + def _restart(self, _: EventBase) -> None: + """Restart the MySQL service.""" + self.unit.status = MaintenanceStatus("restarting MySQL") + self._mysql.restart_mysqld() + self.unit.status = ActiveStatus(self.active_status_message) + if __name__ == "__main__": main(MySQLOperatorCharm) diff --git a/src/config.py b/src/config.py new file mode 100644 index 000000000..aa0c729e4 --- /dev/null +++ b/src/config.py @@ -0,0 +1,104 @@ +#!/usr/bin/env python3 +# Copyright 2023 Canonical Ltd. +# See LICENSE file for licensing details. + +"""Structured configuration for the Kafka charm.""" +import configparser +import logging +import os +import re +from typing import Optional + +from charms.data_platform_libs.v0.data_models import BaseConfigModel +from pydantic import validator + +from constants import MYSQLD_CUSTOM_CONFIG_FILE + +logger = logging.getLogger(__name__) + +# Static config requires workload restart +STATIC_CONFIGS = { + "innodb_buffer_pool_size", + "innodb_buffer_pool_chunk_size", + "group_replication_message_cache_size", +} + + +class StrippedConfigParser(configparser.RawConfigParser): + """ConfigParser that strips quotes from values. + + Necessary because MySQL config files use quotes around some values. + """ + + def get(self, section, option): + """Return the value of the named option in the named section.""" + return super().get(section, option).strip('"') + + +class MySQLConfig: + """Configuration.""" + + @property + def custom_config(self) -> Optional[dict]: + """Return current custom config dict.""" + if not os.path.exists(MYSQLD_CUSTOM_CONFIG_FILE): + return None + + cp = StrippedConfigParser() + + with open(MYSQLD_CUSTOM_CONFIG_FILE, "r") as config_file: + cp.read_file(config_file) + + return dict(cp["mysqld"]) + + +class CharmConfig(BaseConfigModel): + """Manager for the structured configuration.""" + + profile: str + cluster_name: Optional[str] + profile_limit_memory: Optional[int] + mysql_interface_user: Optional[str] + mysql_interface_database: Optional[str] + + @validator("profile") + @classmethod + def profile_values(cls, value: str) -> Optional[str]: + """Check profile config option is one of `testing` or `production`.""" + if value not in ["testing", "production"]: + raise ValueError("Value not one of 'testing' or 'production'") + + return value + + @validator("cluster_name") + @classmethod + def cluster_name_validator(cls, value: str) -> Optional[str]: + """Check for valid cluster name. + + Limited to 63 characters, and must start with a letter and + contain only alphanumeric characters, `-`, `_` and `.` + """ + if len(value) > 63: + raise ValueError("Cluster name must be less than 63 characters") + + if not value[0].isalpha(): + raise ValueError("Cluster name must start with a letter") + + if not re.match(r"^[a-zA-Z0-9-_.]*$", value): + raise ValueError( + "Cluster name must contain only alphanumeric characters, " + "hyphens, underscores and periods" + ) + + return value + + @validator("profile_limit_memory") + @classmethod + def profile_limit_memory_validator(cls, value: int) -> Optional[int]: + """Check profile limit memory.""" + if value < 600: + raise ValueError("MySQL Charm requires at least 600MB for bootstrapping") + if value > 9999999: + raise ValueError("`profile-limit-memory` limited to 7 digits (9999999MB)") + + return value diff --git a/src/constants.py b/src/constants.py index bb26bf00e..3777b29f6 100644 --- a/src/constants.py +++ b/src/constants.py @@ -35,6 +35,7 @@ MYSQLD_SOCK_FILE = f"{CHARMED_MYSQL_COMMON_DIRECTORY}/var/run/mysqld/mysqld.sock" MYSQLD_CONFIG_DIRECTORY = f"{CHARMED_MYSQL_DATA_DIRECTORY}/etc/mysql/mysql.conf.d" MYSQLD_DEFAULTS_CONFIG_FILE = f"{CHARMED_MYSQL_DATA_DIRECTORY}/etc/mysql/mysql.cnf" +MYSQLD_CUSTOM_CONFIG_FILE = f"{MYSQLD_CONFIG_DIRECTORY}/z-custom-mysqld.cnf" MYSQL_SYSTEM_USER = "snap_daemon" MYSQL_DATA_DIR = f"{CHARMED_MYSQL_COMMON_DIRECTORY}/var/lib/mysql" S3_INTEGRATOR_RELATION_NAME = "s3-parameters" diff --git a/src/mysql_vm_helpers.py b/src/mysql_vm_helpers.py index 5145b8338..bc30e61ba 100644 --- a/src/mysql_vm_helpers.py +++ b/src/mysql_vm_helpers.py @@ -12,6 +12,7 @@ from typing import Dict, List, Optional, Tuple from charms.mysql.v0.mysql import ( + BYTES_1MB, Error, MySQLBase, MySQLClientError, @@ -41,6 +42,7 @@ MYSQL_DATA_DIR, MYSQL_SYSTEM_USER, MYSQLD_CONFIG_DIRECTORY, + MYSQLD_CUSTOM_CONFIG_FILE, MYSQLD_DEFAULTS_CONFIG_FILE, MYSQLD_SOCK_FILE, ROOT_SYSTEM_USER, @@ -189,34 +191,33 @@ def install_and_configure_mysql_dependencies() -> None: def get_available_memory(self) -> int: """Retrieves the total memory of the server where mysql is running.""" try: - logger.info("Retrieving the total memory of the server") - - """Below is an example output of `free --bytes`: - total used free shared buff/cache available - Mem: 16484458496 11890454528 265670656 2906722304 4328333312 1321193472 - Swap: 1027600384 1027600384 0 - """ - # need to use sh -c to be able to use pipes - get_total_memory_command = "free --bytes | awk '/^Mem:/{print $2; exit}'".split() - - total_memory, _ = self._execute_commands( - get_total_memory_command, - bash=True, - ) - return int(total_memory) - except MySQLExecError: - logger.exception("Failed to execute commands to query total memory") + logger.debug("Querying system total memory") + with open("/proc/meminfo") as meminfo: + for line in meminfo: + if "MemTotal" in line: + return int(line.split()[1]) * 1024 + + raise MySQLGetAvailableMemoryError + except OSError: + logger.error("Failed to query system memory") raise MySQLGetAvailableMemoryError - def write_mysqld_config(self, profile: str) -> None: + def write_mysqld_config(self, profile: str, memory_limit: Optional[int]) -> None: """Create custom mysql config file. - Raises MySQLCreateCustomMySQLDConfigError if there is an error creating the + Args: + profile: profile to use for the mysql config + memory_limit: memory limit to use for the mysql config in MB + + Raises: MySQLCreateCustomMySQLDConfigError if there is an error creating the custom mysqld config """ - logger.debug("Copying custom mysqld config") + logger.debug("Writing mysql configuration file") + if memory_limit: + # Convert from config value in MB to bytes + memory_limit = memory_limit * BYTES_1MB try: - content = self.render_myqld_configuration(profile=profile) + content = self.render_myqld_configuration(profile=profile, memory_limit=memory_limit) except (MySQLGetAvailableMemoryError, MySQLGetAutoTunningParametersError): logger.exception("Failed to get available memory or auto tuning parameters") raise MySQLCreateCustomMySQLDConfigError @@ -224,8 +225,10 @@ def write_mysqld_config(self, profile: str) -> None: # create the mysqld config directory if it does not exist pathlib.Path(MYSQLD_CONFIG_DIRECTORY).mkdir(mode=0o755, parents=True, exist_ok=True) - with open(f"{MYSQLD_CONFIG_DIRECTORY}/z-custom-mysqld.cnf", "w") as config_file: - config_file.write(content) + self.write_content_to_file( + path=MYSQLD_CUSTOM_CONFIG_FILE, + content=content, + ) def reset_root_password_and_start_mysqld(self) -> None: """Reset the root user password and start mysqld.""" @@ -522,6 +525,11 @@ def start_mysqld(self) -> None: raise MySQLStartMySQLDError(e.message) + def restart_mysqld(self) -> None: + """Restarts the mysqld process.""" + self.stop_mysqld() + self.start_mysqld() + def flush_host_cache(self) -> None: """Flush the MySQL in-memory host cache.""" if not self.is_mysqld_running(): diff --git a/src/relations/mysql.py b/src/relations/mysql.py index a0ffde55a..ae9602c42 100644 --- a/src/relations/mysql.py +++ b/src/relations/mysql.py @@ -6,6 +6,7 @@ import json import logging import socket +import typing from charms.mysql.v0.mysql import ( MySQLCheckUserExistenceError, @@ -26,11 +27,14 @@ MYSQL_RELATION_USER_KEY = "mysql-interface-user" MYSQL_RELATION_DATABASE_KEY = "mysql-interface-database" +if typing.TYPE_CHECKING: + from charm import MySQLOperatorCharm + class MySQLRelation(Object): """Encapsulation of the legacy mysql relation.""" - def __init__(self, charm): + def __init__(self, charm: "MySQLOperatorCharm"): super().__init__(charm, LEGACY_MYSQL) self.charm = charm @@ -71,7 +75,7 @@ def _get_or_generate_username(self, event_relation_id: int) -> str: """ return self.charm.app_peer_data.setdefault( MYSQL_RELATION_USER_KEY, - self.charm.config.get(MYSQL_RELATION_USER_KEY) or f"relation-{event_relation_id}", + self.charm.config.mysql_interface_user or f"relation-{event_relation_id}", ) def _get_or_generate_database(self, event_relation_id: int) -> str: @@ -81,7 +85,7 @@ def _get_or_generate_database(self, event_relation_id: int) -> str: """ return self.charm.app_peer_data.setdefault( MYSQL_RELATION_DATABASE_KEY, - self.charm.config.get(MYSQL_RELATION_DATABASE_KEY) or f"database-{event_relation_id}", + self.charm.config.mysql_interface_database or f"database-{event_relation_id}", ) def _on_leader_elected(self, _) -> None: @@ -130,13 +134,15 @@ def _on_config_changed(self, _) -> None: if isinstance(self.charm.unit.status, ActiveStatus) and self.model.relations.get( LEGACY_MYSQL ): - for key in (MYSQL_RELATION_USER_KEY, MYSQL_RELATION_DATABASE_KEY): - config_value = self.charm.config.get(key) - if config_value and config_value != self.charm.app_peer_data[key]: - self.charm.app.status = BlockedStatus( - f"Remove `mysql` relations in order to change `{key}` config" - ) - return + if ( + self.charm.config.mysql_interface_database + != self.charm.app_peer_data[MYSQL_RELATION_DATABASE_KEY] + or self.charm.config.mysql_interface_user + != self.charm.app_peer_data[MYSQL_RELATION_USER_KEY] + ): + self.charm.app.status = BlockedStatus( + "Remove and re-relate `mysql` relations in order to change config" + ) def _on_mysql_relation_created(self, event: RelationCreatedEvent) -> None: """Handle the legacy `mysql` relation created event. diff --git a/src/utils.py b/src/utils.py index 7dd55dd47..985706770 100644 --- a/src/utils.py +++ b/src/utils.py @@ -29,3 +29,18 @@ def generate_random_hash() -> str: """ random_characters = generate_random_password(20) return hashlib.md5(random_characters.encode("utf-8")).hexdigest() + + +def compare_dictionaries(dict1: dict, dict2: dict) -> set: + """Compare two dictionaries and return a set of keys that are different.""" + different_keys = set() + + # exiting keys with different values + for key in dict1.keys(): + if key in dict2 and dict1[key] != dict2[key]: + different_keys.add(key) + + # non existent keys + different_keys = different_keys | dict2.keys() ^ dict1.keys() + + return different_keys diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index 801c72809..c8e923bb1 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -88,13 +88,15 @@ def test_on_leader_elected_sets_mysql_passwords_in_peer_databag(self): "cluster-admin-password", "monitoring-password", "backups-password", + "cluster-name", + "cluster-set-domain-name", ] self.assertEqual( sorted(peer_relation_databag.keys()), sorted(expected_peer_relation_databag_keys) ) @patch_network_get(private_address="1.1.1.1") - def test_on_config_changed_sets_config_cluster_name_in_peer_databag(self): + def test_on_leader_elected_sets_config_cluster_name_in_peer_databag(self): # ensure that the peer relation databag is empty peer_relation_databag = self.harness.get_relation_data( self.peer_relation_id, self.harness.charm.app @@ -102,8 +104,8 @@ def test_on_config_changed_sets_config_cluster_name_in_peer_databag(self): self.assertEqual(peer_relation_databag, {}) # trigger the leader_elected and config_changed events - self.harness.set_leader(True) self.harness.update_config({"cluster-name": "test-cluster"}) + self.harness.set_leader(True) # ensure that the peer relation has 'cluster_name' set to the config value peer_relation_databag = self.harness.get_relation_data( diff --git a/tests/unit/test_mysql.py b/tests/unit/test_mysql.py index b1ea8b0ff..f3c978b4c 100644 --- a/tests/unit/test_mysql.py +++ b/tests/unit/test_mysql.py @@ -258,7 +258,6 @@ def test_configure_instance(self, _wait_until_mysql_connection, _run_mysqlsh_scr configure_instance_commands = [ "dba.configure_instance('serverconfig:serverconfigpassword@127.0.0.1', ", '{"restart": "true"})', - # , "clusterAdmin": "clusteradmin", "clusterAdminPassword": "clusteradminpassword"})', ] self.mysql.configure_instance(create_cluster_admin=False) @@ -783,7 +782,6 @@ def test_get_member_state(self, _run_mysqlcli_script): state = self.mysql.get_member_state() self.assertEqual(state, ("online", "primary")) - _run_mysqlcli_script.return_value = ( "MEMBER_STATE\tMEMBER_ROLE\tMEMBER_ID\t@@server_uuid\n" "ONLINE\tSECONDARY\t\t\n" diff --git a/tests/unit/test_mysqlsh_helpers.py b/tests/unit/test_mysqlsh_helpers.py index 5b64d0864..103b435b1 100644 --- a/tests/unit/test_mysqlsh_helpers.py +++ b/tests/unit/test_mysqlsh_helpers.py @@ -6,7 +6,7 @@ import os import subprocess import unittest -from unittest.mock import MagicMock, call, patch +from unittest.mock import MagicMock, call, mock_open, patch from charms.mysql.v0.mysql import ( MySQLClientError, @@ -21,6 +21,7 @@ CHARMED_MYSQL_SNAP_NAME, CHARMED_MYSQLD_SERVICE, MYSQLD_CONFIG_DIRECTORY, + MYSQLD_CUSTOM_CONFIG_FILE, ) from mysql_vm_helpers import ( MySQL, @@ -228,6 +229,9 @@ def test_snap_service_operation_exception(self, _snap_cache): _snap_cache.assert_not_called() + @patch("shutil.chown") + @patch("os.chmod") + @patch("mysql_vm_helpers.MySQL.get_available_memory", return_value=16475447296) @patch( "mysql_vm_helpers.MySQL.get_innodb_buffer_pool_parameters", return_value=(1234, 5678, None) ) @@ -235,7 +239,14 @@ def test_snap_service_operation_exception(self, _snap_cache): @patch("pathlib.Path") @patch("builtins.open") def test_write_mysqld_config( - self, _open, _path, _get_innodb_buffer_pool_parameters, _get_max_connections + self, + _open, + _path, + _get_innodb_buffer_pool_parameters, + _get_max_connections, + _get_available_memory, + _chmod, + _chown, ): """Test successful execution of create_custom_mysqld_config.""" self.maxDiff = None @@ -245,7 +256,7 @@ def test_write_mysqld_config( _open_mock = unittest.mock.mock_open() _open.side_effect = _open_mock - self.mysql.write_mysqld_config(profile="production") + self.mysql.write_mysqld_config(profile="production", memory_limit=None) config = "\n".join( ( @@ -263,13 +274,14 @@ def test_write_mysqld_config( _get_max_connections.assert_called_once() _get_innodb_buffer_pool_parameters.assert_called_once() _path_mock.mkdir.assert_called_once_with(mode=0o755, parents=True, exist_ok=True) - _open.assert_called_once_with(f"{MYSQLD_CONFIG_DIRECTORY}/z-custom-mysqld.cnf", "w") + _open.assert_called_once_with(MYSQLD_CUSTOM_CONFIG_FILE, "w", encoding="utf-8") + _get_available_memory.assert_called_once() self.assertEqual( sorted(_open_mock.mock_calls), sorted( [ - call(f"{MYSQLD_CONFIG_DIRECTORY}/z-custom-mysqld.cnf", "w"), + call(MYSQLD_CUSTOM_CONFIG_FILE, "w", encoding="utf-8"), call().__enter__(), call().write(config), call().__exit__(None, None, None), @@ -279,7 +291,7 @@ def test_write_mysqld_config( # Test `testing` profile _open_mock.reset_mock() - self.mysql.write_mysqld_config(profile="testing") + self.mysql.write_mysqld_config(profile="testing", memory_limit=None) config = "\n".join( ( @@ -300,7 +312,7 @@ def test_write_mysqld_config( sorted(_open_mock.mock_calls), sorted( [ - call(f"{MYSQLD_CONFIG_DIRECTORY}/z-custom-mysqld.cnf", "w"), + call(f"{MYSQLD_CONFIG_DIRECTORY}/z-custom-mysqld.cnf", "w", encoding="utf-8"), call().__enter__(), call().write(config), call().__exit__(None, None, None), @@ -324,7 +336,7 @@ def test_create_custom_mysqld_config_exception( _open.side_effect = _open_mock with self.assertRaises(MySQLCreateCustomMySQLDConfigError): - self.mysql.write_mysqld_config(profile="production") + self.mysql.write_mysqld_config(profile="production", memory_limit=None) @patch("subprocess.run") def test_execute_commands(self, _run): @@ -444,20 +456,20 @@ def test_install_snap(self, _cache, _path_exists, _run, _check_call, _pathlib, _ _check_call.assert_called_once_with(["charmed-mysql.mysqlsh", "--help"], stderr=-1) _run.assert_called_once_with(["snap", "alias", "charmed-mysql.mysql", "mysql"], check=True) - @patch("mysql_vm_helpers.MySQL._execute_commands") - def test_get_available_memory(self, _execute_commands): - """Test successful execution of get_available_memory().""" - _execute_commands.return_value = "16484458496\n", None - - total_memory = self.mysql.get_available_memory() + def test_get_available_memory(self): + meminfo = ( + "MemTotal: 16089488 kB" + "MemFree: 799284 kB" + "MemAvailable: 3926924 kB" + "Buffers: 187232 kB" + "Cached: 4445936 kB" + "SwapCached: 156012 kB" + "Active: 11890336 kB" + ) - self.assertEqual(16484458496, total_memory) + with patch("builtins.open", mock_open(read_data=meminfo)): + self.assertEqual(self.mysql.get_available_memory(), 16475635712) - _execute_commands.assert_called_once_with( - "free --bytes | awk '/^Mem:/{print $2; exit}'".split(), - bash=True, - ) - _execute_commands.reset_mock() - _execute_commands.side_effect = MySQLExecError - with self.assertRaises(MySQLGetAvailableMemoryError): - self.mysql.get_available_memory() + with patch("builtins.open", mock_open(read_data="")): + with self.assertRaises(MySQLGetAvailableMemoryError): + self.mysql.get_available_memory() From ec89ebdc6d51fe4187c25329e1d7cbda8bd94c80 Mon Sep 17 00:00:00 2001 From: Paulo Machado Date: Fri, 29 Sep 2023 12:35:57 -0300 Subject: [PATCH 02/16] increasing coverage --- tests/unit/test_config.py | 68 +++++++++++++++++++++++++++++++++++++++ tests/unit/test_mysql.py | 8 +++++ tests/unit/test_utils.py | 28 ++++++++++++++++ 3 files changed, 104 insertions(+) create mode 100644 tests/unit/test_config.py create mode 100644 tests/unit/test_utils.py diff --git a/tests/unit/test_config.py b/tests/unit/test_config.py new file mode 100644 index 000000000..4a2c91fb1 --- /dev/null +++ b/tests/unit/test_config.py @@ -0,0 +1,68 @@ +#!/usr/bin/env python3 +# Copyright 2023 Canonical Ltd. +# See LICENSE file for licensing details. + +import logging +from pathlib import Path + +import pytest +import yaml +from ops.testing import Harness + +from charm import MySQLOperatorCharm + +CONFIG = str(yaml.safe_load(Path("./config.yaml").read_text())) +ACTIONS = str(yaml.safe_load(Path("./actions.yaml").read_text())) +METADATA = str(yaml.safe_load(Path("./metadata.yaml").read_text())) + +logger = logging.getLogger(__name__) + + +@pytest.fixture +def harness(): + harness = Harness(MySQLOperatorCharm, meta=METADATA, config=CONFIG, actions=ACTIONS) + harness.add_relation("restart", "mysql") + harness.begin() + return harness + + +def _check_valid_values(_harness, field: str, accepted_values: list, is_long_field=False) -> None: + """Check the correcteness of the passed values for a field.""" + for value in accepted_values: + _harness.update_config({field: value}) + assert _harness.charm.config[field] == value if not is_long_field else int(value) + + +def _check_invalid_values(_harness, field: str, erroneus_values: list) -> None: + """Check the incorrectness of the passed values for a field.""" + for value in erroneus_values: + _harness.update_config({field: value}) + with pytest.raises(ValueError): + _ = _harness.charm.config[field] + + +def test_profile_limit_values(harness) -> None: + """Check that integer fields are parsed correctly.""" + erroneus_values = [599, 10**7, -354343] + _check_invalid_values(harness, "profile-limit-memory", erroneus_values) + + valid_values = [600, 1000, 35000] + _check_valid_values(harness, "profile-limit-memory", valid_values) + + +def test_profile_values(harness) -> None: + """Test profile values.""" + erroneus_values = ["prod", "Test", "foo", "bar"] + _check_invalid_values(harness, "profile", erroneus_values) + + accepted_values = ["production", "testing"] + _check_valid_values(harness, "profile", accepted_values) + + +def test_cluster_name_values(harness) -> None: + """Test cluster name values.""" + erroneus_values = [64 * "a", "1-cluster", "cluster$"] + _check_invalid_values(harness, "cluster-name", erroneus_values) + + accepted_values = ["c1", "cluster_name", "cluster.name", "Cluster-name", 63 * "c"] + _check_valid_values(harness, "cluster-name", accepted_values) diff --git a/tests/unit/test_mysql.py b/tests/unit/test_mysql.py index f3c978b4c..7ed7a3fc4 100644 --- a/tests/unit/test_mysql.py +++ b/tests/unit/test_mysql.py @@ -1734,6 +1734,14 @@ def test_get_primary_label(self, _get_cluster_status): self.assertEqual(self.mysql.get_primary_label(), "mysql-k8s-1") + @patch("charms.mysql.v0.mysql.MySQLBase.get_cluster_status") + def test_is_unit_primary(self, _get_cluster_status): + """Test is_unit_primary.""" + _get_cluster_status.return_value = SHORT_CLUSTER_STATUS + + self.assertTrue(self.mysql.is_unit_primary("mysql-k8s-1")) + self.assertFalse(self.mysql.is_unit_primary("mysql-k8s-2")) + @patch("charms.mysql.v0.mysql.RECOVERY_CHECK_TIME", 0.1) @patch("charms.mysql.v0.mysql.MySQLBase.get_member_state") def test_hold_if_recovering(self, mock_get_member_state): diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py new file mode 100644 index 000000000..1a097394f --- /dev/null +++ b/tests/unit/test_utils.py @@ -0,0 +1,28 @@ +#!/usr/bin/env python3 +# Copyright 2023 Canonical Ltd. +# See LICENSE file for licensing details. + +import re + +from utils import compare_dictionaries, generate_random_hash, generate_random_password + + +def test_generate_random_hash(): + """Test generate_random_hash function.""" + random_hash = generate_random_hash() + assert len(random_hash) == 32 + assert re.match(r"^[a-f0-9]{32}$", random_hash) + + +def test_generate_random_password(): + """Test generate_random_password function.""" + random_password = generate_random_password(20) + assert len(random_password) == 20 + assert re.match(r"^[a-zA-Z0-9]{20}$", random_password) + + +def test_compare_dictionaries(): + dict1 = {"a": 1, "b": 2, "c": 3, "f": 4} + dict2 = {"a": 1, "b": 3, "d": 5, "e": 6, "f": 4} + + assert compare_dictionaries(dict1, dict2) == {"b", "c", "d", "e"} From 44e92f92fd7be9f2cdcb7d22c7d8d033a0a467ec Mon Sep 17 00:00:00 2001 From: Paulo Machado Date: Fri, 29 Sep 2023 18:11:27 -0300 Subject: [PATCH 03/16] defer primary restart to be last --- lib/charms/mysql/v0/mysql.py | 8 ++--- src/charm.py | 62 +++++++++++++++++++++++------------- src/config.py | 41 ++++++++++++------------ src/mysql_vm_helpers.py | 4 +-- tests/unit/test_charm.py | 1 + 5 files changed, 66 insertions(+), 50 deletions(-) diff --git a/lib/charms/mysql/v0/mysql.py b/lib/charms/mysql/v0/mysql.py index 646e0a8fc..fb6884ee0 100644 --- a/lib/charms/mysql/v0/mysql.py +++ b/lib/charms/mysql/v0/mysql.py @@ -758,13 +758,13 @@ def __init__( self.backups_user = backups_user self.backups_password = backups_password - def render_myqld_configuration( + def render_mysqld_configuration( self, *, profile: str, memory_limit: Optional[int] = None, snap_common: str = "", - ) -> str: + ) -> tuple[str, dict]: """Render mysqld ini configuration file. Args: @@ -772,7 +772,7 @@ def render_myqld_configuration( memory_limit: memory limit to use for the configuration in bytes snap_common: snap common directory (for log files locations in vm) - Returns: mysqld ini file string content + Returns: a tuple with mysqld ini file string content and a the config dict """ performance_schema_instrument = "" if profile == "testing": @@ -825,7 +825,7 @@ def render_myqld_configuration( with io.StringIO() as string_io: config.write(string_io) - return string_io.getvalue() + return string_io.getvalue(), dict(config["mysqld"]) def configure_mysql_users(self): """Configure the MySQL users for the instance. diff --git a/src/charm.py b/src/charm.py index 0f09d049e..5275038ea 100755 --- a/src/charm.py +++ b/src/charm.py @@ -9,11 +9,13 @@ import subprocess from typing import Optional +import ops from charms.data_platform_libs.v0.data_models import TypedCharmBase from charms.data_platform_libs.v0.s3 import S3Requirer from charms.grafana_agent.v0.cos_agent import COSAgentProvider from charms.mysql.v0.backups import MySQLBackups from charms.mysql.v0.mysql import ( + BYTES_1MB, Error, MySQLAddInstanceToClusterError, MySQLCharmBase, @@ -57,10 +59,11 @@ wait_fixed, ) -from config import STATIC_CONFIGS, CharmConfig, MySQLConfig +from config import CharmConfig, MySQLConfig from constants import ( BACKUPS_PASSWORD_KEY, BACKUPS_USERNAME, + CHARMED_MYSQL_COMMON_DIRECTORY, CHARMED_MYSQL_SNAP_NAME, CHARMED_MYSQLD_SERVICE, CLUSTER_ADMIN_PASSWORD_KEY, @@ -70,6 +73,7 @@ MONITORING_PASSWORD_KEY, MONITORING_USERNAME, MYSQL_EXPORTER_PORT, + MYSQLD_CUSTOM_CONFIG_FILE, PASSWORD_LENGTH, PEER, ROOT_PASSWORD_KEY, @@ -127,7 +131,7 @@ def __init__(self, *args): self.framework.observe(self.on[PEER].relation_changed, self._on_peer_relation_changed) - self.mysql_config = MySQLConfig() + self.mysql_config = MySQLConfig(MYSQLD_CUSTOM_CONFIG_FILE) self.shared_db_relation = SharedDBRelation(self) self.db_router_relation = DBRouterRelation(self) self.database_relation = MySQLProvider(self) @@ -223,39 +227,43 @@ def _on_config_changed(self, event: EventBase) -> None: # empty config means not initialized, skipping return - # always update config file - self._mysql.write_mysqld_config( - profile=self.config.profile, memory_limit=self.config.profile_limit_memory + # render the new config + memory_limit_bytes = (self.config.profile_limit_memory or 0) * BYTES_1MB + new_config_content, new_config_dict = self._mysql.render_mysqld_configuration( + profile=self.config.profile, + snap_common=CHARMED_MYSQL_COMMON_DIRECTORY, + memory_limit=memory_limit_bytes, ) - # retrieve the new config - new_config = self.mysql_config.custom_config or dict() + changed_config = compare_dictionaries(previous_config, new_config_dict) - changed_config = compare_dictionaries(previous_config, new_config) - - if changed_config & STATIC_CONFIGS: + if self.mysql_config.keys_requires_restart(changed_config): # there are static configurations in changed keys logger.info("Configuration change requires restart") - # if self._mysql.is_unit_primary(self.unit_label): - # restart_data = self.model.get_relation("restart").data - # restart_states = { - # restart_data[unit].get("state", "unset") for unit in self.peers.units - # } - # if restart_states != {"release"}: - # # other units restarting, Defer - # logger.debug("Primary is waiting for other units to restart") - # event.defer() - # return - + if self._mysql.is_unit_primary(self.unit_label): + restart_states = { + self.restart_peers.data[unit].get("state", "unset") + for unit in self.peers.units + } + if restart_states != {"release"}: + # Wait other units restart first to minimize primary switchover + logger.debug("Primary is waiting for other units to restart") + event.defer() + return + + # persist config to file + self._mysql.write_content_to_file( + path=MYSQLD_CUSTOM_CONFIG_FILE, content=new_config_content + ) self.on[f"{self.restart.name}"].acquire_lock.emit() return - if dynamic_config := changed_config - STATIC_CONFIGS: + if dynamic_config := self.mysql_config.filter_static_keys(changed_config): # if only dynamic config changed, apply it logger.info("Configuration does not requires restart") for config in dynamic_config: - self._mysql.set_dynamic_variable(config, new_config[config]) + self._mysql.set_dynamic_variable(config, new_config_dict[config]) def _on_start(self, event: StartEvent) -> None: """Handle the start event. @@ -424,6 +432,9 @@ def _on_update_status(self, _) -> None: logger.debug("skip status update while upgrading") return + # unset restart control flag + del self.restart_peers.data[self.unit]["state"] + if self._is_unit_waiting_to_join_cluster(): self._join_unit_to_cluster() return @@ -525,6 +536,11 @@ def unit_fqdn(self) -> str: """Returns the unit's FQDN.""" return socket.getfqdn() + @property + def restart_peers(self) -> Optional[ops.model.Relation]: + """Retrieve the peer relation.""" + return self.model.get_relation("restart") + def is_unit_busy(self) -> bool: """Returns whether the unit is in blocked state and should not run any operations.""" return self.unit_peer_data.get("member-state") == "waiting" diff --git a/src/config.py b/src/config.py index aa0c729e4..770dcf226 100644 --- a/src/config.py +++ b/src/config.py @@ -12,41 +12,40 @@ from charms.data_platform_libs.v0.data_models import BaseConfigModel from pydantic import validator -from constants import MYSQLD_CUSTOM_CONFIG_FILE - logger = logging.getLogger(__name__) -# Static config requires workload restart -STATIC_CONFIGS = { - "innodb_buffer_pool_size", - "innodb_buffer_pool_chunk_size", - "group_replication_message_cache_size", -} - -class StrippedConfigParser(configparser.RawConfigParser): - """ConfigParser that strips quotes from values. +class MySQLConfig: + """Configuration.""" - Necessary because MySQL config files use quotes around some values. - """ + # Static config requires workload restart + static_config = { + "innodb_buffer_pool_size", + "innodb_buffer_pool_chunk_size", + "group_replication_message_cache_size", + } - def get(self, section, option): - """Return the value of the named option in the named section.""" - return super().get(section, option).strip('"') + def __init__(self, config_file_path: str): + """Initialize config.""" + self.config_file_path = config_file_path + def keys_requires_restart(self, keys: set) -> bool: + """Check if keys require restart.""" + return bool(keys & self.static_config) -class MySQLConfig: - """Configuration.""" + def filter_static_keys(self, keys: set) -> set: + """Filter static keys.""" + return keys - self.static_config @property def custom_config(self) -> Optional[dict]: """Return current custom config dict.""" - if not os.path.exists(MYSQLD_CUSTOM_CONFIG_FILE): + if not os.path.exists(self.config_file_path): return None - cp = StrippedConfigParser() + cp = configparser.ConfigParser(interpolation=None) - with open(MYSQLD_CUSTOM_CONFIG_FILE, "r") as config_file: + with open(self.config_file_path, "r") as config_file: cp.read_file(config_file) return dict(cp["mysqld"]) diff --git a/src/mysql_vm_helpers.py b/src/mysql_vm_helpers.py index f5352e6dd..a732fd14c 100644 --- a/src/mysql_vm_helpers.py +++ b/src/mysql_vm_helpers.py @@ -223,7 +223,7 @@ def write_mysqld_config(self, profile: str, memory_limit: Optional[int]) -> None # Convert from config value in MB to bytes memory_limit = memory_limit * BYTES_1MB try: - content = self.render_myqld_configuration( + content_str, _ = self.render_mysqld_configuration( profile=profile, snap_common=CHARMED_MYSQL_COMMON_DIRECTORY, memory_limit=memory_limit, @@ -237,7 +237,7 @@ def write_mysqld_config(self, profile: str, memory_limit: Optional[int]) -> None self.write_content_to_file( path=MYSQLD_CUSTOM_CONFIG_FILE, - content=content, + content=content_str, ) def setup_logrotate_and_cron(self) -> None: diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index b16241177..63c9264e3 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -37,6 +37,7 @@ def setUp(self): self.harness.add_relation_unit(self.peer_relation_id, "mysql/1") self.db_router_relation_id = self.harness.add_relation("db-router", "app") self.harness.add_relation_unit(self.db_router_relation_id, "app/0") + self.harness.add_relation("restart", "restart") @patch_network_get(private_address="1.1.1.1") @patch("upgrade.MySQLVMUpgrade.cluster_state", return_value="idle") From b5c4659935575e840f58d26b9e6329e18b6f0217 Mon Sep 17 00:00:00 2001 From: Paulo Machado Date: Sat, 30 Sep 2023 11:21:46 -0300 Subject: [PATCH 04/16] stable now has profile --- .../integration/high_availability/test_upgrade_from_stable.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/integration/high_availability/test_upgrade_from_stable.py b/tests/integration/high_availability/test_upgrade_from_stable.py index 6dbc715e3..f8b79e38d 100644 --- a/tests/integration/high_availability/test_upgrade_from_stable.py +++ b/tests/integration/high_availability/test_upgrade_from_stable.py @@ -29,9 +29,8 @@ async def test_deploy_latest(ops_test: OpsTest, mysql_charm_series: str) -> None application_name=MYSQL_APP_NAME, num_units=3, channel="8.0/stable", - constraints="mem=2G", series=mysql_charm_series, - # config={"profile": "testing"}, # config not available in 8.0/stable@r151 + config={"profile": "testing"}, ), ops_test.model.deploy( TEST_APP_NAME, From e1e9c8cdc64c0cf869d5891e0f5bbb9b70d54682 Mon Sep 17 00:00:00 2001 From: Paulo Machado Date: Mon, 2 Oct 2023 15:05:09 -0300 Subject: [PATCH 05/16] escape variables dynamically --- lib/charms/mysql/v0/mysql.py | 5 +++++ tests/unit/test_mysql.py | 7 +++++++ 2 files changed, 12 insertions(+) diff --git a/lib/charms/mysql/v0/mysql.py b/lib/charms/mysql/v0/mysql.py index fb6884ee0..9d3ebefe8 100644 --- a/lib/charms/mysql/v0/mysql.py +++ b/lib/charms/mysql/v0/mysql.py @@ -1149,6 +1149,11 @@ def set_dynamic_variable( """ if not instance_address: instance_address = self.instance_address + + # escape variable values when needed + if not re.match(r"^[0-9,a-z,A-Z$_]+$", value): + value = f"`{value}`" + logger.debug(f"Setting {variable} to {value} on {instance_address}") set_var_command = [ f"shell.connect('{self.server_config_user}:{self.server_config_password}@{instance_address}')", diff --git a/tests/unit/test_mysql.py b/tests/unit/test_mysql.py index 7ed7a3fc4..ea7151144 100644 --- a/tests/unit/test_mysql.py +++ b/tests/unit/test_mysql.py @@ -1661,6 +1661,13 @@ def test_set_dynamic_variables(self, _run_mysqlsh_script): self.mysql.set_dynamic_variable(variable="variable", value="value") _run_mysqlsh_script.assert_called_with("\n".join(commands)) + commands = ( + f"shell.connect('{self.mysql.server_config_user}:{self.mysql.server_config_password}@127.0.0.1')", + 'session.run_sql("SET GLOBAL variable=`/a/path/value`")', + ) + self.mysql.set_dynamic_variable(variable="variable", value="/a/path/value") + _run_mysqlsh_script.assert_called_with("\n".join(commands)) + _run_mysqlsh_script.reset_mock() _run_mysqlsh_script.side_effect = MySQLClientError From 40849eb23ab06be3bffc688de94d684a138872fe Mon Sep 17 00:00:00 2001 From: Paulo Machado Date: Mon, 2 Oct 2023 15:06:50 -0300 Subject: [PATCH 06/16] additional static config and defer when not running --- src/charm.py | 9 ++++++++- src/config.py | 1 + 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/charm.py b/src/charm.py index 5275038ea..2b79fe5ed 100755 --- a/src/charm.py +++ b/src/charm.py @@ -218,7 +218,14 @@ def _on_config_changed(self, event: EventBase) -> None: return if not self.upgrade.idle: - # Defer config-changed event until upgrade is complete + # defer config-changed event until upgrade is complete + logger.debug("Deferring config-changed event until upgrade is complete") + event.defer() + return + + if not self._mysql.is_mysqld_running: + # defer config-changed event until MySQL is running + logger.debug("Deferring config-changed event until MySQL is running") event.defer() return diff --git a/src/config.py b/src/config.py index 770dcf226..cd68e898c 100644 --- a/src/config.py +++ b/src/config.py @@ -23,6 +23,7 @@ class MySQLConfig: "innodb_buffer_pool_size", "innodb_buffer_pool_chunk_size", "group_replication_message_cache_size", + "log_error", } def __init__(self, config_file_path: str): From 20fef555f18d8bb3d2372c4fb19dd509eae4568e Mon Sep 17 00:00:00 2001 From: Paulo Machado Date: Mon, 2 Oct 2023 15:07:20 -0300 Subject: [PATCH 07/16] simpler method for primary retrieval on tests --- tests/integration/helpers.py | 68 +++++-------------- .../high_availability/test_replication.py | 11 +-- tests/integration/relations/test_database.py | 28 +------- tests/integration/relations/test_shared_db.py | 9 +-- 4 files changed, 21 insertions(+), 95 deletions(-) diff --git a/tests/integration/helpers.py b/tests/integration/helpers.py index 43e040b96..2b07d3cc5 100644 --- a/tests/integration/helpers.py +++ b/tests/integration/helpers.py @@ -4,7 +4,6 @@ import itertools import json import logging -import re import secrets import string import subprocess @@ -119,10 +118,7 @@ async def get_primary_unit( ops_test: OpsTest, unit: Unit, app_name: str, - cluster_name: str, - server_config_username: str, - server_config_password: str, -) -> str: +) -> Unit: """Helper to retrieve the primary unit. Args: @@ -130,45 +126,23 @@ async def get_primary_unit( unit: A unit on which to run dba.get_cluster().status() on app_name: The name of the test application cluster_name: The name of the test cluster - server_config_username: The server config username - server_config_password: The server config password Returns: A juju unit that is a MySQL primary """ - commands = [ - "charmed-mysql.mysqlsh", - "--python", - f"{server_config_username}:{server_config_password}@127.0.0.1", - "-e", - f"\"print('' + dba.get_cluster('{cluster_name}').status().__repr__() + '')\"", - ] - raw_output = await run_command_on_unit(unit, " ".join(commands)) - - if not raw_output: - raise ValueError("Command return nothing") - - matches = re.search("(.+)", raw_output) - if not matches: - raise ValueError("Cluster status not found") - - # strip and remove escape characters `\` - string_output = matches.group(1).strip().replace("\\", "") - - cluster_status = json.loads(string_output) - - primary_label = [ - label - for label, member in cluster_status["defaultReplicaSet"]["topology"].items() - if member["mode"] == "R/W" - ][0] - primary_name = "/".join(primary_label.rsplit("-", 1)) + units = ops_test.model.applications[app_name].units + action = await unit.run_action("get-cluster-status") + result = await action.wait() - for unit in ops_test.model.applications[app_name].units: - if unit.name == primary_name: - return unit + primary_unit = None + for k, v in result.results["status"]["defaultreplicaset"]["topology"].items(): + if v["memberrole"] == "primary": + unit_name = k.replace("-", "/") + primary_unit = [unit for unit in units if unit.name == unit_name][0] - return None + if not primary_unit: + raise ValueError("Unable to find primary unit") + return primary_unit async def get_server_config_credentials(unit: Unit) -> Dict: @@ -537,25 +511,15 @@ async def get_primary_unit_wrapper(ops_test: OpsTest, app_name: str, unit_exclud The primary Unit object """ logger.info("Retrieving primary unit") + units = ops_test.model.applications[app_name].units if unit_excluded: # if defined, exclude unit from available unit to run command on # useful when the workload is stopped on unit - unit = ( - { - unit - for unit in ops_test.model.applications[app_name].units - if unit.name != unit_excluded.name - } - ).pop() + unit = ({unit for unit in units if unit.name != unit_excluded.name}).pop() else: - unit = ops_test.model.applications[app_name].units[0] - cluster = cluster_name(unit, ops_test.model.info.name) + unit = units[0] - server_config_password = await get_system_user_password(unit, SERVER_CONFIG_USERNAME) - - primary_unit = await get_primary_unit( - ops_test, unit, app_name, cluster, SERVER_CONFIG_USERNAME, server_config_password - ) + primary_unit = await get_primary_unit(ops_test, unit, app_name) return primary_unit diff --git a/tests/integration/high_availability/test_replication.py b/tests/integration/high_availability/test_replication.py index 6015c2f47..29d14c6bf 100644 --- a/tests/integration/high_availability/test_replication.py +++ b/tests/integration/high_availability/test_replication.py @@ -16,7 +16,6 @@ from constants import CHARMED_MYSQL_COMMON_DIRECTORY from ..helpers import ( - cluster_name, delete_file_or_directory_in_unit, execute_queries_on_unit, fetch_credentials, @@ -310,16 +309,8 @@ async def test_cluster_isolation(ops_test: OpsTest, mysql_charm_series: str) -> connection_data = dict() for application in apps: random_unit = ops_test.model.applications[application].units[0] - cluster = cluster_name(random_unit, ops_test.model.info.name) server_config_credentials = await get_server_config_credentials(random_unit) - primary_unit = await get_primary_unit( - ops_test, - random_unit, - application, - cluster, - server_config_credentials["username"], - server_config_credentials["password"], - ) + primary_unit = await get_primary_unit(ops_test, random_unit, application) primary_unit_address = await primary_unit.get_public_address() diff --git a/tests/integration/relations/test_database.py b/tests/integration/relations/test_database.py index b049d53bd..7478fd90a 100644 --- a/tests/integration/relations/test_database.py +++ b/tests/integration/relations/test_database.py @@ -113,14 +113,7 @@ async def test_password_rotation(ops_test: OpsTest): old_credentials = await fetch_credentials(random_unit, SERVER_CONFIG_USERNAME) # get primary unit first, need that to invoke set-password action - primary_unit = await get_primary_unit( - ops_test, - random_unit, - DATABASE_APP_NAME, - CLUSTER_NAME, - old_credentials["username"], - old_credentials["password"], - ) + primary_unit = await get_primary_unit(ops_test, random_unit, DATABASE_APP_NAME) primary_unit_address = await primary_unit.get_public_address() logger.debug( "Test succeeded Primary unit detected before password rotation is %s", primary_unit_address @@ -158,14 +151,7 @@ async def test_password_rotation_silent(ops_test: OpsTest): old_credentials = await fetch_credentials(random_unit, SERVER_CONFIG_USERNAME) # get primary unit first, need that to invoke set-password action - primary_unit = await get_primary_unit( - ops_test, - random_unit, - DATABASE_APP_NAME, - CLUSTER_NAME, - old_credentials["username"], - old_credentials["password"], - ) + primary_unit = await get_primary_unit(ops_test, random_unit, DATABASE_APP_NAME) primary_unit_address = await primary_unit.get_public_address() logger.debug( "Test succeeded Primary unit detected before password rotation is %s", primary_unit_address @@ -196,20 +182,12 @@ async def test_password_rotation_root_user_implicit(ops_test: OpsTest): random_unit = ops_test.model.applications[DATABASE_APP_NAME].units[-1] root_credentials = await fetch_credentials(random_unit, ROOT_USERNAME) - server_config_credentials = await fetch_credentials(random_unit, SERVER_CONFIG_USERNAME) old_credentials = await fetch_credentials(random_unit) assert old_credentials["password"] == root_credentials["password"] # get primary unit first, need that to invoke set-password action - primary_unit = await get_primary_unit( - ops_test, - random_unit, - DATABASE_APP_NAME, - CLUSTER_NAME, - server_config_credentials["username"], - server_config_credentials["password"], - ) + primary_unit = await get_primary_unit(ops_test, random_unit, DATABASE_APP_NAME) primary_unit_address = await primary_unit.get_public_address() logger.debug( "Test succeeded Primary unit detected before password rotation is %s", primary_unit_address diff --git a/tests/integration/relations/test_shared_db.py b/tests/integration/relations/test_shared_db.py index d816a066e..c87e1611f 100644 --- a/tests/integration/relations/test_shared_db.py +++ b/tests/integration/relations/test_shared_db.py @@ -222,14 +222,7 @@ async def test_keystone_bundle_shared_db(ops_test: OpsTest, mysql_charm_series: ) # Scale down the primary unit of mysql - primary_unit = await get_primary_unit( - ops_test, - random_unit, - APP_NAME, - CLUSTER_NAME, - server_config_credentials["username"], - server_config_credentials["password"], - ) + primary_unit = await get_primary_unit(ops_test, random_unit, APP_NAME) primary_unit_name = primary_unit.name await ops_test.model.destroy_units(primary_unit_name) From 6ec4096a4e9865c26430486f5dbd60b58a6d5274 Mon Sep 17 00:00:00 2001 From: Paulo Machado Date: Tue, 3 Oct 2023 17:53:19 -0300 Subject: [PATCH 08/16] better description --- config.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config.yaml b/config.yaml index 864d4c3b9..aaddb0653 100644 --- a/config.yaml +++ b/config.yaml @@ -17,7 +17,7 @@ options: description: | Amount of memory in Megabytes to limit MySQL and associated process to. If unset, this will be decided according to the default memory limit in the selected profile. - Only comes into effect when a profile is selected. + Only comes into effect when the `production` profile is selected. # Config options for the legacy 'mysql relation' mysql-interface-user: description: "The database username for the legacy 'mysql' relation" From dd019d4c8d4806dc34ad645da774844ba86372d8 Mon Sep 17 00:00:00 2001 From: Paulo Machado Date: Tue, 3 Oct 2023 17:53:53 -0300 Subject: [PATCH 09/16] skip handler on upgrade --- src/charm.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/charm.py b/src/charm.py index 2b79fe5ed..ac83c7053 100755 --- a/src/charm.py +++ b/src/charm.py @@ -218,9 +218,8 @@ def _on_config_changed(self, event: EventBase) -> None: return if not self.upgrade.idle: - # defer config-changed event until upgrade is complete - logger.debug("Deferring config-changed event until upgrade is complete") - event.defer() + # skip when upgrade is in progress + # the upgrade already restart the daemon return if not self._mysql.is_mysqld_running: From fa71dfe5784227af5e74227e84ffca067ce09b83 Mon Sep 17 00:00:00 2001 From: Paulo Machado Date: Tue, 3 Oct 2023 17:54:30 -0300 Subject: [PATCH 10/16] adds pre-check since upgrade is in stable --- .../test_upgrade_from_stable.py | 32 ++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/tests/integration/high_availability/test_upgrade_from_stable.py b/tests/integration/high_availability/test_upgrade_from_stable.py index f8b79e38d..2013446e4 100644 --- a/tests/integration/high_availability/test_upgrade_from_stable.py +++ b/tests/integration/high_availability/test_upgrade_from_stable.py @@ -5,6 +5,11 @@ import logging import pytest +from integration.helpers import ( + get_leader_unit, + get_primary_unit_wrapper, + retrieve_database_variable_value, +) from integration.high_availability.high_availability_helpers import ( ensure_all_units_continuous_writes_incrementing, relate_mysql_and_application, @@ -21,7 +26,7 @@ @pytest.mark.group(1) @pytest.mark.abort_on_fail -async def test_deploy_latest(ops_test: OpsTest, mysql_charm_series: str) -> None: +async def test_deploy_stable(ops_test: OpsTest, mysql_charm_series: str) -> None: """Simple test to ensure that the mysql and application charms get deployed.""" await asyncio.gather( ops_test.model.deploy( @@ -49,6 +54,31 @@ async def test_deploy_latest(ops_test: OpsTest, mysql_charm_series: str) -> None assert len(ops_test.model.applications[MYSQL_APP_NAME].units) == 3 +@pytest.mark.group(1) +@pytest.mark.abort_on_fail +async def test_pre_upgrade_check(ops_test: OpsTest) -> None: + """Test that the pre-upgrade-check action runs successfully.""" + mysql_units = ops_test.model.applications[MYSQL_APP_NAME].units + + logger.info("Get leader unit") + leader_unit = await get_leader_unit(ops_test, MYSQL_APP_NAME) + + assert leader_unit is not None, "No leader unit found" + logger.info("Run pre-upgrade-check action") + action = await leader_unit.run_action("pre-upgrade-check") + await action.wait() + + logger.info("Assert slow shutdown is enabled") + for unit in mysql_units: + value = await retrieve_database_variable_value(ops_test, unit, "innodb_fast_shutdown") + assert value == 0, f"innodb_fast_shutdown not 0 at {unit.name}" + + primary_unit = await get_primary_unit_wrapper(ops_test, MYSQL_APP_NAME) + + logger.info("Assert primary is set to leader") + assert await primary_unit.is_leader_from_status(), "Primary unit not set to leader" + + @pytest.mark.group(1) async def test_upgrade_from_stable(ops_test: OpsTest): """Test updating from stable channel.""" From af0404214e40399805fdbaf1c6dabb8c225685c7 Mon Sep 17 00:00:00 2001 From: Paulo Machado Date: Tue, 3 Oct 2023 22:06:56 -0300 Subject: [PATCH 11/16] fix get primary for app name with hyphens --- tests/integration/helpers.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/integration/helpers.py b/tests/integration/helpers.py index 2b07d3cc5..d3dce10bc 100644 --- a/tests/integration/helpers.py +++ b/tests/integration/helpers.py @@ -137,8 +137,9 @@ async def get_primary_unit( primary_unit = None for k, v in result.results["status"]["defaultreplicaset"]["topology"].items(): if v["memberrole"] == "primary": - unit_name = k.replace("-", "/") + unit_name = f"{app_name}/{k.split('-')[-1]}" primary_unit = [unit for unit in units if unit.name == unit_name][0] + break if not primary_unit: raise ValueError("Unable to find primary unit") From 14685866b06757a08847ba743b4a44bacc6ea02d Mon Sep 17 00:00:00 2001 From: Paulo Machado Date: Wed, 4 Oct 2023 09:00:14 -0300 Subject: [PATCH 12/16] exact match app name --- .../integration/high_availability/high_availability_helpers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/high_availability/high_availability_helpers.py b/tests/integration/high_availability/high_availability_helpers.py index a2e0947e4..a8a576778 100644 --- a/tests/integration/high_availability/high_availability_helpers.py +++ b/tests/integration/high_availability/high_availability_helpers.py @@ -69,7 +69,7 @@ def get_application_name(ops_test: OpsTest, application_name_substring: str) -> the first one found will be returned. """ for application in ops_test.model.applications: - if application_name_substring in application: + if application_name_substring == application: return application return "" From 79829e3ef24811f1dceafa652a91f57a16b7788e Mon Sep 17 00:00:00 2001 From: Paulo Machado Date: Fri, 6 Oct 2023 08:32:26 -0300 Subject: [PATCH 13/16] fix bad c-c c-v --- src/config.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/config.py b/src/config.py index cd68e898c..bd3ce466c 100644 --- a/src/config.py +++ b/src/config.py @@ -2,7 +2,7 @@ # Copyright 2023 Canonical Ltd. # See LICENSE file for licensing details. -"""Structured configuration for the Kafka charm.""" +"""Structured configuration for the MySQL charm.""" import configparser import logging import os From 740b58e40f48107bc428baa3be8aa235bfd13ec1 Mon Sep 17 00:00:00 2001 From: Paulo Machado Date: Wed, 11 Oct 2023 10:30:15 -0300 Subject: [PATCH 14/16] defer on restart and bug fix --- src/charm.py | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/src/charm.py b/src/charm.py index 8b9c74b6d..a797bc87e 100755 --- a/src/charm.py +++ b/src/charm.py @@ -222,7 +222,7 @@ def _on_config_changed(self, event: EventBase) -> None: # the upgrade already restart the daemon return - if not self._mysql.is_mysqld_running: + if not self._mysql.is_mysqld_running(): # defer config-changed event until MySQL is running logger.debug("Deferring config-changed event until MySQL is running") event.defer() @@ -247,17 +247,6 @@ def _on_config_changed(self, event: EventBase) -> None: # there are static configurations in changed keys logger.info("Configuration change requires restart") - if self._mysql.is_unit_primary(self.unit_label): - restart_states = { - self.restart_peers.data[unit].get("state", "unset") - for unit in self.peers.units - } - if restart_states != {"release"}: - # Wait other units restart first to minimize primary switchover - logger.debug("Primary is waiting for other units to restart") - event.defer() - return - # persist config to file self._mysql.write_content_to_file( path=MYSQLD_CUSTOM_CONFIG_FILE, content=new_config_content @@ -773,8 +762,18 @@ def _join_unit_to_cluster(self) -> None: self.unit.status = WaitingStatus("waiting to join the cluster") logger.debug("Waiting to joing the cluster, failed to acquire lock.") - def _restart(self, _: EventBase) -> None: + def _restart(self, event: EventBase) -> None: """Restart the MySQL service.""" + if self._mysql.is_unit_primary(self.unit_label): + restart_states = { + self.restart_peers.data[unit].get("state", "unset") for unit in self.peers.units + } + if restart_states != {"release"}: + # Wait other units restart first to minimize primary switchover + logger.debug("Primary is waiting for other units to restart") + event.defer() + return + self.unit.status = MaintenanceStatus("restarting MySQL") self._mysql.restart_mysqld() self.unit.status = ActiveStatus(self.active_status_message) From 096fe7f2e579133d43ff8dbe007bd6ac3b5b2c9a Mon Sep 17 00:00:00 2001 From: Paulo Machado Date: Wed, 11 Oct 2023 11:13:33 -0300 Subject: [PATCH 15/16] always escape --- lib/charms/mysql/v0/mysql.py | 8 ++------ tests/unit/test_mysql.py | 2 +- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/lib/charms/mysql/v0/mysql.py b/lib/charms/mysql/v0/mysql.py index 4099714e3..53d295176 100644 --- a/lib/charms/mysql/v0/mysql.py +++ b/lib/charms/mysql/v0/mysql.py @@ -116,7 +116,7 @@ def wait_until_mysql_connection(self) -> None: # Increment this PATCH version before using `charmcraft publish-lib` or reset # to 0 if you are raising the major API version -LIBPATCH = 50 +LIBPATCH = 51 UNIT_TEARDOWN_LOCKNAME = "unit-teardown" UNIT_ADD_LOCKNAME = "unit-add" @@ -1150,14 +1150,10 @@ def set_dynamic_variable( if not instance_address: instance_address = self.instance_address - # escape variable values when needed - if not re.match(r"^[0-9,a-z,A-Z$_]+$", value): - value = f"`{value}`" - logger.debug(f"Setting {variable} to {value} on {instance_address}") set_var_command = [ f"shell.connect('{self.server_config_user}:{self.server_config_password}@{instance_address}')", - f"session.run_sql(\"SET {'PERSIST' if persist else 'GLOBAL'} {variable}={value}\")", + f"session.run_sql(\"SET {'PERSIST' if persist else 'GLOBAL'} {variable}=`{value}`\")", ] try: diff --git a/tests/unit/test_mysql.py b/tests/unit/test_mysql.py index ea7151144..0123a024c 100644 --- a/tests/unit/test_mysql.py +++ b/tests/unit/test_mysql.py @@ -1656,7 +1656,7 @@ def test_set_dynamic_variables(self, _run_mysqlsh_script): """Test dynamic_variables.""" commands = ( f"shell.connect('{self.mysql.server_config_user}:{self.mysql.server_config_password}@127.0.0.1')", - 'session.run_sql("SET GLOBAL variable=value")', + 'session.run_sql("SET GLOBAL variable=`value`")', ) self.mysql.set_dynamic_variable(variable="variable", value="value") _run_mysqlsh_script.assert_called_with("\n".join(commands)) From ea0ecb933698dea0cbf526981a58a519bab28bdb Mon Sep 17 00:00:00 2001 From: Paulo Machado Date: Wed, 11 Oct 2023 16:10:38 -0300 Subject: [PATCH 16/16] escape needs to be conditional --- lib/charms/data_platform_libs/v0/upgrade.py | 9 ++++++--- lib/charms/mysql/v0/mysql.py | 6 +++++- tests/unit/test_mysql.py | 2 +- 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/lib/charms/data_platform_libs/v0/upgrade.py b/lib/charms/data_platform_libs/v0/upgrade.py index 5b900681c..b8c753776 100644 --- a/lib/charms/data_platform_libs/v0/upgrade.py +++ b/lib/charms/data_platform_libs/v0/upgrade.py @@ -285,7 +285,7 @@ def restart(self, event) -> None: # Increment this PATCH version before using `charmcraft publish-lib` or reset # to 0 if you are raising the major API version -LIBPATCH = 12 +LIBPATCH = 13 PYDEPS = ["pydantic>=1.10,<2", "poetry-core"] @@ -921,7 +921,10 @@ def on_upgrade_changed(self, event: EventBase) -> None: logger.debug("Cluster failed to upgrade, exiting...") return - if self.cluster_state == "recovery": + if self.substrate == "vm" and self.cluster_state == "recovery": + # Only defer for vm, that will set unit states to "ready" on upgrade-charm + # on k8s only the upgrading unit will receive the upgrade-charm event + # and deferring will prevent the upgrade stack from being popped logger.debug("Cluster in recovery, deferring...") event.defer() return @@ -944,7 +947,7 @@ def on_upgrade_changed(self, event: EventBase) -> None: logger.debug("upgrade-changed event handled before pre-checks, exiting...") return - logger.debug("Did not find upgrade-stack or completed cluster state, deferring...") + logger.debug("Did not find upgrade-stack or completed cluster state, skipping...") return # upgrade ongoing, set status for waiting units diff --git a/lib/charms/mysql/v0/mysql.py b/lib/charms/mysql/v0/mysql.py index 53d295176..504bc2211 100644 --- a/lib/charms/mysql/v0/mysql.py +++ b/lib/charms/mysql/v0/mysql.py @@ -1150,10 +1150,14 @@ def set_dynamic_variable( if not instance_address: instance_address = self.instance_address + # escape variable values when needed + if not re.match(r"^[0-9,a-z,A-Z$_]+$", value): + value = f"`{value}`" + logger.debug(f"Setting {variable} to {value} on {instance_address}") set_var_command = [ f"shell.connect('{self.server_config_user}:{self.server_config_password}@{instance_address}')", - f"session.run_sql(\"SET {'PERSIST' if persist else 'GLOBAL'} {variable}=`{value}`\")", + f"session.run_sql(\"SET {'PERSIST' if persist else 'GLOBAL'} {variable}={value}\")", ] try: diff --git a/tests/unit/test_mysql.py b/tests/unit/test_mysql.py index 0123a024c..ea7151144 100644 --- a/tests/unit/test_mysql.py +++ b/tests/unit/test_mysql.py @@ -1656,7 +1656,7 @@ def test_set_dynamic_variables(self, _run_mysqlsh_script): """Test dynamic_variables.""" commands = ( f"shell.connect('{self.mysql.server_config_user}:{self.mysql.server_config_password}@127.0.0.1')", - 'session.run_sql("SET GLOBAL variable=`value`")', + 'session.run_sql("SET GLOBAL variable=value")', ) self.mysql.set_dynamic_variable(variable="variable", value="value") _run_mysqlsh_script.assert_called_with("\n".join(commands))