Skip to content

Commit

Permalink
Fix bug in tie strategy prioritization
Browse files Browse the repository at this point in the history
- 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 <[email protected]>
  • Loading branch information
Joshua Aresty authored and Difan Zhao committed Oct 20, 2017
1 parent 80ab5ba commit f6b71dc
Show file tree
Hide file tree
Showing 4 changed files with 24 additions and 9 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ def unclaimed
end

def azs
@az_name_to_existing_instances.keys
@instances.map(&:availability_zone)
end

private
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
18 changes: 15 additions & 3 deletions src/bosh-director/spec/unit/deployment_plan/tie_strategy_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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
Expand All @@ -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
Expand Down

0 comments on commit f6b71dc

Please sign in to comment.