From 4099c72289d7fc35e7b09a8d7ad497808f2f7344 Mon Sep 17 00:00:00 2001 From: rohithb Date: Fri, 21 Aug 2026 15:46:51 +0530 Subject: [PATCH 1/4] fix(cli): patch NVCFBackend CR instead of agent-config for cordon-and-drain --- src/clis/nvcf-cli/README.md | 21 +- .../nvcf-cli/cmd/cluster_agent_maintenance.go | 16 +- .../internal/clusteragent/k8s_maintainer.go | 307 +++++++------- .../clusteragent/k8s_maintainer_test.go | 374 +++++++++--------- .../internal/clusteragent/maintainer.go | 13 +- 5 files changed, 353 insertions(+), 378 deletions(-) diff --git a/src/clis/nvcf-cli/README.md b/src/clis/nvcf-cli/README.md index 7d043b4c0..2c0240677 100644 --- a/src/clis/nvcf-cli/README.md +++ b/src/clis/nvcf-cli/README.md @@ -1730,14 +1730,19 @@ 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`) 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); a timeout is +reported as a warning because the CR change is already persisted and +re-running is a no-op. ### How kill works diff --git a/src/clis/nvcf-cli/cmd/cluster_agent_maintenance.go b/src/clis/nvcf-cli/cmd/cluster_agent_maintenance.go index 966900c1b..ddc1353bc 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); waiting for the NVCA operator's rollout\n", orDash(res.Mode)) } else { - fmt.Fprintln(w, " agent-config updated (maintenance cleared); NVCA restart triggered") + fmt.Fprintln(w, " NVCFBackend updated (maintenance cleared); waiting for the NVCA operator's rollout") } 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..d0095f568 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,19 @@ 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" maintenanceModeCordonAndDrain = "CordonAndDrain" - restartedAtAnnotation = "kubectl.kubernetes.io/restartedAt" // Namespace defaults applied when the NVCFBackend CR leaves them empty, // matching DefaultNVCASystemNamespace / DefaultNVCARequestsNamespace upstream. @@ -73,15 +78,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 +92,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 +166,43 @@ 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 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. 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) + 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 +210,52 @@ 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 } -// 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 +264,93 @@ 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 { + // addFeatureFlagToConfig is idempotent: it returns cur unchanged + // exactly when the flag is already present, so this doubles as a + // membership check without a separate parser. + hasFlag := addFeatureFlagToConfig(cur, cordonAndDrainFeatureFlag) == cur + configReady = hasFlag == 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 = true + 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(): @@ -496,79 +544,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..238b2b4b9 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,90 @@ 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 cm.Data[agentConfigKey] + 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("reading deployment back: %v", err) + t.Fatalf("listing NVCFBackend: %v", err) } - return d.Spec.Template.Annotations + 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 spec.overrides.featureGate.values: %v", err) + } + 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 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 +206,132 @@ 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) { - m, _, _ := newFakeMaintainer( - []runtime.Object{defaultBackend()}, - []runtime.Object{nvcaDeployObj(testSystemNS, 1, true)}, +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, ) - _, 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.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 TestDrainNoBackend(t *testing.T) { - m, _, _ := newFakeMaintainer( +func TestUndrainIdempotent(t *testing.T) { + m, dc, _ := newFakeMaintainer( + []runtime.Object{backendObjWithOverrideValues(testBackendNS, testClusterID, testCluster, testSystemNS, testRequestsNS, "LogPosting")}, nil, - []runtime.Object{agentConfigObj(testSystemNS, "agent:\n"), 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.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{agentConfigObj(testSystemNS, cfg), nvcaDeployObj(testSystemNS, 1, true)}, + ) + + 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 TestDrainRolloutTimeoutIsWarningNotError(t *testing.T) { +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" - m, _, cs := newFakeMaintainer( + cfg := "agent:\n featureFlags:\n - LogPosting\n" + m, dc, _ := newFakeMaintainer( []runtime.Object{defaultBackend()}, - // Deployment never reaches the complete state. - []runtime.Object{agentConfigObj(testSystemNS, cfg), nvcaDeployObj(testSystemNS, 1, false)}, + []runtime.Object{agentConfigObj(testSystemNS, cfg), nvcaDeployObj(testSystemNS, 1, true)}, ) res, err := m.Drain(context.Background(), DrainOptions{BackendNS: testBackendNS, Timeout: 10 * time.Millisecond}) @@ -282,16 +341,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 +384,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,60 +475,6 @@ 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) - } - }) -} - // --- Kill --- func killSeed() []runtime.Object { diff --git a/src/clis/nvcf-cli/internal/clusteragent/maintainer.go b/src/clis/nvcf-cli/internal/clusteragent/maintainer.go index 8ea88b61a..55c14d0b4 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 From 6bf08326f8900ae67e8e849b873d177bc5dfebf0 Mon Sep 17 00:00:00 2001 From: rohithb Date: Fri, 21 Aug 2026 19:05:50 +0530 Subject: [PATCH 2/4] fix(cli): address CodeRabbit review findings on cordon-and-drain overrides Signed-off-by: rohithb --- src/clis/nvcf-cli/README.md | 7 +- .../nvcf-cli/cmd/cluster_agent_maintenance.go | 4 +- .../internal/clusteragent/k8s_maintainer.go | 79 +++++++++++- .../clusteragent/k8s_maintainer_test.go | 120 ++++++++++++++++++ .../internal/clusteragent/maintainer.go | 2 +- 5 files changed, 200 insertions(+), 12 deletions(-) diff --git a/src/clis/nvcf-cli/README.md b/src/clis/nvcf-cli/README.md index 2c0240677..350139dc4 100644 --- a/src/clis/nvcf-cli/README.md +++ b/src/clis/nvcf-cli/README.md @@ -1767,9 +1767,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 ddc1353bc..3eb8fb8de 100644 --- a/src/clis/nvcf-cli/cmd/cluster_agent_maintenance.go +++ b/src/clis/nvcf-cli/cmd/cluster_agent_maintenance.go @@ -453,9 +453,9 @@ func printDrainResult(cmd *cobra.Command, res *clusteragent.DrainResult, drain b return } if drain { - fmt.Fprintf(w, " NVCFBackend updated (maintenanceMode=%s); waiting for the NVCA operator's rollout\n", orDash(res.Mode)) + fmt.Fprintf(w, " NVCFBackend updated (maintenanceMode=%s)\n", orDash(res.Mode)) } else { - fmt.Fprintln(w, " NVCFBackend updated (maintenance cleared); waiting for the NVCA operator's rollout") + 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 d0095f568..cece3ec0d 100644 --- a/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer.go +++ b/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer.go @@ -49,6 +49,7 @@ const ( agentConfigKey = "config.yaml" nvcaDeploymentName = "nvca" cordonAndDrainFeatureFlag = "CordonAndDrainMaintenance" + cordonMaintenanceFeatureFlag = "CordonMaintenance" maintenanceModeCordonAndDrain = "CordonAndDrain" // Namespace defaults applied when the NVCFBackend CR leaves them empty, @@ -170,6 +171,10 @@ func (m *k8sMaintainer) setMaintenance(ctx context.Context, opts DrainOptions, d result.Mode = maintenanceModeCordonAndDrain } + if err := m.checkMaintenanceConflict(ctx, opts.BackendNS, drain); err != nil { + return nil, err + } + if opts.DryRun { has, err := m.nvcfBackendHasMaintenanceFlag(ctx, opts.BackendNS) if err != nil { @@ -200,7 +205,12 @@ func (m *k8sMaintainer) setMaintenance(ctx context.Context, opts DrainOptions, d } result.RolloutTriggered = true - if !opts.Force && opts.Timeout > 0 { + 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 @@ -225,6 +235,37 @@ func (m *k8sMaintainer) nvcfBackendHasMaintenanceFlag(ctx context.Context, backe 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 +} + // 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. @@ -322,11 +363,7 @@ func (m *k8sMaintainer) waitForMaintenanceRollout(ctx context.Context, backendNS for { configReady := false if _, cur, err := m.getAgentConfig(ctx, systemNS); err == nil { - // addFeatureFlagToConfig is idempotent: it returns cur unchanged - // exactly when the flag is already present, so this doubles as a - // membership check without a separate parser. - hasFlag := addFeatureFlagToConfig(cur, cordonAndDrainFeatureFlag) == cur - configReady = hasFlag == drain + configReady = configHasFeatureFlag(cur, cordonAndDrainFeatureFlag) == drain } rolloutReady := false @@ -504,6 +541,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") 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 238b2b4b9..23a1f61b0 100644 --- a/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer_test.go +++ b/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer_test.go @@ -130,6 +130,19 @@ func backendObjWithOverrideValues(backendNS, clusterID, clusterName, systemNS, r 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 + } + b.Object["spec"].(map[string]interface{})["featureGate"] = map[string]interface{}{"values": vals} + return b +} + // backendOverrideValues reads spec.overrides.featureGate.values back off the // single NVCFBackend CR in backendNS, the same field patchMaintenanceFeatureFlag // writes. @@ -193,6 +206,36 @@ func TestDrainIdempotent(t *testing.T) { } } +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) { m, dc, _ := newFakeMaintainer( []runtime.Object{backendObjWithOverrideValues(testBackendNS, testClusterID, testCluster, testSystemNS, testRequestsNS, "LogPosting")}, @@ -318,6 +361,30 @@ func TestDrainReportsRolloutCompleteWhenOperatorHasAlreadyReconciled(t *testing. } } +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( + []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}) + 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 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 @@ -475,6 +542,59 @@ func TestAddFeatureFlagToConfig(t *testing.T) { } } +// 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 --- func killSeed() []runtime.Object { diff --git a/src/clis/nvcf-cli/internal/clusteragent/maintainer.go b/src/clis/nvcf-cli/internal/clusteragent/maintainer.go index 55c14d0b4..fd92c0d13 100644 --- a/src/clis/nvcf-cli/internal/clusteragent/maintainer.go +++ b/src/clis/nvcf-cli/internal/clusteragent/maintainer.go @@ -25,7 +25,7 @@ import ( // AgentMaintainer performs maintenance mutations against a compute-plane // 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 +// 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 From d77b818744d48b2f80681a203099ad720ecd68e0 Mon Sep 17 00:00:00 2001 From: rohithb Date: Fri, 21 Aug 2026 19:24:47 +0530 Subject: [PATCH 3/4] fix(cli): treat missing nvca Deployment as rollout not ready and document --timeout 0 Signed-off-by: rohithb --- src/clis/nvcf-cli/README.md | 14 ++++++----- .../internal/clusteragent/k8s_maintainer.go | 2 +- .../clusteragent/k8s_maintainer_test.go | 24 +++++++++++++++++++ 3 files changed, 33 insertions(+), 7 deletions(-) diff --git a/src/clis/nvcf-cli/README.md b/src/clis/nvcf-cli/README.md index 350139dc4..be7e99ba7 100644 --- a/src/clis/nvcf-cli/README.md +++ b/src/clis/nvcf-cli/README.md @@ -1737,12 +1737,14 @@ 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`) 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); a timeout is -reported as a warning because the CR change is already persisted and -re-running is a no-op. +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. ### How kill works diff --git a/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer.go b/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer.go index cece3ec0d..e55f7733b 100644 --- a/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer.go +++ b/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer.go @@ -370,7 +370,7 @@ func (m *k8sMaintainer) waitForMaintenanceRollout(ctx context.Context, backendNS deploy, err := m.cs.AppsV1().Deployments(systemNS).Get(ctx, nvcaDeploymentName, metav1.GetOptions{}) switch { case apierrors.IsNotFound(err): - rolloutReady = true + rolloutReady = false case err == nil: desired := int32(1) if deploy.Spec.Replicas != nil { 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 23a1f61b0..ca9668247 100644 --- a/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer_test.go +++ b/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer_test.go @@ -385,6 +385,30 @@ func TestDrainDoesNotFalselyReportCompleteWhenConfigHasNoFeatureFlagsSection(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 featureFlags:\n - " + cordonAndDrainFeatureFlag + "\n" + m, _, _ := newFakeMaintainer( + []runtime.Object{defaultBackend()}, + []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 From 612213dd5e8f28597580e5dc36c24a9606404807 Mon Sep 17 00:00:00 2001 From: rohithb Date: Tue, 25 Aug 2026 01:25:34 +0530 Subject: [PATCH 4/4] docs(cli): call out that --force no longer retriggers a stuck rollout Signed-off-by: rohithb --- src/clis/nvcf-cli/README.md | 8 ++++++++ .../nvcf-cli/internal/clusteragent/k8s_maintainer.go | 9 +++++++++ 2 files changed, 17 insertions(+) diff --git a/src/clis/nvcf-cli/README.md b/src/clis/nvcf-cli/README.md index be7e99ba7..d32769998 100644 --- a/src/clis/nvcf-cli/README.md +++ b/src/clis/nvcf-cli/README.md @@ -1746,6 +1746,14 @@ 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 `kill-function` and `kill-all` delete the matching `ICMSRequest` CRs; the NVCA diff --git a/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer.go b/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer.go index e55f7733b..3b873044d 100644 --- a/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer.go +++ b/src/clis/nvcf-cli/internal/clusteragent/k8s_maintainer.go @@ -200,6 +200,15 @@ func (m *k8sMaintainer) setMaintenance(ctx context.Context, opts DrainOptions, d // 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 }