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

DB models and API changes for host mapping #606

Merged

Conversation

sushanthakumar
Copy link
Collaborator

@sushanthakumar sushanthakumar commented Jun 14, 2021

What this PR does / why we need it:
This PR adds DB APIs and models for host mapping models

Feature Design doc for reference:
RequirementsDesignSpec_HostMapping.docx

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:
UT scripts to be added to this PR
Currently fake values are random values without any relation just to verify db operations

Test report
DB Tables
Selection_462

DB Table Entries

Selection_454

Selection_455

Selection_456

Selection_457

Selection_458

Selection_459

Selection_460

Selection_461

Selection_464

Release note:

@sushanthakumar sushanthakumar changed the base branch from master to host_mapping June 14, 2021 02:28
@codecov
Copy link

codecov bot commented Jun 14, 2021

Codecov Report

Merging #606 (80b2563) into host_mapping (73fe3a1) will decrease coverage by 2.73%.
The diff coverage is 33.56%.

@@               Coverage Diff                @@
##           host_mapping     #606      +/-   ##
================================================
- Coverage         72.13%   69.40%   -2.74%     
================================================
  Files               141      144       +3     
  Lines             12211    13795    +1584     
  Branches           1446     1653     +207     
================================================
+ Hits               8809     9575     +766     
- Misses             2909     3688     +779     
- Partials            493      532      +39     
Impacted Files Coverage Δ
delfin/db/sqlalchemy/api.py 59.39% <17.54%> (-20.63%) ⬇️
delfin/db/api.py 79.68% <50.00%> (-15.13%) ⬇️
delfin/db/sqlalchemy/models.py 99.66% <100.00%> (+0.12%) ⬆️
delfin/exception.py 98.94% <100.00%> (+0.11%) ⬆️
delfin/coordination.py 63.93% <0.00%> (-23.08%) ⬇️
...er/scheduler/schedulers/telemetry/telemetry_job.py 77.27% <0.00%> (-15.04%) ⬇️
...duler/schedulers/telemetry/failed_telemetry_job.py 79.74% <0.00%> (-11.03%) ⬇️
delfin/exporter/prometheus/prometheus.py 26.38% <0.00%> (-5.62%) ⬇️
delfin/drivers/dell_emc/unity/unity.py 71.42% <0.00%> (-5.20%) ⬇️
delfin/task_manager/scheduler/schedule_manager.py 55.55% <0.00%> (-4.45%) ⬇️
... and 28 more

@sushanthakumar sushanthakumar changed the title [WIP] DB API changes for host mapping DB API changes for host mapping Jun 14, 2021
@sushanthakumar
Copy link
Collaborator Author

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

delfin/db/api.py Outdated
@@ -841,3 +841,491 @@ def failed_task_delete_by_storage(context, storage_id):
does not exist.
"""
return IMPL.failed_task_delete_by_storage(context, storage_id)


def storage_initiators_create(context, values):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we use just initiators instead of storage_initiators ?

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

return IMPL.storage_hosts_delete(context, values)


def storage_hosts_get(context, storage_host_id):
Copy link
Collaborator

Choose a reason for hiding this comment

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

storage_hosts -> hosts ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This need not change

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.

This may not require as we keep host relation in initiator

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

@sushanthakumar
Copy link
Collaborator Author

Pls help to review @wisererik @kumarashit @joseph-v @AmitRoushan @ThisIsClark

@sushanthakumar sushanthakumar changed the title DB API changes for host mapping DB models and API changes for host mapping Jun 19, 2021
@@ -170,6 +170,42 @@ class VolumeNotFound(NotFound):
msg_fmt = _("Volume {0} could not be found.")


class StorageHostInitiatorNotFound(NotFound):
msg_fmt = _("Storage host initiator{0} could not be found.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

initiator{0} should be initiator {0}

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

@sushanthakumar
Copy link
Collaborator Author

sushanthakumar commented Jun 23, 2021

Updated for the comments
@NajmudheenCT @kumarashit @wisererik @ThisIsClark @AmitRoushan pls check and approve



class VolumeGroupNotFound(NotFound):
msg_fmt = _("Port group {0} could not be found.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

msg_fmt is same as PortGroupNotFound

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

Copy link
Collaborator

@ThisIsClark ThisIsClark left a comment

Choose a reason for hiding this comment

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

LGTM

delfin/db/api.py Outdated
host_grp_host_relation_id)


def storage_host_grp_host_relations_get_all(context, marker=None, limit=None,
Copy link
Member

Choose a reason for hiding this comment

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

lets use table name as suffix for all operations storage_host_grp_host_rels_get_all ? applicable for all group relation related APS

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

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

native_storage_host_id = Column(String(255))
storage_host_initiators = Column(Text)
volumes = Column(Text)
ports = Column(Text)
Copy link
Collaborator

Choose a reason for hiding this comment

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

confused about the below defintion, can you explain it

    storage_host_initiators = Column(Text)
    volumes = Column(Text)
    ports = Column(Text)

Copy link
Collaborator Author

@sushanthakumar sushanthakumar Jun 25, 2021

Choose a reason for hiding this comment

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

Provided some details on this at driver api side.
image

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

@wisererik wisererik merged commit ef852eb into sodafoundation:host_mapping Jun 26, 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.

6 participants