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 istio sidecar inject false to deployments in kubeflow ns #712

Merged
merged 4 commits into from
Jan 29, 2020

Conversation

Bobgy
Copy link
Contributor

@Bobgy Bobgy commented Jan 17, 2020

Which issue is resolved by this Pull Request:
Part of kubeflow/pipelines#1223
Unblocks kubeflow/kfctl#131
Description of your changes:
Disabled istio sidecar injection for deployments in kubeflow namespace except for metadata related manifests. There was a problem with auto generated test cases for metadata, discussed in #716 separately.

Checklist:

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

/assign @jlewi


This change is Reviewable

@Bobgy Bobgy changed the title Add istio sidecar inject false to deployments Add istio sidecar inject false to deployments in kubeflow ns Jan 17, 2020
@Bobgy Bobgy requested a review from jlewi January 17, 2020 08:15
@Bobgy
Copy link
Contributor Author

Bobgy commented Jan 21, 2020

@jlewi Can I get approval on this one? I will address issues in #716 separately.

Copy link
Contributor

@krishnadurai krishnadurai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Bobgy since this is for 'kubeflow' namespace, I suggest we avoid annotating 'cert-manager' with the istio-injection label? It is deployed to 'cert-manager' namespace.

@Bobgy
Copy link
Contributor Author

Bobgy commented Jan 21, 2020

Thanks for pointing that out. I will fix that.

EDIT: already fixed

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign jlewi
You can assign the PR to them by writing /assign @jlewi in a comment when ready.

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

@Bobgy
Copy link
Contributor Author

Bobgy commented Jan 21, 2020

/retest

@Bobgy
Copy link
Contributor Author

Bobgy commented Jan 21, 2020

I tried looking at test error message and found this in link

util.py                     72 INFO     failed to apply:  (kubeflow.error): Code 500 with message: coordinator Apply failed for gcp:  (kubeflow.error): Code 400 with message: gcp apply c
ould not update deployment manager Error Set New IamPolicy error: googleapi: Error 400: The number of members in the policy (1,505) is larger than the maximum allowed size 1,500., badReq
uest

Looks like the test project is reaching quota limit. Can someone help me clean them up?

@jlewi
Copy link
Contributor

jlewi commented Jan 21, 2020

@Bobgy Can you please open a bug in kubeflow/testing ? And then ping in #buildcop in kubeflow.slack.com?

@jlewi
Copy link
Contributor

jlewi commented Jan 21, 2020

Looks like we have kubeflow/testing#543

@jlewi
Copy link
Contributor

jlewi commented Jan 21, 2020

/test all

@Bobgy
Copy link
Contributor Author

Bobgy commented Jan 22, 2020

/retest

1 similar comment
@Bobgy
Copy link
Contributor Author

Bobgy commented Jan 28, 2020

/retest

@jlewi
Copy link
Contributor

jlewi commented Jan 28, 2020

@Bobgy The RC branch for Kubeflow 1.0 has been cut so if you want this to make the cut I would suggest focusing on getting the tests passed and then cherry-picking it into the release branch.

It looks like the test_katib is ready test failed and the test_kf_is_ready test failed

The latter error

E     kubeflow.testing.util.TimeoutError: Timeout waiting for deployment cloud-endpoints-controller in namespace kubeflow

Looks like it might be related to kubeflow/kubeflow#4693 which should be fixed on master.
So you might want to rebase and try again.

/test all

@Bobgy
Copy link
Contributor Author

Bobgy commented Jan 29, 2020

/test kubeflow-manifests-presubmit
Each test seems to fail on a different case, seems flaky.

@Bobgy
Copy link
Contributor Author

Bobgy commented Jan 29, 2020

/retest

@k8s-ci-robot
Copy link
Contributor

@Bobgy: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
kubeflow-manifests-presubmit 4b1ca00 link /test kubeflow-manifests-presubmit

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@jlewi
Copy link
Contributor

jlewi commented Jan 29, 2020

Most recent failure was the pytorch job failed to run.

I don't see why adding an annotation to the statefulset would affect that. So I'll go ahead and merge manually.

@jlewi jlewi merged commit 3fe91f5 into kubeflow:master Jan 29, 2020
@Bobgy Bobgy deleted the disable_istio_injection_deployment branch January 29, 2020 15:11
@Bobgy
Copy link
Contributor Author

Bobgy commented Jan 29, 2020

Thanks, @jlewi!

vpavlin pushed a commit to vpavlin/manifests that referenced this pull request Feb 18, 2020
…w#712)

* Add istio sidecar inject false to deployments

* Revert cert-manager changes

* Revert unneeded changes
crobby pushed a commit to opendatahub-io/manifests that referenced this pull request Feb 20, 2020
…w#712)

* Add istio sidecar inject false to deployments

* Revert cert-manager changes

* Revert unneeded changes
yanniszark pushed a commit to arrikto/kubeflow-manifests that referenced this pull request Jul 13, 2020
…beflow ns (kubeflow#712)

* Add istio sidecar inject false to deployments

* Revert cert-manager changes

* Revert unneeded changes

* Fix Katib labels changed by mistake
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.

5 participants