✨ Refactor context for vspherevm controller

[xiujuanx]

What this PR does / why we need it:
Refactor context for vSphereVM controller and underlying functions that vSphereVM controller are consuming, this is a 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):
Fixes part of #2295

[k8s-ci-robot]

Hi @xiujuanx. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[zhanggbj]

/ok-to-test

[codecov]

Codecov Report

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

Comparison is base (1156e2e) 61.02% compared to head (ab095d6) 60.96%.
Report is 16 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2398      +/-   ##
==========================================
- Coverage   61.02%   60.96%   -0.07%     
==========================================
  Files         163      163              
  Lines        9435     9434       -1     
==========================================
- Hits         5758     5751       -7     
- Misses       3263     3268       +5     
- Partials      414      415       +1     

Files	Coverage Δ
pkg/services/fake/vmservice.go	100.00% <100.00%> (ø)
pkg/services/govmomi/cluster/vmgroup.go	85.00% <100.00%> (ø)
pkg/util/fetch_object.go	84.21% <80.00%> (ø)
controllers/vspherevm_ipaddress_reconciler.go	72.84% <75.00%> (ø)
pkg/services/govmomi/create.go	40.00% <50.00%> (ø)
pkg/services/govmomi/ipam/status.go	88.28% <81.25%> (ø)
pkg/services/govmomi/template/template.go	43.75% <70.00%> (ø)
controllers/vspherevm_controller.go	63.76% <82.50%> (-0.17%)	⬇️
pkg/services/govmomi/power.go	44.30% <44.44%> (ø)
pkg/services/govmomi/vcenter/clone.go	54.87% <61.40%> (ø)
... and 2 more

... and 7 files with indirect coverage changes

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

[zhanggbj]

Thank you for the PR!!

This is a large one :-) As vspherevm_controller.go will call lots of util functions, many of them need a refactoring. Our goal is to remove the wrapped context.Context in the customized Context here, so any function that's using customized context as a "context", we need to refactor and add a new parameter (ctx context.Context), that is the right context we need to pass down.

[Ankitasw]

One small question, otherwise LGTM

[zhanggbj]

Please address the comments above, then I think we are good to go :-)

[xiujuanx]

Hi @zhanggbj @Ankitasw , thanks for your review and valuable comments, I am very appreciate for this! Now, I have addressed all comments for this PR, so could you please help take a look again? Thank you again!
BTW, about the golint and apidiff check errors, some of codes I don't touch any more, they are still marked to error. Do I need to fix all errors or not in this PR?

[chrischdi]

@xiujuanx for golangci-lint: you may have to rebase (just guessing).

apidiff is expected to fail because we change the signature of some functions.

[chrischdi]

Linter findings seem to be valid though.

This PR is also on my review list :-)

[Ankitasw]

BTW, about the golint and apidiff check errors, some of codes I don't touch any more, they are still marked to error. Do I need to fix all errors or not in this PR?

@xiujuanx I think it's better we fix those lint errors in this PR itself, otherwise we won't be able to merge this PR, as other PRs would also start breaking with same error. I see majorly lint errors related to ctx field is unused, we can just mark those fields as empty (_ context.Context). Whenever in further PRs if it is required, we can add back that field declaration.

[zhanggbj]

@xiujuanx For linter errors, please evaluate them and fix it in this PR, basically if there's logger(which will be refactored in separate PR) or we need context in the function, we should keep the ctx context.Context or _ context.Context for future usage.

For apidiff check, this is in expectation as we changed signature, and this test is optional for merging :-)

[xiujuanx]

Thanks @chrischdi @Ankitasw @zhanggbj for your comments and suggestions! I rebased the main branch and fixed some lint errors. Now, the golint job passes.

[zhanggbj]

Thanks for the hard word :-)
/lgtm

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 3e148a2653b6348ac582e58d983f0257f9dbde9c

[Ankitasw]

/lgtm

[zhanggbj]

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

[Ankitasw]

@zhanggbj looks like the E2E tests are failing due to other reasons. @chrischdi is following up on the same.

[chrischdi]

Yeah, tests which are using the storage policy currently all fail because of an issue in VMC.

[chrischdi]

/retest

[k8s-ci-robot]

@xiujuanx: 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	ab095d6	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.

[chrischdi]

/approve

/hold

if @killianmuldoon wants to take a look too.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrischdi

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]

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

[chrischdi]

/hold cancel

[chrischdi]

Thanks for this huge PR 🎉