Add setting to not track conditions of VirtualService when reconciling (#941)

* Default to not track conditions of VirtualService when reconciling
Routes.
Status change is causing too much updates to VirtualService.

* Fix tests

* trackRouteStatus default to true. Add tests

* RouteTrackVirtualService default to false

* Revert dates and url for license

Author: guangyw

config/config-defaults.yaml

@@ -163,6 +163,8 @@ data:
     # The grace period is the duration in seconds after the processes running in the pod are sent a termination signal and
     # the time when the processes are forcibly halted with a kill signal.
     terminationGracePeriodSeconds: 120
+    # When set to true, Route status will be updated with VirtualService conditions.
+    routeTrackVirtualService: "false"
 
     # The following images are used to execute builds. They SHOULD NOT be
     # modified except in rare circumstances.
@@ -233,4 +235,5 @@ data:
   appCPUMin: "100m"
   progressDeadlineSeconds: "600"
   terminationGracePeriodSeconds: "30"
+  routeTrackVirtualService: "false"
 

operator/pkg/apis/kfsystem/kf/defaults.go

@@ -43,6 +43,7 @@ const (
 	appCPUMinKey                     = "appCPUMin"
 	progressDeadlineSecondsKey       = "progressDeadlineSeconds"
 	terminationGracePeriodSecondsKey = "terminationGracePeriodSeconds"
+	routeTrackVirtualServiceKey      = "routeTrackVirtualService"
 
 	// Images used for build purposes
 
@@ -140,6 +141,9 @@ type DefaultsConfig struct {
 	// The grace period is the duration in seconds after the processes running in the pod are sent
 	// a termination signal and the time when the processes are forcibly halted with a kill signal.
 	TerminationGracePeriodSeconds *int64 `json:"terminationGracePeriodSeconds,omitempty"`
+
+	// RouteTrackVirtualService when set to true, will update Route status with VirtualService conditions.
+	RouteTrackVirtualService bool `json:"routeTrackVirtualService,omitempty"`
 }
 
 // BuiltinDefaultsConfig creates a defaults configuration with default values.
@@ -247,6 +251,7 @@ func (defaultsConfig *DefaultsConfig) getInterfaceValues(leaveEmpty bool) map[st
 		appCPUMinKey:                     &defaultsConfig.AppCPUMin,
 		progressDeadlineSecondsKey:       &defaultsConfig.ProgressDeadlineSeconds,
 		terminationGracePeriodSecondsKey: &defaultsConfig.TerminationGracePeriodSeconds,
+		routeTrackVirtualServiceKey:      &defaultsConfig.RouteTrackVirtualService,
 	}
 
 	if !leaveEmpty {

pkg/apis/kf/config/defaults.go

@@ -43,6 +43,7 @@ const (
 	appCPUMinKey                     = "appCPUMin"
 	progressDeadlineSecondsKey       = "progressDeadlineSeconds"
 	terminationGracePeriodSecondsKey = "terminationGracePeriodSeconds"
+	routeTrackVirtualServiceKey      = "routeTrackVirtualService"
 
 	// Images used for build purposes
 
@@ -140,6 +141,9 @@ type DefaultsConfig struct {
 	// The grace period is the duration in seconds after the processes running in the pod are sent
 	// a termination signal and the time when the processes are forcibly halted with a kill signal.
 	TerminationGracePeriodSeconds *int64 `json:"terminationGracePeriodSeconds,omitempty"`
+
+	// RouteTrackVirtualService when set to true, will update Route status with VirtualService conditions.
+	RouteTrackVirtualService bool `json:"routeTrackVirtualService,omitempty"`
 }
 
 // BuiltinDefaultsConfig creates a defaults configuration with default values.
@@ -247,6 +251,7 @@ func (defaultsConfig *DefaultsConfig) getInterfaceValues(leaveEmpty bool) map[st
 		appCPUMinKey:                     &defaultsConfig.AppCPUMin,
 		progressDeadlineSecondsKey:       &defaultsConfig.ProgressDeadlineSeconds,
 		terminationGracePeriodSecondsKey: &defaultsConfig.TerminationGracePeriodSeconds,
+		routeTrackVirtualServiceKey:      &defaultsConfig.RouteTrackVirtualService,
 	}
 
 	if !leaveEmpty {

pkg/apis/kf/config/defaults_test.go

@@ -50,6 +50,7 @@ func TestPatchConfigMap(t *testing.T) {
 		appCPUPerGBOfRAMKey,
 		progressDeadlineSecondsKey,
 		terminationGracePeriodSecondsKey,
+		routeTrackVirtualServiceKey,
 	}
 	_, configMap := cmtesting.ConfigMapsFromTestFile(t, DefaultsConfigTestName, allowedPredefinedKey...)
 	// sanity check the configmap, add more assertions below when new fields

pkg/apis/kf/config/store_test.go

@@ -56,6 +56,7 @@ func TestStoreLoadWithContext(t *testing.T) {
 		appCPUPerGBOfRAMKey,
 		progressDeadlineSecondsKey,
 		terminationGracePeriodSecondsKey,
+		routeTrackVirtualServiceKey,
 	}
 	_, configMap := cmtesting.ConfigMapsFromTestFile(t, DefaultsConfigName, allowedPredefinedKey...)
 	// sanity check the configmap, add more assertions below when new fields

pkg/apis/kf/v1alpha1/app_lifecycle_test.go

@@ -669,7 +669,7 @@ func TestAppStatus_PropagateRouteStatus(t *testing.T) {
 		route.Status.PropagateRouteSpecFields(qrb.Source)
 		route.Status.PropagateVirtualService(&networking.VirtualService{
 			ObjectMeta: metav1.ObjectMeta{Name: "some-vs"},
-		}, nil)
+		}, nil, true)
 		route.Status.PropagateBindings([]RouteDestination{qrb.Destination})
 		route.Status.PropagateSpaceDomain(&SpaceDomain{
 			Domain: qrb.Source.Domain,

pkg/apis/kf/v1alpha1/route_lifecycle.go

@@ -35,14 +35,20 @@ var _ kmeta.OwnerRefable = (*Route)(nil)
 // PropagateVirtualService stores the VirtualService in the RouteStatus.
 // If vsErr is set, a reconciliation error is triggered and the name will be
 // empty. The state is unknown if both VirtualService and error are nil.
-func (status *RouteStatus) PropagateVirtualService(vs *networking.VirtualService, vsErr error) {
+// If shouldTrack is false, directly setting the condition to True.
+func (status *RouteStatus) PropagateVirtualService(vs *networking.VirtualService, vsErr error, shouldTrack bool) {
 	cond := status.VirtualServiceCondition()
 
 	switch {
+	case !shouldTrack:
+		cond.MarkSuccess()
+		// Update VirtualService when it's not nil. Do not overwrite it back to empty otherwise.
+		if vs != nil {
+			status.VirtualService = corev1.LocalObjectReference{Name: vs.Name}
+		}
 	case vsErr != nil:
 		cond.MarkReconciliationError("reconciling", vsErr)
 		status.VirtualService = corev1.LocalObjectReference{}
-
 	case vs == nil:
 		cond.MarkReconciliationPending()
 		status.VirtualService = corev1.LocalObjectReference{}

pkg/apis/kf/v1alpha1/route_lifecycle_test.go

@@ -56,15 +56,15 @@ func TestRouteStatus_VirtualServiceError(t *testing.T) {
 	t.Parallel()
 
 	status := RouteStatus{}
-	status.PropagateVirtualService(nil, errors.New("some-reason: some-message"))
+	status.PropagateVirtualService(nil, errors.New("some-reason: some-message"), true)
 	apitesting.CheckConditionFailed(status.duck(), RouteConditionVirtualServiceReady, t)
 }
 
 func TestRouteStatus_VirtualServiceNil(t *testing.T) {
 	t.Parallel()
 
 	status := RouteStatus{}
-	status.PropagateVirtualService(nil, nil)
+	status.PropagateVirtualService(nil, nil, true)
 	testutil.AssertEqual(t, "VirtualService", "", status.VirtualService.Name)
 }
 
@@ -74,7 +74,36 @@ func TestRouteStatus_VirtualService(t *testing.T) {
 	status := RouteStatus{}
 	status.PropagateVirtualService(&networking.VirtualService{
 		ObjectMeta: metav1.ObjectMeta{Name: "some-name"},
-	}, nil)
+	}, nil, true)
+	testutil.AssertEqual(t, "VirtualService", "some-name", status.VirtualService.Name)
+}
+
+func TestRouteStatus_VirtualServiceErrorNotTracking(t *testing.T) {
+	t.Parallel()
+
+	status := RouteStatus{}
+	status.PropagateVirtualService(nil, errors.New("some-reason: some-message"), false)
+	apitesting.CheckConditionSucceeded(status.duck(), RouteConditionVirtualServiceReady, t)
+}
+
+func TestRouteStatus_VirtualServiceErrorNotTrackingVSNamePopulated(t *testing.T) {
+	t.Parallel()
+	status := RouteStatus{}
+	status.PropagateVirtualService(
+		&networking.VirtualService{
+			ObjectMeta: metav1.ObjectMeta{Name: "some-name"},
+		},
+		errors.New("some-reason: some-message"),
+		false)
+	apitesting.CheckConditionSucceeded(status.duck(), RouteConditionVirtualServiceReady, t)
+	testutil.AssertEqual(t, "VirtualService", "some-name", status.VirtualService.Name)
+
+	status.PropagateVirtualService(
+		nil,
+		errors.New("some-reason: some-message"),
+		false)
+	apitesting.CheckConditionSucceeded(status.duck(), RouteConditionVirtualServiceReady, t)
+	// Asserting VirtualService Name didn't change.
 	testutil.AssertEqual(t, "VirtualService", "some-name", status.VirtualService.Name)
 }
 

pkg/reconciler/route/controller.go

@@ -18,6 +18,7 @@ import (
 	"context"
 	"fmt"
 
+	config "github.com/google/kf/v2/pkg/apis/kf/config"
 	v1alpha1 "github.com/google/kf/v2/pkg/apis/kf/v1alpha1"
 	networking "github.com/google/kf/v2/pkg/apis/networking/v1alpha3"
 	appinformer "github.com/google/kf/v2/pkg/client/kf/injection/informers/kf/v1alpha1/app"
@@ -46,6 +47,10 @@ func NewController(ctx context.Context, cmw configmap.Watcher) *controller.Impl
 	spaceInformer := spaceinformer.Get(ctx)
 	serviceInstanceBindingInformer := serviceinstancebindinginformer.Get(ctx)
 
+	// Setting up ConfigMap receivers
+	kfConfigStore := config.NewStore(logger.Named("kf-config-store"))
+	kfConfigStore.WatchConfigs(cmw)
+
 	// Create reconciler
 	c := &Reconciler{
 		Base:                         reconciler.NewBase(ctx, cmw),
@@ -55,6 +60,7 @@ func NewController(ctx context.Context, cmw configmap.Watcher) *controller.Impl
 		virtualServiceLister:         vsInformer.Lister(),
 		networkingClientSet:          networkingclient.Get(ctx),
 		serviceInstanceBindingLister: serviceInstanceBindingInformer.Lister(),
+		kfConfigStore:                kfConfigStore,
 	}
 
 	impl := controller.NewContext(ctx, c, controller.ControllerOptions{WorkQueueName: "Routes", Logger: logger})

pkg/reconciler/route/reconciler.go

@@ -19,6 +19,7 @@ import (
 	"fmt"
 	"sort"
 
+	"github.com/google/kf/v2/pkg/apis/kf/config"
 	"github.com/google/kf/v2/pkg/apis/kf/v1alpha1"
 	networking "github.com/google/kf/v2/pkg/apis/networking/v1alpha3"
 	kflisters "github.com/google/kf/v2/pkg/client/kf/listers/kf/v1alpha1"
@@ -34,6 +35,7 @@ import (
 	"k8s.io/client-go/tools/cache"
 	"knative.dev/pkg/controller"
 	"knative.dev/pkg/logging"
+	pkgreconciler "knative.dev/pkg/reconciler"
 )
 
 // Reconciler reconciles a Route object with the K8s cluster.
@@ -47,6 +49,8 @@ type Reconciler struct {
 	appLister                    kflisters.AppLister
 	spaceLister                  kflisters.SpaceLister
 	serviceInstanceBindingLister kflisters.ServiceInstanceBindingLister
+
+	kfConfigStore pkgreconciler.ConfigStore
 }
 
 // Check that our Reconciler implements controller.Reconciler
@@ -85,6 +89,7 @@ func (r *Reconciler) ApplyChanges(
 	domain string,
 ) error {
 	logger := logging.FromContext(ctx)
+	ctx = r.kfConfigStore.ToContext(ctx)
 
 	// Check that the domain is valid for the Space
 	var spaceDomain *v1alpha1.SpaceDomain
@@ -207,7 +212,14 @@ func (r *Reconciler) ApplyChanges(
 		// https://github.com/kubernetes/community/blob/master/contributors/devel/sig-api-machinery/controllers.md
 		toReconcile.Status.ObservedGeneration = toReconcile.Generation
 
-		toReconcile.Status.PropagateVirtualService(actualVS, sErr)
+		configDefaults, err := config.FromContext(ctx).Defaults()
+		if err != nil {
+			return fmt.Errorf("failed to read config-defaults: %v", err)
+		}
+
+		shouldTrackVirtualService := configDefaults.RouteTrackVirtualService
+
+		toReconcile.Status.PropagateVirtualService(actualVS, sErr, shouldTrackVirtualService)
 		toReconcile.Status.PropagateRouteSpecFields(origRoute.Spec.RouteSpecFields)
 
 		rsfString := toReconcile.Spec.RouteSpecFields.String()