-
Notifications
You must be signed in to change notification settings - Fork 305
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
Improve exception handling for listing Kubernetes resources #837
Improve exception handling for listing Kubernetes resources #837
Conversation
Thanks for submitting your first pull request! You are awesome! 🤗 |
for more information, see https://pre-commit.ci
kubespawner/reflector.py
Outdated
initial_resources_raw = await list_method(**kwargs) | ||
if not initial_resources_raw.ok: | ||
self.log.error( |
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.
Can you use self.log.exception
here instead, and pass it the exception you're creating?
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.
Yes, but as far as I know, it's not possible without a try-catch. I've pushed a fix
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.
Based on python docs:
This method should only be called from an exception handler.
So I think you were forced to do that for this to make sense. But, doesn't this mean we get stack trace details from raising the error a few lines above, and that in turn doesn't help us get informed about the original error anyhow associated with getting a response where response.ok == False
?
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.
With that in mind, was the original thing better or not? I'm not sure - I'll defer to @yuvipanda or someone with more experience with this than me to make a decision and then we go for it.
@yuvipanda I tried catching up with differences of log.error and log.exception, but I've not come to a conclusion if 0f2555d was what you wanted - could you review this again? |
I think either works, and given that the ultimate goal is to figure out better error handling, I'm just going to merge this. Thanks for your patience @josefhandl and thanks for the ping, @consideRatio |
When the Kubernetes API fails to list Kubernetes resources in the reflector (for example too strict permissions), no relevant error message appears in the logs, and the program fails. I'm adding a check to see if the call is successful. If not, an error with the API is printed, and an exception is thrown.
This is what the previous logs look like (
enable_user_namespaces = True
):...and this now: