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

⚠️ Allow configuring RecoverPanic for controllers globally #2093

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions pkg/config/v1alpha1/types.go
Expand Up @@ -94,6 +94,10 @@ type ControllerConfigurationSpec struct {
// Defaults to 2 minutes if not set.
// +optional
CacheSyncTimeout *time.Duration `json:"cacheSyncTimeout,omitempty"`

// RecoverPanic indicates if panics should be recovered.
// +optional
RecoverPanic *bool `json:"recoverPanic,omitempty"`
}

// ControllerMetrics defines the metrics configs.
Expand Down
7 changes: 6 additions & 1 deletion pkg/controller/controller.go
Expand Up @@ -56,7 +56,8 @@ type Options struct {
CacheSyncTimeout time.Duration

// RecoverPanic indicates whether the panic caused by reconcile should be recovered.
RecoverPanic bool
// Defaults to the Controller.RecoverPanic setting from the Manager if unset.
RecoverPanic *bool
}

// Controller implements a Kubernetes API. A Controller manages a work queue fed reconcile.Requests
Expand Down Expand Up @@ -139,6 +140,10 @@ func NewUnmanaged(name string, mgr manager.Manager, options Options) (Controller
return nil, err
}

if options.RecoverPanic == nil {
options.RecoverPanic = mgr.GetControllerOptions().RecoverPanic
}

// Create controller with dependencies set
return &controller.Controller{
Do: options.Reconciler,
Expand Down
36 changes: 36 additions & 0 deletions pkg/controller/controller_test.go
Expand Up @@ -25,11 +25,14 @@ import (
. "github.com/onsi/gomega"
"go.uber.org/goleak"
corev1 "k8s.io/api/core/v1"
"k8s.io/utils/pointer"

"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/config/v1alpha1"
"sigs.k8s.io/controller-runtime/pkg/controller"
"sigs.k8s.io/controller-runtime/pkg/event"
"sigs.k8s.io/controller-runtime/pkg/handler"
internalcontroller "sigs.k8s.io/controller-runtime/pkg/internal/controller"
"sigs.k8s.io/controller-runtime/pkg/manager"
"sigs.k8s.io/controller-runtime/pkg/reconcile"
"sigs.k8s.io/controller-runtime/pkg/runtime/inject"
Expand Down Expand Up @@ -142,6 +145,39 @@ var _ = Describe("controller.Controller", func() {
clientTransport.CloseIdleConnections()
Eventually(func() error { return goleak.Find(currentGRs) }).Should(Succeed())
})

It("should default RecoverPanic from the manager", func() {
m, err := manager.New(cfg, manager.Options{Controller: v1alpha1.ControllerConfigurationSpec{RecoverPanic: pointer.Bool(true)}})
Expect(err).NotTo(HaveOccurred())

c, err := controller.New("new-controller", m, controller.Options{
Reconciler: reconcile.Func(nil),
})
Expect(err).NotTo(HaveOccurred())

ctrl, ok := c.(*internalcontroller.Controller)
Expect(ok).To(BeTrue())

Expect(ctrl.RecoverPanic).NotTo(BeNil())
Expect(*ctrl.RecoverPanic).To(BeTrue())
})

It("should not override RecoverPanic on the controller", func() {
m, err := manager.New(cfg, manager.Options{Controller: v1alpha1.ControllerConfigurationSpec{RecoverPanic: pointer.Bool(true)}})
Expect(err).NotTo(HaveOccurred())

c, err := controller.New("new-controller", m, controller.Options{
Reconciler: reconcile.Func(nil),
RecoverPanic: pointer.Bool(false),
})
Expect(err).NotTo(HaveOccurred())

ctrl, ok := c.(*internalcontroller.Controller)
Expect(ok).To(BeTrue())

Expect(ctrl.RecoverPanic).NotTo(BeNil())
Expect(*ctrl.RecoverPanic).To(BeFalse())
})
})
})

Expand Down
4 changes: 2 additions & 2 deletions pkg/internal/controller/controller.go
Expand Up @@ -92,7 +92,7 @@ type Controller struct {
LogConstructor func(request *reconcile.Request) logr.Logger

// RecoverPanic indicates whether the panic caused by reconcile should be recovered.
RecoverPanic bool
RecoverPanic *bool
}

// watchDescription contains all the information necessary to start a watch.
Expand All @@ -106,7 +106,7 @@ type watchDescription struct {
func (c *Controller) Reconcile(ctx context.Context, req reconcile.Request) (_ reconcile.Result, err error) {
defer func() {
if r := recover(); r != nil {
if c.RecoverPanic {
if c.RecoverPanic != nil && *c.RecoverPanic {
for _, fn := range utilruntime.PanicHandlers {
fn(r)
}
Expand Down
3 changes: 2 additions & 1 deletion pkg/internal/controller/controller_test.go
Expand Up @@ -33,6 +33,7 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
"k8s.io/client-go/util/workqueue"
"k8s.io/utils/pointer"
"sigs.k8s.io/controller-runtime/pkg/cache"
"sigs.k8s.io/controller-runtime/pkg/cache/informertest"
"sigs.k8s.io/controller-runtime/pkg/client"
Expand Down Expand Up @@ -114,7 +115,7 @@ var _ = Describe("controller", func() {
defer func() {
Expect(recover()).To(BeNil())
}()
ctrl.RecoverPanic = true
ctrl.RecoverPanic = pointer.Bool(true)
ctrl.Do = reconcile.Func(func(context.Context, reconcile.Request) (reconcile.Result, error) {
var res *reconcile.Result
return *res, nil
Expand Down