Feature: Set Installation Status to Warning when all expected packages are installed by feng-j678 · Pull Request #291 · Azure/LinuxPatchExtension

feng-j678 · 2025-02-26T22:10:19Z

[x] feature 26928297
[x] Currently when security/critical package(s) installation is failed, the installation operation will mark as an error even after retry package is succeeded.
[x] new code changes for first time when security/critical package(s) installation failed, but after retry if all package(s) are installed as planned, mark installation operation to Warning and add details in error object that "all supposed package(s) are installed"

![image](https://github.com/user-attachments/assets/d5d60969-96de-4c15-9e70-d30642e61d8b)

High-level PR comments by @rane-rajasi were in this 'comments PR' - leaving link: #314 (this will be closed)

codecov · 2025-02-26T22:12:41Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.75%. Comparing base (cfdca4c) to head (cdacc2b).
⚠️ Report is 11 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #291      +/-   ##
==========================================
+ Coverage   93.73%   93.75%   +0.02%     
==========================================
  Files         103      103              
  Lines       17881    17961      +80     
==========================================
+ Hits        16760    16840      +80     
  Misses       1121     1121

Flag	Coverage Δ
python27	`93.75% <100.00%> (+0.02%)`	⬆️
python312	`93.75% <100.00%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

kjohn-msft · 2025-03-11T22:58:23Z

Note left offline to clean up miscellaneous noisy line changes to make the PR easier to review.

kjohn-msft

See comment history - please remove misc noisy lines (whitespace changes and non-functional changes) to make this easier to review.

rane-rajasi

Please add a description of the current scenario and what this change is trying to achieve,

…into feature/pkg_failed_retry_set_status_warning

…/github.com/Azure/LinuxPatchExtension into feature/pkg_failed_retry_set_status_warning pull master changes

src/core/src/core_logic/PatchInstaller.py

rane-rajasi

See this PR commit: f9ae07e
as reference for what is expected out of this feature.

NOTE: The new UTs you've added are failing due to legacy code from your changes that weren't carried over to this one, please look into those.
I believe we can also avoid returning maintenance_window_exceeded from start_installation() but that would require a lot of existing code changes which again shouldn't happen in here.

…/github.com/Azure/LinuxPatchExtension into feature/pkg_failed_retry_set_status_warning pull master changes

…into feature/pkg_failed_retry_set_status_warning pull master changes.

src/core/src/core_logic/PatchInstaller.py

rane-rajasi · 2025-04-29T14:15:35Z

src/core/src/core_logic/PatchInstaller.py

+        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, current_operation_override_for_error=Constants.INSTALLATION)
+


Why is [current_operation_override_for_error=Constants.INSTALLATION] explicitly passed to status_handler.add_error_to_status()?

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.

self.status_handler.set_current_operation(Constants.ASSESSMENT)

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.

not rlly liking the idea setting operation back and forth when we can pass parameter(current_operation_override_for_error) in the function to correctly update the substatus error, if this is strongly against then we should remove that parameter

rane-rajasi · 2025-04-29T14:16:37Z

src/core/src/core_logic/PatchInstaller.py

+        """ 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()
+    # endregion


Is this the end of this file? If so, make sure to leave 2 empty lines at the end

2 empty lines

not sure why, but there are 2 empty lines

rane-rajasi

Minor comments inline

…into feature/pkg_failed_retry_set_status_warning pull master changes

rane-rajasi · 2025-05-01T13:30:53Z

src/core/src/core_logic/PatchAssessor.py

Why is this change necessary? This is a major breaking change, current operation needs to be set to whichever sub operation (i.e. assessment, installation, configure patching) is in progress for the code to be able to update the correct substatus

See my other comment for what needs to change: https://github.com/Azure/LinuxPatchExtension/pull/291/files#r2070283468

change is corrected

rane-rajasi · 2025-05-01T13:42:04Z

src/core/src/core_logic/PatchInstaller.py

+        """ 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. """
+


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

this is done

rane-rajasi · 2025-05-01T14:00:22Z

src/core/tests/Test_CoreMain.py

+
+        # 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())


Add a Installed patches and/or pending patches assertion here if possible. i.e. say, Pending patch count == 0

rane-rajasi · 2025-05-01T14:10:29Z

src/core/src/core_logic/PatchInstaller.py

+    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()


@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

rane-rajasi

5 open comments inline (2 from previous review) and 1 comment open for discussion

…into feature/pkg_failed_retry_set_status_warning pull master changes

kjohn-msft · 2025-06-06T16:28:15Z

High-level PR comments by @rane-rajasi were in this 'comments PR' - leaving link: #314 (this will be closed)

feng-j678 added 4 commits February 23, 2025 23:36

add logic to set installation warning when all pkgs are installed

9a9b48a

add print in statushandler to help with debug

b6e7003

add print in patchinstaller

091af98

add unit test to test install successful on retry

3f77ecd

feng-j678 added 2 commits February 26, 2025 14:19

remove print, revert linebreak

a9b23f8

correct spelling

68421aa

feng-j678 marked this pull request as ready for review February 26, 2025 22:39

feng-j678 requested review from kjohn-msft, najams and rane-rajasi as code owners February 26, 2025 22:39

nikhim-um assigned nikhim-um and feng-j678 and unassigned nikhim-um and feng-j678 Feb 27, 2025

nikhim-um self-requested a review February 27, 2025 07:04

remove comments

1f757ef

feng-j678 force-pushed the feature/pkg_failed_retry_set_status_warning branch from e6a78e3 to 1f757ef Compare February 27, 2025 07:12

kjohn-msft requested changes Mar 11, 2025

View reviewed changes

rane-rajasi requested changes Mar 12, 2025

View reviewed changes

feng-j678 added 3 commits March 12, 2025 22:56

Merge branch 'master' of https://github.com/Azure/LinuxPatchExtension …

f4dd3d5

…into feature/pkg_failed_retry_set_status_warning

Merge branch 'feature/pkg_failed_retry_set_status_warning' of https:/…

67fb578

…/github.com/Azure/LinuxPatchExtension into feature/pkg_failed_retry_set_status_warning pull master changes

revert format changes

4be150a

kjohn-msft added the feature New feature or request label Mar 14, 2025

kjohn-msft requested a review from michellemcdaniel March 14, 2025 22:59

michellemcdaniel reviewed Mar 17, 2025

View reviewed changes

src/core/src/core_logic/PatchInstaller.py Outdated Show resolved Hide resolved

rane-rajasi requested changes Apr 18, 2025

View reviewed changes

feng-j678 added 2 commits April 21, 2025 12:15

refactor are_all_requested_packages_installed

8aaecfa

Merge branch 'feature/pkg_failed_retry_set_status_warning' of https:/…

0a80cc6

…/github.com/Azure/LinuxPatchExtension into feature/pkg_failed_retry_set_status_warning pull master changes

feng-j678 force-pushed the feature/pkg_failed_retry_set_status_warning branch from 8cc2f48 to 0a80cc6 Compare April 21, 2025 20:48

feng-j678 added 6 commits April 21, 2025 14:19

refactor code

ecdfccd

update new UTs

928ab88

merge branch 'master' of https://github.com/Azure/LinuxPatchExtension …

48945d1

…into feature/pkg_failed_retry_set_status_warning pull master changes.

add pr 314 changes

48826cc

remove unused variable

9d10428

merge branch 'master' of https://github.com/Azure/LinuxPatchExtension …

00e34fe

…into feature/pkg_failed_retry_set_status_warning pull master changes.

rane-rajasi reviewed Apr 29, 2025

View reviewed changes

src/core/src/core_logic/PatchInstaller.py Outdated Show resolved Hide resolved

rane-rajasi reviewed Apr 29, 2025

View reviewed changes

src/core/src/core_logic/PatchInstaller.py Show resolved Hide resolved

rane-rajasi reviewed Apr 29, 2025

View reviewed changes

rane-rajasi requested changes Apr 29, 2025

View reviewed changes

feng-j678 added 5 commits April 29, 2025 14:29

add condition to prevent operation override

00e9cbf

Merge branch 'master' of https://github.com/Azure/LinuxPatchExtension …

809c337

…into feature/pkg_failed_retry_set_status_warning pull master changes

undo line breaks

430d8c8

add extra line

51ffaa1

remove line break

2611f98

feng-j678 force-pushed the feature/pkg_failed_retry_set_status_warning branch from a243c5c to 2611f98 Compare April 29, 2025 22:06

rane-rajasi reviewed May 1, 2025

View reviewed changes

rane-rajasi requested changes May 1, 2025

View reviewed changes

feng-j678 added 3 commits May 2, 2025 11:21

revert assessor and add set_current_operation

ca21c3a

add assertion for pendingpatchcount

963af02

Merge branch 'master' of https://github.com/Azure/LinuxPatchExtension …

cdacc2b

…into feature/pkg_failed_retry_set_status_warning pull master changes

Conversation

feng-j678 commented Feb 26, 2025 • edited by kjohn-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Feb 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kjohn-msft commented Mar 11, 2025

Uh oh!

kjohn-msft left a comment

Choose a reason for hiding this comment

Uh oh!

rane-rajasi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rane-rajasi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

feng-j678 May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rane-rajasi left a comment

Choose a reason for hiding this comment

Uh oh!

rane-rajasi May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rane-rajasi left a comment

Choose a reason for hiding this comment

Uh oh!

kjohn-msft commented Jun 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Comments

feng-j678 commented Feb 26, 2025 •

edited by kjohn-msft

Loading

codecov bot commented Feb 26, 2025 •

edited

Loading

rane-rajasi left a comment •

edited

Loading

feng-j678 May 2, 2025 •

edited

Loading

rane-rajasi May 1, 2025 •

edited

Loading