Fix subnets and the common error: Pool overlaping #149
Conversation
WalkthroughAfter creating a Docker network, the code now re-inspects the network to capture Docker-assigned IPAM results, propagates actual IPv6/IPv4 Subnet and Gateway values into runtime interfaces, and retries network creation without explicit IPAM when Docker reports pool-overlap/invalid-pool errors. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
NaysKutzu
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
environment/docker.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
environment/docker.go (2)
config/config.go (1)
Get(421-429)server/filesystem/errors.go (1)
Error(25-37)
⏰ 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: Build and Test (ubuntu-22.04, 1.25.1, linux, amd64)
- GitHub Check: Build and Test (ubuntu-22.04, 1.24.7, linux, amd64)
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
environment/docker.go (3)
52-57: LGTM: Re-inspecting the network after creation ensures configuration accuracy.This correctly captures the actual IPAM settings Docker assigned, especially important when the retry path uses auto-assigned subnets.
100-115: LGTM: IPAM configuration building is correct.The logic properly builds IPAM configs from the configured subnets, handling IPv4 and optional IPv6 appropriately.
136-158: Based on my research, I can now provide you with the rewritten review comment:
Error message substring matching is a pragmatic and stable approach; no better alternative exists in the Docker Go client.
The error messages
"Pool overlaps"and"invalid pool request"originate from Docker's libnetwork IPAM error constants and have remained stable across Docker versions 20–27. The Go docker SDK does not expose a typed error for pool-overlap conditions, so string matching onerr.Error()is the documented and recommended pattern (see Docker Go client examples and moby/moby issue discussions). The substring-based approach (strings.Contains()) is actually more robust than exact matching, as it tolerates minor wording variations in the daemon message. The implementation follows best practices for this scenario.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
environment/docker.go (1)
141-143: String-based error detection is fragile.Docker error messages may vary across versions or locales. Consider using case-insensitive matching for more robustness.
// Check if the error is a pool overlap issue errStr := err.Error() - if strings.Contains(errStr, "Pool overlaps") || strings.Contains(errStr, "invalid pool request") { + errLower := strings.ToLower(errStr) + if strings.Contains(errLower, "pool overlaps") || strings.Contains(errLower, "invalid pool request") {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
environment/docker.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
environment/docker.go (1)
config/config.go (1)
Get(421-429)
⏰ 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: Build and Test (ubuntu-22.04, 1.24.7, linux, amd64)
- GitHub Check: Build and Test (ubuntu-22.04, 1.25.1, linux, amd64)
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
environment/docker.go (3)
52-57: LGTM!Re-inspecting the network after creation is the correct approach to obtain Docker-assigned values, especially when the retry logic falls back to auto-assignment.
74-93: Previous review concern addressed.The iteration-based approach correctly handles Docker's unordered IPAM config by detecting address family via colon presence. This is robust since IPv6 addresses always contain colons while IPv4 addresses never do.
98-164: Retry logic for subnet conflicts looks good.The approach of falling back to Docker auto-assignment when configured subnets conflict is a practical solution for the migration scenario described in the PR. The error handling is appropriate, and the flow is clear.
No, not true those kind of errors are the same :/ |
…host, overlay, weavemesh) that don't use standard IPAM configs.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
environment/docker.go (1)
145-146: Error string matching is case-sensitive and fragile.Matching exact error-string fragments can break if Docker changes message wording across versions or locales. While I see from PR comments that you disagree, consider at minimum using case-insensitive matching for resilience:
- errStr := err.Error() - if strings.Contains(errStr, "Pool overlaps") || strings.Contains(errStr, "invalid pool request") { + errStr := strings.ToLower(err.Error()) + if strings.Contains(errStr, "pool overlaps") || strings.Contains(errStr, "invalid pool request") {Alternatively, Docker client errors often implement specific types or error codes that could be checked instead of string matching.
Docker SDK Go network create error types for pool overlap
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
environment/docker.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
environment/docker.go (1)
config/config.go (1)
Get(421-429)
⏰ 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: Analyze (go)
- GitHub Check: Build and Test (ubuntu-22.04, 1.24.7, linux, amd64)
- GitHub Check: Build and Test (ubuntu-22.04, 1.25.1, linux, amd64)
🔇 Additional comments (4)
environment/docker.go (4)
52-57: LGTM!Re-inspecting the network after creation ensures the configuration reflects actual Docker-assigned values. Error handling with context is properly implemented.
74-96: LGTM - correctly handles non-deterministic IPAM config ordering.The iteration approach with colon-based IPv6 detection properly addresses the previous review concern about Docker not guaranteeing IPAM config order. The driver exclusion list is consistent with the switch statement above.
109-121: LGTM!IPAM config building correctly includes only non-empty subnets, with IPv6 config added only when IPv6 is enabled.
149-160: Retry logic is sound.Clearing the IPAM config on pool overlap and letting Docker auto-assign is a practical fallback. The log messages clearly communicate what's happening.
|
@NaysKutzu Will this update the wings config with the new ip range once it creates properly? |
yes |
3 participants
Yes, tested in FeatherWings by around 400 hosts migrating from pterodactyl wings
Summary by CodeRabbit
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.