From 35f80b808bf39b5c60d1cd4c469acd28494bab62 Mon Sep 17 00:00:00 2001 From: Joseph Date: Mon, 16 Feb 2026 19:12:58 -0500 Subject: [PATCH 1/3] Optimize NonAdminScheme and CodecFactory with singleton pattern Add thread-safe singleton pattern using sync.Once for NonAdminScheme() and CodecFactory creation in output package. This eliminates redundant allocations on every JSON/YAML output operation. Changes: - NonAdminScheme() now uses sync.Once to create scheme only once - CodecFactory is cached as singleton, created only on first use - Add TestNonAdminSchemeConcurrency to verify thread-safety Impact: Reduces latency and memory allocations for all nonadmin get commands with -o yaml/json output. Co-Authored-By: Claude Sonnet 4.5 --- cmd/non-admin/output/output.go | 39 ++++++++++++++++++++--------- cmd/non-admin/output/output_test.go | 24 ++++++++++++++++++ 2 files changed, 51 insertions(+), 12 deletions(-) diff --git a/cmd/non-admin/output/output.go b/cmd/non-admin/output/output.go index a27eca01..39659900 100644 --- a/cmd/non-admin/output/output.go +++ b/cmd/non-admin/output/output.go @@ -20,6 +20,7 @@ import ( "bytes" "fmt" "io" + "sync" nacv1alpha1 "github.com/migtools/oadp-non-admin/api/v1alpha1" "github.com/pkg/errors" @@ -32,21 +33,33 @@ import ( "k8s.io/apimachinery/pkg/runtime/serializer" ) +var ( + // Singleton scheme instance + nonAdminScheme *runtime.Scheme + nonAdminSchemeOnce sync.Once + + // Singleton codec factory instance + codecFactory serializer.CodecFactory + codecFactoryOnce sync.Once +) + // NonAdminScheme returns a runtime.Scheme with NonAdmin types registered func NonAdminScheme() *runtime.Scheme { - scheme := runtime.NewScheme() + nonAdminSchemeOnce.Do(func() { + nonAdminScheme = runtime.NewScheme() - // Add NonAdmin types - if err := nacv1alpha1.AddToScheme(scheme); err != nil { - panic(fmt.Sprintf("failed to add NonAdmin types to scheme: %v", err)) - } + // Add NonAdmin types + if err := nacv1alpha1.AddToScheme(nonAdminScheme); err != nil { + panic(fmt.Sprintf("failed to add NonAdmin types to scheme: %v", err)) + } - // Add Velero types for compatibility - if err := velerov1api.AddToScheme(scheme); err != nil { - panic(fmt.Sprintf("failed to add Velero types to scheme: %v", err)) - } + // Add Velero types for compatibility + if err := velerov1api.AddToScheme(nonAdminScheme); err != nil { + panic(fmt.Sprintf("failed to add Velero types to scheme: %v", err)) + } + }) - return scheme + return nonAdminScheme } // BindFlags wraps Velero's BindFlags to add output flags @@ -123,8 +136,10 @@ func encodeTo(obj runtime.Object, format string, w io.Writer) error { func encoderFor(format string, obj runtime.Object) (runtime.Encoder, error) { var encoder runtime.Encoder - // Use NonAdminScheme instead of Velero's scheme - codecFactory := serializer.NewCodecFactory(NonAdminScheme()) + // Initialize codec factory once using singleton scheme + codecFactoryOnce.Do(func() { + codecFactory = serializer.NewCodecFactory(NonAdminScheme()) + }) desiredMediaType := fmt.Sprintf("application/%s", format) serializerInfo, found := runtime.SerializerInfoForMediaType(codecFactory.SupportedMediaTypes(), desiredMediaType) diff --git a/cmd/non-admin/output/output_test.go b/cmd/non-admin/output/output_test.go index 4a130fa6..a3e0a4ad 100644 --- a/cmd/non-admin/output/output_test.go +++ b/cmd/non-admin/output/output_test.go @@ -21,6 +21,7 @@ import ( "io" "os" "strings" + "sync" "testing" nacv1alpha1 "github.com/migtools/oadp-non-admin/api/v1alpha1" @@ -119,6 +120,29 @@ func TestNonAdminScheme(t *testing.T) { } } +// TestNonAdminSchemeConcurrency verifies that NonAdminScheme() is thread-safe and returns singleton +func TestNonAdminSchemeConcurrency(t *testing.T) { + var wg sync.WaitGroup + schemes := make([]*runtime.Scheme, 100) + + // Call from 100 goroutines + for i := 0; i < 100; i++ { + wg.Add(1) + go func(idx int) { + defer wg.Done() + schemes[idx] = NonAdminScheme() + }(i) + } + wg.Wait() + + // Verify all got same instance + for i := 1; i < 100; i++ { + if schemes[i] != schemes[0] { + t.Errorf("Expected same scheme instance, got different at index %d", i) + } + } +} + func TestBindFlags(t *testing.T) { cmd := &cobra.Command{} BindFlags(cmd.Flags()) From af14144e1446b22251ec612bd81654e10c0005db Mon Sep 17 00:00:00 2001 From: Joseph Date: Mon, 16 Feb 2026 19:13:27 -0500 Subject: [PATCH 2/3] Optimize HTTP client creation with caching by timeout Add sync.Map-based cache for HTTP clients indexed by timeout duration. This eliminates repeated client allocations during download operations. Changes: - httpClientWithTimeout() now caches clients by timeout value - Use sync.Map for thread-safe caching without lock contention - Add TestHTTPClientCaching to verify caching behavior Impact: Reduces allocations during describe --details operations (4 downloads per command) and all other download workflows. Co-Authored-By: Claude Sonnet 4.5 --- cmd/shared/download.go | 18 +++++++++++++++++- cmd/shared/download_test.go | 17 +++++++++++++++++ 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/cmd/shared/download.go b/cmd/shared/download.go index 5e5a75c2..47aeb3c0 100644 --- a/cmd/shared/download.go +++ b/cmd/shared/download.go @@ -25,6 +25,7 @@ import ( "net/http" "os" "strings" + "sync" "time" nacv1alpha1 "github.com/migtools/oadp-non-admin/api/v1alpha1" @@ -33,6 +34,11 @@ import ( kbclient "sigs.k8s.io/controller-runtime/pkg/client" ) +var ( + // Cache HTTP clients by timeout duration + httpClientCache sync.Map // map[time.Duration]*http.Client +) + // DefaultHTTPTimeout is the default timeout for HTTP requests when downloading content from object storage. // This prevents the CLI from hanging indefinitely if the connection stalls. const DefaultHTTPTimeout = 10 * time.Minute @@ -69,9 +75,19 @@ func GetHTTPTimeoutWithOverride(override time.Duration) time.Duration { // httpClientWithTimeout returns an HTTP client with a configured timeout. // Using a custom client instead of http.DefaultClient ensures downloads don't hang indefinitely. func httpClientWithTimeout(timeout time.Duration) *http.Client { - return &http.Client{ + // Try to load from cache + if cached, ok := httpClientCache.Load(timeout); ok { + return cached.(*http.Client) + } + + // Create new client + client := &http.Client{ Timeout: timeout, } + + // Store in cache (LoadOrStore handles race conditions) + actual, _ := httpClientCache.LoadOrStore(timeout, client) + return actual.(*http.Client) } // DownloadRequestOptions holds configuration for creating and processing NonAdminDownloadRequests diff --git a/cmd/shared/download_test.go b/cmd/shared/download_test.go index 01c825aa..98d23af0 100644 --- a/cmd/shared/download_test.go +++ b/cmd/shared/download_test.go @@ -147,6 +147,23 @@ func TestHttpClientWithTimeout(t *testing.T) { } } +// TestHTTPClientCaching verifies that HTTP clients are cached by timeout duration +func TestHTTPClientCaching(t *testing.T) { + timeout := 10 * time.Minute + client1 := httpClientWithTimeout(timeout) + client2 := httpClientWithTimeout(timeout) + + if client1 != client2 { + t.Error("Expected same client instance for same timeout") + } + + differentTimeout := 5 * time.Minute + client3 := httpClientWithTimeout(differentTimeout) + if client1 == client3 { + t.Error("Expected different client for different timeout") + } +} + // TestDownloadContentWithTimeout tests downloading content with explicit timeout func TestDownloadContentWithTimeout(t *testing.T) { tests := []struct { From cf096e64ea63c55fb65946906e4189d6348fe7e6 Mon Sep 17 00:00:00 2001 From: Joseph Date: Mon, 16 Feb 2026 19:13:35 -0500 Subject: [PATCH 3/3] Optimize Kubernetes scheme creation with configuration-based caching Add sync.Map-based cache for runtime.Scheme objects indexed by ClientOptions configuration. This eliminates repeated scheme creation across different commands using the same configuration. Changes: - NewSchemeWithTypes() now caches schemes by configuration key - Add schemeKey type to uniquely identify scheme configurations - Add TestNewSchemeWithTypesCaching tests to verify caching Impact: Reduces allocations for ~19 client creation calls across different commands that use the same scheme configuration. Co-Authored-By: Claude Sonnet 4.5 --- cmd/shared/client.go | 30 +++++++++- cmd/shared/client_test.go | 115 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 144 insertions(+), 1 deletion(-) create mode 100644 cmd/shared/client_test.go diff --git a/cmd/shared/client.go b/cmd/shared/client.go index 1de83a61..4a81c237 100644 --- a/cmd/shared/client.go +++ b/cmd/shared/client.go @@ -20,6 +20,7 @@ import ( "context" "fmt" "net" + "sync" "time" nacv1alpha1 "github.com/migtools/oadp-non-admin/api/v1alpha1" @@ -31,6 +32,18 @@ import ( kbclient "sigs.k8s.io/controller-runtime/pkg/client" ) +// schemeKey uniquely identifies a scheme configuration +type schemeKey struct { + hasNonAdmin bool + hasVelero bool + hasCore bool +} + +var ( + // Cache schemes by configuration + schemeCache sync.Map // map[schemeKey]*runtime.Scheme +) + // ClientOptions holds configuration for creating Kubernetes clients type ClientOptions struct { // IncludeNonAdminTypes adds OADP NonAdmin CRD types to the scheme @@ -122,6 +135,19 @@ func NewClientWithFullScheme(f client.Factory) (kbclient.WithWatch, error) { // NewSchemeWithTypes creates a new runtime scheme with the specified types func NewSchemeWithTypes(opts ClientOptions) (*runtime.Scheme, error) { + // Create cache key from options + key := schemeKey{ + hasNonAdmin: opts.IncludeNonAdminTypes, + hasVelero: opts.IncludeVeleroTypes, + hasCore: opts.IncludeCoreTypes, + } + + // Try cache first + if cached, ok := schemeCache.Load(key); ok { + return cached.(*runtime.Scheme), nil + } + + // Create new scheme scheme := runtime.NewScheme() if opts.IncludeNonAdminTypes { @@ -142,7 +168,9 @@ func NewSchemeWithTypes(opts ClientOptions) (*runtime.Scheme, error) { } } - return scheme, nil + // Store in cache (handles race conditions) + actual, _ := schemeCache.LoadOrStore(key, scheme) + return actual.(*runtime.Scheme), nil } // GetCurrentNamespace gets the current namespace from the kubeconfig context diff --git a/cmd/shared/client_test.go b/cmd/shared/client_test.go new file mode 100644 index 00000000..f15f7a59 --- /dev/null +++ b/cmd/shared/client_test.go @@ -0,0 +1,115 @@ +/* +Copyright 2025 The OADP CLI Contributors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package shared + +import ( + "testing" +) + +// TestNewSchemeWithTypesCaching verifies that schemes are cached by configuration +func TestNewSchemeWithTypesCaching(t *testing.T) { + opts := ClientOptions{ + IncludeNonAdminTypes: true, + IncludeVeleroTypes: true, + } + + scheme1, err1 := NewSchemeWithTypes(opts) + scheme2, err2 := NewSchemeWithTypes(opts) + + if err1 != nil || err2 != nil { + t.Fatal("Unexpected error creating schemes") + } + + if scheme1 != scheme2 { + t.Error("Expected same scheme instance for same options") + } +} + +// TestNewSchemeWithTypesCachingDifferentOptions verifies different options yield different schemes +func TestNewSchemeWithTypesCachingDifferentOptions(t *testing.T) { + opts1 := ClientOptions{ + IncludeNonAdminTypes: true, + IncludeVeleroTypes: true, + } + + opts2 := ClientOptions{ + IncludeNonAdminTypes: true, + IncludeVeleroTypes: false, + } + + scheme1, err1 := NewSchemeWithTypes(opts1) + scheme2, err2 := NewSchemeWithTypes(opts2) + + if err1 != nil || err2 != nil { + t.Fatal("Unexpected error creating schemes") + } + + if scheme1 == scheme2 { + t.Error("Expected different scheme instances for different options") + } +} + +// TestNewSchemeWithTypes verifies that the function creates schemes with correct types +func TestNewSchemeWithTypes(t *testing.T) { + tests := []struct { + name string + opts ClientOptions + }{ + { + name: "NonAdmin types only", + opts: ClientOptions{ + IncludeNonAdminTypes: true, + }, + }, + { + name: "Velero types only", + opts: ClientOptions{ + IncludeVeleroTypes: true, + }, + }, + { + name: "Core types only", + opts: ClientOptions{ + IncludeCoreTypes: true, + }, + }, + { + name: "All types", + opts: ClientOptions{ + IncludeNonAdminTypes: true, + IncludeVeleroTypes: true, + IncludeCoreTypes: true, + }, + }, + { + name: "No types", + opts: ClientOptions{}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + scheme, err := NewSchemeWithTypes(tt.opts) + if err != nil { + t.Errorf("NewSchemeWithTypes() unexpected error: %v", err) + } + if scheme == nil { + t.Error("NewSchemeWithTypes() returned nil scheme") + } + }) + } +}