CA-388295: Fix bugs from py2->py3 conversion in perfmon and hfx_filename #5373

stephenchengCloud · 2024-01-19T00:27:27Z

Fix py2->py3 bugs in perfmon and hfx_filename
Added unit test for hfx_filename
XenRT test passed: 192908 Ring3 BST + BVT
Scanned by pytype manually

scripts/hfx_filename

scripts/test_hfx_filename.py

liulinC · 2024-01-22T13:22:23Z

@robhoes Given on xs8 we are doing stream release, maybe no patches will applied on xs8 stream, thus this script will not be used? (and no way to test it manually)

But it may still good to keep this script just in case we want to support hotfix again someday in future?

liulinC · 2024-01-22T16:19:30Z

@stephenchengCloud Another issue may not related to your update.

Regarding raise "Unable to parse string response" (and some others), both py2 and py3 gives me
TypeError: exceptions must be old-style classes or derived from BaseException, not str

stephenchengCloud · 2024-01-23T07:31:13Z

@stephenchengCloud Another issue may not related to your update.

Regarding raise "Unable to parse string response" (and some others), both py2 and py3 gives me TypeError: exceptions must be old-style classes or derived from BaseException, not str

This is the error handling of this script (see line 56): if the input is not started with <value><array><data><value>success</value><value> for parse_string(), this error will be raised.
You probably run this script with the wrong input. I've already mocked the right input and tested this function in the unit test.

As for the TypeError: exceptions must be old-style classes or derived from BaseException, not str, I also modified the raise usage to raise Exception.

stephenchengCloud · 2024-01-24T07:32:14Z

XenRT test passed: 193536 Ring3 BST + BVT

scripts/hfx_filename

scripts/import_file.py

scripts/test_hfx_filename.py

stephenchengCloud · 2024-01-29T01:39:20Z

Fixed Unsupported escape sequence issue.
Added comment on bug fix
Squash the commits

psafont · 2024-01-29T14:16:34Z

Waiting for bernhard to rereview the changes.

Is this passing tests that previously failed? Can we inspect the results?

stephenchengCloud · 2024-01-30T01:21:20Z

Waiting for bernhard to rereview the changes.

Is this passing tests that previously failed? Can we inspect the results?

@bernhardkaindl Could you review this? Thanks.

@psafont The tests previsously failed passed ( see job 3916062), but it has nothing to do with the perfmon script.
For the perfmon fix, I tested the fix (changed size during iteration) locally before I pushed the commit.
I will test the new build and check the user.log (The job is still running 3918881)

codecov · 2024-02-01T02:44:59Z

Codecov Report

Attention: 27 lines in your changes are missing coverage. Please review.

Comparison is base (e3602e1) 45.38% compared to head (2891c85) 46.60%.

Files	Patch %	Lines
scripts/perfmon	19.35%	25 Missing ⚠️
scripts/hfx_filename	66.66%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5373      +/-   ##
==========================================
+ Coverage   45.38%   46.60%   +1.22%     
==========================================
  Files          18       21       +3     
  Lines        2937     3038     +101     
==========================================
+ Hits         1333     1416      +83     
- Misses       1604     1622      +18

