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.

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

Handle various microcluster ready endpoints #142

Merged
merged 2 commits into from
Aug 12, 2024

Conversation

addyess
Copy link
Contributor

@addyess addyess commented Aug 9, 2024

Overview

With the advent of v2 microcluster, the ready endpoint moved from /cluster/1.0/ready to /core/1.0/ready because changing endpoints is a totally natural thing to do for run

Details

  • loop through available endpoints til we find one that doesn't 404.
  • reraise any non-404 excptions
  • raise a K8sdConnectionError if we run out of endpoints to try

@addyess addyess requested a review from a team as a code owner August 9, 2024 21:57
@addyess addyess force-pushed the akd/try-microcluster-endpoints branch from 01eed65 to 6141475 Compare August 9, 2024 21:59
Raises:
K8sdConnectionError: If the response is Not Found on all endpoints.
"""
ready_endpoints = ["/core/1.0/ready", "/cluster/1.0/ready"]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll try the "new one" first -- then back up with the old one

Comment on lines 790 to 797
if ex.code == 404:
logger.warning(
"Encountered 404 while checking if micro-cluster is ready @ %s: %s",
endpoint,
ex,
)
# Try the next endpoint if the current one is not found
continue
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

k8sd responding with a 404 is its way of saying "I see that request -- but that doesn't exist". A clear sign we should try another endpoint!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So... this may be misleading the user into thinking something is wrong with the code.
Could the log be modified to mention this could be normal and we're cycling through possible endpoints?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe Trying endpoint 1 of 2: ...

Comment on lines +166 to +167
not_found = InvalidResponseError(code=404, msg="Not Found")
mock_send_request.side_effect = [not_found, not_found]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A wise man once said...

two wrongs don't make a right, but three lefts do

Comment on lines 790 to 797
if ex.code == 404:
logger.warning(
"Encountered 404 while checking if micro-cluster is ready @ %s: %s",
endpoint,
ex,
)
# Try the next endpoint if the current one is not found
continue
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So... this may be misleading the user into thinking something is wrong with the code.
Could the log be modified to mention this could be normal and we're cycling through possible endpoints?

@addyess addyess merged commit 113b69c into main Aug 12, 2024
34 checks passed
@addyess addyess deleted the akd/try-microcluster-endpoints branch August 12, 2024 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants