fix: remove unneccessary ns validation #2066
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: zhixian82 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
google-oss-prow
bot
requested review from
anencore94,
gaocegege and
johnugeorge
| @@ -79,25 +78,6 @@ func (v *ExperimentValidator) Handle(ctx context.Context, req admission.Request) | |||
| } | |||
| } | |||
|
|
|||
| // After metrics collector sidecar injection in Job level done, delete validation for namespace labels | |||
There was a problem hiding this comment.
I'm not sure what sidecar injection in Job level means. This comment seemed to add in #828 3 years ago.
After that, @andreyvelich refactored the getKatibJob function (maybe, for job level injection?) in #1303.
@andreyvelich @johnugeorge Do you remember what this comment means?
There was a problem hiding this comment.
I don't think, this comment is still valid.
In Job level, we look at type of Job(TFJob, PytorchJob etc) and decide the job pod which needs metrics collector injection. In Pod level, we check pod level labels and decide if we need to inject metrics collector or not. Right now, we are doing pod level where inject metric collector in the pods that contain PrimaryPodLabels or not.
There was a problem hiding this comment.
I see. Thanks for clarifying! @johnugeorge
There was a problem hiding this comment.
@tenzen-y I guess the idea behind Job level mutation was described here: https://github.com/kubeflow/katib/blob/master/docs/proposals/metrics-collector.md#mutating-webhook.
But since we dynamically support any kind of Kubernetes resources, we mutate only in the Pod level.
There was a problem hiding this comment.
Yes, I agree. I was thinking we cannot inject sidecars in Job level since we support any K8s resources, as you say.
Maybe, it would be good to remove the comment to avoid confusion.
There was a problem hiding this comment.
@johnugeorge @andreyvelich @tenzen-y Thanks for the explanation! I will close this pr
|
The reason behind namespace selector was that we need to identify namespaces that can run katib workloads. In order to reduce the performance effect(due to webhooks) for the whole cluster which can have non Kubeflow/Katib workloads as well, we were specifically looking for namespaces that has |
| @@ -63,9 +63,6 @@ webhooks: | |||
| name: katib-controller | |||
| namespace: kubeflow | |||
| path: /mutate-pod | |||
| namespaceSelector: | |||
| matchLabels: | |||
| katib.kubeflow.org/metrics-collector-injection: enabled | |||
There was a problem hiding this comment.
We shouldn't remove this namespaceSelector.
Otherwise, Katib Mutation Webhook will be executing in every Kubernetes namespace.
#2062