Use unique name for test VMs. Remove hardcoded log path (#2767)

* Use unique name for test VMs. Remove hardcoded log path

---------

Co-authored-by: narrieta <narrieta>

tests_e2e/orchestrator/lib/agent_junit.py

@@ -56,6 +56,8 @@ def _received_message(self, message: MessageBase) -> None:
             message.suite_name = message.suite_full_name
             image = message.information.get('image')
             if image is not None:
-                message.full_name = image
+                # NOTE: message.information['environment'] is similar to "[generated_2]" and can be correlated
+                # with the main LISA log to find the specific VM for the message.
+                message.full_name = f"{image} [{message.information['environment']}]"
                 message.name = message.full_name
         super()._received_message(message)

tests_e2e/orchestrator/lib/agent_test_suite.py

@@ -107,6 +107,7 @@ class _Context(AgentTestContext):
         def __init__(self, vm: VmIdentifier, paths: AgentTestContext.Paths, connection: AgentTestContext.Connection):
             super().__init__(vm=vm, paths=paths, connection=connection)
             # These are initialized by AgentTestSuite._set_context().
+            self.log_path: Path = None
             self.log: Logger = None
             self.node: Node = None
             self.runbook_name: str = None
@@ -121,7 +122,7 @@ def __init__(self, metadata: TestSuiteMetadata) -> None:
         # The context is initialized by _set_context() via the call to execute()
         self.__context: AgentTestSuite._Context = None
 
-    def _set_context(self, node: Node, variables: Dict[str, Any], log: Logger):
+    def _set_context(self, node: Node, variables: Dict[str, Any], lisa_log_path: str, log: Logger):
         connection_info = node.connection_info
         node_context = get_node_context(node)
         runbook = node.capability.get_extended_runbook(AzureNodeSchema, AZURE)
@@ -144,10 +145,11 @@ def _set_context(self, node: Node, variables: Dict[str, Any], log: Logger):
                 private_key_file=connection_info['private_key_file'],
                 ssh_port=connection_info['port']))
 
+        self.__context.log_path = self._get_log_path(variables, lisa_log_path)
         self.__context.log = log
         self.__context.node = node
         self.__context.is_vhd = self._get_required_parameter(variables, "c_vhd") != ""
-        self.__context.image_name = f"{node.os.name}-vhd" if self.__context.is_vhd else f"{runbook.marketplace.offer}-{runbook.marketplace.sku}"
+        self.__context.image_name = f"{node.os.name}-vhd" if self.__context.is_vhd else self._get_required_parameter(variables, "c_name")
         self.__context.test_suites = self._get_required_parameter(variables, "c_test_suites")
         self.__context.collect_logs = self._get_required_parameter(variables, "collect_logs")
         self.__context.skip_setup = self._get_required_parameter(variables, "skip_setup")
@@ -165,6 +167,14 @@ def _get_required_parameter(variables: Dict[str, Any], name: str) -> Any:
             raise Exception(f"The runbook is missing required parameter '{name}'")
         return value
 
+    @staticmethod
+    def _get_log_path(variables: Dict[str, Any], lisa_log_path: str):
+        # NOTE: If "log_path" is not given as argument to the runbook, use a path derived from LISA's log for the test suite.
+        # That path is derived from LISA's "--log_path" command line argument and has a value similar to
+        # "<--log_path>/20230217/20230217-040022-342/tests/20230217-040119-288-agent_test_suite"; use the directory
+        # 2 levels up.
+        return Path(variables["log_path"]) if "log_path" in variables else Path(lisa_log_path).parent.parent
+
     @property
     def context(self):
         if self.__context is None:
@@ -287,18 +297,18 @@ def _collect_node_logs(self) -> None:
 
             # Copy the tarball to the local logs directory
             remote_path = "/tmp/waagent-logs.tgz"
-            local_path = Path.home()/'logs'/'{0}.tgz'.format(self.context.image_name)
+            local_path = self.context.log_path/'{0}.tgz'.format(self.context.image_name)
             self._log.info("Copying %s:%s to %s", self.context.node.name, remote_path, local_path)
             self.context.node.shell.copy_back(remote_path, local_path)
         except:  # pylint: disable=bare-except
             self._log.exception("Failed to collect logs from the test machine")
 
     @TestCaseMetadata(description="", priority=0)
-    def agent_test_suite(self, node: Node, variables: Dict[str, Any], log: Logger) -> None:
+    def agent_test_suite(self, node: Node, variables: Dict[str, Any], log_path: str, log: Logger) -> None:
         """
         Executes each of the AgentTests included in the "c_test_suites" variable (which is generated by the AgentTestSuitesCombinator).
         """
-        self._set_context(node, variables, log)
+        self._set_context(node, variables, log_path, log)
 
         test_suite_success = True
 
@@ -343,7 +353,7 @@ def _execute_test_suite(self, suite: TestSuiteInfo) -> bool:
         success: bool = True  # True if all the tests succeed
 
         with _set_thread_name(suite_full_name):  # The thread name is added to self._log
-            with set_current_thread_log(Path.home()/'logs'/f"{suite_full_name}.log"):
+            with set_current_thread_log(self.context.log_path/f"{suite_full_name}.log"):
                 try:
                     agent_test_logger.info("")
                     agent_test_logger.info("**************************************** %s ****************************************", suite_name)

tests_e2e/orchestrator/lib/agent_test_suite_combinator.py

@@ -39,14 +39,15 @@ class AgentTestSuitesCombinatorSchema(schema.Combinator):
 
 class AgentTestSuitesCombinator(Combinator):
     """
-    The "agent_test_suites" combinator returns a list of items containing five variables that specify the environments
+    The "agent_test_suites" combinator returns a list of items containing six variables that specify the environments
     that the agent test suites must be executed on:
 
         * c_marketplace_image: e.g. "Canonical UbuntuServer 18.04-LTS latest",
         * c_location: e.g. "westus2",
         * c_vm_size: e.g. "Standard_D2pls_v5"
         * c_vhd: e.g "https://rhel.blob.core.windows.net/images/RHEL_8_Standard-8.3.202006170423.vhd?se=..."
         * c_test_suites: e.g. [AgentBvt, FastTrack]
+        * c_name: Unique name for the image, e.g. "0001-com-ubuntu-server-focal-20_04-lts-westus2"
 
     (c_marketplace_image, c_location, c_vm_size) and vhd are mutually exclusive and define the environment (i.e. the test VM)
     in which the test will be executed. c_test_suites defines the test suites that should be executed in that
@@ -115,13 +116,18 @@ def create_environment_list(self) -> List[Dict[str, Any]]:
                 images_info = unique_images.values()
 
             for image in images_info:
-                # The URN can be a VHD if the runbook provided a VHD in the 'images' parameter
+                # The URN can actually point to a VHD if the runbook provided a VHD in the 'images' parameter
                 if self._is_vhd(image.urn):
                     marketplace_image = ""
                     vhd = image.urn
+                    name = "vhd"
                 else:
                     marketplace_image = image.urn
                     vhd = ""
+                    match = AgentTestSuitesCombinator._URN.match(image.urn)
+                    if match is None:
+                        raise Exception(f"Invalid URN: {image.urn}")
+                    name = f"{match.group('offer')}-{match.group('sku')}"
 
                 # If the runbook specified a location, use it. Then try the suite location, if any. Otherwise, check if the image specifies
                 # a list of locations and use any of them. If no location is specified so far, use the default.
@@ -150,11 +156,12 @@ def create_environment_list(self) -> List[Dict[str, Any]]:
                         "c_location": location,
                         "c_vm_size": vm_size,
                         "c_vhd": vhd,
-                        "c_test_suites": [suite_info]
+                        "c_test_suites": [suite_info],
+                        "c_name": f"{name}-{suite_info.name}"
                     })
                 else:
                     # add this suite to the shared environments
-                    key: str = f"{image.urn}:{location}"
+                    key: str = f"{name}-{location}"
                     if key in shared_environments:
                         shared_environments[key]["c_test_suites"].append(suite_info)
                     else:
@@ -163,7 +170,8 @@ def create_environment_list(self) -> List[Dict[str, Any]]:
                             "c_location": location,
                             "c_vm_size": vm_size,
                             "c_vhd": vhd,
-                            "c_test_suites": [suite_info]
+                            "c_test_suites": [suite_info],
+                            "c_name": key
                         }
 
         environment_list.extend(shared_environments.values())
@@ -172,16 +180,18 @@ def create_environment_list(self) -> List[Dict[str, Any]]:
         log.info("******** Environments *****")
         for e in environment_list:
             log.info(
-                "{ c_marketplace_image: '%s', c_location: '%s', c_vm_size: '%s', c_vhd: '%s', c_test_suites: '%s' }",
-                e['c_marketplace_image'], e['c_location'], e['c_vm_size'], e['c_vhd'], [s.name for s in e['c_test_suites']])
+                "{ c_marketplace_image: '%s', c_location: '%s', c_vm_size: '%s', c_vhd: '%s', c_test_suites: '%s', c_name: '%s' }",
+                e['c_marketplace_image'], e['c_location'], e['c_vm_size'], e['c_vhd'], [s.name for s in e['c_test_suites']], e['c_name'])
         log.info("***************************")
 
         return environment_list
 
+    _URN = re.compile(r"(?P<publisher>[^\s:]+)[\s:](?P<offer>[^\s:]+)[\s:](?P<sku>[^\s:]+)[\s:](?P<version>[^\s:]+)")
+
     @staticmethod
     def _is_urn(urn: str) -> bool:
         # URNs can be given as '<Publisher> <Offer> <Sku> <Version>' or '<Publisher>:<Offer>:<Sku>:<Version>'
-        return re.match(r"(\S+\s\S+\s\S+\s\S+)|([^:]+:[^:]+:[^:]+:[^:]+)", urn) is not None
+        return AgentTestSuitesCombinator._URN.match(urn) is not None
 
     @staticmethod
     def _is_vhd(vhd: str) -> bool:

tests_e2e/orchestrator/runbook.yml

@@ -29,6 +29,11 @@ variable:
   # NOTE: c_test_suites, generated by the AgentTestSuitesCombinator, is also a parameter
   #       for the AgentTestSuite
   #
+  # Root directory for log files (optional)
+  - name: log_path
+    value: ""
+    is_case_visible: true
+
   # Whether to collect logs from the test VM
   - name: collect_logs
     value: "failed"
@@ -58,9 +63,10 @@ variable:
   # the command line.
   #
   # c_marketplace_image, c_vm_size, c_location, and c_vhd are handled by LISA and define
-  # the set of test VMs that need to be created, while c_test_suites is a parameter
-  # for the AgentTestSuite and defines the test suites that must be executed on each
-  # of those test VMs (the AgentTestSuite also uses c_vhd)
+  # the set of test VMs that need to be created, while c_test_suites and c_name are parameters
+  # for the AgentTestSuite; the former defines the test suites that must be executed on each
+  # of those test VMs and the latter is the name of the environment, which is used for logging
+  # purposes (NOTE: the AgentTestSuite also uses c_vhd).
   #
   - name: c_marketplace_image
     value: ""
@@ -74,6 +80,9 @@ variable:
   - name: c_test_suites
     value: []
     is_case_visible: true
+  - name: c_name
+    value: ""
+    is_case_visible: true
 
   #
   # Set these variables to use an SSH proxy when executing the runbook

tests_e2e/orchestrator/sample_runbooks/existing_vm.yml

@@ -97,6 +97,9 @@ variable:
   - name: c_test_suites
     value: []
     is_case_visible: true
+  - name: c_name
+    value: ""
+    is_case_visible: true
 
   #
   # Set these variables to use an SSH proxy when executing the runbook

tests_e2e/pipeline/scripts/execute_tests.sh

@@ -62,6 +62,7 @@ docker run --rm \
           -v subscription_id:$SUBSCRIPTION_ID \
           -v identity_file:\$HOME/.ssh/id_rsa \
           -v test_suites:\"$TEST_SUITES\" \
+          -v log_path:\$HOME/logs \
           -v collect_logs:\"$COLLECT_LOGS\" \
           -v keep_environment:\"$KEEP_ENVIRONMENT\" \
           -v image:\"$IMAGE\" \
@@ -86,10 +87,13 @@ sudo find "$BUILD_ARTIFACTSTAGINGDIRECTORY" -exec chown "$USER" {} \;
 #    etc
 #
 # Remove the 2 levels of the tree that indicate the time of the test run to make navigation
-# in the Azure Pipelines UI easier.
+# in the Azure Pipelines UI easier. Also, move the lisa log one level up and remove some of
+# the logs that are not needed
 #
 mv "$BUILD_ARTIFACTSTAGINGDIRECTORY"/lisa/[0-9][0-9][0-9][0-9][0-9][0-9][0-9][0-9]/*/* "$BUILD_ARTIFACTSTAGINGDIRECTORY"/lisa
 rm -r "$BUILD_ARTIFACTSTAGINGDIRECTORY"/lisa/[0-9][0-9][0-9][0-9][0-9][0-9][0-9][0-9]
+mv "$BUILD_ARTIFACTSTAGINGDIRECTORY"/lisa/lisa-*.log "$BUILD_ARTIFACTSTAGINGDIRECTORY"
+rm "$BUILD_ARTIFACTSTAGINGDIRECTORY"/lisa/messages.log
 
 cat /tmp/exit.sh
 bash /tmp/exit.sh

tests_e2e/tests/lib/vm_extension.py

@@ -99,9 +99,9 @@ def enable(
                 extension_parameters
             ).result(timeout=_TIMEOUT))
 
-        if result.provisioning_state != 'Succeeded':
+        if result.provisioning_state not in ('Succeeded', 'Updating'):
             raise Exception(f"Enable {self._identifier} failed. Provisioning state: {result.provisioning_state}")
-        log.info("Enable succeeded.")
+        log.info("Enable completed (provisioning state: %s).", result.provisioning_state)
 
     def get_instance_view(self) -> VirtualMachineExtensionInstanceView:  # TODO: Check type for scale sets
         """