Bug fixes and improvements #70
Merged
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
kjohn-msft
reviewed
Jun 2, 2021
kjohn-msft
reviewed
Jun 2, 2021
kjohn-msft
reviewed
Jun 2, 2021
kjohn-msft
reviewed
Jun 2, 2021
Comment on lines
+124
to
+133
| def update_patch_substatus_if_pending(patch_operation_requested, overall_patch_installation_operation_successful, patch_assessment_successful, status_handler, composite_logger): | ||
| if patch_operation_requested == Constants.INSTALLATION.lower() and not overall_patch_installation_operation_successful: | ||
| if not patch_assessment_successful: | ||
| status_handler.set_current_operation(Constants.INSTALLATION) | ||
| status_handler.add_error_to_status("Installation failed due to assessment failure. Please refer the error details in assessment substatus") | ||
| status_handler.set_installation_substatus_json(status=Constants.STATUS_ERROR) | ||
| composite_logger.log_debug(' -- Persisted failed installation substatus.') | ||
| if not patch_assessment_successful: | ||
| status_handler.set_assessment_substatus_json(status=Constants.STATUS_ERROR) | ||
| composite_logger.log_debug(' -- Persisted failed assessment substatus.') |
There was a problem hiding this comment.
This would have been best encapsulated within the separate assessor and installer classes and invoked as needed. But that can't be done neatly as they may not have been initialized correctly. For completeness of positioning, it shouldn't be inside status handler as it's a level of detail that is not for status handler. It's not for CoreMain as it's usurping some logic that belongs elsewhere.
Maybe not for this PR, but consider what can be done for this.
kjohn-msft
reviewed
Jun 6, 2021
kjohn-msft
approved these changes
Jun 6, 2021
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.
Contains:
Fix for bug where the last status read in CRP for an installation operation has availablePatchSummary marked as 'In Progress'.
This was due to the status for PatchInstallationSummary in status blob. CRP considers this status as the terminal state. So once PatchInstallationSummary is marked as 'success', CRP stops polling substatus and does not wait for the implicit (or 2nd) assessment to complete, if that is pending.
Changed the implementation to only mark PatchInstallationSummary as 'success' once the implicit assessment is completed.
code change includes: removing substatus set to success in PatchInstaller and adding that in CoreMain after implicit assessment has competed
Updated error message text for when Linux agent does not support telemetry, to ensure the error object within status blob includes the complete message
Code change in Constants.py
For an installation operation, adding error details in installation substatus also if assessment fails. The error within installation substatus just mentions that an error occurred due to assessment failure and to refer the assessment substatus for more details.
Code change in CoreMain: Added a new variable to track overall installation substatus (var: overall_patch_installation_operation_successful), which includes implicit assessment, and using this to determine if failure occurred during assessment, in function update_patch_substatus_if_pending()
Fix for adding error details in substatus, when failure occurs during core setup.
Code change in CoreMain: Moved execution config read above any actions and adding all setup errors in assessment substatus. For an installation operation, a generic error ~ install failed due to assessment fail will be added in installation substatus due to the change in point 3 above.