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

TPR objects still exist after deleting namespaces #46736

Closed
hongchaodeng opened this issue May 31, 2017 · 18 comments · Fixed by #47086
Closed

TPR objects still exist after deleting namespaces #46736

hongchaodeng opened this issue May 31, 2017 · 18 comments · Fixed by #47086
Assignees
Labels
area/custom-resources sig/cli Categorizes an issue or PR as relevant to SIG CLI.

Comments

@hongchaodeng
Copy link
Contributor

hongchaodeng commented May 31, 2017

BUG REPORT

Kubernetes version (use kubectl version): v1.6.2

Environment:

  • Cloud provider or hardware configuration: GKE

What happened:

  • Create TPR "MyResource"
  • Create namespace "soak-test"
  • Create a "MyResource" "a" under "soak-test":
$ cat my-resource-a.yaml
apiVersion: "example.com/v1"
kind: "MyResource"
metadata:
  name: "a"
...
$ kubectl --namespace soak-test create -f my-resource-a.yaml
$ kubectl --namespace soak-test get MyResource
NAME           LABELS    DATA
a           ...
  • Delete namespace "soak-test"
$ kubectl delete ns soak-test
  • wait namespace to be deleted

MyResource "a" still exists after ns is deleted.

$ kubectl --namespace soak-test get MyResource
NAMESPACE   NAME                          KIND
soak-test   a
@xiangpengzhao
Copy link
Contributor

/area third-party-resource

@k8s-github-robot k8s-github-robot added the needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. label May 31, 2017
@xiangpengzhao
Copy link
Contributor

/sig cli

@k8s-ci-robot k8s-ci-robot added the sig/cli Categorizes an issue or PR as relevant to SIG CLI. label Jun 1, 2017
@k8s-github-robot k8s-github-robot removed the needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. label Jun 1, 2017
@xingzhou
Copy link
Contributor

xingzhou commented Jun 1, 2017

I'd like to take a look at the problem if nobody else is working on this

@suyogbarve
Copy link
Contributor

@hongchaodeng @deads2k @xingzhou Root Cause of this is the fact that Namespace on ThirdPartyResources is getting ignored (both passed as command --namespace, or the one present in yaml)
ThirdPartyResources always end up in default namespace (which is causing confusion reported in bug)

@deads2k
Copy link
Contributor

deads2k commented Jun 5, 2017

@hongchaodeng @deads2k @xingzhou Root Cause of this is the fact that Namespace on ThirdPartyResources is getting ignored (both passed as command --namespace, or the one present in yaml)
ThirdPartyResources always end up in default namespace (which is causing confusion reported in bug)

How long has that bug existed? @enisoc might related to your other findings.

We know that bug doesn't exist in CRDs (we have tests specifically checking namespace handling), so I'd suggest converting to those as soon as possible.

@hongchaodeng
Copy link
Contributor Author

@deads2k
Isn't CRD available in 1.7+ ?

@deads2k
Copy link
Contributor

deads2k commented Jun 5, 2017

@deads2k
Isn't CRD available in 1.7+ ?

Yes. If the bug has existed for multiple releases (not a regression), I don't think this is a blocker for 1.7. It could still be patched, just not a blocker for the current release.

@hongchaodeng
Copy link
Contributor Author

Migration to CRD still is not automated and will still take some time. Have you considered that?

@deads2k
Copy link
Contributor

deads2k commented Jun 5, 2017

Migration to CRD still is not automated and will still take some time. Have you considered that?

Yes, but if the behavior is not a regression, then this does not block the release. The fact that we have a clear path forward to a working resource is gravy.

@hongchaodeng
Copy link
Contributor Author

if the behavior is not a regression

This bug existed in 1.4 (#32306), got fixed in 1.5, but appears again in 1.6 .

@deads2k deads2k removed this from the v1.7 milestone Jun 5, 2017
@deads2k
Copy link
Contributor

deads2k commented Jun 5, 2017

This bug existed in 1.4 (#32306), got fixed in 1.5, but appears again in 1.6 .

Ok. Not a blocker for 1.7, but if the patch is reasonable as a fix we can merge it into the service stream. For 1.7, I'd still recommend moving to CRD as quickly as possible.

@enisoc
Copy link
Member

enisoc commented Jun 5, 2017

@deads2k I was able to reproduce this with CRD on master HEAD. What level of tests are there? Anything e2e?

@deads2k
Copy link
Contributor

deads2k commented Jun 5, 2017

@enisoc most of them and specifically a storage one. You're saying we have the ThirdPartyResources always end up in default namespace (which is causing confusion reported in bug) from above?

@enisoc
Copy link
Member

enisoc commented Jun 5, 2017

@deads2k No, I don't see any evidence that they always end up in the default namespace (neither for TPR nor CRD). I'm not sure what that's referring to.

What I was able to reproduce is the bug described at the top of this issue: Create item in namespace, delete namespace, item remains. I observed this for TPR on release-1.6 and master, and for CRD on master.

@enisoc
Copy link
Member

enisoc commented Jun 5, 2017

The problem seems to be that thirdpartyresources is misspelled as thirdpartyresource in this line:

https://github.com/kubernetes/kubernetes/blob/0cff839/cmd/kube-controller-manager/app/core.go#L257-L266

If I fix that, namespace cleanup works for TPR. However, for CRD, now namespace cleanup gets stuck and controller manager logs this:

namespace_controller.go:148] an error on the server ("unknown") has prevented the request from succeeding (delete crontabs.stable.example.com)

Edit: To clarify, I "fixed" the check in the linked code by removing the entire highlighted block. It seems like the singular thirdpartyresource is not the only problem. It was also using the internal version of extensions when v1beta1 was expected.

@enisoc
Copy link
Member

enisoc commented Jun 5, 2017

Regarding namespace cleanup getting stuck for CRD after the "fix", the error on the apiserver side is:

status.go:62] apiserver received an error that is not an metav1.Status: Object 'Kind' is missing in '{"orphanDependents":false}

According to the stack trace, the error comes from here:

https://github.com/kubernetes/kubernetes/blob/0cff839/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest.go#L1102

@xingzhou
Copy link
Contributor

xingzhou commented Jun 6, 2017

@enisoc, yes, not only the thirdpartyresources is misspelled, but also the mismatch of the version. this is why in my original patch, I tried to ignore the version of thirdpartyresources.

k8s-github-robot pushed a commit that referenced this issue Jun 6, 2017
Automatic merge from submit-queue (batch tested with PRs 44883, 46836, 46765, 46683, 46050)

While deleting a namespace, the TPR instances under this ns should be…

… deleted.

While deleting a namespace, the TPR instances under this ns should be deleted.

Fixed #46736 

**Release note**:
```
None
```
@enisoc
Copy link
Member

enisoc commented Jun 6, 2017

I'd like to keep this open until we have a regression test since this is at least the third time we've fixed this. I'll add a test as part of making sure this fix works for CRD.

@enisoc enisoc reopened this Jun 6, 2017
@enisoc enisoc self-assigned this Jun 6, 2017
k8s-github-robot pushed a commit that referenced this issue Jun 7, 2017
Automatic merge from submit-queue (batch tested with PRs 47024, 47050, 47086, 47081, 47013)

apiextensions-apiserver: Fix decoding of DeleteOptions.

Fixes #47072 by making apiextensions-apiserver capable of decoding unversioned DeleteOptions, rather than only handling Unstructured objects (i.e. Custom Resources).

This also closes #46736 and #37554 since the added regression test works for TPR as well.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/custom-resources sig/cli Categorizes an issue or PR as relevant to SIG CLI.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants