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

PAASTA-18479: Add list-namespaces tool #3997

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jfongatyelp
Copy link
Contributor

@jfongatyelp jfongatyelp commented Dec 20, 2024

Our iam_role configs depend on role owners being able to fill out which k8s namespaces their role is going to be used in in pod_identity_namespaces, but paasta normally is figuring out that translation, and users don't usually need to care about this info.

This list-namespaces tool should just spit out the namespaces their service will be using, in list format to match what our iam_roles module needs.

I don't expect people will need to specify the instance/cluster filters at all since roles can be used across those lines, but this command leaves the hard work of filtering on those values to the already-existing get_instance_configs_for_service util.

The tests already capture most of the list-namespaces specific logic we want, but testing this against some example services seems to also work as expected:

$ paasta list-namespaces -s ad_event
['paasta-spark', 'tron']

$ paasta list-namespaces -s ad_event -i ad_event.wait_for_ad_opportunity # not a spark action
['tron']

$ paasta list-namespaces -s ad_event -i ad_event.wait_for_ad_opportunity -c pnw-devc # Currently no errors thrown if filters do not match a real instance
[]

There are also 2 non-list-namespaces changes in here:

  1. LongRunningServiceConfig used to throw a NotImplementedError for get_namespace(), but by me deleting that override, LongRunningServiceConfig.get_namespace() will use InstanceConfig.get_namespace() which uses INSTANCE_TYPE_TO_K8S_NAMESPACE to return the correct value. I think the exception may not be necessary, as from a search of cases of us catching the NotImplementedError, we never expect to do $something if it was due to get_namespaces() throwing this error.
  • This will actually stop throwing exceptions for anything trying to run get_namespace on a LongRunningServiceConfig subclass that does not define this method, such as Flink or NRTSearch. However, I didn't find any code that expected to hit an exception when using get_namespace, as they mostly related to secret syncing, which special-case operator namespaces already. In fact, by allowing flink instances to actually get_namespace, we may be able to remove the special-case sync jobs later!
  • From original discussion when we started supporting arbitrary namespaces (PR) on the get_namespace method, it seems like we generally want this to come from InstanceConfig anyhow, using default values if no override is specified, so this seems fine.
  • PaaSTA services will still default to the correct default of paastasvc-* since we override get_namespace here
  1. I noticed some copypaasta in the autoscale cli cmd's help text, so I s/stop/autoscale/ where I found this issue.

@jfongatyelp jfongatyelp requested a review from a team as a code owner December 20, 2024 00:10
# Tron instances are TronActionConfigs
if (
instance.get_instance_type() == "tron"
and instance.get_executor() == "spark"
Copy link
Member

Choose a reason for hiding this comment

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

i think it might be possible for batch-box-launched executors to use pod identity, but imo it's fine to leave them out u!less this becomes a problem (especially since things are actively migrating away from batch boxes)

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