Fix flaky IP assignment by neddp · Pull Request #2706 · cloudfoundry/bosh

neddp · 2026-04-09T15:43:03Z

What is this change about?

Follow-up fix to #2657 to eliminate residual flaky behaviour in IP allocation from reserved CIDR ranges.

PR #2657 fixed the algorithmic logic in find_next_available_ip (pruning by last address, using overlaps?, capturing blocking_ip before mutation). However, the very second test run in Concourse failed. It looks like the issue is still present under certain Ruby hash seeds:

Changes:

IpAddrOrCidr#eql?: now compares both to_i and prefix directly, treating objects with the same base address but different prefix lengths as distinct
IpAddrOrCidr#hash: now uses [to_i, prefix].hash to satisfy the contract
find_next_available_ip sort key: changed from ip.to_i to [ip.to_i, ip.prefix] so larger blocks (smaller prefix) always sort before the /32s they contain

Please provide contextual information.

Fixes flakiness missed by Fix IP Allocation Bug: Reserved Range Not Detected #2657
The CI failure that prompted this investigation: https://bosh.ci.cloudfoundry.org/teams/main/pipelines/bosh-director/jobs/unit-director-sqlite/builds/581

 -#<Bosh::Director::IpAddrOrCidr:0x000076df19e9bb18
 - @ipaddr=#<IPAddr: IPv4:10.0.11.41/255.255.255.255>>
 +#<Bosh::Director::IpAddrOrCidr:0x000076df19f33760
 + @ipaddr=#<IPAddr: IPv4:10.0.11.34/255.255.255.255>>

 -#<Bosh::Director::IpAddrOrCidr:0x000076df199f91f0
 - @ipaddr=#<IPAddr: IPv4:10.0.11.36/255.255.255.255>>
 +#<Bosh::Director::IpAddrOrCidr:0x000076df199fe240
 + @ipaddr=#<IPAddr: IPv4:10.0.11.35/255.255.255.255>>

What tests have you run against this PR?

All existing IpAddrOrCidr unit specs pass (spec/unit/bosh/director/ip_addr_or_cidr_spec.rb)
All existing IpRepo unit specs pass, including the CIDR block tests added by Fix IP Allocation Bug: Reserved Range Not Detected #2657 (spec/unit/bosh/director/deployment_plan/ip_provider/ip_repo_spec.rb)
New unit tests added for #eql? and #hash covering:
- Same base address + same prefix: eql? true, equal hashes, deduplicates in Set
- Same base address + different prefix: eql? false, different hashes, coexist as distinct Set elements
- Different base address: eql? false
- Hash contract explicitly verified

How should this change be described in bosh release notes?

Fixed: IP allocation could intermittently assign addresses from reserved CIDR ranges due to non-deterministic Set behaviour in the IP dedup logic.

Does this PR introduce a breaking change?

No. This is a bug fix. The observable behaviour - that IPs are not allocated from reserved ranges - is unchanged; the fix makes it deterministic.

…rved IP allocation eql? delegated to == which loses CIDR prefix via IPAddr#coerce_other, causing eql?/hash to disagree and making Set dedup non-deterministic. Fix both to include prefix. Also change sort key to [to_i, prefix] so larger blocks always precede the /32s they contain, ensuring find_next_available_ip skips entire reserved ranges.

coderabbitai · 2026-04-09T15:43:21Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 37a63fb8-da80-4052-af11-76ec8d429d65

📥 Commits

Reviewing files that changed from the base of the PR and between 7b650f7 and ebc5c5c.

📒 Files selected for processing (1)

src/bosh-director/spec/unit/bosh/director/ip_addr_or_cidr_spec.rb

Walkthrough

IpAddrOrCidr#eql? was changed to require the other object be an IpAddrOrCidr and to compare @ipaddr.to_i, @ipaddr.prefix, and @ipaddr.ipv4?. IpAddrOrCidr#hash now hashes the tuple [ @ipaddr.to_i, @ipaddr.prefix, @ipaddr.ipv4? ]. ip_repo.rb's find_next_available_ip now sorts addresses_we_cant_allocate by the two-key ordering [ip.to_i, ip.prefix]. New RSpec coverage was added to verify eql? and hash behavior across equal and unequal-address/prefix/family cases.

Suggested reviewers

aramprice
fmoehler
rkoster

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Fix flaky IP assignment' directly and concisely describes the main fix in this PR—eliminating nondeterministic IP allocation behavior that caused test flakiness.
Description check	✅ Passed	The description comprehensively addresses all required template sections: the change and its rationale, contextual references (PR `#2657` and CI build link), test coverage details, release notes, breaking change assessment, and team tags.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix_flaky_ip_assignment

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/bosh-director/lib/bosh/director/ip_addr_or_cidr.rb`:
- Around line 47-51: The equality and hash implementations in IpAddrOrCidr
ignore address family causing IPv4 and IPv6 addresses with the same
integer/prefix to collide; update the eql? method (the equality check that
currently compares `@ipaddr.to_i` and `@ipaddr.prefix`) to also compare the address
family (e.g., `@ipaddr.family` or using ipv4?/ipv6?) and change the hash method to
include the same family value (e.g., include `@ipaddr.family` in the array used to
compute the hash) so that family is part of both equality and hashing.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 0554b280-a5ef-4ddc-8135-62fbc2037f04

📥 Commits

Reviewing files that changed from the base of the PR and between ef36ef9 and 5041f8d.

📒 Files selected for processing (3)

src/bosh-director/lib/bosh/director/deployment_plan/ip_provider/ip_repo.rb
src/bosh-director/lib/bosh/director/ip_addr_or_cidr.rb
src/bosh-director/spec/unit/bosh/director/ip_addr_or_cidr_spec.rb

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/bosh-director/spec/unit/bosh/director/ip_addr_or_cidr_spec.rb`:
- Around line 293-304: The test is using different prefixes (/32 vs /128) so
failure could be due to prefix mismatch rather than address family; update the
two examples to use the same prefix to isolate family as the cause. In the spec
examples that create IpAddrOrCidr objects (IpAddrOrCidr.new('0.0.0.1/32') and
IpAddrOrCidr.new('::1/128')), change the IPv6 case to use the same prefix (e.g.
IpAddrOrCidr.new('::1/32')), then keep the assertions on ipv4.eql?(ipv6) being
false and Set.new([ipv4, ipv6]).size eq(2) to verify distinctness by family.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 9a842170-4b3f-4c7b-9b64-5d586ba51b86

📥 Commits

Reviewing files that changed from the base of the PR and between 5041f8d and 7b650f7.

📒 Files selected for processing (2)

src/bosh-director/lib/bosh/director/ip_addr_or_cidr.rb
src/bosh-director/spec/unit/bosh/director/ip_addr_or_cidr_spec.rb

aramprice · 2026-04-15T00:31:41Z

Excited for this fix to land! I've seen a few flakes locally and now seeing one in GHA
https://github.com/cloudfoundry/bosh/actions/runs/24429878587/job/71371767691?pr=2707

neddp added 2 commits April 9, 2026 17:18

Add unit tests

5041f8d

neddp changed the title ~~Fix flaky ip assignment~~ Fix flaky IP assignment Apr 9, 2026

coderabbitai bot requested changes Apr 9, 2026

View reviewed changes

Comment thread src/bosh-director/lib/bosh/director/ip_addr_or_cidr.rb Outdated

cf-foundation-community-automation bot added this to Foundational Infrastructure Working Group Apr 9, 2026

cf-foundation-community-automation bot moved this to Inbox in Foundational Infrastructure Working Group Apr 9, 2026

github-project-automation bot moved this from Inbox to Waiting for Changes | Open for Contribution in Foundational Infrastructure Working Group Apr 9, 2026

coderabbitai bot previously approved these changes Apr 10, 2026

View reviewed changes

github-project-automation bot moved this from Waiting for Changes | Open for Contribution to Pending Merge | Prioritized in Foundational Infrastructure Working Group Apr 10, 2026

aramprice previously approved these changes Apr 10, 2026

View reviewed changes

Add address family check and method documentation

7b650f7

neddp dismissed stale reviews from aramprice and coderabbitai[bot] via 7b650f7 April 14, 2026 08:34

neddp requested a review from aramprice April 14, 2026 08:36

coderabbitai bot requested changes Apr 14, 2026

View reviewed changes

Comment thread src/bosh-director/spec/unit/bosh/director/ip_addr_or_cidr_spec.rb

github-project-automation bot moved this from Pending Merge | Prioritized to Waiting for Changes | Open for Contribution in Foundational Infrastructure Working Group Apr 14, 2026

Fix IP family check tests

ebc5c5c

coderabbitai bot approved these changes Apr 14, 2026

View reviewed changes

github-project-automation bot moved this from Waiting for Changes | Open for Contribution to Pending Merge | Prioritized in Foundational Infrastructure Working Group Apr 14, 2026

aramprice approved these changes Apr 14, 2026

View reviewed changes

rkoster approved these changes Apr 16, 2026

View reviewed changes

beyhan merged commit 1427cc0 into main Apr 16, 2026
20 checks passed

github-project-automation bot moved this from Pending Merge | Prioritized to Done in Foundational Infrastructure Working Group Apr 16, 2026

beyhan deleted the fix_flaky_ip_assignment branch April 16, 2026 14:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix flaky IP assignment#2706

Fix flaky IP assignment#2706
beyhan merged 4 commits intomainfrom
fix_flaky_ip_assignment

neddp commented Apr 9, 2026 •

edited

Loading

Uh oh!

coderabbitai bot commented Apr 9, 2026 •

edited

Loading

❌ Failed checks (1 warning)

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

aramprice commented Apr 15, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

neddp commented Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is this change about?

Please provide contextual information.

What tests have you run against this PR?

How should this change be described in bosh release notes?

Does this PR introduce a breaking change?

Uh oh!

coderabbitai bot commented Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Suggested reviewers

❌ Failed checks (1 warning)

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aramprice commented Apr 15, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

neddp commented Apr 9, 2026 •

edited

Loading

coderabbitai bot commented Apr 9, 2026 •

edited

Loading