feat: implement OSSFS storage backend in sandbox lifecycle#340
feat: implement OSSFS storage backend in sandbox lifecycle#340hittyt wants to merge 6 commits intoalibaba:mainfrom
Conversation
hittyt
requested review from
Generalwin,
Pangjiping,
Spground,
fengcone,
jwx0925,
kevinlynx and
ninan-nn
as code owners
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a72893a549
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
server/src/services/docker.py
Outdated
| for volume in volumes: | ||
| if volume.ossfs is not None: | ||
| mount_keys.append(self._ensure_ossfs_mounted(volume)) | ||
| return list(dict.fromkeys(mount_keys)) |
There was a problem hiding this comment.
When a sandbox defines two ossfs volumes that share the same backend path (e.g., same bucket/path but different subPath), _prepare_ossfs_mounts calls _ensure_ossfs_mounted for each volume (incrementing the refcount twice) and then deduplicates the returned keys before storing them on the container label. Deletion later releases each labeled key only once, so the refcount never reaches zero and the mount is never unmounted, causing persistent leaked OSSFS mounts.
Useful? React with 👍 / 👎.
| current = self._ossfs_mount_ref_counts.get(mount_key, 0) | ||
| if current <= 1: | ||
| self._ossfs_mount_ref_counts.pop(mount_key, None) | ||
| should_unmount = True |
There was a problem hiding this comment.
The release logic treats current == 0 the same as current == 1 and proceeds to unmount. After process restart, _ossfs_mount_ref_counts is empty (it is initialized in-memory and not rebuilt from existing containers), so deleting one sandbox can unmount a shared OSSFS path still used by other running sandboxes. This can break active workloads by tearing down their underlying bind source unexpectedly.
Useful? React with 👍 / 👎.
hittyt
force-pushed
the
feat/support-ossfs-api
branch
from
a72893a to
ce216ac
Compare
|
Please rebase main to resolve pyproject.toml format error |
- Implement OSSFS volume support in Docker runtime with mount reference counting. - Add comprehensive validation and unit tests for the new storage backend.
hittyt
force-pushed
the
feat/support-ossfs-api
branch
from
ce216ac to
c12516d
Compare
hittyt
force-pushed
the
feat/support-ossfs-api
branch
from
aaf4e73 to
25eb4ed
Compare
sdks/sandbox/python/src/opensandbox/api/lifecycle/models/credential_ref.py
Outdated
Show resolved
Hide resolved
| return pvc_inspect_cache | ||
|
|
||
| @staticmethod | ||
| def _derive_oss_region(endpoint: str) -> Optional[str]: |
There was a problem hiding this comment.
What is the purpose of resolving the region? It looks like the region is only required for ossfs 1.0 with sigv4 enabled, and it is not the region of the OSS bucket, but rather the region of client.
region=cn-hangzhou：OSS Bucket请求地域标识符，挂载时添加-oregion=<region_id>，默认为空。在使用V4签名时必须添加该选项来作为发起请求地域的标识符。
| return endpoint[start:end] | ||
|
|
||
| @staticmethod | ||
| def _normalize_oss_endpoint_url(endpoint: str) -> str: |
There was a problem hiding this comment.
These methods could be abstracted into a helper.py or endpoint.py file. This should be a general external endpoint util, rather than being specific to OSS endpoints only.
| ) | ||
|
|
||
|
|
||
| def ensure_valid_ossfs_volume(ossfs: "OSSFS") -> None: |
There was a problem hiding this comment.
It should be made clear that this implementation does not support OpenSandbox Server running on Windows.
|
|
||
| return backend_path, backend_path | ||
|
|
||
| def _build_ossfs_v1_command( |
There was a problem hiding this comment.
Could you abstract all the ossfs-specific logic into a separate file? Too many branching logics are cluttering the core flow of docker provisioning.
| Raises: | ||
| HTTPException: When any OSSFS field is invalid. | ||
| """ | ||
| if not ossfs.bucket or not OSS_BUCKET_RE.match(ossfs.bucket): |
There was a problem hiding this comment.
We should not validate the OSS bucket name.
| }, | ||
| ) | ||
|
|
||
| if ossfs.version not in OSSFS_SUPPORTED_VERSIONS: |
There was a problem hiding this comment.
It looks like this is a redundant validation. In the schema structure, you can restrict it to only accept 1.0 and 2.0, so the value here is guaranteed to be valid.
4 participants
This PR implements the OSSFS storage backend for the sandbox lifecycle, as detailed in OSEP-0003.
Key features:
ossfsmounting.