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

Disable istio sidecar injection in simple tfjob test #1148

Merged
merged 2 commits into from
Mar 20, 2020

Conversation

Bobgy
Copy link
Contributor

@Bobgy Bobgy commented Mar 18, 2020

Unblocks kubeflow/kfctl#131, because kfctl uses tf-operator head for presubmit test.
After enabling istio sidecar injection in kubeflow namespace, tfjob-smoke-test is the only test timing out.

I haven't been able to look at pod log/info because there were permission issues, but I've found other people mentioning tfjob cannot work with istio sidecar: kubeflow/kubeflow#3935

/cc @jlewi


This change is Reviewable

@Bobgy
Copy link
Contributor Author

Bobgy commented Mar 18, 2020

/assign @richardsliu

@gaocegege
Copy link
Member

/lgtm

@Bobgy
Copy link
Contributor Author

Bobgy commented Mar 18, 2020

/cc @jlewi

@coveralls
Copy link

coveralls commented Mar 18, 2020

Coverage Status

Coverage remained the same at 96.512% when pulling 2c4afd4 on Bobgy:patch-1 into 8048775 on kubeflow:master.

@Bobgy
Copy link
Contributor Author

Bobgy commented Mar 19, 2020

Looks like a flaky test.
/retest

@Bobgy
Copy link
Contributor Author

Bobgy commented Mar 19, 2020

Tests are passing now.
@gaocegege @richardsliu Any concerns? Can you approve?

@Bobgy Bobgy changed the title Disable istio sidecar injection in simple tfjob Disable istio sidecar injection in simple tfjob test Mar 19, 2020
@jlewi
Copy link
Contributor

jlewi commented Mar 19, 2020

/lgtm
/approve

@ChanYiLin
Copy link
Member

@gaocegege @richardsliu
do you think we should disable istio injection for all tf-operator job?
I wonder will istio affect the communication between components of tfjob (ps, workers).
we can probably open an issue and discussed there

/approve

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ChanYiLin, jlewi

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

@k8s-ci-robot k8s-ci-robot merged commit 011b3ec into kubeflow:master Mar 20, 2020
@Bobgy Bobgy deleted the patch-1 branch March 20, 2020 08:07
@Bobgy
Copy link
Contributor Author

Bobgy commented Mar 20, 2020

Thank you @ChanYiLin!

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.

7 participants