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.

Sign up for GitHub

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

Suggesting some new methods #18

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
+146 −15
Open

Suggesting some new methods #18

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
238d284
Suggesting some new methods
Aug 23, 2023
bc79604
Show command property to avoid playing with cache responses
Aug 23, 2023
5c58834
Fix to return empty str just in case prop is missing
Aug 23, 2023
150ad2e
added dep humanfriendly
ralequi Aug 24, 2023
c87f581
mypy fixes
ralequi Aug 24, 2023
e81932e
Some logic format fixes and repr methods
Aug 24, 2023
12fc009
rebase
Aug 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ classifiers = [
'Programming Language :: Python',
'Development Status :: 3 - Alpha',
]
dependencies = []
dependencies = ['humanfriendly']
dynamic = ["version"]

[project.urls]
Expand All @@ -47,4 +47,11 @@ pystorcli-metrics = "pystorcli2.bin.metrics:main"

[project.optional-dependencies]
# Requirements only needed for development
dev = ['pytest', 'pytest-cov', 'coveralls', 'pdoc', 'mypy']
dev = [
'coveralls',
'mypy',
'pdoc',
'pytest-cov',
'pytest',
'types-humanfriendly',
]
100 changes: 92 additions & 8 deletions pystorcli2/controller/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,36 @@ def name(self):
"""
return self._name

@ property
def show(self):
"""Caching show output to allow getting info of static attributes
Return:
dict: controller data from show command
"""
if not getattr(self, '_show', None):
out = self._storcli.run(['show'], allow_error_codes=[
StorcliErrorCode.INCOMPLETE_FOREIGN_CONFIGURATION])
self._show = common.response_data(out)
return self._show

@ property
def serial(self):
""" (str|None): get serial number if exists
"""
return self.show.get('Serial Number')

@ property
def model(self):
""" (str|None): get model if exists
"""
return self.show.get('Model')

@ property
def pci_address(self):
""" (str|None): get pci address if exists
"""
return self.show.get('PCI Address')

@property
def facts(self):
""" (dict): raw controller facts
Expand Down Expand Up @@ -503,6 +533,48 @@ def import_foreign_configurations(self, securitykey: Optional[str] = None):
args.append(f'securitykey={securitykey}')
return common.response_cmd(self._run(args))

def set_controller_mode(self, mode):
"""Set controller mode command

Returns:
(dict): response cmd data
"""
args = [
'set',
f'personality={mode}'
]
return common.response_cmd(self._run(args))

def delete_vds(self):
"""Delete all virtual drives

Returns:
(dict): response cmd data
"""
args = [
'/vall',
'delete'
]
return common.response_cmd(self._run(args))

@property
def phyerrorcounters(self):
"""Get Phy Error Counters

Returns:
(dict): response cmd data
"""
args = [
'/pall',
'show'
]
try:
# RAID
return common.response_data(self._run(args + ['all']))['Phy Error Counters']
except exc.StorCliCmdError:
# HBA
return common.response_data(self._run(args + ['phyerrorcounters']))


class Controllers(object):
"""StorCLI Controllers
Expand All @@ -528,16 +600,28 @@ def __init__(self, binary='storcli64'):
self._binary = binary
self._storcli = StorCLI(binary)

@ property
def show(self) -> List[dict]:
"""Caching show output to allow getting info of static attributes
Return:
list: list of dicts of controllers data from show command
"""
if not getattr(self, '_show', None):
out = self._storcli.run(['show'], allow_error_codes=[
StorcliErrorCode.INCOMPLETE_FOREIGN_CONFIGURATION])
response = common.response_data(out)
if "Number of Controllers" in response and response["Number of Controllers"] == 0:
self._show = []
else:
self._show = common.response_data_subkey(
out, ['System Overview', 'IT System Overview'])
return self._show
Copy link
Member

Choose a reason for hiding this comment

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

What is exactly the point of this?
The cache is used to avoid duplicated calls, and this seems to be a custom cache over a "global" cache.
Don't see the point but another issue-maker to have multiple versions of the same "show".

Have you found an issue with the cache? Maybe that's what we must address instead of this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, i have a list of issues:)

  1. each time user should pay attention in code when to turn it on and off - code gets complicated

  2. if not storing some basic info somewhere, run logs become huge, since for each property the command is invoked from scratch. But many properties are static, will not change, so those calls can be saved.

  3. if using singleton, then user should address only one pystorcli object to enable/disable the global cache
    but if not using singleton, then user should first find the exact pystrocli object where he wants to enable/disable cache, and it can be multiple objects - for example all drives of a VD.

4.And specifically regarding singleton: suppose host has two raid/hba cards of different models that also use different cli versions (did not see it yet with broadcom but saw it with adaptecs), so user wants to have now two different storcli versions - now he can't use singleton option and global caching also becomes complicated.

So basically the global cache approach is not really needed, because for static properties they can be stored somewhere once and for all other properties which are dynamic - why would some one use caching for dynamic things, it will be a wrong code flow.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And this would be also relevant for #14
Would be great to dispose the singleton and global cache, the runner should simple be the same one which pysmart uses

Copy link
Member

Choose a reason for hiding this comment

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

I saw cache on pysmart... and I think it really improves performance without much coding effort.

In the other hand, I see your point. There are scenarios where singleton+global configs/cache can be harder than initially expected.
Maybe a cache-per-property may be much better (probably using custom decorator? and/or some external cache-lib decorator?)
I have to think about this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The difference with pysmart is that many objects can change dynamically - creating/deleting vds, switching modes, switching drive states, etc..
A cache-per-property would be great, that is sort of what i tried to do with a static show property.

In pyarcconf i did it differently - most of objects and properties are always being cached, but each object has an update method. So when doing an action which might change the object, after that it will call for its update. Sort of a cache per object.

Copy link
Member

Choose a reason for hiding this comment

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

I should check that !

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

tell me later if you are ok with the static show prop, maybe as a temp solution, because i want to also add a show_all prop for drive object, most of runs use a lot of the drive props

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

correction, in pyrcconf only the "show all" command is being cached, the update is refreshing the "show all", all the rest is fetched on the fly


@ property
def _ctl_ids(self) -> List[int]:
out = self._storcli.run(['show'], allow_error_codes=[
StorcliErrorCode.INCOMPLETE_FOREIGN_CONFIGURATION])
response = common.response_data(out)

if "Number of Controllers" in response and response["Number of Controllers"] == 0:
return []
else:
return [ctl['Ctl'] for ctl in common.response_data_subkey(out, ['System Overview', 'IT System Overview'])]
"""(list of str): controllers id
"""
return [ctl['Ctl'] for ctl in self.show]

@ property
def _ctls(self):
Expand Down
29 changes: 27 additions & 2 deletions pystorcli2/drive/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

'''StorCLI drive python module
'''
import humanfriendly

from .. import StorCLI
from .. import common
Expand Down Expand Up @@ -104,6 +105,13 @@ def _response_attributes(self, out):
self._ctl_id, self._encl_id, self._slot_id)
return common.response_data(out)[detailed_info][attr]

def __repr__(self):
"""Define a basic representation of the class object."""
return '<PD {} | {}>'.format(
self._name,
' '.join([self.serial, self.medium, self.state])
)

def _run(self, args, **kwargs):
args = args[:]
args.insert(0, self._name)
Expand Down Expand Up @@ -146,12 +154,29 @@ def metrics(self):

@property
def size(self):
"""(str): drive size
"""(str): drive size in bytes (pysmart compliance)
"""
args = [
'show'
]
size = self._response_properties(self._run(args))['Size']
return humanfriendly.parse_size(size)

@property
def capacity(self):
"""Size in human readable format (pysmart compliance)
"""
return humanfriendly.format_size(self.size)

@property
@common.upper
def block_size(self):
"""(str): block size 4KB / 512B
"""
args = [
'show'
]
return self._response_properties(self._run(args))['Size']
return self._response_properties(self._run(args))['SeSz'].replace(' ', '').strip()

@property
@common.upper
Expand Down
2 changes: 1 addition & 1 deletion pystorcli2/storcli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ class StorCLI(object):
binary (str): storcli binary or full path to the binary

Properties:
cache_enable (boolean): enable disable resposne cache (also setter)
cache_enable (boolean): enable disable response cache (also setter)
cache (dict): get / set raw cache content

Methods:
Expand Down
19 changes: 17 additions & 2 deletions pystorcli2/virtualdrive/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

'''StorCLI virtual virtual drive python module
'''
import humanfriendly

from .. import StorCLI
from .. import common
Expand Down Expand Up @@ -85,6 +86,13 @@ def __init__(self, ctl_id, vd_id, binary='storcli64'):

self._exist()

def __repr__(self):
"""Define a basic representation of the class object."""
return '<VD {} | {}>'.format(
self._name,
' '.join([self.raid, self.capacity, self.state])
)

def _run(self, args, **kwargs):
args = args[:]
args.insert(0, self._name)
Expand Down Expand Up @@ -143,12 +151,19 @@ def raid(self):

@property
def size(self):
"""(str): virtual drive size
"""(str): virtual drive size in bytes (pysmart compliance)
"""
args = [
'show'
]
return self._response_properties(self._run(args))['Size']
size = self._response_properties(self._run(args))['Size']
return humanfriendly.parse_size(size)

@property
def capacity(self):
"""Size in human readable format (pysmart compliance)
"""
return humanfriendly.format_size(self.size)

@property
def state(self) -> VDState:
Expand Down