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.

Sign up for GitHub

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

Jump to bottom

Datastores duplicated after a refresh #16408

Merged
merged 1 commit into from
Nov 9, 2017
Merged

Datastores duplicated after a refresh #16408

merged 1 commit into from
Nov 9, 2017

Conversation

pkliczewski
Copy link
Contributor

There was no way to identify uniquely datastore during a refresh
which resulted in duplications. This PR solves this issue.

Bug-Url:
https://bugzilla.redhat.com/1510053

@pkliczewski
Copy link
Contributor Author

@agrare @Ladas please review

@pkliczewski
Copy link
Contributor Author

Bug 1510053 - CFME with RHV Graph refresh turned ON - RHV Datastores are displayed multiple times.

@agrare
Copy link
Member

agrare commented Nov 7, 2017

@pkliczewski I thought we couldn't just use :location for some reason but it is escaping me now

@agrare
Copy link
Member

agrare commented Nov 7, 2017

Oh it was because this is using the ems.storages relation which ignores storages from other EMSs, so it wasn't that using location didn't work but that it wasn't enough to really fix the issue.

@pkliczewski
Copy link
Contributor Author

@agrare :location is unique for RHV. Do you have suggestions how to fix it for InfraManager globally or should I move it to RHV specific defaults?

BTW. Travis failure is not related to this PR

Ladas
Ladas reviewed Nov 8, 2017
View reviewed changes
app/models/manager_refresh/inventory_collection_default/infra_manager.rb Outdated
@@ -124,7 +124,7 @@ def ems_clusters(extra_attributes = {})
def storages(extra_attributes = {})
attributes = {
:model_class => ::Storage,
# TODO: change :manager_ref => [:location],
:manager_ref => [:location],
Copy link
Contributor

Choose a reason for hiding this comment

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

So, is the location unique in a whole provider? If yes, then it's ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is unique

Copy link
Member

Choose a reason for hiding this comment

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

Well it can be just an NFS path, https://github.com/ManageIQ/manageiq-providers-ovirt/blob/master/app/models/manageiq/providers/redhat/infra_manager/refresh/parse/parser.rb#L46

This will not be unique across providers on purpose, the idea is that it will link the same storage across providers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct it is unique within single provider. Do you have a suggestion how to make sure it is unique across the providers?

@pkliczewski
Datastores duplicated after a refresh
fc3684e 
There was no way to identify uniqely datastore during a refresh
which resulted in duplications. This PR solves this issue.

Bug-Url:
https://bugzilla.redhat.com/1510053
@miq-bot
Copy link
Member

miq-bot commented Nov 9, 2017

Checked commit pkliczewski@fc3684e with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks fine. 🏆

Ladas
Ladas approved these changes Nov 9, 2017
View reviewed changes
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.

Ok, this should work, but without spec, we will not know for sure :-)

@agrare
Copy link
Member

agrare commented Nov 9, 2017

@pkliczewski can you test that this picks up existing storages? Maybe create a storage manually with an expected location and make sure that is picked up and not duplicated.

@pkliczewski
Copy link
Contributor Author

@agrare I can confirm it picks up existing storages

@agrare
Copy link
Member

agrare commented Nov 9, 2017

👍 thanks @pkliczewski

@agrare agrare merged commit c736542 into ManageIQ:master Nov 9, 2017
@agrare agrare added this to the Sprint 73 Ending Nov 13, 2017 milestone Nov 9, 2017
@agrare agrare mentioned this pull request Nov 13, 2017
Merged
@agrare
Copy link
Member

agrare commented Nov 13, 2017

@simaishi please backport ManageIQ/manageiq-providers-vmware#135 once this is backported

simaishi pushed a commit that referenced this pull request Nov 15, 2017
@agrare @simaishi
Merge pull request #16408 from pkliczewski/master
09f9fe6 
Datastores duplicated after a refresh
(cherry picked from commit c736542)
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit 09f9fe66dafb95ae4fca0c01d8e945977573ad00
Author: Adam Grare <agrare@redhat.com>
Date:   Thu Nov 9 10:54:12 2017 -0500

    Merge pull request #16408 from pkliczewski/master
    
    Datastores duplicated after a refresh
    (cherry picked from commit c736542bd941e275f1765a27fe12989c9a73704d)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ladas Ladas Ladas approved these changes

@agrare agrare agrare approved these changes

Assignees

@agrare agrare

Labels
bug gaprindashvili/backported providers/inventory
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@pkliczewski @agrare @miq-bot @simaishi @Ladas @chessbyte