⚠ Add a context w/ logger to Reconciler interface #1054

vincepri · 2020-07-18T15:23:20Z

No description provided.

k8s-ci-robot · 2020-07-18T15:23:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [vincepri]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

vincepri · 2020-07-18T15:23:39Z

This is mostly a concept, and would like some feedback on the approach

vincepri · 2020-07-20T15:54:59Z

/test pull-controller-runtime-test-master

vincepri · 2020-07-20T15:55:28Z

/cc @alvaroaleman @christopherhein @devigned @alexeldeib

for feedback :)

luxas

This would be a great addition to the project!
After this we can take a stab at global timeouts.
Generally, I prefer interfaces for this kind of thing though

pkg/reconcile/reconcile.go

pkg/builder/example_test.go

pkg/controller/controller_integration_test.go

pkg/builder/controller_test.go

luxas · 2020-07-20T16:52:46Z

pkg/internal/controller/controller.go

@@ -228,11 +229,16 @@ func (c *Controller) reconcileHandler(obj interface{}) bool {
 		return true
 	}

+	ctx := reconcile.Context{
+		Context: context.Background(),
+		Log:     c.Log.WithValues("name", req.Name, "namespace", req.Namespace),


also add gvk information? and/or reconciler name?

The reconciler/controller name is already within the c.Log logger, +1 adding gvk info

alvaroaleman · 2020-07-20T17:01:11Z

Generally I really like it. One thing I am wondering though is how we expect ppl to use this, because they will rightfully want to use the standard With{Cancel,Deadline,Timeout} however if they do that, they get a context.Context and not a reconcile.Context. Maybe it would make sense to provide a context package that exposes those?

vincepri · 2020-07-20T17:04:42Z

@alvaroaleman That's a good point, we can have our context utilities that would follow upstream and return our own representation instead

pkg/context/context.go

luxas · 2020-07-21T06:36:41Z

/assign

vincepri · 2020-07-21T14:17:12Z

/milestone v0.7.x

devigned

This is an exciting and promising PR! My only concern is the creation of an extended context package.

vincepri · 2020-07-23T01:26:14Z

Thank you all for the great feedback. Pushed some changes to use the stdlib's context.Context and have some helper methods to retrieve and set the logger.

Let me know what do you all think, ready for another round 😊

pkg/builder/controller.go

vincepri · 2020-08-03T19:47:29Z

/test pull-controller-runtime-test-master

Signed-off-by: Vince Prignano <vincepri@vmware.com>

k8s-ci-robot · 2020-08-04T20:46:14Z

@vincepri: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-controller-runtime-apidiff-master	`e230cb4`	link	`/test pull-controller-runtime-apidiff-master`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

vincepri · 2020-08-04T20:47:19Z

This should be good to go as well, if there are no other comments

vincepri · 2020-08-05T14:53:02Z

@alvaroaleman This should also be good to go

alvaroaleman

Really, really happy about this change btw, thanks for pushing it!
/lgtm

detiber · 2020-08-05T14:57:17Z

examples/builtins/main.go

 	"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() {


😬 is doing this in init() actually needed? It would be great if we could avoid requiring the use of init functions.

I know I'm a bit late to the party here, but it's not clear why we switched to using init here

detiber · 2020-08-05T14:58:01Z

examples/builtins/main.go


 func main() {
-	logf.SetLogger(zap.New())
-	entryLog := log.WithName("entrypoint")
+	entryLog := log.Log.WithName("entrypoint")

 	// Setup a Manager
 	entryLog.Info("setting up manager")


Did you want to demonstrate plumbing the logger through to the manager here?

detiber · 2020-08-05T15:00:13Z

examples/crd/main.go

-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)


The use of .WithValues shouldn't be needed here, since you can pass in the key/value pairs to log.FromContext

Suggested change

log := log.FromContext(ctx).WithValues("chaospod", req.NamespacedName)

log := log.FromContext(ctx, "chaospod", req.NamespacedName)

detiber · 2020-08-05T15:03:45Z

examples/builtins/controller.go

 	// set up a convenient log object so we don't have to type request over and over again
-	log := r.log.WithValues("request", request)
+	log := log.FromContext(ctx)

 	// Fetch the ReplicaSet from the cache
 	rs := &appsv1.ReplicaSet{}


Should the error cases below be updated to use the logger from the context to output error info rather than returning wrapped errors?

detiber · 2020-08-05T15:06:03Z

examples/crd/main.go

 	log.V(1).Info("reconciling chaos pod")
-	ctx := context.Background()

 	var chaospod api.ChaosPod
 	if err := r.Get(ctx, req.NamespacedName, &chaospod); err != nil {


Should this example also show plumbing the logger through the manager in main() below?

detiber · 2020-08-05T15:22:30Z

pkg/internal/controller/controller.go

@@ -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)


Would context.TODO() work a bit better here? It seems like eventually a WithCancel context can be plumbed into here from Start() that could eventually be used to more cleanly stop in-flight operations rather than just forcefully killing the workers.

vincepri · 2020-08-05T15:35:39Z

@detiber I'll follow-up with another PR for your comments / respond soon

DirectXMan12

friendly reminder to please try to not merge things with empty commit messages -- it makes it harder to see an overview of what was merged and why

DirectXMan12 · 2020-08-12T17:47:10Z

examples/builtins/main.go

 	"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() {


I know I'm a bit late to the party here, but it's not clear why we switched to using init here

Prep code for supporting k8s 1.21: * Add context.Context parameter to Reconcile() functions [1] * Add RBAC get/create/update coordination.k8s.io/v1 Leases (replacing a configmap for leader election locks) [2] * Adapt event handler code to reflect simplification [3] * Adapt to apimachinery/pkg/runtime Log deprecation (replaced with pkg/client Log) [4] * client.Object is preferred in favor of runtime.Object (v0.7.0 release) [5] * Use admission/v1 instead of v1beta1 for webhook requests [6] [1] - kubernetes-sigs/controller-runtime#1054 [2] - kubernetes-sigs/controller-runtime#1144 [3] - kubernetes-sigs/controller-runtime#1119 [4] - kubernetes-sigs/controller-runtime#1105 [5] - kubernetes-sigs/controller-runtime#898 kubernetes-sigs/controller-runtime#1118 [6] - kubernetes-sigs/controller-runtime#1284 kubernetes-sigs/controller-runtime@a32b29d Signed-off-by: Angel Misevski <amisevsk@redhat.com>

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Jul 18, 2020

k8s-ci-robot requested review from droot and joelanford July 18, 2020 15:23

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 18, 2020

vincepri force-pushed the reconciler-context branch from feda045 to f870d3a Compare July 18, 2020 17:37

k8s-ci-robot requested review from alexeldeib, alvaroaleman, christopherhein and devigned July 20, 2020 15:55

alvaroaleman mentioned this pull request Jul 20, 2020

Reconcile methods should take a context and logger #801

Closed

luxas reviewed Jul 20, 2020

View reviewed changes

vincepri force-pushed the reconciler-context branch from f870d3a to 57c1a6b Compare July 20, 2020 17:49

alvaroaleman reviewed Jul 20, 2020

View reviewed changes

pkg/context/context.go Outdated Show resolved Hide resolved

k8s-ci-robot assigned luxas Jul 21, 2020

k8s-ci-robot added this to the v0.7.x milestone Jul 21, 2020

devigned reviewed Jul 21, 2020

View reviewed changes

vincepri force-pushed the reconciler-context branch from 57c1a6b to 99cf920 Compare July 23, 2020 01:25

devigned approved these changes Jul 23, 2020

View reviewed changes

vincepri changed the title ~~[WIP] ⚠ Add a context to Reconciler interface~~ ⚠ Add a context to Reconciler interface Jul 31, 2020

vincepri force-pushed the reconciler-context branch from df909ff to e774d6e Compare August 3, 2020 19:38

detiber reviewed Aug 3, 2020

View reviewed changes

pkg/builder/controller.go Show resolved Hide resolved

vincepri force-pushed the reconciler-context branch from e774d6e to ce65986 Compare August 3, 2020 19:45

⚠ Add a context to Reconciler interface

e230cb4

Signed-off-by: Vince Prignano <vincepri@vmware.com>

vincepri force-pushed the reconciler-context branch from ce65986 to e230cb4 Compare August 4, 2020 20:44

alvaroaleman approved these changes Aug 5, 2020

View reviewed changes

k8s-ci-robot assigned alvaroaleman Aug 5, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 5, 2020

k8s-ci-robot merged commit 5f112fb into kubernetes-sigs:master Aug 5, 2020

detiber reviewed Aug 5, 2020

View reviewed changes

DirectXMan12 reviewed Aug 12, 2020

View reviewed changes

joelanford mentioned this pull request Oct 27, 2020

Timeout operator-framework/operator-sdk#4115

Closed

2 tasks

timebertt mentioned this pull request Nov 4, 2020

☂️-Issue for upgrading to k/*@v0.19 and controller-runtime@v0.7 gardener/gardener#3109

Closed

ChunyiLyu mentioned this pull request Jan 7, 2021

Use logger from context in Reconcile() rabbitmq/cluster-operator#550

Merged

zubron mentioned this pull request Jan 26, 2021

Use manager context in controller Reconcile functions vmware-tanzu/velero#3353

Closed

wallrj mentioned this pull request Mar 25, 2021

Upgrade to controller-runtime 0.8.3 cert-manager/sample-external-issuer#9

Merged

panyuenlau mentioned this pull request Oct 11, 2021

Bump github.com/layer5io/meshkit from 0.2.29 to 0.2.30 meshery/meshery-operator#207

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

⚠ Add a context w/ logger to Reconciler interface #1054

⚠ Add a context w/ logger to Reconciler interface #1054

vincepri commented Jul 18, 2020

k8s-ci-robot commented Jul 18, 2020

vincepri commented Jul 18, 2020

vincepri commented Jul 20, 2020

vincepri commented Jul 20, 2020

luxas left a comment

luxas Jul 20, 2020

vincepri Jul 21, 2020

alvaroaleman commented Jul 20, 2020

vincepri commented Jul 20, 2020 •

edited

luxas commented Jul 21, 2020

vincepri commented Jul 21, 2020

devigned left a comment

vincepri commented Jul 23, 2020

vincepri commented Aug 3, 2020

k8s-ci-robot commented Aug 4, 2020

vincepri commented Aug 4, 2020

vincepri commented Aug 5, 2020

alvaroaleman left a comment

detiber Aug 5, 2020

DirectXMan12 Aug 12, 2020

detiber Aug 5, 2020

detiber Aug 5, 2020

detiber Aug 5, 2020

detiber Aug 5, 2020

detiber Aug 5, 2020

vincepri commented Aug 5, 2020

DirectXMan12 left a comment

DirectXMan12 Aug 12, 2020

	log := log.FromContext(ctx).WithValues("chaospod", req.NamespacedName)
	log := log.FromContext(ctx, "chaospod", req.NamespacedName)

⚠ Add a context w/ logger to Reconciler interface #1054

⚠ Add a context w/ logger to Reconciler interface #1054

Conversation

vincepri commented Jul 18, 2020

k8s-ci-robot commented Jul 18, 2020

vincepri commented Jul 18, 2020

vincepri commented Jul 20, 2020

vincepri commented Jul 20, 2020

luxas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alvaroaleman commented Jul 20, 2020

vincepri commented Jul 20, 2020 • edited

luxas commented Jul 21, 2020

vincepri commented Jul 21, 2020

devigned left a comment

Choose a reason for hiding this comment

vincepri commented Jul 23, 2020

vincepri commented Aug 3, 2020

k8s-ci-robot commented Aug 4, 2020

vincepri commented Aug 4, 2020

vincepri commented Aug 5, 2020

alvaroaleman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincepri commented Aug 5, 2020

DirectXMan12 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincepri commented Jul 20, 2020 •

edited