feat(BA-3150): Create Valkey client for artifact storages synchronization#6964
feat(BA-3150): Create Valkey client for artifact storages synchronization#6964jopemachine wants to merge 16 commits into
Conversation
github-actions
Bot
added
size:XL
Co-authored-by: octodog <mu001@lablup.com>
There was a problem hiding this comment.
Pull request overview
This PR introduces a new Valkey client for caching and synchronizing artifact storage configurations between the manager and storage-proxy components.
Key Changes:
- Creates
ValkeyArtifactStorageClientfor caching object storage and VFS storage configurations - Moves
ObjectStorageDataandVFSStorageDatatypes from manager-specific to common modules for shared access - Adds comprehensive test coverage for the new client functionality
Reviewed changes
Copilot reviewed 24 out of 28 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/ai/backend/common/clients/valkey_client/valkey_artifact_storages/client.py |
Main client implementation with set/get/delete operations for both object and VFS storage types |
src/ai/backend/common/clients/valkey_client/valkey_artifact_storages/__init__.py |
Package exports for the new client |
src/ai/backend/common/data/object_storage/types.py |
Moved ObjectStorageData dataclass from manager to common for sharing |
src/ai/backend/common/data/vfs_storage/types.py |
Moved VFSStorageData dataclass from manager to common for sharing |
src/ai/backend/common/metrics/metric.py |
Added VALKEY_ARTIFACT_STORAGES layer type for metrics tracking |
src/ai/backend/manager/services/vfs_storage/actions/*.py |
Updated imports to use common VFSStorageData |
src/ai/backend/manager/services/object_storage/actions/*.py |
Updated imports to use common ObjectStorageData |
src/ai/backend/manager/repositories/*/repository.py |
Updated imports for refactored data types |
src/ai/backend/manager/repositories/*/db_source/db_source.py |
Updated imports for refactored data types |
src/ai/backend/manager/models/*.py |
Updated imports for refactored data types |
src/ai/backend/manager/api/gql/*.py |
Updated imports for refactored data types |
src/ai/backend/manager/event_dispatcher/handlers/artifact_registry.py |
Updated imports for refactored data types |
tests/common/clients/valkey_client/test_valkey_artifact_storage_client.py |
Comprehensive test suite covering all client operations |
changes/6964.feature.md |
Changelog entry for the new feature |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| if "id" in data and isinstance(data["id"], str): | ||
| data["id"] = uuid.UUID(data["id"]) | ||
| if "base_path" in data and isinstance(data["base_path"], str): | ||
| from pathlib import Path |
There was a problem hiding this comment.
[nitpick] The Path import is done inline within the function, but Path is not imported at the module level. Consider adding from pathlib import Path at the top of the file with other imports to maintain consistency with import organization.
| @@ -0,0 +1 @@ | |||
| Add valkey client for artifact storages configuration sharing between manager and storage-proxy No newline at end of file | |||
There was a problem hiding this comment.
| return | ||
| self._closed = True | ||
| await self._client.disconnect() | ||
|
|
There was a problem hiding this comment.
The ping method is missing the @valkey_artifact_storages_resilience.apply() decorator that is applied to all other async methods in this class. This inconsistency means ping operations won't benefit from retry logic and metrics collection. Consider adding the decorator for consistency and resilience.
| @valkey_artifact_storages_resilience.apply() |
| @valkey_artifact_storages_resilience.apply() | ||
| async def set_vfs_storage( | ||
| self, | ||
| storage_id: uuid.UUID, | ||
| storage_data: VFSStorageStatefulData, | ||
| ) -> None: |
There was a problem hiding this comment.
It seems better to group VFSStorageStatefulData and ObjectStorageStatefulData into a common ABC and store values with hset, rather than storing each object storage separately.
The current method seems to lack scalability.
|
It will be replaced by another PR |
resolves #6948 (BA-3150)
Checklist: (if applicable)
📚 Documentation preview 📚: https://sorna--6964.org.readthedocs.build/en/6964/
📚 Documentation preview 📚: https://sorna-ko--6964.org.readthedocs.build/ko/6964/