Add NAS resources for filesystem, qtree & share #491

joseph-v · 2021-02-17T02:51:55Z

What this PR does / why we need it:
Add NAS resource filesystem

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

Special notes for your reviewer:

Release note:

REST API tests:

1. Filesystem

2. Qtree

3. Share

codecov · 2021-02-18T03:41:39Z

Codecov Report

Merging #491 (e4c64a7) into master (aa77007) will increase coverage by 0.19%.
The diff coverage is 72.01%.

@@            Coverage Diff             @@
##           master     #491      +/-   ##
==========================================
+ Coverage   67.96%   68.15%   +0.19%     
==========================================
  Files         113      119       +6     
  Lines        8218     8806     +588     
  Branches      926      983      +57     
==========================================
+ Hits         5585     6002     +417     
- Misses       2321     2467     +146     
- Partials      312      337      +25

Impacted Files	Coverage Δ
delfin/task_manager/tasks/resources.py	`67.76% <39.08%> (-10.07%)`	⬇️
delfin/api/views/filesystems.py	`42.85% <42.85%> (ø)`
delfin/api/views/qtrees.py	`42.85% <42.85%> (ø)`
delfin/api/views/shares.py	`42.85% <42.85%> (ø)`
delfin/drivers/driver.py	`68.75% <50.00%> (-2.68%)`	⬇️
delfin/api/v1/filesystems.py	`52.00% <52.00%> (ø)`
delfin/api/v1/qtrees.py	`52.00% <52.00%> (ø)`
delfin/api/v1/shares.py	`52.00% <52.00%> (ø)`
delfin/drivers/fake_storage/__init__.py	`76.74% <53.84%> (-5.08%)`	⬇️
delfin/db/sqlalchemy/api.py	`79.56% <80.08%> (+0.18%)`	⬆️
... and 12 more

delfin/api/v1/filesystems.py

NajmudheenCT · 2021-02-26T10:49:14Z

delfin/common/constants.py

+
+class QuotaState(object):
+ NORMAL = 'normal'
+ SOFT = 'soft_limit'


This state is not clear, Qtree having state= soft_limit means what?

State can be either in normal, soft_limit hit, hard_limit hit. When soft_limit, it is between normal and hard_limit

If Quota State reached Hard limit, what do we call "Hard" or "Abnormal"? Also, for Soft limit "Normal" or "Soft"?

NajmudheenCT · 2021-02-26T10:50:57Z

delfin/db/sqlalchemy/models.py

+ free_capacity = Column(BigInteger)
+ compression = Column(Boolean)
+ deduplication = Column(Boolean)
+ worm = Column(Boolean)


Specifies if this is WORM filesystem

NajmudheenCT · 2021-02-26T11:05:50Z

delfin/task_manager/tasks/resources.py

+
+ @check_deleted()
+ @set_synced_after()
+ def sync(self):


Sync will be called for each subclass of StorageResourceTask ! how do we make sure it is called only if it the driver is an instance of NAS driver ?

kumarashit · 2021-03-09T17:06:53Z

delfin/common/constants.py

+ ALL = (NORMAL, FAULTY)
+
+
+class WORMType(object):


Not very clear as WORM is basically compliance or protection. Can we call it WORMProtection?

This is Filesystem's worm type. Filesystem can be either worm or non_worm, if Filesystem is worm we give the type of worm.

kumarashit · 2021-03-09T17:10:24Z

delfin/common/constants.py

+
+class QuotaState(object):
+ NORMAL = 'normal'
+ SOFT = 'soft_limit'


If Quota State reached Hard limit, what do we call "Hard" or "Abnormal"? Also, for Soft limit "Normal" or "Soft"?

wisererik

for the driver definition, please consider it carefully

wisererik · 2021-03-11T06:36:16Z

delfin/drivers/driver.py

+
+
+@six.add_metaclass(abc.ABCMeta)
+class NASDriver(StorageDriver):


there is an impact for sync storage api by defining sub driver, it will fail because sync api use subclass to generate the resource task.
plz check

delfin/common/constants.py

