feat: release IPAM subnet on network delete to avoid range exhaustion#276
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis change introduces a new subnet release capability in the IPAM service. It adds a public ReleaseSubnet method with retry logic, a helper Contains method on SubnetRange for validation, and integrates the release mechanism into network removal operations. Changes
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/installation/configuration.md`:
- Around line 49-59: Clarify the contradictory guidance about changing
docker_swarm.database_networks_cidr and database_networks_subnet_bits:
explicitly state that these values must not be changed after any databases are
created (or only may be changed if all databases using the old range have been
deleted and re-created), and add a prominent warning/admonition callout
indicating the risk and required steps (delete databases or perform full
migration) before changing; also specify what the CIDR must not overlap with
(e.g., host Docker bridge networks, other Docker Swarm IPAM ranges, or any
on-prem/VPC networks) and note that the Control Plane restart is required only
when performed before creating databases or after completing the safe migration
steps.
In `@server/internal/ipam/service.go`:
- Around line 56-71: The loop in releaseSubnet uses releaseMaxRetries with for
retries := releaseMaxRetries; retries >= 0; retries-- which makes 1 +
releaseMaxRetries attempts (e.g. releaseMaxRetries=2 -> 3 attempts) and is
inconsistent with allocateSubnet's semantics; fix by either renaming the
constant to reflect attempts (e.g. releaseMaxAttempts = 3) and keep the loop, or
keep releaseMaxRetries and change the loop to run retries > 0 (so it performs 1
initial attempt plus releaseMaxRetries retries); update the declaration of
releaseMaxRetries/releaseMaxAttempts and the loop around s.releaseSubnet(ctx,
prefix, bits, subnet) accordingly to restore consistent semantics and naming.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/installation/configuration.mdserver/internal/ipam/service.goserver/internal/ipam/subnet.goserver/internal/orchestrator/swarm/network.go
docs/installation/configuration.md
Outdated
|
|
||
| To avoid subnet range conflicts, use a non-overlapping CIDR. Restart the Control Plane after changing. | ||
|
|
||
| ```json | ||
| { | ||
| "docker_swarm": { | ||
| "database_networks_cidr": "10.129.0.0/18", | ||
| "database_networks_subnet_bits": 26 | ||
| } | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Contradictory guidance: table says "Must not be changed after creating databases" but the note encourages changing CIDR.
Lines 46–47 state the CIDR and subnet bits "Must not be changed after creating databases," yet this new note advises users to switch to a different CIDR and restart. This will confuse operators—clarify when this workaround is safe (e.g., only before any databases exist, or only after all databases on the old range are deleted) and consider adding a warning/admonition callout for visibility.
Also, the note doesn't explain what the CIDR should not overlap with (host Docker networks? other IPAM ranges?). A brief clarification would help.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/installation/configuration.md` around lines 49 - 59, Clarify the
contradictory guidance about changing docker_swarm.database_networks_cidr and
database_networks_subnet_bits: explicitly state that these values must not be
changed after any databases are created (or only may be changed if all databases
using the old range have been deleted and re-created), and add a prominent
warning/admonition callout indicating the risk and required steps (delete
databases or perform full migration) before changing; also specify what the CIDR
must not overlap with (e.g., host Docker bridge networks, other Docker Swarm
IPAM ranges, or any on-prem/VPC networks) and note that the Control Plane
restart is required only when performed before creating databases or after
completing the safe migration steps.
There was a problem hiding this comment.
@moizpgedge could you please remove this advice? Coderabbit is right that this is contradictory with our existing documentation.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
jason-lynch
left a comment
jason-lynch
left a comment
There was a problem hiding this comment.
Nice! I think this is looking good. It should be good to merge once you've addressed the CodeRabbit comments.
docs/installation/configuration.md
Outdated
|
|
||
| To avoid subnet range conflicts, use a non-overlapping CIDR. Restart the Control Plane after changing. | ||
|
|
||
| ```json | ||
| { | ||
| "docker_swarm": { | ||
| "database_networks_cidr": "10.129.0.0/18", | ||
| "database_networks_subnet_bits": 26 | ||
| } | ||
| } | ||
| ``` |
There was a problem hiding this comment.
@moizpgedge could you please remove this advice? Coderabbit is right that this is contradictory with our existing documentation.
moizpgedge
force-pushed
the
feat/PLAT-399/Implement-release-deallocate-method-on-IPAM-service
branch
from
58c23fd to
6e503a6
Compare
jason-lynch
left a comment
jason-lynch
left a comment
There was a problem hiding this comment.
Awesome! Thanks for making this change.
2 participants
Summary
Adds subnet release on database network delete so the IPAM pool is reused and "range is full" no longer occurs after repeated create/delete. Release is best-effort and non-fatal (warnings only) when the subnet is invalid, missing, or out of range.
Changes
Testing
go build ./server/... and go test ./server/internal/ipam/... ./server/internal/orchestrator/swarm/...
make test (full unit tests)
Manual: small CIDR (e.g. /26), create database → delete → create again; second create succeeds (no "range is full").
...
Checklist
Notes for Reviewers
PLAT-399