Repository: Azure/WALinuxAgent
Pull request #2011 (2020-09-18 23:51:57)

Add LIS version to OSInfo.message
## Description
This uses the Linux kernel’s `modinfo` command to retrieve the “version”
field for the “hv_vmbus” kernel module (the LIS drivers). This is the
documented method to retrieve the LIS version.

This command should not fail, given that it’s part of the Linux kernel.
Moreover, the existing RDMA code already relies on `modinfo` without a
timeout etc. It is also distribution-agnostic.

If for some reason the module’s version is unavailable, or `modinfo`
somehow doesn’t exist, we catch the exception and return “Unknown”
instead.

While a timeout would be nice, the existing `run_command` doesn’t
currently support it (and adding it will require some work because we
can’t use Python 3.3’s `timeout` due to Python 2.6 being our minimum
version). Again, there is already existing code which calls `modinfo`
without a timeout, so if this were to fail, it seems it would fail
regardless of this patch.

---

### PR information
- [x] The title of the PR is clear and informative.
- [x] There are a small number of commits, each of which has an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, [see this page](https://github.com/Azure/azure-powershell/blob/master/documentation/development-docs/cleaning-up-commits.md).
- [x] Except for special cases involving multiple contributors, the PR is started from a fork of the main repository, not a branch.
- [x] If applicable, the PR references the bug/issue that it fixes in the description.
- [x] New Unit tests were added for the changes made and Travis.CI is passing.

### Quality of Code and Contribution Guidelines
- [x] I have read the [contribution guidelines](https://github.com/Azure/WALinuxAgent/blob/master/.github/CONTRIBUTING.md).

andschwa: Good morning @narrieta, is there anything else needed here?
narrieta: @andschwa - Please add a comment specifying that this is just a temporary change and can be removed in a few releases
narrieta: code looks good; running automation
andschwa: You can just auto-rebase this on top of develop, the only new commit to develop was my fix for the readme. There's no need to merge develop back into this. (I believe I have "allow edits by maintainers" checked too, so you don't have to wait on me to do that.)
narrieta: Automation failed on several distros

    std_out:
    waagent.log contains the following ERROR log(s): 
    ['2020-09-23T19:44:13.176091Z ERROR ExtHandler ExtHandler Command: [modinfo -F version hv_vmbus], return code: [1], 
    stdout: [] stderr: [modinfo: ERROR: Module hv_vmbus not found.\n']

will contact @andschwa offline

andschwa: Hello @narrieta and @larohra, is there anything left for this?
larohra: > Hello @narrieta and @larohra, is there anything left for this?

@andschwa I'm just running a sanity test automation run, will merge once it passes.
andschwa: Thanks for the update @larohra!
larohra: DCR was successful

Patch:
diff --git a/azurelinuxagent/common/version.py b/azurelinuxagent/common/version.py
index 8c14ada2..d2498c08 100644
--- a/azurelinuxagent/common/version.py
+++ b/azurelinuxagent/common/version.py
@@ -21,6 +21,7 @@ import platform
 import sys
 
 import azurelinuxagent.common.conf as conf
+import azurelinuxagent.common.utils.shellutil as shellutil
 from azurelinuxagent.common.utils.flexible_version import FlexibleVersion
 from azurelinuxagent.common.future import ustr, get_linux_distribution
 
@@ -88,7 +89,7 @@ def get_distro():
         try:
             # dist() removed in Python 3.8
             osinfo = list(platform.dist()) + [''] # pylint: disable=W1505,E1101
-        except: # pylint: disable=W0702
+        except Exception:
             osinfo = ['UNKNOWN', 'FFFF', '', '']
 
     # The platform.py lib has issue with detecting oracle linux distribution.
@@ -116,6 +117,37 @@ def get_distro():
     return osinfo
 
 
+def get_lis_version():
+    """
+    This uses the Linux kernel's 'modinfo' command to retrieve the
+    "version" field for the "hv_vmbus" kernel module (the LIS
+    drivers). This is the documented method to retrieve the LIS module
+    version. Every Linux guest on Hyper-V will have this driver, but
+    it may not be installed as a module (it could instead be built
+    into the kernel). In that case, this will return "Absent" instead
+    of the version, indicating the driver version can be deduced from
+    the kernel version. It will only return "Failed" in the presence
+    of an exception.
+
+    This function is used to generate telemetry for the version of the
+    LIS drivers installed on the VM. The function and associated
+    telemetry can be removed after a few releases.
+    """
+    try:
+        modinfo_output = shellutil.run_command(["modinfo", "-F", "version", "hv_vmbus"])
+        if modinfo_output:
+            return modinfo_output
+        # If the system doesn't have LIS drivers, 'modinfo' will
+        # return nothing on stdout, which will cause 'run_command'
+        # to return an empty string.
+        return "Absent"
+    except Exception:
+        # Ignore almost every possible exception because this is in a
+        # critical code path. Unfortunately the logger isn't already
+        # imported in this module or we'd log this too.
+        return "Failed"
+
+
 AGENT_NAME = "WALinuxAgent"
 AGENT_LONG_NAME = "Azure Linux Agent"
 AGENT_VERSION = '2.2.52'
diff --git a/azurelinuxagent/ga/update.py b/azurelinuxagent/ga/update.py
index 224adddb..72a84b2a 100644
--- a/azurelinuxagent/ga/update.py
+++ b/azurelinuxagent/ga/update.py
@@ -49,8 +49,8 @@ from azurelinuxagent.common.protocol.util import get_protocol_util
 from azurelinuxagent.common.protocol.hostplugin import HostPluginProtocol
 from azurelinuxagent.common.utils.flexible_version import FlexibleVersion
 from azurelinuxagent.common.version import AGENT_NAME, AGENT_VERSION, AGENT_DIR_PATTERN, CURRENT_AGENT,\
-    CURRENT_VERSION, DISTRO_NAME, DISTRO_VERSION, is_current_agent_installed, PY_VERSION_MAJOR, PY_VERSION_MINOR, \
-    PY_VERSION_MICRO
+    CURRENT_VERSION, DISTRO_NAME, DISTRO_VERSION, is_current_agent_installed, get_lis_version, PY_VERSION_MAJOR, \
+    PY_VERSION_MINOR, PY_VERSION_MICRO
 from azurelinuxagent.ga.collect_logs import get_collect_logs_handler, is_log_collection_allowed
 from azurelinuxagent.ga.env import get_env_handler
 from azurelinuxagent.ga.extension_telemetry import get_extension_telemetry_handler
@@ -262,9 +262,9 @@ class UpdateHandler(object): # pylint: disable=R0902
             initialize_event_logger_vminfo_common_parameters(protocol)
 
             # Log OS-specific info.
-            os_info_msg = u"Distro: {0}-{1}; OSUtil: {2}; AgentService: {3}; Python: {4}.{5}.{6}".format(
+            os_info_msg = u"Distro: {0}-{1}; OSUtil: {2}; AgentService: {3}; Python: {4}.{5}.{6}; LISDrivers: {7}".format(
                 DISTRO_NAME, DISTRO_VERSION, type(self.osutil).__name__, self.osutil.service_name, PY_VERSION_MAJOR,
-                PY_VERSION_MINOR, PY_VERSION_MICRO)
+                PY_VERSION_MINOR, PY_VERSION_MICRO, get_lis_version())
             logger.info(os_info_msg)
             add_event(AGENT_NAME, op=WALAEventOperation.OSInfo, message=os_info_msg)
 


Test patch:
diff --git a/tests/common/test_version.py b/tests/common/test_version.py
index f2fd0859..15c60f2a 100644
--- a/tests/common/test_version.py
+++ b/tests/common/test_version.py
@@ -17,15 +17,17 @@
 
 from __future__ import print_function
 
+import os
+import textwrap
+
 import mock
-import os # pylint: disable=wrong-import-order
-import textwrap # pylint: disable=wrong-import-order
 
 import azurelinuxagent.common.conf as conf
 from azurelinuxagent.common.event import EVENTS_DIRECTORY
 from azurelinuxagent.common.version import set_current_agent, \
     AGENT_LONG_VERSION, AGENT_VERSION, AGENT_NAME, AGENT_NAME_PATTERN, \
-    get_f5_platform, get_distro, PY_VERSION_MAJOR, PY_VERSION_MINOR
+    get_f5_platform, get_distro, get_lis_version, PY_VERSION_MAJOR, \
+    PY_VERSION_MINOR
 from tests.tools import AgentTestCase, open_patch, patch
 
 
@@ -59,34 +61,31 @@ def default_system_exception():
 
 def is_platform_dist_supported():
     # platform.dist() and platform.linux_distribution() is deprecated from Python 3.8+
-    if PY_VERSION_MAJOR == 3 and PY_VERSION_MINOR >= 8: # pylint: disable=no-else-return
+    if PY_VERSION_MAJOR == 3 and PY_VERSION_MINOR >= 8:
         return False
-    else:
-        return True
+    return True
+
 
 class TestAgentVersion(AgentTestCase):
-    def setUp(self): # pylint: disable=useless-return
+    def setUp(self):
         AgentTestCase.setUp(self)
-        return
 
     @mock.patch('platform.system', side_effect=freebsd_system)
     @mock.patch('re.sub', side_effect=freebsd_system_release)
-    def test_distro_is_correct_format_when_freebsd(self, platform_system_name, mock_variable): # pylint: disable=useless-return,unused-argument
+    def test_distro_is_correct_format_when_freebsd(self, platform_system_name, mock_variable): # pylint: disable=unused-argument
         osinfo = get_distro()
         freebsd_list = ['freebsd', "10.0", '', 'freebsd']
         self.assertListEqual(freebsd_list, osinfo)
-        return
 
     @mock.patch('platform.system', side_effect=openbsd_system)
     @mock.patch('re.sub', side_effect=openbsd_system_release)
-    def test_distro_is_correct_format_when_openbsd(self, platform_system_name, mock_variable): # pylint: disable=useless-return,unused-argument
+    def test_distro_is_correct_format_when_openbsd(self, platform_system_name, mock_variable): # pylint: disable=unused-argument
         osinfo = get_distro()
         openbsd_list = ['openbsd', "20.0", '', 'openbsd']
         self.assertListEqual(openbsd_list, osinfo)
-        return
 
     @mock.patch('platform.system', side_effect=default_system)
-    def test_distro_is_correct_format_when_default_case(self, *args): # pylint: disable=useless-return,unused-argument
+    def test_distro_is_correct_format_when_default_case(self, *args): # pylint: disable=unused-argument
         default_list = ['', '', '', '']
         unknown_list = ['unknown', 'FFFF', '', '']
 
@@ -98,10 +97,9 @@ class TestAgentVersion(AgentTestCase):
             # platform.dist() is deprecated in Python 3.7+ and would throw, resulting in unknown distro
             osinfo = get_distro()
             self.assertListEqual(unknown_list, osinfo)
-        return
 
     @mock.patch('platform.system', side_effect=default_system)
-    def test_distro_is_correct_for_exception_case(self, *args): # pylint: disable=useless-return,unused-argument
+    def test_distro_is_correct_for_exception_case(self, *args): # pylint: disable=unused-argument
         default_list = ['unknown', 'FFFF', '', '']
 
         if is_platform_dist_supported():
@@ -113,39 +111,45 @@ class TestAgentVersion(AgentTestCase):
             osinfo = get_distro()
 
         self.assertListEqual(default_list, osinfo)
-        return
+
+    def test_get_lis_version_should_return_a_string(self):
+        """
+        On a Hyper-V guest with the LIS drivers installed as a module,
+        this function should return a string of the version, like
+        '4.3.5'. Anywhere else it should return 'Absent' and possibly
+        return 'Failed' if an exception was raised, so we check that
+        it returns a string'.
+        """
+        lis_version = get_lis_version()
+        self.assertIsInstance(lis_version, str)
 
 
 class TestCurrentAgentName(AgentTestCase):
-    def setUp(self): # pylint: disable=useless-return
+    def setUp(self):
         AgentTestCase.setUp(self)
-        return
 
     @patch("os.getcwd", return_value="/default/install/directory")
-    def test_extract_name_finds_installed(self, mock_cwd): # pylint: disable=useless-return,unused-argument
+    def test_extract_name_finds_installed(self, mock_cwd): # pylint: disable=unused-argument
         current_agent, current_version = set_current_agent()
         self.assertEqual(AGENT_LONG_VERSION, current_agent)
         self.assertEqual(AGENT_VERSION, str(current_version))
-        return
 
     @patch("os.getcwd", return_value="/")
-    def test_extract_name_root_finds_installed(self, mock_cwd): # pylint: disable=useless-return,unused-argument
+    def test_extract_name_root_finds_installed(self, mock_cwd): # pylint: disable=unused-argument
         current_agent, current_version = set_current_agent()
         self.assertEqual(AGENT_LONG_VERSION, current_agent)
         self.assertEqual(AGENT_VERSION, str(current_version))
-        return
 
     @patch("os.getcwd")
-    def test_extract_name_in_path_finds_installed(self, mock_cwd): # pylint: disable=useless-return
+    def test_extract_name_in_path_finds_installed(self, mock_cwd):
         path = os.path.join(conf.get_lib_dir(), EVENTS_DIRECTORY)
         mock_cwd.return_value = path
         current_agent, current_version = set_current_agent()
         self.assertEqual(AGENT_LONG_VERSION, current_agent)
         self.assertEqual(AGENT_VERSION, str(current_version))
-        return
 
     @patch("os.getcwd")
-    def test_extract_name_finds_latest_agent(self, mock_cwd): # pylint: disable=useless-return
+    def test_extract_name_finds_latest_agent(self, mock_cwd):
         path = os.path.join(conf.get_lib_dir(), "{0}-{1}".format(
             AGENT_NAME,
             "1.2.3"))
@@ -155,7 +159,6 @@ class TestCurrentAgentName(AgentTestCase):
         current_agent, current_version = set_current_agent()
         self.assertEqual(agent, current_agent)
         self.assertEqual(version, str(current_version))
-        return
 
 
 class TestGetF5Platforms(AgentTestCase):
@@ -172,8 +175,8 @@ class TestGetF5Platforms(AgentTestCase):
         Changelist: 1874858
         JobID: 705993""")
 
-        mo = mock.mock_open(read_data=version_file) # pylint: disable=invalid-name
-        with patch(open_patch(), mo):
+        mocked_open = mock.mock_open(read_data=version_file)
+        with patch(open_patch(), mocked_open):
             platform = get_f5_platform()
             self.assertTrue(platform[0] == 'bigip')
             self.assertTrue(platform[1] == '12.1.1')
@@ -193,8 +196,8 @@ class TestGetF5Platforms(AgentTestCase):
         Changelist: 1773831
         JobID: 673467""")
 
-        mo = mock.mock_open(read_data=version_file) # pylint: disable=invalid-name
-        with patch(open_patch(), mo):
+        mocked_open = mock.mock_open(read_data=version_file)
+        with patch(open_patch(), mocked_open):
             platform = get_f5_platform()
             self.assertTrue(platform[0] == 'bigip')
             self.assertTrue(platform[1] == '12.1.0')
@@ -214,8 +217,8 @@ class TestGetF5Platforms(AgentTestCase):
         Changelist: 1486072
         JobID: 536212""")
 
-        mo = mock.mock_open(read_data=version_file) # pylint: disable=invalid-name
-        with patch(open_patch(), mo):
+        mocked_open = mock.mock_open(read_data=version_file)
+        with patch(open_patch(), mocked_open):
             platform = get_f5_platform()
             self.assertTrue(platform[0] == 'bigip')
             self.assertTrue(platform[1] == '12.0.0')
@@ -235,8 +238,8 @@ class TestGetF5Platforms(AgentTestCase):
         Changelist: 1924048
         JobID: 734712""")
 
-        mo = mock.mock_open(read_data=version_file) # pylint: disable=invalid-name
-        with patch(open_patch(), mo):
+        mocked_open = mock.mock_open(read_data=version_file)
+        with patch(open_patch(), mocked_open):
             platform = get_f5_platform()
             self.assertTrue(platform[0] == 'iworkflow')
             self.assertTrue(platform[1] == '2.0.1')
@@ -256,8 +259,8 @@ class TestGetF5Platforms(AgentTestCase):
         Changelist: 1907534
         JobID: 726344""")
 
-        mo = mock.mock_open(read_data=version_file) # pylint: disable=invalid-name
-        with patch(open_patch(), mo):
+        mocked_open = mock.mock_open(read_data=version_file)
+        with patch(open_patch(), mocked_open):
             platform = get_f5_platform()
             self.assertTrue(platform[0] == 'bigiq')
             self.assertTrue(platform[1] == '5.1.0')