Resilient NodePool Informer — Solution Design

1. Problem Statement

A single NodePool CR with an invalid *metav1.Duration value (e.g., nodeDrainTimeout: "1" — missing the unit suffix) causes the HyperShift operator's cluster-wide v1beta1.NodePool informer to fail on every LIST/WATCH cycle.

Because the informer uses typed deserialization (NodePoolList decoded all-or-nothing), one unparseable object prevents the shared cache from ever syncing — blocking NodePool reconciliation for every hosted cluster on the management cluster.

Error Chain

1. Bad NodePool with nodeDrainTimeout: "1" (no unit suffix) exists in etcd
2. metav1.Duration.UnmarshalJSON calls time.ParseDuration("1") → error
3. v1beta1.NodePool typed LIST fails entirely (all-or-nothing deserialization)
4. Informer shared cache never syncs (cluster-wide, not per-namespace)
5. No NodePool reconciled for any cluster on the management cluster
6. New clusters: CP up, no workers. Existing clusters: cannot delete/scale/modify NodePools

Affected Duration Fields

The NodePool spec has exactly two fields of type *metav1.Duration, both defined in api/hypershift/v1beta1/nodepool_types.go:

2. Relationship with Companion Bug (OCPBUGS-94251)

3. Codebase Exploration Findings

3.1 Operator Manager Configuration

The manager is created in hypershift-operator/main.go:createManager (line 431) with ctrl.NewManager. The cache.Options struct is left at its zero value — no DefaultWatchErrorHandler, no DefaultTransform, no NewInformer, and no namespace scoping. This means:

• All informers are cluster-wide (watch all namespaces)
• No custom error handling for deserialization failures
• One bad object in any namespace poisons the entire informer

3.2 NodePool Controller Setup

In nodepool_controller.go:136-177, the controller uses .For(&hyperv1.NodePool{}), which creates a typed, cluster-wide informer. Under the hood, controller-runtime calls mgr.GetCache().GetInformer(ctx, &hyperv1.NodePool{}), which creates a SharedIndexInformer with a typed ListerWatcher.

3.3 Available Extension Points (controller-runtime v0.22.5)

3.4 Why the Dynamic Client Avoids the Problem

The dynamic.Interface client uses unstructured JSON deserialization (map[string]interface{}), which treats all fields as generic JSON values. A duration string like "1" is stored as a plain Go string — no time.ParseDuration is called. This means the dynamic LIST cannot fail for valid JSON stored in etcd, regardless of whether the duration values are semantically valid.

4. Approaches Considered

5. Chosen Solution: Resilient ListerWatcher via cache.Options.NewInformer

Inject a custom informer factory using the cache.Options.NewInformer hook. For the NodePool GVK only, the factory wraps the standard ListerWatcher with a resilient version that falls back to the dynamic client on deserialization errors.

5.1 How It Works

5.2 Key Design Decisions

5.3 WATCH Error Handling

WATCH errors are not intercepted. Here's why this is safe:

1. If a bad NodePool exists but is never modified, the WATCH won't see it (it was skipped during LIST). Everything works normally.
2. If the bad NodePool IS modified, the WATCH event fails deserialization, closing the stream.
3. The Reflector detects the closed stream and restarts with a new LIST.
4. The resilient LIST handles the re-sync, skipping the bad object again.
5. A new WATCH opens from the latest ResourceVersion. If the bad object isn't modified again, the cycle doesn't repeat.

The worst case is if the bad object is continuously modified, causing repeated watch restarts. But with CEL validation preventing new bad writes, this scenario is extremely unlikely.

5.4 Sanitization Logic

When a per-object conversion fails, the sanitizer removes known *metav1.Duration fields that have invalid values:

var durationFields = []string{"nodeDrainTimeout", "nodeVolumeDetachTimeout"}

