diff --git a/src/clis/nvcf-cli/README.md b/src/clis/nvcf-cli/README.md index 7d043b4c0..d32769998 100644 --- a/src/clis/nvcf-cli/README.md +++ b/src/clis/nvcf-cli/README.md @@ -1730,14 +1730,29 @@ cluster identity and the system and requests namespaces. ### How drain works -`cordon-and-drain` adds the `CordonAndDrainMaintenance` feature flag and sets -`maintenanceMode: CordonAndDrain` on the NVCA `agent-config` ConfigMap, then -restarts the NVCA deployment so the change takes effect. `uncordon` reverses -both. The command returns once NVCA has been told to drain and (unless `--force`) -the restart has rolled out; it does not wait for every instance to reach zero. -Watch progress with `cluster agent list-functions --phase DRAINING`. `--timeout` -bounds the rollout wait (default 5m); a timeout is reported as a warning because -the config change is already persisted and re-running is a no-op. +`cordon-and-drain` adds the `CordonAndDrainMaintenance` feature flag to the +`NVCFBackend` CR's `spec.overrides.featureGate.values`. `uncordon` removes it. +The CLI never edits the NVCA `agent-config` ConfigMap or restarts the NVCA +deployment directly: the NVCA operator treats `agent-config` as fully +generated from the CR and reverts any direct edit on its next reconcile, so +the CLI's job is only to submit the desired state and let the operator's own +reconcile regenerate `agent-config` and roll NVCA out. The command returns +once the CR update is accepted and (unless `--force` or `--timeout 0`) the +operator's rollout has completed; it does not wait for every instance to +reach zero. Watch progress with `cluster agent list-functions --phase +DRAINING`. `--timeout` bounds the wait for the operator's rollout (default +5m); `--force` or `--timeout 0` skip the wait entirely and return right after +the CR update, leaving the operator's reconciliation to finish +asynchronously. A timeout is reported as a warning because the CR change is +already persisted and re-running is a no-op. + +`--force` only affects a run that changes the NVCFBackend CR; it has no +effect when the CR is already in the requested state. In an earlier version +of this command, `--force` also retriggered the NVCA restart directly, so it +could be used to kick a stuck rollout even without a state change. The CLI no +longer performs that restart; the NVCA operator's own reconcile owns it, so +there is nothing left for `--force` to retrigger once the CR already matches +the desired state. ### How kill works @@ -1762,9 +1777,10 @@ All maintenance commands accept `--dry-run` to preview without mutating, and name matches (guards against a wrong `--compute-plane-context`). `kill-function` and `kill-all` accept `--reason` for an audit note, and `--json` for automation. -These commands need write access to the target cluster: get/update on the -`agent-config` ConfigMap and the `nvca` Deployment for drain, and list/delete -(and update, with `--force`) on `ICMSRequest` CRs for kill. +These commands need write access to the target cluster: list/update on the +`NVCFBackend` CR for drain (plus read access to the `agent-config` ConfigMap +and the `nvca` Deployment, to wait for the NVCA operator's rollout), and +list/delete (and update, with `--force`) on `ICMSRequest` CRs for kill. ### Examples diff --git a/src/clis/nvcf-cli/cmd/cluster_agent_maintenance.go b/src/clis/nvcf-cli/cmd/cluster_agent_maintenance.go index 966900c1b..3eb8fb8de 100644 --- a/src/clis/nvcf-cli/cmd/cluster_agent_maintenance.go +++ b/src/clis/nvcf-cli/cmd/cluster_agent_maintenance.go @@ -55,9 +55,10 @@ var clusterAgentCordonDrainCmd = &cobra.Command{ deployments, let in-flight requests complete, and scale all function instances to zero. -This sets the CordonAndDrainMaintenance feature flag and maintenanceMode on the -NVCA agent-config ConfigMap and restarts the NVCA deployment. The command returns -once NVCA has been told to drain (and, by default, once the restart rolls out); +This sets the CordonAndDrainMaintenance feature flag on the NVCFBackend CR's +spec.overrides.featureGate.values; the NVCA operator's own reconcile then +regenerates agent-config and restarts NVCA. The command returns once the CR +update is accepted (and, by default, once the operator's rollout completes); use "cluster agent list-functions --phase DRAINING" to watch instances wind down. Select the cluster with --compute-plane-context, as with the inspection commands.`, @@ -71,7 +72,8 @@ var clusterAgentUncordonCmd = &cobra.Command{ SilenceUsage: true, Args: cobra.NoArgs, Long: `Reverse a cordon-and-drain: remove the CordonAndDrainMaintenance feature -flag and maintenanceMode from the NVCA agent-config ConfigMap and restart NVCA so +flag from the NVCFBackend CR's spec.overrides.featureGate.values; the NVCA +operator's own reconcile then regenerates agent-config and restarts NVCA so the cluster accepts new deployments again.`, RunE: runClusterAgentUncordon, } @@ -447,13 +449,13 @@ func printDrainResult(cmd *cobra.Command, res *clusteragent.DrainResult, drain b return } if res.DryRun { - fmt.Fprintln(w, " would update agent-config and restart NVCA") + fmt.Fprintln(w, " would update the NVCFBackend CR; the NVCA operator would then roll out the change") return } if drain { - fmt.Fprintf(w, " agent-config updated (maintenanceMode=%s); NVCA restart triggered\n", orDash(res.Mode)) + fmt.Fprintf(w, " NVCFBackend updated (maintenanceMode=%s)\n", orDash(res.Mode)) } else { - fmt.Fprintln(w, " agent-config updated (maintenance cleared); NVCA restart triggered") + fmt.Fprintln(w, " NVCFBackend updated (maintenance cleared)") } switch { case res.RolloutComplete: diff --git a/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer.go b/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer.go index ca913fc86..3b873044d 100644 --- a/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer.go +++ b/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer.go @@ -20,6 +20,7 @@ package clusteragent import ( "context" "fmt" + "slices" "strings" "time" @@ -28,6 +29,7 @@ import ( 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/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/client-go/dynamic" "k8s.io/client-go/kubernetes" @@ -35,16 +37,20 @@ import ( ) // Maintenance constants. These mirror the NVCA operator contract defined in -// nvca/pkg/operator/cleanup/cleanup.go and pkg/operator/types/types.go. Drain -// flips CordonAndDrain maintenance on the agent-config ConfigMap and restarts -// the NVCA Deployment; the operator picks the change up on the rollout. +// nvca/pkg/operator/reconcile/backendk8scache.go. Drain adds the +// CordonAndDrainMaintenance flag to the NVCFBackend CR's +// spec.overrides.featureGate.values; the operator's own reconcile loop +// regenerates agent-config from the CR and rolls out NVCA itself. The CLI +// never writes agent-config or the NVCA Deployment directly: the operator +// treats both as generated artifacts and reverts direct edits on its next +// reconcile (informer resync, CR change, or operator restart). const ( agentConfigConfigMapName = "agent-config" agentConfigKey = "config.yaml" nvcaDeploymentName = "nvca" cordonAndDrainFeatureFlag = "CordonAndDrainMaintenance" + cordonMaintenanceFeatureFlag = "CordonMaintenance" maintenanceModeCordonAndDrain = "CordonAndDrain" - restartedAtAnnotation = "kubectl.kubernetes.io/restartedAt" // Namespace defaults applied when the NVCFBackend CR leaves them empty, // matching DefaultNVCASystemNamespace / DefaultNVCARequestsNamespace upstream. @@ -73,15 +79,12 @@ func NewK8sMaintainer(dc dynamic.Interface, cs kubernetes.Interface) AgentMainta // ResolveCluster reads the NVCFBackend CR and returns the cluster identity and // namespace layout, applying defaults for unset namespaces. func (m *k8sMaintainer) ResolveCluster(ctx context.Context, backendNS string) (*ClusterTarget, error) { - list, err := m.dc.Resource(nvcfBackendGVR).Namespace(backendNS).List(ctx, metav1.ListOptions{}) + item, err := m.getNVCFBackendObject(ctx, backendNS) if err != nil { - return nil, wrapCRDError(err, "NVCFBackend", backendNS) - } - if len(list.Items) == 0 { - return nil, fmt.Errorf("no NVCFBackend resource found in namespace %q; is this context pointed at an NVCF compute-plane cluster (try --backend-namespace)?", backendNS) + return nil, err } - obj := list.Items[0].Object + obj := item.Object return &ClusterTarget{ ClusterID: firstNonEmpty(nestedString(obj, "spec", "clusterConfig", "clusterId"), nestedString(obj, "spec", "clusterConfig", "clusterID")), ClusterName: nestedString(obj, "spec", "clusterConfig", "clusterName"), @@ -90,6 +93,19 @@ func (m *k8sMaintainer) ResolveCluster(ctx context.Context, backendNS string) (* }, nil } +// getNVCFBackendObject fetches the single NVCFBackend CR in backendNS. The +// NVCA operator contract guarantees exactly one per compute-plane cluster. +func (m *k8sMaintainer) getNVCFBackendObject(ctx context.Context, backendNS string) (*unstructured.Unstructured, error) { + list, err := m.dc.Resource(nvcfBackendGVR).Namespace(backendNS).List(ctx, metav1.ListOptions{}) + if err != nil { + return nil, wrapCRDError(err, "NVCFBackend", backendNS) + } + if len(list.Items) == 0 { + return nil, fmt.Errorf("no NVCFBackend resource found in namespace %q; is this context pointed at an NVCF compute-plane cluster (try --backend-namespace)?", backendNS) + } + return &list.Items[0], nil +} + // resolveAndVerify is the common preamble: resolve the cluster, then enforce the // optional --expect-cluster-id guard. func (m *k8sMaintainer) resolveAndVerify(ctx context.Context, backendNS, expectClusterID string) (*ClusterTarget, error) { @@ -151,54 +167,61 @@ func (m *k8sMaintainer) setMaintenance(ctx context.Context, opts DrainOptions, d SystemNamespace: systemNS, DryRun: opts.DryRun, } - - var transform func(string) string if drain { result.Mode = maintenanceModeCordonAndDrain - transform = func(y string) string { - return addMaintenanceModeToConfig(addFeatureFlagToConfig(y, cordonAndDrainFeatureFlag), maintenanceModeCordonAndDrain) - } - } else { - transform = func(y string) string { - return clearMaintenanceModeFromConfig(removeFeatureFlagFromConfig(y, cordonAndDrainFeatureFlag)) - } + } + + if err := m.checkMaintenanceConflict(ctx, opts.BackendNS, drain); err != nil { + return nil, err } if opts.DryRun { - _, cur, err := m.getAgentConfig(ctx, systemNS) + has, err := m.nvcfBackendHasMaintenanceFlag(ctx, opts.BackendNS) if err != nil { return nil, err } - result.ConfigChanged = transform(cur) != cur + result.ConfigChanged = has != drain if result.ConfigChanged { - result.Message = "dry run: would update agent-config and restart NVCA" + result.Message = "dry run: would update the NVCFBackend CR; the NVCA operator would then regenerate agent-config and roll out NVCA" } else { result.Message = "dry run: already in the requested state; no change" } return result, nil } - changed, err := m.patchAgentConfig(ctx, systemNS, transform) + changed, err := m.patchMaintenanceFeatureFlag(ctx, opts.BackendNS, drain) if err != nil { return nil, err } result.ConfigChanged = changed - if !changed && !opts.Force { - // Idempotent: skip the rollout so an in-flight drain is not disrupted. - // Use --force to retry the restart if a previous run failed after the - // config patch but before the rollout completed. + if !changed { + // Idempotent: nothing to wait for. Re-running the same command is + // always safe here (unlike the old ConfigMap/Deployment-restart + // approach), since the CLI no longer performs a mutation the + // operator could race with; it only submits a desired-state change + // the operator's own reconcile owns. + // + // Behavior change from the old agent-config/Deployment-restart + // approach: --force no longer has an effect here. It used to also + // retrigger the NVCA restart even when the desired state was + // already reached, which let a stuck rollout be kicked by + // re-running with --force. Since the CLI no longer performs that + // restart directly, there is nothing left for --force to retrigger + // when the CR is already in the desired state; only the operator's + // own reconcile can recover a stuck rollout now. result.Message = "already in the requested state; no change" return result, nil } - - if err := m.triggerRollout(ctx, systemNS); err != nil { - return result, fmt.Errorf("agent-config updated but failed to restart NVCA: %w\n(re-run with --force to retry the restart)", err) - } result.RolloutTriggered = true - if !opts.Force && opts.Timeout > 0 { - if err := m.waitForRollout(ctx, systemNS, opts.Timeout); err != nil { - result.Message = fmt.Sprintf("agent-config updated and restart triggered, but the rollout did not complete in time: %v", err) + switch { + case opts.Force: + result.Message = "NVCFBackend updated; not waiting for the NVCA operator's rollout (--force)" + case opts.Timeout <= 0: + result.Message = "NVCFBackend updated; not waiting for the NVCA operator's rollout (--timeout 0)" + default: + if err := m.waitForMaintenanceRollout(ctx, opts.BackendNS, systemNS, opts.Timeout, drain); err != nil { + result.Message = fmt.Sprintf("NVCFBackend updated, but the NVCA operator has not finished reconciling and rolling out the change: %v", err) return result, nil } result.RolloutComplete = true @@ -206,46 +229,83 @@ func (m *k8sMaintainer) setMaintenance(ctx context.Context, opts DrainOptions, d return result, nil } -// getAgentConfig fetches the agent-config ConfigMap and its config.yaml payload, -// translating common failures into actionable messages. -func (m *k8sMaintainer) getAgentConfig(ctx context.Context, systemNS string) (*corev1.ConfigMap, string, error) { - cm, err := m.cs.CoreV1().ConfigMaps(systemNS).Get(ctx, agentConfigConfigMapName, metav1.GetOptions{}) +// nvcfBackendHasMaintenanceFlag reports whether the NVCFBackend CR's +// spec.overrides.featureGate.values in backendNS currently contains +// cordonAndDrainFeatureFlag. +func (m *k8sMaintainer) nvcfBackendHasMaintenanceFlag(ctx context.Context, backendNS string) (bool, error) { + obj, err := m.getNVCFBackendObject(ctx, backendNS) if err != nil { - switch { - case apierrors.IsNotFound(err): - return nil, "", fmt.Errorf("agent-config ConfigMap not found in namespace %s; is NVCA installed on this cluster?", systemNS) - case apierrors.IsForbidden(err): - return nil, "", fmt.Errorf("not permitted to read the agent-config ConfigMap in namespace %s: %w", systemNS, err) - default: - return nil, "", fmt.Errorf("failed to read agent-config ConfigMap in namespace %s: %w", systemNS, err) - } + return false, err } - cur, ok := cm.Data[agentConfigKey] - if !ok { - return nil, "", fmt.Errorf("agent-config ConfigMap %s/%s is missing the %q key", systemNS, agentConfigConfigMapName, agentConfigKey) + values, _, err := unstructured.NestedStringSlice(obj.Object, "spec", "overrides", "featureGate", "values") + if err != nil { + return false, fmt.Errorf("reading NVCFBackend spec.overrides.featureGate.values: %w", err) } - return cm, cur, nil + return slices.Contains(values, cordonAndDrainFeatureFlag), nil +} + +// checkMaintenanceConflict reads the NVCFBackend CR's base +// spec.featureGate.values and returns an error if the operator's +// mergeOverrides logic would keep the requested drain/undrain from ever +// taking effect, even though patching spec.overrides would succeed: +// +// - drain: if the base spec already sets cordonMaintenanceFeatureFlag, the +// operator prefers CordonMaintenance over CordonAndDrainMaintenance +// whenever both are present in the merged result, so adding +// cordonAndDrainFeatureFlag to the overrides would be silently dropped. +// - undrain: if the base spec already sets cordonAndDrainFeatureFlag, the +// operator's merge keeps a maintenance flag that was present in either +// the base spec or the overrides, so removing it from overrides alone +// would not clear it from the merged result. +func (m *k8sMaintainer) checkMaintenanceConflict(ctx context.Context, backendNS string, drain bool) error { + obj, err := m.getNVCFBackendObject(ctx, backendNS) + if err != nil { + return err + } + baseValues, _, err := unstructured.NestedStringSlice(obj.Object, "spec", "featureGate", "values") + if err != nil { + return fmt.Errorf("reading NVCFBackend spec.featureGate.values: %w", err) + } + switch { + case drain && slices.Contains(baseValues, cordonMaintenanceFeatureFlag): + return fmt.Errorf("cannot drain: NVCFBackend spec.featureGate.values already sets %q, which the NVCA operator prefers over %q, so drain would have no effect; remove %q from the base spec first", cordonMaintenanceFeatureFlag, cordonAndDrainFeatureFlag, cordonMaintenanceFeatureFlag) + case !drain && slices.Contains(baseValues, cordonAndDrainFeatureFlag): + return fmt.Errorf("cannot undrain: NVCFBackend spec.featureGate.values already sets %q, which the NVCA operator keeps regardless of overrides, so undrain would have no effect; remove %q from the base spec first", cordonAndDrainFeatureFlag, cordonAndDrainFeatureFlag) + } + return nil } -// patchAgentConfig reads, transforms, and writes config.yaml under retry-on- -// conflict. It reports whether the transform actually changed the content. -func (m *k8sMaintainer) patchAgentConfig(ctx context.Context, systemNS string, transform func(string) string) (bool, error) { +// patchMaintenanceFeatureFlag adds or removes cordonAndDrainFeatureFlag on +// the NVCFBackend CR's spec.overrides.featureGate.values, retrying on update +// conflicts. It reports whether the value actually changed. +// +// This patches the NVCFBackend CR rather than agent-config directly: the +// NVCA operator treats agent-config as a fully generated artifact rebuilt +// from this CR on every reconcile (informer resync, CR change, operator +// restart), so a direct ConfigMap edit gets silently reverted on the +// operator's next reconcile. Patching spec.overrides here (rather than the +// base spec.featureGate) lets the operator's own additive merge apply it and +// its reconcile regenerate agent-config correctly and roll out NVCA itself. +func (m *k8sMaintainer) patchMaintenanceFeatureFlag(ctx context.Context, backendNS string, drain bool) (bool, error) { changed := false err := retry.RetryOnConflict(retry.DefaultRetry, func() error { - cm, cur, err := m.getAgentConfig(ctx, systemNS) + obj, err := m.getNVCFBackendObject(ctx, backendNS) if err != nil { return err } - next := transform(cur) - if next == cur { + values, _, err := unstructured.NestedStringSlice(obj.Object, "spec", "overrides", "featureGate", "values") + if err != nil { + return fmt.Errorf("reading NVCFBackend spec.overrides.featureGate.values: %w", err) + } + next := setMaintenanceFeatureFlag(values, drain) + if slices.Equal(values, next) { changed = false return nil } - if cm.Data == nil { - cm.Data = map[string]string{} + if err := unstructured.SetNestedStringSlice(obj.Object, next, "spec", "overrides", "featureGate", "values"); err != nil { + return fmt.Errorf("writing NVCFBackend spec.overrides.featureGate.values: %w", err) } - cm.Data[agentConfigKey] = next - if _, err := m.cs.CoreV1().ConfigMaps(systemNS).Update(ctx, cm, metav1.UpdateOptions{}); err != nil { + if _, err := m.dc.Resource(nvcfBackendGVR).Namespace(backendNS).Update(ctx, obj, metav1.UpdateOptions{}); err != nil { return err } changed = true @@ -254,55 +314,89 @@ func (m *k8sMaintainer) patchAgentConfig(ctx context.Context, systemNS string, t return changed, err } -// triggerRollout restarts the NVCA Deployment by stamping the standard restart -// annotation on its pod template, mirroring triggerNVCARollout in the operator. -func (m *k8sMaintainer) triggerRollout(ctx context.Context, systemNS string) error { - return retry.RetryOnConflict(retry.DefaultRetry, func() error { - deploy, err := m.cs.AppsV1().Deployments(systemNS).Get(ctx, nvcaDeploymentName, metav1.GetOptions{}) - if err != nil { - if apierrors.IsNotFound(err) { - return fmt.Errorf("NVCA deployment %s/%s not found", systemNS, nvcaDeploymentName) +// setMaintenanceFeatureFlag returns values with cordonAndDrainFeatureFlag +// added (drain) or removed (undrain), preserving the order and content of +// every other entry. +func setMaintenanceFeatureFlag(values []string, drain bool) []string { + next := make([]string, 0, len(values)+1) + has := false + for _, v := range values { + if v == cordonAndDrainFeatureFlag { + has = true + if !drain { + continue } - return fmt.Errorf("failed to get NVCA deployment %s/%s: %w", systemNS, nvcaDeploymentName, err) } - if deploy.Spec.Template.Annotations == nil { - deploy.Spec.Template.Annotations = map[string]string{} + next = append(next, v) + } + if drain && !has { + next = append(next, cordonAndDrainFeatureFlag) + } + return next +} + +// getAgentConfig fetches the agent-config ConfigMap and its config.yaml payload, +// translating common failures into actionable messages. It is read-only: the +// CLI never writes this ConfigMap (see patchMaintenanceFeatureFlag). +func (m *k8sMaintainer) getAgentConfig(ctx context.Context, systemNS string) (*corev1.ConfigMap, string, error) { + cm, err := m.cs.CoreV1().ConfigMaps(systemNS).Get(ctx, agentConfigConfigMapName, metav1.GetOptions{}) + if err != nil { + switch { + case apierrors.IsNotFound(err): + return nil, "", fmt.Errorf("agent-config ConfigMap not found in namespace %s; is NVCA installed on this cluster?", systemNS) + case apierrors.IsForbidden(err): + return nil, "", fmt.Errorf("not permitted to read the agent-config ConfigMap in namespace %s: %w", systemNS, err) + default: + return nil, "", fmt.Errorf("failed to read agent-config ConfigMap in namespace %s: %w", systemNS, err) } - deploy.Spec.Template.Annotations[restartedAtAnnotation] = time.Now().UTC().Format(time.RFC3339) - _, err = m.cs.AppsV1().Deployments(systemNS).Update(ctx, deploy, metav1.UpdateOptions{}) - return err - }) + } + cur, ok := cm.Data[agentConfigKey] + if !ok { + return nil, "", fmt.Errorf("agent-config ConfigMap %s/%s is missing the %q key", systemNS, agentConfigConfigMapName, agentConfigKey) + } + return cm, cur, nil } -// waitForRollout polls until the NVCA Deployment rollout completes or the -// timeout elapses, mirroring waitForDeploymentRollout in the operator. -func (m *k8sMaintainer) waitForRollout(ctx context.Context, systemNS string, timeout time.Duration) error { +// waitForMaintenanceRollout polls until the NVCA operator has both +// regenerated agent-config to reflect the new maintenance state and rolled +// the NVCA Deployment out to match, or timeout elapses. +// +// Both conditions are checked together deliberately: checking only the +// Deployment's rollout status is not sufficient, because immediately after +// patching the CR the Deployment may still trivially satisfy the "rollout +// complete" condition from before the operator has even started reconciling +// the change, which would report success without the operator having done +// anything yet. +func (m *k8sMaintainer) waitForMaintenanceRollout(ctx context.Context, backendNS, systemNS string, timeout time.Duration, drain bool) error { deadline := time.Now().Add(timeout) for { + configReady := false + if _, cur, err := m.getAgentConfig(ctx, systemNS); err == nil { + configReady = configHasFeatureFlag(cur, cordonAndDrainFeatureFlag) == drain + } + + rolloutReady := false deploy, err := m.cs.AppsV1().Deployments(systemNS).Get(ctx, nvcaDeploymentName, metav1.GetOptions{}) - if err != nil { - if apierrors.IsNotFound(err) { - return nil + switch { + case apierrors.IsNotFound(err): + rolloutReady = false + case err == nil: + desired := int32(1) + if deploy.Spec.Replicas != nil { + desired = *deploy.Spec.Replicas } - return fmt.Errorf("failed to get NVCA deployment %s/%s: %w", systemNS, nvcaDeploymentName, err) + rolloutReady = deploy.Status.ObservedGeneration >= deploy.Generation && + deploy.Status.UpdatedReplicas == desired && + deploy.Status.AvailableReplicas == desired && + deploy.Status.UnavailableReplicas == 0 } - desired := int32(1) - if deploy.Spec.Replicas != nil { - desired = *deploy.Spec.Replicas - } - // ObservedGeneration must catch up to the spec generation first, otherwise - // the status still reflects the previous rollout and we could report - // completion before the restart we just triggered has even begun. - if deploy.Status.ObservedGeneration >= deploy.Generation && - deploy.Status.UpdatedReplicas == desired && - deploy.Status.AvailableReplicas == desired && - deploy.Status.UnavailableReplicas == 0 { + if configReady && rolloutReady { return nil } if time.Now().After(deadline) { - return fmt.Errorf("timeout waiting for NVCA deployment %s/%s rollout", systemNS, nvcaDeploymentName) + return fmt.Errorf("timeout waiting for the NVCA operator to reconcile NVCFBackend %s and roll out %s/%s", backendNS, systemNS, nvcaDeploymentName) } select { case <-ctx.Done(): @@ -456,6 +550,36 @@ func aggregateKillError(result *KillResult) error { // drops comments. Missing sections degrade to a no-op rather than corrupting the // file. +// configHasFeatureFlag reports whether featureFlag is listed in the +// featureFlags: section of configYAML. Unlike checking +// addFeatureFlagToConfig's return value for a no-op, this is a pure +// membership check: addFeatureFlagToConfig also returns configYAML +// unchanged when there is no featureFlags: (or even agent:) section to +// insert into at all, which would misreport an absent flag as present. +func configHasFeatureFlag(configYAML, featureFlag string) bool { + inFlags := false + for _, line := range strings.Split(configYAML, "\n") { + trimmed := strings.TrimLeft(line, " \t") + if trimmed == "featureFlags:" { + inFlags = true + continue + } + if !inFlags { + continue + } + if strings.HasPrefix(trimmed, "- ") { + if trimmed == "- "+featureFlag { + return true + } + continue + } + if trimmed != "" { + inFlags = false + } + } + return false +} + func addFeatureFlagToConfig(configYAML, featureFlag string) string { lines := strings.Split(configYAML, "\n") @@ -496,79 +620,6 @@ func addFeatureFlagToConfig(configYAML, featureFlag string) string { return configYAML } -func removeFeatureFlagFromConfig(configYAML, featureFlag string) string { - lines := strings.Split(configYAML, "\n") - - // Remove the flag only within the featureFlags: section. - inFlags := false - without := make([]string, 0, len(lines)) - for _, line := range lines { - trimmed := strings.TrimLeft(line, " \t") - if trimmed == "featureFlags:" { - inFlags = true - } else if inFlags && !strings.HasPrefix(trimmed, "- ") && trimmed != "" { - inFlags = false - } - if inFlags && trimmed == "- "+featureFlag { - continue - } - without = append(without, line) - } - - // Drop an orphaned featureFlags: key whose list is now empty. - out := make([]string, 0, len(without)) - for i, line := range without { - if strings.TrimLeft(line, " \t") == "featureFlags:" { - hasItem := false - for j := i + 1; j < len(without); j++ { - trimmed := strings.TrimLeft(without[j], " \t") - if trimmed == "" { - continue - } - hasItem = strings.HasPrefix(trimmed, "- ") - break - } - if !hasItem { - continue - } - } - out = append(out, line) - } - return strings.Join(out, "\n") -} - -func addMaintenanceModeToConfig(configYAML, maintenanceMode string) string { - lines := strings.Split(configYAML, "\n") - - for i, line := range lines { - if strings.HasPrefix(strings.TrimLeft(line, " \t"), "maintenanceMode:") { - indent := line[:len(line)-len(strings.TrimLeft(line, " \t"))] - lines[i] = indent + "maintenanceMode: " + maintenanceMode - return strings.Join(lines, "\n") - } - } - - for i, line := range lines { - if strings.TrimRight(line, " \t\r") == "agent:" { - lines = insertAfter(lines, i, " maintenanceMode: "+maintenanceMode) - break - } - } - return strings.Join(lines, "\n") -} - -func clearMaintenanceModeFromConfig(configYAML string) string { - lines := strings.Split(configYAML, "\n") - out := make([]string, 0, len(lines)) - for _, line := range lines { - if strings.HasPrefix(strings.TrimLeft(line, " \t"), "maintenanceMode:") { - continue - } - out = append(out, line) - } - return strings.Join(out, "\n") -} - func insertAfter(lines []string, index int, newLine string) []string { result := make([]string, 0, len(lines)+1) result = append(result, lines[:index+1]...) diff --git a/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer_test.go b/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer_test.go index ccbdb0711..ca9668247 100644 --- a/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer_test.go +++ b/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer_test.go @@ -20,6 +20,7 @@ package clusteragent import ( "context" "fmt" + "slices" "strings" "testing" "time" @@ -112,83 +113,133 @@ func icmsRequestWithFinalizers(ns, name, fid, vid string, finalizers ...string) return u } -func readConfig(t *testing.T, cs *k8sfake.Clientset, systemNS string) string { - t.Helper() - cm, err := cs.CoreV1().ConfigMaps(systemNS).Get(context.Background(), agentConfigConfigMapName, metav1.GetOptions{}) - if err != nil { - t.Fatalf("reading agent-config back: %v", err) +// --- Drain / Undrain --- + +// backendObjWithOverrideValues seeds an NVCFBackend CR with a pre-existing +// spec.overrides.featureGate.values list, simulating a cluster already +// carrying prior CLI-set overrides. +func backendObjWithOverrideValues(backendNS, clusterID, clusterName, systemNS, requestsNS string, overrideValues ...string) *unstructured.Unstructured { + b := backendObj(backendNS, clusterID, clusterName, systemNS, requestsNS) + vals := make([]interface{}, len(overrideValues)) + for i, v := range overrideValues { + vals[i] = v + } + b.Object["spec"].(map[string]interface{})["overrides"] = map[string]interface{}{ + "featureGate": map[string]interface{}{"values": vals}, + } + return b +} + +// backendObjWithBaseValues seeds an NVCFBackend CR with a pre-existing +// spec.featureGate.values list (the base spec, not overrides), simulating a +// cluster whose base spec already sets a maintenance flag directly. +func backendObjWithBaseValues(backendNS, clusterID, clusterName, systemNS, requestsNS string, baseValues ...string) *unstructured.Unstructured { + b := backendObj(backendNS, clusterID, clusterName, systemNS, requestsNS) + vals := make([]interface{}, len(baseValues)) + for i, v := range baseValues { + vals[i] = v } - return cm.Data[agentConfigKey] + b.Object["spec"].(map[string]interface{})["featureGate"] = map[string]interface{}{"values": vals} + return b } -func deployAnnotations(t *testing.T, cs *k8sfake.Clientset, systemNS string) map[string]string { +// backendOverrideValues reads spec.overrides.featureGate.values back off the +// single NVCFBackend CR in backendNS, the same field patchMaintenanceFeatureFlag +// writes. +func backendOverrideValues(t *testing.T, dc *dynamicfake.FakeDynamicClient, backendNS string) []string { t.Helper() - d, err := cs.AppsV1().Deployments(systemNS).Get(context.Background(), nvcaDeploymentName, metav1.GetOptions{}) + list, err := dc.Resource(nvcfBackendGVR).Namespace(backendNS).List(context.Background(), metav1.ListOptions{}) + if err != nil { + t.Fatalf("listing NVCFBackend: %v", err) + } + if len(list.Items) == 0 { + t.Fatalf("no NVCFBackend found in namespace %q", backendNS) + } + values, _, err := unstructured.NestedStringSlice(list.Items[0].Object, "spec", "overrides", "featureGate", "values") if err != nil { - t.Fatalf("reading deployment back: %v", err) + t.Fatalf("reading spec.overrides.featureGate.values: %v", err) } - return d.Spec.Template.Annotations + return values } -// --- Drain / Undrain --- - -func TestDrainAddsMaintenanceAndRestarts(t *testing.T) { - cfg := "agent:\n featureFlags:\n - LogPosting\n" - m, _, cs := newFakeMaintainer( - []runtime.Object{defaultBackend()}, - []runtime.Object{agentConfigObj(testSystemNS, cfg), nvcaDeployObj(testSystemNS, 1, true)}, +func TestDrainPatchesNVCFBackendOverrides(t *testing.T) { + m, dc, _ := newFakeMaintainer( + []runtime.Object{backendObjWithOverrideValues(testBackendNS, testClusterID, testCluster, testSystemNS, testRequestsNS, "LogPosting")}, + nil, ) - res, err := m.Drain(context.Background(), DrainOptions{BackendNS: testBackendNS, Timeout: time.Second}) + res, err := m.Drain(context.Background(), DrainOptions{BackendNS: testBackendNS}) if err != nil { t.Fatalf("Drain returned error: %v", err) } - if !res.ConfigChanged || !res.RolloutTriggered || !res.RolloutComplete { + if !res.ConfigChanged || !res.RolloutTriggered { t.Fatalf("unexpected result: %+v", res) } if res.Mode != maintenanceModeCordonAndDrain { t.Errorf("Mode = %q, want %q", res.Mode, maintenanceModeCordonAndDrain) } - - got := readConfig(t, cs, testSystemNS) - if !strings.Contains(got, "- "+cordonAndDrainFeatureFlag) { - t.Errorf("config missing feature flag:\n%s", got) - } - if !strings.Contains(got, "maintenanceMode: "+maintenanceModeCordonAndDrain) { - t.Errorf("config missing maintenanceMode:\n%s", got) + got := backendOverrideValues(t, dc, testBackendNS) + if !slices.Contains(got, cordonAndDrainFeatureFlag) { + t.Errorf("overrides missing feature flag: %v", got) } - if !strings.Contains(got, "- LogPosting") { - t.Errorf("config dropped the pre-existing LogPosting flag:\n%s", got) - } - if _, ok := deployAnnotations(t, cs, testSystemNS)[restartedAtAnnotation]; !ok { - t.Errorf("deployment was not restarted (no %s annotation)", restartedAtAnnotation) + if !slices.Contains(got, "LogPosting") { + t.Errorf("drain dropped the pre-existing LogPosting override: %v", got) } } func TestDrainIdempotent(t *testing.T) { - cfg := "agent:\n maintenanceMode: CordonAndDrain\n featureFlags:\n - CordonAndDrainMaintenance\n" - m, _, cs := newFakeMaintainer( - []runtime.Object{defaultBackend()}, - []runtime.Object{agentConfigObj(testSystemNS, cfg), nvcaDeployObj(testSystemNS, 1, true)}, + m, dc, _ := newFakeMaintainer( + []runtime.Object{backendObjWithOverrideValues(testBackendNS, testClusterID, testCluster, testSystemNS, testRequestsNS, cordonAndDrainFeatureFlag)}, + nil, ) - res, err := m.Drain(context.Background(), DrainOptions{BackendNS: testBackendNS, Timeout: time.Second}) + res, err := m.Drain(context.Background(), DrainOptions{BackendNS: testBackendNS}) if err != nil { t.Fatalf("Drain returned error: %v", err) } if res.ConfigChanged || res.RolloutTriggered { t.Fatalf("expected no-op, got %+v", res) } - if _, ok := deployAnnotations(t, cs, testSystemNS)[restartedAtAnnotation]; ok { - t.Error("idempotent drain must not restart NVCA") + before := backendOverrideValues(t, dc, testBackendNS) + if !slices.Equal(before, []string{cordonAndDrainFeatureFlag}) { + t.Errorf("idempotent drain must not touch overrides, got %v", before) + } +} + +func TestDrainConflictsWithBaseCordonMaintenance(t *testing.T) { + m, dc, _ := newFakeMaintainer( + []runtime.Object{backendObjWithBaseValues(testBackendNS, testClusterID, testCluster, testSystemNS, testRequestsNS, cordonMaintenanceFeatureFlag)}, + nil, + ) + + _, err := m.Drain(context.Background(), DrainOptions{BackendNS: testBackendNS}) + if err == nil { + t.Fatal("expected an error: base spec already sets CordonMaintenance, which the operator prefers over CordonAndDrainMaintenance") + } + if got := backendOverrideValues(t, dc, testBackendNS); len(got) != 0 { + t.Errorf("overrides mutated despite conflict: %v", got) + } +} + +func TestUndrainConflictsWithBaseCordonAndDrain(t *testing.T) { + m, dc, _ := newFakeMaintainer( + []runtime.Object{backendObjWithBaseValues(testBackendNS, testClusterID, testCluster, testSystemNS, testRequestsNS, cordonAndDrainFeatureFlag)}, + nil, + ) + + _, err := m.Undrain(context.Background(), DrainOptions{BackendNS: testBackendNS}) + if err == nil { + t.Fatal("expected an error: base spec already sets CordonAndDrainMaintenance, which the operator keeps regardless of overrides") + } + if got := backendOverrideValues(t, dc, testBackendNS); len(got) != 0 { + t.Errorf("overrides mutated despite conflict: %v", got) } } func TestDrainDryRunMutatesNothing(t *testing.T) { - cfg := "agent:\n featureFlags:\n - LogPosting\n" - m, _, cs := newFakeMaintainer( - []runtime.Object{defaultBackend()}, - []runtime.Object{agentConfigObj(testSystemNS, cfg), nvcaDeployObj(testSystemNS, 1, true)}, + m, dc, _ := newFakeMaintainer( + []runtime.Object{backendObjWithOverrideValues(testBackendNS, testClusterID, testCluster, testSystemNS, testRequestsNS, "LogPosting")}, + nil, ) res, err := m.Drain(context.Background(), DrainOptions{BackendNS: testBackendNS, DryRun: true}) @@ -198,81 +249,180 @@ func TestDrainDryRunMutatesNothing(t *testing.T) { if !res.DryRun || !res.ConfigChanged || res.RolloutTriggered { t.Fatalf("unexpected dry-run result: %+v", res) } - if got := readConfig(t, cs, testSystemNS); got != cfg { - t.Errorf("dry-run mutated config:\n%s", got) - } - if _, ok := deployAnnotations(t, cs, testSystemNS)[restartedAtAnnotation]; ok { - t.Error("dry-run must not restart NVCA") + got := backendOverrideValues(t, dc, testBackendNS) + if !slices.Equal(got, []string{"LogPosting"}) { + t.Errorf("dry-run mutated overrides: %v", got) } } func TestDrainExpectClusterID(t *testing.T) { - cfg := "agent:\n" - newM := func() (*k8sMaintainer, *k8sfake.Clientset) { - m, _, cs := newFakeMaintainer( - []runtime.Object{defaultBackend()}, - []runtime.Object{agentConfigObj(testSystemNS, cfg), nvcaDeployObj(testSystemNS, 1, true)}, - ) - return m, cs + newM := func() (*k8sMaintainer, *dynamicfake.FakeDynamicClient) { + m, dc, _ := newFakeMaintainer([]runtime.Object{defaultBackend()}, nil) + return m, dc } t.Run("mismatch aborts before any write", func(t *testing.T) { - m, cs := newM() - _, err := m.Drain(context.Background(), DrainOptions{BackendNS: testBackendNS, ExpectClusterID: "wrong-id", Timeout: time.Second}) + m, dc := newM() + _, err := m.Drain(context.Background(), DrainOptions{BackendNS: testBackendNS, ExpectClusterID: "wrong-id"}) if err == nil { t.Fatal("expected refusal on cluster-id mismatch") } - if got := readConfig(t, cs, testSystemNS); got != cfg { - t.Errorf("config mutated despite mismatch:\n%s", got) + if got := backendOverrideValues(t, dc, testBackendNS); len(got) != 0 { + t.Errorf("overrides mutated despite mismatch: %v", got) } }) t.Run("matches by id", func(t *testing.T) { m, _ := newM() - if _, err := m.Drain(context.Background(), DrainOptions{BackendNS: testBackendNS, ExpectClusterID: testClusterID, Timeout: time.Second}); err != nil { + if _, err := m.Drain(context.Background(), DrainOptions{BackendNS: testBackendNS, ExpectClusterID: testClusterID}); err != nil { t.Fatalf("expected match by id to proceed: %v", err) } }) t.Run("matches by name", func(t *testing.T) { m, _ := newM() - if _, err := m.Drain(context.Background(), DrainOptions{BackendNS: testBackendNS, ExpectClusterID: testCluster, Timeout: time.Second}); err != nil { + if _, err := m.Drain(context.Background(), DrainOptions{BackendNS: testBackendNS, ExpectClusterID: testCluster}); err != nil { t.Fatalf("expected match by name to proceed: %v", err) } }) } -func TestDrainMissingAgentConfig(t *testing.T) { +func TestDrainNoBackend(t *testing.T) { + m, _, _ := newFakeMaintainer(nil, nil) + if _, err := m.Drain(context.Background(), DrainOptions{BackendNS: testBackendNS}); err == nil { + t.Fatal("expected error when no NVCFBackend exists") + } +} + +func TestUndrainRemovesOverride(t *testing.T) { + m, dc, _ := newFakeMaintainer( + []runtime.Object{backendObjWithOverrideValues(testBackendNS, testClusterID, testCluster, testSystemNS, testRequestsNS, cordonAndDrainFeatureFlag, "LogPosting")}, + nil, + ) + + res, err := m.Undrain(context.Background(), DrainOptions{BackendNS: testBackendNS}) + if err != nil { + t.Fatalf("Undrain returned error: %v", err) + } + if !res.ConfigChanged || !res.RolloutTriggered { + t.Fatalf("unexpected result: %+v", res) + } + got := backendOverrideValues(t, dc, testBackendNS) + if slices.Contains(got, cordonAndDrainFeatureFlag) { + t.Errorf("undrain left the feature flag: %v", got) + } + if !slices.Contains(got, "LogPosting") { + t.Errorf("undrain removed an unrelated override: %v", got) + } +} + +func TestUndrainIdempotent(t *testing.T) { + m, dc, _ := newFakeMaintainer( + []runtime.Object{backendObjWithOverrideValues(testBackendNS, testClusterID, testCluster, testSystemNS, testRequestsNS, "LogPosting")}, + nil, + ) + res, err := m.Undrain(context.Background(), DrainOptions{BackendNS: testBackendNS}) + if err != nil { + t.Fatalf("Undrain returned error: %v", err) + } + if res.ConfigChanged || res.RolloutTriggered { + t.Fatalf("expected no-op undrain, got %+v", res) + } + got := backendOverrideValues(t, dc, testBackendNS) + if !slices.Equal(got, []string{"LogPosting"}) { + t.Errorf("idempotent undrain must not touch overrides, got %v", got) + } +} + +// --- Drain / Undrain: waiting for the NVCA operator's own reconcile --- +// +// These tests simulate the operator's effect by pre-seeding agent-config and +// the NVCA Deployment directly, since no real operator runs against the fake +// client. That is also what makes them regression tests for the original +// bug: waitForMaintenanceRollout must not report success just because the +// Deployment trivially already satisfies the completion check before the +// operator has done anything (see TestDrainRolloutTimesOutWhenConfigNeverUpdates). + +func TestDrainReportsRolloutCompleteWhenOperatorHasAlreadyReconciled(t *testing.T) { + // Simulates the operator having already regenerated agent-config and + // rolled out NVCA by the time the CLI's first poll runs. + cfg := "agent:\n featureFlags:\n - " + cordonAndDrainFeatureFlag + "\n" m, _, _ := newFakeMaintainer( []runtime.Object{defaultBackend()}, - []runtime.Object{nvcaDeployObj(testSystemNS, 1, true)}, + []runtime.Object{agentConfigObj(testSystemNS, cfg), nvcaDeployObj(testSystemNS, 1, true)}, ) - _, err := m.Drain(context.Background(), DrainOptions{BackendNS: testBackendNS}) - if err == nil || !strings.Contains(err.Error(), "agent-config ConfigMap not found") { - t.Fatalf("expected a clear missing-configmap error, got %v", err) + + res, err := m.Drain(context.Background(), DrainOptions{BackendNS: testBackendNS, Timeout: time.Second}) + if err != nil { + t.Fatalf("Drain returned error: %v", err) + } + if !res.RolloutComplete { + t.Fatalf("expected rollout to be reported complete, got %+v", res) } } -func TestDrainNoBackend(t *testing.T) { +func TestDrainDoesNotFalselyReportCompleteWhenConfigHasNoFeatureFlagsSection(t *testing.T) { + // Regression test for a false-positive in the config membership check: + // a config with no featureFlags: (or even agent:) section at all must + // not be misread as "already has the flag". Deployment looks complete, + // so this isolates the config-side check specifically. + prev := rolloutPollInterval + rolloutPollInterval = time.Millisecond + t.Cleanup(func() { rolloutPollInterval = prev }) + + cfg := "other:\n x: y\n" m, _, _ := newFakeMaintainer( - nil, - []runtime.Object{agentConfigObj(testSystemNS, "agent:\n"), nvcaDeployObj(testSystemNS, 1, true)}, + []runtime.Object{defaultBackend()}, + []runtime.Object{agentConfigObj(testSystemNS, cfg), nvcaDeployObj(testSystemNS, 1, true)}, ) - if _, err := m.Drain(context.Background(), DrainOptions{BackendNS: testBackendNS}); err == nil { - t.Fatal("expected error when no NVCFBackend exists") + + res, err := m.Drain(context.Background(), DrainOptions{BackendNS: testBackendNS, Timeout: 10 * time.Millisecond}) + if err != nil { + t.Fatalf("timeout must not be a hard error: %v", err) + } + if res.RolloutComplete { + t.Fatal("must not report complete: agent-config has no featureFlags section, so the flag cannot be present") } } -func TestDrainRolloutTimeoutIsWarningNotError(t *testing.T) { +func TestDrainDoesNotFalselyReportCompleteWhenDeploymentMissing(t *testing.T) { + // Regression test: a missing nvca Deployment must not be treated as a + // trivially-satisfied rollout. Otherwise a stale agent-config left over + // from a prior install (matching the requested flag state) combined with + // no running nvca workload would be misreported as a complete rollout. prev := rolloutPollInterval rolloutPollInterval = time.Millisecond t.Cleanup(func() { rolloutPollInterval = prev }) - cfg := "agent:\n" - m, _, cs := newFakeMaintainer( + cfg := "agent:\n featureFlags:\n - " + cordonAndDrainFeatureFlag + "\n" + m, _, _ := newFakeMaintainer( []runtime.Object{defaultBackend()}, - // Deployment never reaches the complete state. - []runtime.Object{agentConfigObj(testSystemNS, cfg), nvcaDeployObj(testSystemNS, 1, false)}, + []runtime.Object{agentConfigObj(testSystemNS, cfg)}, + ) + + res, err := m.Drain(context.Background(), DrainOptions{BackendNS: testBackendNS, Timeout: 10 * time.Millisecond}) + if err != nil { + t.Fatalf("timeout must not be a hard error: %v", err) + } + if res.RolloutComplete { + t.Fatal("must not report complete: the nvca Deployment does not exist") + } +} + +func TestDrainRolloutTimesOutWhenConfigNeverUpdates(t *testing.T) { + // Regression test for the original bug: the Deployment already looks + // "complete" from a prior rollout (this is exactly the trivially-true + // state that misled the old Deployment-only check), but agent-config + // was never regenerated with the flag, i.e. the operator never actually + // reconciled the CR change. The wait must not report success. + prev := rolloutPollInterval + rolloutPollInterval = time.Millisecond + t.Cleanup(func() { rolloutPollInterval = prev }) + + cfg := "agent:\n featureFlags:\n - LogPosting\n" + m, dc, _ := newFakeMaintainer( + []runtime.Object{defaultBackend()}, + []runtime.Object{agentConfigObj(testSystemNS, cfg), nvcaDeployObj(testSystemNS, 1, true)}, ) res, err := m.Drain(context.Background(), DrainOptions{BackendNS: testBackendNS, Timeout: 10 * time.Millisecond}) @@ -282,16 +432,40 @@ func TestDrainRolloutTimeoutIsWarningNotError(t *testing.T) { if !res.ConfigChanged || !res.RolloutTriggered || res.RolloutComplete { t.Fatalf("unexpected result: %+v", res) } - if !strings.Contains(res.Message, "did not complete") { + if !strings.Contains(res.Message, "has not finished") { t.Errorf("message = %q, want a timeout note", res.Message) } - // Config was still persisted. - if got := readConfig(t, cs, testSystemNS); !strings.Contains(got, cordonAndDrainFeatureFlag) { - t.Errorf("config not persisted on timeout:\n%s", got) + // The CR patch itself is still what we're verifying was submitted. + got := backendOverrideValues(t, dc, testBackendNS) + if !slices.Contains(got, cordonAndDrainFeatureFlag) { + t.Errorf("overrides not patched: %v", got) + } +} + +func TestDrainRolloutTimesOutWhenDeploymentNeverStabilizes(t *testing.T) { + // The inverse partial case: agent-config already reflects the flag (the + // operator started reconciling), but the Deployment rollout has not + // stabilized yet. + prev := rolloutPollInterval + rolloutPollInterval = time.Millisecond + t.Cleanup(func() { rolloutPollInterval = prev }) + + cfg := "agent:\n featureFlags:\n - " + cordonAndDrainFeatureFlag + "\n" + m, _, _ := newFakeMaintainer( + []runtime.Object{defaultBackend()}, + []runtime.Object{agentConfigObj(testSystemNS, cfg), nvcaDeployObj(testSystemNS, 1, false)}, + ) + + res, err := m.Drain(context.Background(), DrainOptions{BackendNS: testBackendNS, Timeout: 10 * time.Millisecond}) + if err != nil { + t.Fatalf("timeout must not be a hard error: %v", err) + } + if res.RolloutComplete { + t.Fatal("expected timeout while the Deployment has not stabilized") } } -func TestWaitForRolloutWaitsForObservedGeneration(t *testing.T) { +func TestWaitForMaintenanceRolloutWaitsForObservedGeneration(t *testing.T) { prev := rolloutPollInterval rolloutPollInterval = time.Millisecond t.Cleanup(func() { rolloutPollInterval = prev }) @@ -301,87 +475,46 @@ func TestWaitForRolloutWaitsForObservedGeneration(t *testing.T) { d := nvcaDeployObj(testSystemNS, 1, true) d.Generation = 3 d.Status.ObservedGeneration = 2 - m, _, _ := newFakeMaintainer(nil, []runtime.Object{d}) + cfg := "agent:\n featureFlags:\n - " + cordonAndDrainFeatureFlag + "\n" + m, _, _ := newFakeMaintainer(nil, []runtime.Object{agentConfigObj(testSystemNS, cfg), d}) - if err := m.waitForRollout(context.Background(), testSystemNS, 10*time.Millisecond); err == nil { + if err := m.waitForMaintenanceRollout(context.Background(), testBackendNS, testSystemNS, 10*time.Millisecond, true); err == nil { t.Fatal("expected timeout while ObservedGeneration < Generation, got nil") } } func TestDrainForceSkipsRolloutWait(t *testing.T) { - cfg := "agent:\n" m, _, _ := newFakeMaintainer( []runtime.Object{defaultBackend()}, - []runtime.Object{agentConfigObj(testSystemNS, cfg), nvcaDeployObj(testSystemNS, 1, false)}, + []runtime.Object{agentConfigObj(testSystemNS, "agent:\n"), nvcaDeployObj(testSystemNS, 1, false)}, ) res, err := m.Drain(context.Background(), DrainOptions{BackendNS: testBackendNS, Force: true, Timeout: time.Hour}) if err != nil { t.Fatalf("Drain --force returned error: %v", err) } if !res.RolloutTriggered || res.RolloutComplete { - t.Fatalf("force should trigger rollout but not wait: %+v", res) + t.Fatalf("force should submit the CR change but not wait: %+v", res) } } -func TestDrainForceRetriggersRolloutWhenConfigAlreadySet(t *testing.T) { - // Simulate a prior run that patched the config but failed before triggering - // the rollout. The config is already in the target state (changed=false), - // but --force must bypass the idempotency guard and trigger the rollout. - cfg := "agent:\n maintenanceMode: CordonAndDrain\n featureFlags:\n - CordonAndDrainMaintenance\n" +func TestDrainForceHasNoEffectWhenAlreadyInDesiredState(t *testing.T) { + // Unlike the old ConfigMap/Deployment-restart mechanism, there is no + // separate "restart" action for --force to retrigger once the CR is + // already in the desired state: the operator owns the actual rollout, + // and re-submitting an unchanged CR produces no new reconcile. m, _, _ := newFakeMaintainer( - []runtime.Object{defaultBackend()}, - []runtime.Object{agentConfigObj(testSystemNS, cfg), nvcaDeployObj(testSystemNS, 1, false)}, + []runtime.Object{backendObjWithOverrideValues(testBackendNS, testClusterID, testCluster, testSystemNS, testRequestsNS, cordonAndDrainFeatureFlag)}, + nil, ) res, err := m.Drain(context.Background(), DrainOptions{BackendNS: testBackendNS, Force: true}) if err != nil { t.Fatalf("Drain --force returned error: %v", err) } - if res.ConfigChanged { - t.Errorf("expected no config change (already set), got ConfigChanged=true") - } - if !res.RolloutTriggered { - t.Errorf("--force should trigger rollout even when config is unchanged: %+v", res) - } -} - -func TestUndrainRemovesMaintenance(t *testing.T) { - cfg := "agent:\n maintenanceMode: CordonAndDrain\n featureFlags:\n - CordonAndDrainMaintenance\n - LogPosting\n" - m, _, cs := newFakeMaintainer( - []runtime.Object{defaultBackend()}, - []runtime.Object{agentConfigObj(testSystemNS, cfg), nvcaDeployObj(testSystemNS, 1, true)}, - ) - - res, err := m.Undrain(context.Background(), DrainOptions{BackendNS: testBackendNS, Timeout: time.Second}) - if err != nil { - t.Fatalf("Undrain returned error: %v", err) - } - if !res.ConfigChanged || !res.RolloutTriggered { - t.Fatalf("unexpected result: %+v", res) - } - got := readConfig(t, cs, testSystemNS) - if strings.Contains(got, cordonAndDrainFeatureFlag) { - t.Errorf("undrain left the feature flag:\n%s", got) - } - if strings.Contains(got, "maintenanceMode:") { - t.Errorf("undrain left maintenanceMode:\n%s", got) - } - if !strings.Contains(got, "- LogPosting") { - t.Errorf("undrain removed an unrelated flag:\n%s", got) - } -} - -func TestUndrainIdempotent(t *testing.T) { - cfg := "agent:\n featureFlags:\n - LogPosting\n" - m, _, _ := newFakeMaintainer( - []runtime.Object{defaultBackend()}, - []runtime.Object{agentConfigObj(testSystemNS, cfg), nvcaDeployObj(testSystemNS, 1, true)}, - ) - res, err := m.Undrain(context.Background(), DrainOptions{BackendNS: testBackendNS}) - if err != nil { - t.Fatalf("Undrain returned error: %v", err) - } if res.ConfigChanged || res.RolloutTriggered { - t.Fatalf("expected no-op undrain, got %+v", res) + t.Fatalf("expected a no-op, got %+v", res) + } + if res.Message != "already in the requested state; no change" { + t.Errorf("Message = %q", res.Message) } } @@ -433,58 +566,57 @@ func TestAddFeatureFlagToConfig(t *testing.T) { } } -func TestAddMaintenanceModeToConfig(t *testing.T) { - t.Run("replaces existing", func(t *testing.T) { - in := "agent:\n maintenanceMode: CordonOnly\n" - want := "agent:\n maintenanceMode: CordonAndDrain\n" - if got := addMaintenanceModeToConfig(in, maintenanceModeCordonAndDrain); got != want { - t.Errorf("got %q want %q", got, want) - } - }) - t.Run("inserts when absent", func(t *testing.T) { - in := "agent:\n logLevel: info\n" - want := "agent:\n maintenanceMode: CordonAndDrain\n logLevel: info\n" - if got := addMaintenanceModeToConfig(in, maintenanceModeCordonAndDrain); got != want { - t.Errorf("got %q want %q", got, want) - } - }) -} - -func TestRemoveAndClearHelpers(t *testing.T) { - t.Run("remove feature flag", func(t *testing.T) { - in := "agent:\n featureFlags:\n - CordonAndDrainMaintenance\n - LogPosting\n" - want := "agent:\n featureFlags:\n - LogPosting\n" - if got := removeFeatureFlagFromConfig(in, cordonAndDrainFeatureFlag); got != want { - t.Errorf("got %q want %q", got, want) - } - }) - t.Run("remove absent flag is unchanged", func(t *testing.T) { - in := "agent:\n featureFlags:\n - LogPosting\n" - if got := removeFeatureFlagFromConfig(in, cordonAndDrainFeatureFlag); got != in { - t.Errorf("got %q want %q", got, in) - } - }) - t.Run("remove last flag drops orphaned featureFlags key", func(t *testing.T) { - in := "agent:\n featureFlags:\n - CordonAndDrainMaintenance\n logLevel: info\n" - want := "agent:\n logLevel: info\n" - if got := removeFeatureFlagFromConfig(in, cordonAndDrainFeatureFlag); got != want { - t.Errorf("got %q want %q", got, want) - } - }) - t.Run("remove scoped to featureFlags section only", func(t *testing.T) { - in := "other:\n- CordonAndDrainMaintenance\nagent:\n featureFlags:\n - CordonAndDrainMaintenance\n - LogPosting\n" - want := "other:\n- CordonAndDrainMaintenance\nagent:\n featureFlags:\n - LogPosting\n" - if got := removeFeatureFlagFromConfig(in, cordonAndDrainFeatureFlag); got != want { - t.Errorf("got %q want %q", got, want) - } - }) - t.Run("clear maintenance mode", func(t *testing.T) { - in := "agent:\n maintenanceMode: CordonAndDrain\n logLevel: info\n" - want := "agent:\n logLevel: info\n" - if got := clearMaintenanceModeFromConfig(in); got != want { - t.Errorf("got %q want %q", got, want) - } - }) +// TestConfigHasFeatureFlag is a regression test for a false-positive in the +// prior membership check, which inferred "flag present" from +// addFeatureFlagToConfig returning its input unchanged. That mutator also +// returns its input unchanged when there is no featureFlags: (or even +// agent:) section to insert into at all, which would misreport an absent +// flag as present. configHasFeatureFlag must not have that false-positive +// path: it only ever returns true when the flag is actually listed. +func TestConfigHasFeatureFlag(t *testing.T) { + tests := []struct { + name string + in string + want bool + }{ + { + name: "present in featureFlags section", + in: "agent:\n featureFlags:\n - CordonAndDrainMaintenance\n - LogPosting\n", + want: true, + }, + { + name: "absent from populated featureFlags section", + in: "agent:\n featureFlags:\n - LogPosting\n", + want: false, + }, + { + name: "no featureFlags or agent section at all: must not false-positive", + in: "other:\n x: y\n", + want: false, + }, + { + name: "agent section present but no featureFlags key: must not false-positive", + in: "agent:\n logLevel: info\n", + want: false, + }, + { + name: "empty config: must not false-positive", + in: "", + want: false, + }, + { + name: "flag in another section is not treated as a match", + in: "other:\n- CordonAndDrainMaintenance\nagent:\n featureFlags:\n - LogPosting\n", + want: false, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := configHasFeatureFlag(tc.in, cordonAndDrainFeatureFlag); got != tc.want { + t.Errorf("configHasFeatureFlag(%q) = %v, want %v", tc.in, got, tc.want) + } + }) + } } // --- Kill --- diff --git a/src/clis/nvcf-cli/internal/clusteragent/maintainer.go b/src/clis/nvcf-cli/internal/clusteragent/maintainer.go index 8ea88b61a..fd92c0d13 100644 --- a/src/clis/nvcf-cli/internal/clusteragent/maintainer.go +++ b/src/clis/nvcf-cli/internal/clusteragent/maintainer.go @@ -23,10 +23,15 @@ import ( ) // AgentMaintainer performs maintenance mutations against a compute-plane -// cluster's NVCA. It is the write-side counterpart to AgentInspector: drain and -// undrain toggle CordonAndDrain maintenance on the NVCA agent-config ConfigMap -// (which the operator picks up on a rollout restart), and the kill operations -// delete ICMSRequest CRs so the NVCA reconciler evicts the workloads. +// cluster's NVCA. It is the write-side counterpart to AgentInspector: drain +// and undrain toggle the CordonAndDrainMaintenance feature gate on the +// NVCFBackend CR (spec.overrides.featureGate.values). The NVCA operator +// treats the agent-config ConfigMap as a fully generated artifact rebuilt +// from that CR on every reconcile, so editing the ConfigMap directly gets +// silently reverted on the operator's next reconcile. Patching the CR lets +// the operator regenerate agent-config correctly and perform its own +// rollout; the kill operations delete ICMSRequest CRs so the NVCA +// reconciler evicts the workloads. // // The Kubernetes implementation (k8s_maintainer.go) mirrors the proven operator // logic in nvca/pkg/operator/cleanup/cleanup.go. The interface is the seam where