Skip to content

Commit

Permalink
Merge pull request #1155 from dimaqq/chore-refactor-facade-versions
Browse files Browse the repository at this point in the history 
#1155

#### Description

Streamlining facades code:
- removing intermediate "versions" key from:
 - static facade version declaration
 - specified facades argument
- deprecating the `specified_facades` argument as it's
 - not covered by unit or integration tests
 - not used by any library user
  • Loading branch information
@jujubot
jujubot authored Oct 22, 2024
2 parents 60821da + dfeb7fc commit c58ec9d
Show file tree
Hide file tree
Showing 5 changed files with 190 additions and 101 deletions.
112 changes: 33 additions & 79 deletions juju/client/connection.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,11 @@
import logging
import ssl
import urllib.request
import warnings
import weakref
from http.client import HTTPSConnection
from dateutil.parser import parse
from typing import Dict, List
from typing import Dict, Literal, Optional, Sequence

import macaroonbakery.bakery as bakery
import macaroonbakery.httpbakery as httpbakery
Expand All @@ -18,53 +19,10 @@
from juju.client import client
from juju.utils import IdQueue
from juju.version import CLIENT_VERSION
from .facade_versions import client_facade_versions, known_unsupported_facades

log = logging.getLogger('juju.client.connection')

# Manual list of facades present in schemas + codegen which python-libjuju does not yet support
excluded_facades: Dict[str, List[int]] = {
'Charms': [7],
}
# Please keep in alphabetical order
# in future this will likely be generated automatically (perhaps at runtime)
client_facades = {
'Action': {'versions': [7]},
'Admin': {'versions': [3]},
'AllModelWatcher': {'versions': [4]},
'AllWatcher': {'versions': [3]},
'Annotations': {'versions': [2]},
'Application': {'versions': [17, 19]},
'ApplicationOffers': {'versions': [4]},
'Backups': {'versions': [3]},
'Block': {'versions': [2]},
'Bundle': {'versions': [6]},
'Charms': {'versions': [6]},
'Client': {'versions': [6, 7]},
'Cloud': {'versions': [7]},
'Controller': {'versions': [11]},
'CredentialManager': {'versions': [1]},
'FirewallRules': {'versions': [1]},
'HighAvailability': {'versions': [2]},
'ImageMetadataManager': {'versions': [1]},
'KeyManager': {'versions': [1]},
'MachineManager': {'versions': [10]},
'MetricsDebug': {'versions': [2]},
'ModelConfig': {'versions': [3]},
'ModelGeneration': {'versions': [4]},
'ModelManager': {'versions': [9]},
'ModelUpgrader': {'versions': [1]},
'Payloads': {'versions': [1]},
'Pinger': {'versions': [1]},
'Resources': {'versions': [3]},
'SSHClient': {'versions': [4]},
'SecretBackends': {'versions': [1]},
'Secrets': {'versions': [1, 2]},
'Spaces': {'versions': [6]},
'Storage': {'versions': [6]},
'Subnets': {'versions': [5]},
'UserManager': {'versions': [3]},
}


