BUG: a label key removed from spec.classifierLabels is never removed from still-matching clusters

[maikelvl]

Problem Description

#437 made the Classifier controller remove the labels it manages in two cases: the cluster stops matching, and the Classifier is deleted.
A third case is still uncovered: the Classifier exists, the cluster still matches, and a key is deleted from spec.classifierLabels.
That label is never removed from the cluster, and after the next reconcile no Classifier owns it any more, so nothing can remove it later either.

Steps to reproduce

1. Apply a Classifier with two labels:

apiVersion: lib.projectsveltos.io/v1beta1
kind: Classifier
metadata:
  name: repro
spec:
  classifierLabels:
  - key: repro-alpha
    value: v1
  - key: repro-beta
    value: v1
  deployedResourceConstraint:
    resourceSelectors:
    - group: ""
      version: v1
      kind: Namespace
      name: default

Every cluster has a default namespace, so every classified cluster matches, and it keeps matching for the whole sequence.
2. Wait for both labels to land on the cluster.
Both also appear in status.machingClusterStatuses[].managedLabels.
3. Remove the repro-beta entry from spec.classifierLabels.
Leave the constraint alone, so the cluster keeps matching.
4. Delete the Classifier.

Observed: after step 3, repro-beta: v1 is still on the cluster, while managedLabels and the ClassifierReport status drop to repro-alpha alone.
The controller has stopped accounting for a label it put there.
After step 4, repro-alpha is gone and repro-beta is still there, because the delete path also works off the current spec.
The label outlives the Classifier that created it and has to be deleted by hand.

Expected: repro-beta goes at step 3, the way it would if the cluster had stopped matching.

That sequence is the minimal form of the case described under "Why this bites" below, which is where the observed behaviour comes from.

Code path

Line numbers are v1.15.0, unchanged on main at 3169445.

• updateLabelsOnCluster (controllers/classifier_controller.go:669, loop at :679) writes every key in spec.classifierLabels and never diffs against the keys it wrote on the previous pass, so a dropped key is simply not written.
• removeLabelsFromCluster (:630, loop at :646) picks its removal candidates from spec.classifierLabels, so a key absent from the spec can never be a candidate.
  This is the function behind both covered cases: cleanUpNonMatchingClusters (:853) and reconcileDelete (:202, via removeLabelsFromClusters :754).
• registerMatchingClusters (:829) calls keymanager.RemoveStaleRegistrations (controllers/keymanager/keymanager.go:154, cleanRegistrations at :170) for each matching cluster, which drops this Classifier's ownership of any key no longer in the spec.
  That is what makes it unrecoverable rather than merely missed.
  The label is left with no owner, CanManageLabel returns false for the key, and while the key stays out of the spec no later pass can claim it back in order to remove it.

The controller already persists what it needs to close this.
ClassifierReport.Status.ManagedLabels holds the keys managed on the previous pass, and cleanUpManagedResources (:547) already reads those reports to build the old cluster set.
Diffing that set against the current spec keys for still-matching clusters, and removing the difference before RemoveStaleRegistrations clears the registrations, would cover the case with the existing bookkeeping.

ManagementClusterClassifier

Affected, and for the same reason.

• reconcileNormal (controllers/mgmtcluster_classifier_controller.go:97) calls removeStaleClusterLabels (:366), which only walks clusters present in oldClusters and absent from newClusters.
  A still-matching cluster never enters that loop.
• For a still-matching cluster it calls km.RemoveStaleRegistrations (:150) and then applyLabelsToCluster (controllers/mgmtcluster_classifier_utils.go:410), which only adds or updates the keys in the spec.
• reconcileDelete (:219) builds keysToRemove from mcc.Spec.ClassifierLabels, so a key dropped from the spec survives deletion of the ManagementClusterClassifier too.

Test coverage

• test/fv/labels_test.go verifyFlow covers stops-matching, matches-again and Classifier-deleted.
• test/fv/mgmtcluster_classifier_test.go updates classifierLabels but changes only the values, keeping both keys.

No case removes a key from the spec while the cluster still matches.

Why this bites

On a management cluster, three keys were commented out of a Classifier's spec.classifierLabels to switch those addons off.
The labels stayed on the SveltosCluster registration.
So the ClusterProfiles whose selectors named those keys kept matching, and nothing acted on that for ten days.
Then a Sveltos upgrade restarted the addon-controller, it re-evaluated matching clusters, and it deployed one of the addons that had been switched off in git.
That meant a new ClusterSummary and a live Helm release for a profile nobody had touched in ten days.
The upgrade is incidental.
Any restart of the controller would have done the same.

That makes a stale managed label more than untidy metadata.
Where a ClusterProfile selector names the key, the label is a deploy waiting for the next restart of the controller.

sveltosctl show classifier-labels already names the owning instance per key, so ownership is tracked.
Releasing it when a key leaves the spec is the part that is missing.

System Information

CLUSTERAPI VERSION: n/a, SveltosCluster registration
SVELTOS VERSION: code paths read at v1.15.0 and at main 3169445; first seen on a cluster running classifier v1.5.1, which already carries the #437 fix
KUBERNETES VERSION: v1.36.4+k3s1

Logs

Nothing controller-side to attach.
The cluster's labels, Classifier.status.machingClusterStatuses[].managedLabels and the ClassifierReport status show it between them.

[gianlucam76]

Thank you @maikelvl