🌱 Refactor KubeadmConfig object update during cluster upgrades #4049

srm09 · 2021-01-05T21:37:26Z

What this PR does / why we need it:
This patch removes the multiple update calls made to ETCD while updating the kubeadmConfig spec during upgrades. Instead, all the updates are batched into a single patch call made at the end of the reconcile loop.

Which issue(s) this PR fixes:
Fixes #4007

srm09 · 2021-01-05T21:38:49Z

/hold
Waiting for #3994 to get merged.

srm09 · 2021-01-05T21:39:33Z

/cc @shysank @fabriziopandini

k8s-ci-robot · 2021-01-05T21:39:35Z

@srm09: GitHub didn't allow me to request PR reviews from the following users: shysank.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @shysank @fabriziopandini

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.

srm09 · 2021-01-05T21:52:47Z

/retitle 🌱 Refactor KubeadmConfig object update during cluster upgrades

srm09 · 2021-01-06T19:21:15Z

controlplane/kubeadm/controllers/upgrade.go

+	patchHelper, err := patch.NewHelper(kubeadmConfig.GetConfigMap(), r.Client)
+	if err != nil {
+		return ctrl.Result{}, err
+	}


@fabriziopandini @vincepri I was having a conversation with @shysank around this, and he mentioned that workload clusters might be managing their own config maps (and these config maps do not get stored in the management cluster). Is that actually the case, in which case, the client used to initialize the patchHelper above would not do the right thing.

The kubeadm configmap should be technically only managed through a Cluster API deployment (KCP in this case),

With the changes above, it doesn't seem to me that we'd overwrite other data, apart from the one that we're managing?

So the patchHelper initialized with the managementCluster.Client would correctly persist the changes to the kubeadmConfig. Thanks for clearing it up.

Just want to confirm on the patch helper client. The code above gets the config map from the workload cluster, but the patch helper updates the config map in the management cluster. Shouldn't the patch helper also use the workload cluster's client? I'm not entirely sure if we should be updating the management cluster's kubeadm config map.

fabriziopandini

In general, I really like the fact that we are cleaning up the Workload Cluster interface.
I still have to complete the review, but I will start with few initial comments on the new KubeamConfig interface

controlplane/kubeadm/controllers/upgrade.go

controlplane/kubeadm/internal/kubeadm_config_map.go

controlplane/kubeadm/controllers/upgrade.go

k8s-ci-robot · 2021-01-11T21:09:10Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign justinsb after the PR has been reviewed.
You can assign the PR to them by writing /assign @justinsb in a comment when ready.

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

Needs approval from an approver in each of these files:

OWNERS

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

srm09 · 2021-01-12T17:22:38Z

@fabriziopandini I refactored the methods according to the pointers you had provided. I have a doubt that I need to clear and then I will finalize the changes in the PR

srm09 · 2021-01-13T00:23:42Z

@vincepri Added the change to update the patchHelper with the workload cluster's client. PTAL.

srm09 · 2021-01-13T00:26:36Z

/test pull-cluster-api-test-main

controlplane/kubeadm/internal/workload_cluster.go

controlplane/kubeadm/internal/workload_cluster_coredns.go

controlplane/kubeadm/internal/workload_cluster_etcd_test.go

controlplane/kubeadm/internal/workload_cluster.go

srm09 · 2021-01-14T01:02:51Z

/retest

fabriziopandini · 2021-01-19T12:53:04Z

/milestone v0.4.0
Changes lgtm to me and as I said before I like the fact that we are cleaning up the Workload Cluster interface.
Hopefully, we can get some more opinions about this PR...

srm09 · 2021-01-19T17:09:00Z

/hold cancel

vincepri · 2021-01-19T17:10:22Z

@srm09 Please squash :)

Instead of issuing multiple update calls during the KCP upgrade, the reconciler makes the changes to the same kubeadmConfig object and the changes to the object are persisted using a patch() call. This also removes the intermediate methods on the WorkloadCluster interface and the reconciler calls these methods directly on the kubeadmConfig object. This also exposes a getter for workload cluster's client Signed-off-by: Sagar Muchhal <[email protected]>

fabriziopandini · 2021-01-19T17:50:24Z

/lgtm

srm09 · 2021-01-27T23:39:53Z

@justinsb @vincepri Do the changes look fine? Any suggestions around improvements or feedback?

srm09 · 2021-03-19T18:23:49Z

Might need to rework this, this PR stopped working. If I could get some feedback around the approach, I can open another PR for this one.

k8s-ci-robot · 2021-03-19T18:23:55Z

@srm09: PR needs rebase.

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.

vincepri · 2021-03-24T01:52:18Z

@srm09 I can chat more tomorrow sync if that helps, although I'd suggest to add more description about the solution chosen and the problem we're trying to solve

srm09 · 2021-03-24T05:57:27Z

@vincepri I tried revising this PR off the latest main branch, but it is broken. Maybe, I need to rethink this change a bit since there is a lotta mocking that needs to be done for the unit tests to be run.

CecileRobertMichon · 2021-04-13T23:03:08Z

/lgtm cancel

PR needs rebase

fabriziopandini · 2021-05-05T13:04:31Z

