Fix "Garbage collector should support cascading deletion of custom resources" flake #61499

jennybuckley · 2018-03-21T21:24:25Z

What this PR does / why we need it:
These webhook configuration objects were intended to have no effect on admission requests, and are just used to ensure that webhook configuration objects can always be deleted.

However, I think they are causing an entirely separate test to flake (Garbage collector should support cascading deletion of custom resources) because of the interaction between these two webhooks, which are run on all admission requests, and #61355 (Unable to create/update CRD when mutating webhook configured) when the two tests are run at around the same time.

This PR will make the effects of the webhook e2e test on other tests easier to reason about.

The flakiness is being caused by a legitimate failure of the underlying code (That CRDs currently don't work with webhooks), but not a failure which either of "Garbage collector should support cascading deletion of custom resources" or "AdmissionWebhooks Should not be able to prevent deleting validating-webhook-configurations or mutating-webhook-configurations" is supposed to reliably catch.

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 #59997

Release note:

NONE

/sig api-machinery
/kind flake

jennybuckley · 2018-03-21T21:27:33Z

To support this claim, https://storage.googleapis.com/k8s-gubernator/triage/index.html?pr=1&text=internal%20error&test=should%20support%20cascading%20deletion%20of%20custom%20resources shows that the error only starts occuring around 16 days ago, right when my PR to add that webhook test was merged.

jennybuckley · 2018-03-21T21:29:08Z

/priority failing-test
/priority important-soon

caesarxuchao · 2018-03-21T22:05:32Z

test/e2e/apimachinery/webhook.go

-						Resources:   []string{"*"},
+						APIGroups:   []string{""},
+						APIVersions: []string{"v1"},
+						Resources:   []string{"pods"},


Maybe change it to some non-exist resources? Intercepting CRUD of "pods" can still break other tests.

jennybuckley · 2018-03-21T22:32:26Z

/test pull-kubernetes-e2e-kops-aws

jennybuckley · 2018-03-21T22:51:52Z

/test pull-kubernetes-integration

jennybuckley · 2018-03-21T23:10:53Z

/test pull-kubernetes-e2e-kops-aws

caesarxuchao · 2018-03-21T23:41:51Z

/lgtm
/approve

janetkuo · 2018-03-21T23:56:03Z

/approve

k8s-ci-robot · 2018-03-21T23:56:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: caesarxuchao, janetkuo, jennybuckley

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:

~~test/OWNERS~~ [janetkuo]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

liggitt · 2018-03-21T22:25:25Z

test/e2e/apimachinery/webhook.go

-						Resources:   []string{"*"},
+						APIGroups:   []string{""},
+						APIVersions: []string{"v1"},
+						Resources:   []string{"invalid"},


If you want to make sure a failed hook can't prevent its own removal, you should match the webhook configuration resource and the delete operation, right?

There is actually one configuration already doing that, these ones are there to test if a hook can prevent deleting another hook's configuration (to prevent a cycle of hooks preventing each other's deletions)

https://github.com/jennybuckley/kubernetes/blob/cd06e4db1fa8c2eda0e08d8699964fa41fca3cfa/test/e2e/apimachinery/webhook.go#L737-L748

these ones are there to test if a hook can prevent deleting another hook's configuration

I don't see how selecting an invalid resource tests that case

These webhook configurations should be able to be created and deleted without interference from the webhook I linked above. If webhooks are able to prevent deletion of other webhook configurations, the webhook I linked above will cause this check to fail, which it did, as shown in this test result from #59840 with just the test commit and not the fix:

test/e2e/apimachinery/webhook.go:147 deleting webhook config e2e-test-should-be-removable-validating-webhook-config with namespace e2e-tests-webhook-qlmqf Expected error: <*errors.StatusError | 0xc421454b40>: { ErrStatus: { TypeMeta: {Kind: "", APIVersion: ""}, ListMeta: {SelfLink: "", ResourceVersion: "", Continue: ""}, Status: "Failure", Message: "admission webhook \"deny-webhook-configuration-deletions.k8s.io\" denied the request: this webhook denies all requests", Reason: "", Details: nil, Code: 500, }, } admission webhook "deny-webhook-configuration-deletions.k8s.io" denied the request: this webhook denies all requests not to have occurred test/e2e/apimachinery/webhook.go:815

The test goes like this:

Create a webhook which is configured to prevent deletion of ValidatingWebhookConfiguration and MutatingWebhookConfiguration objects

Attempt to create and delete a dummy ValidatingWebhookConfiguration

Attempt to create and delete a dummy MutatingWebhookConfiguration

Attempt to delete the webhook registered in (1) using this cleanup function which is called here

I see, thanks.

liggitt · 2018-03-21T22:25:37Z

test/e2e/apimachinery/webhook.go

-						Resources:   []string{"*"},
+						APIGroups:   []string{""},
+						APIVersions: []string{"v1"},
+						Resources:   []string{"invalid"},


k8s-github-robot · 2018-03-22T14:15:24Z

Automatic merge from submit-queue (batch tested with PRs 61378, 60915, 61499, 61507, 61478). If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot requested review from deads2k and liggitt March 21, 2018 21:24

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 21, 2018

k8s-ci-robot added kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Mar 21, 2018

caesarxuchao reviewed Mar 21, 2018

View reviewed changes

Remove wildcard matching of no-op test webhooks

cd06e4d

jennybuckley force-pushed the crd-test-flake branch from d73643e to cd06e4d Compare March 21, 2018 22:14

k8s-ci-robot assigned caesarxuchao Mar 21, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 21, 2018

janetkuo approved these changes Mar 21, 2018

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 21, 2018

jennybuckley changed the title ~~Remove wildcard matching from no-op e2e test webhooks~~ Fix "Garbage collector should support cascading deletion of custom resources" flake Mar 21, 2018

liggitt reviewed Mar 22, 2018

View reviewed changes

k8s-github-robot merged commit 2919818 into kubernetes:master Mar 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix "Garbage collector should support cascading deletion of custom resources" flake #61499

Fix "Garbage collector should support cascading deletion of custom resources" flake #61499

jennybuckley commented Mar 21, 2018 •

edited

Loading

jennybuckley commented Mar 21, 2018 •

edited

Loading

jennybuckley commented Mar 21, 2018

caesarxuchao Mar 21, 2018

jennybuckley commented Mar 21, 2018

jennybuckley commented Mar 21, 2018

jennybuckley commented Mar 21, 2018

caesarxuchao commented Mar 21, 2018

janetkuo commented Mar 21, 2018

k8s-ci-robot commented Mar 21, 2018

liggitt Mar 21, 2018

jennybuckley Mar 22, 2018 •

edited

Loading

jennybuckley Mar 22, 2018 •

edited

Loading

liggitt Mar 22, 2018

jennybuckley Mar 22, 2018 •

edited

Loading

jennybuckley Mar 22, 2018 •

edited

Loading

liggitt Mar 22, 2018

liggitt Mar 21, 2018

k8s-github-robot commented Mar 22, 2018

Fix "Garbage collector should support cascading deletion of custom resources" flake #61499

Fix "Garbage collector should support cascading deletion of custom resources" flake #61499

Conversation

jennybuckley commented Mar 21, 2018 • edited Loading

jennybuckley commented Mar 21, 2018 • edited Loading

jennybuckley commented Mar 21, 2018

caesarxuchao Mar 21, 2018

Choose a reason for hiding this comment

jennybuckley commented Mar 21, 2018

jennybuckley commented Mar 21, 2018

jennybuckley commented Mar 21, 2018

caesarxuchao commented Mar 21, 2018

janetkuo commented Mar 21, 2018

k8s-ci-robot commented Mar 21, 2018

liggitt Mar 21, 2018

Choose a reason for hiding this comment

jennybuckley Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

jennybuckley Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

liggitt Mar 22, 2018

Choose a reason for hiding this comment

jennybuckley Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

jennybuckley Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

liggitt Mar 22, 2018

Choose a reason for hiding this comment

liggitt Mar 21, 2018

Choose a reason for hiding this comment

k8s-github-robot commented Mar 22, 2018

jennybuckley commented Mar 21, 2018 •

edited

Loading

jennybuckley commented Mar 21, 2018 •

edited

Loading

jennybuckley Mar 22, 2018 •

edited

Loading

jennybuckley Mar 22, 2018 •

edited

Loading

jennybuckley Mar 22, 2018 •

edited

Loading

jennybuckley Mar 22, 2018 •

edited

Loading