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

fix(backend): Caching - Only send cache-enabled pods to the caching webhook #4429

Conversation

Ark-kun
Copy link
Contributor

@Ark-kun Ark-kun commented Aug 28, 2020

The caching webhook already checks whether the pod is cache-enabled, but this change makes the check happen sooner - even before calling the webhook.

This way the webhook cannot possibly affect any non-KFP pods.

This feature requires API v1 and Kubernetes v1.15, so we use it conditionally.

@kubeflow-bot
Copy link

This change is Reviewable

@Ark-kun Ark-kun added the cherrypick-approved area OWNER approves to cherry pick this PR to current active release branch label Aug 28, 2020
@Bobgy
Copy link
Contributor

Bobgy commented Aug 28, 2020

+1000
This will make it so much safer to enable.

/lgtm
/approve

Thanks @Ark-kun!

/hold
looks like the PR needs to wait for #4428 get merged first

@rmgogogo
Copy link
Contributor

/lgtm
/approve

The caching webhook already checks whether the pod is cache-enabled, but this change makes the check happen sooner - even before calling the webhook.
This way the webhook cannot possibly affect any non-KFP pods.

This feature requires API v1 and Kubernetes v1.15, so we use it conditionally.
@Ark-kun Ark-kun force-pushed the Backend---Caching---Only-send-cache-enabled-pods-to-the-caching-webhook branch from b6cc5dc to b12a34b Compare August 29, 2020 05:14
@k8s-ci-robot k8s-ci-robot removed the lgtm label Aug 29, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Bobgy, rmgogogo

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

@Ark-kun
Copy link
Contributor Author

Ark-kun commented Aug 29, 2020

/retest

@Ark-kun
Copy link
Contributor Author

Ark-kun commented Aug 29, 2020

/test kubeflow-pipeline-backend-test

@Ark-kun
Copy link
Contributor Author

Ark-kun commented Aug 29, 2020

/retest

@Ark-kun
Copy link
Contributor Author

Ark-kun commented Aug 29, 2020

The change seems to be working. The KFP pods reach cache-server (logs), but non-KFP pods no longer reach it.

@Ark-kun
Copy link
Contributor Author

Ark-kun commented Aug 29, 2020

/unhold

@Ark-kun
Copy link
Contributor Author

Ark-kun commented Aug 29, 2020

/test kubeflow-pipeline-backend-test

@Ark-kun
Copy link
Contributor Author

Ark-kun commented Sep 1, 2020

@rmgogogo Can you re-LGTM please?

@Bobgy
Copy link
Contributor

Bobgy commented Sep 1, 2020

/lgtm

@Ark-kun
Copy link
Contributor Author

Ark-kun commented Sep 2, 2020

/retest

1 similar comment
@Bobgy
Copy link
Contributor

Bobgy commented Sep 2, 2020

/retest

@k8s-ci-robot k8s-ci-robot merged commit 6b54eec into kubeflow:master Sep 2, 2020
@Bobgy Bobgy added the cherrypicked cherry picked to release branch `release-x.y` label Sep 4, 2020
Bobgy pushed a commit to Bobgy/pipelines that referenced this pull request Sep 4, 2020
…ebhook (kubeflow#4429)

* Backend - Caching - Only send cache-enabled pods to the caching webhook

The caching webhook already checks whether the pod is cache-enabled, but this change makes the check happen sooner - even before calling the webhook.
This way the webhook cannot possibly affect any non-KFP pods.

This feature requires API v1 and Kubernetes v1.15, so we use it conditionally.

* Support filtering on Kubernetes v1.15 as well
Bobgy pushed a commit that referenced this pull request Sep 4, 2020
…ebhook (#4429)

* Backend - Caching - Only send cache-enabled pods to the caching webhook

The caching webhook already checks whether the pod is cache-enabled, but this change makes the check happen sooner - even before calling the webhook.
This way the webhook cannot possibly affect any non-KFP pods.

This feature requires API v1 and Kubernetes v1.15, so we use it conditionally.

* Support filtering on Kubernetes v1.15 as well
Jeffwan pushed a commit to Jeffwan/pipelines that referenced this pull request Dec 9, 2020
…ebhook (kubeflow#4429)

* Backend - Caching - Only send cache-enabled pods to the caching webhook

The caching webhook already checks whether the pod is cache-enabled, but this change makes the check happen sooner - even before calling the webhook.
This way the webhook cannot possibly affect any non-KFP pods.

This feature requires API v1 and Kubernetes v1.15, so we use it conditionally.

* Support filtering on Kubernetes v1.15 as well
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved area/backend area/execution_cache cherrypick-approved area OWNER approves to cherry pick this PR to current active release branch cherrypicked cherry picked to release branch `release-x.y` cla: yes lgtm size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants