From f6b71dc422f8f8a4b9844c7d839b5d0f6d237872 Mon Sep 17 00:00:00 2001 From: Joshua Aresty Date: Fri, 20 Oct 2017 09:53:57 -0700 Subject: [PATCH] Fix bug in tie strategy prioritization - It now will adjust prioritized list (of azs) each time it matches one - This ensures that the prioritization will only happen dependent on the number of existing instance azs at the time the TieStrategy was created. [#137919209](https://www.pivotaltracker.com/story/show/137919209) Signed-off-by: Difan Zhao --- .../placement_planner/tie_strategy.rb | 6 ++++-- .../unplaced_existing_instances.rb | 2 +- .../unplaced_existing_instances_spec.rb | 7 ++++--- .../unit/deployment_plan/tie_strategy_spec.rb | 18 +++++++++++++++--- 4 files changed, 24 insertions(+), 9 deletions(-) diff --git a/src/bosh-director/lib/bosh/director/deployment_plan/placement_planner/tie_strategy.rb b/src/bosh-director/lib/bosh/director/deployment_plan/placement_planner/tie_strategy.rb index 07eff0fa05f..0d1f2b7b774 100644 --- a/src/bosh-director/lib/bosh/director/deployment_plan/placement_planner/tie_strategy.rb +++ b/src/bosh-director/lib/bosh/director/deployment_plan/placement_planner/tie_strategy.rb @@ -10,7 +10,8 @@ def initialize(preferred_az_names) def call(azs) azs.find { |az| - @preferred_az_names.any? {|i| i == az.name } + i = @preferred_az_names.find_index {|az_name| az_name == az.name } + @preferred_az_names.delete_at(i) if i } || azs.min end end @@ -23,7 +24,8 @@ def initialize(preferred_az_names, random: Random) def call(azs) azs.find { |az| - @preferred_az_names.any? {|i| i == az.name } + i = @preferred_az_names.find_index {|az_name| az_name == az.name } + @preferred_az_names.delete_at(i) if i } || azs.sample(random: @random) end end diff --git a/src/bosh-director/lib/bosh/director/deployment_plan/placement_planner/unplaced_existing_instances.rb b/src/bosh-director/lib/bosh/director/deployment_plan/placement_planner/unplaced_existing_instances.rb index 310ce3b85d9..c400c80321a 100644 --- a/src/bosh-director/lib/bosh/director/deployment_plan/placement_planner/unplaced_existing_instances.rb +++ b/src/bosh-director/lib/bosh/director/deployment_plan/placement_planner/unplaced_existing_instances.rb @@ -40,7 +40,7 @@ def unclaimed end def azs - @az_name_to_existing_instances.keys + @instances.map(&:availability_zone) end private diff --git a/src/bosh-director/spec/unit/deployment_plan/placement_planner/unplaced_existing_instances_spec.rb b/src/bosh-director/spec/unit/deployment_plan/placement_planner/unplaced_existing_instances_spec.rb index 84ff963c934..bc500da105c 100644 --- a/src/bosh-director/spec/unit/deployment_plan/placement_planner/unplaced_existing_instances_spec.rb +++ b/src/bosh-director/spec/unit/deployment_plan/placement_planner/unplaced_existing_instances_spec.rb @@ -3,15 +3,16 @@ module Bosh::Director::DeploymentPlan describe PlacementPlanner::UnplacedExistingInstances do - subject { PlacementPlanner::UnplacedExistingInstances.new([instance1, instance2, instance3]) } + subject { PlacementPlanner::UnplacedExistingInstances.new([instance1, instance2, instance3, instance4]) } let(:instance1) { Bosh::Director::Models::Instance.make(availability_zone: '1') } let(:instance2) { Bosh::Director::Models::Instance.make(availability_zone: '2') } let(:instance3) { Bosh::Director::Models::Instance.make(availability_zone: '3') } + let(:instance4) { Bosh::Director::Models::Instance.make(availability_zone: '2') } describe 'azs' do - it 'should return a list of azs' do - expect(subject.azs).to eq(%w(1 2 3)) + it 'should return an un-de-duped list of azs' do + expect(subject.azs).to eq(%w(1 2 3 2)) end end end diff --git a/src/bosh-director/spec/unit/deployment_plan/tie_strategy_spec.rb b/src/bosh-director/spec/unit/deployment_plan/tie_strategy_spec.rb index e485fb9a4e1..9d1c3bda4dd 100644 --- a/src/bosh-director/spec/unit/deployment_plan/tie_strategy_spec.rb +++ b/src/bosh-director/spec/unit/deployment_plan/tie_strategy_spec.rb @@ -8,7 +8,7 @@ module PlacementPlanner let(:az3) { AvailabilityZone.new("3", {}) } describe TieStrategy::MinWins do - subject { described_class.new(%w(3)) } + subject { described_class.new(%w(3 3)) } it 'chooses the minimum' do expect(subject.call([az1, az2])).to eq(az1) @@ -17,10 +17,16 @@ module PlacementPlanner it 'prioritizes the priority' do expect(subject.call([az2, az3])).to eq(az3) end + + it 'deletes an item from priority when we match based on priority' do + expect(subject.call([az1, az2, az3])).to eq(az3) + expect(subject.call([az1, az2, az3])).to eq(az3) + expect(subject.call([az1, az2, az3])).to eq(az1) + end end describe TieStrategy::RandomWins do - subject { described_class.new(%w(3), random: fake_random) } + subject { described_class.new(%w(3 3), random: fake_random) } let(:fake_random) do r = Object.new @@ -35,7 +41,13 @@ def r.rand(n) end it 'prioritizes the priority' do - expect(subject.call([az2, az3])).to eq(az3) + expect(subject.call([az1, az2, az3])).to eq(az3) + end + + it 'deletes an item from priority when we match based on priority' do + expect(subject.call([az1, az2, az3])).to eq(az3) + expect(subject.call([az1, az2, az3])).to eq(az3) + expect(subject.call([az1, az2, az3])).to eq(az2) end end end