Properly calculate a cluster's free memory

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

@@ -17,7 +17,7 @@ class Cluster
       HOST_PROPERTIES = %w(name hardware.memorySize runtime.connectionState runtime.inMaintenanceMode runtime.powerState)
       HOST_COUNTERS = %w(mem.usage.average)
 
-      MEMORY_HEADROOM = 128
+      MEMORY_HEADROOM_MB = 128
 
       # @!attribute mob
       #   @return [Vim::ClusterComputeResource] cluster vSphere MOB.
@@ -27,7 +27,7 @@ class Cluster
       #   @return [ResourcePool] resource pool.
       attr_reader :resource_pool
 
-      class FreeMemory < Struct.new(:cluster_free_memory, :host_group_free_memory); end
+      class FreeMemory < Struct.new(:cluster_free_memory_mb, :host_group_free_memory_mb); end
 
       # Creates a new Cluster resource from the specified datacenter, cluster
       # configuration, and prefetched properties.
@@ -60,7 +60,7 @@ def free_memory
                                   FreeMemory.new(fetch_host_group_utilization, fetch_host_group_utilization)
                                 end
         else
-          @synced_free_memory = FreeMemory.new(fetch_resource_pool_utilization, fetch_resource_pool_utilization)
+          @synced_free_memory = FreeMemory.new(fetch_resource_pool_utilization_mb, fetch_resource_pool_utilization_mb)
         end
       end
 
@@ -212,7 +212,7 @@ def fetch_cluster_utilization()
         raise "Failed to get utilization for cluster'#{self.mob.name}'" if properties.nil?
 
         compute_resource_summary = properties["summary"]
-        return compute_resource_summary.effective_memory
+        return compute_resource_summary.effective_memory - compute_resource_summary.usage_summary.mem_demand_mb
       end
 
       # Fetches the resource pool utilization from vSphere.
@@ -224,15 +224,17 @@ def fetch_cluster_utilization()
       # so we can't use it for the raw clusters.
       #
       # @return [void]
-      def fetch_resource_pool_utilization
+      def fetch_resource_pool_utilization_mb
         logger.debug("Fetching Memory utilization for Resource Pool #{resource_pool.name}")
         properties = @client.cloud_searcher.get_properties(resource_pool.mob, Vim::ResourcePool, 'summary')
         raise "Failed to get utilization for resource pool '#{resource_pool}'" if properties.nil?
 
         runtime_info = properties["summary"].runtime
         quick_stats = properties["summary"].quick_stats
         memory = runtime_info.memory
-        return (memory.max_usage - (quick_stats.host_memory_usage) * 1024) / BYTES_IN_MB
+        memory_max_mb = memory.max_usage / BYTES_IN_MB
+        host_memory_usage_mb = quick_stats.host_memory_usage
+        return memory_max_mb - host_memory_usage_mb
       end
     end
   end

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

@@ -219,7 +219,7 @@ def validate_clusters
     def cluster_placement_internal(clusters:)
       return @cluster_placement if @cluster_placement
 
-      vm_selection_placement_pipeline = VmPlacementSelectionPipeline.new(disk_config: disk_configurations, req_memory: vm_type.ram) do
+      vm_selection_placement_pipeline = VmPlacementSelectionPipeline.new(disk_config: disk_configurations, req_memory_mb: vm_type.ram) do
         logger.info("Gathering vm placement resources for vm placement allocator pipeline")
         clusters.map do |cluster|
           VmPlacement.new(cluster: cluster, datastores: cluster.accessible_datastores.values, hosts: nil)

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

@@ -38,12 +38,12 @@ def balance_score
       @balance_score = min + mean + median
     end
 
-    def cluster_free_memory
-      cluster.free_memory.cluster_free_memory
+    def cluster_free_memory_mb
+      cluster.free_memory.cluster_free_memory_mb
     end
 
-    def host_group_free_memory
-      cluster.free_memory.host_group_free_memory
+    def host_group_free_memory_mb
+      cluster.free_memory.host_group_free_memory_mb
     end
 
     def inspect_before
@@ -56,7 +56,7 @@ def inspect
     end
 
     def cluster_inspect
-      "VM Placement Cluster: #{cluster.name} Cluster free memory: #{cluster_free_memory}, host group free memory: #{host_group_free_memory}"
+      "VM Placement Cluster: #{cluster.name} Cluster free memory (MiB): #{cluster_free_memory_mb}, host group free memory (MiB): #{host_group_free_memory_mb}"
     end
 
     def datastore_inspect
@@ -66,29 +66,29 @@ def datastore_inspect
 
   class VmPlacementSelectionPipeline < SelectionPipeline
     class VmPlacementCriteria
-      DEFAULT_MEMORY_HEADROOM = 128
+      DEFAULT_MEMORY_HEADROOM_MB = 128
 
-      attr_reader :disk_config, :req_memory, :mem_headroom
+      attr_reader :disk_config, :req_memory_mb, :mem_headroom_mb
 
-      def required_memory
-        req_memory + mem_headroom
+      def required_memory_mb
+        req_memory_mb + mem_headroom_mb
       end
 
       def initialize(criteria = {})
         @disk_config = criteria[:disk_config]
-        @req_memory = criteria[:req_memory]
-        @mem_headroom = criteria[:mem_headroom] || DEFAULT_MEMORY_HEADROOM
+        @req_memory_mb = criteria[:req_memory_mb]
+        @mem_headroom_mb = criteria[:mem_headroom_mb] || DEFAULT_MEMORY_HEADROOM_MB
       end
 
       def inspect
-        "VM Placement Criteria: Disk Config: #{disk_config}  Req Memory: #{req_memory}  Mem Headroom: #{mem_headroom}"
+        "VM Placement Criteria: Disk Config: #{disk_config}  Req Memory (MiB): #{req_memory_mb}  Mem Headroom (MiB): #{mem_headroom_mb}"
       end
     end
     private_constant :VmPlacementCriteria
 
     with_filter do |vm_placement, criteria_object|
-      logger.debug("Filter #{vm_placement.cluster_inspect} for free memory required: #{criteria_object.required_memory}")
-      vm_placement.cluster_free_memory > criteria_object.required_memory
+      logger.debug("Filter #{vm_placement.cluster_inspect} for free memory required (MiB): #{criteria_object.required_memory_mb}")
+      vm_placement.cluster_free_memory_mb > criteria_object.required_memory_mb
     end
 
     with_filter ->(vm_placement, criteria_object) do
@@ -111,7 +111,7 @@ def inspect
         # There are two possibilities at this point. Either:
         #   1. The disk is an ephemeral disk
         #   2. The disk is a persistent disk that must be migrated
-        pipeline = DiskPlacementSelectionPipeline.new(disk.size, disk.target_datastore_pattern, nil, disk.ephemeral?, criteria_object.required_memory) do
+        pipeline = DiskPlacementSelectionPipeline.new(disk.size, disk.target_datastore_pattern, nil, disk.ephemeral?, criteria_object.required_memory_mb) do
           vm_placement.datastores
         end.with_filter do |storage_placement|
           # TODO: both accessible? and accessible_from? will be queried for
@@ -145,7 +145,7 @@ def inspect
     end
 
     with_scorer do |p1, p2|
-      -(p1.host_group_free_memory <=> p2.host_group_free_memory)
+      -(p1.host_group_free_memory_mb <=> p2.host_group_free_memory_mb)
     end
 
     def initialize(*args)

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

@@ -130,21 +130,21 @@ module VSphereCloud::Resources
                 overall_status: 'green',
                 memory: instance_double(
                   'VimSdk::Vim::ResourcePool::ResourceUsage',
-                  max_usage: 1024 * 1024 * 100,
+                  max_usage: 1024 * 1024 * 100, # 100 MiB
                 )
               )
             end
 
             let(:fake_quick_stats) do
               instance_double(
                 'VimSdk::Vim::ResourcePool::Summary::QuickStats',
-                host_memory_usage: 1024 * 75
+                host_memory_usage: 75 # 75 MiB
               )
             end
 
             it 'sets resources to values in the runtime status' do
