add hitachi vsp storage driver to community #388

jiangyutan · 2020-11-21T06:09:41Z

What this PR does / why we need it:
this PR is for hitachi vsp storage,it provide hitachi storage's information collect,such as system,pools,volumes,it support VSP F/G seriers,and VSP F/G 350,370,700,900 support get the alerts,other seriers do not support alerts For now.

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:

codecov · 2020-11-21T11:15:39Z

Codecov Report

Merging #388 (2f906a2) into v0.8.0-maint (068fcc8) will increase coverage by 0.12%.
The diff coverage is 67.10%.

@@               Coverage Diff                @@
##           v0.8.0-maint     #388      +/-   ##
================================================
+ Coverage         66.91%   67.04%   +0.12%     
================================================
  Files                90       93       +3     
  Lines              6399     6700     +301     
  Branches            748      784      +36     
================================================
+ Hits               4282     4492     +210     
- Misses             1872     1944      +72     
- Partials            245      264      +19

Impacted Files	Coverage Δ
delfin/drivers/hitachi/vsp/rest_handler.py	`55.97% <55.97%> (ø)`
delfin/drivers/hitachi/vsp/vsp_stor.py	`75.30% <75.30%> (ø)`
delfin/drivers/hitachi/vsp/consts.py	`100.00% <100.00%> (ø)`
delfin/ssl_utils.py	`55.31% <0.00%> (+4.25%)`	⬆️
delfin/drivers/utils/rest_client.py	`59.70% <0.00%> (+5.97%)`	⬆️
delfin/common/alert_util.py	`86.20% <0.00%> (+6.89%)`	⬆️

ThisIsClark · 2020-11-23T02:09:15Z

delfin/drivers/hds/vsp/rest_handler.py

@@ -0,0 +1,220 @@
+# Copyright 2020 The SODA Authors.
+# Copyright (c) 2016 Huawei Technologies Co., Ltd.


Remove huawei copyright

ThisIsClark · 2020-11-23T02:36:27Z

delfin/drivers/hds/vsp/vspstor.py

@@ -0,0 +1,266 @@
+# Copyright 2020 The SODA Authors.


File name should be lower case with underscore.

ThisIsClark · 2020-11-23T02:39:46Z

delfin/drivers/hds/vsp/rest_handler.py

+    HDSVSP_SESSION_ID = None
+    STORAGE_DEVICE_ID = None
+    DEVICE_MODEL = None
+    SERIALNUMBER = None


Are these constants? Should these be member variable?

when login or get_device_id everytime,they will be refresh

Yes, so I think it should be member variables and should not be upper case.

now changed them to member variables

ThisIsClark · 2020-11-23T02:42:52Z

delfin/drivers/hds/vsp/rest_handler.py

+                              .format(res.status_code, res.text))
+                    # if method is logout,return immediately
+                    if method == 'DELETE' and RestHandler.\
+                            REST_LOGOUT_URL in url:


Where is the definition of REST_LOGOUT_URL?

ThisIsClark · 2020-11-23T02:45:03Z

delfin/drivers/hds/vsp/rest_handler.py

+                elif res.status_code == 503:
+                    raise exception.InvalidResults(res.text)
+            else:
+                LOG.error('Rest exec failed')


Please give more details in this log about what rest command exec failed.

have changed

ThisIsClark · 2020-11-23T02:46:28Z

delfin/drivers/hds/vsp/rest_handler.py

+            raise exception.InvalidResults(err_msg)
+
+    def get_resinfo_call(self, url, data=None, method=None, resName=None):
+        rejson = None


Local variable name rejson is not correct.

have changed

ThisIsClark · 2020-11-23T02:52:04Z

delfin/drivers/hds/vsp/rest_handler.py

+    def logout(self):
+        try:
+            url = RestHandler.HDSVSP_LOGOUT_URL
+            if RestHandler.HDSVSP_SESSION_ID is not None:


If RestHandler.HDSVSP_SESSION_ID is none, the url is RestHandler.HDSVSP_LOGOUT_URL, then we call delete of this url, what would happen? All the sessions would be deleted?

ThisIsClark · 2020-11-23T02:55:55Z

delfin/drivers/hds/vsp/rest_handler.py

