Conversation
📝 WalkthroughWalkthroughThe pull request introduces multi-architecture build support and updates the container initialization system. The RPM build workflow is converted from single-runner to matrix-driven, building for x86_64 and aarch64 separately. The Docker publish workflow adds QEMU and Docker Buildx initialization for cross-platform image builds targeting linux/amd64 and linux/arm64. Both Containerfiles are updated to replace the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/package.yml (1)
156-156:⚠️ Potential issue | 🟠 MajorBug: ccache key is shared across the new arch matrix.
Both matrix legs (
x86_64andaarch64) now use the same hardcoded keyccache-x86_64-rpm. The two jobs will race on save, thedelete old cachestep will wipe the other arch's entry, and the cache will ping-pong between architectures every run — so neither leg gets a useful warm cache. The label is also misleading on the aarch64 runner.🔧 Suggested fix
- uses: actions/cache/restore@v4 id: cache with: path: ${{ env.CCACHE_DIR }} - key: ccache-x86_64-rpm + key: ccache-${{ matrix.arch_name }}-rpm🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/package.yml at line 156, The cache key is hardcoded as "ccache-x86_64-rpm" causing races between architectures; update the cache key in the package.yml step that currently sets key: ccache-x86_64-rpm to include the matrix architecture (e.g. use matrix.arch interpolation like key: ccache-${{ matrix.arch }}-rpm) so x86_64 and aarch64 use separate caches, and adjust any related restore-keys/labels to reflect the architecture change (refer to the "key: ccache-x86_64-rpm" entry to locate where to edit).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In @.github/workflows/package.yml:
- Line 156: The cache key is hardcoded as "ccache-x86_64-rpm" causing races
between architectures; update the cache key in the package.yml step that
currently sets key: ccache-x86_64-rpm to include the matrix architecture (e.g.
use matrix.arch interpolation like key: ccache-${{ matrix.arch }}-rpm) so x86_64
and aarch64 use separate caches, and adjust any related restore-keys/labels to
reflect the architecture change (refer to the "key: ccache-x86_64-rpm" entry to
locate where to edit).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 814159d6-547b-4dfc-808a-0bc1f9f5ca32
📒 Files selected for processing (4)
.github/workflows/package.yml.github/workflows/publish.ymlContainerfile.frrContainerfile.grout
Use catatonit as a signal-forwarding process manager instead of tini. It is available as RPM in standard fedora and centos repositories. Signed-off-by: Matej Mužila <mmuzila@redhat.com>
Build multiarch container for aarch64 and x86_64 machines, instead of building x86_64 only container. Signed-off-by: Matej Mužila <mmuzila@redhat.com>
054deee to
ebc51c0
Compare
|
Looks good to me. Could you add another commit to replace the cross compilation check with a native arm64 build+smoke tests ? |
| with: | ||
| path: ${{ env.CCACHE_DIR }} | ||
| key: ccache-x86_64-rpm |
There was a problem hiding this comment.
The cache key will need to contain ccache-${{ matrix.runs_on }}-rpm
Build multiarch container for aarch64 and x86_64 machines, instead of building x86_64 only container.
Use catatonit as a signal-forwarding process manager instead of tini. It is
available as RPM in standard fedora and centos repositories.
Container Init Process
Replaced
tiniwithcatatonitas the init process in bothContainerfile.groutandContainerfile.frr:tiniRPM download and checksum verificationcatatonitto thednfpackage installation listENTRYPOINTfrom["/usr/bin/tini", "--"]to["/usr/bin/catatonit", "--"]Catatonit is available as an RPM package in standard Fedora and CentOS repositories, eliminating the need for external downloads.
Multiarch Container Builds
Workflow: .github/workflows/publish.yml
docker/setup-qemu-action@v3) and Docker Buildx (docker/setup-buildx-action@v3) before registry authenticationquay.io/${{ vars.REGISTRY_NAMESPACE || 'grout' }}/for bothgrout/groutandgrout/frrimagesdocker/build-push-actionsteps:platforms: linux/amd64,linux/arm64Workflow: .github/workflows/package.yml
rpmjob across two runners:ubuntu-24.04for x86_64 architectureubuntu-24.04-armfor aarch64 architecturerpm-${{ matrix.arch_name }}withruns-on: ${{ matrix.runs_on }}rpm-packagesartifact to architecture-specific artifacts namedrpm-${{ matrix.arch_name }}-packagesPackage Installation
Updated
dnfinstall commands in both Containerfiles to select architecture-specific RPM files using glob pattern:/tmp/*."rpm --eval '%_arch'".rpm