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 loading and pulling: a new K3s API LoadImagesWithPlatform, platform-capable save/import flow with improved error output, Docker pull options supporting target platform, platform-aware tests, and dependency updates for containerd/platforms and image-spec. 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.
| provider, err := testcontainers.ProviderDocker.GetProvider() | ||
| require.NoError(t, err) | ||
|
|
||
| dockerProvider, _ := provider.(*testcontainers.DockerProvider) |
There was a problem hiding this comment.
Check the type assertion result to prevent potential panics.
The type assertion ignores the boolean return value. If the assertion fails, dockerProvider will be nil, and the call to dockerProvider.PullImageWithOpts on line 65 will panic.
Apply this diff to handle the assertion safely:
- dockerProvider, _ := provider.(*testcontainers.DockerProvider)
+ dockerProvider, ok := provider.(*testcontainers.DockerProvider)
+ require.True(t, ok, "provider must be a DockerProvider")📝 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.
| dockerProvider, _ := provider.(*testcontainers.DockerProvider) | |
| dockerProvider, ok := provider.(*testcontainers.DockerProvider) | |
| require.True(t, ok, "provider must be a DockerProvider") |
🤖 Prompt for AI Agents
In modules/k3s/k3s_test.go around line 43, the type assertion for
provider.(*testcontainers.DockerProvider) ignores the boolean result and can
yield a nil value leading to a panic when calling
dockerProvider.PullImageWithOpts; change the code to use the two-value form (dp,
ok := provider.(*testcontainers.DockerProvider)), check ok, and if false fail
the test or return an error (e.g. t.Fatalf or t.Fatalf with context) so
subsequent calls safely use the typed variable.
| provider, err := testcontainers.ProviderDocker.GetProvider() | ||
| require.NoError(t, err) | ||
|
|
||
| dockerProvider, _ := provider.(*testcontainers.DockerProvider) |
There was a problem hiding this comment.
Check the type assertion result to prevent potential panics.
The type assertion ignores the boolean return value. If the assertion fails, dockerProvider will be nil, and the call to dockerProvider.PullImageWithOpts on line 153 will panic.
Apply this diff to handle the assertion safely:
- dockerProvider, _ := provider.(*testcontainers.DockerProvider)
+ dockerProvider, ok := provider.(*testcontainers.DockerProvider)
+ require.True(t, ok, "provider must be a DockerProvider")📝 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.
| dockerProvider, _ := provider.(*testcontainers.DockerProvider) | |
| dockerProvider, ok := provider.(*testcontainers.DockerProvider) | |
| require.True(t, ok, "provider must be a DockerProvider") |
🤖 Prompt for AI Agents
In modules/k3s/k3s_test.go around line 138, the type assertion that casts
provider to *testcontainers.DockerProvider ignores the boolean result and may
yield a nil dockerProvider causing a panic later; update the code to capture the
assertion boolean (e.g., dockerProvider, ok :=
provider.(*testcontainers.DockerProvider)) and handle the failure immediately
(return an error or fail the test with a clear message) so subsequent calls like
dockerProvider.PullImageWithOpts are safe.
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.