Revert "Only place disks on cluster-accessible datastores"

- These changes were originally made because it was believed that in deployments
  where "vertical" storage was used (that is, storage accessible only to
  a specific cluster, not all clusters), the disk could sometimes be
  incorrectly placed, resulting in a CPI/deployment failure.
- Symptoms of this failure were errors like "Unable to access the
  virtual machine configuration", and "Invalid configuration for device
  '0'."
- There were also reports of "No valid hosts could be found due to Das
  state" with this topology.
- These prior changes ran some additional accessibility checks that
  verified that all datastores connected to a cluster were ALSO
  explicitly mounted by each ESXi host in that cluster. This behavior
  was identified as causing large performance degradations to
  deployments with many hosts and/or datastores.
- Originally it was thought the errors seen above could occur _with any_
  vertical storage configuration, further investigation has demonstrated
  that this only occurs if:

  * a deployment uses vertical storage.
  * datastores specified by the deployment are _connected_ but not used
    by cluster hosts (e.g., a datastores has been detached and unmounted from
    a host BUT the host has not rescanned disks.)

  In this circumstance, the API reports this disconnected datastore as
  being a part of the cluster, but these hosts will not be able to
  access it.

- While this prior change is _technically correct_ as it guards against
  a possible but likely unintentional configuration, given the
  associated performance cost (borne by all users, including those with
  "correct" vertical storage configurations and those not using vertical
  storage at all) it makes sense to remove this extra check.

- n.b. the behavior after revert still checks: 1) that the datastore is
  "connected" to hosts on the cluster and 2) the datastore is
  "accessible" (turned on).

This reverts commits 3d50f05 and 3ff70d1

Signed-off-by: Brian Cunnie <bcunnie@vmware.com>

Author: ragaskar

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

@@ -153,7 +153,7 @@ def accessible_datastores
         @accessible_datastores ||= Datastore.build_from_client(
           @client,
           properties['datastore']
-        ).select { |datastore| datastore.accessible_from?(@mob) }
+        ).select { |datastore| datastore.accessible }
         .inject({}) do |acc, datastore|
           acc[datastore.name] = datastore
           acc

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

@@ -89,11 +89,10 @@ def accessible?
       # maintenance mode and is part of the very same cluster that datastore
       # can access
       #
-      def accessible_from?(cluster_mob)
+      def accessible_from?(cluster)
         @mob.host.any? do |host_mount|
           next if host_mount.key.runtime.in_maintenance_mode
-          next unless host_mount.mount_info.accessible
-          cluster_mob.host.include?(host_mount.key)
+          cluster.host.include?(host_mount.key)
         end
       end
 

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

@@ -30,11 +30,6 @@ def cluster
         cluster['name']
       end
 
-      def cluster_mob
-        cluster = cloud_searcher.get_properties(host_properties['parent'], Vim::ClusterComputeResource, 'name', ensure_all: true)
-        cluster[:obj]
-      end
-
       def resource_pool
         properties['resourcePool'].name
       end
@@ -46,7 +41,7 @@ def accessible_datastores
             ensure_all: true
         ).select(&:accessible)
             .inject({}) do |acc, datastore|
-          acc[datastore.name] = datastore if datastore.accessible_from?(cluster_mob)
+          acc[datastore.name] = datastore
           acc
         end
       end

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

@@ -35,8 +35,7 @@ module VSphereCloud::Resources
         'resourcePool' => fake_resource_pool_mob,
       }
     end
-    let(:fake_cluster_mob_host) { double('VimSdk::Vim::HostSystem') }
-    let(:cluster_mob) { instance_double('VimSdk::Vim::ClusterComputeResource', name: 'cluster1', host: fake_cluster_mob_host) }
+    let(:cluster_mob) { instance_double('VimSdk::Vim::ClusterComputeResource', name: 'cluster1') }
     let(:cluster_hosts) { [instance_double('VimSdk::Vim::HostSystem')] }
     let(:fake_resource_pool_mob) { instance_double('VimSdk::Vim::ResourcePool') }
 
@@ -47,29 +46,22 @@ module VSphereCloud::Resources
     end
     let(:fake_resource_pool_mob) { instance_double('VimSdk::Vim::ResourcePool') }
 
-    let(:fake_available_host_mount_mob) { instance_double('VimSdk::Vim::Datastore::HostMount', key: 'an-INCLUDED-key') }
-    let(:fake_unavailable_host_mount_mob) { instance_double('VimSdk::Vim::Datastore::HostMount', key: 'an-EXCLUDED-key') }
-
-    let(:ephemeral_store_datastore_mob) { instance_double('VimSdk::Vim::Datastore', host: [fake_available_host_mount_mob]) }
-    let(:ephemeral_store_properties) { {obj: ephemeral_store_datastore_mob,'name' => 'ephemeral_1', 'summary.freeSpace' => 15000 * BYTES_IN_MB} }
-    let(:ephemeral_store_2_datastore_mob) { instance_double('VimSdk::Vim::Datastore', host: [fake_available_host_mount_mob]) }
-    let(:ephemeral_store_2_properties) { {obj: ephemeral_store_2_datastore_mob,'name' => 'ephemeral_2', 'summary.freeSpace' => 25000 * BYTES_IN_MB} }
-    let(:persistent_store_datastore_mob) { instance_double('VimSdk::Vim::Datastore', host: [fake_available_host_mount_mob]) }
-    let(:persistent_store_properties) { {obj: persistent_store_datastore_mob,'name' => 'persistent_1', 'summary.freeSpace' => 10000 * BYTES_IN_MB, 'summary.capacity' => 20000 * BYTES_IN_MB} }
-    let(:persistent_store_2_datastore_mob) { instance_double('VimSdk::Vim::Datastore', host: [fake_available_host_mount_mob]) }
-    let(:persistent_store_2_properties) { {obj: persistent_store_2_datastore_mob,'name' => 'persistent_2', 'summary.freeSpace' => 20000 * BYTES_IN_MB, 'summary.capacity' => 40000 * BYTES_IN_MB} }
-    let(:inaccessible_persistent_store_datastore_mob) { instance_double('VimSdk::Vim::Datastore', host: [fake_unavailable_host_mount_mob]) }
-    let(:inaccessible_persistent_store_properties) { {obj: inaccessible_persistent_store_datastore_mob,'name' => 'persistent_inaccess'} }
-    let(:other_store_properties) { {obj: inaccessible_persistent_store_datastore_mob, 'name' => 'other' } }
+    let(:ephemeral_store_properties) { {'name' => 'ephemeral_1', 'summary.accessible' => true, 'summary.freeSpace' => 15000 * BYTES_IN_MB} }
+    let(:ephemeral_store_2_properties) { {'name' => 'ephemeral_2', 'summary.accessible' => true, 'summary.freeSpace' => 25000 * BYTES_IN_MB} }
+    let(:persistent_store_properties) { {'name' => 'persistent_1', 'summary.accessible' => true, 'summary.freeSpace' => 10000 * BYTES_IN_MB, 'summary.capacity' => 20000 * BYTES_IN_MB} }
+    let(:persistent_store_2_properties) { {'name' => 'persistent_2',  'summary.accessible' => true, 'summary.freeSpace' => 20000 * BYTES_IN_MB, 'summary.capacity' => 40000 * BYTES_IN_MB} }
+    let(:inaccessible_persistent_store_properties) { {'name' => 'persistent_inaccess', 'summary.accessible' => false} }
+
+    let(:other_store_properties) { { 'name' => 'other' } }
 
     let(:fake_datastore_properties) do
       {
-        keys: ephemeral_store_properties,
-        are: ephemeral_store_2_properties,
-        ignored: persistent_store_properties,
-        in: persistent_store_2_properties,
-        this: inaccessible_persistent_store_properties,
-        hash: other_store_properties,
+        instance_double('VimSdk::Vim::Datastore') => ephemeral_store_properties,
+        instance_double('VimSdk::Vim::Datastore') => ephemeral_store_2_properties,
+        instance_double('VimSdk::Vim::Datastore') => persistent_store_properties,
+        instance_double('VimSdk::Vim::Datastore') => persistent_store_2_properties,
+        instance_double('VimSdk::Vim::Datastore') => inaccessible_persistent_store_properties,
+        instance_double('VimSdk::Vim::Datastore') => other_store_properties,
       }
     end
 
