-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
⚠ Add a context w/ logger to Reconciler interface #1054
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,19 +25,20 @@ import ( | |
"sigs.k8s.io/controller-runtime/pkg/client/config" | ||
"sigs.k8s.io/controller-runtime/pkg/controller" | ||
"sigs.k8s.io/controller-runtime/pkg/handler" | ||
logf "sigs.k8s.io/controller-runtime/pkg/log" | ||
"sigs.k8s.io/controller-runtime/pkg/log" | ||
"sigs.k8s.io/controller-runtime/pkg/log/zap" | ||
"sigs.k8s.io/controller-runtime/pkg/manager" | ||
"sigs.k8s.io/controller-runtime/pkg/manager/signals" | ||
"sigs.k8s.io/controller-runtime/pkg/source" | ||
"sigs.k8s.io/controller-runtime/pkg/webhook" | ||
) | ||
|
||
var log = logf.Log.WithName("example-controller") | ||
func init() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 😬 is doing this in init() actually needed? It would be great if we could avoid requiring the use of init functions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know I'm a bit late to the party here, but it's not clear why we switched to using init here |
||
log.SetLogger(zap.New()) | ||
} | ||
|
||
func main() { | ||
logf.SetLogger(zap.New()) | ||
entryLog := log.WithName("entrypoint") | ||
entryLog := log.Log.WithName("entrypoint") | ||
|
||
// Setup a Manager | ||
entryLog.Info("setting up manager") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did you want to demonstrate plumbing the logger through to the manager here? |
||
|
@@ -50,7 +51,7 @@ func main() { | |
// Setup a new controller to reconcile ReplicaSets | ||
entryLog.Info("Setting up controller") | ||
c, err := controller.New("foo-controller", mgr, controller.Options{ | ||
Reconciler: &reconcileReplicaSet{client: mgr.GetClient(), log: log.WithName("reconciler")}, | ||
Reconciler: &reconcileReplicaSet{client: mgr.GetClient()}, | ||
}) | ||
if err != nil { | ||
entryLog.Error(err, "unable to set up individual controller") | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -29,23 +29,22 @@ import ( | |||||
ctrl "sigs.k8s.io/controller-runtime" | ||||||
api "sigs.k8s.io/controller-runtime/examples/crd/pkg" | ||||||
"sigs.k8s.io/controller-runtime/pkg/client" | ||||||
"sigs.k8s.io/controller-runtime/pkg/log" | ||||||
"sigs.k8s.io/controller-runtime/pkg/log/zap" | ||||||
) | ||||||
|
||||||
var ( | ||||||
setupLog = ctrl.Log.WithName("setup") | ||||||
recLog = ctrl.Log.WithName("reconciler") | ||||||
) | ||||||
|
||||||
type reconciler struct { | ||||||
client.Client | ||||||
scheme *runtime.Scheme | ||||||
} | ||||||
|
||||||
func (r *reconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { | ||||||
log := recLog.WithValues("chaospod", req.NamespacedName) | ||||||
func (r *reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { | ||||||
log := log.FromContext(ctx).WithValues("chaospod", req.NamespacedName) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The use of .WithValues shouldn't be needed here, since you can pass in the key/value pairs to
Suggested change
|
||||||
log.V(1).Info("reconciling chaos pod") | ||||||
ctx := context.Background() | ||||||
|
||||||
var chaospod api.ChaosPod | ||||||
if err := r.Get(ctx, req.NamespacedName, &chaospod); err != nil { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this example also show plumbing the logger through the manager in |
||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,7 @@ limitations under the License. | |
package controller | ||
|
||
import ( | ||
"context" | ||
"fmt" | ||
"sync" | ||
"time" | ||
|
@@ -27,6 +28,7 @@ import ( | |
"k8s.io/client-go/util/workqueue" | ||
"sigs.k8s.io/controller-runtime/pkg/handler" | ||
ctrlmetrics "sigs.k8s.io/controller-runtime/pkg/internal/controller/metrics" | ||
logf "sigs.k8s.io/controller-runtime/pkg/log" | ||
"sigs.k8s.io/controller-runtime/pkg/predicate" | ||
"sigs.k8s.io/controller-runtime/pkg/reconcile" | ||
"sigs.k8s.io/controller-runtime/pkg/runtime/inject" | ||
|
@@ -86,8 +88,10 @@ type watchDescription struct { | |
} | ||
|
||
// Reconcile implements reconcile.Reconciler | ||
func (c *Controller) Reconcile(r reconcile.Request) (reconcile.Result, error) { | ||
return c.Do.Reconcile(r) | ||
func (c *Controller) Reconcile(ctx context.Context, req reconcile.Request) (reconcile.Result, error) { | ||
log := c.Log.WithValues("name", req.Name, "namespace", req.Namespace) | ||
ctx = logf.IntoContext(ctx, log) | ||
return c.Do.Reconcile(ctx, req) | ||
} | ||
|
||
// Watch implements controller.Controller | ||
|
@@ -229,12 +233,15 @@ func (c *Controller) reconcileHandler(obj interface{}) bool { | |
} | ||
|
||
log := c.Log.WithValues("name", req.Name, "namespace", req.Namespace) | ||
ctx := logf.IntoContext(context.Background(), log) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would |
||
|
||
// RunInformersAndControllers the syncHandler, passing it the namespace/Name string of the | ||
// resource to be synced. | ||
if result, err := c.Do.Reconcile(req); err != nil { | ||
if result, err := c.Do.Reconcile(ctx, req); err != nil { | ||
c.Queue.AddRateLimited(req) | ||
log.Error(err, "Reconciler error") | ||
if log.V(3).Enabled() { | ||
log.Error(err, "Reconciler error") | ||
} | ||
ctrlmetrics.ReconcileErrors.WithLabelValues(c.Name).Inc() | ||
ctrlmetrics.ReconcileTotal.WithLabelValues(c.Name, "error").Inc() | ||
return false | ||
|
@@ -258,7 +265,7 @@ func (c *Controller) reconcileHandler(obj interface{}) bool { | |
c.Queue.Forget(obj) | ||
|
||
// TODO(directxman12): What does 1 mean? Do we want level constants? Do we want levels at all? | ||
log.V(1).Info("Successfully Reconciled") | ||
log.V(5).Info("Successfully Reconciled") | ||
|
||
ctrlmetrics.ReconcileTotal.WithLabelValues(c.Name, "success").Inc() | ||
// Return true, don't take a break | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the error cases below be updated to use the logger from the context to output error info rather than returning wrapped errors?