diff --git a/simplyblock_core/controllers/backup_controller.py b/simplyblock_core/controllers/backup_controller.py index 83ee41ef8e..ef8b2fd1a6 100644 --- a/simplyblock_core/controllers/backup_controller.py +++ b/simplyblock_core/controllers/backup_controller.py @@ -17,7 +17,7 @@ lvol_dek_path, pool_kek_name, ) from simplyblock_core.utils.secrets import unwrap_secret -from simplyblock_core.exceptions import PreconditionError +from simplyblock_core.exceptions import InsufficientCapacityError, PreconditionError from simplyblock_core.rpc_client import RPCException logger = logging.getLogger() @@ -487,6 +487,8 @@ def restore_backup(backup_id: str, lvol_name: str, pool_id_or_name: str, fabric="tcp", ) if error or not lvol_id: + if lvol_controller.is_capacity_error(error): + raise InsufficientCapacityError(str(error)) raise RuntimeError(f"Failed to create restore volume: {error}") # Mark volume as restoring diff --git a/simplyblock_core/controllers/lvol_controller.py b/simplyblock_core/controllers/lvol_controller.py index be26f7b939..418557e3b6 100644 --- a/simplyblock_core/controllers/lvol_controller.py +++ b/simplyblock_core/controllers/lvol_controller.py @@ -30,6 +30,28 @@ logger = utils.get_logger(__name__) +#: The cluster-wide placement refusal: no node can hold another object. +ERR_NO_NODE_WITH_CAPACITY = "No nodes found with enough resources to create the LVol" + +#: The per-lvstore refusal, raised when a pinned node is already at its object cap. +#: A prefix rather than a whole message: the rest names the node and the counts. +ERR_OBJECT_LIMIT_PREFIX = "Object limit reached on lvstore" + + +def is_capacity_error(error) -> bool: + """True when ``error`` is one of the placement refusals above. + + Callers converting ``(id, error)`` returns into exceptions use this to tell a + cluster that has no room from one that genuinely failed. Both refusals are + matched through the constants they are built from, so a reworded message + cannot silently stop being recognized. + """ + if not error: + return False + error = str(error) + return ERR_NO_NODE_WITH_CAPACITY in error or ERR_OBJECT_LIMIT_PREFIX in error + + def _create_crypto_lvol(rpc_client, lvol, cluster): name = lvol.crypto_bdev base_name = f"{lvol.lvs_name}/{lvol.lvol_bdev}" @@ -404,7 +426,7 @@ def check_lvstore_object_limit(host_node, all_lvols, all_snaps, new_objects=1): if s.lvol and s.lvol.node_id == node_id and not s.deleted) total = lvol_count + snap_count if total + new_objects > limit: - return (f"Object limit reached on lvstore of node {node_id}: {total} " + return (f"{ERR_OBJECT_LIMIT_PREFIX} of node {node_id}: {total} " f"objects (lvols/clones: {lvol_count}, snapshots: " f"{snap_count}); the hard limit is {limit} per lvstore") return None @@ -628,7 +650,7 @@ def add_lvol_ha(name, size, host_id_or_name, ha_type, pool_id_or_name, use_comp= if not host_node: nodes = _get_next_3_nodes(cl.get_id(), lvol.size, all_lvols, namespaced=bool(namespaced)) if not nodes: - return False, "No nodes found with enough resources to create the LVol" + return False, ERR_NO_NODE_WITH_CAPACITY host_node = nodes[0] limit_error = check_lvstore_object_limit(host_node, all_lvols, all_snaps) @@ -4098,7 +4120,7 @@ def replicate_lvol_on_source_cluster(lvol_id, cluster_id=None, pool_uuid=None): # get new source node from the new cluster nodes = _get_next_3_nodes(new_source_cluster.get_id(), lvol.size) if not nodes: - return False, "No nodes found with enough resources to create the LVol" + return False, ERR_NO_NODE_WITH_CAPACITY source_node = nodes[0] if not source_node: diff --git a/simplyblock_core/exceptions.py b/simplyblock_core/exceptions.py index 5ec273492d..bed7ada903 100644 --- a/simplyblock_core/exceptions.py +++ b/simplyblock_core/exceptions.py @@ -4,3 +4,13 @@ class PreconditionError(Exception): class MigrationConflictError(Exception): """Raised when a conflicting active migration already exists.""" + + +class InsufficientCapacityError(Exception): + """Raised when the cluster has nowhere to place a new logical volume. + + A refusal, not a fault: the request is well-formed and the control plane is + healthy, but no node can hold another object until capacity is freed or a + node is added. Retrying an identical request cannot change the answer, so + this surfaces as a 409 rather than a 5xx. + """ diff --git a/simplyblock_web/api/v2/cluster/storage_pool/volume/__init__.py b/simplyblock_web/api/v2/cluster/storage_pool/volume/__init__.py index 10b6e9120e..46034c1b33 100644 --- a/simplyblock_web/api/v2/cluster/storage_pool/volume/__init__.py +++ b/simplyblock_web/api/v2/cluster/storage_pool/volume/__init__.py @@ -7,6 +7,7 @@ from simplyblock_core.db_controller import DBController from simplyblock_core import utils as core_utils from simplyblock_core.controllers import backup_controller, lvol_controller, snapshot_controller +from simplyblock_core.exceptions import InsufficientCapacityError from simplyblock_core.models.lvol_model import LVol from ...._dependencies import Cluster, StoragePool, Volume @@ -121,6 +122,8 @@ def add( raise AssertionError('unreachable') if volume_id_or_false == False: # noqa + if lvol_controller.is_capacity_error(error): + raise InsufficientCapacityError(str(error)) raise ValueError(error) return util.creation_response( diff --git a/simplyblock_web/app.py b/simplyblock_web/app.py index a96211ac39..de216718bf 100644 --- a/simplyblock_web/app.py +++ b/simplyblock_web/app.py @@ -18,7 +18,7 @@ from simplyblock_web.settings import Settings as WebSettings from simplyblock_core import constants, utils as core_utils from simplyblock_core.settings import Settings -from simplyblock_core.exceptions import PreconditionError +from simplyblock_core.exceptions import InsufficientCapacityError, PreconditionError logger = core_utils.get_logger(__name__) logger.setLevel(constants.LOG_WEB_LEVEL) @@ -78,22 +78,46 @@ async def dispatch(self, request: Request, call_next): app: FastAPI = FastAPI() Instrumentator().instrument(app).expose(app, endpoint="/_meta/metrics") -@app.exception_handler(PreconditionError) -async def precondition_handler(request: Request, exc: PreconditionError): - logger.exception("Preciondition checks failed", exc_info=exc) - return JSONResponse(status_code=400, content={ - "error": "Preconditions are not met", - "detail": str(exc), - }) - - -@app.exception_handler(RuntimeError) -async def runtime_error_handler(request: Request, exc: RuntimeError): - logger.exception("Unexcpected error while processing request", exc_info=exc) - return JSONResponse(status_code=500, content={ - "status": "An error occured while processing the request", - "detail": str(exc), - }) + +def register_exception_handlers(app: FastAPI) -> None: + """Map the core exception types onto their HTTP responses. + + Kept as a function so tests can build an app that answers the way the real + one does. An endpoint's status code is part of its contract, and a test app + without these handlers cannot observe it. + """ + + @app.exception_handler(PreconditionError) + async def precondition_handler(request: Request, exc: PreconditionError): + logger.exception("Precondition checks failed", exc_info=exc) + return JSONResponse(status_code=400, content={ + "error": "Preconditions are not met", + "detail": str(exc), + }) + + @app.exception_handler(InsufficientCapacityError) + async def insufficient_capacity_handler(request: Request, exc: InsufficientCapacityError): + # Deliberately 409 and not 507: 507 is the more precise name for the + # condition, but it is a 5xx, and generic retry policies retry those. A + # refusal that no retry can change has to land in the 4xx class to be + # read correctly by a client that has not special-cased it. + logger.warning("Cluster has no capacity for the requested volume: %s", exc) + return JSONResponse(status_code=409, content={ + "error": "Insufficient capacity", + "code": "insufficient_capacity", + "detail": str(exc), + }) + + @app.exception_handler(RuntimeError) + async def runtime_error_handler(request: Request, exc: RuntimeError): + logger.exception("Unexpected error while processing request", exc_info=exc) + return JSONResponse(status_code=500, content={ + "status": "An error occured while processing the request", + "detail": str(exc), + }) + + +register_exception_handlers(app) _web_settings = WebSettings() diff --git a/tests/unit/test_backup_restore_node_selection.py b/tests/unit/test_backup_restore_node_selection.py index fa089b8b78..140633f452 100644 --- a/tests/unit/test_backup_restore_node_selection.py +++ b/tests/unit/test_backup_restore_node_selection.py @@ -11,7 +11,8 @@ import pytest -from simplyblock_core.exceptions import PreconditionError +from simplyblock_core.controllers import lvol_controller +from simplyblock_core.exceptions import InsufficientCapacityError, PreconditionError from simplyblock_core.models.backup import Backup from simplyblock_core.models.storage_node import StorageNode @@ -194,6 +195,22 @@ def test_volume_creation_failure_is_a_runtime_error(self, db, add_lvol_ha): with pytest.raises(RuntimeError, match="Failed to create restore volume"): _restore() + @pytest.mark.parametrize("refusal", [ + lvol_controller.ERR_NO_NODE_WITH_CAPACITY, + f"{lvol_controller.ERR_OBJECT_LIMIT_PREFIX} of node abc: 35 objects " + f"(lvols/clones: 35, snapshots: 0); the hard limit is 35 per lvstore", + ]) + def test_capacity_exhaustion_is_not_a_server_error(self, db, add_lvol_ha, refusal): + """A cluster with nowhere to put the volume has not failed, it has declined. + + Reported as a RuntimeError, it reached clients as a 500 and every generic + retry policy treated a permanent refusal as a transient fault. + """ + add_lvol_ha.return_value = (None, refusal) + + with pytest.raises(InsufficientCapacityError): + _restore() + def test_task_creation_failure_is_a_runtime_error(self, db, add_lvol_ha, tasks): tasks.add_backup_restore_task.return_value = False diff --git a/tests/unit/web/api/v2/conftest.py b/tests/unit/web/api/v2/conftest.py index ea214c1a25..39c28af7a0 100644 --- a/tests/unit/web/api/v2/conftest.py +++ b/tests/unit/web/api/v2/conftest.py @@ -12,6 +12,8 @@ parameters. - Authentication is bypassed via ``app.dependency_overrides``; it has its own unit tests in ``test_auth.py``. +- The app registers the same exception handlers as the real one, so a test sees + the status code a client would actually get rather than a raised exception. - ``Thread`` is replaced with an inline runner so fire-and-forget endpoints (cluster start/shutdown, node restart, …) can be asserted synchronously. """ @@ -23,6 +25,7 @@ from fastapi.testclient import TestClient from simplyblock_core.db_controller import DBController +from simplyblock_web.app import register_exception_handlers import simplyblock_web.api.v2 as v2 import simplyblock_web.api.v2._auth as auth_module @@ -105,6 +108,7 @@ def db(monkeypatch): @pytest.fixture(scope='session') def app(): app = FastAPI() + register_exception_handlers(app) app.include_router(v2.api, prefix='/api/v2') app.dependency_overrides[auth_module.verify_api_token] = lambda: None app.dependency_overrides[auth_module.verify_metrics_token] = lambda: None diff --git a/tests/unit/web/api/v2/test_backup_endpoints.py b/tests/unit/web/api/v2/test_backup_endpoints.py index 59ac27868c..9f2abcdbc0 100644 --- a/tests/unit/web/api/v2/test_backup_endpoints.py +++ b/tests/unit/web/api/v2/test_backup_endpoints.py @@ -1,6 +1,8 @@ # coding=utf-8 """Unit tests for /api/v2/clusters/{id}/backups endpoints (backup_controller mocked).""" +from simplyblock_core.exceptions import InsufficientCapacityError + from tests.unit.web.api.v2 import _factories as factories from tests.unit.web.api.v2._factories import ( BACKUP_ID, @@ -83,6 +85,20 @@ def test_restores_backup(self, client, db, cluster, backup_controller): backup_controller.restore_backup.assert_called_once_with( BACKUP_ID, 'restored-volume', 'pool-1', target_node_id=None) + def test_capacity_exhaustion_is_a_conflict_not_a_server_error( + self, client, db, cluster, backup_controller): + backup_controller.restore_backup.side_effect = InsufficientCapacityError( + 'No nodes found with enough resources to create the LVol') + + response = client.post(f'{BASE}/restore', json={ + 'backup_id': BACKUP_ID, + 'lvol_name': 'restored-volume', + 'pool': 'pool-1', + }) + + assert response.status_code == 409 + assert response.json()['code'] == 'insufficient_capacity' + class TestBackupPolicies: diff --git a/tests/unit/web/api/v2/test_volume_endpoints.py b/tests/unit/web/api/v2/test_volume_endpoints.py index 9534adfc13..78cb95a36d 100644 --- a/tests/unit/web/api/v2/test_volume_endpoints.py +++ b/tests/unit/web/api/v2/test_volume_endpoints.py @@ -31,6 +31,17 @@ def test_returns_volumes_of_pool(self, client, db, volume): class TestCreateVolume: + def test_capacity_exhaustion_is_a_conflict_not_a_server_error( + self, client, db, pool, lvol_controller): + db.get_lvol_by_name.side_effect = KeyError('LVol not found') + lvol_controller.add_lvol_ha.return_value = ( + False, 'No nodes found with enough resources to create the LVol') + + response = client.post(f'{BASE}/', json={'name': 'volume-1', 'size': '10G'}) + + assert response.status_code == 409 + assert response.json()['code'] == 'insufficient_capacity' + def test_calls_add_lvol_ha(self, client, db, pool, lvol_controller): db.get_lvol_by_name.side_effect = KeyError('LVol not found') lvol_controller.add_lvol_ha.return_value = (VOLUME_ID, None)