diff --git a/bindata/tnfdeployment/cronjob.yaml b/bindata/tnfdeployment/cronjob.yaml index 282f8eea04..b0121c8312 100644 --- a/bindata/tnfdeployment/cronjob.yaml +++ b/bindata/tnfdeployment/cronjob.yaml @@ -7,7 +7,7 @@ metadata: app.kubernetes.io/name: spec: schedule: - concurrencyPolicy: "Replace" + concurrencyPolicy: "Forbid" startingDeadlineSeconds: 120 failedJobsHistoryLimit: 3 # successfulJobsHistoryLimit + ttlSecondsAfterFinished: TTL deletes finished Jobs before CronJob pruning (OCPBUGS-81340). diff --git a/docs/tnf/job-controllers.md b/docs/tnf/job-controllers.md new file mode 100644 index 0000000000..87c3e8f5ff --- /dev/null +++ b/docs/tnf/job-controllers.md @@ -0,0 +1,311 @@ +# TNF Job Controllers + +## Overview + +Job controllers manage the lifecycle of Kubernetes Jobs that configure and maintain the Pacemaker cluster. The framework provides both single-node and multi-node job execution patterns with automatic retry logic and status tracking. + +## Operational Modes + +The job controller system operates in two distinct modes based on whether the external etcd transition is complete: + +### Bootstrap Mode (Pre-Transition) + +Initial cluster setup during first-time installation: + +- **Entry Point:** Operator sync loop during startup +- **Requirements:** + - Exactly 2 ready control plane nodes (Pacemaker limitation - errors if > 2, waits if < 2) + - Both nodes must report Ready status + - etcd bootstrap must complete + - All nodes must reach latest static pod revision +- **Behavior:** + - Exponential backoff retry (5s → 2min cap) with 10-minute timeout + - Blocks until etcd bootstrap completes and all nodes at stable revision + - Creates auth jobs (per-node), setup job (cluster-wide), fencing job (cluster-wide) + - Waits for setup job to complete before marking transition complete + - Sets `TNFJobControllersDegraded` condition if setup fails after 10-minute retry +- **Exit:** Marks external etcd transition complete when setup job succeeds + +### Runtime Mode (Post-Transition) + +Operations after initial setup is complete (operator restart, upgrade, node replacement): + +- **Entry Point:** Operator sync loop detects transition already complete +- **Requirements:** At least 1 ready control plane node +- **Behavior:** + - Idempotent restart support - safe to call repeatedly + - Setup job is one-time execution (not recreated), but controller runs to maintain conditions + - Ensures auth/after-setup controllers running for each node + - Ensures fencing controller running +- **Use Cases:** Operator restart, upgrade, node replacement, degraded node scenarios + +**Key Difference:** Bootstrap mode blocks until setup completes (initial cluster creation), while runtime mode is non-blocking and assumes cluster already exists. + +## Condition Model + +Job controllers use a three-condition model to track lifecycle and health status. All conditions are set on the etcd Operator CR and propagate to the ClusterOperator CR: + +**Available Condition**: +- Type format: `Available` (e.g., `TNFSetupJobAvailable`) +- Indicates the job has **successfully completed** +- Set when job reaches Complete status +- Cleared when job is restarted or deleted +- Reason: `JobComplete` + +**Progressing Condition**: +- Type format: `Progressing` (e.g., `TNFSetupJobProgressing`) +- Indicates the job is **actively running** +- Set when job is created and running +- Cleared when job completes or fails +- Reason: `JobRunning` + +**Degraded Condition**: +- Type format: `Degraded` (e.g., `TNFSetupJobDegraded`) +- Indicates a job has **failed OR is blocked** from running +- Reasons: + - `SyncError`: Job blocked (nodes not ready or no schedulable nodes) for >10 minutes + - `MaxRetriesExceeded`: Job exhausted all retry attempts in current cycle (e.g., 6 tries across 2 nodes × 3 attempts) +- **Blocked jobs** (SyncError): + - Set after 10-minute timeout when blocking condition persists + - Automatically cleared when condition resolves (nodes become ready, schedulable nodes available) + - Message contains details: "Affected nodes not ready: [master-0]" or "No schedulable nodes available" +- **Failed jobs** (MaxRetriesExceeded): + - Set immediately when max retry attempts exhausted in a cycle + - **Retry cycles continue** - after setting Degraded, state resets to attempt 1 and retries resume + - Remains set until the job succeeds + +All conditions are set on the **etcd Operator CR** and propagate to the **ClusterOperator CR** via the cluster status controller. This allows per-job granularity on the Operator CR while the ClusterOperator shows overall health state to cluster administrators. + +## Job Controller Framework + +### RunNodeJobController + +Entry point for starting node-specific job controllers. Provides duplicate prevention (safe to call repeatedly) and integrates with the operator's controller framework. + +**Characteristics:** +- Checks single node readiness before creating job +- Uses Kubernetes backoffLimit for retries on same node +- Examples: auth, after-setup + +**Signature:** +```go +RunNodeJobController(ctx, jobType, node, retries, ...) +``` + +### RunClusterJobController + +Entry point for starting cluster-wide job controllers with multi-node retry logic. + +**Characteristics:** +- Round-robin retry across schedulable nodes +- Config drift detection triggers job restart +- Uses backoffLimit=0 with manual retry state management +- Examples: setup, fencing + +**Signature:** +```go +RunClusterJobController(ctx, jobType, schedulableNodesFunc, affectedNodesFunc, jobConfigFunc, retries, ...) +``` + +### RestartClusterJobOrRunController + +Handles job restart logic with blocking cleanup: + +- Deletes existing job and waits for deletion (blocking) +- Starts new job controller after cleanup completes +- Used when job config changes or manual restart needed +- Only used for cluster-wide jobs (node jobs use drift detection instead) + +## Job Types and Characteristics + +**Node-Specific (single-node pattern):** +- **auth:** Sets hacluster password (required before Pacemaker operations) +- **after-setup:** Disables kubelet systemd service (Pacemaker owns it now) + +**Cluster-Wide (multi-node pattern):** +- **setup:** Creates Pacemaker cluster, marks transition complete. Returns success immediately if transition already complete (e.g., job recreated due to drift detection) +- **fencing:** Configures STONITH for BMC power control +- **update-setup:** Handles node replacements (removes offline node, adds new node, updates IPs). Validates: (1) Pacemaker cluster running on scheduled node, (2) exactly 2 nodes online after operations complete. Returns error to trigger retry if validation fails (e.g., auth hasn't run on new node yet) + +## Single-Node Job Pattern + +Single-node jobs run on a specific target node with Kubernetes-native retry (backoffLimit). Used for node-specific operations like setting auth credentials. + +**Execution Flow:** + +```text +RunNodeJobController(jobType, node, retries, ...) + │ + ├─ Create JobController with getJob hook + │ │ + │ └─ getJob hook (runs on every sync): + │ │ + │ ├─ Check node readiness via checkNodesReadinessAndSetCondition + │ │ ├─ Not ready < 10min → Return (skip job, retry on next sync) + │ │ ├─ Not ready ≥ 10min → Return error (triggers Degraded via WithSyncDegradedOnError) + │ │ └─ Ready → Clear Degraded condition (if was blocked), continue + │ │ + │ └─ Create job spec: + │ - Fetch fresh node from informer (handles node replacement) + │ - Pin to target node (NodeName fixed assignment) + │ - Label with node UID (job.Labels["node"] = string(node.UID)) + │ - Set backoffLimit=retries (Kubernetes handles retries) + │ + └─ JobController syncs every minute + - Complete → Clear conditions, done + - Failed → Framework sets Degraded condition +``` + +**Key Characteristics:** +- **No round-robin:** Job always runs on the same target node +- **Node replacement detection:** Detects when node is replaced (same name, new UID) via `node` label, triggers job recreation +- **Graceful cleanup:** When node is deleted, controller skips job application and waits for context cancellation +- **Kubernetes retries:** Uses Job's built-in backoffLimit mechanism + +**Node Replacement Handling:** + +Node-specific jobs handle node replacement by fetching fresh node data from the node informer on every sync and labeling the job with the node's UID: + +```go +// Fetch fresh node from informer to handle node replacement +freshNode, err := nodeLister.Get(node.Name) +if err != nil { + if apierrors.IsNotFound(err) { + // Node deleted - skip job application, wait for context cancellation + return false, nil + } + return false, err +} + +// Label job with node UID for drift detection +job.Labels["node"] = string(freshNode.UID) +``` + +This ensures: +- **Node replacement detected:** When a node is replaced (same name, different UID), the `node` label drift is detected by `ApplyJob`, triggering job deletion and recreation +- **Deleted nodes handled gracefully:** When a node is deleted entirely, the controller skips job application instead of erroring repeatedly +- **No stale data:** Fetching from the informer each sync prevents closure capture of stale node objects + +## Multi-Node Job Pattern + +Multi-node jobs implement round-robin retry across schedulable nodes with drift detection. Used for cluster-wide operations like setup and fencing. + +**Function Roles:** +- `schedulableNodesFunc`: Returns nodes where job can run (intersection of K8s Ready nodes and Pacemaker members). Changes trigger retry reset. See [Lifecycle Manager](lifecycle-manager.md#node-selection-logic) for implementation details. +- `affectedNodesFunc`: Returns nodes that must be ready before job runs (can include nodes being added). Job blocked if any not ready. +- `jobConfigFunc`: Returns config string for drift detection (generation, secret ResourceVersions). Changes trigger retry reset. + +**Execution Flow:** + +```text +RunClusterJobController(jobType, schedulableNodesFunc, affectedNodesFunc, jobConfigFunc, retries, ...) + │ + ├─ Create JobController with getJob hook + │ │ + │ └─ getJob hook (runs on every sync): + │ │ + │ ├─ Call syncMultiNodeJobState (manages retry state): + │ │ │ + │ │ ├─ Check affectedNodesFunc() → any nodes not ready? + │ │ │ ├─ YES → Return error after 10min (triggers Degraded via WithSyncDegradedOnError) + │ │ │ └─ NO → Clear Degraded condition (if was blocked), continue + │ │ │ + │ │ ├─ Check schedulableNodesFunc() → any nodes available? + │ │ │ ├─ NO → Return error after 10min (triggers Degraded via WithSyncDegradedOnError) + │ │ │ └─ YES → Clear Degraded condition (if was blocked), continue + │ │ │ + │ │ ├─ Get or initialize retry state (AttemptNumber, NodeIndex, config) + │ │ │ + │ │ ├─ Check schedulableNodesFunc() → nodes changed? + │ │ │ └─ YES → Reset state (attempt=1, index=0), delete job, return + │ │ │ + │ │ ├─ Check jobConfigFunc() → config changed? + │ │ │ └─ YES → Reset state (attempt=1, index=0), delete job, return + │ │ │ + │ │ ├─ Get current job from cluster + │ │ │ │ + │ │ │ ├─ Complete? → Clear degraded, preserve state, return + │ │ │ │ + │ │ │ └─ Failed? + │ │ │ ├─ Move to next node (NodeIndex++) + │ │ │ ├─ All nodes tried? → Increment attempt (AttemptNumber++) + │ │ │ ├─ Max attempts exhausted? → Set Degraded, reset to attempt 1 + │ │ │ └─ Update retry state (ApplyJob will detect retry field drift and delete/recreate) + │ │ │ + │ │ └─ ApplyJob detects drift (NodeName, node-index, or attempt labels changed) → Deletes and recreates job + │ │ + │ ├─ Check if retry state exists (skipped if nodes not ready) + │ │ └─ NO → Return nil (skip job creation, retry on next sync) + │ │ + │ └─ Call configureMultiNodeJob (configures job with retry state): + │ │ + │ ├─ Read retry state (NodeIndex, AttemptNumber) + │ ├─ Get schedulable nodes + │ ├─ Select node: schedulableNodes[NodeIndex] + │ └─ Create job spec: + │ - Schedule on selected node (nodeSelector) + │ - Set backoffLimit=0 (manual retry only) + │ + └─ JobController syncs every minute + - Complete → Clear conditions, done + - Failed → syncMultiNodeJobState advances to next node +``` + +**Round-Robin Retry:** +- **Attempt 1:** Try node 0 → fails → try node 1 → fails → continue +- **Attempt 2:** Try node 0 → fails → try node 1 → fails → continue +- **Attempt 3:** Try node 0 → fails → try node 1 → fails → **Degraded** +- **Total:** 6 tries (2 nodes × 3 attempts) with maxRetryAttempts=3 +- **After Degraded:** Reset to attempt 1, retry continues (allows recovery) + +**Job-Specific Validation:** +Cluster jobs may perform node-specific validation before and after executing their operations. For example, update-setup validates: +1. **Pre-execution:** Pacemaker cluster is running on the scheduled node (`pcs cluster status`). If not, returns error to trigger round-robin retry to find the node where cluster is running. +2. **Post-execution:** Exactly 2 nodes are online in the cluster (`pcs status xml`). If not, returns error to trigger retry, ensuring the job doesn't succeed until the new node is fully online (i.e., auth job has run and node joined cluster). + +This allows cluster-wide jobs to find the correct execution context and validate complete state without requiring explicit knowledge at scheduling time. + +**Drift Detection:** +When infrastructure changes (nodes added/removed, config updated), retry state resets: +- `schedulableNodesFunc` returns different nodes → Reset (Pacemaker membership changed) +- `jobConfigFunc` returns different config → Reset (infrastructure changed, e.g., secret rotation) +- Old job deleted, new job starts on first node with attempt 1 + +**ApplyJob Drift Handling:** +`ApplyJob` detects drift between existing and required job specs and handles recreation: +- **Non-failed cluster jobs (completed or running):** Drift in retry fields (NodeName, node-index, attempt labels) is ignored to prevent recreation on operator restart when retry state is reinitialized +- **Non-failed cluster jobs with real drift:** Image/command/config changes trigger recreation even for non-failed jobs +- **Failed cluster jobs:** Any drift (including NodeName from retry) triggers delete/recreate for round-robin retry + +**Config String Components:** +- Generation counters (detect config changes) +- Node UIDs (detect node replacements) +- Secret ResourceVersions (detect credential rotation and data updates) + +**Key Characteristics:** +- **Round-robin:** Tries all schedulable nodes before giving up +- **Drift detection:** Automatically restarts job when config changes +- **Manual retry:** Uses backoffLimit=0, framework manages retry state +- **Admission control:** Blocks job creation until all affected nodes ready + +**Why Round-Robin Instead of Sticky-on-Success:** +Cluster jobs use round-robin retry **only on failure**. Once a job succeeds, it's done—there's no retry needed. This differs from the status collector CronJob which runs continuously and uses sticky-on-success (if a node succeeded last time, it's likely to succeed again). For one-time success jobs, there is no "next time" after success, so retry logic only applies to failures. + +## Related Files + +**Job Controller Framework:** +- **[pkg/tnf/pkg/jobs/lifecycle.go](../../pkg/tnf/pkg/jobs/lifecycle.go)** - Core job controller framework (RunNodeJobController, RunClusterJobController, retry logic) +- **[pkg/tnf/pkg/jobs/jobcontroller.go](../../pkg/tnf/pkg/jobs/jobcontroller.go)** - JobController type and sync implementation +- **[pkg/tnf/pkg/jobs/utils.go](../../pkg/tnf/pkg/jobs/utils.go)** - Job status helpers and utility functions +- **[pkg/tnf/pkg/tools/jobs.go](../../pkg/tnf/pkg/tools/jobs.go)** - JobType definitions, timeout constants, ToPascalCase + +**Lifecycle Manager:** +- **[lifecycle-manager.md](lifecycle-manager.md)** - PacemakerLifecycleManager, node selection, status collector +- **[pkg/tnf/operator/lifecycle_manager.go](../../pkg/tnf/operator/lifecycle_manager.go)** - Lifecycle controller sync loop, node event handlers +- **[pkg/tnf/operator/helpers.go](../../pkg/tnf/operator/helpers.go)** - getActivePacemakerNodes(), intersection logic +- **[pkg/tnf/operator/status_collector.go](../../pkg/tnf/operator/status_collector.go)** - Status collector CronJob +- **[pkg/tnf/operator/job_controllers.go](../../pkg/tnf/operator/job_controllers.go)** - Job controller startup logic + +**Job Implementations:** +- **[pkg/tnf/setup/runner.go](../../pkg/tnf/setup/runner.go)** - Setup job implementation (creates Pacemaker cluster) +- **[pkg/tnf/update-setup/runner.go](../../pkg/tnf/update-setup/runner.go)** - Update-setup job implementation (node replacement orchestration). Validates Pacemaker cluster running on scheduled node before proceeding, and validates exactly 2 nodes online after completion; returns error to trigger retry otherwise diff --git a/docs/tnf/lifecycle-manager.md b/docs/tnf/lifecycle-manager.md new file mode 100644 index 0000000000..ab9b5f23b1 --- /dev/null +++ b/docs/tnf/lifecycle-manager.md @@ -0,0 +1,300 @@ +# Controller Lifecycle + +## Overview + +The PacemakerLifecycleManager is a Kubernetes controller that runs continuously as part of the cluster-etcd-operator. It manages TNF job controller startup and provides shared logic for determining which nodes are valid targets for Pacemaker operations. + +## Responsibilities + +The lifecycle manager ([pkg/tnf/operator/lifecycle_manager.go](../../pkg/tnf/operator/lifecycle_manager.go)) handles: + +1. **Job Controller Startup** - Starts job controllers when conditions are met (bootstrap or runtime mode) +2. **Node Selection** - Provides `schedulableNodesFunc` for job controllers (intersection logic) +3. **Status Collection** - Runs CronJob to collect Pacemaker status and populate PacemakerCluster CR +4. **Bootstrap Detection** - Responds to node Ready transitions during initial cluster setup + +## Startup Sequence + +```text +┌──────────────────────────────────────────────────────────────┐ +│ cluster-etcd-operator starts │ +└──────────────────────────────────────────────────────────────┘ + │ + ▼ + ┌────────────────────────────────┐ + │ Static resource controller │ + │ applies PacemakerCluster CRD │ + └────────────────────────────────┘ + │ + ▼ + ┌────────────────────────────────┐ + │ runPacemakerControllers() │ + │ (background goroutine) │ + └────────────────────────────────┘ + │ + ▼ + ┌────────────────────────────────┐ + │ Wait for CRD to be established │ + │ (polls with backoff) │ + └────────────────────────────────┘ + │ + ▼ + ┌────────────────────────────────┐ + │ newPacemakerLifecycleManager() │ + │ - Creates REST client │ + │ - Creates PacemakerCluster │ + │ informer (resync: 1min) │ + │ - Registers node event handlers│ + └────────────────────────────────┘ + │ + ▼ + ┌────────────────────────────────┐ + │ Start PacemakerCluster informer│ + │ (go pacemakerInformer.Run()) │ + └────────────────────────────────┘ + │ + ▼ + ┌────────────────────────────────┐ + │ Start lifecycle controller │ + │ (go lifecycleController.Run()) │ + │ Syncs every 1 minute │ + └────────────────────────────────┘ + │ + ▼ + ┌────────────────────────────────┐ + │ Start status collector CronJob │ + │ (runs every 1 minute) │ + └────────────────────────────────┘ +``` + +**Key Points:** +- Lifecycle manager starts regardless of external etcd transition state +- The `sync()` function handles both bootstrap and runtime modes internally +- CRD must be established before informers can start +- Controller syncs every 1 minute to ensure job controllers are running + +## Sync Loop + +The controller's `sync()` function runs every 1 minute and on informer events: + +```text +sync() called (every 1 minute + on events) + │ + ▼ +startJobControllers() + │ + ├─ Check: External etcd transition complete? + │ + ├─ NO (Bootstrap Mode): + │ │ + │ ├─ Wait for exactly 2 ready control plane nodes + │ ├─ Wait for etcd bootstrap to complete + │ ├─ Start: auth jobs (per-node) + │ ├─ Start: setup job (cluster-wide, one-time) + │ ├─ Start: fencing job (cluster-wide) + │ └─ Wait for setup job completion → mark transition complete + │ + └─ YES (Runtime Mode): + │ + ├─ Ensure auth/after-setup controllers running (per-node) + ├─ Ensure update-setup controller running (if 2 nodes) + └─ Ensure fencing controller running (cluster-wide) +``` + +See [Job Controllers](job-controllers.md) for details on job execution patterns and retry logic. + +## Node Selection Logic + +### getActivePacemakerNodes() + +Job controllers need to know which nodes are valid targets for Pacemaker operations. The `getActivePacemakerNodes()` function (in [pkg/tnf/operator/helpers.go](../../pkg/tnf/operator/helpers.go)) provides this logic: + +```text +Get all K8s control plane nodes + │ + ▼ +Filter to Ready nodes only + │ + ▼ +Try to get Pacemaker nodes from PacemakerCluster CR + │ + ▼ +CR exists and fresh (age <= 5min)? + │ + ├─ YES: + │ │ + │ ├─ Calculate intersection (K8s Ready ∩ Pacemaker) + │ │ + │ └─ Intersection not empty? + │ ├─ YES → Return intersection (sorted by name) + │ └─ NO → Fall back to all ready K8s nodes + │ + └─ NO (CR missing or stale): + │ + └─ Return all ready K8s nodes (sorted by name) +``` + +**Staleness Check:** +- PacemakerCluster CR is considered stale if `Status.LastUpdated` age is > 5 minutes +- Stale CR indicates status collector isn't running or Pacemaker isn't responding +- Graceful degradation: fall back to all ready nodes rather than blocking operations + +**Intersection Logic:** +- Normal operation: Jobs only run on nodes in BOTH Kubernetes AND Pacemaker +- Prevents targeting nodes that haven't joined Pacemaker yet +- Prevents targeting nodes that have left Pacemaker but still exist in K8s +- Graceful degradation (stale/missing CR): Falls back to all ready K8s nodes, waiving the both-systems requirement to prevent blocking operations + +**Deterministic Ordering:** +- Nodes are always sorted by name before returning (in all code paths) +- Critical for round-robin retry logic (NodeIndex must point to same node across syncs) +- Sorting happens directly in `getActivePacemakerNodes()` and `getIntersection()` + +## Status Collector CronJob + +### Overview + +The PacemakerCluster CR is populated by a status collector CronJob that runs every minute: + +```text +Every 1 minute: + │ + ▼ +┌────────────────────────────────┐ +│ Status Collector Job │ +│ (pod scheduled on one node) │ +└────────────────────────────────┘ + │ + ▼ +Run: sudo -n pcs status xml + │ + ▼ +Parse XML into structured data + │ + ▼ +Update PacemakerCluster CR .status: +- state (online/offline/standby) +- lastUpdated (timestamp) +- nodes[] (name, status, IP) +``` + +### Node Rotation Strategy + +The status collector uses **sticky-on-success, rotate-on-failure** node selection to maximize success while providing automatic failover: + +**Implementation:** +- Maintains in-memory `JobRetryState` with `NodeIndex` +- Nodes are sorted deterministically by name +- Strategy: + - **Job succeeded** → Keep same node (sticky behavior, minimize overhead) + - **Job failed** → Rotate to next node in sorted list + - **Node list changed** → Reset to first node + +**Example Flow (2 nodes: master-0, master-1):** +```text +Sync 1: No job exists → Schedule on master-0 (index 0) +Sync 2: Job succeeded → Stay on master-0 +Sync 3: Job succeeded → Stay on master-0 +Sync 4: Job failed → Rotate to master-1 (index 1) +Sync 5: Job succeeded → Stay on master-1 +Sync 6: Job succeeded → Stay on master-1 +``` + +**Failure Detection:** +- Detects jobs with `Failed` or `FailureTarget` conditions (status=True) +- `FailureTarget` is a newer Kubernetes condition (1.31+) for jobs exceeding `activeDeadlineSeconds` +- Failed jobs are automatically deleted after rotation state is updated +- Deletion unblocks the CronJob's `concurrencyPolicy: Forbid` to allow next run + +**CronJob Concurrency:** +- Uses `concurrencyPolicy: Forbid` to prevent overlapping jobs +- When a job fails (especially with FailureTarget on a node with kubelet issues): + 1. CronJob hook detects failure condition + 2. Updates rotation state to next node + 3. Deletes failed job (with background propagation policy) + 4. Next CronJob schedule creates new job on rotated node +- Without deletion, Forbid policy would block new jobs until terminating pods cleanup (can be indefinite if kubelet down) + +**Why This Strategy:** +- Minimizes unnecessary job churn (don't rotate on success) +- Automatic failover when node has issues (rotate on failure) +- No retry budget exhaustion (status collector doesn't set Degraded) +- Staleness detection happens via CR timestamp check instead + +See [pkg/tnf/operator/status_collector.go](../../pkg/tnf/operator/status_collector.go) for implementation. + +## Node Event Handlers + +The lifecycle manager registers an `UpdateFunc` event handler on the node informer to trigger update-setup job restarts: + +### UpdateFunc (Node Ready Transition) + +```text +Node updated + │ + ▼ +Not Ready → Ready transition? + │ + ┌───┴───┐ + No Yes + │ │ + ▼ ▼ + Ignore Trigger: restartUpdateSetupJob() + │ + ▼ + (Goroutine spawned) + │ + ▼ + Check transition complete? + │ + ┌───┴───┐ + No Yes (post-transition only) + │ │ + ▼ ▼ + Skip Check 2 ready nodes? + │ + ┌───┴───┐ + No Yes + │ │ + ▼ ▼ + Skip Restart update-setup job +``` + +**Why Goroutine?** +- Event handlers must return quickly +- Job restart involves blocking cleanup (delete, wait for deletion) +- Prevents blocking the informer event loop + +**Use Case:** +- Node becomes ready after replacement → update-setup job is restarted to handle node replacement operations +- Without this event handler, update-setup would remain in Complete state even if it finished before auth ran on the new node +- The restart ensures update-setup reruns its validation (expects exactly 2 online nodes in Pacemaker) + +## Integration with Job Controllers + +The lifecycle manager provides `schedulableNodesFunc` to job controllers: + +```go +schedulableNodesFunc := func() ([]*corev1.Node, error) { + return lifecycleManager.getActivePacemakerNodes() +} +``` + +This function is passed to: +- `RunClusterJobController()` for setup, update-setup, and fencing jobs +- Status collector CronJob for node pinning + +Job controllers call this function to: +1. Determine which nodes to target for round-robin retry +2. Detect node list changes (triggers retry state reset) +3. Ensure jobs only run on valid Pacemaker members + +See [Job Controllers - Multi-Node Job Pattern](job-controllers.md#multi-node-job-pattern) for how `schedulableNodesFunc` integrates with retry logic. + +## Related Files + +- **[pkg/tnf/operator/lifecycle_manager.go](../../pkg/tnf/operator/lifecycle_manager.go)** - PacemakerLifecycleManager controller, sync loop, node event handlers +- **[pkg/tnf/operator/helpers.go](../../pkg/tnf/operator/helpers.go)** - getActivePacemakerNodes(), intersection logic, staleness checking, node sorting +- **[pkg/tnf/operator/status_collector.go](../../pkg/tnf/operator/status_collector.go)** - Status collector CronJob, node rotation logic +- **[pkg/tnf/operator/job_controllers.go](../../pkg/tnf/operator/job_controllers.go)** - startJobControllers(), bootstrap vs runtime mode +- **[pkg/tnf/pkg/tools/nodes.go](../../pkg/tnf/pkg/tools/nodes.go)** - Node helpers (IsNodeReady, GetNodeNames, StringSlicesEqual, ListNodesFromInformer) diff --git a/pkg/tnf/auth/runner.go b/pkg/tnf/auth/runner.go index e583fb7f3d..615b6b2adf 100644 --- a/pkg/tnf/auth/runner.go +++ b/pkg/tnf/auth/runner.go @@ -49,8 +49,7 @@ func RunTnfAuth() error { klog.Info("Running TNF auth") - // create tnf cluster config - cfg, err := config.GetClusterConfig(ctx, kubeClient) + cfg, err := config.GetClusterConfigIgnoreMissingNode(ctx, kubeClient) if err != nil { klog.Errorf("Failed to get cluster config: %v", err) return err diff --git a/pkg/tnf/operator/helpers.go b/pkg/tnf/operator/helpers.go new file mode 100644 index 0000000000..803b7cd41a --- /dev/null +++ b/pkg/tnf/operator/helpers.go @@ -0,0 +1,136 @@ +package operator + +import ( + "fmt" + "sort" + "time" + + corev1 "k8s.io/api/core/v1" + "k8s.io/klog/v2" + + pacmkrv1 "github.com/openshift/api/etcd/v1" + "github.com/openshift/cluster-etcd-operator/pkg/tnf/pkg/pacemaker" + "github.com/openshift/cluster-etcd-operator/pkg/tnf/pkg/tools" +) + +const ( + // pacemakerCRStalenessThreshold is how long before a PacemakerCluster CR status is considered stale. + // Status collector runs every minute, so 5 minutes means we've missed ~5 consecutive updates. + pacemakerCRStalenessThreshold = 5 * time.Minute +) + +// getActivePacemakerNodes returns nodes eligible for job scheduling. +// Returns K8s ∩ Pacemaker intersection (ready nodes only) if CR is fresh. +// Falls back to all ready control plane nodes if CR unavailable or stale. +func (c *pacemakerLifecycleManager) getActivePacemakerNodes() ([]*corev1.Node, error) { + if c.controlPlaneNodeInformer == nil || !c.controlPlaneNodeInformer.HasSynced() { + return nil, fmt.Errorf("node informer not synced yet") + } + + k8sNodes, err := tools.ListNodesFromInformer(c.controlPlaneNodeInformer) + if err != nil { + return nil, fmt.Errorf("failed to list control plane nodes: %w", err) + } + + readyNodes := []*corev1.Node{} + for _, node := range k8sNodes { + if tools.IsNodeReady(node) { + readyNodes = append(readyNodes, node) + } + } + + if len(readyNodes) == 0 { + return nil, fmt.Errorf("no ready control plane nodes found") + } + + pacemakerNodes, pacemakerCR, err := c.getPacemakerNodesWithCR() + pacemakerNodesAvailable := (err == nil && !isPacemakerCRStale(pacemakerCR)) + + if pacemakerNodesAvailable { + intersection := c.getIntersection(readyNodes, pacemakerNodes) + if len(intersection) > 0 { + klog.V(4).Infof("Valid targets (intersection, ready only): %v", tools.GetNodeNames(intersection)) + return intersection, nil + } + klog.V(4).Infof("No nodes in intersection (K8s ∩ pacemaker) - using all ready nodes") + } else { + if err != nil { + klog.V(4).Infof("PacemakerCluster CR not available: %v - using all ready nodes", err) + } else { + klog.V(4).Infof("PacemakerCluster CR status is stale (last updated %v) - using all ready nodes", pacemakerCR.Status.LastUpdated) + } + } + + // Sort for deterministic ordering (consistent with intersection path) + sort.Slice(readyNodes, func(i, j int) bool { + return readyNodes[i].Name < readyNodes[j].Name + }) + + klog.V(4).Infof("Valid targets (all ready nodes): %v", tools.GetNodeNames(readyNodes)) + return readyNodes, nil +} + +// getPacemakerNodesWithCR retrieves pacemaker node information from the PacemakerCluster CR. +// Returns a map of nodeName -> IP address and the CR itself. +func (c *pacemakerLifecycleManager) getPacemakerNodesWithCR() (map[string]string, *pacmkrv1.PacemakerCluster, error) { + if c.pacemakerInformer == nil { + return nil, nil, fmt.Errorf("pacemakerInformer is nil") + } + + // Note: We don't check HasSynced() here because the watch stream sometimes fails with decode errors + // even though the cache is populated via List. The cache will be refreshed on resync interval. + item, exists, err := c.pacemakerInformer.GetStore().GetByKey(pacemaker.PacemakerClusterResourceName) + if err != nil { + return nil, nil, fmt.Errorf("failed to get PacemakerCluster from cache: %w", err) + } + if !exists { + return nil, nil, fmt.Errorf("PacemakerCluster CR not found") + } + + pacemakerCR, ok := item.(*pacmkrv1.PacemakerCluster) + if !ok { + return nil, nil, fmt.Errorf("failed to convert to PacemakerCluster") + } + + if pacemakerCR.Status.Nodes == nil { + return nil, pacemakerCR, fmt.Errorf("PacemakerCluster CR has no nodes in status") + } + + pmNodes := make(map[string]string) + for _, node := range *pacemakerCR.Status.Nodes { + if len(node.Addresses) == 0 { + klog.Warningf("Pacemaker node %q has no addresses in CR status - skipping (possible CR population race)", node.NodeName) + continue + } + pmNodes[node.NodeName] = node.Addresses[0].Address + } + + return pmNodes, pacemakerCR, nil +} + +// getIntersection returns nodes that exist in BOTH K8s and pacemaker. +// Returns nodes sorted by name for deterministic ordering. +func (c *pacemakerLifecycleManager) getIntersection(k8sNodes []*corev1.Node, pacemakerNodes map[string]string) []*corev1.Node { + intersection := []*corev1.Node{} + for _, k8sNode := range k8sNodes { + if _, exists := pacemakerNodes[k8sNode.Name]; exists { + intersection = append(intersection, k8sNode) + } + } + // Sort by name for deterministic target selection + sort.Slice(intersection, func(i, j int) bool { + return intersection[i].Name < intersection[j].Name + }) + return intersection +} + +// isPacemakerCRStale checks if the PacemakerCluster CR status is stale (hasn't been updated recently). +// A stale CR indicates the status collector isn't running or pacemaker isn't responding. +func isPacemakerCRStale(cr *pacmkrv1.PacemakerCluster) bool { + if cr == nil { + return true + } + + timeSinceUpdate := time.Since(cr.Status.LastUpdated.Time) + return timeSinceUpdate > pacemakerCRStalenessThreshold +} diff --git a/pkg/tnf/operator/helpers_test.go b/pkg/tnf/operator/helpers_test.go new file mode 100644 index 0000000000..aa78d218a9 --- /dev/null +++ b/pkg/tnf/operator/helpers_test.go @@ -0,0 +1,415 @@ +package operator + +/* +TEST COVERAGE SUMMARY - helpers_test.go +======================================== + +This file tests pacemaker node selection logic for job targeting. + +WHAT'S TESTED +------------- + +Node Selection (getActivePacemakerNodes): +├── Happy path - K8s ∩ Pacemaker intersection +│ ├── Both nodes in K8s and Pacemaker → return intersection +│ └── One node in intersection → return it +├── CR unavailable cases +│ ├── PacemakerCluster CR doesn't exist → return all ready K8s nodes +│ └── CR is stale (>5min old) → return all ready K8s nodes +├── Edge cases +│ ├── No intersection (K8s nodes not in Pacemaker) → fall back to all ready nodes +│ ├── No ready nodes → error +│ └── Node informer not synced → error +└── Node readiness filtering + ├── Some nodes not ready → filter to ready only + └── All nodes ready → return all +*/ + +import ( + "context" + "testing" + "time" + + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/tools/cache" + + pacmkrv1 "github.com/openshift/api/etcd/v1" + "github.com/openshift/cluster-etcd-operator/pkg/tnf/pkg/pacemaker" +) + +func TestGetActivePacemakerNodes(t *testing.T) { + tests := []struct { + name string + k8sNodes []*corev1.Node + pacemakerCR *pacmkrv1.PacemakerCluster + controlPlaneNodeInformerSynced bool + expectedNodeNames []string + expectError bool + errorContains string + }{ + { + name: "happy path - both nodes in K8s and Pacemaker intersection", + k8sNodes: []*corev1.Node{ + makeReadyNode("master-0", "10.0.0.1"), + makeReadyNode("master-1", "10.0.0.2"), + }, + pacemakerCR: makePacemakerCR( + map[string]string{ + "master-0": "10.0.0.1", + "master-1": "10.0.0.2", + }, + time.Now(), // Fresh CR + ), + controlPlaneNodeInformerSynced: true, + expectedNodeNames: []string{"master-0", "master-1"}, + expectError: false, + }, + { + name: "one node in intersection - returns that node", + k8sNodes: []*corev1.Node{ + makeReadyNode("master-0", "10.0.0.1"), + makeReadyNode("master-1", "10.0.0.2"), + }, + pacemakerCR: makePacemakerCR( + map[string]string{ + "master-0": "10.0.0.1", + // master-1 not in pacemaker yet + }, + time.Now(), + ), + controlPlaneNodeInformerSynced: true, + expectedNodeNames: []string{"master-0"}, + expectError: false, + }, + { + name: "CR doesn't exist - returns all ready K8s nodes", + k8sNodes: []*corev1.Node{ + makeReadyNode("master-0", "10.0.0.1"), + makeReadyNode("master-1", "10.0.0.2"), + }, + pacemakerCR: nil, // CR doesn't exist + controlPlaneNodeInformerSynced: true, + expectedNodeNames: []string{"master-0", "master-1"}, + expectError: false, + }, + { + name: "CR is stale - returns all ready K8s nodes", + k8sNodes: []*corev1.Node{ + makeReadyNode("master-0", "10.0.0.1"), + makeReadyNode("master-1", "10.0.0.2"), + }, + pacemakerCR: makePacemakerCR( + map[string]string{ + "master-0": "10.0.0.1", + }, + time.Now().Add(-10*time.Minute), // Stale (>5min) + ), + controlPlaneNodeInformerSynced: true, + expectedNodeNames: []string{"master-0", "master-1"}, + expectError: false, + }, + { + name: "no intersection - falls back to all ready nodes", + k8sNodes: []*corev1.Node{ + makeReadyNode("master-2", "10.0.0.3"), // Different nodes in K8s + makeReadyNode("master-3", "10.0.0.4"), + }, + pacemakerCR: makePacemakerCR( + map[string]string{ + "master-0": "10.0.0.1", // Different nodes in Pacemaker + "master-1": "10.0.0.2", + }, + time.Now(), + ), + controlPlaneNodeInformerSynced: true, + expectedNodeNames: []string{"master-2", "master-3"}, // All ready nodes + expectError: false, + }, + { + name: "filters out not-ready nodes", + k8sNodes: []*corev1.Node{ + makeReadyNode("master-0", "10.0.0.1"), + makeNotReadyNode("master-1", "10.0.0.2"), + }, + pacemakerCR: makePacemakerCR( + map[string]string{ + "master-0": "10.0.0.1", + "master-1": "10.0.0.2", + }, + time.Now(), + ), + controlPlaneNodeInformerSynced: true, + expectedNodeNames: []string{"master-0"}, // Only ready node + expectError: false, + }, + { + name: "no ready nodes - error", + k8sNodes: []*corev1.Node{ + makeNotReadyNode("master-0", "10.0.0.1"), + makeNotReadyNode("master-1", "10.0.0.2"), + }, + pacemakerCR: nil, + controlPlaneNodeInformerSynced: true, + expectedNodeNames: nil, + expectError: true, + errorContains: "no ready control plane nodes found", + }, + { + name: "node informer not synced - error", + k8sNodes: []*corev1.Node{ + makeReadyNode("master-0", "10.0.0.1"), + }, + pacemakerCR: nil, + controlPlaneNodeInformerSynced: false, + expectedNodeNames: nil, + expectError: true, + errorContains: "node informer not synced yet", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Setup node informer + nodeIndexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{}) + for _, node := range tt.k8sNodes { + require.NoError(t, nodeIndexer.Add(node)) + } + controlPlaneNodeInformer := &mockNodeInformer{ + indexer: nodeIndexer, + synced: tt.controlPlaneNodeInformerSynced, + } + + // Setup pacemaker informer + pacemakerIndexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{}) + if tt.pacemakerCR != nil { + require.NoError(t, pacemakerIndexer.Add(tt.pacemakerCR)) + } + pacemakerInformer := &mockPacemakerInformer{ + indexer: pacemakerIndexer, + } + + // Create manager + manager := &pacemakerLifecycleManager{ + controlPlaneNodeInformer: controlPlaneNodeInformer, + pacemakerInformer: pacemakerInformer, + } + + // Execute + result, err := manager.getActivePacemakerNodes() + + // Verify + if tt.expectError { + require.Error(t, err) + if tt.errorContains != "" { + require.Contains(t, err.Error(), tt.errorContains) + } + } else { + require.NoError(t, err) + actualNodeNames := make([]string, len(result)) + for i, node := range result { + actualNodeNames[i] = node.Name + } + require.ElementsMatch(t, tt.expectedNodeNames, actualNodeNames) + } + }) + } +} + +// Test helpers + +func makeReadyNode(name, ip string) *corev1.Node { + return &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + }, + Status: corev1.NodeStatus{ + Addresses: []corev1.NodeAddress{ + {Type: corev1.NodeInternalIP, Address: ip}, + }, + Conditions: []corev1.NodeCondition{ + { + Type: corev1.NodeReady, + Status: corev1.ConditionTrue, + }, + }, + }, + } +} + +func makeNotReadyNode(name, ip string) *corev1.Node { + return &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + }, + Status: corev1.NodeStatus{ + Addresses: []corev1.NodeAddress{ + {Type: corev1.NodeInternalIP, Address: ip}, + }, + Conditions: []corev1.NodeCondition{ + { + Type: corev1.NodeReady, + Status: corev1.ConditionFalse, + }, + }, + }, + } +} + +func makePacemakerCR(nodes map[string]string, lastUpdated time.Time) *pacmkrv1.PacemakerCluster { + var statusNodes []pacmkrv1.PacemakerClusterNodeStatus + for nodeName, ip := range nodes { + statusNodes = append(statusNodes, pacmkrv1.PacemakerClusterNodeStatus{ + NodeName: nodeName, + Addresses: []pacmkrv1.PacemakerNodeAddress{ + {Address: ip}, + }, + }) + } + + return &pacmkrv1.PacemakerCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: pacemaker.PacemakerClusterResourceName, + }, + Status: pacmkrv1.PacemakerClusterStatus{ + Nodes: &statusNodes, + LastUpdated: metav1.NewTime(lastUpdated), + }, + } +} + +// Mock informers + +type mockNodeInformer struct { + indexer cache.Indexer + synced bool +} + +func (m *mockNodeInformer) GetIndexer() cache.Indexer { + return m.indexer +} + +func (m *mockNodeInformer) HasSynced() bool { + return m.synced +} + +func (m *mockNodeInformer) AddEventHandler(handler cache.ResourceEventHandler) (cache.ResourceEventHandlerRegistration, error) { + return nil, nil +} + +func (m *mockNodeInformer) AddEventHandlerWithResyncPeriod(handler cache.ResourceEventHandler, resyncPeriod time.Duration) (cache.ResourceEventHandlerRegistration, error) { + return nil, nil +} + +func (m *mockNodeInformer) RemoveEventHandler(handle cache.ResourceEventHandlerRegistration) error { + return nil +} + +func (m *mockNodeInformer) GetStore() cache.Store { + return m.indexer +} + +func (m *mockNodeInformer) GetController() cache.Controller { + return nil +} + +func (m *mockNodeInformer) Run(stopCh <-chan struct{}) { +} + +func (m *mockNodeInformer) LastSyncResourceVersion() string { + return "" +} + +func (m *mockNodeInformer) SetWatchErrorHandler(handler cache.WatchErrorHandler) error { + return nil +} + +func (m *mockNodeInformer) SetTransform(f cache.TransformFunc) error { + return nil +} + +func (m *mockNodeInformer) IsStopped() bool { + return false +} + +type mockPacemakerInformer struct { + indexer cache.Indexer +} + +func (m *mockPacemakerInformer) GetStore() cache.Store { + return m.indexer +} + +func (m *mockPacemakerInformer) GetIndexer() cache.Indexer { + return m.indexer +} + +func (m *mockPacemakerInformer) AddEventHandler(handler cache.ResourceEventHandler) (cache.ResourceEventHandlerRegistration, error) { + return nil, nil +} + +func (m *mockPacemakerInformer) AddEventHandlerWithResyncPeriod(handler cache.ResourceEventHandler, resyncPeriod time.Duration) (cache.ResourceEventHandlerRegistration, error) { + return nil, nil +} + +func (m *mockPacemakerInformer) RemoveEventHandler(handle cache.ResourceEventHandlerRegistration) error { + return nil +} + +func (m *mockPacemakerInformer) HasSynced() bool { + return true +} + +func (m *mockPacemakerInformer) Run(stopCh <-chan struct{}) { +} + +func (m *mockPacemakerInformer) LastSyncResourceVersion() string { + return "" +} + +func (m *mockPacemakerInformer) GetController() cache.Controller { + return nil +} + +func (m *mockPacemakerInformer) SetWatchErrorHandler(handler cache.WatchErrorHandler) error { + return nil +} + +func (m *mockPacemakerInformer) SetTransform(f cache.TransformFunc) error { + return nil +} + +func (m *mockPacemakerInformer) IsStopped() bool { + return false +} + +func (m *mockPacemakerInformer) AddEventHandlerWithOptions(handler cache.ResourceEventHandler, options cache.HandlerOptions) (cache.ResourceEventHandlerRegistration, error) { + return nil, nil +} + +func (m *mockPacemakerInformer) AddIndexers(indexers cache.Indexers) error { + return nil +} + +func (m *mockPacemakerInformer) RunWithContext(ctx context.Context) { +} + +func (m *mockPacemakerInformer) SetWatchErrorHandlerWithContext(handler cache.WatchErrorHandlerWithContext) error { + return nil +} + +func (m *mockNodeInformer) AddEventHandlerWithOptions(handler cache.ResourceEventHandler, options cache.HandlerOptions) (cache.ResourceEventHandlerRegistration, error) { + return nil, nil +} + +func (m *mockNodeInformer) AddIndexers(indexers cache.Indexers) error { + return nil +} + +func (m *mockNodeInformer) RunWithContext(ctx context.Context) { +} + +func (m *mockNodeInformer) SetWatchErrorHandlerWithContext(handler cache.WatchErrorHandlerWithContext) error { + return nil +} diff --git a/pkg/tnf/operator/job_controllers.go b/pkg/tnf/operator/job_controllers.go new file mode 100644 index 0000000000..44f7615e63 --- /dev/null +++ b/pkg/tnf/operator/job_controllers.go @@ -0,0 +1,473 @@ +package operator + +import ( + "context" + "encoding/json" + "fmt" + "sort" + "strings" + "time" + + operatorv1 "github.com/openshift/api/operator/v1" + operatorv1informers "github.com/openshift/client-go/operator/informers/externalversions/operator/v1" + "github.com/openshift/library-go/pkg/controller/controllercmd" + "github.com/openshift/library-go/pkg/operator/v1helpers" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/client-go/kubernetes" + "k8s.io/client-go/rest" + "k8s.io/client-go/tools/cache" + "k8s.io/klog/v2" + + "github.com/openshift/cluster-etcd-operator/pkg/operator/bootstrapteardown" + "github.com/openshift/cluster-etcd-operator/pkg/operator/ceohelpers" + "github.com/openshift/cluster-etcd-operator/pkg/operator/operatorclient" + "github.com/openshift/cluster-etcd-operator/pkg/tnf/pkg/etcd" + "github.com/openshift/cluster-etcd-operator/pkg/tnf/pkg/jobs" + "github.com/openshift/cluster-etcd-operator/pkg/tnf/pkg/tools" +) + +const ( + // Operator condition types + conditionTypeTNFJobControllersDegraded = "TNFJobControllersDegraded" +) + +var ( + // startTnfJobcontrollersFunc is a variable to allow mocking in tests + startTnfJobcontrollersFunc = startTnfJobcontrollers + + // retryBackoffConfig allows customizing retry behavior for tests + retryBackoffConfig = wait.Backoff{ + Duration: 5 * time.Second, + Factor: 2.0, + Steps: 9, // ~10 minutes total: 5s + 10s + 20s + 40s + 80s + 120s + 120s + 120s + 120s + Cap: 2 * time.Minute, + } +) + +// startJobControllers starts TNF job controllers based on transition state. +// +// Before ExternalEtcdTransitionCompleted: +// - Requires exactly 2 ready control plane nodes +// - Uses exponential backoff retry (5s to 2min, ~10 min total) +// - Sets TNFJobControllersDegraded condition on failure +// - Mutex protects concurrent retry attempts +// +// After ExternalEtcdTransitionCompleted: +// - Accepts any number of ready control plane nodes (handles single-node case) +// - Idempotent (safe to call repeatedly, handles operator restarts) +// - No retry logic (controller framework retries sync() on error) +// - No mutex needed (job-level locking provides adequate protection) +// +// The job controllers are idempotent and will not create duplicate jobs. +func (c *pacemakerLifecycleManager) startJobControllers(ctx context.Context) error { + transitionComplete, err := ceohelpers.HasExternalEtcdCompletedTransition(ctx, c.operatorClient) + if err != nil { + return fmt.Errorf("failed to check external etcd transition status: %w", err) + } + + if c.controlPlaneNodeInformer == nil || !c.controlPlaneNodeInformer.HasSynced() { + klog.V(4).Infof("Skipping job controller startup - node informer not synced yet") + return nil + } + + // TNF and arbiters are mutually exclusive - no filtering needed + controlPlaneNodes, err := tools.ListNodesFromInformer(c.controlPlaneNodeInformer) + if err != nil { + return fmt.Errorf("failed to list control plane nodes: %w", err) + } + if !transitionComplete { + // Pacemaker only supports 2 control plane nodes + if len(controlPlaneNodes) > 2 { + return fmt.Errorf("TNF requires exactly 2 control plane nodes for initial setup, found %d - pacemaker does not support >2 nodes", len(controlPlaneNodes)) + } + if len(controlPlaneNodes) < 2 { + klog.V(4).Infof("Waiting for 2 control plane nodes for initial setup (current: %d)", len(controlPlaneNodes)) + return nil + } + + for _, node := range controlPlaneNodes { + if !tools.IsNodeReady(node) { + klog.V(4).Infof("Control plane node %s not Ready - waiting for both nodes before initial setup", node.Name) + return nil + } + } + + klog.V(2).Infof("Both control plane nodes ready - starting initial job controllers with retry") + return c.retryInitialTransitionOrDegrade(ctx, controlPlaneNodes) + } else { + // Called on every sync to handle missed node events + if len(controlPlaneNodes) == 0 { + klog.V(4).Infof("No control plane nodes found, skipping job controller startup") + return nil + } + + // Note: Node readiness checked at job admission level (not controller startup). + // Jobs report TNFDegraded if affected nodes not ready or no schedulable nodes (after 10 min timeout). + + klog.V(4).Infof("Transition complete - ensuring job controllers running for %d control plane nodes", len(controlPlaneNodes)) + err = c.startJobControllersWithLock(ctx, controlPlaneNodes) + if err != nil { + return err + } + + return nil + } +} + +// retryInitialTransitionOrDegrade starts job controllers with exponential backoff. +// Sets TNFJobControllersDegraded based on success or failure. +// Only used for initial transition (before ExternalEtcdTransitionCompleted). +func (c *pacemakerLifecycleManager) retryInitialTransitionOrDegrade(ctx context.Context, nodes []*corev1.Node) error { + var setupErr error + err := wait.ExponentialBackoffWithContext(ctx, retryBackoffConfig, func(ctx context.Context) (bool, error) { + setupErr = c.startJobControllersWithLock(ctx, nodes) + if setupErr != nil { + klog.Warningf("failed to setup TNF job controllers, will retry: %v", setupErr) + return false, nil + } + return true, nil + }) + + if err != nil || setupErr != nil { + displayErr := setupErr + if displayErr == nil { + displayErr = err + } + klog.Errorf("failed to setup TNF job controllers after retries: %v", displayErr) + + _, _, updateErr := v1helpers.UpdateStatus(ctx, c.operatorClient, v1helpers.UpdateConditionFn(operatorv1.OperatorCondition{ + Type: conditionTypeTNFJobControllersDegraded, + Status: operatorv1.ConditionTrue, + Reason: "SetupFailed", + Message: fmt.Sprintf("Failed to setup TNF job controllers after retries: %v", displayErr), + })) + if updateErr != nil { + klog.Errorf("failed to update operator status to degraded: %v", updateErr) + } + return displayErr + } + + _, _, updateErr := v1helpers.UpdateStatus(ctx, c.operatorClient, v1helpers.UpdateConditionFn(operatorv1.OperatorCondition{ + Type: conditionTypeTNFJobControllersDegraded, + Status: operatorv1.ConditionFalse, + Reason: "AsExpected", + Message: "TNF job controllers setup completed successfully", + })) + if updateErr != nil { + klog.Errorf("failed to update operator status: %v", updateErr) + } + + return nil +} + +// startJobControllersWithLock serializes job controller startup to prevent concurrent: +// - etcd bootstrap / stable revision waits +// - duplicate job controller creation +// - races in wait logic +func (c *pacemakerLifecycleManager) startJobControllersWithLock(ctx context.Context, nodes []*corev1.Node) error { + c.startJobControllersMu.Lock() + defer c.startJobControllersMu.Unlock() + + return startTnfJobcontrollersFunc(ctx, nodes, c.controllerContext, c.operatorClient, c.kubeClient, c.kubeInformersForNamespaces, c.etcdInformer, c) +} + +// startTnfJobcontrollers creates TNF job controllers for the given nodes. +// During bootstrap: waits for etcd bootstrap completion and stable revision before creating controllers. +// Post-transition: skips bootstrap flow and ensures controllers are running (idempotent restart). +// Creates auth jobs (per-node), setup job (cluster-wide), fencing job (cluster-wide), and after-setup jobs (per-node). +// Setup job is one-time execution but controller runs in both modes to maintain conditions. +func startTnfJobcontrollers( + ctx context.Context, + controlPlaneNodeList []*corev1.Node, + controllerContext *controllercmd.ControllerContext, + operatorClient v1helpers.StaticPodOperatorClient, + kubeClient kubernetes.Interface, + kubeInformersForNamespaces v1helpers.KubeInformersForNamespaces, + etcdInformer operatorv1informers.EtcdInformer, + lifecycleManager *pacemakerLifecycleManager) error { + + // Check if transition already complete (operator restart scenario) + // If so, skip bootstrap flow and just ensure controllers are running + transitionComplete, err := ceohelpers.HasExternalEtcdCompletedTransition(ctx, operatorClient) + if err != nil { + klog.Warningf("Failed to check transition status: %v - proceeding with bootstrap flow", err) + } + + if transitionComplete { + klog.V(4).Infof("Transition already complete - skipping bootstrap flow, ensuring controllers are running") + + // Just start the controllers without going through bootstrap/setup again + // This prevents recreating setup job and racing with reconciliation + for _, node := range controlPlaneNodeList { + jobs.RunNodeJobController(ctx, tools.JobTypeAuth, node, 3, controllerContext, operatorClient, kubeClient, kubeInformersForNamespaces, lifecycleManager.controlPlaneNodeInformer, jobs.DefaultConditions) + jobs.RunNodeJobController(ctx, tools.JobTypeAfterSetup, node, 3, controllerContext, operatorClient, kubeClient, kubeInformersForNamespaces, lifecycleManager.controlPlaneNodeInformer, jobs.DefaultConditions) + } + + // schedulableNodesFunc: returns ready nodes where job can run (K8s ∩ Pacemaker intersection) + schedulableNodesFunc := func() ([]*corev1.Node, error) { + return lifecycleManager.getActivePacemakerNodes() + } + + // affectedNodesFunc for update-setup: all control plane nodes (ready or not) + // Job waits for these nodes to become ready before proceeding + // Query dynamically from informer to avoid stale node list on node replacement + updateSetupAffectedNodesFunc := func() ([]*corev1.Node, error) { + return tools.ListNodesFromInformer(lifecycleManager.controlPlaneNodeInformer) + } + + // affectedNodesFunc for fencing: all control plane nodes with fencing secrets (ready or not) + // Job waits for these nodes to become ready before proceeding + // Nodes without secrets won't block the job; when secret is added, drift detection triggers restart + // Query dynamically from informer to avoid stale node list on node replacement + fencingAffectedNodesFunc := func() ([]*corev1.Node, error) { + nodes, err := tools.ListNodesFromInformer(lifecycleManager.controlPlaneNodeInformer) + if err != nil { + return nil, err + } + return getNodesWithFencingSecrets(nodes, kubeInformersForNamespaces) + } + + // Setup job controller: maintains conditions for completed setup job + // Even though setup is one-time only, the controller must run to keep conditions current + // (fencing/auth/after-setup jobs wait for setup job completion status) + // Query dynamically from informer to avoid stale node list on node replacement + setupAffectedNodesFunc := func() ([]*corev1.Node, error) { + return tools.ListNodesFromInformer(lifecycleManager.controlPlaneNodeInformer) + } + jobs.RunClusterJobController(ctx, tools.JobTypeSetup, schedulableNodesFunc, setupAffectedNodesFunc, nil, 3, controllerContext, operatorClient, kubeClient, kubeInformersForNamespaces, jobs.AllConditions) + + // Update-setup job: ensures pacemaker cluster configuration is current + // Runs post-transition only (not needed during bootstrap) + // Only runs when exactly 2 control plane nodes exist (pacemaker limitation) + if len(controlPlaneNodeList) == 2 { + jobs.RunClusterJobController(ctx, tools.JobTypeUpdateSetup, schedulableNodesFunc, updateSetupAffectedNodesFunc, nil, 3, controllerContext, operatorClient, kubeClient, kubeInformersForNamespaces, jobs.DefaultConditions) + } else { + klog.V(4).Infof("Skipping update-setup job controller: requires exactly 2 control plane nodes, have %d", len(controlPlaneNodeList)) + } + + fencingJobConfigFunc := createFencingJobConfigFunc(lifecycleManager, kubeInformersForNamespaces) + jobs.RunClusterJobController(ctx, tools.JobTypeFencing, schedulableNodesFunc, fencingAffectedNodesFunc, fencingJobConfigFunc, 3, controllerContext, operatorClient, kubeClient, kubeInformersForNamespaces, jobs.DefaultConditions) + + // Start status collector (only after transition is complete, when Pacemaker exists) + lifecycleManager.runPacemakerStatusCollectorCronJob(ctx) + + // Start health check controller (only after transition is complete, when Pacemaker exists) + lifecycleManager.runPacemakerHealthCheckController(ctx) + + // Clear legacy condition names from upgrades (controllers recreate with new names) + clearLegacyConditions(ctx, operatorClient) + + klog.V(4).Infof("Controllers running (post-transition)") + return nil + } + + klog.Infof("Running TNF setup procedure. Waiting for etcd bootstrap to complete") + + // Wait for the etcd informer to sync before checking bootstrap status + // This ensures operatorClient.GetStaticPodOperatorState() has data to work with + klog.Infof("waiting for etcd informer to sync...") + if !cache.WaitForCacheSync(ctx.Done(), etcdInformer.Informer().HasSynced) { + return fmt.Errorf("failed to sync etcd informer") + } + klog.Infof("etcd informer synced") + + if err := waitForEtcdBootstrapCompleted(ctx, operatorClient); err != nil { + return fmt.Errorf("failed to wait for etcd bootstrap: %w", err) + } + + // Wait for all nodes to have their installers complete (creates /var/lib/etcd) + klog.Infof("bootstrap completed, waiting for all nodes to reach latest revision") + if err := etcd.WaitForStableRevision(ctx, operatorClient); err != nil { + return fmt.Errorf("failed to wait for all nodes at latest revision: %w", err) + } + + klog.Infof("all nodes at latest revision, creating TNF job controllers") + + // the order of job creation does not matter, the jobs wait on each other as needed + for _, node := range controlPlaneNodeList { + jobs.RunNodeJobController(ctx, tools.JobTypeAuth, node, 3, controllerContext, operatorClient, kubeClient, kubeInformersForNamespaces, lifecycleManager.controlPlaneNodeInformer, jobs.DefaultConditions) + jobs.RunNodeJobController(ctx, tools.JobTypeAfterSetup, node, 3, controllerContext, operatorClient, kubeClient, kubeInformersForNamespaces, lifecycleManager.controlPlaneNodeInformer, jobs.DefaultConditions) + } + + // schedulableNodesFunc: returns ready nodes where job can run (K8s ∩ Pacemaker intersection) + // During bootstrap: PacemakerCluster CR doesn't exist yet, so getActivePacemakerNodes falls back to controlPlaneNodeList + schedulableNodesFunc := func() ([]*corev1.Node, error) { + return lifecycleManager.getActivePacemakerNodes() + } + + // affectedNodesFunc for setup: all control plane nodes (ready or not) + // Job waits for these nodes to become ready before proceeding + // Query dynamically from informer to avoid stale node list + setupAffectedNodesFunc := func() ([]*corev1.Node, error) { + return tools.ListNodesFromInformer(lifecycleManager.controlPlaneNodeInformer) + } + + // affectedNodesFunc for fencing: all control plane nodes with fencing secrets (ready or not) + // Job waits for these nodes to become ready before proceeding + // Nodes without secrets won't block the job; when secret is added, drift detection triggers restart + // Query dynamically from informer to avoid stale node list + fencingAffectedNodesFunc := func() ([]*corev1.Node, error) { + nodes, err := tools.ListNodesFromInformer(lifecycleManager.controlPlaneNodeInformer) + if err != nil { + return nil, err + } + return getNodesWithFencingSecrets(nodes, kubeInformersForNamespaces) + } + + // Cluster-wide jobs: setup and fencing can run on any node + jobs.RunClusterJobController(ctx, tools.JobTypeSetup, schedulableNodesFunc, setupAffectedNodesFunc, nil, 3, controllerContext, operatorClient, kubeClient, kubeInformersForNamespaces, jobs.AllConditions) + + // Fencing job with drift detection: captures node UIDs + fencing secret UIDs + fencingJobConfigFunc := createFencingJobConfigFunc(lifecycleManager, kubeInformersForNamespaces) + jobs.RunClusterJobController(ctx, tools.JobTypeFencing, schedulableNodesFunc, fencingAffectedNodesFunc, fencingJobConfigFunc, 3, controllerContext, operatorClient, kubeClient, kubeInformersForNamespaces, jobs.DefaultConditions) + + // Clear legacy condition names from upgrades (controllers recreate with new names) + clearLegacyConditions(ctx, operatorClient) + + return nil +} + +// RemoveConditionFn returns a func to remove a condition entirely from operator status. +func RemoveConditionFn(conditionType string) v1helpers.UpdateStatusFunc { + return func(status *operatorv1.OperatorStatus) error { + v1helpers.RemoveOperatorCondition(&status.Conditions, conditionType) + return nil + } +} + +// clearLegacyConditions removes old-format TNF condition names from operator status. +// Old format: tnf-{job}-job{Condition} (e.g., tnf-setup-jobDegraded, tnf-auth-job-master-0-637363beAvailable) +// New format: TNF{Job}{Condition} (e.g., TNFSetupJobDegraded, TNFAuthJobMaster0637363beAvailable) +// Safe to call after job controllers start - they recreate conditions with new names on first sync. +func clearLegacyConditions(ctx context.Context, operatorClient v1helpers.StaticPodOperatorClient) { + _, opStatus, _, err := operatorClient.GetStaticPodOperatorState() + if err != nil || opStatus == nil { + klog.V(2).Infof("Cannot get operator status for legacy condition cleanup: %v", err) + return + } + + var removeFuncs []v1helpers.UpdateStatusFunc + for _, cond := range opStatus.Conditions { + // Match old pattern: starts with "tnf-", contains "job", and ends with condition type + // Cluster jobs: tnf-setup-jobAvailable, tnf-fencing-jobDegraded + // Node jobs: tnf-auth-job-master-0-637363beProgressing, tnf-after-setup-job-master-1-64736551Degraded + if strings.HasPrefix(cond.Type, "tnf-") && + strings.Contains(cond.Type, "job") && + (strings.HasSuffix(cond.Type, "Degraded") || + strings.HasSuffix(cond.Type, "Available") || + strings.HasSuffix(cond.Type, "Progressing")) { + klog.V(2).Infof("Found legacy condition to remove: %s", cond.Type) + removeFuncs = append(removeFuncs, RemoveConditionFn(cond.Type)) + } + } + + if len(removeFuncs) > 0 { + klog.Infof("Removing %d legacy TNF conditions from upgrade", len(removeFuncs)) + _, _, err := v1helpers.UpdateStatus(ctx, operatorClient, removeFuncs...) + if err != nil { + klog.Warningf("Failed to remove legacy conditions: %v", err) + } + } +} + +// Matches secrets by name: fencing-credentials-{nodeName}. +// Returns nodes (ready or not) that have secrets. Job will wait for these nodes to become ready. +// Nodes without secrets are excluded from the result (job won't be blocked waiting for them). +// When a fencing secret is added, drift detection (via ResourceVersion tracking) triggers a job restart. +func getNodesWithFencingSecrets(nodes []*corev1.Node, kubeInformersForNamespaces v1helpers.KubeInformersForNamespaces) ([]*corev1.Node, error) { + secretsLister := kubeInformersForNamespaces.InformersFor(operatorclient.TargetNamespace).Core().V1().Secrets().Lister() + + // Build set of node names that have fencing secrets + nodesWithSecrets := make(map[string]bool) + for _, node := range nodes { + // Check for fencing-credentials-{nodeName} + secretName := fmt.Sprintf("fencing-credentials-%s", node.Name) + _, err := secretsLister.Secrets(operatorclient.TargetNamespace).Get(secretName) + if err == nil { + nodesWithSecrets[node.Name] = true + } + // Note: We don't check for MAC-hashed secrets here (would require expensive matching). + // Those nodes will be configured when the fencing job runs and succeeds. + } + + // Filter to only nodes with secrets + var result []*corev1.Node + for _, node := range nodes { + if nodesWithSecrets[node.Name] { + result = append(result, node) + } + } + + return result, nil +} + +// createFencingJobConfigFunc creates a JobConfigFunc for the fencing job. +// Returns a function that captures node UIDs and fencing secret ResourceVersions for drift detection. +// ResourceVersion changes on every secret update (including data changes), enabling drift detection +// without requiring separate event handlers. +func createFencingJobConfigFunc(lifecycleManager *pacemakerLifecycleManager, kubeInformersForNamespaces v1helpers.KubeInformersForNamespaces) jobs.JobConfigFunc { + return func() (string, error) { + // Query nodes dynamically to avoid stale closure capture + nodes, err := tools.ListNodesFromInformer(lifecycleManager.controlPlaneNodeInformer) + if err != nil { + return "", fmt.Errorf("failed to list nodes: %w", err) + } + + // Collect node UIDs + nodeUIDs := make([]string, len(nodes)) + for i, node := range nodes { + nodeUIDs[i] = string(node.UID) + } + sort.Strings(nodeUIDs) + + // Collect fencing secret ResourceVersions from informer + secretsLister := kubeInformersForNamespaces.InformersFor(operatorclient.TargetNamespace).Core().V1().Secrets().Lister() + allSecrets, err := secretsLister.List(labels.Everything()) + if err != nil { + return "", fmt.Errorf("failed to list secrets: %w", err) + } + + var secretVersions []string + for _, secret := range allSecrets { + if tools.IsFencingSecret(secret.Name) { + secretVersions = append(secretVersions, fmt.Sprintf("%s=%s", secret.Name, secret.ResourceVersion)) + } + } + sort.Strings(secretVersions) + + config := map[string]interface{}{ + "nodeUIDs": nodeUIDs, + "secretVersions": secretVersions, + } + + configJSON, err := json.Marshal(config) + if err != nil { + return "", fmt.Errorf("failed to marshal fencing config: %w", err) + } + + return string(configJSON), nil + } +} + +// waitForEtcdBootstrapCompleted waits for etcd bootstrap to complete. +// If etcd is not yet running in cluster, it waits for bootstrap teardown. +func waitForEtcdBootstrapCompleted(ctx context.Context, operatorClient v1helpers.StaticPodOperatorClient) error { + isEtcdRunningInCluster, err := ceohelpers.IsEtcdRunningInCluster(ctx, operatorClient) + if err != nil { + return fmt.Errorf("failed to check if bootstrap is completed: %w", err) + } + if !isEtcdRunningInCluster { + klog.Infof("waiting for bootstrap to complete with etcd running in cluster") + clientConfig, err := rest.InClusterConfig() + if err != nil { + return fmt.Errorf("failed to get in-cluster config: %w", err) + } + err = bootstrapteardown.WaitForEtcdBootstrap(ctx, clientConfig) + if err != nil { + return fmt.Errorf("failed to wait for bootstrap to complete: %w", err) + } + } + return nil +} diff --git a/pkg/tnf/operator/job_controllers_test.go b/pkg/tnf/operator/job_controllers_test.go new file mode 100644 index 0000000000..0fcaf7f3a4 --- /dev/null +++ b/pkg/tnf/operator/job_controllers_test.go @@ -0,0 +1,503 @@ +package operator + +/* +TEST COVERAGE SUMMARY - job_controllers_test.go +================================================ + +This file tests TNF job controller startup logic for Two-Node Fencing clusters. + +WHAT'S TESTED +------------- + +Job Controller Startup: +├── TestRetryInitialTransitionOrDegrade - Bootstrap retry and degraded condition +│ ├── Success on first attempt (condition cleared) +│ ├── Success after retries (condition cleared) +│ └── Failure after all retries (condition set to degraded) +└── TestStartJobControllers - Entry point logic and path selection + ├── Before transition (bootstrap path): + │ ├── 2 ready nodes → bootstrap with retry + │ └── 3 nodes → skip (pacemaker only supports 2) + └── After transition (post-transition path): + ├── 2 ready nodes → ensure running + └── 1 ready node → ensure running (single-node case) + +Affected Node Filtering: +├── TestGetNodesWithFencingSecrets - Fencing job node filtering + ├── All nodes have fencing secrets + ├── Some nodes missing secrets (partial filtering) + └── No nodes have secrets (empty result) +*/ + +import ( + "context" + "fmt" + "testing" + "time" + + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/client-go/kubernetes" + "k8s.io/client-go/kubernetes/fake" + "k8s.io/client-go/tools/cache" + "k8s.io/utils/clock" + + operatorv1 "github.com/openshift/api/operator/v1" + operatorversionedclientfake "github.com/openshift/client-go/operator/clientset/versioned/fake" + extinfops "github.com/openshift/client-go/operator/informers/externalversions" + operatorv1informers "github.com/openshift/client-go/operator/informers/externalversions/operator/v1" + "github.com/openshift/cluster-etcd-operator/pkg/operator/ceohelpers" + "github.com/openshift/cluster-etcd-operator/pkg/operator/operatorclient" + u "github.com/openshift/cluster-etcd-operator/pkg/testutils" + "github.com/openshift/cluster-etcd-operator/pkg/tnf/pkg/tools" + "github.com/openshift/library-go/pkg/controller/controllercmd" + "github.com/openshift/library-go/pkg/operator/events" + "github.com/openshift/library-go/pkg/operator/v1helpers" +) + +// TestRetryInitialTransitionOrDegrade tests the exponential backoff retry logic +// and TNFJobControllersDegraded condition management during initial bootstrap. +func TestRetryInitialTransitionOrDegrade(t *testing.T) { + tests := []struct { + name string + setupMockStartFunc func() func(context.Context, []*corev1.Node, *controllercmd.ControllerContext, v1helpers.StaticPodOperatorClient, kubernetes.Interface, v1helpers.KubeInformersForNamespaces, operatorv1informers.EtcdInformer, *pacemakerLifecycleManager) error + expectDegradedCondition bool + expectDegradedStatus operatorv1.ConditionStatus + expectRetries bool + }{ + { + name: "Success on first attempt", + setupMockStartFunc: func() func(context.Context, []*corev1.Node, *controllercmd.ControllerContext, v1helpers.StaticPodOperatorClient, kubernetes.Interface, v1helpers.KubeInformersForNamespaces, operatorv1informers.EtcdInformer, *pacemakerLifecycleManager) error { + return func(_ context.Context, _ []*corev1.Node, _ *controllercmd.ControllerContext, _ v1helpers.StaticPodOperatorClient, _ kubernetes.Interface, _ v1helpers.KubeInformersForNamespaces, _ operatorv1informers.EtcdInformer, _ *pacemakerLifecycleManager) error { + return nil + } + }, + expectDegradedCondition: true, + expectDegradedStatus: operatorv1.ConditionFalse, + expectRetries: false, + }, + { + name: "Success after retries", + setupMockStartFunc: func() func(context.Context, []*corev1.Node, *controllercmd.ControllerContext, v1helpers.StaticPodOperatorClient, kubernetes.Interface, v1helpers.KubeInformersForNamespaces, operatorv1informers.EtcdInformer, *pacemakerLifecycleManager) error { + attemptCount := 0 + return func(_ context.Context, _ []*corev1.Node, _ *controllercmd.ControllerContext, _ v1helpers.StaticPodOperatorClient, _ kubernetes.Interface, _ v1helpers.KubeInformersForNamespaces, _ operatorv1informers.EtcdInformer, _ *pacemakerLifecycleManager) error { + attemptCount++ + if attemptCount < 3 { + return &retryableError{msg: "temporary failure"} + } + return nil + } + }, + expectDegradedCondition: true, + expectDegradedStatus: operatorv1.ConditionFalse, + expectRetries: true, + }, + { + name: "Failure after all retries", + setupMockStartFunc: func() func(context.Context, []*corev1.Node, *controllercmd.ControllerContext, v1helpers.StaticPodOperatorClient, kubernetes.Interface, v1helpers.KubeInformersForNamespaces, operatorv1informers.EtcdInformer, *pacemakerLifecycleManager) error { + return func(_ context.Context, _ []*corev1.Node, _ *controllercmd.ControllerContext, _ v1helpers.StaticPodOperatorClient, _ kubernetes.Interface, _ v1helpers.KubeInformersForNamespaces, _ operatorv1informers.EtcdInformer, _ *pacemakerLifecycleManager) error { + return &retryableError{msg: "persistent failure"} + } + }, + expectDegradedCondition: true, + expectDegradedStatus: operatorv1.ConditionTrue, + expectRetries: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Setup test environment + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + fakeKubeClient := fake.NewClientset() + fakeOperatorClient := v1helpers.NewFakeStaticPodOperatorClient( + &operatorv1.StaticPodOperatorSpec{}, + u.StaticPodOperatorStatus(), + nil, + nil, + ) + + eventRecorder := events.NewRecorder(fakeKubeClient.CoreV1().Events(operatorclient.TargetNamespace), + "test-retry", &corev1.ObjectReference{}, clock.RealClock{}) + + controllerContext := &controllercmd.ControllerContext{ + EventRecorder: eventRecorder, + } + + kubeInformersForNamespaces := v1helpers.NewKubeInformersForNamespaces( + fakeKubeClient, + operatorclient.TargetNamespace, + ) + + // Create etcd informer + operatorClientFake := operatorversionedclientfake.NewClientset() + etcdInformers := extinfops.NewSharedInformerFactory(operatorClientFake, 10*time.Minute) + + // Create lifecycle manager + manager := &pacemakerLifecycleManager{ + operatorClient: fakeOperatorClient, + kubeClient: fakeKubeClient, + controllerContext: controllerContext, + kubeInformersForNamespaces: kubeInformersForNamespaces, + etcdInformer: etcdInformers.Operator().V1().Etcds(), + } + + // Store original startTnfJobcontrollersFunc and replace with mock + originalStartFunc := startTnfJobcontrollersFunc + startTnfJobcontrollersFunc = tt.setupMockStartFunc() + defer func() { startTnfJobcontrollersFunc = originalStartFunc }() + + // Store original backoff config and use faster settings for testing + originalBackoff := retryBackoffConfig + retryBackoffConfig = wait.Backoff{ + Duration: 100 * time.Millisecond, + Factor: 2.0, + Steps: 5, // Much shorter for testing + Cap: 500 * time.Millisecond, + } + defer func() { retryBackoffConfig = originalBackoff }() + + // Create test nodes + nodes := []*corev1.Node{ + {ObjectMeta: metav1.ObjectMeta{Name: "master-0"}}, + {ObjectMeta: metav1.ObjectMeta{Name: "master-1"}}, + } + + // Run retryInitialTransitionOrDegrade + err := manager.retryInitialTransitionOrDegrade(ctx, nodes) + + // Verify error expectation + if tt.expectDegradedStatus == operatorv1.ConditionTrue { + require.Error(t, err, "Expected error when retries exhausted") + } else { + require.NoError(t, err, "Expected no error on success") + } + + // Verify the operator condition was set correctly + if tt.expectDegradedCondition { + _, status, _, err := fakeOperatorClient.GetStaticPodOperatorState() + require.NoError(t, err, "failed to get operator status") + + // Find the TNFJobControllersDegraded condition + var foundCondition *operatorv1.OperatorCondition + for i, condition := range status.Conditions { + if condition.Type == conditionTypeTNFJobControllersDegraded { + foundCondition = &status.Conditions[i] + break + } + } + + require.NotNil(t, foundCondition, "TNFJobControllersDegraded condition not found") + require.Equal(t, tt.expectDegradedStatus, foundCondition.Status, + "Expected degraded status %v but got %v", tt.expectDegradedStatus, foundCondition.Status) + + if tt.expectDegradedStatus == operatorv1.ConditionTrue { + require.Equal(t, "SetupFailed", foundCondition.Reason, + "Expected reason SetupFailed but got %s", foundCondition.Reason) + require.Contains(t, foundCondition.Message, "Failed to setup TNF job controllers", + "Expected message to contain failure info") + } else { + require.Equal(t, "AsExpected", foundCondition.Reason, + "Expected reason AsExpected but got %s", foundCondition.Reason) + require.Contains(t, foundCondition.Message, "successfully", + "Expected success message") + } + } + }) + } +} + +// TestStartJobControllers tests the entry point logic for starting job controllers. +// This includes transition checks, node count validation, and path selection. +func TestStartJobControllers(t *testing.T) { + tests := []struct { + name string + transitionComplete bool + nodeCount int + readyNodeCount int + expectStartCalled bool + expectRetryPath bool + expectError bool + }{ + { + name: "before transition - 2 ready nodes - bootstrap with retry", + transitionComplete: false, + nodeCount: 2, + readyNodeCount: 2, + expectStartCalled: true, + expectRetryPath: true, + expectError: false, + }, + { + name: "before transition - 3 nodes - error (pacemaker only supports 2)", + transitionComplete: false, + nodeCount: 3, + readyNodeCount: 3, + expectStartCalled: false, + expectRetryPath: false, + expectError: true, + }, + { + name: "after transition - 2 ready nodes - ensure running", + transitionComplete: true, + nodeCount: 2, + readyNodeCount: 2, + expectStartCalled: true, + expectRetryPath: false, + expectError: false, + }, + { + name: "after transition - 1 ready node - ensure running (handles single-node)", + transitionComplete: true, + nodeCount: 1, + readyNodeCount: 1, + expectStartCalled: true, + expectRetryPath: false, + expectError: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := context.Background() + + // Create fake clients + fakeKubeClient := fake.NewClientset() + + // Create nodes + var nodes []runtime.Object + for i := 0; i < tt.nodeCount; i++ { + node := &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf("master-%d", i), + Labels: map[string]string{tools.ControlPlaneNodeLabelSelector: ""}, + }, + Status: corev1.NodeStatus{}, + } + // Mark nodes as ready up to readyNodeCount + if i < tt.readyNodeCount { + node.Status.Conditions = []corev1.NodeCondition{ + {Type: corev1.NodeReady, Status: corev1.ConditionTrue}, + } + } else { + node.Status.Conditions = []corev1.NodeCondition{ + {Type: corev1.NodeReady, Status: corev1.ConditionFalse}, + } + } + nodes = append(nodes, node) + } + + // Add nodes to fake client + for _, node := range nodes { + _, err := fakeKubeClient.CoreV1().Nodes().Create(ctx, node.(*corev1.Node), metav1.CreateOptions{}) + require.NoError(t, err) + } + + // Create operator client with transition state + var conditions []operatorv1.OperatorCondition + if tt.transitionComplete { + conditions = append(conditions, operatorv1.OperatorCondition{ + Type: ceohelpers.OperatorConditionExternalEtcdHasCompletedTransition, + Status: operatorv1.ConditionTrue, + }) + } + fakeOperatorClient := v1helpers.NewFakeStaticPodOperatorClient( + &operatorv1.StaticPodOperatorSpec{}, + &operatorv1.StaticPodOperatorStatus{ + OperatorStatus: operatorv1.OperatorStatus{ + Conditions: conditions, + }, + }, + nil, + nil, + ) + + eventRecorder := events.NewRecorder(fakeKubeClient.CoreV1().Events(operatorclient.TargetNamespace), + "test-start", &corev1.ObjectReference{}, clock.RealClock{}) + + controllerContext := &controllercmd.ControllerContext{ + EventRecorder: eventRecorder, + } + + kubeInformersForNamespaces := v1helpers.NewKubeInformersForNamespaces( + fakeKubeClient, + operatorclient.TargetNamespace, + ) + + // Create node informer + nodeIndexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{}) + for _, node := range nodes { + require.NoError(t, nodeIndexer.Add(node), "failed to add node to indexer") + } + controlPlaneNodeInformer := &mockNodeInformer{ + indexer: nodeIndexer, + synced: true, + } + + // Create etcd informer + operatorClientFake := operatorversionedclientfake.NewClientset() + etcdInformers := extinfops.NewSharedInformerFactory(operatorClientFake, 10*time.Minute) + + // Create lifecycle manager + manager := &pacemakerLifecycleManager{ + operatorClient: fakeOperatorClient, + kubeClient: fakeKubeClient, + controlPlaneNodeInformer: controlPlaneNodeInformer, + controllerContext: controllerContext, + kubeInformersForNamespaces: kubeInformersForNamespaces, + etcdInformer: etcdInformers.Operator().V1().Etcds(), + } + + // Track if startTnfJobcontrollersFunc was called + startCalled := false + originalStartFunc := startTnfJobcontrollersFunc + startTnfJobcontrollersFunc = func(_ context.Context, _ []*corev1.Node, _ *controllercmd.ControllerContext, _ v1helpers.StaticPodOperatorClient, _ kubernetes.Interface, _ v1helpers.KubeInformersForNamespaces, _ operatorv1informers.EtcdInformer, _ *pacemakerLifecycleManager) error { + startCalled = true + return nil + } + defer func() { startTnfJobcontrollersFunc = originalStartFunc }() + + // Use faster backoff for testing + originalBackoff := retryBackoffConfig + retryBackoffConfig = wait.Backoff{ + Duration: 10 * time.Millisecond, + Factor: 2.0, + Steps: 2, + Cap: 50 * time.Millisecond, + } + defer func() { retryBackoffConfig = originalBackoff }() + + // Execute + err := manager.startJobControllers(ctx) + + // Verify + if tt.expectError { + require.Error(t, err) + } else { + require.NoError(t, err) + } + require.Equal(t, tt.expectStartCalled, startCalled, + "Expected startTnfJobcontrollersFunc called=%v, got=%v", tt.expectStartCalled, startCalled) + + // If retry path expected, verify TNFJobControllersDegraded condition was set + if tt.expectRetryPath && tt.expectStartCalled { + _, status, _, err := fakeOperatorClient.GetStaticPodOperatorState() + require.NoError(t, err) + + var foundCondition *operatorv1.OperatorCondition + for i, condition := range status.Conditions { + if condition.Type == conditionTypeTNFJobControllersDegraded { + foundCondition = &status.Conditions[i] + break + } + } + + require.NotNil(t, foundCondition, "TNFJobControllersDegraded condition should be set on retry path") + require.Equal(t, operatorv1.ConditionFalse, foundCondition.Status, + "TNFJobControllersDegraded should be False on success") + } + }) + } +} + +// TestGetNodesWithFencingSecrets tests the secret filtering logic +// that ensures we only wait for nodes that have fencing configured. +func TestGetNodesWithFencingSecrets(t *testing.T) { + tests := []struct { + name string + nodes []*corev1.Node + secrets []*corev1.Secret + expectedNodes []string + }{ + { + name: "all nodes have fencing secrets", + nodes: []*corev1.Node{ + {ObjectMeta: metav1.ObjectMeta{Name: "master-0"}}, + {ObjectMeta: metav1.ObjectMeta{Name: "master-1"}}, + }, + secrets: []*corev1.Secret{ + {ObjectMeta: metav1.ObjectMeta{Name: "fencing-credentials-master-0", Namespace: operatorclient.TargetNamespace}}, + {ObjectMeta: metav1.ObjectMeta{Name: "fencing-credentials-master-1", Namespace: operatorclient.TargetNamespace}}, + }, + expectedNodes: []string{"master-0", "master-1"}, + }, + { + name: "some nodes missing secrets", + nodes: []*corev1.Node{ + {ObjectMeta: metav1.ObjectMeta{Name: "master-0"}}, + {ObjectMeta: metav1.ObjectMeta{Name: "master-1"}}, + {ObjectMeta: metav1.ObjectMeta{Name: "master-2"}}, + }, + secrets: []*corev1.Secret{ + {ObjectMeta: metav1.ObjectMeta{Name: "fencing-credentials-master-0", Namespace: operatorclient.TargetNamespace}}, + // master-1 missing + {ObjectMeta: metav1.ObjectMeta{Name: "fencing-credentials-master-2", Namespace: operatorclient.TargetNamespace}}, + }, + expectedNodes: []string{"master-0", "master-2"}, + }, + { + name: "no nodes have secrets", + nodes: []*corev1.Node{ + {ObjectMeta: metav1.ObjectMeta{Name: "master-0"}}, + {ObjectMeta: metav1.ObjectMeta{Name: "master-1"}}, + }, + secrets: []*corev1.Secret{}, + expectedNodes: []string{}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Setup fake clients + fakeKubeClient := fake.NewClientset() + + // Create informers + kubeInformersForNamespaces := v1helpers.NewKubeInformersForNamespaces( + fakeKubeClient, + operatorclient.TargetNamespace, + ) + + // Start informers first + stopCh := make(chan struct{}) + defer close(stopCh) + kubeInformersForNamespaces.Start(stopCh) + synced := kubeInformersForNamespaces.WaitForCacheSync(stopCh) + for ns, typeSynced := range synced { + for typ, s := range typeSynced { + require.True(t, s, "informer for namespace %s type %v failed to sync", ns, typ) + } + } + + // Add secrets manually to the informer's indexer AFTER sync + secretInformer := kubeInformersForNamespaces.InformersFor(operatorclient.TargetNamespace).Core().V1().Secrets().Informer() + for _, secret := range tt.secrets { + require.NoError(t, secretInformer.GetIndexer().Add(secret)) + } + + // Execute + result, err := getNodesWithFencingSecrets(tt.nodes, kubeInformersForNamespaces) + + // Verify + require.NoError(t, err) + actualNodeNames := make([]string, len(result)) + for i, node := range result { + actualNodeNames[i] = node.Name + } + require.ElementsMatch(t, tt.expectedNodes, actualNodeNames) + }) + } +} + +// retryableError is a helper type for testing retry logic +type retryableError struct { + msg string +} + +func (e *retryableError) Error() string { + return e.msg +} diff --git a/pkg/tnf/operator/lifecycle_manager.go b/pkg/tnf/operator/lifecycle_manager.go new file mode 100644 index 0000000000..14ed822869 --- /dev/null +++ b/pkg/tnf/operator/lifecycle_manager.go @@ -0,0 +1,335 @@ +package operator + +import ( + "context" + "fmt" + "sync" + "time" + + operatorv1informers "github.com/openshift/client-go/operator/informers/externalversions/operator/v1" + "github.com/openshift/library-go/pkg/controller/controllercmd" + "github.com/openshift/library-go/pkg/controller/factory" + "github.com/openshift/library-go/pkg/operator/events" + "github.com/openshift/library-go/pkg/operator/v1helpers" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/watch" + "k8s.io/client-go/kubernetes" + "k8s.io/client-go/rest" + "k8s.io/client-go/tools/cache" + "k8s.io/klog/v2" + + pacmkrv1 "github.com/openshift/api/etcd/v1" + "github.com/openshift/cluster-etcd-operator/pkg/operator/ceohelpers" + "github.com/openshift/cluster-etcd-operator/pkg/tnf/pkg/jobs" + "github.com/openshift/cluster-etcd-operator/pkg/tnf/pkg/pacemaker" + "github.com/openshift/cluster-etcd-operator/pkg/tnf/pkg/tools" +) + +// Local constants for lifecycle controller +const ( + // Controller name + controllerNamePacemakerLifecycle = "PacemakerLifecycleManager" +) + +// PacemakerLifecycleManager manages job controller startup for TNF clusters. +// Starts job controllers when conditions are met (bootstrap or runtime mode). +type pacemakerLifecycleManager struct { + operatorClient v1helpers.StaticPodOperatorClient + kubeClient kubernetes.Interface + eventRecorder events.Recorder + pacemakerInformer cache.SharedIndexInformer + + // For node lifecycle management + controlPlaneNodeInformer cache.SharedIndexInformer + controllerContext *controllercmd.ControllerContext + kubeInformersForNamespaces v1helpers.KubeInformersForNamespaces + etcdInformer operatorv1informers.EtcdInformer + + // Job controller startup protection: prevents concurrent startJobControllers calls + startJobControllersMu sync.Mutex + // Track if job controllers have been started (set once, never reset) + jobControllersStarted bool + + // Status collector startup protection: prevents duplicate status collector starts + statusCollectorMu sync.Mutex + statusCollectorStarted bool + + // Health check controller startup protection: prevents duplicate health check starts + healthCheckMu sync.Mutex + healthCheckStarted bool + + // Controller context for goroutines (set on first sync, cancelled on shutdown) + controllerCtx context.Context + controllerCtxMu sync.Mutex +} + +// newPacemakerLifecycleManager creates a new PacemakerLifecycleManager for monitoring pacemaker status +// and managing node membership reconciliation in clusters that use ExternalEtcd. +// Returns the controller, the PacemakerLifecycleManager instance, and the PacemakerCluster informer +// (which must be started separately - see runPacemakerControllers in pkg/tnf/operator/starter.go). +func newPacemakerLifecycleManager( + operatorClient v1helpers.StaticPodOperatorClient, + kubeClient kubernetes.Interface, + eventRecorder events.Recorder, + restConfig *rest.Config, + controlPlaneNodeInformer cache.SharedIndexInformer, + controllerContext *controllercmd.ControllerContext, + kubeInformersForNamespaces v1helpers.KubeInformersForNamespaces, + etcdInformer operatorv1informers.EtcdInformer, +) (factory.Controller, *pacemakerLifecycleManager, cache.SharedIndexInformer, error) { + // Create REST client for PacemakerStatus CRs + restClient, err := pacemaker.CreatePacemakerRESTClient(restConfig) + if err != nil { + return nil, nil, nil, fmt.Errorf("failed to create REST client: %w", err) + } + + // Create scheme for the parameter codec + scheme := runtime.NewScheme() + if err := pacmkrv1.AddToScheme(scheme); err != nil { + return nil, nil, nil, fmt.Errorf("failed to add scheme for informer: %w", err) + } + + // Create informer for PacemakerCluster + klog.Infof("Creating PacemakerCluster informer for group %s, resource %s", pacmkrv1.SchemeGroupVersion.String(), pacemaker.PacemakerResourceName) + informer := cache.NewSharedIndexInformer( + &pacemaker.PacemakerListWatch{ListWatch: cache.ListWatch{ + ListFunc: func(options metav1.ListOptions) (runtime.Object, error) { + klog.V(4).Infof("PacemakerCluster informer ListFunc called for resource %s", pacemaker.PacemakerResourceName) + sanitizedOptions := pacemaker.SanitizeListOptions(options) + result := &pacmkrv1.PacemakerClusterList{} + err := restClient.Get(). + Resource(pacemaker.PacemakerResourceName). + VersionedParams(&sanitizedOptions, runtime.NewParameterCodec(scheme)). + Do(context.Background()). + Into(result) + if err != nil { + klog.Errorf("Failed to list PacemakerCluster resources (%s): %v", pacemaker.PacemakerResourceName, err) + } else { + klog.V(4).Infof("Successfully listed PacemakerCluster resources, found %d items", len(result.Items)) + } + return result, err + }, + WatchFunc: func(options metav1.ListOptions) (watch.Interface, error) { + klog.V(4).Infof("PacemakerCluster informer WatchFunc called for resource %s", pacemaker.PacemakerResourceName) + sanitizedOptions := pacemaker.SanitizeListOptions(options) + watcher, err := restClient.Get(). + Resource(pacemaker.PacemakerResourceName). + VersionedParams(&sanitizedOptions, runtime.NewParameterCodec(scheme)). + Watch(context.Background()) + if err != nil { + klog.Errorf("Failed to watch PacemakerCluster resources (%s): %v", pacemaker.PacemakerResourceName, err) + } + return watcher, err + }, + }}, + &pacmkrv1.PacemakerCluster{}, + pacemaker.HealthCheckResyncInterval, + cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc}, + ) + + c := &pacemakerLifecycleManager{ + operatorClient: operatorClient, + kubeClient: kubeClient, + eventRecorder: eventRecorder, + pacemakerInformer: informer, + controlPlaneNodeInformer: controlPlaneNodeInformer, + controllerContext: controllerContext, + kubeInformersForNamespaces: kubeInformersForNamespaces, + etcdInformer: etcdInformer, + } + + syncCtx := factory.NewSyncContext(controllerNamePacemakerLifecycle, eventRecorder.WithComponentSuffix("pacemaker-lifecycle-manager")) + + klog.Infof("%s controller created, waiting for informers to sync before starting", controllerNamePacemakerLifecycle) + klog.Infof("PacemakerLifecycleManager will watch: operatorClient and %s/%s resource", pacmkrv1.SchemeGroupVersion.String(), pacemaker.PacemakerResourceName) + + // ResyncEvery ensures the sync function is called at regular intervals (1 minute) + // even if no informer events are detected. + controller := factory.New(). + WithSyncContext(syncCtx). + ResyncEvery(time.Minute). + WithSync(c.sync). + WithInformers( + operatorClient.Informer(), + informer, + controlPlaneNodeInformer, + ).ToController(controllerNamePacemakerLifecycle, syncCtx.Recorder()) + + // PacemakerCluster informer is started in runPacemakerControllers (pkg/tnf/operator/starter.go) + // Node informer is started in RunOperator (pkg/operator/starter.go) + klog.Infof("PacemakerLifecycleManager controller created, will wait up to 10 minutes for informers to sync") + + // Register node event handlers for drift-triggered reconciliation + if err := c.registerNodeEventHandlers(); err != nil { + return nil, nil, nil, fmt.Errorf("failed to register node event handlers: %w", err) + } + + return controller, c, informer, nil +} + +// registerNodeEventHandlers registers Update event handler on the node informer. +// Returns an error if registration fails, as the lifecycle manager cannot function without +// receiving node events for update-setup job restarts. +// UpdateFunc handles Ready transitions to trigger update-setup job restart (post-transition only). +func (c *pacemakerLifecycleManager) registerNodeEventHandlers() error { + if c.controlPlaneNodeInformer == nil { + return fmt.Errorf("controlPlaneNodeInformer is nil") + } + + _, err := c.controlPlaneNodeInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{ + UpdateFunc: func(oldObj, newObj any) { + oldNode, oldOk := oldObj.(*corev1.Node) + newNode, newOk := newObj.(*corev1.Node) + if !oldOk || !newOk { + klog.Warningf("failed to convert updated object to Node, old=%+v, new=%+v", oldObj, newObj) + return + } + + // Check for Ready transition - triggers update-setup restart post-transition + oldReady := tools.IsNodeReady(oldNode) + newReady := tools.IsNodeReady(newNode) + if !oldReady && newReady { + klog.Infof("node %s transitioned to ready state - restarting update-setup job", newNode.GetName()) + go func() { + // Use controller context (cancelled on shutdown) instead of background context + c.controllerCtxMu.Lock() + ctx := c.controllerCtx + c.controllerCtxMu.Unlock() + + if ctx == nil { + // Controller hasn't started yet, event fired before first sync + // This shouldn't happen (informers sync before events fire), but be defensive + klog.V(4).Infof("Skipping node ready event handler - controller context not yet available") + return + } + + // Restart update-setup job when nodes become ready (e.g., after replacement) + // This ensures update-setup reruns if it completed before auth ran on new node + if err := c.restartUpdateSetupJob(ctx); err != nil { + klog.Errorf("Failed to restart update-setup job on node ready: %v", err) + } + }() + } + }, + }) + + if err != nil { + return fmt.Errorf("failed to add event handler to node informer: %w", err) + } + + klog.Infof("Registered Update event handler for node lifecycle management") + return nil +} + +// sync is the main sync function that gets called periodically to check pacemaker status +func (c *pacemakerLifecycleManager) sync(ctx context.Context, syncCtx factory.SyncContext) error { + klog.V(4).Infof("PacemakerLifecycleManager sync started") + defer klog.V(4).Infof("PacemakerLifecycleManager sync completed") + + // Store controller context on first sync (for event handler goroutines) + c.controllerCtxMu.Lock() + if c.controllerCtx == nil { + c.controllerCtx = ctx + } + c.controllerCtxMu.Unlock() + + // Start job controllers (runs in both bootstrap and runtime modes) + if err := c.startJobControllers(ctx); err != nil { + klog.Errorf("Failed to start job controllers: %v", err) + return fmt.Errorf("failed to start job controllers: %w", err) + } + + return nil +} + +// runPacemakerHealthCheckController starts the health check controller. +// Monitors Pacemaker cluster health via PacemakerCluster CR and sets operator Degraded conditions. +// Only starts if not already started (idempotent). +func (c *pacemakerLifecycleManager) runPacemakerHealthCheckController(ctx context.Context) { + // Prevent duplicate starts + c.healthCheckMu.Lock() + if c.healthCheckStarted { + c.healthCheckMu.Unlock() + klog.V(4).Infof("Health check controller already started, skipping duplicate start") + return + } + c.healthCheckStarted = true + c.healthCheckMu.Unlock() + + healthCheckController, _, err := pacemaker.NewHealthCheckWithInformer( + c.operatorClient, + c.kubeClient, + c.eventRecorder, + c.pacemakerInformer, + ) + if err != nil { + klog.Errorf("Failed to create health check controller: %v", err) + return + } + + go healthCheckController.Run(ctx, 1) + klog.Infof("Health check controller started") +} + +// restartUpdateSetupJob restarts the update-setup job controller when nodes become ready. +// Only runs post-transition when exactly 2 control plane nodes exist. +func (c *pacemakerLifecycleManager) restartUpdateSetupJob(ctx context.Context) error { + // Check if transition is complete - update-setup only runs post-transition + transitionComplete, err := ceohelpers.HasExternalEtcdCompletedTransition(ctx, c.operatorClient) + if err != nil { + return fmt.Errorf("failed to check external etcd transition status: %w", err) + } + if !transitionComplete { + klog.V(4).Infof("Skipping update-setup restart - transition not yet complete") + return nil + } + + // Get control plane nodes + if c.controlPlaneNodeInformer == nil || !c.controlPlaneNodeInformer.HasSynced() { + klog.V(4).Infof("Skipping update-setup restart - node informer not synced yet") + return nil + } + controlPlaneNodes, err := tools.ListNodesFromInformer(c.controlPlaneNodeInformer) + if err != nil { + return fmt.Errorf("failed to list control plane nodes: %w", err) + } + + // Update-setup requires exactly 2 nodes (pacemaker limitation) + if len(controlPlaneNodes) != 2 { + klog.V(4).Infof("Skipping update-setup restart: requires exactly 2 control plane nodes, have %d", len(controlPlaneNodes)) + return nil + } + + // schedulableNodesFunc: returns ready nodes where job can run (K8s ∩ Pacemaker intersection) + schedulableNodesFunc := func() ([]*corev1.Node, error) { + return c.getActivePacemakerNodes() + } + + // affectedNodesFunc: all control plane nodes (ready or not) + // Job waits for these nodes to become ready before proceeding + updateSetupAffectedNodesFunc := func() ([]*corev1.Node, error) { + return tools.ListNodesFromInformer(c.controlPlaneNodeInformer) + } + + klog.Infof("Restarting update-setup job controller after node ready event") + if err := jobs.RestartClusterJobOrRunController( + ctx, + tools.JobTypeUpdateSetup, + schedulableNodesFunc, + updateSetupAffectedNodesFunc, + nil, // no jobConfigFunc for update-setup + 3, // retries + c.controllerContext, + c.operatorClient, + c.kubeClient, + c.kubeInformersForNamespaces, + jobs.DefaultConditions, + 10*time.Second, // existingJobCompletionTimeout + ); err != nil { + return fmt.Errorf("failed to restart update-setup job controller: %w", err) + } + + return nil +} diff --git a/pkg/tnf/operator/nodehandler.go b/pkg/tnf/operator/nodehandler.go deleted file mode 100644 index 14b2b20ec9..0000000000 --- a/pkg/tnf/operator/nodehandler.go +++ /dev/null @@ -1,320 +0,0 @@ -package operator - -import ( - "context" - "fmt" - "sync" - "time" - - operatorv1 "github.com/openshift/api/operator/v1" - operatorv1informers "github.com/openshift/client-go/operator/informers/externalversions/operator/v1" - "github.com/openshift/library-go/pkg/controller/controllercmd" - "github.com/openshift/library-go/pkg/operator/v1helpers" - corev1 "k8s.io/api/core/v1" - v1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/labels" - "k8s.io/apimachinery/pkg/util/wait" - "k8s.io/client-go/kubernetes" - corev1listers "k8s.io/client-go/listers/core/v1" - "k8s.io/client-go/rest" - "k8s.io/client-go/tools/cache" - "k8s.io/klog/v2" - - "github.com/openshift/cluster-etcd-operator/pkg/operator/bootstrapteardown" - "github.com/openshift/cluster-etcd-operator/pkg/operator/ceohelpers" - "github.com/openshift/cluster-etcd-operator/pkg/operator/operatorclient" - "github.com/openshift/cluster-etcd-operator/pkg/tnf/pkg/etcd" - "github.com/openshift/cluster-etcd-operator/pkg/tnf/pkg/jobs" - "github.com/openshift/cluster-etcd-operator/pkg/tnf/pkg/tools" -) - -var ( - // handleNodesMutex ensures that node handling code doesn't run concurrently - handleNodesMutex sync.Mutex - - // handleNodesFunc is a variable to allow mocking in tests - handleNodesFunc = handleNodes - - // startTnfJobcontrollersFunc is a variable to allow mocking in tests - startTnfJobcontrollersFunc = startTnfJobcontrollers - - // updateSetupFunc is a variable to allow mocking in tests - updateSetupFunc = updateSetup - - // retryBackoffConfig allows customizing retry behavior for tests - retryBackoffConfig = wait.Backoff{ - Duration: 5 * time.Second, - Factor: 2.0, - Steps: 9, // ~10 minutes total: 5s + 10s + 20s + 40s + 80s + 120s + 120s + 120s + 120s - Cap: 2 * time.Minute, - } -) - -func handleNodesWithRetry( - controllerContext *controllercmd.ControllerContext, - controlPlaneNodeLister corev1listers.NodeLister, - ctx context.Context, - operatorClient v1helpers.StaticPodOperatorClient, - kubeClient kubernetes.Interface, - kubeInformersForNamespaces v1helpers.KubeInformersForNamespaces, - etcdInformer operatorv1informers.EtcdInformer, -) { - - // Ensure only one execution at a time - don't run in parallel - handleNodesMutex.Lock() - defer handleNodesMutex.Unlock() - - // Retry with exponential backoff to handle transient failures - var setupErr error - err := wait.ExponentialBackoffWithContext(ctx, retryBackoffConfig, func(ctx context.Context) (bool, error) { - setupErr = handleNodesFunc(controllerContext, controlPlaneNodeLister, ctx, operatorClient, kubeClient, kubeInformersForNamespaces, etcdInformer) - if setupErr != nil { - klog.Warningf("failed to setup TNF job controllers, will retry: %v", setupErr) - return false, nil - } - return true, nil - }) - - if err != nil || setupErr != nil { - klog.Errorf("failed to setup TNF job controllers after 10 minutes of retries: %v", setupErr) - - // Degrade the operator to indicate TNF job controller setup failed - _, _, updateErr := v1helpers.UpdateStatus(ctx, operatorClient, v1helpers.UpdateConditionFn(operatorv1.OperatorCondition{ - Type: "TNFJobControllersDegraded", - Status: operatorv1.ConditionTrue, - Reason: "SetupFailed", - Message: fmt.Sprintf("Failed to setup TNF job controllers after retries: %v", setupErr), - })) - if updateErr != nil { - klog.Errorf("failed to update operator status to degraded: %v", updateErr) - } - } else { - // Clear any previous degraded condition on success - _, _, updateErr := v1helpers.UpdateStatus(ctx, operatorClient, v1helpers.UpdateConditionFn(operatorv1.OperatorCondition{ - Type: "TNFJobControllersDegraded", - Status: operatorv1.ConditionFalse, - Reason: "AsExpected", - Message: "TNF job controllers setup completed successfully", - })) - if updateErr != nil { - klog.Errorf("failed to update operator status: %v", updateErr) - } - } -} - -func handleNodes( - controllerContext *controllercmd.ControllerContext, - controlPlaneNodeLister corev1listers.NodeLister, - ctx context.Context, - operatorClient v1helpers.StaticPodOperatorClient, - kubeClient kubernetes.Interface, - kubeInformersForNamespaces v1helpers.KubeInformersForNamespaces, - etcdInformer operatorv1informers.EtcdInformer, -) error { - - // ensure we have 2 control plane nodes before doing anything - nodeList, err := controlPlaneNodeLister.List(labels.Everything()) - if err != nil { - return fmt.Errorf("failed to list control plane nodes: %w", err) - } - if len(nodeList) > 2 { - klog.Warningf("found more than 2 control plane nodes (%d), unsupported use case, no further steps are taken for now", len(nodeList)) - // don't retry - return nil - } - if len(nodeList) < 2 { - klog.Warningf("found a single control plane node only, waiting for the second one") - // don't retry - return nil - } - bothReady := true - for _, node := range nodeList { - if !tools.IsNodeReady(node) { - klog.Warningf("node %q is not ready yet, waiting for it to become ready", node.GetName()) - bothReady = false - } - } - if !bothReady { - // a node condition change will retrigger node handling automatically, no need to trigger a retry here - return nil - } - - klog.Infof("found 2 control plane nodes (%q, %q)", nodeList[0].GetName(), nodeList[1].GetName()) - - // check if TNF was already set up, by looking for existing jobs - jobsExist, err := tnfSetupJobsExist(ctx, kubeClient) - if err != nil { - return fmt.Errorf("failed to check for existing TNF jobs: %w", err) - } - - // always start job controllers, otherwise jobs won't be recreated after a CEO restart - err = startTnfJobcontrollersFunc(nodeList, ctx, controllerContext, operatorClient, kubeClient, kubeInformersForNamespaces, etcdInformer) - if err != nil { - return fmt.Errorf("failed to start TNF job controllers: %w", err) - } - - // in case TNF was not setup yet, we are done here - if !jobsExist { - return nil - } - - // if TNF was already set up, we might need to update - err = updateSetupFunc(nodeList, ctx, controllerContext, operatorClient, kubeClient, kubeInformersForNamespaces) - if err != nil { - return fmt.Errorf("failed to update pacemaker setup: %w", err) - } - - return nil -} - -func startTnfJobcontrollers( - nodeList []*corev1.Node, - ctx context.Context, - controllerContext *controllercmd.ControllerContext, - operatorClient v1helpers.StaticPodOperatorClient, - kubeClient kubernetes.Interface, - kubeInformersForNamespaces v1helpers.KubeInformersForNamespaces, - etcdInformer operatorv1informers.EtcdInformer) error { - - klog.Infof("Running TNF setup procedure. Waiting for etcd bootstrap to complete") - - // Wait for the etcd informer to sync before checking bootstrap status - // This ensures operatorClient.GetStaticPodOperatorState() has data to work with - klog.Infof("waiting for etcd informer to sync...") - if !cache.WaitForCacheSync(ctx.Done(), etcdInformer.Informer().HasSynced) { - return fmt.Errorf("failed to sync etcd informer") - } - klog.Infof("etcd informer synced") - - if err := waitForEtcdBootstrapCompleted(ctx, operatorClient); err != nil { - return fmt.Errorf("failed to wait for etcd bootstrap: %w", err) - } - - // Wait for all nodes to have their installers complete (creates /var/lib/etcd) - klog.Infof("bootstrap completed, waiting for all nodes to reach latest revision") - if err := etcd.WaitForStableRevision(ctx, operatorClient); err != nil { - return fmt.Errorf("failed to wait for all nodes at latest revision: %w", err) - } - - klog.Infof("all nodes at latest revision, creating TNF job controllers") - - // the order of job creation does not matter, the jobs wait on each other as needed - for _, node := range nodeList { - jobs.RunTNFJobController(ctx, tools.JobTypeAuth, &node.Name, controllerContext, operatorClient, kubeClient, kubeInformersForNamespaces, jobs.DefaultConditions) - jobs.RunTNFJobController(ctx, tools.JobTypeAfterSetup, &node.Name, controllerContext, operatorClient, kubeClient, kubeInformersForNamespaces, jobs.DefaultConditions) - } - - jobs.RunTNFJobController(ctx, tools.JobTypeSetup, nil, controllerContext, operatorClient, kubeClient, kubeInformersForNamespaces, jobs.AllConditions) - jobs.RunTNFJobController(ctx, tools.JobTypeFencing, nil, controllerContext, operatorClient, kubeClient, kubeInformersForNamespaces, jobs.DefaultConditions) - - // wait until the after-setup jobs finished, - // in order to avoid races with update jobs - return waitForTnfAfterSetupJobsCompletion(ctx, kubeClient, nodeList) -} - -func waitForEtcdBootstrapCompleted(ctx context.Context, operatorClient v1helpers.StaticPodOperatorClient) error { - isEtcdRunningInCluster, err := ceohelpers.IsEtcdRunningInCluster(ctx, operatorClient) - if err != nil { - return fmt.Errorf("failed to check if bootstrap is completed: %w", err) - } - if !isEtcdRunningInCluster { - klog.Infof("waiting for bootstrap to complete with etcd running in cluster") - clientConfig, err := rest.InClusterConfig() - if err != nil { - return fmt.Errorf("failed to get in-cluster config: %w", err) - } - err = bootstrapteardown.WaitForEtcdBootstrap(ctx, clientConfig) - if err != nil { - return fmt.Errorf("failed to wait for bootstrap to complete: %w", err) - } - } - return nil -} - -func updateSetup( - nodeList []*corev1.Node, - ctx context.Context, - controllerContext *controllercmd.ControllerContext, - operatorClient v1helpers.StaticPodOperatorClient, - kubeClient kubernetes.Interface, - kubeInformersForNamespaces v1helpers.KubeInformersForNamespaces, -) error { - - klog.Info("TNF was already setup, checking for needed changes for modified nodes") - - // re-run auth job on both nodes - for _, node := range nodeList { - klog.Infof("(Re-)running auth job on node %s", node.GetName()) - err := jobs.RestartJobOrRunController(ctx, tools.JobTypeAuth, &node.Name, controllerContext, operatorClient, kubeClient, kubeInformersForNamespaces, jobs.DefaultConditions, tools.AuthJobCompletedTimeout) - if err != nil { - return fmt.Errorf("failed to (re-)start auth job on node %s: %w", node.GetName(), err) - } - } - // wait for completion - for _, node := range nodeList { - err := jobs.WaitForCompletion(ctx, kubeClient, tools.JobTypeAuth.GetJobName(&node.Name), operatorclient.TargetNamespace, tools.AuthJobCompletedTimeout) - if err != nil { - return fmt.Errorf("failed to wait for auth job on node %s to complete: %w", node.Name, err) - } - } - - // run update-setup job on both nodes, it will detect on which node it needs to do something - for _, node := range nodeList { - klog.Infof("(Re-)running update setup job on node %s", node.GetName()) - err := jobs.RestartJobOrRunController(ctx, tools.JobTypeUpdateSetup, &node.Name, controllerContext, operatorClient, kubeClient, kubeInformersForNamespaces, jobs.DefaultConditions, tools.SetupJobCompletedTimeout) - if err != nil { - return fmt.Errorf("failed to (re-)start update setup job on node %s: %w", node.GetName(), err) - } - - } - // wait for completion - for _, node := range nodeList { - err := jobs.WaitForCompletion(ctx, kubeClient, tools.JobTypeUpdateSetup.GetJobName(&node.Name), operatorclient.TargetNamespace, tools.UpdateSetupJobCompletedTimeout) - if err != nil { - return fmt.Errorf("failed to wait for update setup job on node %s to complete: %w", node.GetName(), err) - } - } - - // run after-setup job on both nodes - for _, node := range nodeList { - klog.Infof("(Re-)running after setup job on node %s", node.GetName()) - err := jobs.RestartJobOrRunController(ctx, tools.JobTypeAfterSetup, &node.Name, controllerContext, operatorClient, kubeClient, kubeInformersForNamespaces, jobs.DefaultConditions, tools.AfterSetupJobCompletedTimeout) - if err != nil { - return fmt.Errorf("failed to (re-)start after setup job on node %s: %w", node.GetName(), err) - } - - } - // wait for completion - for _, node := range nodeList { - err := jobs.WaitForCompletion(ctx, kubeClient, tools.JobTypeAfterSetup.GetJobName(&node.Name), operatorclient.TargetNamespace, tools.AfterSetupJobCompletedTimeout) - if err != nil { - return fmt.Errorf("failed to wait for after setup job on node %s to complete: %w", node.GetName(), err) - } - } - - return nil -} - -// tnfSetupJobsExist checks if TNF was already set up by checking for any existing TNF jobs -func tnfSetupJobsExist(ctx context.Context, kubeClient kubernetes.Interface) (bool, error) { - // Check if any TNF jobs exist in the target namespace - jobsClient := kubeClient.BatchV1().Jobs(operatorclient.TargetNamespace) - list, err := jobsClient.List(ctx, v1.ListOptions{ - LabelSelector: "app.kubernetes.io/component=two-node-fencing-setup", - }) - if err != nil { - return false, err - } - return len(list.Items) > 0, nil -} - -func waitForTnfAfterSetupJobsCompletion(ctx context.Context, kubeClient kubernetes.Interface, nodeList []*corev1.Node) error { - for _, node := range nodeList { - jobName := tools.JobTypeAfterSetup.GetJobName(&node.Name) - klog.Infof("Waiting for after-setup job %s to complete", jobName) - if err := jobs.WaitForCompletion(ctx, kubeClient, jobName, operatorclient.TargetNamespace, tools.AllCompletedTimeout); err != nil { - return fmt.Errorf("failed to wait for after-setup job %s to complete: %w", jobName, err) - } - } - return nil -} diff --git a/pkg/tnf/operator/nodehandler_test.go b/pkg/tnf/operator/nodehandler_test.go deleted file mode 100644 index 0e510cd7ce..0000000000 --- a/pkg/tnf/operator/nodehandler_test.go +++ /dev/null @@ -1,343 +0,0 @@ -package operator - -import ( - "context" - "errors" - "testing" - "time" - - "github.com/stretchr/testify/require" - batchv1 "k8s.io/api/batch/v1" - corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/client-go/informers" - "k8s.io/client-go/kubernetes" - "k8s.io/client-go/kubernetes/fake" - "k8s.io/client-go/tools/cache" - - operatorv1 "github.com/openshift/api/operator/v1" - operatorversionedclientfake "github.com/openshift/client-go/operator/clientset/versioned/fake" - extinfops "github.com/openshift/client-go/operator/informers/externalversions" - operatorv1informers "github.com/openshift/client-go/operator/informers/externalversions/operator/v1" - "github.com/openshift/library-go/pkg/controller/controllercmd" - "github.com/openshift/library-go/pkg/operator/events" - "github.com/openshift/library-go/pkg/operator/v1helpers" - corev1listers "k8s.io/client-go/listers/core/v1" - "k8s.io/utils/clock" - - "github.com/openshift/cluster-etcd-operator/pkg/operator/ceohelpers" - "github.com/openshift/cluster-etcd-operator/pkg/operator/operatorclient" - u "github.com/openshift/cluster-etcd-operator/pkg/testutils" -) - -func TestHandleNodes(t *testing.T) { - tests := []struct { - name string - nodes []*corev1.Node - existingJobs []runtime.Object - mockStartControllers func() error - mockUpdateSetup func() error - expectError bool - expectStartControllers bool - expectUpdateSetup bool - errorContains string - }{ - { - name: "Less than 2 nodes - returns nil without action", - nodes: []*corev1.Node{ - createReadyNode("master-0"), - }, - existingJobs: []runtime.Object{}, - expectError: false, - expectStartControllers: false, - expectUpdateSetup: false, - }, - { - name: "More than 2 nodes - returns nil without action", - nodes: []*corev1.Node{ - createReadyNode("master-0"), - createReadyNode("master-1"), - createReadyNode("master-2"), - }, - existingJobs: []runtime.Object{}, - expectError: false, - expectStartControllers: false, - expectUpdateSetup: false, - }, - { - name: "2 nodes but first not ready - returns nil without action", - nodes: []*corev1.Node{ - createNotReadyNode("master-0"), - createReadyNode("master-1"), - }, - existingJobs: []runtime.Object{}, - expectError: false, - expectStartControllers: false, - expectUpdateSetup: false, - }, - { - name: "2 nodes but second not ready - returns nil without action", - nodes: []*corev1.Node{ - createReadyNode("master-0"), - createNotReadyNode("master-1"), - }, - existingJobs: []runtime.Object{}, - expectError: false, - expectStartControllers: false, - expectUpdateSetup: false, - }, - { - name: "2 ready nodes, no existing jobs - starts controllers only", - nodes: []*corev1.Node{ - createReadyNode("master-0"), - createReadyNode("master-1"), - }, - existingJobs: []runtime.Object{}, - mockStartControllers: func() error { - return nil - }, - expectError: false, - expectStartControllers: true, - expectUpdateSetup: false, - }, - { - name: "2 ready nodes, existing jobs - starts controllers and updates setup", - nodes: []*corev1.Node{ - createReadyNode("master-0"), - createReadyNode("master-1"), - }, - existingJobs: []runtime.Object{ - createTNFJob("tnf-setup"), - }, - mockStartControllers: func() error { - return nil - }, - mockUpdateSetup: func() error { - return nil - }, - expectError: false, - expectStartControllers: true, - expectUpdateSetup: true, - }, - { - name: "2 ready nodes - error starting controllers", - nodes: []*corev1.Node{ - createReadyNode("master-0"), - createReadyNode("master-1"), - }, - existingJobs: []runtime.Object{}, - mockStartControllers: func() error { - return errors.New("failed to start controllers") - }, - expectError: true, - expectStartControllers: true, - expectUpdateSetup: false, - errorContains: "failed to start TNF job controllers", - }, - { - name: "2 ready nodes, existing jobs - error updating setup", - nodes: []*corev1.Node{ - createReadyNode("master-0"), - createReadyNode("master-1"), - }, - existingJobs: []runtime.Object{ - createTNFJob("tnf-setup"), - }, - mockStartControllers: func() error { - return nil - }, - mockUpdateSetup: func() error { - return errors.New("failed to update") - }, - expectError: true, - expectStartControllers: true, - expectUpdateSetup: true, - errorContains: "failed to update pacemaker setup", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - // Setup - ctx := context.Background() - - // Create fake kubernetes client with jobs - fakeKubeClient := fake.NewSimpleClientset(tt.existingJobs...) - - // Create fake operator client - fakeOperatorClient := v1helpers.NewFakeStaticPodOperatorClient( - &operatorv1.StaticPodOperatorSpec{}, - u.StaticPodOperatorStatus( - u.WithLatestRevision(1), - u.WithNodeStatusAtCurrentRevision(1), - u.WithNodeStatusAtCurrentRevision(1), - ), - nil, - nil, - ) - - // Create controller context - eventRecorder := events.NewRecorder( - fakeKubeClient.CoreV1().Events(operatorclient.TargetNamespace), - "test-nodehandler", - &corev1.ObjectReference{}, - clock.RealClock{}, - ) - controllerContext := &controllercmd.ControllerContext{ - EventRecorder: eventRecorder, - } - - // Create node informer and lister - nodeInformer := informers.NewSharedInformerFactory(fakeKubeClient, 0).Core().V1().Nodes() - for _, node := range tt.nodes { - err := nodeInformer.Informer().GetIndexer().Add(node) - require.NoError(t, err) - } - controlPlaneNodeLister := corev1listers.NewNodeLister(nodeInformer.Informer().GetIndexer()) - - // Create etcd informer - operatorClientFake := operatorversionedclientfake.NewClientset() - etcdInformers := extinfops.NewSharedInformerFactory(operatorClientFake, 10*time.Minute) - etcdIndexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc}) - require.NoError(t, etcdIndexer.Add(&operatorv1.Etcd{ - ObjectMeta: metav1.ObjectMeta{ - Name: ceohelpers.InfrastructureClusterName, - }, - })) - etcdInformers.Operator().V1().Etcds().Informer().AddIndexers(etcdIndexer.GetIndexers()) - ctx2 := t.Context() - etcdInformers.Start(ctx2.Done()) - synced := etcdInformers.WaitForCacheSync(ctx2.Done()) - for v, ok := range synced { - require.True(t, ok, "cache failed to sync: %v", v) - } - - // Create kubeInformersForNamespaces - kubeInformersForNamespaces := v1helpers.NewKubeInformersForNamespaces( - fakeKubeClient, - "", - operatorclient.GlobalUserSpecifiedConfigNamespace, - operatorclient.GlobalMachineSpecifiedConfigNamespace, - operatorclient.TargetNamespace, - operatorclient.OperatorNamespace, - "kube-system", - ) - - // Track function calls - startControllersCalled := false - updateSetupCalled := false - - // Mock startTnfJobcontrollers - originalStartFunc := startTnfJobcontrollersFunc - if tt.mockStartControllers != nil { - startTnfJobcontrollersFunc = func( - nodeList []*corev1.Node, - ctx context.Context, - controllerContext *controllercmd.ControllerContext, - operatorClient v1helpers.StaticPodOperatorClient, - kubeClient kubernetes.Interface, - kubeInformersForNamespaces v1helpers.KubeInformersForNamespaces, - etcdInformer operatorv1informers.EtcdInformer, - ) error { - startControllersCalled = true - return tt.mockStartControllers() - } - } - defer func() { startTnfJobcontrollersFunc = originalStartFunc }() - - // Mock updateSetup - originalUpdateFunc := updateSetupFunc - if tt.mockUpdateSetup != nil { - updateSetupFunc = func( - nodeList []*corev1.Node, - ctx context.Context, - controllerContext *controllercmd.ControllerContext, - operatorClient v1helpers.StaticPodOperatorClient, - kubeClient kubernetes.Interface, - kubeInformersForNamespaces v1helpers.KubeInformersForNamespaces, - ) error { - updateSetupCalled = true - return tt.mockUpdateSetup() - } - } - defer func() { updateSetupFunc = originalUpdateFunc }() - - // Execute - err := handleNodes( - controllerContext, - controlPlaneNodeLister, - ctx, - fakeOperatorClient, - fakeKubeClient, - kubeInformersForNamespaces, - etcdInformers.Operator().V1().Etcds(), - ) - - // Verify - if tt.expectError { - require.Error(t, err, "Expected error but got none") - if tt.errorContains != "" { - require.Contains(t, err.Error(), tt.errorContains, - "Expected error to contain %q but got: %v", tt.errorContains, err) - } - } else { - require.NoError(t, err, "Expected no error but got: %v", err) - } - - require.Equal(t, tt.expectStartControllers, startControllersCalled, - "Expected startTnfJobcontrollers called=%v, but got=%v", - tt.expectStartControllers, startControllersCalled) - - require.Equal(t, tt.expectUpdateSetup, updateSetupCalled, - "Expected updateSetup called=%v, but got=%v", - tt.expectUpdateSetup, updateSetupCalled) - }) - } -} - -// Helper functions - -func createReadyNode(name string) *corev1.Node { - return &corev1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: name, - }, - Status: corev1.NodeStatus{ - Conditions: []corev1.NodeCondition{ - { - Type: corev1.NodeReady, - Status: corev1.ConditionTrue, - }, - }, - }, - } -} - -func createNotReadyNode(name string) *corev1.Node { - return &corev1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: name, - }, - Status: corev1.NodeStatus{ - Conditions: []corev1.NodeCondition{ - { - Type: corev1.NodeReady, - Status: corev1.ConditionFalse, - }, - }, - }, - } -} - -func createTNFJob(name string) *batchv1.Job { - return &batchv1.Job{ - ObjectMeta: metav1.ObjectMeta{ - Name: name, - Namespace: operatorclient.TargetNamespace, - Labels: map[string]string{ - "app.kubernetes.io/component": "two-node-fencing-setup", - }, - }, - } -} diff --git a/pkg/tnf/operator/starter.go b/pkg/tnf/operator/starter.go index eeb52f30f4..7929cf7f0e 100644 --- a/pkg/tnf/operator/starter.go +++ b/pkg/tnf/operator/starter.go @@ -1,28 +1,23 @@ package operator import ( - "bytes" "context" "fmt" "os" "time" - operatorv1 "github.com/openshift/api/operator/v1" configv1informers "github.com/openshift/client-go/config/informers/externalversions/config/v1" operatorv1informers "github.com/openshift/client-go/operator/informers/externalversions/operator/v1" "github.com/openshift/library-go/pkg/controller/controllercmd" "github.com/openshift/library-go/pkg/operator/resource/resourceapply" "github.com/openshift/library-go/pkg/operator/staticresourcecontroller" "github.com/openshift/library-go/pkg/operator/v1helpers" - batchv1 "k8s.io/api/batch/v1" - corev1 "k8s.io/api/core/v1" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" apiextensionsclient "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/wait" "k8s.io/client-go/dynamic" "k8s.io/client-go/kubernetes" - corev1listers "k8s.io/client-go/listers/core/v1" "k8s.io/client-go/tools/cache" "k8s.io/component-base/metrics" "k8s.io/component-base/metrics/legacyregistry" @@ -32,16 +27,8 @@ import ( "github.com/openshift/cluster-etcd-operator/pkg/etcdenvvar" "github.com/openshift/cluster-etcd-operator/pkg/operator/ceohelpers" "github.com/openshift/cluster-etcd-operator/pkg/operator/externaletcdsupportcontroller" - "github.com/openshift/cluster-etcd-operator/pkg/operator/operatorclient" - "github.com/openshift/cluster-etcd-operator/pkg/tnf/pkg/jobs" "github.com/openshift/cluster-etcd-operator/pkg/tnf/pkg/metriccontroller" "github.com/openshift/cluster-etcd-operator/pkg/tnf/pkg/pacemaker" - "github.com/openshift/cluster-etcd-operator/pkg/tnf/pkg/tools" -) - -const ( - // pacemakerStatusCollectorName is the name of the Pacemaker status collector CronJob - pacemakerStatusCollectorName = "pacemaker-status-collector" ) // HandleDualReplicaClusters checks feature gate and control plane topology, @@ -80,88 +67,12 @@ func HandleDualReplicaClusters( if err := runTnfResourceController(ctx, controllerContext, kubeClient, dynamicClient, operatorClient, kubeInformersForNamespaces); err != nil { return false, err } - runPacemakerControllers(ctx, controllerContext, operatorClient, kubeClient, dynamicClient, etcdInformer) - - // we need node names for assigning auth and after-setup jobs to specific nodes - controlPlaneNodeLister := corev1listers.NewNodeLister(controlPlaneNodeInformer.GetIndexer()) - klog.Infof("watching for nodes...") - _, err = controlPlaneNodeInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{ - AddFunc: func(obj any) { - node, ok := obj.(*corev1.Node) - if !ok { - klog.Warningf("failed to convert added object to Node %+v", obj) - return - } - - // ignore nodes which are not ready yet - if !tools.IsNodeReady(node) { - klog.Infof("added node %s is not ready yet, skipping handling", node.GetName()) - return - } - - // this potentially needs some time when we wait for etcd bootstrap to complete, so run it in a goroutine, - // to not block the event handler - klog.Infof("node added and ready: %s", node.GetName()) - go handleNodesWithRetry(controllerContext, controlPlaneNodeLister, ctx, operatorClient, kubeClient, kubeInformersForNamespaces, etcdInformer) - }, - UpdateFunc: func(oldObj, newObj any) { - oldNode, oldOk := oldObj.(*corev1.Node) - newNode, newOk := newObj.(*corev1.Node) - if !oldOk || !newOk { - klog.Warningf("failed to convert updated object to Node, old=%+v, new=%+v", oldObj, newObj) - return - } - - // only handle if node transitioned from not ready to ready - oldReady := tools.IsNodeReady(oldNode) - newReady := tools.IsNodeReady(newNode) - if !oldReady && newReady { - klog.Infof("node %s transitioned to ready state", newNode.GetName()) - // this potentially needs some time when we wait for etcd bootstrap to complete, so run it in a goroutine, - // to not block the event handler - go handleNodesWithRetry(controllerContext, controlPlaneNodeLister, ctx, operatorClient, kubeClient, kubeInformersForNamespaces, etcdInformer) - } - }, - DeleteFunc: func(obj any) { - node, ok := obj.(*corev1.Node) - if !ok { - klog.Warningf("failed to convert deleted object to Node %+v", obj) - return - } - klog.Infof("node deleted: %s", node.GetName()) - - // always handle node deletion - // this potentially needs some time when we wait for etcd bootstrap to complete, so run it in a goroutine, - // to not block the event handler - go handleNodesWithRetry(controllerContext, controlPlaneNodeLister, ctx, operatorClient, kubeClient, kubeInformersForNamespaces, etcdInformer) - }, - }) - if err != nil { - klog.Errorf("failed to add eventhandler to control plane informer: %v", err) - return false, err - } - - // we need to update fencing config when fencing secrets change - // adding a secret informer to the jobcontroller would trigger fencing setup for *every* secret change, - // that's why we do it with an event handler - klog.Infof("watching for secrets...") - _, err = kubeInformersForNamespaces.InformersFor(operatorclient.TargetNamespace).Core().V1().Secrets().Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{ - AddFunc: func(obj any) { - go handleFencingSecretChange(ctx, nil, obj, controllerContext, operatorClient, kubeClient, kubeInformersForNamespaces) - }, - UpdateFunc: func(oldObj, newObj any) { - go handleFencingSecretChange(ctx, oldObj, newObj, controllerContext, operatorClient, kubeClient, kubeInformersForNamespaces) - }, - DeleteFunc: func(obj any) { - // nothing to do - // removing orphaned fence devices is handled by update-setup job - // when handling node replacements - }, - }) - if err != nil { - klog.Errorf("failed to add eventhandler to secrets informer: %v", err) - return false, err - } + // Start pacemaker controllers (lifecycle manager, status collector) + // PacemakerLifecycleManager handles ALL node lifecycle events: + // - UpdateFunc: Ready transitions for initial bootstrap + // - AddFunc/DeleteFunc: drift-driven reconciliation + // Secret handler registration happens inside runPacemakerControllers after lifecycleManager is created + runPacemakerControllers(ctx, controllerContext, operatorClient, kubeClient, kubeInformersForNamespaces, etcdInformer, controlPlaneNodeInformer, dynamicClient) return true, nil } @@ -223,35 +134,12 @@ func runTnfResourceController(ctx context.Context, controllerContext *controller return nil } -func runPacemakerControllers(ctx context.Context, controllerContext *controllercmd.ControllerContext, operatorClient v1helpers.StaticPodOperatorClient, kubeClient kubernetes.Interface, dynamicClient dynamic.Interface, etcdInformer operatorv1informers.EtcdInformer) { - // Wait for external etcd transition before creating any Pacemaker controllers +func runPacemakerControllers(ctx context.Context, controllerContext *controllercmd.ControllerContext, operatorClient v1helpers.StaticPodOperatorClient, kubeClient kubernetes.Interface, kubeInformersForNamespaces v1helpers.KubeInformersForNamespaces, etcdInformer operatorv1informers.EtcdInformer, controlPlaneNodeInformer cache.SharedIndexInformer, dynamicClient dynamic.Interface) { + // Pacemaker controllers start after PacemakerCluster CRD is established. + // The lifecycle manager's sync() handles bootstrap vs post-transition modes internally. // This runs in a background goroutine to avoid blocking the main thread. go func() { - klog.Infof("waiting for etcd to transition to external before creating Pacemaker controllers") - - // Wait for external etcd transition to complete - for { - if err := ceohelpers.WaitForEtcdCondition( - ctx, etcdInformer, operatorClient, ceohelpers.HasExternalEtcdCompletedTransition, - 10*time.Second, 30*time.Minute, "external etcd transition", - ); err != nil { - if ctx.Err() != nil { - klog.Infof("context done while waiting for external etcd transition: %v", err) - return - } - klog.Warningf("external etcd transition not complete yet, will retry in 1m: %v", err) - select { - case <-time.After(time.Minute): - continue - case <-ctx.Done(): - return - } - } - // External etcd transition is complete, break out of the wait loop - break - } - - klog.Infof("etcd has transitioned to external; verifying PacemakerCluster CRD is established") + klog.Infof("waiting for PacemakerCluster CRD to be established before starting Pacemaker controllers") // The PacemakerCluster CRD is applied by the static resource controller. // Wait for it to be established before starting the informer. @@ -261,61 +149,51 @@ func runPacemakerControllers(ctx context.Context, controllerContext *controllerc return } - // Retry until the CRD is established or context is cancelled - for { - err = wait.PollUntilContextTimeout(ctx, 2*time.Second, time.Minute, true, func(ctx context.Context) (bool, error) { - crd, getErr := apiextClient.ApiextensionsV1().CustomResourceDefinitions().Get(ctx, "pacemakerclusters.etcd.openshift.io", metav1.GetOptions{}) - if getErr != nil { - klog.V(2).Infof("waiting for PacemakerCluster CRD: %v", getErr) - return false, nil - } - for _, cond := range crd.Status.Conditions { - if cond.Type == apiextensionsv1.Established && cond.Status == apiextensionsv1.ConditionTrue { - return true, nil - } - } - klog.V(2).Infof("PacemakerCluster CRD not yet established") + // Wait for CRD to be established. + err = wait.PollUntilContextCancel(ctx, 5*time.Second, true, func(ctx context.Context) (bool, error) { + crd, getErr := apiextClient.ApiextensionsV1().CustomResourceDefinitions().Get(ctx, "pacemakerclusters.etcd.openshift.io", metav1.GetOptions{}) + if getErr != nil { + klog.V(2).Infof("waiting for PacemakerCluster CRD: %v", getErr) return false, nil - }) - if err == nil { - // CRD is established, break out of the retry loop - break } - if ctx.Err() != nil { - klog.Infof("context done while waiting for PacemakerCluster CRD: %v", err) - return - } - klog.Warningf("PacemakerCluster CRD not established yet, will retry in 30s: %v", err) - select { - case <-time.After(30 * time.Second): - continue - case <-ctx.Done(): - return + for _, cond := range crd.Status.Conditions { + if cond.Type == apiextensionsv1.Established && cond.Status == apiextensionsv1.ConditionTrue { + return true, nil + } } + klog.V(2).Infof("PacemakerCluster CRD not yet established") + return false, nil + }) + if err != nil { + klog.Infof("context done while waiting for PacemakerCluster CRD: %v", err) + return } + klog.Infof("PacemakerCluster CRD is established") - // Now create the healthcheck controller after transition is complete - klog.Infof("creating Pacemaker healthcheck controller") - healthCheckController, pacemakerInformer, err := pacemaker.NewHealthCheck( + // Prerequisites met: create and start lifecycle manager controller. + lifecycleController, _, pacemakerInformer, err := newPacemakerLifecycleManager( operatorClient, kubeClient, controllerContext.EventRecorder, controllerContext.KubeConfig, + controlPlaneNodeInformer, + controllerContext, + kubeInformersForNamespaces, + etcdInformer, ) if err != nil { - klog.Errorf("Failed to create Pacemaker healthcheck controller: %v", err) - return + klog.Fatalf("Failed to create Pacemaker lifecycle manager: %v", err) } - // Start the PacemakerCluster informer now that the CRD exists - // The controller will wait for this informer to sync before processing events - klog.Infof("starting PacemakerCluster informer") + // Start the PacemakerCluster informer (controller waits for sync before processing events). go pacemakerInformer.Run(ctx.Done()) - // Start the healthcheck controller - klog.Infof("starting Pacemaker healthcheck controller") - go healthCheckController.Run(ctx, 1) + // Start the lifecycle manager controller. + // PacemakerLifecycleManager.sync() handles both bootstrap and post-transition: + // - Bootstrap: StartJobControllers() drives external etcd transition + // - Post-transition: MonitorHealth(), ReconcilePacemakerConfig(), CleanupOrphanedJobs() + go lifecycleController.Run(ctx, 1) // Create and start the metrics controller, sharing the same informer klog.Infof("creating Pacemaker metrics controller") @@ -337,108 +215,9 @@ func runPacemakerControllers(ctx context.Context, controllerContext *controllerc klog.Infof("starting Pacemaker console notification controller") go notificationController.Run(ctx, 1) - // Start the status collector CronJob - klog.Infof("starting Pacemaker status collector CronJob") - runPacemakerStatusCollectorCronJob(ctx, controllerContext, operatorClient, kubeClient) - }() -} - -func runPacemakerStatusCollectorCronJob(ctx context.Context, controllerContext *controllercmd.ControllerContext, operatorClient v1helpers.StaticPodOperatorClient, kubeClient kubernetes.Interface) { - // Start the cronjob controller to create a CronJob for periodic status collection - // The CronJob runs "tnf-monitor collect" which executes "sudo -n pcs status xml" - // and updates the PacemakerCluster CR - klog.Infof("starting Pacemaker status collector CronJob controller") - statusCronJobController := jobs.NewCronJobController( - pacemakerStatusCollectorName, - bindata.MustAsset("tnfdeployment/cronjob.yaml"), - operatorClient, - kubeClient, - controllerContext.EventRecorder, - func(_ *operatorv1.OperatorSpec, cronJob *batchv1.CronJob) error { - // Set the name and namespace - cronJob.SetName(pacemakerStatusCollectorName) - cronJob.SetNamespace(operatorclient.TargetNamespace) - - // Set the schedule - run every minute - cronJob.Spec.Schedule = "* * * * *" - - // Initialize labels maps if nil and set labels at all levels - if cronJob.Labels == nil { - cronJob.Labels = make(map[string]string) - } - cronJob.Labels["app.kubernetes.io/name"] = pacemakerStatusCollectorName - - if cronJob.Spec.JobTemplate.Labels == nil { - cronJob.Spec.JobTemplate.Labels = make(map[string]string) - } - cronJob.Spec.JobTemplate.Labels["app.kubernetes.io/name"] = pacemakerStatusCollectorName - - if cronJob.Spec.JobTemplate.Spec.Template.Labels == nil { - cronJob.Spec.JobTemplate.Spec.Template.Labels = make(map[string]string) - } - cronJob.Spec.JobTemplate.Spec.Template.Labels["app.kubernetes.io/name"] = pacemakerStatusCollectorName - - // Configure the container - cronJob.Spec.JobTemplate.Spec.Template.Spec.Containers[0].Image = os.Getenv("OPERATOR_IMAGE") - cronJob.Spec.JobTemplate.Spec.Template.Spec.Containers[0].Command = []string{"tnf-monitor", "collect", "-v=4"} - - return nil - }, - ) - go statusCronJobController.Run(ctx, 1) -} + // Note: Status collector is started by lifecycle manager only after transition is complete + // (see startJobControllersWithLock in job_controllers.go) -func handleFencingSecretChange( - ctx context.Context, - oldObj, obj any, - controllerContext *controllercmd.ControllerContext, - operatorClient v1helpers.StaticPodOperatorClient, - kubeClient kubernetes.Interface, - kubeInformersForNamespaces v1helpers.KubeInformersForNamespaces, -) { - - // obj can be nil, always restart fencing job in that case - if obj != nil { - secret, ok := obj.(*corev1.Secret) - if !ok { - klog.Warningf("failed to convert added / modified / deleted object to Secret %+v", obj) - return - } - if !tools.IsFencingSecret(secret.GetName()) { - // nothing to do - return - } - - if oldObj != nil { - oldSecret, ok := oldObj.(*corev1.Secret) - if !ok { - klog.Warningf("failed to convert old object to Secret %+v", oldObj) - } - // check if data changed - changed := false - if len(oldSecret.Data) != len(secret.Data) { - changed = true - } else { - for key, oldValue := range oldSecret.Data { - newValue, exists := secret.Data[key] - if !exists || !bytes.Equal(oldValue, newValue) { - changed = true - break - } - } - } - if !changed { - return - } - klog.Infof("handling modified fencing secret %s", secret.GetName()) - } else { - klog.Infof("handling added or deleted fencing secret %s", secret.GetName()) - } - } - - err := jobs.RestartJobOrRunController(ctx, tools.JobTypeFencing, nil, controllerContext, operatorClient, kubeClient, kubeInformersForNamespaces, jobs.DefaultConditions, tools.FencingJobCompletedTimeout) - if err != nil { - klog.Errorf("failed to restart fencing job: %v", err) - return - } + klog.Infof("started Pacemaker controllers (lifecycle manager, metrics controller, console notification)") + }() } diff --git a/pkg/tnf/operator/starter_test.go b/pkg/tnf/operator/starter_test.go deleted file mode 100644 index fffb46c346..0000000000 --- a/pkg/tnf/operator/starter_test.go +++ /dev/null @@ -1,382 +0,0 @@ -package operator - -import ( - "context" - "errors" - "maps" - "slices" - "testing" - "time" - - "github.com/stretchr/testify/require" - "k8s.io/apimachinery/pkg/util/wait" - - configv1 "github.com/openshift/api/config/v1" - operatorv1 "github.com/openshift/api/operator/v1" - - fakeconfig "github.com/openshift/client-go/config/clientset/versioned/fake" - "github.com/openshift/client-go/config/informers/externalversions" - configv1informers "github.com/openshift/client-go/config/informers/externalversions/config/v1" - v1 "github.com/openshift/client-go/config/informers/externalversions/config/v1" - operatorversionedclientfake "github.com/openshift/client-go/operator/clientset/versioned/fake" - extinfops "github.com/openshift/client-go/operator/informers/externalversions" - operatorv1informers "github.com/openshift/client-go/operator/informers/externalversions/operator/v1" - "github.com/openshift/library-go/pkg/controller/controllercmd" - "github.com/openshift/library-go/pkg/operator/events" - "github.com/openshift/library-go/pkg/operator/v1helpers" - corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/client-go/dynamic" - fakedynamic "k8s.io/client-go/dynamic/fake" - corev1informers "k8s.io/client-go/informers/core/v1" - "k8s.io/client-go/kubernetes" - "k8s.io/client-go/kubernetes/fake" - "k8s.io/client-go/kubernetes/scheme" - corev1listers "k8s.io/client-go/listers/core/v1" - "k8s.io/client-go/rest" - "k8s.io/client-go/tools/cache" - "k8s.io/utils/clock" - - "github.com/openshift/cluster-etcd-operator/pkg/etcdenvvar" - "github.com/openshift/cluster-etcd-operator/pkg/operator/ceohelpers" - "github.com/openshift/cluster-etcd-operator/pkg/operator/operatorclient" - u "github.com/openshift/cluster-etcd-operator/pkg/testutils" - "github.com/openshift/cluster-etcd-operator/pkg/tnf/pkg/jobs" -) - -type args struct { - ctx context.Context - controllerContext *controllercmd.ControllerContext - infrastructureInformer configv1informers.InfrastructureInformer - operatorClient v1helpers.StaticPodOperatorClient - envVarGetter etcdenvvar.EnvVar - kubeInformersForNamespaces v1helpers.KubeInformersForNamespaces - networkInformer v1.NetworkInformer - controlPlaneNodeInformer cache.SharedIndexInformer - etcdInformer operatorv1informers.EtcdInformer - kubeClient kubernetes.Interface - dynamicClient dynamic.Interface - initErr error -} - -func TestHandleDualReplicaClusters(t *testing.T) { - tests := []struct { - name string - args args - wantHandlerInitErr bool - wantStarted bool - wantErr bool - }{ - { - name: "Normal cluster", - args: getArgs(t, false), - wantHandlerInitErr: false, - wantStarted: false, - wantErr: false, - }, - { - name: "DualReplica topology", - args: getArgs(t, true), - wantHandlerInitErr: false, - wantStarted: true, - wantErr: false, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - - if tt.wantHandlerInitErr && tt.args.initErr == nil || !tt.wantHandlerInitErr && tt.args.initErr != nil { - t.Errorf("NewDualReplicaClusterHandler handlerInitErr = %v, wantHandlerInitErr %v", tt.args.initErr, tt.wantHandlerInitErr) - } - if tt.wantHandlerInitErr { - return - } - - started, err := HandleDualReplicaClusters( - tt.args.ctx, - tt.args.controllerContext, - tt.args.infrastructureInformer, - tt.args.operatorClient, - tt.args.envVarGetter, - tt.args.kubeInformersForNamespaces, - tt.args.networkInformer, - tt.args.controlPlaneNodeInformer, - tt.args.etcdInformer, - tt.args.kubeClient, - tt.args.dynamicClient) - - if started != tt.wantStarted { - t.Errorf("HandleDualReplicaClusters() started = %v, wantStarted %v", started, tt.wantStarted) - } - - if (err != nil) != tt.wantErr { - t.Errorf("HandleDualReplicaClusters() error = %v, wantErr %v", err, tt.wantErr) - } - }) - } -} - -func TestSetupJobConditionsBasedOnExternalEtcd(t *testing.T) { - tests := []struct { - name string - isReadyForEtcdTransition bool - expectedAvailableInSetup bool - }{ - { - name: "Job sets the Available condition when not ready for etcd transition", - isReadyForEtcdTransition: false, - expectedAvailableInSetup: true, - }, - { - name: "Job does not set the Available condition when ready for etcd transition", - isReadyForEtcdTransition: true, - expectedAvailableInSetup: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - // Set up operator status with the appropriate condition - operatorStatus := &operatorv1.StaticPodOperatorStatus{} - if tt.isReadyForEtcdTransition { - operatorStatus.Conditions = []operatorv1.OperatorCondition{ - { - Type: ceohelpers.OperatorConditionExternalEtcdHasCompletedTransition, - Status: operatorv1.ConditionTrue, - }, - } - } - - fakeOperatorClient := v1helpers.NewFakeStaticPodOperatorClient( - &operatorv1.StaticPodOperatorSpec{}, - operatorStatus, - nil, - nil, - ) - - hasExternalEtcdCompletedTransition, err := ceohelpers.HasExternalEtcdCompletedTransition(context.Background(), fakeOperatorClient) - if err != nil { - t.Errorf("failed to get external etcd status: %v", err) - } - - // Determine setup conditions based on the etcd transition status - setupConditions := jobs.DefaultConditions - if !hasExternalEtcdCompletedTransition { - setupConditions = jobs.AllConditions - } - - hasAvailableCondition := slices.Contains(setupConditions, operatorv1.OperatorStatusTypeAvailable) - - require.Equalf(t, tt.expectedAvailableInSetup, hasAvailableCondition, - "Setup job should have Available condition: %v, but got: %v", - tt.expectedAvailableInSetup, hasAvailableCondition) - }) - } -} - -func getArgs(t *testing.T, dualReplicaControlPlaneEnabled bool) args { - - fakeKubeClient := fake.NewSimpleClientset() - fakeDynamicClient := fakedynamic.NewSimpleDynamicClient(scheme.Scheme) - - cpt := configv1.HighlyAvailableTopologyMode - if dualReplicaControlPlaneEnabled { - cpt = configv1.DualReplicaTopologyMode - } - infra := &configv1.Infrastructure{ - ObjectMeta: metav1.ObjectMeta{ - Name: ceohelpers.InfrastructureClusterName, - }, - Status: configv1.InfrastructureStatus{ - ControlPlaneTopology: cpt, - }, - } - fakeConfigClient := fakeconfig.NewSimpleClientset([]runtime.Object{infra}...) - - fakeOperatorClient := v1helpers.NewFakeStaticPodOperatorClient( - &operatorv1.StaticPodOperatorSpec{}, - u.StaticPodOperatorStatus(), - nil, - nil, - ) - - eventRecorder := events.NewRecorder(fakeKubeClient.CoreV1().Events(operatorclient.TargetNamespace), - "test-tnfcontrollers", &corev1.ObjectReference{}, clock.RealClock{}) - - envVar := etcdenvvar.FakeEnvVar{} - - kubeInformersForNamespaces := v1helpers.NewKubeInformersForNamespaces( - fakeKubeClient, - "", - operatorclient.GlobalUserSpecifiedConfigNamespace, - operatorclient.GlobalMachineSpecifiedConfigNamespace, - operatorclient.TargetNamespace, - operatorclient.OperatorNamespace, - "kube-system", - ) - - controlPlaneNodeInformer := corev1informers.NewFilteredNodeInformer(fakeKubeClient, 1*time.Hour, cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc}, func(listOptions *metav1.ListOptions) { - listOptions.LabelSelector = "node-role.kubernetes.io/master" - }) - - etcdIndexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc}) - require.NoError(t, etcdIndexer.Add(&operatorv1.Etcd{ - TypeMeta: metav1.TypeMeta{}, - ObjectMeta: metav1.ObjectMeta{ - Name: ceohelpers.InfrastructureClusterName, - }, - })) - - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - - operatorClientFake := operatorversionedclientfake.NewClientset() - etcdInformers := extinfops.NewSharedInformerFactory(operatorClientFake, 10*time.Minute) - etcdInformers.Operator().V1().Etcds().Informer().AddIndexers(etcdIndexer.GetIndexers()) - etcdInformers.Start(ctx.Done()) - - configInformers := externalversions.NewSharedInformerFactory(fakeConfigClient, 10*time.Minute) - configInformers.Config().V1().Infrastructures().Informer().AddIndexers(cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc}) - networkInformer := configInformers.Config().V1().Networks() - configInformers.Start(ctx.Done()) - synced := configInformers.WaitForCacheSync(ctx.Done()) - maps.Copy(synced, etcdInformers.WaitForCacheSync(ctx.Done())) - - for v, ok := range synced { - if !ok { - t.Errorf("caches failed to sync: %v", v) - } - } - - return args{ - initErr: nil, // Default to no error - ctx: context.Background(), - controllerContext: &controllercmd.ControllerContext{ - EventRecorder: eventRecorder, - KubeConfig: &rest.Config{Host: "https://localhost:6443"}, - }, - infrastructureInformer: configInformers.Config().V1().Infrastructures(), - operatorClient: fakeOperatorClient, - envVarGetter: envVar, - kubeInformersForNamespaces: kubeInformersForNamespaces, - networkInformer: networkInformer, - controlPlaneNodeInformer: controlPlaneNodeInformer, - etcdInformer: etcdInformers.Operator().V1().Etcds(), - kubeClient: fakeKubeClient, - dynamicClient: fakeDynamicClient, - } -} - -func TestHandleNodesWithRetry(t *testing.T) { - tests := []struct { - name string - setupMockHandleNodes func() func(*controllercmd.ControllerContext, corev1listers.NodeLister, context.Context, v1helpers.StaticPodOperatorClient, kubernetes.Interface, v1helpers.KubeInformersForNamespaces, operatorv1informers.EtcdInformer) error - expectDegradedCondition bool - expectDegradedStatus operatorv1.ConditionStatus - expectRetries bool - }{ - { - name: "Success on first attempt", - setupMockHandleNodes: func() func(*controllercmd.ControllerContext, corev1listers.NodeLister, context.Context, v1helpers.StaticPodOperatorClient, kubernetes.Interface, v1helpers.KubeInformersForNamespaces, operatorv1informers.EtcdInformer) error { - return func(_ *controllercmd.ControllerContext, _ corev1listers.NodeLister, _ context.Context, _ v1helpers.StaticPodOperatorClient, _ kubernetes.Interface, _ v1helpers.KubeInformersForNamespaces, _ operatorv1informers.EtcdInformer) error { - return nil - } - }, - expectDegradedCondition: true, - expectDegradedStatus: operatorv1.ConditionFalse, - expectRetries: false, - }, - { - name: "Success after retries", - setupMockHandleNodes: func() func(*controllercmd.ControllerContext, corev1listers.NodeLister, context.Context, v1helpers.StaticPodOperatorClient, kubernetes.Interface, v1helpers.KubeInformersForNamespaces, operatorv1informers.EtcdInformer) error { - attemptCount := 0 - return func(_ *controllercmd.ControllerContext, _ corev1listers.NodeLister, _ context.Context, _ v1helpers.StaticPodOperatorClient, _ kubernetes.Interface, _ v1helpers.KubeInformersForNamespaces, _ operatorv1informers.EtcdInformer) error { - attemptCount++ - if attemptCount < 3 { - return errors.New("temporary failure") - } - return nil - } - }, - expectDegradedCondition: true, - expectDegradedStatus: operatorv1.ConditionFalse, - expectRetries: true, - }, - { - name: "Failure after all retries", - setupMockHandleNodes: func() func(*controllercmd.ControllerContext, corev1listers.NodeLister, context.Context, v1helpers.StaticPodOperatorClient, kubernetes.Interface, v1helpers.KubeInformersForNamespaces, operatorv1informers.EtcdInformer) error { - return func(_ *controllercmd.ControllerContext, _ corev1listers.NodeLister, _ context.Context, _ v1helpers.StaticPodOperatorClient, _ kubernetes.Interface, _ v1helpers.KubeInformersForNamespaces, _ operatorv1informers.EtcdInformer) error { - return errors.New("persistent failure") - } - }, - expectDegradedCondition: true, - expectDegradedStatus: operatorv1.ConditionTrue, - expectRetries: true, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - // Setup test environment - testArgs := getArgs(t, true) - - // Create NodeLister from the informer - controlPlaneNodeLister := corev1listers.NewNodeLister(testArgs.controlPlaneNodeInformer.GetIndexer()) - - // Store original handleNodesFunc and replace with mock - originalHandleNodesFunc := handleNodesFunc - handleNodesFunc = tt.setupMockHandleNodes() - defer func() { handleNodesFunc = originalHandleNodesFunc }() - - // Store original backoff config and use faster settings for testing - originalBackoff := retryBackoffConfig - retryBackoffConfig = wait.Backoff{ - Duration: 100 * time.Millisecond, - Factor: 2.0, - Steps: 5, // Much shorter for testing - Cap: 500 * time.Millisecond, - } - defer func() { retryBackoffConfig = originalBackoff }() - - // For faster testing, use a reasonable timeout - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) - defer cancel() - - // Run handleNodesWithRetry - handleNodesWithRetry(testArgs.controllerContext, controlPlaneNodeLister, ctx, testArgs.operatorClient, - testArgs.kubeClient, testArgs.kubeInformersForNamespaces, testArgs.etcdInformer) - - // Verify the operator condition was set correctly - if tt.expectDegradedCondition { - _, status, _, err := testArgs.operatorClient.GetStaticPodOperatorState() - require.NoError(t, err, "failed to get operator status") - - // Find the TNFJobControllersDegraded condition - var foundCondition *operatorv1.OperatorCondition - for i, condition := range status.Conditions { - if condition.Type == "TNFJobControllersDegraded" { - foundCondition = &status.Conditions[i] - break - } - } - - require.NotNil(t, foundCondition, "TNFJobControllersDegraded condition not found") - require.Equal(t, tt.expectDegradedStatus, foundCondition.Status, - "Expected degraded status %v but got %v", tt.expectDegradedStatus, foundCondition.Status) - - if tt.expectDegradedStatus == operatorv1.ConditionTrue { - require.Equal(t, "SetupFailed", foundCondition.Reason, - "Expected reason SetupFailed but got %s", foundCondition.Reason) - require.Contains(t, foundCondition.Message, "Failed to setup TNF job controllers", - "Expected message to contain failure info") - } else { - require.Equal(t, "AsExpected", foundCondition.Reason, - "Expected reason AsExpected but got %s", foundCondition.Reason) - require.Contains(t, foundCondition.Message, "successfully", - "Expected success message") - } - } - }) - } -} diff --git a/pkg/tnf/operator/status_collector.go b/pkg/tnf/operator/status_collector.go new file mode 100644 index 0000000000..c2a03870df --- /dev/null +++ b/pkg/tnf/operator/status_collector.go @@ -0,0 +1,231 @@ +package operator + +import ( + "context" + "os" + + operatorv1 "github.com/openshift/api/operator/v1" + "github.com/openshift/cluster-etcd-operator/bindata" + "github.com/openshift/cluster-etcd-operator/pkg/operator/operatorclient" + "github.com/openshift/cluster-etcd-operator/pkg/tnf/pkg/jobs" + "github.com/openshift/cluster-etcd-operator/pkg/tnf/pkg/tools" + batchv1 "k8s.io/api/batch/v1" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes" + "k8s.io/klog/v2" +) + +const ( + pacemakerStatusCollectorName = "pacemaker-status-collector" +) + +var ( + // statusCollectorState tracks node rotation state for the status collector CronJob. + // Strategy: stick with success, rotate to next node on failure. + // MaxRetryAttempts is not set - status collector failures don't trigger Degraded condition directly. + // Instead, staleness is detected via PacemakerCluster CR's Status.LastUpdated timestamp + // in the lifecycle manager (see isPacemakerCRStale in helpers.go). + statusCollectorState = &jobs.JobRetryState{ + NodeIndex: 0, + } +) + +// runPacemakerStatusCollectorCronJob starts the status collector CronJob. +// Runs "tnf-monitor collect" every minute to update PacemakerCluster CR. +// Only starts if not already started (idempotent). +func (c *pacemakerLifecycleManager) runPacemakerStatusCollectorCronJob(ctx context.Context) { + // Prevent duplicate starts + c.statusCollectorMu.Lock() + if c.statusCollectorStarted { + c.statusCollectorMu.Unlock() + klog.V(4).Infof("Status collector already started, skipping duplicate start") + return + } + c.statusCollectorStarted = true + c.statusCollectorMu.Unlock() + + schedulableNodesFunc := func() ([]*corev1.Node, error) { + return c.getActivePacemakerNodes() + } + statusCronJobController := jobs.NewCronJobController( + pacemakerStatusCollectorName, + bindata.MustAsset("tnfdeployment/cronjob.yaml"), + c.operatorClient, + c.kubeClient, + c.controllerContext.EventRecorder, + func(_ *operatorv1.OperatorSpec, cronJob *batchv1.CronJob) error { + cronJob.SetName(pacemakerStatusCollectorName) + cronJob.SetNamespace(operatorclient.TargetNamespace) + + cronJob.Spec.Schedule = "* * * * *" + + if cronJob.Labels == nil { + cronJob.Labels = make(map[string]string) + } + cronJob.Labels["app.kubernetes.io/name"] = pacemakerStatusCollectorName + + if cronJob.Spec.JobTemplate.Labels == nil { + cronJob.Spec.JobTemplate.Labels = make(map[string]string) + } + cronJob.Spec.JobTemplate.Labels["app.kubernetes.io/name"] = pacemakerStatusCollectorName + + if cronJob.Spec.JobTemplate.Spec.Template.Labels == nil { + cronJob.Spec.JobTemplate.Spec.Template.Labels = make(map[string]string) + } + cronJob.Spec.JobTemplate.Spec.Template.Labels["app.kubernetes.io/name"] = pacemakerStatusCollectorName + + cronJob.Spec.JobTemplate.Spec.Template.Spec.Containers[0].Image = os.Getenv("OPERATOR_IMAGE") + cronJob.Spec.JobTemplate.Spec.Template.Spec.Containers[0].Command = []string{"tnf-monitor", "collect"} + + // Get schedulable nodes (K8s ∩ Pacemaker intersection, ready only) + // Nodes are already sorted by getActivePacemakerNodes() + schedulableNodes, err := schedulableNodesFunc() + if err != nil || len(schedulableNodes) == 0 { + klog.V(4).Infof("Failed to determine schedulable nodes for status collector: %v - falling back to nodeSelector from manifest", err) + return nil + } + + currentNodeNames := tools.GetNodeNames(schedulableNodes) + + // Check last job status before acquiring lock (avoid blocking I/O while holding lock) + lastJobFailed, failedJob, err := checkLastJobFailedWithJob(ctx, c.kubeClient) + if err != nil { + klog.V(4).Infof("Failed to check last job status: %v - using current node index", err) + } + + statusCollectorState.Mu.Lock() + newIndex, newTargetNodes := nextStatusCollectorNodeIndex( + statusCollectorState.NodeIndex, + statusCollectorState.TargetNodes, + currentNodeNames, + err == nil && lastJobFailed, + ) + + // Log state changes + if !tools.StringSlicesEqual(statusCollectorState.TargetNodes, newTargetNodes) { + klog.Infof("Status collector: node list changed from %v to %v - resetting to first node", + statusCollectorState.TargetNodes, newTargetNodes) + } else if err == nil && lastJobFailed { + klog.Infof("Status collector: last job failed, rotating to node index %d (%s)", + newIndex, schedulableNodes[newIndex].Name) + } + + statusCollectorState.NodeIndex = newIndex + statusCollectorState.TargetNodes = newTargetNodes + targetNodeIndex := newIndex + statusCollectorState.Mu.Unlock() + + // Delete failed job to unblock CronJob with Forbid concurrencyPolicy + // Must be done after rotation state is updated (above) so we don't lose track of the failure + if err == nil && lastJobFailed && failedJob != nil { + deleteErr := c.kubeClient.BatchV1().Jobs(operatorclient.TargetNamespace).Delete(ctx, failedJob.Name, metav1.DeleteOptions{ + PropagationPolicy: &[]metav1.DeletionPropagation{metav1.DeletePropagationBackground}[0], + }) + if deleteErr != nil && !apierrors.IsNotFound(deleteErr) { + klog.Warningf("Failed to delete failed job %s: %v - CronJob may be blocked by Forbid policy", failedJob.Name, deleteErr) + } else { + klog.V(4).Infof("Deleted failed job %s to unblock CronJob", failedJob.Name) + } + } + + // Pin to target node (sticky on success, rotate on failure) + targetNode := schedulableNodes[targetNodeIndex].Name + cronJob.Spec.JobTemplate.Spec.Template.Spec.NodeName = targetNode + // Clear affinity to ensure NodeName takes precedence + cronJob.Spec.JobTemplate.Spec.Template.Spec.Affinity = nil + + klog.V(4).Infof("Status collector pinned to node: %s (index %d)", targetNode, targetNodeIndex) + return nil + }, + ) + go statusCronJobController.Run(ctx, 1) +} + +// nextStatusCollectorNodeIndex determines the next node index for status collector based on +// node list changes and last job status. Returns the new node index and updated target nodes list. +// +// Behavior: +// - Node list changed → reset to index 0, update target nodes +// - Last job failed → rotate to next index (wrap around), preserve target nodes +// - Last job succeeded → keep current index (sticky), preserve target nodes +func nextStatusCollectorNodeIndex( + currentIndex int, + currentTargetNodes []string, + newNodeNames []string, + lastJobFailed bool, +) (newIndex int, newTargetNodes []string) { + // Check if node list changed + if !tools.StringSlicesEqual(currentTargetNodes, newNodeNames) { + return 0, newNodeNames + } + + // Node list unchanged - check if we should rotate + if lastJobFailed && len(newNodeNames) > 0 { + return (currentIndex + 1) % len(newNodeNames), currentTargetNodes + } + + // Sticky behavior - stay on same node + return currentIndex, currentTargetNodes +} + +// checkLastJobFailedWithJob checks if the most recent job created by the status collector CronJob failed. +// Returns (failed bool, job *Job, error). +// Returns (false, nil, nil) if no jobs exist, job succeeded, or job is still running. +// Returns (true, job, nil) if the most recent completed job failed. +func checkLastJobFailedWithJob(ctx context.Context, kubeClient kubernetes.Interface) (bool, *batchv1.Job, error) { + jobsList, err := kubeClient.BatchV1().Jobs(operatorclient.TargetNamespace).List(ctx, metav1.ListOptions{ + LabelSelector: "app.kubernetes.io/name=" + pacemakerStatusCollectorName, + }) + if err != nil { + return false, nil, err + } + + if len(jobsList.Items) == 0 { + return false, nil, nil + } + + // Find most recent COMPLETED job (ignore active jobs) + // CronJob concurrencyPolicy: Forbid may leave jobs running past the next schedule, + // so we must skip active jobs and only look at jobs that finished. + var mostRecent *batchv1.Job + for i := range jobsList.Items { + job := &jobsList.Items[i] + + // Skip jobs that haven't completed - only examine finished jobs + // FailureTarget is a newer condition (K8s 1.31+) for jobs that exceeded activeDeadlineSeconds + hasCompletionCondition := false + for _, condition := range job.Status.Conditions { + if (condition.Type == batchv1.JobComplete || condition.Type == batchv1.JobFailed || condition.Type == batchv1.JobFailureTarget) && + condition.Status == corev1.ConditionTrue { + hasCompletionCondition = true + break + } + } + if !hasCompletionCondition { + continue // Job still running, skip it + } + + if mostRecent == nil || job.CreationTimestamp.After(mostRecent.CreationTimestamp.Time) { + mostRecent = job + } + } + + if mostRecent == nil { + return false, nil, nil // No completed jobs yet + } + + // Check if the completed job succeeded or failed + for _, condition := range mostRecent.Status.Conditions { + if condition.Type == batchv1.JobComplete && condition.Status == corev1.ConditionTrue { + return false, mostRecent, nil + } + if (condition.Type == batchv1.JobFailed || condition.Type == batchv1.JobFailureTarget) && + condition.Status == corev1.ConditionTrue { + return true, mostRecent, nil + } + } + + return false, nil, nil +} diff --git a/pkg/tnf/operator/status_collector_test.go b/pkg/tnf/operator/status_collector_test.go new file mode 100644 index 0000000000..6682960952 --- /dev/null +++ b/pkg/tnf/operator/status_collector_test.go @@ -0,0 +1,249 @@ +package operator + +/* +TEST COVERAGE SUMMARY - status_collector_test.go +================================================= + +This file tests status collector node rotation and failure handling logic. + +WHAT'S TESTED +------------- + +checkLastJobFailed(): +├── Job completed successfully → false +├── Job failed → true +├── Job failed with FailureTarget (deadline exceeded) → true +├── Job completed despite earlier pod failures (Complete takes precedence) → false +├── Job still running (no conditions) → false +└── No jobs exist → false + +Node rotation state machine: +├── Success → stays on same node (sticky behavior) +├── Failure → rotates to next node +├── Failure on last node → wraps around to first node +└── Node list changed → resets to first node +*/ + +import ( + "context" + "fmt" + "testing" + "time" + + "github.com/stretchr/testify/require" + batchv1 "k8s.io/api/batch/v1" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes/fake" + + "github.com/openshift/cluster-etcd-operator/pkg/operator/operatorclient" +) + +type jobSetup struct { + timestamp time.Time + conditions []batchv1.JobCondition +} + +func TestCheckLastJobFailed(t *testing.T) { + tests := []struct { + name string + jobConditions []batchv1.JobCondition + jobFailedCount int32 + wantFailed bool + // For multi-job tests + multipleJobs []jobSetup + }{ + { + name: "job completed successfully", + jobConditions: []batchv1.JobCondition{ + {Type: batchv1.JobComplete, Status: corev1.ConditionTrue}, + }, + wantFailed: false, + }, + { + name: "job failed", + jobConditions: []batchv1.JobCondition{ + {Type: batchv1.JobFailed, Status: corev1.ConditionTrue}, + }, + wantFailed: true, + }, + { + name: "job failed with FailureTarget - deadline exceeded", + jobConditions: []batchv1.JobCondition{ + {Type: batchv1.JobFailureTarget, Status: corev1.ConditionTrue}, + }, + wantFailed: true, + }, + { + name: "job completed successfully despite earlier pod failures - Complete takes precedence", + jobConditions: []batchv1.JobCondition{ + {Type: batchv1.JobComplete, Status: corev1.ConditionTrue}, + {Type: batchv1.JobFailed, Status: corev1.ConditionTrue}, + }, + jobFailedCount: 2, // Had pod failures during retries + wantFailed: false, + }, + { + name: "job still running - no conditions set", + jobConditions: []batchv1.JobCondition{}, + wantFailed: false, + }, + { + name: "no jobs exist", + jobConditions: nil, // Will result in empty list + wantFailed: false, + }, + { + name: "multiple jobs - newest failed overrides older success", + multipleJobs: []jobSetup{ + { + timestamp: time.Now().Add(-2 * time.Minute), + conditions: []batchv1.JobCondition{ + {Type: batchv1.JobComplete, Status: corev1.ConditionTrue}, + }, + }, + { + timestamp: time.Now().Add(-1 * time.Minute), + conditions: []batchv1.JobCondition{ + {Type: batchv1.JobFailed, Status: corev1.ConditionTrue}, + }, + }, + }, + wantFailed: true, + }, + { + name: "multiple jobs - newest success overrides older failure", + multipleJobs: []jobSetup{ + { + timestamp: time.Now().Add(-2 * time.Minute), + conditions: []batchv1.JobCondition{ + {Type: batchv1.JobFailed, Status: corev1.ConditionTrue}, + }, + }, + { + timestamp: time.Now().Add(-1 * time.Minute), + conditions: []batchv1.JobCondition{ + {Type: batchv1.JobComplete, Status: corev1.ConditionTrue}, + }, + }, + }, + wantFailed: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + kubeClient := fake.NewSimpleClientset() + + // Create single job or multiple jobs + if tt.multipleJobs != nil { + for i, js := range tt.multipleJobs { + job := &batchv1.Job{ + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf("test-job-%d", i+1), + Namespace: operatorclient.TargetNamespace, + Labels: map[string]string{ + "app.kubernetes.io/name": pacemakerStatusCollectorName, + }, + CreationTimestamp: metav1.NewTime(js.timestamp), + }, + Status: batchv1.JobStatus{ + Conditions: js.conditions, + }, + } + _, err := kubeClient.BatchV1().Jobs(operatorclient.TargetNamespace).Create(context.Background(), job, metav1.CreateOptions{}) + require.NoError(t, err) + } + } else if tt.jobConditions != nil { + job := &batchv1.Job{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-job-1", + Namespace: operatorclient.TargetNamespace, + Labels: map[string]string{ + "app.kubernetes.io/name": pacemakerStatusCollectorName, + }, + CreationTimestamp: metav1.NewTime(time.Now()), + }, + Status: batchv1.JobStatus{ + Conditions: tt.jobConditions, + Failed: tt.jobFailedCount, + }, + } + _, err := kubeClient.BatchV1().Jobs(operatorclient.TargetNamespace).Create(context.Background(), job, metav1.CreateOptions{}) + require.NoError(t, err) + } + + failed, job, err := checkLastJobFailedWithJob(context.Background(), kubeClient) + require.NoError(t, err) + require.Equal(t, tt.wantFailed, failed) + if tt.wantFailed { + require.NotNil(t, job, "expected job to be returned when failed=true") + } + }) + } +} + +func TestNextStatusCollectorNodeIndex(t *testing.T) { + tests := []struct { + name string + currentIndex int + currentTargetNodes []string + newNodeNames []string + lastJobFailed bool + expectedIndex int + expectedTargetNodes []string + }{ + { + name: "success - stays on same node (sticky behavior)", + currentIndex: 1, + currentTargetNodes: []string{"master-0", "master-1"}, + newNodeNames: []string{"master-0", "master-1"}, + lastJobFailed: false, + expectedIndex: 1, + expectedTargetNodes: []string{"master-0", "master-1"}, + }, + { + name: "failure - rotates to next node", + currentIndex: 0, + currentTargetNodes: []string{"master-0", "master-1"}, + newNodeNames: []string{"master-0", "master-1"}, + lastJobFailed: true, + expectedIndex: 1, + expectedTargetNodes: []string{"master-0", "master-1"}, + }, + { + name: "failure on last node - wraps around to first", + currentIndex: 1, + currentTargetNodes: []string{"master-0", "master-1"}, + newNodeNames: []string{"master-0", "master-1"}, + lastJobFailed: true, + expectedIndex: 0, + expectedTargetNodes: []string{"master-0", "master-1"}, + }, + { + name: "node list changed - resets to first node", + currentIndex: 1, + currentTargetNodes: []string{"master-0", "master-1"}, + newNodeNames: []string{"master-0", "master-2"}, // master-1 replaced with master-2 + lastJobFailed: false, + expectedIndex: 0, + expectedTargetNodes: []string{"master-0", "master-2"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Test the pure helper function + actualIndex, actualTargetNodes := nextStatusCollectorNodeIndex( + tt.currentIndex, + tt.currentTargetNodes, + tt.newNodeNames, + tt.lastJobFailed, + ) + + // Verify results + require.Equal(t, tt.expectedIndex, actualIndex, "Node index mismatch") + require.Equal(t, tt.expectedTargetNodes, actualTargetNodes, "Target nodes mismatch") + }) + } +} diff --git a/pkg/tnf/pkg/jobs/batch.go b/pkg/tnf/pkg/jobs/batch.go index db7aa6071d..0e916a939d 100644 --- a/pkg/tnf/pkg/jobs/batch.go +++ b/pkg/tnf/pkg/jobs/batch.go @@ -16,10 +16,18 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/runtime/serializer" batchclientv1 "k8s.io/client-go/kubernetes/typed/batch/v1" + "k8s.io/klog/v2" ) // TODO move to github.com/openshift/library-go/pkg/operator/resource/resource[read,apply,merge] +const ( + // Label keys for TNF job metadata + LabelJobType = "tnf.etcd.openshift.io/job-type" + LabelNodeIndex = "tnf.etcd.openshift.io/node-index" + LabelAttempt = "tnf.etcd.openshift.io/attempt" +) + var ( batchScheme = runtime.NewScheme() batchCodecs = serializer.NewCodecFactory(batchScheme) @@ -48,8 +56,11 @@ func ReadCronJobV1OrDie(objBytes []byte) *batchv1.CronJob { } // ApplyJob ensures the form of the specified job is present in the API. If it -// does not exist, it will be created. If it does exist, the existing job will be stopped, -// and a new Job will be created. +// does not exist, it will be created. If it does exist and has drifted from the required +// spec, the existing job will be deleted and recreated. For all non-failed cluster jobs +// (labeled with job-type=cluster), drift in retry-related fields (NodeName, node-index, +// attempt labels) is ignored to prevent recreation on operator restart when retry state +// is reinitialized. func ApplyJob(ctx context.Context, client batchclientv1.JobsGetter, recorder events.Recorder, requiredOriginal *batchv1.Job, expectedGeneration int64) (*batchv1.Job, bool, error) { @@ -69,8 +80,8 @@ func ApplyJob(ctx context.Context, client batchclientv1.JobsGetter, recorder eve return nil, false, err } - modified := false existingCopy := existing.DeepCopy() + modified := false resourcemerge.EnsureObjectMeta(&modified, &existingCopy.ObjectMeta, required.ObjectMeta) // there was no change to metadata, and the generation was right @@ -78,6 +89,20 @@ func ApplyJob(ctx context.Context, client batchclientv1.JobsGetter, recorder eve return existingCopy, false, nil } + // Drift detected - for non-failed cluster jobs, check if drift is only in retry-related fields + isClusterJob := required.Labels != nil && required.Labels[LabelJobType] == "cluster" + if isClusterJob && !IsFailed(*existing) { + // Check if drift is only in retry-related fields (NodeName, node-index, attempt labels) + // If so, this is expected (operator restart, retry state reinitialized) and safe to ignore + // This applies to both completed and running jobs - we only want to recreate failed jobs for round-robin retry + if isDriftOnlyInRetryFields(existing, required) { + klog.V(4).Infof("Job %s/%s is not failed with drift only in retry fields - ignoring", existing.Namespace, existing.Name) + return existing, false, nil + } + // Real drift detected in non-failed job (image, command, config changed) + klog.Warningf("Job %s/%s is not failed but has real config drift - recreating", existing.Namespace, existing.Name) + } + // We do not update jobs, we always recreate them, since significant parts are immutable. // Delete here, recreate on next sync. err = client.Jobs(required.Namespace).Delete(ctx, required.Name, metav1.DeleteOptions{}) @@ -95,3 +120,37 @@ func ExpectedJobGeneration(required *batchv1.Job, previousGenerations []operator } return -1 } + +// isDriftOnlyInRetryFields checks if the drift between existing and required jobs +// is only in retry-related fields (NodeName, node-index label, attempt label). +// Returns true if drift is only in these fields, false if there's drift in other fields. +func isDriftOnlyInRetryFields(existing, required *batchv1.Job) bool { + // Create copies with retry fields stripped + existingStripped := existing.DeepCopy() + requiredStripped := required.DeepCopy() + + // Strip NodeName from spec + existingStripped.Spec.Template.Spec.NodeName = "" + requiredStripped.Spec.Template.Spec.NodeName = "" + + // Strip retry labels + delete(existingStripped.Labels, LabelNodeIndex) + delete(existingStripped.Labels, LabelAttempt) + delete(requiredStripped.Labels, LabelNodeIndex) + delete(requiredStripped.Labels, LabelAttempt) + + // Recompute spec hashes without retry fields + existingErr := resourceapply.SetSpecHashAnnotation(&existingStripped.ObjectMeta, existingStripped.Spec) + requiredErr := resourceapply.SetSpecHashAnnotation(&requiredStripped.ObjectMeta, requiredStripped.Spec) + if existingErr != nil || requiredErr != nil { + // If we can't compute hashes, assume there's real drift to be safe + return false + } + + // Compare metadata after stripping + modified := false + resourcemerge.EnsureObjectMeta(&modified, &existingStripped.ObjectMeta, requiredStripped.ObjectMeta) + + // If no drift after stripping, then original drift was only in retry fields + return !modified +} diff --git a/pkg/tnf/pkg/jobs/batch_test.go b/pkg/tnf/pkg/jobs/batch_test.go index 1adb586cbb..b7e6ac9513 100644 --- a/pkg/tnf/pkg/jobs/batch_test.go +++ b/pkg/tnf/pkg/jobs/batch_test.go @@ -1,11 +1,18 @@ package jobs import ( + "context" "testing" + "github.com/openshift/cluster-etcd-operator/pkg/operator/operatorclient" + "github.com/openshift/library-go/pkg/operator/events" + "github.com/openshift/library-go/pkg/operator/resource/resourceapply" "github.com/stretchr/testify/require" batchv1 "k8s.io/api/batch/v1" corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes/fake" + "k8s.io/utils/clock" ) func TestReadCronJobV1OrDie(t *testing.T) { @@ -333,3 +340,262 @@ func TestInferImagePullPolicy(t *testing.T) { }) } } + +func TestApplyJob_ClusterJobIgnoresRetryState(t *testing.T) { + // Test that cluster jobs (with job-type=cluster label) are not recreated + // when only NodeName and retry-related labels differ (operator restart scenario). + // BUT they ARE recreated if there's real drift (image, command, etc.) + + tests := []struct { + name string + existingNodeName string + existingNodeIndex string + existingAttempt string + existingImage string + requiredNodeName string + requiredNodeIndex string + requiredAttempt string + requiredImage string + jobStatus string // "complete", "failed", or "running" + expectRecreation bool + }{ + { + name: "completed cluster job with different NodeName and retry labels - should NOT recreate", + existingNodeName: "master-2", + existingNodeIndex: "2", + existingAttempt: "1", + existingImage: "busybox:1.36", + requiredNodeName: "master-0", + requiredNodeIndex: "0", + requiredAttempt: "1", + requiredImage: "busybox:1.36", + jobStatus: "complete", + expectRecreation: false, + }, + { + name: "completed cluster job with different image - SHOULD recreate", + existingNodeName: "master-0", + existingNodeIndex: "0", + existingAttempt: "1", + existingImage: "busybox:1.36", + requiredNodeName: "master-0", + requiredNodeIndex: "0", + requiredAttempt: "1", + requiredImage: "busybox:1.37", + jobStatus: "complete", + expectRecreation: true, + }, + { + name: "running cluster job with different NodeName and retry labels - should NOT recreate", + existingNodeName: "master-2", + existingNodeIndex: "2", + existingAttempt: "1", + existingImage: "busybox:1.36", + requiredNodeName: "master-0", + requiredNodeIndex: "0", + requiredAttempt: "1", + requiredImage: "busybox:1.36", + jobStatus: "running", + expectRecreation: false, + }, + { + name: "failed cluster job with different NodeName - SHOULD recreate (round-robin retry)", + existingNodeName: "master-0", + existingNodeIndex: "0", + existingAttempt: "1", + existingImage: "busybox:1.36", + requiredNodeName: "master-1", + requiredNodeIndex: "1", + requiredAttempt: "1", + requiredImage: "busybox:1.36", + jobStatus: "failed", + expectRecreation: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := context.Background() + fakeKubeClient := fake.NewSimpleClientset() + + // Create existing cluster job with specific NodeName and retry labels + existingJob := &batchv1.Job{ + ObjectMeta: metav1.ObjectMeta{ + Name: "tnf-setup-job", + Namespace: "openshift-etcd", + Labels: map[string]string{ + LabelJobType: "cluster", + LabelNodeIndex: tt.existingNodeIndex, + LabelAttempt: tt.existingAttempt, + }, + Generation: 1, + }, + Spec: batchv1.JobSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + NodeName: tt.existingNodeName, + Containers: []corev1.Container{ + {Name: "test", Image: tt.existingImage}, + }, + RestartPolicy: corev1.RestartPolicyNever, + }, + }, + }, + } + + // Set job status based on test case + switch tt.jobStatus { + case "complete": + existingJob.Status.Conditions = []batchv1.JobCondition{ + {Type: batchv1.JobComplete, Status: corev1.ConditionTrue}, + } + case "failed": + existingJob.Status.Conditions = []batchv1.JobCondition{ + {Type: batchv1.JobFailed, Status: corev1.ConditionTrue}, + } + case "running": + // No conditions - job is running + existingJob.Status.Conditions = []batchv1.JobCondition{} + } + + // Apply spec hash to existing job + err := resourceapply.SetSpecHashAnnotation(&existingJob.ObjectMeta, existingJob.Spec) + require.NoError(t, err) + + _, err = fakeKubeClient.BatchV1().Jobs(existingJob.Namespace).Create(ctx, existingJob, metav1.CreateOptions{}) + require.NoError(t, err) + + // Create required job with potentially different NodeName, retry labels, and image + requiredJob := &batchv1.Job{ + ObjectMeta: metav1.ObjectMeta{ + Name: "tnf-setup-job", + Namespace: "openshift-etcd", + Labels: map[string]string{ + LabelJobType: "cluster", + LabelNodeIndex: tt.requiredNodeIndex, + LabelAttempt: tt.requiredAttempt, + }, + }, + Spec: batchv1.JobSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + NodeName: tt.requiredNodeName, + Containers: []corev1.Container{ + {Name: "test", Image: tt.requiredImage}, + }, + RestartPolicy: corev1.RestartPolicyNever, + }, + }, + }, + } + + // Call ApplyJob + recorder := events.NewRecorder( + fakeKubeClient.CoreV1().Events(operatorclient.TargetNamespace), + "test-apply-job", + &corev1.ObjectReference{}, + clock.RealClock{}, + ) + resultJob, created, err := ApplyJob(ctx, fakeKubeClient.BatchV1(), recorder, requiredJob, 1) + + if tt.expectRecreation { + // Should return error indicating job was deleted for recreation + require.Error(t, err) + require.Contains(t, err.Error(), "job spec was modified, old job is deleted") + require.Nil(t, resultJob) + require.False(t, created) + } else { + // Should return existing job unchanged + require.NoError(t, err) + require.NotNil(t, resultJob) + require.False(t, created) + require.Equal(t, existingJob.Name, resultJob.Name) + // Verify job was not deleted + job, getErr := fakeKubeClient.BatchV1().Jobs(existingJob.Namespace).Get(ctx, existingJob.Name, metav1.GetOptions{}) + require.NoError(t, getErr) + require.NotNil(t, job) + } + }) + } +} + +func TestApplyJob_NodeUIDDrift(t *testing.T) { + // Test that node-specific jobs are recreated when the node UID changes + // (node replaced with same name but different UID). + // The "node" label contains the UID, so changing it triggers real drift detection. + + ctx := context.Background() + fakeKubeClient := fake.NewSimpleClientset() + + // Create existing node job with UID from old node + existingJob := &batchv1.Job{ + ObjectMeta: metav1.ObjectMeta{ + Name: "tnf-auth-job-master-0", + Namespace: "openshift-etcd", + Labels: map[string]string{ + "node": "old-uid-12345", // Old node UID + }, + Generation: 1, + }, + Spec: batchv1.JobSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + NodeName: "master-0", + Containers: []corev1.Container{ + {Name: "test", Image: "busybox:1.36"}, + }, + RestartPolicy: corev1.RestartPolicyNever, + }, + }, + }, + Status: batchv1.JobStatus{ + Conditions: []batchv1.JobCondition{ + {Type: batchv1.JobComplete, Status: corev1.ConditionTrue}, + }, + }, + } + + // Apply spec hash to existing job + err := resourceapply.SetSpecHashAnnotation(&existingJob.ObjectMeta, existingJob.Spec) + require.NoError(t, err) + + _, err = fakeKubeClient.BatchV1().Jobs(existingJob.Namespace).Create(ctx, existingJob, metav1.CreateOptions{}) + require.NoError(t, err) + + // Create required job with UID from new node (node was replaced) + requiredJob := &batchv1.Job{ + ObjectMeta: metav1.ObjectMeta{ + Name: "tnf-auth-job-master-0", + Namespace: "openshift-etcd", + Labels: map[string]string{ + "node": "new-uid-67890", // New node UID - this is real drift\! + }, + }, + Spec: batchv1.JobSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + NodeName: "master-0", // Same node name + Containers: []corev1.Container{ + {Name: "test", Image: "busybox:1.36"}, // Same image + }, + RestartPolicy: corev1.RestartPolicyNever, + }, + }, + }, + } + + // Call ApplyJob + recorder := events.NewRecorder( + fakeKubeClient.CoreV1().Events(operatorclient.TargetNamespace), + "test-apply-job", + &corev1.ObjectReference{}, + clock.RealClock{}, + ) + resultJob, created, err := ApplyJob(ctx, fakeKubeClient.BatchV1(), recorder, requiredJob, 1) + + // Should detect drift and delete job for recreation + require.Error(t, err) + require.Contains(t, err.Error(), "job spec was modified, old job is deleted") + require.Nil(t, resultJob) + require.False(t, created) +} diff --git a/pkg/tnf/pkg/jobs/jobcontroller.go b/pkg/tnf/pkg/jobs/jobcontroller.go index 3d11156e01..cae9d7a92e 100644 --- a/pkg/tnf/pkg/jobs/jobcontroller.go +++ b/pkg/tnf/pkg/jobs/jobcontroller.go @@ -9,11 +9,13 @@ import ( opv1 "github.com/openshift/api/operator/v1" applyoperatorv1 "github.com/openshift/client-go/operator/applyconfigurations/operator/v1" + "github.com/openshift/cluster-etcd-operator/pkg/tnf/pkg/tools" "github.com/openshift/library-go/pkg/controller/factory" "github.com/openshift/library-go/pkg/operator/events" "github.com/openshift/library-go/pkg/operator/management" "github.com/openshift/library-go/pkg/operator/v1helpers" batchv1 "k8s.io/api/batch/v1" + corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/errors" @@ -30,11 +32,21 @@ import ( var DefaultConditions = []string{opv1.OperatorStatusTypeProgressing, opv1.OperatorStatusTypeDegraded} var AllConditions = []string{opv1.OperatorStatusTypeAvailable, opv1.OperatorStatusTypeProgressing, opv1.OperatorStatusTypeDegraded} +const ( + // stuckJobRecoveryTimeout is how long a job must be in Failed state before auto-deletion. + // Used to recover from controller parameter migrations or stuck jobs. + stuckJobRecoveryTimeout = 10 * time.Minute +) + // TODO This based on DeploymentController in openshift/library-go // TODO should be moved there once it proved to be useful // JobHookFunc is a hook function to modify the Job. -type JobHookFunc func(*opv1.OperatorSpec, *batchv1.Job) error +// Returns (shouldApply bool, error): +// - (true, nil): Apply the job (proceed with ApplyJob) +// - (false, nil): Skip applying the job (nodes not ready, conditions not met) +// - (false, error): Error occurred, set Degraded condition +type JobHookFunc func(*opv1.OperatorSpec, *batchv1.Job) (bool, error) // JobController is a generic controller that manages a job. // @@ -56,9 +68,10 @@ type JobController struct { recorder events.Recorder conditions []string // Optional hook functions to modify the Job. - // If one of these functions returns an error, the sync - // fails indicating the ordinal position of the failed function. - // Also, in that scenario the Degraded status is set to True. + // Each hook returns (shouldApply bool, error): + // - If error is non-nil, sync fails and Degraded status is set to True + // - If shouldApply is false, job creation is skipped (nodes not ready, etc.) + // - If shouldApply is true, job is applied via ApplyJob optionalJobHooks []JobHookFunc // errors contains any errors that occur during the configuration // and setup of the JobController. @@ -177,7 +190,7 @@ func (c *JobController) ToController() (factory.Controller, error) { controller = controller.WithSyncDegradedOnError(c.operatorClient) } return controller.ToController( - c.instanceName, // don't change what is passed here unless you also remove the old FooDegraded condition + tools.ToPascalCase(c.instanceName), // Use PascalCase for condition names (e.g., TNFSetupJob, TNFAuthJobMaster0637363be) c.recorder.WithComponentSuffix(strings.ToLower(c.instanceName)+"-job-controller-"), ), errors.NewAggregate(c.errors) } @@ -222,6 +235,12 @@ func (c *JobController) syncManaged(ctx context.Context, opSpec *opv1.OperatorSp return err } + // If hook returned nil job, skip applying (nodes not ready, etc.) + if required == nil { + klog.V(4).Infof("Skipping job application: hook requested skip") + return nil + } + job, _, err := ApplyJob( ctx, c.kubeClient.BatchV1(), @@ -233,6 +252,35 @@ func (c *JobController) syncManaged(ctx context.Context, opSpec *opv1.OperatorSp return err } + // Auto-recovery: delete jobs stuck in Failed state for > 10 minutes + // This handles controller parameter migrations and stuck jobs + if IsFailed(*job) { + // Check when the job failed + var failedTime *metav1.Time + for _, condition := range job.Status.Conditions { + if condition.Type == batchv1.JobFailed && condition.Status == corev1.ConditionTrue { + failedTime = &condition.LastTransitionTime + break + } + } + + if failedTime != nil && time.Since(failedTime.Time) > stuckJobRecoveryTimeout { + klog.Warningf("Job %s has been failed for %v, deleting to force restart (controller parameter migration or stuck job recovery)", + job.Name, time.Since(failedTime.Time).Round(time.Second)) + + err := c.kubeClient.BatchV1().Jobs(job.Namespace).Delete(ctx, job.Name, metav1.DeleteOptions{ + PropagationPolicy: ptr.To(metav1.DeletePropagationBackground), + }) + if err != nil && !apierrors.IsNotFound(err) { + klog.Warningf("Failed to delete stuck job %s: %v", job.Name, err) + } else { + klog.Infof("Deleted stuck failed job %s, will recreate on next sync", job.Name) + } + // Return early - next sync will recreate the job + return nil + } + } + // Create an OperatorStatusApplyConfiguration with generations status := applyoperatorv1.OperatorStatus(). WithGenerations(&applyoperatorv1.GenerationStatusApplyConfiguration{ @@ -246,7 +294,7 @@ func (c *JobController) syncManaged(ctx context.Context, opSpec *opv1.OperatorSp // Set Available condition if slices.Contains(c.conditions, opv1.OperatorStatusTypeAvailable) { availableCondition := applyoperatorv1. - OperatorCondition().WithType(c.instanceName + opv1.OperatorStatusTypeAvailable) + OperatorCondition().WithType(tools.ToPascalCase(c.instanceName) + opv1.OperatorStatusTypeAvailable) if IsComplete(*job) { availableCondition = availableCondition. @@ -271,7 +319,7 @@ func (c *JobController) syncManaged(ctx context.Context, opSpec *opv1.OperatorSp // Set Progressing condition if slices.Contains(c.conditions, opv1.OperatorStatusTypeProgressing) { progressingCondition := applyoperatorv1.OperatorCondition(). - WithType(c.instanceName + opv1.OperatorStatusTypeProgressing) + WithType(tools.ToPascalCase(c.instanceName) + opv1.OperatorStatusTypeProgressing) if IsComplete(*job) { progressingCondition = progressingCondition. @@ -330,10 +378,14 @@ func (c *JobController) getJob(opSpec *opv1.OperatorSpec) (*batchv1.Job, error) manifest := c.manifest required := ReadJobV1OrDie(manifest) for i := range c.optionalJobHooks { - err := c.optionalJobHooks[i](opSpec, required) + shouldApply, err := c.optionalJobHooks[i](opSpec, required) if err != nil { return nil, fmt.Errorf("error running hook function (index=%d): %w", i, err) } + if !shouldApply { + // Hook requested to skip job application (nodes not ready, etc.) + return nil, nil + } } return required, nil } diff --git a/pkg/tnf/pkg/jobs/jobcontroller_test.go b/pkg/tnf/pkg/jobs/jobcontroller_test.go index f5cca5a067..688ca2134c 100644 --- a/pkg/tnf/pkg/jobs/jobcontroller_test.go +++ b/pkg/tnf/pkg/jobs/jobcontroller_test.go @@ -14,6 +14,7 @@ import ( fakeconfig "github.com/openshift/client-go/config/clientset/versioned/fake" configinformers "github.com/openshift/client-go/config/informers/externalversions" applyoperatorv1 "github.com/openshift/client-go/operator/applyconfigurations/operator/v1" + "github.com/openshift/cluster-etcd-operator/pkg/tnf/pkg/tools" "github.com/openshift/library-go/pkg/controller/factory" "github.com/openshift/library-go/pkg/operator/events" "github.com/openshift/library-go/pkg/operator/management" @@ -43,9 +44,9 @@ const ( ) var ( - conditionAvailable = controllerName + opv1.OperatorStatusTypeAvailable - conditionProgressing = controllerName + opv1.OperatorStatusTypeProgressing - conditionDegraded = controllerName + opv1.OperatorStatusTypeDegraded + conditionAvailable = tools.ToPascalCase(controllerName) + opv1.OperatorStatusTypeAvailable + conditionProgressing = tools.ToPascalCase(controllerName) + opv1.OperatorStatusTypeProgressing + conditionDegraded = tools.ToPascalCase(controllerName) + opv1.OperatorStatusTypeDegraded allConditions = []string{conditionAvailable, conditionProgressing, conditionDegraded} availableCondition = []string{conditionAvailable} ) @@ -167,6 +168,30 @@ func TestSync(t *testing.T) { }, expectErr: true, // Job failure should return an error }, + { + // Job stuck in failed state for >10 minutes - auto-delete and succeed (will recreate next sync) + name: "job failed stuck auto-deleted", + initialObjects: testObjects{ + job: makeJob( + withJobGeneration(1), + withJobStatus(0, 0, 1), + withJobFailedStuck()), + operator: makeFakeOperatorInstance( + withGenerations(1), + withTrueConditions(conditionAvailable), + withFalseConditions(conditionProgressing, conditionDegraded), + ), + }, + expectedObjects: testObjects{ + job: nil, // Job should be deleted + operator: makeFakeOperatorInstance( + withGenerations(1), + withTrueConditions(conditionAvailable), + withFalseConditions(conditionProgressing, conditionDegraded), + ), + }, + expectErr: false, // Auto-deletion succeeds (will recreate next sync) + }, } for _, test := range testCases { @@ -720,9 +745,26 @@ func withJobComplete() jobModifier { func withJobFailed() jobModifier { return func(instance *batchv1.Job) *batchv1.Job { + // Set LastTransitionTime to recent time (1 minute ago) to avoid auto-deletion + // Timestamp will be cleared by sanitizeJob() for deterministic comparison instance.Status.Conditions = append(instance.Status.Conditions, batchv1.JobCondition{ - Type: batchv1.JobFailed, - Status: v1.ConditionTrue, + Type: batchv1.JobFailed, + Status: v1.ConditionTrue, + LastTransitionTime: metav1.NewTime(time.Now().Add(-1 * time.Minute)), + }) + return instance + } +} + +func withJobFailedStuck() jobModifier { + return func(instance *batchv1.Job) *batchv1.Job { + // Set LastTransitionTime to fixed old time (>10 minutes ago) to trigger auto-deletion + // Using time way in the past to ensure auto-deletion threshold is met + stuckTime := time.Date(2020, time.January, 1, 12, 0, 0, 0, time.UTC) + instance.Status.Conditions = append(instance.Status.Conditions, batchv1.JobCondition{ + Type: batchv1.JobFailed, + Status: v1.ConditionTrue, + LastTransitionTime: metav1.NewTime(stuckTime), }) return instance } @@ -755,6 +797,11 @@ func sanitizeJob(job *batchv1.Job) { } // Remove random annotations set by ApplyJob delete(job.Annotations, specHashAnnotation) + // Clear condition timestamps for deterministic comparison + for i := range job.Status.Conditions { + job.Status.Conditions[i].LastTransitionTime = metav1.Time{} + job.Status.Conditions[i].LastProbeTime = metav1.Time{} + } } func sanitizeInstanceStatus(status *opv1.OperatorStatus, testedConditions []string) { diff --git a/pkg/tnf/pkg/jobs/lifecycle.go b/pkg/tnf/pkg/jobs/lifecycle.go new file mode 100644 index 0000000000..526b9b3082 --- /dev/null +++ b/pkg/tnf/pkg/jobs/lifecycle.go @@ -0,0 +1,689 @@ +package jobs + +import ( + "context" + "fmt" + "os" + "sync" + "time" + + operatorv1 "github.com/openshift/api/operator/v1" + "github.com/openshift/library-go/pkg/controller/controllercmd" + "github.com/openshift/library-go/pkg/controller/factory" + "github.com/openshift/library-go/pkg/operator/v1helpers" + batchv1 "k8s.io/api/batch/v1" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes" + corev1listers "k8s.io/client-go/listers/core/v1" + "k8s.io/client-go/tools/cache" + "k8s.io/klog/v2" + "k8s.io/utils/ptr" + + "github.com/openshift/cluster-etcd-operator/bindata" + "github.com/openshift/cluster-etcd-operator/pkg/operator/operatorclient" + "github.com/openshift/cluster-etcd-operator/pkg/tnf/pkg/tools" +) + +// SchedulableNodesFunc returns ready nodes where job can run (round-robin target). +type SchedulableNodesFunc func() ([]*corev1.Node, error) + +// AffectedNodesFunc returns nodes that must be Ready before job admission (ready check target). +type AffectedNodesFunc func() ([]*corev1.Node, error) + +// JobConfigFunc returns deterministic JSON for drift detection. Changes trigger job restart. +type JobConfigFunc func() (string, error) + +var ( + // runningControllers tracks which controllers are already running to prevent duplicates + runningControllers = make(map[string]bool) + // runningControllersMutex protects the runningControllers map + runningControllersMutex sync.Mutex + + // restartJobLocks tracks in-flight RestartJobOrRunController calls to prevent parallel execution + restartJobLocks = make(map[string]*sync.Mutex) + // restartJobLocksMutex protects the restartJobLocks map + restartJobLocksMutex sync.Mutex + + // retryState tracks multi-node retry state for jobs using TargetNodesFunc + // Map key is job name, value tracks current attempt and node index + retryState = make(map[string]*JobRetryState) + // retryStateMutex protects access to retryState map + retryStateMutex sync.Mutex + + // jobBlockedSince tracks when jobs first became blocked due to affected nodes not being ready. + // Map key is job name, value is timestamp when first blocked. + // Used to return error after 10 minutes, triggering Degraded condition via WithSyncDegradedOnError. + jobBlockedSince = make(map[string]time.Time) + // jobBlockedMutex protects access to jobBlockedSince map + jobBlockedMutex sync.Mutex + + // jobNoSchedulableNodesSince tracks when jobs first had no schedulable nodes. + // Map key is job name, value is timestamp when first detected. + // Used to return error after 10 minutes, triggering Degraded condition via WithSyncDegradedOnError. + jobNoSchedulableNodesSince = make(map[string]time.Time) + // jobNoSchedulableNodesMutex protects access to jobNoSchedulableNodesSince map + jobNoSchedulableNodesMutex sync.Mutex +) + +// JobRetryState tracks retry progress for multi-node jobs +type JobRetryState struct { + Mu sync.Mutex // Protects fields below + AttemptNumber int // Current attempt (1-N) + NodeIndex int // Index of node to try in current attempt + TargetNodes []string // Cached node names from last targetNodesFunc call + MaxRetryAttempts int // Maximum attempts before degrading + LastFailTime time.Time // When last failure occurred + LastJobConfig string // Serialized config when job was created (for drift detection) +} + +const ( + // blockedConditionTimeout is how long to wait before returning error when jobs are blocked. + // Error triggers Degraded condition via WithSyncDegradedOnError. + blockedConditionTimeout = 10 * time.Minute + + // Degraded condition reasons + degradedReasonAsExpected = "AsExpected" + degradedReasonMaxRetriesExceeded = "MaxRetriesExceeded" +) + +// manageTimedBlockedCondition manages the Degraded condition for blocked jobs. +// Returns error after timeout to trigger WithSyncDegradedOnError, manually clears when unblocked. +// Returns (ready bool, error): +// - (true, nil): unblocked, ready to proceed +// - (false, nil): blocked but < timeout, wait without reporting +// - (false, error): blocked >= timeout, error triggers Degraded +func manageTimedBlockedCondition( + ctx context.Context, + jobName string, + isBlocked bool, + blockedSinceMap map[string]time.Time, + mapMutex *sync.Mutex, + errorMessage string, + operatorClient v1helpers.StaticPodOperatorClient, +) (bool, error) { + conditionName := tools.ToPascalCase(jobName) + "Degraded" + + if isBlocked { + // Track blocked time + mapMutex.Lock() + blockedSince, wasBlocked := blockedSinceMap[jobName] + if !wasBlocked { + // First time blocked - record timestamp + blockedSinceMap[jobName] = time.Now() + blockedSince = blockedSinceMap[jobName] + } + mapMutex.Unlock() + + // Return error if blocked long enough (WithSyncDegradedOnError will handle it) + if time.Since(blockedSince) > blockedConditionTimeout { + return false, fmt.Errorf("%s", errorMessage) + } + + return false, nil // Blocked but not long enough yet + } + + // Clear blocked tracking and Degraded condition + mapMutex.Lock() + _, wasBlocked := blockedSinceMap[jobName] + if wasBlocked { + delete(blockedSinceMap, jobName) + } + mapMutex.Unlock() + + // Manually clear Degraded condition when unblocked (empty message to avoid cluttering ClusterOperator rollup) + if wasBlocked { + _, _, updateErr := v1helpers.UpdateStatus(ctx, operatorClient, v1helpers.UpdateConditionFn(operatorv1.OperatorCondition{ + Type: conditionName, + Status: operatorv1.ConditionFalse, + Reason: degradedReasonAsExpected, + Message: "", + })) + if updateErr != nil { + klog.Errorf("Failed to clear %s condition: %v", conditionName, updateErr) + } + } + + return true, nil +} + +// manageBlockedCondition manages Degraded for jobs blocked by nodes not ready. +// Returns error after 10 min timeout, manually clears when ready. +func manageBlockedCondition(ctx context.Context, jobName string, notReadyNodes []string, operatorClient v1helpers.StaticPodOperatorClient) (bool, error) { + return manageTimedBlockedCondition( + ctx, + jobName, + len(notReadyNodes) > 0, + jobBlockedSince, + &jobBlockedMutex, + fmt.Sprintf("Affected nodes not ready: %v", notReadyNodes), + operatorClient, + ) +} + +// manageNoSchedulableNodesBlockedCondition manages Degraded when no schedulable nodes available. +// Returns error after 10 min timeout, manually clears when nodes available. +func manageNoSchedulableNodesBlockedCondition(ctx context.Context, jobName string, hasSchedulableNodes bool, operatorClient v1helpers.StaticPodOperatorClient) (bool, error) { + return manageTimedBlockedCondition( + ctx, + jobName, + !hasSchedulableNodes, + jobNoSchedulableNodesSince, + &jobNoSchedulableNodesMutex, + "No schedulable nodes available", + operatorClient, + ) +} + +// checkNodesReadinessAndSetCondition checks node readiness and manages Degraded condition. +// Returns error if nodes not ready > 10 min (triggers WithSyncDegradedOnError). +func checkNodesReadinessAndSetCondition(ctx context.Context, nodes []*corev1.Node, jobName string, operatorClient v1helpers.StaticPodOperatorClient) (bool, error) { + // Collect all not-ready nodes + var notReadyNodes []string + + for _, node := range nodes { + if !tools.IsNodeReady(node) { + notReadyNodes = append(notReadyNodes, node.Name) + } + } + + // Manage degraded condition, propagate error if blocked >= timeout + ready, err := manageBlockedCondition(ctx, jobName, notReadyNodes, operatorClient) + if err != nil { + return false, err // Propagate error to trigger WithSyncDegradedOnError + } + if !ready { + return false, nil // Blocked but not long enough yet + } + + return true, nil +} + +// syncMultiNodeJobState manages retry state for multi-node job (node changes, config drift, failures). +// Detects drift and failures, updates retry state accordingly. Job deletion/recreation is handled +// by ApplyJob via drift detection (NodeName changed), except for real config/node drift where +// jobs are explicitly deleted before resetting state. +func syncMultiNodeJobState(ctx context.Context, jobName string, schedulableNodesFunc SchedulableNodesFunc, affectedNodesFunc AffectedNodesFunc, jobConfigFunc JobConfigFunc, maxRetryAttempts int, kubeClient kubernetes.Interface, operatorClient v1helpers.StaticPodOperatorClient) error { + // Check affected nodes readiness before admitting/retrying job + if affectedNodesFunc != nil { + affectedNodes, err := affectedNodesFunc() + if err != nil { + return fmt.Errorf("failed to get affected nodes: %w", err) + } + + // Check readiness and manage blocked condition + ready, err := checkNodesReadinessAndSetCondition(ctx, affectedNodes, jobName, operatorClient) + if err != nil { + return err + } + if !ready { + // Block job admission - nodes not ready yet + return nil + } + } + + // Lock the global state map + // Check if state already exists before computing (fast path) + retryStateMutex.Lock() + state, exists := retryState[jobName] + retryStateMutex.Unlock() + + if !exists { + // Compute schedulable nodes and config outside lock to avoid blocking + schedulableNodes, err := schedulableNodesFunc() + if err != nil { + return fmt.Errorf("failed to get schedulable nodes: %w", err) + } + if len(schedulableNodes) == 0 { + _, err := manageNoSchedulableNodesBlockedCondition(ctx, jobName, false, operatorClient) + return err // Propagate error to trigger WithSyncDegradedOnError if blocked >= timeout + } + + // Get initial job config + var initialConfig string + if jobConfigFunc != nil { + initialConfig, err = jobConfigFunc() + if err != nil { + return fmt.Errorf("failed to get initial job config: %w", err) + } + } + + // Now acquire lock and re-check state + retryStateMutex.Lock() + state, exists = retryState[jobName] + if !exists { + // Another controller didn't create it while we were computing, safe to initialize + state = &JobRetryState{ + AttemptNumber: 1, + NodeIndex: 0, + TargetNodes: tools.GetNodeNames(schedulableNodes), + MaxRetryAttempts: maxRetryAttempts, + LastJobConfig: initialConfig, + } + retryState[jobName] = state + } + retryStateMutex.Unlock() + + // Clear blocked condition now that schedulable nodes are available + if _, err := manageNoSchedulableNodesBlockedCondition(ctx, jobName, true, operatorClient); err != nil { + klog.Errorf("Failed to clear blocked condition for %s: %v", jobName, err) + } + + klog.V(4).Infof("Starting job %s - attempt %d/%d, will try schedulable nodes: %v", + jobName, state.AttemptNumber, state.MaxRetryAttempts, state.TargetNodes) + return nil + } + + // Lock this job's state for the rest of the sync + state.Mu.Lock() + defer state.Mu.Unlock() + + // Check if schedulable nodes have changed + schedulableNodes, err := schedulableNodesFunc() + if err != nil { + return fmt.Errorf("failed to get schedulable nodes: %w", err) + } + if len(schedulableNodes) == 0 { + _, err = manageNoSchedulableNodesBlockedCondition(ctx, jobName, false, operatorClient) + return err // Propagate error to trigger WithSyncDegradedOnError if blocked >= timeout + } + + // Clear blocked condition if schedulable nodes are now available + if _, err := manageNoSchedulableNodesBlockedCondition(ctx, jobName, true, operatorClient); err != nil { + klog.Errorf("Failed to clear blocked condition for %s: %v", jobName, err) + } + + // Nodes are already sorted by schedulableNodesFunc (critical for round-robin NodeIndex) + currentSchedulableNodes := tools.GetNodeNames(schedulableNodes) + nodesChanged := !tools.StringSlicesEqual(state.TargetNodes, currentSchedulableNodes) + + // Check if job config has changed + var configChanged bool + var currentConfig string + if jobConfigFunc != nil { + currentConfig, err = jobConfigFunc() + if err != nil { + return fmt.Errorf("failed to get current job config: %w", err) + } + configChanged = state.LastJobConfig != currentConfig + } + + // If nodes or config changed, restart job + if nodesChanged || configChanged { + if nodesChanged { + klog.Infof("Job %s schedulable nodes changed from %v to %v - resetting retry state and deleting job", + jobName, state.TargetNodes, currentSchedulableNodes) + } + if configChanged { + klog.Infof("Job %s config changed - resetting retry state and deleting job. Old: %s, New: %s", + jobName, state.LastJobConfig, currentConfig) + } + + // Delete existing job if it exists (config drift or wrong target node) + _, err := kubeClient.BatchV1().Jobs(operatorclient.TargetNamespace).Get(ctx, jobName, v1.GetOptions{}) + if err == nil { + // Job exists - delete it so we can recreate with new config + if err := DeleteAndWait(ctx, kubeClient, jobName, operatorclient.TargetNamespace); err != nil { + return fmt.Errorf("failed to delete job %s after config/nodes changed: %w", jobName, err) + } + klog.Infof("Deleted job %s after config/nodes changed", jobName) + } else if !apierrors.IsNotFound(err) { + return fmt.Errorf("failed to check for existing job %s: %w", jobName, err) + } + + // Update state only after successful deletion + state.AttemptNumber = 1 + state.NodeIndex = 0 + state.TargetNodes = currentSchedulableNodes + state.LastJobConfig = currentConfig + + return nil + } + + // Get existing job (if any) + existingJob, err := kubeClient.BatchV1().Jobs(operatorclient.TargetNamespace).Get(ctx, jobName, v1.GetOptions{}) + if err != nil { + if apierrors.IsNotFound(err) { + // No job exists - nothing to sync (will be created by JobController) + return nil + } + return fmt.Errorf("failed to get job %s: %w", jobName, err) + } + + // Job exists - check if it's done + if IsComplete(*existingJob) { + // Success - clear degraded condition + klog.V(4).Infof("Job %s completed successfully", jobName) + + // Clear degraded condition if it was set + _, _, err := v1helpers.UpdateStatus(ctx, operatorClient, v1helpers.UpdateConditionFn(operatorv1.OperatorCondition{ + Type: tools.ToPascalCase(jobName) + operatorv1.OperatorStatusTypeDegraded, + Status: operatorv1.ConditionFalse, + Reason: "AsExpected", + Message: fmt.Sprintf("Job %s completed successfully", jobName), + })) + if err != nil { + klog.Errorf("Failed to clear degraded condition for %s: %v", jobName, err) + } + + return nil + } + + if IsFailed(*existingJob) || IsStopped(*existingJob) { + // Failed - calculate next retry position + currentNodeIndex := state.NodeIndex + currentAttemptNumber := state.AttemptNumber + klog.V(4).Infof("Job %s failed on node index %d (attempt %d) - moving to next node", + jobName, currentNodeIndex, currentAttemptNumber) + + // Calculate next position + nextNodeIndex := currentNodeIndex + 1 + nextAttemptNumber := currentAttemptNumber + + // Check if we've exhausted all nodes in this attempt + exhaustedNodes := nextNodeIndex >= len(schedulableNodes) + if exhaustedNodes { + nextNodeIndex = 0 + exhaustedAttempts := currentAttemptNumber >= state.MaxRetryAttempts + if exhaustedAttempts { + // Exceeded max attempts - set degraded condition and reset to attempt 1 + klog.Warningf("Job %s exhausted all %d attempts (tried %d nodes each), marking degraded", + jobName, state.MaxRetryAttempts, len(schedulableNodes)) + + // Set degraded condition to indicate job has failed after all retries + _, _, err := v1helpers.UpdateStatus(ctx, operatorClient, v1helpers.UpdateConditionFn(operatorv1.OperatorCondition{ + Type: tools.ToPascalCase(jobName) + operatorv1.OperatorStatusTypeDegraded, + Status: operatorv1.ConditionTrue, + Reason: degradedReasonMaxRetriesExceeded, + Message: fmt.Sprintf("Job failed after %d attempts across all nodes", state.MaxRetryAttempts), + })) + if err != nil { + klog.Errorf("Failed to set degraded condition for %s: %v", jobName, err) + } + + // Reset to attempt 1 and continue trying (degraded condition remains set until success) + nextAttemptNumber = 1 + } else { + // Start new attempt + nextAttemptNumber++ + klog.V(4).Infof("Job %s exhausted all nodes in attempt %d, starting attempt %d/%d", + jobName, currentAttemptNumber, nextAttemptNumber, state.MaxRetryAttempts) + } + } + + // Update retry state - ApplyJob will detect drift (NodeName, node-index, or attempt labels changed) and recreate job + klog.V(4).Infof("Job %s failed - updating retry state to node index %d", jobName, nextNodeIndex) + state.NodeIndex = nextNodeIndex + state.AttemptNumber = nextAttemptNumber + } + + // Job is running - nothing to do + return nil +} + +// configureMultiNodeJob configures job based on current retry state (pure function - reads state only). +func configureMultiNodeJob(job *batchv1.Job, maxRetryAttempts int) error { + jobName := job.Name + + // Get current state (must have been initialized by syncMultiNodeJobState) + retryStateMutex.Lock() + state, exists := retryState[jobName] + retryStateMutex.Unlock() + + if !exists { + // State should always exist when this function is called + // If it doesn't, it's a programming error + return fmt.Errorf("retry state for job %s does not exist (should have been created by syncMultiNodeJobState)", jobName) + } + + // Lock state for reading + state.Mu.Lock() + nodeIndex := state.NodeIndex + attemptNumber := state.AttemptNumber + targetNodes := state.TargetNodes + state.Mu.Unlock() + + // Validate node index against cached target nodes + if nodeIndex >= len(targetNodes) { + return fmt.Errorf("invalid node index %d (only %d nodes in cached state)", nodeIndex, len(targetNodes)) + } + + selectedNodeName := targetNodes[nodeIndex] + klog.V(4).Infof("Job %s attempt %d/%d: scheduling on node %s (index %d/%d)", + jobName, attemptNumber, maxRetryAttempts, selectedNodeName, + nodeIndex+1, len(targetNodes)) + + // Configure job to run on selected node + job.Spec.Template.Spec.NodeName = selectedNodeName + job.Labels[LabelAttempt] = fmt.Sprintf("%d", attemptNumber) + job.Labels[LabelNodeIndex] = fmt.Sprintf("%d", nodeIndex) + job.Labels[LabelJobType] = "cluster" + + return nil +} + +// resetJobRetryState clears the retry state for a job (called on success or when starting fresh) +func resetJobRetryState(jobName string) { + retryStateMutex.Lock() + defer retryStateMutex.Unlock() + delete(retryState, jobName) + klog.V(2).Infof("Reset retry state for job %s", jobName) +} + +// RunNodeJobController starts job controller for node-specific job (auth, after-setup). +func RunNodeJobController(ctx context.Context, jobType tools.JobType, node *corev1.Node, retries int, controllerContext *controllercmd.ControllerContext, operatorClient v1helpers.StaticPodOperatorClient, kubeClient kubernetes.Interface, kubeInformersForNamespaces v1helpers.KubeInformersForNamespaces, controlPlaneNodeInformer cache.SharedIndexInformer, conditions []string) { + jobNodeName := &node.Name + jobName := jobType.GetJobName(jobNodeName) + + // Check if controller already running + runningControllersMutex.Lock() + if runningControllers[jobName] { + runningControllersMutex.Unlock() + klog.V(4).Infof("Node job controller for %q on node %q is already running, skipping duplicate start", jobType.GetSubCommand(), node.Name) + return + } + runningControllers[jobName] = true + runningControllersMutex.Unlock() + + klog.Infof("starting node job controller for %q on node %q", jobType.GetSubCommand(), node.Name) + + // Create node lister for fetching fresh node data in hook + nodeLister := corev1listers.NewNodeLister(controlPlaneNodeInformer.GetIndexer()) + + tnfJobController := NewJobController( + jobName, + bindata.MustAsset("tnfdeployment/job.yaml"), + controllerContext.EventRecorder, + operatorClient, + kubeClient, + kubeInformersForNamespaces.InformersFor(operatorclient.TargetNamespace).Batch().V1().Jobs(), + conditions, + []factory.Informer{}, + []JobHookFunc{ + func(_ *operatorv1.OperatorSpec, job *batchv1.Job) (bool, error) { + job.SetName(jobName) + job.Labels["app.kubernetes.io/name"] = jobType.GetNameLabelValue() + + // Fetch fresh node from informer to handle node replacement (same name, different UID) + freshNode, err := nodeLister.Get(node.Name) + if err != nil { + if apierrors.IsNotFound(err) { + // Node was deleted - skip job application and let controller wait for context cancellation + klog.V(4).Infof("Node %s no longer exists, skipping job %s application (controller will stop when context is canceled)", node.Name, job.Name) + return false, nil + } + return false, fmt.Errorf("failed to get node %s from informer: %w", node.Name, err) + } + + // Check node readiness before configuring job + ready, err := checkNodesReadinessAndSetCondition(ctx, []*corev1.Node{freshNode}, job.Name, operatorClient) + if err != nil { + return false, err + } + if !ready { + klog.V(4).Infof("Skipping job %s creation: node %s not ready", job.Name, freshNode.Name) + return false, nil + } + + // Configure node job: schedule on specific node, label with UID, set backoffLimit + // Use freshNode.UID to detect drift when node is replaced (same name, different UID) + job.Spec.Template.Spec.NodeName = freshNode.Name + job.Labels["node"] = string(freshNode.UID) + job.Spec.BackoffLimit = ptr.To(int32(retries)) + + // Set image and command + job.Spec.Template.Spec.Containers[0].Image = os.Getenv("OPERATOR_IMAGE") + job.Spec.Template.Spec.Containers[0].Command[1] = jobType.GetSubCommand() + + return true, nil + }}..., + ) + + go func() { + defer func() { + runningControllersMutex.Lock() + delete(runningControllers, jobName) + runningControllersMutex.Unlock() + klog.Infof("Node job controller for %q on node %q stopped", jobType.GetSubCommand(), node.Name) + }() + tnfJobController.Run(ctx, 1) + }() +} + +// RunClusterJobController starts job controller for cluster-wide job with round-robin retry logic. +func RunClusterJobController(ctx context.Context, jobType tools.JobType, schedulableNodesFunc SchedulableNodesFunc, affectedNodesFunc AffectedNodesFunc, jobConfigFunc JobConfigFunc, retries int, controllerContext *controllercmd.ControllerContext, operatorClient v1helpers.StaticPodOperatorClient, kubeClient kubernetes.Interface, kubeInformersForNamespaces v1helpers.KubeInformersForNamespaces, conditions []string) { + jobName := jobType.GetJobName(nil) + + // Check if controller already running + runningControllersMutex.Lock() + if runningControllers[jobName] { + runningControllersMutex.Unlock() + klog.V(4).Infof("Cluster job controller for %q is already running, skipping duplicate start", jobType.GetSubCommand()) + return + } + runningControllers[jobName] = true + runningControllersMutex.Unlock() + + klog.Infof("starting cluster job controller for %q", jobType.GetSubCommand()) + + tnfJobController := NewJobController( + jobName, + bindata.MustAsset("tnfdeployment/job.yaml"), + controllerContext.EventRecorder, + operatorClient, + kubeClient, + kubeInformersForNamespaces.InformersFor(operatorclient.TargetNamespace).Batch().V1().Jobs(), + conditions, + []factory.Informer{}, + []JobHookFunc{ + func(_ *operatorv1.OperatorSpec, job *batchv1.Job) (bool, error) { + job.SetName(jobName) + job.Labels["app.kubernetes.io/name"] = jobType.GetNameLabelValue() + + // Sync multi-node job state (handles transitions based on job status) + if err := syncMultiNodeJobState(ctx, job.Name, schedulableNodesFunc, affectedNodesFunc, jobConfigFunc, retries, kubeClient, operatorClient); err != nil { + return false, err + } + + // Check if retry state was created (won't exist if affected nodes not ready) + retryStateMutex.Lock() + _, stateExists := retryState[job.Name] + retryStateMutex.Unlock() + + if !stateExists { + klog.V(4).Infof("Skipping job %s creation: retry state not initialized", job.Name) + return false, nil + } + + // Configure cluster job: round-robin scheduling, no k8s retries (backoffLimit=0) + job.Spec.BackoffLimit = ptr.To(int32(0)) + if err := configureMultiNodeJob(job, retries); err != nil { + return false, err + } + + // Set image and command + job.Spec.Template.Spec.Containers[0].Image = os.Getenv("OPERATOR_IMAGE") + job.Spec.Template.Spec.Containers[0].Command[1] = jobType.GetSubCommand() + + return true, nil + }}..., + ) + + go func() { + defer func() { + runningControllersMutex.Lock() + delete(runningControllers, jobName) + runningControllersMutex.Unlock() + klog.Infof("Cluster job controller for %q stopped", jobType.GetSubCommand()) + }() + tnfJobController.Run(ctx, 1) + }() +} + +// RestartClusterJobOrRunController ensures cluster job controller is running, restarting job if it exists. +func RestartClusterJobOrRunController( + ctx context.Context, + jobType tools.JobType, + schedulableNodesFunc SchedulableNodesFunc, + affectedNodesFunc AffectedNodesFunc, + jobConfigFunc JobConfigFunc, + retries int, + controllerContext *controllercmd.ControllerContext, + operatorClient v1helpers.StaticPodOperatorClient, + kubeClient kubernetes.Interface, + kubeInformersForNamespaces v1helpers.KubeInformersForNamespaces, + conditions []string, + existingJobCompletionTimeout time.Duration) error { + + jobName := jobType.GetJobName(nil) + + // Acquire a lock for this specific job to prevent parallel execution + restartJobLocksMutex.Lock() + jobLock, exists := restartJobLocks[jobName] + if !exists { + jobLock = &sync.Mutex{} + restartJobLocks[jobName] = jobLock + } + restartJobLocksMutex.Unlock() + + jobLock.Lock() + defer jobLock.Unlock() + + // Check if job already exists + jobExists := true + _, err := kubeClient.BatchV1().Jobs(operatorclient.TargetNamespace).Get(ctx, jobName, v1.GetOptions{}) + if err != nil { + if !apierrors.IsNotFound(err) { + return fmt.Errorf("failed to check for existing job %s: %w", jobName, err) + } + jobExists = false + } + + if !jobExists { + // No existing job - reset retry state to start fresh, then run controller + resetJobRetryState(jobName) + RunClusterJobController(ctx, jobType, schedulableNodesFunc, affectedNodesFunc, jobConfigFunc, retries, controllerContext, operatorClient, kubeClient, kubeInformersForNamespaces, conditions) + return nil + } + + // Job exists, wait for it to stop + klog.Infof("Job %s already exists, waiting for it to stop", jobName) + if err := WaitForStopped(ctx, kubeClient, jobName, operatorclient.TargetNamespace, existingJobCompletionTimeout); err != nil { + return fmt.Errorf("failed to wait for job %s to stop: %w", jobName, err) + } + + // Delete the job so the controller can recreate it + klog.Infof("Deleting existing job %s", jobName) + if err := DeleteAndWait(ctx, kubeClient, jobName, operatorclient.TargetNamespace); err != nil { + return fmt.Errorf("failed to delete existing job %s: %w", jobName, err) + } + + // Reset retry state when starting fresh after deleting old job + resetJobRetryState(jobName) + + // Run controller after cleanup completes (CEO might have been restarted) + RunClusterJobController(ctx, jobType, schedulableNodesFunc, affectedNodesFunc, jobConfigFunc, retries, controllerContext, operatorClient, kubeClient, kubeInformersForNamespaces, conditions) + + return nil +} diff --git a/pkg/tnf/pkg/jobs/lifecycle_test.go b/pkg/tnf/pkg/jobs/lifecycle_test.go new file mode 100644 index 0000000000..b2ec07fb4b --- /dev/null +++ b/pkg/tnf/pkg/jobs/lifecycle_test.go @@ -0,0 +1,856 @@ +package jobs + +/* +TEST COVERAGE SUMMARY - lifecycle_test.go +========================================== + +This file tests TNF job controller restart logic and multi-node retry state machine. + +WHAT'S TESTED +------------- + +Job Controller Lifecycle: +├── TestRestartJobOrRunController - Job restart and cleanup logic +│ ├── Job does not exist - just runs controller +│ └── Job exists and stops successfully - deletes and runs controller +├── TestSyncMultiNodeJobState_RetryProgression - Multi-node retry state machine +│ ├── Job fails on node 0 -> advances to node 1 +│ ├── All nodes fail in attempt 1 -> starts attempt 2 +│ ├── Max attempts exhausted -> degraded condition set, reset to attempt 1 +│ └── Job succeeds -> degraded cleared +├── TestSyncMultiNodeJobState_DriftDetection - Infrastructure drift detection +│ ├── schedulableNodesFunc changes (node added) -> reset state, delete job +│ ├── schedulableNodesFunc changes (node removed) -> reset state, delete job +│ ├── jobConfigFunc changes (config updated) -> reset state, delete job +│ └── No drift (nodes and config unchanged) -> state progression continues +├── TestSyncMultiNodeJobState_DegradedCondition - Affected nodes readiness check +│ ├── Affected nodes ready -> job admitted, retry state created +│ ├── Affected nodes not ready < 10min -> job blocked, no degraded condition +│ └── Affected nodes not ready >= 10min -> job blocked, returns error (triggers Degraded) +└── TestSyncMultiNodeJobState_NoSchedulableNodesDegradedCondition - Schedulable nodes availability + ├── No schedulable nodes < 10min -> job blocked, no degraded condition + └── No schedulable nodes >= 10min -> job blocked, returns error (triggers Degraded) +*/ + +import ( + "context" + "sync" + "testing" + "time" + + "github.com/stretchr/testify/require" + batchv1 "k8s.io/api/batch/v1" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/kubernetes/fake" + k8stesting "k8s.io/client-go/testing" + "k8s.io/client-go/tools/cache" + + operatorv1 "github.com/openshift/api/operator/v1" + "github.com/openshift/library-go/pkg/controller/controllercmd" + "github.com/openshift/library-go/pkg/operator/events" + "github.com/openshift/library-go/pkg/operator/v1helpers" + "k8s.io/utils/clock" + + "github.com/openshift/cluster-etcd-operator/pkg/operator/operatorclient" + u "github.com/openshift/cluster-etcd-operator/pkg/testutils" + "github.com/openshift/cluster-etcd-operator/pkg/tnf/pkg/tools" +) + +func TestRestartJobOrRunController(t *testing.T) { + tests := []struct { + name string + jobType tools.JobType + schedulableNodesFunc SchedulableNodesFunc + retries int + setupClient func() *fake.Clientset + expectError bool + errorContains string + expectControllerStarted bool + }{ + { + name: "Job does not exist - just runs controller", + jobType: tools.JobTypeSetup, + schedulableNodesFunc: nil, + retries: 3, + setupClient: func() *fake.Clientset { + // No job exists + return fake.NewClientset() + }, + expectError: false, + expectControllerStarted: true, + }, + { + name: "Job exists and stops successfully - deletes and runs controller", + jobType: tools.JobTypeSetup, + schedulableNodesFunc: nil, + retries: 3, + setupClient: func() *fake.Clientset { + job := &batchv1.Job{ + ObjectMeta: metav1.ObjectMeta{ + Name: "tnf-setup-job", + Namespace: operatorclient.TargetNamespace, + UID: "test-uid", + }, + Status: batchv1.JobStatus{ + Conditions: []batchv1.JobCondition{ + { + Type: batchv1.JobComplete, + Status: corev1.ConditionTrue, + }, + }, + }, + } + client := fake.NewClientset(job) + + // After delete, subsequent Gets should return NotFound + deleted := false + client.PrependReactor("delete", "jobs", func(action k8stesting.Action) (handled bool, ret runtime.Object, err error) { + deleted = true + return false, nil, nil + }) + client.PrependReactor("get", "jobs", func(action k8stesting.Action) (handled bool, ret runtime.Object, err error) { + if deleted { + return true, nil, apierrors.NewNotFound(batchv1.Resource("jobs"), "tnf-setup-job") + } + return false, nil, nil + }) + + return client + }, + expectError: false, + expectControllerStarted: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Reset the global tracking maps for each test + runningControllersMutex.Lock() + runningControllers = make(map[string]bool) + runningControllersMutex.Unlock() + + restartJobLocksMutex.Lock() + restartJobLocks = make(map[string]*sync.Mutex) + restartJobLocksMutex.Unlock() + + // Setup + ctx := context.Background() + client := tt.setupClient() + + fakeOperatorClient := v1helpers.NewFakeStaticPodOperatorClient( + &operatorv1.StaticPodOperatorSpec{}, + u.StaticPodOperatorStatus(), + nil, + nil, + ) + + eventRecorder := events.NewRecorder( + client.CoreV1().Events(operatorclient.TargetNamespace), + "test-tnf", + &corev1.ObjectReference{}, + clock.RealClock{}, + ) + controllerContext := &controllercmd.ControllerContext{ + EventRecorder: eventRecorder, + } + + kubeInformersForNamespaces := v1helpers.NewKubeInformersForNamespaces( + client, + operatorclient.TargetNamespace, + ) + + // Execute - only testing cluster job restart + err := RestartClusterJobOrRunController( + ctx, + tt.jobType, + tt.schedulableNodesFunc, + nil, // affectedNodesFunc + nil, // jobConfigFunc + tt.retries, + controllerContext, + fakeOperatorClient, + client, + kubeInformersForNamespaces, + DefaultConditions, + 1*time.Second, // timeout + ) + + // Verify + if tt.expectError { + require.Error(t, err, "Expected error but got none") + if tt.errorContains != "" { + require.Contains(t, err.Error(), tt.errorContains, + "Expected error to contain %q but got: %v", tt.errorContains, err) + } + } else { + require.NoError(t, err, "Expected no error but got: %v", err) + } + + // Cluster jobs don't have node-specific names + jobName := tt.jobType.GetJobName(nil) + + // Verify controller started based on explicit expectation + runningControllersMutex.Lock() + isRunning := runningControllers[jobName] + runningControllersMutex.Unlock() + + if tt.expectControllerStarted { + require.True(t, isRunning, "Expected controller to be started") + } else { + require.False(t, isRunning, "Expected controller NOT to be started") + } + + // Verify lock was created (always created, even on error) + restartJobLocksMutex.Lock() + _, lockExists := restartJobLocks[jobName] + restartJobLocksMutex.Unlock() + require.True(t, lockExists, "Expected lock to be created for job %q", jobName) + }) + } +} + +func TestSyncMultiNodeJobState_RetryProgression(t *testing.T) { + // This test verifies the multi-node retry state machine: + // - Job fails on node 0 -> retries on node 1 + // - Job fails on node 1 -> new attempt, back to node 0 + // - Max attempts exhausted -> degraded condition set, reset to attempt 1 + // - Job succeeds -> degraded cleared, state reset + + ctx := context.Background() + jobName := "tnf-update-setup-job" + maxRetries := 2 + + // Create two fake nodes + node0 := &corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: "master-0", UID: "uid-0"}} + node1 := &corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: "master-1", UID: "uid-1"}} + targetNodesFunc := func() ([]*corev1.Node, error) { + return []*corev1.Node{node0, node1}, nil + } + + // Setup fake clients + fakeKubeClient := fake.NewClientset() + fakeOperatorClient := v1helpers.NewFakeStaticPodOperatorClient( + &operatorv1.StaticPodOperatorSpec{}, + &operatorv1.StaticPodOperatorStatus{}, + nil, + nil, + ) + + // Reset global state + retryStateMutex.Lock() + retryState = make(map[string]*JobRetryState) + retryStateMutex.Unlock() + + // Helper to get current retry state + getState := func() *JobRetryState { + retryStateMutex.Lock() + defer retryStateMutex.Unlock() + state, exists := retryState[jobName] + if !exists { + return nil + } + state.Mu.Lock() + defer state.Mu.Unlock() + return &JobRetryState{ + AttemptNumber: state.AttemptNumber, + NodeIndex: state.NodeIndex, + TargetNodes: append([]string{}, state.TargetNodes...), + } + } + + // Helper to check degraded condition + isDegraded := func() bool { + _, status, _, _ := fakeOperatorClient.GetStaticPodOperatorState() + for _, cond := range status.Conditions { + if cond.Type == tools.ToPascalCase(jobName)+operatorv1.OperatorStatusTypeDegraded && cond.Status == operatorv1.ConditionTrue { + return true + } + } + return false + } + + // Step 1: Initialize - should create state at attempt 1, node 0 + err := syncMultiNodeJobState(ctx, jobName, targetNodesFunc, nil, nil, maxRetries, fakeKubeClient, fakeOperatorClient) + require.NoError(t, err) + state := getState() + require.NotNil(t, state) + require.Equal(t, 1, state.AttemptNumber, "Should start at attempt 1") + require.Equal(t, 0, state.NodeIndex, "Should start at node index 0") + + // Step 2: Job fails on node 0 -> should advance to node 1 + failedJob := &batchv1.Job{ + ObjectMeta: metav1.ObjectMeta{Name: jobName, Namespace: operatorclient.TargetNamespace}, + Status: batchv1.JobStatus{ + Conditions: []batchv1.JobCondition{ + {Type: batchv1.JobFailed, Status: corev1.ConditionTrue}, + }, + }, + } + _, err = fakeKubeClient.BatchV1().Jobs(operatorclient.TargetNamespace).Create(ctx, failedJob, metav1.CreateOptions{}) + require.NoError(t, err) + + err = syncMultiNodeJobState(ctx, jobName, targetNodesFunc, nil, nil, maxRetries, fakeKubeClient, fakeOperatorClient) + require.NoError(t, err) + state = getState() + require.Equal(t, 1, state.AttemptNumber, "Should still be attempt 1") + require.Equal(t, 1, state.NodeIndex, "Should advance to node index 1") + + // Delete job to simulate ApplyJob detecting drift (NodeName changed) and recreating + err = fakeKubeClient.BatchV1().Jobs(operatorclient.TargetNamespace).Delete(ctx, jobName, metav1.DeleteOptions{}) + require.NoError(t, err) + + // Step 3: Job fails on node 1 -> should start attempt 2, back to node 0 + _, err = fakeKubeClient.BatchV1().Jobs(operatorclient.TargetNamespace).Create(ctx, failedJob.DeepCopy(), metav1.CreateOptions{}) + require.NoError(t, err) + err = syncMultiNodeJobState(ctx, jobName, targetNodesFunc, nil, nil, maxRetries, fakeKubeClient, fakeOperatorClient) + require.NoError(t, err) + state = getState() + require.Equal(t, 2, state.AttemptNumber, "Should advance to attempt 2") + require.Equal(t, 0, state.NodeIndex, "Should reset to node index 0") + require.False(t, isDegraded(), "Should not be degraded yet") + + // Delete job to simulate ApplyJob detecting drift + err = fakeKubeClient.BatchV1().Jobs(operatorclient.TargetNamespace).Delete(ctx, jobName, metav1.DeleteOptions{}) + require.NoError(t, err) + + // Step 4: Exhaust attempt 2 (fail on both nodes) -> should set degraded and reset to attempt 1 + // Fail on node 0 + _, err = fakeKubeClient.BatchV1().Jobs(operatorclient.TargetNamespace).Create(ctx, failedJob.DeepCopy(), metav1.CreateOptions{}) + require.NoError(t, err) + err = syncMultiNodeJobState(ctx, jobName, targetNodesFunc, nil, nil, maxRetries, fakeKubeClient, fakeOperatorClient) + require.NoError(t, err) + // Delete and recreate on node 1 + err = fakeKubeClient.BatchV1().Jobs(operatorclient.TargetNamespace).Delete(ctx, jobName, metav1.DeleteOptions{}) + require.NoError(t, err) + _, err = fakeKubeClient.BatchV1().Jobs(operatorclient.TargetNamespace).Create(ctx, failedJob.DeepCopy(), metav1.CreateOptions{}) + require.NoError(t, err) + err = syncMultiNodeJobState(ctx, jobName, targetNodesFunc, nil, nil, maxRetries, fakeKubeClient, fakeOperatorClient) + require.NoError(t, err) + state = getState() + require.Equal(t, 1, state.AttemptNumber, "Should reset to attempt 1 after exhausting max attempts") + require.Equal(t, 0, state.NodeIndex, "Should reset to node index 0") + require.True(t, isDegraded(), "Should be degraded after exhausting max attempts") + + // Delete job to simulate ApplyJob detecting drift + fakeKubeClient.BatchV1().Jobs(operatorclient.TargetNamespace).Delete(ctx, jobName, metav1.DeleteOptions{}) + + // Step 5: Job succeeds -> should clear degraded and preserve state + successJob := &batchv1.Job{ + ObjectMeta: metav1.ObjectMeta{Name: jobName, Namespace: operatorclient.TargetNamespace}, + Status: batchv1.JobStatus{ + Conditions: []batchv1.JobCondition{ + {Type: batchv1.JobComplete, Status: corev1.ConditionTrue}, + }, + }, + } + fakeKubeClient.BatchV1().Jobs(operatorclient.TargetNamespace).Create(ctx, successJob, metav1.CreateOptions{}) + + err = syncMultiNodeJobState(ctx, jobName, targetNodesFunc, nil, nil, maxRetries, fakeKubeClient, fakeOperatorClient) + require.NoError(t, err) + require.False(t, isDegraded(), "Degraded should be cleared after success") +} + +func TestSyncMultiNodeJobState_DriftDetection(t *testing.T) { + // This test verifies drift detection: + // - schedulableNodesFunc returns different nodes -> reset state, delete job + // - jobConfigFunc returns different config -> reset state, delete job + + ctx := context.Background() + jobName := "tnf-fencing-job" + maxRetries := 3 + + // Create initial nodes + node0 := &corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: "master-0", UID: "uid-0"}} + node1 := &corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: "master-1", UID: "uid-1"}} + node2 := &corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: "master-2", UID: "uid-2"}} + + tests := []struct { + name string + initialNodes []*corev1.Node + driftNodes []*corev1.Node + initialConfig string + driftConfig string + testDriftType string // "nodes" or "config" + expectStateReset bool + expectJobDeleted bool + }{ + { + name: "schedulableNodesFunc changes - node added", + initialNodes: []*corev1.Node{node0, node1}, + driftNodes: []*corev1.Node{node0, node1, node2}, + initialConfig: "config-v1", + driftConfig: "config-v1", + testDriftType: "nodes", + expectStateReset: true, + expectJobDeleted: true, + }, + { + name: "schedulableNodesFunc changes - node removed", + initialNodes: []*corev1.Node{node0, node1, node2}, + driftNodes: []*corev1.Node{node0, node1}, + initialConfig: "config-v1", + driftConfig: "config-v1", + testDriftType: "nodes", + expectStateReset: true, + expectJobDeleted: true, + }, + { + name: "jobConfigFunc changes - config updated", + initialNodes: []*corev1.Node{node0, node1}, + driftNodes: []*corev1.Node{node0, node1}, + initialConfig: "config-v1", + driftConfig: "config-v2", + testDriftType: "config", + expectStateReset: true, + expectJobDeleted: true, + }, + { + name: "no drift - nodes and config unchanged", + initialNodes: []*corev1.Node{node0, node1}, + driftNodes: []*corev1.Node{node0, node1}, + initialConfig: "config-v1", + driftConfig: "config-v1", + testDriftType: "none", + expectStateReset: false, + expectJobDeleted: true, // Job still gets deleted for normal retry progression + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Reset global state + retryStateMutex.Lock() + retryState = make(map[string]*JobRetryState) + retryStateMutex.Unlock() + + // Setup fake clients + fakeKubeClient := fake.NewClientset() + fakeOperatorClient := v1helpers.NewFakeStaticPodOperatorClient( + &operatorv1.StaticPodOperatorSpec{}, + &operatorv1.StaticPodOperatorStatus{}, + nil, + nil, + ) + + // Create initial state with schedulableNodesFunc and jobConfigFunc + currentNodes := tt.initialNodes + currentConfig := tt.initialConfig + + schedulableNodesFunc := func() ([]*corev1.Node, error) { + return currentNodes, nil + } + + jobConfigFunc := func() (string, error) { + return currentConfig, nil + } + + // Step 1: Initialize state + err := syncMultiNodeJobState(ctx, jobName, schedulableNodesFunc, nil, jobConfigFunc, maxRetries, fakeKubeClient, fakeOperatorClient) + require.NoError(t, err) + + // Verify initial state + retryStateMutex.Lock() + state, exists := retryState[jobName] + retryStateMutex.Unlock() + require.True(t, exists, "State should be initialized") + require.Equal(t, 1, state.AttemptNumber) + require.Equal(t, 0, state.NodeIndex) + + // Advance state to attempt 2, node 1 (simulate some retry progression) + state.Mu.Lock() + state.AttemptNumber = 2 + state.NodeIndex = 1 + state.Mu.Unlock() + + // Create a failed job + failedJob := &batchv1.Job{ + ObjectMeta: metav1.ObjectMeta{Name: jobName, Namespace: operatorclient.TargetNamespace}, + Status: batchv1.JobStatus{ + Conditions: []batchv1.JobCondition{ + {Type: batchv1.JobFailed, Status: corev1.ConditionTrue}, + }, + }, + } + fakeKubeClient.BatchV1().Jobs(operatorclient.TargetNamespace).Create(ctx, failedJob, metav1.CreateOptions{}) + + // Step 2: Trigger drift (either nodes or config change) + if tt.testDriftType == "nodes" { + currentNodes = tt.driftNodes + } else if tt.testDriftType == "config" { + currentConfig = tt.driftConfig + } + + // Step 3: Sync again - should detect drift and reset state (or advance state for retry) + err = syncMultiNodeJobState(ctx, jobName, schedulableNodesFunc, nil, jobConfigFunc, maxRetries, fakeKubeClient, fakeOperatorClient) + require.NoError(t, err) + + // For non-drift cases, simulate ApplyJob deleting the job due to NodeName change from retry progression + // (syncMultiNodeJobState updated state, next sync ApplyJob would detect NodeName drift and delete) + if !tt.expectStateReset && tt.expectJobDeleted { + fakeKubeClient.BatchV1().Jobs(operatorclient.TargetNamespace).Delete(ctx, jobName, metav1.DeleteOptions{}) + } + + // Step 4: Verify state reset + retryStateMutex.Lock() + state, exists = retryState[jobName] + retryStateMutex.Unlock() + + if tt.expectStateReset { + require.True(t, exists, "State should exist after reset") + state.Mu.Lock() + require.Equal(t, 1, state.AttemptNumber, "State should reset to attempt 1") + require.Equal(t, 0, state.NodeIndex, "State should reset to node index 0") + state.Mu.Unlock() + } else { + require.True(t, exists, "State should still exist") + state.Mu.Lock() + require.Equal(t, 3, state.AttemptNumber, "State should advance to attempt 3 (no reset)") + require.Equal(t, 0, state.NodeIndex, "State should advance to node 0 after exhausting node 1") + state.Mu.Unlock() + } + + // Step 5: Verify job deleted if drift detected + jobs, err := fakeKubeClient.BatchV1().Jobs(operatorclient.TargetNamespace).List(ctx, metav1.ListOptions{}) + require.NoError(t, err) + if tt.expectJobDeleted { + require.Equal(t, 0, len(jobs.Items), "Job should be deleted on drift") + } else { + require.Equal(t, 1, len(jobs.Items), "Job should still exist (no drift)") + } + }) + } +} + +func TestSyncMultiNodeJobState_DegradedCondition(t *testing.T) { + // This test verifies degraded condition logic: + // - affectedNodesFunc returns not-ready nodes -> blocks job, returns error after 10min + // - affectedNodesFunc returns ready nodes -> clears degraded condition, allows job + + ctx := context.Background() + jobName := "tnf-update-setup-job" + maxRetries := 3 + + // Create nodes + readyNode := &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{Name: "master-0", UID: "uid-0"}, + Status: corev1.NodeStatus{ + Conditions: []corev1.NodeCondition{ + {Type: corev1.NodeReady, Status: corev1.ConditionTrue}, + }, + }, + } + + notReadyNode := &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{Name: "master-1", UID: "uid-1"}, + Status: corev1.NodeStatus{ + Conditions: []corev1.NodeCondition{ + {Type: corev1.NodeReady, Status: corev1.ConditionFalse}, + }, + }, + } + + tests := []struct { + name string + affectedNodes []*corev1.Node + blockedDuration time.Duration + expectRetryStateExist bool + expectError bool + }{ + { + name: "affected nodes ready - job admitted", + affectedNodes: []*corev1.Node{readyNode}, + blockedDuration: 0, + expectRetryStateExist: true, + expectError: false, + }, + { + name: "affected nodes not ready < 10min - job blocked, no error", + affectedNodes: []*corev1.Node{notReadyNode}, + blockedDuration: 5 * time.Minute, + expectRetryStateExist: false, + expectError: false, + }, + { + name: "affected nodes not ready >= 10min - job blocked, error returned", + affectedNodes: []*corev1.Node{notReadyNode}, + blockedDuration: 11 * time.Minute, + expectRetryStateExist: false, + expectError: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Reset global state + retryStateMutex.Lock() + retryState = make(map[string]*JobRetryState) + retryStateMutex.Unlock() + + jobBlockedMutex.Lock() + jobBlockedSince = make(map[string]time.Time) + jobBlockedMutex.Unlock() + + // Setup fake clients + fakeKubeClient := fake.NewClientset() + // Add nodes to fake client + for _, node := range tt.affectedNodes { + _, err := fakeKubeClient.CoreV1().Nodes().Create(ctx, node, metav1.CreateOptions{}) + require.NoError(t, err) + } + + fakeOperatorClient := v1helpers.NewFakeStaticPodOperatorClient( + &operatorv1.StaticPodOperatorSpec{}, + u.StaticPodOperatorStatus(), + nil, + nil, + ) + + schedulableNodesFunc := func() ([]*corev1.Node, error) { + return []*corev1.Node{readyNode}, nil + } + + affectedNodesFunc := func() ([]*corev1.Node, error) { + return tt.affectedNodes, nil + } + + // Simulate blocked time if needed + if tt.blockedDuration > 0 { + jobBlockedMutex.Lock() + jobBlockedSince[jobName] = time.Now().Add(-tt.blockedDuration) + jobBlockedMutex.Unlock() + } + + // Sync state + err := syncMultiNodeJobState(ctx, jobName, schedulableNodesFunc, affectedNodesFunc, nil, maxRetries, fakeKubeClient, fakeOperatorClient) + + // Verify error expectation + if tt.expectError { + require.Error(t, err, "Expected error when blocked >= 10min") + } else { + require.NoError(t, err, "Expected no error") + } + + // Verify retry state existence + retryStateMutex.Lock() + _, stateExists := retryState[jobName] + retryStateMutex.Unlock() + require.Equal(t, tt.expectRetryStateExist, stateExists, "Retry state existence mismatch") + + // Verify degraded condition cleared when unblocked (if it exists) + // Note: Condition might not exist if blocking resolved before 10min timeout + if !tt.expectError && tt.blockedDuration > 0 { + _, status, _, _ := fakeOperatorClient.GetStaticPodOperatorState() + expectedConditionType := tools.ToPascalCase(jobName) + "Degraded" + for _, cond := range status.Conditions { + if cond.Type == expectedConditionType { + require.Equal(t, operatorv1.ConditionFalse, cond.Status, "Degraded should be cleared when unblocked") + break + } + } + } + }) + } +} + +func TestSyncMultiNodeJobState_NoSchedulableNodesDegradedCondition(t *testing.T) { + // This test verifies degraded condition logic for schedulable nodes: + // - schedulableNodesFunc returns empty array -> blocks job, returns error after 10min + // - schedulableNodesFunc returns nodes -> clears degraded condition, allows job + + ctx := context.Background() + jobName := "tnf-fencing-job" + maxRetries := 3 + + // Create a ready node + readyNode := &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{Name: "master-0", UID: "uid-0"}, + Status: corev1.NodeStatus{ + Conditions: []corev1.NodeCondition{ + {Type: corev1.NodeReady, Status: corev1.ConditionTrue}, + }, + }, + } + + tests := []struct { + name string + schedulableNodes []*corev1.Node + blockedDuration time.Duration + expectRetryStateExist bool + expectError bool + }{ + { + name: "schedulable nodes available - job admitted", + schedulableNodes: []*corev1.Node{readyNode}, + blockedDuration: 0, + expectRetryStateExist: true, + expectError: false, + }, + { + name: "no schedulable nodes < 10min - job blocked, no error", + schedulableNodes: []*corev1.Node{}, + blockedDuration: 5 * time.Minute, + expectRetryStateExist: false, + expectError: false, + }, + { + name: "no schedulable nodes >= 10min - job blocked, error returned", + schedulableNodes: []*corev1.Node{}, + blockedDuration: 11 * time.Minute, + expectRetryStateExist: false, + expectError: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Reset global state + retryStateMutex.Lock() + retryState = make(map[string]*JobRetryState) + retryStateMutex.Unlock() + + jobNoSchedulableNodesMutex.Lock() + jobNoSchedulableNodesSince = make(map[string]time.Time) + jobNoSchedulableNodesMutex.Unlock() + + // Setup fake clients + fakeKubeClient := fake.NewClientset() + // Add readyNode to fake client (used by affectedNodesFunc) + _, err := fakeKubeClient.CoreV1().Nodes().Create(ctx, readyNode, metav1.CreateOptions{}) + require.NoError(t, err) + // Add schedulable nodes to fake client (if any beyond readyNode) + for _, node := range tt.schedulableNodes { + if node.Name != readyNode.Name { + _, err := fakeKubeClient.CoreV1().Nodes().Create(ctx, node, metav1.CreateOptions{}) + require.NoError(t, err) + } + } + + fakeOperatorClient := v1helpers.NewFakeStaticPodOperatorClient( + &operatorv1.StaticPodOperatorSpec{}, + u.StaticPodOperatorStatus(), + nil, + nil, + ) + + schedulableNodesFunc := func() ([]*corev1.Node, error) { + return tt.schedulableNodes, nil + } + + affectedNodesFunc := func() ([]*corev1.Node, error) { + // All affected nodes are ready (not blocking) + return []*corev1.Node{readyNode}, nil + } + + // Simulate blocked time if needed + if tt.blockedDuration > 0 { + jobNoSchedulableNodesMutex.Lock() + jobNoSchedulableNodesSince[jobName] = time.Now().Add(-tt.blockedDuration) + jobNoSchedulableNodesMutex.Unlock() + } + + // Sync state + err = syncMultiNodeJobState(ctx, jobName, schedulableNodesFunc, affectedNodesFunc, nil, maxRetries, fakeKubeClient, fakeOperatorClient) + + // Verify error expectation + if tt.expectError { + require.Error(t, err, "Expected error when blocked >= 10min") + } else { + require.NoError(t, err, "Expected no error") + } + + // Verify retry state existence + retryStateMutex.Lock() + _, stateExists := retryState[jobName] + retryStateMutex.Unlock() + require.Equal(t, tt.expectRetryStateExist, stateExists, "Retry state existence mismatch") + + // Verify degraded condition cleared when unblocked (if it exists) + // Note: Condition might not exist if blocking resolved before 10min timeout + if !tt.expectError && tt.blockedDuration > 0 { + _, status, _, _ := fakeOperatorClient.GetStaticPodOperatorState() + expectedConditionType := tools.ToPascalCase(jobName) + "Degraded" + for _, cond := range status.Conditions { + if cond.Type == expectedConditionType { + require.Equal(t, operatorv1.ConditionFalse, cond.Status, "Degraded should be cleared when unblocked") + break + } + } + } + }) + } +} + +// testNodeInformer is a minimal mock of cache.SharedIndexInformer for testing +type testNodeInformer struct { + indexer cache.Indexer + synced bool +} + +func (m *testNodeInformer) GetIndexer() cache.Indexer { + return m.indexer +} + +func (m *testNodeInformer) HasSynced() bool { + return m.synced +} + +func (m *testNodeInformer) AddEventHandler(handler cache.ResourceEventHandler) (cache.ResourceEventHandlerRegistration, error) { + return nil, nil +} + +func (m *testNodeInformer) AddEventHandlerWithResyncPeriod(handler cache.ResourceEventHandler, resyncPeriod time.Duration) (cache.ResourceEventHandlerRegistration, error) { + return nil, nil +} + +func (m *testNodeInformer) RemoveEventHandler(handle cache.ResourceEventHandlerRegistration) error { + return nil +} + +func (m *testNodeInformer) GetStore() cache.Store { + return m.indexer +} + +func (m *testNodeInformer) GetController() cache.Controller { + return nil +} + +func (m *testNodeInformer) Run(stopCh <-chan struct{}) { +} + +func (m *testNodeInformer) HasStarted() bool { + return true +} + +func (m *testNodeInformer) LastSyncResourceVersion() string { + return "" +} + +func (m *testNodeInformer) SetWatchErrorHandler(handler cache.WatchErrorHandler) error { + return nil +} + +func (m *testNodeInformer) SetTransform(f cache.TransformFunc) error { + return nil +} + +func (m *testNodeInformer) IsStopped() bool { + return false +} + +func (m *testNodeInformer) AddEventHandlerWithOptions(handler cache.ResourceEventHandler, options cache.HandlerOptions) (cache.ResourceEventHandlerRegistration, error) { + return nil, nil +} + +func (m *testNodeInformer) AddIndexers(indexers cache.Indexers) error { + return nil +} + +func (m *testNodeInformer) RunWithContext(ctx context.Context) { +} + +func (m *testNodeInformer) SetWatchErrorHandlerWithContext(handler cache.WatchErrorHandlerWithContext) error { + return nil +} diff --git a/pkg/tnf/pkg/jobs/tnf.go b/pkg/tnf/pkg/jobs/tnf.go deleted file mode 100644 index f0fe57538d..0000000000 --- a/pkg/tnf/pkg/jobs/tnf.go +++ /dev/null @@ -1,144 +0,0 @@ -package jobs - -import ( - "context" - "fmt" - "os" - "sync" - "time" - - operatorv1 "github.com/openshift/api/operator/v1" - "github.com/openshift/library-go/pkg/controller/controllercmd" - "github.com/openshift/library-go/pkg/controller/factory" - "github.com/openshift/library-go/pkg/operator/v1helpers" - batchv1 "k8s.io/api/batch/v1" - apierrors "k8s.io/apimachinery/pkg/api/errors" - v1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/client-go/kubernetes" - "k8s.io/klog/v2" - - "github.com/openshift/cluster-etcd-operator/bindata" - "github.com/openshift/cluster-etcd-operator/pkg/operator/operatorclient" - "github.com/openshift/cluster-etcd-operator/pkg/tnf/pkg/tools" -) - -var ( - // runningControllers tracks which controllers are already running to prevent duplicates - runningControllers = make(map[string]bool) - // runningControllersMutex protects the runningControllers map - runningControllersMutex sync.Mutex - - // restartJobLocks tracks in-flight RestartJobOrRunController calls to prevent parallel execution - restartJobLocks = make(map[string]*sync.Mutex) - // restartJobLocksMutex protects the restartJobLocks map - restartJobLocksMutex sync.Mutex -) - -func RunTNFJobController(ctx context.Context, jobType tools.JobType, nodeName *string, controllerContext *controllercmd.ControllerContext, operatorClient v1helpers.StaticPodOperatorClient, kubeClient kubernetes.Interface, kubeInformersForNamespaces v1helpers.KubeInformersForNamespaces, conditions []string) { - nodeNameForLogs := "undefined" - if nodeName != nil { - nodeNameForLogs = *nodeName - } - - // Check if a controller for this jobType and nodeName is already running - controllerKey := jobType.GetJobName(nodeName) - runningControllersMutex.Lock() - if runningControllers[controllerKey] { - runningControllersMutex.Unlock() - klog.Infof("Two Node Fencing job controller for command %q on node %q is already running, skipping duplicate start", jobType.GetSubCommand(), nodeNameForLogs) - return - } - // Mark this controller as running - runningControllers[controllerKey] = true - runningControllersMutex.Unlock() - - klog.Infof("starting Two Node Fencing job controller for command %q on node %q", jobType.GetSubCommand(), nodeNameForLogs) - tnfJobController := NewJobController( - jobType.GetJobName(nodeName), - bindata.MustAsset("tnfdeployment/job.yaml"), - controllerContext.EventRecorder, - operatorClient, - kubeClient, - kubeInformersForNamespaces.InformersFor(operatorclient.TargetNamespace).Batch().V1().Jobs(), - conditions, - []factory.Informer{}, - []JobHookFunc{ - func(_ *operatorv1.OperatorSpec, job *batchv1.Job) error { - if nodeName != nil { - job.Spec.Template.Spec.NodeName = *nodeName - } - job.SetName(jobType.GetJobName(nodeName)) - job.Labels["app.kubernetes.io/name"] = jobType.GetNameLabelValue() - job.Spec.Template.Spec.Containers[0].Image = os.Getenv("OPERATOR_IMAGE") - job.Spec.Template.Spec.Containers[0].Command[1] = jobType.GetSubCommand() - return nil - }}..., - ) - go func() { - defer func() { - runningControllersMutex.Lock() - delete(runningControllers, controllerKey) - runningControllersMutex.Unlock() - klog.Infof("Two Node Fencing job controller for command %q on node %q stopped", jobType.GetSubCommand(), nodeNameForLogs) - }() - tnfJobController.Run(ctx, 1) - }() -} - -func RestartJobOrRunController( - ctx context.Context, - jobType tools.JobType, - nodeName *string, - controllerContext *controllercmd.ControllerContext, - operatorClient v1helpers.StaticPodOperatorClient, - kubeClient kubernetes.Interface, - kubeInformersForNamespaces v1helpers.KubeInformersForNamespaces, - conditions []string, - existingJobCompletionTimeout time.Duration) error { - - // Acquire a lock for this specific jobType/nodeName combination to prevent parallel execution - jobName := jobType.GetJobName(nodeName) - - restartJobLocksMutex.Lock() - jobLock, exists := restartJobLocks[jobName] - if !exists { - jobLock = &sync.Mutex{} - restartJobLocks[jobName] = jobLock - } - restartJobLocksMutex.Unlock() - - jobLock.Lock() - defer jobLock.Unlock() - - // Check if job already exists - jobExists := true - _, err := kubeClient.BatchV1().Jobs(operatorclient.TargetNamespace).Get(ctx, jobName, v1.GetOptions{}) - if err != nil { - if !apierrors.IsNotFound(err) { - return fmt.Errorf("failed to check for existing job %s: %w", jobName, err) - } - jobExists = false - } - - // always try to run the controller, CEO might have been restarted - RunTNFJobController(ctx, jobType, nodeName, controllerContext, operatorClient, kubeClient, kubeInformersForNamespaces, conditions) - - if !jobExists { - // we are done - return nil - } - - // Job exists, wait for completion - klog.Infof("Job %s already exists, waiting for being stopped", jobName) - if err := WaitForStopped(ctx, kubeClient, jobName, operatorclient.TargetNamespace, existingJobCompletionTimeout); err != nil { - return fmt.Errorf("failed to wait for update-setup job %s to complete: %w", jobName, err) - } - - // Delete the job so the controller can recreate it - klog.Infof("Deleting existing job %s", jobName) - if err := DeleteAndWait(ctx, kubeClient, jobName, operatorclient.TargetNamespace); err != nil { - return fmt.Errorf("failed to delete existing update-setup job %s: %w", jobName, err) - } - - return nil -} diff --git a/pkg/tnf/pkg/jobs/tnf_test.go b/pkg/tnf/pkg/jobs/tnf_test.go deleted file mode 100644 index 955532fdb9..0000000000 --- a/pkg/tnf/pkg/jobs/tnf_test.go +++ /dev/null @@ -1,491 +0,0 @@ -package jobs - -import ( - "context" - "maps" - "sync" - "testing" - "time" - - "github.com/stretchr/testify/require" - batchv1 "k8s.io/api/batch/v1" - corev1 "k8s.io/api/core/v1" - apierrors "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/client-go/kubernetes/fake" - k8stesting "k8s.io/client-go/testing" - - operatorv1 "github.com/openshift/api/operator/v1" - "github.com/openshift/library-go/pkg/controller/controllercmd" - "github.com/openshift/library-go/pkg/operator/events" - "github.com/openshift/library-go/pkg/operator/v1helpers" - "k8s.io/utils/clock" - - "github.com/openshift/cluster-etcd-operator/pkg/operator/operatorclient" - u "github.com/openshift/cluster-etcd-operator/pkg/testutils" - "github.com/openshift/cluster-etcd-operator/pkg/tnf/pkg/tools" -) - -func TestRunTNFJobController(t *testing.T) { - tests := []struct { - name string - jobType tools.JobType - nodeName *string - existingControllers map[string]bool - expectControllerRun bool - expectedControllerKey string - }{ - { - name: "Start controller for auth job without node name", - jobType: tools.JobTypeAuth, - nodeName: nil, - existingControllers: make(map[string]bool), - expectControllerRun: true, - expectedControllerKey: "tnf-auth-job", - }, - { - name: "Start controller for auth job with node name", - jobType: tools.JobTypeAuth, - nodeName: stringPtr("master-0"), - existingControllers: make(map[string]bool), - expectControllerRun: true, - expectedControllerKey: tools.JobTypeAuth.GetJobName(stringPtr("master-0")), - }, - { - name: "Skip starting controller when already running", - jobType: tools.JobTypeSetup, - nodeName: nil, - existingControllers: map[string]bool{ - "tnf-setup-job": true, - }, - expectControllerRun: false, - expectedControllerKey: "tnf-setup-job", - }, - { - name: "Start different controller when another is running", - jobType: tools.JobTypeFencing, - nodeName: nil, - existingControllers: map[string]bool{ - "tnf-setup-job": true, - }, - expectControllerRun: true, - expectedControllerKey: "tnf-fencing-job", - }, - { - name: "Start controller for different node when same job type exists", - jobType: tools.JobTypeAuth, - nodeName: stringPtr("master-1"), - existingControllers: map[string]bool{ - tools.JobTypeAuth.GetJobName(stringPtr("master-0")): true, - }, - expectControllerRun: true, - expectedControllerKey: tools.JobTypeAuth.GetJobName(stringPtr("master-1")), - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - // Reset the global tracking maps - runningControllersMutex.Lock() - runningControllers = make(map[string]bool) - maps.Copy(runningControllers, tt.existingControllers) - runningControllersMutex.Unlock() - - // Setup - ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) - defer cancel() - - fakeKubeClient := fake.NewSimpleClientset() - fakeOperatorClient := v1helpers.NewFakeStaticPodOperatorClient( - &operatorv1.StaticPodOperatorSpec{}, - u.StaticPodOperatorStatus(), - nil, - nil, - ) - - eventRecorder := events.NewRecorder( - fakeKubeClient.CoreV1().Events(operatorclient.TargetNamespace), - "test-tnf", - &corev1.ObjectReference{}, - clock.RealClock{}, - ) - controllerContext := &controllercmd.ControllerContext{ - EventRecorder: eventRecorder, - } - - kubeInformersForNamespaces := v1helpers.NewKubeInformersForNamespaces( - fakeKubeClient, - operatorclient.TargetNamespace, - ) - - // Execute - RunTNFJobController( - ctx, - tt.jobType, - tt.nodeName, - controllerContext, - fakeOperatorClient, - fakeKubeClient, - kubeInformersForNamespaces, - DefaultConditions, - ) - - // Give the goroutine a moment to start if expected - time.Sleep(100 * time.Millisecond) - - // Verify - runningControllersMutex.Lock() - isRunning := runningControllers[tt.expectedControllerKey] - runningControllersMutex.Unlock() - - if tt.expectControllerRun { - require.True(t, isRunning, - "Expected controller %q to be marked as running", tt.expectedControllerKey) - } else { - // Controller should still be marked as running from before - require.True(t, isRunning, - "Expected controller %q to still be marked as running", tt.expectedControllerKey) - } - }) - } -} - -func TestRestartJobOrRunController(t *testing.T) { - tests := []struct { - name string - jobType tools.JobType - nodeName *string - setupClient func() *fake.Clientset - expectError bool - errorContains string - expectJobDeleted bool - expectWaitForStopped bool - }{ - { - name: "Job does not exist - just runs controller", - jobType: tools.JobTypeAuth, - nodeName: stringPtr("master-0"), - setupClient: func() *fake.Clientset { - // No job exists - return fake.NewSimpleClientset() - }, - expectError: false, - expectJobDeleted: false, - expectWaitForStopped: false, - }, - { - name: "Job exists and stops successfully - deletes and runs controller", - jobType: tools.JobTypeSetup, - nodeName: nil, - setupClient: func() *fake.Clientset { - job := &batchv1.Job{ - ObjectMeta: metav1.ObjectMeta{ - Name: "tnf-setup-job", - Namespace: operatorclient.TargetNamespace, - UID: "test-uid", - }, - Status: batchv1.JobStatus{ - Conditions: []batchv1.JobCondition{ - { - Type: batchv1.JobComplete, - Status: corev1.ConditionTrue, - }, - }, - }, - } - client := fake.NewSimpleClientset(job) - - // After delete, subsequent Gets should return NotFound - deleted := false - client.PrependReactor("delete", "jobs", func(action k8stesting.Action) (handled bool, ret runtime.Object, err error) { - deleted = true - return false, nil, nil - }) - client.PrependReactor("get", "jobs", func(action k8stesting.Action) (handled bool, ret runtime.Object, err error) { - if deleted { - return true, nil, apierrors.NewNotFound(batchv1.Resource("jobs"), "tnf-setup-job") - } - return false, nil, nil - }) - - return client - }, - expectError: false, - expectJobDeleted: true, - expectWaitForStopped: true, - }, - { - name: "Job exists but Get returns error - returns error", - jobType: tools.JobTypeAuth, - nodeName: stringPtr("master-0"), - setupClient: func() *fake.Clientset { - client := fake.NewSimpleClientset() - client.PrependReactor("get", "jobs", func(action k8stesting.Action) (handled bool, ret runtime.Object, err error) { - return true, nil, apierrors.NewServiceUnavailable("service unavailable") - }) - return client - }, - expectError: true, - errorContains: "failed to check for existing job", - expectJobDeleted: false, - expectWaitForStopped: false, - }, - { - name: "Job exists but WaitForStopped times out - returns error", - jobType: tools.JobTypeFencing, - nodeName: nil, - setupClient: func() *fake.Clientset { - job := &batchv1.Job{ - ObjectMeta: metav1.ObjectMeta{ - Name: "tnf-fencing-job", - Namespace: operatorclient.TargetNamespace, - UID: "test-uid", - }, - Status: batchv1.JobStatus{ - Conditions: []batchv1.JobCondition{}, // Still running - }, - } - return fake.NewSimpleClientset(job) - }, - expectError: true, - errorContains: "failed to wait for update-setup job", - expectJobDeleted: false, - expectWaitForStopped: true, - }, - { - name: "Job exists, stops, but delete fails - returns error", - jobType: tools.JobTypeAfterSetup, - nodeName: stringPtr("master-1"), - setupClient: func() *fake.Clientset { - jobName := tools.JobTypeAfterSetup.GetJobName(stringPtr("master-1")) - job := &batchv1.Job{ - ObjectMeta: metav1.ObjectMeta{ - Name: jobName, - Namespace: operatorclient.TargetNamespace, - UID: "test-uid", - }, - Status: batchv1.JobStatus{ - Conditions: []batchv1.JobCondition{ - { - Type: batchv1.JobComplete, - Status: corev1.ConditionTrue, - }, - }, - }, - } - client := fake.NewSimpleClientset(job) - - // Make Get succeed initially, then fail on delete - client.PrependReactor("delete", "jobs", func(action k8stesting.Action) (handled bool, ret runtime.Object, err error) { - return true, nil, apierrors.NewForbidden(batchv1.Resource("jobs"), jobName, nil) - }) - - return client - }, - expectError: true, - errorContains: "failed to delete existing update-setup job", - expectJobDeleted: false, - expectWaitForStopped: true, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - // Reset the global tracking maps for each test - runningControllersMutex.Lock() - runningControllers = make(map[string]bool) - runningControllersMutex.Unlock() - - restartJobLocksMutex.Lock() - restartJobLocks = make(map[string]*sync.Mutex) - restartJobLocksMutex.Unlock() - - // Setup - ctx := context.Background() - client := tt.setupClient() - - fakeOperatorClient := v1helpers.NewFakeStaticPodOperatorClient( - &operatorv1.StaticPodOperatorSpec{}, - u.StaticPodOperatorStatus(), - nil, - nil, - ) - - eventRecorder := events.NewRecorder( - client.CoreV1().Events(operatorclient.TargetNamespace), - "test-tnf", - &corev1.ObjectReference{}, - clock.RealClock{}, - ) - controllerContext := &controllercmd.ControllerContext{ - EventRecorder: eventRecorder, - } - - kubeInformersForNamespaces := v1helpers.NewKubeInformersForNamespaces( - client, - operatorclient.TargetNamespace, - ) - - // Execute - err := RestartJobOrRunController( - ctx, - tt.jobType, - tt.nodeName, - controllerContext, - fakeOperatorClient, - client, - kubeInformersForNamespaces, - DefaultConditions, - 1*time.Second, // Short timeout for tests - ) - - // Verify - if tt.expectError { - require.Error(t, err, "Expected error but got none") - if tt.errorContains != "" { - require.Contains(t, err.Error(), tt.errorContains, - "Expected error to contain %q but got: %v", tt.errorContains, err) - } - } else { - require.NoError(t, err, "Expected no error but got: %v", err) - } - - // Verify controller was started (only if no early error) - jobName := tt.jobType.GetJobName(tt.nodeName) - - // Only verify controller started if we didn't get an error before RunTNFJobController - if !tt.expectError || tt.expectWaitForStopped { - runningControllersMutex.Lock() - isRunning := runningControllers[jobName] - runningControllersMutex.Unlock() - require.True(t, isRunning, "Expected controller to be started") - } - - // Verify lock was created (always created, even on error) - restartJobLocksMutex.Lock() - _, lockExists := restartJobLocks[jobName] - restartJobLocksMutex.Unlock() - require.True(t, lockExists, "Expected lock to be created for job %q", jobName) - }) - } -} - -func TestRestartJobOrRunController_ParallelExecution(t *testing.T) { - // Reset the global tracking maps - runningControllersMutex.Lock() - runningControllers = make(map[string]bool) - runningControllersMutex.Unlock() - - restartJobLocksMutex.Lock() - restartJobLocks = make(map[string]*sync.Mutex) - restartJobLocksMutex.Unlock() - - // Setup - job := &batchv1.Job{ - ObjectMeta: metav1.ObjectMeta{ - Name: "tnf-setup-job", - Namespace: operatorclient.TargetNamespace, - UID: "test-uid", - }, - Status: batchv1.JobStatus{ - Conditions: []batchv1.JobCondition{ - { - Type: batchv1.JobComplete, - Status: corev1.ConditionTrue, - }, - }, - }, - } - client := fake.NewSimpleClientset(job) - - // Track delete calls - var deleteCalls int - var deleteCallsMutex sync.Mutex - deleted := false - - client.PrependReactor("delete", "jobs", func(action k8stesting.Action) (handled bool, ret runtime.Object, err error) { - deleteCallsMutex.Lock() - deleteCalls++ - deleted = true - deleteCallsMutex.Unlock() - return false, nil, nil - }) - - client.PrependReactor("get", "jobs", func(action k8stesting.Action) (handled bool, ret runtime.Object, err error) { - deleteCallsMutex.Lock() - isDeleted := deleted - deleteCallsMutex.Unlock() - - if isDeleted { - return true, nil, apierrors.NewNotFound(batchv1.Resource("jobs"), "tnf-setup-job") - } - return false, nil, nil - }) - - fakeOperatorClient := v1helpers.NewFakeStaticPodOperatorClient( - &operatorv1.StaticPodOperatorSpec{}, - u.StaticPodOperatorStatus(), - nil, - nil, - ) - - eventRecorder := events.NewRecorder( - client.CoreV1().Events(operatorclient.TargetNamespace), - "test-tnf", - &corev1.ObjectReference{}, - clock.RealClock{}, - ) - controllerContext := &controllercmd.ControllerContext{ - EventRecorder: eventRecorder, - } - - kubeInformersForNamespaces := v1helpers.NewKubeInformersForNamespaces( - client, - operatorclient.TargetNamespace, - ) - - // Execute multiple calls in parallel - var wg sync.WaitGroup - numCalls := 3 - errors := make([]error, numCalls) - - for i := range numCalls { - wg.Add(1) - go func(idx int) { - defer wg.Done() - errors[idx] = RestartJobOrRunController( - context.Background(), - tools.JobTypeSetup, - nil, - controllerContext, - fakeOperatorClient, - client, - kubeInformersForNamespaces, - DefaultConditions, - 2*time.Second, - ) - }(i) - } - - wg.Wait() - - // Verify - all calls should succeed - for i, err := range errors { - require.NoError(t, err, "Call %d should succeed", i) - } - - // Verify delete was only called once due to locking - deleteCallsMutex.Lock() - actualDeleteCalls := deleteCalls - deleteCallsMutex.Unlock() - - require.Equal(t, 1, actualDeleteCalls, - "Delete should only be called once despite parallel execution") -} - -// Helper functions - -func stringPtr(s string) *string { - return &s -} diff --git a/pkg/tnf/pkg/jobs/utils.go b/pkg/tnf/pkg/jobs/utils.go index a1869e6d86..0cfd119683 100644 --- a/pkg/tnf/pkg/jobs/utils.go +++ b/pkg/tnf/pkg/jobs/utils.go @@ -60,7 +60,8 @@ func waitWithConditionFunc(ctx context.Context, kubeClient kubernetes.Interface, }) } -// DeleteAndWait deletes a job and waits until it disappears from the API +// DeleteAndWait deletes a job and waits until it disappears from the API. +// Uses Background propagation to automatically delete child pods. func DeleteAndWait(ctx context.Context, kubeClient kubernetes.Interface, jobName string, jobNamespace string) error { klog.V(4).Infof("deleting oldJob %s", jobName) oldJob, err := kubeClient.BatchV1().Jobs(jobNamespace).Get(ctx, jobName, v1.GetOptions{}) @@ -73,7 +74,11 @@ func DeleteAndWait(ctx context.Context, kubeClient kubernetes.Interface, jobName } oldJobUID := oldJob.GetUID() - err = kubeClient.BatchV1().Jobs(jobNamespace).Delete(ctx, jobName, v1.DeleteOptions{}) + // Delete job with Background propagation to automatically clean up pods + propagationPolicy := v1.DeletePropagationBackground + err = kubeClient.BatchV1().Jobs(jobNamespace).Delete(ctx, jobName, v1.DeleteOptions{ + PropagationPolicy: &propagationPolicy, + }) if err != nil { if apierrors.IsNotFound(err) { return nil @@ -98,7 +103,34 @@ func DeleteAndWait(ctx context.Context, kubeClient kubernetes.Interface, jobName } func IsStopped(job batchv1.Job) bool { - return IsComplete(job) || IsFailed(job) + // Check for explicit conditions + if IsComplete(job) || IsFailed(job) { + return true + } + + // Check for FailureTarget condition (Kubernetes 1.31+) + // FailureTarget means job is targeting failure but pods may still be terminating + if IsConditionTrue(job.Status.Conditions, batchv1.JobConditionType("FailureTarget")) { + klog.V(2).Infof("Job %s considered stopped: FailureTarget condition is True", job.Name) + return true + } + + // Also check if job has no active pods and has exceeded backoff limit + // This handles cases where pods are stuck (e.g., Terminating on dead node) and + // Kubernetes hasn't set the Failed condition yet + if job.Status.Active == 0 && job.Status.Failed > 0 { + backoffLimit := int32(6) // Kubernetes default + if job.Spec.BackoffLimit != nil { + backoffLimit = *job.Spec.BackoffLimit + } + if job.Status.Failed > backoffLimit { + klog.V(2).Infof("Job %s considered stopped: Active=0, Failed=%d > BackoffLimit=%d (no explicit condition set)", + job.Name, job.Status.Failed, backoffLimit) + return true + } + } + + return false } func IsComplete(job batchv1.Job) bool { diff --git a/pkg/tnf/pkg/pacemaker/client_helpers.go b/pkg/tnf/pkg/pacemaker/client_helpers.go deleted file mode 100644 index 0e99eb1718..0000000000 --- a/pkg/tnf/pkg/pacemaker/client_helpers.go +++ /dev/null @@ -1,52 +0,0 @@ -package pacemaker - -import ( - "fmt" - - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/runtime/serializer" - "k8s.io/client-go/rest" - - pacmkrv1 "github.com/openshift/api/etcd/v1" -) - -// getKubeConfig returns a Kubernetes REST config for in-cluster use. -// This function is designed for code running inside a Kubernetes pod (e.g., CronJob). -// It does not support kubeconfig file fallback - if you need that, use the -// clientcmd package directly with NewDefaultClientConfigLoadingRules(). -func getKubeConfig() (*rest.Config, error) { - config, err := rest.InClusterConfig() - if err != nil { - return nil, fmt.Errorf("failed to get in-cluster config (is this running inside a pod?): %w", err) - } - return config, nil -} - -// createPacemakerRESTClient creates a REST client configured for PacemakerStatus CRs. -// It takes a base Kubernetes REST config and configures it with the necessary -// scheme, group version, and serializer for the PacemakerStatus CRD. -func createPacemakerRESTClient(baseConfig *rest.Config) (rest.Interface, error) { - if baseConfig == nil { - return nil, fmt.Errorf("baseConfig cannot be nil") - } - - // Set up the scheme for PacemakerStatus CRD - scheme := runtime.NewScheme() - if err := pacmkrv1.AddToScheme(scheme); err != nil { - return nil, fmt.Errorf("failed to add PacemakerStatus scheme: %w", err) - } - - // Configure the REST client for the PacemakerStatus CRD - pacemakerConfig := rest.CopyConfig(baseConfig) - pacemakerConfig.GroupVersion = &pacmkrv1.SchemeGroupVersion - pacemakerConfig.APIPath = KubernetesAPIPath - pacemakerConfig.NegotiatedSerializer = serializer.NewCodecFactory(scheme) - pacemakerConfig.ContentConfig.ContentType = "application/json" - - restClient, err := rest.RESTClientFor(pacemakerConfig) - if err != nil { - return nil, fmt.Errorf("failed to create REST client for PacemakerStatus: %w", err) - } - - return restClient, nil -} diff --git a/pkg/tnf/pkg/pacemaker/healthcheck.go b/pkg/tnf/pkg/pacemaker/healthcheck.go index 13422d40ac..3ddf2cff31 100644 --- a/pkg/tnf/pkg/pacemaker/healthcheck.go +++ b/pkg/tnf/pkg/pacemaker/healthcheck.go @@ -109,16 +109,6 @@ type HealthStatus struct { CRLastUpdated time.Time } -// pacemakerListWatch wraps cache.ListWatch to opt out of WatchList semantics. -// The OCP apiextensions-apiserver does not enable the server-side WatchList gate, -// so the reflector's sendInitialEvents request is rejected. This wrapper tells -// the reflector to skip watchList() and use list+watch directly. -type pacemakerListWatch struct { - cache.ListWatch -} - -func (pacemakerListWatch) IsWatchListSemanticsUnSupported() bool { return true } - // HealthCheck monitors pacemaker status in ExternalEtcd topology clusters type HealthCheck struct { operatorClient v1helpers.StaticPodOperatorClient @@ -152,7 +142,7 @@ func NewHealthCheck( restConfig *rest.Config, ) (factory.Controller, cache.SharedIndexInformer, error) { // Create REST client for PacemakerStatus CRs - restClient, err := createPacemakerRESTClient(restConfig) + restClient, err := CreatePacemakerRESTClient(restConfig) if err != nil { return nil, nil, fmt.Errorf("failed to create REST client: %w", err) } @@ -166,13 +156,14 @@ func NewHealthCheck( // Create informer for PacemakerCluster klog.Infof("Creating PacemakerCluster informer for group %s, resource %s", pacmkrv1.SchemeGroupVersion.String(), PacemakerResourceName) informer := cache.NewSharedIndexInformer( - &pacemakerListWatch{cache.ListWatch{ + &PacemakerListWatch{ListWatch: cache.ListWatch{ ListFunc: func(options metav1.ListOptions) (runtime.Object, error) { klog.V(4).Infof("PacemakerCluster informer ListFunc called for resource %s", PacemakerResourceName) + sanitizedOptions := SanitizeListOptions(options) result := &pacmkrv1.PacemakerClusterList{} err := restClient.Get(). Resource(PacemakerResourceName). - VersionedParams(&options, runtime.NewParameterCodec(scheme)). + VersionedParams(&sanitizedOptions, runtime.NewParameterCodec(scheme)). Do(context.Background()). Into(result) if err != nil { @@ -184,9 +175,10 @@ func NewHealthCheck( }, WatchFunc: func(options metav1.ListOptions) (watch.Interface, error) { klog.V(4).Infof("PacemakerCluster informer WatchFunc called for resource %s", PacemakerResourceName) + sanitizedOptions := SanitizeListOptions(options) watcher, err := restClient.Get(). Resource(PacemakerResourceName). - VersionedParams(&options, runtime.NewParameterCodec(scheme)). + VersionedParams(&sanitizedOptions, runtime.NewParameterCodec(scheme)). Watch(context.Background()) if err != nil { klog.Errorf("Failed to watch PacemakerCluster resources (%s): %v", PacemakerResourceName, err) @@ -199,11 +191,22 @@ func NewHealthCheck( cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc}, ) + return NewHealthCheckWithInformer(operatorClient, kubeClient, eventRecorder, informer) +} + +// NewHealthCheckWithInformer creates a new HealthCheck using an existing PacemakerCluster informer. +// Use this when the informer is already created and managed by another controller (e.g., lifecycle manager). +func NewHealthCheckWithInformer( + operatorClient v1helpers.StaticPodOperatorClient, + kubeClient kubernetes.Interface, + eventRecorder events.Recorder, + pacemakerInformer cache.SharedIndexInformer, +) (factory.Controller, cache.SharedIndexInformer, error) { c := &HealthCheck{ operatorClient: operatorClient, kubeClient: kubeClient, eventRecorder: eventRecorder, - pacemakerInformer: informer, + pacemakerInformer: pacemakerInformer, recordedEvents: make(map[string]time.Time), disruptionTracker: NewDisruptionTracker(), // previous starts as nil - first sync will be treated as "from Unknown" @@ -222,14 +225,14 @@ func NewHealthCheck( WithSync(c.sync). WithInformers( operatorClient.Informer(), - informer, + pacemakerInformer, ).ToController("PacemakerHealthCheck", syncCtx.Recorder()) klog.Infof("PacemakerHealthCheck controller successfully created and ready to start") klog.Infof("PacemakerHealthCheck informers to sync: 1) operatorClient.Informer(), 2) PacemakerCluster informer") klog.Infof("PacemakerCluster informer must be started separately before controller.Run() is called") klog.Infof("factory.Controller will wait up to 10 minutes for informers to sync, then exit if they fail to sync") - return controller, informer, nil + return controller, pacemakerInformer, nil } // sync is the main sync function that gets called periodically to check pacemaker status @@ -253,7 +256,7 @@ func (c *HealthCheck) sync(ctx context.Context, syncCtx factory.SyncContext) err c.trackResourceDisruptions() // Log the determined status for visibility - klog.V(2).Infof("Pacemaker health status: %s (errors: %d, warnings: %d)", + klog.V(4).Infof("Pacemaker health status: %s (errors: %d, warnings: %d)", currentStatus.OverallStatus, len(currentStatus.Errors), len(currentStatus.Warnings)) if err := c.updateOperatorStatus(ctx, currentStatus, previousStatus); err != nil { diff --git a/pkg/tnf/pkg/pacemaker/helpers.go b/pkg/tnf/pkg/pacemaker/helpers.go new file mode 100644 index 0000000000..ea36aa115e --- /dev/null +++ b/pkg/tnf/pkg/pacemaker/helpers.go @@ -0,0 +1,74 @@ +package pacemaker + +import ( + "fmt" + + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/serializer" + "k8s.io/client-go/rest" + "k8s.io/client-go/tools/cache" + + pacmkrv1 "github.com/openshift/api/etcd/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +// PacemakerListWatch wraps cache.ListWatch to opt out of WatchList semantics. +// The OCP apiextensions-apiserver does not enable the server-side WatchList gate, +// so the reflector's sendInitialEvents request is rejected. This wrapper tells +// the reflector to skip watchList() and use list+watch directly. +type PacemakerListWatch struct { + cache.ListWatch +} + +func (PacemakerListWatch) IsWatchListSemanticsUnSupported() bool { return true } + +// SanitizeListOptions removes fields not supported by older Kubernetes versions. +// sendInitialEvents and resourceVersionMatch were added in 1.27+ and cause errors on older clusters. +// This helper ensures informer ListWatch functions work across all supported Kubernetes versions. +func SanitizeListOptions(options metav1.ListOptions) metav1.ListOptions { + return metav1.ListOptions{ + LabelSelector: options.LabelSelector, + FieldSelector: options.FieldSelector, + Watch: options.Watch, + AllowWatchBookmarks: options.AllowWatchBookmarks, + ResourceVersion: options.ResourceVersion, + TimeoutSeconds: options.TimeoutSeconds, + Limit: options.Limit, + Continue: options.Continue, + } +} + +// getKubeConfig returns in-cluster Kubernetes REST config. +// No kubeconfig file fallback - use clientcmd package if needed. +func getKubeConfig() (*rest.Config, error) { + config, err := rest.InClusterConfig() + if err != nil { + return nil, fmt.Errorf("failed to get in-cluster config (is this running inside a pod?): %w", err) + } + return config, nil +} + +// CreatePacemakerRESTClient creates REST client for PacemakerStatus CRs. +func CreatePacemakerRESTClient(baseConfig *rest.Config) (rest.Interface, error) { + if baseConfig == nil { + return nil, fmt.Errorf("baseConfig cannot be nil") + } + + scheme := runtime.NewScheme() + if err := pacmkrv1.AddToScheme(scheme); err != nil { + return nil, fmt.Errorf("failed to add PacemakerStatus scheme: %w", err) + } + + pacemakerConfig := rest.CopyConfig(baseConfig) + pacemakerConfig.GroupVersion = &pacmkrv1.SchemeGroupVersion + pacemakerConfig.APIPath = KubernetesAPIPath + pacemakerConfig.NegotiatedSerializer = serializer.NewCodecFactory(scheme) + pacemakerConfig.ContentConfig.ContentType = "application/json" + + restClient, err := rest.RESTClientFor(pacemakerConfig) + if err != nil { + return nil, fmt.Errorf("failed to create REST client for PacemakerStatus: %w", err) + } + + return restClient, nil +} diff --git a/pkg/tnf/pkg/pacemaker/statuscollector.go b/pkg/tnf/pkg/pacemaker/statuscollector.go index bfd6649f75..b822f9a955 100644 --- a/pkg/tnf/pkg/pacemaker/statuscollector.go +++ b/pkg/tnf/pkg/pacemaker/statuscollector.go @@ -788,7 +788,7 @@ func updatePacemakerStatusCR(ctx context.Context, status *pacmkrv1.PacemakerClus return err } - restClient, err := createPacemakerRESTClient(config) + restClient, err := CreatePacemakerRESTClient(config) if err != nil { return err } diff --git a/pkg/tnf/pkg/pcs/auth.go b/pkg/tnf/pkg/pcs/auth.go index 52e1fef1e6..075860dcc7 100644 --- a/pkg/tnf/pkg/pcs/auth.go +++ b/pkg/tnf/pkg/pcs/auth.go @@ -46,7 +46,11 @@ func Authenticate(ctx context.Context, configClient *versioned.Clientset, cfg co return false, fmt.Errorf("failed to accept token: %w", err) } - command = fmt.Sprintf("/usr/sbin/pcs host auth %s addr=%s %s addr=%s --token %s --debug", cfg.NodeName1, cfg.NodeIP1, cfg.NodeName2, cfg.NodeIP2, TokenPath) + if cfg.NodeName2 != "" && cfg.NodeIP2 != "" { + command = fmt.Sprintf("/usr/sbin/pcs host auth %s addr=%s %s addr=%s --token %s --debug", cfg.NodeName1, cfg.NodeIP1, cfg.NodeName2, cfg.NodeIP2, TokenPath) + } else { + command = fmt.Sprintf("/usr/sbin/pcs host auth %s addr=%s --token %s --debug", cfg.NodeName1, cfg.NodeIP1, TokenPath) + } _, _, err = exec.Execute(ctx, command) if err != nil { return false, fmt.Errorf("failed to authenticate node: %w", err) diff --git a/pkg/tnf/pkg/tools/jobs.go b/pkg/tnf/pkg/tools/jobs.go index 8ba2163cea..59d1077bd6 100644 --- a/pkg/tnf/pkg/tools/jobs.go +++ b/pkg/tnf/pkg/tools/jobs.go @@ -142,3 +142,35 @@ func sanitizeDNSLabel(name string) string { name = strings.TrimLeft(name, "-") return name } + +// ToPascalCase converts kebab-case job names to PascalCase for condition names. +// Capitalizes the first letter of each part, keeping the rest lowercase. +// Special cases "tnf" to be all uppercase "TNF" (acronym). +// Examples: +// - "tnf-setup-job" → "TNFSetupJob" +// - "tnf-auth-master-0" → "TNFAuthMaster0" +// - "tnf-update-setup-job" → "TNFUpdateSetupJob" +func ToPascalCase(s string) string { + if s == "" { + return "" + } + + // Split on hyphens + parts := strings.Split(s, "-") + + // Capitalize first letter of each part + for i, part := range parts { + if len(part) == 0 { + continue + } + // Special case: keep "tnf" as uppercase acronym + if strings.ToLower(part) == "tnf" { + parts[i] = "TNF" + } else { + // Capitalize first letter, keep rest as-is (handles numbers like "0") + parts[i] = strings.ToUpper(part[:1]) + part[1:] + } + } + + return strings.Join(parts, "") +} diff --git a/pkg/tnf/pkg/tools/nodes.go b/pkg/tnf/pkg/tools/nodes.go index e1603ff8d0..c44ce4e53c 100644 --- a/pkg/tnf/pkg/tools/nodes.go +++ b/pkg/tnf/pkg/tools/nodes.go @@ -5,6 +5,13 @@ import ( "net" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/labels" + corev1listers "k8s.io/client-go/listers/core/v1" + "k8s.io/client-go/tools/cache" +) + +const ( + ControlPlaneNodeLabelSelector = "node-role.kubernetes.io/control-plane" ) // IsNodeReady checks if a node is in Ready state. @@ -27,7 +34,6 @@ func GetNodeIPForPacemaker(node corev1.Node) (string, error) { return "", fmt.Errorf("node %q has no configured address", node.Name) } - // find internal ip for _, addr := range addresses { switch addr.Type { case corev1.NodeInternalIP: @@ -38,6 +44,37 @@ func GetNodeIPForPacemaker(node corev1.Node) (string, error) { } } - // fallback return addresses[0].Address, nil } + +func GetNodeNames(nodes []*corev1.Node) []string { + names := make([]string, len(nodes)) + for i, node := range nodes { + names[i] = node.Name + } + return names +} + +// StringSlicesEqual checks if two string slices are equal (same order). +func StringSlicesEqual(a, b []string) bool { + if len(a) != len(b) { + return false + } + for i := range a { + if a[i] != b[i] { + return false + } + } + return true +} + +// ListNodesFromInformer returns all nodes from the informer. +// Returns only nodes matching the informer's filter (e.g., controlPlaneNodeInformer). +func ListNodesFromInformer(informer cache.SharedIndexInformer) ([]*corev1.Node, error) { + if informer == nil { + return nil, fmt.Errorf("informer is nil") + } + + lister := corev1listers.NewNodeLister(informer.GetIndexer()) + return lister.List(labels.Everything()) +} diff --git a/pkg/tnf/setup/runner.go b/pkg/tnf/setup/runner.go index 6f90330bc0..1e2ad64153 100644 --- a/pkg/tnf/setup/runner.go +++ b/pkg/tnf/setup/runner.go @@ -18,6 +18,7 @@ import ( "k8s.io/utils/clock" "github.com/openshift/cluster-etcd-operator/pkg/operator" + "github.com/openshift/cluster-etcd-operator/pkg/operator/ceohelpers" "github.com/openshift/cluster-etcd-operator/pkg/tnf/pkg/config" "github.com/openshift/cluster-etcd-operator/pkg/tnf/pkg/etcd" "github.com/openshift/cluster-etcd-operator/pkg/tnf/pkg/jobs" @@ -55,6 +56,17 @@ func RunTnfSetup() error { dynamicInformers.Start(ctx.Done()) dynamicInformers.WaitForCacheSync(ctx.Done()) + // Check if transition already complete (e.g., setup job recreated due to drift detection) + // If complete, nothing to do - update-setup handles all post-transition configuration changes + transitionComplete, err := ceohelpers.HasExternalEtcdCompletedTransition(ctx, operatorClient) + if err != nil { + return fmt.Errorf("failed to check external etcd transition status: %w", err) + } + if transitionComplete { + klog.Info("External etcd transition already complete - setup already ran successfully, nothing to do") + return nil + } + klog.Info("Waiting for completed auth jobs") authDone := func(context.Context) (done bool, err error) { authJobs, err := kubeClient.BatchV1().Jobs("openshift-etcd").List(ctx, metav1.ListOptions{ diff --git a/pkg/tnf/update-setup/runner.go b/pkg/tnf/update-setup/runner.go index ca6e060ad8..ef8bcbfbda 100644 --- a/pkg/tnf/update-setup/runner.go +++ b/pkg/tnf/update-setup/runner.go @@ -2,6 +2,7 @@ package updatesetup import ( "context" + "encoding/xml" "fmt" "os" "strings" @@ -19,6 +20,7 @@ import ( "github.com/openshift/cluster-etcd-operator/pkg/tnf/pkg/config" "github.com/openshift/cluster-etcd-operator/pkg/tnf/pkg/etcd" "github.com/openshift/cluster-etcd-operator/pkg/tnf/pkg/exec" + "github.com/openshift/cluster-etcd-operator/pkg/tnf/pkg/pacemaker" "github.com/openshift/cluster-etcd-operator/pkg/tnf/pkg/pcs" ) @@ -64,8 +66,7 @@ func RunTnfUpdateSetup() error { command := "/usr/sbin/pcs cluster status" _, _, err = exec.Execute(ctx, command) if err != nil { - klog.Infof("Cluster not running (err: %v), skipping update-setup on this node", err) - return nil + return fmt.Errorf("cluster not running on this node, will retry on other node: %w", err) } // Register pacemaker alert agents for fencing taint/untaint. This runs on @@ -183,6 +184,40 @@ func RunTnfUpdateSetup() error { return err } + // Wait for cluster to fully start before validating + klog.Info("Waiting for cluster to stabilize...") + time.Sleep(10 * time.Second) + + // Validate final cluster state: must have exactly 2 nodes + // This ensures we don't succeed if auth hasn't run on the new node yet + // or if node add/remove operations didn't complete correctly + klog.Info("Validating final cluster configuration...") + command = "/usr/sbin/pcs status xml" + stdOut, stdErr, err = exec.Execute(ctx, command) + if err != nil { + klog.Errorf("Failed to query cluster status: %s, stdout: %s, stderr: %s, err: %v", command, stdOut, stdErr, err) + return fmt.Errorf("failed to validate cluster state: %w", err) + } + + var result pacemaker.PacemakerResult + if parseErr := xml.Unmarshal([]byte(stdOut), &result); parseErr != nil { + klog.Errorf("Failed to parse pcs status xml: %v", parseErr) + return fmt.Errorf("failed to parse cluster status: %w", parseErr) + } + + // Count online nodes + onlineNodes := []string{} + for _, node := range result.Nodes.Node { + if node.Online == "true" { + onlineNodes = append(onlineNodes, node.Name) + } + } + + if len(onlineNodes) != 2 { + return fmt.Errorf("invalid cluster state: expected 2 online nodes, found %d: %v (this will retry until auth runs on new node and cluster is complete)", len(onlineNodes), onlineNodes) + } + + klog.Infof("Cluster validation successful: 2 nodes online: %v", onlineNodes) return nil }