Add VMAX driver support for controller & port resources #663

joseph-v · 2021-07-29T04:49:31Z

What this PR does / why we need it:
Add VMAX driver support for controller & port resources

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

Special notes for your reviewer:

Release note:

LIST CONTROLLERS

LIST PORTS

codecov · 2021-07-29T04:54:28Z

Codecov Report

Merging #663 (4faf004) into master (16ea0bc) will decrease coverage by 0.06%.
The diff coverage is 55.63%.

@@            Coverage Diff             @@
##           master     #663      +/-   ##
==========================================
- Coverage   70.13%   70.07%   -0.07%     
==========================================
  Files         156      156              
  Lines       14434    14536     +102     
  Branches     1741     1764      +23     
==========================================
+ Hits        10124    10186      +62     
- Misses       3756     3778      +22     
- Partials      554      572      +18

Impacted Files	Coverage Δ
delfin/drivers/dell_emc/vmax/rest.py	`32.36% <30.55%> (-0.11%)`	⬇️
delfin/drivers/dell_emc/vmax/vmax.py	`84.31% <66.66%> (+3.92%)`	⬆️
delfin/drivers/dell_emc/vmax/client.py	`81.36% <80.95%> (+5.44%)`	⬆️
delfin/exception.py	`98.97% <100.00%> (+0.02%)`	⬆️

NajmudheenCT · 2021-07-30T05:12:35Z

delfin/drivers/dell_emc/vmax/client.py

+                    status = constants.ControllerStatus.OFFLINE
+
+                controller = {
+                    'name': 'director_'


I think we dont need to append 'director'.. lets pick director_id as name, if there is no name available

Thanks, Done.

NajmudheenCT · 2021-07-30T05:13:38Z

delfin/drivers/dell_emc/vmax/client.py

+                        'slot_' +
+                        str(director_info.get('director_slot_number')),
+                    'soft_version': None,
+                    'cpu_info': 'number_of_cores_'


expecting a model here.. having 'number_of_cores_' here does not looks good for me..

Changed to 'Cores-*'

NajmudheenCT · 2021-07-30T05:23:36Z

delfin/drivers/dell_emc/vmax/client.py

+                        'connection_status': connection_status,
+                        'health_status': constants.PortHealthStatus.NORMAL,
+                        'type': port_type,
+                        'logical_type': None,


Port logical type is required to distuinquish FA, RDF , etc

NajmudheenCT · 2021-08-03T09:25:15Z

delfin/drivers/dell_emc/vmax/client.py

+            directors = self.rest.get_director_list(self.array_id,
+                                                    self.uni_version)
+            switcher = {
+                'A': constants.PortLogicalType.MANAGEMENT,


Why is the switcher keys (A,B,C,D) . It has to be (FA, RF, SE ) etc right?

In VMAX director name, (Eg. FA-1D) last letter is for Slice. Slice A is for Infrastructure Manager system emulation. Slice B is for HYPERMAX OS Data Services, Slice C for Backend emulation, Slice D-H remaining emulation.

That means D-H slice can be anything (Front End (FA), RDF( RF).. So I might see a port number RF-1D also.. I still believe the the first 2 letters should be used.. below are my understanding.. though I dont see any document explain this..
[DF,DA,DX]-> backend
[RF.RA,RE]-> RDF replication
[FA, FE, EA, EF, SE]-> Front End
[IM] - Infrastrucre manager

This doc has some info https://psrathore244.wordpress.com/2020/09/18/vmax-3-emulation/

NajmudheenCT · 2021-08-04T04:18:51Z

delfin/drivers/dell_emc/vmax/client.py

+                        director_emulation, constants.PortLogicalType.OTHER)
+                    if logical_type == constants.PortLogicalType.OTHER:
+                        port_prefix = port_key['directorId'][:2]
+                        if port_prefix in ['FA', 'FE', 'EA', 'EF', 'SE']:


I guess it is safe to have one more comparison here to map backend directors [DF,DA,DX]-> backend. becasue D-H directors are universal.

NajmudheenCT

LGTM

ThisIsClark · 2021-08-05T02:12:08Z

delfin/drivers/dell_emc/vmax/client.py

+        except Exception as err:
+            msg = "Failed to get controller metrics from VMAX: {}".format(err)
+            LOG.error(msg)
+            raise exception.ControllerNotFound(self.array_id)


Is it correct to wrap all the exception except SSLCertificateFailed to ControllerNotFound?
For example, if user modify the password on the device but not modify the access info in delfin, it would throw ControllerNotFound here.
Same as other list API.

Thanks, updated to raise generated exception.

ThisIsClark · 2021-08-05T02:15:48Z

delfin/drivers/dell_emc/vmax/rest.py

+        resource_type = None
+        if args:
+            resource_type = args[2]
+        elif not args and kwargs:


When comes to this elif, it means that the first if in line 397 not match, so I think there is no need to add the condition not args here.

ThisIsClark · 2021-08-05T02:21:38Z

delfin/drivers/dell_emc/vmax/client.py

+            }
+            port_list = []
+            for director in directors:
+                port_keys = self.rest.get_port_list(


get_port_list would raise NotImplementedError when version < 84, I think we need to handle this exception in this function seperately, to raise NotImplementedError or just write a log without exception raised.
Now it will raise a PortNotFound exception.
Same as other API.

Modified to output error log

ThisIsClark

LGTM

NajmudheenCT

LGTM

joseph-v force-pushed the vmax-controller-port branch from 0d394f1 to a276148 Compare July 29, 2021 04:52

NajmudheenCT reviewed Jul 30, 2021

View reviewed changes

joseph-v force-pushed the vmax-controller-port branch from cf3e7d2 to 26d5e87 Compare August 2, 2021 10:42

NajmudheenCT reviewed Aug 3, 2021

View reviewed changes

joseph-v force-pushed the vmax-controller-port branch 4 times, most recently from 3843d8c to 877cb2f Compare August 4, 2021 02:36

NajmudheenCT reviewed Aug 4, 2021

View reviewed changes

joseph-v force-pushed the vmax-controller-port branch 4 times, most recently from a36d6b7 to 193bb93 Compare August 4, 2021 07:21

NajmudheenCT previously approved these changes Aug 4, 2021

View reviewed changes

joseph-v added 2 commits August 4, 2021 19:37

Add controller and port resources to VMAX driver

1606bc2

Add unit tests for controller and port resources

1aca6d8

joseph-v dismissed NajmudheenCT’s stale review via b85588b August 4, 2021 14:08

joseph-v force-pushed the vmax-controller-port branch from 193bb93 to b85588b Compare August 4, 2021 14:08

ThisIsClark reviewed Aug 5, 2021

View reviewed changes

joseph-v force-pushed the vmax-controller-port branch 2 times, most recently from b2c3293 to dc3ec7f Compare August 5, 2021 04:26

Implement review comments

4faf004

joseph-v force-pushed the vmax-controller-port branch from dc3ec7f to 4faf004 Compare August 5, 2021 04:38

ThisIsClark approved these changes Aug 5, 2021

View reviewed changes

NajmudheenCT approved these changes Aug 5, 2021

View reviewed changes

NajmudheenCT merged commit ee30ed8 into sodafoundation:master Aug 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add VMAX driver support for controller & port resources #663

Add VMAX driver support for controller & port resources #663

joseph-v commented Jul 29, 2021

codecov bot commented Jul 29, 2021 •

edited

Loading

NajmudheenCT Jul 30, 2021

joseph-v Aug 3, 2021

NajmudheenCT Jul 30, 2021

joseph-v Aug 3, 2021

NajmudheenCT Jul 30, 2021

joseph-v Aug 3, 2021

NajmudheenCT Aug 3, 2021

joseph-v Aug 3, 2021

NajmudheenCT Aug 3, 2021

NajmudheenCT Aug 4, 2021

NajmudheenCT left a comment

ThisIsClark Aug 5, 2021

joseph-v Aug 5, 2021

ThisIsClark Aug 5, 2021

joseph-v Aug 5, 2021

ThisIsClark Aug 5, 2021

joseph-v Aug 5, 2021

ThisIsClark left a comment

NajmudheenCT left a comment

Add VMAX driver support for controller & port resources #663

Add VMAX driver support for controller & port resources #663

Conversation

joseph-v commented Jul 29, 2021

codecov bot commented Jul 29, 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

Choose a reason for hiding this comment

NajmudheenCT 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

ThisIsClark left a comment

Choose a reason for hiding this comment

NajmudheenCT left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 29, 2021 •

edited

Loading