Repository: Azure/WALinuxAgent
Pull request #2061 (2020-11-03 16:50:24)

Change the way the PA checks for cloud-init existence and status
<!-- DO NOT DELETE THIS TEMPLATE -->

## Description

This PR changes the way that the provisioning agent in walinuxagent determines if cloud-init is already acting as the provisioning agent. This uses unit/service status to make this determination, instead of reading processes. This should be a more reliable implementation to make the correct determination. And it also reuses existing logic in walinuxagent for this discovery.

### PR information
- [ ] The title of the PR is clear and informative.
- [ ] 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).
- [ ] Except for special cases involving multiple contributors, the PR is started from a fork of the main repository, not a branch.
- [ ] If applicable, the PR references the bug/issue that it fixes in the description.
- [ ] New Unit tests were added for the changes made and Travis.CI is passing.

### Quality of Code and Contribution Guidelines
- [ ] I have read the [contribution guidelines](https://github.com/Azure/WALinuxAgent/blob/master/.github/CONTRIBUTING.md).

narrieta: LGTM; running automation
trstringer: @narrieta, I fixed the unit tests. The reason they were passing on our local machines is because we didn't have cloud-init installed on the test host. Therefore the call to `azurelinux.pa.provision.cloudinitdetect.cloud_init_is_enabled` came back `False`. But it seems like on the CI servers there probably was cloud-init installed and enabled, and it was causing the failure then. All I had to do was mock this function to return `False`.

Now that the tests are passing, this PR should be ready for re-review.
narrieta: @trstringer thanks; running automation
narrieta: automation ok
anhvoms: LGTM
trstringer: Thanks, all, for the reviews! @narrieta, let me know if there's anything else you need from me prior to merging.
narrieta: @trstringer Just waiting for the Travis run to complete. Travis has been extremely slow these days. It seems like your run is still queued.

Patch:
diff --git a/azurelinuxagent/pa/provision/cloudinit.py b/azurelinuxagent/pa/provision/cloudinit.py
index a373acbc..a3b32c66 100644
--- a/azurelinuxagent/pa/provision/cloudinit.py
+++ b/azurelinuxagent/pa/provision/cloudinit.py
@@ -19,7 +19,6 @@
 
 import os
 import os.path
-import subprocess
 import time
 
 from datetime import datetime
@@ -35,6 +34,7 @@ from azurelinuxagent.common.future import ustr
 from azurelinuxagent.common.protocol.util import OVF_FILE_NAME
 from azurelinuxagent.common.protocol.ovfenv import OvfEnv
 from azurelinuxagent.pa.provision.default import ProvisionHandler
+from azurelinuxagent.pa.provision.cloudinitdetect import cloud_init_is_enabled
 
 
 class CloudInitProvisionHandler(ProvisionHandler):
@@ -92,8 +92,8 @@ class CloudInitProvisionHandler(ProvisionHandler):
                         "sleeping {2}s]".format(ovf_file_path,
                                                 max_retry - retry,
                                                 sleep_time))
