daemon: Fix the "close of closed channel" panic #11056

Sen666666 · 2020-04-20T09:37:46Z

When enable ipv4&ipv6 with crd ipam option，cilium panic after restoring
old endpoint because of "close of closed channel"

We should check whether or not the channel has been closed, before we do
the action.

Signed-off-by: yangxingwu yangxingwu@kuaishou.com
Signed-off-by: huangxuesen huangxuesen@kuaishou.com

maintainer-s-little-helper · 2020-04-20T09:37:48Z

Please set the appropriate release note label.

aanm · 2020-04-20T09:42:45Z

@Sen666666 Thank you for your PR. Do you have the golang's stack trace for this panic?

Sen666666 · 2020-04-20T09:51:49Z

@aanm yeap

cilium-agent[25601]: panic: close of closed channel
cilium-agent[25601]: goroutine 1 [running]:
cilium-agent[25601]: github.com/cilium/cilium/pkg/ipam.(*crdAllocator).RestoreFinished(0xc000963540)
cilium-agent[25601]: /home/vagrant/go/src/github.com/cilium/cilium/pkg/ipam/crd.go:685 +0x32
cilium-agent[25601]: github.com/cilium/cilium/daemon/cmd.NewDaemon(0x2773260, 0xc0000ca140, 0x27b6680, 0xc00017d8f0, 0x2790960, 0xc0003fbc60, 0x27b6680, 0xc00017d8f0)
cilium-agent[25601]: /home/vagrant/go/src/github.com/cilium/cilium/daemon/cmd/daemon.go:489 +0x21f4
cilium-agent[25601]: github.com/cilium/cilium/daemon/cmd.runDaemon()
cilium-agent[25601]: /home/vagrant/go/src/github.com/cilium/cilium/daemon/cmd/daemon_main.go:1202 +0x34a
cilium-agent[25601]: github.com/cilium/cilium/daemon/cmd.glob..func1(0x3aa0aa0, 0xc0004ee7e0, 0x0, 0x11)
cilium-agent[25601]: /home/vagrant/go/src/github.com/cilium/cilium/daemon/cmd/daemon_main.go:103 +0x7b
cilium-agent[25601]: github.com/spf13/cobra.(*Command).execute(0x3aa0aa0, 0xc0000cc010, 0x11, 0x11, 0x3aa0aa0, 0xc0000cc010)
cilium-agent[25601]: /home/vagrant/go/src/github.com/cilium/cilium/vendor/github.com/spf13/cobra/command.go:830 +0x2aa
cilium-agent[25601]: github.com/spf13/cobra.(*Command).ExecuteC(0x3aa0aa0, 0xc00009cde0, 0x0, 0x0)
cilium-agent[25601]: /home/vagrant/go/src/github.com/cilium/cilium/vendor/github.com/spf13/cobra/command.go:914 +0x2fb
cilium-agent[25601]: github.com/spf13/cobra.(*Command).Execute(...)
cilium-agent[25601]: /home/vagrant/go/src/github.com/cilium/cilium/vendor/github.com/spf13/cobra/command.go:864
cilium-agent[25601]: github.com/cilium/cilium/daemon/cmd.Execute()
cilium-agent[25601]: /home/vagrant/go/src/github.com/cilium/cilium/daemon/cmd/daemon_main.go:134 +0x184
cilium-agent[25601]: main.main()
cilium-agent[25601]: /home/vagrant/go/src/github.com/cilium/cilium/daemon/main.go:22 +0x20

coveralls · 2020-04-20T10:29:42Z

Coverage increased (+0.02%) to 44.683% when pulling 11e6809 on Sen666666:master into 5c0b426 on cilium:master.

aanm

@Sen666666 thanks for the stack trace. I think with a once.Do will more readable.

aanm · 2020-04-20T14:48:01Z

pkg/ipam/crd.go

@@ -623,5 +623,10 @@ func (a *crdAllocator) Dump() (map[string]string, string) {

 // RestoreFinished marks the status of restoration as done
 func (a *crdAllocator) RestoreFinished() {
-	close(a.store.restoreFinished)


please use a Once.Do here, it will guarantee that this call is only executed once. (The Once will be a field of nodeStore.

thanks for the suggestion, I will try to do some modification.

aanm · 2020-04-20T14:51:00Z

Regression introduced by #9888

Sen666666 · 2020-04-21T02:32:19Z

use once.Do to make the check more readable

When enable ipv4&ipv6 with crd ipam option, cilium panic after restoring old endpoint because of "close of closed channel" Use once.Do to make sure the close action only happen once. Signed-off-by: yangxingwu <yangxingwu@kuaishou.com> Signed-off-by: huangxuesen <huangxuesen@kuaishou.com>

pchaigno · 2020-04-21T07:26:17Z

test-me-please

GKE failed with:

Cilium cannot be installed
Expected
    <*errors.errorString | 0xc0004c6100>: {
        s: "could not find pods in namespace 'cilium' with label 'io.cilium/app=operator': signal: killed",
    }
to be nil

pchaigno · 2020-04-21T13:05:27Z

test-gke

jaffcheng

Sorry for the regression introduced by me, I think we should ensure endpoints of both stacks are restored

jaffcheng · 2020-04-21T13:08:34Z

pkg/ipam/crd.go

@@ -623,5 +624,7 @@ func (a *crdAllocator) Dump() (map[string]string, string) {

 // RestoreFinished marks the status of restoration as done
 func (a *crdAllocator) RestoreFinished() {


The aim here is to restore all endpoints before triggering sync with upstream, so I think we should close a.store.restoreFinished only after the restoration of both IPv6Allocator and IPv4Allocator are finished. They are called from here:

cilium/daemon/cmd/daemon.go

Lines 491 to 498 in 90b256e

if option.Config.IPAM == option.IPAMCRD || option.Config.IPAM == option.IPAMENI || option.Config.IPAM == option.IPAMAzure {

if option.Config.EnableIPv6 {

d.ipam.IPv6Allocator.RestoreFinished()

}

if option.Config.EnableIPv4 {

d.ipam.IPv4Allocator.RestoreFinished()

}

}

@jaffcheng @Sen666666 Looking at the code, this comment seems to be a potential further fix that should go in at some point, but does not directly interact with this specific PR #11056 that just fixes a specific crash issue. I'd like to merge this PR as-is to fix the crash, then you can follow up to address these comments. Does that sound reasonable?

Yes, let's address this comment in a follow-up PR.

pchaigno · 2020-04-23T06:32:39Z

test-gke

pchaigno · 2020-04-23T08:32:31Z

Merging despite GKE build being stuck because 1) GKE builds are not required to merge anymore, and 2) failure n-1 was flake #11117 and affected different test case than failure n-2.

Sen666666 requested a review from a team as a code owner April 20, 2020 09:37

maintainer-s-little-helper bot added the dont-merge/needs-release-note label Apr 20, 2020

maintainer-s-little-helper bot added this to In progress in 1.8.0 Apr 20, 2020

aanm added kind/bug This is a bug in the Cilium logic. pending-review release-note/bug This PR fixes an issue in a previous release of Cilium. labels Apr 20, 2020

maintainer-s-little-helper bot removed the dont-merge/needs-release-note label Apr 20, 2020

Sen666666 force-pushed the master branch from ec85e72 to e82b5d2 Compare April 20, 2020 09:45

aanm requested changes Apr 20, 2020

View reviewed changes

aanm added kind/regression This functionality worked fine before, but was broken in a newer release of Cilium. needs-backport/1.7 labels Apr 20, 2020

maintainer-s-little-helper bot added this to Needs backport from master in 1.7.3 Apr 20, 2020

Sen666666 force-pushed the master branch from e82b5d2 to 48a6e4a Compare April 21, 2020 02:30

Sen666666 force-pushed the master branch from 48a6e4a to 11e6809 Compare April 21, 2020 02:37

jaffcheng suggested changes Apr 21, 2020

View reviewed changes

aanm approved these changes Apr 21, 2020

View reviewed changes

joestringer added the priority/release-blocker label Apr 21, 2020

pchaigno mentioned this pull request Apr 23, 2020

CI: GKE: Policy cannot be applied in "default" namespace #11117

Closed

pchaigno approved these changes Apr 23, 2020

View reviewed changes

pchaigno merged commit 2412167 into cilium:master Apr 23, 2020

1.8.0 automation moved this from In progress to Merged Apr 23, 2020

jrajahalme mentioned this pull request Apr 28, 2020

v1.7 backports 2020-04-28 #11200

Merged

jrajahalme added backport-pending/1.7 and removed needs-backport/1.7 labels Apr 28, 2020

maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.7 in 1.7.3 Apr 28, 2020

qmonnet added backport-done/1.7 and removed backport-pending/1.7 labels Apr 29, 2020

maintainer-s-little-helper bot moved this from Backport pending to v1.7 to Backport done to v1.7 in 1.7.3 Apr 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

daemon: Fix the "close of closed channel" panic #11056

daemon: Fix the "close of closed channel" panic #11056

Sen666666 commented Apr 20, 2020

maintainer-s-little-helper bot commented Apr 20, 2020

aanm commented Apr 20, 2020

Sen666666 commented Apr 20, 2020

coveralls commented Apr 20, 2020 •

edited

aanm left a comment

aanm Apr 20, 2020

Sen666666 Apr 21, 2020

aanm commented Apr 20, 2020

Sen666666 commented Apr 21, 2020

pchaigno commented Apr 21, 2020 •

edited

pchaigno commented Apr 21, 2020

jaffcheng left a comment

jaffcheng Apr 21, 2020

joestringer Apr 22, 2020 •

edited

jaffcheng Apr 23, 2020

pchaigno commented Apr 23, 2020

pchaigno commented Apr 23, 2020

		@@ -623,5 +624,7 @@ func (a *crdAllocator) Dump() (map[string]string, string) {

		// RestoreFinished marks the status of restoration as done
		func (a *crdAllocator) RestoreFinished() {

	if option.Config.IPAM == option.IPAMCRD \|\| option.Config.IPAM == option.IPAMENI \|\| option.Config.IPAM == option.IPAMAzure {
	if option.Config.EnableIPv6 {
	d.ipam.IPv6Allocator.RestoreFinished()
	}
	if option.Config.EnableIPv4 {
	d.ipam.IPv4Allocator.RestoreFinished()
	}
	}

daemon: Fix the "close of closed channel" panic #11056

daemon: Fix the "close of closed channel" panic #11056

Conversation

Sen666666 commented Apr 20, 2020

maintainer-s-little-helper bot commented Apr 20, 2020

aanm commented Apr 20, 2020

Sen666666 commented Apr 20, 2020

coveralls commented Apr 20, 2020 • edited

aanm left a comment

Choose a reason for hiding this comment

aanm Apr 20, 2020

Choose a reason for hiding this comment

Sen666666 Apr 21, 2020

Choose a reason for hiding this comment

aanm commented Apr 20, 2020

Sen666666 commented Apr 21, 2020

pchaigno commented Apr 21, 2020 • edited

pchaigno commented Apr 21, 2020

jaffcheng left a comment

Choose a reason for hiding this comment

jaffcheng Apr 21, 2020

Choose a reason for hiding this comment

joestringer Apr 22, 2020 • edited

Choose a reason for hiding this comment

jaffcheng Apr 23, 2020

Choose a reason for hiding this comment

pchaigno commented Apr 23, 2020

pchaigno commented Apr 23, 2020

coveralls commented Apr 20, 2020 •

edited

pchaigno commented Apr 21, 2020 •

edited

joestringer Apr 22, 2020 •

edited