feat: add ARM64 support for AgentENV - #86
Conversation
|
🔍 OpenCodeReview found 3 issue(s) in this PR.
[security · high] 📄
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| run: echo "OWNER=${GITHUB_REPOSITORY_OWNER,,}" >> "$GITHUB_ENV" | ||
|
|
||
| - name: Set up Docker Buildx | ||
| uses: docker/setup-buildx-action@v4 |
There was a problem hiding this comment.
[security · high]
This newly added publishing job runs Docker-maintained third-party actions by mutable tags while holding packages: write; the following login action also receives GITHUB_TOKEN. If either tag is retargeted or compromised, its code can steal the token or publish a tampered manifest. Pin both docker/setup-buildx-action and docker/login-action in this job to audited full commit SHAs.
| docker-publish: | ||
| name: Publish multi-architecture Docker image | ||
| needs: docker-build | ||
| runs-on: ubuntu-24.04 |
There was a problem hiding this comment.
[other · medium]
This new registry-publishing job has no timeout-minutes. A stalled Buildx setup, registry manifest operation, or inspection can occupy the runner and leave the release workflow blocked until the platform limit. Add a bounded timeout appropriate for registry operations (for example, 30 minutes).
Suggestion:
| docker-publish: | |
| name: Publish multi-architecture Docker image | |
| needs: docker-build | |
| runs-on: ubuntu-24.04 | |
| docker-publish: | |
| name: Publish multi-architecture Docker image | |
| needs: docker-build | |
| runs-on: ubuntu-24.04 | |
| timeout-minutes: 30 |
|
Where could I build vmlinux myself |
This is just a Linux kernel built from Firecracker's microvm-kernel-ci-aarch64-6.1.config. You can build your own kernel using this config and enable additional options as needed. |
cfa3d2c to
ba9871d
Compare
| name: e2e-tests (${{ matrix.e2e_name }}, ${{ matrix.os }}) | ||
| # Ubuntu 24.04's Linux 6.17 kernel can crash in ublk ADD_DEV. | ||
| runs-on: ubuntu-22.04 | ||
| runs-on: ${{ matrix.os }} |
There was a problem hiding this comment.
[other · medium]
This expanded matrix has no timeout-minutes, so any hung KVM/ublk or compose test can occupy both x64 and ARM runners indefinitely. Add a job-level timeout sized for the slowest ARM leg to bound resource consumption.
Suggestion:
| runs-on: ${{ matrix.os }} | |
| runs-on: ${{ matrix.os }} | |
| timeout-minutes: 60 |
| os: | ||
| - ubuntu-22.04 | ||
| - ubuntu-22.04-arm |
There was a problem hiding this comment.
[performance · medium]
The new architecture legs share the same BuildKit GHA cache scopes (agentenv-runtime, agentenv-gateway, and agentenv-scheduler). Because both OS legs can export those scopes concurrently on main, the last writer can replace the other architecture's cache, causing persistent misses or cache thrashing. Include the architecture/OS in each cache-from and cache-to scope (for example, ${{ runner.arch }}), as the release workflow does.
| docker-publish: | ||
| name: Publish multi-architecture Docker image | ||
| needs: docker-build | ||
| runs-on: ubuntu-24.04 |
There was a problem hiding this comment.
[other · medium]
Add timeout-minutes to this new publication job. Registry login, manifest creation, or inspection can otherwise stall until GitHub's much longer default job limit, leaving a release run and runner occupied. A timeout consistent with the adjacent Docker build job (for example, 30 minutes) would bound this failure mode.
Suggestion:
| docker-publish: | |
| name: Publish multi-architecture Docker image | |
| needs: docker-build | |
| runs-on: ubuntu-24.04 | |
| docker-publish: | |
| name: Publish multi-architecture Docker image | |
| needs: docker-build | |
| runs-on: ubuntu-24.04 | |
| timeout-minutes: 30 |
| permissions: | ||
| contents: read | ||
| packages: write | ||
| steps: | ||
| - name: Set lowercase owner |
There was a problem hiding this comment.
[security · low]
This manifest-only job does not check out or otherwise read repository contents, so contents: read is unnecessary. Set contents: none (or omit it, since declaring job permissions makes unspecified scopes none) to reduce the token privileges exposed to the publishing steps.
Suggestion:
| permissions: | |
| contents: read | |
| packages: write | |
| steps: | |
| - name: Set lowercase owner | |
| permissions: | |
| packages: write | |
| steps: | |
| - name: Set lowercase owner |
b042bc8 to
9903e36
Compare
| --tag "${image}:${version}" \ | ||
| --tag "${image}:latest" \ |
There was a problem hiding this comment.
[bug · low]
Every tag-triggered workflow unconditionally rewrites the shared latest tag, and this workflow has no concurrency or version-order guard. Overlapping or retried releases can therefore let an older release finish last and point latest backward. Serialize publication and/or update latest only after verifying this tag is the newest release; alternatively publish only the immutable version tag.
Suggestion:
| --tag "${image}:${version}" \ | |
| --tag "${image}:latest" \ | |
| --tag "${image}:${version}" \ |
9903e36 to
4e529b1
Compare
| --tag "${image}:${version}" \ | ||
| --tag "${image}:latest" \ | ||
| "${image}:${version}-amd64" \ | ||
| "${image}:${version}-arm64" |
There was a problem hiding this comment.
[other · low]
This workflow has no release concurrency control, so overlapping tag runs can complete out of order and an older release can overwrite latest after a newer one. Since the documentation actively directs users to this mutable tag, serialize manifest publication (for example, with a shared release concurrency group and cancel-in-progress: false) or avoid publishing latest.
4e529b1 to
f384f8f
Compare
| platforms: ${{ matrix.platform }} | ||
| push: true |
There was a problem hiding this comment.
[performance · medium]
The previous external cache-from was removed while this change adds a second native architecture build. BuildKit cache mounts are local to each ephemeral runner, so both legs now rebuild the Rust dependency layers from scratch on every release and may exceed the new 30-minute timeout, especially on ARM. Configure cache-from and cache-to with architecture-specific GHA scopes to avoid cross-architecture cache collisions.
Suggestion:
| platforms: ${{ matrix.platform }} | |
| push: true | |
| platforms: ${{ matrix.platform }} | |
| push: true | |
| cache-from: type=gha,scope=agentenv-runtime-${{ matrix.arch }} | |
| cache-to: type=gha,scope=agentenv-runtime-${{ matrix.arch }},mode=max |
| version = "0.1.1" | ||
| url = "ghcr.io/zlzgithub-0801/agentenv-tools:{version}" |
There was a problem hiding this comment.
[maintainability · medium]
This changes the runtime default to agentenv-tools:0.1.1, but the repository's tools-image producer still defaults TOOLS_VERSION to 0.1.0 in both tools-image/Makefile and tools-image/Dockerfile, and the release workflow does not publish the tools image. Consequently, the standard/default build cannot produce the artifact referenced here, and build-server will fail during --setup-only unless 0.1.1 has been published out of band. Keep the producer defaults synchronized with this manifest (or add an automated publication/existence check before consuming the tag).
f384f8f to
f981a99
Compare
| platforms: ${{ matrix.platform }} | ||
| push: true |
There was a problem hiding this comment.
[performance · medium]
The previous cache-from was removed while image publication now builds two architectures on fresh hosted runners. The Dockerfile performs apt installation and a full Cargo build, so both matrix legs will rebuild all layers and may exceed the new 30-minute timeout, especially ARM64. Configure cache-from and cache-to using architecture-specific GHA scopes (for example, include matrix.arch in the scope).
| --tag "${image}:${version}" \ | ||
| --tag "${image}:latest" \ |
There was a problem hiding this comment.
[bug · medium]
Concurrent tag-release runs can publish latest out of order because this workflow has no concurrency/promotion guard: an older release finishing after a newer one will overwrite latest with the older manifest. Prefer publishing only immutable version tags, or update latest through a serialized promotion job that verifies this is still the newest release before writing it.
705fcc1 to
d08443b
Compare
3b71fca to
479694a
Compare
479694a to
4a07bc6
Compare
| docker buildx imagetools create \ | ||
| --tag "${image}:${version}" \ | ||
| --tag "${image}:latest" \ | ||
| "${image}:${version}-amd64" \ | ||
| "${image}:${version}-arm64" |
There was a problem hiding this comment.
[other · medium]
Release runs are not serialized, so two tag pushes can publish concurrently and an older run that finishes last will move latest back to the older manifest. The new latest-pvm publication has the same race. Add workflow concurrency that serializes image publication (without cancelling an in-progress release), or update floating tags in a separately serialized job; immutable version tags can continue to publish concurrently.
| CARGO_HOST_TARGET_ENV := $(shell $(CARGO) -vV | sed -n 's/^host: //p' | tr '[:lower:]-' '[:upper:]_') | ||
| CAPABILITY_RUNNER := CARGO_TARGET_$(CARGO_HOST_TARGET_ENV)_RUNNER="$(CURDIR)/scripts/run-with-capabilities.sh" |
There was a problem hiding this comment.
[maintainability · low]
These immediate (:=) assignments execute cargo -vV while Make parses the file, even for unrelated targets such as deployment or cleanup. This introduces a new global Cargo dependency (and emits an error/makes an empty CARGO_TARGET__RUNNER when Cargo is unavailable) for targets that do not use CAPABILITY_RUNNER. Defer host detection until a capability-enabled target expands this variable, or scope it to those targets.
Suggestion:
| CARGO_HOST_TARGET_ENV := $(shell $(CARGO) -vV | sed -n 's/^host: //p' | tr '[:lower:]-' '[:upper:]_') | |
| CAPABILITY_RUNNER := CARGO_TARGET_$(CARGO_HOST_TARGET_ENV)_RUNNER="$(CURDIR)/scripts/run-with-capabilities.sh" | |
| CARGO_HOST_TARGET_ENV = $(shell $(CARGO) -vV | sed -n 's/^host: //p' | tr '[:lower:]-' '[:upper:]_') | |
| CAPABILITY_RUNNER = CARGO_TARGET_$(CARGO_HOST_TARGET_ENV)_RUNNER="$(CURDIR)/scripts/run-with-capabilities.sh" |
4 participants
What
Add ARM64/aarch64 support across AgentENV runtime dependency provisioning, release artifacts, and Docker images.
Why
AgentENV installation and Server releases previously assumed x86_64. On ARM64 hosts, the installer rejected the architecture and dependency setup could not resolve an architecture-specific kernel artifact.
Related issue
Closes #60, #63
Scope and non-goals
Included:
Not included:
Design and behavior changes
aenv-server-linux-aarch64.tar.gzto the release artifacts and publish the Docker image as a multi-architecture image.Compatibility and operations
kvcache-ai/firecrackerGitHub Release and include architecture-specific asset names.Validation
make fmtmake clippymake test-unitmake -C services test(required whenservices/changes)maketargetCommands and results:
Skipped checks and reasons:
This PR does not change
services/and doesn't have performance-sensitive change either.Risks and reviewer notes
Checklist