Skip to content
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

feat: Enhance VFolder mount with additional explicit and verbose options #1838

Merged
merged 39 commits into from
Mar 31, 2024

Conversation

rapsealk
Copy link
Member

@rapsealk rapsealk commented Jan 14, 2024

This PR resolves #1836 by introducing a new handler, prepare_mount_arg_v2().

For instance, users can now use the following formats with two VFolders: test-folder-808 with READ_WRITE permission and test-folder-809 with READ_ONLY permission.

  1. Users can use the same format as NAME[=PATH] or NAME[:PATH], as shown below:
$ ./backend.ai run --rm --mount test-folder-808:/home/work/abc ...
  1. Additionally, the format below is supported:
$ ./backend.ai run --rm --mount type=bind,source=test-folder-808,target=/home/work/abc ...

# with explicit READ_ONLY permission
$ ./backend.ai run --rm --mount type=bind,source=test-folder-809,target=/home/work/abc,permission=ro ...
$ ./backend.ai run --rm --mount type=bind,source=test-folder-808,target=/home/work/abc,perm=ro ...

# with explicit READ_WRITE permission
$ ./backend.ai run --rm --mount type=bind,source=test-folder-808,target=/home/work/abc,perm=rw ...

Each field separated by commas is optional, except for the source field, which is mandatory. If permission is ungiven, it will follow the permitted VFolderPermission.

For client, a new parameter mount_options has been added:

mount_options: Mapping[str, Mapping[str, Any]] = {
  "vf-001": {
    "type": "bind",
    "permission": "ro",
  },
}

Checklist: (if applicable)

  • Milestone metadata specifying the target backport version
  • Mention to the original issue
  • Installer updates including:
    • Fixtures for db schema changes
    • New mandatory config options
  • Update of end-to-end CLI integration tests in ai.backend.test
  • API server-client counterparts (e.g., manager API -> client SDK)
  • Test case(s) to:
    • Demonstrate the difference of before/after
    • Demonstrate the flow of abstract/conceptual models with a concrete implementation
  • Documentation
    • Contents in the docs directory
    • docstrings in public interfaces and type annotations

@rapsealk rapsealk added this to the 24.03 milestone Jan 14, 2024
@rapsealk rapsealk self-assigned this Jan 14, 2024
@github-actions github-actions bot added comp:cli Related to CLI component comp:manager Related to Manager component type:feature Add new features comp:client Related to Client component size:L 100~500 LoC labels Jan 14, 2024
@rapsealk rapsealk added action:on hold Hold it. Wait for the restart. and removed action:on hold Hold it. Wait for the restart. labels Jan 15, 2024
@rapsealk rapsealk marked this pull request as ready for review January 15, 2024 08:01
@rapsealk rapsealk marked this pull request as draft January 15, 2024 13:00
Copy link
Member

@achimnol achimnol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM!
Just let's trafaret-fy MountTypes and MountPermission in the API IV checks.

src/ai/backend/common/types.py Show resolved Hide resolved
tests/client/cli/test_mount.py Outdated Show resolved Hide resolved
src/ai/backend/manager/api/session.py Outdated Show resolved Hide resolved
@rapsealk
Copy link
Member Author

rror(\'{0: DataError(\\\'value should be None\\\'), 1: DataError(\\\'{\\\\\\\'pipeline-0-YynSBt\\\\\\\': DataError(\\\\\\\'{\\\\\\\\\\\\\\\'value\\\\\\\\\\\\\\\': DataError(\\\\\\\\\\\\\\\'{\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\'permission\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\': DataError("{0: DataError(\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\'value should be None\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\'), 1: DataError(\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\'value is not a valid member of MountPermission\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\')}")}\\\\\\\\\\\\\\\')}\\\\\\\')}\\\')}\')}')

@rapsealk rapsealk requested a review from achimnol January 31, 2024 02:45
Copy link
Member

@achimnol achimnol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new vfolder mount argument format should be also recognized by the backend.ai session create command as well!

@rapsealk rapsealk added urgency:1 If no other duties are available, volunteer to help. and removed urgency:1 If no other duties are available, volunteer to help. labels Mar 7, 2024
Comment on lines 247 to 259
def prepare_mount_arg(
mount_args: Optional[Sequence[str]],
mount_args: Optional[Sequence[str]] = None,
) -> Tuple[Sequence[str], Mapping[str, str]]:
"""
Parse the list of mount arguments into a list of
vfolder name and in-container mount path pairs.

:param mount_args: A list of mount arguments such as
[
"vf-5194d5d8",
"vf-70b99ea5=/home/work/abc",
"vf-cd6c0b91:/home/work/zxc",
]
Copy link
Member

@kyujin-cho kyujin-cho Mar 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When would be the case of this old prepare_mount_arg() being utilized on the new code base? I am in doubt of leaving old function on a new code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was considering keeping the old one for potential use cases. However, it would be reasonable to remove it since the new one (prepare_mount_arg_v2()) supersedes its functionality.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes removing old one looks ideal. Please update the PR accordingly.

@kyujin-cho kyujin-cho enabled auto-merge March 31, 2024 05:40
@kyujin-cho kyujin-cho added comp:cli Related to CLI component and removed comp:cli Related to CLI component labels Mar 31, 2024
@kyujin-cho kyujin-cho added this pull request to the merge queue Mar 31, 2024
Merged via the queue into main with commit fbeb34b Mar 31, 2024
21 of 29 checks passed
@kyujin-cho kyujin-cho deleted the feature/enhance-vfolder-mount-command branch March 31, 2024 05:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:cli Related to CLI component comp:client Related to Client component comp:manager Related to Manager component effort:easy Need to understand only a specific region of codes (good first issue, easy). size:L 100~500 LoC type:bug Reports about that are not working type:feature Add new features urgency:1 If no other duties are available, volunteer to help.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable explicit permission override for vfolder mounts Invalid vfolder mount target path
3 participants