feat: Migrate filebrowser into storage proxy #710
Conversation
leksikov
added
type:feature
While implementing the filebrowser, I decoupled filebrowser from Vfolder as a separate app. Previously, did the implementation on the server of preventing multiple monitors run with common.FileLock. Implemented UID and GID settings in a startup.sh script after the containers are set. The input variables are defined in the .toml settings file. Refactoring, in order to deal with multiple volumes and their path in the settings file. Added a host:volume option in client-py. This is needed for the following things: to specify the volume on which folders are located. The same name folder can occur on multiple volumes. Then based on given argument options such as volume and name the vfid of vfolder is infered. Then filebrowser based on volume path and vfid is able to mount those directories into the container. On client-py command execution example: |
Create 711.feature.md
achimnol
changed the title
github-actions
bot
added
comp:client
There was a problem hiding this comment.
Initial batch of reviews. Also, since new file browser feature requires Docker to run, I think we need to describe it on our storage proxy's README. Please take that in mind. cc: @achimnol
| cors.add(app.router.add_route("POST", r"/create", create_or_update_filebrowser)) | ||
| cors.add(app.router.add_route("DELETE", r"/destroy", destroy_filebrowser)) |
There was a problem hiding this comment.
If you're not intending to represent regex literal, I think we need to remove leading r.
| cors.add(app.router.add_route("POST", r"/create", create_or_update_filebrowser)) | |
| cors.add(app.router.add_route("DELETE", r"/destroy", destroy_filebrowser)) | |
| cors.add(app.router.add_route("POST", "/create", create_or_update_filebrowser)) | |
| cors.add(app.router.add_route("DELETE", "/destroy", destroy_filebrowser)) |
src/ai/backend/storage/config.py
Outdated
| @asynccontextmanager | ||
| async def closing_async(thing): | ||
| try: | ||
| yield thing | ||
| finally: | ||
| await thing.close() | ||
|
|
||
|
|
There was a problem hiding this comment.
Now that we've migrated into mono-repo setup, I think it's a good chance to remove merge duplicate declarations of closing_async into one, maybe on ai.backend.common.utils. Please replace closing_async usage of cuda plugins and agent also.
| python_tests( | ||
| name="tests", | ||
| ) |
There was a problem hiding this comment.
No test case defined inside this package, so this should be removed.
| python_tests( | |
| name="tests", | |
| ) |
| cors.add(app.router.add_route("POST", r"/create", create_or_update_filebrowser)) | ||
| cors.add(app.router.add_route("DELETE", r"/destroy", destroy_filebrowser)) |
There was a problem hiding this comment.
These regex literals are not used to represent regex literal, so I think it's better to replace with plain string literals.
| cors.add(app.router.add_route("POST", r"/create", create_or_update_filebrowser)) | |
| cors.add(app.router.add_route("DELETE", r"/destroy", destroy_filebrowser)) | |
| cors.add(app.router.add_route("POST", "/create", create_or_update_filebrowser)) | |
| cors.add(app.router.add_route("DELETE", "/destroy", destroy_filebrowser)) |
| headers = {} | ||
| headers["X-BackendAI-Storage-Auth-Token"] = proxy_info.secret |
There was a problem hiding this comment.
Please refrain from using raw string itself rather than referencing AUTH_TOKEN_HDR constant under ai.backend.manager.models.storage.
| headers = {} | |
| headers["X-BackendAI-Storage-Auth-Token"] = proxy_info.secret | |
| headers = {AUTH_TOKEN_HDR: proxy_info.secret} |
| for volume_name in volumes.keys(): | ||
| if requested_volume == volume_name: | ||
| volume_cls: Type[AbstractVolume] = BACKENDS[volumes.get(volume_name)["backend"]] | ||
| mount_path = Path(volumes.get(volume_name)["path"]) | ||
| volume_obj = volume_cls( | ||
| local_config=ctx.local_config, | ||
| mount_path=mount_path, | ||
| fsprefix=None, | ||
| options={}, | ||
| ) |
There was a problem hiding this comment.
What happens if desired volume does not exist actually?
| ctx: Context, | ||
| host: str, | ||
| vfolders: list[dict], | ||
| ) -> tuple[str, int, str]: |
There was a problem hiding this comment.
Returning just tuple can make code readers confused about what the components do. Please replace it with NamedTuple or TypedDict so that readers can easily understand its role.
| ) | ||
| config["HostConfig"]["Mounts"].append( | ||
| { | ||
| "Target": f"/data/{str(vfolder['name'])}", |
There was a problem hiding this comment.
vfolder['name'] will be automatically converted to str when it's in f-strings.
| "Target": f"/data/{str(vfolder['name'])}", | |
| "Target": f"/data/{vfolder['name']}", |
| ) | ||
| await container.start() | ||
| except Exception as e: | ||
| print("Failure to recreate container ", e) |
There was a problem hiding this comment.
We need to notify user that the operation is failed, instead of just eating up raised Exception.
| await container.delete() | ||
| await tracker_db.delete_container_record(container_id) | ||
| except Exception as e: | ||
| print(f"Failure to destroy container {container_id[0:7]} ", e) |
There was a problem hiding this comment.
Same here: please alert user.
Merge from Main to branch
|
Recent commit changes introduce the following updates:
|
There was a problem hiding this comment.
Is there any infra set up with multiple storage proxies?
I am curious of DB in storage-proxy.
Is storage-proxy using any DB like sqlite currently or this be the first time to use DB in storage-proxy?
And is there any reason to use aiosqlite rather SQLAlchemy?
Co-authored-by: Sanghun Lee <[email protected]>
Migration to Mono Repository.
Moves file browsers to the storage proxy. This will greatly reduce the burden of manager and webserver to mediate large file transfers.
vfoldersubcommands to use the filebrowser session APIs