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

Speed up namespace switching in big clusters #11

Merged
merged 3 commits into from
Jun 16, 2023

Conversation

imbstack
Copy link
Contributor

Hi, thank you so much for this tool, it is the best one of its kind!

This patch just switches the check that a namespace exists from listing all namespaces to seeing if a specific label exists. This doesn't make much difference on clusters with reasonable numbers of namespaces but for me it is roughly a 8 second speedup:

$ time (kubechn namespace)      
Context "context" modified.
Switched to namespace "namespace"
( kubechn namespace; )  5.32s user 0.23s system 61% cpu 8.978 total

# switch to this patch

$ time (kubechn namespace)       
Context "context" modified.
Switched to namespace "namespace"
( kubechn namespace; )  0.21s user 0.06s system 39% cpu 0.714 total

This label should always exist in 1.22+ clusters. I expect relying on this would break kubechc for clusters older than that? Happy to make this fallback to the listing-all-namespaces way when the label check fails or something along those lines. Whatever sounds best to you.

Thanks again!

@aabouzaid
Copy link
Collaborator

@imbstack, thanks for your PR, that's a pretty good addition 🚀
since this feature is a bit new, I'd suggest making it disabled by default and enabling it by a flag (env var).

Later on, we can flip the flag and make it the default and keep the old style for anyone who works with older K8s versions.

@aabouzaid aabouzaid self-requested a review April 12, 2023 10:40
@aabouzaid aabouzaid added the enhancement New feature or request label Apr 12, 2023
Copy link
Collaborator

@aabouzaid aabouzaid left a comment

Choose a reason for hiding this comment

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

I will merge the PR once it has a flag (env var) to control the feature as mentioned.

@imbstack
Copy link
Contributor Author

Ah yes sorry, I've been busy lately. I'll get a patch with that change before too long!

@imbstack
Copy link
Contributor Author

Finally got around to this. I've tested this with the env var unset, and set to both listed values across the following cases:

  1. namespace passed in and exists
  2. namespace passed in and does not exist
  3. no namespace passed in

I believe it works correctly in all of those cases. That said, I'm not a shell scripting expert so if there's a better way to go about any of this I'm happy to change whatever!

@imbstack
Copy link
Contributor Author

I'll update README usage and such if this looks good!

@aabouzaid
Copy link
Collaborator

@imbstack Thanks for the change.
Apart from the typo, as shown in the CI pipeline, the current implementation is a bit complex.

You can simplify it by setting the default value the same way as here:
https://github.com/aabouzaid/kubech/blob/9f11a5486152de8ea13ef0fc1e269187ecaa7db5/kubech#L10

if you don't want to bother, I can do the whole change anyway.

@imbstack
Copy link
Contributor Author

Ah, good call! Hopefully this is more what you're thinking of. If not, feel free to edit or drop the PR and make it yourself. Thanks again!

@aabouzaid
Copy link
Collaborator

@imbstack I've made a small fix and it's ready to merge.
Thanks for your contribution Brian 🙇

@aabouzaid aabouzaid merged commit c978cf1 into DevOpsHiveHQ:master Jun 16, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants