Optimization fixes

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)

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())

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

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")
+			}
+		})
+	}
+}

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

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 {