From 09a2e89f283dd5a82a8325f17bd5adb834a22e25 Mon Sep 17 00:00:00 2001 From: Stefan Bueringer Date: Fri, 5 Sep 2025 10:02:21 +0200 Subject: [PATCH 01/12] Revert deprecation of client.Apply MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Stefan Büringer buringerst@vmware.com --- pkg/client/fake/client_test.go | 18 +++++++++--------- pkg/client/patch.go | 5 ++++- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/pkg/client/fake/client_test.go b/pkg/client/fake/client_test.go index beb8d38433..72c20fd56f 100644 --- a/pkg/client/fake/client_test.go +++ b/pkg/client/fake/client_test.go @@ -2618,7 +2618,7 @@ var _ = Describe("Fake client", func() { obj.SetName("foo") Expect(unstructured.SetNestedField(obj.Object, map[string]any{"some": "data"}, "data")).To(Succeed()) - Expect(cl.Patch(ctx, obj, client.Apply, client.FieldOwner("foo"))).To(Succeed()) //nolint:staticcheck // will be removed once client.Apply is removed + Expect(cl.Patch(ctx, obj, client.Apply, client.FieldOwner("foo"))).To(Succeed()) cm := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "foo"}} @@ -2626,7 +2626,7 @@ var _ = Describe("Fake client", func() { Expect(cm.Data).To(Equal(map[string]string{"some": "data"})) Expect(unstructured.SetNestedField(obj.Object, map[string]any{"other": "data"}, "data")).To(Succeed()) - Expect(cl.Patch(ctx, obj, client.Apply, client.FieldOwner("foo"))).To(Succeed()) //nolint:staticcheck // will be removed once client.Apply is removed + Expect(cl.Patch(ctx, obj, client.Apply, client.FieldOwner("foo"))).To(Succeed()) Expect(cl.Get(ctx, client.ObjectKeyFromObject(cm), cm)).To(Succeed()) Expect(cm.Data).To(Equal(map[string]string{"other": "data"})) @@ -2642,13 +2642,13 @@ var _ = Describe("Fake client", func() { Expect(unstructured.SetNestedField(obj.Object, map[string]any{"some": "data"}, "spec")).To(Succeed()) - Expect(cl.Patch(ctx, obj, client.Apply, client.FieldOwner("foo"))).To(Succeed()) //nolint:staticcheck // will be removed once client.Apply is removed + Expect(cl.Patch(ctx, obj, client.Apply, client.FieldOwner("foo"))).To(Succeed()) Expect(cl.Get(ctx, client.ObjectKeyFromObject(result), result)).To(Succeed()) Expect(result.Object["spec"]).To(Equal(map[string]any{"some": "data"})) Expect(unstructured.SetNestedField(obj.Object, map[string]any{"other": "data"}, "spec")).To(Succeed()) - Expect(cl.Patch(ctx, obj, client.Apply, client.FieldOwner("foo"))).To(Succeed()) //nolint:staticcheck // will be removed once client.Apply is removed + Expect(cl.Patch(ctx, obj, client.Apply, client.FieldOwner("foo"))).To(Succeed()) Expect(cl.Get(ctx, client.ObjectKeyFromObject(result), result)).To(Succeed()) Expect(result.Object["spec"]).To(Equal(map[string]any{"other": "data"})) @@ -2662,9 +2662,9 @@ var _ = Describe("Fake client", func() { obj.SetName("foo") Expect(unstructured.SetNestedField(obj.Object, map[string]any{"some": "data"}, "data")).To(Succeed()) - Expect(cl.Patch(ctx, obj, client.Apply, client.FieldOwner("foo"))).To(Succeed()) //nolint:staticcheck // will be removed once client.Apply is removed + Expect(cl.Patch(ctx, obj, client.Apply, client.FieldOwner("foo"))).To(Succeed()) - err := cl.Patch(ctx, obj, client.Apply, client.FieldOwner("foo")) //nolint:staticcheck // will be removed once client.Apply is removed + err := cl.Patch(ctx, obj, client.Apply, client.FieldOwner("foo")) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("metadata.managedFields must be nil")) }) @@ -2680,7 +2680,7 @@ var _ = Describe("Fake client", func() { Expect(unstructured.SetNestedField(obj.Object, map[string]any{"some": "data"}, "data")).To(Succeed()) - Expect(cl.Patch(ctx, obj, client.Apply, client.FieldOwner("foo"))).To(Succeed()) //nolint:staticcheck // will be removed once client.Apply is removed + Expect(cl.Patch(ctx, obj, client.Apply, client.FieldOwner("foo"))).To(Succeed()) cm := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "foo"}} @@ -2688,7 +2688,7 @@ var _ = Describe("Fake client", func() { Expect(cm.Data).To(Equal(map[string]string{"some": "data"})) Expect(unstructured.SetNestedField(obj.Object, map[string]any{"other": "data"}, "data")).To(Succeed()) - Expect(cl.Patch(ctx, obj, client.Apply, client.FieldOwner("foo"))).To(Succeed()) //nolint:staticcheck // will be removed once client.Apply is removed + Expect(cl.Patch(ctx, obj, client.Apply, client.FieldOwner("foo"))).To(Succeed()) Expect(cl.Get(ctx, client.ObjectKeyFromObject(cm), cm)).To(Succeed()) Expect(cm.Data).To(Equal(map[string]string{"other": "data"})) @@ -2734,7 +2734,7 @@ var _ = Describe("Fake client", func() { "ssa": "value", }, }} - Expect(cl.Patch(ctx, u, client.Apply, client.FieldOwner("foo"))).NotTo(HaveOccurred()) //nolint:staticcheck // will be removed once client.Apply is removed + Expect(cl.Patch(ctx, u, client.Apply, client.FieldOwner("foo"))).NotTo(HaveOccurred()) _, exists, err := unstructured.NestedFieldNoCopy(u.Object, "metadata", "managedFields") Expect(err).NotTo(HaveOccurred()) Expect(exists).To(BeTrue()) diff --git a/pkg/client/patch.go b/pkg/client/patch.go index ec55861080..b99d7663bd 100644 --- a/pkg/client/patch.go +++ b/pkg/client/patch.go @@ -28,7 +28,10 @@ import ( var ( // Apply uses server-side apply to patch the given object. // - // Deprecated: Use client.Client.Apply() instead. + // This should now only be used to patch sub resources, e.g. with client.Client.Status().Patch(). + // Use client.Client.Apply() instead of client.Client.Patch(..., client.Apply, ...) + // This will be deprecated once the Apply method has been added for sub resources. + // See the following issue for more details: https://github.com/kubernetes-sigs/controller-runtime/issues/3183 Apply Patch = applyPatch{} // Merge uses the raw object as a merge patch, without modifications. From 04c6a08c9b98a0abdfc99f09aa5de516c27959f1 Mon Sep 17 00:00:00 2001 From: k8s-infra-cherrypick-robot <90416843+k8s-infra-cherrypick-robot@users.noreply.github.com> Date: Tue, 9 Sep 2025 17:25:58 -0700 Subject: [PATCH 02/12] =?UTF-8?q?[release-0.22]=20=F0=9F=90=9BPanic=20when?= =?UTF-8?q?=20trying=20to=20build=20more=20than=20one=20instance=20of=20fa?= =?UTF-8?q?ke.ClientBuilder=20(#3315)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * panic when trying to build more than one instance of fake.ClientBuilder Signed-off-by: Troy Connor * pr feedback Signed-off-by: Troy Connor --------- Signed-off-by: Troy Connor Co-authored-by: Troy Connor --- pkg/client/fake/client.go | 5 +++++ pkg/client/fake/client_test.go | 9 +++++++++ 2 files changed, 14 insertions(+) diff --git a/pkg/client/fake/client.go b/pkg/client/fake/client.go index 45f9e00e18..f88a44edd2 100644 --- a/pkg/client/fake/client.go +++ b/pkg/client/fake/client.go @@ -141,6 +141,7 @@ type ClientBuilder struct { interceptorFuncs *interceptor.Funcs typeConverters []managedfields.TypeConverter returnManagedFields bool + isBuilt bool // indexes maps each GroupVersionKind (GVK) to the indexes registered for that GVK. // The inner map maps from index name to IndexerFunc. @@ -267,6 +268,9 @@ func (f *ClientBuilder) WithReturnManagedFields() *ClientBuilder { // Build builds and returns a new fake client. func (f *ClientBuilder) Build() client.WithWatch { + if f.isBuilt { + panic("Build() must not be called multiple times when creating a ClientBuilder") + } if f.scheme == nil { f.scheme = scheme.Scheme } @@ -344,6 +348,7 @@ func (f *ClientBuilder) Build() client.WithWatch { result = interceptor.NewClient(result, *f.interceptorFuncs) } + f.isBuilt = true return result } diff --git a/pkg/client/fake/client_test.go b/pkg/client/fake/client_test.go index 72c20fd56f..21c5f21dd7 100644 --- a/pkg/client/fake/client_test.go +++ b/pkg/client/fake/client_test.go @@ -3159,4 +3159,13 @@ var _ = Describe("Fake client builder", func() { Expect(err).NotTo(HaveOccurred()) Expect(called).To(BeTrue()) }) + + It("should panic when calling build more than once", func() { + cb := NewClientBuilder() + anotherCb := cb + cb.Build() + Expect(func() { + anotherCb.Build() + }).To(Panic()) + }) }) From f3b9e4f96392b66ba2067f53e1d2ab77a0410c82 Mon Sep 17 00:00:00 2001 From: dongjiang Date: Thu, 11 Sep 2025 16:15:35 +0800 Subject: [PATCH 03/12] Bump to k8s.io/* v0.34.1 Signed-off-by: dongjiang --- examples/scratch-env/go.mod | 8 ++++---- examples/scratch-env/go.sum | 16 ++++++++-------- go.mod | 12 ++++++------ go.sum | 24 ++++++++++++------------ tools/setup-envtest/go.mod | 2 +- tools/setup-envtest/go.sum | 4 ++-- 6 files changed, 33 insertions(+), 33 deletions(-) diff --git a/examples/scratch-env/go.mod b/examples/scratch-env/go.mod index a92a25b7d8..546c7c39ee 100644 --- a/examples/scratch-env/go.mod +++ b/examples/scratch-env/go.mod @@ -54,10 +54,10 @@ require ( gopkg.in/evanphx/json-patch.v4 v4.12.0 // indirect gopkg.in/inf.v0 v0.9.1 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect - k8s.io/api v0.34.0 // indirect - k8s.io/apiextensions-apiserver v0.34.0 // indirect - k8s.io/apimachinery v0.34.0 // indirect - k8s.io/client-go v0.34.0 // indirect + k8s.io/api v0.34.1 // indirect + k8s.io/apiextensions-apiserver v0.34.1 // indirect + k8s.io/apimachinery v0.34.1 // indirect + k8s.io/client-go v0.34.1 // indirect k8s.io/klog/v2 v2.130.1 // indirect k8s.io/kube-openapi v0.0.0-20250710124328-f3f2b991d03b // indirect k8s.io/utils v0.0.0-20250604170112-4c0f3b243397 // indirect diff --git a/examples/scratch-env/go.sum b/examples/scratch-env/go.sum index 703b352e28..012b88f447 100644 --- a/examples/scratch-env/go.sum +++ b/examples/scratch-env/go.sum @@ -173,14 +173,14 @@ gopkg.in/inf.v0 v0.9.1/go.mod h1:cWUDdTG/fYaXco+Dcufb5Vnc6Gp2YChqWtbxRZE0mXw= gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= -k8s.io/api v0.34.0 h1:L+JtP2wDbEYPUeNGbeSa/5GwFtIA662EmT2YSLOkAVE= -k8s.io/api v0.34.0/go.mod h1:YzgkIzOOlhl9uwWCZNqpw6RJy9L2FK4dlJeayUoydug= -k8s.io/apiextensions-apiserver v0.34.0 h1:B3hiB32jV7BcyKcMU5fDaDxk882YrJ1KU+ZSkA9Qxoc= -k8s.io/apiextensions-apiserver v0.34.0/go.mod h1:hLI4GxE1BDBy9adJKxUxCEHBGZtGfIg98Q+JmTD7+g0= -k8s.io/apimachinery v0.34.0 h1:eR1WO5fo0HyoQZt1wdISpFDffnWOvFLOOeJ7MgIv4z0= -k8s.io/apimachinery v0.34.0/go.mod h1:/GwIlEcWuTX9zKIg2mbw0LRFIsXwrfoVxn+ef0X13lw= -k8s.io/client-go v0.34.0 h1:YoWv5r7bsBfb0Hs2jh8SOvFbKzzxyNo0nSb0zC19KZo= -k8s.io/client-go v0.34.0/go.mod h1:ozgMnEKXkRjeMvBZdV1AijMHLTh3pbACPvK7zFR+QQY= +k8s.io/api v0.34.1 h1:jC+153630BMdlFukegoEL8E/yT7aLyQkIVuwhmwDgJM= +k8s.io/api v0.34.1/go.mod h1:SB80FxFtXn5/gwzCoN6QCtPD7Vbu5w2n1S0J5gFfTYk= +k8s.io/apiextensions-apiserver v0.34.1 h1:NNPBva8FNAPt1iSVwIE0FsdrVriRXMsaWFMqJbII2CI= +k8s.io/apiextensions-apiserver v0.34.1/go.mod h1:hP9Rld3zF5Ay2Of3BeEpLAToP+l4s5UlxiHfqRaRcMc= +k8s.io/apimachinery v0.34.1 h1:dTlxFls/eikpJxmAC7MVE8oOeP1zryV7iRyIjB0gky4= +k8s.io/apimachinery v0.34.1/go.mod h1:/GwIlEcWuTX9zKIg2mbw0LRFIsXwrfoVxn+ef0X13lw= +k8s.io/client-go v0.34.1 h1:ZUPJKgXsnKwVwmKKdPfw4tB58+7/Ik3CrjOEhsiZ7mY= +k8s.io/client-go v0.34.1/go.mod h1:kA8v0FP+tk6sZA0yKLRG67LWjqufAoSHA2xVGKw9Of8= k8s.io/klog/v2 v2.130.1 h1:n9Xl7H1Xvksem4KFG4PYbdQCQxqc/tTUyrgXaOhHSzk= k8s.io/klog/v2 v2.130.1/go.mod h1:3Jpz1GvMt720eyJH1ckRHK1EDfpxISzJ7I9OYgaDtPE= k8s.io/kube-openapi v0.0.0-20250710124328-f3f2b991d03b h1:MloQ9/bdJyIu9lb1PzujOPolHyvO06MXG5TUIj2mNAA= diff --git a/go.mod b/go.mod index 36bce9c9e5..4d998fe2fc 100644 --- a/go.mod +++ b/go.mod @@ -21,11 +21,11 @@ require ( golang.org/x/sys v0.31.0 gomodules.xyz/jsonpatch/v2 v2.4.0 gopkg.in/evanphx/json-patch.v4 v4.12.0 // Using v4 to match upstream - k8s.io/api v0.34.0 - k8s.io/apiextensions-apiserver v0.34.0 - k8s.io/apimachinery v0.34.0 - k8s.io/apiserver v0.34.0 - k8s.io/client-go v0.34.0 + k8s.io/api v0.34.1 + k8s.io/apiextensions-apiserver v0.34.1 + k8s.io/apimachinery v0.34.1 + k8s.io/apiserver v0.34.1 + k8s.io/client-go v0.34.1 k8s.io/klog/v2 v2.130.1 k8s.io/utils v0.0.0-20250604170112-4c0f3b243397 sigs.k8s.io/structured-merge-diff/v6 v6.3.0 @@ -95,7 +95,7 @@ require ( google.golang.org/protobuf v1.36.5 // indirect gopkg.in/inf.v0 v0.9.1 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect - k8s.io/component-base v0.34.0 // indirect + k8s.io/component-base v0.34.1 // indirect k8s.io/kube-openapi v0.0.0-20250710124328-f3f2b991d03b // indirect sigs.k8s.io/apiserver-network-proxy/konnectivity-client v0.31.2 // indirect sigs.k8s.io/json v0.0.0-20241014173422-cfa47c3a1cc8 // indirect diff --git a/go.sum b/go.sum index 102a137d04..d6278d8a7d 100644 --- a/go.sum +++ b/go.sum @@ -229,18 +229,18 @@ gopkg.in/inf.v0 v0.9.1/go.mod h1:cWUDdTG/fYaXco+Dcufb5Vnc6Gp2YChqWtbxRZE0mXw= gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= -k8s.io/api v0.34.0 h1:L+JtP2wDbEYPUeNGbeSa/5GwFtIA662EmT2YSLOkAVE= -k8s.io/api v0.34.0/go.mod h1:YzgkIzOOlhl9uwWCZNqpw6RJy9L2FK4dlJeayUoydug= -k8s.io/apiextensions-apiserver v0.34.0 h1:B3hiB32jV7BcyKcMU5fDaDxk882YrJ1KU+ZSkA9Qxoc= -k8s.io/apiextensions-apiserver v0.34.0/go.mod h1:hLI4GxE1BDBy9adJKxUxCEHBGZtGfIg98Q+JmTD7+g0= -k8s.io/apimachinery v0.34.0 h1:eR1WO5fo0HyoQZt1wdISpFDffnWOvFLOOeJ7MgIv4z0= -k8s.io/apimachinery v0.34.0/go.mod h1:/GwIlEcWuTX9zKIg2mbw0LRFIsXwrfoVxn+ef0X13lw= -k8s.io/apiserver v0.34.0 h1:Z51fw1iGMqN7uJ1kEaynf2Aec1Y774PqU+FVWCFV3Jg= -k8s.io/apiserver v0.34.0/go.mod h1:52ti5YhxAvewmmpVRqlASvaqxt0gKJxvCeW7ZrwgazQ= -k8s.io/client-go v0.34.0 h1:YoWv5r7bsBfb0Hs2jh8SOvFbKzzxyNo0nSb0zC19KZo= -k8s.io/client-go v0.34.0/go.mod h1:ozgMnEKXkRjeMvBZdV1AijMHLTh3pbACPvK7zFR+QQY= -k8s.io/component-base v0.34.0 h1:bS8Ua3zlJzapklsB1dZgjEJuJEeHjj8yTu1gxE2zQX8= -k8s.io/component-base v0.34.0/go.mod h1:RSCqUdvIjjrEm81epPcjQ/DS+49fADvGSCkIP3IC6vg= +k8s.io/api v0.34.1 h1:jC+153630BMdlFukegoEL8E/yT7aLyQkIVuwhmwDgJM= +k8s.io/api v0.34.1/go.mod h1:SB80FxFtXn5/gwzCoN6QCtPD7Vbu5w2n1S0J5gFfTYk= +k8s.io/apiextensions-apiserver v0.34.1 h1:NNPBva8FNAPt1iSVwIE0FsdrVriRXMsaWFMqJbII2CI= +k8s.io/apiextensions-apiserver v0.34.1/go.mod h1:hP9Rld3zF5Ay2Of3BeEpLAToP+l4s5UlxiHfqRaRcMc= +k8s.io/apimachinery v0.34.1 h1:dTlxFls/eikpJxmAC7MVE8oOeP1zryV7iRyIjB0gky4= +k8s.io/apimachinery v0.34.1/go.mod h1:/GwIlEcWuTX9zKIg2mbw0LRFIsXwrfoVxn+ef0X13lw= +k8s.io/apiserver v0.34.1 h1:U3JBGdgANK3dfFcyknWde1G6X1F4bg7PXuvlqt8lITA= +k8s.io/apiserver v0.34.1/go.mod h1:eOOc9nrVqlBI1AFCvVzsob0OxtPZUCPiUJL45JOTBG0= +k8s.io/client-go v0.34.1 h1:ZUPJKgXsnKwVwmKKdPfw4tB58+7/Ik3CrjOEhsiZ7mY= +k8s.io/client-go v0.34.1/go.mod h1:kA8v0FP+tk6sZA0yKLRG67LWjqufAoSHA2xVGKw9Of8= +k8s.io/component-base v0.34.1 h1:v7xFgG+ONhytZNFpIz5/kecwD+sUhVE6HU7qQUiRM4A= +k8s.io/component-base v0.34.1/go.mod h1:mknCpLlTSKHzAQJJnnHVKqjxR7gBeHRv0rPXA7gdtQ0= k8s.io/klog/v2 v2.130.1 h1:n9Xl7H1Xvksem4KFG4PYbdQCQxqc/tTUyrgXaOhHSzk= k8s.io/klog/v2 v2.130.1/go.mod h1:3Jpz1GvMt720eyJH1ckRHK1EDfpxISzJ7I9OYgaDtPE= k8s.io/kube-openapi v0.0.0-20250710124328-f3f2b991d03b h1:MloQ9/bdJyIu9lb1PzujOPolHyvO06MXG5TUIj2mNAA= diff --git a/tools/setup-envtest/go.mod b/tools/setup-envtest/go.mod index 5cb31d8bf2..15c64f8b57 100644 --- a/tools/setup-envtest/go.mod +++ b/tools/setup-envtest/go.mod @@ -10,7 +10,7 @@ require ( github.com/spf13/afero v1.12.0 github.com/spf13/pflag v1.0.6 go.uber.org/zap v1.27.0 - k8s.io/apimachinery v0.34.0 + k8s.io/apimachinery v0.34.1 sigs.k8s.io/yaml v1.6.0 ) diff --git a/tools/setup-envtest/go.sum b/tools/setup-envtest/go.sum index c9dcc6499b..dfc8e7cce2 100644 --- a/tools/setup-envtest/go.sum +++ b/tools/setup-envtest/go.sum @@ -46,7 +46,7 @@ gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+ gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= -k8s.io/apimachinery v0.34.0 h1:eR1WO5fo0HyoQZt1wdISpFDffnWOvFLOOeJ7MgIv4z0= -k8s.io/apimachinery v0.34.0/go.mod h1:/GwIlEcWuTX9zKIg2mbw0LRFIsXwrfoVxn+ef0X13lw= +k8s.io/apimachinery v0.34.1 h1:dTlxFls/eikpJxmAC7MVE8oOeP1zryV7iRyIjB0gky4= +k8s.io/apimachinery v0.34.1/go.mod h1:/GwIlEcWuTX9zKIg2mbw0LRFIsXwrfoVxn+ef0X13lw= sigs.k8s.io/yaml v1.6.0 h1:G8fkbMSAFqgEFgh4b1wmtzDnioxFCUgTZhlbj5P9QYs= sigs.k8s.io/yaml v1.6.0/go.mod h1:796bPqUfzR/0jLAl6XjHl3Ck7MiyVv8dbTdyT3/pMf4= From d04f428ec56cd59349a7337c39d4e32e4da7a461 Mon Sep 17 00:00:00 2001 From: Stefan Bueringer Date: Sun, 5 Oct 2025 11:40:55 +0200 Subject: [PATCH 04/12] Don't block on Get when queue is shutdown (2nd try) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Stefan Büringer buringerst@vmware.com --- pkg/controller/priorityqueue/priorityqueue.go | 15 ++++++++--- .../priorityqueue/priorityqueue_test.go | 26 +++++++++++++++++++ 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/pkg/controller/priorityqueue/priorityqueue.go b/pkg/controller/priorityqueue/priorityqueue.go index 49942186c0..f702600fc9 100644 --- a/pkg/controller/priorityqueue/priorityqueue.go +++ b/pkg/controller/priorityqueue/priorityqueue.go @@ -290,9 +290,18 @@ func (w *priorityqueue[T]) GetWithPriority() (_ T, priority int, shutdown bool) w.notifyItemOrWaiterAdded() - item := <-w.get - - return item.Key, item.Priority, w.shutdown.Load() + select { + case <-w.done: + // Return if the queue was shutdown while we were already waiting for an item here. + // For example controller workers are continuously calling GetWithPriority and + // GetWithPriority is blocking the workers if there are no items in the queue. + // If the controller and accordingly the queue is then shut down, without this code + // branch the controller workers remain blocked here and are unable to shut down. + var zero T + return zero, 0, true + case item := <-w.get: + return item.Key, item.Priority, w.shutdown.Load() + } } func (w *priorityqueue[T]) Get() (item T, shutdown bool) { diff --git a/pkg/controller/priorityqueue/priorityqueue_test.go b/pkg/controller/priorityqueue/priorityqueue_test.go index 13cf59b7e8..884844efab 100644 --- a/pkg/controller/priorityqueue/priorityqueue_test.go +++ b/pkg/controller/priorityqueue/priorityqueue_test.go @@ -321,6 +321,32 @@ var _ = Describe("Controllerworkqueue", func() { Expect(isShutDown).To(BeTrue()) }) + It("Get from priority queue should get unblocked when the priority queue is shut down", func() { + q, _ := newQueue() + + getUnblocked := make(chan struct{}) + + go func() { + defer GinkgoRecover() + defer close(getUnblocked) + + item, priority, isShutDown := q.GetWithPriority() + Expect(item).To(Equal("")) + Expect(priority).To(Equal(0)) + Expect(isShutDown).To(BeTrue()) + }() + + // Verify the go routine above is now waiting for an item. + Eventually(q.waiters.Load).Should(Equal(int64(1))) + Consistently(getUnblocked).ShouldNot(BeClosed()) + + // shut down + q.ShutDown() + + // Verify the shutdown unblocked the go routine. + Eventually(getUnblocked).Should(BeClosed()) + }) + It("items are included in Len() and the queueDepth metric once they are ready", func() { q, metrics := newQueue() defer q.ShutDown() From 6d368ce0f7e7218c8e1ce8ddceff43354760d535 Mon Sep 17 00:00:00 2001 From: Alvaro Aleman Date: Sun, 5 Oct 2025 13:44:02 -0400 Subject: [PATCH 05/12] Rebase priorityqueue shutdown fix for release-0.22 --- pkg/controller/priorityqueue/priorityqueue_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/controller/priorityqueue/priorityqueue_test.go b/pkg/controller/priorityqueue/priorityqueue_test.go index 884844efab..6127cd99ba 100644 --- a/pkg/controller/priorityqueue/priorityqueue_test.go +++ b/pkg/controller/priorityqueue/priorityqueue_test.go @@ -337,7 +337,7 @@ var _ = Describe("Controllerworkqueue", func() { }() // Verify the go routine above is now waiting for an item. - Eventually(q.waiters.Load).Should(Equal(int64(1))) + Eventually(q.(*priorityqueue[string]).waiters.Load).Should(Equal(int64(1))) Consistently(getUnblocked).ShouldNot(BeClosed()) // shut down From 7fb34b509fcf92c7e775261c4ea1999fbace5463 Mon Sep 17 00:00:00 2001 From: k8s-infra-cherrypick-robot <90416843+k8s-infra-cherrypick-robot@users.noreply.github.com> Date: Mon, 6 Oct 2025 09:02:59 -0700 Subject: [PATCH 06/12] =?UTF-8?q?[release-0.22]=20=F0=9F=90=9B=20Fix=20a?= =?UTF-8?q?=20bug=20where=20the=20priorityqueue=20would=20sometimes=20not?= =?UTF-8?q?=20return=20high-priority=20items=20first=20(#3340)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: adjust priority queue order and spin Co-authored-by: kstiehl * fix: do not hand out item during metrics ascend Co-authored-by: kstiehl * test: add test case Co-authored-by: kstiehl * rm async from test * rm metricsAscend flag * fix test Co-authored-by: kstiehl * add comments Co-authored-by: kstiehl * Update pkg/controller/priorityqueue/priorityqueue.go Co-authored-by: Alvaro Aleman * Rebase for releasebranch without newQueueWithTimeForwarder --------- Co-authored-by: moritzmoe <51033452+moritzmoe@users.noreply.github.com> Co-authored-by: kstiehl Co-authored-by: Alvaro Aleman --- pkg/controller/priorityqueue/priorityqueue.go | 95 ++++++++++++------- .../priorityqueue/priorityqueue_test.go | 41 ++++++++ 2 files changed, 104 insertions(+), 32 deletions(-) diff --git a/pkg/controller/priorityqueue/priorityqueue.go b/pkg/controller/priorityqueue/priorityqueue.go index f702600fc9..98df84c56b 100644 --- a/pkg/controller/priorityqueue/priorityqueue.go +++ b/pkg/controller/priorityqueue/priorityqueue.go @@ -1,6 +1,7 @@ package priorityqueue import ( + "math" "sync" "sync/atomic" "time" @@ -206,6 +207,7 @@ func (w *priorityqueue[T]) spin() { blockForever := make(chan time.Time) var nextReady <-chan time.Time nextReady = blockForever + var nextItemReadyAt time.Time for { select { @@ -213,10 +215,10 @@ func (w *priorityqueue[T]) spin() { return case <-w.itemOrWaiterAdded: case <-nextReady: + nextReady = blockForever + nextItemReadyAt = time.Time{} } - nextReady = blockForever - func() { w.lock.Lock() defer w.lock.Unlock() @@ -227,39 +229,67 @@ func (w *priorityqueue[T]) spin() { // manipulating the tree from within Ascend might lead to panics, so // track what we want to delete and do it after we are done ascending. var toDelete []*item[T] - w.queue.Ascend(func(item *item[T]) bool { - if item.ReadyAt != nil { - if readyAt := item.ReadyAt.Sub(w.now()); readyAt > 0 { - nextReady = w.tick(readyAt) - return false + + var key T + + // Items in the queue tree are sorted first by priority and second by readiness, so + // items with a lower priority might be ready further down in the queue. + // We iterate through the priorities high to low until we find a ready item + pivot := item[T]{ + Key: key, + AddedCounter: 0, + Priority: math.MaxInt, + ReadyAt: nil, + } + + for { + pivotChange := false + + w.queue.AscendGreaterOrEqual(&pivot, func(item *item[T]) bool { + // Item is locked, we can not hand it out + if w.locked.Has(item.Key) { + return true } - if !w.becameReady.Has(item.Key) { - w.metrics.add(item.Key, item.Priority) - w.becameReady.Insert(item.Key) + + if item.ReadyAt != nil { + if readyAt := item.ReadyAt.Sub(w.now()); readyAt > 0 { + if nextItemReadyAt.After(*item.ReadyAt) || nextItemReadyAt.IsZero() { + nextReady = w.tick(readyAt) + nextItemReadyAt = *item.ReadyAt + } + + // Adjusting the pivot item moves the ascend to the next lower priority + pivot.Priority = item.Priority - 1 + pivotChange = true + return false + } + if !w.becameReady.Has(item.Key) { + w.metrics.add(item.Key, item.Priority) + w.becameReady.Insert(item.Key) + } } - } - if w.waiters.Load() == 0 { - // Have to keep iterating here to ensure we update metrics - // for further items that became ready and set nextReady. - return true - } + if w.waiters.Load() == 0 { + // Have to keep iterating here to ensure we update metrics + // for further items that became ready and set nextReady. + return true + } - // Item is locked, we can not hand it out - if w.locked.Has(item.Key) { - return true - } + w.metrics.get(item.Key, item.Priority) + w.locked.Insert(item.Key) + w.waiters.Add(-1) + delete(w.items, item.Key) + toDelete = append(toDelete, item) + w.becameReady.Delete(item.Key) + w.get <- *item - w.metrics.get(item.Key, item.Priority) - w.locked.Insert(item.Key) - w.waiters.Add(-1) - delete(w.items, item.Key) - toDelete = append(toDelete, item) - w.becameReady.Delete(item.Key) - w.get <- *item + return true + }) - return true - }) + if !pivotChange { + break + } + } for _, item := range toDelete { w.queue.Delete(item) @@ -387,6 +417,9 @@ func (w *priorityqueue[T]) logState() { } func less[T comparable](a, b *item[T]) bool { + if a.Priority != b.Priority { + return a.Priority > b.Priority + } if a.ReadyAt == nil && b.ReadyAt != nil { return true } @@ -396,9 +429,6 @@ func less[T comparable](a, b *item[T]) bool { if a.ReadyAt != nil && b.ReadyAt != nil && !a.ReadyAt.Equal(*b.ReadyAt) { return a.ReadyAt.Before(*b.ReadyAt) } - if a.Priority != b.Priority { - return a.Priority > b.Priority - } return a.AddedCounter < b.AddedCounter } @@ -426,4 +456,5 @@ type bTree[T any] interface { ReplaceOrInsert(item T) (_ T, _ bool) Delete(item T) (T, bool) Ascend(iterator btree.ItemIteratorG[T]) + AscendGreaterOrEqual(pivot T, iterator btree.ItemIteratorG[T]) } diff --git a/pkg/controller/priorityqueue/priorityqueue_test.go b/pkg/controller/priorityqueue/priorityqueue_test.go index 6127cd99ba..d0cc51f7c5 100644 --- a/pkg/controller/priorityqueue/priorityqueue_test.go +++ b/pkg/controller/priorityqueue/priorityqueue_test.go @@ -197,6 +197,47 @@ var _ = Describe("Controllerworkqueue", func() { Expect(metrics.retries["test"]).To(Equal(1)) }) + It("returns high priority item that became ready before low priority item", func() { + q, metrics := newQueue() + defer q.ShutDown() + + now := time.Now().Round(time.Second) + nowLock := sync.Mutex{} + tick := make(chan time.Time) + + cwq := q.(*priorityqueue[string]) + cwq.now = func() time.Time { + nowLock.Lock() + defer nowLock.Unlock() + return now + } + tickSetup := make(chan any) + cwq.tick = func(d time.Duration) <-chan time.Time { + Expect(d).To(Equal(time.Second)) + close(tickSetup) + return tick + } + + lowPriority := -100 + highPriority := 0 + q.AddWithOpts(AddOpts{After: 0, Priority: &lowPriority}, "foo") + q.AddWithOpts(AddOpts{After: time.Second, Priority: &highPriority}, "prio") + + Eventually(tickSetup).Should(BeClosed()) + + nowLock.Lock() + now = now.Add(time.Second) + nowLock.Unlock() + tick <- now + key, prio, _ := q.GetWithPriority() + + Expect(key).To(Equal("prio")) + Expect(prio).To(Equal(0)) + Expect(metrics.depth["test"]).To(Equal(map[int]int{-100: 1, 0: 0})) + Expect(metrics.adds["test"]).To(Equal(2)) + Expect(metrics.retries["test"]).To(Equal(1)) + }) + It("returns an item to a waiter as soon as it has one", func() { q, metrics := newQueue() defer q.ShutDown() From 3e8b2594ffc4811fc59888a3edab739e3e222e25 Mon Sep 17 00:00:00 2001 From: k8s-infra-cherrypick-robot <90416843+k8s-infra-cherrypick-robot@users.noreply.github.com> Date: Fri, 10 Oct 2025 14:24:59 -0700 Subject: [PATCH 07/12] =?UTF-8?q?[release-0.22]=20=F0=9F=90=9B=20Allow=20S?= =?UTF-8?q?SA=20after=20normal=20resource=20creation=20(#3348)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: enable apply after normal create * improvements: cache now considers disable protobuf flag for lookup * feedback improvements --------- Co-authored-by: filipcirtog --- pkg/client/client.go | 3 +- pkg/client/client_rest_resources.go | 26 +++++++++------- pkg/client/client_test.go | 46 +++++++++++++++++++++++++++++ 3 files changed, 62 insertions(+), 13 deletions(-) diff --git a/pkg/client/client.go b/pkg/client/client.go index 092deb43d4..e9f731453b 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -151,8 +151,7 @@ func newClient(config *rest.Config, options Options) (*client, error) { mapper: options.Mapper, codecs: serializer.NewCodecFactory(options.Scheme), - structuredResourceByType: make(map[schema.GroupVersionKind]*resourceMeta), - unstructuredResourceByType: make(map[schema.GroupVersionKind]*resourceMeta), + resourceByType: make(map[cacheKey]*resourceMeta), } rawMetaClient, err := metadata.NewForConfigAndClient(metadata.ConfigFor(config), options.HTTPClient) diff --git a/pkg/client/client_rest_resources.go b/pkg/client/client_rest_resources.go index acff7a46a4..d75d685cbb 100644 --- a/pkg/client/client_rest_resources.go +++ b/pkg/client/client_rest_resources.go @@ -48,11 +48,15 @@ type clientRestResources struct { // codecs are used to create a REST client for a gvk codecs serializer.CodecFactory - // structuredResourceByType stores structured type metadata - structuredResourceByType map[schema.GroupVersionKind]*resourceMeta - // unstructuredResourceByType stores unstructured type metadata - unstructuredResourceByType map[schema.GroupVersionKind]*resourceMeta - mu sync.RWMutex + // resourceByType stores type metadata + resourceByType map[cacheKey]*resourceMeta + + mu sync.RWMutex +} + +type cacheKey struct { + gvk schema.GroupVersionKind + forceDisableProtoBuf bool } // newResource maps obj to a Kubernetes Resource and constructs a client for that Resource. @@ -117,11 +121,11 @@ func (c *clientRestResources) getResource(obj any) (*resourceMeta, error) { // It's better to do creation work twice than to not let multiple // people make requests at once c.mu.RLock() - resourceByType := c.structuredResourceByType - if isUnstructured { - resourceByType = c.unstructuredResourceByType - } - r, known := resourceByType[gvk] + + cacheKey := cacheKey{gvk: gvk, forceDisableProtoBuf: forceDisableProtoBuf} + + r, known := c.resourceByType[cacheKey] + c.mu.RUnlock() if known { @@ -140,7 +144,7 @@ func (c *clientRestResources) getResource(obj any) (*resourceMeta, error) { if err != nil { return nil, err } - resourceByType[gvk] = r + c.resourceByType[cacheKey] = r return r, err } diff --git a/pkg/client/client_test.go b/pkg/client/client_test.go index c775f28718..63d64ce838 100644 --- a/pkg/client/client_test.go +++ b/pkg/client/client_test.go @@ -951,6 +951,52 @@ U5wwSivyi7vmegHKmblOzNVKA5qPO8zWzqBC Expect(cm.Data).To(BeComparableTo(data)) Expect(cm.Data).To(BeComparableTo(obj.Data)) }) + + It("should create a secret without SSA and later create update a secret using SSA", func(ctx SpecContext) { + cl, err := client.New(cfg, client.Options{}) + Expect(err).NotTo(HaveOccurred()) + Expect(cl).NotTo(BeNil()) + data := map[string][]byte{ + "some-key": []byte("some-value"), + } + secretObject := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "secret-one", + Namespace: "default", + }, + Data: data, + } + + secretApplyConfiguration := corev1applyconfigurations. + Secret("secret-two", "default"). + WithData(data) + + err = cl.Create(ctx, secretObject) + Expect(err).NotTo(HaveOccurred()) + + err = cl.Apply(ctx, secretApplyConfiguration, &client.ApplyOptions{FieldManager: "test-manager"}) + Expect(err).NotTo(HaveOccurred()) + + secret, err := clientset.CoreV1().Secrets(ptr.Deref(secretApplyConfiguration.GetNamespace(), "")).Get(ctx, ptr.Deref(secretApplyConfiguration.GetName(), ""), metav1.GetOptions{}) + Expect(err).NotTo(HaveOccurred()) + + Expect(secret.Data).To(BeComparableTo(data)) + Expect(secret.Data).To(BeComparableTo(secretApplyConfiguration.Data)) + + data = map[string][]byte{ + "some-key": []byte("some-new-value"), + } + secretApplyConfiguration.Data = data + + err = cl.Apply(ctx, secretApplyConfiguration, &client.ApplyOptions{FieldManager: "test-manager"}) + Expect(err).NotTo(HaveOccurred()) + + secret, err = clientset.CoreV1().Secrets(ptr.Deref(secretApplyConfiguration.GetNamespace(), "")).Get(ctx, ptr.Deref(secretApplyConfiguration.GetName(), ""), metav1.GetOptions{}) + Expect(err).NotTo(HaveOccurred()) + + Expect(secret.Data).To(BeComparableTo(data)) + Expect(secret.Data).To(BeComparableTo(secretApplyConfiguration.Data)) + }) }) }) From f5a978108b8c67a09193b1a09d1a471260ecb16d Mon Sep 17 00:00:00 2001 From: Troy Connor Date: Mon, 13 Oct 2025 12:05:27 -0400 Subject: [PATCH 08/12] update List in namespaced client Signed-off-by: Troy Connor --- pkg/client/namespaced_client.go | 7 ++++++- pkg/client/namespaced_client_test.go | 9 +++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/pkg/client/namespaced_client.go b/pkg/client/namespaced_client.go index cacba4a9c6..d4223eda26 100644 --- a/pkg/client/namespaced_client.go +++ b/pkg/client/namespaced_client.go @@ -213,7 +213,12 @@ func (n *namespacedClient) Get(ctx context.Context, key ObjectKey, obj Object, o // List implements client.Client. func (n *namespacedClient) List(ctx context.Context, obj ObjectList, opts ...ListOption) error { - if n.namespace != "" { + isNamespaceScoped, err := n.IsObjectNamespaced(obj) + if err != nil { + return fmt.Errorf("error finding the scope of the object: %w", err) + } + + if isNamespaceScoped && n.namespace != "" { opts = append(opts, InNamespace(n.namespace)) } return n.client.List(ctx, obj, opts...) diff --git a/pkg/client/namespaced_client_test.go b/pkg/client/namespaced_client_test.go index cf28289e72..c8d61a0bd2 100644 --- a/pkg/client/namespaced_client_test.go +++ b/pkg/client/namespaced_client_test.go @@ -53,6 +53,8 @@ var _ = Describe("NamespacedClient", func() { err := rbacv1.AddToScheme(sch) Expect(err).ToNot(HaveOccurred()) + err = corev1.AddToScheme(sch) + Expect(err).ToNot(HaveOccurred()) err = appsv1.AddToScheme(sch) Expect(err).ToNot(HaveOccurred()) @@ -146,6 +148,13 @@ var _ = Describe("NamespacedClient", func() { Expect(result.Items[0]).To(BeEquivalentTo(*dep)) }) + It("should successfully List objects when object is not namespaced scoped", func(ctx SpecContext) { + result := &corev1.NodeList{} + opts := &client.ListOptions{} + Expect(getClient().List(ctx, result, opts)).NotTo(HaveOccurred()) + Expect(result.Items).NotTo(BeEmpty()) + }) + It("should List objects from the namespace specified in the client", func(ctx SpecContext) { result := &appsv1.DeploymentList{} opts := client.InNamespace("non-default") From c7df7c9e5037a8d5b7d2b16c741c9bbe6b886a57 Mon Sep 17 00:00:00 2001 From: Troy Connor Date: Mon, 13 Oct 2025 15:34:16 -0400 Subject: [PATCH 09/12] add namespace for test with namespace_client Signed-off-by: Troy Connor --- pkg/client/namespaced_client_test.go | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/pkg/client/namespaced_client_test.go b/pkg/client/namespaced_client_test.go index c8d61a0bd2..a80d0d46f8 100644 --- a/pkg/client/namespaced_client_test.go +++ b/pkg/client/namespaced_client_test.go @@ -43,6 +43,7 @@ import ( var _ = Describe("NamespacedClient", func() { var dep *appsv1.Deployment + var nameSpace *corev1.Namespace var acDep *appsv1applyconfigurations.DeploymentApplyConfiguration var ns = "default" var count uint64 = 0 @@ -82,6 +83,12 @@ var _ = Describe("NamespacedClient", func() { }, }, } + nameSpace = &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf("namespace-%v", count), + Labels: map[string]string{"name": fmt.Sprintf("namespace-%v", count)}, + }, + } acDep = appsv1applyconfigurations.Deployment(dep.Name, ""). WithLabels(dep.Labels). WithSpec(appsv1applyconfigurations.DeploymentSpec(). @@ -133,10 +140,13 @@ var _ = Describe("NamespacedClient", func() { var err error dep, err = clientset.AppsV1().Deployments(ns).Create(ctx, dep, metav1.CreateOptions{}) Expect(err).NotTo(HaveOccurred()) + nameSpace, err = clientset.CoreV1().Namespaces().Create(ctx, nameSpace, metav1.CreateOptions{}) + Expect(err).NotTo(HaveOccurred()) }) AfterEach(func(ctx SpecContext) { deleteDeployment(ctx, dep, ns) + deleteNamespace(ctx, nameSpace) }) It("should successfully List objects when namespace is not specified with the object", func(ctx SpecContext) { @@ -149,7 +159,7 @@ var _ = Describe("NamespacedClient", func() { }) It("should successfully List objects when object is not namespaced scoped", func(ctx SpecContext) { - result := &corev1.NodeList{} + result := &corev1.NamespaceList{} opts := &client.ListOptions{} Expect(getClient().List(ctx, result, opts)).NotTo(HaveOccurred()) Expect(result.Items).NotTo(BeEmpty()) From b3eff6de5a384ff83cdc8052d1c7fd9d33a40520 Mon Sep 17 00:00:00 2001 From: fossedihelm Date: Tue, 28 Oct 2025 18:45:56 +0100 Subject: [PATCH 10/12] priority queue: properly sync the `waiter` manipulation As described in https://github.com/kubernetes-sigs/controller-runtime/issues/3363, there are some circumstances under which `GetWithPriority` is not returning the correct/expected element. This can happen when a `GetWithPriority` is executed and the `Ascend` of the queue is not completed yet, causing not all the items of the BTree to evaluate the same w.waiters.Load() value. Adding a lock to manipulate the waiters will solve the issue. Since the lock is required, there is no need to use an atomic.Int64 anymore. Signed-off-by: fossedihelm --- pkg/controller/priorityqueue/priorityqueue.go | 12 +++++++----- pkg/controller/priorityqueue/priorityqueue_test.go | 7 ++++++- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/pkg/controller/priorityqueue/priorityqueue.go b/pkg/controller/priorityqueue/priorityqueue.go index 98df84c56b..71363f0d17 100644 --- a/pkg/controller/priorityqueue/priorityqueue.go +++ b/pkg/controller/priorityqueue/priorityqueue.go @@ -124,8 +124,8 @@ type priorityqueue[T comparable] struct { get chan item[T] // waiters is the number of routines blocked in Get, we use it to determine - // if we can push items. - waiters atomic.Int64 + // if we can push items. Every manipulation has to be protected with the lock. + waiters int64 // Configurable for testing now func() time.Time @@ -269,7 +269,7 @@ func (w *priorityqueue[T]) spin() { } } - if w.waiters.Load() == 0 { + if w.waiters == 0 { // Have to keep iterating here to ensure we update metrics // for further items that became ready and set nextReady. return true @@ -277,7 +277,7 @@ func (w *priorityqueue[T]) spin() { w.metrics.get(item.Key, item.Priority) w.locked.Insert(item.Key) - w.waiters.Add(-1) + w.waiters-- delete(w.items, item.Key) toDelete = append(toDelete, item) w.becameReady.Delete(item.Key) @@ -316,7 +316,9 @@ func (w *priorityqueue[T]) GetWithPriority() (_ T, priority int, shutdown bool) return zero, 0, true } - w.waiters.Add(1) + w.lock.Lock() + w.waiters++ + w.lock.Unlock() w.notifyItemOrWaiterAdded() diff --git a/pkg/controller/priorityqueue/priorityqueue_test.go b/pkg/controller/priorityqueue/priorityqueue_test.go index d0cc51f7c5..5cade57e3c 100644 --- a/pkg/controller/priorityqueue/priorityqueue_test.go +++ b/pkg/controller/priorityqueue/priorityqueue_test.go @@ -378,7 +378,12 @@ var _ = Describe("Controllerworkqueue", func() { }() // Verify the go routine above is now waiting for an item. - Eventually(q.(*priorityqueue[string]).waiters.Load).Should(Equal(int64(1))) + Eventually(func() int64 { + q.(*priorityqueue[string]).lock.Lock() + defer q.(*priorityqueue[string]).lock.Unlock() + + return q.(*priorityqueue[string]).waiters + }).Should(Equal(int64(1))) Consistently(getUnblocked).ShouldNot(BeClosed()) // shut down From 3f86a105c53d399d355c35ebc37eacb23f95d007 Mon Sep 17 00:00:00 2001 From: Ming Zhao Date: Thu, 30 Oct 2025 09:36:22 -0700 Subject: [PATCH 11/12] envtest: respect pre-configured binary paths in ControlPlane This commit fixes an issue where Environment.Start() would ignore pre-configured binary paths (APIServer.Path, Etcd.Path, KubectlPath) set in ControlPlane when DownloadBinaryAssets is false. Changes: - Extract path configuration logic into configureBinaryPaths() method for better testability and separation of concerns - Only auto-configure binary paths when they are empty (not pre-set) - When DownloadBinaryAssets is true, downloaded paths are still used (preserving existing behavior) - Update ControlPlane field documentation to clarify path behavior - Add tests in envtest_test.go to verify path handling logic --- pkg/envtest/envtest_test.go | 52 ++++++++++++++++++++++++++++++ pkg/envtest/server.go | 63 ++++++++++++++++++++++++++----------- 2 files changed, 96 insertions(+), 19 deletions(-) diff --git a/pkg/envtest/envtest_test.go b/pkg/envtest/envtest_test.go index ce3e9a4d3f..806c9f43cc 100644 --- a/pkg/envtest/envtest_test.go +++ b/pkg/envtest/envtest_test.go @@ -963,4 +963,56 @@ var _ = Describe("Test", func() { Expect(env.WebhookInstallOptions.LocalServingCertDir).ShouldNot(BeADirectory()) }) }) + + Describe("Binary Path Handling", func() { + It("should respect pre-configured binary paths when not downloading", func() { + // Setup custom paths + customAPIServerPath := "/custom/path/to/kube-apiserver" + customEtcdPath := "/custom/path/to/etcd" + customKubectlPath := "/custom/path/to/kubectl" + + // Create an environment with pre-configured paths + testEnv := &Environment{} + testEnv.ControlPlane.GetAPIServer().Path = customAPIServerPath + testEnv.ControlPlane.Etcd = &Etcd{} + testEnv.ControlPlane.Etcd.Path = customEtcdPath + testEnv.ControlPlane.KubectlPath = customKubectlPath + + // Set BinaryAssetsDirectory to ensure it's not using defaults + testEnv.BinaryAssetsDirectory = "/should/not/be/used" + testEnv.DownloadBinaryAssets = false + + // Call configureBinaryPaths to test the path configuration logic + err := testEnv.configureBinaryPaths() + Expect(err).NotTo(HaveOccurred()) + + // Verify paths were preserved (not overwritten) + apiServer := testEnv.ControlPlane.GetAPIServer() + Expect(apiServer.Path).To(Equal(customAPIServerPath)) + Expect(testEnv.ControlPlane.Etcd.Path).To(Equal(customEtcdPath)) + Expect(testEnv.ControlPlane.KubectlPath).To(Equal(customKubectlPath)) + }) + + It("should auto-configure binary paths when not pre-configured", func() { + // Create an environment without pre-configured paths + testEnv := &Environment{} + testEnv.BinaryAssetsDirectory = "/test/assets" + testEnv.DownloadBinaryAssets = false + + // Call configureBinaryPaths + err := testEnv.configureBinaryPaths() + Expect(err).NotTo(HaveOccurred()) + + // Verify paths were set using BinPathFinder + apiServer := testEnv.ControlPlane.GetAPIServer() + Expect(apiServer.Path).NotTo(BeEmpty()) + Expect(testEnv.ControlPlane.Etcd.Path).NotTo(BeEmpty()) + Expect(testEnv.ControlPlane.KubectlPath).NotTo(BeEmpty()) + + // Verify the paths contain the binary names + Expect(apiServer.Path).To(ContainSubstring("kube-apiserver")) + Expect(testEnv.ControlPlane.Etcd.Path).To(ContainSubstring("etcd")) + Expect(testEnv.ControlPlane.KubectlPath).To(ContainSubstring("kubectl")) + }) + }) }) diff --git a/pkg/envtest/server.go b/pkg/envtest/server.go index 9bb81ed2ab..c9f19da977 100644 --- a/pkg/envtest/server.go +++ b/pkg/envtest/server.go @@ -109,7 +109,11 @@ var ( // Environment creates a Kubernetes test environment that will start / stop the Kubernetes control plane and // install extension APIs. type Environment struct { - // ControlPlane is the ControlPlane including the apiserver and etcd + // ControlPlane is the ControlPlane including the apiserver and etcd. + // Binary paths (APIServer.Path, Etcd.Path, KubectlPath) can be pre-configured in ControlPlane. + // If DownloadBinaryAssets is true, the downloaded paths will always be used. + // If DownloadBinaryAssets is false and paths are not pre-configured (default is empty), they will be + // automatically resolved using BinaryAssetsDirectory. ControlPlane controlplane.ControlPlane // Scheme is used to determine if conversion webhooks should be enabled @@ -211,6 +215,40 @@ func (te *Environment) Stop() error { return te.ControlPlane.Stop() } +// configureBinaryPaths configures the binary paths for the API server, etcd, and kubectl. +// If DownloadBinaryAssets is true, it downloads and uses those paths. +// If DownloadBinaryAssets is false, it only sets paths that are not already configured (empty). +func (te *Environment) configureBinaryPaths() error { + apiServer := te.ControlPlane.GetAPIServer() + + if te.ControlPlane.Etcd == nil { + te.ControlPlane.Etcd = &controlplane.Etcd{} + } + + if te.DownloadBinaryAssets { + apiServerPath, etcdPath, kubectlPath, err := downloadBinaryAssets(context.TODO(), + te.BinaryAssetsDirectory, te.DownloadBinaryAssetsVersion, te.DownloadBinaryAssetsIndexURL) + if err != nil { + return err + } + + apiServer.Path = apiServerPath + te.ControlPlane.Etcd.Path = etcdPath + te.ControlPlane.KubectlPath = kubectlPath + } else { + if apiServer.Path == "" { + apiServer.Path = process.BinPathFinder("kube-apiserver", te.BinaryAssetsDirectory) + } + if te.ControlPlane.Etcd.Path == "" { + te.ControlPlane.Etcd.Path = process.BinPathFinder("etcd", te.BinaryAssetsDirectory) + } + if te.ControlPlane.KubectlPath == "" { + te.ControlPlane.KubectlPath = process.BinPathFinder("kubectl", te.BinaryAssetsDirectory) + } + } + return nil +} + // Start starts a local Kubernetes server and updates te.ApiserverPort with the port it is listening on. func (te *Environment) Start() (*rest.Config, error) { if te.useExistingCluster() { @@ -229,10 +267,6 @@ func (te *Environment) Start() (*rest.Config, error) { } else { apiServer := te.ControlPlane.GetAPIServer() - if te.ControlPlane.Etcd == nil { - te.ControlPlane.Etcd = &controlplane.Etcd{} - } - if os.Getenv(envAttachOutput) == "true" { te.AttachControlPlaneOutput = true } @@ -243,6 +277,9 @@ func (te *Environment) Start() (*rest.Config, error) { if apiServer.Err == nil { apiServer.Err = os.Stderr } + if te.ControlPlane.Etcd == nil { + te.ControlPlane.Etcd = &controlplane.Etcd{} + } if te.ControlPlane.Etcd.Out == nil { te.ControlPlane.Etcd.Out = os.Stdout } @@ -251,20 +288,8 @@ func (te *Environment) Start() (*rest.Config, error) { } } - if te.DownloadBinaryAssets { - apiServerPath, etcdPath, kubectlPath, err := downloadBinaryAssets(context.TODO(), - te.BinaryAssetsDirectory, te.DownloadBinaryAssetsVersion, te.DownloadBinaryAssetsIndexURL) - if err != nil { - return nil, err - } - - apiServer.Path = apiServerPath - te.ControlPlane.Etcd.Path = etcdPath - te.ControlPlane.KubectlPath = kubectlPath - } else { - apiServer.Path = process.BinPathFinder("kube-apiserver", te.BinaryAssetsDirectory) - te.ControlPlane.Etcd.Path = process.BinPathFinder("etcd", te.BinaryAssetsDirectory) - te.ControlPlane.KubectlPath = process.BinPathFinder("kubectl", te.BinaryAssetsDirectory) + if err := te.configureBinaryPaths(); err != nil { + return nil, fmt.Errorf("failed to configure binary paths: %w", err) } if err := te.defaultTimeouts(); err != nil { From 539c94fdb173ee13b80e558c5d4978b72fc33979 Mon Sep 17 00:00:00 2001 From: Stefan Bueringer Date: Sat, 1 Nov 2025 14:15:46 +0100 Subject: [PATCH 12/12] cache: Allow fine-granular configuration of SyncPeriod MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Stefan Büringer buringerst@vmware.com --- pkg/cache/cache.go | 84 +++++++++++++++++++++++++++++++++--- pkg/cache/defaulting_test.go | 24 +++++++++++ 2 files changed, 103 insertions(+), 5 deletions(-) diff --git a/pkg/cache/cache.go b/pkg/cache/cache.go index a7e491855a..a94ec6cc32 100644 --- a/pkg/cache/cache.go +++ b/pkg/cache/cache.go @@ -308,6 +308,42 @@ type ByObject struct { // // Defaults to true. EnableWatchBookmarks *bool + + // SyncPeriod determines the minimum frequency at which watched resources are + // reconciled. A lower period will correct entropy more quickly, but reduce + // responsiveness to change if there are many watched resources. Change this + // value only if you know what you are doing. Defaults to 10 hours if unset. + // there will a 10 percent jitter between the SyncPeriod of all controllers + // so that all controllers will not send list requests simultaneously. + // + // This applies to all controllers. + // + // A period sync happens for two reasons: + // 1. To insure against a bug in the controller that causes an object to not + // be requeued, when it otherwise should be requeued. + // 2. To insure against an unknown bug in controller-runtime, or its dependencies, + // that causes an object to not be requeued, when it otherwise should be + // requeued, or to be removed from the queue, when it otherwise should not + // be removed. + // + // If you want + // 1. to insure against missed watch events, or + // 2. to poll services that cannot be watched, + // then we recommend that, instead of changing the default period, the + // controller requeue, with a constant duration `t`, whenever the controller + // is "done" with an object, and would otherwise not requeue it, i.e., we + // recommend the `Reconcile` function return `reconcile.Result{RequeueAfter: t}`, + // instead of `reconcile.Result{}`. + // + // SyncPeriod will locally trigger an artificial Update event with the same + // object in both ObjectOld and ObjectNew for everything that is in the + // cache. + // + // Predicates or Handlers that expect ObjectOld and ObjectNew to be different + // (such as GenerationChangedPredicate) will filter out this event, preventing + // it from triggering a reconciliation. + // SyncPeriod does not sync between the local cache and the server. + SyncPeriod *time.Duration } // Config describes all potential options for a given watch. @@ -343,6 +379,42 @@ type Config struct { // // Defaults to true. EnableWatchBookmarks *bool + + // SyncPeriod determines the minimum frequency at which watched resources are + // reconciled. A lower period will correct entropy more quickly, but reduce + // responsiveness to change if there are many watched resources. Change this + // value only if you know what you are doing. Defaults to 10 hours if unset. + // there will a 10 percent jitter between the SyncPeriod of all controllers + // so that all controllers will not send list requests simultaneously. + // + // This applies to all controllers. + // + // A period sync happens for two reasons: + // 1. To insure against a bug in the controller that causes an object to not + // be requeued, when it otherwise should be requeued. + // 2. To insure against an unknown bug in controller-runtime, or its dependencies, + // that causes an object to not be requeued, when it otherwise should be + // requeued, or to be removed from the queue, when it otherwise should not + // be removed. + // + // If you want + // 1. to insure against missed watch events, or + // 2. to poll services that cannot be watched, + // then we recommend that, instead of changing the default period, the + // controller requeue, with a constant duration `t`, whenever the controller + // is "done" with an object, and would otherwise not requeue it, i.e., we + // recommend the `Reconcile` function return `reconcile.Result{RequeueAfter: t}`, + // instead of `reconcile.Result{}`. + // + // SyncPeriod will locally trigger an artificial Update event with the same + // object in both ObjectOld and ObjectNew for everything that is in the + // cache. + // + // Predicates or Handlers that expect ObjectOld and ObjectNew to be different + // (such as GenerationChangedPredicate) will filter out this event, preventing + // it from triggering a reconciliation. + // SyncPeriod does not sync between the local cache and the server. + SyncPeriod *time.Duration } // NewCacheFunc - Function for creating a new cache from the options and a rest config. @@ -413,6 +485,7 @@ func optionDefaultsToConfig(opts *Options) Config { Transform: opts.DefaultTransform, UnsafeDisableDeepCopy: opts.DefaultUnsafeDisableDeepCopy, EnableWatchBookmarks: opts.DefaultEnableWatchBookmarks, + SyncPeriod: opts.SyncPeriod, } } @@ -423,6 +496,7 @@ func byObjectToConfig(byObject ByObject) Config { Transform: byObject.Transform, UnsafeDisableDeepCopy: byObject.UnsafeDisableDeepCopy, EnableWatchBookmarks: byObject.EnableWatchBookmarks, + SyncPeriod: byObject.SyncPeriod, } } @@ -436,7 +510,7 @@ func newCache(restConfig *rest.Config, opts Options) newCacheFunc { HTTPClient: opts.HTTPClient, Scheme: opts.Scheme, Mapper: opts.Mapper, - ResyncPeriod: *opts.SyncPeriod, + ResyncPeriod: ptr.Deref(config.SyncPeriod, defaultSyncPeriod), Namespace: namespace, Selector: internal.Selector{ Label: config.LabelSelector, @@ -534,6 +608,7 @@ func defaultOpts(config *rest.Config, opts Options) (Options, error) { byObject.Transform = defaultedConfig.Transform byObject.UnsafeDisableDeepCopy = defaultedConfig.UnsafeDisableDeepCopy byObject.EnableWatchBookmarks = defaultedConfig.EnableWatchBookmarks + byObject.SyncPeriod = defaultedConfig.SyncPeriod } opts.ByObject[obj] = byObject @@ -555,10 +630,6 @@ func defaultOpts(config *rest.Config, opts Options) (Options, error) { opts.DefaultNamespaces[namespace] = cfg } - // Default the resync period to 10 hours if unset - if opts.SyncPeriod == nil { - opts.SyncPeriod = &defaultSyncPeriod - } return opts, nil } @@ -578,6 +649,9 @@ func defaultConfig(toDefault, defaultFrom Config) Config { if toDefault.EnableWatchBookmarks == nil { toDefault.EnableWatchBookmarks = defaultFrom.EnableWatchBookmarks } + if toDefault.SyncPeriod == nil { + toDefault.SyncPeriod = defaultFrom.SyncPeriod + } return toDefault } diff --git a/pkg/cache/defaulting_test.go b/pkg/cache/defaulting_test.go index d9d0dcceb3..89a0334324 100644 --- a/pkg/cache/defaulting_test.go +++ b/pkg/cache/defaulting_test.go @@ -249,6 +249,30 @@ func TestDefaultOpts(t *testing.T) { return cmp.Diff(expected, o.ByObject[pod].EnableWatchBookmarks) }, }, + { + name: "ByObject.SyncPeriod gets defaulted from SyncPeriod", + in: Options{ + ByObject: map[client.Object]ByObject{pod: {}}, + SyncPeriod: ptr.To(5 * time.Minute), + }, + + verification: func(o Options) string { + expected := ptr.To(5 * time.Minute) + return cmp.Diff(expected, o.ByObject[pod].SyncPeriod) + }, + }, + { + name: "ByObject.SyncPeriod doesn't get defaulted when set", + in: Options{ + ByObject: map[client.Object]ByObject{pod: {SyncPeriod: ptr.To(1 * time.Minute)}}, + SyncPeriod: ptr.To(5 * time.Minute), + }, + + verification: func(o Options) string { + expected := ptr.To(1 * time.Minute) + return cmp.Diff(expected, o.ByObject[pod].SyncPeriod) + }, + }, { name: "DefaultNamespace label selector gets defaulted from DefaultLabelSelector", in: Options{