wisererik · 2021-03-11T06:44:09Z

delfin/common/constants.py

+ CIFS = 'cifs'
+ NFS = 'nfs'
+ FTP = 'ftp'
+ UNKNOWN = 'unknown'


protocol should not be unkown. should be specificy.

suggest add hdfs as one of protocols

wisererik · 2021-03-11T06:45:27Z

delfin/db/sqlalchemy/models.py

+ storage_id = Column(String(36))
+ native_filesystem_id = Column(String(255))
+ native_pool_id = Column(String(255))
+ status = Column(String(255))


please put accociated object info to the last location

wisererik · 2021-03-11T06:45:48Z

delfin/db/sqlalchemy/models.py

+ storage_id = Column(String(36))
+ native_qtree_id = Column(String(255))
+ native_filesystem_id = Column(String(255))
+ path = Column(String(255))


delfin/db/sqlalchemy/models.py

wisererik · 2021-03-11T11:20:13Z

delfin/drivers/driver.py

+
+ def list_filesystems(self, context):
+ """List all filesystems from storage system."""
+ raise exception.StorageDriverAPINotImplemented("list_filesystems()")


please use

raise NotImplementedError()

wisererik · 2021-03-11T11:20:41Z

delfin/exception.py

+
+
+class StorageDriverAPINotImplemented(DelfinException):
+ msg_fmt = _("Storage driver do not support API: {0}.")


please remove it

wisererik · 2021-03-11T13:47:06Z

delfin/drivers/driver.py

@@ -115,3 +115,18 @@ def list_alerts(self, context, query_para=None):
 def clear_alert(self, context, sequence_number):
 """Clear alert from storage system."""
 pass
+
+ @abc.abstractmethod


suggest remove @abc.abstractmethod, otherewise you need to add method for all drivers

wisererik · 2021-03-11T14:41:47Z

delfin/task_manager/tasks/resources.py

+
+ if add_list:
+ db.filesystems_create(self.context, add_list)
+ except AttributeError as e:


NotImplementedErr should be catched and ignored(using pass) same with other resource tasks

wisererik · 2021-03-12T02:22:14Z

delfin/task_manager/tasks/resources.py

+ except AttributeError as e:
+ LOG.error(e)
+ except NotImplementedError as e:
+ LOG.info(e)


using pass, suggest not log info, because there will be many NotImplementError from storages.

# Ignore this exception because driver may not support it. pass

wisererik

LGTM, @NajmudheenCT please squash and merge it

NajmudheenCT

LGTM

* Add NAS resource filesystem * Separate drivers to NAS, SAN and Unified * Fix formatting * Add Qtree resource support to delfin * Add Share resource support to delfin * Add quota limit params to qtree * Update unit tests * Update model changes * Implement review comments