def facade_versions(name, versions):
"""
Expand Down Expand Up @@ -156,6 +114,8 @@ class Connection:

MAX_FRAME_SIZE = 2**22
"Maximum size for a single frame. Defaults to 4MB."
facades: Dict[str, int]
_specified_facades: Dict[str, Sequence[int]]

@classmethod
async def connect(
Expand All @@ -169,7 +129,7 @@ async def connect(
max_frame_size=None,
retries=3,
retry_backoff=10,
specified_facades=None,
specified_facades: Optional[Dict[str, Dict[Literal["versions"], Sequence[int]]]] = None,
proxy=None,
debug_log_conn=None,
debug_log_params={}
Expand Down Expand Up @@ -197,7 +157,7 @@ async def connect(
:param int retry_backoff: Number of seconds to increase the wait
between connection retry attempts (a backoff of 10 with 3 retries
would wait 10s, 20s, and 30s).
:param specified_facades: Define a series of facade versions you wish to override
:param specified_facades: (deprecated) define a series of facade versions you wish to override
to prevent using the conservative client pinning with in the client.
:param TextIOWrapper debug_log_conn: target if this is a debug log connection
:param dict debug_log_params: filtering parameters for the debug-log output
Expand Down Expand Up @@ -247,7 +207,18 @@ async def connect(
self._retry_backoff = retry_backoff

self.facades = {}
self.specified_facades = specified_facades or {}

if specified_facades:
warnings.warn(
"The `specified_facades` argument is deprecated and will be removed soon",
DeprecationWarning,
stacklevel=3,
)
self._specified_facades = {
name: d["versions"] for name, d in specified_facades.items()
}
else:
self._specified_facades = {}

self.messages = IdQueue()
self.monitor = Monitor(connection=self)
Expand Down Expand Up @@ -826,48 +797,31 @@ async def _connect_with_redirect(self, endpoints):
self._pinger_task = jasyncio.create_task(self._pinger(), name="Task_Pinger")

# _build_facades takes the facade list that comes from the connection with the controller,
# validates that the client knows about them (client_facades) and builds the facade list
# (into the self.specified facades) with the max versions that both the client and the controller
# validates that the client knows about them (client_facade_versions) and builds the facade list
# (into the self._specified facades) with the max versions that both the client and the controller
# can negotiate on
def _build_facades(self, facades_from_connection):
self.facades.clear()
for facade in facades_from_connection:
name = facade['name']
# the following attempts to get the best facade version for the
# client. The client knows about the best facade versions it speaks,
# so in order to be compatible forwards and backwards we speak a
# common facade versions.
if (name not in client_facades) and (name not in self.specified_facades):
# if a facade is required but the client doesn't know about
# it, then log a warning.
if name in self._specified_facades:
client_versions = self._specified_facades[name]
elif name in client_facade_versions:
client_versions = client_facade_versions[name]
elif name in known_unsupported_facades:
continue
else:
log.warning(f'unexpected facade {name} received from the controller')
continue

try:
# allow the ability to specify a set of facade versions, so the
# client can define the non-conservative facade client pinning.
if name in self.specified_facades:
client_versions = self.specified_facades[name]['versions']
elif name in client_facades:
client_versions = client_facades[name]['versions']

controller_versions = facade['versions']
# select the max version that both the client and the controller know
version = max(set(client_versions).intersection(set(controller_versions)))
except ValueError:
# this can occur if client_verisons is [1, 2] and controller_versions is [3, 4]
# there is just no way to know how to communicate with the facades we're trying to call.
controller_versions = facade['versions']
candidates = set(client_versions) & set(controller_versions)
if not candidates:
log.warning(f'unknown common facade version for {name},\n'
f'versions known to client : {client_versions}\n'
f'versions known to controller : {controller_versions}')
except errors.JujuConnectionError:
# If the facade isn't with in the local facades then it's not
# possible to reason about what version should be used. In this
# case we should log the facade was found, but we couldn't
# handle it.
log.warning(f'unexpected facade {name} found, unable to determine which version to use')
else:
self.facades[name] = version
continue
self.facades[name] = max(candidates)

async def login(self):
params = {}
Expand Down
141 changes: 141 additions & 0 deletions juju/client/facade_versions.py
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,141 @@
# Copyright 2024 Canonical Ltd.
# Licensed under the Apache V2, see LICENCE file for details.
"""Constants for facade version negotation."""
from typing import Dict, Sequence


# Please keep in alphabetical order
# in future this will likely be generated automatically (perhaps at runtime)
client_facade_versions = {
'Action': (7, ),
'Admin': (3, ),
'AllModelWatcher': (4, ),
'AllWatcher': (3, ),
'Annotations': (2, ),
'Application': (17, 19),
'ApplicationOffers': (4, ),
'Backups': (3, ),
'Block': (2, ),
'Bundle': (6, ),
'Charms': (6, ),
'Client': (6, 7),
'Cloud': (7, ),
'Controller': (11, ),
'CredentialManager': (1, ),
'FirewallRules': (1, ),
'HighAvailability': (2, ),
'ImageMetadataManager': (1, ),
'KeyManager': (1, ),
'MachineManager': (10, ),
'MetricsDebug': (2, ),
'ModelConfig': (3, ),
'ModelGeneration': (4, ),
'ModelManager': (9, ),
'ModelUpgrader': (1, ),
'Payloads': (1, ),
'Pinger': (1, ),
'Resources': (3, ),
'SSHClient': (4, ),
'SecretBackends': (1, ),
'Secrets': (1, 2),
'Spaces': (6, ),
'Storage': (6, ),
'Subnets': (5, ),
'UserManager': (3, ),
}

# Manual list of facades present in schemas + codegen which python-libjuju does not yet support
excluded_facade_versions: Dict[str, Sequence[int]] = {
'Charms': (7, )
}


# We don't generate code for these, as we can never use them.
# The controller happily lists them though, don't warn about these.
known_unsupported_facades = (
'ActionPruner',
'Agent',
'AgentLifeFlag',
'AgentTools',
'ApplicationScaler',
'CAASAdmission',
'CAASAgent',
'CAASApplication',
'CAASApplicationProvisioner',
'CAASFirewaller',
'CAASFirewallerSidecar',
'CAASModelConfigManager',
'CAASModelOperator',
'CAASOperator',
'CAASOperatorProvisioner',
'CAASOperatorUpgrader',
'CAASUnitProvisioner',
'CharmDownloader',
'CharmRevisionUpdater',
'Cleaner',
'CredentialValidator',
'CrossController',
'CrossModelRelations',
'CrossModelSecrets',
'Deployer',
'DiskManager',
'EntityWatcher',
'EnvironUpgrader',
'ExternalControllerUpdater',
'FanConfigurer',
'FilesystemAttachmentsWatcher',
'Firewaller',
'HostKeyReporter',
'ImageMetadata',
'InstanceMutater',
'InstancePoller',
'KeyUpdater',
'LeadershipService',
'LifeFlag',
'LogForwarding',
'Logger',
'MachineActions',
'MachineUndertaker',
'Machiner',
'MeterStatus',
'MetricsAdder',
'MetricsManager',
'MigrationFlag',
'MigrationMaster',
'MigrationMinion',
'MigrationStatusWatcher',
'MigrationTarget',
'ModelSummaryWatcher',
'NotifyWatcher',
'OfferStatusWatcher',
'PayloadsHookContext',
'Provisioner',
'ProxyUpdater',
'Reboot',
'RelationStatusWatcher',
'RelationUnitsWatcher',
'RemoteRelationWatcher',
'RemoteRelations',
'ResourcesHookContext',
'RetryStrategy',
'SecretBackendsManager',
'SecretBackendsRotateWatcher',
'SecretsDrain',
'SecretsManager',
'SecretsRevisionWatcher',
'SecretsTriggerWatcher',
'Singular',
'StatusHistory',
'StorageProvisioner',
'StringsWatcher',
'Undertaker',
'UnitAssigner',
'Uniter',
'UpgradeSeries',
'UpgradeSteps',
'Upgrader',
'UserSecretsDrain',
'UserSecretsManager',
'VolumeAttachmentPlansWatcher',
'VolumeAttachmentsWatcher'
)
2 changes: 1 addition & 1 deletion juju/controller.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ async def connect(self, *args, **kwargs):
:param list macaroons: List of macaroons to load into the
``bakery_client``.
:param int max_frame_size: The maximum websocket frame size to allow.
:param specified_facades: Overwrite the facades with a series of
:param specified_facades: (deprecated) overwrite the facades with a series of
specified facades.
"""
await self.disconnect()
Expand Down
2 changes: 1 addition & 1 deletion juju/model.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -649,7 +649,7 @@ async def connect(self, *args, **kwargs):
:param list macaroons: List of macaroons to load into the
``bakery_client``.
:param int max_frame_size: The maximum websocket frame size to allow.
:param specified_facades: Overwrite the facades with a series of
:param specified_facades: (deprecated) overwrite the facades with a series of
specified facades.
"""
is_debug_log_conn = 'debug_log_conn' in kwargs
Expand Down
34 changes: 14 additions & 20 deletions tests/validate/test_facades.py → tests/validate/test_facade_versions.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,11 @@
import importlib
from collections import defaultdict
from pathlib import Path
from typing import Dict, List, TypedDict
from typing import Dict, List, Sequence

import pytest

from juju.client.connection import client_facades, excluded_facades


class Versions(TypedDict, total=True):
versions: List[int]


ClientFacades = Dict[str, Versions]
from juju.client.facade_versions import client_facade_versions, excluded_facade_versions, known_unsupported_facades


@pytest.fixture
Expand All @@ -24,8 +17,8 @@ def project_root(pytestconfig: pytest.Config) -> Path:


@pytest.fixture
def generated_code_facades(project_root: Path) -> ClientFacades:
"""Return a client_facades dictionary from generated code under project_root.
def generated_code_facades(project_root: Path) -> Dict[str, Sequence[int]]:
"""Return a {facade_name: (versions,)} dictionary from the generated code.
Iterates through all the generated files matching juju/client/_client*.py,
extracting facade types (those that have .name and .version properties).
Expand All @@ -42,19 +35,20 @@ def generated_code_facades(project_root: Path) -> ClientFacades:
cls.version
except AttributeError:
continue
if cls.version in excluded_facades.get(cls.name, []):
if cls.version in excluded_facade_versions.get(cls.name, []):
continue
facades[cls.name].append(cls.version)
return {name: {'versions': sorted(facades[name])} for name in sorted(facades)}
return {name: tuple(sorted(facades[name])) for name in sorted(facades)}


def test_client_facades(project_root: Path, generated_code_facades: ClientFacades) -> None:
"""Ensure that juju.client.connection.client_facades matches expected facades.
def test_client_facades(generated_code_facades: Dict[str, Sequence[int]]) -> None:
"""Ensure that juju.client.facade_versions.client_facade_versions matches expected facades.
See generated_code_facades for how expected facades are computed.
"""
assert {
k: v['versions'] for k, v in client_facades.items()
} == {
k: v['versions'] for k, v in generated_code_facades.items()
}
assert client_facade_versions == generated_code_facades


def test_unsupported_facades(generated_code_facades: Dict[str, Sequence[int]]) -> None:
"""Ensure that we don't accidentally ignore our own generated code."""
assert not set(generated_code_facades) & set(known_unsupported_facades)

0 comments on commit c58ec9d

Please sign in to comment.