-
Notifications
You must be signed in to change notification settings - Fork 12
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
Add autoscaling agent rest api #130
Conversation
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.
Some comments!
skynet/agent.py
Outdated
|
||
log = get_logger(__name__) | ||
app = FastAPI() |
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.
Since we are mounting a new app, I reckon we'd want to contain it in its own file perhaps, rather than together with this agent code?
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.
They need to communicate with one another (that was initially the reason for the temp file as they were in separate modules). I dropped the temp file as they can share a global. If the autoscaler sets the app's state to drain, that state needs to be reflected in the HAProxy agent response. So the LB can stop sending new connections to this machine. No need to stop accepting connections from Skynet itself as HAProxy will handle it.
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.
I also addressed the rest of the concerns, please have another look
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.
LGTM! Left 2 non-blocking questions, feel free to land it as-is.
except Exception as ex: | ||
log.warning('TCP exception during haproxy-agent check.', ex) | ||
finally: | ||
await writer.drain() |
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.
If the TCP connection is broken, won't this fail?
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.
nope
# bound to the REST API server as the latter will inform HAProxy if the system was set to drain mode. | ||
|
||
autoscaler_rest_app = FastAPI() | ||
TRANSCRIBE_GRACEFUL_SHUTDOWN.set(0) |
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.
Is it necwesssary to initialize it?
No description provided.