The goal of this PR still is valid but I'm wondering we we should freeze this effort and reconsider at later stage because a lot is changing in this part of the codebase as a consequence of the the introduction of embedded kubeadm types & the introduction of the support for multiple kubeadm API versions.
e.g #4443 is going to refactor quite a bit how the kubeadm config map is managed...

srm09 · 2021-07-23T15:19:04Z

/close

k8s-ci-robot · 2021-07-23T15:19:09Z

@srm09: Closed this PR.

In response to this:

/close

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.

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 5, 2021

k8s-ci-robot requested review from justinsb and vincepri January 5, 2021 21:37

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

k8s-ci-robot requested a review from fabriziopandini January 5, 2021 21:39

srm09 force-pushed the refactor/kubeadm_config_map branch 2 times, most recently from 4ad6f75 to c229ddd Compare January 5, 2021 21:47

k8s-ci-robot changed the title ~~🌱 Refactor KubeadmConfig object~~ 🌱 Refactor KubeadmConfig object update during cluster upgrades Jan 5, 2021

srm09 force-pushed the refactor/kubeadm_config_map branch from c229ddd to a395518 Compare January 6, 2021 18:41

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 6, 2021

srm09 commented Jan 6, 2021

View reviewed changes

fabriziopandini reviewed Jan 7, 2021

View reviewed changes

controlplane/kubeadm/controllers/upgrade.go Outdated Show resolved Hide resolved

controlplane/kubeadm/internal/kubeadm_config_map.go Outdated Show resolved Hide resolved

controlplane/kubeadm/controllers/upgrade.go Outdated Show resolved Hide resolved

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 11, 2021

fabriziopandini reviewed Jan 13, 2021

View reviewed changes

k8s-ci-robot added this to the v0.4.0 milestone Jan 19, 2021

srm09 force-pushed the refactor/kubeadm_config_map branch from dc1c5f0 to 7d29954 Compare January 19, 2021 17:07

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

srm09 force-pushed the refactor/kubeadm_config_map branch from 7d29954 to 1e171e5 Compare January 19, 2021 17:12

k8s-ci-robot assigned fabriziopandini Jan 19, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 19, 2021

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

k8s-ci-robot assigned CecileRobertMichon Apr 13, 2021

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 13, 2021

k8s-ci-robot closed this Jul 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 Refactor KubeadmConfig object update during cluster upgrades #4049

🌱 Refactor KubeadmConfig object update during cluster upgrades #4049

srm09 commented Jan 5, 2021 •

edited

Loading

srm09 commented Jan 5, 2021

srm09 commented Jan 5, 2021

k8s-ci-robot commented Jan 5, 2021

srm09 commented Jan 5, 2021

srm09 Jan 6, 2021

vincepri Jan 6, 2021

srm09 Jan 6, 2021

shysank Jan 7, 2021

vincepri Jan 11, 2021

fabriziopandini left a comment

k8s-ci-robot commented Jan 11, 2021

srm09 commented Jan 12, 2021

srm09 commented Jan 13, 2021

srm09 commented Jan 13, 2021

srm09 commented Jan 14, 2021

fabriziopandini commented Jan 19, 2021

srm09 commented Jan 19, 2021

vincepri commented Jan 19, 2021

fabriziopandini commented Jan 19, 2021

srm09 commented Jan 27, 2021

srm09 commented Mar 19, 2021

k8s-ci-robot commented Mar 19, 2021

vincepri commented Mar 24, 2021

srm09 commented Mar 24, 2021

CecileRobertMichon commented Apr 13, 2021

fabriziopandini commented May 5, 2021

srm09 commented Jul 23, 2021

k8s-ci-robot commented Jul 23, 2021

🌱 Refactor KubeadmConfig object update during cluster upgrades #4049

🌱 Refactor KubeadmConfig object update during cluster upgrades #4049

Conversation

srm09 commented Jan 5, 2021 • edited Loading

srm09 commented Jan 5, 2021

srm09 commented Jan 5, 2021

k8s-ci-robot commented Jan 5, 2021

srm09 commented Jan 5, 2021

srm09 Jan 6, 2021

Choose a reason for hiding this comment

vincepri Jan 6, 2021

Choose a reason for hiding this comment

srm09 Jan 6, 2021

Choose a reason for hiding this comment

shysank Jan 7, 2021

Choose a reason for hiding this comment

vincepri Jan 11, 2021

Choose a reason for hiding this comment

fabriziopandini left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 11, 2021

srm09 commented Jan 12, 2021

srm09 commented Jan 13, 2021

srm09 commented Jan 13, 2021

srm09 commented Jan 14, 2021

fabriziopandini commented Jan 19, 2021

srm09 commented Jan 19, 2021

vincepri commented Jan 19, 2021

fabriziopandini commented Jan 19, 2021

srm09 commented Jan 27, 2021

srm09 commented Mar 19, 2021

k8s-ci-robot commented Mar 19, 2021

vincepri commented Mar 24, 2021

srm09 commented Mar 24, 2021

CecileRobertMichon commented Apr 13, 2021

fabriziopandini commented May 5, 2021

srm09 commented Jul 23, 2021

k8s-ci-robot commented Jul 23, 2021

srm09 commented Jan 5, 2021 •

edited

Loading