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

Build resource group and resource relations #673

Merged
merged 2 commits into from
Aug 31, 2021

Conversation

joseph-v
Copy link
Collaborator

What this PR does / why we need it:
Build resource group and resource relations for resources of storage host, port and volume.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

Release note:

@@ -2490,7 +2490,7 @@ def masking_views_delete_by_storage(context, storage_id):


def _storage_host_grp_host_rels_get_query(context, session=None):
return model_query(context, models.StorageHostGrpHostRel,
return model_query(context, models.StorageHostGroupRelation,
Copy link
Collaborator

Choose a reason for hiding this comment

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

U can refer table names from existing models and update the usages accordingly

@codecov
Copy link

codecov bot commented Aug 23, 2021

Codecov Report

Merging #673 (ed0ef0a) into master (d228c06) will increase coverage by 0.68%.
The diff coverage is 81.30%.

@@            Coverage Diff             @@
##           master     #673      +/-   ##
==========================================
+ Coverage   70.15%   70.83%   +0.68%     
==========================================
  Files         156      156              
  Lines       14801    14913     +112     
  Branches     1822     1855      +33     
==========================================
+ Hits        10384    10564     +180     
+ Misses       3816     3743      -73     
- Partials      601      606       +5     
Impacted Files Coverage Δ
delfin/api/v1/port_groups.py 42.85% <0.00%> (-14.29%) ⬇️
delfin/api/v1/storage_host_groups.py 42.85% <0.00%> (-14.29%) ⬇️
delfin/api/v1/volume_groups.py 42.85% <0.00%> (-14.29%) ⬇️
delfin/db/sqlalchemy/api.py 71.59% <ø> (+2.51%) ⬆️
delfin/drivers/fake_storage/__init__.py 95.14% <96.07%> (+4.49%) ⬆️
delfin/task_manager/tasks/resources.py 81.93% <100.00%> (+8.11%) ⬆️
delfin/db/api.py 89.06% <0.00%> (+1.87%) ⬆️

@@ -741,6 +815,12 @@ def sync(self):
# Build relation between host grp and host to be handled here.
storage_host_groups = self.driver_api \
.list_storage_host_groups(self.context, self.storage_id)
if storage_host_groups:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Whenever we build group relations for any sync cycle, old relations to be flushed first.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Already doing it, line no: 89,115,136

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, fine, missed it

@@ -366,7 +366,7 @@ class StorageHostGrpHostRel(BASE, DelfinBase):
"""Represents the storage host group and storage host relation
attributes.
"""
__tablename__ = 'storage_host_grp_host_rels'
__tablename__ = 'storage_host_group_relations'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Currently tablename is in sync with class name, I think we can follow same convention here as well

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done, thanks.

@joseph-v joseph-v changed the title [WIP] Build resource group and resource relations Build resource group and resource relations Aug 24, 2021
Copy link
Member

@NajmudheenCT NajmudheenCT left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@NajmudheenCT NajmudheenCT left a comment

Choose a reason for hiding this comment

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

We should add some UTs for host mapping APIS..

@sushanthakumar
Copy link
Collaborator

LGTM

@joseph-v joseph-v force-pushed the hostmapping-relations branch 4 times, most recently from 9d5e0ee to 53aad5f Compare August 25, 2021 10:53
Copy link
Member

@NajmudheenCT NajmudheenCT left a comment

Choose a reason for hiding this comment

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

LGTM

@sushanthakumar
Copy link
Collaborator

LGTM

Copy link
Collaborator

@skdwriting skdwriting left a comment

Choose a reason for hiding this comment

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

LGTM

@skdwriting skdwriting merged commit cfd5a1c into sodafoundation:master Aug 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants