Calculate maximum batch size in batch patching as per time remaining in maintenance window #250
Conversation
GAURAVRAMRAKHYANI
requested review from
kjohn-msft,
rane-rajasi and
najams
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #250 +/- ##
==========================================
- Coverage 90.95% 83.54% -7.41%
==========================================
Files 91 91
Lines 15723 15749 +26
==========================================
- Hits 14301 13158 -1143
- Misses 1422 2591 +1169
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #250 +/- ##
==========================================
- Coverage 90.98% 90.97% -0.01%
==========================================
Files 91 91
Lines 15737 15799 +62
==========================================
+ Hits 14318 14373 +55
- Misses 1419 1426 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
fixed some issues in code after manual testing. Updated the average time to install package in different package managers based on update in kusto query to get average time.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
get_max_batch_size() looks good.
An observation on the changes in install_updates() in PatchInstaller.py, the function is getting longer, please extract them out to another function in your revised code changes.
Also since get_max_batch_size() is an independent function, we can have UTs to validate it. For any future revisions, please try to add UTs to test out the smaller units. Overall UT to validate E2E scenario can be added in the iteration that has all code changes
(a) There should be two new lines at the end of file (b) Extract some code of install_updates to another function as install_updates was getting longer.
(a) I have addressed the comment that as install_updates is getting longer, extracted some part of it to another function. Did some manual testings and no issues found. Continuing doing more testing around this change. (b) I have not yet implemented UT for get_max_batch_size(), will implement it tomorrow IST. Meanwhile, can you review the product changes? |
There was a problem hiding this comment.
the cicd pipeline is failing, the code coverage does not meet the threshold, and there are failing tests
per Gaurav:
It is expected, the unit tests are not updated, As batch size has changed, the unit tests will fail as they expect older batch size.
I will update unit tests once product changes are reviewed otherwise will have to update the unit tests again if there are changes in product
…l_time_available in test code
…into garamrak-multiphase
The fix is in the function get_max_batch_size, there is difference in behaviour in python2 and python3. In python2, int/int is int but in python3 int/int is float Addressed PR comment by adding the class PackageBatchConfig for batch patching constants
|
Code cov is having a partial service outage. https://status.codecov.com/ |
GAURAVRAMRAKHYANI
dismissed stale reviews from feng-j678 and kjohn-msft
via
a8ad80a
There are four changes at high level:
(a) Calculate maximum batch size in batch patching as per time remaining in maintenance window. The upper limit of max batch size is 300.
(b) If there are some remaining packages after batch patching due to failures in batch patching or maintenance window cutoff exceeded, then do batch patching again with lower batch size (1/10th of original batch size) instead of directly falling back to sequential patching. In case there is again remaining packages after batch patching with lower batch size then do sequential patching.
The code changes to calculate batch size in phase1 and phase2 will be done in later iterations once the code structure is approved.
(c) Use different value for average time to install packages for different package managers as per the telemetry data. Using current average time to install packages, it should improve after this change, and we can update the values later as per the improvement.
(d) In the maintenance window cutoff calculation, assumption of the number of packages in the batch which could take max time to install was 3, changing it to 1. Rest of the packages should take average time to install.
(e) In sequential patching, for the packages which failed to install in first attempt are retried 3 times. So, at max, there are total 4 attempts in sequential patching for a package. Now, with multi-phase batch patching implementation, we have two attempts in batch patching and 4 attempts in sequential patching. So, at max, there are total 6 attempts for a package. These are too many attempts. Each attempt to install package requires running two commands, one to install package and the other to check if package is installed successfully. These commands take up to total 1 second per package. These commands generate some logs which will add in telemetry throttling and slow down. To reduce attempts, removing 3 retries in sequential patching. So, there are total 2 attempts in batch patching and 1 in sequential patching.
Only adding function signatures and their caller from the code in the first iteration. Will implement the functions in later iterations.
Testing:
Manual testing is not done as function implementation is pending.
Test changes will be done once the product changes are approved.
Update:
Function implementation for get_max_batch_size is done.
Completed the manual testings. Fixed some issues in the code changes which were found during unit testing.
Unit tests are not done. Will do it once product code changes are reviewed.
Manual testing done:
(a) Done testing in all the 3 package managers: apt, yum and zypper.
(b) Tested the scenario in which multi-phase batch patching is done in only 1 phase and in 2 phases due to different conditions like remaining packages to install greater than 0 after phase 1 and maintenance window batch cutoff reached with phase 1 so need to do phase2. Also, tested that the packages remaining after phase 2 are attempted in sequential patching.
(c) Done testing for the case when number of packages to install is 0 in the beginning of the patch installation job.