diff --git a/src/bosh-director/lib/bosh/director/deployment_plan/placement_planner/networks_to_static_ips.rb b/src/bosh-director/lib/bosh/director/deployment_plan/placement_planner/networks_to_static_ips.rb index 8667555696..85b0b0eea1 100644 --- a/src/bosh-director/lib/bosh/director/deployment_plan/placement_planner/networks_to_static_ips.rb +++ b/src/bosh-director/lib/bosh/director/deployment_plan/placement_planner/networks_to_static_ips.rb @@ -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 @@ -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| @@ -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 diff --git a/src/bosh-director/lib/bosh/director/deployment_plan/placement_planner/static_ips_availability_zone_picker.rb b/src/bosh-director/lib/bosh/director/deployment_plan/placement_planner/static_ips_availability_zone_picker.rb index da699931f4..7cac9e9602 100644 --- a/src/bosh-director/lib/bosh/director/deployment_plan/placement_planner/static_ips_availability_zone_picker.rb +++ b/src/bosh-director/lib/bosh/director/deployment_plan/placement_planner/static_ips_availability_zone_picker.rb @@ -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 + 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 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}'") @@ -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 @@ -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 + def instance_name(existing_instance_model) "#{existing_instance_model.job}/#{existing_instance_model.index}" end diff --git a/src/bosh-director/spec/unit/bosh/director/deployment_plan/placement_planner/networks_to_static_ips_spec.rb b/src/bosh-director/spec/unit/bosh/director/deployment_plan/placement_planner/networks_to_static_ips_spec.rb index 7b18a63666..cf1f0cf059 100644 --- a/src/bosh-director/spec/unit/bosh/director/deployment_plan/placement_planner/networks_to_static_ips_spec.rb +++ b/src/bosh-director/spec/unit/bosh/director/deployment_plan/placement_planner/networks_to_static_ips_spec.rb @@ -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 diff --git a/src/bosh-director/spec/unit/bosh/director/deployment_plan/placement_planner/static_ips_availability_zone_picker_spec.rb b/src/bosh-director/spec/unit/bosh/director/deployment_plan/placement_planner/static_ips_availability_zone_picker_spec.rb index a84f37a88c..8dc6bb95b9 100644 --- a/src/bosh-director/spec/unit/bosh/director/deployment_plan/placement_planner/static_ips_availability_zone_picker_spec.rb +++ b/src/bosh-director/spec/unit/bosh/director/deployment_plan/placement_planner/static_ips_availability_zone_picker_spec.rb @@ -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', @@ -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 }] }