@@ -103,17 +95,6 @@ module VSphereCloud::Resources
     end
 
     describe '#accessible_datastores' do
-      before do
-        allow(fake_available_host_mount_mob).to receive_message_chain(:key, :runtime, :in_maintenance_mode).and_return(false)
-        allow(fake_available_host_mount_mob).to receive_message_chain(:mount_info,:accessible).and_return(true)
-
-        allow(fake_unavailable_host_mount_mob).to receive_message_chain(:key, :runtime, :in_maintenance_mode).and_return(false)
-        allow(fake_unavailable_host_mount_mob).to receive_message_chain(:mount_info,:accessible).and_return(true)
-
-        allow(fake_cluster_mob_host).to receive(:include?).with(fake_available_host_mount_mob.key).and_return(true)
-        allow(fake_cluster_mob_host).to receive(:include?).with(fake_unavailable_host_mount_mob.key).and_return(false)
-      end
-
       it 'returns the full list of datastores without inaccessible stores' do
         accessible_datastores = cluster.accessible_datastores
         expect(accessible_datastores.keys).to match_array(%w(persistent_1 persistent_2 ephemeral_1 ephemeral_2))

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

@@ -85,17 +85,13 @@
       context 'when all hosts are in maintenance mode' do
         let(:maintenance_mode) { true }
         let(:is_included) { true }
-        before do
-          allow(host_mount).to receive_message_chain(:mount_info, :accessible).and_return(true)
-        end
         it 'is false' do
           expect(subject).to_not be_accessible_from(cluster)
         end
       end
 
       context 'when at least one of the hosts is not in maintenance mode' do
         before do
-          allow(host_mount).to receive_message_chain(:mount_info, :accessible).and_return(true)
           expect(cluster).to receive_message_chain(:host, :include?).with(host_mob).and_return(is_included)
         end
         let(:maintenance_mode) { false }
@@ -113,25 +109,6 @@
           end
         end
       end
-
-      context 'datastore-host accessibility' do
-        let(:maintenance_mode) { false }
-        before do
-          expect(host_mount).to receive_message_chain(:mount_info, :accessible).and_return(is_accessible)
-        end
-        context "when the host can access the datastore" do
-          let(:is_accessible) { true }
-          it 'is true' do
-            expect(subject).to be_accessible_from(cluster)
-          end
-        end
-        context "when the host can't access the datastore" do
-          let(:is_accessible) { false }
-          it 'is false' do
-            expect(subject).to_not be_accessible_from(cluster)
-          end
-        end
-      end
     end
   end
 

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

@@ -34,50 +34,20 @@
 
   context 'accessible datastores' do
     let(:bytes_in_mb) {1024 * 1024}
-    let(:cluster_mob) { instance_double('VimSdk::Vim::ClusterComputeResource') }
     let(:accessible_datastore_properties) { {'name' => 'datastore-name',  'summary.accessible' => true, 'summary.freeSpace' => 20000 * bytes_in_mb, 'summary.capacity' => 40000 * bytes_in_mb} }
     let(:inaccessible_datastore_properties) { {'name' => 'inaccessible-datastore-name', 'summary.accessible' => false} }
-    let(:vertically_inaccessible_datastore_properties) { {'name' => 'vertically-inaccessible-datastore-name', 'summary.accessible' => true} }
     let(:datastore_mob) { instance_double('VimSdk::Vim::Datastore') }
     let(:inaccessible_datastore_mob) { instance_double('VimSdk::Vim::Datastore') }
