address comments

azurelinuxagent/agent.py

@@ -34,6 +34,7 @@
 import azurelinuxagent.common.event as event
 import azurelinuxagent.common.logger as logger
 from azurelinuxagent.common.future import ustr
+from azurelinuxagent.common.logcollector import LogCollector, LOG_COLLECTOR_FULL_MODE_FLAG, OUTPUT_RESULTS_FILE_PATH
 from azurelinuxagent.common.osutil import get_osutil
 from azurelinuxagent.common.utils import fileutil
 from azurelinuxagent.common.version import AGENT_NAME, AGENT_LONG_VERSION, \
@@ -152,15 +153,14 @@ def show_configuration(self):
             print("{0} = {1}".format(k, configuration[k]))
 
     def collect_logs(self, log_collector_mode):
-        from azurelinuxagent.common.logcollector import LogCollector, OUTPUT_RESULTS_FILE_PATH
-        if log_collector_mode == "full":
+        if log_collector_mode == LOG_COLLECTOR_FULL_MODE_FLAG:
             print("Running log collector mode full")
         else:
             print("Running log collector mode normal")
 
         try:
-            log_collector = LogCollector(log_collector_mode == "full")
-            archive = log_collector.collect_logs()
+            log_collector = LogCollector(log_collector_mode == LOG_COLLECTOR_FULL_MODE_FLAG)
+            archive = log_collector.collect_logs_and_get_archive()
             print("Log collection successfully completed. Archive can be found at {0} "
                   "and detailed log output can be found at {1}".format(archive, OUTPUT_RESULTS_FILE_PATH))
         except Exception as e: # pylint: disable=C0103
@@ -226,13 +226,13 @@ def parse_args(sys_args): # pylint: disable=R0912
             if not os.path.exists(conf_file_path):
                 print("Error: Configuration file {0} does not exist".format(
                         conf_file_path), file=sys.stderr) 
-                usage()
+                print(usage())
                 sys.exit(1)
 
         m = re.match("^(?:[-/]*)-mode:([\w/\.\-_]+)", arg) # pylint: disable=W1401,C0103
         if m is not None:
             log_collector_mode = m.group(1)
-            if log_collector_mode != "full":
+            if log_collector_mode != LOG_COLLECTOR_FULL_MODE_FLAG:
                 print("Error: Invalid value for log collector mode: {0}. Accepted value: full. "
                       "If mode is not specified, will use normal mode.".format(log_collector_mode))
                 print(usage())

azurelinuxagent/common/logcollector.py

@@ -40,6 +40,7 @@
 _LOG_COLLECTOR_DIR = os.path.join(_AGENT_LIB_DIR, "logcollector")
 _TRUNCATED_FILES_DIR = os.path.join(_LOG_COLLECTOR_DIR, "truncated")
 
+LOG_COLLECTOR_FULL_MODE_FLAG = "full"
 OUTPUT_RESULTS_FILE_PATH = os.path.join(_LOG_COLLECTOR_DIR, "results.txt")
 COMPRESSED_ARCHIVE_PATH = os.path.join(_LOG_COLLECTOR_DIR, "logs.zip")
 
@@ -168,7 +169,8 @@ def _convert_file_name_to_archive_name(file_name):
         # File name is the name of the file on disk, whereas archive name is the name of that same file in the archive.
         # For non-truncated files: /var/log/waagent.log on disk becomes var/log/waagent.log in archive
         # (leading separator is removed by the archive).
-        # For truncated files: /var/truncated/var/log/syslog.1 on disk becomes truncated_var_log_syslog.1 in archive.
+        # For truncated files: /var/lib/waagent/logcollector/truncated/var/log/syslog.1 on disk becomes
+        # truncated_var_log_syslog.1 in the archive.
         if file_name.startswith(_TRUNCATED_FILES_DIR): # pylint: disable=R1705
             original_file_path = file_name[len(_TRUNCATED_FILES_DIR):].lstrip(os.path.sep)
             archive_file_name = LogCollector._TRUNCATED_FILE_PREFIX + original_file_path.replace(os.path.sep, "_")
@@ -324,11 +326,10 @@ def _create_list_of_files_to_collect(self):
         files_to_collect = self._get_final_list_for_archive(prioritized_file_paths)
         return files_to_collect
 
-    def collect_logs(self):
+    def collect_logs_and_get_archive(self):
         """
-        Public method that collects necessary log files in a tarball that is updated each time this method is invoked.
-        The tarball is then compressed into a zip.
-        :return: Returns True if the log collection succeeded
+        Public method that collects necessary log files in a compressed zip archive.
+        :return: Returns the path of the collected compressed archive
         """
         files_to_collect = []
 

azurelinuxagent/common/utils/restutil.py

@@ -312,11 +312,17 @@ def _http_request(method, host, rel_uri, port=None, data=None, secure=False, # p
                                          conn_port,
                                          timeout=10)
 
+
+    if method == "PUT" and "vmAgentLog" in url:
+        payload = "[redacted binary contents of logs.zip]"
+    else:
+        payload = textutil.str_to_encoded_ustr(data)
+
     # Logger requires the data to be a ustr to log properly, ensuring that the data string that we log is always ustr.
     logger.verbose("HTTP connection [{0}] [{1}] [{2}] [{3}]",
                    method,
                    redact_sas_tokens_in_urls(url),
-                   textutil.str_to_encoded_ustr(data),
+                   payload,
                    headers)
 
     conn.request(method=method, url=url, body=data, headers=headers)

azurelinuxagent/common/utils/textutil.py

@@ -427,8 +427,6 @@ def str_to_encoded_ustr(s, encoding='utf-8'): # pylint: disable=C0103
         except Exception:
             # If some issues in decoding, just return the string
             return ustr(s)
-    try:
-        # For Py2, explicitly convert the string to unicode with the specified encoding
-        return ustr(s, encoding=encoding)
-    except Exception as e: # pylint: disable=C0103
-        return ustr(e)
+
+    # For Py2, explicitly convert the string to unicode with the specified encoding
+    return ustr(s, encoding=encoding)

azurelinuxagent/ga/collect_logs.py

@@ -30,7 +30,7 @@
 from azurelinuxagent.common.logcollector import COMPRESSED_ARCHIVE_PATH
 from azurelinuxagent.common.protocol.util import get_protocol_util
 from azurelinuxagent.common.utils import shellutil
-from azurelinuxagent.common.utils.shellutil import get_python_cmd
+from azurelinuxagent.common.utils.shellutil import get_python_cmd, CommandError
 from azurelinuxagent.common.version import PY_VERSION_MAJOR, PY_VERSION_MINOR, AGENT_NAME, CURRENT_VERSION
 from azurelinuxagent.ga.periodic_operation import PeriodicOperation
 
@@ -39,6 +39,33 @@ def get_collect_logs_handler():
     return CollectLogsHandler()
 
 
