Fixed semaphore leak in kubernetes client #7678
Conversation
Signed-off-by: Aylei <rayingecho@gmail.com>
Signed-off-by: Aylei <rayingecho@gmail.com>
|
/quicktest-core |
sky/adaptors/kubernetes.py
Outdated
| real_client = self._client | ||
| elif isinstance(self._client, kubernetes.watch.Watch): | ||
| # For watcher, stop the watch before cleanup. | ||
| kubernetes.watch.Watch().stop() |
There was a problem hiding this comment.
Should we call this on the self._client? It's surprising to me that this works when we call it on a newly instantiated Watch object. Why is that?
There was a problem hiding this comment.
Actually, after looking at the kubernetes client code, I'm pretty sure this line does nothing.
There was a problem hiding this comment.
I think we may need to do something to make sure that the Watch.stream iterator cleans up its connection, but I guess that doing that here doesn't make much sense. The way .stop() works, it seems like it will not actually take effect until we call next() on the iterator and it actually terminates (StopIteration)
sky/adaptors/kubernetes.py
Outdated
| if isinstance(obj, ClientWrapper): | ||
| return obj |
There was a problem hiding this comment.
How would this happen? I feel like we should either ignore this case or throw an exception.
sky/adaptors/kubernetes.py
Outdated
| """Delegate to the underlying client""" | ||
| return getattr(self._client, name) | ||
|
|
||
| def __del__(self): |
There was a problem hiding this comment.
Would love to try and find a way to avoid relying on __del__ for this - I think it's better than the current state, but if we can manage the lifecycle more explicitly that seems good. E.g. some alternative to the lru_cache that will call close or __exit__
|
I pushed some changes to the PR. @cblmemo could you take another look? |
|
/quicktest-core |
close #7638
Tested (run the relevant ones):
bash format.sh/smoke-test(CI) orpytest tests/test_smoke.py(local)/smoke-test -k test_name(CI) orpytest tests/test_smoke.py::test_name(local)/quicktest-core(CI) orpytest tests/smoke_tests/test_backward_compat.py(local)