-              expect(cluster.free_memory.cluster_free_memory).to eq(25)
-              expect(cluster.free_memory.host_group_free_memory).to eq(25)
+              expect(cluster.free_memory.cluster_free_memory_mb).to eq(25)
+              expect(cluster.free_memory.host_group_free_memory_mb).to eq(25)
             end
           end
         end
@@ -171,7 +171,8 @@ def generate_host_property(mob:, name:, connection_state:, maintenance_mode:, me
         let(:active_host_1_mob) { instance_double('VimSdk::Vim::ClusterComputeResource') }
         let(:active_host_2_mob) { instance_double('VimSdk::Vim::ClusterComputeResource') }
         let(:active_host_mobs) { [active_host_1_mob, active_host_2_mob] }
-        let(:compute_summary) { instance_double('VimSdk::Vim::ComputeResource::Summary') }
+        let(:compute_summary) { instance_double('VimSdk::Vim::ClusterComputeResource::Summary') }
+        let(:cluster_usage_summary) { instance_double('VimSdk::Vim::Cluster::UsageSummary') }
         let(:active_hosts_properties) do
           {}.merge(
             generate_host_property(mob: active_host_1_mob, name: 'mob-1', maintenance_mode: false, memory_size: 100 * 1024 * 1024, power_state: 'poweredOn', connection_state: 'connected')
@@ -183,12 +184,14 @@ def generate_host_property(mob:, name:, connection_state:, maintenance_mode:, me
 
         before do
           allow(cloud_searcher).to receive(:get_properties).and_return({"summary" => compute_summary})
-          allow(compute_summary).to receive(:effective_memory).and_return(85)
+          allow(compute_summary).to receive(:effective_memory).and_return(90)
+          allow(compute_summary).to receive(:usage_summary).and_return(cluster_usage_summary)
+          allow(cluster_usage_summary).to receive(:mem_demand_mb).and_return(5)
         end
 
         it 'sets resources to values based on the active hosts in the cluster' do
-          expect(cluster.free_memory.cluster_free_memory).to eq(85)
-          expect(cluster.free_memory.host_group_free_memory).to eq(85)
+          expect(cluster.free_memory.cluster_free_memory_mb).to eq(85)
+          expect(cluster.free_memory.host_group_free_memory_mb).to eq(85)
         end
 
         context 'when an ESXi host is not powered on' do
@@ -201,8 +204,8 @@ def generate_host_property(mob:, name:, connection_state:, maintenance_mode:, me
           end
 
           it 'includes the free memory of only powered on hosts' do
-            expect(cluster.free_memory.cluster_free_memory).to eq(85)
-            expect(cluster.free_memory.host_group_free_memory).to eq(85)
+            expect(cluster.free_memory.cluster_free_memory_mb).to eq(85)
+            expect(cluster.free_memory.host_group_free_memory_mb).to eq(85)
           end
         end
 
@@ -216,8 +219,8 @@ def generate_host_property(mob:, name:, connection_state:, maintenance_mode:, me
           end
 
           it 'includes the free memory of only connected hosts' do
-            expect(cluster.free_memory.cluster_free_memory).to eq(85)
-            expect(cluster.free_memory.host_group_free_memory).to eq(85)
+            expect(cluster.free_memory.cluster_free_memory_mb).to eq(85)
+            expect(cluster.free_memory.host_group_free_memory_mb).to eq(85)
           end
         end
 
@@ -231,8 +234,8 @@ def generate_host_property(mob:, name:, connection_state:, maintenance_mode:, me
           end
 
           it 'includes the free memory of only hosts not in maintenance mode' do
-            expect(cluster.free_memory.cluster_free_memory).to eq(85)
-            expect(cluster.free_memory.host_group_free_memory).to eq(85)
+            expect(cluster.free_memory.cluster_free_memory_mb).to eq(85)
+            expect(cluster.free_memory.host_group_free_memory_mb).to eq(85)
           end
         end
 
@@ -244,10 +247,11 @@ def generate_host_property(mob:, name:, connection_state:, maintenance_mode:, me
           end
           before do
             allow(compute_summary).to receive(:effective_memory).and_return(0)
+            allow(cluster_usage_summary).to receive(:mem_demand_mb).and_return(0)
           end
           it 'defaults free memory to zero' do
-            expect(cluster.free_memory.cluster_free_memory).to eq(0)
-            expect(cluster.free_memory.host_group_free_memory).to eq(0)
+            expect(cluster.free_memory.cluster_free_memory_mb).to eq(0)
+            expect(cluster.free_memory.host_group_free_memory_mb).to eq(0)
           end
         end
       end
@@ -282,29 +286,32 @@ def generate_host_property(mob:, name:, connection_state:, maintenance_mode:, me
 
         context 'when host group rule type is MUST' do
           it 'returns sum of raw available memory on two hosts in Megabytes for cluster free memory' do
-            expect(cluster.free_memory.cluster_free_memory).to eq(18)
+            expect(cluster.free_memory.cluster_free_memory_mb).to eq(18)
           end
 
           it 'returns sum of raw available memory on two hosts in Megabytes for host group free memory' do
-            expect(cluster.free_memory.host_group_free_memory).to eq(18)
+            expect(cluster.free_memory.host_group_free_memory_mb).to eq(18)
           end
         end
 
         context 'when host group rule type is SHOULD' do
-          let(:compute_summary) { instance_double('VimSdk::Vim::ComputeResource::Summary') }
+          let(:compute_summary) { instance_double('VimSdk::Vim::ClusterComputeResource::Summary') }
+          let(:cluster_usage_summary) { instance_double('VimSdk::Vim::Cluster::UsageSummary') }
 
           before do
             allow(subject).to receive(:host_group_rule_type).and_return('SHOULD')
             allow(cloud_searcher).to receive(:get_properties).and_return({"summary" => compute_summary})
-            allow(compute_summary).to receive(:effective_memory).and_return(85)
+            allow(compute_summary).to receive(:usage_summary).and_return(cluster_usage_summary)
+            allow(compute_summary).to receive(:effective_memory).and_return(90)
+            allow(cluster_usage_summary).to receive(:mem_demand_mb).and_return(5)
           end
 
           it 'returns full cluster free memory in Megabytes for cluster free memory' do
-            expect(cluster.free_memory.cluster_free_memory).to eq(85)
+            expect(cluster.free_memory.cluster_free_memory_mb).to eq(85)
           end
 
           it 'returns sum of raw available memory on two hosts in Megabytes for host group free memory' do
-            expect(cluster.free_memory.host_group_free_memory).to eq(18)
+            expect(cluster.free_memory.host_group_free_memory_mb).to eq(18)
           end
         end
 
@@ -315,8 +322,8 @@ def generate_host_property(mob:, name:, connection_state:, maintenance_mode:, me
           end
 
           it 'returns 0' do
-            expect(cluster.free_memory.cluster_free_memory).to eq(0)
-            expect(cluster.free_memory.host_group_free_memory).to eq(0)
+            expect(cluster.free_memory.cluster_free_memory_mb).to eq(0)
+            expect(cluster.free_memory.host_group_free_memory_mb).to eq(0)
           end
         end
 
@@ -329,8 +336,8 @@ def generate_host_property(mob:, name:, connection_state:, maintenance_mode:, me
           end
 
           it 'returns 0' do
-            expect(cluster.free_memory.cluster_free_memory).to eq(0)
-            expect(cluster.free_memory.host_group_free_memory).to eq(0)
+            expect(cluster.free_memory.cluster_free_memory_mb).to eq(0)
+            expect(cluster.free_memory.host_group_free_memory_mb).to eq(0)
           end
         end
       end
@@ -343,21 +350,21 @@ def generate_host_property(mob:, name:, connection_state:, maintenance_mode:, me
           overall_status: 'green',
           memory: instance_double(
             'VimSdk::Vim::ResourcePool::ResourceUsage',
-            max_usage: 1024 * 1024 * 100,
+            max_usage: 1024 * 1024 * 100, # 100 MiB
           )
         )
       end
 
       let(:fake_quick_stats) do
         instance_double(
           'VimSdk::Vim::ResourcePool::Summary::QuickStats',
-          host_memory_usage: 1024 * 75
+          host_memory_usage: 75 # 75 MiB
         )
       end
 
       it 'returns the amount of free memory in the cluster' do
-        expect(cluster.free_memory.cluster_free_memory).to eq(25)
-        expect(cluster.free_memory.host_group_free_memory).to eq(25)
+        expect(cluster.free_memory.cluster_free_memory_mb).to eq(25)
+        expect(cluster.free_memory.host_group_free_memory_mb).to eq(25)
       end
 
       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

@@ -26,7 +26,7 @@ def fake_cluster(name, *args)
   end
 
   # Simulating a mini datacenter with two clusters. Each cluster has two dedicated datastores and one shared datastore.
-  let(:criteria) { [disk_config: disk_config, req_memory: 1024] }
+  let(:criteria) { [disk_config: disk_config, req_memory_mb: 1024] }
   let(:ds_cl1_1) { fake_datastore('fake-ds-cl1-1') }
   let(:ds_cl1_2) { fake_datastore('fake-ds-cl1-2') }
   let(:ds_cl2_1) { fake_datastore('fake-ds-cl2-1') }
@@ -65,19 +65,19 @@ def fake_cluster(name, *args)
       allow_any_instance_of(VSphereCloud::Resources::Datastore).to receive(:maintenance_mode?).and_return(false)
       allow_any_instance_of(VSphereCloud::VmPlacement).to receive(:migration_size).and_return(10)
       allow_any_instance_of(VSphereCloud::VmPlacement).to receive(:balance_score).and_return(10)
-      allow(placement_1).to receive(:host_group_free_memory).and_return (10000)
-      allow(placement_2).to receive(:host_group_free_memory).and_return (20000)
+      allow(placement_1).to receive(:host_group_free_memory_mb).and_return (10000)
+      allow(placement_2).to receive(:host_group_free_memory_mb).and_return (20000)
       expect(subject.to_a.first).to eq(placement_2)
     end
 
     it 'sorts the placements in ascending order of migration size then descending order of balance score and then free memory' do
       allow_any_instance_of(VSphereCloud::Resources::Datastore).to receive(:maintenance_mode?).and_return(false)
       placement_4 = placement_3 = placement_5 = placement_1.dup
-      allow(placement_1).to receive_messages(:host_group_free_memory => 10000, :balance_score => 10 , :migration_size => 10)
-      allow(placement_2).to receive_messages(:host_group_free_memory => 20000, :balance_score => 10 , :migration_size => 10)
-      allow(placement_3).to receive_messages(:host_group_free_memory => 10000, :balance_score => 4 , :migration_size => 5)
-      allow(placement_4).to receive_messages(:host_group_free_memory => 10000, :balance_score => 7 , :migration_size => 5)
-      allow(placement_5).to receive_messages(:host_group_free_memory => 40000, :balance_score => 10 , :migration_size => 1)
+      allow(placement_1).to receive_messages(:host_group_free_memory_mb => 10000, :balance_score => 10 , :migration_size => 10)
+      allow(placement_2).to receive_messages(:host_group_free_memory_mb => 20000, :balance_score => 10 , :migration_size => 10)
+      allow(placement_3).to receive_messages(:host_group_free_memory_mb => 10000, :balance_score => 4 , :migration_size => 5)
+      allow(placement_4).to receive_messages(:host_group_free_memory_mb => 10000, :balance_score => 7 , :migration_size => 5)
+      allow(placement_5).to receive_messages(:host_group_free_memory_mb => 40000, :balance_score => 10 , :migration_size => 1)
       pipeline = described_class.new(*criteria) {[placement_1, placement_2, placement_3, placement_4, placement_5]}
       expect(pipeline.to_a).to eq([placement_5, placement_4, placement_3, placement_2, placement_1])
     end