-
Notifications
You must be signed in to change notification settings - Fork 11
Feature: Set Installation Status to Warning when all expected packages are installed #291
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
9a9b48a
b6e7003
091af98
3f77ecd
a9b23f8
68421aa
1f757ef
f4dd3d5
67fb578
4be150a
9dff776
1d830c7
34bd06a
37dd5d5
0fdab66
000dba1
abeb3d2
f2bc6d8
ead5296
7e57abc
cf01d5b
2171a61
1182105
e64f078
c3f80b3
008e919
4a4dcc3
cc28d0e
8922159
e12f2d4
30f867e
7930d79
7a5b139
c57be77
12a1c2b
2de4b07
7d909f0
f9ae07e
48453da
8aaecfa
0a80cc6
ecdfccd
928ab88
48945d1
48826cc
9d10428
00e34fe
00e9cbf
809c337
430d8c8
51ffaa1
2611f98
ca21c3a
963af02
cdacc2b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,7 @@ | |
from core.src.bootstrap.Constants import Constants | ||
from core.src.core_logic.Stopwatch import Stopwatch | ||
|
||
|
||
class PatchInstaller(object): | ||
"""" Wrapper class for a single patch installation operation """ | ||
def __init__(self, env_layer, execution_config, composite_logger, telemetry_writer, status_handler, lifecycle_manager, package_manager, package_filter, maintenance_window, reboot_manager): | ||
|
@@ -119,7 +120,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 | ||
|
@@ -394,16 +395,14 @@ def install_updates(self, maintenance_window, package_manager, simulate=False): | |
def log_final_metrics(self, maintenance_window, patch_installation_successful, maintenance_window_exceeded, installed_update_count): | ||
""" | ||
logs the final metrics. | ||
|
||
Parameters: | ||
maintenance_window (MaintenanceWindow): Maintenance window for the job. | ||
patch_installation_successful (bool): Whether patch installation succeeded. | ||
maintenance_window_exceeded (bool): Whether maintenance window exceeded. | ||
installed_update_count (int): Number of updates installed. | ||
""" | ||
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) | ||
|
||
self.__log_progress_status(maintenance_window, installed_update_count) | ||
|
||
if not patch_installation_successful or maintenance_window_exceeded: | ||
message = "\n\nOperation status was marked as failed because: " | ||
|
@@ -415,7 +414,6 @@ def log_final_metrics(self, maintenance_window, patch_installation_successful, m | |
def include_dependencies(self, package_manager, packages_in_batch, package_versions_in_batch, all_packages, all_package_versions, packages, package_versions, package_and_dependencies, package_and_dependency_versions): | ||
""" | ||
Add dependent packages in the list of packages to install i.e. package_and_dependencies. | ||
|
||
Parameters: | ||
package_manager (PackageManager): Package manager used. | ||
packages_in_batch (List of strings): List of packages to be installed in the current batch. | ||
|
@@ -505,9 +503,7 @@ def batch_patching(self, all_packages, all_package_versions, packages, package_v | |
def install_packages_in_batches(self, all_packages, all_package_versions, packages, package_versions, maintenance_window, package_manager, max_batch_size_for_packages, simulate=False): | ||
""" | ||
Install packages in batches. | ||
|
||
Parameters: | ||
|
||
all_packages (List of strings): List of all available packages to install. | ||
all_package_versions (List of strings): Versions of the packages in the list all_packages. | ||
packages (List of strings): List of all packages selected by user to install. | ||
|
@@ -516,7 +512,6 @@ def install_packages_in_batches(self, all_packages, all_package_versions, packag | |
package_manager (PackageManager): Package manager used. | ||
max_batch_size_for_packages (Integer): Maximum batch size. | ||
simulate (bool): Whether this function is called from a test run. | ||
|
||
Returns: | ||
installed_update_count (int): Number of packages installed through installing packages in batches. | ||
patch_installation_successful (bool): Whether package installation succeeded for all attempted packages. | ||
|
@@ -525,7 +520,6 @@ def install_packages_in_batches(self, all_packages, all_package_versions, packag | |
not_attempted_and_failed_packages (List of strings): List of packages which are (a) Not attempted due to not enough time in maintenance window to install in batch. | ||
(b) Failed to install in batch patching. | ||
not_attempted_and_failed_package_versions (List of strings): Versions of packages in the list not_attempted_and_failed_packages. | ||
|
||
""" | ||
number_of_batches = int(math.ceil(len(packages) / float(max_batch_size_for_packages))) | ||
self.composite_logger.log("\nDividing package install in batches. \nNumber of packages to be installed: " + str(len(packages)) + "\nBatch Size: " + str(max_batch_size_for_packages) + "\nNumber of batches: " + str(number_of_batches)) | ||
|
@@ -679,12 +673,23 @@ def mark_installation_completed(self): | |
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 | ||
self.composite_logger.log_debug("[PI] Reviewing final healthstore record write. [HealthStoreId={0}][MaintenanceRunId={1}]".format(str(self.execution_config.health_store_id), str(self.execution_config.maintenance_run_id))) | ||
if self.execution_config.health_store_id is not None: | ||
self.status_handler.set_patch_metadata_for_healthstore_substatus_json( | ||
patch_version=self.execution_config.health_store_id, | ||
report_to_healthstore=True, | ||
wait_after_update=False) | ||
self.__send_metadata_to_health_store() | ||
|
||
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 customer requested packages are installed. """ | ||
|
||
# setting current operation to ensure correct substatus is updated | ||
self.status_handler.set_current_operation(Constants.INSTALLATION) | ||
|
||
# Update status handler error msg | ||
self.__log_final_warning_metric() | ||
|
||
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 | ||
self.__send_metadata_to_health_store() | ||
|
||
# region Installation Progress support | ||
def perform_status_reconciliation_conditionally(self, package_manager, condition=True): | ||
|
@@ -797,3 +802,31 @@ 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), | ||
"Completed processing packages!") | ||
self.composite_logger.log(progress_status) | ||
|
||
def __send_metadata_to_health_store(self): | ||
rane-rajasi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
""" store patch metadata in status files for health store. """ | ||
self.composite_logger.log_debug("[PI] Reviewing final healthstore record write. [HealthStoreId={0}][MaintenanceRunId={1}]".format(str(self.execution_config.health_store_id), str(self.execution_config.maintenance_run_id))) | ||
if self.execution_config.health_store_id is not None: | ||
self.status_handler.set_patch_metadata_for_healthstore_substatus_json( | ||
patch_version=self.execution_config.health_store_id, | ||
report_to_healthstore=True, | ||
wait_after_update=False) | ||
|
||
# region - Failed packages retry succeeded | ||
def __log_final_warning_metric(self): | ||
""" Log the final metrics for status handler error json for setting installation status to warning. """ | ||
|
||
message = "All requested package(s) are installed. Any patch errors marked are from previous attempts." | ||
self.composite_logger.log(message) | ||
self.status_handler.add_error_to_status(message=message, error_code=Constants.PatchOperationErrorCodes.PACKAGES_RETRY_SUCCEEDED) | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is [current_operation_override_for_error=Constants.INSTALLATION] explicitly passed to status_handler.add_error_to_status()? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. because in coremain.py, we are invoking assessment logic and in assessment first line of code is set operation to assessment. I was trying to avoid making unnecessary code changes to minimize new regressions so i passed in a parm to override the operation then it will update the correct error json. since you raise this up, I've added a minor condition check in patchassessor.py to check for installation operation, so no overriding.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My other comment applies here as well: https://github.com/Azure/LinuxPatchExtension/pull/291/files#r2070283468. set current_operation in mark_installation_completed_with_warning(). Try to use similar code as reference and always test the code for all scenarios when introducing a change in existing code. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
def should_patch_installation_status_be_set_to_warning(self, patch_installation_successful, maintenance_window_exceeded): | ||
""" Check if patch installation status can be set to warning from failed. """ | ||
# type (bool, bool) -> bool | ||
return not patch_installation_successful and not maintenance_window_exceeded and self.status_handler.are_all_requested_packages_installed() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @kjohn-msft I think we also need to check if reboot is pending here. We have an existing function mark_installation_completed() which also sets installation status to warning based on a few conditions, wondering if we should merge this one with that |
||
# endregion | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this the end of this file? If so, make sure to leave 2 empty lines at the end There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 2 empty lines There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -56,6 +56,10 @@ def mock_os_remove(self, file_to_remove): | |
def mock_os_path_exists(self, patch_to_validate): | ||
return False | ||
|
||
def mock_batch_patching_with_packages(self, all_packages, all_package_versions, packages, package_versions, maintenance_window, package_manager): | ||
"""Mock batch_patching to simulate package installation failure, return some packages """ | ||
return packages, package_versions, 0, False | ||
|
||
def test_operation_fail_for_non_autopatching_request(self): | ||
# Test for non auto patching request | ||
argument_composer = ArgumentComposer() | ||
|
@@ -1275,6 +1279,36 @@ def test_delete_temp_folder_contents_failure(self): | |
os.remove = self.backup_os_remove | ||
runtime.stop() | ||
|
||
def test_warning_status_when_packages_initially_fail_but_succeed_on_retry(self): | ||
""" | ||
Tests installation status set warning when: | ||
1. Packages initially fail installation | ||
2. Package manager indicates retry is needed | ||
3. On retry, all supposed packages are installed successfully | ||
4. Batch_patching returns some packages | ||
""" | ||
argument_composer = ArgumentComposer() | ||
argument_composer.operation = Constants.INSTALLATION | ||
runtime = RuntimeCompositor(argument_composer.get_composed_arguments(), True, Constants.ZYPPER) | ||
|
||
runtime.set_legacy_test_type('PackageRetrySuccessPath') | ||
|
||
# Store original methods | ||
original_batch_patching = runtime.patch_installer.batch_patching | ||
|
||
# Mock batch_patching with packages to return false | ||
runtime.patch_installer.batch_patching = self.mock_batch_patching_with_packages | ||
|
||
# Run CoreMain to execute the installation | ||
try: | ||
CoreMain(argument_composer.get_composed_arguments()) | ||
self.__assertion_pkg_succeed_on_retry(runtime) | ||
|
||
finally: | ||
# reset mock | ||
runtime.patch_installer.batch_patching = original_batch_patching | ||
runtime.stop() | ||
|
||
def __check_telemetry_events(self, runtime): | ||
all_events = os.listdir(runtime.telemetry_writer.events_folder_path) | ||
self.assertTrue(len(all_events) > 0) | ||
|
@@ -1285,6 +1319,27 @@ def __check_telemetry_events(self, runtime): | |
self.assertTrue('Core' in events[0]['TaskName']) | ||
f.close() | ||
|
||
def __assertion_pkg_succeed_on_retry(self, runtime): | ||
# Check telemetry events | ||
self.__check_telemetry_events(runtime) | ||
|
||
# Check status file | ||
with runtime.env_layer.file_system.open(runtime.execution_config.status_file_path, 'r') as file_handle: | ||
substatus_file_data = json.load(file_handle)[0]["status"]["substatus"] | ||
|
||
self.assertEqual(len(substatus_file_data), 3) | ||
self.assertTrue(substatus_file_data[0]["name"] == Constants.PATCH_ASSESSMENT_SUMMARY) | ||
self.assertTrue(substatus_file_data[0]["status"].lower() == Constants.STATUS_SUCCESS.lower()) | ||
|
||
# Check installation status is WARNING | ||
self.assertTrue(substatus_file_data[1]["name"] == Constants.PATCH_INSTALLATION_SUMMARY) | ||
self.assertTrue(substatus_file_data[1]["status"].lower() == Constants.STATUS_WARNING.lower()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add a Installed patches and/or pending patches assertion here if possible. i.e. say, Pending patch count == 0 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
self.assertEqual(json.loads(substatus_file_data[1]["formattedMessage"]["message"])["pendingPatchCount"], 0) | ||
|
||
# Verify at least one error detail about package retry | ||
error_details = json.loads(substatus_file_data[1]["formattedMessage"]["message"])["errors"]["details"] | ||
self.assertTrue(any("package(s) are installed" in detail["message"] for detail in error_details)) | ||
|
||
|
||
if __name__ == '__main__': | ||
unittest.main() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refer the function mark_installation_completed()
Incase you want to ensure PatchInstallationSummary substatus is updated by this function, you need to set current_operation to Installation here, instead of changing existing code in PatchAssessor.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is done