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

Add sidecar.istio.io/inject false label to all deployment/stateful-sets in kubeflow namespace #700

Closed
wants to merge 0 commits into from

Conversation

Bobgy
Copy link
Contributor

@Bobgy Bobgy commented Jan 9, 2020

Which issue is resolved by this Pull Request:
Part of kubeflow/pipelines#1223

Description of your changes:
Add sidecar.istio.io/inject: "false" to all deployments to unblock kubeflow/kfctl#131

  1. changed each deployment/statefulset to add annotation sidecar.istio.io/inject "false"
  2. updated https://github.com/kubeflow/manifests/blob/master/kfdef/kfctl_gcp_iap.yaml to use the new overlays

Checklist:

  • Unit tests have been rebuilt:
    1. cd manifests/tests
    2. make generate-changed-only
    3. make test

This change is Reviewable

@jlewi
Copy link
Contributor

jlewi commented Jan 13, 2020

@Bobgy Thanks.

  • Can we avoid creating new overlays? It looks like you are creating an ISTIO overlay for some applications that don't already have them.

    • The downside of doing this is that means the overlay needs to be added to all KFDef specs. Your PR currently only updates the GCP spec.
  • Instead of adding the annotation in the ISTIO overlay why not add it in the base package? Adding the annotation "sidecar.istio.io/inject: "false"" should be harmless in the case ISTIO isn't installed.

    • The ISTIO overlays are kind of a historical artifact at this point due to the transition from non-istio to ISTIO so I'm not sure we should keep adding to them.
  • Rather than iterate on all of the applications at once; it might be faster to split off one or two applications from this PR and do those first and then once we've figured everything out update all the rest to match.

@Bobgy
Copy link
Contributor Author

Bobgy commented Jan 14, 2020

@jlewi Thanks, I didn't know every deployment uses istio now. I will adjust that accordingly using one component as an example. Let's move on after that change is good.

@Bobgy
Copy link
Contributor Author

Bobgy commented Jan 14, 2020

@jlewi made example PR in #703

@Bobgy
Copy link
Contributor Author

Bobgy commented Jan 14, 2020

/hold
wait for discussion in #703 first

@jlewi
Copy link
Contributor

jlewi commented Jan 16, 2020

@Bobgy looks like #703 was merged do you want to update this?

@Bobgy
Copy link
Contributor Author

Bobgy commented Jan 17, 2020

Will do

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by:

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@Bobgy Bobgy deleted the disable_istio_injection branch January 17, 2020 05:29
@Bobgy
Copy link
Contributor Author

Bobgy commented Jan 17, 2020

Made some mistake, will open a new PR instead

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants