From c7681e244d0beeeeac9868cc3efd1d6cc51be784 Mon Sep 17 00:00:00 2001 From: Calum Murray Date: Tue, 21 Jul 2026 12:30:37 -0400 Subject: [PATCH 1/2] fix: use NewForConfig instead of NewForConfigOrDie in builder The generated newBuilder function previously used NewForConfigOrDie for every typed client, which would panic if any client construction failed. Switch to NewForConfig so errors propagate instead. On failure, getOrCreateClientSet returns the error without caching, so the next reconciliation attempt retries client creation from scratch. The exponential backoff in Task.Run handles transient failures automatically. If retries exhaust, the error surfaces as a sync failure on the ClusterVersion status rather than crashing the process. Signed-off-by: Calum Murray --- hack/generate-lib-resources.py | 24 ++++++--- lib/resourcebuilder/interface.go | 6 +-- lib/resourcebuilder/resourcebuilder.go | 70 +++++++++++++++++++++----- pkg/cvo/internal/operatorstatus.go | 10 ++-- pkg/cvo/sync_test.go | 4 +- 5 files changed, 87 insertions(+), 27 deletions(-) diff --git a/hack/generate-lib-resources.py b/hack/generate-lib-resources.py index f18a1dcc1f..9c1098f7ad 100755 --- a/hack/generate-lib-resources.py +++ b/hack/generate-lib-resources.py @@ -150,19 +150,31 @@ def generate_resourcebuilder(directory, types, clients, modifiers, health_checks lines.extend([ '}', '', - 'func newBuilder(config *rest.Config, m manifest.Manifest) Interface {', - '\treturn &builder{', - '\t\traw: m.Raw,', - '', + 'func newBuilder(config *rest.Config, m manifest.Manifest) (Interface, error) {', ]) for prop_name, data in sorted(client_properties.items()): new_client_arg = 'config' if data.get('protobuf'): new_client_arg = 'withProtobuf({})'.format(new_client_arg) - lines.append('\t\t{:{width}} {}.NewForConfigOrDie({}),'.format(prop_name + ':', data['client_short_name'], new_client_arg, width=longest_property+1)) + var_name = prop_name[0].lower() + prop_name[1:] + lines.extend([ + '\t{}, err := {}.NewForConfig({})'.format(var_name, data['client_short_name'], new_client_arg), + '\tif err != nil {', + '\t\treturn nil, err', + '\t}', + ]) lines.extend([ - '\t}', + '\treturn &builder{', + '\t\traw: m.Raw,', + '', + ]) + for prop_name, data in sorted(client_properties.items()): + var_name = prop_name[0].lower() + prop_name[1:] + lines.append('\t\t{:{width}} {},'.format(prop_name + ':', var_name, width=longest_property+1)) + + lines.extend([ + '\t}, nil', '}', '', 'func (b *builder) WithMode(m Mode) Interface {', diff --git a/lib/resourcebuilder/interface.go b/lib/resourcebuilder/interface.go index 3cea33944e..dba60c25d6 100644 --- a/lib/resourcebuilder/interface.go +++ b/lib/resourcebuilder/interface.go @@ -58,10 +58,10 @@ func NewResourceMapper() *ResourceMapper { type MetaV1ObjectModifierFunc func(metav1.Object) -// NewInterfaceFunc returns an Interface. +// NewInterfaceFunc returns an Interface or an error. // It requires rest Config that can be used to create a client // and the Manifest. -type NewInterfaceFunc func(rest *rest.Config, m manifest.Manifest) Interface +type NewInterfaceFunc func(rest *rest.Config, m manifest.Manifest) (Interface, error) // Mode is how this builder is being used. type Mode int @@ -85,5 +85,5 @@ func New(mapper *ResourceMapper, rest *rest.Config, m manifest.Manifest) (Interf if !ok { return nil, fmt.Errorf("no mapping found for gvk: %v", m.GVK) } - return f(rest, m), nil + return f(rest, m) } diff --git a/lib/resourcebuilder/resourcebuilder.go b/lib/resourcebuilder/resourcebuilder.go index 87e1c52b35..fbb7383fb7 100644 --- a/lib/resourcebuilder/resourcebuilder.go +++ b/lib/resourcebuilder/resourcebuilder.go @@ -56,22 +56,66 @@ type builder struct { securityClientv1 securityclientv1.SecurityV1Interface } -func newBuilder(config *rest.Config, m manifest.Manifest) Interface { +func newBuilder(config *rest.Config, m manifest.Manifest) (Interface, error) { + admissionregistrationClientv1, err := admissionregistrationclientv1.NewForConfig(withProtobuf(config)) + if err != nil { + return nil, err + } + apiextensionsClientv1, err := apiextensionsclientv1.NewForConfig(withProtobuf(config)) + if err != nil { + return nil, err + } + apiregistrationClientv1, err := apiregistrationclientv1.NewForConfig(config) + if err != nil { + return nil, err + } + appsClientv1, err := appsclientv1.NewForConfig(withProtobuf(config)) + if err != nil { + return nil, err + } + batchClientv1, err := batchclientv1.NewForConfig(withProtobuf(config)) + if err != nil { + return nil, err + } + configClientv1, err := configclientv1.NewForConfig(config) + if err != nil { + return nil, err + } + coreClientv1, err := coreclientv1.NewForConfig(withProtobuf(config)) + if err != nil { + return nil, err + } + imageClientv1, err := imageclientv1.NewForConfig(config) + if err != nil { + return nil, err + } + operatorsClientv1, err := operatorsclientv1.NewForConfig(config) + if err != nil { + return nil, err + } + rbacClientv1, err := rbacclientv1.NewForConfig(withProtobuf(config)) + if err != nil { + return nil, err + } + securityClientv1, err := securityclientv1.NewForConfig(config) + if err != nil { + return nil, err + } return &builder{ raw: m.Raw, - admissionregistrationClientv1: admissionregistrationclientv1.NewForConfigOrDie(withProtobuf(config)), - apiextensionsClientv1: apiextensionsclientv1.NewForConfigOrDie(withProtobuf(config)), - apiregistrationClientv1: apiregistrationclientv1.NewForConfigOrDie(config), - appsClientv1: appsclientv1.NewForConfigOrDie(withProtobuf(config)), - batchClientv1: batchclientv1.NewForConfigOrDie(withProtobuf(config)), - configClientv1: configclientv1.NewForConfigOrDie(config), - coreClientv1: coreclientv1.NewForConfigOrDie(withProtobuf(config)), - imageClientv1: imageclientv1.NewForConfigOrDie(config), - operatorsClientv1: operatorsclientv1.NewForConfigOrDie(config), - rbacClientv1: rbacclientv1.NewForConfigOrDie(withProtobuf(config)), - securityClientv1: securityclientv1.NewForConfigOrDie(config), - } + admissionregistrationClientv1: admissionregistrationClientv1, + apiextensionsClientv1: apiextensionsClientv1, + apiregistrationClientv1: apiregistrationClientv1, + appsClientv1: appsClientv1, + batchClientv1: batchClientv1, + configClientv1: configClientv1, + coreClientv1: coreClientv1, + imageClientv1: imageClientv1, + operatorsClientv1: operatorsClientv1, + rbacClientv1: rbacClientv1, + securityClientv1: securityClientv1, + }, nil } func (b *builder) WithMode(m Mode) Interface { diff --git a/pkg/cvo/internal/operatorstatus.go b/pkg/cvo/internal/operatorstatus.go index b2342825a7..174a748031 100644 --- a/pkg/cvo/internal/operatorstatus.go +++ b/pkg/cvo/internal/operatorstatus.go @@ -56,9 +56,13 @@ type clusterOperatorBuilder struct { mode resourcebuilder.Mode } -func newClusterOperatorBuilder(config *rest.Config, m manifest.Manifest) resourcebuilder.Interface { - client := configclientv1.NewForConfigOrDie(config).ClusterOperators() - return NewClusterOperatorBuilder(clientClusterOperatorsGetter{getter: client}, client, m) +func newClusterOperatorBuilder(config *rest.Config, m manifest.Manifest) (resourcebuilder.Interface, error) { + configClient, err := configclientv1.NewForConfig(config) + if err != nil { + return nil, err + } + client := configClient.ClusterOperators() + return NewClusterOperatorBuilder(clientClusterOperatorsGetter{getter: client}, client, m), nil } // ClusterOperatorsGetter abstracts object access with a client or a cache lister. diff --git a/pkg/cvo/sync_test.go b/pkg/cvo/sync_test.go index 4894dd7729..91735745a5 100644 --- a/pkg/cvo/sync_test.go +++ b/pkg/cvo/sync_test.go @@ -446,8 +446,8 @@ func (t *testBuilder) Do(_ context.Context) error { } func newTestBuilder(r *recorder, rts map[action]error) resourcebuilder.NewInterfaceFunc { - return func(_ *rest.Config, m manifest.Manifest) resourcebuilder.Interface { - return &testBuilder{recorder: r, reactors: rts, m: &m} + return func(_ *rest.Config, m manifest.Manifest) (resourcebuilder.Interface, error) { + return &testBuilder{recorder: r, reactors: rts, m: &m}, nil } } From 6b0ac610e9987f09a4bc8a6153576aed88ede585 Mon Sep 17 00:00:00 2001 From: Calum Murray Date: Tue, 21 Jul 2026 13:22:06 -0400 Subject: [PATCH 2/2] chore: cache client sets with fixed-size slots The CVO creates exactly two rest.Config pointers at startup (default-QPS and burst-QPS). Cache the client sets built from each using a fixed-size [2]clientSetSlot array so the two-slot constraint is explicit in the type system. A third config will produce a clear error rather than silently growing. Signed-off-by: Calum Murray --- hack/generate-lib-resources.py | 102 +++++++++++++++++++++++-- lib/resourcebuilder/resourcebuilder.go | 83 +++++++++++++++++++- 2 files changed, 174 insertions(+), 11 deletions(-) diff --git a/hack/generate-lib-resources.py b/hack/generate-lib-resources.py index 9c1098f7ad..1161f1b0e6 100755 --- a/hack/generate-lib-resources.py +++ b/hack/generate-lib-resources.py @@ -32,7 +32,7 @@ def generate_resourceread(directory, types): short_name = os.path.basename(base) imports[package] = '\t{}{} "{}"'.format(short_name, version, package) - lines.extend([import_line for _, import_line in sorted(imports.items(), key=lambda package_line: package_line[0])]) + lines.extend(group_imports(imports)) lines.extend([ ')', '', @@ -90,6 +90,7 @@ def generate_resourcebuilder(directory, types, clients, modifiers, health_checks 'import (', '\t"context"', '\t"fmt"', + '\t"sync"', '', ] @@ -128,7 +129,7 @@ def generate_resourcebuilder(directory, types, clients, modifiers, health_checks 'protobuf': client['package'].startswith('k8s.io/') and 'kube-aggregator' not in client['package'], } - lines.extend([import_line for _, import_line in sorted(imports.items(), key=lambda package_line: package_line[0])]) + lines.extend(group_imports(imports)) longest_property = max(len(prop_name) for prop_name in client_properties.keys()) @@ -150,7 +151,17 @@ def generate_resourcebuilder(directory, types, clients, modifiers, health_checks lines.extend([ '}', '', - 'func newBuilder(config *rest.Config, m manifest.Manifest) (Interface, error) {', + '// clientSet holds cached typed clients for resource building.', + 'type clientSet struct {', + ]) + lines.extend([ + '\t{:{width}} {}'.format(prop_name, data['type'], width=longest_property) + for prop_name, data in sorted(client_properties.items()) + ]) + lines.extend([ + '}', + '', + 'func newClientSet(config *rest.Config) (*clientSet, error) {', ]) for prop_name, data in sorted(client_properties.items()): new_client_arg = 'config' @@ -163,16 +174,66 @@ def generate_resourcebuilder(directory, types, clients, modifiers, health_checks '\t\treturn nil, err', '\t}', ]) - lines.extend([ + '\treturn &clientSet{', + ]) + for prop_name, data in sorted(client_properties.items()): + var_name = prop_name[0].lower() + prop_name[1:] + lines.append('\t\t{:{width}} {},'.format(prop_name + ':', var_name, width=longest_property+1)) + lines.extend([ + '\t}, nil', + '}', + '', + '// clientSetSlot pairs a rest.Config pointer with its lazily-created clientSet.', + 'type clientSetSlot struct {', + '\tconfig *rest.Config', + '\tclients *clientSet', + '}', + '', + '// clientSetSlots caches clientSets for the two rest.Config pointers used by the', + '// CVO: one for default-QPS and one for burst-QPS during initialization. The', + '// fixed-size array makes the two-client constraint visible in the code and will', + '// produce a clear error if a future change introduces a third config.', + 'var (', + '\tclientSetMu sync.Mutex', + '\tclientSetSlots [2]clientSetSlot', + ')', + '', + 'func getOrCreateClientSet(config *rest.Config) (*clientSet, error) {', + '\tif config == nil {', + '\t\treturn nil, fmt.Errorf("cannot create client set from nil rest.Config")', + '\t}', + '\tclientSetMu.Lock()', + '\tdefer clientSetMu.Unlock()', + '\tfor i := range clientSetSlots {', + '\t\tif clientSetSlots[i].config == config {', + '\t\t\treturn clientSetSlots[i].clients, nil', + '\t\t}', + '\t}', + '\tcs, err := newClientSet(config)', + '\tif err != nil {', + '\t\treturn nil, err', + '\t}', + '\tfor i := range clientSetSlots {', + '\t\tif clientSetSlots[i].config == nil {', + '\t\t\tclientSetSlots[i] = clientSetSlot{config: config, clients: cs}', + '\t\t\treturn cs, nil', + '\t\t}', + '\t}', + '\treturn nil, fmt.Errorf("exceeded %d cached client sets, which is the maximum supported by the CVO", len(clientSetSlots))', + '}', + '', + 'func newBuilder(config *rest.Config, m manifest.Manifest) (Interface, error) {', + '\tcs, err := getOrCreateClientSet(config)', + '\tif err != nil {', + '\t\treturn nil, err', + '\t}', '\treturn &builder{', '\t\traw: m.Raw,', '', ]) for prop_name, data in sorted(client_properties.items()): - var_name = prop_name[0].lower() + prop_name[1:] - lines.append('\t\t{:{width}} {},'.format(prop_name + ':', var_name, width=longest_property+1)) - + lines.append('\t\t{:{width}} cs.{},'.format(prop_name + ':', prop_name, width=longest_property+1)) lines.extend([ '\t}, nil', '}', @@ -275,6 +336,33 @@ def generate_resourcebuilder(directory, types, clients, modifiers, health_checks f.write('\n'.join(lines)) +def group_imports(imports): + """Group import lines by gci category: default, k8s.io, github.com/openshift, localmodule.""" + local_module = 'github.com/openshift/cluster-version-operator' + groups = { + 'default': [], + 'k8s.io': [], + 'openshift': [], + 'localmodule': [], + } + for package, line in sorted(imports.items(), key=lambda package_line: package_line[0]): + if package.startswith(local_module): + groups['localmodule'].append(line) + elif package.startswith('github.com/openshift'): + groups['openshift'].append(line) + elif package.startswith('k8s.io'): + groups['k8s.io'].append(line) + else: + groups['default'].append(line) + result = [] + for key in ['default', 'k8s.io', 'openshift', 'localmodule']: + if groups[key]: + if result: + result.append('') + result.extend(groups[key]) + return result + + def scheme_group_versions(types): sgvs = {} for package, type_names in types.items(): diff --git a/lib/resourcebuilder/resourcebuilder.go b/lib/resourcebuilder/resourcebuilder.go index fbb7383fb7..da8fadfc60 100644 --- a/lib/resourcebuilder/resourcebuilder.go +++ b/lib/resourcebuilder/resourcebuilder.go @@ -6,6 +6,7 @@ package resourcebuilder import ( "context" "fmt" + "sync" operatorsv1 "github.com/operator-framework/api/pkg/operators/v1" operatorsclientv1 "github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/clientset/versioned/typed/operators/v1" @@ -56,7 +57,22 @@ type builder struct { securityClientv1 securityclientv1.SecurityV1Interface } -func newBuilder(config *rest.Config, m manifest.Manifest) (Interface, error) { +// clientSet holds cached typed clients for resource building. +type clientSet struct { + admissionregistrationClientv1 admissionregistrationclientv1.AdmissionregistrationV1Interface + apiextensionsClientv1 apiextensionsclientv1.ApiextensionsV1Interface + apiregistrationClientv1 apiregistrationclientv1.ApiregistrationV1Interface + appsClientv1 appsclientv1.AppsV1Interface + batchClientv1 batchclientv1.BatchV1Interface + configClientv1 configclientv1.ConfigV1Interface + coreClientv1 coreclientv1.CoreV1Interface + imageClientv1 imageclientv1.ImageV1Interface + operatorsClientv1 operatorsclientv1.OperatorsV1Interface + rbacClientv1 rbacclientv1.RbacV1Interface + securityClientv1 securityclientv1.SecurityV1Interface +} + +func newClientSet(config *rest.Config) (*clientSet, error) { admissionregistrationClientv1, err := admissionregistrationclientv1.NewForConfig(withProtobuf(config)) if err != nil { return nil, err @@ -101,9 +117,7 @@ func newBuilder(config *rest.Config, m manifest.Manifest) (Interface, error) { if err != nil { return nil, err } - return &builder{ - raw: m.Raw, - + return &clientSet{ admissionregistrationClientv1: admissionregistrationClientv1, apiextensionsClientv1: apiextensionsClientv1, apiregistrationClientv1: apiregistrationClientv1, @@ -118,6 +132,67 @@ func newBuilder(config *rest.Config, m manifest.Manifest) (Interface, error) { }, nil } +// clientSetSlot pairs a rest.Config pointer with its lazily-created clientSet. +type clientSetSlot struct { + config *rest.Config + clients *clientSet +} + +// clientSetSlots caches clientSets for the two rest.Config pointers used by the +// CVO: one for default-QPS and one for burst-QPS during initialization. The +// fixed-size array makes the two-client constraint visible in the code and will +// produce a clear error if a future change introduces a third config. +var ( + clientSetMu sync.Mutex + clientSetSlots [2]clientSetSlot +) + +func getOrCreateClientSet(config *rest.Config) (*clientSet, error) { + if config == nil { + return nil, fmt.Errorf("cannot create client set from nil rest.Config") + } + clientSetMu.Lock() + defer clientSetMu.Unlock() + for i := range clientSetSlots { + if clientSetSlots[i].config == config { + return clientSetSlots[i].clients, nil + } + } + cs, err := newClientSet(config) + if err != nil { + return nil, err + } + for i := range clientSetSlots { + if clientSetSlots[i].config == nil { + clientSetSlots[i] = clientSetSlot{config: config, clients: cs} + return cs, nil + } + } + return nil, fmt.Errorf("exceeded %d cached client sets, which is the maximum supported by the CVO", len(clientSetSlots)) +} + +func newBuilder(config *rest.Config, m manifest.Manifest) (Interface, error) { + cs, err := getOrCreateClientSet(config) + if err != nil { + return nil, err + } + return &builder{ + raw: m.Raw, + + admissionregistrationClientv1: cs.admissionregistrationClientv1, + apiextensionsClientv1: cs.apiextensionsClientv1, + apiregistrationClientv1: cs.apiregistrationClientv1, + appsClientv1: cs.appsClientv1, + batchClientv1: cs.batchClientv1, + configClientv1: cs.configClientv1, + coreClientv1: cs.coreClientv1, + imageClientv1: cs.imageClientv1, + operatorsClientv1: cs.operatorsClientv1, + rbacClientv1: cs.rbacClientv1, + securityClientv1: cs.securityClientv1, + }, nil +} + func (b *builder) WithMode(m Mode) Interface { b.mode = m return b