From e29b4d3ca08ed9c8ce8465d46f52030c58a741b3 Mon Sep 17 00:00:00 2001 From: lpichler Date: Fri, 23 Mar 2018 10:05:26 +0100 Subject: [PATCH 1/3] Set current tenant parent it can happen when the new user has greater count of cloud tenants than previous provider's user and some of them have different names so parent of existing tenant was not updated correcltly. --- app/models/manageiq/providers/cloud_manager.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/models/manageiq/providers/cloud_manager.rb b/app/models/manageiq/providers/cloud_manager.rb index 9ea7c8d98fb..1caa5fbe8bd 100644 --- a/app/models/manageiq/providers/cloud_manager.rb +++ b/app/models/manageiq/providers/cloud_manager.rb @@ -85,6 +85,7 @@ def sync_tenants elsif existing_source_tenant = Tenant.descendants_of(tenant_parent).find_by(:name => tenant_params[:name]) _log.info("CloudTenant #{cloud_tenant.name} has orphaned tenant #{existing_source_tenant.name}") cloud_tenant.source_tenant = existing_source_tenant + tenant_params[:parent] = tenant_parent cloud_tenant.update_source_tenant(tenant_params) else _log.info("CloudTenant #{cloud_tenant.name} has no tenant") From 5092db6e8e39637636caeca02dd576642222859b Mon Sep 17 00:00:00 2001 From: lpichler Date: Fri, 23 Mar 2018 10:12:09 +0100 Subject: [PATCH 2/3] When tenant is moved out under provider tenant we need to nil source columns because such source (CloudTenant) no longer exists --- app/models/manageiq/providers/cloud_manager.rb | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/app/models/manageiq/providers/cloud_manager.rb b/app/models/manageiq/providers/cloud_manager.rb index 1caa5fbe8bd..e9e6d507388 100644 --- a/app/models/manageiq/providers/cloud_manager.rb +++ b/app/models/manageiq/providers/cloud_manager.rb @@ -113,11 +113,16 @@ def sync_deleted_cloud_tenants source_tenant.descendants.each do |tenant| next if tenant.source - next if tenant.parent == source_tenant # tenant is already under provider's tenant + + if tenant.parent == source_tenant # tenant is already under provider's tenant + _log.info("Setting source_id and source_type to nil for #{tenant.name} under provider's tenant #{source_tenant.name}") + tenant.update_attributes(:source_id => nil, :source_type => nil) + next + end # move tenant under the provider's tenant _log.info("Moving out #{tenant.name} under provider's tenant #{source_tenant.name}") - tenant.update_attributes(:parent => source_tenant) + tenant.update_attributes(:parent => source_tenant, :source_id => nil, :source_type => nil) end end From 59d8272f3de7de8fa35759d567b4ce9cfafbf076 Mon Sep 17 00:00:00 2001 From: lpichler Date: Fri, 23 Mar 2018 10:17:05 +0100 Subject: [PATCH 3/3] These specs are covering previous fixes and covering more cases there is several cases when user of provider is changed and cloud tenants are changed as well. Each case is testing resulted tenant and all his relations as parent, children, source_tenant and source. --- .../manageiq/providers/cloud_manager_spec.rb | 448 ++++++++++++++++++ 1 file changed, 448 insertions(+) diff --git a/spec/models/manageiq/providers/cloud_manager_spec.rb b/spec/models/manageiq/providers/cloud_manager_spec.rb index 78818b0bbf5..936a106b81e 100644 --- a/spec/models/manageiq/providers/cloud_manager_spec.rb +++ b/spec/models/manageiq/providers/cloud_manager_spec.rb @@ -278,6 +278,454 @@ def expect_created_tenant_tree expect(ems_cloud.source_tenant).to eq(tenant.parent) end end + + context "provider's user is changed between two synchronizations" do + let!(:vm_1) { FactoryGirl.create(:vm_openstack) } + let!(:vm_2) { FactoryGirl.create(:vm_openstack) } + let(:ct_name_1) { "c_t_1" } + let(:ct_name_2) { "c_t_2" } + let(:ct_name_3) { "c_t_3" } + let(:provider_tenant) { Tenant.root_tenant.children.first } + + def tenant_from_cloud_tenant_by(vm) + Tenant.find_by(:name => vm.cloud_tenant.name) + end + + let(:ct_3) do + FactoryGirl.create(:cloud_tenant_openstack, :ext_management_system => ems_cloud, :name => ct_name_3) + end + + before(:each) do + vm_1.cloud_tenant.update_attributes!(:parent => ct_3, :ext_management_system => ems_cloud, :name => ct_name_1) + vm_2.cloud_tenant.update_attributes!(:ext_management_system => ems_cloud, :name => ct_name_2) + end + + ###### + # ct - Cloud Tenant, t - Tenant + # ===== + # CloudTenant tree: + # ct_3 -> ct_1 (vm_1s) + # ct_2 + # + # Expected Tenant tree: + # My Company t + # -> Provider Tenant + # -> t_3 (ct_3's) -> t_1 + # -> t_2 (ct_2's) + ####### + it 'creates tenant tree from cloud tenants with VMs' do + ems_cloud.sync_cloud_tenants_with_tenants + + t_one_ct_vm_one = tenant_from_cloud_tenant_by(vm_1) + expect(t_one_ct_vm_one.id).to eq(vm_1.cloud_tenant.source_tenant.id) + expect(t_one_ct_vm_one.parent.id).to eq(ct_3.source_tenant.id) + expect(t_one_ct_vm_one.children.first).to be_nil + expect(t_one_ct_vm_one.source_id).to eq(vm_1.cloud_tenant.id) + expect(t_one_ct_vm_one.source_type).to eq("CloudTenant") + + t_two_ct_vm_two = tenant_from_cloud_tenant_by(vm_2) + expect(t_two_ct_vm_two.id).to eq(vm_2.cloud_tenant.source_tenant.id) + expect(t_two_ct_vm_two.parent.id).to eq(provider_tenant.id) + expect(t_two_ct_vm_two.children.first).to be_nil + expect(t_two_ct_vm_two.source_id).to eq(vm_2.cloud_tenant.id) + expect(t_two_ct_vm_two.source_type).to eq("CloudTenant") + + t_three_ct_three = Tenant.find_by(:name => ct_name_3) + expect(t_three_ct_three.id).to eq(ct_3.source_tenant.id) + expect(t_three_ct_three.parent.id).to eq(provider_tenant.id) + expect(t_three_ct_three.children.first.id).to eq(vm_1.cloud_tenant.source_tenant.id) + expect(t_three_ct_three.source_id).to eq(ct_3.id) + expect(t_three_ct_three.source_type).to eq("CloudTenant") + + expect(t_one_ct_vm_one.parent.id).to eq(ct_3.source_tenant.id) + expect(Tenant.find_by(:name => ct_3.name).id).to eq(ct_3.source_tenant.id) + + expect(Tenant.root_tenant.children.first.id).to eq(ct_3.source_tenant.parent.id) + end + + context "when the new user has cloud tenants with same names than previous provider's user" do + ###### + # ct - Cloud Tenant, t - Tenant + # ===== + # CloudTenant tree: + # ct_3 -> ct_1 (vm_1s) + # ct_2 + # + # first tenant synchronization + # + # Expected Tenant tree: + # My Company t + # -> Provider Tenant + # -> t_3 (ct_3's) -> t_1 + # -> t_2 (ct_2's) + # + ### after changed user with different set of cloud tenants + ### second synchronization + # ===== + # CloudTenant tree: + # new_ct_3(same name) -> new_ct_1 (vm_1s) -> new_ct_4(vm_4's cloud_tenant)) + # new_ct_2 + # + # second tenant synchronization + # + # Expected Tenant tree: + # My Company t + # -> Provider Tenant + # -> t_3 (new_ct_3's) -> t_1(new_ct_1's) + # -> t_2 (new_ct_2's) + ###### + let(:ct_1_new) { FactoryGirl.create(:cloud_tenant_openstack, :ext_management_system => ems_cloud, :parent => ct_3_new, :name => ct_name_1) } + let(:ct_2_new) { FactoryGirl.create(:cloud_tenant_openstack, :ext_management_system => ems_cloud, :name => ct_name_2) } + let(:ct_3_new) { FactoryGirl.create(:cloud_tenant_openstack, :ext_management_system => ems_cloud, :name => ct_name_3) } + + it "creates tenant tree from cloud tenants with correct source_tenant relations" do + expect(CloudTenant.count).to eq(3) + ems_cloud.sync_cloud_tenants_with_tenants + expect(Tenant.count).to eq(5) + + tenant_ids = Tenant.ids + old_cloud_tenant_ids = CloudTenant.ids + + # destroy old cloud tenants and replace them with new + vm_1.cloud_tenant.destroy + vm_1.update_attributes!(:cloud_tenant => ct_1_new) + vm_2.cloud_tenant.destroy + vm_2.update_attributes!(:cloud_tenant => ct_2_new) + ct_3.destroy + + expect(CloudTenant.count).to eq(3) + expect(CloudTenant.ids).not_to match_array(old_cloud_tenant_ids) + + ems_cloud.sync_cloud_tenants_with_tenants + expect(Tenant.ids).to match_array(tenant_ids) + + t_one_ct_vm_one = tenant_from_cloud_tenant_by(vm_1) + expect(t_one_ct_vm_one.id).to eq(ct_1_new.source_tenant.id) + expect(t_one_ct_vm_one.parent.id).to eq(ct_3_new.source_tenant.id) + expect(t_one_ct_vm_one.children.first).to be_nil + expect(t_one_ct_vm_one.source_id).to eq(ct_1_new.id) + expect(t_one_ct_vm_one.source_type).to eq("CloudTenant") + + t_two_ct_vm_two = tenant_from_cloud_tenant_by(vm_2) + expect(t_two_ct_vm_two.id).to eq(ct_2_new.source_tenant.id) + expect(t_two_ct_vm_two.parent.id).to eq(provider_tenant.id) + expect(t_two_ct_vm_two.children.first).to be_nil + expect(t_two_ct_vm_two.source_id).to eq(ct_2_new.id) + expect(t_two_ct_vm_two.source_type).to eq("CloudTenant") + + t_three_ct_three = Tenant.find_by(:name => ct_name_3) + expect(t_three_ct_three.id).to eq(ct_3_new.source_tenant.id) + expect(t_three_ct_three.parent.id).to eq(provider_tenant.id) + expect(t_three_ct_three.children.first.id).to eq(ct_1_new.source_tenant.id) + expect(t_three_ct_three.source_id).to eq(ct_3_new.id) + expect(t_three_ct_three.source_type).to eq("CloudTenant") + + expect(t_one_ct_vm_one.parent.id).to eq(ct_3_new.source_tenant.id) + expect(Tenant.find_by(:name => ct_3_new.name).id).to eq(ct_3_new.source_tenant.id) + + expect(Tenant.root_tenant.children.first.id).to eq(ct_3_new.source_tenant.parent.id) + expect(Tenant.count).to eq(5) + end + end + + context "when the new user has cloud tenants with different names than previous provider's user" do + ###### + # ct - Cloud Tenant, t - Tenant + # ===== + # CloudTenant tree: + # ct_3 -> ct_1 (vm_1s) + # ct_2 + # + # first tenant synchronization + # + # Expected Tenant tree: + # My Company t + # -> Provider Tenant + # -> t_3 (ct_3's) -> t_1 + # -> t_2 (ct_2's) + # + ### after changed user with different set of cloud tenants + ### second synchronization + # ===== + # CloudTenant tree: + # new_ct_3(same name) -> new_ct_1 (vm_1s) + # new_ct_2 + # + # second tenant synchronization + # + # Expected Tenant tree: + # My Company t + # -> Provider Tenant + # -> new_t_3 (new_ct_3's) -> new_t_1(new_ct_1's) + # -> new_t_2 (new_ct_2's) + # -> t_1(old - moved out) + # -> t_2(old - moved out) + # -> t_3(old - moved out) + ###### + let(:ct_1_new) { FactoryGirl.create(:cloud_tenant_openstack, :ext_management_system => ems_cloud, :parent => ct_3_new, :name => ct_name_1 + "X") } + let(:ct_2_new) { FactoryGirl.create(:cloud_tenant_openstack, :ext_management_system => ems_cloud, :name => ct_name_2 + "X") } + let(:ct_3_new) { FactoryGirl.create(:cloud_tenant_openstack, :ext_management_system => ems_cloud, :name => ct_name_3 + "X") } + + it "creates tenant tree from cloud tenants with correct source_tenant relations" do + expect(CloudTenant.count).to eq(3) + ems_cloud.sync_cloud_tenants_with_tenants + expect(Tenant.count).to eq(5) + + old_cloud_tenant_ids = CloudTenant.ids + + # destroy old cloud tenants and replace them with new + vm_1.cloud_tenant.destroy + vm_1.update_attributes!(:cloud_tenant => ct_1_new) + vm_2.cloud_tenant.destroy + vm_2.update_attributes!(:cloud_tenant => ct_2_new) + ct_3.destroy + + expect(CloudTenant.count).to eq(3) + expect(CloudTenant.ids).not_to match_array(old_cloud_tenant_ids) + + ems_cloud.sync_cloud_tenants_with_tenants + + t_one_ct_vm_one = tenant_from_cloud_tenant_by(vm_1) + expect(t_one_ct_vm_one.id).to eq(ct_1_new.source_tenant.id) + expect(t_one_ct_vm_one.parent.id).to eq(ct_3_new.source_tenant.id) + expect(t_one_ct_vm_one.children.first).to be_nil + expect(t_one_ct_vm_one.source_id).to eq(ct_1_new.id) + expect(t_one_ct_vm_one.source_type).to eq("CloudTenant") + + t_two_ct_vm_two = tenant_from_cloud_tenant_by(vm_2) + expect(t_two_ct_vm_two.id).to eq(ct_2_new.source_tenant.id) + expect(t_two_ct_vm_two.parent.id).to eq(provider_tenant.id) + expect(t_two_ct_vm_two.children.first).to be_nil + expect(t_two_ct_vm_two.source_id).to eq(ct_2_new.id) + expect(t_two_ct_vm_two.source_type).to eq("CloudTenant") + + t_three_ct_three = Tenant.find_by(:name => ct_name_3 + "X") + expect(t_three_ct_three.id).to eq(ct_3_new.source_tenant.id) + expect(t_three_ct_three.parent.id).to eq(provider_tenant.id) + expect(t_three_ct_three.children.first.id).to eq(ct_1_new.source_tenant.id) + expect(t_three_ct_three.source_id).to eq(ct_3_new.id) + expect(t_three_ct_three.source_type).to eq("CloudTenant") + + expect(t_one_ct_vm_one.parent.id).to eq(ct_3_new.source_tenant.id) + expect(t_three_ct_three.id).to eq(ct_3_new.source_tenant.id) + + provider_tenant = Tenant.root_tenant.children.first + expect(provider_tenant.id).to eq(ct_3_new.source_tenant.parent.id) + expect(Tenant.count).to eq(8) + + old_tenant_first = Tenant.find_by(:name => ct_name_1) + expect(old_tenant_first.id).not_to eq(ct_1_new.source_tenant.id) + expect(old_tenant_first.parent.id).to eq(provider_tenant.id) + expect(old_tenant_first.children.first).to be_nil + expect(old_tenant_first.source_id).to be_nil + expect(old_tenant_first.source_type).to be_nil + + old_tenant_two = Tenant.find_by(:name => ct_name_2) + expect(old_tenant_two.id).not_to eq(ct_2_new.source_tenant.id) + expect(old_tenant_two.parent.id).to eq(provider_tenant.id) + expect(old_tenant_two.children.first).to be_nil + expect(old_tenant_first.source_id).to be_nil + expect(old_tenant_first.source_type).to be_nil + + old_tenant_three = Tenant.find_by(:name => ct_name_3) + expect(old_tenant_three.parent.id).to eq(provider_tenant.id) + expect(old_tenant_three.children.first).to be_nil + expect(old_tenant_three.source_id).to be_nil + expect(old_tenant_three.source_type).to be_nil + end + end + + context "when the new user has lower count of cloud tenants with than previous provider's user" do + ###### + # ct - Cloud Tenant, t - Tenant + # ===== + # CloudTenant tree: + # ct_3 -> ct_1 (vm_1s) + # ct_2 + # + # first tenant synchronization + # + # Expected Tenant tree: + # My Company t + # -> Provider Tenant + # -> t_3 (ct_3's) -> t_1 + # -> t_2 (ct_2's) + # + ### after changed user with different set of cloud tenants + ### second synchronization + # ===== + # CloudTenant tree: + # new_ct_1 (vm_1s) + # + # second tenant synchronization + # + # Expected Tenant tree: + # My Company t + # -> Provider Tenant + # -> new_t_1(new_ct_1's) + # -> t_2(old - moved out) + # -> t_3(old - moved out) + ###### + let(:ct_1_new) { FactoryGirl.create(:cloud_tenant_openstack, :ext_management_system => ems_cloud, :name => ct_name_1) } + + it "creates tenant tree from cloud tenants with correct source_tenant relations" do + expect(CloudTenant.count).to eq(3) + ems_cloud.sync_cloud_tenants_with_tenants + expect(Tenant.count).to eq(5) + + old_cloud_tenant_ids = CloudTenant.ids + + # destroy old cloud tenants and replace them with new + vm_1.cloud_tenant.destroy + vm_1.update_attributes!(:cloud_tenant => ct_1_new) + vm_2.cloud_tenant.destroy + vm_2.destroy + ct_3.destroy + + expect(CloudTenant.count).to eq(1) + expect(CloudTenant.ids).not_to match_array(old_cloud_tenant_ids) + ems_cloud.sync_cloud_tenants_with_tenants + + t_one_ct_vm_one = tenant_from_cloud_tenant_by(vm_1) + expect(t_one_ct_vm_one.id).to eq(ct_1_new.source_tenant.id) + expect(t_one_ct_vm_one.parent.id).to eq(provider_tenant.id) + expect(t_one_ct_vm_one.children.first).to be_nil + expect(t_one_ct_vm_one.source_id).to eq(ct_1_new.id) + expect(t_one_ct_vm_one.source_type).to eq("CloudTenant") + + expect(provider_tenant.id).to eq(ct_1_new.source_tenant.parent.id) + expect(Tenant.count).to eq(5) + + # old tenants (related to deleted cloud tenants) are under provider tenant + old_tenant_two = Tenant.find_by(:name => ct_name_2) + expect(old_tenant_two.parent.id).to eq(provider_tenant.id) + expect(old_tenant_two.children.first).to be_nil + expect(old_tenant_two.source_id).to be_nil + expect(old_tenant_two.source_type).to be_nil + + old_tenant_three = Tenant.find_by(:name => ct_name_3) + expect(old_tenant_three.parent.id).to eq(provider_tenant.id) + expect(old_tenant_three.children.first).to be_nil + expect(old_tenant_three.source_id).to be_nil + expect(old_tenant_three.source_type).to be_nil + end + end + + context "when the new user has greater count of cloud tenants with than previous provider's user, some and different name" do + ###### + # ct - Cloud Tenant, t - Tenant + # ===== + # CloudTenant tree: + # ct_3 -> ct_1 (vm_1s) + # ct_2 + # + # first tenant synchronization + # + # Expected Tenant tree: + # My Company t + # -> Provider Tenant + # -> t_3 (ct_3's) -> t_1 + # -> t_2 (ct_2's) + # + ### after changed user with different set of cloud tenants + ### second synchronization + # ===== + # CloudTenant tree: + # new_ct_3(same name) -> new_ct_1 (vm_1s) -> new_ct_4(vm_4's cloud_tenant)) + # new_ct_2 + # + # second tenant synchronization + # + # Expected Tenant tree: + # My Company t + # -> Provider Tenant + # -> t_3 (new_ct_3's) -> new_t_1(new_ct_1's) -> new_t_4(vm_4's cloud_tenant) + # -> new_t_2 (new_ct_2's) + # -> t_1(old - moved out) + # -> t_2(old - moved out) + ###### + let(:ct_1_new) { FactoryGirl.create(:cloud_tenant_openstack, :ext_management_system => ems_cloud, :parent => ct_3_new, :name => ct_name_1 + "X") } + let(:ct_2_new) { FactoryGirl.create(:cloud_tenant_openstack, :ext_management_system => ems_cloud, :name => ct_name_2 + "X") } + let(:ct_3_new) { FactoryGirl.create(:cloud_tenant_openstack, :ext_management_system => ems_cloud, :name => ct_name_3) } + let(:ct_name_4) { "ct_name_4" } + let(:vm_4) do + vm = FactoryGirl.create(:vm_openstack) + vm.cloud_tenant.update_attributes!(:parent => ct_1_new, :ext_management_system => ems_cloud, :name => ct_name_4) + vm + end + + it "creates tenant tree from cloud tenants with correct source_tenant relations" do + expect(CloudTenant.count).to eq(3) + ems_cloud.sync_cloud_tenants_with_tenants + expect(Tenant.count).to eq(5) + + old_cloud_tenant_ids = CloudTenant.ids + + # destroy old cloud tenants and replace them with new + vm_1.cloud_tenant.destroy + vm_1.update_attributes!(:cloud_tenant => ct_1_new) + vm_2.cloud_tenant.destroy + vm_2.cloud_tenant.destroy + vm_2.update_attributes!(:cloud_tenant => ct_2_new) + ct_3.destroy + ct_2_new + vm_4 + + expect(CloudTenant.count).to eq(4) + expect(CloudTenant.ids).not_to match_array(old_cloud_tenant_ids) + + ems_cloud.sync_cloud_tenants_with_tenants + + # testing relations source, parent, children, source_tenant for each tenant + t_one_ct_vm_one = tenant_from_cloud_tenant_by(vm_1) + expect(t_one_ct_vm_one.id).to eq(ct_1_new.source_tenant.id) + expect(t_one_ct_vm_one.parent.id).to eq(ct_3_new.source_tenant.id) + expect(t_one_ct_vm_one.children.first.id).to eq(vm_4.cloud_tenant.source_tenant.id) + expect(t_one_ct_vm_one.source_id).to eq(ct_1_new.id) + expect(t_one_ct_vm_one.source_type).to eq("CloudTenant") + + t_two_ct_vm_two = tenant_from_cloud_tenant_by(vm_2) + expect(t_two_ct_vm_two.id).to eq(ct_2_new.source_tenant.id) + expect(t_two_ct_vm_two.parent.id).to eq(provider_tenant.id) + expect(t_two_ct_vm_two.children.first).to be_nil + expect(t_two_ct_vm_two.source_id).to eq(ct_2_new.id) + expect(t_two_ct_vm_two.source_type).to eq("CloudTenant") + + t_three_ct_three = Tenant.find_by(:name => ct_name_3) + expect(t_three_ct_three.id).to eq(ct_3_new.source_tenant.id) + expect(t_three_ct_three.parent.id).to eq(provider_tenant.id) + expect(t_three_ct_three.children.first.id).to eq(ct_1_new.source_tenant.id) + expect(t_three_ct_three.source_id).to eq(ct_3_new.id) + expect(t_three_ct_three.source_type).to eq("CloudTenant") + t_four_ct_four = Tenant.find_by(:name => ct_name_4) + expect(t_three_ct_three.descendants.ids).to match_array([t_one_ct_vm_one.id, t_four_ct_four.id]) + + expect(t_four_ct_four.id).to eq(vm_4.cloud_tenant.source_tenant.id) + expect(t_four_ct_four.parent.id).to eq(ct_1_new.source_tenant.id) + expect(t_four_ct_four.children.first).to be_nil + expect(t_four_ct_four.source_id).to eq(vm_4.cloud_tenant.id) + expect(t_four_ct_four.source_type).to eq("CloudTenant") + + expect(provider_tenant.id).to eq(ct_3_new.source_tenant.parent.id) + expect(Tenant.count).to eq(8) + + # old tenants (related to deleted cloud tenants) are under provider tenant + old_tenant_first = Tenant.find_by(:name => ct_name_1) + expect(old_tenant_first.id).not_to eq(ct_1_new.source_tenant.id) + expect(old_tenant_first.parent.id).to eq(provider_tenant.id) + expect(old_tenant_first.children.first).to be_nil + expect(old_tenant_first.source_id).to be_nil + expect(old_tenant_first.source_type).to be_nil + + old_tenant_two = Tenant.find_by(:name => ct_name_2) + expect(old_tenant_two.id).not_to eq(ct_2_new.source_tenant.id) + expect(old_tenant_two.parent.id).to eq(provider_tenant.id) + expect(old_tenant_two.children.first).to be_nil + expect(old_tenant_first.source_id).to be_nil + expect(old_tenant_first.source_type).to be_nil + end + end + end end end end