fix: stuck serviceset reconcile

[BROngineer]

What this PR does / why we need it:
This PR fixes how the serviceset controller handles the serviceset reconciliation and re-queuing. So that the serviceset will be reconciled until it enters the state confirmed by corresponding sveltos objects - clustersummaries, clusterconfigurations, etc.
Aside from that this PR fixes the stale service status which was not properly updated in case there were no applicable health rules found.

[Copilot]

Pull request overview

This PR updates the Sveltos ServiceSet controller’s reconcile/poller behavior to avoid getting “stuck” when health rules are absent and to ensure the status “stamp” (hash + version) continues to advance so dependency gating can make progress.

Changes:

• Distinguishes “no health rules configured” from “all rules valid” via a dedicated ServiceSet condition reason (NoRulesConfigured).
• Decouples stamping (hash/version convergence) from on-cluster health verification so Status.Version doesn’t freeze when rules are missing.
• Updates the poller quiescence logic to require both health completion and version-stamp convergence before stopping requeues; adds regression tests.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
internal/controller/adapters/sveltos/verify.go	Extends the ServiceSet health-rules condition to distinguish “no rules configured” from “all valid”.
internal/controller/adapters/sveltos/verify_test.go	Updates unit tests for the new condition behavior and signature.
internal/controller/adapters/sveltos/serviceset_controller.go	Adjusts verifier/stamping flow so stamping can proceed even when no rules are configured.
internal/controller/adapters/sveltos/serviceset_controller_test.go	Adds integration regressions covering stamping behavior when rules are absent.
internal/controller/adapters/sveltos/enqueue.go	Changes poller quiescence to require stamp convergence, preventing “green but frozen” states.
internal/controller/adapters/sveltos/enqueue_test.go	Adds unit regressions for quiescence and stamp convergence logic.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[wahabmk]

4. The stamp binds spec.Version to a hash computed from possibly-stale sveltos artifacts

Reconcile runs ensureProfile (writes the new chart version into the Profile) → collectServiceStatuses → verifyServiceStates, which reads a ClusterSummary sveltos hasn't refreshed yet. The stamp guard is currentHash != s.LastDeployedHash, so when LastDeployedHash is empty that's trivially true against the old chart — and s.Version is set to the new spec.Version.

That matters more after this PR than before it: LastDeployedHash is "" for every service on a no-rules cluster today, so the first reconcile after this ships is exactly that window. If a version bump is in flight, the stamp records the new version, stampConverged goes true, the poller quiesces, and FilterServiceDependencies releases dependents on a version not yet on cluster. It self-heals on the next CS RV bump, but the dependents are already unlocked.

computeServiceHash already has chart.ChartVersion in hand (verify.go:1309). Stamping that, or gating the stamp on chart.ChartVersion == *spec.Version, would close this properly.

[wahabmk]

1. getChildClient moved inside the per-service loop — serviceset_controller.go:425

It used to be called once per reconcile, before the loop. It now runs once per Helm service. Each call is a Secret GET plus clientcmd.RESTConfigFromKubeConfig plus client.New (internal/util/kube/client.go:34) — the shared RESTMapper cache saves the discovery round-trip, but not the rest. A ServiceSet with 10 Helm services now builds 10 child clients per reconcile. Hoist it above the loop under if len(rules) > 0.

That also fixes a secondary issue: an error there now returns false, err from mid-loop, discarding accumulated errs, any pendingStamp already set for earlier services, and the updateServicesInReadyStateCondition call — after some services have already had State/Conditions mutated.

[BROngineer]

To avoid possible runtime errs, since we do not check the client interface, and to revert the invalid fix with moving getting client under the for loop, now it's under the closure: it's safe in runtime because we build the closure and use it under the same condition if len(rules) > 0

[wahabmk]

2. specVersions is written with a defaulted key and read with a raw one — serviceset_controller.go:375 vs :478

specVersions[serviceset.ServiceKey(svc.Namespace, svc.Name)] = svc.Version  // ""  → "default"
...
serviceKey := client.ObjectKey{Namespace: s.Namespace, Name: s.Name}        // ""  stays ""
s.Version = specVersions[serviceKey]

The PR changed line 375 to ServiceKey but left the lookup raw. For a service with an empty Namespace the write lands under default/<name> and the read misses, so the stamp writes nil. versionsEqual(specVersion, nil) is then false forever, stampConverged never returns true, and the poller never quiesces — the exact failure this PR fixes.

Note serviceHashes (line 396) is keyed raw from rs.ReleaseNamespace, so the two keys genuinely need to differ; the clean fix is a separate specKey := serviceset.ServiceKey(s.Namespace, s.Name) for line 478 only.

[BROngineer]

Same as in comment below - empty service namespace is not reachable for serviceset, because services added to serviceset passes through the makeService + effectiveNamespace functions.
Will update for consistency tho

[wahabmk]

3. New unbounded 10s requeue loop in the default configuration — serviceset_controller.go:450-453

} else if s.State == kcmv1.ServiceStateDeployed && currentHash == "" {
    pendingStamp = true
}

pendingStamp drives RequeueAfter: r.requeueInterval (10s, fixed, no back-off) at line 275. Where the hash can never be computed, that requeues forever. The concrete case is again empty-namespace services: getHelmCharts defaults ReleaseNamespace to svc.Name (line 1230) while Status.Services[i].Namespace stays "", so the serviceHashes lookup can never match. Pre-PR, no-rules clusters returned early and never reached this; now every such ServiceSet spins at 10s indefinitely. Worth either bounding the requeue or logging once when the hash is unresolvable.

[BROngineer]

Services added to ServiceSet are produced by the makeService function:

func makeService(s kcmv1.Service, version, template string) kcmv1.ServiceWithValues {
	return kcmv1.ServiceWithValues{
		Name: s.Name,
		// We should always use effective namespace, because service namespace in
		// serviceSet's service definition is never empty while service namespace
		// in clusterDeployment's/multiClusterService's service definition can be empty.
		// This will lead to persistent discrepancy between service definitions and
		// lead to continuous serviceSet updates.
		Namespace:   effectiveNamespace(s.Namespace),
		Version:     new(version),
		Template:    template,
		Values:      s.Values,
		ValuesFrom:  s.ValuesFrom,
		HelmOptions: s.HelmOptions,
		HelmAction:  s.HelmAction,
	}
}

which in turn sets the effective namespace instead of just copying the whatever namespace is defined in CLD/MCS spec:

func effectiveNamespace(serviceNamespace string) string {
	if serviceNamespace == "" {
		return metav1.NamespaceDefault
	}
	return serviceNamespace
}

Thus the state where service namespace in spec is empty is not reachable

[josef-hak]

@BROngineer, I tested the PR using ksm test and it failed: https://github.com/josef-hak/k0rdent-ksm-tests/actions/runs/34253626643/job/102154143834 . It's probably because the feature branch is not rebased with main. Could you please rebase so we can ensure it's not a regression?