From fce564084ce5dfb3213243ab8f5a5114bfcef4cc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Juli=C3=A1n=20Espina?= Date: Tue, 17 Dec 2024 16:46:02 -0600 Subject: [PATCH] feat: remove peer relation and improve integration tests --- .github/workflows/ci.yaml | 2 +- charmcraft.yaml | 8 +- justfile | 2 +- .../v0/{interfaces.py => filesystem_info.py} | 187 +++++++------- src/charm.py | 106 +++----- src/utils/manager.py | 241 +++++++----------- tests/integration/server/charmcraft.yaml | 6 +- .../v0/{interfaces.py => filesystem_info.py} | 187 +++++++------- tests/integration/server/requirements.txt | 3 +- tests/integration/server/src/charm.py | 156 ++++++++++-- tests/integration/test_charm.py | 59 +---- tests/unit/test_interfaces.py | 10 +- uv.lock | 12 +- 13 files changed, 482 insertions(+), 497 deletions(-) rename lib/charms/filesystem_client/v0/{interfaces.py => filesystem_info.py} (77%) rename tests/integration/server/lib/charms/filesystem_client/v0/{interfaces.py => filesystem_info.py} (77%) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 6f5942a..af3b333 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -80,7 +80,7 @@ jobs: with: version: 0.5.8 - name: Run tests - run: just type + run: just typecheck integration-test: name: Integration tests (LXD) diff --git a/charmcraft.yaml b/charmcraft.yaml index 0284f5d..d75a883 100644 --- a/charmcraft.yaml +++ b/charmcraft.yaml @@ -33,13 +33,9 @@ parts: subordinate: true -peers: - storage-peers: - interface: storage_peers - requires: - fs-share: - interface: fs_share + filesystem: + interface: filesystem_info juju-info: interface: juju-info scope: container diff --git a/justfile b/justfile index 5061b8b..210924a 100644 --- a/justfile +++ b/justfile @@ -39,7 +39,7 @@ lint: lock {{uv_run}} ruff format --check --diff {{all}} # Run static type checks -type *args: lock +typecheck *args: lock {{uv_run}} pyright {{args}} # Run unit tests diff --git a/lib/charms/filesystem_client/v0/interfaces.py b/lib/charms/filesystem_client/v0/filesystem_info.py similarity index 77% rename from lib/charms/filesystem_client/v0/interfaces.py rename to lib/charms/filesystem_client/v0/filesystem_info.py index 4927980..398b895 100644 --- a/lib/charms/filesystem_client/v0/interfaces.py +++ b/lib/charms/filesystem_client/v0/filesystem_info.py @@ -14,33 +14,33 @@ """Library to manage integrations between filesystem providers and consumers. -This library contains the FsProvides and FsRequires classes for managing an +This library contains the FilesystemProvides and FilesystemRequires classes for managing an integration between a filesystem server operator and a filesystem client operator. -## FsInfo (filesystem mount information) +## FilesystemInfo (filesystem mount information) This abstract class defines the methods that a filesystem type must expose for providers and consumers. Any subclass of this class will be compatible with the other methods exposed by the interface library, but the server and the client are the ones responsible for deciding which filesystems to support. -## FsRequires (filesystem client) +## FilesystemRequires (filesystem client) This class provides a uniform interface for charms that need to mount or unmount filesystems, and convenience methods for consuming data sent by a filesystem server charm. ### Defined events -- `mount_fs`: Event emitted when the filesystem is ready to be mounted. -- `umount_fs`: Event emitted when the filesystem needs to be unmounted. +- `mount_filesystem`: Event emitted when the filesystem is ready to be mounted. +- `umount_filesystem`: Event emitted when the filesystem needs to be unmounted. ### Example ``python import ops -from charms.storage_libs.v0.fs_interfaces import ( - FsRequires, - MountFsEvent, +from charms.filesystem_client.v0.filesystem_info import ( + FilesystemRequires, + MountFilesystemEvent, ) @@ -50,49 +50,49 @@ class StorageClientCharm(ops.CharmBase): def __init__(self, *args, **kwargs) -> None: super().__init__(*args, **kwargs) - # Charm events defined in the FsRequires class. - self._fs = FsRequires(self, "fs-share") + # Charm events defined in the FilesystemRequires class. + self._fs = FilesystemRequires(self, "filesystem") self.framework.observe( self._fs.on.mount_fs, self._on_mount_fs, ) - def _on_mount_fs(self, event: MountShareEvent) -> None: + def _on_mount_fs(self, event: MountFilesystemEvent) -> None: # Handle when new filesystem server is connected. - fs_info = event.fs_info + endpoint = event.endpoint - self.mount("/mnt", fs_info) + self.mount("/mnt", endpoint.info) self.unit.status = ops.ActiveStatus("Mounted filesystem at `/mnt`.") ``` -## FsProvides (filesystem server) +## FilesystemProvides (filesystem server) This library provides a uniform interface for charms that expose filesystems. > __Note:__ It is the responsibility of the provider charm to have -> the implementation for creating a new filesystem share. FsProvides just exposes +> the implementation for creating a new filesystem share. FilesystemProvides just exposes > the interface for the integration. ### Example ```python import ops -from charms.filesystem_client.v0.fs_interfaces import ( - FsProvides, +from charms.filesystem_client.v0.filesystem_info import ( + FilesystemProvides, NfsInfo, ) class StorageServerCharm(ops.CharmBase): def __init__(self, framework: ops.Framework): super().__init__(framework) - self._fs = FsProvides(self, "fs-share", "server-peers") + self._filesystem = FilesystemProvides(self, "filesystem", "server-peers") framework.observe(self.on.start, self._on_start) def _on_start(self, event: ops.StartEvent): # Handle start event. - self._fs.set_fs_info(NfsInfo("192.168.1.254", 65535, "/srv")) + self._filesystem.set_info(NfsInfo("192.168.1.254", 65535, "/srv")) self.unit.status = ops.ActiveStatus() ``` """ @@ -117,16 +117,18 @@ def _on_start(self, event: ops.StartEvent): from ops.model import Model, Relation __all__ = [ - "InterfacesError", - "ParseError", - "FsInfo", + "FilesystemInfoError", + "ParseUriError", + "FilesystemInfo", "NfsInfo", "CephfsInfo", "Endpoint", - "MountFsEvent", - "UmountFsEvent", - "FsRequires", - "FsProvides", + "FilesystemEvent", + "MountFilesystemEvent", + "UmountFilesystemEvent", + "FilesystemRequiresEvents", + "FilesystemRequires", + "FilesystemProvides", ] # The unique Charmhub library identifier, never change it @@ -142,11 +144,11 @@ def _on_start(self, event: ops.StartEvent): _logger = logging.getLogger(__name__) -class InterfacesError(Exception): +class FilesystemInfoError(Exception): """Exception raised when an operation failed.""" -class ParseError(InterfacesError): +class ParseUriError(FilesystemInfoError): """Exception raised when a parse operation from an URI failed.""" @@ -215,9 +217,9 @@ def __init__( options: dict[str, str] = {}, ): if not scheme: - raise InterfacesError("scheme cannot be empty") + raise FilesystemInfoError("scheme cannot be empty") if len(hosts) == 0: - raise InterfacesError("list of hosts cannot be empty") + raise FilesystemInfoError("list of hosts cannot be empty") # Strictly convert to the required types to avoid passing through weird data. object.__setattr__(self, "scheme", str(scheme)) @@ -239,7 +241,7 @@ def from_uri(cls, uri: str) -> "_UriData": hostname = unquote(result.hostname or "") if not hostname or hostname[0] != "(" or hostname[-1] != ")": - raise ParseError(f"invalid list of hosts for endpoint `{uri}`") + raise ParseUriError(f"invalid list of hosts for endpoint `{uri}`") hosts = hostname[1:-1].split(",") path = unquote(result.path or "") @@ -249,11 +251,11 @@ def from_uri(cls, uri: str) -> "_UriData": for key, values in parse_qs(result.query, strict_parsing=True).items() } except ValueError: - raise ParseError(f"invalid options for endpoint `{uri}`") + raise ParseUriError(f"invalid options for endpoint `{uri}`") try: return _UriData(scheme=scheme, user=user, hosts=hosts, path=path, options=options) - except InterfacesError as e: - raise ParseError(*e.args) + except FilesystemInfoError as e: + raise ParseUriError(*e.args) def __str__(self) -> str: user = quote(self.user) @@ -268,14 +270,14 @@ def _hostinfo(host: str) -> tuple[str, Optional[int]]: """Parse a host string into the hostname and the port.""" _logger.debug(f"_hostinfo: parsing `{host}`") if len(host) == 0: - raise ParseError("invalid empty host") + raise ParseUriError("invalid empty host") pos = 0 if host[pos] == "[": # IPv6 pos = host.find("]", pos) if pos == -1: - raise ParseError("unclosed bracket for host") + raise ParseUriError("unclosed bracket for host") hostname = host[1:pos] pos = pos + 1 else: @@ -291,19 +293,19 @@ def _hostinfo(host: str) -> tuple[str, Optional[int]]: # more characters after the hostname <==> port if host[pos] != ":": - raise ParseError("expected `:` after IPv6 address") + raise ParseUriError("expected `:` after IPv6 address") try: port = int(host[pos + 1 :]) except ValueError: - raise ParseError("expected int after `:` in host") + raise ParseUriError("expected int after `:` in host") return hostname, port -T = TypeVar("T", bound="FsInfo") +T = TypeVar("T", bound="FilesystemInfo") -class FsInfo(ABC): +class FilesystemInfo(ABC): """Information to mount a filesystem. This is an abstract class that exposes a set of required methods. All filesystems that @@ -313,14 +315,14 @@ class FsInfo(ABC): @classmethod @abstractmethod def from_uri(cls: type[T], uri: str, model: Model) -> T: - """Convert an URI string into a `FsInfo` object.""" + """Convert an URI string into a `FilesystemInfo` object.""" @abstractmethod def to_uri(self, model: Model) -> str: - """Convert this `FsInfo` object into an URI string.""" + """Convert this `FilesystemInfo` object into an URI string.""" def grant(self, model: Model, relation: ops.Relation): - """Grant permissions for a certain relation to any secrets that this `FsInfo` has. + """Grant permissions for a certain relation to any secrets that this `FilesystemInfo` has. This is an optional method because not all filesystems will require secrets to be mounted on the client. @@ -328,12 +330,12 @@ def grant(self, model: Model, relation: ops.Relation): @classmethod @abstractmethod - def fs_type(cls) -> str: + def filesystem_type(cls) -> str: """Get the string identifier of this filesystem type.""" @dataclass(frozen=True) -class NfsInfo(FsInfo): +class NfsInfo(FilesystemInfo): """Information required to mount an NFS share.""" hostname: str @@ -347,13 +349,13 @@ class NfsInfo(FsInfo): @classmethod def from_uri(cls, uri: str, _model: Model) -> "NfsInfo": - """See :py:meth:`FsInfo.from_uri` for documentation on this method.""" + """See :py:meth:`FilesystemInfo.from_uri` for documentation on this method.""" _logger.debug(f"NfsInfo.from_uri: parsing `{uri}`") info = _UriData.from_uri(uri) - if info.scheme != cls.fs_type(): - raise ParseError( + if info.scheme != cls.filesystem_type(): + raise ParseUriError( "could not parse `EndpointInfo` with incompatible scheme into `NfsInfo`" ) @@ -372,7 +374,7 @@ def from_uri(cls, uri: str, _model: Model) -> "NfsInfo": return NfsInfo(hostname=hostname, port=port, path=path) def to_uri(self, _model: Model) -> str: - """See :py:meth:`FsInfo.to_uri` for documentation on this method.""" + """See :py:meth:`FilesystemInfo.to_uri` for documentation on this method.""" try: IPv6Address(self.hostname) host = f"[{self.hostname}]" @@ -381,16 +383,16 @@ def to_uri(self, _model: Model) -> str: hosts = [host + f":{self.port}" if self.port else ""] - return str(_UriData(scheme=self.fs_type(), hosts=hosts, path=self.path)) + return str(_UriData(scheme=self.filesystem_type(), hosts=hosts, path=self.path)) @classmethod - def fs_type(cls) -> str: - """See :py:meth:`FsInfo.fs_type` for documentation on this method.""" + def filesystem_type(cls) -> str: + """See :py:meth:`FilesystemInfo.fs_type` for documentation on this method.""" return "nfs" @dataclass(frozen=True) -class CephfsInfo(FsInfo): +class CephfsInfo(FilesystemInfo): """Information required to mount a CephFS share.""" fsid: str @@ -413,35 +415,33 @@ class CephfsInfo(FsInfo): @classmethod def from_uri(cls, uri: str, model: Model) -> "CephfsInfo": - """See :py:meth:`FsInfo.from_uri` for documentation on this method.""" + """See :py:meth:`FilesystemInfo.from_uri` for documentation on this method.""" _logger.debug(f"CephfsInfo.from_uri: parsing `{uri}`") info = _UriData.from_uri(uri) - if info.scheme != cls.fs_type(): - raise ParseError( - "could not parse `EndpointInfo` with incompatible scheme into `CephfsInfo`" - ) + if info.scheme != cls.filesystem_type(): + raise ParseUriError("could not parse uri with incompatible scheme into `CephfsInfo`") path = info.path if not (user := info.user): - raise ParseError("missing user in uri for `CephfsInfo") + raise ParseUriError("missing user in uri for `CephfsInfo") if not (name := info.options.get("name")): - raise ParseError("missing name in uri for `CephfsInfo`") + raise ParseUriError("missing name in uri for `CephfsInfo`") if not (fsid := info.options.get("fsid")): - raise ParseError("missing fsid in uri for `CephfsInfo`") + raise ParseUriError("missing fsid in uri for `CephfsInfo`") monitor_hosts = info.hosts if not (auth := info.options.get("auth")): - raise ParseError("missing auth info in uri for `CephsInfo`") + raise ParseUriError("missing auth info in uri for `CephsInfo`") try: kind, data = auth.split(":", 1) except ValueError: - raise ParseError("Could not get the kind of auth info") + raise ParseUriError("Could not get the kind of auth info") if kind == "secret": key = model.get_secret(id=auth).get_content(refresh=True)["key"] @@ -450,14 +450,14 @@ def from_uri(cls, uri: str, model: Model) -> "CephfsInfo": # they don't support secrets. key = data else: - raise ParseError(f"Invalid kind `{kind}` for auth info") + raise ParseUriError(f"Invalid kind `{kind}` for auth info") return CephfsInfo( fsid=fsid, name=name, path=path, monitor_hosts=monitor_hosts, user=user, key=key ) def to_uri(self, model: Model) -> str: - """See :py:meth:`FsInfo.to_uri` for documentation on this method.""" + """See :py:meth:`FilesystemInfo.to_uri` for documentation on this method.""" secret = self._get_or_create_auth_secret(model) options = { @@ -469,7 +469,7 @@ def to_uri(self, model: Model) -> str: return str( _UriData( - scheme=self.fs_type(), + scheme=self.filesystem_type(), hosts=self.monitor_hosts, path=self.path, user=self.user, @@ -478,14 +478,14 @@ def to_uri(self, model: Model) -> str: ) def grant(self, model: Model, relation: Relation): - """See :py:meth:`FsInfo.grant` for documentation on this method.""" + """See :py:meth:`FilesystemInfo.grant` for documentation on this method.""" secret = self._get_or_create_auth_secret(model) secret.grant(relation) @classmethod - def fs_type(cls) -> str: - """See :py:meth:`FsInfo.fs_type` for documentation on this method.""" + def filesystem_type(cls) -> str: + """See :py:meth:`FilesystemInfo.fs_type` for documentation on this method.""" return "cephfs" def _get_or_create_auth_secret(self, model: Model) -> ops.Secret: @@ -505,25 +505,28 @@ def _get_or_create_auth_secret(self, model: Model) -> ops.Secret: class Endpoint: """Endpoint data exposed by a filesystem server.""" - fs_info: FsInfo + info: FilesystemInfo """Filesystem information required to mount this endpoint.""" uri: str """Raw URI exposed by this endpoint.""" + # Right now this is unused on the client, but having the raw uri + # available was useful on a previous version of the charm, so leaving + # this exposed just in case we need it in the future. -def _uri_to_fs_info(uri: str, model: Model) -> FsInfo: +def _uri_to_fs_info(uri: str, model: Model) -> FilesystemInfo: scheme = uri.split("://", maxsplit=1)[0] - if scheme == NfsInfo.fs_type(): + if scheme == NfsInfo.filesystem_type(): return NfsInfo.from_uri(uri, model) - elif scheme == CephfsInfo.fs_type(): + elif scheme == CephfsInfo.filesystem_type(): return CephfsInfo.from_uri(uri, model) else: - raise InterfacesError(f"unsupported filesystem type `{scheme}`") + raise FilesystemInfoError(f"unsupported filesystem type `{scheme}`") -class _MountEvent(RelationEvent): - """Base event for mount-related events.""" +class FilesystemEvent(RelationEvent): + """Base event for filesystem-related events.""" @property def endpoint(self) -> Optional[Endpoint]: @@ -533,19 +536,19 @@ def endpoint(self) -> Optional[Endpoint]: return Endpoint(_uri_to_fs_info(uri, self.framework.model), uri) -class MountFsEvent(_MountEvent): +class MountFilesystemEvent(FilesystemEvent): """Emit when a filesystem is ready to be mounted.""" -class UmountFsEvent(_MountEvent): +class UmountFilesystemEvent(FilesystemEvent): """Emit when a filesystem needs to be unmounted.""" -class _FsRequiresEvents(CharmEvents): +class FilesystemRequiresEvents(CharmEvents): """Events that FS servers can emit.""" - mount_fs = EventSource(MountFsEvent) - umount_fs = EventSource(UmountFsEvent) + mount_filesystem = EventSource(MountFilesystemEvent) + umount_filesystem = EventSource(UmountFilesystemEvent) class _BaseInterface(Object): @@ -572,10 +575,10 @@ def relations(self) -> List[Relation]: return result -class FsRequires(_BaseInterface): +class FilesystemRequires(_BaseInterface): """Consumer-side interface of filesystem integrations.""" - on = _FsRequiresEvents() + on = FilesystemRequiresEvents() def __init__(self, charm: CharmBase, relation_name: str) -> None: super().__init__(charm, relation_name) @@ -587,12 +590,12 @@ def __init__(self, charm: CharmBase, relation_name: str) -> None: def _on_relation_changed(self, event: RelationChangedEvent) -> None: """Handle when the databag between client and server has been updated.""" _logger.debug("Emitting `MountShare` event from `RelationChanged` hook") - self.on.mount_fs.emit(event.relation, app=event.app, unit=event.unit) + self.on.mount_filesystem.emit(event.relation, app=event.app, unit=event.unit) def _on_relation_departed(self, event: RelationDepartedEvent) -> None: """Handle when server departs integration.""" _logger.debug("Emitting `UmountShare` event from `RelationDeparted` hook") - self.on.umount_fs.emit(event.relation, app=event.app, unit=event.unit) + self.on.umount_filesystem.emit(event.relation, app=event.app, unit=event.unit) @property def endpoints(self) -> List[Endpoint]: @@ -601,11 +604,11 @@ def endpoints(self) -> List[Endpoint]: for relation in self.relations: if not (uri := relation.data[relation.app].get("endpoint")): continue - result.append(Endpoint(fs_info=_uri_to_fs_info(uri, self.model), uri=uri)) + result.append(Endpoint(info=_uri_to_fs_info(uri, self.model), uri=uri)) return result -class FsProvides(_BaseInterface): +class FilesystemProvides(_BaseInterface): """Provider-side interface of filesystem integrations.""" def __init__(self, charm: CharmBase, relation_name: str, peer_relation_name: str) -> None: @@ -613,11 +616,11 @@ def __init__(self, charm: CharmBase, relation_name: str, peer_relation_name: str self._peer_relation_name = peer_relation_name self.framework.observe(charm.on[relation_name].relation_joined, self._update_relation) - def set_fs_info(self, fs_info: FsInfo) -> None: + def set_info(self, info: FilesystemInfo) -> None: """Set information to mount a filesystem. Args: - fs_info: Information required to mount the filesystem. + info: Information required to mount the filesystem. Notes: Only the application leader unit can set the filesystem data. @@ -625,12 +628,12 @@ def set_fs_info(self, fs_info: FsInfo) -> None: if not self.unit.is_leader(): return - uri = fs_info.to_uri(self.model) + uri = info.to_uri(self.model) self._endpoint = uri for relation in self.relations: - fs_info.grant(self.model, relation) + info.grant(self.model, relation) relation.data[self.app]["endpoint"] = uri def _update_relation(self, event: RelationJoinedEvent) -> None: @@ -666,7 +669,7 @@ def _get_state(self, key: str) -> Optional[str]: def _set_state(self, key: str, data: str) -> None: """Insert a value into the global state.""" if not self._peers: - raise InterfacesError( + raise FilesystemInfoError( "Peer relation can only be accessed after the relation is established" ) diff --git a/src/charm.py b/src/charm.py index 9bbe0de..473be18 100755 --- a/src/charm.py +++ b/src/charm.py @@ -7,12 +7,9 @@ import json import logging from collections import Counter -from contextlib import contextmanager -from typing import Any, Generator, Optional -import charms.operator_libs_linux.v0.apt as apt import ops -from charms.filesystem_client.v0.interfaces import FsRequires +from charms.filesystem_client.v0.filesystem_info import FilesystemRequires from jsonschema import ValidationError, validate from utils.manager import MountsManager @@ -35,12 +32,13 @@ }, } -PEER_NAME = "storage-peers" - class StopCharmError(Exception): """Exception raised when a method needs to finish the execution of the charm code.""" + def __init__(self, status: ops.StatusBase): + self.status = status + # Trying to use a delta charm (one method per event) proved to be a bit unwieldy, since # we would have to handle multiple updates at once: @@ -60,13 +58,13 @@ class FilesystemClientCharm(ops.CharmBase): def __init__(self, framework: ops.Framework): super().__init__(framework) - self._fs_share = FsRequires(self, "fs-share") - self._mounts_manager = MountsManager() + self._filesystems = FilesystemRequires(self, "filesystem") + self._mounts_manager = MountsManager(self) framework.observe(self.on.upgrade_charm, self._handle_event) framework.observe(self.on.update_status, self._handle_event) framework.observe(self.on.config_changed, self._handle_event) - framework.observe(self._fs_share.on.mount_fs, self._handle_event) - framework.observe(self._fs_share.on.umount_fs, self._handle_event) + framework.observe(self._filesystems.on.mount_filesystem, self._handle_event) + framework.observe(self._filesystems.on.umount_filesystem, self._handle_event) def _handle_event(self, event: ops.EventBase) -> None: # noqa: C901 try: @@ -75,18 +73,19 @@ def _handle_event(self, event: ops.EventBase) -> None: # noqa: C901 self._ensure_installed() config = self._get_config() self._mount_filesystems(config) - except StopCharmError: + except StopCharmError as e: # This was the cleanest way to ensure the inner methods can still return prematurely # when an error occurs. + self.app.status = e.status return - self.unit.status = ops.ActiveStatus("Mounted shares.") + self.unit.status = ops.ActiveStatus("Mounted filesystems.") def _ensure_installed(self): """Ensure the required packages are installed into the unit.""" if not self._mounts_manager.installed: self.unit.status = ops.MaintenanceStatus("Installing required packages.") - self._mounts_manager.ensure(apt.PackageState.Present) + self._mounts_manager.install() def _get_config(self) -> dict[str, dict[str, str | bool]]: """Get and validate the configuration of the charm.""" @@ -99,39 +98,30 @@ def _get_config(self) -> dict[str, dict[str, str | bool]]: opts[opt] = opts.get(opt, False) return config except (json.JSONDecodeError, ValidationError) as e: - self.app.status = ops.BlockedStatus( - f"invalid configuration for option `mountinfo`. reason: {e}" + raise StopCharmError( + ops.BlockedStatus(f"invalid configuration for option `mountinfo`. reason:\n{e}") ) - raise StopCharmError() def _mount_filesystems(self, config: dict[str, dict[str, str | bool]]): """Mount all available filesystems for the charm.""" - shares = self._fs_share.endpoints - active_filesystems = set() - for fs_type, count in Counter([share.fs_info.fs_type() for share in shares]).items(): + endpoints = self._filesystems.endpoints + for fs_type, count in Counter( + [endpoint.info.filesystem_type() for endpoint in endpoints] + ).items(): if count > 1: - self.app.status = ops.BlockedStatus( - f"Too many relations for mount type `{fs_type}`." + raise StopCharmError( + ops.BlockedStatus(f"Too many relations for mount type `{fs_type}`.") ) - raise StopCharmError() - active_filesystems.add(fs_type) - - with self.mounts() as mounts: - # Cleanup and unmount all the mounts that are not available. - for fs_type in list(mounts.keys()): - if fs_type not in active_filesystems: - self._mounts_manager.umount(str(mounts[fs_type]["mountpoint"])) - del mounts[fs_type] - - for share in shares: - fs_type = share.fs_info.fs_type() + + self.unit.status = ops.MaintenanceStatus("Ensuring filesystems are mounted.") + + with self._mounts_manager.mounts() as mounts: + for endpoint in endpoints: + fs_type = endpoint.info.filesystem_type() if not (options := config.get(fs_type)): - self.app.status = ops.BlockedStatus( - f"Missing configuration for mount type `{fs_type}." + raise StopCharmError( + ops.BlockedStatus(f"Missing configuration for mount type `{fs_type}`.") ) - raise StopCharmError() - - options["uri"] = share.uri mountpoint = str(options["mountpoint"]) @@ -140,45 +130,7 @@ def _mount_filesystems(self, config: dict[str, dict[str, str | bool]]): opts.append("nosuid" if options.get("nosuid") else "suid") opts.append("nodev" if options.get("nodev") else "dev") opts.append("ro" if options.get("read-only") else "rw") - - self.unit.status = ops.MaintenanceStatus(f"Mounting `{mountpoint}`") - - if not (mount := mounts.get(fs_type)) or mount != options: - # Just in case, unmount the previously mounted share - if mount: - self._mounts_manager.umount(str(mount["mountpoint"])) - self._mounts_manager.mount(share.fs_info, mountpoint, options=opts) - mounts[fs_type] = options - - @property - def peers(self) -> Optional[ops.Relation]: - """Fetch the peer relation.""" - return self.model.get_relation(PEER_NAME) - - @contextmanager - def mounts(self) -> Generator[dict[str, dict[str, str | bool]], None, None]: - """Get the mounted filesystems.""" - mounts = self.get_state("mounts") - yield mounts - # Don't set the state if the program throws an error. - # This guarantees we're in a clean state after the charm unblocks. - self.set_state("mounts", mounts) - - def set_state(self, key: str, data: Any) -> None: - """Insert a value into the global state.""" - if not self.peers: - raise RuntimeError( - "Peer relation can only be written to after the relation is established" - ) - self.peers.data[self.app][key] = json.dumps(data) - - def get_state(self, key: str) -> dict[Any, Any]: - """Get a value from the global state.""" - if not self.peers: - return {} - - data = self.peers.data[self.app].get(key, "{}") - return json.loads(data) + mounts.add(info=endpoint.info, mountpoint=mountpoint, options=opts) if __name__ == "__main__": # pragma: nocover diff --git a/src/utils/manager.py b/src/utils/manager.py index 423df74..3a9fe7a 100644 --- a/src/utils/manager.py +++ b/src/utils/manager.py @@ -3,18 +3,19 @@ """Manage machine mounts and dependencies.""" +import contextlib import logging import os import pathlib -import shutil import subprocess from dataclasses import dataclass from ipaddress import AddressValueError, IPv6Address -from typing import Iterator, List, Optional, Union +from typing import Generator, List, Optional, Union import charms.operator_libs_linux.v0.apt as apt import charms.operator_libs_linux.v1.systemd as systemd -from charms.filesystem_client.v0.interfaces import CephfsInfo, FsInfo, NfsInfo +import ops +from charms.filesystem_client.v0.filesystem_info import CephfsInfo, FilesystemInfo, NfsInfo _logger = logging.getLogger(__name__) @@ -53,15 +54,57 @@ class MountInfo: passno: str +@dataclass +class _MountInfo: + endpoint: str + options: [str] + + +class Mounts: + """Collection of mounts that need to be managed by the `MountsManager`.""" + + _mounts: dict[str, _MountInfo] + + def __init__(self): + self._mounts = {} + + def add( + self, + info: FilesystemInfo, + mountpoint: Union[str, os.PathLike], + options: Optional[List[str]] = None, + ) -> None: + """Add a mount to the list of managed mounts. + + Args: + info: Share information required to mount the share. + mountpoint: System location to mount the share. + options: Mount options to pass when mounting the share. + + Raises: + Error: Raised if the mount operation fails. + """ + if options is None: + options = [] + + endpoint, additional_opts = _get_endpoint_and_opts(info) + options = sorted(options + additional_opts) + + self._mounts[str(mountpoint)] = _MountInfo(endpoint=endpoint, options=options) + + class MountsManager: """Manager for mounted filesystems in the current system.""" - def __init__(self): + def __init__(self, charm: ops.CharmBase): # Lazily initialized self._pkgs = None + self._master_file = pathlib.Path(f"/etc/auto.master.d/{charm.app.name}.autofs") + self._autofs_file = pathlib.Path(f"/etc/auto.{charm.app.name}") @property def _packages(self) -> List[apt.DebianPackage]: + """List of packages required by the client.""" if not self._pkgs: self._pkgs = [ apt.DebianPackage.from_system(pkg) @@ -75,23 +118,35 @@ def installed(self) -> bool: for pkg in self._packages: if not pkg.present: return False + + if not self._master_file.exists or not self._autofs_file.exists: + return False + return True - def ensure(self, state: apt.PackageState) -> None: - """Ensure that the mount packages are in the specified state. + def install(self): + """Install the required mount packages. Raises: Error: Raised if this failed to change the state of any of the required packages. """ try: for pkg in self._packages: - pkg.ensure(state) + pkg.ensure(apt.PackageState.Present) except (apt.PackageError, apt.PackageNotFoundError) as e: _logger.error( - f"failed to change the state of the required packages. Reason:\n{e.message}" + f"failed to change the state of the required packages. reason:\n{e.message}" ) raise Error(e.message) + try: + self._master_file.touch(mode=0o600) + self._autofs_file.touch(mode=0o600) + self._master_file.write_text(f"/- {self._autofs_file}") + except IOError as e: + _logger.error(f"failed to create the required autofs files. reason:\n{e}") + raise Error("failed to create the required autofs files") + def supported(self) -> bool: """Check if underlying base supports mounting shares.""" try: @@ -107,156 +162,46 @@ def supported(self) -> bool: _logger.warning("Could not detect execution in virtualized environment") return True - def fetch(self, target: str) -> Optional[MountInfo]: - """Fetch information about a mount. + @contextlib.contextmanager + def mounts(self, force_mount=False) -> Generator[Mounts, None, None]: + """Get the list of `Mounts` that need to be managed by the `MountsManager`. - Args: - target: share mountpoint information to fetch. - - Returns: - Optional[MountInfo]: Mount information. None if share is not mounted. + It will initially contain no mounts, and any mount that is added to + `Mounts` will be mounted by the manager. Mounts that were + added on previous executions will get removed if they're not added again + to the `Mounts` object. """ - # We need to trigger an automount for the mounts that are of type `autofs`, - # since those could contain an unlisted mount. - _trigger_autofs() - - for mount in _mounts(): - if mount.mountpoint == target: - return mount - - return None - - def mounts(self) -> List[MountInfo]: - """Get all mounts on a machine. - - Returns: - List[MountInfo]: All current mounts on machine. - """ - _trigger_autofs() - - return list(_mounts("autofs")) - - def mounted(self, target: str) -> bool: - """Determine if mountpoint is mounted. - - Args: - target: share mountpoint to check. - """ - return self.fetch(target) is not None - - def mount( - self, - share_info: FsInfo, - mountpoint: Union[str, os.PathLike], - options: Optional[List[str]] = None, - ) -> None: - """Mount a share. - - Args: - share_info: Share information required to mount the share. - mountpoint: System location to mount the share. - options: Mount options to pass when mounting the share. - - Raises: - Error: Raised if the mount operation fails. - """ - if options is None: - options = [] - # Try to create the mountpoint without checking if it exists to avoid TOCTOU. - target = pathlib.Path(mountpoint) - try: - target.mkdir() - _logger.debug(f"Created mountpoint {mountpoint}.") - except FileExistsError: - _logger.warning(f"Mountpoint {mountpoint} already exists.") - - endpoint, additional_opts = _get_endpoint_and_opts(share_info) - options = options + additional_opts - - _logger.debug(f"Mounting share {endpoint} at {target}") - autofs_id = _mountpoint_to_autofs_id(target) - pathlib.Path(f"/etc/auto.master.d/{autofs_id}.autofs").write_text( - f"/- /etc/auto.{autofs_id}" - ) - pathlib.Path(f"/etc/auto.{autofs_id}").write_text( - f"{target} -{','.join(options)} {endpoint}" + mounts = Mounts() + yield mounts + # This will not resume if the caller raised an exception, which + # should be enough to ensure the file is not written if the charm entered + # an error state. + new_autofs = "\n".join( + ( + f"{mountpoint} -{','.join(info.options)} {info.endpoint}" + for mountpoint, info in sorted(mounts._mounts.items()) + ) ) - try: - systemd.service_reload("autofs", restart_on_failure=True) - except systemd.SystemdError as e: - _logger.error(f"Failed to mount {endpoint} at {target}. Reason:\n{e}") - if "Operation not permitted" in str(e) and not self.supported(): - raise Error("Mounting shares not supported on LXD containers") - raise Error(f"Failed to mount {endpoint} at {target}") - - def umount(self, mountpoint: Union[str, os.PathLike]) -> None: - """Unmount a share. - - Args: - mountpoint: share mountpoint to unmount. + old_autofs = self._autofs_file.read_text() - Raises: - Error: Raised if the unmount operation fails. - """ - _logger.debug(f"Unmounting share at mountpoint {mountpoint}") - autofs_id = _mountpoint_to_autofs_id(mountpoint) - pathlib.Path(f"/etc/auto.{autofs_id}").unlink(missing_ok=True) - pathlib.Path(f"/etc/auto.master.d/{autofs_id}.autofs").unlink(missing_ok=True) + # Avoid restarting autofs if the config didn't change. + if not force_mount and new_autofs == old_autofs: + return try: + for mount in mounts._mounts.keys(): + pathlib.Path(mount).mkdir(parents=True, exist_ok=True) + self._autofs_file.write_text(new_autofs) systemd.service_reload("autofs", restart_on_failure=True) except systemd.SystemdError as e: - _logger.error(f"Failed to unmount {mountpoint}. Reason:\n{e}") - raise Error(f"Failed to unmount {mountpoint}") - - shutil.rmtree(mountpoint, ignore_errors=True) - - -def _trigger_autofs() -> None: - """Triggers a mount on all filesystems handled by autofs. - - This function is useful to make autofs-managed mounts appear on the - `/proc/mount` file, since they could be unmounted when reading the file. - """ - for fs in _mounts("autofs"): - _logger.info(f"triggering automount for `{fs.mountpoint}`") - try: - os.scandir(fs.mountpoint).close() - except OSError as e: - # Not critical since it could also be caused by unrelated mounts, - # but should be good to log it in case this causes problems. - _logger.warning(f"Could not trigger automount for `{fs.mountpoint}`. Reason:\n{e}") - - -def _mountpoint_to_autofs_id(mountpoint: Union[str, os.PathLike]) -> str: - """Get the autofs id of a mountpoint path. - - Args: - mountpoint: share mountpoint. - """ - path = pathlib.Path(mountpoint).resolve() - return str(path).lstrip("/").replace("/", "-") - - -def _mounts(fstype: str = "") -> Iterator[MountInfo]: - """Get an iterator of all mounts in the system that have the requested fstype. - - Returns: - Iterator[MountInfo]: All the mounts with a valid fstype. - """ - with pathlib.Path("/proc/mounts").open("rt") as mounts: - for mount in mounts: - # Lines in /proc/mounts follow the standard format - # - m = MountInfo(*mount.split()) - if fstype and not m.fstype.startswith(fstype): - continue - - yield m + _logger.error(f"failed to mount filesystems. reason:\n{e}") + if "Operation not permitted" in str(e) and not self.supported(): + raise Error("mounting shares not supported on LXD containers") + raise Error("failed to mount filesystems") -def _get_endpoint_and_opts(info: FsInfo) -> tuple[str, [str]]: +def _get_endpoint_and_opts(info: FilesystemInfo) -> tuple[str, [str]]: match info: case NfsInfo(hostname=hostname, port=port, path=path): try: @@ -279,6 +224,6 @@ def _get_endpoint_and_opts(info: FsInfo) -> tuple[str, [str]]: f"secret={secret}", ] case _: - raise Error(f"unsupported filesystem type `{info.fs_type()}`") + raise Error(f"unsupported filesystem type `{info.filesystem_type()}`") return endpoint, options diff --git a/tests/integration/server/charmcraft.yaml b/tests/integration/server/charmcraft.yaml index 92cbf7b..0ad49e7 100644 --- a/tests/integration/server/charmcraft.yaml +++ b/tests/integration/server/charmcraft.yaml @@ -18,11 +18,11 @@ parts: peers: server-peers: - interface: server_peers + interface: server_peers provides: - fs-share: - interface: fs_share + filesystem: + interface: filesystem_info limit: 1 config: diff --git a/tests/integration/server/lib/charms/filesystem_client/v0/interfaces.py b/tests/integration/server/lib/charms/filesystem_client/v0/filesystem_info.py similarity index 77% rename from tests/integration/server/lib/charms/filesystem_client/v0/interfaces.py rename to tests/integration/server/lib/charms/filesystem_client/v0/filesystem_info.py index 4927980..398b895 100644 --- a/tests/integration/server/lib/charms/filesystem_client/v0/interfaces.py +++ b/tests/integration/server/lib/charms/filesystem_client/v0/filesystem_info.py @@ -14,33 +14,33 @@ """Library to manage integrations between filesystem providers and consumers. -This library contains the FsProvides and FsRequires classes for managing an +This library contains the FilesystemProvides and FilesystemRequires classes for managing an integration between a filesystem server operator and a filesystem client operator. -## FsInfo (filesystem mount information) +## FilesystemInfo (filesystem mount information) This abstract class defines the methods that a filesystem type must expose for providers and consumers. Any subclass of this class will be compatible with the other methods exposed by the interface library, but the server and the client are the ones responsible for deciding which filesystems to support. -## FsRequires (filesystem client) +## FilesystemRequires (filesystem client) This class provides a uniform interface for charms that need to mount or unmount filesystems, and convenience methods for consuming data sent by a filesystem server charm. ### Defined events -- `mount_fs`: Event emitted when the filesystem is ready to be mounted. -- `umount_fs`: Event emitted when the filesystem needs to be unmounted. +- `mount_filesystem`: Event emitted when the filesystem is ready to be mounted. +- `umount_filesystem`: Event emitted when the filesystem needs to be unmounted. ### Example ``python import ops -from charms.storage_libs.v0.fs_interfaces import ( - FsRequires, - MountFsEvent, +from charms.filesystem_client.v0.filesystem_info import ( + FilesystemRequires, + MountFilesystemEvent, ) @@ -50,49 +50,49 @@ class StorageClientCharm(ops.CharmBase): def __init__(self, *args, **kwargs) -> None: super().__init__(*args, **kwargs) - # Charm events defined in the FsRequires class. - self._fs = FsRequires(self, "fs-share") + # Charm events defined in the FilesystemRequires class. + self._fs = FilesystemRequires(self, "filesystem") self.framework.observe( self._fs.on.mount_fs, self._on_mount_fs, ) - def _on_mount_fs(self, event: MountShareEvent) -> None: + def _on_mount_fs(self, event: MountFilesystemEvent) -> None: # Handle when new filesystem server is connected. - fs_info = event.fs_info + endpoint = event.endpoint - self.mount("/mnt", fs_info) + self.mount("/mnt", endpoint.info) self.unit.status = ops.ActiveStatus("Mounted filesystem at `/mnt`.") ``` -## FsProvides (filesystem server) +## FilesystemProvides (filesystem server) This library provides a uniform interface for charms that expose filesystems. > __Note:__ It is the responsibility of the provider charm to have -> the implementation for creating a new filesystem share. FsProvides just exposes +> the implementation for creating a new filesystem share. FilesystemProvides just exposes > the interface for the integration. ### Example ```python import ops -from charms.filesystem_client.v0.fs_interfaces import ( - FsProvides, +from charms.filesystem_client.v0.filesystem_info import ( + FilesystemProvides, NfsInfo, ) class StorageServerCharm(ops.CharmBase): def __init__(self, framework: ops.Framework): super().__init__(framework) - self._fs = FsProvides(self, "fs-share", "server-peers") + self._filesystem = FilesystemProvides(self, "filesystem", "server-peers") framework.observe(self.on.start, self._on_start) def _on_start(self, event: ops.StartEvent): # Handle start event. - self._fs.set_fs_info(NfsInfo("192.168.1.254", 65535, "/srv")) + self._filesystem.set_info(NfsInfo("192.168.1.254", 65535, "/srv")) self.unit.status = ops.ActiveStatus() ``` """ @@ -117,16 +117,18 @@ def _on_start(self, event: ops.StartEvent): from ops.model import Model, Relation __all__ = [ - "InterfacesError", - "ParseError", - "FsInfo", + "FilesystemInfoError", + "ParseUriError", + "FilesystemInfo", "NfsInfo", "CephfsInfo", "Endpoint", - "MountFsEvent", - "UmountFsEvent", - "FsRequires", - "FsProvides", + "FilesystemEvent", + "MountFilesystemEvent", + "UmountFilesystemEvent", + "FilesystemRequiresEvents", + "FilesystemRequires", + "FilesystemProvides", ] # The unique Charmhub library identifier, never change it @@ -142,11 +144,11 @@ def _on_start(self, event: ops.StartEvent): _logger = logging.getLogger(__name__) -class InterfacesError(Exception): +class FilesystemInfoError(Exception): """Exception raised when an operation failed.""" -class ParseError(InterfacesError): +class ParseUriError(FilesystemInfoError): """Exception raised when a parse operation from an URI failed.""" @@ -215,9 +217,9 @@ def __init__( options: dict[str, str] = {}, ): if not scheme: - raise InterfacesError("scheme cannot be empty") + raise FilesystemInfoError("scheme cannot be empty") if len(hosts) == 0: - raise InterfacesError("list of hosts cannot be empty") + raise FilesystemInfoError("list of hosts cannot be empty") # Strictly convert to the required types to avoid passing through weird data. object.__setattr__(self, "scheme", str(scheme)) @@ -239,7 +241,7 @@ def from_uri(cls, uri: str) -> "_UriData": hostname = unquote(result.hostname or "") if not hostname or hostname[0] != "(" or hostname[-1] != ")": - raise ParseError(f"invalid list of hosts for endpoint `{uri}`") + raise ParseUriError(f"invalid list of hosts for endpoint `{uri}`") hosts = hostname[1:-1].split(",") path = unquote(result.path or "") @@ -249,11 +251,11 @@ def from_uri(cls, uri: str) -> "_UriData": for key, values in parse_qs(result.query, strict_parsing=True).items() } except ValueError: - raise ParseError(f"invalid options for endpoint `{uri}`") + raise ParseUriError(f"invalid options for endpoint `{uri}`") try: return _UriData(scheme=scheme, user=user, hosts=hosts, path=path, options=options) - except InterfacesError as e: - raise ParseError(*e.args) + except FilesystemInfoError as e: + raise ParseUriError(*e.args) def __str__(self) -> str: user = quote(self.user) @@ -268,14 +270,14 @@ def _hostinfo(host: str) -> tuple[str, Optional[int]]: """Parse a host string into the hostname and the port.""" _logger.debug(f"_hostinfo: parsing `{host}`") if len(host) == 0: - raise ParseError("invalid empty host") + raise ParseUriError("invalid empty host") pos = 0 if host[pos] == "[": # IPv6 pos = host.find("]", pos) if pos == -1: - raise ParseError("unclosed bracket for host") + raise ParseUriError("unclosed bracket for host") hostname = host[1:pos] pos = pos + 1 else: @@ -291,19 +293,19 @@ def _hostinfo(host: str) -> tuple[str, Optional[int]]: # more characters after the hostname <==> port if host[pos] != ":": - raise ParseError("expected `:` after IPv6 address") + raise ParseUriError("expected `:` after IPv6 address") try: port = int(host[pos + 1 :]) except ValueError: - raise ParseError("expected int after `:` in host") + raise ParseUriError("expected int after `:` in host") return hostname, port -T = TypeVar("T", bound="FsInfo") +T = TypeVar("T", bound="FilesystemInfo") -class FsInfo(ABC): +class FilesystemInfo(ABC): """Information to mount a filesystem. This is an abstract class that exposes a set of required methods. All filesystems that @@ -313,14 +315,14 @@ class FsInfo(ABC): @classmethod @abstractmethod def from_uri(cls: type[T], uri: str, model: Model) -> T: - """Convert an URI string into a `FsInfo` object.""" + """Convert an URI string into a `FilesystemInfo` object.""" @abstractmethod def to_uri(self, model: Model) -> str: - """Convert this `FsInfo` object into an URI string.""" + """Convert this `FilesystemInfo` object into an URI string.""" def grant(self, model: Model, relation: ops.Relation): - """Grant permissions for a certain relation to any secrets that this `FsInfo` has. + """Grant permissions for a certain relation to any secrets that this `FilesystemInfo` has. This is an optional method because not all filesystems will require secrets to be mounted on the client. @@ -328,12 +330,12 @@ def grant(self, model: Model, relation: ops.Relation): @classmethod @abstractmethod - def fs_type(cls) -> str: + def filesystem_type(cls) -> str: """Get the string identifier of this filesystem type.""" @dataclass(frozen=True) -class NfsInfo(FsInfo): +class NfsInfo(FilesystemInfo): """Information required to mount an NFS share.""" hostname: str @@ -347,13 +349,13 @@ class NfsInfo(FsInfo): @classmethod def from_uri(cls, uri: str, _model: Model) -> "NfsInfo": - """See :py:meth:`FsInfo.from_uri` for documentation on this method.""" + """See :py:meth:`FilesystemInfo.from_uri` for documentation on this method.""" _logger.debug(f"NfsInfo.from_uri: parsing `{uri}`") info = _UriData.from_uri(uri) - if info.scheme != cls.fs_type(): - raise ParseError( + if info.scheme != cls.filesystem_type(): + raise ParseUriError( "could not parse `EndpointInfo` with incompatible scheme into `NfsInfo`" ) @@ -372,7 +374,7 @@ def from_uri(cls, uri: str, _model: Model) -> "NfsInfo": return NfsInfo(hostname=hostname, port=port, path=path) def to_uri(self, _model: Model) -> str: - """See :py:meth:`FsInfo.to_uri` for documentation on this method.""" + """See :py:meth:`FilesystemInfo.to_uri` for documentation on this method.""" try: IPv6Address(self.hostname) host = f"[{self.hostname}]" @@ -381,16 +383,16 @@ def to_uri(self, _model: Model) -> str: hosts = [host + f":{self.port}" if self.port else ""] - return str(_UriData(scheme=self.fs_type(), hosts=hosts, path=self.path)) + return str(_UriData(scheme=self.filesystem_type(), hosts=hosts, path=self.path)) @classmethod - def fs_type(cls) -> str: - """See :py:meth:`FsInfo.fs_type` for documentation on this method.""" + def filesystem_type(cls) -> str: + """See :py:meth:`FilesystemInfo.fs_type` for documentation on this method.""" return "nfs" @dataclass(frozen=True) -class CephfsInfo(FsInfo): +class CephfsInfo(FilesystemInfo): """Information required to mount a CephFS share.""" fsid: str @@ -413,35 +415,33 @@ class CephfsInfo(FsInfo): @classmethod def from_uri(cls, uri: str, model: Model) -> "CephfsInfo": - """See :py:meth:`FsInfo.from_uri` for documentation on this method.""" + """See :py:meth:`FilesystemInfo.from_uri` for documentation on this method.""" _logger.debug(f"CephfsInfo.from_uri: parsing `{uri}`") info = _UriData.from_uri(uri) - if info.scheme != cls.fs_type(): - raise ParseError( - "could not parse `EndpointInfo` with incompatible scheme into `CephfsInfo`" - ) + if info.scheme != cls.filesystem_type(): + raise ParseUriError("could not parse uri with incompatible scheme into `CephfsInfo`") path = info.path if not (user := info.user): - raise ParseError("missing user in uri for `CephfsInfo") + raise ParseUriError("missing user in uri for `CephfsInfo") if not (name := info.options.get("name")): - raise ParseError("missing name in uri for `CephfsInfo`") + raise ParseUriError("missing name in uri for `CephfsInfo`") if not (fsid := info.options.get("fsid")): - raise ParseError("missing fsid in uri for `CephfsInfo`") + raise ParseUriError("missing fsid in uri for `CephfsInfo`") monitor_hosts = info.hosts if not (auth := info.options.get("auth")): - raise ParseError("missing auth info in uri for `CephsInfo`") + raise ParseUriError("missing auth info in uri for `CephsInfo`") try: kind, data = auth.split(":", 1) except ValueError: - raise ParseError("Could not get the kind of auth info") + raise ParseUriError("Could not get the kind of auth info") if kind == "secret": key = model.get_secret(id=auth).get_content(refresh=True)["key"] @@ -450,14 +450,14 @@ def from_uri(cls, uri: str, model: Model) -> "CephfsInfo": # they don't support secrets. key = data else: - raise ParseError(f"Invalid kind `{kind}` for auth info") + raise ParseUriError(f"Invalid kind `{kind}` for auth info") return CephfsInfo( fsid=fsid, name=name, path=path, monitor_hosts=monitor_hosts, user=user, key=key ) def to_uri(self, model: Model) -> str: - """See :py:meth:`FsInfo.to_uri` for documentation on this method.""" + """See :py:meth:`FilesystemInfo.to_uri` for documentation on this method.""" secret = self._get_or_create_auth_secret(model) options = { @@ -469,7 +469,7 @@ def to_uri(self, model: Model) -> str: return str( _UriData( - scheme=self.fs_type(), + scheme=self.filesystem_type(), hosts=self.monitor_hosts, path=self.path, user=self.user, @@ -478,14 +478,14 @@ def to_uri(self, model: Model) -> str: ) def grant(self, model: Model, relation: Relation): - """See :py:meth:`FsInfo.grant` for documentation on this method.""" + """See :py:meth:`FilesystemInfo.grant` for documentation on this method.""" secret = self._get_or_create_auth_secret(model) secret.grant(relation) @classmethod - def fs_type(cls) -> str: - """See :py:meth:`FsInfo.fs_type` for documentation on this method.""" + def filesystem_type(cls) -> str: + """See :py:meth:`FilesystemInfo.fs_type` for documentation on this method.""" return "cephfs" def _get_or_create_auth_secret(self, model: Model) -> ops.Secret: @@ -505,25 +505,28 @@ def _get_or_create_auth_secret(self, model: Model) -> ops.Secret: class Endpoint: """Endpoint data exposed by a filesystem server.""" - fs_info: FsInfo + info: FilesystemInfo """Filesystem information required to mount this endpoint.""" uri: str """Raw URI exposed by this endpoint.""" + # Right now this is unused on the client, but having the raw uri + # available was useful on a previous version of the charm, so leaving + # this exposed just in case we need it in the future. -def _uri_to_fs_info(uri: str, model: Model) -> FsInfo: +def _uri_to_fs_info(uri: str, model: Model) -> FilesystemInfo: scheme = uri.split("://", maxsplit=1)[0] - if scheme == NfsInfo.fs_type(): + if scheme == NfsInfo.filesystem_type(): return NfsInfo.from_uri(uri, model) - elif scheme == CephfsInfo.fs_type(): + elif scheme == CephfsInfo.filesystem_type(): return CephfsInfo.from_uri(uri, model) else: - raise InterfacesError(f"unsupported filesystem type `{scheme}`") + raise FilesystemInfoError(f"unsupported filesystem type `{scheme}`") -class _MountEvent(RelationEvent): - """Base event for mount-related events.""" +class FilesystemEvent(RelationEvent): + """Base event for filesystem-related events.""" @property def endpoint(self) -> Optional[Endpoint]: @@ -533,19 +536,19 @@ def endpoint(self) -> Optional[Endpoint]: return Endpoint(_uri_to_fs_info(uri, self.framework.model), uri) -class MountFsEvent(_MountEvent): +class MountFilesystemEvent(FilesystemEvent): """Emit when a filesystem is ready to be mounted.""" -class UmountFsEvent(_MountEvent): +class UmountFilesystemEvent(FilesystemEvent): """Emit when a filesystem needs to be unmounted.""" -class _FsRequiresEvents(CharmEvents): +class FilesystemRequiresEvents(CharmEvents): """Events that FS servers can emit.""" - mount_fs = EventSource(MountFsEvent) - umount_fs = EventSource(UmountFsEvent) + mount_filesystem = EventSource(MountFilesystemEvent) + umount_filesystem = EventSource(UmountFilesystemEvent) class _BaseInterface(Object): @@ -572,10 +575,10 @@ def relations(self) -> List[Relation]: return result -class FsRequires(_BaseInterface): +class FilesystemRequires(_BaseInterface): """Consumer-side interface of filesystem integrations.""" - on = _FsRequiresEvents() + on = FilesystemRequiresEvents() def __init__(self, charm: CharmBase, relation_name: str) -> None: super().__init__(charm, relation_name) @@ -587,12 +590,12 @@ def __init__(self, charm: CharmBase, relation_name: str) -> None: def _on_relation_changed(self, event: RelationChangedEvent) -> None: """Handle when the databag between client and server has been updated.""" _logger.debug("Emitting `MountShare` event from `RelationChanged` hook") - self.on.mount_fs.emit(event.relation, app=event.app, unit=event.unit) + self.on.mount_filesystem.emit(event.relation, app=event.app, unit=event.unit) def _on_relation_departed(self, event: RelationDepartedEvent) -> None: """Handle when server departs integration.""" _logger.debug("Emitting `UmountShare` event from `RelationDeparted` hook") - self.on.umount_fs.emit(event.relation, app=event.app, unit=event.unit) + self.on.umount_filesystem.emit(event.relation, app=event.app, unit=event.unit) @property def endpoints(self) -> List[Endpoint]: @@ -601,11 +604,11 @@ def endpoints(self) -> List[Endpoint]: for relation in self.relations: if not (uri := relation.data[relation.app].get("endpoint")): continue - result.append(Endpoint(fs_info=_uri_to_fs_info(uri, self.model), uri=uri)) + result.append(Endpoint(info=_uri_to_fs_info(uri, self.model), uri=uri)) return result -class FsProvides(_BaseInterface): +class FilesystemProvides(_BaseInterface): """Provider-side interface of filesystem integrations.""" def __init__(self, charm: CharmBase, relation_name: str, peer_relation_name: str) -> None: @@ -613,11 +616,11 @@ def __init__(self, charm: CharmBase, relation_name: str, peer_relation_name: str self._peer_relation_name = peer_relation_name self.framework.observe(charm.on[relation_name].relation_joined, self._update_relation) - def set_fs_info(self, fs_info: FsInfo) -> None: + def set_info(self, info: FilesystemInfo) -> None: """Set information to mount a filesystem. Args: - fs_info: Information required to mount the filesystem. + info: Information required to mount the filesystem. Notes: Only the application leader unit can set the filesystem data. @@ -625,12 +628,12 @@ def set_fs_info(self, fs_info: FsInfo) -> None: if not self.unit.is_leader(): return - uri = fs_info.to_uri(self.model) + uri = info.to_uri(self.model) self._endpoint = uri for relation in self.relations: - fs_info.grant(self.model, relation) + info.grant(self.model, relation) relation.data[self.app]["endpoint"] = uri def _update_relation(self, event: RelationJoinedEvent) -> None: @@ -666,7 +669,7 @@ def _get_state(self, key: str) -> Optional[str]: def _set_state(self, key: str, data: str) -> None: """Insert a value into the global state.""" if not self._peers: - raise InterfacesError( + raise FilesystemInfoError( "Peer relation can only be accessed after the relation is established" ) diff --git a/tests/integration/server/requirements.txt b/tests/integration/server/requirements.txt index dcb03f2..5a9cac8 100644 --- a/tests/integration/server/requirements.txt +++ b/tests/integration/server/requirements.txt @@ -1 +1,2 @@ -ops ~= 2.8 \ No newline at end of file +ops ~= 2.8 +tenacity ~= 9.0.0 diff --git a/tests/integration/server/src/charm.py b/tests/integration/server/src/charm.py index fb12494..6a7ce24 100755 --- a/tests/integration/server/src/charm.py +++ b/tests/integration/server/src/charm.py @@ -4,33 +4,43 @@ """Charm the application.""" +import json import logging +import os +import socket +import subprocess +import textwrap +from pathlib import Path import ops -from charms.filesystem_client.v0.interfaces import CephfsInfo, FsProvides, NfsInfo +from charms.filesystem_client.v0.filesystem_info import CephfsInfo, FilesystemProvides, NfsInfo +from tenacity import retry, stop_after_attempt, wait_exponential _logger = logging.getLogger(__name__) -NFS_INFO = NfsInfo(hostname="192.168.1.254", path="/srv", port=65535) -CEPHFS_INFO = CephfsInfo( - fsid="123456789-0abc-defg-hijk-lmnopqrstuvw", - name="filesystem", - path="/export", - monitor_hosts=[ - "192.168.1.1:6789", - "192.168.1.2:6789", - "192.168.1.3:6789", - ], - user="user", - key="R//appdqz4NP4Bxcc5XWrg==", -) +def _exec_commands(cmds: [[str]]) -> None: + for cmd in cmds: + _exec_command(cmd) + + +def _exec_command(cmd: [str]) -> str: + _logger.info(f"Executing `{' '.join(cmd)}`") + env = os.environ.copy() + env["DEBIAN_FRONTEND"] = "noninteractive" + try: + output = subprocess.check_output(cmd, text=True, env=env) + except subprocess.CalledProcessError as e: + _logger.error(e.output) + raise Exception(f"Failed to execute command `{' '.join(cmd)}` in instance") + _logger.info(output) + return output class FilesystemServerCharm(ops.CharmBase): def __init__(self, framework: ops.Framework): super().__init__(framework) - self._fs_share = FsProvides(self, "fs-share", "server-peers") + self._filesystem = FilesystemProvides(self, "filesystem", "server-peers") framework.observe(self.on.start, self._on_start) def _on_start(self, event: ops.StartEvent): @@ -38,16 +48,122 @@ def _on_start(self, event: ops.StartEvent): _logger.info(self.config.get("type")) typ = self.config["type"] if "nfs" == typ: - info = NFS_INFO + info = self._deploy_nfs() elif "cephfs" == typ: - info = CEPHFS_INFO + info = self._deploy_cephfs() else: raise ValueError("invalid filesystem type") - self._fs_share.set_fs_info(info) - _logger.info("set info") + self._filesystem.set_info(info) + self.unit.status = ops.ActiveStatus() - _logger.info("transitioned to active") + + def _deploy_nfs(self) -> NfsInfo: + _exec_commands( + [ + ["apt", "update", "-y"], + ["apt", "upgrade", "-y"], + ["apt", "install", "-y", "nfs-kernel-server"], + ] + ) + + exports = textwrap.dedent( + """ + /srv *(ro,sync,subtree_check) + /data *(rw,sync,no_subtree_check,no_root_squash) + """ + ).strip("\n") + _logger.info(f"Uploading the following /etc/exports file:\n{exports}") + Path("/etc/exports").write_text(exports) + _logger.info("starting NFS server") + Path("/data").mkdir() + + _exec_commands([["exportfs", "-a"], ["systemctl", "restart", "nfs-kernel-server"]]) + + for i in range(3): + Path(f"/data/test-{i}").touch() + + self.unit.set_ports(111, 2049, ops.Port("udp", 111), ops.Port("udp", 2049)) + public_ip = socket.gethostbyname(socket.gethostname()) + + return NfsInfo(hostname=public_ip, port=2049, path="/data") + + def _deploy_cephfs(self) -> CephfsInfo: + fs_name = "filesystem" + fs_user = "fs-client" + fs_path = "/" + + @retry(wait=wait_exponential(max=6), stop=stop_after_attempt(20)) + def _mount_cephfs() -> None: + # Need to extract this into its own function to apply the tenacity decorator + # Wait until the cluster is ready to mount the filesystem. + status = json.loads(_exec_command(["microceph.ceph", "-s", "-f", "json"])) + if status["health"]["status"] != "HEALTH_OK": + raise Exception("CephFS is not available") + + _exec_command(["mount", "-t", "ceph", f"admin@.{fs_name}={fs_path}", "/mnt"]) + + _exec_commands( + [ + ["ln", "-s", "/bin/true"], + ["apt", "update", "-y"], + ["apt", "upgrade", "-y"], + ["apt", "install", "-y", "ceph-common"], + ["snap", "install", "microceph"], + ["microceph", "cluster", "bootstrap"], + ["microceph", "disk", "add", "loop,2G,3"], + ["microceph.ceph", "osd", "pool", "create", f"{fs_name}_data"], + ["microceph.ceph", "osd", "pool", "create", f"{fs_name}_metadata"], + ["microceph.ceph", "fs", "new", fs_name, f"{fs_name}_metadata", f"{fs_name}_data"], + [ + "microceph.ceph", + "fs", + "authorize", + fs_name, + f"client.{fs_user}", + fs_path, + "rw", + ], + # Need to generate the test files inside microceph itself. + [ + "ln", + "-sf", + "/var/snap/microceph/current/conf/ceph.client.admin.keyring", + "/etc/ceph/ceph.client.admin.keyring", + ], + [ + "ln", + "-sf", + "/var/snap/microceph/current/conf/ceph.keyring", + "/etc/ceph/ceph.keyring", + ], + ["ln", "-sf", "/var/snap/microceph/current/conf/ceph.conf", "/etc/ceph/ceph.conf"], + ] + ) + + _mount_cephfs() + + for i in range(3): + Path(f"/mnt/test-{i}").touch() + + self.unit.set_ports(3300, 6789) + + status = json.loads(_exec_command(["microceph.ceph", "-s", "-f", "json"])) + fsid = status["fsid"] + key = _exec_command(["microceph.ceph", "auth", "print-key", f"client.{fs_user}"]) + hostname = socket.gethostbyname(socket.gethostname()) + + return CephfsInfo( + fsid=fsid, + name=fs_name, + path=fs_path, + monitor_hosts=[ + hostname + ":3300", + hostname + ":6789", + ], + user=fs_user, + key=key, + ) if __name__ == "__main__": # pragma: nocover diff --git a/tests/integration/test_charm.py b/tests/integration/test_charm.py index f71712c..d296b2b 100644 --- a/tests/integration/test_charm.py +++ b/tests/integration/test_charm.py @@ -9,7 +9,7 @@ import juju import pytest import yaml -from charms.filesystem_client.v0.interfaces import CephfsInfo, NfsInfo +from charms.filesystem_client.v0.filesystem_info import CephfsInfo, NfsInfo from pytest_operator.plugin import OpsTest logger = logging.getLogger(__name__) @@ -86,6 +86,7 @@ async def test_build_and_deploy(ops_test: OpsTest): config={ "type": "cephfs", }, + constraints=juju.constraints.parse("virt-type=virtual-machine root-disk=50G mem=8G"), ), ops_test.model.wait_for_idle( apps=["nfs-server", "cephfs-server", "ubuntu"], @@ -100,59 +101,27 @@ async def test_build_and_deploy(ops_test: OpsTest): @pytest.mark.order(2) async def test_integrate(ops_test: OpsTest): await ops_test.model.integrate(f"{APP_NAME}:juju-info", "ubuntu:juju-info") - await ops_test.model.integrate(f"{APP_NAME}:fs-share", "nfs-server:fs-share") - await ops_test.model.integrate(f"{APP_NAME}:fs-share", "cephfs-server:fs-share") + await ops_test.model.integrate(f"{APP_NAME}:filesystem", "nfs-server:filesystem") + await ops_test.model.integrate(f"{APP_NAME}:filesystem", "cephfs-server:filesystem") await ops_test.model.wait_for_idle( apps=[APP_NAME, "ubuntu", "nfs-server", "cephfs-server"], status="active", timeout=1000 ) -async def check_autofs( - ops_test: OpsTest, - master_file: str, - autofs_file: str, - mountpoint: str, - opts: [str], - endpoint: str, -): - unit = ops_test.model.applications["ubuntu"].units[0] - master = (await unit.ssh(f"cat {master_file}")).split() - mount = (await unit.ssh(f"cat {autofs_file}")).split() - - assert autofs_file == master[1] - assert mountpoint == mount[0] - assert set(opts) == set(mount[1].removeprefix("-").split(",")) - assert endpoint == mount[2] - - @pytest.mark.order(3) async def test_nfs_files(ops_test: OpsTest): - await check_autofs( - ops_test=ops_test, - master_file="/etc/auto.master.d/nfs.autofs", - autofs_file="/etc/auto.nfs", - mountpoint="/nfs", - opts=["exec", "suid", "nodev", "ro", f"port={NFS_INFO.port}"], - endpoint=f"{NFS_INFO.hostname}:{NFS_INFO.path}", - ) + unit = ops_test.model.applications["ubuntu"].units[0] + result = (await unit.ssh("ls /nfs")).strip("\n") + assert "test-0" in result + assert "test-1" in result + assert "test-2" in result @pytest.mark.order(4) async def test_cephfs_files(ops_test: OpsTest): - await check_autofs( - ops_test=ops_test, - master_file="/etc/auto.master.d/cephfs.autofs", - autofs_file="/etc/auto.cephfs", - mountpoint="/cephfs", - opts=[ - "noexec", - "nosuid", - "dev", - "rw", - "fstype=ceph", - f"mon_addr={"/".join(CEPHFS_INFO.monitor_hosts)}", - f"secret={CEPHFS_INFO.key}", - ], - endpoint=f"{CEPHFS_INFO.user}@{CEPHFS_INFO.fsid}.{CEPHFS_INFO.name}={CEPHFS_INFO.path}", - ) + unit = ops_test.model.applications["ubuntu"].units[0] + result = (await unit.ssh("ls /cephfs")).strip("\n") + assert "test-0" in result + assert "test-1" in result + assert "test-2" in result diff --git a/tests/unit/test_interfaces.py b/tests/unit/test_interfaces.py index cf30162..077f3a0 100644 --- a/tests/unit/test_interfaces.py +++ b/tests/unit/test_interfaces.py @@ -5,14 +5,14 @@ """Test the interfaces charm library.""" import pytest -from charms.filesystem_client.v0.interfaces import ( - FsRequires, +from charms.filesystem_client.v0.filesystem_info import ( + FilesystemRequires, _hostinfo, ) from ops import CharmBase -FS_INTEGRATION_NAME = "fs-share" -FS_INTEGRATION_INTERFACE = "fs_share" +FS_INTEGRATION_NAME = "filesystem" +FS_INTEGRATION_INTERFACE = "filesystem_info" FS_CLIENT_METADATA = f""" name: fs-client requires: @@ -33,7 +33,7 @@ class FsClientCharm(CharmBase): def __init__(self, *args, **kwargs) -> None: super().__init__(*args, **kwargs) - self.requirer = FsRequires(self, FS_INTEGRATION_NAME) + self.requirer = FilesystemRequires(self, FS_INTEGRATION_NAME) self.framework.observe(self.requirer.on.mount_fs, lambda *_: None) self.framework.observe(self.requirer.on.umount_fs, lambda *_: None) diff --git a/uv.lock b/uv.lock index ce79325..d065cad 100644 --- a/uv.lock +++ b/uv.lock @@ -12,11 +12,11 @@ wheels = [ [[package]] name = "attrs" -version = "24.2.0" +version = "24.3.0" source = { registry = "https://pypi.org/simple" } -sdist = { url = "https://files.pythonhosted.org/packages/fc/0f/aafca9af9315aee06a89ffde799a10a582fe8de76c563ee80bbcdc08b3fb/attrs-24.2.0.tar.gz", hash = "sha256:5cfb1b9148b5b086569baec03f20d7b6bf3bcacc9a42bebf87ffaaca362f6346", size = 792678 } +sdist = { url = "https://files.pythonhosted.org/packages/48/c8/6260f8ccc11f0917360fc0da435c5c9c7504e3db174d5a12a1494887b045/attrs-24.3.0.tar.gz", hash = "sha256:8f5c07333d543103541ba7be0e2ce16eeee8130cb0b3f9238ab904ce1e85baff", size = 805984 } wheels = [ - { url = "https://files.pythonhosted.org/packages/6a/21/5b6702a7f963e95456c0de2d495f67bf5fd62840ac655dc451586d23d39a/attrs-24.2.0-py3-none-any.whl", hash = "sha256:81921eb96de3191c8258c199618104dd27ac608d9366f5e35d011eae1867ede2", size = 63001 }, + { url = "https://files.pythonhosted.org/packages/89/aa/ab0f7891a01eeb2d2e338ae8fecbe57fcebea1a24dbb64d45801bfab481d/attrs-24.3.0-py3-none-any.whl", hash = "sha256:ac96cd038792094f438ad1f6ff80837353805ac950cd2aa0e0625ef19850c308", size = 63397 }, ] [[package]] @@ -60,11 +60,11 @@ wheels = [ [[package]] name = "certifi" -version = "2024.8.30" +version = "2024.12.14" source = { registry = "https://pypi.org/simple" } -sdist = { url = "https://files.pythonhosted.org/packages/b0/ee/9b19140fe824b367c04c5e1b369942dd754c4c5462d5674002f75c4dedc1/certifi-2024.8.30.tar.gz", hash = "sha256:bec941d2aa8195e248a60b31ff9f0558284cf01a52591ceda73ea9afffd69fd9", size = 168507 } +sdist = { url = "https://files.pythonhosted.org/packages/0f/bd/1d41ee578ce09523c81a15426705dd20969f5abf006d1afe8aeff0dd776a/certifi-2024.12.14.tar.gz", hash = "sha256:b650d30f370c2b724812bee08008be0c4163b163ddaec3f2546c1caf65f191db", size = 166010 } wheels = [ - { url = "https://files.pythonhosted.org/packages/12/90/3c9ff0512038035f59d279fddeb79f5f1eccd8859f06d6163c58798b9487/certifi-2024.8.30-py3-none-any.whl", hash = "sha256:922820b53db7a7257ffbda3f597266d435245903d80737e34f8a45ff3e3230d8", size = 167321 }, + { url = "https://files.pythonhosted.org/packages/a5/32/8f6669fc4798494966bf446c8c4a162e0b5d893dff088afddf76414f70e1/certifi-2024.12.14-py3-none-any.whl", hash = "sha256:1275f7a45be9464efc1173084eaa30f866fe2e47d389406136d332ed4967ec56", size = 164927 }, ] [[package]]