[WIP] Add events for pending namespace deletions

[wozniakjan]

What type of PR is this?
/kind feature
/sig cli
/sig api-machinery

What this PR does / why we need it:
For users to understand what is preventing the namespace deletion and not remove kubernetes finalizer from namespaces: #64002, #60807, #66735

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

Special notes for your reviewer:
sample output

$ kubectl describe ns test
Name:         test
Labels:       <none>
Annotations:  <none>
Status:       Terminating

No resource quota.

No resource limits.
Events:
  Type     Reason            Age                  From                  Message
  ----     ------            ----                 ----                  -------
  Warning  FailedDiscovery   2m8s (x18 over 12m)  namespace-controller  Failed to discover resources: unable to retrieve the complete list of server APIs: mutators.kubedb.com/v1alpha1: the server is currently unable to handle the request

Does this PR introduce a user-facing change?:

NONE

[k8s-ci-robot]

Hi @wozniakjan. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[liggitt]

thanks for the pull request, but if the namespace is terminating, no events will be allowed to be created in it

[wozniakjan]

interesting, this differs from my observation. I clearly see a namespace in terminating with events created for it. Do you think this will be better implemented as status condition?

[liggitt]

interesting, this differs from my observation. I clearly see a namespace in terminating with events created for it.

I added my own finalizer to a namespace to get it to stick in terminating mode:

$ kubectl get ns foo -o yaml
apiVersion: v1
kind: Namespace
metadata:
  creationTimestamp: "2018-11-27T14:13:59Z"
  deletionTimestamp: "2018-11-27T14:20:25Z"
  name: foo
  resourceVersion: "346"
  selfLink: /api/v1/namespaces/foo
  uid: aef36ed2-f24e-11e8-8fbd-8a006092ee01
spec:
  finalizers:
  - foo.bar.com/foo
status:
  phase: Terminating

then tried to create an event in it:

$ more event.json 
{"kind":"Event","apiVersion":"v1","metadata":{"name":"foo","namespace":"foo"},"involvedObject":{}}

$ kubectl create -f event.json 
Error from server (Forbidden): error when creating "event.json": events "foo" is forbidden: unable to create new content in namespace foo because it is being terminated

what invocation are you using to start your API server? do you have the NamespaceLifecycle admission plugin enabled? It is enabled by default unless you override admission plugins with --admission-plugins and omit it

[dims]

/ok-to-test

[wozniakjan]

what invocation are you using to start your API server? do you have the NamespaceLifecycle admission plugin enabled? It is enabled by default unless you override admission plugins with --admission-plugins and omit it

@liggitt I believe NamespaceLifecycle is enabled

--admission-control=Initializers,NamespaceLifecycle,LimitRanger,ServiceAccount,DefaultStorageClass,MutatingAdmissionWebhook,ValidatingAdmissionWebhook,ResourceQuota

I examined the event

LAST SEEN   FIRST SEEN   COUNT     NAME                                                        KIND        SUBOBJECT                                TYPE      REASON             SOURCE                                   MESSAGE
3m          1h           28        test.156afe43094989d5                                       Namespace                                            Warning   FailedDiscovery    namespace-controller                     Failed to discover resources: unable to retrieve the complete list of server APIs: mutators.kubedb.com/v1alpha1: the server is currently unable to handle the request

The involved object is populated correctly but namespace is actually default

Name:             test.156afe43094989d5
Namespace:        default
...
Involved Object:
  API Version:       v1
  Kind:              Namespace
  Name:              test
  Resource Version:  17608019
  UID:               b09b5e9f-f23a-11e8-b89b-fa163e041cac

And I suspect the culprit is this part of the code
https://github.com/kubernetes/kubernetes/blob/7c8274ecdfb885daf606adf222b45af0e39b151a/pkg/controller/namespace/namespace_controller.go#L77

I will take a look how persistent volume controller implements the events, PVs are "also" not namespaced so there should be a pattern I was supposed to follow I think

Thanks for extremely valuable and super quick feedback :)

[wozniakjan]

@dims thanks for running the tests, I completely forgot about checking if I didn't break any (turns out I did)

[fedebongio]

/cc @logicalhan

[k8s-ci-robot]

@fedebongio: GitHub didn't allow me to request PR reviews from the following users: logicalhan.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @logicalhan

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.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: wozniakjan
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: deads2k

If they are not already assigned, you can assign the PR to them by writing /assign @deads2k in a comment when ready.

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:

• pkg/controller/OWNERS
• pkg/kubectl/OWNERS

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]

@wozniakjan: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-gce-device-plugin-gpu	919c0a1	link	/test pull-kubernetes-e2e-gce-device-plugin-gpu

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.

[k8s-ci-robot]

@wozniakjan: PR needs rebase.

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.

[wozniakjan]

closing in favour of #73405