Fix Platform Compatibility Check for Locally Cached Docker Images #4868
Conversation
WalkthroughThe changes update the Docker client to better handle platform compatibility. A new constant Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Logger
participant Registry
Client->>Client: getHostPlatform(ctx)
Note over Client: Retrieves and caches host platform info
Client->>Client: isPlatformCompatible(imageInfo, hostPlatform)
alt Platform compatible
Client->>Registry: Proceed with ImageDistribution
else Platform mismatch
Client->>Logger: Log debug message for platform mismatch
end
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.62.2){"Issues":[{"FromLinter":"typecheck","Text":"pattern build/: no matching files found","Severity":"","SourceLines":["//go:embed build/"],"Replacement":null,"Pos":{"Filename":"webui/webui.go","Offset":0,"Line":23,"Column":12},"ExpectNoLint":false,"ExpectedNoLintLinter":""}],"Report":{"Linters":[{"Name":"asasalint"},{"Name":"asciicheck"},{"Name":"bidichk"},{"Name":"bodyclose","Enabled":true},{"Name":"canonicalheader"},{"Name":"containedctx"},{"Name":"contextcheck"},{"Name":"copyloopvar","Enabled":true},{"Name":"cyclop"},{"Name":"decorder"},{"Name":"deadcode"},{"Name":"depguard","Enabled":true},{"Name":"dogsled","Enabled":true},{"Name":"dupl"},{"Name":"dupword"},{"Name":"durationcheck"},{"Name":"errcheck","Enabled":true,"EnabledByDefault":true},{"Name":"errchkjson"},{"Name":"errname"},{"Name":"errorlint"},{"Name":"execinquery"},{"Name":"exhaustive"},{"Name":"exhaustivestruct"},{"Name":"exhaustruct"},{"Name":"exportloopref"},{"Name":"forbidigo","Enabled":true},{"Name":"forcetypeassert"},{"Name":"fatcontext"},{"Name":"funlen","Enabled":true},{"Name":"gci"},{"Name":"ginkgolinter"},{"Name":"gocheckcompilerdirectives"},{"Name":"gochecknoglobals"},{"Name":"gochecknoinits","Enabled":true},{"Name":"gochecksumtype"},{"Name":"gocognit"},{"Name":"goconst","Enabled":true},{"Name":"gocritic"},{"Name":"gocyclo","Enabled":true},{"Name":"godot"},{"Name":"godox"},{"Name":"err113"},{"Name":"gofmt","Enabled":true},{"Name":"gofumpt"},{"Name":"goheader"},{"Name":"goimports","Enabled":true},{"Name":"golint"},{"Name":"mnd","Enabled":true},{"Name":"gomnd"},{"Name":"gomoddirectives"},{"Name":"gomodguard"},{"Name":"goprintffuncname","Enabled":true},{"Name":"gosec","Enabled":true},{"Name":"gosimple","Enabled":true,"EnabledByDefault":true},{"Name":"gosmopolitan"},{"Name":"govet","Enabled":true,"EnabledByDefault":true},{"Name":"grouper"},{"Name":"ifshort"},{"Name":"iface"},{"Name":"importas"},{"Name":"inamedparam"},{"Name":"ineffassign","Enabled":true,"EnabledByDefault":true},{"Name":"interfacebloat"},{"Name":"interfacer"},{"Name":"intrange"},{"Name":"ireturn"},{"Name":"lll","Enabled":true},{"Name":"loggercheck"},{"Name":"maintidx"},{"Name":"makezero"},{"Name":"maligned"},{"Name":"mirror"},{"Name":"misspell"},{"Name":"musttag"},{"Name":"nakedret","Enabled":true},{"Name":"nestif"},{"Name":"nilerr"},{"Name":"nilnil"},{"Name":"nlreturn"},{"Name":"noctx","Enabled":true},{"Name":"nonamedreturns"},{"Name":"nosnakecase"},{"Name":"nosprintfhostport"},{"Name":"paralleltest"},{"Name":"perfsprint"},{"Name":"prealloc"},{"Name":"predeclared"},{"Name":"promlinter"},{"Name":"protogetter"},{"Name":"reassign"},{"Name":"recvcheck"},{"Name":"revive"},{"Name":"rowserrcheck"},{"Name":"sloglint"},{"Name":"scopelint"},{"Name":"sqlclosecheck"},{"Name":"spancheck"},{"Name":"staticcheck","Enabled":true,"EnabledByDefault":true},{"Name":"structcheck"},{"Name":"stylecheck","Enabled":true},{"Name":"tagalign"},{"Name":"tagliatelle"},{"Name":"tenv"},{"Name":"testableexamples"},{"Name":"testifylint"},{"Name":"testpackage"},{"Name":"thelper"},{"Name":"tparallel"},{"Name":"typecheck","Enabled":true,"EnabledByDefault":true},{"Name":"unconvert","Enabled":true},{"Name":"unparam"},{"Name":"unused","Enabled":true,"EnabledByDefault":true},{"Name":"usestdlibvars"},{"Name":"varcheck"},{"Name":"varnamelen"},{"Name":"wastedassign"},{"Name":"whitespace","Enabled":true},{"Name":"wrapcheck"},{"Name":"wsl"},{"Name":"zerologlint"},{"Name":"nolintlint","Enabled":true}]}} ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
pkg/executor/docker/bidstrategy/semantic/image_platform.go (2)
76-82: Consider making cache duration configurable
The in-memory caching approach with a fixed duration is workable but somewhat inflexible. You might consider making the cache TTL user-configurable or rely on a more robust cache mechanism as you indicated in the TODO.I can open a new issue or provide a patch if you'd like to incorporate a more dynamic caching approach (e.g., LRU with user-configurable TTL).
85-91: Minimal fallback if caching fails
Logging and continuing is user-friendly in a best-effort scenario, but repeated failures may degrade performance if images are not cached at all. Consider adding a fallback strategy or periodic reattempt.pkg/docker/docker.go (2)
256-268: Eliminate repeated 'unknown' string usage
The static analysis warns about repeated usage of the string 'unknown'. Consider extracting it into a shared constant to improve maintainability.Here's an example diff:
+const unknownStr = "unknown" func (c *Client) isPlatformCompatible(info types.ImageInspect, hostPlatform v1.Platform) bool { - if info.Os == "unknown" || info.Architecture == "unknown" { + if info.Os == unknownStr || info.Architecture == unknownStr { log.Debug()... return false } ... }🧰 Tools
🪛 GitHub Check: lint / go-lint (ubuntu-latest)
[failure] 258-258:
stringunknownhas 4 occurrences, make it a constant (goconst)🪛 GitHub Actions: Main Pipeline
[warning] 258-258: string
unknownhas 4 occurrences, make it a constant (goconst)
358-363: Unnecessary leading newline
A linter flags an unnecessary leading newline around line 356. Consider removing extra blank lines to comply with style guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/docker/docker.go(5 hunks)pkg/executor/docker/bidstrategy/semantic/image_platform.go(3 hunks)
🧰 Additional context used
🪛 GitHub Check: lint / go-lint (ubuntu-latest)
pkg/docker/docker.go
[failure] 258-258:
string unknown has 4 occurrences, make it a constant (goconst)
[failure] 356-356:
unnecessary leading newline (whitespace)
🪛 GitHub Actions: Main Pipeline
pkg/docker/docker.go
[warning] 258-258: string unknown has 4 occurrences, make it a constant (goconst)
🔇 Additional comments (25)
pkg/executor/docker/bidstrategy/semantic/image_platform.go (7)
19-19: One-hour caching constant is well-chosen
Having a shorter caching duration (one hour) can help ensure more up-to-date platform information, especially if your environment changes frequently.
62-65: No issues found.
67-67: No issues found.
94-94: No issues found.
105-105: No issues found.
111-111: No issues found.
115-116: No issues found.pkg/docker/docker.go (18)
41-43: Good concurrency design for caching
These new fields for hostPlatform, platformSet, and platformMu effectively manage concurrency around retrieving host platform data.
52-52: No issues found.
210-246: Synchronized Host Platform Retrieval
This function uses a mutex to safely cache the host platform. The logic is clear and avoids repeated calls to Docker. Looks good.
248-254: Single-element slice is straightforward
Returning just one platform from getHostPlatform is logical for local runs.
324-327: No issues found.
329-355: Local image digest parsing
Parsing the digest from the first entry in repoDigests is a practical solution, ensuring minimal overhead.
364-366: No issues found.
368-368: No issues found.
375-386: No issues found.
389-389: No issues found.
390-390: No issues found.
395-395: No issues found.
397-401: No issues found.
403-403: No issues found.
404-405: No issues found.
407-407: No issues found.
410-410: No issues found.
524-526: No issues found.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/docker/docker.go (1)
326-329: Consider additional logging for empty repository digestsThe implementation for checking local image compatibility is good, but there's a potential edge case where
info.RepoDigestsmight be empty, which would cause the function to silently continue to the registry check without any logging or notification.Consider adding a debug log when repository digests are empty:
if c.isPlatformCompatible(info, hostPlatform) { repos := info.RepoDigests if len(repos) >= 1 { // ...existing code... + } else { + log.Ctx(ctx).Debug(). + Str("image", image). + Msg("Local image has compatible platform but no repository digests, checking registry") } }Also applies to: 335-357
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/docker/docker.go(5 hunks)
🔇 Additional comments (10)
pkg/docker/docker.go (10)
39-39: Well-defined constant for unknown platform valuesGood addition - using a named constant instead of magic strings improves code readability and prevents typo-based errors when dealing with unknown platform values.
43-45: Good thread-safe implementation for caching platform informationThe addition of these fields to cache host platform information with proper mutex synchronization is a solid approach. This prevents redundant calls to the Docker daemon and ensures thread safety when multiple goroutines access platform information.
212-248: Well-structured platform detection with proper error handlingThis method effectively retrieves and caches the host platform information with good error handling and thread safety. The step-by-step approach to extract platform details is clear and robust.
Some additional points to note:
- The use of a mutex ensures thread-safe access to cached platform information
- The error messages are specific and informative
- The cache mechanism prevents redundant Docker API calls
250-256: Refactored to use the new centralized platform detectionGood refactoring to leverage the new
getHostPlatformmethod instead of duplicating platform detection logic.
258-270: Effective platform compatibility check with debug loggingThe
isPlatformCompatiblemethod provides a clear and effective way to determine if an image's platform matches the host platform. The debug logging for unknown platform values is helpful for troubleshooting.
358-364: Good debugging for platform mismatchesThe debug logging when a platform mismatch is detected provides valuable information for troubleshooting. The use of
platformStringfor consistent formatting is a nice touch.
376-382: Improved platform filtering to prevent issues with unknown platformsFiltering out platforms with unknown values is an important improvement that prevents potential problems with incomplete platform information from the registry.
391-406: Platform compatibility check before pulling imagesThis change ensures that Docker images are only pulled when necessary - either when they don't exist locally or when the local version doesn't match the host platform. This optimization reduces unnecessary network traffic and improves performance.
408-412: Explicit platform specification in pull optionsUsing the
platformStringhelper to set the platform in pull options ensures that the correct platform-specific image is pulled. This is crucial for preventing issues with multi-architecture images.
525-527: Useful helper for consistent platform string formattingThis simple helper function ensures consistent formatting of platform information throughout the codebase, which is good for maintainability and readability.
Overview
This PR addresses a specific issue with platform compatibility verification for Docker images that are already cached locally. The fix ensures that Bacalhau properly verifies platform compatibility before using locally available Docker images.
Problem
We identified a specific issue in our Docker image handling logic:
When a Docker image is already pulled and available in the local cache:
This issue only occurred when incompatible images were already present in the local Docker cache. The remote registry verification was working correctly when no local image was available.
Solution
This PR implements a targeted fix:
isPlatformCompatiblecheck to properly verify locally cached images against the host platformHow to Reproduce the Issue
The issue can be reproduced with these steps:
Summary by CodeRabbit
New Features
Chores