Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

query also archived objects for a new ems #16886

Merged
merged 1 commit into from
Feb 23, 2018

Conversation

AlexanderZagaynov
Copy link
Contributor

@AlexanderZagaynov AlexanderZagaynov commented Jan 25, 2018

@AlexanderZagaynov
Copy link
Contributor Author

@bronaghs @Ladas it was very hard to find the place to hack, and its probably an ugly hack...
Please review and guide me if you think it can be done in a better way.

@miq-bot miq-bot added the wip label Jan 25, 2018
@@ -975,7 +975,17 @@ def db_collection_for_comparison_for_complement_of(manager_uuids_set)

def full_collection_for_comparison
return arel unless arel.nil?
parent.send(association)
collection = parent.send(association)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, there is a prepared section for this (we should never modify the base code with specifics like this), and we already do this for infra:

https://github.com/Ladas/manageiq/blob/efaa339af78a230eed513d002ce4487ae162a977/app/models/manager_refresh/inventory_collection_default/infra_manager.rb#L315

We should put it to base class: https://github.com/Ladas/manageiq/blob/b3110ffdd8dcabc816e503877422516296e57ea8/app/models/manager_refresh/inventory_collection_default.rb#L3

Then it should work for all cloud/infra vms. And we should have a simple spec, that checks it works. (at least for AWS for now). Also we need to checks specs for all cloud managers, since the Infra guys were reporting something was failing.


It's questionable whether this is needed (we should have some discussion about this) and it can affect performance a lot. The removal/adding of provider is not a standard workflow, by removing a provider, we should expect to loose all data (this was all done for a specific VMware usecase). Part of the workflow will be replaced by proper soft delete and unique indexes.

That being said, we can add it for now and talk about removal once the unique indexes and proper soft delete are in place.

@agrare ^

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm good with moving that reconnect block up to the base class since currently that is what standard refresh does. Technically it should reconnect based on uid_ems not ems_ref since once a VM/instance is disconnected the ems_ref uniqueness isn't guaranteed.

end

it 'pickups existing Vm records on EMS re-adding' do
expect(Vm.count).to_not be_zero
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AlexanderZagaynov unless I am mistaken, each of the specs will cause a refresh? That is why we don't do the normal clean way of writing specs (test 1 thing). But we run refresh once and just do all the assert sequentially. (refresh can take units or dozens of seconds, so that quickly adds up)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -310,37 +310,6 @@ def hosts(extra_attributes = {})
attributes.merge!(extra_attributes)
end

def vms(extra_attributes = {})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@borod108 do you have specs for the reconnect in RHEV? We are trying to unify the behavior.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ladas I do not think so, I will add some, can you please explain the functionality?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@borod108 you had similar BZ for RHEV https://bugzilla.redhat.com/show_bug.cgi?id=1536406
So just that the archived Vm is reused, when deleting/re-adding provider.

Copy link

@borod108 borod108 Feb 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pkliczewski iirc you wrote a test for that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@borod108 Yes, I wrote a test for reconnecting a host and vm

return if relation.count <= 0

inventory_objects_index.each_slice(100) do |batch|
relation.where(inventory_collection.manager_ref.first => batch.map(&:first)).each do |record|
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@agrare so we are doing reconnect using :ems_ref, which is ok for clouds and also RHEV (right @borod108 ?)

So I would just keep it this way and when we play with VMware, we would figure out what to do. E.g. we can just use uid_ems as unique attribute? It's unique for VMware, so we can use it both for matching and reconnect?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ladas according to our "API" (aka general knowledge haha):ems_ref isn't unique across ems_ids, even if it happens to be for cloud providers. Most just set ems_ref = uid_ems = some unique id but RHEV actually has two different values for ems_ref and uid_ems. Why not just use uid_ems as the manager_ref and be done with it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@agrare right, that is what I was thinking, but I would rather keep it this way for backport (switching manager_ref globally could break some specs and we might need to do minor parser changes)

and for upstream, we could just switch to :manager_ref => [:uid_ems], for both Vm and MiqTemplate, does that sound good?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 works for me

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I got this right, you're talking about some change, that's not fully related to the current PR/issue?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AlexanderZagaynov yeah, we can ignore it for now. We will do changes on other PR, to make sure this works for VMware


it 'pickups existing Template records on EMS re-adding' do
expect(MiqTemplate.count).to_not be_zero
with_vcr_data { expect { EmsRefresh.refresh(new_amazon_ems) }.to_not change { MiqTemplate.count } }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should also test the :ems_id is set to the new_amazon_ems.id, in both cases (that is the actual reconnect)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

private

def new_amazon_ems
FactoryGirl.create(:ems_amazon_with_vcr_authentication, :zone => zone, :provider_region => 'eu-central-1')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, its some leftover from the preliminary research

Copy link
Contributor

@Ladas Ladas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AlexanderZagaynov I think it looks good now, just few rubocop issue left

Also, can you make sure this passes Ovirt, OpenStack and AWS specs? Since the CI doesn't run them for us.

@agrare
Copy link
Member

agrare commented Feb 16, 2018

@AlexanderZagaynov is this still WIP?

@miq-bot
Copy link
Member

miq-bot commented Feb 19, 2018

This pull request is not mergeable. Please rebase and repush.

@AlexanderZagaynov AlexanderZagaynov changed the title [WIP] query also archived objects for a new ems query also archived objects for a new ems Feb 19, 2018
@AlexanderZagaynov
Copy link
Contributor Author

@Ladas @agrare please review.

P.S. There is a kinda big code refactoring, I did write a specific tests for that (they're not included here though - because of their temporary nature and dirty hacking), code and results can be found there: AlexanderZagaynov#2

Copy link
Contributor

@Ladas Ladas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

class << self
private

def define_attribute_getters
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, I don't mind the code not being 100% dry. It's better to be able to easily grep each config (or having IDE just click through, which we can do with then ansible version of the persister)

So less meta is better in this case :-)

@AlexanderZagaynov
Copy link
Contributor Author

@Ladas it is now plain down-to-earth

# frozen_string_literal: true

describe ManagerRefresh::InventoryCollectionDefault do
it 'pickups existing Vm and MiqTemplate records on Amazon EMS re-adding' do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AlexanderZagaynov can we move this spec into amazon repo? ems_amazon_with_vcr_authentication would create cyclical dependecy

Copy link
Contributor

@Ladas Ladas Feb 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it you would like to test it here, we would do just persiter spec, without VCRs

some thing like https://github.com/Ladas/manageiq/blob/19e7b46b2769b4f5c64601232586dfd98875b62f/spec/models/manager_refresh/persister/local_db_finders_spec.rb#L150

just create a mock persister, include there the InventoryCollection of interest, build some mock data, call perister.persist! and check it reused the Vm that was already in the db

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ladas AWS specs removed

@miq-bot
Copy link
Member

miq-bot commented Feb 20, 2018

Checked commit AlexanderZagaynov@103a1cf with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 5 offenses detected

app/models/manager_refresh/inventory_collection_default.rb

@AlexanderZagaynov
Copy link
Contributor Author

specs for AWS moved here: ManageIQ/manageiq-providers-amazon#414

Copy link
Contributor

@Ladas Ladas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AlexanderZagaynov looks great 👍

@Ladas
Copy link
Contributor

Ladas commented Feb 20, 2018

@miq-bot assign @agrare

@Ladas
Copy link
Contributor

Ladas commented Feb 20, 2018

@agrare looks good, @AlexanderZagaynov will test if this doesn't break any cloud/infra provider, then it's ready. (specs are in AWS)

@AlexanderZagaynov
Copy link
Contributor Author

Amazon plugin testing log:

azagayno@azagayno:~/work/manageiq/plugins/amazon$ bundle exec rspec
** override_gem: manageiq-providers-amazon, [{:path=>"/home/azagayno/work/manageiq/plugins/amazon/spec/manageiq/plugins/amazon"}], caller: /home/azagayno/work/manageiq/plugins/amazon/spec/manageiq/bundler.d/local_plugins.rb

Randomized with seed 34784
............................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................

Finished in 3 minutes 23.7 seconds (files took 7.73 seconds to load)
524 examples, 0 failures

Randomized with seed 34784

Openstack plugin testing log:

azagayno@azagayno:~/work/manageiq/plugins/openstack$ bundle exec rspec
** override_gem: manageiq-providers-amazon, [{:path=>"/home/azagayno/work/manageiq/plugins/openstack/spec/manageiq/plugins/amazon"}], caller: /home/azagayno/work/manageiq/plugins/openstack/spec/manageiq/bundler.d/local_plugins.rb

