-
Notifications
You must be signed in to change notification settings - Fork 167
Refactoring cloud compute instance creation with external state of banned zones. #9115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| // Map from zone to DateTime when zone is allowed again | ||
| final zoneBannedUntil = <String, DateTime>{ | ||
| for (final zone in compute.zones) zone: DateTime(0), | ||
| ...state.zoneBannedUntil, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's remove the old bans that are too old, so zoneBannedUntil can't grow indefinitely, should compute zones over time be large.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: the current code relies on the zoneBannedUntil to contain all available zones. I think it is better to rely on the zones list directly, will refactor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, will do this in a follow-up
| // A zone being exhausted is normal operations, we just use another | ||
| // zone for 15 minutes. | ||
| _log.info( | ||
| 'zone resources exhausted, banning ${e.zone} for 30 minutes', | ||
| e, | ||
| st, | ||
| ); | ||
| // Ban usage of zone for 30 minutes | ||
| banZone(e.zone, minutes: 30); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
15 or 30 minutes, did we change this recently.
Code and comments are out of sync.
Co-authored-by: Jonas Finnemann Jensen <[email protected]>
Co-authored-by: Jonas Finnemann Jensen <[email protected]>
No description provided.