fix(k3s): fix cross platform image loading #3437
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Summary by CodeRabbit
WalkthroughAdds platform-aware image load and pull APIs: K3s gains LoadImagesWithPlatform, DockerProvider gains PullImageWithOpts and a platform option, import flow runs ctr with optional --platform and returns ctr output on failures, plus platform-aware tests and dependency updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant K3s as K3sContainer
participant Saver as ImageSaver
participant Host as HostFS
participant Ctr as k3s-ctr
Note over Caller,K3s: Platform-aware image load request
Caller->>K3s: LoadImagesWithPlatform(images, platform?)
K3s->>Saver: Save image (with or without platform metadata)
Saver->>Host: write tar file
Host-->>K3s: tar path
K3s->>Ctr: exec "ctr images import [--platform os/arch] <tar>"
Ctr-->>K3s: exit code + output
alt success
K3s-->Caller: success
else failure
K3s-->Caller: error (includes ctr output)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
mdelapenya
changed the title
LaurentGoderre
force-pushed
the
fix-k3s-load-images
branch
from
c482bb4 to
7236b2f
Compare
LaurentGoderre
force-pushed
the
fix-k3s-load-images
branch
3 times, most recently
from
0701b86 to
c4b42ac
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
modules/k3s/k3s.go (3)
184-187: Update comment to reflect plural images.The function accepts variadic
images ...stringbut the comment says "imports a local image" (singular). Consider updating to "imports local images" for accuracy.Apply this diff:
-// LoadImages imports a local image into the cluster using containerd +// LoadImages imports local images into the cluster using containerd func (c *K3sContainer) LoadImages(ctx context.Context, images ...string) error {
189-189: Update comment to reflect plural images.Similar to
LoadImages, this function handles multiple images but the comment says "imports a local image" (singular).Apply this diff:
-// LoadImagesWithPlatform imports a local image into the cluster using containerd for a specific platform +// LoadImagesWithPlatform imports local images into the cluster using containerd for a specific platform func (c *K3sContainer) LoadImagesWithPlatform(ctx context.Context, images []string, platform *v1.Platform) error {
233-236: Consider improving the error message clarity.The error message "importing image %s" with command output might not be immediately clear to users. Consider a more descriptive message like "failed to import images: %s" or "image import command failed: %s".
Apply this diff:
if exit != 0 { b, _ := io.ReadAll(reader) - return fmt.Errorf("importing image %s", string(b)) + return fmt.Errorf("failed to import images: %s", string(b)) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
modules/k3s/go.mod(1 hunks)modules/k3s/k3s.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
modules/k3s/k3s.go (3)
provider.go (1)
ProviderDocker(18-18)image.go (1)
SaveImageOption(19-19)docker.go (1)
SaveDockerImageWithPlatforms(1831-1837)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test (1.25.x, modules/k3s) / test: modules/k3s/1.25.x
- GitHub Check: test (1.24.x, modules/k3s) / test: modules/k3s/1.24.x
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
modules/k3s/go.mod (1)
8-8: LGTM! Dependencies correctly promoted to direct requires.Moving
containerd/platformsandopencontainers/image-specto direct dependencies aligns with the new explicit imports inmodules/k3s/k3s.go.Also applies to: 11-11
modules/k3s/k3s.go (4)
10-10: LGTM! Imports support the new platform-aware API.The new imports for
containerd/platformsandopencontainers/image-specare necessary for the platform-specific image loading functionality.Also applies to: 14-14
196-199: LGTM! Platform-specific save options correctly applied.The conditional inclusion of platform-specific save options when a platform is provided is implemented correctly.
221-228: LGTM! Command construction with optional platform flag is correct.The incremental command building with conditional
--platformflag is clear and correctly usesplatforms.Format()to format the platform specification.
210-210: Verify SaveImagesWithOpts context usage
modules/k3s/k3s.go:210 usescontext.Background()instead of the caller’sctx, so cancellation won’t propagate. Replace withctxor document why a background context is required.
LaurentGoderre
force-pushed
the
fix-k3s-load-images
branch
from
c4b42ac to
49fa463
Compare
LaurentGoderre
force-pushed
the
fix-k3s-load-images
branch
2 times, most recently
from
cd99afd to
a2d5b8c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docker.go (1)
1849-1851: Fix the option error messageThe failure text still says “save image option”; please rename it to “pull image option” so the diagnostic matches this code path.
- if err := opt(&pullOpts); err != nil { - return fmt.Errorf("applying save image option: %w", err) + if err := opt(&pullOpts); err != nil { + return fmt.Errorf("applying pull image option: %w", err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docker.go(1 hunks)image.go(2 hunks)modules/k3s/go.mod(1 hunks)modules/k3s/k3s.go(3 hunks)modules/k3s/k3s_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- modules/k3s/go.mod
🧰 Additional context used
🧬 Code graph analysis (3)
docker.go (1)
image.go (1)
PullImageOption(26-26)
modules/k3s/k3s_test.go (3)
modules/k3s/k3s.go (1)
Run(58-99)docker.go (1)
PullDockerImageWithPlatform(1857-1863)testing.go (1)
CleanupContainer(91-97)
modules/k3s/k3s.go (3)
provider.go (1)
ProviderDocker(18-18)image.go (1)
SaveImageOption(20-20)docker.go (1)
SaveDockerImageWithPlatforms(1831-1837)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint (modules/dolt) / lint: modules/dolt
- GitHub Check: lint (modules/redpanda) / lint: modules/redpanda
- GitHub Check: Analyze (go)
LaurentGoderre
force-pushed
the
fix-k3s-load-images
branch
2 times, most recently
from
15e7e07 to
f204382
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docker.go(1 hunks)image.go(2 hunks)modules/k3s/go.mod(1 hunks)modules/k3s/k3s.go(3 hunks)modules/k3s/k3s_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- modules/k3s/go.mod
🧰 Additional context used
🧬 Code graph analysis (3)
modules/k3s/k3s.go (3)
provider.go (1)
ProviderDocker(18-18)image.go (1)
SaveImageOption(20-20)docker.go (1)
SaveDockerImageWithPlatforms(1831-1837)
modules/k3s/k3s_test.go (4)
modules/k3s/k3s.go (1)
Run(58-99)docker.go (1)
PullDockerImageWithPlatform(1857-1863)testing.go (1)
CleanupContainer(91-97)provider.go (1)
ProviderDocker(18-18)
docker.go (1)
image.go (1)
PullImageOption(26-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (9)
image.go (2)
22-26: LGTM: Clean functional options pattern.The
pullImageOptionsstruct andPullImageOptionfunction type follow the same pattern as the existingSaveImageOption, maintaining consistency across the codebase.
34-34: Breaking change discussion ongoing.Adding
PullImageWithOptsto the exportedImageProviderinterface is a breaking change for downstream implementations. Based on the past review comments, this is being discussed with maintainers. The pattern is consistent with the existingSaveImagesWithOptsmethod already in the interface, but external implementations will need to add this method.If you haven't already, consider whether an optional interface pattern (as suggested in the past comments) would better preserve backward compatibility while still enabling this functionality for implementations that need it.
docker.go (1)
1857-1863: LGTM: Correct platform formatting.The factory function correctly uses
platforms.Format(platform)to convert the platform struct to the string format expected by Docker's pull API.modules/k3s/k3s_test.go (3)
43-49: Clever architecture-specific image selection.Constructing a single-architecture image tag (e.g., "amd64/nginx", "arm64v8/nginx") is a robust approach for testing scenarios where multi-arch images could mask platform-specific issues. This ensures the test exercises the actual platform-specific loading logic.
60-69: Good coverage for wrong-architecture scenario.This test verifies that attempting to load an image with a mismatched architecture fails gracefully. The regex
"content digest .* not found"matches the expected containerd error when the requested platform digest isn't present.
115-207: Comprehensive platform-aware loading tests.The new
Test_LoadImagesWithPlatformfunction provides thorough coverage of the new API:
- Non-existing images (error handling)
- Wrong architecture (platform mismatch validation with correct error message assertion)
- Successful in-cluster loading (end-to-end verification)
The past review comment about the assertion at lines 155-160 has been correctly addressed.
modules/k3s/k3s.go (3)
184-187: Good backward compatibility approach.Delegating
LoadImagestoLoadImagesWithPlatformwith a nil platform maintains the existing behavior while enabling the new functionality. This allows existing code to continue working unchanged.
189-226: Improved error handling in deprecated path.Lines 220-223 add error output capture when the
ctr importcommand fails, which significantly improves debuggability. Even though this method is deprecated, the improvement benefits users still relying on it during the transition period.
228-278: Well-structured platform-aware loading.The implementation correctly:
- Conditionally adds
SaveDockerImageWithPlatformsonly when a platform is specified (lines 236-238)- Builds the
ctr importcommand with an optional--platformflag (lines 260-266)- Uses
platforms.Format(*platform)to convert the platform struct to the string format expected by containerd (line 263)- Captures and returns command output on error for better diagnostics (lines 272-275)
The nil-check pattern allows callers to pass
nilfor default platform behavior while enabling explicit platform targeting when needed.
deprecate LoadImagesWithOpts with a new LoadImagesWithPlatform
LaurentGoderre
force-pushed
the
fix-k3s-load-images
branch
from
f204382 to
67f7ac8
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
modules/k3s/k3s_test.go (1)
117-143: Consider extracting common test setup to reduce duplication.The setup code for
Test_LoadImagesWithPlatform(lines 119-142) is nearly identical toTest_LoadImages(lines 24-42). Both create a k3s container, get kubeconfig, build a Kubernetes client, and get the Docker provider.Consider extracting this into a helper function:
func setupK3sTest(t *testing.T, ctx context.Context) (*k3s.K3sContainer, *kubernetes.Clientset, testcontainers.ContainerProvider) { t.Helper() k3sContainer, err := k3s.Run(ctx, "rancher/k3s:v1.27.1-k3s1") testcontainers.CleanupContainer(t, k3sContainer) require.NoError(t, err) kubeConfigYaml, err := k3sContainer.GetKubeConfig(ctx) require.NoError(t, err) restcfg, err := clientcmd.RESTConfigFromKubeConfig(kubeConfigYaml) require.NoError(t, err) k8s, err := kubernetes.NewForConfig(restcfg) require.NoError(t, err) provider, err := testcontainers.ProviderDocker.GetProvider() require.NoError(t, err) return k3sContainer, k8s, provider }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docker.go(1 hunks)image.go(2 hunks)modules/k3s/go.mod(1 hunks)modules/k3s/k3s.go(3 hunks)modules/k3s/k3s_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- modules/k3s/k3s.go
- docker.go
- image.go
🧰 Additional context used
🧬 Code graph analysis (1)
modules/k3s/k3s_test.go (3)
docker.go (2)
DockerProvider(977-984)PullDockerImageWithPlatform(1857-1863)testing.go (1)
CleanupContainer(91-97)provider.go (1)
ProviderDocker(18-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint (modules/grafana-lgtm) / lint: modules/grafana-lgtm
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
modules/k3s/k3s_test.go (4)
45-51: The architecture-specific image naming may be brittle.The code constructs platform-specific image names like
amd64/nginxusing the local platform's architecture. While this works for official Docker Hub images, this naming convention may not be universally supported and could fail for:
- Non-official images that don't follow this repo structure
- Platforms where Docker Hub doesn't provide architecture-specific repos
- Custom registries with different naming conventions
Consider documenting this limitation or adding a fallback mechanism if the test needs to work across a wider range of images and registries.
62-71: LGTM! Good coverage for cross-platform error case.The test correctly validates that loading an image pulled for a different platform (s390x) fails with an appropriate error message.
73-114: LGTM! Thorough validation of in-cluster loading.The test properly validates the complete flow: loading the platform-specific image into k3s, creating a pod with
PullNeverpolicy, and verifying the pod reaches running state.
167-210: LGTM! Comprehensive platform-aware loading test.The test thoroughly validates the new
LoadImagesWithPlatformAPI by loading an image with explicit platform specification and verifying it runs successfully in the cluster.modules/k3s/go.mod (1)
8-11: Dependency versions are up-to-date. containerd/platforms v0.2.1 is the latest stable release (v1.0.0-rc.1 is a pre-release) and opencontainers/image-spec v1.1.1 matches the latest.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
modules/k3s/k3s_test.go (3)
43-45: Nice: safe type assertion prevents panicsCapturing and asserting the ok result avoids nil deref when casting the provider.
118-141: Good: new platform-aware test scaffolding and safe castThe platform-aware setup and provider cast look correct.
152-167: LGTM: wrong-architecture assertion fixed as discussed previouslyFormatting with platforms.Format and asserting err.Error() contains the expected substring is correct.
🧹 Nitpick comments (4)
modules/k3s/k3s_test.go (4)
63-72: Handle Parse errors and relax brittle regex
- Check the error from platforms.Parse; ignoring it can mask typos and cause confusing failures later.
- The containerd error message can vary by version; consider a looser match to reduce flakiness.
Apply this diff:
- p, _ := platforms.Parse("linux/s390x") + p, perr := platforms.Parse("linux/s390x") + require.NoError(t, perr) @@ - require.Regexp(t, "content digest .* not found", err) + // tolerate minor wording differences across containerd versions + require.Regexp(t, `(content digest .* not found)|(no match for.*manifest)|(not found: .*digest)`, err.Error())
74-115: Good in-cluster verification; consider pinning tag for test stabilityThe flow is solid. To avoid “latest” drift over time, pin a known nginx tag (or digest) for deterministic results.
142-145: Optional: be explicit about platform on pull for symmetryUsing provider.PullImage is fine, but you could use PullImageWithOpts with platforms.DefaultSpec() for clarity and parity with the rest of the test.
146-150: Check Parse errorAvoid discarding the parse error for linux/amd64.
Apply this diff:
- p, _ := platforms.Parse("linux/amd64") + p, perr := platforms.Parse("linux/amd64") + require.NoError(t, perr)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
modules/k3s/k3s_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
modules/k3s/k3s_test.go (2)
docker.go (2)
DockerProvider(977-984)PullDockerImageWithPlatform(1857-1863)testing.go (1)
CleanupContainer(91-97)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: lint (modules/scylladb) / lint: modules/scylladb
- GitHub Check: lint (modules/nats) / lint: modules/nats
- GitHub Check: lint (modules/inbucket) / lint: modules/inbucket
- GitHub Check: lint (modules/influxdb) / lint: modules/influxdb
- GitHub Check: lint (modules/weaviate) / lint: modules/weaviate
- GitHub Check: lint (modules/gcloud) / lint: modules/gcloud
- GitHub Check: lint (modules/milvus) / lint: modules/milvus
- GitHub Check: lint (modules/redpanda) / lint: modules/redpanda
- GitHub Check: lint (modules/postgres) / lint: modules/postgres
- GitHub Check: lint (modules/mariadb) / lint: modules/mariadb
- GitHub Check: lint (modules/clickhouse) / lint: modules/clickhouse
- GitHub Check: lint (modules/k3s) / lint: modules/k3s
- GitHub Check: lint (modules/valkey) / lint: modules/valkey
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
modules/k3s/k3s_test.go (3)
9-9: Import looks goodBrings in containerd/platforms needed for platform formatting/parsing.
55-55: Pull is fineExplicit pre-pull of the single-arch ref keeps the subsequent import path deterministic.
170-173: LGTM: explicit linux platform for in-cluster loadForcing OS=linux on the default spec is appropriate here.
| // This function only works for single architecture images | ||
| // Forces the test to use a single-arch version of the image | ||
| arch := platforms.DefaultSpec().Architecture | ||
| if platforms.DefaultSpec().Variant != "" { | ||
| arch += platforms.DefaultSpec().Variant | ||
| } | ||
| nginxImg := arch + "/nginx" | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Fix arch-specific repo derivation for non-amd64 (arm/arm64 fail today)
Docker Hub uses prefixes like arm32v7/nginx and arm64v8/nginx, not armv7/nginx or arm64/nginx. The current concatenation will 404 on many hosts. Replace with a small mapping and skip unsupported variants to keep the test portable.
Apply this diff:
- // This function only works for single architecture images
- // Forces the test to use a single-arch version of the image
- arch := platforms.DefaultSpec().Architecture
- if platforms.DefaultSpec().Variant != "" {
- arch += platforms.DefaultSpec().Variant
- }
- nginxImg := arch + "/nginx"
+ // LoadImages (legacy) works with single‑arch images only.
+ // Use the Docker Library arch-specific repos that actually exist.
+ spec := platforms.DefaultSpec()
+ var repoPrefix string
+ switch spec.Architecture {
+ case "amd64":
+ repoPrefix = "amd64"
+ case "arm64":
+ repoPrefix = "arm64v8"
+ case "arm":
+ switch spec.Variant {
+ case "v7":
+ repoPrefix = "arm32v7"
+ case "v6":
+ repoPrefix = "arm32v6"
+ default:
+ t.Skipf("no single-arch nginx repo for %s", platforms.Format(spec))
+ }
+ case "ppc64le":
+ repoPrefix = "ppc64le"
+ case "s390x":
+ repoPrefix = "s390x"
+ default:
+ t.Skipf("no single-arch nginx repo for %s", platforms.Format(spec))
+ }
+ nginxImg := fmt.Sprintf("%s/nginx", repoPrefix)Optional: Add an arm64/amd64 CI job matrix to surface regressions early.
[/run_scripts]
[/web_search]
🏁 Script executed:
#!/bin/bash
# Verify the file exists and check current code at lines 46-53
fd "k3s_test.go" --type fLength of output: 23
🏁 Script executed:
#!/bin/bash
# Check the exact code at the specified lines
if [ -f "modules/k3s/k3s_test.go" ]; then
sed -n '46,53p' "modules/k3s/k3s_test.go"
fiLength of output: 286
🏁 Script executed:
#!/bin/bash
# Search for similar arch derivation patterns in the codebase
rg -n "platforms\.DefaultSpec\(\)" --type go -C 3Length of output: 939
🏁 Script executed:
#!/bin/bash
# Check imports in the k3s_test.go to see what packages are available
if [ -f "modules/k3s/k3s_test.go" ]; then
head -30 "modules/k3s/k3s_test.go"
fiLength of output: 815
🌐 Web query:
Docker Hub official nginx image repository naming conventions arm32v7 arm64v8
💡 Result:
Short answer
- Official Docker "nginx" uses multi-arch manifest lists so a plain docker pull nginx:TAG resolves the correct architecture automatically. (github.com) [1][2]
How architecture-specific names work
- Legacy/explicit per-arch repositories: images for specific ARM families have historically been published under prefixed namespaces such as arm32v7/ and arm64v8/ (e.g., arm32v7/nginx or arm64v8/nginx) when you want to pull a specific architecture. (github.com) [1][3]
- Tag/label conventions: community images sometimes append the arch in the tag (or use tags like :arm64v8 or :arm32v7), but official images prefer manifest lists (no arch suffix needed). (docker.com) [2][4]
Practical guidance
- To let Docker choose the right image for your host: docker pull nginx[:TAG]. (github.com) [1]
- To force a specific arch (explicit): docker pull arm64v8/nginx[:TAG] or docker pull arm32v7/nginx[:TAG]. (github.com) [1][3]
Sources
[1] docker-library/official-images (architectures, manifest behavior). (github.com)
[2] Docker blog — multi-arch build & manifest example. (docker.com)
[3] ROS / Docker examples & arm-prefixed repos (usage of arm32v7/arm64v8 namespaces). (discourse.ros.org)
[4] Multi-arch build/tagging examples (community docs). (lobradov.github.io)
Citations:
- 1: https://github.com/docker-library/official-images?utm_source=openai
- 2: https://github.com/docker-library/official-images?utm_source=openai
- 3: https://www.docker.com/blog/multi-arch-build-and-images-the-simple-way/?utm_source=openai
- 4: https://github.com/docker-library/official-images?utm_source=openai
- 5: https://github.com/docker-library/official-images?utm_source=openai
- 6: https://github.com/docker-library/official-images?utm_source=openai
- 7: https://www.docker.com/blog/multi-arch-build-and-images-the-simple-way/?utm_source=openai
- 8: https://discourse.ros.org/t/announcing-ros-docker-images-for-arm-and-debian/2467?utm_source=openai
- 9: https://lobradov.github.io/Building-docker-multiarch-images/?utm_source=openai
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Search for platforms.Format usage
rg -n "platforms\.Format" --type go -C 2Length of output: 801
🏁 Script executed:
#!/bin/bash
# Search for t.Skip usage in test files
rg -n "t\.Skip" --type go -C 2Length of output: 10505
🏁 Script executed:
#!/bin/bash
# Look for other arch/image derivation patterns in k3s module
rg -n "Architecture|Variant" modules/k3s/ --type go -C 3Length of output: 530
🏁 Script executed:
#!/bin/bash
# Check the containerd/platforms package usage and imports more broadly
rg -n "platforms\." modules/k3s/k3s_test.go | head -20Length of output: 415
Fix arch-specific Docker Hub repo prefixes (arm/arm64 fail today on 404s)
Docker Hub's official repository uses legacy/explicit per-arch repositories such as arm32v7/ and arm64v8/ when pulling a specific architecture. The current code concatenates architecture strings directly, producing invalid repo names like arm64/nginx and armv7/nginx that don't exist on Docker Hub. This will cause test failures on any non-amd64 platform.
Replace lines 46–52 with the suggested diff to map each architecture to its correct Docker Hub prefix and skip unsupported variants:
- // This function only works for single architecture images
- // Forces the test to use a single-arch version of the image
- arch := platforms.DefaultSpec().Architecture
- if platforms.DefaultSpec().Variant != "" {
- arch += platforms.DefaultSpec().Variant
- }
- nginxImg := arch + "/nginx"
+ // LoadImages (legacy) works with single‑arch images only.
+ // Use the Docker Library arch-specific repos that actually exist.
+ spec := platforms.DefaultSpec()
+ var repoPrefix string
+ switch spec.Architecture {
+ case "amd64":
+ repoPrefix = "amd64"
+ case "arm64":
+ repoPrefix = "arm64v8"
+ case "arm":
+ switch spec.Variant {
+ case "v7":
+ repoPrefix = "arm32v7"
+ case "v6":
+ repoPrefix = "arm32v6"
+ default:
+ t.Skipf("no single-arch nginx repo for %s", platforms.Format(spec))
+ }
+ case "ppc64le":
+ repoPrefix = "ppc64le"
+ case "s390x":
+ repoPrefix = "s390x"
+ default:
+ t.Skipf("no single-arch nginx repo for %s", platforms.Format(spec))
+ }
+ nginxImg := fmt.Sprintf("%s/nginx", repoPrefix)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // This function only works for single architecture images | |
| // Forces the test to use a single-arch version of the image | |
| arch := platforms.DefaultSpec().Architecture | |
| if platforms.DefaultSpec().Variant != "" { | |
| arch += platforms.DefaultSpec().Variant | |
| } | |
| nginxImg := arch + "/nginx" | |
| // LoadImages (legacy) works with single‑arch images only. | |
| // Use the Docker Library arch-specific repos that actually exist. | |
| spec := platforms.DefaultSpec() | |
| var repoPrefix string | |
| switch spec.Architecture { | |
| case "amd64": | |
| repoPrefix = "amd64" | |
| case "arm64": | |
| repoPrefix = "arm64v8" | |
| case "arm": | |
| switch spec.Variant { | |
| case "v7": | |
| repoPrefix = "arm32v7" | |
| case "v6": | |
| repoPrefix = "arm32v6" | |
| default: | |
| t.Skipf("no single-arch nginx repo for %s", platforms.Format(spec)) | |
| } | |
| case "ppc64le": | |
| repoPrefix = "ppc64le" | |
| case "s390x": | |
| repoPrefix = "s390x" | |
| default: | |
| t.Skipf("no single-arch nginx repo for %s", platforms.Format(spec)) | |
| } | |
| nginxImg := fmt.Sprintf("%s/nginx", repoPrefix) |
🤖 Prompt for AI Agents
In modules/k3s/k3s_test.go around lines 46 to 53, the test builds image names by
concatenating platform architecture/variant, producing invalid Docker Hub repo
prefixes (e.g., "arm64/nginx"); update this section to map architectures to
Docker Hub legacy prefixes (e.g., amd64 -> "nginx", arm -> "arm32v7/nginx",
arm64 -> "arm64v8/nginx") and skip unsupported variants (return or t.Skip for
unknown variants), so the test pulls the correct per-arch image names; replace
lines 46–53 with logic that reads platforms.DefaultSpec(), switches on
Architecture (and Variant when relevant), sets nginxImg to the mapped prefix +
"/nginx" (or "nginx" for amd64), and skips the test for unsupported
combinations.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
modules/k3s/k3s_test.go (1)
46-52: Duplicate: Fix arch-specific Docker Hub repo prefixes.This issue was already flagged in a previous review. The current code produces invalid Docker Hub repository names (e.g., "arm64/nginx" instead of "arm64v8/nginx"), causing 404 errors on non-amd64 platforms.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
modules/k3s/k3s_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
modules/k3s/k3s_test.go (2)
docker.go (2)
DockerProvider(977-984)PullDockerImageWithPlatform(1857-1863)testing.go (1)
CleanupContainer(91-97)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint (modules/cockroachdb) / lint: modules/cockroachdb
- GitHub Check: lint (modules/memcached) / lint: modules/memcached
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
modules/k3s/k3s_test.go (5)
43-44: LGTM! Type assertion now properly checked.The type assertion correctly captures the boolean result and validates it before use, preventing potential nil pointer panics.
63-73: LGTM! Wrong architecture test is correctly structured.The test appropriately validates that the legacy
LoadImagesAPI fails when attempting to load an image that only has a specific platform digest available locally.
75-117: LGTM! In-cluster image loading test is well-structured.The test correctly validates the complete flow: loading an image into the cluster, creating a Pod with
PullNever, and verifying it reaches the Running state. Note that this test depends on the arch-specific image name from lines 46-52 being corrected.
140-141: LGTM! Type assertion properly validated.The type assertion is now handled safely with proper boolean checking.
171-214: LGTM! Platform-aware image loading test is correctly implemented.The test properly uses
LoadImagesWithPlatformwith the default platform spec, ensuring the correct architecture-specific image is loaded and can be used in the cluster withPullNever.
| t.Run("Test load image with wrong architecture", func(t *testing.T) { | ||
| pullPlatform, _ := platforms.Parse("linux/s390x") | ||
| img := "nginx:mainline" | ||
| err = dockerProvider.PullImageWithOpts(ctx, img, testcontainers.PullDockerImageWithPlatform(pullPlatform)) | ||
| require.NoError(t, err) | ||
|
|
||
| loadPlatform, _ := platforms.Parse("linux/amd64") | ||
| err := k3sContainer.LoadImagesWithPlatform(ctx, []string{img}, &loadPlatform) | ||
| require.Error(t, err) | ||
| expected := fmt.Sprintf( | ||
| "image with reference %s was found but does not provide the specified platform (%s)", | ||
| img, | ||
| platforms.Format(loadPlatform), | ||
| ) | ||
| require.Contains(t, err.Error(), expected) | ||
| }) |
There was a problem hiding this comment.
Check error returns from platforms.Parse.
Lines 155 and 160 ignore the error return from platforms.Parse. While parsing these well-formed platform strings is unlikely to fail, omitting the error check can cause nil pointer dereferences if parsing fails unexpectedly.
Apply this diff to add error checking:
- pullPlatform, _ := platforms.Parse("linux/s390x")
+ pullPlatform, err := platforms.Parse("linux/s390x")
+ require.NoError(t, err)
img := "nginx:mainline"
err = dockerProvider.PullImageWithOpts(ctx, img, testcontainers.PullDockerImageWithPlatform(pullPlatform))
require.NoError(t, err)
- loadPlatform, _ := platforms.Parse("linux/amd64")
- err := k3sContainer.LoadImagesWithPlatform(ctx, []string{img}, &loadPlatform)
+ loadPlatform, err := platforms.Parse("linux/amd64")
+ require.NoError(t, err)
+ err = k3sContainer.LoadImagesWithPlatform(ctx, []string{img}, &loadPlatform)
require.Error(t, err)🤖 Prompt for AI Agents
In modules/k3s/k3s_test.go around lines 154 to 169, platforms.Parse calls ignore
their error returns which can cause nil pointer dereferences; change the code to
capture both return values and assert on the error (e.g., require.NoError(t,
err) or t.Fatalf) immediately after each platforms.Parse call before using the
parsed platform value so the test fails fast and avoids dereferencing a nil
platform.
2 participants
What does this PR do?
This PR adds more robust error handling around image loading and replaces the LoadImagesWithOpt that uses
--all-platformto aLoadImagesWithPlatformfunction that allows loading the platform correctly.This is required because
--all-platformfails when some digests for some platform are missing.Why is it important?
This is important because loading an image without specifying a platform is now broken.