PR feedback explanation

Author: rane-rajasi

src/core/src/CoreMain.py

@@ -93,15 +93,15 @@ def __init__(self, argv):
                 # setting current operation here, to include patch_installer init within installation actions, ensuring any exceptions during patch_installer init are added in installation summary errors object
                 status_handler.set_current_operation(Constants.INSTALLATION)
                 patch_installer = container.get('patch_installer')
-                patch_installation_successful = patch_installer.start_installation()
+                patch_installation_successful, maintenance_window_exceeded = patch_installer.start_installation()
                 patch_assessment_successful = False
                 patch_assessment_successful = patch_assessor.start_assessment()
 
                 # PatchInstallationSummary to be marked as completed successfully only after the implicit (i.e. 2nd) assessment is completed, as per CRP's restrictions
                 if patch_assessment_successful and patch_installation_successful:
                     patch_installer.mark_installation_completed()
                     overall_patch_installation_operation_successful = True
-                elif patch_installer.set_patch_installation_status_to_warning_from_failed():
+                elif patch_installer.should_patch_installation_status_be_set_to_warning(patch_installation_successful, maintenance_window_exceeded):
                     patch_installer.mark_installation_completed_with_warning()
                     overall_patch_installation_operation_successful = True
                 self.update_patch_substatus_if_pending(patch_operation_requested, overall_patch_installation_operation_successful, patch_assessment_successful, configure_patching_successful, status_handler, composite_logger)

src/core/src/core_logic/PatchInstaller.py

