add installation truncation #192

feng-j678 · 2023-06-06T17:15:05Z

What is in this PR:
[X] Compressed assessment and installation patches
[X] Add tombstone record for compressed assessment per classifications.
[X] Add tombstone record for compressed installation.
[X] Add error details for compressed assessment.
[X] Add error details for compressed installation.
[X] set status as warning when compression applied.
[X] Clean up multi-line formatting.
[X] Test assessment and installation compression.
[X] Unit test coverage...
[x] delay truncation logic - add condition to check current time vs truncation timestamp if 60 secs have passed. when timestamp is below 60 secs, truncation logic will not apply during application process unless the terminal state has reached (success or error)

…tatus_file_path

…ile_component to load complete status

…into user/johnfeng/add_assessment_truncation

…ounting quotes

… map for performance optimization

…ged_termination

…or performance issue

codecov · 2023-06-06T17:24:03Z

Codecov Report

Merging #192 (424953d) into master (b50ef62) will increase coverage by 0.72%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #192      +/-   ##
==========================================
+ Coverage   90.39%   91.11%   +0.72%     
==========================================
  Files          90       90              
  Lines       14810    15941    +1131     
==========================================
+ Hits        13387    14525    +1138     
+ Misses       1423     1416       -7

Flag	Coverage Δ
python27	`91.11% <100.00%> (+0.72%)`	⬆️
python39	`91.11% <100.00%> (+0.72%)`	⬆️

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

Files	Coverage Δ
src/core/src/CoreMain.py	`97.00% <100.00%> (+0.03%)`	⬆️
src/core/src/bootstrap/Constants.py	`98.71% <100.00%> (+0.05%)`	⬆️
.../core/src/core_logic/ConfigurePatchingProcessor.py	`94.84% <100.00%> (ø)`
src/core/src/service_interfaces/StatusHandler.py	`97.10% <100.00%> (+2.29%)`	⬆️
src/core/tests/Test_CoreMain.py	`99.92% <100.00%> (+0.04%)`	⬆️
src/core/tests/Test_StatusHandler.py	`99.88% <100.00%> (+0.10%)`	⬆️

... and 1 file with indirect coverage changes

…n map in 2.7 are randomize

…f elements

…le/method names

kjohn-msft · 2023-09-05T18:32:52Z

@feng-j678 @rane-rajasi
Where is the conditional writes for performance tracked (write truncated status only once in x minutes)?
I don't see it in the PR description as tracked, and its not superficially clear that it's done.

…nditional checker, and unit test

…into user/johnfeng/add_installation_truncation

feng-j678 · 2023-09-28T20:28:07Z

@feng-j678 @rane-rajasi Where is the conditional writes for performance tracked (write truncated status only once in x minutes)? I don't see it in the PR description as tracked, and its not superficially clear that it's done.

checker condition code added.

…ponent

rane-rajasi

Did a very short pass, so this is not my final review for the current iteration.

Please add a brief description on what this PR is trying to achieve with some details. For eg: how is it trying to truncate, when does it truncate, why is the skip by seconds logic added, etc.,

rane-rajasi · 2023-09-07T16:02:43Z

src/core/src/CoreMain.py

@@ -101,6 +101,7 @@ def __init__(self, argv):
                    patch_installer.mark_installation_completed()
                    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)
+            status_handler.log_truncated_packages()



Note: to be raised and discussed between @kjohn-msft, @feng-j678 and me

rane-rajasi · 2023-09-07T17:01:53Z

src/core/src/service_interfaces/StatusHandler.py

+        self.env_layer.file_system.write_with_retry_using_temp_file(self.complete_status_file_path, '[{0}]'.format(status_file_payload_json_dumps), mode='w+')
+
+        if Constants.StatusTruncationConfig.TURN_ON_TRUNCATION:
+            status_file_payload_json_dumps = self.__check_file_byte_size_for_truncation(status_file_payload_json_dumps)


The function name __check_file_byte_size_for_truncation() indicates it only checks the size, however this function not only checks size but also applies truncation if needed. Please rename the function to convey it's actions

method name updated -. __check_status_file_size_perform_truncation_if_needed(). at this point the method name is getting longer like paragraph. looking for suggestion for shorter name. done

rane-rajasi · 2023-09-29T21:55:54Z

src/core/src/service_interfaces/StatusHandler.py

+        curr_timestamp = datetime.datetime.now()
+        return (curr_timestamp - track_truncation_timestamp).total_seconds() >= Constants.StatusTruncationConfig.SKIP_TRUNCATION_LOGIC_IN_X_SEC
+
+    def __set_force_truncation_true(self, status):


Rename to: __set_force_truncation_true_for_terminal_status()

This function is being called for all status values (Success, Error, Transitioning...), so it's important for the name to reflect the functionality

rane-rajasi · 2023-09-29T22:00:57Z

src/core/src/service_interfaces/StatusHandler.py

-        # Write agent status file
-        self.env_layer.file_system.write_with_retry_using_temp_file(self.status_file_path, '[{0}]'.format(json.dumps(complete_status_payload)), mode='w+')
+        if Constants.StatusTruncationConfig.TURN_ON_TRUNCATION:
+            truncated_status_file_json_dumps = self.__validate_status_file_for_truncation(status_file_payload_json_dumps)


__validate_status_file_for_truncation(): Is this just validating or is this performing truncation also? The name indicates it will simply validate

done, see above comment

rane-rajasi · 2023-09-29T22:07:52Z

src/core/src/service_interfaces/StatusHandler.py

+            track_truncation_timestamp = datetime.datetime.now()
+
+        curr_timestamp = datetime.datetime.now()
+        return (curr_timestamp - track_truncation_timestamp).total_seconds() >= Constants.StatusTruncationConfig.SKIP_TRUNCATION_LOGIC_IN_X_SEC


why is this skip truncation by secs added?

Also, the function name again does not clearly state what is happening within

add description, done.

Not a blocker for the first validation round: But the description, within function or in PR, does not explain "why" this skip truncation logic is added.

Also, based on my assumption, of this logic, truncation should be skipped if the remaining time (i.e. current - track_tuncation) <= Constants.StatusTruncationConfig.SKIP_TRUNCATION_LOGIC_IN_X_SEC. Is this correct?

So the last statement should be:
return (curr_timestamp - track_truncation_timestamp).total_seconds() > Constants.StatusTruncationConfig.SKIP_TRUNCATION_LOGIC_IN_X_SEC

rane-rajasi · 2023-10-19T14:13:09Z

Did a very short pass, so this is not my final review for the current iteration.

Please add a brief description on what this PR is trying to achieve with some details. For eg: how is it trying to truncate, when does it truncate, why is the skip by seconds logic added, etc.,

PR description can be made further descriptive:
for eg: The last point states there is a delay added in the truncation logic, but does not explain why the delay is introduced.

PR description should briefly state the issue we see today and our solution. See the previous comment on "Please add a brief description on what this PR is trying to achieve with some details. For eg: how is it trying to truncate, when does it truncate, why is the skip by seconds logic added, etc.,"

rane-rajasi · 2023-10-19T17:01:44Z

src/core/src/service_interfaces/StatusHandler.py

+        if status_file_size_in_bytes > self.__internal_file_capacity:  # perform truncation complete_status_file byte size > 126kb
+            is_delay_truncation_by_x_sec = self.__check_truncation_time_passed_by_x_sec(self.__track_truncation_timestamp)
+
+            if is_delay_truncation_by_x_sec or self.__force_truncation_on:


Should this condition read as:
If there is no delay required for truncation OR force truncation is on?

If so, the check should be:
if not is_delay_truncation_by_x_sec or self.__force_truncation_on:

This is why a description conveying the intent/ desired functionality of the code is important, either in function description or PR description

Also, the function and variable names need to be modified to reflect the intent, but leaving that for the next review

…_by_x_sec

…rm_truncation_if_needed

rane-rajasi · 2023-10-19T23:55:31Z

src/core/src/service_interfaces/StatusHandler.py

+        # Map['classification', count]
+        for package in packages_removed_from_assessment:
+            classifications = package['classifications'][0]
+            assessment_tombstone_map[classifications] = assessment_tombstone_map.get(classifications, 0) + 1


assessment_tombstone_map.get()

Wouldn't this throw an error on empty list?

done. if that's the case, this code is unreachable:

line 1000
if len(packages_removed_from_assessment) > 0:
self.__assessment_packages_removed = packages_removed_from_assessment
assessment_tombstone_records = self.__create_assessment_tombstone_list(self.__assessment_packages_removed)
packages_retained_in_assessment.extend(assessment_tombstone_records)

rane-rajasi · 2023-10-19T23:58:51Z

src/core/src/service_interfaces/StatusHandler.py

+            assessment_tombstone_map[classifications] = assessment_tombstone_map.get(classifications, 0) + 1
+
+        # Add assessment tombstone record per classifications except unclassified
+        for tombstone_classification, tombstone_package_count in assessment_tombstone_map.items():


Where are things being written into assessment_tombstone_map?
Line 1188 initializes it to an empty value, line 1194 does a .get() on an empty list. Don't see any values being set in it

done. if that's the case, this code is unreachable:

line 1000
if len(packages_removed_from_assessment) > 0:
self.__assessment_packages_removed = packages_removed_from_assessment
assessment_tombstone_records = self.__create_assessment_tombstone_list(self.__assessment_packages_removed)
packages_retained_in_assessment.extend(assessment_tombstone_records)

rane-rajasi · 2023-10-20T00:03:13Z

src/core/src/service_interfaces/StatusHandler.py

+        # Add assessment tombstone record per classifications except unclassified
+        for tombstone_classification, tombstone_package_count in assessment_tombstone_map.items():
+            if not tombstone_classification == Constants.PackageClassification.UNCLASSIFIED:
+                tombstone_record_list.append(self.__create_assessment_tombstone(tombstone_package_count, tombstone_classification))


Are we adding 1 tombstone record for every classification type? I don't remember what was discussed on this previously but I have doubts on the usefulness of this.

done. yes, this was a one of the requirements.

rane-rajasi

Left some comments inline, but my overall review is:

Need a description of the current functionality and why each change is being introduced (i.e. what is the desired outcome)
The changes are there but due to the enormity of this PR and the number of things being added, it is difficult to ensure that all intended changes are implemented and everything works together as desired.
we should consider splitting this PR into parts for easier review.

The parts can be: (1 PR for each)

Basic truncation of assessment and installation patches (Simply removing additional patches with No tombstone or error details). This will also include re-composing the status file post truncation. TBD if this can be further split
Adding tombstone records on truncation
Adding error details on truncation
Adding delay on truncation
Any other changes not covered so far

…UNCATION_IN_X_SEC = -1

feng-j678 added 18 commits May 10, 2023 11:47

add logic to write status complete file

977309e

add logic for latest status complete file

012fb39

add truncation logic

86adb29

change logic to binary search and trying utest

4c182ea

add utest for truncated file, change status_complete*** to complete_s…

089fac6

…tatus_file_path

add unit test to CoreMain for truncation and modify the load_status_f…

6cf7d99

…ile_component to load complete status

add unit test for testing errors for truncation

89f2d92

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

6cdce2b

…into user/johnfeng/add_assessment_truncation

remove json.dumps on message to avoid adding \ and remove logic for c…

83f7d17

…ounting quotes

add logic for quote count after truncation

ece5ad6

modify the quote_count logic and set_package_assessment_status to use…

f213d6f

… map for performance optimization

fix file formating and add more tests for report_sequence_number_chan…

7114d6d

…ged_termination

add more unit test

8242685

remove unused variables in test_status

faa0bbc

add truncation logic for installation

e9800ab

modify the set_package_install_status and classification to use map f…

fdf7d8f

…or performance issue

add unit tests for installation truncation

1555c07

add more unit test for testing installation truncation

45823ac

feng-j678 requested review from kjohn-msft, rane-rajasi and najams as code owners June 6, 2023 17:15

feng-j678 added 2 commits June 6, 2023 11:14

reset installation_tmp_map

3e1cd68

add classification to test_installatio_keep_5

6d6f8fa

feng-j678 force-pushed the user/johnfeng/add_installation_truncation branch from a3e0028 to 6d6f8fa Compare June 6, 2023 20:47

feng-j678 added 5 commits June 6, 2023 19:33

revert set_pacakge_install_status to double forloops because values i…

89e99da

…n map in 2.7 are randomize

change hashmap to orderedmap because existing utest check for order o…

beb92bb

…f elements

add unit test for get latest complete status file

9ea154e

add test for get latest complete status file

e0d19b6

add descriptions to truncation tests

790bc13

reset code changes because branch is ahead of master, refactor variab…

f34c848

…le/method names

feng-j678 force-pushed the user/johnfeng/add_installation_truncation branch from fe0f93f to f34c848 Compare August 29, 2023 19:01

feng-j678 added 4 commits August 29, 2023 12:48

add unit test for validating the log message

63351e2

change write log to tmp and read from it

d775cea

move write to temp log file into a method for unit test

c2fdc5b

refact substatus_msg_at_index to substatus_message

442593c

feng-j678 added 4 commits September 10, 2023 23:58

add FORCE_WRITE_TRUNCATION constant, add check for 1 min timestamp co…

aa7fb97

…nditional checker, and unit test

add unit test for 1 min delay

7120636

add checker for delaying truncation logic and unit test

764b59b

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

cf4b555

…into user/johnfeng/add_installation_truncation

add new comments and reset truncation boolean in load_status_file_com…

51a3d53

…ponent

rane-rajasi requested changes Sep 29, 2023

View reviewed changes

refactor method names

6a92ec0

rane-rajasi reviewed Oct 19, 2023

View reviewed changes

feng-j678 added 2 commits October 19, 2023 10:38

initialize truncation_timestamp inside __check_truncation_time_passed…

831cb70

…_by_x_sec

move self.__is_file_truncated logic to __check_status_file_size_perfo…

2521e8a

…rm_truncation_if_needed

feng-j678 force-pushed the user/johnfeng/add_installation_truncation branch from 5882b0d to 2521e8a Compare October 19, 2023 23:31

rane-rajasi reviewed Oct 19, 2023

View reviewed changes

rane-rajasi reviewed Oct 20, 2023

View reviewed changes

rane-rajasi requested changes Oct 20, 2023

View reviewed changes

feng-j678 added 2 commits October 19, 2023 23:14

extract tombstone logic into a variable

d322898

rename var and method, and set Constants.StatusTruncationConfig.NO_TR…

2d411e1

…UNCATION_IN_X_SEC = -1

feng-j678 force-pushed the user/johnfeng/add_installation_truncation branch from 108eb27 to 2d411e1 Compare October 20, 2023 07:12

change NO_TRUNCATION_IN_X_SEC = -1 to ensure truncation happens

424953d

feng-j678 closed this Mar 20, 2024

feng-j678 deleted the user/johnfeng/add_installation_truncation branch September 9, 2024 23:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add installation truncation #192

add installation truncation #192

feng-j678 commented Jun 6, 2023 •

edited

Loading

codecov bot commented Jun 6, 2023 •

edited

Loading

kjohn-msft commented Sep 5, 2023

feng-j678 commented Sep 28, 2023

rane-rajasi left a comment

rane-rajasi Sep 7, 2023

rane-rajasi Sep 7, 2023

feng-j678 Oct 3, 2023

rane-rajasi Sep 29, 2023

feng-j678 Oct 3, 2023

rane-rajasi Sep 29, 2023

feng-j678 Oct 3, 2023

rane-rajasi Sep 29, 2023

feng-j678 Oct 3, 2023

rane-rajasi Oct 19, 2023

rane-rajasi commented Oct 19, 2023

rane-rajasi Oct 19, 2023

rane-rajasi Oct 19, 2023

feng-j678 Oct 20, 2023 •

edited

Loading

rane-rajasi Oct 19, 2023

feng-j678 Oct 20, 2023 •

edited

Loading

rane-rajasi Oct 20, 2023

feng-j678 Oct 20, 2023

rane-rajasi left a comment

add installation truncation #192

add installation truncation #192

Conversation

feng-j678 commented Jun 6, 2023 • edited Loading

codecov bot commented Jun 6, 2023 • edited Loading

Codecov Report

kjohn-msft commented Sep 5, 2023

feng-j678 commented Sep 28, 2023

rane-rajasi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rane-rajasi commented Oct 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feng-j678 Oct 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feng-j678 Oct 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rane-rajasi left a comment

Choose a reason for hiding this comment

feng-j678 commented Jun 6, 2023 •

edited

Loading

codecov bot commented Jun 6, 2023 •

edited

Loading

feng-j678 Oct 20, 2023 •

edited

Loading

feng-j678 Oct 20, 2023 •

edited

Loading