[autoscaler] Fix tag cache bug, don't kill workers on error #14424
Conversation
| @@ -414,12 +414,19 @@ def _print(self, | |||
| record.levelname = _level_str | |||
| rendered_message = self._formatter.format(record) | |||
|
|
|||
| # We aren't using standard python logging convention, so we hardcode | |||
| # the log levels for now. | |||
There was a problem hiding this comment.
Make sure that errors actually appear in monitor.err
| @@ -239,10 +239,6 @@ def destroy_autoscaler_workers(self): | |||
|
|
|||
| def _handle_failure(self, error): | |||
| logger.exception("Error in monitor loop") | |||
| if self.autoscaler is not None: | |||
There was a problem hiding this comment.
Can we please preserve this but put it behind an env var?
| @@ -120,6 +120,22 @@ def __init__(self, provider_config, cluster_name): | |||
| # excessive DescribeInstances requests. | |||
| self.cached_nodes = {} | |||
|
|
|||
| def _gc_tag_cache(self): | |||
There was a problem hiding this comment.
This seems complicated. Why not instead make all accesses to the tag cache fall back on looking up the tags in case of failure?
There was a problem hiding this comment.
You mean making an API call to get the tags?
There was a problem hiding this comment.
Yes, or returning empty dict if the node is terminated.
Hmm another option is to never GC the tags; it seems harmless enough to keep them forever, since the number of nodes will never be that large.
There was a problem hiding this comment.
An empty dict would be an inconsistent state so I'd prefer to avoid that. I guess just leaking the tag cached isn't the end of the world...
|
Is there any test that catches the behavior? |
|
I don't really see how we can test this without a lot of effort, in general we don't have integration tests for the node provider, and it's not clear how to catch this with unit testing (we generally don't have unit tests for the node providers). |
|
Given this is blocking a production user, how about let's merge for now and file a followup to better test the tag cache? |
|
I agree. @AmeerHajAli given that we don't have the right framework for testing this, can you merge this if you think it's ok? |
Why are these changes needed?
This PR fixes 2 related issues.
Related issue number
Closes #14264
Checks
scripts/format.shto lint the changes in this PR.