func sanitizeDurationFields(item *unstructured.Unstructured) *unstructured.Unstructured {
    sanitized := item.DeepCopy()

    spec, found, err := unstructured.NestedMap(sanitized.Object, "spec")
    if err != nil || !found {
        return sanitized
    }

    modified := false
    for _, field := range durationFields {
        val, exists := spec[field]
        if !exists {
            continue
        }
        strVal, ok := val.(string)
        if !ok {
            continue
        }
        if _, parseErr := time.ParseDuration(strVal); parseErr != nil {
            // Remove the invalid field; *metav1.Duration becomes nil (valid zero value)
            delete(spec, field)
            modified = true
        }
    }

    if modified {
        unstructured.SetNestedMap(sanitized.Object, spec, "spec")
    }
    return sanitized
}

5.5 Error Classification

It's critical to distinguish deserialization errors (which should trigger fallback) from API/network errors (which should be propagated):

The design is conservative: if uncertain, it falls back to dynamic LIST. A false-positive fallback is harmless — the dynamic LIST will also fail for real infrastructure errors, and that failure is propagated.

6. Implementation Details

6.1 Files to Create / Modify

• Newhypershift-operator/controllers/nodepool/resilient_lister.go
• Newhypershift-operator/controllers/nodepool/resilient_lister_test.go
• Modhypershift-operator/main.go
• Refapi/hypershift/v1beta1/nodepool_types.go
• Refvendor/sigs.k8s.io/controller-runtime/pkg/cache/cache.go
• Refvendor/k8s.io/client-go/tools/cache/listwatch.go

6.2 New File: resilient_lister.go

Exported Function

func NewResilientInformerFactory(
    restConfig *rest.Config,
    log logr.Logger,
) func(toolscache.ListerWatcher, runtime.Object, time.Duration, toolscache.Indexers) toolscache.SharedIndexInformer

Returns a NewInformer callback. Internally, it creates a dynamic.Interface once from restConfig. The callback type-switches on obj: if *hyperv1.NodePool, wraps the ListerWatcher; otherwise delegates to toolscache.NewSharedIndexInformer unchanged.

Internal Struct

type resilientNodePoolListerWatcher struct {
    delegate      toolscache.ListerWatcher
    dynamicClient dynamic.NamespaceableResourceInterface
    log           logr.Logger
}

// Implements both ListerWatcher and ListerWatcherWithContext

Key Methods

• ListWithContext(ctx, opts) — fast-path typed LIST, deserialization-error fallback to dynamic
• WatchWithContext(ctx, opts) — pure delegation to wrapped ListerWatcher
• dynamicFallbackList(ctx, opts) — dynamic LIST + per-object conversion
• convertUnstructuredNodePool(item) — try direct conversion, sanitize on failure
• sanitizeDurationFields(item) — remove invalid *metav1.Duration fields

6.3 Modified File: main.go

In createManager (line 431), add a Cache field to ctrl.Options:

mgr, err := ctrl.NewManager(restConfig, ctrl.Options{
    // ... existing options ...
    Cache: cache.Options{
        NewInformer: nodepool.NewResilientInformerFactory(
            restConfig,
            ctrl.Log.WithName("resilient-nodepool-cache"),
        ),
    },
})

6.4 Observability

Prometheus metrics registered with the controller-runtime metrics registry:

Additionally, error-level log messages are emitted for every sanitized field and skipped object, including the namespace, name, field path, and original bad value.

7. Test Strategy

Following TESTING.md conventions: table-driven tests, "When <condition>, it should <expected behavior>" naming, tests placed alongside the code in resilient_lister_test.go.

Test Functions

Verification Commands

# Run resilient lister tests
go test ./hypershift-operator/controllers/nodepool/ -run TestResilient -v -count=1
go test ./hypershift-operator/controllers/nodepool/ -run TestSanitize -v -count=1
go test ./hypershift-operator/controllers/nodepool/ -run TestConvert -v -count=1
go test ./hypershift-operator/controllers/nodepool/ -run TestDynamic -v -count=1

# Full nodepool controller tests
go test ./hypershift-operator/controllers/nodepool/... -count=1

# Vet and build
go vet ./hypershift-operator/...
go build ./hypershift-operator/...

8. Behavioral Summary

Before This Fix

