Hpe 3par adds controller, disk, and port interfaces #579
Conversation
Hpe 3par adds controller, disk, and port interfaces
Codecov Report
@@ Coverage Diff @@
## master #579 +/- ##
==========================================
+ Coverage 71.59% 72.04% +0.44%
==========================================
Files 141 141
Lines 11267 11618 +351
Branches 1283 1352 +69
==========================================
+ Hits 8067 8370 +303
- Misses 2787 2807 +20
- Partials 413 441 +28
|
| raise exception.InvalidPrivateKey() | ||
| else: | ||
| raise exception.SSHException(err) | ||
| return result |
There was a problem hiding this comment.
This will create exception if 'result' is not defined (if condition on line 397 fails)
There was a problem hiding this comment.
Modified，In addition, when NetApp is integrated, 3PAR's SSH methods are also modified to use public methods to query
| LOG.error("Get %s info error: %s" % (command, six.text_type(e))) | ||
| if ckeck_excep: | ||
| raise e | ||
| return resources_info |
There was a problem hiding this comment.
Variable resources_info needs to be initialized
| rate = '' | ||
| if ports_connected_map: | ||
| rate = ports_connected_map.get(port_id, '') | ||
| if not ip_addr and ports_iscsi_map: |
There was a problem hiding this comment.
"not ip_addr" can be placed outside as it is neede in multiple cases
There was a problem hiding this comment.
Each map extracts different values, and when a value is found in one of the maps, there is no need to extract values from other maps. If you add NOT IP_ADDR to the outermost layer, it will look up each map once.
| 'NORMAL': constants.DiskStatus.NORMAL, | ||
| 'DEGRADED': constants.DiskStatus.ABNORMAL, | ||
| 'FAILED': constants.DiskStatus.ABNORMAL, | ||
| 'NEW': constants.DiskStatus.ABNORMAL |
There was a problem hiding this comment.
query: Should it not be NORMAL?
There was a problem hiding this comment.
Disk state does not have 'query' property, only contains' NORMAL, DEGRADED, FAILED, NEW '
There was a problem hiding this comment.
I meant It was query or doubt :), sorry, not written clearly
There was a problem hiding this comment.
'NORMAL': constants.DiskStatus.NORMAL,Others are classified as abnormal
| para_map=para_map) | ||
| except Exception as e: | ||
| LOG.error("Get %s info error: %s" % (command, six.text_type(e))) | ||
| if ckeck_excep: |
There was a problem hiding this comment.
ckeck_excep = check_excep
There was a problem hiding this comment.
Modified into 'throw_excep'
| LOG.error("Get %s info error: %s" % (command, six.text_type(e))) | ||
| if ckeck_excep: | ||
| raise e | ||
| return resources_info |
There was a problem hiding this comment.
Incase of ckeck_excep=false, in case of exception, empty lists will be returned like inventory_map
.Pls check if that is ok for all ckeck_excep=false cases. Normally we will just allow exception to pass on if it happens
There was a problem hiding this comment.
Only the main method will throw an exception if there is an exception. The query method of other individual properties does not need to throw an exception. If throwing an exception will affect the overall return result, such as the IP address of the port, it needs another method to find out. If there is an exception in the query, the exception will be thrown, which does not affect the return of other properties
There was a problem hiding this comment.
Got the scenarios, thanks
codecov.yml
Outdated
| default: # This can be anything, but it needs to exist as the name | ||
| # basic settings | ||
| target: auto | ||
| threshold: 5% |
There was a problem hiding this comment.
Can we set coverage threshold drop to 0% ?
There was a problem hiding this comment.
The current coverage rate is 71.25%, which is 70% according to the regulations. However, after modifying the code, the coverage rate of compilation check is reduced by 0.01%, so the coverage rate cannot pass. This problem can be avoided by configuring this parameter
Threshold: 5% means that when the coverage is greater than 70 and the change is less than 5%, it can be passed
| port_list.append(port_model) | ||
| return port_list | ||
|
|
||
| def analyse_speed(self, speed_value): |
There was a problem hiding this comment.
Can we change name to parse_speed ? analyse is confusinf name
| self.analyse_node_version, | ||
| throw_excep=False) | ||
|
|
||
| def analyse_node_version(self, resource_info, pattern_str, para_map=None): |
There was a problem hiding this comment.
can we name it parse_node_version ?
| fcoe_map[fcoe.get('n:s:p')] = fcoe | ||
| return fcoe_map | ||
|
|
||
| def analyse_datas_to_list(self, resource_info, pattern_str, para_map=None): |
| raise exception.InvalidResults(err_msg) | ||
| return obj_list | ||
|
|
||
| def analyse_datas_to_map(self, resource_info, pattern_str, para_map=None): |
| raise exception.InvalidResults(err_msg) | ||
| return obj_model | ||
|
|
||
| def analyse_disk_inventory(self, cols_size, titles_size, str_info, |
| obj_list.append(inventory_map) | ||
| return obj_list | ||
|
|
||
| def analyse_nodes(self, cols_size, titles_size, str_info, obj_list): |
|
LGTM |
What this PR does / why we need it:
1 add controller interface
2 add disk interface
3 add port interface
Which issue this PR fixes (optional, in fixes #(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #
Special notes for your reviewer:
Release note: