diff --git a/docs/plans/control-plane-migration-coverage-plan.md b/docs/plans/control-plane-migration-coverage-plan.md new file mode 100644 index 000000000..dce67944b --- /dev/null +++ b/docs/plans/control-plane-migration-coverage-plan.md @@ -0,0 +1,258 @@ +# Control-plane migration story — coverage verification + +## Context + +User story: as a cluster administrator, migrate control plane nodes from source to +destination failure domains so cluster management runs on the destination vCenter +without losing etcd quorum. Five acceptance criteria (AC1–AC5, listed in the map +below). + +The user states the ControlPlaneMachineSet (CPMS) machinery likely already covers +this story; the task is to **verify coverage, not to implement new code**. The +control plane rollout is implemented as sub-steps inside the existing +`ConditionWorkloadMigrated` handler of the ordered six-condition workflow +(`conditionOrder` in +`internal/controller/vmwarecloudfoundationmigration_controller.go`, lines 68–75): +worker MachineSets are created on the target FDs, then the CPMS is updated in place +to the target failure domains, then the controller waits for the CPMS-driven +rolling rollout to complete, then source workers are drained and deleted. + +Deliverable: a per-AC coverage report — each AC mapped to concrete implementation +anchors (file + symbol) and passing tests, with AC3 (etcd quorum) explicitly +documented as covered by delegation to the upstream OpenShift CPMS operator. +No code changes are expected. A check failing means that AC is NOT covered; see +Assumptions & contingencies for the pre-decided fallback. + +## Approach + +All commands run from the repository root +(`/home/jcallen/Development/vcf-migration-operator`). + +### Step 1 — Run the targeted behavior tests + +These pin the CP rollout behavior. Expect every test to PASS: + +``` +go test ./internal/openshift/ -run 'TestUpdateCPMSFailureDomain|TestCheckControlPlaneRolloutStatus|TestIsCPMSGenerationObserved|TestIsCPMSUpdatedForFailureDomains|TestListControlPlaneMachines' -v + +go test ./internal/controller/ -run 'TestEnsureWorkloadMigratedRolloutAndScaleDown|TestEnsureWorkloadMigratedRolloutGate|TestRolloutLogsMachineLevelDetail' -v +``` + +The second command matches only plain `testing.T` tests; the Ginkgo envtest suite +(`TestControllers` in `internal/controller/suite_test.go`) is excluded by the +`-run` filter and does not need envtest binaries. + +Expected key assertions (these are the behavior under audit): +- `TestUpdateCPMSFailureDomain`: CPMS updated in place to `state: Active` with + `failureDomains.platform: vsphere` and the exact target FD names (3 cases: + nil→target, replace-old→target, multi-FD). +- `TestCheckControlPlaneRolloutStatus`: complete only when + `replicas > 0 && updatedReplicas == replicas && readyReplicas == replicas`. +- `TestEnsureWorkloadMigratedRolloutAndScaleDown`: while CPMS generation unobserved + → requeue 15s with message `"Waiting for control plane rollout to start (CPMS + generation 2/1 observed)"`; mid-rollout → requeue 30s with message `"Control + plane rolling out (1/3 updated, 1/3 ready)"` plus a `ControlPlaneRollout` event; + rollout complete → source worker MachineSet scaled to 0; zero-replica source + MachineSets deleted and `ConditionWorkloadMigrated` set True only after deletion. +- `TestEnsureWorkloadMigratedRolloutGate`: CP rollout path is gated on target + workers being ready AND the CPMS already targeting the target FDs + (`IsCPMSUpdatedForFailureDomains`). + +### Step 2 — Verify AC1 anchors (control plane inputs updated to destination FDs) + +Read and confirm each anchor exists and behaves as described: + +1. `MachineManager.UpdateCPMSFailureDomain` (`internal/openshift/machines.go`, + ~line 230): fetches CPMS named `cluster` in `openshift-machine-api`, sets + `spec.template.openShiftMachineV1Beta1Machine.failureDomains` to + `{platform: configv1.VSpherePlatformType, vsphere: [{Name: }…]}` + and `spec.state = machinev1.ControlPlaneMachineSetStateActive`, then `Update`s + in place (no delete/recreate). Error paths: missing CPMS, missing + `machines_v1beta1_machine_openshift_io` template. +2. Caller: `ensureWorkloadMigrated` + (`internal/controller/vmwarecloudfoundationmigration_controller.go`, ~line + 654–668, "Step 3"): only reached after target workers are ready; guards against + re-updating with `IsCPMSUpdatedForFailureDomains` (idempotent); after the + update it records event `CPMSUpdated` and requeues 15s. +3. Prerequisite — destination FDs resolvable by the CPMS operator: + `ensureMultiSiteConfigured` (443–543; call at ~line 493) calls + `InfrastructureManager.AddTargetVCenter` (`internal/openshift/infrastructure.go`, + ~line 71), which appends the target vCenter entry (server, port 443, + datacenters) and each missing failure domain to + `Infrastructure.spec.platformSpec.vsphere` and persists it. This condition is + ordered before `ConditionWorkloadMigrated` in `conditionOrder`. +4. RBAC: `config/rbac/role.yaml` includes `controlplanemachinesets` under + machine.openshift.io (~line 91; generated from the marker at the controller + file ~line 91 with `get;list;watch;create;update;patch;delete` verbs). Note: + the marker grants `delete` on CPMS, but the operator never issues a CPMS + delete (see Step 4) — the permission is broader than the code paths that use + it, which does not weaken the AC3 invariant. + +Verdict if all confirmed: **AC1 covered** — the operator's "control plane inputs" +are the CPMS spec, updated to the destination failure domains. + +### Step 3 — Verify AC2 + AC4 anchors (replacement CP nodes reach Ready; controlled sequence) + +Read and confirm: + +1. `MachineManager.CheckControlPlaneRolloutStatus` + (`internal/openshift/machines.go`, ~line 267): complete iff + `replicas > 0 && updatedReplicas == replicas && readyReplicas == replicas` — + i.e., all replacement control plane machines are up and Ready (AC2). +2. Wait loop: `ensureWorkloadMigratedRolloutAndScaleDown` (~line 677), "Step 5" + (~line 687–716): waits for `IsCPMSGenerationObserved` first, then polls + `CheckControlPlaneRolloutStatus` every 30s until complete, logging + per-machine detail via `logControlPlaneMachine` (~line 1131). +3. Sequencing (AC4): only after CP rollout is complete does the handler proceed to + scale source worker MachineSets to 0 (Step 6, ~line 718), wait for machine/node + deletion (Step 7, ~line 740), then delete zero-replica source MachineSets + (Step 8, ~line 766). `DeleteMachineSetsByVCenter` + (`internal/openshift/machines.go`, ~line 174) refuses MachineSets with nil or + positive replicas, and rejects an empty vCenter string. +4. The one-at-a-time, quorum-safe *order of control plane machine replacement + itself* is executed by the upstream OpenShift CPMS operator (machine-api), + which the operator triggers via the Step-3 CPMS update; this operator only + writes the desired FDs and waits. + +Verdict if confirmed: **AC2 covered** (operator gates the workflow on +`ReadyReplicas == Replicas`); **AC4 covered by delegation** (rolling replacement +sequence enforced upstream; this operator adds the ordering constraint that source +workers are drained only after CP rollout completes). + +### Step 4 — Verify AC3 invariant (etcd quorum: no destructive CP path in this repo) + +Confirm the operator can never break quorum by verifying it has **no code path +that deletes, scales, or mutates control plane machines or the CPMS spec beyond +the FD/state update**: + +1. Grep `ControlPlaneMachineSets(` across `internal/`: expect exactly two + non-test call sites — the `Get` (machines.go ~line 215) and the single + `Update` (~line 257) — plus one test `Get` (`machines_test.go` ~line 111). + Expect **zero** `Delete` calls on CPMS in code. (The RBAC role grants `delete` + on `controlplanemachinesets` per Step 2.4, but no code path exercises it — + RBAC breadth is not a code path.) +2. Grep for control-plane Machine mutation: the only CP Machine operations are + `ListControlPlaneMachines` (`machines.go` ~line 316, list-only) and + `logControlPlaneMachine` (logging). All `Delete*`/`Scale*` MachineManager + methods operate on `MachineSet` resources (workers) and never on control-plane + Machines. +3. Verify the etcd/quorum claim via behavior anchors rather than literal string + matches: `ensureReady` must call + `OperatorManager.CheckAllOperatorsStable`, and readiness tests should include + `etcd` in unstable/stable operator scenarios (for example in + `internal/controller/ready_test.go` and `internal/openshift/operators_test.go`). + This confirms the operator gates completion on cluster-operator health while + quorum-safe replacement ordering remains delegated to the upstream CPMS + operator. +4. Backstop: `ensureReady` (~line 907) requires **all** ClusterOperators, + including `etcd`, to be Available/not Progressing/not Degraded + (`OperatorManager.CheckAllOperatorsStable`) and only target vCenters present in + Infrastructure before setting `ConditionReady` True. + +Verdict if confirmed: **AC3 covered by delegation** — quorum-safe ordering is the +contract of the upstream CPMS operator; this operator never intervenes in the +rolling replacement and gates completion on the etcd ClusterOperator being +healthy. + +### Step 5 — Verify AC5 anchors (status reflects CP rollout progress in the ordered workflow) + +Read and confirm: + +1. `conditionOrder` (~line 68–75) places `ConditionWorkloadMigrated` 4th of 6 + (after `ConditionInfrastructurePrepared`, `ConditionDestinationInitialized`, + `ConditionMultiSiteConfigured`; before `ConditionSourceCleaned`, + `ConditionReady`); `Reconcile` processes the first non-True condition only. +2. `ConditionWorkloadMigrated` messages track CP rollout progress through the + exact literals asserted by Step 1 tests: + `"Waiting for control plane rollout to start (CPMS generation %d/%d observed)"`, + `"Control plane rolling out (%d/%d updated, %d/%d ready)"`, + `"Workload migrated to target vCenter"`. +3. Events recorded: `CPMSUpdated`, `ControlPlaneRollout` (per 30s poll), + `ControlPlaneRolledOut`, `WorkloadMigrated` (all via `r.Recorder`, visible with + `kubectl describe`). +4. `updateStatus` (~line 1055) persists condition changes with optimistic + concurrency, so progress survives concurrent reconciles. + +Verdict if confirmed: **AC5 covered** — rollout progress is a first-class part of +the ordered workflow's status and events. + +### Step 6 — Run the full suite + +``` +make test +``` + +This runs `manifests generate fmt vet setup-envtest` then the whole unit+ +integration suite (envtest binaries are fetched automatically; e2e excluded). +Expect PASS. If envtest binary download fails in this environment (no network), +fall back to a non-e2e package run: + +``` +KUBEBUILDER_ASSETS="$(bin/setup-envtest use -p path)" go test ./api/... ./cmd/... ./internal/... -v +``` + +and, if that is also impossible, run the Step 1 commands plus +`go test ./internal/openshift/ ./internal/vsphere/ ./internal/metadata/ -v` +and note the limitation in the report. + +### Step 7 — Write the coverage report + +Produce the final report in this exact shape (prose reply, no file needed): + +``` +AC1 control plane inputs → destination FDs: COVERED + impl: + tests: , all passing +AC2 replacement CP nodes Ready on destination: COVERED + impl: + tests: , all passing +AC3 etcd quorum maintained: COVERED (by delegation to upstream CPMS operator) + invariant: +AC4 source CP nodes replaced in controlled sequence: COVERED (by delegation + operator sequencing) + impl: + tests: , all passing +AC5 status reflects CP rollout progress in ordered workflow: COVERED + impl: + tests: , all passing +Full suite: make test → PASS (or fallback note) +``` + +## Critical files & anchors + +- `internal/controller/vmwarecloudfoundationmigration_controller.go` — `conditionOrder` (68–75), `ensureMultiSiteConfigured` (443–543), `ensureWorkloadMigrated` (549–669, CPMS update at 654–668), `ensureWorkloadMigratedRolloutAndScaleDown` (677–793), `ensureReady` (907–970), `logControlPlaneMachine` (1131–1154). +- `internal/openshift/machines.go` — `UpdateCPMSFailureDomain` (230–263), `CheckControlPlaneRolloutStatus` (267–283), `IsCPMSUpdatedForFailureDomains` (288–312), `ListControlPlaneMachines` (316–329), `IsCPMSGenerationObserved` (334–343), `DeleteMachineSetsByVCenter` (174–210). +- `internal/openshift/infrastructure.go` — `AddTargetVCenter` (71–134). +- `internal/controller/workload_migration_rollout_test.go` — behavior tests + fixtures `newCPMSForRollout` (443), `newCPMSUpdatedForRollout` (486). +- `internal/openshift/machines_test.go` — `TestUpdateCPMSFailureDomain` (55), `TestCheckControlPlaneRolloutStatus` (142), `TestIsCPMSGenerationObserved` (209), `TestIsCPMSUpdatedForFailureDomains` (451), `TestListControlPlaneMachines` (549). + +## Verification + +- Step 1 both commands: every listed test passes (`ok` + `--- PASS` per test). +- Step 6: `make test` → `ok` for every package. +- Step 4 greps return exactly the call sites enumerated (no `Delete` on + `ControlPlaneMachineSets`, no CP Machine mutation). +- Report (Step 7) contains all five ACs with verdicts and anchors; any AC whose + checks failed is reported as NOT COVERED with the failing evidence. + +## Assumptions & contingencies + +- **AC3 standard (user-overridable):** "etcd quorum is maintained" is verified as + delegation to the upstream OpenShift CPMS operator (one-at-a-time rolling + replacement, source machine removed only after the replacement joins etcd), + plus this operator's no-destructive-CP-path invariant and the etcd + ClusterOperator health gate in `ensureReady`. Explicit in-operator quorum + monitoring (e.g., watching etcd member counts) would be a new feature and is + out of scope unless the user requests it. +- **CP rollout lives inside `ConditionWorkloadMigrated`** rather than a dedicated + `ConditionControlPlaneMigrated`; AC5's "as part of the ordered workflow" is + satisfied by the condition message/event progress tracking. A dedicated + condition would be a design change, out of scope for a coverage check. +- **If any Step 1–6 check fails** (test failure, missing anchor, unexpected grep + hit): that AC is NOT covered. Then implement or fix the minimum code to match + the behavior described in this plan's Steps 2–5 (the described behavior is the + intended contract; e.g., a missing event literal is restored to the exact + string the tests assert), add/repair the failing test to the existing table + style in the cited test files, and re-run Step 1 + Step 6. Do not redesign + beyond restoring the described behavior. +- Line numbers are hints from the current tree; re-read around the cited symbols + before relying on them. diff --git a/docs/plans/cpms-rollout-logging-plan.md b/docs/plans/cpms-rollout-logging-plan.md new file mode 100644 index 000000000..8b77eb7ea --- /dev/null +++ b/docs/plans/cpms-rollout-logging-plan.md @@ -0,0 +1,429 @@ +# SPLAT-2887: CPMS rollout logging and condition messages + +## Context + +SPLAT-2887 (Story, epic SPLAT-2644) improves logging and condition reporting for the +Control Plane Machine Set (CPMS) rollout phase of the vcf-migration-operator. Today the +`WorkloadMigrated` condition shows the rollout as +`reason=Progressing message=CPMS updated, control plane rolling out (4/4 ready)` and the +ticket requires: + +1. Structured log entries with machine-level detail during the CPMS rollout (individual + machine replacement status, phase transitions). +2. Condition messages that clearly distinguish: waiting for rollout to start, rollout in + progress, rollout completing. +3. Timestamp/duration information to diagnose slow rollouts. +4. Enough detail to diagnose a stalled rollout without extra debugging. + +End state: the two rollout-phase condition messages above are replaced with distinct +phase messages (exact strings below), machine-level detail is logged each rollout +reconcile, and `ensureWorkloadMigrated` routes to the rollout path from **cluster state** +instead of the current fragile condition-message string matching +(`vmwarecloudfoundationmigration_controller.go:501-507`, which would silently break +progression if the messages were merely reworded). No API, CRD, RBAC, or reason-enum +changes: machines `get;list;watch` RBAC already exists (controller line 86). + +## Approach + +Step 1 adds two MachineManager helpers (pure additions). Step 2 changes the controller +phase reporting and gate (includes one signature change; all callsites updated in the +same step). Step 3 adds controller tests. The tree compiles and existing tests pass after +each step. + +### Step 1 — New MachineManager helpers + +File: `internal/openshift/machines.go` (place next to `IsCPMSGenerationObserved`). + +1. Add: + +```go +// IsCPMSUpdatedForFailureDomains reports whether the ControlPlaneMachineSet's spec +// already targets the given failure domain names with state Active. It detects that +// the CPMS update step of workload migration has completed. +func (m *MachineManager) IsCPMSUpdatedForFailureDomains(ctx context.Context, failureDomainNames []string) (bool, error) +``` + +Body: `cpms, err := m.GetControlPlaneMachineSet(ctx)`; on err `return false, err`. +If `cpms.Spec.State != machinev1.ControlPlaneMachineSetStateActive` → `return false, nil`. +`tmpl := cpms.Spec.Template.OpenShiftMachineV1Beta1Machine`; if `tmpl == nil || +tmpl.FailureDomains == nil` → `return false, nil`. Collect +`tmpl.FailureDomains.VSphere[i].Name` into `current []string`. Return true iff `current` +and `failureDomainNames` are equal ignoring order (copy both, `sort.Strings`, compare +with `reflect.DeepEqual`). Imports needed in machines.go: `reflect`, `sort` (check +existing imports first; machinev1 already imported as `machinev1`). + +2. Add: + +```go +// ListControlPlaneMachines lists Machines in openshift-machine-api carrying a +// control-plane role label (master or control-plane). +func (m *MachineManager) ListControlPlaneMachines(ctx context.Context) ([]*machinev1beta1.Machine, error) +``` + +Body: `m.machineClient.MachineV1beta1().Machines(MachineAPINamespace).List(ctx, +metav1.ListOptions{LabelSelector: "machine.openshift.io/cluster-api-machine-role in +(master,control-plane)"})`; wrap error `fmt.Errorf("listing control plane machines: %w", +err)`; return `&machines.Items[i]` pointers (same pattern as `CheckMachinesReady`). + +3. Unit tests in `internal/openshift/machines_test.go`, table-driven like +`TestCheckControlPlaneRolloutStatus` (fake clientset via `fakemachineclient.NewClientset(...)`, +manager via `NewMachineManager(fakekube.NewClientset(), machineClient, nil)`): + +- `TestIsCPMSUpdatedForFailureDomains` — build CPMS with the existing `newTestCPMS(state, + fds)` helper; build `fds` as `&machinev1.FailureDomains{Platform: configv1.VSpherePlatformType, + VSphere: []machinev1.VSphereFailureDomain{{Name: ...}}}`. Cases: + - Active, FDs [a,b], want [a,b] → true + - Active, FDs [a,b], want [b,a] → true (order-independent) + - Active, FDs [a], want [b] → false + - Inactive, FDs [a], want [a] → false + - Active, nil FailureDomains, want [a] → false + - no CPMS in clientset → error containing `getting ControlPlaneMachineSet` +- `TestListControlPlaneMachines` — seed four Machines in the fake clientset: `cp-1` with + label `machine.openshift.io/cluster-api-machine-role: master`, `cp-2` with `control-plane`, + `worker-1` with `worker`, `unlabeled` with no labels. Assert returned set is exactly + {cp-1, cp-2}. + +After this step: `go test ./internal/openshift/ -run 'TestIsCPMSUpdatedForFailureDomains|TestListControlPlaneMachines' -v` +passes and full `go test ./internal/openshift/` still passes. + +### Step 2 — Controller phase reporting and cluster-state gate + +File: `internal/controller/vmwarecloudfoundationmigration_controller.go` unless noted. +All edits below are in this one step (they share the signature change); update every +callsite before moving on so the tree compiles at the end. + +1. **Signature change** — `internal/openshift/machines.go`, `IsCPMSGenerationObserved` + (multi-value returns match the existing `CheckControlPlaneRolloutStatus` style): + +```go +// IsCPMSGenerationObserved checks whether the ControlPlaneMachineSet's observed +// generation matches its metadata generation, indicating the controller has processed +// the latest spec change. It also returns both generation values for reporting. +func (m *MachineManager) IsCPMSGenerationObserved(ctx context.Context) (observed bool, generation, observedGeneration int64, err error) +``` + +Body: after the Get, set `generation = cpms.Generation`, +`observedGeneration = cpms.Status.ObservedGeneration`, return +`generation == observedGeneration, generation, observedGeneration, nil`. + +Callsites (exactly two, grep `IsCPMSGenerationObserved`): +- `internal/openshift/machines_test.go:238` (`TestIsCPMSGenerationObserved`): update the + call to `got, gen, obsGen, err := ...`; assert `got == tt.want` and that `gen`/`obsGen` + equal the fixture values. +- controller line 619: rewritten in substep 3 below. + +2. **Replace the message-string gate** in `ensureWorkloadMigrated` (lines 501–507). + Delete the whole block: + +```go + // If we are past Step 3 (CPMS updated), run Steps 4–6 (rollout and scale-down) from cluster state. + if c := apimeta.FindStatusCondition(migration.Status.Conditions, condType); c != nil { + pastCPMSUpdate := strings.HasPrefix(c.Message, "CPMS updated") || strings.Contains(c.Message, "Control plane rollout") || strings.Contains(c.Message, "Old workers") + if pastCPMSUpdate { + return r.ensureWorkloadMigratedRolloutAndScaleDown(ctx, migration) + } + } +``` + + Then, after `machineMgr := openshift.NewMachineManager(...)` (line 520), hoist + `targetFDNames := failureDomainNames(migration.Spec.FailureDomains)` (it is currently + computed at line 593 inside Step 3 — remove that local; keep the variable name), and + insert the new gate directly after the Step-1 existence loop (after the loop that + sets `allTargetMSExist`, before `if !allTargetMSExist {`): + +```go + // Once the target worker MachineSets exist and the CPMS already targets the target + // failure domains, the CPMS update step is done: continue from the rollout and + // scale-down path, derived entirely from cluster state. + cpmsUpdated, err := machineMgr.IsCPMSUpdatedForFailureDomains(ctx, targetFDNames) + if err != nil { + return ctrl.Result{}, fmt.Errorf("checking CPMS update state: %w", err) + } + if allTargetMSExist && cpmsUpdated { + return r.ensureWorkloadMigratedRolloutAndScaleDown(ctx, migration) + } +``` + + `err` is already in scope (line 511); `:=` is valid because `cpmsUpdated` is new. + The `strings` and `apimeta` imports remain (used elsewhere in the file). + +3. **Step 3 message** (line ~589-599). After `UpdateCPMSFailureDomain` succeeds, fetch + the generations and set the new waiting message (keep the existing `CPMSUpdated` + event unchanged): + +```go + _, generation, observedGeneration, err := machineMgr.IsCPMSGenerationObserved(ctx) + if err != nil { + return ctrl.Result{}, fmt.Errorf("checking CPMS generation: %w", err) + } + r.setCondition(migration, condType, metav1.ConditionFalse, migrationv1alpha1.ReasonProgressing, + fmt.Sprintf("Waiting for control plane rollout to start (CPMS generation %d/%d observed)", generation, observedGeneration)) +``` + +4. **Phase A branch** in `ensureWorkloadMigratedRolloutAndScaleDown` (lines 618–627), + the `if !observed` case: + +```go + observed, generation, observedGeneration, err := machineMgr.IsCPMSGenerationObserved(ctx) + if err != nil { + return ctrl.Result{}, fmt.Errorf("checking CPMS generation: %w", err) + } + if !observed { + log.V(1).Info("CPMS generation not yet observed", "generation", generation, "observedGeneration", observedGeneration) + r.setCondition(migration, condType, metav1.ConditionFalse, migrationv1alpha1.ReasonProgressing, + fmt.Sprintf("Waiting for control plane rollout to start (CPMS generation %d/%d observed)", generation, observedGeneration)) + r.Recorder.Eventf(migration, "Normal", "ControlPlaneRollout", "waiting for rollout to start (CPMS generation %d/%d observed)", generation, observedGeneration) + return ctrl.Result{RequeueAfter: 15 * time.Second}, nil + } +``` + +5. **Phase B branch** (lines 628–637), the `if !complete` case — updated counts in the + message, a progress event, and machine-level detail: + +```go + if !complete { + log.V(1).Info("control plane rollout in progress", "replicas", replicas, "updated", updated, "ready", ready) + r.setCondition(migration, condType, metav1.ConditionFalse, migrationv1alpha1.ReasonProgressing, + fmt.Sprintf("Control plane rolling out (%d/%d updated, %d/%d ready)", updated, replicas, ready, replicas)) + r.Recorder.Eventf(migration, "Normal", "ControlPlaneRollout", "control plane rolling out (%d/%d updated, %d/%d ready)", updated, replicas, ready, replicas) + if machines, merr := machineMgr.ListControlPlaneMachines(ctx); merr != nil { + log.V(2).Info("listing control plane machines failed", "err", merr) + } else { + for _, machine := range machines { + logControlPlaneMachine(log, machine) + } + } + return ctrl.Result{RequeueAfter: 30 * time.Second}, nil + } +``` + + A machine-list failure is logged and skipped (not returned), per the codebase + non-critical-error convention — the rollout counts are already captured from the + CPMS status. + +6. **Machine-level log helper** — add near `checkWorkerReadiness` at the end of the + controller file: + +```go +// logControlPlaneMachine logs the status of a single control plane Machine so that +// rollout progress and stalled machines can be diagnosed from operator logs. +func logControlPlaneMachine(log klog.Logger, machine *machinev1beta1.Machine) { + phase := "" + if machine.Status.Phase != nil { + phase = *machine.Status.Phase + } + kv := []interface{}{ + "machine", machine.Name, + "phase", phase, + "age", time.Since(machine.CreationTimestamp.Time).Round(time.Second), + } + if machine.Status.LastUpdated != nil { + kv = append(kv, "lastUpdated", machine.Status.LastUpdated.Time) + } + if machine.Status.ErrorReason != nil { + kv = append(kv, "errorReason", string(*machine.Status.ErrorReason)) + } + if machine.Status.ErrorMessage != nil { + kv = append(kv, "errorMessage", *machine.Status.ErrorMessage) + } + log.V(1).Info("control plane machine status", kv...) +} +``` + + Add `machinev1beta1 "github.com/openshift/api/machine/v1beta1"` to the controller's + third-party import group (alphabetical, between `configclient` and `machineclient` + lines — place after `configclient`... actual order: `configv1`, + `configclient`, `machinev1beta1`, `machineclient` by package path). + Duration support (ticket item 3): `age` is the machine's age since creation and + `lastUpdated` is the machine-controller's last observation time; klog line + timestamps plus these two fields cover slow/stalled diagnosis without stored state. + +7. **Doc comment** for `ensureWorkloadMigratedRolloutAndScaleDown` (lines 602–607): + replace the sentence "Call when condition message indicates we are past \"CPMS + updated\" (e.g. \"Control plane rollout\" or \"Old workers\" or we have observed + generation and rollout complete)." with "Called from ensureWorkloadMigrated when the + target worker MachineSets exist and the CPMS already targets the target failure + domains." + +8. **Existing test update** — `internal/controller/workload_migration_rollout_test.go`, + case "requeues while CPMS generation is not observed" (lines 50–52): change the + assertion from `strings.Contains(cond.Message, "generation observed")` to an exact + match `cond.Message != "Waiting for control plane rollout to start (CPMS generation + 2/1 observed)"` (fixture `newCPMSForRollout(false, true)` has Generation=2, + ObservedGeneration=1). + +Do not change the other condition messages ("Creating new worker MachineSets", +"Workers created, waiting for machines ready", "Old workers scaled down, waiting for +deletion", "Deleting source MachineSets", "Workload migrated to target vCenter") — the +ticket scope is the CPMS rollout phase. + +After this step: `go build ./...` and `go test ./internal/...` pass (the one existing +rollout test case is updated in substep 8). + +### Step 3 — New controller tests + +File: `internal/controller/workload_migration_rollout_test.go` (add; reuse existing +fixtures and harness pattern). + +1. Extend fixture `newInfrastructureForRollout` to also set + `Status: configv1.InfrastructureStatus{InfrastructureName: "test-infra"}` (required by + `GetInfrastructureID`; harmless to existing cases, which never read it). + +2. Add fixture `newCPMSUpdatedForRollout(fdNames []string, observed bool) *machinev1.ControlPlaneMachineSet`: + Name "cluster", Namespace `openshift.MachineAPINamespace`, Generation 2, + `Spec.State: machinev1.ControlPlaneMachineSetStateActive`, + `Spec.Replicas: &three`, + `Spec.Template.OpenShiftMachineV1Beta1Machine.FailureDomains: &machinev1.FailureDomains{Platform: configv1.VSpherePlatformType, VSphere: }`, + Status Replicas/UpdatedReplicas/ReadyReplicas all 3, + `Status.ObservedGeneration` = 2 if observed else 1. + +3. New case in `TestEnsureWorkloadMigratedRolloutAndScaleDown`: + +``` +name: "reports progress while control plane is rolling out" +objects: [newInfrastructureForRollout("source.example.com"), newCPMSForRollout(true, false)] +wantRequeue: 30 * time.Second +``` + + `newCPMSForRollout(true, false)` gives replicas=3, updated=1, ready=1, observed. + Assertions: `cond.Status == metav1.ConditionFalse`, + `cond.Message == "Control plane rolling out (1/3 updated, 1/3 ready)"`, and the last + entry of `resultReconciler.Recorder.(*record.FakeRecorder).Events` contains + `control plane rolling out (1/3 updated, 1/3 ready)`. + +4. New test `TestEnsureWorkloadMigratedRolloutGate` — table-driven, calls + `reconciler.ensureWorkloadMigrated(ctx, migration)`. Harness mirrors the existing + loop (fakekube with empty `&corev1.NodeList{}`, configfake with the infra object, + fakemachineclient with the rest, `record.NewFakeRecorder(20)`). Migration fixture + (built inline per case): + +```go +migration := &migrationv1alpha1.VmwareCloudFoundationMigration{ + ObjectMeta: metav1.ObjectMeta{Name: migrationv1alpha1.SingletonName, Generation: 1}, + Spec: migrationv1alpha1.VmwareCloudFoundationMigrationSpec{ + FailureDomains: []configv1.VSpherePlatformFailureDomainSpec{{ + Name: "target-fd-1", + Server: "target.example.com", + Topology: configv1.VSpherePlatformFailureDomainTopologySpec{ + Template: "/dc1/vm/target-template", + Datacenter: "dc1", + Datastore: "ds1", + ResourcePool: "rp1", + Cluster: "cl1", + ComputeCluster: "cl1", + }, + }}, + }, +} +``` + + (The target worker MachineSet name is `workerMachineSetName("test-infra", + "target-fd-1")` = `test-infra-worker-target-fd-1`.) + + Cases: + - "routes to rollout path when CPMS targets failure domains and workers exist": + objects = infra, `newSourceMachineSetForRollout("test-infra-worker-target-fd-1", + "target.example.com", 1)`, `newCPMSUpdatedForRollout([]string{"target-fd-1"}, false)`, + `newSourceMachineSetForRollout("source-worker-a", "source.example.com", 1)`. + Expect requeue 15s and + `cond.Message == "Waiting for control plane rollout to start (CPMS generation 2/1 observed)"`. + (Under the old code this scenario would produce "Workers created, waiting for + machines ready" because the fake NodeList is empty — so the exact message proves + routing and that Step 3 is not re-run.) + - "stays in worker phase when CPMS not updated": same objects but + `newCPMSUpdatedForRollout([]string{"source-fd-1"}, false)`. Expect requeue 30s and + `cond.Message == "Workers created, waiting for machines ready"`. + - "stays in worker phase when target machinesets missing": objects = infra, + `newCPMSUpdatedForRollout([]string{"target-fd-1"}, false)`, + `newSourceMachineSetForRollout("source-worker-a", "source.example.com", 2)` (no + target MS). Expect requeue 30s, + `cond.Message == "Workers created, waiting for machines ready"`, and + `GetMachineSet("test-infra-worker-target-fd-1")` to succeed (the create path ran). + +5. New test `TestRolloutLogsMachineLevelDetail` — captures klog output to prove + machine-level structured entries (ticket AC 1): + +```go + var buf bytes.Buffer + klog.SetOutput(&buf) + defer klog.SetOutput(nil) +``` + + Objects: infra, `newCPMSUpdatedForRollout([]string{"target-fd-1"}, true)` with + `Status.UpdatedReplicas = 1; Status.ReadyReplicas = 1` (observed, not complete → + Phase B branch), and two Machines in the fake clientset: + - `cp-1`: label role `master`, `Status.Phase = pointer to "Running"`, NodeRef set. + - `cp-2`: label role `master`, `Status.Phase = pointer to "Provisioning"`, + `Status.ErrorReason = pointer to machinev1beta1.CreateMachineError`, + `Status.ErrorMessage = pointer to "vm creation timed out"`, + `CreationTimestamp` = now minus 2 minutes, `Status.LastUpdated` = now minus 1 minute. + + Call `ensureWorkloadMigratedRolloutAndScaleDown`, `klog.Flush()`, then assert + `buf.String()` contains each of: `control plane machine status`, `cp-1`, `Running`, + `cp-2`, `Provisioning`, `CreateError`, `vm creation timed out`. Also assert requeue + 30s. Tests in this package do not run in parallel (no `t.Parallel`), so the global + klog output swap is safe. If the vendored klog rejects `SetOutput(nil)`, use + `io.Discard` in the defer instead. + +After this step: `go test ./internal/controller/ -run 'TestEnsureWorkloadMigrated' -v` +and `go test ./internal/...` pass. + +## Critical files & anchors + +- `internal/controller/vmwarecloudfoundationmigration_controller.go` — the load-bearing + file: gate at lines 501–507 (delete), Step 3 at 589–599, rollout function at + 608–714 (Phase A 618–627, Phase B 628–637), helper near `checkWorkerReadiness` (976). +- `internal/openshift/machines.go` — new methods next to `IsCPMSGenerationObserved` + (286); signature change there. +- `internal/controller/workload_migration_rollout_test.go` — fixture + `newCPMSForRollout` (209) shows the CPMS shape; harness loop (127–179) to mirror for + the gate test. +- `internal/openshift/machines_test.go` — `newTestCPMS` (22) and + `TestCheckControlPlaneRolloutStatus` (142) are the style/fixtures to copy. +- `vendor/github.com/openshift/api/machine/v1beta1/types_machine.go` — `MachineStatus` + (333): `Phase *string` (Failed/Provisioning/Provisioned/Running/Deleting), + `ErrorReason *MachineStatusError`, `ErrorMessage *string`, `LastUpdated *metav1.Time`. + +## Verification + +Working directory: repo root. Run after each step as noted; final gate: + +1. `go build ./...` +2. `go test ./internal/openshift/ -run 'TestIsCPMSUpdatedForFailureDomains|TestListControlPlaneMachines|TestIsCPMSGenerationObserved' -v` +3. `go test ./internal/controller/ -run 'TestEnsureWorkloadMigrated' -v` — covers the + updated message case, new Phase-B progress case, both gate-routing cases, and the + log-capture test. +4. `KUBEBUILDER_ASSETS="$(bin/setup-envtest use -p path)" make test` — full unit + + envtest suite. +5. `make lint` (golangci-lint v2; formatters gofmt/goimports included). + +Concrete new-behavior checks (each encoded as a test above): +- CPMS observed with replicas=3, updated=1, ready=1 → condition message exactly + `Control plane rolling out (1/3 updated, 1/3 ready)`, requeue 30s, and a + `Normal/ControlPlaneRollout` event with the same counts (previously the message was + `CPMS updated, control plane rolling out (1/3 ready)` with no updated count). +- CPMS generation 2 / observed 1 → condition message exactly `Waiting for control plane + rollout to start (CPMS generation 2/1 observed)`, requeue 15s. +- CPMS Active targeting `target-fd-1` + target worker MachineSet present → + `ensureWorkloadMigrated` routes to the rollout path (message above) instead of + re-running worker creation; CPMS targeting a different name or missing target MS → + stays in the worker phase. +- Operator log at V(1) contains one `control plane machine status` entry per control + plane Machine with `machine`, `phase`, `age`, and (when set) `lastUpdated`, + `errorReason`, `errorMessage` — asserted via klog output capture. + +## Assumptions & contingencies + +- Worker-creation and scale-down phase messages are left unchanged; the ticket targets + the CPMS rollout phase. If the reviewer wants them reworded too, that is a follow-up. +- Control plane machines carry role label `master` or `control-plane`; the selector + matches both (this codebase's CPMS template uses `master`). +- The gate requires `allTargetMSExist && cpmsUpdated`. If a future migration reuses + source failure-domain names verbatim (so CPMS FDs already match before Step 3), the + rollout path runs early — same degenerate behavior the old message gate had after the + first "CPMS updated" message; acceptable, noted here so it is a conscious choice. +- If `newInfrastructureForRollout`'s added `Status.InfrastructureName` ever breaks an + existing case (it shouldn't — the rollout path never reads it), revert that fixture + and build the Infrastructure inline in `TestEnsureWorkloadMigratedRolloutGate` instead. +- Save this plan document to `docs/plans/cpms-rollout-logging-plan.md` (user-requested + location) as part of execution. diff --git a/docs/vcenter-privileges.md b/docs/vcenter-privileges.md new file mode 100644 index 000000000..c8b81d2f1 --- /dev/null +++ b/docs/vcenter-privileges.md @@ -0,0 +1,75 @@ +# vCenter Privilege Requirements + +Privilege set required to run this operator against a vCenter, derived from the +operator's actual vSphere API calls and the VMware vSphere SDK ReferenceGuide +(`vsphere-ws/docs/ReferenceGuide`, vim25 SOAP API). + +## How it was derived + +Each govmomi call in the operator was traced to the underlying SDK API method, +then mapped to the "Required Privileges" section of that method's ReferenceGuide +page (or, for property access, the per-property privilege on the object page). + +### SOAP (vim25) calls + +| Code path | SDK API call | Privilege per ReferenceGuide | +|---|---|---| +| `internal/vsphere/session.go:92`, `internal/vsphere/list.go:31` — client init | `ServiceInstance.RetrieveServiceContent` | `System.Anonymous` (none) | +| `internal/vsphere/session.go:102`, `internal/vsphere/list.go:40` — login | `SessionManager.Login` | `System.Anonymous` (none) | +| `internal/vsphere/session.go:131-143`, `internal/vsphere/list.go:43` — logout | `SessionManager.Logout` | **`System.View`** | +| `internal/vsphere/session.go:108`, `internal/vsphere/list.go:46`, `internal/controller/preflight.go:189-218`, `internal/controller/vmwarecloudfoundationmigration_controller.go:305-311` — all `Finder.*` lookups (datacenter, cluster, datastore, network, resource pool, folder, template) | `PropertyCollector.RetrievePropertiesEx` (method: `System.Anonymous`; per-property access enforced) | **`System.View`** on traversed objects (`vim.ManagedEntity.name` / `parent` are documented as `System.View`) | +| `internal/vsphere/folder.go:31`, `internal/controller/preflight.go:378` — `dc.Folders()` reads `Datacenter.configInfo` | `RetrievePropertiesEx` property access | **`System.View`** | +| `internal/vsphere/folder.go:99` — `task.Wait` reads task `info` | `RetrievePropertiesEx` property access | **`System.View`** | +| `internal/controller/preflight.go:317` — `UserSession` reads `SessionManager.currentSession` | property access | `System.Anonymous` (none; `vim.SessionManager.html` property table) | +| `internal/controller/preflight.go:411` — privilege preflight | `AuthorizationManager.HasUserPrivilegeOnEntities` | method: `None`; `entities` param: **`System.View`** on root folder, vm folder, datacenter, cluster | +| `internal/vsphere/folder.go:47` — `CreateVMFolder` | `Folder.CreateFolder` | **`Folder.Create`** on the parent folder | +| `internal/vsphere/folder.go:94` — `DeleteVMFolder` | `ManagedEntity.Destroy_Task` | **`Folder.Delete`** when the object is a Folder | + +### REST (vapi tag API) calls + +| Code path | HTTP endpoint | Privilege | +|---|---|---| +| `internal/vsphere/session.go:115` — REST login | SAML exchange | none (auth) | +| `internal/vsphere/tags.go:138,194,287` — `GetCategory` / `GetTagForCategory` | `GET /api/v2/category[/{id}]`, `GET /api/v2/tag?category-id=` | **`InventoryService.Tagging.Read`** | +| `internal/vsphere/tags.go:151` — `ListTagsForCategory` | `GET /api/v2/tag` | **`InventoryService.Tagging.Read`** | +| `internal/vsphere/tags.go:160` — `ListAttachedTags` | `POST /api/v2/category/{id}/action/list-attached-tags` | **`InventoryService.Tagging.Read`** | +| `internal/vsphere/tags.go:210` — `CreateCategory` | `POST /api/v2/category` | **`InventoryService.Tagging.CreateCategory`** (root folder) | +| `internal/vsphere/tags.go:299` — `CreateTag` | `POST /api/v2/tag` | **`InventoryService.Tagging.CreateTag`** (root folder) | +| `internal/vsphere/tags.go:333` — `AttachTag` | `POST /api/v2/category/{id}/action/attach` | **`InventoryService.Tagging.AttachTag`** (root folder) + **`InventoryService.Tagging.ObjectAttachable`** on the target object (vSphere ≥ 7.0.3) | + +## Required privilege set (target vCenter) + +| Privilege | Scope | Why | +|---|---|---| +| `System.View` | root folder | every inventory lookup (finder), `Datacenter.configInfo` read, task wait, `Logout`, `HasUserPrivilegeOnEntities` entities param | +| `Folder.Create` | datacenter's VM folder | `CreateVMFolder` (nested parts need it on each parent created) | +| `Folder.Delete` | VM folders the operator creates | `DeleteVMFolder` — **currently dead code in the controller path** (only exercised by tests), so optional until cleanup lands | +| `InventoryService.Tagging.Read` | root folder | category/tag/attachment reads happen on *every* reconcile (`ObjectHasTagInCategory`, `EnsureTagCategory`, `EnsureTag`) | +| `InventoryService.Tagging.CreateCategory` | root folder | `EnsureTagCategory` | +| `InventoryService.Tagging.CreateTag` | root folder | `EnsureTag` | +| `InventoryService.Tagging.AttachTag` | root folder | `AttachTag` | +| `InventoryService.Tagging.ObjectAttachable` | the specific datacenter + cluster (+ folder) | tag attachment to those objects on vSphere 7.0.3+ | + +**Source vCenter:** read-only — `System.View` (datacenter existence check only, +`preflight.go:149`; no mutations). + +## Gaps and notes + +1. **Preflight under-checks the real requirement set** (`preflight.go:45-57`). It + verifies the tag privileges + `ObjectAttachable` + `Folder.Create`, but never + checks `System.View` (needed for every finder call and even `Logout`) nor + `InventoryService.Tagging.Read` (used unconditionally). A user with only the + preflight-checked set would pass preflight, then fail on first reconcile. +2. `Folder.Delete` is required only once folder cleanup is actually wired in; the + controller never calls `DeleteVMFolder`. +3. Sourcing: the ReferenceGuide is SOAP-only — tag REST privileges are not + documented there (only `InventoryService.Tagging.AttachTag` appears, in + `vim.vslm.vcenter.VStorageObjectManager.html`). The tag privilege IDs above + match the operator's own preflight constants (`preflight.go:46-56`) plus + VMware's REST tag API privilege names; `AttachTag` on root folder is the one + grounded in this doc set. +4. Out of scope for "running the operator": the vSphere creds secret the operator + writes into the *destination* cluster is consumed by that cluster's + machine-api/cloud-controller, which needs the full VM-lifecycle privilege set + (`VirtualMachine.*`, `Host.*`, etc.) — a different account requirement than the + operator's own.