@@ -53,8 +53,6 @@ def __init__(self, env_layer, execution_config, composite_logger, telemetry_writ
 
         self.stopwatch = Stopwatch(self.env_layer, self.telemetry_writer, self.composite_logger)
 
-        self.__enable_installation_status_to_warning_flag = False
-
     def start_installation(self, simulate=False):
         """ Kick off a patch installation run """
         self.status_handler.set_current_operation(Constants.INSTALLATION)
@@ -126,7 +124,7 @@ def start_installation(self, simulate=False):
         overall_patch_installation_successful = bool(update_run_successful and not maintenance_window_exceeded)
         # NOTE: Not updating installation substatus at this point because we need to wait for the implicit/second assessment to complete first, as per CRP's instructions
 
-        return overall_patch_installation_successful
+        return overall_patch_installation_successful, maintenance_window_exceeded
 
     def write_installer_perf_logs(self, patch_operation_successful, installed_patch_count, retry_count, maintenance_window, maintenance_window_exceeded, task_status, error_msg):
         perc_maintenance_window_used = -1
@@ -284,7 +282,7 @@ def install_updates(self, maintenance_window, package_manager, simulate=False):
         successful_parent_package_install_count_in_batch_patching = self.successful_parent_package_install_count
 
         if len(packages) == 0:
-            self.log_final_installation_metric(patch_installation_successful, maintenance_window_exceeded, maintenance_window, installed_update_count)
+            self.log_final_metrics(maintenance_window, patch_installation_successful, maintenance_window_exceeded, installed_update_count)
             return installed_update_count, patch_installation_successful, maintenance_window_exceeded
         else:
             progress_status = self.progress_template.format(str(datetime.timedelta(minutes=maintenance_window.get_remaining_time_in_minutes())), str(self.attempted_parent_package_install_count), str(self.successful_parent_package_install_count), str(self.failed_parent_package_install_count), str(installed_update_count - self.successful_parent_package_install_count),
@@ -383,7 +381,7 @@ def install_updates(self, maintenance_window, package_manager, simulate=False):
         self.composite_logger.log_debug("\nPerforming final system state reconciliation...")
         installed_update_count += self.perform_status_reconciliation_conditionally(package_manager, True)
 
-        self.log_final_installation_metric(patch_installation_successful, maintenance_window_exceeded, maintenance_window, installed_update_count)
+        self.log_final_metrics(maintenance_window, patch_installation_successful, maintenance_window_exceeded, installed_update_count)
 
         install_update_count_in_sequential_patching = installed_update_count - install_update_count_in_batch_patching
         attempted_parent_package_install_count_in_sequential_patching = self.attempted_parent_package_install_count - attempted_parent_package_install_count_in_batch_patching
@@ -420,7 +418,6 @@ def log_final_metrics(self, maintenance_window, patch_installation_successful, m
 
     def log_final_warning_metric(self, maintenance_window, installed_update_count):
         """ Log the final metrics for warning installation status. """
-
         self.__log_progress_status(maintenance_window, installed_update_count)
         message = "All requested package(s) are installed. Any patch errors marked are from previous attempts."
         self.status_handler.add_error_to_status(message, Constants.PatchOperationErrorCodes.PACKAGES_RETRY_SUCCEEDED)
@@ -692,9 +689,12 @@ def mark_installation_completed(self):
 
     def mark_installation_completed_with_warning(self):
         """ Marks Installation operation as warning by updating the status of PatchInstallationSummary as warning and patch metadata to be sent to healthstore.
-        This is set outside of start_installation function to a restriction in CRP, where installation substatus should be marked as warning only after the implicit (2nd) assessment operation
-        and all supposed packages are installed as expected. """
+        This is set outside of start_installation function due to a restriction in CRP, where installation substatus should be marked as warning only after the implicit (2nd) assessment operation
+        and all customer requested packages are installed. """
 
+        message = "All requested package(s) are installed. Any patch errors marked are from previous attempts."
+        self.composite_logger.log_error(message)
+        self.status_handler.add_error_to_status(message, Constants.PatchOperationErrorCodes.PACKAGES_RETRY_SUCCEEDED)
         self.status_handler.set_installation_substatus_json(status=Constants.STATUS_WARNING)
 
         # Update patch metadata in status for auto patching request, to be reported to healthStore
@@ -812,7 +812,11 @@ def get_max_batch_size(self, maintenance_window, package_manager):
         return max_batch_size_for_packages
 
     def __log_progress_status(self, maintenance_window, installed_update_count):
-        progress_status = self.progress_template.format(str(datetime.timedelta(minutes=maintenance_window.get_remaining_time_in_minutes())), str(self.attempted_parent_package_install_count), str(self.successful_parent_package_install_count), str(self.failed_parent_package_install_count), str(installed_update_count - self.successful_parent_package_install_count),
+        progress_status = self.progress_template.format(str(datetime.timedelta(minutes=maintenance_window.get_remaining_time_in_minutes())),
+                                                        str(self.attempted_parent_package_install_count),
+                                                        str(self.successful_parent_package_install_count),
+                                                        str(self.failed_parent_package_install_count),
+                                                        str(installed_update_count - self.successful_parent_package_install_count),
                                                         "Completed processing packages!")
         self.composite_logger.log(progress_status)
 
@@ -824,22 +828,5 @@ def __send_metadata_to_health_store(self):
                 report_to_healthstore=True,
                 wait_after_update=False)
 
-    # region - Failed packages retry succeeded
-    def log_final_installation_metric(self, patch_installation_successful, maintenance_window_exceeded, maintenance_window, installed_update_count):
-        """ log final installation operation status. """
-        warning_status = self.__check_installation_status_can_set_to_warning(patch_installation_successful, maintenance_window_exceeded)
-        if warning_status:
-            self.log_final_warning_metric(maintenance_window, installed_update_count)
-        else:
-            self.log_final_metrics(maintenance_window, patch_installation_successful, maintenance_window_exceeded, installed_update_count)
-
-    def __check_installation_status_can_set_to_warning(self, patch_installation_successful, maintenance_window_exceeded):
-        """ Verify patch installation status can be set to warning from failed. """
-        # type (bool, bool) -> bool
-        self.__enable_installation_status_to_warning_flag = not patch_installation_successful and not maintenance_window_exceeded and self.status_handler.check_all_requested_packages_install_state()
-        return self.__enable_installation_status_to_warning_flag
-
-    def set_patch_installation_status_to_warning_from_failed(self):
-        """Access enable_installation_warning_status value"""
-        return self.__enable_installation_status_to_warning_flag
-    # endregion
+    def should_patch_installation_status_be_set_to_warning(self, patch_installation_successful, maintenance_window_exceeded):
+        return not patch_installation_successful and not maintenance_window_exceeded and self.status_handler.are_all_requested_packgaes_installed()

src/core/src/service_interfaces/StatusHandler.py

@@ -263,7 +263,7 @@ def get_os_name_and_version(self):
             self.composite_logger.log_error("Unable to determine platform information: {0}".format(repr(error)))
             return "unknownDist_unknownVer"
 
-    def check_all_requested_packages_install_state(self):
+    def are_all_requested_packgaes_installed(self):
         # type (none) -> bool
         """ Check if all requested package(s) are installed. """
 

src/core/tests/Test_PatchInstaller.py

@@ -241,7 +241,8 @@ def test_patch_installer_for_azgps_coordinated(self):
         argument_composer.maximum_duration = "PT30M"
         runtime = RuntimeCompositor(argument_composer.get_composed_arguments(), True, Constants.APT)
         runtime.set_legacy_test_type('HappyPath')
-        self.assertFalse(runtime.patch_installer.start_installation())                 # failure is in unrelated patch installation batch processing
+        patch_installation_successful, maintenance_window_exceeded = runtime.patch_installer.start_installation()
+        self.assertFalse(patch_installation_successful)                 # failure is in unrelated patch installation batch processing
         self.assertEqual(runtime.execution_config.max_patch_publish_date, "20240401T000000Z")
         self.assertEqual(runtime.package_manager.max_patch_publish_date, "")    # reason: not enough time to use
 
@@ -253,21 +254,24 @@ def test_patch_installer_for_azgps_coordinated(self):
         runtime = RuntimeCompositor(argument_composer.get_composed_arguments(), True, Constants.APT)
         runtime.set_legacy_test_type('HappyPath')
         runtime.package_manager.install_security_updates_azgps_coordinated = lambda: (1, "Failed")
-        self.assertFalse(runtime.patch_installer.start_installation())
+        patch_installation_successful, maintenance_window_exceeded = runtime.patch_installer.start_installation()
+        self.assertFalse(patch_installation_successful)
         self.assertEqual(runtime.execution_config.max_patch_publish_date, "20240401T000000Z")
         self.assertEqual(runtime.package_manager.max_patch_publish_date, "")    # reason: the strict SDP is forced to fail with the lambda above
         runtime.stop()
 
         runtime = RuntimeCompositor(argument_composer.get_composed_arguments(), True, Constants.YUM)
         runtime.set_legacy_test_type('HappyPath')
-        self.assertTrue(runtime.patch_installer.start_installation())
+        patch_installation_successful, maintenance_window_exceeded = runtime.patch_installer.start_installation()
+        self.assertTrue(patch_installation_successful)
         self.assertEqual(runtime.execution_config.max_patch_publish_date, "20240401T000000Z")
         self.assertEqual(runtime.package_manager.max_patch_publish_date, "")    # unsupported in Yum
         runtime.stop()
 
         runtime = RuntimeCompositor(argument_composer.get_composed_arguments(), True, Constants.ZYPPER)
         runtime.set_legacy_test_type('HappyPath')
-        self.assertFalse(runtime.patch_installer.start_installation())                 # failure is in unrelated patch installation batch processing
+        patch_installation_successful, maintenance_window_exceeded = runtime.patch_installer.start_installation()
+        self.assertFalse(patch_installation_successful)                 # failure is in unrelated patch installation batch processing
         self.assertEqual(runtime.execution_config.max_patch_publish_date, "20240401T000000Z")
         self.assertEqual(runtime.package_manager.max_patch_publish_date, "")    # unsupported in Zypper
         runtime.stop()