Use context correctly

[squeedee]

---

name: Pull request
about: Submit changes to the code

---

Changes proposed by this PR

• Use reconciler context in subsequent async calls, all the way to the client.
• Remove redundant WithValues from context based logging

Release Note

PR Checklist

Note: Please do not remove items. Mark items as done [x] or use strikethrough if you believe they are not relevant

• [ ] Linked to a relevant issue. Eg: Fixes #123 or Updates #123
• Removed non-atomic or wip commits
• [ ] Filled in the Release Note section above
• [ ] Modified the docs to match changes

[netlify]

✔️ Deploy Preview for elated-stonebraker-105904 canceled.

🔨 Explore the source changes: 67d8796

🔍 Inspect the deploy log: https://app.netlify.com/sites/elated-stonebraker-105904/deploys/6196976325770900073b2ec4

[cirocosta]

yayy nice! that's a lot of changes 😅 thanks for putting the work! overall, lgtm!

---

trying to follow the chain of ctx being propagated through the code, one thing that I noticed that seems a little off is how we pass from cmd/main.go down to pkg/root:

• here we're passing it via a struct field

cartographer/cmd/cartographer/main.go

Line 54 in 1bd6074

Context: ctx,

• here we're consuming from the struct

cartographer/pkg/root/root.go

Lines 112 to 113 in 1bd6074

	if err := mgr.Start(cmd.Context); err != nil {
	return fmt.Errorf("manager start: %w", err)

that's not very usual, and discouraged - the idiomatic way of doing doing would be to pass it via the function instead, being the first argument of the list.

it's not something introduced in this PR, but, sounds somewhat appropriate to patch given the context (no pun intended) 😁 wdyt?

thx!

[cirocosta]

oh also, here

cartographer/pkg/root/root.go

Lines 68 to 70 in 1bd6074

	if err := registrar.IndexResources(mgr, cmd.Context); err != nil {
	return fmt.Errorf("index resources: %w", err)
	}

we should make ctx the first argument instead.

[cirocosta]

sorry, one more 😅 we're doing the right thing creating a context with an associated cancel function

cartographer/cmd/cartographer/main.go

Lines 42 to 44 in 1bd6074

	ctx, cancel := context.WithCancel(context.Background())
	defer cancel()

but that will not by default lead to a proper smooth termination when a signal is sent (either us sending a SIGINT / SIGTERM to make run, or kubelet trying to stop the pod with SIGINT)

By default, a synchronous signal is converted into a run-time panic. A SIGHUP, SIGINT, or SIGTERM signal causes the program to exit.

(from https://pkg.go.dev/os/signal#hdr-Default_behavior_of_signals_in_Go_programs)

i.e., despite passing the context to the right places, we're not being graceful at all.

controller-runtime provides a nice little utility for covering this though: https://github.com/kubernetes-sigs/controller-runtime/blob/4d10a0615b11507451ecb58bfd59f0f6ef313a29/pkg/manager/signals/signal.go#L26-L30

// SetupSignalHandler registers for SIGTERM and SIGINT. A context is returned
// which is canceled on one of these signals. If a second signal is caught, the program
// is terminated with exit code 1.
func SetupSignalHandler() context.Context {

so, doing something like

-       if err = cmd.Execute(); err != nil {
+       if err = cmd.Execute(ctrl.SetupSignalHandler()); err != nil {
                panic(err)
        }
 }

we should see that after sending a signal, it actually exits gracefully:

^C{"level":"info","ts":1637262059.6318526,"logger":"controller.workload","msg":"Shutdown signal received, waiting for all workers to finish"}
{"level":"info","ts":1637262059.63193,"logger":"controller.supply-chain","msg":"Shutdown signal received, waiting for all workers to finish"}
{"level":"info","ts":1637262059.6319268,"logger":"controller.deliverable","msg":"Shutdown signal received, waiting for all workers to finish"}
{"level":"info","ts":1637262059.6318586,"logger":"controller.delivery","msg":"Shutdown signal received, waiting for all workers to finish"}
{"level":"info","ts":1637262059.6319535,"logger":"controller.runnable-service","msg":"Shutdown signal received, waiting for all workers to finish"}
{"level":"info","ts":1637262059.6319625,"logger":"controller.delivery","msg":"All workers finished"}
{"level":"info","ts":1637262059.6319673,"logger":"controller.runnable-service","msg":"All workers finished"}
{"level":"info","ts":1637262059.6319716,"logger":"controller.deliverable","msg":"All workers finished"}
{"level":"info","ts":1637262059.631974,"logger":"controller.supply-chain","msg":"All workers finished"}
{"level":"info","ts":1637262059.6319742,"logger":"controller.workload","msg":"All workers finished"}

(without that context w/ proper signal handling, we don't have the messages above ^)

[cirocosta]

left a couple comments about some complementary changes that could go in together, but that's it! lgtm 😁

[squeedee]

yayy nice! that's a lot of changes 😅 thanks for putting the work! overall, lgtm!

trying to follow the chain of ctx being propagated through the code, one thing that I noticed that seems a little off is how we pass from cmd/main.go down to pkg/root:

• here we're passing it via a struct field

cartographer/cmd/cartographer/main.go

Line 54 in 1bd6074

Context: ctx,

• here we're consuming from the struct

cartographer/pkg/root/root.go

Lines 112 to 113 in 1bd6074

	if err := mgr.Start(cmd.Context); err != nil {
	return fmt.Errorf("manager start: %w", err)

that's not very usual, and discouraged - the idiomatic way of doing doing would be to pass it via the function instead, being the first argument of the list.

it's not something introduced in this PR, but, sounds somewhat appropriate to patch given the context (no pun intended) 😁 wdyt?

thx!

I agree but I don't think I want to wait to rebase against main again just to get this work in.

[squeedee]

Issue created: #361