✨ enable kubeadm feature gates mutation #10154

adityabhatia · 2024-02-14T19:03:31Z

What this PR does / why we need it:

This PR aims at:

Enabling mutation of FeatureGates in webhooks
Updating the target field for FeatureGates in Kubeadm config map
Reducing the number of API calls to update Kubeadm config map to 1

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

/area api

adityabhatia · 2024-02-14T19:10:26Z

/area api

adityabhatia · 2024-02-14T19:11:12Z

cc: @fabriziopandini

adityabhatia · 2024-02-14T23:25:51Z

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

k8s-ci-robot · 2024-02-14T23:25:54Z

@adityabhatia: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test pull-cluster-api-build-main
/test pull-cluster-api-e2e-blocking-main
/test pull-cluster-api-e2e-conformance-ci-latest-main
/test pull-cluster-api-e2e-conformance-main
/test pull-cluster-api-e2e-dualstack-and-ipv6-main
/test pull-cluster-api-e2e-main
/test pull-cluster-api-e2e-mink8s-main
/test pull-cluster-api-e2e-upgrade-1-29-1-30-main
/test pull-cluster-api-test-main
/test pull-cluster-api-test-mink8s-main
/test pull-cluster-api-verify-main

The following commands are available to trigger optional jobs:

/test pull-cluster-api-apidiff-main

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

pull-cluster-api-apidiff-main
pull-cluster-api-build-main
pull-cluster-api-e2e-blocking-main
pull-cluster-api-test-main
pull-cluster-api-verify-main

In response to this:

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

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.

adityabhatia · 2024-02-14T23:33:44Z

/test pull-cluster-api-e2e-main

neolit123

would FG mutations trigger a machine roll-out?

some FGs would only matter for different kubeadm commands, some of which are not used in CAPI.

i guess CAPI users would be required to understand what FGs can be a NO-OP for them?
e.g. one that is "kubeadm reset" or "kubeadm upgrade apply" specific.

controlplane/kubeadm/internal/controllers/fakes_test.go

controlplane/kubeadm/internal/controllers/upgrade.go

controlplane/kubeadm/internal/workload_cluster_test.go

adityabhatia · 2024-02-15T10:17:54Z

would FG mutations trigger a machine roll-out?

some FGs would only matter for different kubeadm commands, some of which are not used in CAPI.

i guess CAPI users would be required to understand what FGs can be a NO-OP for them? e.g. one that is "kubeadm reset" or "kubeadm upgrade apply" specific.

It does an automatic machine rollout, also mentioned here. I will double check this.

adityabhatia · 2024-02-15T10:37:55Z

/test pull-cluster-api-e2e-main

adityabhatia · 2024-02-15T15:23:16Z

/test pull-cluster-api-e2e-main

chrischdi · 2024-02-16T07:34:59Z

Looks like this adds a panic / nil pointer dereference:

panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x187ad13]

goroutine 168 [running]:
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Reconcile.func1()
	sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:116 +0x1e5
panic({0x1d6e780?, 0x3585650?})
	runtime/panic.go:914 +0x21f
sigs.k8s.io/cluster-api/controlplane/kubeadm/internal/controllers.(*KubeadmControlPlaneReconciler).upgradeControlPlane(0x20eb3c3?, {0x23c8618, 0xc001d07dd0}, 0xc0023ea9a0, 0x2?)
	sigs.k8s.io/cluster-api/controlplane/kubeadm/internal/controllers/upgrade.go:91 +0x5f3
sigs.k8s.io/cluster-api/controlplane/kubeadm/internal/controllers.(*KubeadmControlPlaneReconciler).reconcile(0xc0004ce6e0?, {0x23c8618, 0xc001d07dd0}, 0xc0023ea9a0)
	sigs.k8s.io/cluster-api/controlplane/kubeadm/internal/controllers/controller.go:407 +0x1579
sigs.k8s.io/cluster-api/controlplane/kubeadm/internal/controllers.(*KubeadmControlPlaneReconciler).Reconcile(0xc0004ce6e0, {0x23c8618, 0xc001d07bc0}, {{{0xc0023cc4a6?, 0x0?}, {0xc0023cc4b0?, 0xc001fb0d08?}}})
	sigs.k8s.io/cluster-api/controlplane/kubeadm/internal/controllers/controller.go:251 +0x994
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Reconcile(0x23cced0?, {0x23c8618?, 0xc001d07bc0?}, {{{0xc0023cc4a6?, 0xb?}, {0xc0023cc4b0?, 0x0?}}})
	sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:119 +0xb7
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler(0xc0004ce780, {0x23c8650, 0xc0003582d0}, {0x1e33aa0?, 0xc0016fe4a0?})
	sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:316 +0x3cc
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem(0xc0004ce780, {0x23c8650, 0xc0003582d0})
	sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:266 +0x1af
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2()
	sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:227 +0x79
created by sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2 in goroutine 201
	sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:223 +0x565

controlplane/kubeadm/internal/controllers/upgrade.go

adityabhatia · 2024-02-16T10:42:01Z

/retest

adityabhatia · 2024-02-16T10:53:52Z

/test pull-cluster-api-e2e-main

controlplane/kubeadm/internal/workload_cluster.go

controlplane/kubeadm/internal/controllers/upgrade.go

controlplane/kubeadm/internal/workload_cluster.go

controlplane/kubeadm/internal/workload_cluster_test.go

controlplane/kubeadm/internal/workload_cluster.go

fabriziopandini · 2024-02-19T17:39:15Z

controlplane/kubeadm/internal/workload_cluster_etcd.go

@@ -93,22 +93,24 @@ loopmembers:
 }

 // UpdateEtcdVersionInKubeadmConfigMap sets the imageRepository or the imageTag or both in the kubeadm config map.
-func (w *Workload) UpdateEtcdVersionInKubeadmConfigMap(ctx context.Context, imageRepository, imageTag string, version semver.Version) error {
-	return w.updateClusterConfiguration(ctx, func(c *bootstrapv1.ClusterConfiguration) {
+func (w *Workload) UpdateEtcdVersionInKubeadmConfigMap(imageRepository, imageTag string) func(*bootstrapv1.ClusterConfiguration, *[]string) {


I think we are missing something for etcd.
If I look at what the webhook allows to change, we can change everything both for local etcd and external etcd.

Instead here (and in UpdateEtcdExtraArgsInKubeadmConfigMap) we do not persist changes to:

etcd.local.DataDir

etcd.local.ServerCertSANs

etcd.local.PeerCertSANs

etcd.External.*

What about refactoring UpdateEtcdVersionInKubeadmConfigMap and UpdateEtcdExtraArgsInKubeadmConfigMap into UpdateEtcdLocalInKubeadmConfigMap (+ filling the gaps) and adding a new UpdateEtcdExternalInKubeadmConfigMap to be called from controlplane/kubeadm/internal/controllers/upgrade.go?

Sounds good, we can additionally persist all local and external properties of etcd in the config map.
Since this requires a refactoring to the current methods and subsequently the interface as well (and tests), it makes sense to combine it with the interface clean-up addressed here IMHO #10154 (comment), wdyt? I can take this up directly after this issues is closed.

let's tackle this in this PR (thinking about a potential backport in the 1.6 branch)

adityabhatia · 2024-02-20T20:07:08Z

/test pull-cluster-api-e2e-main

fabriziopandini · 2024-02-22T11:33:17Z

/lgtm
@chrischdi do you have time for a final pass?

k8s-ci-robot · 2024-02-22T11:33:24Z

LGTM label has been added.

Git tree hash: 278f3645cf59d69dc575f439192a09a621799c61

chrischdi · 2024-02-22T11:51:20Z

controlplane/kubeadm/internal/workload_cluster_test.go

@@ -30,6 +30,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+	yaml2 "sigs.k8s.io/yaml"


Suggested change

yaml2 "sigs.k8s.io/yaml"

"sigs.k8s.io/yaml"

Let's alias the capi owned yaml package to utilyaml here (we already do so in other packages)

Also changed in other files, which are a part of this PR.

would not have been necessary but also does not hurt 👍

chrischdi · 2024-02-22T11:52:13Z

One last nit :-)

chrischdi · 2024-02-22T12:25:57Z

/approve

k8s-ci-robot · 2024-02-22T12:26:05Z

[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

fabriziopandini · 2024-02-22T12:42:24Z

/lgtm

k8s-ci-robot · 2024-02-22T12:42:29Z

LGTM label has been added.

Git tree hash: 91b6788c82d31056e1b3a78bb7094878283641d2

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 14, 2024

k8s-ci-robot requested review from g-gaston and stmcginnis February 14, 2024 19:03

k8s-ci-robot added do-not-merge/needs-area PR is missing an area label size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 14, 2024

k8s-ci-robot added area/api Issues or PRs related to the APIs and removed do-not-merge/needs-area PR is missing an area label labels Feb 14, 2024

adityabhatia force-pushed the kcpFeatureGatesMutation branch 2 times, most recently from 35c3ba4 to b5a33bc Compare February 14, 2024 23:04

adityabhatia changed the title ~~✨ enable kubeadm feature flags mutation~~ ✨ enable kubeadm feature gates mutation Feb 15, 2024

neolit123 reviewed Feb 15, 2024

View reviewed changes

adityabhatia force-pushed the kcpFeatureGatesMutation branch from b5a33bc to 822ebcf Compare February 15, 2024 10:20

adityabhatia force-pushed the kcpFeatureGatesMutation branch from 822ebcf to a2b80c0 Compare February 15, 2024 15:15

chrischdi reviewed Feb 16, 2024

View reviewed changes

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

adityabhatia force-pushed the kcpFeatureGatesMutation branch from a2b80c0 to 8702cfc Compare February 16, 2024 10:08

chrischdi reviewed Feb 16, 2024

View reviewed changes

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

chrischdi reviewed Feb 16, 2024

View reviewed changes

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

adityabhatia force-pushed the kcpFeatureGatesMutation branch 2 times, most recently from 2616f47 to b68eefb Compare February 16, 2024 15:12

chrischdi reviewed Feb 19, 2024

View reviewed changes

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

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

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

fabriziopandini reviewed Feb 19, 2024

View reviewed changes

adityabhatia force-pushed the kcpFeatureGatesMutation branch 5 times, most recently from f2d9b42 to 885fd17 Compare February 20, 2024 19:45

k8s-ci-robot assigned fabriziopandini Feb 22, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 22, 2024

chrischdi reviewed Feb 22, 2024

View reviewed changes

enable kubeadm feature flags mutation

d86ba52

adityabhatia force-pushed the kcpFeatureGatesMutation branch from 885fd17 to d86ba52 Compare February 22, 2024 12:24

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 22, 2024

k8s-ci-robot requested a review from fabriziopandini February 22, 2024 12:24

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 22, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 22, 2024

k8s-ci-robot merged commit 065c072 into kubernetes-sigs:main Feb 22, 2024
20 checks passed

k8s-ci-robot added this to the v1.7 milestone Feb 22, 2024

fabriziopandini mentioned this pull request Feb 26, 2024

✨Simplify alternative control-plane implementation #10198

Closed

jonathanmeier5 mentioned this pull request Feb 26, 2024

Disable ETCD Learner Mode aws/eks-anywhere#7669

Merged

abhinavmpandey08 mentioned this pull request Feb 28, 2024

add capi patch to enable kubeadm feature flags mutation aws/eks-anywhere-build-tooling#2958

Merged

sbueringer mentioned this pull request Apr 16, 2024

🚀 Release v1.7.0 #10440

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ enable kubeadm feature gates mutation #10154

✨ enable kubeadm feature gates mutation #10154

adityabhatia commented Feb 14, 2024 •

edited

Loading

adityabhatia commented Feb 14, 2024

adityabhatia commented Feb 14, 2024

adityabhatia commented Feb 14, 2024

k8s-ci-robot commented Feb 14, 2024

adityabhatia commented Feb 14, 2024

neolit123 left a comment

adityabhatia commented Feb 15, 2024

adityabhatia commented Feb 15, 2024

adityabhatia commented Feb 15, 2024

chrischdi commented Feb 16, 2024

adityabhatia commented Feb 16, 2024

adityabhatia commented Feb 16, 2024

fabriziopandini Feb 19, 2024

adityabhatia Feb 19, 2024 •

edited

Loading

fabriziopandini Feb 20, 2024

adityabhatia commented Feb 20, 2024

fabriziopandini commented Feb 22, 2024

k8s-ci-robot commented Feb 22, 2024

chrischdi Feb 22, 2024

adityabhatia Feb 22, 2024

chrischdi Feb 22, 2024

chrischdi commented Feb 22, 2024

chrischdi commented Feb 22, 2024

k8s-ci-robot commented Feb 22, 2024

fabriziopandini commented Feb 22, 2024

k8s-ci-robot commented Feb 22, 2024

✨ enable kubeadm feature gates mutation #10154

✨ enable kubeadm feature gates mutation #10154

Conversation

adityabhatia commented Feb 14, 2024 • edited Loading

adityabhatia commented Feb 14, 2024

adityabhatia commented Feb 14, 2024

adityabhatia commented Feb 14, 2024

k8s-ci-robot commented Feb 14, 2024

adityabhatia commented Feb 14, 2024

neolit123 left a comment

Choose a reason for hiding this comment

adityabhatia commented Feb 15, 2024

adityabhatia commented Feb 15, 2024

adityabhatia commented Feb 15, 2024

chrischdi commented Feb 16, 2024

adityabhatia commented Feb 16, 2024

adityabhatia commented Feb 16, 2024

fabriziopandini Feb 19, 2024

Choose a reason for hiding this comment

adityabhatia Feb 19, 2024 • edited Loading

Choose a reason for hiding this comment

fabriziopandini Feb 20, 2024

Choose a reason for hiding this comment

adityabhatia commented Feb 20, 2024

fabriziopandini commented Feb 22, 2024

k8s-ci-robot commented Feb 22, 2024

chrischdi Feb 22, 2024

Choose a reason for hiding this comment

adityabhatia Feb 22, 2024

Choose a reason for hiding this comment

chrischdi Feb 22, 2024

Choose a reason for hiding this comment

chrischdi commented Feb 22, 2024

chrischdi commented Feb 22, 2024

k8s-ci-robot commented Feb 22, 2024

fabriziopandini commented Feb 22, 2024

k8s-ci-robot commented Feb 22, 2024

adityabhatia commented Feb 14, 2024 •

edited

Loading

adityabhatia Feb 19, 2024 •

edited

Loading