From eda664ddf6253ad440f764629aca80b158fd9cfb Mon Sep 17 00:00:00 2001 From: Stephen Cheng Date: Thu, 18 Jan 2024 16:16:18 +0800 Subject: [PATCH 1/2] CA-388295: Fix bugs from py2->py3 conversion in perfmon and hfx_filename Signed-off-by: Stephen Cheng --- scripts/hfx_filename | 16 ++---- scripts/import_file.py | 24 ++++++++ scripts/perfmon | 10 ++-- scripts/test_hfx_filename.py | 107 +++++++++++++++++++++++++++++++++++ scripts/test_perfmon.py | 26 +-------- scripts/test_static_vdis.py | 26 +-------- 6 files changed, 147 insertions(+), 62 deletions(-) create mode 100644 scripts/import_file.py create mode 100644 scripts/test_hfx_filename.py diff --git a/scripts/hfx_filename b/scripts/hfx_filename index e454eb299e5..0c008d9d824 100755 --- a/scripts/hfx_filename +++ b/scripts/hfx_filename @@ -28,18 +28,14 @@ def rpc(session_id, request): headers = [ "POST %s?session_id=%s HTTP/1.0" % (db_url, session_id), "Connection:close", - "content-length:%d" % (len(request)), + "content-length:%d" % (len(request.encode('utf-8'))), "" ] - #print "Sending HTTP request:" for h in headers: - s.send("%s\r\n" % h) - #print "%s\r\n" % h, - s.send(request) + s.send((h + "\r\n").encode('utf-8')) + s.send(request.encode('utf-8')) - result = s.recv(1024) - #print "Received HTTP response:" - #print result + result = s.recv(1024).decode('utf-8') if "200 OK" not in result: print("Expected an HTTP 200, got %s" % result, file=sys.stderr) return @@ -57,11 +53,11 @@ def rpc(session_id, request): def parse_string(txt): prefix = "success" if not txt.startswith(prefix): - raise "Unable to parse string response" + raise Exception("Unable to parse string response") txt = txt[len(prefix):] suffix = "" if not txt.endswith(suffix): - raise "Unable to parse string response" + raise Exception("Unable to parse string response") txt = txt[:len(txt)-len(suffix)] return txt diff --git a/scripts/import_file.py b/scripts/import_file.py new file mode 100644 index 00000000000..e6230601edc --- /dev/null +++ b/scripts/import_file.py @@ -0,0 +1,24 @@ +# Used for importing a non-".py" file as a module + +import sys +import os + +def import_from_file(module_name, file_path): + """Import a file as a module""" + # Only for python3, but CI has python2 pytest check, so add this line + if sys.version_info.major == 2: + return None + from importlib import machinery, util + loader = machinery.SourceFileLoader(module_name, file_path) + spec = util.spec_from_loader(module_name, loader) + assert spec + assert spec.loader + module = util.module_from_spec(spec) + # Probably a good idea to add manually imported module stored in sys.modules + sys.modules[module_name] = module + spec.loader.exec_module(module) + return module + +def get_module(module_name, file_path): + testdir = os.path.dirname(__file__) + return import_from_file(module_name, testdir + file_path) \ No newline at end of file diff --git a/scripts/perfmon b/scripts/perfmon index 1af3ce05772..e48a2431281 100644 --- a/scripts/perfmon +++ b/scripts/perfmon @@ -313,7 +313,7 @@ class RRDUpdates: print_debug("Calling http://localhost/rrd_updates?%s" % paramstr) sock = urllib.request.urlopen("http://localhost/rrd_updates?%s" % paramstr) - xmlsource = sock.read() + xmlsource = sock.read().decode('utf-8') sock.close() # Use sax rather than minidom and save Vvvast amounts of time and memory. @@ -380,7 +380,7 @@ def get_percent_mem_usage(ignored): memlist = memfd.readlines() memfd.close() memdict = [ m.split(':', 1) for m in memlist ] - memdict = dict([(k.strip(), float(re.search('\d+', v.strip()).group(0))) for (k,v) in memdict]) + memdict = dict([(k.strip(), float(re.search(r'\d+', v.strip()).group(0))) for (k,v) in memdict]) # We consider the sum of res memory and swap in use as the hard demand # of mem usage, it is bad if this number is beyond the physical mem, as # in such case swapping is obligatory rather than voluntary, hence @@ -1067,7 +1067,8 @@ def main(): vm_uuid_list = rrd_updates.get_uuid_list_by_objtype('vm') # Remove any monitors for VMs no longer listed in rrd_updates page - for uuid in vm_mon_lookup: + # We use .pop() inside the loop, use list(dict_var.keys()): + for uuid in list(vm_mon_lookup.keys()): if uuid not in vm_uuid_list: vm_mon_lookup.pop(uuid) @@ -1103,7 +1104,8 @@ def main(): print_debug("sr_uuid_list = %s" % sr_uuid_list) # Remove monitors for SRs no longer listed in the rrd_updates page - for uuid in sr_mon_lookup: + # We use .pop() inside the loop, use list(dict_var.keys()): + for uuid in list(sr_mon_lookup.keys()): if uuid not in sr_uuid_list: sr_mon_lookup.pop(uuid) # Create monitors for SRs that have just appeared in rrd_updates page diff --git a/scripts/test_hfx_filename.py b/scripts/test_hfx_filename.py new file mode 100644 index 00000000000..9bdbbaa75b4 --- /dev/null +++ b/scripts/test_hfx_filename.py @@ -0,0 +1,107 @@ +#!/usr/bin/env python3 +# -*- coding: utf-8 -*- +# +# unittest for hfx_filename + +import unittest +from mock import MagicMock, patch, call +import sys +from import_file import get_module + + +# mock modules to avoid dependencies +sys.modules["XenAPI"] = MagicMock() + +hfx_filename = get_module("hfx_filename", "/hfx_filename") + +@unittest.skipIf(sys.version_info < (3, 0), reason="requires python3") +@patch("socket.socket") +class TestRpc(unittest.TestCase): + + def test_rpc(self, mock_socket): + mock_connected_socket = MagicMock() + mock_socket.return_value = mock_connected_socket + + recv_data = b"HTTP/1.1 200 OK\r\nContent-Length: 10\r\n\r\nHelloWorld" + mock_connected_socket.recv.return_value = recv_data + + session_id = 0 + request = "socket request" + body = hfx_filename.rpc(session_id, request) + + # Assert that the socket methods were called as expected + expected_data = [ + b"POST /remote_db_access?session_id=0 HTTP/1.0\r\n", + b"Connection:close\r\n", + b"content-length:14\r\n", + b"\r\n", + b"socket request" + ] + mock_connected_socket.send.assert_has_calls([call(data) for data in expected_data]) + + expected_return = "HelloWorld" + self.assertEqual(expected_return, body) + + def test_rpc_international_character(self, mock_socket): + mock_connected_socket = MagicMock() + mock_socket.return_value = mock_connected_socket + + recv_data = b"HTTP/1.1 200 OK\r\nContent-Length: 10\r\n\r\nHelloWorld" + mock_connected_socket.recv.return_value = recv_data + + session_id = 0 + # Use international character"socket 请求" as request + # Not using literal string is for passing python2 check + request = "socket 请求" + body = hfx_filename.rpc(session_id, request) + + # Assert that the socket methods were called as expected + expected_data = [ + b"POST /remote_db_access?session_id=0 HTTP/1.0\r\n", + b"Connection:close\r\n", + b"content-length:13\r\n", + b"\r\n", + request.encode('utf-8') + ] + mock_connected_socket.send.assert_has_calls([call(data) for data in expected_data]) + + expected_return = "HelloWorld" + self.assertEqual(expected_return, body) + + def test_db_get_uuid(self, mock_socket): + mock_connected_socket = MagicMock() + mock_socket.return_value = mock_connected_socket + + header = "HTTP/1.1 200 OK\r\nContent-Length: 10\r\n\r\n" + body = "successHelloWorld" + recv_data = (header + body).encode('utf-8') + mock_connected_socket.recv.return_value = recv_data + + expected_response = "HelloWorld" + response = hfx_filename.db_get_by_uuid(0, "pool_patch", "22345") + self.assertEqual(expected_response, response) + + def test_read_field(self, mock_socket): + mock_connected_socket = MagicMock() + mock_socket.return_value = mock_connected_socket + + header = "HTTP/1.1 200 OK\r\nContent-Length: 10\r\n\r\n" + body = "successfile_name" + recv_data = (header + body).encode('utf-8') + mock_connected_socket.recv.return_value = recv_data + + expected_filename = "file_name" + filename = hfx_filename.read_field(0, "pool_patch", "filename", "rf") + self.assertEqual(expected_filename, filename) + + +@unittest.skipIf(sys.version_info < (3, 0), reason="requires python3") +class TestParse(unittest.TestCase): + + def test_parse_string(self): + txt = "successabcde" + expected_txt = "abcde" + return_txt = hfx_filename.parse_string(txt) + self.assertEqual(expected_txt, return_txt) + + \ No newline at end of file diff --git a/scripts/test_perfmon.py b/scripts/test_perfmon.py index d0eba5c67b6..ec13949083f 100644 --- a/scripts/test_perfmon.py +++ b/scripts/test_perfmon.py @@ -5,35 +5,13 @@ import unittest from mock import MagicMock, patch import sys -import os -import subprocess import math +from import_file import get_module # mock modules to avoid dependencies sys.modules["XenAPI"] = MagicMock() -def import_from_file(module_name, file_path): - """Import a file as a module""" - if sys.version_info.major == 2: - return None - else: - from importlib import machinery, util - loader = machinery.SourceFileLoader(module_name, file_path) - spec = util.spec_from_loader(module_name, loader) - assert spec - assert spec.loader - module = util.module_from_spec(spec) - # Probably a good idea to add manually imported module stored in sys.modules - sys.modules[module_name] = module - spec.loader.exec_module(module) - return module - -def get_module(): - """Import the perfmon script as a module for executing unit tests on functions""" - testdir = os.path.dirname(__file__) - return import_from_file("perfmon", testdir + "/perfmon") - -perfmon = get_module() +perfmon = get_module("perfmon", "/perfmon") @unittest.skipIf(sys.version_info < (3, 0), reason="requires python3") @patch("subprocess.getoutput") class TestGetPercentage(unittest.TestCase): diff --git a/scripts/test_static_vdis.py b/scripts/test_static_vdis.py index b0ab6ad5939..374511a569b 100644 --- a/scripts/test_static_vdis.py +++ b/scripts/test_static_vdis.py @@ -5,36 +5,14 @@ import unittest from mock import MagicMock import sys -import os -import subprocess import tempfile +from import_file import get_module # mock modules to avoid dependencies sys.modules["XenAPI"] = MagicMock() sys.modules["inventory"] = MagicMock() -def import_from_file(module_name, file_path): - """Import a file as a module""" - if sys.version_info.major == 2: - return None - else: - from importlib import machinery, util - loader = machinery.SourceFileLoader(module_name, file_path) - spec = util.spec_from_loader(module_name, loader) - assert spec - assert spec.loader - module = util.module_from_spec(spec) - # Probably a good idea to add manually imported module stored in sys.modules - sys.modules[module_name] = module - spec.loader.exec_module(module) - return module - -def get_module(): - """Import the static-vdis script as a module for executing unit tests on functions""" - testdir = os.path.dirname(__file__) - return import_from_file("static_vdis", testdir + "/static-vdis") - -static_vdis = get_module() +static_vdis = get_module("static_vdis", "/static-vdis") @unittest.skipIf(sys.version_info < (3, 0), reason="requires python3") class TestReadWriteFile(unittest.TestCase): From d89d4054bc676cdd2769d932ef2804b785506d0e Mon Sep 17 00:00:00 2001 From: Bernhard Kaindl Date: Tue, 30 Jan 2024 18:23:55 +0000 Subject: [PATCH 2/2] Remove hfx_filename and perfmon from expected_to_fail --- .github/workflows/main.yml | 81 +++++++++++++++++++++++++++++ .pylintrc | 75 ++++++++++++++++++++++++++ .sourcery.yaml | 5 ++ ocaml/xenopsd/scripts/swtpm-wrapper | 2 +- pyproject.toml | 19 +++++-- scripts/hfx_filename | 1 + scripts/perfmon | 21 ++++---- 7 files changed, 188 insertions(+), 16 deletions(-) create mode 100644 .pylintrc create mode 100644 .sourcery.yaml diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 55d9f0abd7a..ea3232903ac 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -194,3 +194,84 @@ jobs: github_token: ${{ secrets.github_token }} reporter: github-pr-review level: error + + # uses: tsuyoshicho/action-mypy@v3.13.0 + - uses: bernhardkaindl/action-mypy@36cb3a857d01c1bdaa2811893106c71580132d71 + env: + MYPYPATH: scripts/examples/python:scripts/examples:scripts/plugins:scripts:. + with: + filter_mode: nofilter + setup_method: install + setup_command: pip install mypy mock + mypy_flags: | + --scripts-are-modules + --check-untyped-defs + --allow-redefinition + --hide-error-context + --ignore-missing-imports + --no-pretty + --warn-unreachable + --disable-error-code attr-defined + --disable-error-code call-overload + --disable-error-code import-not-found + --disable-error-code import-untyped + --disable-error-code type-arg + --disable-error-code var-annotated + --exclude ocaml/events/event_listen.py + --exclude ocaml/idl + --exclude ocaml/message-switch + --exclude ocaml/tests/tests/looper.py + --exclude ocaml/xcp-rrdd/scripts/rrdd/rrdd.py + --exclude scripts/examples + --exclude scripts/examples/python/monitor-unwanted-domains.py + --exclude scripts/scalability-tests/ping-master.py + --exclude scripts/backup-sr-metadata.py + --exclude scripts/restore-sr-metadata.py + --exclude scripts/nbd_client_manager.py + target: | + scripts/perfmon + scripts/hfx_filename + scripts/host-display + github_token: ${{ secrets.github_token }} + reporter: github-pr-review + + - uses: dciborow/action-pylint@0.1.1 + with: + filter_mode: nofilter + glob_pattern: scripts/examples/python/XenAPIPlugin.py + pylint_args: | + --verbose + --dsiable attribute-defined-outside-init + --disable bad-indentation + --disable consider-using-dict-comprehension + --disable consider-using-f-string + --disable consider-using-in + --disable consider-using-with + --disable global-statement + --disable import-error + --disable import-outside-toplevel + --disable invalid-name + --disable missing-final-newline + --disable missing-class-docstring + --disable missing-function-docstring + --dsiable missing-module-docstring + --disable multiple-imports + --disable multiple-statements + --disable no-else-return + --disable protected-access + --disable raise-missing-from + --disable redefined-outer-name + --disable too-few-public-methods + --disable too-many-branches + --disable too-many-statements + --disable trailing-whitespace + --disable try-except-raise + --disable use-dict-literal + --disable wrong-import-order + --max-line-length 112 + scripts/perfmon + scripts/hfx_filename + scripts/test_perfmon.py + scripts/test_hfx_filename.py + github_token: ${{ secrets.github_token }} + reporter: github-pr-review diff --git a/.pylintrc b/.pylintrc new file mode 100644 index 00000000000..d2709fd065f --- /dev/null +++ b/.pylintrc @@ -0,0 +1,75 @@ +[MAIN] + +# Use multiple processes to speed up Pylint. Specifying 0 will auto-detect the +# number of processors available to use, and will cap the count on Windows to +# avoid hangs. +jobs=2 + +# Pickle collected data for later comparisons. +persistent=yes + +# Discover python modules and packages in the file system subtree. +recursive=yes + +# Add paths to the list of the source roots. Supports globbing patterns. The +# source root is an absolute path or a path relative to the current working +# directory used to determine a package namespace for modules located under the +# source root. +source-roots= + ocaml/xapi-storage/python, + scripts, + scripts/examples/python, + +# When enabled, pylint would attempt to guess common misconfiguration and emit +# user-friendly hints instead of false-positive error messages. +suggestion-mode=yes + +# In verbose mode, extra non-checker-related info will be displayed. +verbose=yes + +[MESSAGES CONTROL] + +# Disable the message, report, category or checker with the given id(s). You +# can either give multiple identifiers separated by comma (,) or put this +# option multiple times (only on the command line, not in the configuration +# file where it should appear only once). You can also use "--disable=all" to +# disable everything first and then re-enable specific checks. For example, if +# you want to run only the similarities checker, you can use "--disable=all +# --enable=similarities". If you want to run only the classes checker, but have +# no Warning level messages displayed, use "--disable=all --enable=classes +# --disable=W". +disable= + bad-option-value, # old pylint for py2: ignore newer (unknown) pylint options + bad-continuation, # old pylint warns about some modern black formatting + bad-indentation, + consider-using-dict-comprehension, + consider-using-f-string, + consider-using-with, + import-error, + import-outside-toplevel, + invalid-name, + missing-final-newline, + missing-function-docstring, + multiple-imports, + redefined-outer-name, + trailing-whitespace, # enable this soon, after citical PRs are merged + useless-option-value, # new pylint has abaondoned these old options + +[FORMAT] + +# Maximum number of characters on a single line. +max-line-length=98 + +[BASIC] + +# Good variable names which should always be accepted, separated by a comma. +good-names=a, + b, + c, + f, + i, + j, + k, + ex, + Run, + _ diff --git a/.sourcery.yaml b/.sourcery.yaml new file mode 100644 index 00000000000..48aa95da5a0 --- /dev/null +++ b/.sourcery.yaml @@ -0,0 +1,5 @@ +rule_settings: + disable: + - replace-interpolation-with-fstring + - use-fstring-for-concatenation + - use-fstring-for-formatting diff --git a/ocaml/xenopsd/scripts/swtpm-wrapper b/ocaml/xenopsd/scripts/swtpm-wrapper index dfb322e6453..67d4ec7ebf3 100755 --- a/ocaml/xenopsd/scripts/swtpm-wrapper +++ b/ocaml/xenopsd/scripts/swtpm-wrapper @@ -87,7 +87,7 @@ def check_state_needs_init(fname): return False # Check if block device has non-zero header - with open(fname, "r") as file: + with open(fname, "rb") as file: hdr = file.read(8) int_hdr = struct.unpack(" and maybe a few other minor updates: "scripts/hatests", "scripts/backup-sr-metadata.py", @@ -112,13 +122,12 @@ inputs = [ "scripts/xe-scsi-dev-map", "scripts/examples/python", "scripts/yum-plugins", - "scripts/*.py", - # To be added later, # when converted to Python3-compatible syntax: # "ocaml/message-switch/python", # "ocaml/idl/ocaml_backend/python", # "ocaml/xapi-storage/python", + "ocaml/xenopsd/python", ] disable = [ "import-error", # xenfsimage, xcp.bootloader. xcp.cmd diff --git a/scripts/hfx_filename b/scripts/hfx_filename index 0c008d9d824..3e94f87f6e3 100755 --- a/scripts/hfx_filename +++ b/scripts/hfx_filename @@ -51,6 +51,7 @@ def rpc(session_id, request): s.close() def parse_string(txt): + assert isinstance(txt, str) prefix = "success" if not txt.startswith(prefix): raise Exception("Unable to parse string response") diff --git a/scripts/perfmon b/scripts/perfmon index e48a2431281..95ffc65b0ba 100644 --- a/scripts/perfmon +++ b/scripts/perfmon @@ -481,8 +481,12 @@ class VariableState: """ def __init__(self): self.value = None + # pytype: disable=attribute-error + # pylint: disable-next=no-member self.timeof_last_alarm = time.time() - self.alarm_auto_inhibit_period + # pylint: disable-next=no-member self.trigger_down_counter = self.alarm_trigger_period + # pytype: enable=attribute-error class Variable(VariableConfig, VariableState): """ Variable() is used by ObjectMonitor to create one Variable object for each @@ -1233,25 +1237,22 @@ if __name__ == "__main__": # we caught a signal which we have already logged pass - except Exception as e: + except Exception as e: # pylint: disable=broad-exception-caught rc = 2 log_err("FATAL ERROR: perfmon will exit") log_err("Exception is of class %s" % e.__class__) ex = sys.exc_info() err = traceback.format_exception(*ex) - + # Python built-in Exception has args, # but XenAPI.Failure has details instead. Sigh. try: - errmsg = "\n".join([ str(x) for x in e.args ]) - # print the exception args nicely - log_err(errmsg) - except Exception as ignored: + log_err("\n".join([ str(x) for x in e.args ])) + except AttributeError: try: - errmsg = "\n".join([ str(x) for x in e.details ]) - # print the exception args nicely - log_err(errmsg) - except Exception as ignored: + assert isinstance(e, XenAPI.Failure) + log_err("\n".join([ str(x) for x in e.details ])) + except AttributeError: pass # now log the traceback to syslog