Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Improve exception handling for listing Kubernetes resources #837

Merged
merged 4 commits into from
Jun 10, 2024
Merged

Improve exception handling for listing Kubernetes resources #837

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
08c5e47
Improve exception handling for enumerating Kubernetes resources
josefhandl May 2, 2024
a4fc645
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] May 3, 2024
0f2555d
Replace error log with exception log for K8s API exception handling
josefhandl May 6, 2024
4fcd345
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] May 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion kubespawner/reflector.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
import time
from functools import partial

from kubernetes_asyncio import watch
from kubernetes_asyncio import client, watch
from traitlets import Any, Bool, Dict, Int, Unicode
from traitlets.config import LoggingConfigurable
from urllib3.exceptions import ReadTimeoutError
Expand Down Expand Up @@ -228,7 +228,18 @@ async def _list_and_update(self, resource_version=None):
kwargs["namespace"] = self.namespace

list_method = getattr(self.api, self.list_method_name)

initial_resources_raw = await list_method(**kwargs)
if not initial_resources_raw.ok:
self.log.error(
Copy link
Collaborator

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?

Copy link
Contributor Author

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

Copy link
Member

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?

Copy link
Member

@consideRatio consideRatio Jun 1, 2024 •
edited
Loading

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.

f'Error when calling Kubernetes API.'
f' Status: {initial_resources_raw.status} {initial_resources_raw.reason}.'
f' Message: {(await initial_resources_raw.json())["message"]}'
)
raise client.ApiException(
status=initial_resources_raw.status, reason=initial_resources_raw.reason
)

# This is an atomic operation on the dictionary!
initial_resources = json.loads(await initial_resources_raw.read())
self.resources = {
Expand Down