-
Notifications
You must be signed in to change notification settings - Fork 241
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
Adding LimitRanges for paasta managed namespaces #3726
Conversation
@@ -2676,14 +2679,15 @@ def ensure_namespace(kube_client: KubeClient, namespace: str) -> None: | |||
kube_client.core.create_namespace(body=paasta_namespace) | |||
|
|||
ensure_paasta_api_rolebinding(kube_client, namespace) | |||
ensure_paasta_namespace_limits(kube_client, namespace) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we move this into the if namespace not in namespace_names:
once this has run on all our clusters? seems a bit wasteful to always be checking limit ranges when we know that they're guaranteed to exist (once this has run at least once)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice suggestion but we currently have a bunch of existing namespaces without limits :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe? We're inevitably gonna forget about this and then we'll have namespaces that are created by Flux which don't get these limits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea, exactly - once we've released this as-is all of these existing namespace will now have limits and we can clean this up to only set limit ranges on brand new namespaces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops, looks like I responded with a stale page and never saw your comment
@EvanKrall yea, I guess it depends on how we want to treat folks manually creating namespaces (and then reusing those namespaces for paasta: do we want them to have all of paasta's defaults as well or allow for them to do their own thing?
(this bit us earlier today in #cid - they're reusing a namespace for paasta and the limitrange we added affected pre-existing workloads)
Co-authored-by: Luis Pérez <[email protected]>
Adding LimitRanges for PaaSTA managed namespaces with
ensure_namespace()