+    def get_specific_storage(self):
+        url = '%s%s' % \
+              (RestHandler.HDSVSP_COMM_URL, RestHandler.STORAGE_DEVICE_ID)
+        rejson = self.get_resinfo_call(url,


Local variable name should be imporved.
Please check all other local variable names.

ThisIsClark · 2020-11-23T02:56:38Z

delfin/drivers/hds/vsp/rest_handler.py

+            LOG.error("Get device id error: %s", six.text_type(e))
+            raise e
+
+    def get_specific_storage(self):


Should it be get_storage?

ThisIsClark · 2020-11-23T03:00:46Z

delfin/drivers/hds/vsp/vspstor.py

+        self.rest_client = RestClient(**kwargs)
+        self.rest_handler = rest_handler.RestHandler(self.rest_client)
+        self.rest_handler.get_device_id()
+        self.rest_handler.login()


It seems that everytime login() was called, get_device_id() need be called at first, so would it be better to put get_device_id() into login()?

get_device_id() need at the first time,becuase we need device_id,when the session overdue and login again,we do not need get device again

ThisIsClark · 2020-11-23T03:25:02Z

delfin/drivers/hds/vsp/rest_handler.py

+        rejson = None
+        res = self.call(url, data, method)
+        if res is not None:
+            if res.status_code == consts.SUCCESS_STATUS_CODES:


Why do we need to assign SUCCESS_STATUS_CODES to status_code manually?
res should have its own status_code.

ThisIsClark · 2020-11-23T03:31:58Z

delfin/drivers/hds/vsp/rest_handler.py

+
+                    if res is None:
+                        LOG.error('Login res is None')
+                        raise exception.InvalidResults('res is None')


What res is None? And it should be started with capital letter.

ThisIsClark · 2020-11-23T04:08:44Z

delfin/drivers/hds/vsp/consts.py

+ERROR_SESSION_INVALID_CODE = 403
+ERROR_SESSION_IS_BEING_USED_CODE = 409
+SUCCESS_STATUS_CODES = 200
+# block size


This comment seems to be meaningless.

SUCCESS_STATUS_CODES changed

ThisIsClark · 2020-11-23T04:10:34Z

delfin/drivers/hds/vsp/rest_handler.py

+
+    HDSVSP_AUTH_KEY = 'Authorization'
+
+    def __init__(self, restclient):


restclient should be rest_client.

ThisIsClark · 2020-11-23T06:40:57Z

delfin/drivers/hds/vsp/consts.py

+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+ERROR_CONNECT_TO_SERVER = -403


ERROR_CONNECT_TO_SERVER and LOGIN_SOCKET_TIMEOUT not used

ThisIsClark · 2020-11-23T07:05:55Z

delfin/drivers/hds/vsp/rest_handler.py

+            LOG.error(err_msg)
+            raise exception.InvalidResults(err_msg)
+
+    def get_resinfo_call(self, url, data=None, method=None, resName=None):


If this function is going to 'get' resource info, should the method always be 'GET'?
If the method always be 'GET', we do not need to set method as parameter.

not always 'GET'

For example?

I checked that,get_resinfo_call just is GET,have changed

ThisIsClark · 2020-11-23T07:10:27Z

delfin/drivers/hds/vsp/vspstor.py

+
+    def __init__(self, **kwargs):
+        super().__init__(**kwargs)
+        self.rest_client = RestClient(**kwargs)


One doubt.
HdsVspDriver have rest_client and rest_handler, and rest_handler has rest_client, so when should HdsVspDriver use rest_client and when use rest_handler? What the difference between rest_client and rest_handler?

rest_handler will use rest_client,and in HdsVspDriver, i init rest_client first and init rest_handler with it to rest_handler,it's a same rest_client

rest_client just do rest init, http request,rest_handler do bussiness

Yes, but for a storage driver, only one rest object is enough, two object for rest would confuse other developers.
If rest_handler already has rest_client, would it be better that not operate rest_client directly anymore?
Or we can make rest_handler a sub class of rest_client, so it can has all the method what rest_client has.

ThisIsClark · 2020-11-23T07:13:00Z

delfin/drivers/hds/vsp/vspstor.py

+            if pools_info is not None:
+                pools = pools_info.get('data')
+            else:
+                return pool_list


This branch meanings some error during get_all_pools()? Or some other means?
I'm concerning about that, if I got a empty list from list_storage_pools(), it means actually there is no pool or met some issue during querying pool info?
Same for list_volumes

yes,there may be met some issue during querying pool info,so i return a empty list

So when I get an empty list from list_storage_pools, how can I know whether it has no pool or it has pools but I met some issue when querying?

ibm storwize_svc add sshpool and fixed some issue (#381)

ThisIsClark · 2020-11-24T04:05:54Z

delfin/tests/unit/drivers/hitachi/vsp/test_hitachi_vspstor.py

+from delfin.drivers.hitachi.vsp.rest_handler import RestHandler
+from delfin.drivers.hitachi.vsp.vsp_stor import HitachiVspDriver
+from delfin.drivers.utils.rest_client import RestClient
+from requests import Session


Why move this import sentence to this line?

kumarashit · 2020-11-24T05:52:09Z

delfin/drivers/hitachi/vsp/rest_handler.py

+LOG = logging.getLogger(__name__)
+
+
+class RestHandler(RestClient):


Why this new class and why can't the existing res_client util be used/enhanced?

kumarashit · 2020-11-24T05:56:56Z

delfin/drivers/hitachi/vsp/vsp_stor.py

+            used_total = total_total - free_total
+        else:
+            free_total = 0
+            total_total = 0


can we think of some better variable name? like total_total -> total_capacity or total_raw_cap

kumarashit · 2020-11-24T05:58:51Z

delfin/drivers/hitachi/vsp/vsp_stor.py

+                    'description': 'Hitachi VSP Pool',
+                    'status': status,
+                    'storage_type': storage_type,
+                    'subscribed_capacity': int(total_cap),


How come total_cap and subscribed_Cap be same?

What happened to this comment?

kumarashit · 2020-11-24T06:00:31Z

delfin/drivers/hitachi/vsp/vsp_stor.py

+            volume_list = []
+            volumes = volumes_info.get('data')
+            for volume in volumes:
+                if volume.get('emulationType') == 'NOT DEFINED':


if a volume doesn't have emulation type, where will be the capacity or storage be accounted?

in hitachi rest api of the volume,there always have emulationType

Then what is this 'NOT DEFINED?

kumarashit · 2020-11-24T06:03:19Z

delfin/drivers/hitachi/vsp/vsp_stor.py

+
+                total_cap = \
+                    int(volume.get('blockCapacity')) * consts.BLOCK_SIZE
+                used_cap = \


This is going to screw the storage report. Used_cap and total_cap is same so free_cap is 0 always.

Making free =0 may be true for a thick , because total_capacity=allocated_capacity (which is used from the perspective of array). but there should be a beeter way to update this for a thin volume. please check.

yes,i kown that,but there is no used_capacity or free_capacity

So for thin devices, won't the Storage Summary report be affected by this?

kumarashit · 2020-11-24T06:05:06Z

delfin/drivers/hitachi/vsp/vsp_stor.py

+
+        return alert_list
+
+    def add_trap_config(self, context, trap_config):


put a TODO if it has to be implemented later, else put comment

ThisIsClark · 2020-11-24T07:24:09Z

delfin/drivers/hitachi/vsp/consts.py

+ERROR_SESSION_IS_BEING_USED_CODE = 409
+BLOCK_SIZE = 512
+
+VSP_FXXX_GXXX_SERIES = ('VSP G350', 'VSP G370', 'VSP G700', 'VSP G900',


VSP_F_G_SERIES is better for me.

there also have like F200,G200 they are different

ThisIsClark · 2020-11-24T07:30:31Z

delfin/drivers/hitachi/vsp/vsp_stor.py

+        self.rest_handler.login()
+
+    def reset_connection(self, context, **kwargs):
+        self.rest_handler.logout()


verify should be updated before login

Verified accessing with the certificate?

delfin/drivers/hitachi/vsp/rest_handler.py

NajmudheenCT · 2020-11-24T13:34:03Z

delfin/drivers/hitachi/vsp/vsp_stor.py

+            'raw_capacity': int(total_capacity),
+            'total_capacity': int(total_capacity),
+            'used_capacity': int(total_capacity - free_capacity),
+            'free_capacity': int(free_capacity)


Does it provide subscribed capacity?

no, nit provide

NajmudheenCT · 2020-11-24T13:41:16Z

delfin/drivers/hitachi/vsp/vsp_stor.py

+
+                total_cap = \
+                    int(volume.get('blockCapacity')) * consts.BLOCK_SIZE
+                used_cap = \


Making free =0 may be true for a thick , because total_capacity=allocated_capacity (which is used from the perspective of array). but there should be a beeter way to update this for a thin volume. please check.

wisererik · 2020-11-25T01:57:51Z

delfin/drivers/hitachi/vsp/consts.py

+ERROR_SESSION_IS_BEING_USED_CODE = 409
+BLOCK_SIZE = 512
+
+VSP_F_G_350_370_700_900_SERIES = ('VSP G350', 'VSP G370', 'VSP G700',


change to SUPPORTED_VSP_SERIES

delfin/drivers/hitachi/vsp/rest_handler.py

NajmudheenCT

LGTM

kumarashit · 2020-11-25T18:27:27Z

I still see the comments not addressed. What is the plan for it?

kumarashit

Is it possible to address the comments before merging?

Add hitachi vsp driver

* Code improvement (#363) * ibm storwize_svc add sshpool and fixed some issue (#381) Add storage driver for IBM Storwize and svc series * add hitachi vsp storage driver to community (#388) Add hitachi vsp driver * Fix vsp driver issue * change pool status and some optimize for vsp (#395) * rest api change timeout and fix trap parse (#401) * fix ssh excption error when the port is wrong and do some optimize (#402) * Update the value of 'update_at' of storage when going to sync the storage (#425) * Code improvement (#363) * ibm storwize_svc add sshpool and fixed some issue (#381) Add storage driver for IBM Storwize and svc series * add hitachi vsp storage driver to community (#388) Add hitachi vsp driver * Fix vsp driver issue * change pool status and some optimize for vsp (#395) * rest api change timeout and fix trap parse (#401) * fix ssh excption error when the port is wrong and do some optimize (#402) * Update the value of 'update_at' of storage when going to sync the storage (#425) * Fix rebase issue * Fixing CI failure issues Co-authored-by: ThisIsClark <liuyuchibubao@gmail.com> Co-authored-by: jiangyutan <69443713+jiangyutan@users.noreply.github.com> Co-authored-by: Joseph Vazhappilly <josephvp@gmail.com> Co-authored-by: root <pravin ranjan>

jiangyutan and others added 17 commits November 2, 2020 15:16

edit

bf50dc4

过滤rest警告

770cc02

add vnx block driver

bc9b126

111

72ebc49

11

747387d

11

b3890d5

hds vsp

b6a4761

hitachi vsp do some optimize

2e63905

remove exception which for vnx

54a5fc6

do some optimize

5a920e4

remove vsp info in setup

b32ae70

add hitachi vsp in setup

147f953

do some optimize

6458e05

remove change from the rest_client

77b4e34

change decode

0a9cfbf

change the password in test

a7e2af0

change the password in test

f938324

optimize rest

5089595

ThisIsClark reviewed Nov 23, 2020

View reviewed changes

jiangyutan added 2 commits November 23, 2020 11:43

de some optimize

baf9c41

change for optimize

195a667

ThisIsClark reviewed Nov 23, 2020

View reviewed changes

some optimize for pr

7b49efb

ThisIsClark reviewed Nov 23, 2020

View reviewed changes

jiangyutan and others added 4 commits November 23, 2020 16:00

optimezi for pr

ff39e5e

Merge pull request #1 from sodafoundation/v0.8.0-maint

32b4000

ibm storwize_svc add sshpool and fixed some issue (#381)

Merge remote-tracking branch 'origin/v0.8.0-maint' into hds_vsp

4ec19dd

add vsp in setup and do some optimize

484d20d

add list alerts

56841dc

ThisIsClark reviewed Nov 24, 2020

View reviewed changes

jiangyutan added 3 commits November 24, 2020 13:24

change RestHandler inherit RestClient

5f2ede7

change RestHandler inherit RestClient

875af48

change RestHandler inherit RestClient

057fe20

kumarashit requested changes Nov 24, 2020

View reviewed changes

change test alert info

8595a5f

ThisIsClark reviewed Nov 24, 2020

View reviewed changes

jiangyutan added 6 commits November 24, 2020 15:37

add some comments and do some optimize

9cd4a91

fix line lenth longer than 79

c466e23

update verify before login

ca19b2e

update verify before login

6aa6d3a

change tuple name in consts

115776d

change tuple name in consts

be46db0

NajmudheenCT reviewed Nov 24, 2020

View reviewed changes

wisererik reviewed Nov 25, 2020

View reviewed changes

jiangyutan added 4 commits November 25, 2020 11:14

do some change for optimize

d86f599

do some change for optimize

582e3ee

add test info for rest call

7d9d634

do some change for optimize

2f906a2

NajmudheenCT approved these changes Nov 25, 2020

View reviewed changes

wisererik approved these changes Nov 25, 2020

View reviewed changes

wisererik requested a review from kumarashit November 25, 2020 07:25

wisererik merged commit ae88070 into sodafoundation:v0.8.0-maint Nov 25, 2020

kumarashit reviewed Nov 25, 2020

View reviewed changes

joseph-v pushed a commit to joseph-v/delfin that referenced this pull request Dec 23, 2020

add hitachi vsp storage driver to community (sodafoundation#388)

ac7b88e

Add hitachi vsp driver

joseph-v pushed a commit to joseph-v/delfin that referenced this pull request Dec 23, 2020

add hitachi vsp storage driver to community (sodafoundation#388)

64aa602

Add hitachi vsp driver

		@@ -0,0 +1,220 @@
		# Copyright 2020 The SODA Authors.
		# Copyright (c) 2016 Huawei Technologies Co., Ltd.


		HDSVSP_AUTH_KEY = 'Authorization'

		def __init__(self, restclient):

		LOG = logging.getLogger(__name__)


		class RestHandler(RestClient):


		return alert_list

		def add_trap_config(self, context, trap_config):

add hitachi vsp storage driver to community #388

add hitachi vsp storage driver to community #388

Conversation

jiangyutan commented Nov 21, 2020 • edited Loading

codecov bot commented Nov 21, 2020 • 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

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

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

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

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

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

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

jiangyutan commented Nov 21, 2020 •

edited

Loading

codecov bot commented Nov 21, 2020 •

edited

Loading