…e addition (#503) * db changes for task template addition * db changes for task instance addition * Fix for travis CI issue on cryptography dependency (#494) * Fix CI Issue on cryptography dependency * Removing python_version check on crytography package requirement * Add MIXED storage pool type (#482) * Add NAS resources for filesystem, qtree & share (#491) * Add NAS resource filesystem * Separate drivers to NAS, SAN and Unified * Fix formatting * Add Qtree resource support to delfin * Add Share resource support to delfin * Add quota limit params to qtree * Update unit tests * Update model changes * Implement review comments * update db model with task and failed task schema * Fix some comments and name changes * Unit test changes Co-authored-by: Najmudheen <45681499+NajmudheenCT@users.noreply.github.com> Co-authored-by: Joseph Vazhappilly <josephvp@gmail.com> Co-authored-by: Amit Roushan <amit.roushan@huawei.com>

joseph-v force-pushed the nas-driver-support branch 2 times, most recently from 6fa64b2 to 930d760 Compare February 17, 2021 06:55

joseph-v force-pushed the nas-driver-support branch 2 times, most recently from 5282466 to 84b1cd5 Compare February 18, 2021 05:22

joseph-v changed the title ~~[WIP] Add NAS resource filesystem~~ Add NAS resource filesystem Feb 26, 2021

NajmudheenCT reviewed Feb 26, 2021

View reviewed changes

joseph-v changed the title ~~Add NAS resource filesystem~~ Add NAS resources for filesystem, qtree & share Mar 3, 2021

joseph-v force-pushed the nas-driver-support branch 2 times, most recently from 5be7fcb to 1f63916 Compare March 3, 2021 10:47

kumarashit reviewed Mar 9, 2021

View reviewed changes

joseph-v force-pushed the nas-driver-support branch 2 times, most recently from 6464ccc to f307e2c Compare March 10, 2021 13:40

wisererik requested changes Mar 11, 2021

View reviewed changes

joseph-v added 8 commits March 11, 2021 13:08

Add NAS resource filesystem

eb37f7c

Separate drivers to NAS, SAN and Unified

a003f79

Fix formatting

feefade

Add Qtree resource support to delfin

5cbf05d

Add Share resource support to delfin

bee5bd7

Add quota limit params to qtree

f939610

Update unit tests

b469a03

Update model changes

aa15eae

joseph-v force-pushed the nas-driver-support branch from f307e2c to acd113c Compare March 11, 2021 10:15

wisererik requested changes Mar 11, 2021

View reviewed changes

joseph-v force-pushed the nas-driver-support branch 3 times, most recently from 58fdbb3 to 85723c5 Compare March 11, 2021 12:16

wisererik reviewed Mar 11, 2021

View reviewed changes

joseph-v force-pushed the nas-driver-support branch from 85723c5 to 74b5cbf Compare March 11, 2021 14:07

wisererik reviewed Mar 11, 2021

View reviewed changes

joseph-v force-pushed the nas-driver-support branch from 74b5cbf to 6e7ce7c Compare March 11, 2021 17:25

wisererik reviewed Mar 12, 2021

View reviewed changes

joseph-v force-pushed the nas-driver-support branch from 6e7ce7c to bde042e Compare March 12, 2021 03:34

Implement review comments

e4c64a7

joseph-v force-pushed the nas-driver-support branch from bde042e to e4c64a7 Compare March 12, 2021 03:46

wisererik approved these changes Mar 12, 2021

View reviewed changes

NajmudheenCT approved these changes Mar 12, 2021

View reviewed changes

NajmudheenCT merged commit 89b4179 into sodafoundation:master Mar 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add NAS resources for filesystem, qtree & share #491

Add NAS resources for filesystem, qtree & share #491

joseph-v commented Feb 17, 2021 •

edited

Loading

codecov bot commented Feb 18, 2021 •

edited

Loading

NajmudheenCT Feb 26, 2021

joseph-v Mar 1, 2021

kumarashit Mar 9, 2021

NajmudheenCT Feb 26, 2021

joseph-v Mar 1, 2021

NajmudheenCT Feb 26, 2021

kumarashit Mar 9, 2021

joseph-v Mar 10, 2021

kumarashit Mar 9, 2021

wisererik left a comment

wisererik Mar 11, 2021

wisererik Mar 11, 2021

wisererik Mar 11, 2021

wisererik Mar 11, 2021

wisererik Mar 11, 2021

wisererik Mar 11, 2021

wisererik Mar 11, 2021

wisererik Mar 11, 2021

wisererik Mar 12, 2021

wisererik left a comment

NajmudheenCT left a comment



		@six.add_metaclass(abc.ABCMeta)
		class NASDriver(StorageDriver):



		class StorageDriverAPINotImplemented(DelfinException):
		msg_fmt = _("Storage driver do not support API: {0}.")

Add NAS resources for filesystem, qtree & share #491

Add NAS resources for filesystem, qtree & share #491

Conversation

joseph-v commented Feb 17, 2021 • edited Loading

REST API tests:

1. Filesystem

2. Qtree

3. Share

codecov bot commented Feb 18, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wisererik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wisererik left a comment

Choose a reason for hiding this comment

NajmudheenCT left a comment

Choose a reason for hiding this comment

joseph-v commented Feb 17, 2021 •

edited

Loading

codecov bot commented Feb 18, 2021 •

edited

Loading