Merge pull request #82 from xenserver-next/update-dump_xapi_rrds-to-t…

…he-new-world-order

CA-389135: Fix the off-by-default and hidden direct-fetched VM RRDs

.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

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`                                       ;\

requirements-dev.txt

@@ -1,3 +1,5 @@
 lxml
+mock
 pytest-coverage
 pytest-mock
+types-mock

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)

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+')
 
@@ -1118,7 +1132,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)
@@ -1395,7 +1409,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')
@@ -1405,27 +1429,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):
@@ -1903,7 +1938,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())
@@ -1947,7 +1982,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):