CP-47653: py2->py3 for perfmon #5527
Conversation
stephenchengCloud
commented
Mar 25, 2024
stephenchengCloud
commented
- Porting perfmon to Python3
- Added unit tests
- XenRT tests
- StorageAlerts: 3962464 passed (failed case due to an known issue)
- MemoryAlerts 3962465 passed
- alerts 3962466 passed
- user.log from alerts 3962466:
- user.log from MemoryAlerts 3962465:
- user.log from StorageAlerts 3962464:
- Manually test
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## feature/py3 #5527 +/- ##
=============================================
+ Coverage 85.0% 86.0% +0.9%
=============================================
Files 6 13 +7
Lines 556 2130 +1574
=============================================
+ Hits 473 1832 +1359
- Misses 83 298 +215
... and 5 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. |
Signed-off-by: Stephen Cheng <stephen.cheng@cloud.com>
Signed-off-by: Stephen Cheng <stephen.cheng@cloud.com>
Signed-off-by: Stephen Cheng <stephen.cheng@cloud.com>
Signed-off-by: Stephen Cheng <stephen.cheng@cloud.com>
Signed-off-by: Stephen Cheng <stephen.cheng@cloud.com>
Signed-off-by: Stephen Cheng <stephen.cheng@cloud.com>
Signed-off-by: Stephen Cheng <stephen.cheng@cloud.com>
Signed-off-by: Stephen Cheng <stephen.cheng@cloud.com>
Signed-off-by: Stephen Cheng <stephen.cheng@cloud.com>
Signed-off-by: Stephen Cheng <stephen.cheng@cloud.com>
Signed-off-by: Stephen Cheng <stephen.cheng@cloud.com>
Signed-off-by: Stephen Cheng <stephen.cheng@cloud.com>
Signed-off-by: Stephen Cheng <stephen.cheng@cloud.com>
There are some long functions in the code. So disable the below checks: too-many-locals / too-many-statements / too-many-return-statements Signed-off-by: Stephen Cheng <stephen.cheng@cloud.com>
use-dict-literal, dangerous-default-value, consider-using-dict-comprehension Signed-off-by: Stephen Cheng <stephen.cheng@cloud.com>
- In python3, socket.error, IOError are merged into OSError - ConnectionRefusedError is a subclass of OSError: - ConnectionRefusedError -> ConnectionError -> OSError - urllib.error.HTTPError is a subclass of OSError: - urllib.error.HTTPError <- urllib.error.URLError <- OSError - HTTPError doesn't have content in `args`. So we can't use `e.args[0]` Signed-off-by: Stephen Cheng <stephen.cheng@cloud.com>
Signed-off-by: Stephen Cheng <stephen.cheng@cloud.com>
Signed-off-by: Stephen Cheng <stephen.cheng@cloud.com>
Signed-off-by: Stephen Cheng <stephen.cheng@cloud.com>
Previously, for fixing the pylint, I used a specific exception for not getting data. But by testing, it didn't catch the index error. Not sure if there are any other exceptions. So just keep the original logic, use the general exception. Signed-off-by: Stephen Cheng <stephen.cheng@cloud.com>
…ly written as "in_t_tag." Signed-off-by: Stephen Cheng <stephen.cheng@cloud.com>
stephenchengCloud
force-pushed
the
private/stephenche/CP-47653
branch
from
a4a8143 to
a203a54
Compare
stephenchengCloud
force-pushed
the
private/stephenche/CP-47653
branch
3 times, most recently
from
cb721f9 to
95dbaa9
Compare
Signed-off-by: Stephen Cheng <stephen.cheng@cloud.com>
Signed-off-by: Stephen Cheng <stephen.cheng@cloud.com>
stephenchengCloud
force-pushed
the
private/stephenche/CP-47653
branch
from
95dbaa9 to
99391dd
Compare
Signed-off-by: Stephen Cheng <stephen.cheng@cloud.com>
python3/bin/perfmon
Outdated
| Get the percent usage of the host filesystem for logs partition. | ||
| Input list is ignored and should be empty | ||
| ''' | ||
| _ = ignored # unused: not sure if it'll be used later, passing pylint |
There was a problem hiding this comment.
Could it be just like the following? Will the pylint complain this?
def get_percent_log_fs_usage(_):
There was a problem hiding this comment.
I tried and the pylint didn't complain about this.
Will updated the code.
| return "<RRDUpdates object: params=%s>" % str(self.params) | ||
|
|
||
| def refresh(self, session, override_params={}): | ||
| def refresh(self, session, override_params=None): |
There was a problem hiding this comment.
May I know how does pylint complain override_params={} ?
The current change makes the type of override_params ambiguous.
Or, would it be better to do the following:
params = {}
if override_parameter is not None:
params = override_parameter
There was a problem hiding this comment.
This is a notorious problem with using mutable default values. The change in this PR is the best practice.
Briefly speaking, using mutable default values will cause problems where each call of the method will change the mutable value and will impact the other calls of the method.
See https://www.geeksforgeeks.org/use-mutable-default-value-as-an-argument-in-python/
| except ConnectionRefusedError as e: | ||
| # "Connection refused[111]" | ||
| # this happens when we try to restart session and *that* fails | ||
| time.sleep(2) |
There was a problem hiding this comment.
The original logic would cover other socket.error also.
Only the "Connection refused[111]" requires time.sleep(2).
There was a problem hiding this comment.
As I mentioned in the commit message, socket.error is now merged into OSError
The other exceptions of socket.error will be catched later in OSError.
There was a problem hiding this comment.
- In python3, socket.error, IOError are merged into OSError
- ConnectionRefusedError is a subclass of OSError:
- ConnectionRefusedError -> ConnectionError -> OSError
- urllib.error.HTTPError is a subclass of OSError:
- urllib.error.HTTPError -> urllib.error.URLError -> OSError
- HTTPError doesn't have content in
args. So we can't usee.args[0]
python3/bin/perfmon
Outdated
| timeout = rand(interval, interval + dither) | ||
| cmdsock.settimeout(timeout) | ||
| try: | ||
| cmd = cmdsock.recv(cmdmaxlen) |
There was a problem hiding this comment.
Would this be a bytes object? And does it need to be changed to string?
There was a problem hiding this comment.
It is indeed a bytes object.
Nice! Another bytes-str issue.
This code should've been covered by XenRT tests, but because of the issue you fixed days ago, the XenRT test failed.
Will modify it.
There was a problem hiding this comment.
In order to test this change, I think I should also add your fix to this PR
|
|
||
| try: | ||
| # Write out pidfile | ||
| with open(pidfile, "w", encoding="utf-8") as fd: |
There was a problem hiding this comment.
This kind of read-and-write pattern seems not correct.
- read from a file
- check
- write to the file
There was a problem hiding this comment.
Need to add a lock for the file operations.
As suggested, raise another ticket for this kind of refine work.
Please check CP-48628
| lookup of the other-config:perfmon xml of the SRs connected to a host""" | ||
| # `all_xmlconfigs` and `sruuids_by_hostuuid` are updated by clear() and update() | ||
| # pylint: disable=global-variable-not-assigned | ||
| global all_xmlconfigs |
There was a problem hiding this comment.
It would be great if these mutable global variables could be eliminated.
There was a problem hiding this comment.
Also tracked by CP-48628
| ) | ||
| # It's fine to use eval here | ||
| # pylint: disable=eval-used | ||
| self.consolidation_fn = eval(consolidation_fn) |
There was a problem hiding this comment.
Could this eval be get rid of through a map(string -> callable) ?
There was a problem hiding this comment.
Also tracked by CP-48628
Also add the fix for scripts/plugins/perfmon Signed-off-by: Stephen Cheng <stephen.cheng@cloud.com>
|
Code modified. |
|
All tests passd. |
pytype_reporter extracted 33 problem reports from pytype output. You can check the results of the job here |
4 participants
