Merge pull request #1099 from marun/minimize-prop-status-updates

Minimize updates to propagation status

CHANGELOG.md

@@ -1,4 +1,9 @@
 # Unreleased
+-  [#1099](https://github.com/kubernetes-sigs/kubefed/pull/1099)
+   Updates to propagation status are now only made in response to
+   propagation to member clusters or errors in propagation. Previously
+   propagation status was updated every time a federated resource was
+   reconciled which could result in unnecessary resource consumption.
 -  [#1098](https://github.com/kubernetes-sigs/kubefed/pull/1098)
    Propagated version is now only updated when changed.
 -  [#1087](https://github.com/kubernetes-sigs/kubefed/issues/1087)

charts/kubefed/templates/crds.yaml

@@ -105,10 +105,10 @@ spec:
             conditions:
               items:
                 properties:
-                  lastProbeTime:
+                  lastTransitionTime:
                     format: date-time
                     type: string
-                  lastTransitionTime:
+                  lastUpdateTime:
                     format: date-time
                     type: string
                   reason:
@@ -234,10 +234,10 @@ spec:
             conditions:
               items:
                 properties:
-                  lastProbeTime:
+                  lastTransitionTime:
                     format: date-time
                     type: string
-                  lastTransitionTime:
+                  lastUpdateTime:
                     format: date-time
                     type: string
                   reason:
@@ -365,10 +365,10 @@ spec:
             conditions:
               items:
                 properties:
-                  lastProbeTime:
+                  lastTransitionTime:
                     format: date-time
                     type: string
-                  lastTransitionTime:
+                  lastUpdateTime:
                     format: date-time
                     type: string
                   reason:
@@ -494,10 +494,10 @@ spec:
             conditions:
               items:
                 properties:
-                  lastProbeTime:
+                  lastTransitionTime:
                     format: date-time
                     type: string
-                  lastTransitionTime:
+                  lastUpdateTime:
                     format: date-time
                     type: string
                   reason:
@@ -621,10 +621,10 @@ spec:
             conditions:
               items:
                 properties:
-                  lastProbeTime:
+                  lastTransitionTime:
                     format: date-time
                     type: string
-                  lastTransitionTime:
+                  lastUpdateTime:
                     format: date-time
                     type: string
                   reason:
@@ -750,10 +750,10 @@ spec:
             conditions:
               items:
                 properties:
-                  lastProbeTime:
+                  lastTransitionTime:
                     format: date-time
                     type: string
-                  lastTransitionTime:
+                  lastUpdateTime:
                     format: date-time
                     type: string
                   reason:
@@ -881,10 +881,10 @@ spec:
             conditions:
               items:
                 properties:
-                  lastProbeTime:
+                  lastTransitionTime:
                     format: date-time
                     type: string
-                  lastTransitionTime:
+                  lastUpdateTime:
                     format: date-time
                     type: string
                   reason:
@@ -1008,10 +1008,10 @@ spec:
             conditions:
               items:
                 properties:
-                  lastProbeTime:
+                  lastTransitionTime:
                     format: date-time
                     type: string
-                  lastTransitionTime:
+                  lastUpdateTime:
                     format: date-time
                     type: string
                   reason:
@@ -1137,10 +1137,10 @@ spec:
             conditions:
               items:
                 properties:
-                  lastProbeTime:
+                  lastTransitionTime:
                     format: date-time
                     type: string
-                  lastTransitionTime:
+                  lastUpdateTime:
                     format: date-time
                     type: string
                   reason:
@@ -1266,10 +1266,10 @@ spec:
             conditions:
               items:
                 properties:
-                  lastProbeTime:
+                  lastTransitionTime:
                     format: date-time
                     type: string
-                  lastTransitionTime:
+                  lastUpdateTime:
                     format: date-time
                     type: string
                   reason:

docs/userguide.md

@@ -376,11 +376,13 @@ status:
   conditions:
   - type: Propagation
     status: True
-    lastProbeTime: "2019-05-08T01:23:20Z"
     lastTransitionTime: "2019-05-08T01:23:20Z"
+    lastUpdateTime: "2019-05-08T01:23:20Z"
   # The namespace 'myns' has been verified to exist in the
-  # following clusters as of the lastProbeTime recorded
-  # in the 'Propagation' condition.
+  # following clusters as of the lastUpdateTime recorded
+  # in the 'Propagation' condition. Since that time, no
+  # change has been detected to this resource or the
+  # resources it manages.
   clusters:
   - name: cluster1
   - name: cluster2
@@ -435,8 +437,8 @@ status:
   - type: Propagation
     status: False
     reason: CheckClusters
-    lastProbeTime: "2019-05-08T01:23:20Z"
     lastTransitionTime: "2019-05-08T01:23:20Z"
+    lastUpdateTime: "2019-05-08T01:23:20Z"
   clusters:
   - name: cluster1
   - name: cluster2

pkg/controller/sync/controller.go

@@ -272,10 +272,8 @@ func (s *KubeFedSyncController) reconcile(qualifiedName util.QualifiedName) util
 	}()
 
 	if fedResource.Object().GetDeletionTimestamp() != nil {
-		klog.V(3).Infof("Handling deletion of %s %q", kind, key)
 		return s.ensureDeletion(fedResource)
 	}
-	klog.V(3).Infof("Ensuring finalizer exists on %s %q", kind, key)
 	err = s.ensureFinalizer(fedResource)
 	if err != nil {
 		fedResource.RecordError("EnsureFinalizerError", errors.Wrap(err, "Failed to ensure finalizer"))
@@ -302,7 +300,7 @@ func (s *KubeFedSyncController) syncToClusters(fedResource FederatedResource) ut
 
 	kind := fedResource.TargetKind()
 	key := fedResource.TargetName().String()
-	klog.V(4).Infof("Syncing %s %q in underlying clusters, selected clusters are: %s", kind, key, selectedClusterNames)
+	klog.V(4).Infof("Ensuring %s %q in clusters: %s", kind, key, strings.Join(selectedClusterNames.List(), ","))
 
 	dispatcher := dispatch.NewManagedDispatcher(s.informer.GetClientForCluster, fedResource, s.skipAdoptingResources)
 
@@ -380,12 +378,16 @@ func (s *KubeFedSyncController) syncToClusters(fedResource FederatedResource) ut
 		runtime.HandleError(err)
 	}
 
-	statusMap := dispatcher.StatusMap()
-	return s.setPropagationStatus(fedResource, status.AggregateSuccess, statusMap)
+	collectedStatus := dispatcher.CollectedStatus()
+	return s.setPropagationStatus(fedResource, status.AggregateSuccess, &collectedStatus)
 }
 
 func (s *KubeFedSyncController) setPropagationStatus(fedResource FederatedResource,
-	reason status.AggregateReason, statusMap status.PropagationStatusMap) util.ReconciliationStatus {
+	reason status.AggregateReason, collectedStatus *status.CollectedPropagationStatus) util.ReconciliationStatus {
+
+	if collectedStatus == nil {
+		collectedStatus = &status.CollectedPropagationStatus{}
+	}
 
 	kind := fedResource.FederatedKind()
 	name := fedResource.FederatedName()
@@ -405,8 +407,11 @@ func (s *KubeFedSyncController) setPropagationStatus(fedResource FederatedResour
 	// If the underlying resource has changed, attempt to retrieve and
 	// update it repeatedly.
 	err := wait.PollImmediate(1*time.Second, 5*time.Second, func() (bool, error) {
-		if err := status.SetPropagationStatus(obj, reason, statusMap); err != nil {
+		if updateRequired, err := status.SetPropagationStatus(obj, reason, *collectedStatus); err != nil {
 			return false, errors.Wrapf(err, "failed to set the status")
+		} else if !updateRequired {
+			klog.V(4).Infof("No update necessary for %s %q propagation status", kind, name)
+			return true, nil
 		}
 
 		err := s.hostClusterClient.UpdateStatus(context.TODO(), obj)

pkg/controller/sync/dispatch/managed.go

@@ -58,7 +58,7 @@ type ManagedDispatcher interface {
 	Create(clusterName string)
 	Update(clusterName string, clusterObj *unstructured.Unstructured)
 	VersionMap() map[string]string
-	StatusMap() status.PropagationStatusMap
+	CollectedStatus() status.CollectedPropagationStatus
 
 	RecordClusterError(propStatus status.PropagationStatus, clusterName string, err error)
 	RecordStatus(clusterName string, propStatus status.PropagationStatus)
@@ -73,6 +73,10 @@ type managedDispatcherImpl struct {
 	versionMap            map[string]string
 	statusMap             status.PropagationStatusMap
 	skipAdoptingResources bool
+
+	// Track when resource updates are performed to allow indicating
+	// when a change was last propagated to member clusters.
+	resourcesUpdated bool
 }
 
 func NewManagedDispatcher(clientAccessor clientAccessorFunc, fedResource FederatedResourceForDispatch, skipAdoptingResources bool) ManagedDispatcher {
@@ -213,6 +217,7 @@ func (d *managedDispatcherImpl) Update(clusterName string, clusterObj *unstructu
 		if err != nil {
 			return d.recordOperationError(status.UpdateFailed, clusterName, op, err)
 		}
+		d.setResourcesUpdated()
 		version = util.ObjectVersion(obj)
 		d.recordVersion(clusterName, version)
 		return util.StatusAllOK
@@ -276,12 +281,21 @@ func (d *managedDispatcherImpl) recordVersion(clusterName, version string) {
 	d.versionMap[clusterName] = version
 }
 
-func (d *managedDispatcherImpl) StatusMap() status.PropagationStatusMap {
+func (d *managedDispatcherImpl) setResourcesUpdated() {
+	d.Lock()
+	defer d.Unlock()
+	d.resourcesUpdated = true
+}
+
+func (d *managedDispatcherImpl) CollectedStatus() status.CollectedPropagationStatus {
 	d.RLock()
 	defer d.RUnlock()
 	statusMap := make(status.PropagationStatusMap)
 	for key, value := range d.statusMap {
 		statusMap[key] = value
 	}
-	return statusMap
+	return status.CollectedPropagationStatus{
+		StatusMap:        statusMap,
+		ResourcesUpdated: d.resourcesUpdated,
+	}
 }