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

DB models for host mapping #596

Closed
wants to merge 2 commits into from
Closed

DB models for host mapping #596

wants to merge 2 commits into from

Conversation

sushanthakumar
Copy link
Collaborator

@sushanthakumar sushanthakumar commented Jun 6, 2021 •
edited
Loading

What this PR does / why we need it:
This PR adds db models needed for host mapping feature

Below tables are newly added
StorageInitiator
StorageHost
StorageHostGroup
PortGroup
VolumeGroup
MaskingView
StorageHostGrpStorageHostRelation
PortGrpPortRelation
VolumeGrpVolumeRelation

Note : Some of these tables contain array of items for which Text data type is used. Array items will be separated using delimiters

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:

Test report:
image

@NajmudheenCT NajmudheenCT mentioned this pull request Jun 8, 2021
Closed
6 tasks
@codecov
Copy link

codecov bot commented Jun 8, 2021 •
edited
Loading

Codecov Report

Merging #596 (b1b28a2) into host_mapping (73fe3a1) will increase coverage by 0.14%.
The diff coverage is 100.00%.

@@               Coverage Diff                @@
##           host_mapping     #596      +/-   ##
================================================
+ Coverage         72.13%   72.28%   +0.14%     
================================================
  Files               141      141              
  Lines             12211    12545     +334     
  Branches           1446     1480      +34     
================================================
+ Hits               8809     9068     +259     
- Misses             2909     2976      +67     
- Partials            493      501       +8
Impacted Files Coverage Δ
delfin/db/sqlalchemy/models.py 99.66% <100.00%> (+0.12%) ⬆️
delfin/drivers/netapp/dataontap/netapp_handler.py 68.45% <0.00%> (-4.39%) ⬇️
delfin/drivers/netapp/dataontap/constants.py 100.00% <0.00%> (ø)
delfin/drivers/utils/tools.py 90.38% <0.00%> (+0.18%) ⬆️
delfin/drivers/utils/ssh_client.py 19.79% <0.00%> (+0.31%) ⬆️
delfin/drivers/netapp/dataontap/cluster_mode.py 93.18% <0.00%> (+15.90%) ⬆️

@sushanthakumar sushanthakumar changed the title [WIP] DB models for host mapping DB models for host mapping Jun 8, 2021
@sushanthakumar sushanthakumar changed the base branch from master to host_mapping June 11, 2021 08:25
@sushanthakumar sushanthakumar force-pushed the hostmapping-models branch 2 times, most recently from f76942a to 659ef9e Compare June 11, 2021 09:15
@sushanthakumar
Copy link
Collaborator Author

@NajmudheenCT @wisererik @kumarashit @joseph-v @AmitRoushan , pls help to review

joseph-v
joseph-v reviewed Jun 15, 2021
View reviewed changes
delfin/db/sqlalchemy/models.py Outdated
@@ -286,3 +287,115 @@ class FailedTask(BASE, DelfinBase):
job_id = Column(String(36))
deleted_at = Column(DateTime)
deleted = Column(Boolean, default=False)


class StorageInitiator(BASE, DelfinBase):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove Storage prefix for models if it not adds clarity

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated StorageInitiator

NajmudheenCT
NajmudheenCT reviewed Jun 15, 2021
View reviewed changes
delfin/db/sqlalchemy/models.py Outdated
name = Column(String(255))
description = Column(String(255))
os_type = Column(String(255))
storage_initiators = Column(Text)
Copy link
Member

Choose a reason for hiding this comment

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

Is there a poosibility to use Array type https://docs.sqlalchemy.org/en/14/core/type_basics.html#sqlalchemy.types.ARRAY

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, sqlalchemy supports ARRAY type. But only when it is used with PostgreSQL backend. We can not use with SQLite

Selection_388

Selection_389

NajmudheenCT
NajmudheenCT reviewed Jun 19, 2021
View reviewed changes
delfin/db/sqlalchemy/models.py
name = Column(String(255))
description = Column(String(255))
os_type = Column(String(255))
storage_host_initiators = Column(Text)
Copy link
Member

Choose a reason for hiding this comment

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

Remove as we keep host info in initiator

@sushanthakumar
Copy link
Collaborator Author

sushanthakumar commented Jun 19, 2021 •
edited
Loading

These changes are already part of PR #606.
All review comments mentioned here are addressed in above PR, so closing this PR

joseph-v pushed a commit that referenced this pull request Nov 22, 2022
@ghxiaobo
Merge pull request #596 from zhilong-xu/macro
3ebd6d8 
add macro_san storage
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NajmudheenCT NajmudheenCT NajmudheenCT left review comments

@joseph-v joseph-v joseph-v left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sushanthakumar @joseph-v @NajmudheenCT