picks the random time for attempting new update #3275
Conversation
nagworld9
requested review from
narrieta,
ZhidongPeng and
maddieford
as code owners
nagworld9
changed the title
nagworld9
force-pushed
the
upgrade-attempt
branch
from
b847a3d to
ea43990
Compare
nagworld9
force-pushed
the
upgrade-attempt
branch
from
ea43990 to
eb59fe7
Compare
nagworld9
changed the title
| This method called when new update detected and computes random time for next update. | ||
| If the current time on or after upgrade time, we allow the update. | ||
|
|
||
| Note: After we allow the update, and it's not successful, the next update time will be recalculated. |
There was a problem hiding this comment.
could you add more details to this comment? not sure I understand what is trying to point out. thanks
| if next_update_time <= now: | ||
| # Update the last upgrade check time even if no new agent is available for upgrade | ||
| self._last_attempted_self_update_time = now | ||
| if not self._update_time_refreshed: |
There was a problem hiding this comment.
could you add some comments about the intent of self._update_time_refreshed? thanks
There was a problem hiding this comment.
actually, after review, I realized I could achieve same thing without _update_time_refreshed flag. Added more details in the code comments
| @@ -214,7 +214,8 @@ def run(self, goal_state, ext_gs_updated): | |||
|
|
|||
| # Always agent uses self-update for initial update regardless vm enrolled into RSM or not | |||
| # So ignoring the check for updater switch for the initial goal state/update | |||
| if not self._is_initial_update(): | |||
| initial_update = self._is_initial_update() | |||
There was a problem hiding this comment.
you should make _is_initial_update a public static method and call it where you need it, instead of passing the value as argument
There was a problem hiding this comment.
changing like that creating circular dependency issue
There was a problem hiding this comment.
you could refactor it out to a utilities class
| """ | ||
| Get the next upgrade time | ||
| Returns random time in between 0 to 24hrs(regular) or 6hrs(hotfix) from now |
There was a problem hiding this comment.
It looks like the hotfix default is 4 hours, not 6
| if next_update_time <= now: | ||
| # Update the last upgrade check time even if no new agent is available for upgrade | ||
| self._last_attempted_self_update_time = now | ||
| if self._next_update_time == datetime.datetime.min: |
There was a problem hiding this comment.
This may need another update. One scenario that comes to mind is when we release a version, the agent detects the update, and picks the next_update_time. Before the update is attempted, the version is removed from PIR due to it being a bad version. When we release a new version again, the current logic does not pick a new update time and instead assumes the previous update time. I think we need to pick the update time per version. What do you guys think?
There was a problem hiding this comment.
yes, i was also thinking about similar scenarios and, since the goal is to spread updates over time, rather than updating at a specific time or updating to a specific version, my take is that the current approach is ok
Description
Today we count self-update time 6/24 hours period as a sliding window from service start. This strategy may not be good for vmss, for example. All the vms in vmss may get update around same time
New strategy picks the random time[0..6/24] when we detect the new update
Issue #
PR information
developbranch.Quality of Code and Contribution Guidelines