mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-24 00:52:24 +00:00
fix: evict last deleted model in multi-instance deployments
_delete_deployment had an early return when db_models was empty, preventing eviction of the last deleted model during reconciliation. - Remove len(db_models)==0 early return from _delete_deployment - Return None (not []) from _get_models_from_db on DB failure so callers can distinguish a transient failure from a genuinely empty DB - Guard _update_llm_router against None to skip updates on DB failure Fixes #28443
This commit is contained in:
parent
1cfe37d891
commit
7fbd8651fd
2 changed files with 135 additions and 26 deletions
|
|
@ -2710,11 +2710,9 @@ def run_ollama_serve():
|
|||
with open(os.devnull, "w") as devnull:
|
||||
subprocess.Popen(command, stdout=devnull, stderr=devnull)
|
||||
except Exception as e:
|
||||
verbose_proxy_logger.debug(
|
||||
f"""
|
||||
verbose_proxy_logger.debug(f"""
|
||||
LiteLLM Warning: proxy started with `ollama` model\n`ollama serve` failed with Exception{e}. \nEnsure you run `ollama serve`
|
||||
"""
|
||||
)
|
||||
""")
|
||||
|
||||
|
||||
def _get_process_rss_mb() -> Optional[float]:
|
||||
|
|
@ -4762,9 +4760,12 @@ class ProxyConfig:
|
|||
combined_id_list = []
|
||||
|
||||
## BASE CASES ##
|
||||
# if llm_router is None or db_models is empty, return 0
|
||||
if llm_router is None or len(db_models) == 0:
|
||||
if llm_router is None:
|
||||
return 0
|
||||
# NOTE: db_models may be legitimately empty when all DB models have been deleted.
|
||||
# Do NOT short-circuit on len(db_models) == 0 — we must still evict any
|
||||
# DB-sourced deployments that are no longer in the DB. The caller
|
||||
# (_update_llm_router) already guards against None (transient fetch failure).
|
||||
|
||||
## DB MODELS ##
|
||||
for m in db_models:
|
||||
|
|
@ -4914,6 +4915,15 @@ class ProxyConfig:
|
|||
)
|
||||
|
||||
try:
|
||||
# new_models is None when _get_models_from_db failed (transient DB error).
|
||||
# Skip the update entirely so we don't evict valid deployments.
|
||||
if new_models is None:
|
||||
verbose_proxy_logger.warning(
|
||||
"_update_llm_router: DB model fetch returned None (transient failure). "
|
||||
"Skipping router update to preserve existing deployments."
|
||||
)
|
||||
return
|
||||
|
||||
models_list: list = new_models if isinstance(new_models, list) else []
|
||||
if llm_router is None and master_key is not None:
|
||||
verbose_proxy_logger.debug(f"len new_models: {len(models_list)}")
|
||||
|
|
@ -5616,18 +5626,25 @@ class ProxyConfig:
|
|||
# Check if the object type is in the list (supports both str and enum values)
|
||||
return any(str(obj) == object_type_str for obj in supported_db_objects)
|
||||
|
||||
async def _get_models_from_db(self, prisma_client: PrismaClient) -> list:
|
||||
async def _get_models_from_db(self, prisma_client: PrismaClient) -> Optional[list]:
|
||||
"""
|
||||
Fetch all model deployments from the DB.
|
||||
|
||||
Returns:
|
||||
- list: the rows (may be empty if no models exist)
|
||||
- None: signals a DB fetch *failure* — callers must not treat this
|
||||
as "all models deleted" and must not evict existing router deployments.
|
||||
"""
|
||||
try:
|
||||
new_models = await prisma_client.db.litellm_proxymodeltable.find_many()
|
||||
return new_models
|
||||
except Exception as e:
|
||||
verbose_proxy_logger.exception(
|
||||
"litellm.proxy_server.py::add_deployment() - Error getting new models from DB - {}".format(
|
||||
str(e)
|
||||
)
|
||||
)
|
||||
new_models = []
|
||||
|
||||
return new_models
|
||||
return None
|
||||
|
||||
async def add_deployment(
|
||||
self,
|
||||
|
|
|
|||
|
|
@ -1923,11 +1923,23 @@ async def test_delete_deployment_type_mismatch():
|
|||
|
||||
mock_llm_router.delete_deployment = MagicMock(side_effect=mock_delete_deployment)
|
||||
|
||||
# Mock get_config to return empty config (no config models)
|
||||
async def mock_get_config(config_file_path):
|
||||
return {}
|
||||
return {
|
||||
"model_list": [
|
||||
{
|
||||
"model_name": "openai-gpt-4o",
|
||||
"litellm_params": {"model": "gpt-4o"},
|
||||
"model_info": {"id": 12345678},
|
||||
},
|
||||
{
|
||||
"model_name": "openai-gpt-4o",
|
||||
"litellm_params": {"model": "gpt-4o"},
|
||||
"model_info": {"id": 12345679},
|
||||
},
|
||||
]
|
||||
}
|
||||
|
||||
pc.get_config = MagicMock(side_effect=mock_get_config)
|
||||
pc.get_config = AsyncMock(side_effect=mock_get_config)
|
||||
|
||||
# Patch the global llm_router
|
||||
with (
|
||||
|
|
@ -1937,20 +1949,29 @@ async def test_delete_deployment_type_mismatch():
|
|||
# Call the function under test
|
||||
deleted_count = await pc._delete_deployment(db_models=[])
|
||||
|
||||
# Assertions: Models 12345678 and 12345679 should NOT be deleted
|
||||
# because they exist in combined_id_list (as integers) even though
|
||||
# router has them as strings
|
||||
# The two SHA-hash models have no corresponding entry in combined_id_list
|
||||
# and must be evicted.
|
||||
assert (
|
||||
deleted_count == 2
|
||||
), f"Expected 2 deletions (SHA-hash models), got {deleted_count}"
|
||||
assert (
|
||||
"a96e12e76b36a57cfae57a41288eb41567629cac89b4828c6f7074afc3534695"
|
||||
in deleted_ids
|
||||
)
|
||||
assert (
|
||||
"a40186dd0fdb9b7282380277d7f57044d29de95bfbfcd7f4322b3493702d5cd3"
|
||||
in deleted_ids
|
||||
)
|
||||
|
||||
# The function should delete the other 2 models that are not in combined_id_list
|
||||
assert deleted_count == 0, f"Expected 0 deletions, got {deleted_count}"
|
||||
|
||||
# Verify that 12345678 and 12345679 were NOT deleted
|
||||
assert (
|
||||
"12345678" not in deleted_ids
|
||||
), f"Model 12345678 should NOT be deleted. Deleted IDs: {deleted_ids}"
|
||||
assert (
|
||||
"12345679" not in deleted_ids
|
||||
), f"Model 12345679 should NOT be deleted. Deleted IDs: {deleted_ids}"
|
||||
# Models 12345678 and 12345679 exist in the config (as integers); str()
|
||||
# conversion in _delete_deployment makes them match the router's string IDs,
|
||||
# so they must NOT be evicted.
|
||||
assert (
|
||||
"12345678" not in deleted_ids
|
||||
), f"Model 12345678 should NOT be deleted. Deleted IDs: {deleted_ids}"
|
||||
assert (
|
||||
"12345679" not in deleted_ids
|
||||
), f"Model 12345679 should NOT be deleted. Deleted IDs: {deleted_ids}"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
|
|
@ -7427,3 +7448,74 @@ class TestSortModelsByDisplayName:
|
|||
all_models=models, sort_by="model_name", sort_order="asc"
|
||||
)
|
||||
assert [m["model_name"] for m in sorted_models] == ["alpha", "beta"]
|
||||
|
||||
|
||||
class TestDeleteDeploymentSync:
|
||||
@pytest.mark.asyncio
|
||||
async def test_delete_deployment_evicts_model_when_all_db_models_deleted(self):
|
||||
"""
|
||||
Regression test for #28443.
|
||||
When all DB models are deleted, _delete_deployment must evict them from
|
||||
the router. The old code returned 0 early when db_models was empty.
|
||||
"""
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
|
||||
from litellm.proxy.proxy_server import ProxyConfig
|
||||
|
||||
proxy_config = ProxyConfig()
|
||||
mock_router = MagicMock()
|
||||
mock_router.get_model_ids.return_value = ["model-id-to-evict"]
|
||||
mock_router.delete_deployment.return_value = MagicMock()
|
||||
|
||||
with patch("litellm.proxy.proxy_server.llm_router", mock_router):
|
||||
with patch.object(
|
||||
proxy_config, "get_config", AsyncMock(return_value={"model_list": []})
|
||||
):
|
||||
count = await proxy_config._delete_deployment(db_models=[])
|
||||
|
||||
mock_router.delete_deployment.assert_called_once_with(id="model-id-to-evict")
|
||||
assert count == 1
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_update_llm_router_skips_update_on_db_fetch_failure(self):
|
||||
"""
|
||||
When _get_models_from_db returns None (transient DB failure), _update_llm_router
|
||||
must return early without touching the router.
|
||||
"""
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
|
||||
from litellm.proxy.proxy_server import ProxyConfig
|
||||
|
||||
proxy_config = ProxyConfig()
|
||||
mock_router = MagicMock()
|
||||
|
||||
with patch("litellm.proxy.proxy_server.llm_router", mock_router):
|
||||
with patch.object(proxy_config, "get_config", AsyncMock(return_value={})):
|
||||
await proxy_config._update_llm_router(
|
||||
new_models=None, proxy_logging_obj=MagicMock()
|
||||
)
|
||||
|
||||
mock_router.delete_deployment.assert_not_called()
|
||||
mock_router.upsert_deployment.assert_not_called()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_models_from_db_returns_none_on_exception(self):
|
||||
"""
|
||||
_get_models_from_db must return None (not []) when the DB raises an exception,
|
||||
so callers can distinguish a transient failure from a genuinely empty DB.
|
||||
"""
|
||||
from unittest.mock import AsyncMock, MagicMock
|
||||
|
||||
from litellm.proxy.proxy_server import ProxyConfig
|
||||
|
||||
proxy_config = ProxyConfig()
|
||||
mock_prisma = MagicMock()
|
||||
mock_prisma.db.litellm_proxymodeltable.find_many = AsyncMock(
|
||||
side_effect=Exception("DB connection lost")
|
||||
)
|
||||
|
||||
result = await proxy_config._get_models_from_db(prisma_client=mock_prisma)
|
||||
|
||||
assert (
|
||||
result is None
|
||||
), f"Expected None on DB failure to signal fetch error, got {result!r}"
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue