Skip to content

Fix static IP subnet mismatch when adding network to nic_group #2716

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
vlast3k wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix static IP subnet mismatch when adding network to nic_group #2716

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2c66bf3
Fix static IP subnet mismatch when adding network to nic_group
vlast3k Apr 20, 2026
4bdaa97
Fix error message for non-nic_group case and add self-match guard
vlast3k Apr 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions ...sh-director/lib/bosh/director/deployment_plan/placement_planner/networks_to_static_ips.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ def self.create(job_networks, desired_azs, job_name)
az_names = subnet_for_ip.availability_zone_names.nil? ? [nil] : subnet_for_ip.availability_zone_names
filtered_az_names = az_names.select { |static_ip_az_name| desired_az_names.include?(static_ip_az_name) }.uniq
networks_to_static_ips[job_network.name] ||= []
networks_to_static_ips[job_network.name] << StaticIpToAzs.new(static_ip, filtered_az_names)
networks_to_static_ips[job_network.name] << StaticIpToAzs.new(static_ip, filtered_az_names, false, subnet_for_ip.cloud_properties)
end
end

Expand Down Expand Up @@ -88,6 +88,10 @@ def next_ip_for_network(network)
unclaimed_networks_to_static_ips[network.name].first
end

def next_ip_for_network_with_cloud_properties(network, cloud_properties)
unclaimed_networks_to_static_ips[network.name].find { |s| s.cloud_properties == cloud_properties }
end

def claim_in_az(ip, az_name)
@networks_to_static_ips.each do |_, static_ips_to_azs|
static_ips_to_azs.each do |static_ip_to_azs|
Expand All @@ -113,11 +117,17 @@ def find_by_network_and_az(network, az_name)
unclaimed_networks_to_static_ips[network.name].find { |static_ip_to_azs| static_ip_to_azs.az_names.include?(az_name) }
end

def find_by_network_az_and_cloud_properties(network, az_name, cloud_properties)
unclaimed_networks_to_static_ips[network.name].find do |static_ip_to_azs|
static_ip_to_azs.az_names.include?(az_name) && static_ip_to_azs.cloud_properties == cloud_properties
end
end

def unclaimed_networks_to_static_ips
Hash[@networks_to_static_ips.map { |network_name, static_ips_to_azs| [network_name, static_ips_to_azs.reject(&:claimed)] }]
end

class StaticIpToAzs < Struct.new(:ip, :az_names, :claimed); end
class StaticIpToAzs < Struct.new(:ip, :az_names, :claimed, :cloud_properties); end
end
end
end
Expand Down
42 changes: 40 additions & 2 deletions ...ib/bosh/director/deployment_plan/placement_planner/static_ips_availability_zone_picker.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,8 +115,22 @@ def place_new_instance_plans(desired_instances, instance_plans)
def create_network_plan_with_az(instance_plan, network, instance_plans)
desired_instance = instance_plan.desired_instance
instance = instance_plan.instance
sibling_cloud_props = nic_group_sibling_cloud_properties(instance_plan, network)
if desired_instance.az.nil?
static_ip_to_azs = @networks_to_static_ips.next_ip_for_network(network)
static_ip_to_azs = if sibling_cloud_props
@networks_to_static_ips.next_ip_for_network_with_cloud_properties(network, sibling_cloud_props)
else
@networks_to_static_ips.next_ip_for_network(network)
end
Comment on lines +120 to +124
Copy link
Copy Markdown
Member

@aramprice aramprice May 7, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This same conditional appears on lines 143-147 below, should it move into some method? Possibly on @networks_to_static_ips?

if static_ip_to_azs.nil?
if sibling_cloud_props
raise Bosh::Director::NetworkReservationError,
"Failed to find a static IP for network '#{network.name}' on a matching subnet " \
"in nic_group '#{network.nic_group}'"
end
raise Bosh::Director::NetworkReservationError,
'Failed to distribute static IPs to satisfy existing instance reservations'
end
Comment thread
coderabbitai[bot] marked this conversation as resolved.
if static_ip_to_azs.az_names.size == 1
az_name = static_ip_to_azs.az_names.first
@logger.debug("Assigning az '#{az_name}' to instance '#{instance}'")
Expand All @@ -126,9 +140,18 @@ def create_network_plan_with_az(instance_plan, network, instance_plans)
end
desired_instance.az = to_az(az_name)
else
static_ip_to_azs = @networks_to_static_ips.find_by_network_and_az(network, desired_instance.availability_zone)
static_ip_to_azs = if sibling_cloud_props
@networks_to_static_ips.find_by_network_az_and_cloud_properties(network, desired_instance.availability_zone, sibling_cloud_props)
else
@networks_to_static_ips.find_by_network_and_az(network, desired_instance.availability_zone)
end
end
if static_ip_to_azs.nil?
if sibling_cloud_props
raise Bosh::Director::NetworkReservationError,
"Failed to find a static IP for network '#{network.name}' on a matching subnet " \
"in nic_group '#{network.nic_group}' in availability zone '#{desired_instance.availability_zone}'"
end
raise Bosh::Director::NetworkReservationError,
'Failed to distribute static IPs to satisfy existing instance reservations'
end
Expand Down Expand Up @@ -266,6 +289,21 @@ def to_az(az_name)
@desired_azs.to_a.find { |az| az.name == az_name }
end

def nic_group_sibling_cloud_properties(instance_plan, network)
return nil unless network.nic_group

instance_plan.network_plans.each do |plan|
sibling_network = @job_networks.find { |jn| jn.deployment_network == plan.reservation.network }
next if sibling_network == network
next unless sibling_network&.nic_group == network.nic_group
next unless plan.reservation.ip

subnet = plan.reservation.network.find_subnet_containing(plan.reservation.ip)
return subnet.cloud_properties if subnet
end
nil
end
Comment thread
coderabbitai[bot] marked this conversation as resolved.

def instance_name(existing_instance_model)
"#{existing_instance_model.job}/#{existing_instance_model.index}"
end
Expand Down
42 changes: 42 additions & 0 deletions .../spec/unit/bosh/director/deployment_plan/placement_planner/networks_to_static_ips_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,48 @@ module DeploymentPlan
end
end
end

describe '#create stores cloud_properties' do
let(:deployment_subnets) do
[
ManualNetworkSubnet.new(
'network_A',
IPAddr.new('192.168.1.0/24'),
nil, nil, { 'subnet' => 'subnet-aaa' }, nil, ['zone_1'], [],
[to_ipaddr('192.168.1.10')],
),
ManualNetworkSubnet.new(
'network_A',
IPAddr.new('192.168.2.0/24'),
nil, nil, { 'subnet' => 'subnet-bbb' }, nil, ['zone_1'], [],
[to_ipaddr('192.168.2.10')],
),
]
end
let(:deployment_network) { ManualNetwork.new('network_A', deployment_subnets, '32', nil) }
let(:job_networks) do
[FactoryBot.build(:deployment_plan_job_network, name: 'network_A', static_ips: ['192.168.1.10', '192.168.2.10'], deployment_network: deployment_network)]
end
let(:desired_azs) { [AvailabilityZone.new('zone_1', {})] }

it 'records cloud_properties from the containing subnet' do
nsi = PlacementPlanner::NetworksToStaticIps.create(job_networks, desired_azs, 'fake-job')
ip1 = nsi.next_ip_for_network(job_networks[0])
expect(ip1.cloud_properties).to eq({ 'subnet' => 'subnet-aaa' })
end

it 'finds IPs filtered by cloud_properties' do
nsi = PlacementPlanner::NetworksToStaticIps.create(job_networks, desired_azs, 'fake-job')
result = nsi.find_by_network_az_and_cloud_properties(job_networks[0], 'zone_1', { 'subnet' => 'subnet-bbb' })
expect(result.ip).to eq('192.168.2.10')
end

it 'returns nil when no IP matches the cloud_properties' do
nsi = PlacementPlanner::NetworksToStaticIps.create(job_networks, desired_azs, 'fake-job')
result = nsi.find_by_network_az_and_cloud_properties(job_networks[0], 'zone_1', { 'subnet' => 'subnet-xxx' })
expect(result).to be_nil
end
end
end
end
end
Expand Down
101 changes: 101 additions & 0 deletions ...sh/director/deployment_plan/placement_planner/static_ips_availability_zone_picker_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,12 @@ def make_subnet_spec(range, static_ips, zone_names)
spec['azs'] = zone_names if zone_names
spec
end

def make_subnet_spec_with_cloud_props(range, static_ips, zone_names, cloud_properties)
spec = make_subnet_spec(range, static_ips, zone_names)
spec['cloud_properties'] = cloud_properties
spec
end
let(:networks_spec) do
[
{ 'name' => 'a',
Expand Down Expand Up @@ -924,6 +930,101 @@ def make_subnet_spec(range, static_ips, zone_names)
end
end

context 'when networks share a nic_group with multiple subnets per AZ' do
let(:desired_instance_count) { 2 }
let(:instance_group_networks) do
[
{ 'name' => 'a', 'static_ips' => %w[192.168.1.10 192.168.2.10], 'default' => %w[dns gateway], 'nic_group' => '1' },
{ 'name' => 'b', 'static_ips' => %w[10.10.1.10 10.10.2.10], 'nic_group' => '1' },
]
end
let(:networks_spec) do
[
{ 'name' => 'a',
'subnets' => [
make_subnet_spec_with_cloud_props('192.168.1.0/24', ['192.168.1.10 - 192.168.1.14'], ['zone1'], { 'subnet' => 'subnet-aaa' }),
make_subnet_spec_with_cloud_props('192.168.2.0/24', ['192.168.2.10 - 192.168.2.14'], ['zone1'], { 'subnet' => 'subnet-bbb' }),
] },
{ 'name' => 'b',
'subnets' => [
make_subnet_spec_with_cloud_props('10.10.1.0/24', ['10.10.1.10 - 10.10.1.14'], ['zone1'], { 'subnet' => 'subnet-aaa' }),
make_subnet_spec_with_cloud_props('10.10.2.0/24', ['10.10.2.10 - 10.10.2.14'], ['zone1'], { 'subnet' => 'subnet-bbb' }),
] },
]
end
let(:instance_group_availability_zones) { ['zone1'] }

context 'when existing instances have IPs on one network and a second nic_group network is added' do
let(:existing_instances) do
[
existing_instance_with_az_and_ips('zone1', ['192.168.1.10/32']),
existing_instance_with_az_and_ips('zone1', ['192.168.2.10/32']),
]
end

it 'assigns the new network IP on the same cloud subnet as the existing sibling' do
expect(new_instance_plans).to eq([])
expect(obsolete_instance_plans).to eq([])
expect(existing_instance_plans.size).to eq(2)

existing_instance_plans.each do |plan|
a_ip = plan.network_plans.find { |np| np.reservation.network.name == 'a' }.reservation.ip
b_ip = plan.network_plans.find { |np| np.reservation.network.name == 'b' }.reservation.ip

a_subnet = planner.networks.find { |n| n.name == 'a' }.find_subnet_containing(a_ip)
b_subnet = planner.networks.find { |n| n.name == 'b' }.find_subnet_containing(b_ip)

expect(a_subnet.cloud_properties).to eq(b_subnet.cloud_properties),
"Expected IPs #{a_ip} and #{b_ip} to be on same cloud subnet, " \
"got #{a_subnet.cloud_properties} vs #{b_subnet.cloud_properties}"
end
end
end

context 'when no matching IP is available on the sibling cloud subnet' do
let(:instance_group_networks) do
[
{ 'name' => 'a', 'static_ips' => ['192.168.1.10'], 'default' => %w[dns gateway], 'nic_group' => '1' },
{ 'name' => 'b', 'static_ips' => ['10.10.2.10'], 'nic_group' => '1' },
]
end
let(:desired_instance_count) { 1 }
let(:existing_instances) do
[
existing_instance_with_az_and_ips('zone1', ['192.168.1.10/32']),
]
end

it 'raises an error instead of silently assigning a mismatched subnet' do
expect { instance_plans }.to raise_error(
Bosh::Director::NetworkReservationError,
/Failed to find a static IP for network 'b' on a matching subnet in nic_group '1'/,
)
end
end

context 'when nic_group is not used' do
let(:instance_group_networks) do
[
{ 'name' => 'a', 'static_ips' => %w[192.168.1.10 192.168.2.10], 'default' => %w[dns gateway] },
{ 'name' => 'b', 'static_ips' => %w[10.10.1.10 10.10.2.10] },
]
end
let(:existing_instances) do
[
existing_instance_with_az_and_ips('zone1', ['192.168.1.10/32']),
existing_instance_with_az_and_ips('zone1', ['192.168.2.10/32']),
]
end

it 'assigns IPs without cloud subnet constraint (existing behavior)' do
expect(existing_instance_plans.size).to eq(2)
expect(new_instance_plans).to eq([])
expect(obsolete_instance_plans).to eq([])
end
end
end

context 'when network name was changed' do
let(:desired_instance_count) { 2 }
let(:instance_group_networks) { [{ 'name' => 'a', 'static_ips' => static_ips }] }
Expand Down
Loading