Skip to content

Commit 99ea82d

Browse files
sbueringersbueringer
committed
Fix review findings
1 parent 33c245f commit 99ea82d

File tree

11 files changed

+63
-24
lines changed

11 files changed

+63
-24
lines changed

‎controlplane/kubeadm/internal/controllers/inplace.go‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,9 +38,10 @@ func (r *KubeadmControlPlaneReconciler) tryInPlaceUpdate(
3838

3939
// Run preflight checks to ensure that the control plane is stable before proceeding with in-place update operation.
4040
if resultForAllMachines := r.preflightChecks(ctx, controlPlane); !resultForAllMachines.IsZero() {
41-
// We should not block a scale down of an unhealthy Machine that would work.
41+
// If the control plane is not stable, check if the issues are only for machineToInPlaceUpdate.
4242
if result := r.preflightChecks(ctx, controlPlane, machineToInPlaceUpdate); result.IsZero() {
43-
// Fallback to scale down.
43+
// The issues are only for machineToInPlaceUpdate, fallback to scale down.
44+
// Note: The consequence of this is that a Machine with issues is scaled down and not in-place updated.
4445
return true, ctrl.Result{}, nil
4546
}
4647

‎controlplane/kubeadm/internal/controllers/inplace_canupdatemachine.go‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,13 +74,16 @@ func (r *KubeadmControlPlaneReconciler) canUpdateMachine(ctx context.Context, ma
7474
if len(extensionHandlers) == 0 {
7575
return false, nil
7676
}
77+
if len(extensionHandlers) > 1 {
78+
return false, errors.Errorf("found multiple CanUpdateMachine hooks (%s) (more than one is not supported yet)", strings.Join(extensionHandlers, ","))
79+
}
7780

7881
canUpdateMachine, reasons, err := r.canExtensionsUpdateMachine(ctx, machine, machineUpToDateResult, extensionHandlers)
7982
if err != nil {
8083
return false, err
8184
}
8285
if !canUpdateMachine {
83-
log.Info(fmt.Sprintf("Machine cannot be updated in-place: %s", strings.Join(reasons, ",")), "Machine", klog.KObj(machine))
86+
log.Info(fmt.Sprintf("Machine cannot be updated in-place by extensions: %s", strings.Join(reasons, ",")), "Machine", klog.KObj(machine))
8487
return false, nil
8588
}
8689
return true, nil

‎controlplane/kubeadm/internal/controllers/inplace_canupdatemachine_test.go‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,16 @@ func Test_canUpdateMachine(t *testing.T) {
9595
getAllExtensionsResponses: map[runtimecatalog.GroupVersionHook][]string{},
9696
wantCanUpdateMachine: false,
9797
},
98+
{
99+
name: "Return error if more than one CanUpdateMachine extensions registered",
100+
enableInPlaceUpdatesFeatureGate: true,
101+
machineUpToDateResult: nonEmptyMachineUpToDateResult,
102+
getAllExtensionsResponses: map[runtimecatalog.GroupVersionHook][]string{
103+
canUpdateMachineGVH: {"test-update-extension-1", "test-update-extension-2"},
104+
},
105+
wantError: true,
106+
wantErrorMessage: "found multiple CanUpdateMachine hooks (test-update-extension-1,test-update-extension-2) (more than one is not supported yet)",
107+
},
98108
{
99109
name: "Return false if canExtensionsUpdateMachine returns false",
100110
enableInPlaceUpdatesFeatureGate: true,

‎controlplane/kubeadm/internal/controllers/scale.go‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -163,6 +163,10 @@ func (r *KubeadmControlPlaneReconciler) preflightChecks(ctx context.Context, con
163163
return r.overridePreflightChecksFunc(ctx, controlPlane, excludeFor...)
164164
}
165165

166+
// Reset PreflightCheckResults in case this function is called multiple times (e.g. for in-place update code paths)
167+
// Note: The PreflightCheckResults field is only written by this func, so this is safe.
168+
controlPlane.PreflightCheckResults = internal.PreflightCheckResults{}
169+
166170
log := ctrl.LoggerFrom(ctx)
167171

168172
// If there is no KCP-owned control-plane machines, then control-plane has not been initialized yet,

‎exp/runtime/client/client.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ package client
2020
import (
2121
"context"
2222

23-
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
23+
"sigs.k8s.io/controller-runtime/pkg/client"
2424

2525
runtimehooksv1 "sigs.k8s.io/cluster-api/api/runtime/hooks/v1alpha1"
2626
runtimev1 "sigs.k8s.io/cluster-api/api/runtime/v1beta2"
@@ -87,11 +87,11 @@ type Client interface {
8787
Unregister(extensionConfig *runtimev1.ExtensionConfig) error
8888

8989
// GetAllExtensions gets all the ExtensionHandlers registered for the hook.
90-
GetAllExtensions(ctx context.Context, hook runtimecatalog.Hook, forObject metav1.Object) ([]string, error)
90+
GetAllExtensions(ctx context.Context, hook runtimecatalog.Hook, forObject client.Object) ([]string, error)
9191

9292
// CallAllExtensions calls all the ExtensionHandler registered for the hook.
93-
CallAllExtensions(ctx context.Context, hook runtimecatalog.Hook, forObject metav1.Object, request runtimehooksv1.RequestObject, response runtimehooksv1.ResponseObject) error
93+
CallAllExtensions(ctx context.Context, hook runtimecatalog.Hook, forObject client.Object, request runtimehooksv1.RequestObject, response runtimehooksv1.ResponseObject) error
9494

9595
// CallExtension calls the ExtensionHandler with the given name.
96-
CallExtension(ctx context.Context, hook runtimecatalog.Hook, forObject metav1.Object, name string, request runtimehooksv1.RequestObject, response runtimehooksv1.ResponseObject, opts ...CallExtensionOption) error
96+
CallExtension(ctx context.Context, hook runtimecatalog.Hook, forObject client.Object, name string, request runtimehooksv1.RequestObject, response runtimehooksv1.ResponseObject, opts ...CallExtensionOption) error
9797
}

‎internal/controllers/topology/cluster/patches/external/external_patch_generator_test.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,8 @@ import (
2121
"testing"
2222

2323
. "github.com/onsi/gomega"
24-
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2524
utilfeature "k8s.io/component-base/featuregate/testing"
25+
"sigs.k8s.io/controller-runtime/pkg/client"
2626

2727
clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2"
2828
runtimehooksv1 "sigs.k8s.io/cluster-api/api/runtime/hooks/v1alpha1"
@@ -124,15 +124,15 @@ func (f *fakeRuntimeClient) Unregister(_ *runtimev1.ExtensionConfig) error {
124124
panic("implement me")
125125
}
126126

127-
func (f *fakeRuntimeClient) GetAllExtensions(_ context.Context, _ runtimecatalog.Hook, _ metav1.Object) ([]string, error) {
127+
func (f *fakeRuntimeClient) GetAllExtensions(_ context.Context, _ runtimecatalog.Hook, _ client.Object) ([]string, error) {
128128
panic("implement me")
129129
}
130130

131-
func (f *fakeRuntimeClient) CallAllExtensions(_ context.Context, _ runtimecatalog.Hook, _ metav1.Object, _ runtimehooksv1.RequestObject, _ runtimehooksv1.ResponseObject) error {
131+
func (f *fakeRuntimeClient) CallAllExtensions(_ context.Context, _ runtimecatalog.Hook, _ client.Object, _ runtimehooksv1.RequestObject, _ runtimehooksv1.ResponseObject) error {
132132
panic("implement me")
133133
}
134134

135-
func (f *fakeRuntimeClient) CallExtension(_ context.Context, _ runtimecatalog.Hook, _ metav1.Object, _ string, request runtimehooksv1.RequestObject, _ runtimehooksv1.ResponseObject, _ ...runtimeclient.CallExtensionOption) error {
135+
func (f *fakeRuntimeClient) CallExtension(_ context.Context, _ runtimecatalog.Hook, _ client.Object, _ string, request runtimehooksv1.RequestObject, _ runtimehooksv1.ResponseObject, _ ...runtimeclient.CallExtensionOption) error {
136136
// Keep a copy of the request object.
137137
// We keep a copy because the request is modified after the call is made. So we keep a copy to perform assertions.
138138
f.callExtensionRequest = request.DeepCopyObject().(runtimehooksv1.RequestObject)

‎internal/runtime/client/client.go‎

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -41,9 +41,11 @@ import (
4141
utilnet "k8s.io/apimachinery/pkg/util/net"
4242
"k8s.io/apimachinery/pkg/util/validation"
4343
"k8s.io/client-go/transport"
44+
"k8s.io/klog/v2"
4445
"k8s.io/utils/ptr"
4546
ctrl "sigs.k8s.io/controller-runtime"
4647
ctrlclient "sigs.k8s.io/controller-runtime/pkg/client"
48+
"sigs.k8s.io/controller-runtime/pkg/client/apiutil"
4749

4850
runtimehooksv1 "sigs.k8s.io/cluster-api/api/runtime/hooks/v1alpha1"
4951
runtimev1 "sigs.k8s.io/cluster-api/api/runtime/v1beta2"
@@ -172,21 +174,25 @@ func (c *client) Unregister(extensionConfig *runtimev1.ExtensionConfig) error {
172174
return nil
173175
}
174176

175-
func (c *client) GetAllExtensions(ctx context.Context, hook runtimecatalog.Hook, forObject metav1.Object) ([]string, error) {
177+
func (c *client) GetAllExtensions(ctx context.Context, hook runtimecatalog.Hook, forObject ctrlclient.Object) ([]string, error) {
176178
hookName := runtimecatalog.HookName(hook)
177179
log := ctrl.LoggerFrom(ctx).WithValues("hook", hookName)
178180
ctx = ctrl.LoggerInto(ctx, log)
179181
gvh, err := c.catalog.GroupVersionHook(hook)
180182
if err != nil {
181183
return nil, errors.Wrapf(err, "failed to get extension handlers for hook %q: failed to compute GroupVersionHook", hookName)
182184
}
185+
forObjectGVK, err := apiutil.GVKForObject(forObject, c.client.Scheme())
186+
if err != nil {
187+
return nil, errors.Wrapf(err, "failed to get extension handlers for hook %q: failed to get GroupVersionKind for the object the hook is executed for", hookName)
188+
}
183189

184190
registrations, err := c.registry.List(gvh.GroupHook())
185191
if err != nil {
186192
return nil, errors.Wrapf(err, "failed to get extension handlers for hook %q", gvh.GroupHook())
187193
}
188194

189-
log.V(4).Info(fmt.Sprintf("Getting all extensions of hook %q", hookName))
195+
log.V(4).Info(fmt.Sprintf("Getting all extensions of hook %q for %s %s", hookName, forObjectGVK.Kind, klog.KObj(forObject)))
190196
matchingRegistrations := []string{}
191197
for _, registration := range registrations {
192198
// Compute whether the object the get is being made for matches the namespaceSelector
@@ -210,14 +216,18 @@ func (c *client) GetAllExtensions(ctx context.Context, hook runtimecatalog.Hook,
210216
// This ensures we don't end up waiting for timeout from multiple unreachable Extensions.
211217
// See CallExtension for more details on when an ExtensionHandler returns an error.
212218
// The aggregated result of the ExtensionHandlers is updated into the response object passed to the function.
213-
func (c *client) CallAllExtensions(ctx context.Context, hook runtimecatalog.Hook, forObject metav1.Object, request runtimehooksv1.RequestObject, response runtimehooksv1.ResponseObject) error {
219+
func (c *client) CallAllExtensions(ctx context.Context, hook runtimecatalog.Hook, forObject ctrlclient.Object, request runtimehooksv1.RequestObject, response runtimehooksv1.ResponseObject) error {
214220
hookName := runtimecatalog.HookName(hook)
215221
log := ctrl.LoggerFrom(ctx).WithValues("hook", hookName)
216222
ctx = ctrl.LoggerInto(ctx, log)
217223
gvh, err := c.catalog.GroupVersionHook(hook)
218224
if err != nil {
219225
return errors.Wrapf(err, "failed to call extension handlers for hook %q: failed to compute GroupVersionHook", hookName)
220226
}
227+
forObjectGVK, err := apiutil.GVKForObject(forObject, c.client.Scheme())
228+
if err != nil {
229+
return errors.Wrapf(err, "failed to call extension handlers for hook %q: failed to get GroupVersionKind for the object the hook is executed for", hookName)
230+
}
221231
// Make sure the request is compatible with the hook.
222232
if err := c.catalog.ValidateRequest(gvh, request); err != nil {
223233
return errors.Wrapf(err, "failed to call extension handlers for hook %q: request object is invalid for hook", gvh.GroupHook())
@@ -232,7 +242,7 @@ func (c *client) CallAllExtensions(ctx context.Context, hook runtimecatalog.Hook
232242
return errors.Wrapf(err, "failed to call extension handlers for hook %q", gvh.GroupHook())
233243
}
234244

235-
log.V(4).Info(fmt.Sprintf("Calling all extensions of hook %q", hookName))
245+
log.V(4).Info(fmt.Sprintf("Calling all extensions of hook %q for %s %s", hookName, forObjectGVK.Kind, klog.KObj(forObject)))
236246
responses := []runtimehooksv1.ResponseObject{}
237247
for _, registration := range registrations {
238248
// Creates a new instance of the response parameter.
@@ -304,7 +314,7 @@ func aggregateSuccessfulResponses(aggregatedResponse runtimehooksv1.ResponseObje
304314
// Nb. FailurePolicy does not affect the following kinds of errors:
305315
// - Internal errors. Examples: hooks is incompatible with ExtensionHandler, ExtensionHandler information is missing.
306316
// - Error when ExtensionHandler returns a response with `Status` set to `Failure`.
307-
func (c *client) CallExtension(ctx context.Context, hook runtimecatalog.Hook, forObject metav1.Object, name string, request runtimehooksv1.RequestObject, response runtimehooksv1.ResponseObject, opts ...runtimeclient.CallExtensionOption) error {
317+
func (c *client) CallExtension(ctx context.Context, hook runtimecatalog.Hook, forObject ctrlclient.Object, name string, request runtimehooksv1.RequestObject, response runtimehooksv1.ResponseObject, opts ...runtimeclient.CallExtensionOption) error {
308318
// Calculate the options.
309319
options := &runtimeclient.CallExtensionOptions{}
310320
for _, opt := range opts {

‎internal/runtime/client/client_test.go‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1148,10 +1148,15 @@ func TestClient_GetAllExtensions(t *testing.T) {
11481148
t.Run(tt.name, func(t *testing.T) {
11491149
g := NewWithT(t)
11501150

1151+
scheme := runtime.NewScheme()
1152+
g.Expect(clusterv1.AddToScheme(scheme)).To(Succeed())
1153+
g.Expect(corev1.AddToScheme(scheme)).To(Succeed())
1154+
11511155
cat := runtimecatalog.New()
11521156
_ = fakev1alpha1.AddToCatalog(cat)
11531157
_ = fakev1alpha2.AddToCatalog(cat)
11541158
fakeClient := fake.NewClientBuilder().
1159+
WithScheme(scheme).
11551160
WithObjects(ns, nsDifferent).
11561161
Build()
11571162
c := New(Options{
@@ -1346,10 +1351,15 @@ func TestClient_CallAllExtensions(t *testing.T) {
13461351
}
13471352
}
13481353

1354+
scheme := runtime.NewScheme()
1355+
g.Expect(clusterv1.AddToScheme(scheme)).To(Succeed())
1356+
g.Expect(corev1.AddToScheme(scheme)).To(Succeed())
1357+
13491358
cat := runtimecatalog.New()
13501359
_ = fakev1alpha1.AddToCatalog(cat)
13511360
_ = fakev1alpha2.AddToCatalog(cat)
13521361
fakeClient := fake.NewClientBuilder().
1362+
WithScheme(scheme).
13531363
WithObjects(ns).
13541364
Build()
13551365
c := New(Options{

‎internal/runtime/client/fake/fake_client.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ import (
2222
"fmt"
2323

2424
"github.com/pkg/errors"
25-
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
25+
"sigs.k8s.io/controller-runtime/pkg/client"
2626

2727
runtimehooksv1 "sigs.k8s.io/cluster-api/api/runtime/hooks/v1alpha1"
2828
runtimev1 "sigs.k8s.io/cluster-api/api/runtime/v1beta2"
@@ -118,7 +118,7 @@ type RuntimeClient struct {
118118
}
119119

120120
// GetAllExtensions implements Client.
121-
func (fc *RuntimeClient) GetAllExtensions(_ context.Context, hook runtimecatalog.Hook, _ metav1.Object) ([]string, error) {
121+
func (fc *RuntimeClient) GetAllExtensions(_ context.Context, hook runtimecatalog.Hook, _ client.Object) ([]string, error) {
122122
gvh, err := fc.catalog.GroupVersionHook(hook)
123123
if err != nil {
124124
return nil, errors.Wrap(err, "failed to compute GVH")
@@ -127,7 +127,7 @@ func (fc *RuntimeClient) GetAllExtensions(_ context.Context, hook runtimecatalog
127127
}
128128

129129
// CallAllExtensions implements Client.
130-
func (fc *RuntimeClient) CallAllExtensions(ctx context.Context, hook runtimecatalog.Hook, _ metav1.Object, req runtimehooksv1.RequestObject, response runtimehooksv1.ResponseObject) error {
130+
func (fc *RuntimeClient) CallAllExtensions(ctx context.Context, hook runtimecatalog.Hook, _ client.Object, req runtimehooksv1.RequestObject, response runtimehooksv1.ResponseObject) error {
131131
defer func() {
132132
fc.callAllTracker[runtimecatalog.HookName(hook)]++
133133
}()
@@ -161,7 +161,7 @@ func (fc *RuntimeClient) CallAllExtensions(ctx context.Context, hook runtimecata
161161
}
162162

163163
// CallExtension implements Client.
164-
func (fc *RuntimeClient) CallExtension(ctx context.Context, _ runtimecatalog.Hook, _ metav1.Object, name string, req runtimehooksv1.RequestObject, response runtimehooksv1.ResponseObject, _ ...runtimeclient.CallExtensionOption) error {
164+
func (fc *RuntimeClient) CallExtension(ctx context.Context, _ runtimecatalog.Hook, _ client.Object, name string, req runtimehooksv1.RequestObject, response runtimehooksv1.ResponseObject, _ ...runtimeclient.CallExtensionOption) error {
165165
if fc.callValidations != nil {
166166
if err := fc.callValidations(name, req); err != nil {
167167
return err

‎internal/util/ssa/patch.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -130,7 +130,8 @@ func Patch(ctx context.Context, c client.Client, fieldManager string, modified c
130130
// Recover gvk e.g. for logging.
131131
modified.GetObjectKind().SetGroupVersionKind(gvk)
132132

133-
if options.WithCachingProxy {
133+
// Add the request to the cache only if dry-run was not used.
134+
if options.WithCachingProxy && !options.WithDryRun {
134135
// If the SSA call did not update the object, add the request to the cache.
135136
if options.Original.GetResourceVersion() == modifiedUnstructured.GetResourceVersion() {
136137
options.Cache.Add(requestIdentifier)

0 commit comments

Comments
 (0)