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

Handle namespace deletion more gracefully in built-in controllers #84123

Merged
merged 7 commits into from Nov 4, 2019

Conversation

@smarterclayton
Copy link
Contributor

smarterclayton commented Oct 20, 2019

Kubernetes workload controllers deal poorly with namespaces being terminated and often go into tight backoff loops during shutdown. Since namespace termination cannot be reversed, for the majority of controllers when we detect this scenario we should eat / drop / exit the loop we are in and wait for more events. A more sophisticated mechanism could remember that a namespace was being terminated and dequeue all events that impact that namespace, but that also has to contend with races for a new namespace of the same name and is more invasive. In general, detect the namespace terminating error and exit fast, avoiding spawning other events or errors (since event creation is another example of a failure).

To make this possible, clients should be able to identify when a namespace is being terminated and
take special action such as backing off or giving up. Add a helper for getting the cause of an error and then add a special cause to the forbidden error that namespace lifecycle admission returns. We can't change the forbidden reason without potentially breaking older clients and so cause is the
appropriate tool.

This shows up in a lot of controllers during e2e runs. In theory this should take a ton of CPU pressure off the apiserver during e2e and speed up e2e runs.

Oct 20 00:40:06.836 W endpoints/latency-svc-xn7jz Failed to create endpoint for service e2e-svc-latency-285/latency-svc-xn7jz: endpoints "latency-svc-xn7jz" is forbidden: unable to create new content in namespace e2e-svc-latency-285 because it is being terminated
Oct 20 00:40:06.848 W endpoints/latency-svc-wfb5f Failed to create endpoint for service e2e-svc-latency-285/latency-svc-wfb5f: endpoints "latency-svc-wfb5f" is forbidden: unable to create new content in namespace e2e-svc-latency-285 because it is being terminated
Oct 20 00:40:06.862 W endpoints/latency-svc-xv22f Failed to create endpoint for service e2e-svc-latency-285/latency-svc-xv22f: endpoints "latency-svc-xv22f" is forbidden: unable to create new content in namespace e2e-svc-latency-285 because it is being terminated
Oct 20 00:40:06.876 W endpoints/latency-svc-s9zkt Failed to create endpoint for service e2e-svc-latency-285/latency-svc-s9zkt: endpoints "latency-svc-s9zkt" is forbidden: unable to create new content in namespace e2e-svc-latency-285 because it is being terminated
Oct 20 00:40:06.893 W endpoints/latency-svc-29w7r Failed to create endpoint for service e2e-svc-latency-285/latency-svc-29w7r: endpoints "latency-svc-29w7r" is forbidden: unable to create new content in namespace e2e-svc-latency-285 because it is being terminated
Oct 20 00:40:06.907 W endpoints/latency-svc-g64g7 Failed to create endpoint for service e2e-svc-latency-285/latency-svc-g64g7: endpoints "latency-svc-g64g7" is forbidden: unable to create new content in namespace e2e-svc-latency-285 because it is being terminated
Oct 20 00:40:06.920 W endpoints/latency-svc-7xn4g Failed to create endpoint for service e2e-svc-latency-285/latency-svc-7xn4g: endpoints "latency-svc-7xn4g" is forbidden: unable to create new content in namespace e2e-svc-latency-285 because it is being terminated
Oct 20 00:40:06.929 W endpoints/latency-svc-r4brb Failed to create endpoint for service e2e-svc-latency-285/latency-svc-r4brb: endpoints "latency-svc-r4brb" is forbidden: unable to create new content in namespace e2e-svc-latency-285 because it is being terminated

/kind bug

During namespace deletion some controllers create event and log spam because they do not recognize namespace deletion as a terminal state.
@smarterclayton smarterclayton force-pushed the smarterclayton:terminating_cause branch from 524fc6a to cffe4b6 Oct 20, 2019
@k8s-ci-robot k8s-ci-robot requested review from enj and krousey Oct 20, 2019
@smarterclayton smarterclayton force-pushed the smarterclayton:terminating_cause branch from cffe4b6 to f7d6c67 Oct 20, 2019
@smarterclayton smarterclayton changed the title WIP: Handle namespace deletion more gracefully in built-in controllers Handle namespace deletion more gracefully in built-in controllers Oct 20, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Oct 20, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: smarterclayton

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

@smarterclayton smarterclayton force-pushed the smarterclayton:terminating_cause branch from d619699 to ec9fdc4 Oct 20, 2019
@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Oct 20, 2019

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

…ting

Clients should be able to identify when a namespace is being terminated and
take special action such as backing off or giving up. Add a helper for
getting the cause of an error and then add a special cause to the forbidden
error that namespace lifecycle admission returns. We can't change the forbidden
reason without potentially breaking older clients and so cause is the
appropriate tool.

Add `StatusCause` and `HasStatusCause` to the errors package to make checking
for causes simpler. Add `NamespaceTerminatingCause` to the v1 API as a constant.
Avoid sending an event to the namespace that is being terminated,
since it will be rejected.
In some scenarios the service account and token controllers can
race with namespace deletion, causing a burst of errors as they
attempt to recreate secrets being deleted.