Flag	Coverage Δ
python2.7	`51.45% <36.14%> (-0.65%)`	⬇️
python3.11	`51.36% <76.66%> (+1.21%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

stephenchengCloud · 2024-02-01T03:34:19Z

Fixed the warnings by pylint and pytype
Some of the perfmon pytype warnings are false warnings:
Could you please check this? Thanks. @bernhardkaindl

 Warning: No attribute 'alarm_auto_inhibit_period' on VariableState
  Warning: No attribute 'alarm_trigger_period' on VariableState
  pytype_reporter: Progress: 20/28: check echo
  pytype_reporter: Progress: 21/28: check scripts.xc
  pytype_reporter: Progress: 22/28: check scripts.print-custom-templates
  pytype_reporter: Progress: 23/28: check renameif
  pytype_reporter: Progress: 24/28: check lvhd-api-test
  pytype_reporter: Progress: 25/28: check provision
  Warning: No attribute 'details' on Exception
pytype_reporter:   No Node.TEXT_NODE in module xml.dom.minidom, referenced from 'xml.dom.expatbuilder'
pytype_reporter:   No Node.TEXT_NODE in module xml.dom.minidom, referenced from 'xml.dom.expatbuilder'
pytype_reporter: Progress: 26/28: check scripts.xe-reset-networking
pytype_reporter: Progress: 27/28: check scripts.test_hfx_filename
pytype_reporter: Progress: 28/28: check scripts.yum-plugins.accesstoken
pytype for all files which are expected to pass
Error: Process completed with exit code 3.

The issue described in CA-388295 has been fixed. Tested many times.There are no RuntimeError: dictionary changed size during iteration in the user.log (Job 3918881/3919173/3919778/3920228/3921218)
During testing, found another issue, took some time to fix and test.
It's about the Exception handling differences between py2 and py3
Please check the fix in this commit a98ea40

Exception handling changes in python3:

IOError and socket.error now have been merged into OSError.
See
https://docs.python.org/3/library/exceptions.html#IOError
https://docs.python.org/3/library/socket.html#exceptions
HTTPError doesn't have content in args. So we can't use e.args[0]. That's where the issue occured.

Jan 30 08:00:49 fibreuk-01-04d-NIC1 /perfmon: PERFMON: File "/opt/xensource/bin/perfmon", line 1131, in main#012 if e.args[0] == 111:
Jan 30 08:00:49 fibreuk-01-04d-NIC1 /perfmon: PERFMON: IndexError: tuple index out of range

Difference exception classes:
The inheritance relationship among these classes is as follows:
urllib.error.HTTPError <- urllib.error.URLError <- OSError
see :https://docs.python.org/3/library/urllib.error.html
ConnectionRefusedError <- ConnectionError <- OSError
see https://docs.python.org/3/library/exceptions.html#ConnectionRefusedError

Test results
The exceptions were caught correctly.
Below are logs from both python 2 and python 3:

user.log (job: 3919173) with python2 version perfmon:

Line 11: Jan 30 10:24:46 fibreuk-01-01d /perfmon: PERFMON: caught socket.error: (2 No such file or directory) - restarting XAPI session
Line 60: Jan 30 11:28:16 fibreuk-01-01d /perfmon: PERFMON: Caught signal 15 - exiting
Line 72: Jan 30 11:31:43 fibreuk-01-01d-NIC1 perfmon: PERFMON: caught socket.error: (111 Connection refused) - restarting XAPI session
C:\Users\stephenche\Downloads\25437975-py2-perfmon\fibreuk-01-04d\user.log (8 hits)
Line 11: Jan 30 10:25:15 fibreuk-01-04d /perfmon: PERFMON: caught socket.error: (2 No such file or directory) - restarting XAPI session
Line 65: Jan 30 11:15:31 fibreuk-01-04d /perfmon: PERFMON: Caught signal 15 - exiting
Line 113: Jan 30 11:19:00 fibreuk-01-04d-NIC1 perfmon: PERFMON: caught socket.error: (111 Connection refused) - restarting XAPI session
Line 355: Jan 30 11:31:59 fibreuk-01-04d-NIC1 perfmon: PERFMON: caught IOError: (http error 401 Unauthorised content-length: 96#015#012content-type: text/html#015#012connection: close#015#012cache-control: no-cache, no-store#015#012WWW-Authenticate: Basic realm="rrd_updates"#15#012) - restarting XAPI session

user.log(job: 3921218) with python 3 version perfmon:

Line 15: Feb 1 00:59:00 cl14-01 /perfmon: PERFMON: caught socket.error: (2 No such file or directory) - restarting XAPI session
Line 72: Feb 1 01:16:11 cl14-01 /perfmon: PERFMON: Caught signal 15 - exiting
Line 126: Feb 1 01:20:11 cl14-01-NIC2 /perfmon: PERFMON: caught connection refused error: ([Errno 111] Connection refused) - restarting XAPI session
Line 360: Feb 1 01:33:38 cl14-01-NIC2 /perfmon: PERFMON: caught http.error: (HTTP Error 401: Unauthorised) - restarting XAPI session
Line 382: Feb 1 01:43:21 cl14-01-NIC2 /perfmon: PERFMON: Caught signal 15 - exiting
Line 434: Feb 1 01:47:15 cl14-01-NIC2 /perfmon: PERFMON: caught connection refused error: ([Errno 111] Connection refused) - restarting XAPI session
Line 678: Feb 1 02:00:32 cl14-01-NIC2 /perfmon: PERFMON: caught http.error: (HTTP Error 401: Unauthorised) - restarting XAPI session
Search "perfmon" (10 hits in 1 file of 1 searched)
C:\Users\stephenche\Downloads\3921218-py3-perfmon\cl14-03\user.log (10 hits)
Line 15: Feb 1 00:58:33 cl14-03 /perfmon: PERFMON: caught socket.error: (2 No such file or directory) - restarting XAPI session
Line 66: Feb 1 01:29:20 cl14-03 /perfmon: PERFMON: Caught signal 15 - exiting
Line 120: Feb 1 01:33:24 cl14-03-NIC2 /perfmon: PERFMON: caught connection refused error: ([Errno 111] Connection refused) - restarting XAPI session
Line 377: Feb 1 01:56:02 cl14-03-NIC2 /perfmon: PERFMON: Caught signal 15 - exiting
Line 393: Feb 1 02:00:14 cl14-03 /perfmon: PERFMON: caught connection refused error: ([Errno 111] Connection refused) - restarting XAPI session

Signed-off-by: Stephen Cheng <stephen.cheng@cloud.com>

- socket.error and IOError have been merged into OSError in python3 see: https://docs.python.org/3/library/exceptions.html#IOError - HTTPError doesn't have content in `args` in python3 - Different exception classes Signed-off-by: Stephen Cheng <stephen.cheng@cloud.com>

Signed-off-by: Stephen Cheng <stephen.cheng@cloud.com>

acefei · 2024-02-04T07:34:35Z

Fixed the warnings by pylint and pytype
Some of the perfmon pytype warnings are false warnings:
Could you please check this? Thanks. @bernhardkaindl
 Warning: No attribute 'alarm_auto_inhibit_period' on VariableState
  Warning: No attribute 'alarm_trigger_period' on VariableState

I think @bernhardkaindl has already shown you how to address this warning through his actions. see https://github.com/xapi-project/xen-api/pull/5418/files#diff-e0b8a152da73af5c15f34d1fd7200f74f77ef38c4197c77ac2abeb765e78f99aR249

edwintorok · 2024-02-05T10:17:51Z

Please change the target of the PR to the py3 feature branch.

liulinC · 2024-02-05T10:27:04Z

Please change the target of the PR to the py3 feature branch.

This is agreed with @psafont , bugfix goes to master, and the issues already there.
new py2->py3 should goes to feature branch for enough test, to ensure no py2->py3 issues raised.

edwintorok · 2024-02-05T10:46:58Z

scripts/hfx_filename

        s.close()

 def parse_string(txt):
+    assert txt is not None


Does this fix the original problem? It looks like 'rpc' may return on a non-200 error code, without raising an exception, and it'd instead fail on this assert here then.

This is not for the original problem.
The original change is small. Much of the code if for passing the pytype and pylint check added recently.
This is for passing the pytype.

edwintorok · 2024-02-05T10:49:42Z

scripts/perfmon

+            memtype = item[0].strip()
+            size = item[1].strip()
+            match = re.search(r'\d+', size.strip())
+            assert match


If there is no match this will raise an assertion error, it might be better to raise an error that gives you more information on what went wrong: in this case including the original size that we tried to match with the regex might be useful, so we see what it contains instead of digits.

This change is not covered by tests.
When installing and configuring the Codecov plugin for Chrome- or Firefox-based browsers, a red bar is shown in front of these lines. I recommend submitters to install and configure it (more info later, I'm in a meeting now) or check the code coverage on the Link given by the Codecov.io

We also need to run mypy checks

Since there are so many unresolved comments, and many changes, I think it's better to revert the Python3 changes as a fix, and move this to the Python3 branch instead to give this more time for review.

This is also for passing the pytype check.

Agree with @edwintorok.
I'm assuming your modification is inspired by @bernhardkaindl's draft PR (https://github.com/xapi-project/xen-api/pull/5365/files#r1458703889) to avoid pytype errors. However, it would be preferable to make more elegant adjustments to display insightful exceptions.

edwintorok · 2024-02-05T10:50:39Z

scripts/perfmon

+            # reset when reactivating
+            self.value = None
+            self.timeof_last_alarm = time.time() - self.alarm_auto_inhibit_period
+            self.trigger_down_counter = self.alarm_trigger_period


this duplicates code that previously wasn't duplicated.

Also for passing the pytype check.
The original code is a class, and pytype complained that the class has no alarm_auto_inhibit_period and alarm_trigger_period attributes.

bernhardkaindl · 2024-02-05T11:04:15Z

scripts/perfmon

+            memtype = item[0].strip()
+            size = item[1].strip()
+            match = re.search(r'\d+', size.strip())
+            assert match


This change is not covered by tests.
When installing and configuring the Codecov plugin for Chrome- or Firefox-based browsers, a red bar is shown in front of these lines. I recommend submitters to install and configure it (more info later, I'm in a meeting now) or check the code coverage on the Link given by the Codecov.io

We also need to run mypy checks

Since there are so many unresolved comments, and many changes, I think it's better to revert the Python3 changes as a fix, and move this to the Python3 branch instead to give this more time for review.

stephenchengCloud · 2024-02-05T11:21:19Z

OK. I will revert the python3 changes for hfx_filename and perfmon. @bernhardkaindl @psafont @edwintorok @liulinC @acefei

For this PR:

The original fix is small, just a few lines changed, and I tested the fix both by XenRT and manual test.
Most of the changes are Unit test / pylint / pytype fix.

liulinC · 2024-02-05T11:26:42Z

OK. I will revert the python3 changes for hfx_filename and perfmon. @bernhardkaindl @psafont @edwintorok @liulinC @acefei

For this PR:

The original fix is small, just a few lines changed, and I tested the fix both by XenRT and manual test.

Most of the changes are Unit test / pylint / pytype fix.

@psafont @edwintorok @bernhardkaindl @acefei @stephenchengCloud

I am very very very disappointed it is sooooooo hard to merge such a tiny bugfix.
The real code change is very small, it keep growing with pytype/pylint/unitest fixes, and finally end up with close.!
(Looks even more UNRELATED unittest is requesting)

duobei reviewed Jan 19, 2024

View reviewed changes

scripts/hfx_filename Outdated Show resolved Hide resolved

scripts/test_hfx_filename.py Outdated Show resolved Hide resolved

stephenchengCloud force-pushed the python3 branch 2 times, most recently from cfa03dc to c198405 Compare January 19, 2024 09:31

liulinC reviewed Jan 22, 2024

View reviewed changes

scripts/test_hfx_filename.py Outdated Show resolved Hide resolved

liulinC reviewed Jan 22, 2024

View reviewed changes

scripts/test_hfx_filename.py Show resolved Hide resolved

stephenchengCloud force-pushed the python3 branch from c198405 to 5e9d593 Compare January 23, 2024 08:05

freddy77 reviewed Jan 24, 2024

View reviewed changes

scripts/hfx_filename Outdated Show resolved Hide resolved

stephenchengCloud force-pushed the python3 branch 7 times, most recently from b06288f to bba8937 Compare January 25, 2024 03:20

liulinC approved these changes Jan 25, 2024

View reviewed changes

freddy77 reviewed Jan 25, 2024

View reviewed changes

scripts/import_file.py Outdated Show resolved Hide resolved

freddy77 reviewed Jan 25, 2024

View reviewed changes

scripts/import_file.py Outdated Show resolved Hide resolved

freddy77 reviewed Jan 25, 2024

View reviewed changes

scripts/import_file.py Show resolved Hide resolved

freddy77 reviewed Jan 25, 2024

View reviewed changes

scripts/test_hfx_filename.py Outdated Show resolved Hide resolved

stephenchengCloud force-pushed the python3 branch 2 times, most recently from 6dbccc6 to e942c81 Compare January 26, 2024 06:12

freddy77 approved these changes Jan 26, 2024

View reviewed changes

freddy77 self-requested a review January 26, 2024 10:40

freddy77 approved these changes Jan 26, 2024

View reviewed changes

liulinC requested a review from psafont January 26, 2024 14:54

bernhardkaindl self-assigned this Jan 28, 2024

stephenchengCloud force-pushed the python3 branch 2 times, most recently from f66094d to 32c368b Compare January 29, 2024 01:35

stephenchengCloud force-pushed the python3 branch from 32c368b to 06db91c Compare January 29, 2024 01:40

freddy77 approved these changes Jan 29, 2024

View reviewed changes

stephenchengCloud requested a review from bernhardkaindl January 30, 2024 01:39

freddy77 approved these changes Jan 31, 2024

View reviewed changes

stephenchengCloud force-pushed the python3 branch 3 times, most recently from b2edccb to 1bea778 Compare February 2, 2024 03:00

stephenchengCloud added 2 commits February 4, 2024 10:46

CA-388295: Fix bugs from py2->py3 conversion in perfmon and hfx_filename

87d0c6b

Signed-off-by: Stephen Cheng <stephen.cheng@cloud.com>

stephenchengCloud force-pushed the python3 branch from 1bea778 to 461716e Compare February 4, 2024 02:47

Fix pylint and pytype warnings

2891c85

Signed-off-by: Stephen Cheng <stephen.cheng@cloud.com>

stephenchengCloud force-pushed the python3 branch from 461716e to 2891c85 Compare February 4, 2024 07:39

edwintorok reviewed Feb 5, 2024

View reviewed changes

bernhardkaindl suggested changes Feb 5, 2024

View reviewed changes

stephenchengCloud closed this Feb 5, 2024

Uh oh!

CA-388295: Fix bugs from py2->py3 conversion in perfmon and hfx_filename #5373

CA-388295: Fix bugs from py2->py3 conversion in perfmon and hfx_filename #5373

Uh oh!

Conversation

stephenchengCloud commented Jan 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

liulinC commented Jan 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liulinC commented Jan 22, 2024

Uh oh!

stephenchengCloud commented Jan 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stephenchengCloud commented Jan 24, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stephenchengCloud commented Jan 29, 2024

Uh oh!

psafont commented Jan 29, 2024

Uh oh!

stephenchengCloud commented Jan 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Feb 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

stephenchengCloud commented Feb 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

acefei commented Feb 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

edwintorok commented Feb 5, 2024

Uh oh!

liulinC commented Feb 5, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephenchengCloud commented Feb 5, 2024

Uh oh!

liulinC commented Feb 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

stephenchengCloud commented Jan 19, 2024 •

edited

Loading

liulinC commented Jan 22, 2024 •

edited

Loading

stephenchengCloud commented Jan 23, 2024 •

edited

Loading

stephenchengCloud commented Jan 30, 2024 •

edited

Loading

codecov bot commented Feb 1, 2024 •

edited

Loading

stephenchengCloud commented Feb 1, 2024 •

edited

Loading

acefei commented Feb 4, 2024 •

edited

Loading

liulinC commented Feb 5, 2024 •

edited

Loading