Refactor the Configuration controller to use level-based reconciliation.

pkg/apis/serving/v1alpha1/configuration_types.go

@@ -224,10 +224,12 @@ func (cs *ConfigurationStatus) InitializeConditions() {
 
 func (cs *ConfigurationStatus) SetLatestCreatedRevisionName(name string) {
 	cs.LatestCreatedRevisionName = name
-	cs.setCondition(&ConfigurationCondition{
-		Type:   ConfigurationConditionLatestRevisionReady,
-		Status: corev1.ConditionUnknown,
-	})
+	if cs.LatestReadyRevisionName != name {
+		cs.setCondition(&ConfigurationCondition{
+			Type:   ConfigurationConditionLatestRevisionReady,
+			Status: corev1.ConditionUnknown,
+		})
+	}
 }
 
 func (cs *ConfigurationStatus) SetLatestReadyRevisionName(name string) {

pkg/apis/serving/v1alpha1/configuration_types_test.go

@@ -271,6 +271,12 @@ func TestTypicalFlow(t *testing.T) {
 	checkConditionSucceededConfiguration(r.Status, ConfigurationConditionLatestRevisionReady, t)
 	checkConditionSucceededConfiguration(r.Status, ConfigurationConditionReady, t)
 
+	// Verify a second call to SetLatestCreatedRevisionName doesn't change the status from Ready
+	// e.g. on a subsequent reconciliation.
+	r.Status.SetLatestCreatedRevisionName("foo")
+	checkConditionSucceededConfiguration(r.Status, ConfigurationConditionLatestRevisionReady, t)
+	checkConditionSucceededConfiguration(r.Status, ConfigurationConditionReady, t)
+
 	r.Status.SetLatestCreatedRevisionName("bar")
 	checkConditionOngoingConfiguration(r.Status, ConfigurationConditionLatestRevisionReady, t)
 	checkConditionSucceededConfiguration(r.Status, ConfigurationConditionReady, t)

pkg/apis/serving/v1alpha1/register_test.go

@@ -27,6 +27,10 @@ func TestRegisterHelpers(t *testing.T) {
 		t.Errorf("Resource(Revision) = %v, want %v", got.String(), want)
 	}
 
+	if got, want := SchemeGroupVersion.String(), "serving.knative.dev/v1alpha1"; got != want {
+		t.Errorf("SchemeGroupVersion() = %v, want %v", got, want)
+	}
+
 	scheme := runtime.NewScheme()
 	if err := addKnownTypes(scheme); err != nil {
 		t.Errorf("addKnownTypes() = %v", err)

pkg/controller/configuration/configuration.go

@@ -45,7 +45,8 @@ type Controller struct {
 	buildClientSet buildclientset.Interface
 
 	// lister indexes properties about Configuration
-	lister listers.ConfigurationLister
+	lister         listers.ConfigurationLister
+	revisionLister listers.RevisionLister
 }
 
 // NewController creates a new Configuration controller
@@ -66,6 +67,7 @@ func NewController(
 		Base:           controller.NewBase(opt, controllerAgentName, "Configurations", informers),
 		buildClientSet: buildClientSet,
 		lister:         informer.Lister(),
+		revisionLister: revisionInformer.Lister(),
 	}
 
 	controller.Logger.Info("Setting up event handlers")
@@ -77,12 +79,21 @@ func NewController(
 		DeleteFunc: controller.Enqueue,
 	})
 
-	revisionInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{
-		AddFunc: func(obj interface{}) {
-			controller.SyncRevision(obj.(*v1alpha1.Revision))
+	revisionInformer.Informer().AddEventHandler(cache.FilteringResourceEventHandler{
+		FilterFunc: func(obj interface{}) bool {
+			if object, ok := obj.(metav1.Object); ok {
+				owner := metav1.GetControllerOf(object)
+				return owner != nil &&
+					owner.APIVersion == v1alpha1.SchemeGroupVersion.String() &&
+					owner.Kind == "Configuration"
+			}
+			return false
 		},
-		UpdateFunc: func(old, new interface{}) {
-			controller.SyncRevision(new.(*v1alpha1.Revision))
+		Handler: cache.ResourceEventHandlerFuncs{
+			AddFunc: controller.EnqueueControllerOf,
+			UpdateFunc: func(old, new interface{}) {
+				controller.EnqueueControllerOf(new)
+			},
 		},
 	})
 	return controller
@@ -93,211 +104,169 @@ func NewController(
 // is closed, at which point it will shutdown the workqueue and wait for
 // workers to finish processing their current work items.
 func (c *Controller) Run(threadiness int, stopCh <-chan struct{}) error {
-	return c.RunController(threadiness, stopCh, c.syncHandler, "Configuration")
+	return c.RunController(threadiness, stopCh, c.Reconcile, "Configuration")
 }
 
 // loggerWithConfigInfo enriches the logs with configuration name and namespace.
 func loggerWithConfigInfo(logger *zap.SugaredLogger, ns string, name string) *zap.SugaredLogger {
 	return logger.With(zap.String(logkey.Namespace, ns), zap.String(logkey.Configuration, name))
 }
 
-// syncHandler compares the actual state with the desired, and attempts to
+// Reconcile compares the actual state with the desired, and attempts to
 // converge the two. It then updates the Status block of the Configuration
 // resource with the current status of the resource.
-func (c *Controller) syncHandler(key string) error {
+func (c *Controller) Reconcile(key string) error {
 	// Convert the namespace/name string into a distinct namespace and name
 	namespace, name, err := cache.SplitMetaNamespaceKey(key)
 	if err != nil {
 		runtime.HandleError(fmt.Errorf("invalid resource key: %s", key))
 		return nil
 	}
-
+	// Wrap our logger with the additional context of the configuration that we are reconciling.
 	logger := loggerWithConfigInfo(c.Logger, namespace, name)
 
 	// Get the Configuration resource with this namespace/name
 	config, err := c.lister.Configurations(namespace).Get(name)
-	if err != nil {
-		// The resource may no longer exist, in which case we stop
-		// processing.
-		if errors.IsNotFound(err) {
-			runtime.HandleError(fmt.Errorf("configuration %q in work queue no longer exists", key))
-			return nil
-		}
-
+	if errors.IsNotFound(err) {
+		// The resource no longer exists, in which case we stop processing.
+		runtime.HandleError(fmt.Errorf("configuration %q in work queue no longer exists", key))
+		return nil
+	} else if err != nil {
 		return err
 	}
+
 	// Don't modify the informer's copy.
 	config = config.DeepCopy()
 	config.Status.InitializeConditions()
 
-	// Configuration business logic
-	if config.GetGeneration() == config.Status.ObservedGeneration {
-		// TODO(vaikas): Check to see if Status.LatestCreatedRevisionName is ready and update Status.LatestReady
-		logger.Infof("Skipping reconcile since already reconciled %d == %d",
-			config.Spec.Generation, config.Status.ObservedGeneration)
-		return nil
+	// First, fetch the revision that should exist for the current generation
+	revName := generateRevisionName(config)
+	latestCreatedRevision, err := c.revisionLister.Revisions(config.Namespace).Get(revName)
+	if errors.IsNotFound(err) {
+		latestCreatedRevision, err = c.createRevision(config, revName)
+		if err != nil {
+			logger.Errorf("Failed to create Revision %q: %v", revName, err)
+			c.Recorder.Eventf(config, corev1.EventTypeWarning, "CreationFailed", "Failed to create Revision %q: %v", revName, err)
+			return err
+		}
+	} else if err != nil {
+		logger.Errorf("Failed to reconcile Configuration: %q failed to Get Revision: %q", config.Name, revName)
+		return err
+	}
+
+	// Second, set this to be the latest revision that we have created.
+	config.Status.SetLatestCreatedRevisionName(revName)
+	config.Status.ObservedGeneration = config.Spec.Generation
+
+	// Last, determine whether we should set LatestReadyRevisionName to our
+	// LatestCreatedRevision based on its readiness.
+	rc := latestCreatedRevision.Status.GetCondition(v1alpha1.RevisionConditionReady)
+	switch {
+	case rc == nil || rc.Status == corev1.ConditionUnknown:
+		logger.Infof("Revision %q of configuration %q is not ready", revName, config.Name)
+
+	case rc.Status == corev1.ConditionTrue:
+		logger.Infof("Revision %q of configuration %q is ready", revName, config.Name)
+
+		created, ready := config.Status.LatestCreatedRevisionName, config.Status.LatestReadyRevisionName
+		if ready == "" {
+			// Surface an event for the first revision becoming ready.
+			c.Recorder.Eventf(config, corev1.EventTypeNormal, "ConfigurationReady",
+				"Configuration becomes ready")
+		}
+		if created != ready {
+			// Update the LatestReadyRevisionName and surface an event for the transition.
+			config.Status.SetLatestReadyRevisionName(latestCreatedRevision.Name)
+			c.Recorder.Eventf(config, corev1.EventTypeNormal, "LatestReadyUpdate",
+				"LatestReadyRevisionName updated to %q", latestCreatedRevision.Name)
+		}
+
+	case rc.Status == corev1.ConditionFalse:
+		logger.Infof("Revision %q of configuration %q has failed", revName, config.Name)
+
+		// TODO(mattmoor): Only emit the event the first time we see this.
+		config.Status.MarkLatestCreatedFailed(latestCreatedRevision.Name, rc.Message)
+		c.Recorder.Eventf(config, corev1.EventTypeWarning, "LatestCreatedFailed",
+			"Latest created revision %q has failed", latestCreatedRevision.Name)
+
+	default:
+		err := fmt.Errorf("unrecognized condition status: %v on revision %q", rc.Status, revName)
+		logger.Errorf("Error reconciling Configuration %q: %v", config.Name, err)
+		return err
+	}
+
+	// Reflect any status changes that occurred during reconciliation.
+	if _, err := c.updateStatus(config); err != nil {
+		logger.Error("Error updating configuration", zap.Error(err))
+		return err
 	}
 
-	logger.Infof("Running reconcile Configuration for %s\n%+v\n%+v\n",
-		config.Name, config, config.Spec.RevisionTemplate)
+	return nil
+}
+
+func (c *Controller) createRevision(config *v1alpha1.Configuration, revName string) (*v1alpha1.Revision, error) {
+	logger := loggerWithConfigInfo(c.Logger, config.Namespace, config.Name)
 	spec := config.Spec.RevisionTemplate.Spec
 	controllerRef := controller.NewConfigurationControllerRef(config)
 
 	if config.Spec.Build != nil {
 		// TODO(mattmoor): Determine whether we reuse the previous build.
 		build := &buildv1alpha1.Build{
 			ObjectMeta: metav1.ObjectMeta{
-				Namespace:    config.Namespace,
-				GenerateName: fmt.Sprintf("%s-", config.Name),
+				Namespace:       config.Namespace,
+				GenerateName:    fmt.Sprintf("%s-", config.Name),
+				OwnerReferences: []metav1.OwnerReference{*controllerRef},
 			},
 			Spec: *config.Spec.Build,
 		}
-		build.OwnerReferences = append(build.OwnerReferences, *controllerRef)
 		created, err := c.buildClientSet.BuildV1alpha1().Builds(build.Namespace).Create(build)
 		if err != nil {
-			logger.Errorf("Failed to create Build:\n%+v\n%s", build, err)
-			c.Recorder.Eventf(config, corev1.EventTypeWarning, "CreationFailed", "Failed to create Build %q: %v", build.Name, err)
-			return err
+			return nil, err
 		}
 		logger.Infof("Created Build:\n%+v", created.Name)
 		c.Recorder.Eventf(config, corev1.EventTypeNormal, "Created", "Created Build %q", created.Name)
 		spec.BuildName = created.Name
 	}
 
-	revName := generateRevisionName(config)
-	revClient := c.ElaClientSet.ServingV1alpha1().Revisions(config.Namespace)
-	created, err := revClient.Get(revName, metav1.GetOptions{})
-	if err != nil {
-		if !errors.IsNotFound(err) {
-			logger.Error("Revisions Get failed", zap.Error(err), zap.String(logkey.Revision, revName))
-			return err
-		}
-
-		rev := &v1alpha1.Revision{
-			ObjectMeta: config.Spec.RevisionTemplate.ObjectMeta,
-			Spec:       spec,
-		}
-		// TODO: Should this just use rev.ObjectMeta.GenerateName =
-		rev.ObjectMeta.Name = revName
-		// Can't generate objects in a different namespace from what the call is made against,
-		// so use the namespace of the configuration that's being updated for the Revision being
-		// created.
-		rev.ObjectMeta.Namespace = config.Namespace
-
-		if rev.ObjectMeta.Labels == nil {
-			rev.ObjectMeta.Labels = make(map[string]string)
-		}
-		rev.ObjectMeta.Labels[serving.ConfigurationLabelKey] = config.Name
-
-		if rev.ObjectMeta.Annotations == nil {
-			rev.ObjectMeta.Annotations = make(map[string]string)
-		}
-		rev.ObjectMeta.Annotations[serving.ConfigurationGenerationAnnotationKey] = fmt.Sprintf("%v", config.Spec.Generation)
-
-		// Delete revisions when the parent Configuration is deleted.
-		rev.OwnerReferences = append(rev.OwnerReferences, *controllerRef)
-
-		created, err = revClient.Create(rev)
-		if err != nil {
-			logger.Errorf("Failed to create Revision:\n%+v\n%s", rev, err)
-			c.Recorder.Eventf(config, corev1.EventTypeWarning, "CreationFailed", "Failed to create Revision %q: %v", rev.Name, err)
-			return err
-		}
-		c.Recorder.Eventf(config, corev1.EventTypeNormal, "Created", "Created Revision %q", rev.Name)
-		logger.Infof("Created Revision:\n%+v", created)
-	} else {
-		logger.Infof("Revision already created %s: %v", created.ObjectMeta.Name, err)
+	rev := &v1alpha1.Revision{
+		ObjectMeta: config.Spec.RevisionTemplate.ObjectMeta,
+		Spec:       spec,
 	}
-	// Update the Status of the configuration with the latest generation that
-	// we just reconciled against so we don't keep generating revisions.
-	// Also update the LatestCreatedRevisionName so that we'll know revision to check
-	// for ready state so that when ready, we can make it Latest.
-	config.Status.SetLatestCreatedRevisionName(created.ObjectMeta.Name)
-	config.Status.ObservedGeneration = config.Spec.Generation
+	rev.Namespace = config.Namespace
+	rev.Name = revName
+	if rev.Labels == nil {
+		rev.Labels = make(map[string]string)
+	}
+	rev.Labels[serving.ConfigurationLabelKey] = config.Name
+	if rev.Annotations == nil {
+		rev.Annotations = make(map[string]string)
+	}
+	rev.Annotations[serving.ConfigurationGenerationAnnotationKey] = fmt.Sprintf("%v", config.Spec.Generation)
 
-	logger.Infof("Updating the configuration status:\n%+v", config)
+	// Delete revisions when the parent Configuration is deleted.
+	rev.OwnerReferences = append(rev.OwnerReferences, *controllerRef)
 
-	if _, err = c.updateStatus(config); err != nil {
-		logger.Error("Failed to update the configuration", zap.Error(err))
-		return err
+	created, err := c.ElaClientSet.ServingV1alpha1().Revisions(config.Namespace).Create(rev)
+	if err != nil {
+		return nil, err
 	}
+	c.Recorder.Eventf(config, corev1.EventTypeNormal, "Created", "Created Revision %q", rev.Name)
+	logger.Infof("Created Revision:\n%+v", created)
 
-	return nil
+	return created, nil
 }
 
 func generateRevisionName(u *v1alpha1.Configuration) string {
 	return fmt.Sprintf("%s-%05d", u.Name, u.Spec.Generation)
 }
 
 func (c *Controller) updateStatus(u *v1alpha1.Configuration) (*v1alpha1.Configuration, error) {
-	configClient := c.ElaClientSet.ServingV1alpha1().Configurations(u.Namespace)
-	newu, err := configClient.Get(u.Name, metav1.GetOptions{})
+	newu, err := c.lister.Configurations(u.Namespace).Get(u.Name)
 	if err != nil {
 		return nil, err
 	}
 	newu.Status = u.Status
-
 	// TODO: for CRD there's no updatestatus, so use normal update
-	return configClient.Update(newu)
+	return c.ElaClientSet.ServingV1alpha1().Configurations(u.Namespace).Update(newu)
 	//	return configClient.UpdateStatus(newu)
 }
-
-func (c *Controller) SyncRevision(revision *v1alpha1.Revision) {
-	revisionName := revision.Name
-	namespace := revision.Namespace
-
-	or := metav1.GetControllerOf(revision)
-	if or == nil || or.Kind != "Configuration" {
-		return
-	}
-	configName := or.Name
-
-	logger := loggerWithConfigInfo(c.Logger, namespace, configName).With(zap.String(logkey.Revision, revisionName))
-
-	config, err := c.lister.Configurations(namespace).Get(configName)
-	if err != nil {
-		logger.Error("Error fetching configuration upon revision becoming ready", zap.Error(err))
-		return
-	}
-
-	if revision.Name != config.Status.LatestCreatedRevisionName {
-		// The revision isn't the latest created one, so ignore this event.
-		logger.Info("Revision is not the latest created one")
-		return
-	}
-
-	// Don't modify the informer's copy.
-	config = config.DeepCopy()
-
-	// Track whether the configuration was already ready in order to
-	// present an initial readiness event.
-	alreadyReady := config.Status.IsReady()
-
-	rc := revision.Status.GetCondition(v1alpha1.RevisionConditionReady)
-	if rc == nil || rc.Status == corev1.ConditionUnknown {
-		logger.Infof("Revision %q of configuration %q is not ready", revisionName, config.Name)
-		c.Recorder.Eventf(config, corev1.EventTypeNormal, "LatestRevisionUpdate",
-			"Latest revision of configuration is not ready")
-	} else if rc.Status == corev1.ConditionTrue {
-		logger.Infof("Revision %q of configuration %q is ready", revisionName, config.Name)
-		config.Status.SetLatestReadyRevisionName(revision.Name)
-		if _, err := c.updateStatus(config); err != nil {
-			logger.Error("Error updating configuration", zap.Error(err))
-		}
-
-		if !alreadyReady {
-			c.Recorder.Eventf(config, corev1.EventTypeNormal, "ConfigurationReady",
-				"Configuration becomes ready")
-		}
-		c.Recorder.Eventf(config, corev1.EventTypeNormal, "LatestReadyUpdate",
-			"LatestReadyRevisionName updated to %q", revision.Name)
-	} else {
-		logger.Infof("Revision %q of configuration %q has failed", revisionName, config.Name)
-		config.Status.MarkLatestCreatedFailed(revision.Name, rc.Message)
-		if _, err := c.updateStatus(config); err != nil {
-			logger.Error("Error updating configuration", zap.Error(err))
-		}
-		c.Recorder.Eventf(config, corev1.EventTypeWarning, "LatestCreatedFailed",
-			"Latest created revision %q has failed", revision.Name)
-	}
-}