Instead, detect these errors and do not retry.
Instead of reporting an event or displaying an error, simply exit
when the namespace is being terminated. This reduces the amount of
controller churn on namespace shutdown. While we could technically
exit the entire processing loop early for very large replica sets,
we should wait for more evidence that is an issue before changing
that logic substantially.
Instead of reporting an event or displaying an error, simply exit
when the namespace is being terminated. This reduces the amount of
controller churn on namespace shutdown. While we could technically
exit the entire processing loop early for very large daemon sets,
we should wait for more evidence that is an issue before changing
that logic substantially.
Instead of reporting an event or displaying an error, simply exit
when the namespace is being terminated. This reduces the amount of
controller churn on namespace shutdown. While we could technically
exit the entire processing loop early for very large jobs,
we should wait for more evidence that is an issue before changing
that logic substantially.
…sets

Instead of reporting an event or displaying an error, simply exit
when the namespace is being terminated. This reduces the amount of
controller churn on namespace shutdown. Unlike other controllers, we
drop the replica set create error very late (in the queue handleErr)
in order to avoid changing the structure of the controller
substantially.
@smarterclayton smarterclayton force-pushed the smarterclayton:terminating_cause branch from ec9fdc4 to bd92607 Oct 20, 2019
@smarterclayton

This comment has been minimized.

Copy link
Contributor Author

smarterclayton commented Oct 20, 2019

/retest

@smarterclayton

This comment has been minimized.

Copy link
Contributor Author

smarterclayton commented Oct 21, 2019

/test pull-kubernetes-e2e-gce

@fedebongio

This comment has been minimized.

Copy link
Contributor

fedebongio commented Oct 21, 2019

@k8s-ci-robot k8s-ci-robot requested review from lavalamp and yliaog Oct 21, 2019
@smarterclayton

This comment has been minimized.

Copy link
Contributor Author

smarterclayton commented Oct 24, 2019

/test pull-kubernetes-e2e-gce

@smarterclayton

This comment has been minimized.

Copy link
Contributor Author

smarterclayton commented Oct 26, 2019

/retest

@smarterclayton

This comment has been minimized.

Copy link
Contributor Author

smarterclayton commented Oct 29, 2019

@tnozicka @soltysh if you have some time to review

@smarterclayton

This comment has been minimized.

Copy link
Contributor Author

smarterclayton commented Nov 2, 2019

/test pull-kubernetes-e2e-gce

@soltysh
soltysh approved these changes Nov 4, 2019
Copy link
Contributor

soltysh left a comment

/priority critical-urgent
/lgtm
/hold
for @tnozicka

@@ -213,7 +213,10 @@ func (c *ServiceAccountsController) syncNamespace(key string) error {
sa.Namespace = ns.Name

if _, err := c.client.CoreV1().ServiceAccounts(ns.Name).Create(&sa); err != nil && !apierrs.IsAlreadyExists(err) {
createFailures = append(createFailures, err)
// we can safely ignore terminating namespace errors
if !apierrs.HasStatusCause(err, v1.NamespaceTerminatingCause) {

This comment has been minimized.

Copy link
@soltysh

soltysh Nov 4, 2019

Contributor

Nit: this could be placed next to !apierrs.IsAlreadyExists(err)

// pod when the expectation expires.
return nil
if err != nil {
if errors.HasStatusCause(err, v1.NamespaceTerminatingCause) {

This comment has been minimized.

Copy link
@soltysh

soltysh Nov 4, 2019

Contributor

Nit: not sure why you need to have two ifs if both return the same, comment for both handled in one should be sufficient.

@soltysh

This comment has been minimized.

Copy link
Contributor

soltysh commented Nov 4, 2019

@k8s-ci-robot k8s-ci-robot requested review from janetkuo and kow3ns Nov 4, 2019
Copy link
Contributor

tnozicka left a comment

/lgtm

@@ -475,7 +475,7 @@ func (dc *DeploymentController) processNextWorkItem() bool {
}

func (dc *DeploymentController) handleErr(err error, key interface{}) {
if err == nil {
if err == nil || errors.HasStatusCause(err, v1.NamespaceTerminatingCause) {

This comment has been minimized.

Copy link
@tnozicka

tnozicka Nov 4, 2019

Contributor

Will work only for unwrapped errors, but we might make it better when we get golang 1.13

I don't see the calls there wrapping the errors with context so this should be good for now

@soltysh

This comment has been minimized.

Copy link
Contributor

soltysh commented Nov 4, 2019

/hold cancel

@k8s-ci-robot k8s-ci-robot merged commit 6a19261 into kubernetes:master Nov 4, 2019
15 checks passed
15 checks passed
cla/linuxfoundation smarterclayton authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-kind Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details
@k8s-ci-robot k8s-ci-robot added this to the v1.17 milestone Nov 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.