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

Only request running pods from Kubernetes API #1322

Merged
merged 2 commits into from
Sep 26, 2024

Conversation

leviramsey
Copy link
Contributor

It seems like we'd be most interested in the pods which are currently running (cc: @girdharshubham ). Without this filter, especially in environments where rollouts are frequent (e.g. dev/test environments), there's a chance that getting so many pods could cause HTTP responses which akka http rejects.

Can hide this behind a config setting if needed, but I'm not sure why one would want to discover pods which used to be there.

@leviramsey leviramsey requested a review from patriknw September 25, 2024 21:08
@leviramsey
Copy link
Contributor Author

I assume the link checker failure is expected, post #1319

@johanandren
Copy link
Member

Not sure, but, could this cause problems with ready check on a cold cluster start?

@leviramsey
Copy link
Contributor Author

leviramsey commented Sep 26, 2024

Not sure, but, could this cause problems with ready check on a cold cluster start?

From a conversation with @girdharshubham, it appears that running does not take readiness into account (viz. pods which aren't ready will show as status.phase==Running):

kubectl --context=arn:aws:eks:us-east-2:xyz:cluster/dev-us-east-2 get pods -n some-uuid --field-selector=status.phase==Running 
NAME                                                READY   STATUS             RESTARTS           AGE
some-name-644cc5669-pwn62   2/3     CrashLoopBackOff   1550 (4m26s ago)   5d9h
some-name-cf679b76b-2hm46   2/3     CrashLoopBackOff   1576 (2m51s ago)   5d11h
some-name-cf679b76b-p8crf   2/3     CrashLoopBackOff   1570 (112s ago)    5d11h
some-name-cf679b76b-zr772   2/3     CrashLoopBackOff   1580 (3m43s ago)   5d11h

Copy link
Member

@johanandren johanandren left a comment

Choose a reason for hiding this comment

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

LGTM, seems safe

@johanandren johanandren merged commit fb1e26f into akka:main Sep 26, 2024
14 of 15 checks passed
@leviramsey leviramsey deleted the only-the-running branch September 26, 2024 15:15
@leviramsey
Copy link
Contributor Author

Note that this does not remove the filtering of the returned results for running pods on our side. It may make sense to leave that in as a "defense in depth".

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