Fix Istio sidecar injection by moving from annotations to labels #2527
Conversation
|
/ok-to-test |
|
@andreyvelich @Electronic-Waste for approval and merging |
There was a problem hiding this comment.
I'm wondering if we should fix all things: https://github.com/search?q=repo%3Akubeflow%2Fkatib%20sidecar.istio.io%2Finject&type=code
If not, Katib with istio might not work well.
Probably Yes, according to the istio documentations it should be a label https://istio.io/latest/docs/setup/additional-setup/sidecar-injection/#controlling-the-injection-policy |
There was a problem hiding this comment.
@madmecodes Thank you for updating it!
Please can you also update other places where we set these labels.
For example:
katib/pkg/controller.v1beta1/consts/const.go
Line 123 in 07d17b9
@andreyvelich which is dependent on I shall proceed with these changes and create a appropriate PR right? |
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
@andreyvelich looking forward for the feedback |
|
@madmecodes please rebase (not merge) to master and fix the linting tests |
Signed-off-by: madmecodes <[email protected]>
…codebase Replace annotations with labels for Istio sidecar injection according to Istio recommendations. Update conformance tests, examples, constants, composers, and utilities to use the new label-based approach consistently. Signed-off-by: madmecodes <[email protected]>
… Istio label injection Signed-off-by: madmecodes <[email protected]>
Signed-off-by: madmecodes <[email protected]>
madmecodes
force-pushed
the
fix-istio-sidecar-injection
branch
from
3962524 to
6759bb1
Compare
rebased as requested
|
|
maybe, in this repository also, we can add automatic, pre-commit run check when we do : git commit -s -m "" |
@andreyvelich Hey! i did update the changes, is there anything more thats need to be done? I'm open to the feedback. |
|
@anencore94 @andreyvelich @Electronic-Waste any updates, regarding this PR? i am for the feedback. |
| @@ -95,7 +95,7 @@ def generate_trial_template() -> V1beta1TrialTemplate: | |||
| "kind": "Job", | |||
There was a problem hiding this comment.
We might need to also update this one: https://github.com/kubeflow/trainer/blob/release-1.9/sdk/python/kubeflow/training/utils/utils.py#L259
There was a problem hiding this comment.
We might need to also update this one: https://github.com/kubeflow/trainer/blob/release-1.9/sdk/python/kubeflow/training/utils/utils.py#L259
Made the respective Pull Request in the trainer/release-1.9 kubeflow/trainer#2637
|
|
||
| func appendAnnotation(annotations map[string]string, newAnnotationName string, newAnnotationValue string) map[string]string { | ||
| // AppendIstioSidecarLabel adds the Istio sidecar injection label to a labels map | ||
| func AppendIstioSidecarLabel(labels map[string]string) map[string]string { |
There was a problem hiding this comment.
There was a problem hiding this comment.
moved the func in labels.go
Signed-off-by: madmecodes <[email protected]>
|
@juliusvonkohout @madmecodes Just one question, in which release Istio changed this property from annotations to labels? |
shift from annotations to labels for sidecar injection in Istio occurred in version 1.2. annotations: https://istio.io/v1.1/docs/setup/kubernetes/additional-setup/sidecar-injection/#policy https://istio.io/v1.2/docs/setup/kubernetes/additional-setup/sidecar-injection/#deploying-an-app |
|
Oh, I see, thanks for letting me know @madmecodes! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andreyvelich The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What this PR does / why we need it:
This PR fixes the Istio sidecar injection configuration by moving from using annotations to using labels in Katib components.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Issue: kubeflow/manifests#2798
Part of PR: kubeflow/manifests#3044
Checklist: