🌱 Remove context from capv customized controller context #2431

zhanggbj · 2023-10-13T09:39:20Z

What this PR does / why we need it:
Remove context from capv customized controller context, also get rid of containedctx exceptions.
This is part of #2295.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Part of
#2295
#2384

codecov · 2023-10-13T09:44:14Z

Codecov Report

Attention: 13 lines in your changes are missing coverage. Please review.

Comparison is base (013ce3f) 62.65% compared to head (fa689e1) 62.69%.
Report is 14 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2431      +/-   ##
==========================================
+ Coverage   62.65%   62.69%   +0.03%     
==========================================
  Files         122      122              
  Lines        8770     8763       -7     
==========================================
- Hits         5495     5494       -1     
+ Misses       2877     2872       -5     
+ Partials      398      397       -1

Files	Coverage Δ
controllers/vspherevm_controller.go	`63.76% <100.00%> (ø)`
pkg/clustermodule/util.go	`100.00% <100.00%> (ø)`
pkg/context/controller_manager_context.go	`75.00% <ø> (ø)`
pkg/context/fake/fake_controller_context.go	`100.00% <100.00%> (ø)`
...kg/context/fake/fake_controller_manager_context.go	`100.00% <ø> (ø)`
pkg/context/vm_context.go	`100.00% <100.00%> (ø)`
pkg/context/vspheredeploymentzone_context.go	`84.61% <100.00%> (ø)`
pkg/manager/manager.go	`84.41% <100.00%> (-0.20%)`	⬇️
pkg/services/govmomi/cluster/service.go	`78.57% <ø> (ø)`
pkg/services/govmomi/context.go	`0.00% <ø> (ø)`
... and 11 more

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

killianmuldoon · 2023-10-13T12:44:05Z

/test

k8s-ci-robot · 2023-10-13T12:44:08Z

@killianmuldoon: The /test command needs one or more targets.
The following commands are available to trigger required jobs:

/test pull-cluster-api-provider-vsphere-conformance-main
/test pull-cluster-api-provider-vsphere-e2e-full-main
/test pull-cluster-api-provider-vsphere-e2e-main
/test pull-cluster-api-provider-vsphere-test-integration-main
/test pull-cluster-api-provider-vsphere-test-main
/test pull-cluster-api-provider-vsphere-verify-main

The following commands are available to trigger optional jobs:

/test pull-cluster-api-provider-vsphere-apidiff-main

Use /test all to run the following jobs that were automatically triggered:

pull-cluster-api-provider-vsphere-apidiff-main
pull-cluster-api-provider-vsphere-e2e-main
pull-cluster-api-provider-vsphere-test-integration-main
pull-cluster-api-provider-vsphere-test-main
pull-cluster-api-provider-vsphere-verify-main

In response to this:

/test

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.

killianmuldoon

/test pull-cluster-api-provider-vsphere-e2e-full-main

pkg/context/fake/fake_cluster_context.go

pkg/context/fake/fake_vm_context.go

pkg/manager/manager.go

zhanggbj · 2023-10-16T07:25:30Z

@chrischdi Thanks for your review, all comments are addressed, it's ready for a second look.

chrischdi

Some last nits, but that should be it 🎉

controllers/serviceaccount_controller_intg_test.go

test/helpers/vmware/unit_test_context.go

Ankitasw · 2023-10-17T05:03:53Z

/retest

zhanggbj · 2023-10-17T05:51:18Z

/test pull-cluster-api-provider-vsphere-test-main

zhanggbj · 2023-10-17T07:47:26Z

/test pull-cluster-api-provider-vsphere-test-main

chrischdi · 2023-10-17T07:47:52Z

Seems like somethings broken. Failed 3 times in a row at the same test.

zhanggbj · 2023-10-17T08:47:36Z

The failure cannot be reproduced locally by make test.

controllers/vspherecluster_reconciler_test.go

controllers/vmware/vspherecluster_reconciler_test.go

controllers/vspherevm_ipaddress_reconciler_test.go

chrischdi · 2023-10-23T09:07:44Z

/lgtm
/approve
/hold

k8s-ci-robot · 2023-10-23T09:07:51Z

LGTM label has been added.

Git tree hash: 106aacf81c92bda187fe7571e29641e35e5f5dee

chrischdi · 2023-10-23T09:08:34Z

hold for if @sbueringer or @killianmuldoon want to review.

chrischdi · 2023-10-23T09:30:40Z

/retest

pkg/services/govmomi/util.go

Remove context from capv customized controller context, also get rid of containedctx exceptions. Signed-off-by: Gong Zhang <[email protected]>

k8s-ci-robot · 2023-10-25T02:38:54Z

@zhanggbj: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-provider-vsphere-apidiff-main	`fa689e1`	link	false	`/test pull-cluster-api-provider-vsphere-apidiff-main`

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.

zhanggbj · 2023-10-25T03:29:19Z

/test pull-cluster-api-provider-vsphere-test-main

chrischdi · 2023-10-25T06:44:37Z

/lgtm

k8s-ci-robot · 2023-10-25T06:44:42Z

LGTM label has been added.

Git tree hash: f4bb6be1c019ffdef1a60d099e59d8262bb4f497

sbueringer · 2023-10-25T08:16:31Z

Thx!

/lgtm
/approve
/hold cancel

k8s-ci-robot · 2023-10-25T08:16:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrischdi, sbueringer

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~~ [chrischdi,sbueringer]

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

sbueringer · 2023-10-25T08:17:43Z

@chrischdi @zhanggbj Sorry I have a huge backlog right now. Can one of you audit for similar cases across the code base?

Basically when we start goroutines in reconcilers that should continue independent of the current Reconcile we should not passthrough the context and stop the goroutine if that context is Done.

A concrete bug we would get is that we wouldn't reconcile again (before the resyncPeriod) after a task is finished (if a goroutine is waiting for task finish) if that context gets canceled (e.g. by controller-runtime)

But on the other side we of course have the risk of leaking goroutines if the task never finishes. This seems to be a consequence of the current design.

Let's audit to not stop the goroutines based on passed through context's for now and let's create an issue to discuss if we want to maybe put a timeout on the contexts used in those goroutines to avoid leaking goroutines (or some alternative if we have other ideas)

P.S. If I understand the previous code (e.g. release-1.8) correctly context.Done only happened if the ControllerManagerContext.context was canceled. But this context was just a context.Background(), so I guess it never got canceled. So we don't lose anything if we get rid of the ctx.Done()'s for now

zhanggbj · 2023-10-26T05:44:41Z

@sbueringer It makes sense to me, I just opened an issue based on your input to track it, we could discuss more there #2465

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

k8s-ci-robot requested review from killianmuldoon and MaxRink October 13, 2023 09:39

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 13, 2023

zhanggbj mentioned this pull request Oct 13, 2023

Refactor CAPV controller context #2295

Closed

18 tasks

zhanggbj mentioned this pull request Oct 13, 2023

Review use of nolint directives across code #2384

Closed

15 tasks

zhanggbj changed the title ~~[WIP] 🌱 Remove context from capv customized controller context~~ 🌱 Remove context from capv customized controller context Oct 13, 2023

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 13, 2023

killianmuldoon reviewed Oct 13, 2023

View reviewed changes

chrischdi reviewed Oct 13, 2023

View reviewed changes

zhanggbj force-pushed the refactor_ctx_init branch from c74e86e to ebecfaf Compare October 16, 2023 03:50

Ankitasw mentioned this pull request Oct 16, 2023

Improve CAPV logging #2076

Closed

12 tasks

zhanggbj force-pushed the refactor_ctx_init branch from ebecfaf to 59fbb97 Compare October 16, 2023 07:20

zhanggbj force-pushed the refactor_ctx_init branch from 59fbb97 to 85bf07e Compare October 16, 2023 08:00

chrischdi reviewed Oct 16, 2023

View reviewed changes

controllers/serviceaccount_controller_intg_test.go Outdated Show resolved Hide resolved

test/helpers/vmware/unit_test_context.go Outdated Show resolved Hide resolved

test/helpers/vmware/unit_test_context.go Outdated Show resolved Hide resolved

zhanggbj force-pushed the refactor_ctx_init branch from 85bf07e to c1dac02 Compare October 17, 2023 04:16

zhanggbj force-pushed the refactor_ctx_init branch from c1dac02 to 219486a Compare October 17, 2023 09:02

chrischdi reviewed Oct 17, 2023

View reviewed changes

controllers/vspherecluster_reconciler_test.go Outdated Show resolved Hide resolved

chrischdi reviewed Oct 17, 2023

View reviewed changes

controllers/vmware/vspherecluster_reconciler_test.go Show resolved Hide resolved

chrischdi reviewed Oct 17, 2023

View reviewed changes

controllers/vspherevm_ipaddress_reconciler_test.go Show resolved Hide resolved

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 23, 2023

k8s-ci-robot assigned chrischdi Oct 23, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 23, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 23, 2023

sbueringer reviewed Oct 23, 2023

View reviewed changes

pkg/services/govmomi/util.go Outdated Show resolved Hide resolved

zhanggbj force-pushed the refactor_ctx_init branch from dd9842e to b78d12d Compare October 24, 2023 10:26

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 24, 2023

k8s-ci-robot requested a review from chrischdi October 24, 2023 10:26

zhanggbj force-pushed the refactor_ctx_init branch from b78d12d to c4c42b8 Compare October 24, 2023 10:31

Remove context from capv customized controller context

fa689e1

Remove context from capv customized controller context, also get rid of containedctx exceptions. Signed-off-by: Gong Zhang <[email protected]>

zhanggbj force-pushed the refactor_ctx_init branch from c4c42b8 to fa689e1 Compare October 25, 2023 02:27

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 25, 2023

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 25, 2023

k8s-ci-robot assigned sbueringer Oct 25, 2023

k8s-ci-robot merged commit 5211b8e into kubernetes-sigs:main Oct 25, 2023
15 of 16 checks passed

k8s-ci-robot added this to the v1.9 milestone Oct 25, 2023

zhanggbj mentioned this pull request Oct 26, 2023

Review and Refactor Goroutine Context Handling #2465

Closed

chrischdi mentioned this pull request Oct 26, 2023

🌱 Setup separate context for test files to avoid flaky errors #2462

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 Remove context from capv customized controller context #2431

🌱 Remove context from capv customized controller context #2431

zhanggbj commented Oct 13, 2023 •

edited

Loading

codecov bot commented Oct 13, 2023 •

edited

Loading

killianmuldoon commented Oct 13, 2023

k8s-ci-robot commented Oct 13, 2023

killianmuldoon left a comment

zhanggbj commented Oct 16, 2023

chrischdi left a comment

Ankitasw commented Oct 17, 2023

zhanggbj commented Oct 17, 2023

zhanggbj commented Oct 17, 2023

chrischdi commented Oct 17, 2023

zhanggbj commented Oct 17, 2023

chrischdi commented Oct 23, 2023

k8s-ci-robot commented Oct 23, 2023

chrischdi commented Oct 23, 2023

chrischdi commented Oct 23, 2023

k8s-ci-robot commented Oct 25, 2023 •

edited

Loading

zhanggbj commented Oct 25, 2023

chrischdi commented Oct 25, 2023

k8s-ci-robot commented Oct 25, 2023

sbueringer commented Oct 25, 2023

k8s-ci-robot commented Oct 25, 2023

sbueringer commented Oct 25, 2023 •

edited

Loading

zhanggbj commented Oct 26, 2023

🌱 Remove context from capv customized controller context #2431

🌱 Remove context from capv customized controller context #2431

Conversation

zhanggbj commented Oct 13, 2023 • edited Loading

codecov bot commented Oct 13, 2023 • edited Loading

Codecov Report

killianmuldoon commented Oct 13, 2023

k8s-ci-robot commented Oct 13, 2023

killianmuldoon left a comment

Choose a reason for hiding this comment

zhanggbj commented Oct 16, 2023

chrischdi left a comment

Choose a reason for hiding this comment

Ankitasw commented Oct 17, 2023

zhanggbj commented Oct 17, 2023

zhanggbj commented Oct 17, 2023

chrischdi commented Oct 17, 2023

zhanggbj commented Oct 17, 2023

chrischdi commented Oct 23, 2023

k8s-ci-robot commented Oct 23, 2023

chrischdi commented Oct 23, 2023

chrischdi commented Oct 23, 2023

k8s-ci-robot commented Oct 25, 2023 • edited Loading

zhanggbj commented Oct 25, 2023

chrischdi commented Oct 25, 2023

k8s-ci-robot commented Oct 25, 2023

sbueringer commented Oct 25, 2023

k8s-ci-robot commented Oct 25, 2023

sbueringer commented Oct 25, 2023 • edited Loading

zhanggbj commented Oct 26, 2023

zhanggbj commented Oct 13, 2023 •

edited

Loading

codecov bot commented Oct 13, 2023 •

edited

Loading

k8s-ci-robot commented Oct 25, 2023 •

edited

Loading

sbueringer commented Oct 25, 2023 •

edited

Loading