Fix for ConfigurePatching getting stuck in transitioning #154
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
benank
requested review from
a team,
kjohn-msft and
rane-rajasi
and removed request for
a team
Codecov Report
@@ Coverage Diff @@
## master #154 +/- ##
==========================================
+ Coverage 87.28% 87.42% +0.13%
==========================================
Files 48 48
Lines 8324 8326 +2
==========================================
+ Hits 7266 7279 +13
+ Misses 1058 1047 -11
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
mirichmo
reviewed
Jul 14, 2022
| # Signalling take-over of core state by auto-assessment after safety checks for any competing process | ||
| self.update_core_sequence(completed=False) | ||
| # Refresh status file in memory to be up-to-date | ||
| self.status_handler.load_status_file_components() |
There was a problem hiding this comment.
- Should this be done on line 120 so it is done for both cases?
- Should it be done in all code paths within update_core_sequence? Update_core_sequence has an early return.
- What about all invocations of update_core_sequence? Should it be done there as well?
There was a problem hiding this comment.
- The second case should already have up-to-date status file information because there isn't any delay there. It isn't needed on line 120 but can be moved there.
- Core sequence is read at the beginning of each loop within the
execution_start_checkmethod, so it should always have up-to-date information. - I don't think this is needed. It's only needed in areas of contention when there are multiple core processes running at the same time, and this is already taken care of (as mentioned above).
mirichmo
approved these changes
Jul 15, 2022
rane-rajasi
reviewed
Jul 20, 2022
| # Signalling take-over of core state by auto-assessment after safety checks for any competing process | ||
| self.update_core_sequence(completed=False) | ||
| # Refresh status file in memory to be up-to-date | ||
| self.status_handler.load_status_file_components() |
There was a problem hiding this comment.
- Test this change on a machine with Auto Patching and Auto Assessment
- We don't have any tests mocking Auto assessment behavior, to test this scenario. See if it's possible to add that
There was a problem hiding this comment.
- Tested on a machine with those attributes, works as intended.
- Looking into it.
There was a problem hiding this comment.
When testing on a machine, check/follow these:
- Create a VM with Auto Patching and Auto Assessment i.e. patchmode = AutomaticByPlatform and assessmentmode = AutomaticByPlatform
- Check if ConfigurePatching call is triggered. If not, manually invoke ConfigurePatching API. This first ConfigurePatching call should timeout.
- Update the code.
- Call ConfigurePatching again.
- Keep on monitoring the API result. Query ContextActivity for the second and check ConfigurePatchingSummary, it should report status as Success. This is the bug fix
- This part is optional and not a validation for the bug fix. In case you want to verify whether Auto Assessment completed, wait for sometime and then check auto assessment log
There was a problem hiding this comment.
Add a task in our backlog for AutoAssessment tests
rane-rajasi
approved these changes
Jul 25, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix for ConfigurePatching getting stuck in transitioning. ConfigurePatching would run, enable auto-assessment, and auto assessment would start but load the status file from disk. After ConfigurePatching completes, auto assessment would run but would not have updated status file information since it did not read it from disk again after ConfigurePatching updated if when it finished its operation.