Feature/server/v0.1.14 self#933
Conversation
bombomla
requested review from
Generalwin,
Pangjiping,
Spground,
hittyt,
jwx0925 and
ninan-nn
as code owners
|
|
|
Changed directories: Dockerfile-ingress-noroot、components、downloaded_config.json、downloaded_readme.md、server、test_download.txt. 📋 Recommended labels (based on changed files):
Other available labels:
💡 Tip: Use cc @bombomla |
![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: 432e2de89d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| COPY src ./src | ||
| COPY LICENSE README.md README_zh.md example.config.toml example.config.zh.toml \ | ||
| example.config.k8s.toml example.config.k8s.zh.toml example.batchsandbox-template.yaml ./ |
There was a problem hiding this comment.
This Dockerfile copies src and several example.* files from the server/ root, but those paths do not exist in this repository (server/ contains opensandbox_server/ and examples under opensandbox_server/examples/). As written, image builds fail during COPY before runtime, so the new non-root server image cannot be produced.
Useful? React with 👍 / 👎.
| # Convert to cores (e.g., 0.25) | ||
| cores = scaled_millicores / 1000 | ||
| return f"{cores:.3g}" |
There was a problem hiding this comment.
When a CPU limit is small (for example 1m), this branch converts the scaled value to cores and can emit values below 0.001 CPU (for example 0.00025). Kubernetes rejects CPU quantities with precision finer than 1m, so workloads with low CPU limits can now fail admission even though the original limit was valid.
Useful? React with 👍 / 👎.
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| FROM golang:1.24.0 AS builder |
There was a problem hiding this comment.
components/ingress/go.mod declares go 1.25.0, but this Dockerfile pins the builder to golang:1.24.0; that causes go mod download/go build in this build stage to fail on the module minimum Go version check. As a result, the newly added non-root ingress image definition is not buildable.
Useful? React with 👍 / 👎.
| if scaled_amount >= 1: | ||
| if scaled_amount == int(scaled_amount): | ||
| return f"{int(scaled_amount)}{unit}" | ||
| return f"{scaled_amount:.0f}{unit}" |
There was a problem hiding this comment.
This formatting rounds scaled memory to a whole unit whenever the scaled value is >= 1 (for example, scaling 5Gi by 0.25 yields 1Gi instead of 1.25Gi), which materially deviates from the documented fraction behavior and can over- or under-request memory depending on the input. That distorts scheduling and quota behavior compared to the intended 25% request rule.
Useful? React with 👍 / 👎.
| return f"{scaled_cores:.3g}" | ||
|
|
||
| # Handle memory with units (e.g., "512Mi", "1Gi", "1024Ki") | ||
| memory_match = re.match(r'^(\d+(?:\.\d+)?)(Ki|Mi|Gi|Ti|Pi|Ei)$', value_str) |
There was a problem hiding this comment.
The scaler only matches binary memory suffixes (Ki..Ei), so valid Kubernetes decimal-SI values like 2G or 512M bypass scaling and are returned unchanged. After this commit, those requests stay equal to limits while other memory formats are reduced to 25%, creating inconsistent scheduling/quota behavior for equivalent inputs.
Useful? React with 👍 / 👎.
3 participants
Summary
Testing
Breaking Changes
Checklist