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

Add Disk resource to Delfin #407

Merged
merged 3 commits into from
Dec 18, 2020
Merged

Conversation

joseph-v
Copy link
Collaborator

@joseph-v joseph-v commented Dec 3, 2020

What this PR does / why we need it:
Add disk resource to Delfin resource model

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

Special notes for your reviewer:

Release note:

Screenshot from 2020-12-03 12-03-14

@joseph-v joseph-v force-pushed the disk-resource branch 2 times, most recently from 7fa02e9 to 6a6cf4a Compare December 3, 2020 07:05
@codecov
Copy link

codecov bot commented Dec 3, 2020

Codecov Report

Merging #407 (55a5b43) into master (9099b30) will increase coverage by 0.43%.
The diff coverage is 79.51%.

@@            Coverage Diff             @@
##           master     #407      +/-   ##
==========================================
+ Coverage   67.08%   67.52%   +0.43%     
==========================================
  Files          99      101       +2     
  Lines        6800     6996     +196     
  Branches      762      780      +18     
==========================================
+ Hits         4562     4724     +162     
- Misses       2010     2035      +25     
- Partials      228      237       +9     
Impacted Files Coverage Δ
delfin/api/views/disks.py 42.85% <42.85%> (ø)
delfin/drivers/dell_emc/vmax/vmax.py 82.45% <50.00%> (-1.19%) ⬇️
delfin/drivers/hpe/hpe_3par/hpe_3parstor.py 76.00% <50.00%> (-1.09%) ⬇️
delfin/drivers/huawei/oceanstor/oceanstor.py 84.84% <50.00%> (-0.72%) ⬇️
delfin/api/v1/disks.py 52.00% <52.00%> (ø)
delfin/drivers/driver.py 71.42% <66.66%> (-0.37%) ⬇️
delfin/db/sqlalchemy/api.py 79.37% <78.08%> (+0.39%) ⬆️
delfin/task_manager/tasks/resources.py 77.82% <81.48%> (+0.44%) ⬆️
delfin/db/api.py 95.08% <88.88%> (+2.97%) ⬆️
delfin/api/v1/router.py 100.00% <100.00%> (ø)
... and 8 more

@joseph-v joseph-v force-pushed the disk-resource branch 7 times, most recently from 2da166e to 4ae161e Compare December 7, 2020 04:54
delfin/api/v1/disks.py Outdated Show resolved Hide resolved
NajmudheenCT
NajmudheenCT previously approved these changes Dec 15, 2020
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

@ThisIsClark
Copy link
Collaborator

Same doubt with PR 403:

  1. It seems that there is only GET api, so does the create/update/delete api in db has been tested?
  2. The rest api should be added to swagger.yaml

ThisIsClark
ThisIsClark previously approved these changes Dec 17, 2020
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

capacity = Column(BigInteger)
status = Column(String(255))
physical_type = Column(String(255))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why the size of column always 255?AFAIK, Disk table may have a join with table with huge number of rows, this will have performance issue as per memory allocations.
Basically query processing will have side effects

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Raised an issue for this (#434) and will be handled separately.

@joseph-v joseph-v force-pushed the disk-resource branch 3 times, most recently from 56b8381 to ce1d74f Compare December 18, 2020 06:34
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
Collaborator

@kumarashit kumarashit left a comment

Choose a reason for hiding this comment

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

LGTM

@kumarashit kumarashit merged commit b3bcba0 into sodafoundation:master Dec 18, 2020
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.

Resource management improvement implementation
4 participants