+def is_log_collection_allowed():
+    # There are three conditions that need to be met in order to allow periodic log collection:
+    # 1) It should be enabled in the configuration.
+    # 2) The system must be using systemd to manage services. Needed for resource limiting of the log collection.
+    # 3) The python version must be greater than 2.6 in order to support the ZipFile library used when collecting.
+    conf_enabled = conf.get_collect_logs()
+    systemd_present = os.path.exists("/run/systemd/system/")
+    supported_python = PY_VERSION_MINOR >= 7 if PY_VERSION_MAJOR == 2 else PY_VERSION_MAJOR == 3
+    is_allowed = conf_enabled and systemd_present and supported_python
+
+    msg = "Checking if log collection is allowed at this time [{0}]. All three conditions must be met: " \
+          "configuration enabled [{1}], systemd present [{2}], python supported: [{3}]".format(is_allowed,
+                                                                                               conf_enabled,
+                                                                                               systemd_present,
+                                                                                               supported_python)
+    logger.info(msg)
+    add_event(
+        name=AGENT_NAME,
+        version=CURRENT_VERSION,
+        op=WALAEventOperation.LogCollection,
+        is_success=is_allowed,
+        message=msg,
+        log_event=False)
+
+    return is_allowed
+
+
 class CollectLogsHandler(object):
     """
     Periodically collects and uploads logs from the VM to the host.
@@ -61,34 +88,6 @@ def __init__(self):
             PeriodicOperation("collect_and_send_logs", self.collect_and_send_logs, conf.get_collect_logs_period())
         ]
 
-    def _log_collection_allowed(self):
-        # There are three conditions that need to be met in order to allow periodic log collection:
-        # 1) It should be enabled in the configuration.
-        # 2) The system must be using systemd to manage services. Needed for resource limiting of the log collection.
-        # 3) The python version must be greater than 2.6 in order to support the ZipFile library used when collecting.
-        conf_enabled = conf.get_collect_logs()
-        systemd_present = os.path.exists("/run/systemd/system/")
-        supported_python = PY_VERSION_MINOR >= 7 if PY_VERSION_MAJOR == 2 else PY_VERSION_MAJOR == 3
-        is_allowed = conf_enabled and systemd_present and supported_python
-
-        if self.last_state != is_allowed:
-            msg = "Checking if log collection is allowed at this time [{0}]. All three conditions must be met: " \
-                  "configuration enabled [{1}], systemd present [{2}], python supported: [{3}]".format(is_allowed,
-                                                                                                       conf_enabled,
-                                                                                                       systemd_present,
-                                                                                                       supported_python)
-            self.last_state = is_allowed
-            logger.info(msg)
-            add_event(
-                name=AGENT_NAME,
-                version=CURRENT_VERSION,
-                op=WALAEventOperation.LogCollection,
-                is_success=is_allowed,
-                message=msg,
-                log_event=False)
-
-        return is_allowed
-
     def run(self):
         self.start(init_data=True)
 
@@ -121,7 +120,7 @@ def init_protocols(self):
 
     def daemon(self, init_data=False):
         try:
-            if not self._log_collection_allowed():
+            if not is_log_collection_allowed():
                 return
 
             if init_data:
@@ -167,7 +166,10 @@ def _collect_logs():
         # The log tool is invoked from the current agent's egg with the command line option
         collect_logs_cmd = [get_python_cmd(), "-u", sys.argv[0], "-collect-logs"]
         final_command = systemd_cmd + resource_limits + collect_logs_cmd
+
         start_time = datetime.datetime.utcnow()
+        success = False
+        msg = None
         try:
             shellutil.run_command(final_command, log_error=True)
             duration = elapsed_milliseconds(start_time)
@@ -176,50 +178,49 @@ def _collect_logs():
             msg = "Successfully collected logs. Archive size: {0} b, elapsed time: {1} ms.".format(archive_size,
                                                                                                    duration)
             logger.info(msg)
-            add_event(
-                name=AGENT_NAME,
-                version=CURRENT_VERSION,
-                op=WALAEventOperation.LogCollection,
-                is_success=True,
-                message=msg,
-                log_event=False)
+            success = True
+
+            return True
         except Exception as e: # pylint: disable=C0103
             duration = elapsed_milliseconds(start_time)
-            msg = "Failed to collect logs. Elapsed time: {0} ms. Error: {1}".format(duration, ustr(e))
+
+            if isinstance(e, CommandError):
+                exception_message = ustr("[stderr] %s", e.stderr)
+            else:
+                exception_message = ustr(e)
+
+            msg = "Failed to collect logs. Elapsed time: {0} ms. Error: {1}".format(duration, exception_message)
             # No need to log to the local log since we ran run_command with logging errors as enabled
+
+            return False
+        finally:
             add_event(
                 name=AGENT_NAME,
                 version=CURRENT_VERSION,
                 op=WALAEventOperation.LogCollection,
-                is_success=False,
+                is_success=success,
                 message=msg,
                 log_event=False)
 
-            return False
-        return True
-
     def _send_logs(self):
+        msg = None
+        success = False
         try:
             with open(COMPRESSED_ARCHIVE_PATH, "rb") as fh: # pylint: disable=C0103
                 archive_content = fh.read()
                 self.protocol.upload_logs(archive_content)
                 msg = "Successfully uploaded logs."
                 logger.info(msg)
-                add_event(
-                    name=AGENT_NAME,
-                    version=CURRENT_VERSION,
-                    op=WALAEventOperation.LogCollection,
-                    is_success=True,
-                    message=msg,
-                    log_event=False)
 
+            success = True
         except Exception as e: # pylint: disable=C0103
             msg = "Failed to upload logs. Error: {0}".format(ustr(e))
             logger.warn(msg)
+        finally:
             add_event(
                 name=AGENT_NAME,
                 version=CURRENT_VERSION,
                 op=WALAEventOperation.LogCollection,
-                is_success=False,
+                is_success=success,
                 message=msg,
                 log_event=False)

azurelinuxagent/ga/update.py

@@ -52,7 +52,7 @@
 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
-from azurelinuxagent.ga.collect_logs import get_collect_logs_handler
+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
 
@@ -271,10 +271,12 @@ def run(self, debug=False): # pylint: disable=R0912,R0914
             # Get all thread handlers
             all_thread_handlers = [
                 get_monitor_handler(),
-                get_env_handler(),
-                get_collect_logs_handler()
+                get_env_handler()
             ]
 
+            if is_log_collection_allowed():
+                all_thread_handlers.append(get_collect_logs_handler())
+
             if is_extension_telemetry_pipeline_enabled():
                 # Reuse the same protocol_util as the UpdateHandler class to avoid new initializations
                 all_thread_handlers.append(get_extension_telemetry_handler(self.protocol_util))

tests/common/test_logcollector.py

@@ -193,7 +193,7 @@ def test_log_collector_parses_commands_in_manifest(self):
 
         with patch("azurelinuxagent.common.logcollector.MANIFEST_NORMAL", manifest):
             log_collector = LogCollector()
-            archive = log_collector.collect_logs()
+            archive = log_collector.collect_logs_and_get_archive()
 
         with open(self.output_results_file_path, "r") as fh: # pylint: disable=invalid-name
             results = fh.readlines()
@@ -221,7 +221,7 @@ def test_log_collector_uses_full_manifest_when_full_mode_enabled(self):
 
         with patch("azurelinuxagent.common.logcollector.MANIFEST_FULL", manifest):
             log_collector = LogCollector(full_mode=True)
-            archive = log_collector.collect_logs()
+            archive = log_collector.collect_logs_and_get_archive()
 
         self._assert_archive_created(archive)
         self._assert_files_are_in_archive(expected_files=[file_to_collect])
@@ -234,7 +234,7 @@ def test_log_collector_should_collect_all_files(self):
         # and combined they do not cross the archive size threshold.
 
         log_collector = LogCollector()
-        archive = log_collector.collect_logs()
+        archive = log_collector.collect_logs_and_get_archive()
 
         self._assert_archive_created(archive)
 
@@ -255,7 +255,7 @@ def test_log_collector_should_truncate_large_text_files_and_ignore_large_binary_
         # Set the size limit so that some files are too large to collect in full.
         with patch("azurelinuxagent.common.logcollector._FILE_SIZE_LIMIT", SMALL_FILE_SIZE):
             log_collector = LogCollector()
-            archive = log_collector.collect_logs()
+            archive = log_collector.collect_logs_and_get_archive()
 
         self._assert_archive_created(archive)
 
@@ -288,7 +288,7 @@ def test_log_collector_should_prioritize_important_files_if_archive_too_big(self
         with patch("azurelinuxagent.common.logcollector._UNCOMPRESSED_ARCHIVE_SIZE_LIMIT", 10 * 1024 * 1024):
             with patch("azurelinuxagent.common.logcollector._MUST_COLLECT_FILES", must_collect_files):
                 log_collector = LogCollector()
-                archive = log_collector.collect_logs()
+                archive = log_collector.collect_logs_and_get_archive()
 
         self._assert_archive_created(archive)
 
@@ -314,7 +314,7 @@ def test_log_collector_should_prioritize_important_files_if_archive_too_big(self
 
         with patch("azurelinuxagent.common.logcollector._UNCOMPRESSED_ARCHIVE_SIZE_LIMIT", 10 * 1024 * 1024):
             with patch("azurelinuxagent.common.logcollector._MUST_COLLECT_FILES", must_collect_files):
-                second_archive = log_collector.collect_logs()
+                second_archive = log_collector.collect_logs_and_get_archive()
 
         expected_files = [
             os.path.join(self.root_collect_dir, "waagent.log"),
@@ -338,7 +338,7 @@ def test_log_collector_should_update_archive_when_files_are_new_or_modified_or_d
         # Ensure the archive reflects the state of files on the disk at collection time. If a file was updated, it
         # needs to be updated in the archive, deleted if removed from disk, and added if not previously seen.
         log_collector = LogCollector()
-        first_archive = log_collector.collect_logs()
+        first_archive = log_collector.collect_logs_and_get_archive()
         self._assert_archive_created(first_archive)
 
         # Everything should be in the archive
@@ -367,7 +367,7 @@ def test_log_collector_should_update_archive_when_files_are_new_or_modified_or_d
                                            LARGE_FILE_SIZE)
         rm_files(os.path.join(self.root_collect_dir, "waagent.log.1"))
 
-        second_archive = log_collector.collect_logs()
+        second_archive = log_collector.collect_logs_and_get_archive()
         self._assert_archive_created(second_archive)
 
         expected_files = [
@@ -408,7 +408,7 @@ def test_log_collector_should_clean_up_uncollected_truncated_files(self):
             with patch("azurelinuxagent.common.logcollector._MUST_COLLECT_FILES", must_collect_files):
                 with patch("azurelinuxagent.common.logcollector._FILE_SIZE_LIMIT", SMALL_FILE_SIZE):
                     log_collector = LogCollector()
-                    archive = log_collector.collect_logs()
+                    archive = log_collector.collect_logs_and_get_archive()
 
         self._assert_archive_created(archive)
 
@@ -429,7 +429,7 @@ def test_log_collector_should_clean_up_uncollected_truncated_files(self):
             with patch("azurelinuxagent.common.logcollector._MUST_COLLECT_FILES", must_collect_files):
                 with patch("azurelinuxagent.common.logcollector._FILE_SIZE_LIMIT", SMALL_FILE_SIZE):
                     log_collector = LogCollector()
-                    second_archive = log_collector.collect_logs()
+                    second_archive = log_collector.collect_logs_and_get_archive()
 
         expected_files = [
             os.path.join(self.root_collect_dir, "waagent.log"),