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

EMC vnx block adds collect performance interface #906

Merged
merged 26 commits into from
Jun 28, 2022

Conversation

yuanyu-ghca
Copy link
Contributor

What this PR does / why we need it:
1 adds collect performance 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:

@codecov
Copy link

codecov bot commented May 30, 2022

Codecov Report

Merging #906 (0b5eaf1) into master (0e3e740) will decrease coverage by 0.34%.
The diff coverage is 50.27%.

@@            Coverage Diff             @@
##           master     #906      +/-   ##
==========================================
- Coverage   71.47%   71.12%   -0.35%     
==========================================
  Files         182      182              
  Lines       21532    21889     +357     
  Branches     3291     3346      +55     
==========================================
+ Hits        15389    15568     +179     
- Misses       5103     5268     +165     
- Partials     1040     1053      +13     
Impacted Files Coverage Δ
...fin/drivers/dell_emc/vnx/vnx_block/navi_handler.py 71.56% <35.44%> (-5.16%) ⬇️
...rivers/dell_emc/vnx/vnx_block/component_handler.py 64.07% <48.57%> (-11.95%) ⬇️
delfin/drivers/dell_emc/vnx/vnx_block/vnx_block.py 88.13% <88.88%> (+0.13%) ⬆️
delfin/drivers/dell_emc/vnx/vnx_block/consts.py 100.00% <100.00%> (ø)
delfin/drivers/fake_storage/__init__.py 94.41% <0.00%> (ø)

@@ -251,8 +256,10 @@ def list_disks(self, storage_id):
hot_spare = disk.get('hot_spare', '')
if hot_spare and hot_spare != 'N/A':
logical_type = constants.DiskLogicalType.HOTSPARE
disk_name = disk.get('disk_name')
disk_name = disk_name.replace(' Disk', ' Disk')
Copy link
Contributor

Choose a reason for hiding this comment

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

It is recommended to use this way: ' '.join(disk_name.strip().split()) .

Copy link
Contributor

Choose a reason for hiding this comment

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

Modified

metrics = []
archive_file_list = []
try:
if (end_time - start_time) < consts.EXEC_TIME_INTERVAL:
Copy link
Contributor

Choose a reason for hiding this comment

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

This if statement is redundant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modified


def collect_perf_metrics(self, storage_id, resource_metrics,
start_time, end_time):
metrics = []
Copy link
Contributor

Choose a reason for hiding this comment

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

It is recommended to add log info when starting this function, and record storage_id, start_time, and end_time in log info.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modified

start_time, end_time):
performance_lines_map = {}
try:
if not resources_map:
Copy link
Contributor

Choose a reason for hiding this comment

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

It's already judged outside of this function, so it's redundant here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modified

if not resources_map:
return performance_lines_map
tools = Tools()
for archive_file in (archive_file_list or []):
Copy link
Contributor

Choose a reason for hiding this comment

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

It's already judged outside of this function, or [] is redundant here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modified

self.navi_handler.get_local_file_path(),
archive_name_infos[0])
with open(file_path) as file:
f_csv = csv.reader(StringIO(file.read()))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need StringIO ? It's recommended to use simple statement csv.reader(file) .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modified

metric_value_infos = metric_value
if not consts.METRIC_MAP.get(resources_type):
continue
if not consts.METRIC_MAP.get(resources_type).get(metric_name):
Copy link
Contributor

Choose a reason for hiding this comment

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

These two if statements can be simplified as if not consts.METRIC_MAP.get(resources_type, {}).get(metric_name): continue .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modified

collection_timestamp, consts.COLLECTION_TIME_PATTERN)
collection_timestamp = tools.time_str_to_timestamp(
collection_time_str, consts.COLLECTION_TIME_PATTERN)
if "iops" in metric_name.lower():
Copy link
Contributor

Choose a reason for hiding this comment

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

It's recommended to use == , not in .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modified

tools = Tools()
for metric_name in (metric_list or []):
values = {}
obj_labels = copy.deepcopy(labels)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use copy.copy() , not copy.deepcopy().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modified

return metrics
metrics = self.create_metrics(storage_id, resource_metrics,
resources_map, resources_type_map,
performance_lines_map)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add log info to record storage_id, start_time, end_time, and the length of metrics when collect successfully.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modified

storage_id)
if num > consts.EXEC_MAX_NUM:
latest_time = file_latest_time
LOG.warn("Storage:{},Exit after {} executions.".format(
Copy link
Contributor

Choose a reason for hiding this comment

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

Please replace Chinese symbols with English symbols.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modified

Copy link
Collaborator

@wisererik wisererik 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

@joseph-v joseph-v left a comment

Choose a reason for hiding this comment

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

LGTM

@joseph-v joseph-v merged commit 1422a00 into sodafoundation:master Jun 28, 2022
@yuanyu-ghca yuanyu-ghca deleted the emc_vnx_block_20220529 branch October 28, 2022 03:35
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.

5 participants