⚠️ Improve Context handling in clusterctl #8939

Fedosin · 2023-06-29T16:15:40Z

What this PR does / why we need it:
Today we have plenty of places in clusterctl where we use context.TODO(). Moreover in many places ctx is defined as a global variable, which is considered as an anti-pattern. This PR allows to pass context variable to all public functions to manage their behavior.

Note: This code breaks backward compatibility as it changes public function signatures by adding context parameter to them.

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 #8733

enxebre · 2023-06-30T08:06:34Z

Overall change lgtm. This is affecting multiple public functions so we might want to consider a deprecation notice, not sure what would be best way here given the surface of the change though.
Given the size of the PR, splitting this into multiple PRs would probably accelerate the review process.

sbueringer · 2023-06-30T10:26:36Z

Didn't have time to review. Just a quick note about API changes in clusterctl:

When this package is used as a library, we do not currently provide any compatibility guarantees. We will make reasonable efforts to follow a typical deprecation period prior to removal, but breaking changes can happen when necessary.
https://github.com/kubernetes-sigs/cluster-api/tree/main/cmd/clusterctl

Fedosin · 2023-06-30T15:40:09Z

Hey, thanks for your comments! I think capi operator is the main consumer of clusterctl functions, and we are okay with this change.

Although there are a lot of changes, they are very primitive, 80% of them is just about adding ctx as the first parameter to the functions. Additionally, to simplify the code review, I broke it up into several commits. But it seems to me that breaking this up into several PRs won't make the review any easier. Please tell me if you think it is important for you.

fabriziopandini · 2023-06-30T17:21:55Z

It will take some time for me to review such a big PR; it would be great if clusterctl reviewers can make a preliminary pass first

cc @ykakarap @Jont828

I think capi operator is the main consumer of clusterctl functions

We have many other users that have built tooling or automation on top of clusterctl functions; in fact clusterctl has been built as a library for those use cases, and the operator was added on top later.

Considering this, even if we are allowed to do such changes, given that they are so extensive I think we should advertise them a little bit and let the community provide feedback before merging at this stage of the release.

ykakarap · 2023-06-30T17:23:36Z

I can take a pass at this.

Fedosin · 2023-06-30T20:35:18Z

@ykakarap thanks! ping me here or in slack if you have any questions about this PR.

ykakarap

Overall LGTM.

cmd/clusterctl/client/config/client.go

cmd/clusterctl/client/config_test.go

fabriziopandini · 2023-07-03T10:55:50Z

/hold
for community feedback

Fedosin · 2023-07-10T10:45:35Z

/test pull-cluster-api-test-main

Jont828

Nice work! This must have taken a while. Everything looks pretty straightforward to me. Just had a question about if we can use the context we're passing in for logs.

Jont828 · 2023-07-21T21:34:57Z

cmd/clusterctl/client/cluster/cert_manager.go

 }

-func (cm *certManagerClient) install() error {
+func (cm *certManagerClient) install(ctx context.Context) error {


In the line below, can we get a logger from the context now that we're passing one in? I'm not sure as we're not in a controller.

Yeah, technically we can. sigs.k8s.io/controller-runtime/pkg/log provides required mechanisms to do so, and we can replace log := logf.Log with log := crlog.FromContext(ctx).
The issue is that the current pattern is found in many places in the code, not just where we pass the context (for instance here).
So I believe getting a logger from the context is achievable, but not within the scope of this PR, because a change like that would make it harder to review.

That's fine by me, this PR is already pretty large, and we can consider a follow up if we still want a log from context.

Jont828 · 2023-07-31T21:51:11Z

/lgtm

k8s-ci-robot · 2023-08-15T23:41:03Z

LGTM label has been added.

Git tree hash: 37e42ff7f954fb3b54ee354a989884eca5cbb6af

fabriziopandini · 2023-08-17T17:01:42Z

@Fedosin I propose to move forward with this PR by changing the PR type into breaking change and adding a note about this change into https://main.cluster-api.sigs.k8s.io/developer/providers/migrations/v1.5-to-v1.6

@sbueringer @killianmuldoon opinions?

sbueringer · 2023-08-18T09:38:49Z

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

sbueringer · 2023-08-18T09:45:36Z

Thank you very much!!

/lgtm
/approve

/hold
for e2e-full

I'm planning to merge this PR as soon as CI is green to avoid further rebasing (we also have a few more huge PRs in the pipeline)

@Fedosin Please follow-up with a PR to "add a note about this change into main.cluster-api.sigs.k8s.io/developer/providers/migrations/v1.5-to-v1.6"

k8s-ci-robot · 2023-08-18T09:45:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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~~ [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-08-18T09:52:38Z

@Fedosin Not sure why the errors are just showing up now. PTAL at the CI issues. Please add additional changes in separate commits to make delta-reviewing easy for those

Also feel free to directly make the change to the migration doc directly in this PR, now that we need some changes anyway :)

Fedosin · 2023-08-18T12:58:45Z

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

sbueringer · 2023-08-18T13:11:08Z

Thank you very much!!

/lgtm

Can you squash the commits (with just a squash without another rebase onto main the lgtm label should be preserved)

Feel free to hold cancel after squash + e2e-full green

k8s-ci-robot · 2023-08-18T13:11:12Z

LGTM label has been added.

Git tree hash: eceeb15c679ba8107479c3f43c205e990ff3344d

Fedosin · 2023-08-18T14:36:38Z

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

add context suppport to cmd/clusterctl/client/alpha add context suppport to cmd/clusterctl/client/cluster add context suppport to cmd/clusterctl/client/config add context suppport to cmd/clusterctl/client/repository add context suppport to cmd/clusterctl/client add context suppport to cmd/clusterctl/cmd add context suppport to cmd/clusterctl/internal start using clusterctl context in external functions fix conflicts in cmd/clusterctl/client/cluster/mover update migration docs

Fedosin · 2023-08-18T14:38:31Z

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

k8s-ci-robot · 2023-08-18T14:39:54Z

@Fedosin: 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-apidiff-main	`dae1103`	link	false	`/test pull-cluster-api-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.

Fedosin · 2023-08-18T16:00:21Z

thank you all so much for the review!

/hold cancel

sbueringer · 2023-08-18T16:11:54Z

Thx, especially for the patience! :)

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 29, 2023

k8s-ci-robot requested review from elmiko and jackfrancis June 29, 2023 16:15

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. area/clusterctl Issues or PRs related to clusterctl labels Jun 29, 2023

ykakarap reviewed Jul 3, 2023

View reviewed changes

cmd/clusterctl/client/config/client.go Outdated Show resolved Hide resolved

cmd/clusterctl/client/config_test.go Outdated 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 Jul 3, 2023

Fedosin force-pushed the clusterctl_context branch 4 times, most recently from c6a948a to d69185a Compare July 4, 2023 10:59

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 8, 2023

Fedosin force-pushed the clusterctl_context branch from d69185a to 105ddb7 Compare July 9, 2023 19:49

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 9, 2023

Fedosin force-pushed the clusterctl_context branch from 105ddb7 to d69185a Compare July 10, 2023 09:50

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 10, 2023

Fedosin force-pushed the clusterctl_context branch from d69185a to fb926c4 Compare July 10, 2023 10:02

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 10, 2023

Jont828 reviewed Jul 21, 2023

View reviewed changes

k8s-ci-robot assigned Jont828 Jul 31, 2023

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

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

k8s-ci-robot assigned sbueringer Aug 18, 2023

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

sbueringer changed the title ~~🌱 Improve Context handling in clusterctl~~ ⚠️ Improve Context handling in clusterctl Aug 18, 2023

sbueringer mentioned this pull request Aug 18, 2023

🌱 Replacing gomega.Equal with gomega.BeComparableTo #9015

Merged

Fedosin force-pushed the clusterctl_context branch from 46f11e7 to 7d626ae Compare August 18, 2023 12:07

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

k8s-ci-robot requested a review from sbueringer August 18, 2023 12:07

Fedosin force-pushed the clusterctl_context branch from 7d626ae to 8e5c51a Compare August 18, 2023 12:48

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

Fedosin force-pushed the clusterctl_context branch from 8e5c51a to b3b9613 Compare August 18, 2023 14:35

Fedosin force-pushed the clusterctl_context branch from b3b9613 to dae1103 Compare August 18, 2023 14:37

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

k8s-ci-robot merged commit 82eff49 into kubernetes-sigs:main Aug 18, 2023

k8s-ci-robot added this to the v1.6 milestone Aug 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

⚠️ Improve Context handling in clusterctl #8939

⚠️ Improve Context handling in clusterctl #8939

Fedosin commented Jun 29, 2023 •

edited

Loading

enxebre commented Jun 30, 2023 •

edited

Loading

sbueringer commented Jun 30, 2023 •

edited

Loading

Fedosin commented Jun 30, 2023

fabriziopandini commented Jun 30, 2023

ykakarap commented Jun 30, 2023

Fedosin commented Jun 30, 2023

ykakarap left a comment

fabriziopandini commented Jul 3, 2023

Fedosin commented Jul 10, 2023

Jont828 left a comment

Jont828 Jul 21, 2023

Fedosin Jul 31, 2023

Jont828 Jul 31, 2023

Jont828 commented Jul 31, 2023

k8s-ci-robot commented Aug 15, 2023

fabriziopandini commented Aug 17, 2023

sbueringer commented Aug 18, 2023

sbueringer commented Aug 18, 2023

k8s-ci-robot commented Aug 18, 2023

sbueringer commented Aug 18, 2023 •

edited

Loading

Fedosin commented Aug 18, 2023

sbueringer commented Aug 18, 2023

k8s-ci-robot commented Aug 18, 2023

Fedosin commented Aug 18, 2023

Fedosin commented Aug 18, 2023

k8s-ci-robot commented Aug 18, 2023

Fedosin commented Aug 18, 2023

sbueringer commented Aug 18, 2023

⚠️ Improve Context handling in clusterctl #8939

⚠️ Improve Context handling in clusterctl #8939

Conversation

Fedosin commented Jun 29, 2023 • edited Loading

enxebre commented Jun 30, 2023 • edited Loading

sbueringer commented Jun 30, 2023 • edited Loading

Fedosin commented Jun 30, 2023

fabriziopandini commented Jun 30, 2023

ykakarap commented Jun 30, 2023

Fedosin commented Jun 30, 2023

ykakarap left a comment

Choose a reason for hiding this comment

fabriziopandini commented Jul 3, 2023

Fedosin commented Jul 10, 2023

Jont828 left a comment

Choose a reason for hiding this comment

Jont828 Jul 21, 2023

Choose a reason for hiding this comment

Fedosin Jul 31, 2023

Choose a reason for hiding this comment

Jont828 Jul 31, 2023

Choose a reason for hiding this comment

Jont828 commented Jul 31, 2023

k8s-ci-robot commented Aug 15, 2023

fabriziopandini commented Aug 17, 2023

sbueringer commented Aug 18, 2023

sbueringer commented Aug 18, 2023

k8s-ci-robot commented Aug 18, 2023

sbueringer commented Aug 18, 2023 • edited Loading

Fedosin commented Aug 18, 2023

sbueringer commented Aug 18, 2023

k8s-ci-robot commented Aug 18, 2023

Fedosin commented Aug 18, 2023

Fedosin commented Aug 18, 2023

k8s-ci-robot commented Aug 18, 2023

Fedosin commented Aug 18, 2023

sbueringer commented Aug 18, 2023

Fedosin commented Jun 29, 2023 •

edited

Loading

enxebre commented Jun 30, 2023 •

edited

Loading

sbueringer commented Jun 30, 2023 •

edited

Loading

sbueringer commented Aug 18, 2023 •

edited

Loading