-    let(:vertically_inaccessible_datastore_mob) { instance_double('VimSdk::Vim::Datastore') }
     let(:datastore_properties) do
       {
           instance_double('VimSdk::Vim::Datastore') => accessible_datastore_properties,
-          instance_double('VimSdk::Vim::Datastore') => inaccessible_datastore_properties,
-          instance_double('VimSdk::Vim::Datastore') => vertically_inaccessible_datastore_properties
+          instance_double('VimSdk::Vim::Datastore') => inaccessible_datastore_properties
       }
     end
-
     before do
-      accessible_ds = instance_double(VSphereCloud::Resources::Datastore,
-        name: "datastore-name",
-        maintenance_mode?: false,
-        accessible_from?: true,
-        accessible: true
-      )
-      inaccessible_ds = instance_double(VSphereCloud::Resources::Datastore,
-        name: "inaccessible-datastore-name",
-        maintenance_mode?: false,
-        accessible_from?: true,
-        accessible: false
-      )
-      vert_inaccessible_ds = instance_double(VSphereCloud::Resources::Datastore,
-        name: "inaccessible-datastore-name",
-        maintenance_mode?: false,
-        accessible_from?: false,
-        accessible: true
-      )
-
       host_properties = {
-        'datastore' => [datastore_mob, inaccessible_datastore_mob, vertically_inaccessible_datastore_mob]
+          'datastore' => [datastore_mob, inaccessible_datastore_mob]
       }
-      allow(cloud_searcher).to receive(:get_properties).with(
-        host_properties['parent'],
-        VimSdk::Vim::ClusterComputeResource,
-        "name",
-        ensure_all: true,
-      ).and_return({obj: cluster_mob})
       allow(cloud_searcher).to receive(:get_properties).with(
           'vm-host',
           VimSdk::Vim::HostSystem,
@@ -90,11 +60,11 @@
           VSphereCloud::Resources::Datastore::PROPERTIES,
           ensure_all: true,
       ).and_return(datastore_properties)
-      expect(VSphereCloud::Resources::Datastore).to receive(:build_from_client).and_return([accessible_ds,inaccessible_ds,vert_inaccessible_ds])
     end
     describe '#accessible_datastores' do
       it 'returns list of accessible datastores' do
         expect(vm.accessible_datastores.keys).to match_array(['datastore-name'])
+        expect(vm.accessible_datastores['datastore-name']).to be_a(VSphereCloud::Resources::Datastore)
       end
     end
     describe '#accessible_datastore_names' do

src/vsphere_cpi/spec/unit/cloud/vsphere/vm_config_spec.rb

@@ -388,9 +388,8 @@ def fake_datastore(name, free_space, mob = nil)
         end
         let(:global_clusters) { [cluster_1, cluster_2] }
         let(:host_runtime_info) { instance_double(VimSdk::Vim::Host::RuntimeInfo, in_maintenance_mode: false) }
-        let(:host_mount_info) { instance_double(VimSdk::Vim::Host::MountInfo, accessible: true) }
         let(:host_system) {instance_double(VimSdk::Vim::HostSystem, runtime: host_runtime_info)}
-        let(:datastore_host_mount) { [instance_double('VimSdk::Vim::Datastore::HostMount', key: host_system, mount_info: host_mount_info)]}
+        let(:datastore_host_mount) { [instance_double('VimSdk::Vim::Datastore::HostMount', key: host_system)]}
         let(:smaller_datastore_mob) { instance_double('VimSdk::Vim::Datastore', host: datastore_host_mount) }
         let(:larger_datastore_mob) { instance_double('VimSdk::Vim::Datastore', host: datastore_host_mount) }
         let(:smaller_ds) { fake_datastore( 'smaller-ds', 512, smaller_datastore_mob) }