From b5f8585711b5a3ca70437386104e543a8d25678b Mon Sep 17 00:00:00 2001 From: Ladislav Smola Date: Fri, 11 May 2018 14:08:26 +0200 Subject: [PATCH 1/8] Check if reference contains only references to primary indexes inside Check if reference contains only references to primary indexes inside --- .../inventory_collection/reference.rb | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/app/models/manager_refresh/inventory_collection/reference.rb b/app/models/manager_refresh/inventory_collection/reference.rb index 94c8d662a66..8b278d511d2 100644 --- a/app/models/manager_refresh/inventory_collection/reference.rb +++ b/app/models/manager_refresh/inventory_collection/reference.rb @@ -16,14 +16,19 @@ def initialize(data, ref, keys) @ref = ref @keys = keys + @nested_secondary_index = keys.select { |key| full_reference[key].kind_of?(ManagerRefresh::InventoryObjectLazy) }.any? do |key| + !full_reference[key].primary? + end + @stringified_reference = self.class.build_stringified_reference(full_reference, keys) end - # Return true if reference is to primary index, false otherwise. + # Return true if reference is to primary index, false otherwise. Reference is primary, only if all the nested + # references are primary. # # @return [Boolean] true if reference is to primary index, false otherwise def primary? - ref == :manager_ref + ref == :manager_ref && !nested_secondary_index end class << self @@ -57,7 +62,7 @@ def stringify_reference(reference) # Returns joiner for string UIID # - # @return [String] Joiner for stirng UIID + # @return [String] Joiner for string UIID def stringify_joiner "__" end @@ -65,6 +70,10 @@ def stringify_joiner private + # @return Returns true if reference has nested references that are not pointing to primary index, but to + # secondary indexes. + attr_reader :nested_secondary_index + # Returns original Hash, or build hash out of passed string # # @param data [Hash, String] Passed data From d6933effb201cc7f0bde61e2eb6d601d52e9a1dd Mon Sep 17 00:00:00 2001 From: Ladislav Smola Date: Fri, 11 May 2018 14:09:17 +0200 Subject: [PATCH 2/8] Assert we build InventoryObject only with references to primary index Assert we build InventoryObject only with references to primary index --- .../inventory_collection/data_storage.rb | 21 ++++++++++++++++--- .../inventory_collection/index/proxy.rb | 1 + .../manager_refresh/inventory_object_lazy.rb | 2 +- 3 files changed, 20 insertions(+), 4 deletions(-) diff --git a/app/models/manager_refresh/inventory_collection/data_storage.rb b/app/models/manager_refresh/inventory_collection/data_storage.rb index dd2ef841bd0..132d46bc75a 100644 --- a/app/models/manager_refresh/inventory_collection/data_storage.rb +++ b/app/models/manager_refresh/inventory_collection/data_storage.rb @@ -17,7 +17,8 @@ class DataStorage :skeletal_primary_index, :to => :index_proxy - delegate :builder_params, + delegate :association_to_foreign_key_mapping, + :builder_params, :inventory_object?, :inventory_object_lazy?, :manager_ref, @@ -133,12 +134,26 @@ def from_hash(inventory_objects_data, available_inventory_collections) def primary_index_scan(hash) hash = enrich_data(hash) + assert_all_keys_present(hash) + assert_only_primary_index(hash) + + uuid = ::ManagerRefresh::InventoryCollection::Reference.build_stringified_reference(hash, named_ref) + return hash, uuid, primary_index.find(uuid) + end + + def assert_all_keys_present(hash) if manager_ref.any? { |x| !hash.key?(x) } raise "Needed find_or_build_by keys are: #{manager_ref}, data provided: #{hash}" end + end - uuid = ::ManagerRefresh::InventoryCollection::Reference.build_stringified_reference(hash, named_ref) - return hash, uuid, primary_index.find(uuid) + def assert_only_primary_index(data) + named_ref.each do |key| + if data[key].kind_of?(ManagerRefresh::InventoryObjectLazy) && !data[key].primary? + raise "Wrong index for key :#{key}, all references under this index must point to default :ref called"\ + " :manager_ref. Any other :ref is not valid. This applies also to nested lazy links." + end + end end # Returns new hash enriched by (see ManagerRefresh::InventoryCollection#builder_params) hash diff --git a/app/models/manager_refresh/inventory_collection/index/proxy.rb b/app/models/manager_refresh/inventory_collection/index/proxy.rb index e19fde4115b..d666cdfd38a 100644 --- a/app/models/manager_refresh/inventory_collection/index/proxy.rb +++ b/app/models/manager_refresh/inventory_collection/index/proxy.rb @@ -47,6 +47,7 @@ def initialize(inventory_collection, secondary_refs = {}) # @return [ManagerRefresh::InventoryObject] Passed InventoryObject def build_primary_index_for(inventory_object) # Building the object, we need to provide all keys of a primary index + assert_index(inventory_object.data, primary_index_ref) primary_index.store_index_for(inventory_object) end diff --git a/app/models/manager_refresh/inventory_object_lazy.rb b/app/models/manager_refresh/inventory_object_lazy.rb index a6bf31a7e39..ed76df0e78f 100644 --- a/app/models/manager_refresh/inventory_object_lazy.rb +++ b/app/models/manager_refresh/inventory_object_lazy.rb @@ -4,7 +4,7 @@ class InventoryObjectLazy attr_reader :reference, :inventory_collection, :key, :default - delegate :stringified_reference, :ref, :[], :to => :reference + delegate :primary?, :stringified_reference, :ref, :[], :to => :reference # @param inventory_collection [ManagerRefresh::InventoryCollection] InventoryCollection object owning the # InventoryObject From 9374719bdeaef4de5e5d0cb77ce535353f5dba51 Mon Sep 17 00:00:00 2001 From: Ladislav Smola Date: Fri, 11 May 2018 14:10:19 +0200 Subject: [PATCH 3/8] Test that secondary reference, used for InventoryObject manager ref will fail Test that secondary reference, used for InventoryObject manager ref will fail --- .../manager_refresh/persister/finders_spec.rb | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/spec/models/manager_refresh/persister/finders_spec.rb b/spec/models/manager_refresh/persister/finders_spec.rb index 198b310c72a..9841178fb10 100644 --- a/spec/models/manager_refresh/persister/finders_spec.rb +++ b/spec/models/manager_refresh/persister/finders_spec.rb @@ -148,4 +148,31 @@ it "checks find_or_build_by finds existing inventory object instead of duplicating" do expect(persister.vms.find_or_build_by(vm_data(1)).object_id).to eq(persister.vms.find_or_build_by(vm_data(1)).object_id) end + + it "raises exception unless only primary index is used in nested lazy_find when building" do + name = vm_data(1)[:name] + vm_lazy = persister.vms.lazy_find({:name => name}, :ref => :by_name) + + persister.vms.build(vm_data(1)) + + expected_error = "Wrong index for key :vm_or_template, all references under this index must point to default :ref"\ + " called :manager_ref. Any other :ref is not valid. This applies also to nested lazy links." + expect do + persister.hardwares.build(hardware_data(1, :vm_or_template => vm_lazy)) + end.to(raise_error(expected_error)) + end + + it "raises exception unless only primary index is used in deep nested lazy_find when building" do + name = vm_data(1)[:name] + vm_lazy = persister.vms.lazy_find({:name => name}, :ref => :by_name) + hardware_lazy = persister.hardwares.lazy_find(:vm_or_template => vm_lazy) + + persister.vms.build(vm_data(1)) + + expected_error = "Wrong index for key :hardware, all references under this index must point to default :ref called"\ + " :manager_ref. Any other :ref is not valid. This applies also to nested lazy links." + expect do + persister.disks.build(disk_data(1, :hardware => hardware_lazy)) + end.to(raise_error(expected_error)) + end end From dfa5ec80f83af18eab5fb99dcc1820476d868f4e Mon Sep 17 00:00:00 2001 From: Ladislav Smola Date: Fri, 11 May 2018 15:00:29 +0200 Subject: [PATCH 4/8] Transform nested references with secondary index on load Transform nested references with secondary index on load --- .../inventory_collection/reference.rb | 4 +++ .../manager_refresh/inventory_object_lazy.rb | 25 +++++++++++++++++-- 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/app/models/manager_refresh/inventory_collection/reference.rb b/app/models/manager_refresh/inventory_collection/reference.rb index 8b278d511d2..a233d017f90 100644 --- a/app/models/manager_refresh/inventory_collection/reference.rb +++ b/app/models/manager_refresh/inventory_collection/reference.rb @@ -31,6 +31,10 @@ def primary? ref == :manager_ref && !nested_secondary_index end + def nested_secondary_index? + nested_secondary_index + end + class << self # Builds string uuid from passed Hash and keys # diff --git a/app/models/manager_refresh/inventory_object_lazy.rb b/app/models/manager_refresh/inventory_object_lazy.rb index ed76df0e78f..c096222bc5b 100644 --- a/app/models/manager_refresh/inventory_object_lazy.rb +++ b/app/models/manager_refresh/inventory_object_lazy.rb @@ -4,7 +4,7 @@ class InventoryObjectLazy attr_reader :reference, :inventory_collection, :key, :default - delegate :primary?, :stringified_reference, :ref, :[], :to => :reference + delegate :stringified_reference, :ref, :[], :to => :reference # @param inventory_collection [ManagerRefresh::InventoryCollection] InventoryCollection object owning the # InventoryObject @@ -40,6 +40,8 @@ def inspect # @return [ManagerRefresh::InventoryObject, Object] ManagerRefresh::InventoryObject instance or an attribute # on key def load + transform_nested_secondary_indexes! if nested_secondary_index? + key ? load_object_with_key : load_object end @@ -70,10 +72,29 @@ def association?(key) !inventory_collection.association_to_foreign_key_mapping[key].nil? end + def transform_nested_secondary_indexes!(depth = 0) + keys.each do |x| + attr = full_reference[x] + next unless attr.kind_of?(ManagerRefresh::InventoryObjectLazy) + next if attr.primary? + + if attr.nested_secondary_index? + attr.transform_nested_secondary_indexes!(depth + 1) + end + + full_reference[x] = full_reference[x].load + end + + # Rebuild the reference to get the right value + self.reference = inventory_collection.build_reference(full_reference, ref) + end + private delegate :parallel_safe?, :saved?, :saver_strategy, :skeletal_primary_index, :targeted?, :to => :inventory_collection - delegate :full_reference, :keys, :primary?, :to => :reference + delegate :nested_secondary_index?, :primary?, :full_reference, :keys, :primary?, :to => :reference + + attr_writer :reference # Instead of loading the reference from the DB, we'll add the skeletal InventoryObject (having manager_ref and # info from the builder_params) to the correct InventoryCollection. Which will either be found in the DB or From 011f8094c504a1e2b00eae8f9b0ff44770813e39 Mon Sep 17 00:00:00 2001 From: Ladislav Smola Date: Fri, 11 May 2018 15:01:05 +0200 Subject: [PATCH 5/8] Test nested references with secondary index work Test nested references with secondary index work --- .../models/manager_refresh/persister/serializing_spec.rb | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/spec/models/manager_refresh/persister/serializing_spec.rb b/spec/models/manager_refresh/persister/serializing_spec.rb index 197ec64c1ac..349eaae2451 100644 --- a/spec/models/manager_refresh/persister/serializing_spec.rb +++ b/spec/models/manager_refresh/persister/serializing_spec.rb @@ -51,13 +51,8 @@ expect(vm2.hardware.networks.first.ipaddress).to eq("10.10.10.2") expect(vm2.hardware.disks.first.device_name).to eq("disk_name_2") - # TODO(lsmola) interestingly persister.hardwares.lazy_find({:vm_or_template => lazy_find_image_sec}) will not work - # because we build the reference only from the passed data. To make it work, we would have to do internally - # persister.hardwares.lazy_find({:vm_or_template => lazy_find_image_sec.load}), which will allow the nested lazy - # find to reach its :manager_ref data. That would mean the stringified reference would have to be build when - # we try to evaluate the lazy object. We should be able to do that. - expect(vm.hardware.model).to eq(nil) - expect(vm.hardware.manufacturer).to eq(nil) + expect(vm.hardware.model).to eq("test1") + expect(vm.hardware.manufacturer).to eq("test2") expect(Vm.find_by(:ems_ref => "vm_ems_ref_20").ems_id).to be_nil expect(Vm.find_by(:ems_ref => "vm_ems_ref_21").ems_id).not_to be_nil From da82ffb817388533cebdc10aa7b26b4cba65e610 Mon Sep 17 00:00:00 2001 From: Ladislav Smola Date: Fri, 11 May 2018 15:03:19 +0200 Subject: [PATCH 6/8] Add guard for nested references being too deep Add guard for nested references being too deep --- app/models/manager_refresh/inventory_object_lazy.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/models/manager_refresh/inventory_object_lazy.rb b/app/models/manager_refresh/inventory_object_lazy.rb index c096222bc5b..a26c381dc33 100644 --- a/app/models/manager_refresh/inventory_object_lazy.rb +++ b/app/models/manager_refresh/inventory_object_lazy.rb @@ -73,6 +73,8 @@ def association?(key) end def transform_nested_secondary_indexes!(depth = 0) + raise "Nested references are too deep!" if depth > 20 + keys.each do |x| attr = full_reference[x] next unless attr.kind_of?(ManagerRefresh::InventoryObjectLazy) From 6fa1a372b3deb38b07ad91c1bf77478fd4d346a1 Mon Sep 17 00:00:00 2001 From: Ladislav Smola Date: Tue, 15 May 2018 14:08:24 +0200 Subject: [PATCH 7/8] Make nested lazy finds explicit for now Make nested lazy finds explicit for now, before we are able to transform it automatically, when needed. --- .../inventory_collection/index/proxy.rb | 7 +++++-- .../inventory_collection/serialization.rb | 20 ++++++++++--------- .../manager_refresh/inventory_object_lazy.rb | 11 +++++++--- 3 files changed, 24 insertions(+), 14 deletions(-) diff --git a/app/models/manager_refresh/inventory_collection/index/proxy.rb b/app/models/manager_refresh/inventory_collection/index/proxy.rb index d666cdfd38a..9b049875c81 100644 --- a/app/models/manager_refresh/inventory_collection/index/proxy.rb +++ b/app/models/manager_refresh/inventory_collection/index/proxy.rb @@ -99,7 +99,7 @@ def lazy_find_by(manager_uuid_hash, ref: primary_index_ref, key: nil, default: n lazy_find(manager_uuid_hash, :ref => ref, :key => key, :default => default) end - def lazy_find(manager_uuid, ref: primary_index_ref, key: nil, default: nil) + def lazy_find(manager_uuid, ref: primary_index_ref, key: nil, default: nil, transform_nested_lazy_finds: false) # TODO(lsmola) also, it should be enough to have only 1 find method, everything can be lazy, until we try to # access the data # TODO(lsmola) lazy_find will support only hash, then we can remove the _by variant @@ -108,7 +108,10 @@ def lazy_find(manager_uuid, ref: primary_index_ref, key: nil, default: nil) ::ManagerRefresh::InventoryObjectLazy.new(inventory_collection, manager_uuid, - :ref => ref, :key => key, :default => default) + :ref => ref, + :key => key, + :default => default, + :transform_nested_lazy_finds => transform_nested_lazy_finds) end def named_ref(ref = primary_index_ref) diff --git a/app/models/manager_refresh/inventory_collection/serialization.rb b/app/models/manager_refresh/inventory_collection/serialization.rb index 30c0a141320..d3fc99d5df1 100644 --- a/app/models/manager_refresh/inventory_collection/serialization.rb +++ b/app/models/manager_refresh/inventory_collection/serialization.rb @@ -56,12 +56,13 @@ def to_hash # @return [Hash] Serialized ManagerRefresh::InventoryObjectLazy def lazy_relation_to_hash(value, depth = 0) { - :type => "ManagerRefresh::InventoryObjectLazy", - :inventory_collection_name => value.inventory_collection.name, - :reference => data_to_hash(value.reference.full_reference, depth + 1), - :ref => value.reference.ref, - :key => value.try(:key), - :default => value.try(:default), + :type => "ManagerRefresh::InventoryObjectLazy", + :inventory_collection_name => value.inventory_collection.name, + :reference => data_to_hash(value.reference.full_reference, depth + 1), + :ref => value.reference.ref, + :key => value.try(:key), + :default => value.try(:default), + :transform_nested_lazy_finds => value.try(:transform_nested_lazy_finds) } end @@ -78,9 +79,10 @@ def hash_to_lazy_relation(value, available_inventory_collections, depth = 0) inventory_collection.lazy_find( hash_to_data(value['reference'], available_inventory_collections, depth + 1).symbolize_keys!, - :ref => value['ref'].try(:to_sym), - :key => value['key'].try(:to_sym), - :default => value['default'] + :ref => value['ref'].try(:to_sym), + :key => value['key'].try(:to_sym), + :default => value['default'], + :transform_nested_lazy_finds => value['transform_nested_lazy_finds'] ) end diff --git a/app/models/manager_refresh/inventory_object_lazy.rb b/app/models/manager_refresh/inventory_object_lazy.rb index a26c381dc33..481e3a716e0 100644 --- a/app/models/manager_refresh/inventory_object_lazy.rb +++ b/app/models/manager_refresh/inventory_object_lazy.rb @@ -2,7 +2,7 @@ module ManagerRefresh class InventoryObjectLazy include Vmdb::Logging - attr_reader :reference, :inventory_collection, :key, :default + attr_reader :reference, :inventory_collection, :key, :default, :transform_nested_lazy_finds delegate :stringified_reference, :ref, :[], :to => :reference @@ -12,12 +12,17 @@ class InventoryObjectLazy # @param ref [Symbol] reference name # @param key [Symbol] key name, will be used to fetch attribute from resolved InventoryObject # @param default [Object] a default value used if the :key will resolve to nil - def initialize(inventory_collection, index_data, ref: :manager_ref, key: nil, default: nil) + # @param transform_nested_lazy_finds [Boolean] True if we want to convert all lazy objects in InventoryObject + # objects and reset the Reference. TODO(lsmola) we should be able to do this automatically, then we can + # remove this option + def initialize(inventory_collection, index_data, ref: :manager_ref, key: nil, default: nil, transform_nested_lazy_finds: false) @inventory_collection = inventory_collection @reference = inventory_collection.build_reference(index_data, ref) @key = key @default = default + @transform_nested_lazy_finds = transform_nested_lazy_finds + # We do not support skeletal pre-create for :key, since :key will not be available, we want to use local_db_find # instead. skeletal_precreate! unless @key @@ -40,7 +45,7 @@ def inspect # @return [ManagerRefresh::InventoryObject, Object] ManagerRefresh::InventoryObject instance or an attribute # on key def load - transform_nested_secondary_indexes! if nested_secondary_index? + transform_nested_secondary_indexes! if transform_nested_lazy_finds && nested_secondary_index? key ? load_object_with_key : load_object end From d1405c262cde911be5b7e8e533bd95aea66eec0e Mon Sep 17 00:00:00 2001 From: Ladislav Smola Date: Tue, 15 May 2018 14:27:34 +0200 Subject: [PATCH 8/8] Test explicit transform_nested_lazy_finds Test explicit transform_nested_lazy_finds --- .../persister/serializing_spec.rb | 20 +++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/spec/models/manager_refresh/persister/serializing_spec.rb b/spec/models/manager_refresh/persister/serializing_spec.rb index 349eaae2451..a3415a9510c 100644 --- a/spec/models/manager_refresh/persister/serializing_spec.rb +++ b/spec/models/manager_refresh/persister/serializing_spec.rb @@ -77,6 +77,20 @@ def populate_test_data(persister) lazy_find_image_sec1 = persister.miq_templates.lazy_find( {:name => image_data(1)[:name], :uid_ems => image_data(1)[:uid_ems]}, {:ref => :by_uid_ems_and_name} ) + # TODO(lsmola) we build :hardwares with vm_or_template => persister.miq_templates.lazy_find(:ems_ref => image_data(1)[:ems_ref]) + # so it's indexed by vm ems_ref. Then we try to fetch it with lazy_find_image_sec2, which uses a by_uid_ems_and_name. + # So we can convert that to ems_ref, but what if we would build the :hardware with by_uid_ems_and_name? We do this + # in k8s provider + # + # We will need to store in hardware IC what it was build with, then we will know what we can convert it to? And hopefully + # we will not build the hardware with different :keys? But we might be actually doing it and I think it's fine for + # creating? But it sucks for lazy_find, maybe we will just forbid it? + # + # Or we will store what attributes were used to build hardware, then we can try to find it based on all of them? + # Then we would also transform the current lazy_object on find? But it would not work with current recursion. + # + # And we need more focus on having e.g. hardware, using vm with different refs, since we can then have duplicates. + # On saving, those duplicates will be eliminated, but if anything pointed to the duplicate, it will not get it's id. lazy_find_image_sec2 = persister.miq_templates.lazy_find( {:name => image_data(1)[:name], :uid_ems => image_data(1)[:uid_ems]}, {:ref => :by_uid_ems_and_name, :key => :vendor} ) @@ -96,8 +110,10 @@ def populate_test_data(persister) @hardware_data_1 = hardware_data(1).merge( :guest_os => persister.hardwares.lazy_find({:vm_or_template => lazy_find_image}, {:key => :guest_os}), - :model => persister.hardwares.lazy_find({:vm_or_template => lazy_find_image_sec}, {:key => :model}), - :manufacturer => persister.hardwares.lazy_find({:vm_or_template => lazy_find_image_sec1}, {:key => :manufacturer}), + :model => persister.hardwares.lazy_find({:vm_or_template => lazy_find_image_sec}, + {:key => :model, :transform_nested_lazy_finds => true}), + :manufacturer => persister.hardwares.lazy_find({:vm_or_template => lazy_find_image_sec1}, + {:key => :manufacturer, :transform_nested_lazy_finds => true}), :guest_os_full_name => lazy_find_image_sec2, :vm_or_template => persister.vms.lazy_find(:ems_ref => vm_data(1)[:ems_ref]) )