Fix storing direct fetched VM RRD in dump_xapi_rrds()

Signed-off-by: Bernhard Kaindl <[email protected]>

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

@@ -62,15 +62,25 @@ import xml
 from xml.etree import ElementTree
 from xml.etree.ElementTree import Element
 
-try:
-    import hashlib
-    def md5_new():
-        return hashlib.md5()
-except:
-    import md5
-    def md5_new():
-        return md5.new()
+import defusedxml.sax
+
+if sys.version_info.major == 2:  # pragma: no cover
+    from commands import getoutput  # pyright: ignore[reportMissingImports]
+    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 HTTPError, urlopen
+
+    from typing import TYPE_CHECKING
+
+    if TYPE_CHECKING:  # Used for type checking only:
+        from _typeshed import ReadableBuffer  # pylint: disable=unused-import
 
+    unicode_type = str
+
+# Fixed in 3.7: https://github.com/python/cpython/pull/12628
 # Monkey-patch zipfile's __del__ function to be less stupid
 #   Specifically, it calls close which further writes to the file, which
 #   fails with ENOSPC if the root filesystem is full
@@ -1096,7 +1106,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)
@@ -1376,7 +1386,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')
@@ -1386,27 +1406,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 = urllib.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 = urllib.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):
@@ -1848,7 +1879,7 @@ class TarOutput(ArchiveWithTarSubarchives):
         ti.size = s.st_size
         self.tf.addfile(ti, file(filename,'rb'))
 
-    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())
@@ -1892,7 +1923,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):