-                    if not self.validate_cloud_init():
-                        logger.warn("cloud-init does not appear to be running")
+                    if not cloud_init_is_enabled():
+                        logger.warn("cloud-init does not appear to be enabled")
                     time.sleep(sleep_time)
         raise ProvisionError("Giving up, ovf-env.xml was not copied to {0} "
                              "after {1}s".format(ovf_file_path,
@@ -120,75 +120,9 @@ class CloudInitProvisionHandler(ProvisionHandler):
                             "sleeping {2}s]".format(path,
                                                     max_retry - retry,
                                                     sleep_time))
-                if not self.validate_cloud_init():
+                if not cloud_init_is_enabled():
                     logger.warn("cloud-init does not appear to be running")
                 time.sleep(sleep_time)
         raise ProvisionError("Giving up, ssh host key was not found at {0} "
                              "after {1}s".format(path,
                                                  max_retry * sleep_time))
-
-def _cloud_init_is_enabled_systemd():
-    """
-    Determine whether or not cloud-init is enabled on a systemd machine.
-
-    Args:
-        None
-
-    Returns:
-        bool: True if cloud-init is enabled, False if otherwise.
-    """
-
-    try:
-        systemctl_output = subprocess.check_output([
-            'systemctl',
-            'is-enabled',
-            'cloud-init-local.service'
-        ], stderr=subprocess.STDOUT).decode('utf-8').replace('\n', '')
-
-        unit_is_enabled = systemctl_output == 'enabled'
-    except Exception as exc:
-        logger.info('Error getting cloud-init enabled status from systemctl: {0}'.format(exc))
-        unit_is_enabled = False
-
-    return unit_is_enabled
-
-def _cloud_init_is_enabled_service():
-    """
-    Determine whether or not cloud-init is enabled on a non-systemd machine.
-
-    Args:
-        None
-
-    Returns:
-        bool: True if cloud-init is enabled, False if otherwise.
-    """
-
-    try:
-        subprocess.check_output([
-            'service',
-            'cloud-init',
-            'status'
-        ], stderr=subprocess.STDOUT)
-
-        unit_is_enabled = True
-    except Exception as exc:
-        logger.info('Error getting cloud-init enabled status from service: {0}'.format(exc))
-        unit_is_enabled = False
-
-    return unit_is_enabled
-
-def cloud_init_is_enabled():
-    """
-    Determine whether or not cloud-init is enabled.
-
-    Args:
-        None
-
-    Returns:
-        bool: True if cloud-init is enabled, False if otherwise.
-    """
-
-    unit_is_enabled = _cloud_init_is_enabled_systemd() or _cloud_init_is_enabled_service()
-    logger.info('cloud-init is enabled: {0}'.format(unit_is_enabled))
-
-    return unit_is_enabled
diff --git a/azurelinuxagent/pa/provision/cloudinitdetect.py b/azurelinuxagent/pa/provision/cloudinitdetect.py
new file mode 100644
index 00000000..4e43bcba
--- /dev/null
+++ b/azurelinuxagent/pa/provision/cloudinitdetect.py
@@ -0,0 +1,72 @@
+"""Module for detecting the existence of cloud-init"""
+
+import subprocess
+import azurelinuxagent.common.logger as logger
+
+def _cloud_init_is_enabled_systemd():
+    """
+    Determine whether or not cloud-init is enabled on a systemd machine.
+
+    Args:
+        None
+
+    Returns:
+        bool: True if cloud-init is enabled, False if otherwise.
+    """
+
+    try:
+        systemctl_output = subprocess.check_output([
+            'systemctl',
+            'is-enabled',
+            'cloud-init-local.service'
+        ], stderr=subprocess.STDOUT).decode('utf-8').replace('\n', '')
+
+        unit_is_enabled = systemctl_output == 'enabled'
+    # pylint: disable=broad-except
+    except Exception as exc:
+        logger.info('Unable to get cloud-init enabled status from systemctl: {0}'.format(exc))
+        unit_is_enabled = False
+
+    return unit_is_enabled
+
+def _cloud_init_is_enabled_service():
+    """
+    Determine whether or not cloud-init is enabled on a non-systemd machine.
+
+    Args:
+        None
+
+    Returns:
+        bool: True if cloud-init is enabled, False if otherwise.
+    """
+
+    try:
+        subprocess.check_output([
+            'service',
+            'cloud-init',
+            'status'
+        ], stderr=subprocess.STDOUT)
+
+        unit_is_enabled = True
+    # pylint: disable=broad-except
+    except Exception as exc:
+        logger.info('Unable to get cloud-init enabled status from service: {0}'.format(exc))
+        unit_is_enabled = False
+
+    return unit_is_enabled
+
+def cloud_init_is_enabled():
+    """
+    Determine whether or not cloud-init is enabled.
+
+    Args:
+        None
+
+    Returns:
+        bool: True if cloud-init is enabled, False if otherwise.
+    """
+
+    unit_is_enabled = _cloud_init_is_enabled_systemd() or _cloud_init_is_enabled_service()
+    logger.info('cloud-init is enabled: {0}'.format(unit_is_enabled))
+
+    return unit_is_enabled
diff --git a/azurelinuxagent/pa/provision/default.py b/azurelinuxagent/pa/provision/default.py
index 9eb4eba8..9fc5b2f7 100644
--- a/azurelinuxagent/pa/provision/default.py
+++ b/azurelinuxagent/pa/provision/default.py
@@ -40,6 +40,7 @@ from azurelinuxagent.common.osutil import get_osutil
 from azurelinuxagent.common.protocol.restapi import ProvisionStatus
 from azurelinuxagent.common.protocol.util import get_protocol_util
 from azurelinuxagent.common.version import AGENT_NAME
+from azurelinuxagent.pa.provision.cloudinitdetect import cloud_init_is_enabled
 
 CUSTOM_DATA_FILE = "CustomData"
 CLOUD_INIT_PATTERN = b".*/bin/cloud-init.*"
@@ -70,8 +71,8 @@ class ProvisionHandler(object):
 
             logger.info("Running default provisioning handler")
 
-            if not self.validate_cloud_init(is_expected=False):
-                raise ProvisionError("cloud-init appears to be running, "
+            if cloud_init_is_enabled():
+                raise ProvisionError("cloud-init appears to be installed and enabled, "
                                         "this is not expected, cannot continue")
 
             logger.info("Copying ovf-env.xml")
@@ -104,30 +105,6 @@ class ProvisionHandler(object):
             self.report_event(msg, is_success=False)
             return
 
-    @staticmethod
-    def validate_cloud_init(is_expected=True):
-        is_running = False
-        if os.path.isdir("/proc"):
-            pids = [pid for pid in os.listdir('/proc') if pid.isdigit()]
-        else:
-            pids = []
-        for pid in pids:
-            try:
-                with open(os.path.join('/proc', pid, 'cmdline'), 'rb') as fh: # pylint: disable=C0103
-                    pname = fh.read()
-                    if CLOUD_INIT_REGEX.match(pname):
-                        is_running = True
-                        msg = "cloud-init is running [PID {0}, {1}]".format(pid,
-                                                                            pname)
-                        if is_expected:
-                            logger.verbose(msg)
-                        else:
-                            logger.error(msg)
-                        break
-            except IOError:
-                continue
-        return is_running == is_expected
-
     @staticmethod
     def _get_uptime_seconds():
         try:


Test patch:
diff --git a/tests/pa/test_provision.py b/tests/pa/test_provision.py
index 30f7a84a..d9dcebf3 100644
--- a/tests/pa/test_provision.py
+++ b/tests/pa/test_provision.py
@@ -187,6 +187,7 @@ class TestProvision(AgentTestCase):
     @patch('azurelinuxagent.common.osutil.default.DefaultOSUtil.get_instance_id',
            return_value='B9F3C233-9913-9F42-8EB3-BA656DF32502')
     @patch('azurelinuxagent.pa.provision.default.ProvisionHandler.write_agent_disabled')
+    @patch('azurelinuxagent.pa.provision.default.cloud_init_is_enabled', return_value=False)
     def _provision_test(self, # pylint: disable=invalid-name,too-many-arguments
                         distro_name,
                         distro_version,
@@ -194,8 +195,10 @@ class TestProvision(AgentTestCase):
                         ovf_file,
                         provisionMessage,
                         expect_success,
+                        # pylint:disable=unused-argument
+                        patch_cloud_init_is_enabled,
                         patch_write_agent_disabled,
-                        patch_get_instance_id): # pylint: disable=unused-argument
+                        patch_get_instance_id):
         """
         Assert that the agent issues two telemetry messages as part of a
         successful provisioning.
@@ -251,11 +254,16 @@ class TestProvision(AgentTestCase):
         'azurelinuxagent.common.osutil.default.DefaultOSUtil.get_instance_id',
         return_value='B9F3C233-9913-9F42-8EB3-BA656DF32502')
     @patch('azurelinuxagent.common.conf.get_provisioning_agent', return_value='waagent')
+    @patch('azurelinuxagent.pa.provision.default.cloud_init_is_enabled', return_value=False)
+    # pylint:disable=too-many-arguments
     def test_provision_telemetry_fail(self,
-                                      mock_util, # pylint: disable=unused-argument
                                       distro_name,
                                       distro_version,
-                                      distro_full_name, _):
+                                      distro_full_name,
+                                      # pylint:disable=unused-argument
+                                      patch_cloud_init_is_enabled,
+                                      patch_get_provisioning_agent,
+                                      mock_util):
         """
         Assert that the agent issues one telemetry message as part of a
         failed provisioning.