-
Notifications
You must be signed in to change notification settings - Fork 163
feat(BA-1843): Separate repository layer from container registry service #5095
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Separate repository layer from container registry service | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
from typing import Optional | ||
|
||
from ai.backend.manager.data.container_registry.types import ContainerRegistryData | ||
from ai.backend.manager.models.utils import ExtendedAsyncSAEngine | ||
|
||
from .repository import ContainerRegistryRepository | ||
|
||
|
||
class AdminContainerRegistryRepository: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be good to add a permission check hook that runs before the method is executed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This action will all be removed when RBAC is applied, so for now, it is being left as is. |
||
_repository: ContainerRegistryRepository | ||
|
||
def __init__(self, db: ExtendedAsyncSAEngine) -> None: | ||
self._repository = ContainerRegistryRepository(db) | ||
|
||
async def clear_images_force( | ||
self, | ||
registry_name: str, | ||
project: Optional[str] = None, | ||
) -> ContainerRegistryData: | ||
""" | ||
Forcefully clear images from a container registry without any validation. | ||
This is an admin-only operation that should be used with caution. | ||
""" | ||
return await self._repository.clear_images(registry_name, project) | ||
|
||
async def get_by_registry_and_project_force( | ||
self, | ||
registry_name: str, | ||
project: Optional[str] = None, | ||
) -> ContainerRegistryData: | ||
""" | ||
Forcefully get container registry by name and project without any validation. | ||
This is an admin-only operation that should be used with caution. | ||
""" | ||
return await self._repository.get_by_registry_and_project(registry_name, project) | ||
|
||
async def get_by_registry_name_force(self, registry_name: str) -> list[ContainerRegistryData]: | ||
""" | ||
Forcefully get container registries by name without any validation. | ||
This is an admin-only operation that should be used with caution. | ||
""" | ||
return await self._repository.get_by_registry_name(registry_name) | ||
|
||
async def get_all_force(self) -> list[ContainerRegistryData]: | ||
""" | ||
Forcefully get all container registries without any validation. | ||
This is an admin-only operation that should be used with caution. | ||
""" | ||
return await self._repository.get_all() |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,133 @@ | ||
from ai.backend.manager.models.utils import ExtendedAsyncSAEngine | ||
from typing import Optional | ||
|
||
import sqlalchemy as sa | ||
|
||
from ai.backend.manager.data.container_registry.types import ContainerRegistryData | ||
from ai.backend.manager.data.image.types import ImageStatus | ||
from ai.backend.manager.errors.exceptions import ContainerRegistryNotFound | ||
from ai.backend.manager.models.container_registry import ContainerRegistryRow | ||
from ai.backend.manager.models.image import ImageRow | ||
from ai.backend.manager.models.utils import ExtendedAsyncSAEngine, SASession | ||
|
||
|
||
class ContainerRegistryRepository: | ||
_db: ExtendedAsyncSAEngine | ||
|
||
def __init__(self, db: ExtendedAsyncSAEngine) -> None: | ||
self._db = db | ||
|
||
async def get_by_registry_and_project( | ||
self, | ||
registry_name: str, | ||
project: Optional[str] = None, | ||
) -> ContainerRegistryData: | ||
async with self._db.begin_readonly_session() as session: | ||
result = await self._get_by_registry_and_project(session, registry_name, project) | ||
if not result: | ||
raise ContainerRegistryNotFound() | ||
return result | ||
|
||
async def get_by_registry_name(self, registry_name: str) -> list[ContainerRegistryData]: | ||
async with self._db.begin_readonly_session() as session: | ||
return await self._get_by_registry_name(session, registry_name) | ||
|
||
async def get_all(self) -> list[ContainerRegistryData]: | ||
async with self._db.begin_readonly_session() as session: | ||
return await self._get_all(session) | ||
|
||
async def clear_images( | ||
self, | ||
registry_name: str, | ||
project: Optional[str] = None, | ||
) -> ContainerRegistryData: | ||
async with self._db.begin_session() as session: | ||
# Clear images | ||
update_stmt = ( | ||
sa.update(ImageRow) | ||
.where(ImageRow.registry == registry_name) | ||
.where(ImageRow.status != ImageStatus.DELETED) | ||
.values(status=ImageStatus.DELETED) | ||
) | ||
if project: | ||
update_stmt = update_stmt.where(ImageRow.project == project) | ||
|
||
await session.execute(update_stmt) | ||
|
||
# Return registry data | ||
result = await self._get_by_registry_and_project(session, registry_name, project) | ||
if not result: | ||
raise ContainerRegistryNotFound() | ||
return result | ||
|
||
async def get_known_registries(self) -> dict[str, str]: | ||
async with self._db.begin_readonly_session() as session: | ||
from ai.backend.manager.models.container_registry import ContainerRegistryRow | ||
|
||
known_registries_map = await ContainerRegistryRow.get_known_container_registries( | ||
session | ||
) | ||
|
||
known_registries = {} | ||
for project, registries in known_registries_map.items(): | ||
for registry_name, url in registries.items(): | ||
if project: | ||
key = f"{project}/{registry_name}" | ||
else: | ||
key = registry_name | ||
known_registries[key] = url.human_repr() | ||
|
||
return known_registries | ||
|
||
async def get_registry_row_for_scanner( | ||
self, | ||
registry_name: str, | ||
project: Optional[str] = None, | ||
) -> ContainerRegistryRow: | ||
""" | ||
Get the raw ContainerRegistryRow object needed for container registry scanner. | ||
Raises ContainerRegistryNotFound if registry is not found. | ||
""" | ||
async with self._db.begin_readonly_session() as session: | ||
stmt = sa.select(ContainerRegistryRow).where( | ||
ContainerRegistryRow.registry_name == registry_name, | ||
) | ||
if project: | ||
stmt = stmt.where(ContainerRegistryRow.project == project) | ||
|
||
row = await session.scalar(stmt) | ||
if not row: | ||
raise ContainerRegistryNotFound() | ||
return row | ||
|
||
async def _get_by_registry_and_project( | ||
self, | ||
session: SASession, | ||
registry_name: str, | ||
project: Optional[str] = None, | ||
) -> Optional[ContainerRegistryData]: | ||
stmt = sa.select(ContainerRegistryRow).where( | ||
ContainerRegistryRow.registry_name == registry_name, | ||
) | ||
if project: | ||
stmt = stmt.where(ContainerRegistryRow.project == project) | ||
|
||
row = await session.scalar(stmt) | ||
return row.to_dataclass() if row else None | ||
|
||
async def _get_by_registry_name( | ||
self, | ||
session: SASession, | ||
registry_name: str, | ||
) -> list[ContainerRegistryData]: | ||
stmt = sa.select(ContainerRegistryRow).where( | ||
ContainerRegistryRow.registry_name == registry_name | ||
) | ||
result = await session.execute(stmt) | ||
rows = result.scalars().all() | ||
return [row.to_dataclass() for row in rows] | ||
|
||
async def _get_all(self, session: SASession) -> list[ContainerRegistryData]: | ||
stmt = sa.select(ContainerRegistryRow) | ||
result = await session.execute(stmt) | ||
rows = result.scalars().all() | ||
return [row.to_dataclass() for row in rows] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the current message is appropriate because the important thing is not separating the admin, but separating the repository layer from the service.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't the container repository already separated, and isn't this PR about separating the admin container repository layer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't consider it separated because, although there was a repository class, the actual implementation wasn't separated.