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.

Sign up for GitHub

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

Jump to bottom

daemonset handle DeletedFinalStateUnknown #25913

Merged
merged 1 commit into from
May 31, 2016
Merged

daemonset handle DeletedFinalStateUnknown #25913

merged 1 commit into from
May 31, 2016
+31 −5

Conversation

pweil-
Copy link
Contributor

@pweil- pweil- commented May 19, 2016

During an e2e run in OpenShift we ran into the DS controller panic when handling DeletedFinalStateUnknown. This PR checks for DeletedFinalStateUnknown and queues the embedded object if it is a DaemonSet.

@mikedanese - would you mind taking a look?
@deads2k

panic: interface conversion: interface is cache.DeletedFinalStateUnknown, not *extensions.DaemonSet

goroutine 4369 [running]:
k8s.io/kubernetes/pkg/controller/daemon.func·005(0x2f8a0c0, 0xc20b559680)
    /data/src/github.com/openshift/origin/Godeps/_workspace/src/k8s.io/kubernetes/pkg/controller/daemon/controller.go:160 +0x50
k8s.io/kubernetes/pkg/controller/framework.ResourceEventHandlerFuncs.OnDelete(0xc20a0ae090, 0xc20a0ae0a0, 0xc20a0ae0b0, 0x2f8a0c0, 0xc20b559680)
    /data/src/github.com/openshift/origin/Godeps/_workspace/src/k8s.io/kubernetes/pkg/controller/framework/controller.go:178 +0x41
k8s.io/kubernetes/pkg/controller/framework.(*ResourceEventHandlerFuncs).OnDelete(0xc20b8ebf20, 0x2f8a0c0, 0xc20b559680)
    <autogenerated>:25 +0xb5
k8s.io/kubernetes/pkg/controller/framework.func·001(0x2f8a280, 0xc20b5522e0, 0x0, 0x0)
    /data/src/github.com/openshift/origin/Godeps/_workspace/src/k8s.io/kubernetes/pkg/controller/framework/controller.go:248 +0x4be
k8s.io/kubernetes/pkg/controller/framework.(*Controller).processLoop(0xc20bb727e0)
    /data/src/github.com/openshift/origin/Godeps/_workspace/src/k8s.io/kubernetes/pkg/controller/framework/controller.go:122 +0x6f
k8s.io/kubernetes/pkg/controller/framework.*Controller.(k8s.io/kubernetes/pkg/controller/framework.processLoop)·fm()
    /data/src/github.com/openshift/origin/Godeps/_workspace/src/k8s.io/kubernetes/pkg/controller/framework/controller.go:97 +0x27
k8s.io/kubernetes/pkg/util/wait.func·001()
    /data/src/github.com/openshift/origin/Godeps/_workspace/src/k8s.io/kubernetes/pkg/util/wait/wait.go:66 +0x61
k8s.io/kubernetes/pkg/util/wait.JitterUntil(0xc209f8cfb8, 0x3b9aca00, 0x0, 0xc2080543c0)
    /data/src/github.com/openshift/origin/Godeps/_workspace/src/k8s.io/kubernetes/pkg/util/wait/wait.go:67 +0x8f
k8s.io/kubernetes/pkg/util/wait.Until(0xc209f8cfb8, 0x3b9aca00, 0xc2080543c0)
    /data/src/github.com/openshift/origin/Godeps/_workspace/src/k8s.io/kubernetes/pkg/util/wait/wait.go:47 +0x4a
k8s.io/kubernetes/pkg/controller/framework.(*Controller).Run(0xc20bb727e0, 0xc2080543c0)
    /data/src/github.com/openshift/origin/Godeps/_workspace/src/k8s.io/kubernetes/pkg/controller/framework/controller.go:97 +0x1fb
created by k8s.io/kubernetes/pkg/controller/daemon.(*DaemonSetsController).Run
    /data/src/github.com/openshift/origin/Godeps/_workspace/src/k8s.io/kubernetes/pkg/controller/daemon/controller.go:212 +0xae

https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_check/1002/artifact/origin/artifacts/test-cmd/logs/openshift.log

@pweil- pweil- mentioned this pull request May 19, 2016
Closed
@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels May 19, 2016
@mikedanese mikedanese assigned mikedanese and unassigned bprashanth May 19, 2016
@bprashanth
Copy link
Contributor

Can you confirm that none of the other controllers suffer from the same bug? I don't think they do, I think the keyfunc should handle nil gracefully?

@pweil-
Copy link
Contributor Author

pweil- commented May 20, 2016

@k8s-bot test this please flake: #25953

@pweil-
Copy link
Contributor Author

pweil- commented May 20, 2016

@bprashanth it looks like the other controllers that define a delete function were accounting for it by using the keyfunc or directly accessing the tombstone when necessary. Below are the items that I found with delete functions:

  • deployment
  • endpoints
  • job
  • pet set
  • replica set
  • replentishment
  • resource quota
  • service account (doesn't check tombstone but checks ok on cast and notes that a missed delete will be cleaned up)
  • tokens (same as service account)
  • attach_detach (not accounting for it but the impl methods are no ops)

@pweil-
Copy link
Contributor Author

pweil- commented May 20, 2016

I think the keyfunc should handle nil gracefully

It doesn't check for nil but it does provide a check for the DeleteFinalStateUnknown and will use the key from that object or return the cache key.

func DeletionHandlingMetaNamespaceKeyFunc(obj interface{}) (string, error) {
    if d, ok := obj.(cache.DeletedFinalStateUnknown); ok {
        return d.Key, nil
    }
    return cache.MetaNamespaceKeyFunc(obj)
}

@mikedanese
Copy link
Member

LGTM

@mikedanese mikedanese added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 20, 2016
@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 20, 2016
@mikedanese mikedanese added release-note-none Denotes a PR that doesn't merit a release note. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels May 20, 2016
@pweil- pweil- mentioned this pull request May 24, 2016
Closed
@wojtek-t
Copy link
Member

@k8s-bot unit test this please, issue: #26345

@wojtek-t wojtek-t added this to the v1.3 milestone May 30, 2016
@k8s-bot
Copy link

k8s-bot commented May 30, 2016

GCE e2e build/test passed for commit 4d6fee7.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented May 31, 2016

GCE e2e build/test passed for commit 4d6fee7.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit d1277e3 into kubernetes:master May 31, 2016
@0xmichalis 0xmichalis mentioned this pull request Jun 23, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@mikedanese mikedanese

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.3
Development

Successfully merging this pull request may close these issues.
7 participants
@pweil- @bprashanth @mikedanese @wojtek-t @k8s-bot @k8s-github-robot @googlebot