Randomized with seed 18723
............................................................................................................................................................................................................................................................................*.......................................
WARNING: Use of `have_attributes` with array access (:[]) is deprecated and will be removed shortly.
If you're matching attributes in hashes, use appropriate hash matchers instead (`include`, `eq`).
Called from /home/azagayno/work/manageiq/plugins/openstack/spec/models/manageiq/providers/openstack/infra_manager/event_parser_spec.rb:78:in `block (3 levels) in <top (required)>'
.
WARNING: Use of `have_attributes` with array access (:[]) is deprecated and will be removed shortly.
If you're matching attributes in hashes, use appropriate hash matchers instead (`include`, `eq`).
Called from /home/azagayno/work/manageiq/plugins/openstack/spec/models/manageiq/providers/openstack/infra_manager/event_parser_spec.rb:35:in `block (3 levels) in <top (required)>'
.
WARNING: Use of `have_attributes` with array access (:[]) is deprecated and will be removed shortly.
If you're matching attributes in hashes, use appropriate hash matchers instead (`include`, `eq`).
Called from /home/azagayno/work/manageiq/plugins/openstack/spec/models/manageiq/providers/openstack/infra_manager/event_parser_spec.rb:64:in `block (3 levels) in <top (required)>'
.
WARNING: Use of `have_attributes` with array access (:[]) is deprecated and will be removed shortly.
If you're matching attributes in hashes, use appropriate hash matchers instead (`include`, `eq`).
Called from /home/azagayno/work/manageiq/plugins/openstack/spec/models/manageiq/providers/openstack/infra_manager/event_parser_spec.rb:92:in `block (3 levels) in <top (required)>'
.
WARNING: Use of `have_attributes` with array access (:[]) is deprecated and will be removed shortly.
If you're matching attributes in hashes, use appropriate hash matchers instead (`include`, `eq`).
Called from /home/azagayno/work/manageiq/plugins/openstack/spec/models/manageiq/providers/openstack/infra_manager/event_parser_spec.rb:50:in `block (3 levels) in <top (required)>'
.
WARNING: Use of `have_attributes` with array access (:[]) is deprecated and will be removed shortly.
If you're matching attributes in hashes, use appropriate hash matchers instead (`include`, `eq`).
Called from /home/azagayno/work/manageiq/plugins/openstack/spec/models/manageiq/providers/openstack/infra_manager/event_parser_spec.rb:15:in `block (3 levels) in <top (required)>'
...........................................[fog][DEPRECATION] #get(id) is deprecated, use #get(name, id) instead (/home/azagayno/work/manageiq/plugins/openstack/app/models/manageiq/providers/openstack/inventory/collector/target_collection.rb:89:in `block in get_orchestration_stack')
[fog][DEPRECATION] #find_by_id(id) is deprecated, use #get(name, id) instead (/home/azagayno/.rvm/gems/ruby-2.3.6/gems/fog-openstack-0.1.22/lib/fog/orchestration/openstack/models/stacks.rb:33:in `get')
[fog][DEPRECATION] #get(id) is deprecated, use #get(name, id) instead (/home/azagayno/work/manageiq/plugins/openstack/app/models/manageiq/providers/openstack/inventory/collector/target_collection.rb:89:in `block in get_orchestration_stack')
[fog][DEPRECATION] #find_by_id(id) is deprecated, use #get(name, id) instead (/home/azagayno/.rvm/gems/ruby-2.3.6/gems/fog-openstack-0.1.22/lib/fog/orchestration/openstack/models/stacks.rb:33:in `get')
...............................................................................................................

Pending: (Failures listed here are expected and do not affect your suite's status)

  1) ManageIQ::Providers::Openstack::NetworkManager::CloudSubnet refresh should update the subnet's router association correctly if the interface is simultaneously removed and replaced
     # needs a schema update to associate routers to subnets through network ports instead of directly
     Failure/Error: expect(CloudSubnet.first.network_router_id).to eq(NetworkRouter.first.id)
     
       expected: 95000000000263
            got: nil
     
       (compared using ==)
     # ./spec/models/manageiq/providers/openstack/network_manager/cloud_subnet_refresh_spec.rb:85:in `block (3 levels) in <top (required)>'

Finished in 5 minutes 34 seconds (files took 8.96 seconds to load)
467 examples, 0 failures, 1 pending

Randomized with seed 18723

oVirt plugin testing log:

azagayno@azagayno:~/work/manageiq/plugins/ovirt$ bundle exec rspec
** override_gem: manageiq-providers-amazon, [{:path=>"/home/azagayno/work/manageiq/plugins/ovirt/spec/manageiq/plugins/amazon"}], caller: /home/azagayno/work/manageiq/plugins/ovirt/spec/manageiq/bundler.d/local_plugins.rb

Randomized with seed 33633
..............................................................................................................................
WARNING: Use of `have_attributes` with array access (:[]) is deprecated and will be removed shortly.
If you're matching attributes in hashes, use appropriate hash matchers instead (`include`, `eq`).
Called from /home/azagayno/work/manageiq/plugins/ovirt/spec/models/manageiq/providers/redhat/infra_manager/event_catcher/runner_spec.rb:17:in `block (4 levels) in <top (required)>'
.
WARNING: Use of `have_attributes` with array access (:[]) is deprecated and will be removed shortly.
If you're matching attributes in hashes, use appropriate hash matchers instead (`include`, `eq`).
Called from /home/azagayno/work/manageiq/plugins/ovirt/spec/models/manageiq/providers/redhat/infra_manager/event_catcher/runner_spec.rb:22:in `block (4 levels) in <top (required)>'
....................................................................................................................................................
WARNING: Use of `have_attributes` with array access (:[]) is deprecated and will be removed shortly.
If you're matching attributes in hashes, use appropriate hash matchers instead (`include`, `eq`).
Called from /home/azagayno/work/manageiq/plugins/ovirt/spec/models/manageiq/providers/redhat/infra_manager/event_parser_spec.rb:115:in `block (5 levels) in <top (required)>'

WARNING: Use of `have_attributes` with array access (:[]) is deprecated and will be removed shortly.
If you're matching attributes in hashes, use appropriate hash matchers instead (`include`, `eq`).
Called from /home/azagayno/work/manageiq/plugins/ovirt/spec/models/manageiq/providers/redhat/infra_manager/event_parser_spec.rb:127:in `block (5 levels) in <top (required)>'

WARNING: Use of `have_attributes` with array access (:[]) is deprecated and will be removed shortly.
If you're matching attributes in hashes, use appropriate hash matchers instead (`include`, `eq`).
Called from /home/azagayno/work/manageiq/plugins/ovirt/spec/models/manageiq/providers/redhat/infra_manager/event_parser_spec.rb:135:in `block (5 levels) in <top (required)>'

WARNING: Use of `have_attributes` with array access (:[]) is deprecated and will be removed shortly.
If you're matching attributes in hashes, use appropriate hash matchers instead (`include`, `eq`).
Called from /home/azagayno/work/manageiq/plugins/ovirt/spec/models/manageiq/providers/redhat/infra_manager/event_parser_spec.rb:142:in `block (5 levels) in <top (required)>'

WARNING: Use of `have_attributes` with array access (:[]) is deprecated and will be removed shortly.
If you're matching attributes in hashes, use appropriate hash matchers instead (`include`, `eq`).
Called from /home/azagayno/work/manageiq/plugins/ovirt/spec/models/manageiq/providers/redhat/infra_manager/event_parser_spec.rb:115:in `block (5 levels) in <top (required)>'

WARNING: Use of `have_attributes` with array access (:[]) is deprecated and will be removed shortly.
If you're matching attributes in hashes, use appropriate hash matchers instead (`include`, `eq`).
Called from /home/azagayno/work/manageiq/plugins/ovirt/spec/models/manageiq/providers/redhat/infra_manager/event_parser_spec.rb:127:in `block (5 levels) in <top (required)>'

WARNING: Use of `have_attributes` with array access (:[]) is deprecated and will be removed shortly.
If you're matching attributes in hashes, use appropriate hash matchers instead (`include`, `eq`).
Called from /home/azagayno/work/manageiq/plugins/ovirt/spec/models/manageiq/providers/redhat/infra_manager/event_parser_spec.rb:135:in `block (5 levels) in <top (required)>'

WARNING: Use of `have_attributes` with array access (:[]) is deprecated and will be removed shortly.
If you're matching attributes in hashes, use appropriate hash matchers instead (`include`, `eq`).
Called from /home/azagayno/work/manageiq/plugins/ovirt/spec/models/manageiq/providers/redhat/infra_manager/event_parser_spec.rb:142:in `block (5 levels) in <top (required)>'

WARNING: Use of `have_attributes` with array access (:[]) is deprecated and will be removed shortly.
If you're matching attributes in hashes, use appropriate hash matchers instead (`include`, `eq`).
Called from /home/azagayno/work/manageiq/plugins/ovirt/spec/models/manageiq/providers/redhat/infra_manager/event_parser_spec.rb:115:in `block (5 levels) in <top (required)>'

WARNING: Use of `have_attributes` with array access (:[]) is deprecated and will be removed shortly.
If you're matching attributes in hashes, use appropriate hash matchers instead (`include`, `eq`).
Called from /home/azagayno/work/manageiq/plugins/ovirt/spec/models/manageiq/providers/redhat/infra_manager/event_parser_spec.rb:127:in `block (5 levels) in <top (required)>'

WARNING: Use of `have_attributes` with array access (:[]) is deprecated and will be removed shortly.
If you're matching attributes in hashes, use appropriate hash matchers instead (`include`, `eq`).
Called from /home/azagayno/work/manageiq/plugins/ovirt/spec/models/manageiq/providers/redhat/infra_manager/event_parser_spec.rb:135:in `block (5 levels) in <top (required)>'

WARNING: Use of `have_attributes` with array access (:[]) is deprecated and will be removed shortly.
If you're matching attributes in hashes, use appropriate hash matchers instead (`include`, `eq`).
Called from /home/azagayno/work/manageiq/plugins/ovirt/spec/models/manageiq/providers/redhat/infra_manager/event_parser_spec.rb:142:in `block (5 levels) in <top (required)>'
.
WARNING: Use of `have_attributes` with array access (:[]) is deprecated and will be removed shortly.
If you're matching attributes in hashes, use appropriate hash matchers instead (`include`, `eq`).
Called from /home/azagayno/work/manageiq/plugins/ovirt/spec/models/manageiq/providers/redhat/infra_manager/event_parser_spec.rb:42:in `block (4 levels) in <top (required)>'
.
WARNING: Use of `have_attributes` with array access (:[]) is deprecated and will be removed shortly.
If you're matching attributes in hashes, use appropriate hash matchers instead (`include`, `eq`).
Called from /home/azagayno/work/manageiq/plugins/ovirt/spec/models/manageiq/providers/redhat/infra_manager/event_parser_spec.rb:211:in `block (3 levels) in <top (required)>'
....

Finished in 3 minutes 50.3 seconds (files took 7.1 seconds to load)
281 examples, 0 failures

Randomized with seed 33633

Ansible Tower plugin testing log:

azagayno@azagayno:~/work/manageiq/plugins/ansible_tower$ bundle exec rspec
** override_gem: manageiq-providers-amazon, [{:path=>"/home/azagayno/work/manageiq/plugins/ansible_tower/spec/manageiq/plugins/amazon"}], caller: /home/azagayno/work/manageiq/plugins/ansible_tower/spec/manageiq/bundler.d/local_plugins.rb

Randomized with seed 2700
....................................................................

Finished in 24.74 seconds (files took 7.35 seconds to load)
68 examples, 0 failures

Randomized with seed 2700

Copy link
Member

@agrare agrare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 LGTM

@agrare agrare merged commit 80875c7 into ManageIQ:master Feb 23, 2018
@agrare agrare modified the milestones: Roadmap, Sprint 80 Ending Feb 26, 2018 Feb 23, 2018
simaishi pushed a commit that referenced this pull request Mar 7, 2018
query also archived objects for a new ems
(cherry picked from commit 80875c7)

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1552798
@simaishi
Copy link
Contributor

simaishi commented Mar 7, 2018

Gaprindashvili backport details:

$ git log -1
commit b68c6a74f70f52ef69b57224ad1e221e68def28a
Author: Adam Grare <agrare@redhat.com>
Date:   Fri Feb 23 12:46:12 2018 -0500

    Merge pull request #16886 from AlexanderZagaynov/BZ-1536406
    
    query also archived objects for a new ems
    (cherry picked from commit 80875c7c0fbcbfedb9bb1f85dfc6810c693542e5)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1552798

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants