Optimization fixes

[Joeavaikath]

Why the changes were made

This PR introduces performance optimizations and code simplifications to reduce memory allocations and improve command execution latency across the OADP CLI.

Performance Optimizations:

• Singleton pattern for NonAdminScheme and CodecFactory: Eliminates redundant allocations on every JSON/YAML output operation by caching scheme and codec factory as
  singletons using sync.Once
• HTTP client caching: Caches HTTP clients by timeout duration to avoid repeated client allocations during download operations (impacts describe --details and other
  download workflows)
• Kubernetes scheme caching: Caches runtime.Scheme objects by configuration to eliminate repeated scheme creation across ~19 client creation calls that use the same
  configuration

Impact: These changes reduce memory allocations and latency for all non-admin get commands with -o yaml/json output, download operations, and commands that create
Kubernetes clients.

[coderabbitai]

📝 Walkthrough

Walkthrough

Reworks NABSL request lookup, adds caching for runtime schemes and HTTP clients, converts non-admin output to singleton initialization, removes the non-admin backup force flag and related flows, extends command-wrapping detection to parent commands, and adds concurrency and cache tests.

Changes

Cohort / File(s)	Summary
NABSL Request Lookup cmd/nabsl-request/get.go	Simplifies lookup: when a Name is provided, GET the NonAdminBackupStorageLocationRequest directly from the admin namespace; otherwise list requests in admin or across all namespaces. Removes namespace-derivation and UUID→name mapping logic and associated error aggregation.
Non‑Admin Output (singleton) cmd/non-admin/output/output.go, cmd/non-admin/output/output_test.go	Introduce package-level singletons for runtime Scheme and codecFactory using sync.Once; defer initialization until first use. Add concurrency test ensuring NonAdminScheme() is thread-safe and returns a singleton.
Non‑Admin Output Consumer cmd/non-admin/bsl/create.go	Switch output helper import from upstream velero to local cmd/non-admin/output package; update usage to the local output helpers.
Backup create: remove Force cmd/non-admin/backup/create.go, cmd/non-admin/backup/backup_test.go, cmd/non-admin/backup/README.md, docs/oadp-self-service.md	Remove --force flag and Force field from CreateOptions; eliminate prompt/admin-defaults flow and related helpers; require --storage-location unconditionally; update help/docs and tests to remove force references.
Scheme caching cmd/shared/client.go, cmd/shared/client_test.go	Add a sync.Map-backed cache keyed by option flags for runtime Schemes; return cached scheme when options match. Add tests validating caching and distinct instances for differing options.
HTTP client caching cmd/shared/download.go, cmd/shared/download_test.go	Cache *http.Client instances by Timeout using sync.Map and LoadOrStore to reuse clients for identical timeouts; add test verifying reuse and differentiation.
Command wrapping detection cmd/root.go	Extend replaceVeleroWithOADP wrapping logic to consider parent commands; skip wrapping for logs/describe/delete when either current or parent command matches to preserve unbuffered output.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• verb-noun order bugfixes #118: Modifies non-admin output implementation and callers — directly related to cmd/non-admin/output changes and consumer updates.

Suggested labels

ai-generated-test

Suggested reviewers

• weshayutin
• kaovilai

Poem

🐰 I hopped through code with a twitch and a dash,
Cached schemes and clients to speed up the cache,
Singletons snug, concurrency tame,
Flags trimmed away — simpler’s the game,
Celebrating builds with a carrot and bash! 🎉

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 68.75% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check	❓ Inconclusive	The title 'Optimization fixes' is vague and generic, not clearly describing the main changes in the PR which include performance optimizations, code simplifications, bug fixes, and removal of the --force flag.	Use a more specific title that highlights the primary change, such as 'Add scheme and HTTP client caching with NABSL simplifications' or 'Performance optimizations: caching and code simplifications'.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description check	✅ Passed	The PR description clearly explains the performance optimizations and code simplifications with specific details on caching mechanisms and their impact.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

cmd/nabsl-request/get.go (1)

129-136: Comment is misleading for the all-namespaces case.

The comment states "List all requests in admin namespace" but the code also handles the --all-namespaces flag which lists across all namespaces. Consider updating the comment to reflect both cases.

📝 Suggested comment fix

-	// List all requests in admin namespace
+	// List requests: either across all namespaces or in admin namespace only
 	var requestList nacv1alpha1.NonAdminBackupStorageLocationRequestList
 	var err error
 	if o.AllNamespaces {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmd/nabsl-request/get.go` around lines 129 - 136, Update the misleading
comment above the listing logic to accurately describe both branches: when
o.AllNamespaces is true the code lists NonAdminBackupStorageLocationRequestList
across all namespaces via o.client.List, otherwise it lists only in the admin
namespace using kbclient.InNamespace(adminNS); reference the variable
requestList, the type nacv1alpha1.NonAdminBackupStorageLocationRequestList, the
flag o.AllNamespaces, and the call o.client.List so the comment clearly reflects
both cases.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@cmd/nabsl-request/get.go`:
- Around line 129-136: Update the misleading comment above the listing logic to
accurately describe both branches: when o.AllNamespaces is true the code lists
NonAdminBackupStorageLocationRequestList across all namespaces via
o.client.List, otherwise it lists only in the admin namespace using
kbclient.InNamespace(adminNS); reference the variable requestList, the type
nacv1alpha1.NonAdminBackupStorageLocationRequestList, the flag o.AllNamespaces,
and the call o.client.List so the comment clearly reflects both cases.

[kaovilai]

The schemeCache adds complexity (schemeKey struct, sync.Map, LoadOrStore semantics, shared mutable state) to cache something that is created exactly once per process lifetime. It provides no performance benefit for a short-lived CLI process. The sync.Once approach in
output.go is at least defensible since NonAdminScheme() could theoretically be called from multiple places within one command, but the sync.Map scheme cache in client.go solves a problem that doesn't exist.

do others agree?

[kaovilai]

same here..

[kaovilai]

Suggested change

	// NonAdminScheme returns a runtime.Scheme with NonAdmin types registered
	func NonAdminScheme() *runtime.Scheme {
	// NonAdminScheme returns a runtime.Scheme with NonAdmin types registered
	// callers must not mutate the returned object
	func NonAdminScheme() *runtime.Scheme {

[shubham-pampattiwar]

Agree with @kaovilai's points. Since this is a CLI (short-lived process), scheme and client creation happens once per command execution anyway, so the caching wouldn't have much impact here.

The sync.Once approach in output.go makes sense if NonAdminScheme() gets called multiple times within a single command. The sync.Map caching in client.go and download.go is harder to justify for a CLI context.

Would be helpful to see some profiling data or benchmarks showing where time is being spent. That would help us evaluate whether this complexity is worth it.