1. One bad NodePool → entire informer fails → all NodePool reconciliation blocked cluster-wide → complete outage

After This Fix

1. Typed LIST fails → resilient wrapper detects deserialization error
2. Falls back to dynamic LIST → retrieves all NodePools as unstructured JSON
3. Converts each object individually → sanitizes bad duration fields → skips unconvertible objects
4. Informer cache syncs with all valid NodePools → controllers reconcile normally
5. Bad NodePool is absent from cache → logged at error level with details → operators can fix manually

9. Adversarial Review and Revised Recommendation

9.1 Core Issues with the Continuous Fallback Approach

Issue 1: Cache Inclusion Paradox (Silent Data Corruption vs Stuck Finalizers)

The design contains a fundamental contradiction about what happens to the malformed NodePool:

• Section 5.1 & 5.4 claim: Bad duration fields are sanitized and the object is injected into the cache
• Section 5.2 claims: The bad NodePool is "simply absent from the informer cache"

Both implementations lead to catastrophic outcomes:

• If sanitized + injected: Deleting nodeDrainTimeout: "1" results in nil, which in Kubernetes means infinite drain timeout. User intent (1 minute) becomes permanent hangup of worker replacements and cluster upgrades.
• If skipped entirely: The controller is blind to the bad NodePool. When a user attempts deletion, the API server sets DeletionTimestamp, but the controller never sees it in the cache, never cleans up cloud resources, never removes the finalizer. The NodePool becomes permanently stuck in Terminating.

Issue 2: WATCH Stream Thrashing and Self-Inflicted DoS

The design explicitly does not intercept WATCH errors. The stated justification (Section 5.3) assumes the bad object is not continuously modified. But in reality:

• External controllers, mutating webhooks, or automation may continuously update the bad NodePool (status conditions, timestamps, labels)
• Each update breaks the WATCH stream
• The client-go Reflector applies exponential backoff on repeated stream breaks
• Net effect: The operator pauses informer syncing for exponentially longer periods, effectively halting all NodePool reconciliation cluster-wide
• Each broken stream forces a heavy dynamic LIST, spiking API server load and creating a self-inflicted DoS

Issue 3: Memory Exhaustion and OOMKills at Scale

In a large management cluster (hundreds or thousands of NodePools), if a single object has a bad duration, the operator falls back to a dynamic LIST of all NodePools:

• Dynamic LIST returns all objects as map[string]interface{}
• The sanitization logic does DeepCopy() on each unstructured object
• Per-object conversions trigger GC spikes
• Combined with WATCH thrashing: Massive memory allocations, repeated GC, and back-to-back expensive LISTs will almost certainly trigger OOMKill on the HyperShift operator pod, completely crashing the component it tries to protect

Issue 4: Flawed Error Classification (Catch-All Fallback)

Section 5.5 defines error classification: if the error is not a StatusError or ctx.Err(), it falls back to dynamic LIST. This is dangerous:

• Transient network blips, proxy errors, TLS timeouts, or custom API server responses may not strictly match the known error types
• The system incorrectly identifies standard network turbulence as "deserialization error"
• Consequence: Unnecessary expensive fallback LISTs are triggered exactly when the network or API server is already struggling, compounding system strain

Issue 5: Pagination and Chunking Data Loss

The client-go Reflector uses pagination (e.g., Limit: 500, Continue tokens):

• If a typed LIST fails on "Chunk 3 of 10," does the dynamic fallback fetch only Chunk 3 or all NodePools?
• Can the unstructured client reliably resume pagination using a token from a typed request?
• If mishandled: Informer state becomes corrupted with duplicate or dropped NodePools in the cache

9.2 Why the Continuous Fallback Approach Is Fundamentally Flawed

The original proposal tried to be clever with a continuous fallback mechanism. But the real insight is:

9.3 Final Recommendation: Startup Detection with Degraded-Mode Skip

After adversarial review and further analysis, startup detection with degraded-mode skip is the optimal approach. It allows the operator to remain functional while preventing bad data from poisoning the informer, and provides continuous visibility for the operator to fix the problem.

Why Degraded-Mode Skip Is Better Than Alternatives

vs. Refuse to start: Degraded mode keeps the cluster operational on valid NodePools while customer fixes bad data. No downtime.

vs. Auto-fix: We cannot know the operator's intent for malformed `nodeDrainTimeout: "1"` (could be `1s`, `1m`, `1h`, or unknown). Auto-fixing to any default (e.g., `nil` = infinite timeout) creates silent failure modes. Better to skip and ask operator to fix.

vs. Continuous fallback (Section 5): Degraded mode only affects the startup LIST. Bad objects are skipped once and stay out of cache. No WATCH thrashing, no memory exhaustion, no false-positive fallback triggers. Zero ongoing overhead.

How It Works

1. On operator startup, use the dynamic client to LIST all NodePools
2. Check each NodePool for invalid Duration fields
3. If bad data found:
   • Skip the bad NodePool(s) from the cache entirely (not added to informer)
   • Log ERROR message (loudly and repeatedly) with: namespace, name, field, bad value, exact kubectl fix command
   • Continue startup — operator proceeds in degraded mode on valid NodePools
4. Operator continues to reconcile all valid NodePools normally
5. Customer sees persistent ERROR logs → manually fixes the bad NodePool
6. Next LIST or WATCH cycle picks up the fixed object → added to cache automatically
7. No restart needed; degraded mode ends automatically

Why This Is Better

Detailed Implementation: Custom Informer Factory

The approach is similar to Section 5, but only for startup — no continuous fallback mechanism. Create a custom NewInformer factory that wraps the NodePool ListerWatcher:

// In hypershift-operator/controllers/nodepool/degraded_lister.go
func NewDegradedModeInformerFactory(
    ctx context.Context,
    restConfig *rest.Config,
    log logr.Logger,
) (func(lw toolscache.ListerWatcher, obj runtime.Object, resync time.Duration, indexers toolscache.Indexers) toolscache.SharedIndexInformer,
    error) {

    // Scan for bad NodePools once at startup
    dynClient, err := dynamic.NewForConfig(restConfig)
    if err != nil {
        return nil, fmt.Errorf("create dynamic client: %w", err)
    }

    badNodePoolKeys := findAllBadNodePools(ctx, dynClient, log)

    // Return a factory that wraps LIST to skip bad objects
    return func(lw toolscache.ListerWatcher, obj runtime.Object, resync time.Duration, indexers toolscache.Indexers) toolscache.SharedIndexInformer {
        // Type-switch: only wrap NodePool ListerWatchers
        if _, ok := obj.(*hyperv1.NodePool); !ok {
            return toolscache.NewSharedIndexInformer(lw, obj, resync, indexers)
        }

        // Wrap the ListerWatcher to skip bad NodePools
        wrappedLW := °radedNodePoolListerWatcher{
            delegate:       lw,
            badNodePoolKeys: badNodePoolKeys,
            log:            log,
        }
        return toolscache.NewSharedIndexInformer(wrappedLW, obj, resync, indexers)
    }, nil
}

type degradedNodePoolListerWatcher struct {
    delegate        toolscache.ListerWatcher
    badNodePoolKeys map[string]struct{} // "namespace/name" → struct{}
    log             logr.Logger
}

// ListWithContext: call delegate LIST, then filter out bad NodePools
func (d *degradedNodePoolListerWatcher) ListWithContext(ctx context.Context, opts metav1.ListOptions) (runtime.Object, error) {
    list, err := d.delegate.ListWithContext(ctx, opts)
    if err != nil {
        return list, err
    }

    // Filter out bad NodePools from the list
    npList, ok := list.(*hyperv1.NodePoolList)
    if !ok {
        return list, nil
    }

    filtered := npList.DeepCopy()
    filtered.Items = make([]hyperv1.NodePool, 0, len(npList.Items))
    for _, item := range npList.Items {
        key := fmt.Sprintf("%s/%s", item.Namespace, item.Name)
        if _, isBad := d.badNodePoolKeys[key]; !isBad {
            filtered.Items = append(filtered.Items, item)
        }
    }

    return filtered, nil
}

// WatchWithContext: delegate as-is
func (d *degradedNodePoolListerWatcher) WatchWithContext(ctx context.Context, opts metav1.ListOptions) (watch.Interface, error) {
    return d.delegate.WatchWithContext(ctx, opts)
}

// Helper: scan for bad NodePools once at startup
func findAllBadNodePools(ctx context.Context, dynClient dynamic.Interface, log logr.Logger) map[string]struct{} {
    badKeys := make(map[string]struct{})

    npGVR := schema.GroupVersionResource{
        Group:    "hypershift.openshift.io",
        Version:  "v1beta1",
        Resource: "nodepools",
    }

    list, err := dynClient.Resource(npGVR).List(ctx, metav1.ListOptions{})
    if err != nil {
        log.Error(err, "Failed to scan NodePools at startup")
        return badKeys
    }

    for _, item := range list.Items {
        badFields := findBadDurationFields(&item)
        if len(badFields) > 0 {
            key := fmt.Sprintf("%s/%s", item.GetNamespace(), item.GetName())
            badKeys[key] = struct{}{}

            // Log ERROR loudly with fix instructions
            log.Error(nil, "NodePool has invalid duration field(s); it will not be reconciled until fixed",
                "namespace", item.GetNamespace(),
                "name", item.GetName(),
                "badFields", badFields,
                "fixCommand", fmt.Sprintf(
                    "kubectl patch nodepool %s -n %s -p '{\"spec\":{...}}'",
                    item.GetName(), item.GetNamespace()),
            )
        }
    }

    return badKeys
}

Integration in main.go

// In hypershift-operator/main.go:createManager
factory, err := nodepool.NewDegradedModeInformerFactory(ctx, restConfig, log)
if err != nil {
    log.Error(err, "Failed to initialize degraded mode informer factory")
    os.Exit(1)
}

mgr, err := ctrl.NewManager(restConfig, ctrl.Options{
    // ... existing options ...
    Cache: cache.Options{
        NewInformer: factory,
    },
})

Why This Avoids All Five Failure Modes from Section 5

Auditability

• ERROR logs at startup and continuously: Operator sees persistent ERROR messages identifying bad NodePool(s) with exact namespace, name, field, bad value
• No hidden changes: No auto-fix, no data mutation. Operator must consciously fix via kubectl or API
• Clear fix guidance: Error logs include exact kubectl patch command to fix
• Etcd audit trail: Customer's manual PATCH is recorded with full audit context
• Operator continues: Valid NodePools are reconciled normally; customer sees the degraded state immediately

Trade-offs and Design Rationale

• Safe by design: No data mutation, no defaults. We cannot know what the operator intended; let them fix it
• Cluster stays operational: Valid NodePools continue to reconcile; no downtime
• Visible problem: ERROR logs show exactly what's wrong; customer knows immediately and sees persistent error until fixed
• No restart needed: Customer fixes bad NodePool; next LIST/WATCH picks it up automatically
• Startup delay: O(N) LIST scan, negligible unless cluster has thousands of NodePools
• Zero ongoing overhead: Detection happens once at startup; bad objects skipped on every LIST (trivial O(1) key lookup)
• Avoids all five failure modes: No WATCH thrashing, no memory exhaustion, no silent failures

9.4 Summary

Key advantages:

• ✓ No downtime (cluster stays operational on valid NodePools)
• ✓ No silent failure modes (skips bad data entirely, no risky defaults)
• ✓ Avoids all five Section 5 failure modes (one-time scan, no continuous mechanism, no WATCH thrashing)
• ✓ Visible problem (ERROR logs until customer fixes it)
• ✓ Customer accountability (they must consciously fix their bad data)
• ✓ Auto-recovery (no operator restart needed after fix)

Next step: Implement degraded-mode factory in hypershift-operator/controllers/nodepool/degraded_lister.go with custom ListerWatcher that filters bad NodePools.