#1157: Limit the amount of time for reconciliations (#1196)

* Limit the amount of time for reconciliations

* Stop reconciliation of binding when Broker responds with an error

* Do not retry when Broker responds with error for any request. Delete binding after retry window when failure is from binding injection.

* Add binding test assertions around whether OperationStartTime is set

* Remove code to delete a binding that we failed to inject

cmd/controller-manager/app/controller_manager.go

@@ -335,6 +335,7 @@ func StartControllers(s *options.ControllerManagerServer,
 			s.ServiceBrokerRelistInterval,
 			s.OSBAPIPreferredVersion,
 			recorder,
+			s.ReconciliationRetryDuration,
 		)
 		if err != nil {
 			return err

cmd/controller-manager/app/options/options.go

@@ -48,6 +48,7 @@ const (
 	defaultOSBAPIContextProfile         = true
 	defaultConcurrentSyncs              = 5
 	defaultLeaderElectionNamespace      = "kube-system"
+	defaultReconciliationRetryDuration  = 7 * 24 * time.Hour
 )
 
 var defaultOSBAPIPreferredVersion = osb.LatestAPIVersion().HeaderValue()
@@ -71,6 +72,7 @@ func NewControllerManagerServer() *ControllerManagerServer {
 			LeaderElectionNamespace:      defaultLeaderElectionNamespace,
 			EnableProfiling:              true,
 			EnableContentionProfiling:    false,
+			ReconciliationRetryDuration:  defaultReconciliationRetryDuration,
 		},
 	}
 	s.LeaderElection.LeaderElect = true
@@ -96,4 +98,5 @@ func (s *ControllerManagerServer) AddFlags(fs *pflag.FlagSet) {
 	fs.BoolVar(&s.EnableContentionProfiling, "contention-profiling", s.EnableContentionProfiling, "Enable lock contention profiling, if profiling is enabled")
 	leaderelection.BindFlags(&s.LeaderElection, fs)
 	fs.StringVar(&s.LeaderElectionNamespace, "leader-election-namespace", s.LeaderElectionNamespace, "Namespace to use for leader election lock")
+	fs.DurationVar(&s.ReconciliationRetryDuration, "reconciliation-retry-duration", s.ReconciliationRetryDuration, "The maximum amount of time to retry reconciliations on a resource before failing")
 }

cmd/controller-manager/controller-manager.go

@@ -14,10 +14,10 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-// The controller manager is responsible for running the controller(s) backing
-// the service-catalog API.  The controllers list/watch the service-catalog
-// API resources and implement the behaviors backing those resources by
-// communicating with service brokers and the main Kubernetes API server.
+// The controller manager is responsible for monitoring replication
+// controllers, and creating corresponding pods to achieve the desired
+// state.  It uses the API to listen for new controllers and to create/delete
+// pods.
 package main
 
 import (

pkg/apis/componentconfig/types.go

@@ -92,4 +92,8 @@ type ControllerManagerConfiguration struct {
 
 	// enableContentionProfiling enables lock contention profiling, if enableProfiling is true.
 	EnableContentionProfiling bool
+
+	// ReconciliationRetryDuration is the longest time to attempt reconciliation
+	// on a given resource before failing the reconciliation
+	ReconciliationRetryDuration time.Duration
 }

pkg/apis/servicecatalog/types.go

@@ -118,6 +118,9 @@ type ServiceBrokerStatus struct {
 	// was last processed by the controller. The reconciled generation is updated
 	// even if the controller failed to process the spec.
 	ReconciledGeneration int64
+
+	// OperationStartTime is the time at which the current operation began.
+	OperationStartTime *metav1.Time
 }
 
 // ServiceBrokerCondition contains condition information for a Broker.
@@ -148,6 +151,10 @@ const (
 	// ServiceBrokerConditionReady represents the fact that a given broker condition
 	// is in ready state.
 	ServiceBrokerConditionReady ServiceBrokerConditionType = "Ready"
+
+	// ServiceBrokerConditionFailed represents information about a final failure
+	// that should not be retried.
+	ServiceBrokerConditionFailed ServiceInstanceConditionType = "Failed"
 )
 
 // ConditionStatus represents a condition's status.
@@ -389,6 +396,9 @@ type ServiceInstanceStatus struct {
 	// was last processed by the controller. The reconciled generation is updated
 	// even if the controller failed to process the spec.
 	ReconciledGeneration int64
+
+	// OperationStartTime is the time at which the current operation began.
+	OperationStartTime *metav1.Time
 }
 
 // ServiceInstanceCondition contains condition information about an Instance.
@@ -501,6 +511,9 @@ type ServiceInstanceCredentialStatus struct {
 	// The reconciled generation is updated even if the controller failed to
 	// process the spec.
 	ReconciledGeneration int64
+
+	// OperationStartTime is the time at which the current operation began.
+	OperationStartTime *metav1.Time
 }
 
 // ServiceInstanceCredentialCondition condition information for a ServiceInstanceCredential.