Skip to content

Commit 9f6166f

Browse files
cunniecunnie
committed
Explicitly call out memory units (MB, GB)
Our `fetch_resource_pool_utilization` method has been broken: `quick_stats.host_memory_usage` was treated as in kiB when it was in bytes. This didn't have much fallout; the miscalculation would've only affected deployments to memory-starved resource pools, and the only manifestation would've been to error later in the deployment phase rather than earlier. To avoid this type of error, we generously update the variables and methods that return memory-related information to include the units (typically MiB).
1 parent a8e1a30 commit 9f6166f

5 files changed

Lines changed: 58 additions & 56 deletions

File tree

‎src/vsphere_cpi/lib/cloud/vsphere/resources/cluster.rb‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ class Cluster
1717
HOST_PROPERTIES = %w(name hardware.memorySize runtime.connectionState runtime.inMaintenanceMode runtime.powerState)
1818
HOST_COUNTERS = %w(mem.usage.average)
1919

20-
MEMORY_HEADROOM = 128
20+
MEMORY_HEADROOM_MB = 128
2121

2222
# @!attribute mob
2323
# @return [Vim::ClusterComputeResource] cluster vSphere MOB.
@@ -27,7 +27,7 @@ class Cluster
2727
# @return [ResourcePool] resource pool.
2828
attr_reader :resource_pool
2929

30-
class FreeMemory < Struct.new(:cluster_free_memory, :host_group_free_memory); end
30+
class FreeMemory < Struct.new(:cluster_free_memory_mb, :host_group_free_memory_mb); end
3131

3232
# Creates a new Cluster resource from the specified datacenter, cluster
3333
# configuration, and prefetched properties.
@@ -60,7 +60,7 @@ def free_memory
6060
FreeMemory.new(fetch_host_group_utilization, fetch_host_group_utilization)
6161
end
6262
else
63-
@synced_free_memory = FreeMemory.new(fetch_resource_pool_utilization, fetch_resource_pool_utilization)
63+
@synced_free_memory = FreeMemory.new(fetch_resource_pool_utilization_mb, fetch_resource_pool_utilization_mb)
6464
end
6565
end
6666

@@ -224,15 +224,17 @@ def fetch_cluster_utilization()
224224
# so we can't use it for the raw clusters.
225225
#
226226
# @return [void]
227-
def fetch_resource_pool_utilization
227+
def fetch_resource_pool_utilization_mb
228228
logger.debug("Fetching Memory utilization for Resource Pool #{resource_pool.name}")
229229
properties = @client.cloud_searcher.get_properties(resource_pool.mob, Vim::ResourcePool, 'summary')
230230
raise "Failed to get utilization for resource pool '#{resource_pool}'" if properties.nil?
231231

232232
runtime_info = properties["summary"].runtime
233233
quick_stats = properties["summary"].quick_stats
234234
memory = runtime_info.memory
235-
return (memory.max_usage - (quick_stats.host_memory_usage) * 1024) / BYTES_IN_MB
235+
memory_max_mb = memory.max_usage / BYTES_IN_MB
236+
host_memory_usage_mb = quick_stats.host_memory_usage
237+
return memory_max_mb - host_memory_usage_mb
236238
end
237239
end
238240
end

‎src/vsphere_cpi/lib/cloud/vsphere/vm_config.rb‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -219,7 +219,7 @@ def validate_clusters
219219
def cluster_placement_internal(clusters:)
220220
return @cluster_placement if @cluster_placement
221221

222-
vm_selection_placement_pipeline = VmPlacementSelectionPipeline.new(disk_config: disk_configurations, req_memory: vm_type.ram) do
222+
vm_selection_placement_pipeline = VmPlacementSelectionPipeline.new(disk_config: disk_configurations, req_memory_mb: vm_type.ram) do
223223
logger.info("Gathering vm placement resources for vm placement allocator pipeline")
224224
clusters.map do |cluster|
225225
VmPlacement.new(cluster: cluster, datastores: cluster.accessible_datastores.values, hosts: nil)

‎src/vsphere_cpi/lib/cloud/vsphere/vm_placement_selection_pipeline.rb‎

Lines changed: 16 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -38,12 +38,12 @@ def balance_score
3838
@balance_score = min + mean + median
3939
end
4040

41-
def cluster_free_memory
42-
cluster.free_memory.cluster_free_memory
41+
def cluster_free_memory_mb
42+
cluster.free_memory.cluster_free_memory_mb
4343
end
4444

45-
def host_group_free_memory
46-
cluster.free_memory.host_group_free_memory
45+
def host_group_free_memory_mb
46+
cluster.free_memory.host_group_free_memory_mb
4747
end
4848

4949
def inspect_before
@@ -56,7 +56,7 @@ def inspect
5656
end
5757

5858
def cluster_inspect
59-
"VM Placement Cluster: #{cluster.name} Cluster free memory: #{cluster_free_memory}, host group free memory: #{host_group_free_memory}"
59+
"VM Placement Cluster: #{cluster.name} Cluster free memory (MiB): #{cluster_free_memory_mb}, host group free memory (MiB): #{host_group_free_memory_mb}"
6060
end
6161

6262
def datastore_inspect
@@ -66,29 +66,29 @@ def datastore_inspect
6666

6767
class VmPlacementSelectionPipeline < SelectionPipeline
6868
class VmPlacementCriteria
69-
DEFAULT_MEMORY_HEADROOM = 128
69+
DEFAULT_MEMORY_HEADROOM_MB = 128
7070

71-
attr_reader :disk_config, :req_memory, :mem_headroom
71+
attr_reader :disk_config, :req_memory_mb, :mem_headroom_mb
7272

73-
def required_memory
74-
req_memory + mem_headroom
73+
def required_memory_mb
74+
req_memory_mb + mem_headroom_mb
7575
end
7676

7777
def initialize(criteria = {})
7878
@disk_config = criteria[:disk_config]
79-
@req_memory = criteria[:req_memory]
80-
@mem_headroom = criteria[:mem_headroom] || DEFAULT_MEMORY_HEADROOM
79+
@req_memory_mb = criteria[:req_memory_mb]
80+
@mem_headroom_mb = criteria[:mem_headroom_mb] || DEFAULT_MEMORY_HEADROOM_MB
8181
end
8282

8383
def inspect
84-
"VM Placement Criteria: Disk Config: #{disk_config} Req Memory: #{req_memory} Mem Headroom: #{mem_headroom}"
84+
"VM Placement Criteria: Disk Config: #{disk_config} Req Memory (MiB): #{req_memory_mb} Mem Headroom (MiB): #{mem_headroom_mb}"
8585
end
8686
end
8787
private_constant :VmPlacementCriteria
8888

8989
with_filter do |vm_placement, criteria_object|
90-
logger.debug("Filter #{vm_placement.cluster_inspect} for free memory required: #{criteria_object.required_memory}")
91-
vm_placement.cluster_free_memory > criteria_object.required_memory
90+
logger.debug("Filter #{vm_placement.cluster_inspect} for free memory required (MiB): #{criteria_object.required_memory_mb}")
91+
vm_placement.cluster_free_memory_mb > criteria_object.required_memory_mb
9292
end
9393

9494
with_filter ->(vm_placement, criteria_object) do
@@ -111,7 +111,7 @@ def inspect
111111
# There are two possibilities at this point. Either:
112112
# 1. The disk is an ephemeral disk
113113
# 2. The disk is a persistent disk that must be migrated
114-
pipeline = DiskPlacementSelectionPipeline.new(disk.size, disk.target_datastore_pattern, nil, disk.ephemeral?, criteria_object.required_memory) do
114+
pipeline = DiskPlacementSelectionPipeline.new(disk.size, disk.target_datastore_pattern, nil, disk.ephemeral?, criteria_object.required_memory_mb) do
115115
vm_placement.datastores
116116
end.with_filter do |storage_placement|
117117
# TODO: both accessible? and accessible_from? will be queried for
@@ -145,7 +145,7 @@ def inspect
145145
end
146146

