From 24f42e910539e71a10d9aa1d7a3595504212a8a8 Mon Sep 17 00:00:00 2001 From: Manohar Reddy Date: Wed, 2 Sep 2026 10:15:41 +0200 Subject: [PATCH] fix(web): answer capacity exhaustion with 409, not 500 A cluster with no room for another logical volume answered create and restore with a 500. Two call sites turn add_lvol_ha's (id, error) tuple into an exception, and neither could tell a refusal from a fault: backup_controller raised RuntimeError, the v2 volume route raised ValueError, and both reached the client as a server error. A full cluster has not failed. It declined a well-formed request, and it will go on declining until capacity is freed or a node is added, so a 5xx is wrong twice over: it blames the control plane for a healthy refusal, and it puts the answer in the status class that every generic retry policy retries. The simplyblock-operator hit exactly that, retrying a restore every 10 seconds indefinitely because a 500 is, by convention, worth retrying. InsufficientCapacityError now carries the condition, and a handler maps it to 409 with a machine-readable "code": "insufficient_capacity", so clients need never match on prose. 409 rather than 507: 507 names the condition more precisely but is a 5xx, which would leave naive clients retrying a permanent refusal. Both refusals are recognized through the constants they are built from (ERR_NO_NODE_WITH_CAPACITY, ERR_OBJECT_LIMIT_PREFIX), so a reworded message cannot silently stop being classified. The endpoint unit tests built a bare FastAPI() carrying none of the real app's exception handlers, so no test in that suite could observe a status code a handler produced. Handler registration moved into register_exception_handlers() and the test app now uses it, which is what lets the new tests assert 409 at all. Two typos in existing handler log messages are fixed in passing. Tests, all four red first: the classification in restore_backup for both refusals, and the 409 at the restore and volume-create endpoints. --- .../controllers/backup_controller.py | 4 +- .../controllers/lvol_controller.py | 28 ++++++++- simplyblock_core/exceptions.py | 10 ++++ .../cluster/storage_pool/volume/__init__.py | 3 + simplyblock_web/app.py | 58 +++++++++++++------ .../test_backup_restore_node_selection.py | 19 +++++- tests/unit/web/api/v2/conftest.py | 4 ++ .../unit/web/api/v2/test_backup_endpoints.py | 16 +++++ .../unit/web/api/v2/test_volume_endpoints.py | 11 ++++ 9 files changed, 131 insertions(+), 22 deletions(-) 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)