From 33b6d6e02bd595d3fad7eb08e30864cf1fd3ef16 Mon Sep 17 00:00:00 2001 From: Bernhard Kaindl Date: Wed, 21 Feb 2024 12:00:00 +0100 Subject: [PATCH] Fix storing direct fetched VM RRD in dump_xapi_rrds() Signed-off-by: Bernhard Kaindl --- .pre-commit-config.yaml | 7 +- Makefile | 2 +- requirements-dev.txt | 2 + tests/unit/test_dump_xapi_rrds.py | 160 ++++++++++++++++++++++++++++++ xen-bugtool | 65 +++++++++--- 5 files changed, 217 insertions(+), 19 deletions(-) create mode 100644 tests/unit/test_dump_xapi_rrds.py diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index f40ca88a..41fb24cc 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -100,7 +100,7 @@ repos: hooks: - id: mypy name: Run mypy to check e.g. that all expected arguments are passed to functions etc - additional_dependencies: [defusedxml, pytest, types-lxml] + additional_dependencies: [defusedxml, pytest, types-lxml, types-mock] - repo: https://github.com/pycqa/pylint @@ -130,9 +130,10 @@ repos: additional_dependencies: - coverage - defusedxml + - lxml + - mock - pytest-coverage - pytest-mock - - lxml - XenAPI @@ -164,7 +165,7 @@ repos: - id: pyright name: Run pyright to check the unit tests for any typing warnings (use for bugtool later) exclude: xen-bugtool - additional_dependencies: [defusedxml, pytest, lxml, XenAPI] + additional_dependencies: [defusedxml, pytest, lxml, mock, XenAPI] - repo: https://github.com/mattseymour/pre-commit-pytype diff --git a/Makefile b/Makefile index 91bfa432..7293c992 100644 --- a/Makefile +++ b/Makefile @@ -1,6 +1,6 @@ .RECIPEPREFIX=> -DARKER_OPTS = --isort -tpy36 --skip-string-normalization +DARKER_OPTS = --isort -tpy36 --skip-string-normalization -l88 darker-xen-bugtool: >@ pip install 'darker[isort]' >@ tmp=`mktemp` ;\ diff --git a/requirements-dev.txt b/requirements-dev.txt index 23821c98..a3405bf1 100644 --- a/requirements-dev.txt +++ b/requirements-dev.txt @@ -1,3 +1,5 @@ lxml +mock pytest-coverage pytest-mock +types-mock diff --git a/tests/unit/test_dump_xapi_rrds.py b/tests/unit/test_dump_xapi_rrds.py new file mode 100644 index 00000000..2fe0e467 --- /dev/null +++ b/tests/unit/test_dump_xapi_rrds.py @@ -0,0 +1,160 @@ +"""Test: test_dump_xapi_rrds""" + +import sys +from email.message import Message + +import pytest +from mock import MagicMock + +if sys.version_info.major == 2: # pragma: no cover + from urllib2 import HTTPError # type:ignore[import-not-found,attr-defined] +else: + from urllib.request import HTTPError + + +@pytest.fixture +def mock_session1(): + """Return a mock XenAPI session. + + xapi_local_session is mocked to return this session. + + The pool has 3 VMs, one of which is not resident and one template. + The test runs on the pool master. + + Because the session runs on the pool master, dump_xapi_rrds() will + fetch the RRDs of resident VMs on any host and of the pool master. + """ + session = MagicMock() + session._session = "id" + session.xenapi.login_with_password.return_value = "session_id" + + # Simulates that the pool has 3 VMs, one of them is a template: + session.xenapi.VM.get_all_records.return_value = { + "invalid VM: mock_urlopen will return 404 on opening the 1st URL": { + "uuid": "0", + "is_a_template": False, + "power_state": "Suspended", + "resident_on": "host0", + }, + "vm1": { + "uuid": "1", + "is_a_template": False, + "resident_on": "host1", + "power_state": "Running", + }, + "vm2": { + "uuid": "2", + "is_a_template": False, + "resident_on": "host2", + "power_state": "Suspended", + }, + "vm4": { + "uuid": "4", + "is_a_template": False, + "resident_on": "OpaqueRef:NULL", + }, + "template": {"is_a_template": True}, + } + # Simulate that the test runs on the pool master: + session.xenapi.pool.get_all_records.return_value = { + "pool1": {"master": "host1"}, + } + session.xenapi.session.get_this_host.return_value = "host1" + return session + + +@pytest.fixture +def mock_urlopen(): + """Return a mock urlopen() that returns different mock RRD data on each call""" + + mock_response = MagicMock() + mock_response.read.side_effect = [b"mock_rrd1", b"mock_rrd2", b"mock_rrd3"] + + mock_urlopen = MagicMock(return_value=mock_response) + + # Mock the urlopen() to return the mock_response with different data on each call + side_effect = [HTTPError("url", 404, "Not Found", Message(), None)] + + side_effect += [mock_response] * 3 + mock_urlopen.side_effect = side_effect + + # Mock the fileno() method to return 0 for use by select(), except for Python3, + # urlopen returns http.client.HTTPResponse, which doesn't have a fileno() method: + if sys.version_info.major == 2: # pragma: no cover + mock_urlopen.return_value.fileno.return_value = 0 + + return mock_urlopen + + +def assert_mock_session1(bugtool, mock_urlopen): + """Assert the results expected from the mock_session1() fixture. + + The pool has 3 VMs, one of which is not resident and one template. + + Because the session was run on the pool master, dump_xapi_rrds() is expected + fetch the RRDs of resident VMs on any host and of the pool master. + """ + # Expect to see 3 calls to urlopen, one for each VM and one for the host + assert mock_urlopen.return_value.read.call_count == 3 + + # Assert that the RRDs of both VM are fetched + mock_urlopen.assert_any_call( + "http://localhost/vm_rrd?session_id=id&uuid=1", + timeout=5, + ) + mock_urlopen.assert_any_call( + "http://localhost/vm_rrd?session_id=id&uuid=2", + timeout=5, + ) + + # Check the keys of the data dictionary + files = sorted(bugtool.data.keys()) + assert files == ["xapi_rrd-1.out", "xapi_rrd-2.out", "xapi_rrd-host"] + + # Check the cap values of the data dictionary + expected_caps = ["persistent-stats"] * 3 + assert [key["cap"] for key in bugtool.data.values()] == expected_caps + + # Call the func_output() functions and check the return values + values = [key["func"]("") for key in bugtool.data.values() if "func" in key] + assert sorted(values) == [b"mock_rrd1", b"mock_rrd2", b"mock_rrd3"] + + with open(bugtool.XEN_BUGTOOL_LOG, "r") as f: + log = f.read() + assert log == "Failed to fetch RRD for VM 0: HTTP Error 404: Not Found\n" + + # Clear the log file, this indicates to the isolated_bugtool fixture + # that we checked the log file contents to be what we expected. + with open(bugtool.XEN_BUGTOOL_LOG, "w") as f: + f.write("") + + +def run_dump_xapi_rrds(mocker, bugtool, mock_session, mock_urlopen): + """Run the bugtool function dump_xapi_rrds(entries) with the given mocks.""" + # Patch the urlopen, xapi_local_session and entries + mocker.patch("xen-bugtool.urlopen", side_effect=mock_urlopen) + mocker.patch("xen-bugtool.xapi_local_session", return_value=mock_session) + mocker.patch("xen-bugtool.entries", [bugtool.CAP_PERSISTENT_STATS]) + + # Run the function + bugtool.dump_xapi_rrds(bugtool.entries) + + # Check if host RRD is fetched + mock_urlopen.assert_any_call("http://localhost/host_rrd?session_id=id") + + # Check the calls to xapi_local_session + assert mock_session.xenapi.VM.get_all_records.call_count == 1 + assert mock_session.xenapi.pool.get_all_records.call_count == 1 + assert mock_session.xenapi.session.get_this_host.call_count == 1 + assert mock_session.xenapi.session.logout.call_count == 1 + + +def test_dump_xapi_rrds_master(mocker, isolated_bugtool, mock_session1, mock_urlopen): + """Test dump_xapi_rrds() on a pool master with 2 VMs in the pool. + + Test the bugtool function dump_xapi_rrds(entries) to perform as expected + with mock_session1 which simulates a pool with 2 VMs and a pool master. + """ + + run_dump_xapi_rrds(mocker, isolated_bugtool, mock_session1, mock_urlopen) + assert_mock_session1(isolated_bugtool, mock_urlopen) diff --git a/xen-bugtool b/xen-bugtool index 56e8f1a9..ee88d182 100755 --- a/xen-bugtool +++ b/xen-bugtool @@ -70,12 +70,13 @@ import defusedxml.sax if sys.version_info.major == 2: # pragma: no cover from commands import getoutput # pyright: ignore[reportMissingImports] - from urllib import urlopen # type:ignore[attr-defined] + from urllib2 import HTTPError, urlopen # type:ignore[attr-defined] unicode_type = unicode # pyright:ignore[reportUndefinedVariable] # pylint: disable=unicode-builtin else: from subprocess import getoutput - from urllib.request import urlopen + from urllib.request import HTTPError, urlopen + from typing import TYPE_CHECKING if TYPE_CHECKING: # Used for type checking only: @@ -483,6 +484,19 @@ ANSWER_YES_TO_ALL = False SILENT_MODE = False entries = None data = {} +"""The global data dictionary of xen-bugtool +It is filled by the various *_output functions, and then used to create the +inventory.xml and the tarball or zipfile. + +The keys are the filenames of the collected data, and the values are +dictionaries with the following keys: + - "cap": The capability of the data: one of the CAP_ constants + - "path": If created by file_output: The path to the file to collect + - "func": If created by func_output: A function that returns the file data + - "cmd_args": If crated by cmd_output: The command to return the file data + - "filter": An optional filter function to pass the file data through +""" + directory_specifications = {} dev_null = open('/dev/null', 'r+') @@ -1103,7 +1117,7 @@ exclude those logs from the archive. # tree_output(CAP_PAM, SAMBA_DATA_DIR) # Explictly skip samba data as it contains credentials tree_output(CAP_PAM, VAR_LOG_DIR + "samba") - func_output(CAP_PERSISTENT_STATS, 'xapi_rrd-host', dump_xapi_rrds) + dump_xapi_rrds(entries) cmd_output(CAP_PROCESS_LIST, [PS, 'wwwaxf', '-eo', 'pid,tty,stat,time,nice,psr,pcpu,pmem,nwchan,wchan:25,args'], label='process-tree') func_output(CAP_PROCESS_LIST, 'fd_usage', fd_usage) @@ -1380,7 +1394,17 @@ def dump_xapi_subprocess_info(cap): pp = pprint.PrettyPrinter(indent=4) return pp.pformat(result) -def dump_xapi_rrds(cap): + +def dump_xapi_rrds(requested_entries): + """ + Query xcp-rrdd to get the RRDs for all VMs and the host or pool master. + + Due to triggering memory leaks, it was disabled by default (unless -a is used) + in 2013, it and was superceeded by collecting the compressed rrd files instead. + It's capability for --entries=persistent-stats is also hidden from the user. + """ + if CAP_PERSISTENT_STATS not in requested_entries: + return socket.setdefaulttimeout(5) session = xapi_local_session() session.xenapi.login_with_password('', '', '', 'xenserver-status-report') @@ -1390,27 +1414,38 @@ def dump_xapi_rrds(cap): i_am_master = (this_host == pool['master']) for vm in session.xenapi.VM.get_all_records().values(): - if vm['is_a_template']: + # CA-376326: Skip templates and VMs that are not resident on a host: + if vm['is_a_template'] or vm.get("resident_on") == "OpaqueRef:NULL": continue if vm['resident_on'] == this_host or (i_am_master and vm['power_state'] in ['Suspended', 'Halted']): - rrd = urlopen("http://localhost/vm_rrd?session_id=%s&uuid=%s" % (session._session, vm["uuid"])) try: - i, _, _ = select([rrd], [], [], 5.0) - if len(i) == 1: - data['xapi_rrd-%s' % vm['uuid']] = {'cap': cap, - 'output': StringIOmtime(rrd.read())} + rrd = urlopen( + "http://localhost/vm_rrd?session_id=%s&uuid=%s" + % (session._session, vm["uuid"]), + timeout=5, + ) + except HTTPError as e: + log("Failed to fetch RRD for VM %s: %s" % (vm["uuid"], e)) + continue + try: + name = "xapi_rrd-%s.out" % vm['uuid'] + vm_rrds = rrd.read() + + def create_lambda(x=None): + return lambda _: x + + func_output(CAP_PERSISTENT_STATS, name, create_lambda(vm_rrds)) finally: rrd.close() - output = '' - rrd = urlopen("http://localhost/host_rrd?session_id=%s" % session._session) try: + rrd = urlopen("http://localhost/host_rrd?session_id=%s" % session._session) output = rrd.read() + func_output(CAP_PERSISTENT_STATS, 'xapi_rrd-host', lambda _: output) finally: rrd.close() session.xenapi.session.logout() - return output class XapiDBContentHandler(xml.sax.ContentHandler): @@ -1888,7 +1923,7 @@ class TarOutput(ArchiveWithTarSubarchives): self.tf.addfile(ti, buffered_reader) - def add_path_with_data(self, name, data): + def add_path_with_data(self, name, data): # type:(str, StringIOmtime) -> None ti = self._getTi(name) ti.mtime = data.mtime ti.size = len(data.getvalue()) @@ -1932,7 +1967,7 @@ class ZipOutput(ArchiveWithTarSubarchives): compress_type = zipfile.ZIP_DEFLATED self.zf.write(filename, name, compress_type) - def add_path_with_data(self, name, data): + def add_path_with_data(self, name, data): # type:(str, StringIOmtime) -> None self.zf.writestr(name, data.getvalue()) def close(self):