147147
with_scorer do |p1, p2|
148-
-(p1.host_group_free_memory <=> p2.host_group_free_memory)
148+
-(p1.host_group_free_memory_mb <=> p2.host_group_free_memory_mb)
149149
end
150150

151151
def initialize(*args)

‎src/vsphere_cpi/spec/unit/cloud/vsphere/resources/cluster_spec.rb‎

Lines changed: 26 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -130,21 +130,21 @@ module VSphereCloud::Resources
130130
overall_status: 'green',
131131
memory: instance_double(
132132
'VimSdk::Vim::ResourcePool::ResourceUsage',
133-
max_usage: 1024 * 1024 * 100,
133+
max_usage: 1024 * 1024 * 100, # 100 MiB
134134
)
135135
)
136136
end
137137

138138
let(:fake_quick_stats) do
139139
instance_double(
140140
'VimSdk::Vim::ResourcePool::Summary::QuickStats',
141-
host_memory_usage: 1024 * 75
141+
host_memory_usage: 75 # 75 MiB
142142
)
143143
end
144144

145145
it 'sets resources to values in the runtime status' do
146-
expect(cluster.free_memory.cluster_free_memory).to eq(25)
147-
expect(cluster.free_memory.host_group_free_memory).to eq(25)
146+
expect(cluster.free_memory.cluster_free_memory_mb).to eq(25)
147+
expect(cluster.free_memory.host_group_free_memory_mb).to eq(25)
148148
end
149149
end
150150
end
@@ -187,8 +187,8 @@ def generate_host_property(mob:, name:, connection_state:, maintenance_mode:, me
187187
end
188188

189189
it 'sets resources to values based on the active hosts in the cluster' do
190-
expect(cluster.free_memory.cluster_free_memory).to eq(85)
191-
expect(cluster.free_memory.host_group_free_memory).to eq(85)
190+
expect(cluster.free_memory.cluster_free_memory_mb).to eq(85)
191+
expect(cluster.free_memory.host_group_free_memory_mb).to eq(85)
192192
end
193193

194194
context 'when an ESXi host is not powered on' do
@@ -201,8 +201,8 @@ def generate_host_property(mob:, name:, connection_state:, maintenance_mode:, me
201201
end
202202

203203
it 'includes the free memory of only powered on hosts' do
204-
expect(cluster.free_memory.cluster_free_memory).to eq(85)
205-
expect(cluster.free_memory.host_group_free_memory).to eq(85)
204+
expect(cluster.free_memory.cluster_free_memory_mb).to eq(85)
205+
expect(cluster.free_memory.host_group_free_memory_mb).to eq(85)
206206
end
207207
end
208208

@@ -216,8 +216,8 @@ def generate_host_property(mob:, name:, connection_state:, maintenance_mode:, me
216216
end
217217

218218
it 'includes the free memory of only connected hosts' do
219-
expect(cluster.free_memory.cluster_free_memory).to eq(85)
220-
expect(cluster.free_memory.host_group_free_memory).to eq(85)
219+
expect(cluster.free_memory.cluster_free_memory_mb).to eq(85)
220+
expect(cluster.free_memory.host_group_free_memory_mb).to eq(85)
221221
end
222222
end
223223

@@ -231,8 +231,8 @@ def generate_host_property(mob:, name:, connection_state:, maintenance_mode:, me
231231
end
232232

233233
it 'includes the free memory of only hosts not in maintenance mode' do
234-
expect(cluster.free_memory.cluster_free_memory).to eq(85)
235-
expect(cluster.free_memory.host_group_free_memory).to eq(85)
234+
expect(cluster.free_memory.cluster_free_memory_mb).to eq(85)
235+
expect(cluster.free_memory.host_group_free_memory_mb).to eq(85)
236236
end
237237
end
238238

@@ -246,8 +246,8 @@ def generate_host_property(mob:, name:, connection_state:, maintenance_mode:, me
246246
allow(compute_summary).to receive(:effective_memory).and_return(0)
247247
end
248248
it 'defaults free memory to zero' do
249-
expect(cluster.free_memory.cluster_free_memory).to eq(0)
250-
expect(cluster.free_memory.host_group_free_memory).to eq(0)
249+
expect(cluster.free_memory.cluster_free_memory_mb).to eq(0)
250+
expect(cluster.free_memory.host_group_free_memory_mb).to eq(0)
251251
end
252252
end
253253
end
@@ -282,11 +282,11 @@ def generate_host_property(mob:, name:, connection_state:, maintenance_mode:, me
282282

283283
context 'when host group rule type is MUST' do
284284
it 'returns sum of raw available memory on two hosts in Megabytes for cluster free memory' do
285-
expect(cluster.free_memory.cluster_free_memory).to eq(18)
285+
expect(cluster.free_memory.cluster_free_memory_mb).to eq(18)
286286
end
287287

288288
it 'returns sum of raw available memory on two hosts in Megabytes for host group free memory' do
289-
expect(cluster.free_memory.host_group_free_memory).to eq(18)
289+
expect(cluster.free_memory.host_group_free_memory_mb).to eq(18)
290290
end
291291
end
292292

@@ -300,11 +300,11 @@ def generate_host_property(mob:, name:, connection_state:, maintenance_mode:, me
300300
end
301301

302302
it 'returns full cluster free memory in Megabytes for cluster free memory' do
303-
expect(cluster.free_memory.cluster_free_memory).to eq(85)
303+
expect(cluster.free_memory.cluster_free_memory_mb).to eq(85)
304304
end
305305

306306
it 'returns sum of raw available memory on two hosts in Megabytes for host group free memory' do
307-
expect(cluster.free_memory.host_group_free_memory).to eq(18)
307+
expect(cluster.free_memory.host_group_free_memory_mb).to eq(18)
308308
end
309309
end
310310

@@ -315,8 +315,8 @@ def generate_host_property(mob:, name:, connection_state:, maintenance_mode:, me
315315
end
316316

317317
it 'returns 0' do
318-
expect(cluster.free_memory.cluster_free_memory).to eq(0)
319-
expect(cluster.free_memory.host_group_free_memory).to eq(0)
318+
expect(cluster.free_memory.cluster_free_memory_mb).to eq(0)
319+
expect(cluster.free_memory.host_group_free_memory_mb).to eq(0)
320320
end
321321
end
322322

@@ -329,8 +329,8 @@ def generate_host_property(mob:, name:, connection_state:, maintenance_mode:, me
329329
end
330330

331331
it 'returns 0' do
332-
expect(cluster.free_memory.cluster_free_memory).to eq(0)
333-
expect(cluster.free_memory.host_group_free_memory).to eq(0)
332+
expect(cluster.free_memory.cluster_free_memory_mb).to eq(0)
333+
expect(cluster.free_memory.host_group_free_memory_mb).to eq(0)
334334
end
335335
end
336336
end
@@ -343,21 +343,21 @@ def generate_host_property(mob:, name:, connection_state:, maintenance_mode:, me
343343
overall_status: 'green',
344344
memory: instance_double(
345345
'VimSdk::Vim::ResourcePool::ResourceUsage',
346-
max_usage: 1024 * 1024 * 100,
346+
max_usage: 1024 * 1024 * 100, # 100 MiB
347347
)
348348
)
349349
end
350350

351351
let(:fake_quick_stats) do
352352
instance_double(
353353
'VimSdk::Vim::ResourcePool::Summary::QuickStats',
354-
host_memory_usage: 1024 * 75
354+
host_memory_usage: 75 # 75 MiB
355355
)
356356
end
357357

358358
it 'returns the amount of free memory in the cluster' do
359-
expect(cluster.free_memory.cluster_free_memory).to eq(25)
360-
expect(cluster.free_memory.host_group_free_memory).to eq(25)
359+
expect(cluster.free_memory.cluster_free_memory_mb).to eq(25)
360+
expect(cluster.free_memory.host_group_free_memory_mb).to eq(25)
361361
end
362362

363363
context 'when we fail to get the utilization for a resource pool' do

‎src/vsphere_cpi/spec/unit/cloud/vsphere/vm_placement_selection_pipeline_spec.rb‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ def fake_cluster(name, *args)
2626
end
2727

2828
# Simulating a mini datacenter with two clusters. Each cluster has two dedicated datastores and one shared datastore.
29-
let(:criteria) { [disk_config: disk_config, req_memory: 1024] }
29+
let(:criteria) { [disk_config: disk_config, req_memory_mb: 1024] }
3030
let(:ds_cl1_1) { fake_datastore('fake-ds-cl1-1') }
3131
let(:ds_cl1_2) { fake_datastore('fake-ds-cl1-2') }
3232
let(:ds_cl2_1) { fake_datastore('fake-ds-cl2-1') }
@@ -65,19 +65,19 @@ def fake_cluster(name, *args)
6565
allow_any_instance_of(VSphereCloud::Resources::Datastore).to receive(:maintenance_mode?).and_return(false)
6666
allow_any_instance_of(VSphereCloud::VmPlacement).to receive(:migration_size).and_return(10)
6767
allow_any_instance_of(VSphereCloud::VmPlacement).to receive(:balance_score).and_return(10)
68-
allow(placement_1).to receive(:host_group_free_memory).and_return (10000)
69-
allow(placement_2).to receive(:host_group_free_memory).and_return (20000)
68+
allow(placement_1).to receive(:host_group_free_memory_mb).and_return (10000)
69+
allow(placement_2).to receive(:host_group_free_memory_mb).and_return (20000)
7070
expect(subject.to_a.first).to eq(placement_2)
7171
end
7272

7373
it 'sorts the placements in ascending order of migration size then descending order of balance score and then free memory' do
7474
allow_any_instance_of(VSphereCloud::Resources::Datastore).to receive(:maintenance_mode?).and_return(false)
7575
placement_4 = placement_3 = placement_5 = placement_1.dup
76-
allow(placement_1).to receive_messages(:host_group_free_memory => 10000, :balance_score => 10 , :migration_size => 10)
77-
allow(placement_2).to receive_messages(:host_group_free_memory => 20000, :balance_score => 10 , :migration_size => 10)
78-
allow(placement_3).to receive_messages(:host_group_free_memory => 10000, :balance_score => 4 , :migration_size => 5)
79-
allow(placement_4).to receive_messages(:host_group_free_memory => 10000, :balance_score => 7 , :migration_size => 5)
80-
allow(placement_5).to receive_messages(:host_group_free_memory => 40000, :balance_score => 10 , :migration_size => 1)
76+
allow(placement_1).to receive_messages(:host_group_free_memory_mb => 10000, :balance_score => 10 , :migration_size => 10)
77+
allow(placement_2).to receive_messages(:host_group_free_memory_mb => 20000, :balance_score => 10 , :migration_size => 10)
78+
allow(placement_3).to receive_messages(:host_group_free_memory_mb => 10000, :balance_score => 4 , :migration_size => 5)
79+
allow(placement_4).to receive_messages(:host_group_free_memory_mb => 10000, :balance_score => 7 , :migration_size => 5)
80+
allow(placement_5).to receive_messages(:host_group_free_memory_mb => 40000, :balance_score => 10 , :migration_size => 1)
8181
pipeline = described_class.new(*criteria) {[placement_1, placement_2, placement_3, placement_4, placement_5]}
8282
expect(pipeline.to_a).to eq([placement_5, placement_4, placement_3, placement_2, placement_1])
8383
end

0 commit comments

Comments
 (0)