Simplify condition handling

While implementing managed labels, I found two problems with our
condition handling:

1. They were using old terminology like "Critical Condition" instead of
the newer "ActivitiesHalted".
2. Instead of identifying problems like cycles right away, we delayed
their identification until the end of the syncing process. This means
that code that depends on knowing about cycles - like setting tree
labels - needed to be at the end of the sync process instead of in the
middle where they belonged (and couldn't set conditions themselves).

This change fixes both problems.

Tested: all existing unit/integ tests; no change in functionality.

Author: adrianludwin

internal/forest/namespaceconditions.go

@@ -1,36 +1,47 @@
 package forest
 
 import (
+	"fmt"
+
 	api "sigs.k8s.io/hierarchical-namespaces/api/v1alpha2"
 )
 
-// HasLocalCritCondition returns if the namespace itself has any local critical conditions, ignoring
-// its ancestors. Any code with the "Crit" prefix is a critical condition.
-func (ns *Namespace) HasLocalCritCondition() bool {
+// IsHalted returns true if this namespace has an ActivitiesHalted condition for any reason *other*
+// than one of its ancestors also being halted.
+func (ns *Namespace) IsHalted() bool {
+	if ns == nil {
+		return false
+	}
 	for _, cond := range ns.conditions {
-		if cond.Type == api.ConditionActivitiesHalted && cond.Reason != api.ReasonAncestor {
+		if cond.Type == api.ConditionActivitiesHalted {
 			return true
 		}
 	}
 	return false
 }
 
-// GetCritAncestor returns the name of the first ancestor with a critical condition, or the empty
-// string if there are no such ancestors. It *can* return the name of the current namespace.
-func (ns *Namespace) GetCritAncestor() string {
-	if ns.HasLocalCritCondition() {
-		return ns.name
-	}
-	if ns.Parent() == nil {
+// GetHaltedRoot returns the name of the lowest subtree that's halted for any reason *other* than a
+// higher ancestor being halted. This can be the name of the current namespace, or the empty string
+// if there are no halted ancestors.
+func (ns *Namespace) GetHaltedRoot() string {
+	if ns == nil {
 		return ""
 	}
-	return ns.Parent().GetCritAncestor()
+	if ns.IsHalted() {
+		return ns.name
+	}
+	return ns.Parent().GetHaltedRoot()
 }
 
 // SetCondition adds a new condition to the current condition list.
+//
+// Any condition with ReasonAncestor is ignored; this is added dynamically when calling
+// Conditions().
 func (ns *Namespace) SetCondition(tp, reason, msg string) {
-	oldCond := ns.conditions
-	ns.conditions = append(oldCond, api.NewCondition(tp, reason, msg))
+	if reason == api.ReasonAncestor {
+		return
+	}
+	ns.conditions = append(ns.conditions, api.NewCondition(tp, reason, msg))
 }
 
 // ClearConditions set conditions to nil.
@@ -40,5 +51,11 @@ func (ns *Namespace) ClearConditions() {
 
 // Conditions returns a full list of the conditions in the namespace.
 func (ns *Namespace) Conditions() []api.Condition {
+	if root := ns.GetHaltedRoot(); root != "" && root != ns.name {
+		msg := fmt.Sprintf("Propagation paused in %q and its descendants due to ActivitiesHalted condition on ancestor %q", ns.name, root)
+		ret := []api.Condition{api.NewCondition(api.ConditionActivitiesHalted, api.ReasonAncestor, msg)}
+		return append(ret, ns.conditions...)
+	}
+
 	return ns.conditions
 }

internal/hierarchyconfig/reconciler.go

@@ -290,11 +290,15 @@ func (r *Reconciler) syncWithForest(log logr.Logger, nsInst *corev1.Namespace, i
 	defer r.Forest.Unlock()
 	ns := r.Forest.Get(inst.ObjectMeta.Namespace)
 
-	// Clear locally-set conditions in the forest; we'll re-add them if they're still relevant. But
-	// first, record whether there were any critical ones since if this changes, we'll need to notify
+	// Clear all conditions; we'll re-add them if they're still relevant. But first, record whether
+	// this namespace (excluding ancestors) was halted, since if that changes, we'll need to notify
 	// other namespaces.
-	hadCrit := ns.HasLocalCritCondition()
+	wasHalted := ns.IsHalted()
 	ns.ClearConditions()
+	// We can figure out this condition pretty easily...
+	if deletingCRD {
+		ns.SetCondition(api.ConditionActivitiesHalted, api.ReasonDeletingCRD, "The HierarchyConfiguration CRD is being deleted; all propagation is disabled.")
+	}
 
 	// Set external tree labels in the forest if this is an external namespace.
 	r.syncExternalNamespace(log, nsInst, ns)
@@ -306,6 +310,9 @@ func (r *Reconciler) syncWithForest(log logr.Logger, nsInst *corev1.Namespace, i
 	r.syncParent(log, inst, ns)
 	initial := r.markExisting(log, ns)
 
+	// Sync labels and annotations, now that the structure's been updated.
+	nsCustomerLabelUpdated := r.syncTreeLabels(log, nsInst, ns)
+
 	// Sync other spec and spec-like info
 	r.syncAnchors(log, ns, anms)
 	if ns.UpdateAllowCascadingDeletion(inst.Spec.AllowCascadingDeletion) {
@@ -315,11 +322,9 @@ func (r *Reconciler) syncWithForest(log logr.Logger, nsInst *corev1.Namespace, i
 
 	// Sync the status
 	inst.Status.Children = ns.ChildNames()
-	r.syncConditions(log, inst, ns, deletingCRD, hadCrit)
-
-	// Sync the tree labels. This should go last since it can depend on the conditions.
-	nsCustomerLabelUpdated := r.syncTreeLabels(log, nsInst, ns)
+	r.syncConditions(log, inst, ns, wasHalted)
 
+	r.HNCConfigReconciler.Enqueue("namespace reconciled")
 	return initial || nsCustomerLabelUpdated
 }
 
@@ -431,6 +436,9 @@ func (r *Reconciler) markExisting(log logr.Logger, ns *forest.Namespace) bool {
 }
 
 func (r *Reconciler) syncParent(log logr.Logger, inst *api.HierarchyConfiguration, ns *forest.Namespace) {
+	// As soon as the structure has been updated, do a cycle check.
+	defer r.setCycleCondition(log, ns)
+
 	if ns.IsExternal() {
 		ns.SetParent(nil)
 		return
@@ -459,10 +467,10 @@ func (r *Reconciler) syncParent(log logr.Logger, inst *api.HierarchyConfiguratio
 	// Change the parent.
 	ns.SetParent(curParent)
 
-	// Finally, enqueue all other namespaces that could be directly affected. The old and new parents
-	// have just gained/lost a child, while the descendants need to have their tree labels updated and
-	// their objects resynced. Note that it's fine if oldParent or curParent is nil - see
-	// enqueueAffected for details.
+	// Enqueue all other namespaces that could be directly affected. The old and new parents have just
+	// gained/lost a child, while the descendants need to have their tree labels updated and their
+	// objects resynced. Note that it's fine if oldParent or curParent is nil - see enqueueAffected
+	// for details.
 	//
 	// If we've just created a cycle, all the members of that cycle will be listed as the descendants,
 	// so enqueuing them will ensure that the conditions show up in all members of the cycle.
@@ -471,6 +479,17 @@ func (r *Reconciler) syncParent(log logr.Logger, inst *api.HierarchyConfiguratio
 	r.enqueueAffected(log, "subtree root has changed", ns.DescendantNames()...)
 }
 
+func (r *Reconciler) setCycleCondition(log logr.Logger, ns *forest.Namespace) {
+	cycle := ns.CycleNames()
+	if cycle == nil {
+		return
+	}
+
+	msg := fmt.Sprintf("Namespace is a member of the cycle: %s", strings.Join(cycle, " <- "))
+	log.Info(msg)
+	ns.SetCondition(api.ConditionActivitiesHalted, api.ReasonInCycle, msg)
+}
+
 // syncAnchors updates the anchor list. If any anchor is created/deleted, it will enqueue
 // the child to update its SubnamespaceAnchorMissing condition. A modified anchor will appear
 // twice in the change list (one in deleted, one in created), both subnamespaces
@@ -498,27 +517,27 @@ func (r *Reconciler) syncTreeLabels(log logr.Logger, nsInst *corev1.Namespace, n
 	// Look for all ancestors. Stop as soon as we find a namespaces that has a critical condition in
 	// the forest (note that AncestorHaltActivities is never included in the forest). This should handle orphans
 	// and cycles.
-	anc := ns
+	curNS := ns
 	depth := 0
-	for anc != nil {
-		l := anc.Name() + api.LabelTreeDepthSuffix
+	for curNS != nil {
+		l := curNS.Name() + api.LabelTreeDepthSuffix
 		metadata.SetLabel(nsInst, l, strconv.Itoa(depth))
-		if anc.HasLocalCritCondition() {
+		if curNS.IsHalted() {
 			break
 		}
 
 		// If the root is an external namespace, add all its external tree labels too.
 		// Note it's impossible to have an external namespace as a non-root, which is
 		// enforced by both admission controllers and the reconciler here.
-		if anc.IsExternal() {
-			for k, v := range anc.ExternalTreeLabels {
+		if curNS.IsExternal() {
+			for k, v := range curNS.ExternalTreeLabels {
 				l = k + api.LabelTreeDepthSuffix
 				metadata.SetLabel(nsInst, l, strconv.Itoa(depth+v))
 			}
 			break
 		}
 
-		anc = anc.Parent()
+		curNS = curNS.Parent()
 		depth++
 	}
 	// Update the labels in the forest so that we can quickly access the labels and
@@ -530,61 +549,22 @@ func (r *Reconciler) syncTreeLabels(log logr.Logger, nsInst *corev1.Namespace, n
 	return false
 }
 
-func (r *Reconciler) syncConditions(log logr.Logger, inst *api.HierarchyConfiguration, ns *forest.Namespace, deletingCRD, hadCrit bool) {
-	// Sync critical conditions after all locally-set conditions are updated.
-	r.syncCritConditions(log, ns, deletingCRD, hadCrit)
+func (r *Reconciler) syncConditions(log logr.Logger, inst *api.HierarchyConfiguration, ns *forest.Namespace, wasHalted bool) {
+	// If the halted status has changed, notify
+	if ns.IsHalted() != wasHalted {
+		msg := ""
+		if wasHalted {
+			log.Info("ActivitiesHalted condition removed")
+			msg = "removed"
+		} else {
+			log.Info("Setting ActivitiesHalted on namespace", "conditions", ns.Conditions())
+			msg = "added"
+		}
+		r.enqueueAffected(log, "descendant of a namespace with ActivitiesHalted "+msg, ns.DescendantNames()...)
+	}
 
 	// Convert and pass in-memory conditions to HierarchyConfiguration object.
 	inst.Status.Conditions = ns.Conditions()
-	setCritAncestorCondition(log, inst, ns)
-	r.HNCConfigReconciler.Enqueue("namespace reconciled")
-}
-
-// syncCritConditions enqueues the children of a namespace if the existing critical conditions in the
-// namespace are gone or critical conditions are newly found.
-func (r *Reconciler) syncCritConditions(log logr.Logger, ns *forest.Namespace, deletingCRD, hadCrit bool) {
-	// If we're in a cycle, determine that now
-	if cycle := ns.CycleNames(); cycle != nil {
-		msg := fmt.Sprintf("Namespace is a member of the cycle: %s", strings.Join(cycle, " <- "))
-		ns.SetCondition(api.ConditionActivitiesHalted, api.ReasonInCycle, msg)
-	}
-
-	if deletingCRD {
-		ns.SetCondition(api.ConditionActivitiesHalted, api.ReasonDeletingCRD, "The HierarchyConfiguration CRD is being deleted; all syncing is disabled.")
-	}
-
-	// Early exit if nothing's changed and there's no need to enqueue relatives.
-	if hadCrit == ns.HasLocalCritCondition() {
-		return
-	}
-
-	msg := ""
-	if hadCrit {
-		log.Info("ActivitiesHalted condition removed")
-		msg = "removed"
-	} else {
-		log.Info("Setting ActivitiesHalted on namespace", "conditions", ns.Conditions())
-		msg = "added"
-	}
-	r.enqueueAffected(log, "descendant of a namespace with ActivitiesHalted "+msg, ns.DescendantNames()...)
-}
-
-func setCritAncestorCondition(log logr.Logger, inst *api.HierarchyConfiguration, ns *forest.Namespace) {
-	if ns.HasLocalCritCondition() {
-		return
-	}
-	ans := ns.Parent()
-	for ans != nil {
-		if !ans.HasLocalCritCondition() {
-			ans = ans.Parent()
-			continue
-		}
-		log.Info("Ancestor has a ActivitiesHalted condition", "ancestor", ans.Name())
-		msg := fmt.Sprintf("Propagation paused in the subtree of %q due to ActivitiesHalted condition", ans.Name())
-		condition := api.NewCondition(api.ConditionActivitiesHalted, api.ReasonAncestor, msg)
-		inst.Status.Conditions = append(inst.Status.Conditions, condition)
-		return
-	}
 }
 
 // enqueueAffected enqueues all affected namespaces for later reconciliation. This occurs in a

internal/hierarchyconfig/validator.go

@@ -175,11 +175,11 @@ func (v *Validator) checkNS(ns *forest.Namespace) admission.Response {
 		return deny(metav1.StatusReasonServiceUnavailable, msg)
 	}
 
-	// Deny the request if the namespace has a critical ancestor - but not if it's critical itself,
-	// since we may be trying to resolve the critical condition.
-	critAnc := ns.GetCritAncestor()
-	if critAnc != "" && critAnc != ns.Name() {
-		msg := fmt.Sprintf("The ancestor %q of namespace %q has a critical condition, which must be resolved before any changes can be made to the hierarchy configuration.", critAnc, ns.Name())
+	// Deny the request if the namespace has a halted root - but not if it's halted itself, since we
+	// may be trying to resolve the halted condition.
+	haltedRoot := ns.GetHaltedRoot()
+	if haltedRoot != "" && haltedRoot != ns.Name() {
+		msg := fmt.Sprintf("The ancestor %q of namespace %q has a critical condition, which must be resolved before any changes can be made to the hierarchy configuration.", haltedRoot, ns.Name())
 		return deny(metav1.StatusReasonForbidden, msg)
 	}
 

internal/objects/reconciler.go

@@ -256,8 +256,8 @@ func (r *Reconciler) syncWithForest(ctx context.Context, log logr.Logger, inst *
 	// what we'd _like_ to do. We still needed to sync the forest since we want to know when objects
 	// are added and removed, so we can sync them properly if the critical condition is resolved, but
 	// don't do anything else for now.
-	if ca := r.Forest.Get(inst.GetNamespace()).GetCritAncestor(); ca != "" {
-		log.Info("Namespace has 'ActivitiesHalted' condition; will not touch propagated object", "affectedNamespace", ca, "suppressedAction", action)
+	if halted := r.Forest.Get(inst.GetNamespace()).GetHaltedRoot(); halted != "" {
+		log.Info("Namespace has 'ActivitiesHalted' condition; will not touch propagated object", "haltedRoot", halted, "suppressedAction", action)
 		return actionNop, nil
 	}
 
@@ -478,10 +478,10 @@ func cleanSource(src *unstructured.Unstructured) *unstructured.Unstructured {
 
 func (r *Reconciler) enqueueDescendants(ctx context.Context, log logr.Logger, src *unstructured.Unstructured, reason string) {
 	sns := r.Forest.Get(src.GetNamespace())
-	if ca := sns.GetCritAncestor(); ca != "" {
-		// There's no point enqueuing anything if the source namespace has a crit condition since we'll
-		// just skip any action anyway.
-		log.V(1).Info("Will not enqueue descendants due to crit ancestor", "critAncestor", ca, "oldReason", reason)
+	if halted := sns.GetHaltedRoot(); halted != "" {
+		// There's no point enqueuing anything if the source namespace is halted since we'll just skip
+		// any action anyway.
+		log.V(1).Info("Will not enqueue descendants since activities are halted", "haltedRoot", halted, "oldReason", reason)
 		return
 	}
 	log.V(1).Info("Enqueuing descendant objects", "reason", reason)