diff --git a/cmd/controller-manager/app/controller_manager.go b/cmd/controller-manager/app/controller_manager.go index eead4596ba0..172c90b1e29 100644 --- a/cmd/controller-manager/app/controller_manager.go +++ b/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 diff --git a/cmd/controller-manager/app/options/options.go b/cmd/controller-manager/app/options/options.go index e52cdb88814..a0e94c136fa 100644 --- a/cmd/controller-manager/app/options/options.go +++ b/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") } diff --git a/cmd/controller-manager/controller-manager.go b/cmd/controller-manager/controller-manager.go index 1562b7c1258..8e60e181b25 100644 --- a/cmd/controller-manager/controller-manager.go +++ b/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 ( diff --git a/pkg/apis/componentconfig/types.go b/pkg/apis/componentconfig/types.go index 5d51c6d9693..36ce9949b97 100644 --- a/pkg/apis/componentconfig/types.go +++ b/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 } diff --git a/pkg/apis/servicecatalog/types.go b/pkg/apis/servicecatalog/types.go index 6cf171a4589..ae93dc64ef0 100644 --- a/pkg/apis/servicecatalog/types.go +++ b/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. diff --git a/pkg/apis/servicecatalog/v1alpha1/types.generated.go b/pkg/apis/servicecatalog/v1alpha1/types.generated.go index 8dfe08844d2..fefb5d1e668 100644 --- a/pkg/apis/servicecatalog/v1alpha1/types.generated.go +++ b/pkg/apis/servicecatalog/v1alpha1/types.generated.go @@ -1833,12 +1833,13 @@ func (x *ServiceBrokerStatus) CodecEncodeSelf(e *codec1978.Encoder) { } else { yysep2 := !z.EncBinary() yy2arr2 := z.EncBasicHandle().StructToArray - var yyq2 [2]bool + var yyq2 [3]bool _, _, _ = yysep2, yyq2, yy2arr2 const yyr2 bool = false + yyq2[2] = x.OperationStartTime != nil var yynn2 int if yyr2 || yy2arr2 { - r.EncodeArrayStart(2) + r.EncodeArrayStart(3) } else { yynn2 = 2 for _, b := range yyq2 { @@ -1895,6 +1896,49 @@ func (x *ServiceBrokerStatus) CodecEncodeSelf(e *codec1978.Encoder) { r.EncodeInt(int64(x.ReconciledGeneration)) } } + if yyr2 || yy2arr2 { + z.EncSendContainerState(codecSelfer_containerArrayElem1234) + if yyq2[2] { + if x.OperationStartTime == nil { + r.EncodeNil() + } else { + yym10 := z.EncBinary() + _ = yym10 + if false { + } else if z.HasExtensions() && z.EncExt(x.OperationStartTime) { + } else if yym10 { + z.EncBinaryMarshal(x.OperationStartTime) + } else if !yym10 && z.IsJSONHandle() { + z.EncJSONMarshal(x.OperationStartTime) + } else { + z.EncFallback(x.OperationStartTime) + } + } + } else { + r.EncodeNil() + } + } else { + if yyq2[2] { + z.EncSendContainerState(codecSelfer_containerMapKey1234) + r.EncodeString(codecSelferC_UTF81234, string("operationStartTime")) + z.EncSendContainerState(codecSelfer_containerMapValue1234) + if x.OperationStartTime == nil { + r.EncodeNil() + } else { + yym11 := z.EncBinary() + _ = yym11 + if false { + } else if z.HasExtensions() && z.EncExt(x.OperationStartTime) { + } else if yym11 { + z.EncBinaryMarshal(x.OperationStartTime) + } else if !yym11 && z.IsJSONHandle() { + z.EncJSONMarshal(x.OperationStartTime) + } else { + z.EncFallback(x.OperationStartTime) + } + } + } + } if yyr2 || yy2arr2 { z.EncSendContainerState(codecSelfer_containerArrayEnd1234) } else { @@ -1980,6 +2024,27 @@ func (x *ServiceBrokerStatus) codecDecodeSelfFromMap(l int, d *codec1978.Decoder *((*int64)(yyv6)) = int64(r.DecodeInt(64)) } } + case "operationStartTime": + if r.TryDecodeAsNil() { + if x.OperationStartTime != nil { + x.OperationStartTime = nil + } + } else { + if x.OperationStartTime == nil { + x.OperationStartTime = new(pkg1_v1.Time) + } + yym9 := z.DecBinary() + _ = yym9 + if false { + } else if z.HasExtensions() && z.DecExt(x.OperationStartTime) { + } else if yym9 { + z.DecBinaryUnmarshal(x.OperationStartTime) + } else if !yym9 && z.IsJSONHandle() { + z.DecJSONUnmarshal(x.OperationStartTime) + } else { + z.DecFallback(x.OperationStartTime, false) + } + } default: z.DecStructFieldNotFound(-1, yys3) } // end switch yys3 @@ -1991,16 +2056,16 @@ func (x *ServiceBrokerStatus) codecDecodeSelfFromArray(l int, d *codec1978.Decod var h codecSelfer1234 z, r := codec1978.GenHelperDecoder(d) _, _, _ = h, z, r - var yyj8 int - var yyb8 bool - var yyhl8 bool = l >= 0 - yyj8++ - if yyhl8 { - yyb8 = yyj8 > l + var yyj10 int + var yyb10 bool + var yyhl10 bool = l >= 0 + yyj10++ + if yyhl10 { + yyb10 = yyj10 > l } else { - yyb8 = r.CheckBreak() + yyb10 = r.CheckBreak() } - if yyb8 { + if yyb10 { z.DecSendContainerState(codecSelfer_containerArrayEnd1234) return } @@ -2008,21 +2073,21 @@ func (x *ServiceBrokerStatus) codecDecodeSelfFromArray(l int, d *codec1978.Decod if r.TryDecodeAsNil() { x.Conditions = nil } else { - yyv9 := &x.Conditions - yym10 := z.DecBinary() - _ = yym10 + yyv11 := &x.Conditions + yym12 := z.DecBinary() + _ = yym12 if false { } else { - h.decSliceServiceBrokerCondition((*[]ServiceBrokerCondition)(yyv9), d) + h.decSliceServiceBrokerCondition((*[]ServiceBrokerCondition)(yyv11), d) } } - yyj8++ - if yyhl8 { - yyb8 = yyj8 > l + yyj10++ + if yyhl10 { + yyb10 = yyj10 > l } else { - yyb8 = r.CheckBreak() + yyb10 = r.CheckBreak() } - if yyb8 { + if yyb10 { z.DecSendContainerState(codecSelfer_containerArrayEnd1234) return } @@ -2030,26 +2095,57 @@ func (x *ServiceBrokerStatus) codecDecodeSelfFromArray(l int, d *codec1978.Decod if r.TryDecodeAsNil() { x.ReconciledGeneration = 0 } else { - yyv11 := &x.ReconciledGeneration - yym12 := z.DecBinary() - _ = yym12 + yyv13 := &x.ReconciledGeneration + yym14 := z.DecBinary() + _ = yym14 + if false { + } else { + *((*int64)(yyv13)) = int64(r.DecodeInt(64)) + } + } + yyj10++ + if yyhl10 { + yyb10 = yyj10 > l + } else { + yyb10 = r.CheckBreak() + } + if yyb10 { + z.DecSendContainerState(codecSelfer_containerArrayEnd1234) + return + } + z.DecSendContainerState(codecSelfer_containerArrayElem1234) + if r.TryDecodeAsNil() { + if x.OperationStartTime != nil { + x.OperationStartTime = nil + } + } else { + if x.OperationStartTime == nil { + x.OperationStartTime = new(pkg1_v1.Time) + } + yym16 := z.DecBinary() + _ = yym16 if false { + } else if z.HasExtensions() && z.DecExt(x.OperationStartTime) { + } else if yym16 { + z.DecBinaryUnmarshal(x.OperationStartTime) + } else if !yym16 && z.IsJSONHandle() { + z.DecJSONUnmarshal(x.OperationStartTime) } else { - *((*int64)(yyv11)) = int64(r.DecodeInt(64)) + z.DecFallback(x.OperationStartTime, false) } } for { - yyj8++ - if yyhl8 { - yyb8 = yyj8 > l + yyj10++ + if yyhl10 { + yyb10 = yyj10 > l } else { - yyb8 = r.CheckBreak() + yyb10 = r.CheckBreak() } - if yyb8 { + if yyb10 { break } z.DecSendContainerState(codecSelfer_containerArrayElem1234) - z.DecStructFieldNotFound(yyj8-1, "") + z.DecStructFieldNotFound(yyj10-1, "") } z.DecSendContainerState(codecSelfer_containerArrayEnd1234) } @@ -6092,14 +6188,15 @@ func (x *ServiceInstanceStatus) CodecEncodeSelf(e *codec1978.Encoder) { } else { yysep2 := !z.EncBinary() yy2arr2 := z.EncBasicHandle().StructToArray - var yyq2 [5]bool + var yyq2 [6]bool _, _, _ = yysep2, yyq2, yy2arr2 const yyr2 bool = false yyq2[2] = x.LastOperation != nil yyq2[3] = x.DashboardURL != nil + yyq2[5] = x.OperationStartTime != nil var yynn2 int if yyr2 || yy2arr2 { - r.EncodeArrayStart(5) + r.EncodeArrayStart(6) } else { yynn2 = 3 for _, b := range yyq2 { @@ -6245,6 +6342,49 @@ func (x *ServiceInstanceStatus) CodecEncodeSelf(e *codec1978.Encoder) { r.EncodeInt(int64(x.ReconciledGeneration)) } } + if yyr2 || yy2arr2 { + z.EncSendContainerState(codecSelfer_containerArrayElem1234) + if yyq2[5] { + if x.OperationStartTime == nil { + r.EncodeNil() + } else { + yym23 := z.EncBinary() + _ = yym23 + if false { + } else if z.HasExtensions() && z.EncExt(x.OperationStartTime) { + } else if yym23 { + z.EncBinaryMarshal(x.OperationStartTime) + } else if !yym23 && z.IsJSONHandle() { + z.EncJSONMarshal(x.OperationStartTime) + } else { + z.EncFallback(x.OperationStartTime) + } + } + } else { + r.EncodeNil() + } + } else { + if yyq2[5] { + z.EncSendContainerState(codecSelfer_containerMapKey1234) + r.EncodeString(codecSelferC_UTF81234, string("operationStartTime")) + z.EncSendContainerState(codecSelfer_containerMapValue1234) + if x.OperationStartTime == nil { + r.EncodeNil() + } else { + yym24 := z.EncBinary() + _ = yym24 + if false { + } else if z.HasExtensions() && z.EncExt(x.OperationStartTime) { + } else if yym24 { + z.EncBinaryMarshal(x.OperationStartTime) + } else if !yym24 && z.IsJSONHandle() { + z.EncJSONMarshal(x.OperationStartTime) + } else { + z.EncFallback(x.OperationStartTime) + } + } + } + } if yyr2 || yy2arr2 { z.EncSendContainerState(codecSelfer_containerArrayEnd1234) } else { @@ -6374,6 +6514,27 @@ func (x *ServiceInstanceStatus) codecDecodeSelfFromMap(l int, d *codec1978.Decod *((*int64)(yyv12)) = int64(r.DecodeInt(64)) } } + case "operationStartTime": + if r.TryDecodeAsNil() { + if x.OperationStartTime != nil { + x.OperationStartTime = nil + } + } else { + if x.OperationStartTime == nil { + x.OperationStartTime = new(pkg1_v1.Time) + } + yym15 := z.DecBinary() + _ = yym15 + if false { + } else if z.HasExtensions() && z.DecExt(x.OperationStartTime) { + } else if yym15 { + z.DecBinaryUnmarshal(x.OperationStartTime) + } else if !yym15 && z.IsJSONHandle() { + z.DecJSONUnmarshal(x.OperationStartTime) + } else { + z.DecFallback(x.OperationStartTime, false) + } + } default: z.DecStructFieldNotFound(-1, yys3) } // end switch yys3 @@ -6385,16 +6546,16 @@ func (x *ServiceInstanceStatus) codecDecodeSelfFromArray(l int, d *codec1978.Dec var h codecSelfer1234 z, r := codec1978.GenHelperDecoder(d) _, _, _ = h, z, r - var yyj14 int - var yyb14 bool - var yyhl14 bool = l >= 0 - yyj14++ - if yyhl14 { - yyb14 = yyj14 > l + var yyj16 int + var yyb16 bool + var yyhl16 bool = l >= 0 + yyj16++ + if yyhl16 { + yyb16 = yyj16 > l } else { - yyb14 = r.CheckBreak() + yyb16 = r.CheckBreak() } - if yyb14 { + if yyb16 { z.DecSendContainerState(codecSelfer_containerArrayEnd1234) return } @@ -6402,21 +6563,21 @@ func (x *ServiceInstanceStatus) codecDecodeSelfFromArray(l int, d *codec1978.Dec if r.TryDecodeAsNil() { x.Conditions = nil } else { - yyv15 := &x.Conditions - yym16 := z.DecBinary() - _ = yym16 + yyv17 := &x.Conditions + yym18 := z.DecBinary() + _ = yym18 if false { } else { - h.decSliceServiceInstanceCondition((*[]ServiceInstanceCondition)(yyv15), d) + h.decSliceServiceInstanceCondition((*[]ServiceInstanceCondition)(yyv17), d) } } - yyj14++ - if yyhl14 { - yyb14 = yyj14 > l + yyj16++ + if yyhl16 { + yyb16 = yyj16 > l } else { - yyb14 = r.CheckBreak() + yyb16 = r.CheckBreak() } - if yyb14 { + if yyb16 { z.DecSendContainerState(codecSelfer_containerArrayEnd1234) return } @@ -6424,21 +6585,21 @@ func (x *ServiceInstanceStatus) codecDecodeSelfFromArray(l int, d *codec1978.Dec if r.TryDecodeAsNil() { x.AsyncOpInProgress = false } else { - yyv17 := &x.AsyncOpInProgress - yym18 := z.DecBinary() - _ = yym18 + yyv19 := &x.AsyncOpInProgress + yym20 := z.DecBinary() + _ = yym20 if false { } else { - *((*bool)(yyv17)) = r.DecodeBool() + *((*bool)(yyv19)) = r.DecodeBool() } } - yyj14++ - if yyhl14 { - yyb14 = yyj14 > l + yyj16++ + if yyhl16 { + yyb16 = yyj16 > l } else { - yyb14 = r.CheckBreak() + yyb16 = r.CheckBreak() } - if yyb14 { + if yyb16 { z.DecSendContainerState(codecSelfer_containerArrayEnd1234) return } @@ -6451,20 +6612,20 @@ func (x *ServiceInstanceStatus) codecDecodeSelfFromArray(l int, d *codec1978.Dec if x.LastOperation == nil { x.LastOperation = new(string) } - yym20 := z.DecBinary() - _ = yym20 + yym22 := z.DecBinary() + _ = yym22 if false { } else { *((*string)(x.LastOperation)) = r.DecodeString() } } - yyj14++ - if yyhl14 { - yyb14 = yyj14 > l + yyj16++ + if yyhl16 { + yyb16 = yyj16 > l } else { - yyb14 = r.CheckBreak() + yyb16 = r.CheckBreak() } - if yyb14 { + if yyb16 { z.DecSendContainerState(codecSelfer_containerArrayEnd1234) return } @@ -6477,20 +6638,20 @@ func (x *ServiceInstanceStatus) codecDecodeSelfFromArray(l int, d *codec1978.Dec if x.DashboardURL == nil { x.DashboardURL = new(string) } - yym22 := z.DecBinary() - _ = yym22 + yym24 := z.DecBinary() + _ = yym24 if false { } else { *((*string)(x.DashboardURL)) = r.DecodeString() } } - yyj14++ - if yyhl14 { - yyb14 = yyj14 > l + yyj16++ + if yyhl16 { + yyb16 = yyj16 > l } else { - yyb14 = r.CheckBreak() + yyb16 = r.CheckBreak() } - if yyb14 { + if yyb16 { z.DecSendContainerState(codecSelfer_containerArrayEnd1234) return } @@ -6498,26 +6659,57 @@ func (x *ServiceInstanceStatus) codecDecodeSelfFromArray(l int, d *codec1978.Dec if r.TryDecodeAsNil() { x.ReconciledGeneration = 0 } else { - yyv23 := &x.ReconciledGeneration - yym24 := z.DecBinary() - _ = yym24 + yyv25 := &x.ReconciledGeneration + yym26 := z.DecBinary() + _ = yym26 if false { } else { - *((*int64)(yyv23)) = int64(r.DecodeInt(64)) + *((*int64)(yyv25)) = int64(r.DecodeInt(64)) + } + } + yyj16++ + if yyhl16 { + yyb16 = yyj16 > l + } else { + yyb16 = r.CheckBreak() + } + if yyb16 { + z.DecSendContainerState(codecSelfer_containerArrayEnd1234) + return + } + z.DecSendContainerState(codecSelfer_containerArrayElem1234) + if r.TryDecodeAsNil() { + if x.OperationStartTime != nil { + x.OperationStartTime = nil + } + } else { + if x.OperationStartTime == nil { + x.OperationStartTime = new(pkg1_v1.Time) + } + yym28 := z.DecBinary() + _ = yym28 + if false { + } else if z.HasExtensions() && z.DecExt(x.OperationStartTime) { + } else if yym28 { + z.DecBinaryUnmarshal(x.OperationStartTime) + } else if !yym28 && z.IsJSONHandle() { + z.DecJSONUnmarshal(x.OperationStartTime) + } else { + z.DecFallback(x.OperationStartTime, false) } } for { - yyj14++ - if yyhl14 { - yyb14 = yyj14 > l + yyj16++ + if yyhl16 { + yyb16 = yyj16 > l } else { - yyb14 = r.CheckBreak() + yyb16 = r.CheckBreak() } - if yyb14 { + if yyb16 { break } z.DecSendContainerState(codecSelfer_containerArrayElem1234) - z.DecStructFieldNotFound(yyj14-1, "") + z.DecStructFieldNotFound(yyj16-1, "") } z.DecSendContainerState(codecSelfer_containerArrayEnd1234) } @@ -8149,12 +8341,13 @@ func (x *ServiceInstanceCredentialStatus) CodecEncodeSelf(e *codec1978.Encoder) } else { yysep2 := !z.EncBinary() yy2arr2 := z.EncBasicHandle().StructToArray - var yyq2 [2]bool + var yyq2 [3]bool _, _, _ = yysep2, yyq2, yy2arr2 const yyr2 bool = false + yyq2[2] = x.OperationStartTime != nil var yynn2 int if yyr2 || yy2arr2 { - r.EncodeArrayStart(2) + r.EncodeArrayStart(3) } else { yynn2 = 2 for _, b := range yyq2 { @@ -8211,6 +8404,49 @@ func (x *ServiceInstanceCredentialStatus) CodecEncodeSelf(e *codec1978.Encoder) r.EncodeInt(int64(x.ReconciledGeneration)) } } + if yyr2 || yy2arr2 { + z.EncSendContainerState(codecSelfer_containerArrayElem1234) + if yyq2[2] { + if x.OperationStartTime == nil { + r.EncodeNil() + } else { + yym10 := z.EncBinary() + _ = yym10 + if false { + } else if z.HasExtensions() && z.EncExt(x.OperationStartTime) { + } else if yym10 { + z.EncBinaryMarshal(x.OperationStartTime) + } else if !yym10 && z.IsJSONHandle() { + z.EncJSONMarshal(x.OperationStartTime) + } else { + z.EncFallback(x.OperationStartTime) + } + } + } else { + r.EncodeNil() + } + } else { + if yyq2[2] { + z.EncSendContainerState(codecSelfer_containerMapKey1234) + r.EncodeString(codecSelferC_UTF81234, string("operationStartTime")) + z.EncSendContainerState(codecSelfer_containerMapValue1234) + if x.OperationStartTime == nil { + r.EncodeNil() + } else { + yym11 := z.EncBinary() + _ = yym11 + if false { + } else if z.HasExtensions() && z.EncExt(x.OperationStartTime) { + } else if yym11 { + z.EncBinaryMarshal(x.OperationStartTime) + } else if !yym11 && z.IsJSONHandle() { + z.EncJSONMarshal(x.OperationStartTime) + } else { + z.EncFallback(x.OperationStartTime) + } + } + } + } if yyr2 || yy2arr2 { z.EncSendContainerState(codecSelfer_containerArrayEnd1234) } else { @@ -8296,6 +8532,27 @@ func (x *ServiceInstanceCredentialStatus) codecDecodeSelfFromMap(l int, d *codec *((*int64)(yyv6)) = int64(r.DecodeInt(64)) } } + case "operationStartTime": + if r.TryDecodeAsNil() { + if x.OperationStartTime != nil { + x.OperationStartTime = nil + } + } else { + if x.OperationStartTime == nil { + x.OperationStartTime = new(pkg1_v1.Time) + } + yym9 := z.DecBinary() + _ = yym9 + if false { + } else if z.HasExtensions() && z.DecExt(x.OperationStartTime) { + } else if yym9 { + z.DecBinaryUnmarshal(x.OperationStartTime) + } else if !yym9 && z.IsJSONHandle() { + z.DecJSONUnmarshal(x.OperationStartTime) + } else { + z.DecFallback(x.OperationStartTime, false) + } + } default: z.DecStructFieldNotFound(-1, yys3) } // end switch yys3 @@ -8307,16 +8564,16 @@ func (x *ServiceInstanceCredentialStatus) codecDecodeSelfFromArray(l int, d *cod var h codecSelfer1234 z, r := codec1978.GenHelperDecoder(d) _, _, _ = h, z, r - var yyj8 int - var yyb8 bool - var yyhl8 bool = l >= 0 - yyj8++ - if yyhl8 { - yyb8 = yyj8 > l + var yyj10 int + var yyb10 bool + var yyhl10 bool = l >= 0 + yyj10++ + if yyhl10 { + yyb10 = yyj10 > l } else { - yyb8 = r.CheckBreak() + yyb10 = r.CheckBreak() } - if yyb8 { + if yyb10 { z.DecSendContainerState(codecSelfer_containerArrayEnd1234) return } @@ -8324,21 +8581,21 @@ func (x *ServiceInstanceCredentialStatus) codecDecodeSelfFromArray(l int, d *cod if r.TryDecodeAsNil() { x.Conditions = nil } else { - yyv9 := &x.Conditions - yym10 := z.DecBinary() - _ = yym10 + yyv11 := &x.Conditions + yym12 := z.DecBinary() + _ = yym12 if false { } else { - h.decSliceServiceInstanceCredentialCondition((*[]ServiceInstanceCredentialCondition)(yyv9), d) + h.decSliceServiceInstanceCredentialCondition((*[]ServiceInstanceCredentialCondition)(yyv11), d) } } - yyj8++ - if yyhl8 { - yyb8 = yyj8 > l + yyj10++ + if yyhl10 { + yyb10 = yyj10 > l } else { - yyb8 = r.CheckBreak() + yyb10 = r.CheckBreak() } - if yyb8 { + if yyb10 { z.DecSendContainerState(codecSelfer_containerArrayEnd1234) return } @@ -8346,26 +8603,57 @@ func (x *ServiceInstanceCredentialStatus) codecDecodeSelfFromArray(l int, d *cod if r.TryDecodeAsNil() { x.ReconciledGeneration = 0 } else { - yyv11 := &x.ReconciledGeneration - yym12 := z.DecBinary() - _ = yym12 + yyv13 := &x.ReconciledGeneration + yym14 := z.DecBinary() + _ = yym14 + if false { + } else { + *((*int64)(yyv13)) = int64(r.DecodeInt(64)) + } + } + yyj10++ + if yyhl10 { + yyb10 = yyj10 > l + } else { + yyb10 = r.CheckBreak() + } + if yyb10 { + z.DecSendContainerState(codecSelfer_containerArrayEnd1234) + return + } + z.DecSendContainerState(codecSelfer_containerArrayElem1234) + if r.TryDecodeAsNil() { + if x.OperationStartTime != nil { + x.OperationStartTime = nil + } + } else { + if x.OperationStartTime == nil { + x.OperationStartTime = new(pkg1_v1.Time) + } + yym16 := z.DecBinary() + _ = yym16 if false { + } else if z.HasExtensions() && z.DecExt(x.OperationStartTime) { + } else if yym16 { + z.DecBinaryUnmarshal(x.OperationStartTime) + } else if !yym16 && z.IsJSONHandle() { + z.DecJSONUnmarshal(x.OperationStartTime) } else { - *((*int64)(yyv11)) = int64(r.DecodeInt(64)) + z.DecFallback(x.OperationStartTime, false) } } for { - yyj8++ - if yyhl8 { - yyb8 = yyj8 > l + yyj10++ + if yyhl10 { + yyb10 = yyj10 > l } else { - yyb8 = r.CheckBreak() + yyb10 = r.CheckBreak() } - if yyb8 { + if yyb10 { break } z.DecSendContainerState(codecSelfer_containerArrayElem1234) - z.DecStructFieldNotFound(yyj8-1, "") + z.DecStructFieldNotFound(yyj10-1, "") } z.DecSendContainerState(codecSelfer_containerArrayEnd1234) } @@ -9207,7 +9495,7 @@ func (x codecSelfer1234) decSliceServiceBroker(v *[]ServiceBroker, d *codec1978. yyrg1 := len(yyv1) > 0 yyv21 := yyv1 - yyrl1, yyrt1 = z.DecInferLen(yyl1, z.DecBasicHandle().MaxInitLen, 352) + yyrl1, yyrt1 = z.DecInferLen(yyl1, z.DecBasicHandle().MaxInitLen, 360) if yyrt1 { if yyrl1 <= cap(yyv1) { yyv1 = yyv1[:yyrl1] @@ -9683,7 +9971,7 @@ func (x codecSelfer1234) decSliceServiceInstance(v *[]ServiceInstance, d *codec1 yyrg1 := len(yyv1) > 0 yyv21 := yyv1 - yyrl1, yyrt1 = z.DecInferLen(yyl1, z.DecBasicHandle().MaxInitLen, 408) + yyrl1, yyrt1 = z.DecInferLen(yyl1, z.DecBasicHandle().MaxInitLen, 416) if yyrt1 { if yyrl1 <= cap(yyv1) { yyv1 = yyv1[:yyrl1] @@ -10283,7 +10571,7 @@ func (x codecSelfer1234) decSliceServiceInstanceCredential(v *[]ServiceInstanceC yyrg1 := len(yyv1) > 0 yyv21 := yyv1 - yyrl1, yyrt1 = z.DecInferLen(yyl1, z.DecBasicHandle().MaxInitLen, 384) + yyrl1, yyrt1 = z.DecInferLen(yyl1, z.DecBasicHandle().MaxInitLen, 392) if yyrt1 { if yyrl1 <= cap(yyv1) { yyv1 = yyv1[:yyrl1] diff --git a/pkg/apis/servicecatalog/v1alpha1/types.go b/pkg/apis/servicecatalog/v1alpha1/types.go index 8bdfbf37a28..f35a869faaf 100644 --- a/pkg/apis/servicecatalog/v1alpha1/types.go +++ b/pkg/apis/servicecatalog/v1alpha1/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 `json:"reconciledGeneration"` + + // OperationStartTime is the time at which the current operation began. + OperationStartTime *metav1.Time `json:"operationStartTime,omitempty"` } // 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 ServiceBrokerConditionType = "Failed" ) // ConditionStatus represents a condition's status. @@ -393,6 +400,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 `json:"reconciledGeneration"` + + // OperationStartTime is the time at which the current operation began. + OperationStartTime *metav1.Time `json:"operationStartTime,omitempty"` } // ServiceInstanceCondition contains condition information about an Instance. @@ -505,6 +515,9 @@ type ServiceInstanceCredentialStatus struct { // The reconciled generation is updated even if the controller failed to // process the spec. ReconciledGeneration int64 `json:"reconciledGeneration"` + + // OperationStartTime is the time at which the current operation began. + OperationStartTime *metav1.Time `json:"operationStartTime,omitempty"` } // ServiceInstanceCredentialCondition condition information for a ServiceInstanceCredential. diff --git a/pkg/apis/servicecatalog/v1alpha1/zz_generated.conversion.go b/pkg/apis/servicecatalog/v1alpha1/zz_generated.conversion.go index 4e955be1508..e236802da3a 100644 --- a/pkg/apis/servicecatalog/v1alpha1/zz_generated.conversion.go +++ b/pkg/apis/servicecatalog/v1alpha1/zz_generated.conversion.go @@ -22,6 +22,7 @@ package v1alpha1 import ( servicecatalog "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog" + meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1" conversion "k8s.io/apimachinery/pkg/conversion" runtime "k8s.io/apimachinery/pkg/runtime" v1 "k8s.io/client-go/pkg/api/v1" @@ -308,6 +309,7 @@ func Convert_servicecatalog_ServiceBrokerSpec_To_v1alpha1_ServiceBrokerSpec(in * func autoConvert_v1alpha1_ServiceBrokerStatus_To_servicecatalog_ServiceBrokerStatus(in *ServiceBrokerStatus, out *servicecatalog.ServiceBrokerStatus, s conversion.Scope) error { out.Conditions = *(*[]servicecatalog.ServiceBrokerCondition)(unsafe.Pointer(&in.Conditions)) out.ReconciledGeneration = in.ReconciledGeneration + out.OperationStartTime = (*meta_v1.Time)(unsafe.Pointer(in.OperationStartTime)) return nil } @@ -323,6 +325,7 @@ func autoConvert_servicecatalog_ServiceBrokerStatus_To_v1alpha1_ServiceBrokerSta out.Conditions = *(*[]ServiceBrokerCondition)(unsafe.Pointer(&in.Conditions)) } out.ReconciledGeneration = in.ReconciledGeneration + out.OperationStartTime = (*meta_v1.Time)(unsafe.Pointer(in.OperationStartTime)) return nil } @@ -578,6 +581,7 @@ func Convert_servicecatalog_ServiceInstanceCredentialSpec_To_v1alpha1_ServiceIns func autoConvert_v1alpha1_ServiceInstanceCredentialStatus_To_servicecatalog_ServiceInstanceCredentialStatus(in *ServiceInstanceCredentialStatus, out *servicecatalog.ServiceInstanceCredentialStatus, s conversion.Scope) error { out.Conditions = *(*[]servicecatalog.ServiceInstanceCredentialCondition)(unsafe.Pointer(&in.Conditions)) out.ReconciledGeneration = in.ReconciledGeneration + out.OperationStartTime = (*meta_v1.Time)(unsafe.Pointer(in.OperationStartTime)) return nil } @@ -593,6 +597,7 @@ func autoConvert_servicecatalog_ServiceInstanceCredentialStatus_To_v1alpha1_Serv out.Conditions = *(*[]ServiceInstanceCredentialCondition)(unsafe.Pointer(&in.Conditions)) } out.ReconciledGeneration = in.ReconciledGeneration + out.OperationStartTime = (*meta_v1.Time)(unsafe.Pointer(in.OperationStartTime)) return nil } @@ -663,6 +668,7 @@ func autoConvert_v1alpha1_ServiceInstanceStatus_To_servicecatalog_ServiceInstanc out.LastOperation = (*string)(unsafe.Pointer(in.LastOperation)) out.DashboardURL = (*string)(unsafe.Pointer(in.DashboardURL)) out.ReconciledGeneration = in.ReconciledGeneration + out.OperationStartTime = (*meta_v1.Time)(unsafe.Pointer(in.OperationStartTime)) return nil } @@ -681,6 +687,7 @@ func autoConvert_servicecatalog_ServiceInstanceStatus_To_v1alpha1_ServiceInstanc out.LastOperation = (*string)(unsafe.Pointer(in.LastOperation)) out.DashboardURL = (*string)(unsafe.Pointer(in.DashboardURL)) out.ReconciledGeneration = in.ReconciledGeneration + out.OperationStartTime = (*meta_v1.Time)(unsafe.Pointer(in.OperationStartTime)) return nil } diff --git a/pkg/apis/servicecatalog/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/servicecatalog/v1alpha1/zz_generated.deepcopy.go index 487102164b3..c0d0818e754 100644 --- a/pkg/apis/servicecatalog/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/servicecatalog/v1alpha1/zz_generated.deepcopy.go @@ -235,6 +235,11 @@ func DeepCopy_v1alpha1_ServiceBrokerStatus(in interface{}, out interface{}, c *c } } } + if in.OperationStartTime != nil { + in, out := &in.OperationStartTime, &out.OperationStartTime + *out = new(meta_v1.Time) + **out = (*in).DeepCopy() + } return nil } } @@ -432,6 +437,11 @@ func DeepCopy_v1alpha1_ServiceInstanceCredentialStatus(in interface{}, out inter } } } + if in.OperationStartTime != nil { + in, out := &in.OperationStartTime, &out.OperationStartTime + *out = new(meta_v1.Time) + **out = (*in).DeepCopy() + } return nil } } @@ -514,6 +524,11 @@ func DeepCopy_v1alpha1_ServiceInstanceStatus(in interface{}, out interface{}, c *out = new(string) **out = **in } + if in.OperationStartTime != nil { + in, out := &in.OperationStartTime, &out.OperationStartTime + *out = new(meta_v1.Time) + **out = (*in).DeepCopy() + } return nil } } diff --git a/pkg/apis/servicecatalog/zz_generated.deepcopy.go b/pkg/apis/servicecatalog/zz_generated.deepcopy.go index 3dd02713818..1761ecb95ef 100644 --- a/pkg/apis/servicecatalog/zz_generated.deepcopy.go +++ b/pkg/apis/servicecatalog/zz_generated.deepcopy.go @@ -235,6 +235,11 @@ func DeepCopy_servicecatalog_ServiceBrokerStatus(in interface{}, out interface{} } } } + if in.OperationStartTime != nil { + in, out := &in.OperationStartTime, &out.OperationStartTime + *out = new(meta_v1.Time) + **out = (*in).DeepCopy() + } return nil } } @@ -432,6 +437,11 @@ func DeepCopy_servicecatalog_ServiceInstanceCredentialStatus(in interface{}, out } } } + if in.OperationStartTime != nil { + in, out := &in.OperationStartTime, &out.OperationStartTime + *out = new(meta_v1.Time) + **out = (*in).DeepCopy() + } return nil } } @@ -514,6 +524,11 @@ func DeepCopy_servicecatalog_ServiceInstanceStatus(in interface{}, out interface *out = new(string) **out = **in } + if in.OperationStartTime != nil { + in, out := &in.OperationStartTime, &out.OperationStartTime + *out = new(meta_v1.Time) + **out = (*in).DeepCopy() + } return nil } } diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index b0495fd0e84..b0d9a2b7817 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -67,19 +67,21 @@ func NewController( brokerRelistInterval time.Duration, osbAPIPreferredVersion string, recorder record.EventRecorder, + reconciliationRetryDuration time.Duration, ) (Controller, error) { controller := &controller{ - kubeClient: kubeClient, - serviceCatalogClient: serviceCatalogClient, - brokerClientCreateFunc: brokerClientCreateFunc, - brokerRelistInterval: brokerRelistInterval, - OSBAPIPreferredVersion: osbAPIPreferredVersion, - recorder: recorder, - brokerQueue: workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "service-broker"), - serviceClassQueue: workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "service-class"), - instanceQueue: workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "service-instance"), - bindingQueue: workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "service-instance-credential"), - pollingQueue: workqueue.NewNamedRateLimitingQueue(workqueue.NewItemExponentialFailureRateLimiter(pollingStartInterval, pollingMaxBackoffDuration), "poller"), + kubeClient: kubeClient, + serviceCatalogClient: serviceCatalogClient, + brokerClientCreateFunc: brokerClientCreateFunc, + brokerRelistInterval: brokerRelistInterval, + OSBAPIPreferredVersion: osbAPIPreferredVersion, + recorder: recorder, + reconciliationRetryDuration: reconciliationRetryDuration, + brokerQueue: workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "service-broker"), + serviceClassQueue: workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "service-class"), + instanceQueue: workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "service-instance"), + bindingQueue: workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "service-instance-credential"), + pollingQueue: workqueue.NewNamedRateLimitingQueue(workqueue.NewItemExponentialFailureRateLimiter(pollingStartInterval, pollingMaxBackoffDuration), "poller"), } brokerInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{ @@ -124,20 +126,21 @@ type Controller interface { // controller is a concrete Controller. type controller struct { - kubeClient kubernetes.Interface - serviceCatalogClient servicecatalogclientset.ServicecatalogV1alpha1Interface - brokerClientCreateFunc osb.CreateFunc - brokerLister listers.ServiceBrokerLister - serviceClassLister listers.ServiceClassLister - instanceLister listers.ServiceInstanceLister - bindingLister listers.ServiceInstanceCredentialLister - brokerRelistInterval time.Duration - OSBAPIPreferredVersion string - recorder record.EventRecorder - brokerQueue workqueue.RateLimitingInterface - serviceClassQueue workqueue.RateLimitingInterface - instanceQueue workqueue.RateLimitingInterface - bindingQueue workqueue.RateLimitingInterface + kubeClient kubernetes.Interface + serviceCatalogClient servicecatalogclientset.ServicecatalogV1alpha1Interface + brokerClientCreateFunc osb.CreateFunc + brokerLister listers.ServiceBrokerLister + serviceClassLister listers.ServiceClassLister + instanceLister listers.ServiceInstanceLister + bindingLister listers.ServiceInstanceCredentialLister + brokerRelistInterval time.Duration + OSBAPIPreferredVersion string + recorder record.EventRecorder + reconciliationRetryDuration time.Duration + brokerQueue workqueue.RateLimitingInterface + serviceClassQueue workqueue.RateLimitingInterface + instanceQueue workqueue.RateLimitingInterface + bindingQueue workqueue.RateLimitingInterface // pollingQueue is separate from instanceQueue because we want // it to have different backoff / timeout characteristics from // a reconciling of an instance. diff --git a/pkg/controller/controller_binding.go b/pkg/controller/controller_binding.go index 3411f80790e..cfac7757823 100644 --- a/pkg/controller/controller_binding.go +++ b/pkg/controller/controller_binding.go @@ -18,6 +18,7 @@ package controller import ( "fmt" + "time" "github.com/golang/glog" osb "github.com/pmorie/go-open-service-broker-client/v2" @@ -277,6 +278,8 @@ func (c *controller) reconcileServiceInstanceCredential(binding *v1alpha1.Servic request.OriginatingIdentity = originatingIdentity } + now := metav1.Now() + response, err := brokerClient.Bind(request) if err != nil { httpErr, isError := osb.IsHTTPError(err) @@ -305,9 +308,11 @@ func (c *controller) reconcileServiceInstanceCredential(binding *v1alpha1.Servic v1alpha1.ConditionFalse, errorBindCallReason, "Bind call failed. "+s) + toUpdate.Status.OperationStartTime = nil + toUpdate.Status.ReconciledGeneration = toUpdate.Generation c.updateServiceInstanceCredentialStatus(toUpdate) c.recorder.Event(binding, api.EventTypeWarning, errorBindCallReason, s) - return err + return nil } s := fmt.Sprintf("Error creating ServiceInstanceCredential \"%s/%s\" for ServiceInstance \"%s/%s\" of ServiceClass %q at ServiceBroker %q: %s", binding.Name, binding.Namespace, instance.Namespace, instance.Name, serviceClass.Name, brokerName, err) @@ -318,8 +323,26 @@ func (c *controller) reconcileServiceInstanceCredential(binding *v1alpha1.Servic v1alpha1.ConditionFalse, errorBindCallReason, "Bind call failed. "+s) - c.updateServiceInstanceCredentialStatus(toUpdate) c.recorder.Event(binding, api.EventTypeWarning, errorBindCallReason, s) + + if binding.Status.OperationStartTime == nil { + toUpdate.Status.OperationStartTime = &now + } else if !time.Now().Before(binding.Status.OperationStartTime.Time.Add(c.reconciliationRetryDuration)) { + s := fmt.Sprintf(`Stopping reconciliation retries on ServiceInstanceCredential "%v/%v" because too much time has elapsed`, binding.Namespace, binding.Name) + glog.Info(s) + c.recorder.Event(binding, api.EventTypeWarning, errorReconciliationRetryTimeoutReason, s) + c.setServiceInstanceCredentialCondition(toUpdate, + v1alpha1.ServiceInstanceCredentialConditionFailed, + v1alpha1.ConditionTrue, + errorReconciliationRetryTimeoutReason, + s) + toUpdate.Status.OperationStartTime = nil + toUpdate.Status.ReconciledGeneration = toUpdate.Generation + c.updateServiceInstanceCredentialStatus(toUpdate) + return nil + } + + c.updateServiceInstanceCredentialStatus(toUpdate) return err } @@ -334,11 +357,41 @@ func (c *controller) reconcileServiceInstanceCredential(binding *v1alpha1.Servic errorInjectingBindResultReason, "Error injecting bind result "+s, ) - c.updateServiceInstanceCredentialStatus(toUpdate) c.recorder.Event(binding, api.EventTypeWarning, errorInjectingBindResultReason, s) + + if binding.Status.OperationStartTime == nil { + toUpdate.Status.OperationStartTime = &now + } else if !time.Now().Before(binding.Status.OperationStartTime.Time.Add(c.reconciliationRetryDuration)) { + s := fmt.Sprintf(`Stopping reconciliation retries on ServiceInstanceCredential "%v/%v" because too much time has elapsed`, binding.Namespace, binding.Name) + glog.Info(s) + c.recorder.Event(binding, api.EventTypeWarning, errorReconciliationRetryTimeoutReason, s) + c.setServiceInstanceCredentialCondition(toUpdate, + v1alpha1.ServiceInstanceCredentialConditionReady, + v1alpha1.ConditionFalse, + errorReconciliationRetryTimeoutReason, + s) + c.setServiceInstanceCredentialCondition(toUpdate, + v1alpha1.ServiceInstanceCredentialConditionFailed, + v1alpha1.ConditionTrue, + errorReconciliationRetryTimeoutReason, + s) + toUpdate.Status.OperationStartTime = nil + toUpdate.Status.ReconciledGeneration = toUpdate.Generation + c.updateServiceInstanceCredentialStatus(toUpdate) + + // TODO: We need to delete the ServiceInstanceCredential from the + // Broker since the Bind request was successful. This needs to be + // addressed as part of orphan mitigiation. + + return nil + } + + c.updateServiceInstanceCredentialStatus(toUpdate) return err } + toUpdate.Status.OperationStartTime = nil + // The bind operation completed successfully, so set // Status.ReconciledGeneration to the Generation used. toUpdate.Status.ReconciledGeneration = toUpdate.Generation @@ -404,11 +457,13 @@ func (c *controller) reconcileServiceInstanceCredential(binding *v1alpha1.Servic unbindRequest.OriginatingIdentity = originatingIdentity } + now := metav1.Now() + _, err = brokerClient.Unbind(unbindRequest) if err != nil { httpErr, isError := osb.IsHTTPError(err) if isError { - s := fmt.Sprintf("Error creating Unbinding \"%s/%s\" for ServiceInstance \"%s/%s\" of ServiceClass %q at ServiceBroker %q, %v", + s := fmt.Sprintf("Error unbinding ServiceInstanceCredential \"%s/%s\" for ServiceInstance \"%s/%s\" of ServiceClass %q at ServiceBroker %q: %s", binding.Name, binding.Namespace, instance.Namespace, @@ -424,9 +479,16 @@ func (c *controller) reconcileServiceInstanceCredential(binding *v1alpha1.Servic v1alpha1.ConditionFalse, errorUnbindCallReason, "Unbind call failed. "+s) + c.setServiceInstanceCredentialCondition( + toUpdate, + v1alpha1.ServiceInstanceCredentialConditionFailed, + v1alpha1.ConditionTrue, + errorUnbindCallReason, + "Unbind call failed. "+s) + toUpdate.Status.OperationStartTime = nil c.updateServiceInstanceCredentialStatus(toUpdate) c.recorder.Event(binding, api.EventTypeWarning, errorUnbindCallReason, s) - return err + return nil } s := fmt.Sprintf( "Error unbinding ServiceInstanceCredential \"%s/%s\" for ServiceInstance \"%s/%s\" of ServiceClass %q at ServiceBroker %q: %s", @@ -445,8 +507,25 @@ func (c *controller) reconcileServiceInstanceCredential(binding *v1alpha1.Servic v1alpha1.ConditionFalse, errorUnbindCallReason, "Unbind call failed. "+s) - c.updateServiceInstanceCredentialStatus(toUpdate) c.recorder.Event(binding, api.EventTypeWarning, errorUnbindCallReason, s) + + if binding.Status.OperationStartTime == nil { + toUpdate.Status.OperationStartTime = &now + } else if !time.Now().Before(binding.Status.OperationStartTime.Time.Add(c.reconciliationRetryDuration)) { + s := fmt.Sprintf(`Stopping reconciliation retries on ServiceInstanceCredential "%v/%v" because too much time has elapsed`, binding.Namespace, binding.Name) + glog.Info(s) + c.recorder.Event(binding, api.EventTypeWarning, errorReconciliationRetryTimeoutReason, s) + c.setServiceInstanceCredentialCondition(toUpdate, + v1alpha1.ServiceInstanceCredentialConditionFailed, + v1alpha1.ConditionTrue, + errorReconciliationRetryTimeoutReason, + s) + toUpdate.Status.OperationStartTime = nil + c.updateServiceInstanceCredentialStatus(toUpdate) + return nil + } + + c.updateServiceInstanceCredentialStatus(toUpdate) return err } @@ -457,6 +536,7 @@ func (c *controller) reconcileServiceInstanceCredential(binding *v1alpha1.Servic successUnboundReason, "The binding was deleted successfully", ) + toUpdate.Status.OperationStartTime = nil c.updateServiceInstanceCredentialStatus(toUpdate) // Clear the finalizer finalizers.Delete(v1alpha1.FinalizerServiceCatalog) diff --git a/pkg/controller/controller_binding_test.go b/pkg/controller/controller_binding_test.go index 60ec5c1f35d..53db4b85617 100644 --- a/pkg/controller/controller_binding_test.go +++ b/pkg/controller/controller_binding_test.go @@ -1069,8 +1069,8 @@ func TestReconcileServiceInstanceCredentialWithServiceBrokerHTTPError(t *testing } err := testController.reconcileServiceInstanceCredential(binding) - if err == nil { - t.Fatal("reconcileServiceInstanceCredential should have returned an error") + if err != nil { + t.Fatal("reconcileServiceInstanceCredential should not have returned an error") } events := getRecordedEvents(testController) @@ -1183,8 +1183,8 @@ func TestReconcileServiceInstanceCredentialWithServiceInstanceCredentialFailure( binding := getTestServiceInstanceCredential() - if err := testController.reconcileServiceInstanceCredential(binding); err == nil { - t.Fatal("ServiceInstanceCredential creation should fail") + if err := testController.reconcileServiceInstanceCredential(binding); err != nil { + t.Fatalf("ServiceInstanceCredential creation should complete: %v", err) } // verify one kube action occurred @@ -1387,7 +1387,7 @@ func TestUpdateServiceInstanceCredentialCondition(t *testing.T) { // TestReconcileUnbindingWithBrokerError tests reconcileBinding to ensure an // unbinding request response that contains a broker error fails as expected. func TestReconcileUnbindingWithServiceBrokerError(t *testing.T) { - _, _, _, testController, sharedInformers := newTestController(t, fakeosb.FakeClientConfiguration{ + _, fakeCatalogClient, _, testController, sharedInformers := newTestController(t, fakeosb.FakeClientConfiguration{ UnbindReaction: &fakeosb.UnbindReaction{ Response: &osb.UnbindResponse{}, Error: fakeosb.UnexpectedActionError(), @@ -1419,6 +1419,15 @@ func TestReconcileUnbindingWithServiceBrokerError(t *testing.T) { t.Fatal("reconcileServiceInstanceCredential should have returned an error") } + actions := fakeCatalogClient.Actions() + assertNumberOfActions(t, actions, 1) + + updatedServiceInstanceCredential := assertUpdateStatus(t, actions[0], binding) + assertServiceInstanceCredentialReadyFalse(t, updatedServiceInstanceCredential) + + assertServiceInstanceCredentialReconciledGeneration(t, updatedServiceInstanceCredential, 0) + assertServiceInstanceCredentialOperationStartTimeSet(t, updatedServiceInstanceCredential, true) + events := getRecordedEvents(testController) expectedEvent := api.EventTypeWarning + " " + errorUnbindCallReason + " " + `Error unbinding ServiceInstanceCredential "test-binding/test-ns" for ServiceInstance "test-ns/test-instance" of ServiceClass "test-serviceclass" at ServiceBroker "test-broker": Unexpected action` if 1 != len(events) { @@ -1433,7 +1442,7 @@ func TestReconcileUnbindingWithServiceBrokerError(t *testing.T) { // unbinding request response that contains a broker HTTP error fails as // expected. func TestReconcileUnbindingWithServiceBrokerHTTPError(t *testing.T) { - _, _, _, testController, sharedInformers := newTestController(t, fakeosb.FakeClientConfiguration{ + _, fakeCatalogClient, _, testController, sharedInformers := newTestController(t, fakeosb.FakeClientConfiguration{ UnbindReaction: &fakeosb.UnbindReaction{ Response: &osb.UnbindResponse{}, Error: osb.HTTPStatusCodeError{ @@ -1463,12 +1472,19 @@ func TestReconcileUnbindingWithServiceBrokerHTTPError(t *testing.T) { if err := scmeta.AddFinalizer(binding, v1alpha1.FinalizerServiceCatalog); err != nil { t.Fatalf("Finalizer error: %v", err) } - if err := testController.reconcileServiceInstanceCredential(binding); err == nil { - t.Fatal("reconcileServiceInstanceCredential should have returned an error") + if err := testController.reconcileServiceInstanceCredential(binding); err != nil { + t.Fatalf("reconcileServiceInstanceCredential should not have returned an error: %v", err) } + actions := fakeCatalogClient.Actions() + assertNumberOfActions(t, actions, 1) + updatedServiceInstanceCredential := assertUpdateStatus(t, actions[0], binding) + assertServiceInstanceCredentialReadyFalse(t, updatedServiceInstanceCredential) + assertServiceInstanceCredentialCondition(t, updatedServiceInstanceCredential, v1alpha1.ServiceInstanceCredentialConditionFailed, v1alpha1.ConditionTrue) + assertServiceInstanceCredentialOperationStartTimeSet(t, updatedServiceInstanceCredential, false) + events := getRecordedEvents(testController) - expectedEvent := api.EventTypeWarning + " " + errorUnbindCallReason + " " + `Error creating Unbinding "test-binding/test-ns" for ServiceInstance "test-ns/test-instance" of ServiceClass "test-serviceclass" at ServiceBroker "test-broker", Status: 410; ErrorMessage: ; Description: ; ResponseError: ` + expectedEvent := api.EventTypeWarning + " " + errorUnbindCallReason + " " + `Error unbinding ServiceInstanceCredential "test-binding/test-ns" for ServiceInstance "test-ns/test-instance" of ServiceClass "test-serviceclass" at ServiceBroker "test-broker": Status: 410; ErrorMessage: ; Description: ; ResponseError: ` if 1 != len(events) { t.Fatalf("Did not record expected event, expecting: %v", expectedEvent) } @@ -1573,3 +1589,199 @@ func TestReconcileBindingDeleteUsingOriginatingIdentity(t *testing.T) { }() } } + +// TestReconcileBindingSuccessOnFinalRetry verifies that reconciliation can +// succeed on the last attempt before timing out of the retry loop +func TestReconcileBindingSuccessOnFinalRetry(t *testing.T) { + fakeKubeClient, fakeCatalogClient, fakeServiceBrokerClient, testController, sharedInformers := newTestController(t, fakeosb.FakeClientConfiguration{ + BindReaction: &fakeosb.BindReaction{ + Response: &osb.BindResponse{ + Credentials: map[string]interface{}{ + "a": "b", + "c": "d", + }, + }, + }, + }) + + addGetNamespaceReaction(fakeKubeClient) + addGetSecretNotFoundReaction(fakeKubeClient) + + sharedInformers.ServiceBrokers().Informer().GetStore().Add(getTestServiceBroker()) + sharedInformers.ServiceClasses().Informer().GetStore().Add(getTestServiceClass()) + sharedInformers.ServiceInstances().Informer().GetStore().Add(getTestServiceInstanceWithStatus(v1alpha1.ConditionTrue)) + + binding := getTestServiceInstanceCredential() + startTime := metav1.NewTime(time.Now().Add(-7 * 24 * time.Hour)) + binding.Status.OperationStartTime = &startTime + + if err := testController.reconcileServiceInstanceCredential(binding); err != nil { + t.Fatalf("a valid binding should not fail: %v", err) + } + + brokerActions := fakeServiceBrokerClient.Actions() + assertNumberOfServiceBrokerActions(t, brokerActions, 1) + assertBind(t, brokerActions[0], &osb.BindRequest{ + BindingID: bindingGUID, + InstanceID: instanceGUID, + ServiceID: serviceClassGUID, + PlanID: planGUID, + AppGUID: strPtr(testNsUID), + BindResource: &osb.BindResource{ + AppGUID: strPtr(testNsUID), + }, + }) + + actions := fakeCatalogClient.Actions() + assertNumberOfActions(t, actions, 1) + updatedServiceInstanceCredential := assertUpdateStatus(t, actions[0], binding).(*v1alpha1.ServiceInstanceCredential) + assertServiceInstanceCredentialReadyTrue(t, updatedServiceInstanceCredential) + assertServiceInstanceCredentialOperationStartTimeSet(t, updatedServiceInstanceCredential, false) + + events := getRecordedEvents(testController) + assertNumEvents(t, events, 1) + + expectedEvent := api.EventTypeNormal + " " + successInjectedBindResultReason + " " + successInjectedBindResultMessage + if e, a := expectedEvent, events[0]; e != a { + t.Fatalf("Received unexpected event: %v", a) + } +} + +// TestReconcileBindingFailureOnFinalRetry verifies that reconciliation +// completes in the event of an error after the retry duration elapses. +func TestReconcileBindingFailureOnFinalRetry(t *testing.T) { + _, fakeCatalogClient, _, testController, sharedInformers := newTestController(t, fakeosb.FakeClientConfiguration{ + BindReaction: &fakeosb.BindReaction{ + Response: &osb.BindResponse{ + Credentials: map[string]interface{}{ + "a": "b", + "c": "d", + }, + }, + Error: fakeosb.UnexpectedActionError(), + }, + }) + + sharedInformers.ServiceBrokers().Informer().GetStore().Add(getTestServiceBroker()) + sharedInformers.ServiceClasses().Informer().GetStore().Add(getTestServiceClass()) + sharedInformers.ServiceInstances().Informer().GetStore().Add(getTestServiceInstanceWithStatus(v1alpha1.ConditionTrue)) + + binding := getTestServiceInstanceCredential() + startTime := metav1.NewTime(time.Now().Add(-7 * 24 * time.Hour)) + binding.Status.OperationStartTime = &startTime + + if err := testController.reconcileServiceInstanceCredential(binding); err != nil { + t.Fatalf("Should have return no error because the retry duration has elapsed: %v", err) + } + + actions := fakeCatalogClient.Actions() + assertNumberOfActions(t, actions, 1) + updatedServiceInstanceCredential := assertUpdateStatus(t, actions[0], binding).(*v1alpha1.ServiceInstanceCredential) + assertServiceInstanceCredentialReadyFalse(t, updatedServiceInstanceCredential) + assertServiceInstanceCredentialCondition(t, updatedServiceInstanceCredential, v1alpha1.ServiceInstanceCredentialConditionFailed, v1alpha1.ConditionTrue, errorReconciliationRetryTimeoutReason) + assertServiceInstanceCredentialOperationStartTimeSet(t, updatedServiceInstanceCredential, false) + + expectedEventPrefixes := []string{ + api.EventTypeWarning + " " + errorBindCallReason, + api.EventTypeWarning + " " + errorReconciliationRetryTimeoutReason, + } + events := getRecordedEvents(testController) + assertNumEvents(t, events, len(expectedEventPrefixes)) + + for i, e := range expectedEventPrefixes { + a := events[i] + if !strings.HasPrefix(a, e) { + t.Fatalf("Received unexpected event:\n expected prefix: %v\n got: %v", e, a) + } + } +} + +// TestReconcileBindingWithSecretConflictFailedAfterFinalRetry tests +// reconcileBinding to ensure a binding with an existing secret not owned by the +// bindings is marked as failed after the retry duration elapses. +func TestReconcileBindingWithSecretConflictFailedAfterFinalRetry(t *testing.T) { + fakeKubeClient, fakeCatalogClient, fakeServiceBrokerClient, testController, sharedInformers := newTestController(t, fakeosb.FakeClientConfiguration{ + BindReaction: &fakeosb.BindReaction{ + Response: &osb.BindResponse{ + Credentials: map[string]interface{}{ + "a": "b", + "c": "d", + }, + }, + }, + }) + + addGetNamespaceReaction(fakeKubeClient) + // existing Secret with nil controllerRef + addGetSecretReaction(fakeKubeClient, &v1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: testServiceInstanceCredentialName, Namespace: testNamespace}, + }) + + sharedInformers.ServiceBrokers().Informer().GetStore().Add(getTestServiceBroker()) + sharedInformers.ServiceClasses().Informer().GetStore().Add(getTestServiceClass()) + sharedInformers.ServiceInstances().Informer().GetStore().Add(getTestServiceInstanceWithStatus(v1alpha1.ConditionTrue)) + + binding := &v1alpha1.ServiceInstanceCredential{ + ObjectMeta: metav1.ObjectMeta{ + Name: testServiceInstanceCredentialName, + Namespace: testNamespace, + Generation: 1, + }, + Spec: v1alpha1.ServiceInstanceCredentialSpec{ + ServiceInstanceRef: v1.LocalObjectReference{Name: testServiceInstanceName}, + ExternalID: bindingGUID, + SecretName: testServiceInstanceCredentialSecretName, + }, + } + startTime := metav1.NewTime(time.Now().Add(-7 * 24 * time.Hour)) + binding.Status.OperationStartTime = &startTime + + if err := testController.reconcileServiceInstanceCredential(binding); err != nil { + t.Fatalf("reconciliation should complete since the retry duration has elapsed: %v", err) + } + + brokerActions := fakeServiceBrokerClient.Actions() + assertNumberOfServiceBrokerActions(t, brokerActions, 1) + assertBind(t, brokerActions[0], &osb.BindRequest{ + BindingID: bindingGUID, + InstanceID: instanceGUID, + ServiceID: serviceClassGUID, + PlanID: planGUID, + AppGUID: strPtr(testNsUID), + BindResource: &osb.BindResource{ + AppGUID: strPtr(testNsUID), + }, + }) + + actions := fakeCatalogClient.Actions() + assertNumberOfActions(t, actions, 1) + + updatedServiceInstanceCredential := assertUpdateStatus(t, actions[0], binding).(*v1alpha1.ServiceInstanceCredential) + assertServiceInstanceCredentialReadyFalse(t, updatedServiceInstanceCredential) + assertServiceInstanceCredentialCondition(t, updatedServiceInstanceCredential, v1alpha1.ServiceInstanceCredentialConditionFailed, v1alpha1.ConditionTrue) + + kubeActions := fakeKubeClient.Actions() + assertNumberOfActions(t, kubeActions, 2) + + // first action is a get on the namespace + // second action is a get on the secret + action := kubeActions[1].(clientgotesting.GetAction) + if e, a := "get", action.GetVerb(); e != a { + t.Fatalf("Unexpected verb on action; expected %v, got %v", e, a) + } + if e, a := "secrets", action.GetResource().Resource; e != a { + t.Fatalf("Unexpected resource on action; expected %v, got %v", e, a) + } + + expectedEventPrefixes := []string{ + api.EventTypeWarning + " " + errorInjectingBindResultReason, + api.EventTypeWarning + " " + errorReconciliationRetryTimeoutReason, + } + events := getRecordedEvents(testController) + assertNumEvents(t, events, len(expectedEventPrefixes)) + for i, e := range expectedEventPrefixes { + if a := events[i]; !strings.HasPrefix(a, e) { + t.Fatalf("Received unexpected event:\n expected prefix: %v\n got: %v", e, a) + } + } +} diff --git a/pkg/controller/controller_broker.go b/pkg/controller/controller_broker.go index b9d28cd0b4b..ae52cee3fff 100644 --- a/pkg/controller/controller_broker.go +++ b/pkg/controller/controller_broker.go @@ -93,6 +93,7 @@ const ( errorServiceInstanceNotReadyReason string = "ErrorInstanceNotReady" errorPollingLastOperationReason string = "ErrorPollingLastOperation" errorWithOriginatingIdentity string = "Error with Originating Identity" + errorReconciliationRetryTimeoutReason string = "ErrorReconciliationRetryTimeout" successInjectedBindResultReason string = "InjectedBindResult" successInjectedBindResultMessage string = "Injected bind result" @@ -203,6 +204,7 @@ func (c *controller) reconcileServiceBroker(broker *v1alpha1.ServiceBroker) erro } glog.V(4).Infof("Adding/Updating ServiceBroker %v", broker.Name) + now := metav1.Now() brokerCatalog, err := brokerClient.GetCatalog() if err != nil { s := fmt.Sprintf("Error getting broker catalog for broker %q: %s", broker.Name, err) @@ -210,10 +212,50 @@ func (c *controller) reconcileServiceBroker(broker *v1alpha1.ServiceBroker) erro c.recorder.Eventf(broker, api.EventTypeWarning, errorFetchingCatalogReason, s) c.updateServiceBrokerCondition(broker, v1alpha1.ServiceBrokerConditionReady, v1alpha1.ConditionFalse, errorFetchingCatalogReason, errorFetchingCatalogMessage+s) + if broker.Status.OperationStartTime == nil { + clone, err := api.Scheme.DeepCopy(broker) + if err == nil { + toUpdate := clone.(*v1alpha1.ServiceBroker) + toUpdate.Status.OperationStartTime = &now + _, err := c.serviceCatalogClient.ServiceBrokers().UpdateStatus(toUpdate) + if err != nil { + glog.Errorf("Error updating operation start time of ServiceBroker %q: %v", broker.Name, err) + } + } + } else if !time.Now().Before(broker.Status.OperationStartTime.Time.Add(c.reconciliationRetryDuration)) { + s := fmt.Sprintf("Stopping reconciliation retries on ServiceBroker %q because too much time has elapsed", broker.Name) + glog.Info(s) + c.recorder.Event(broker, api.EventTypeWarning, errorReconciliationRetryTimeoutReason, s) + clone, err := api.Scheme.DeepCopy(broker) + if err == nil { + toUpdate := clone.(*v1alpha1.ServiceBroker) + toUpdate.Status.OperationStartTime = nil + toUpdate.Status.ReconciledGeneration = toUpdate.Generation + c.updateServiceBrokerCondition(toUpdate, + v1alpha1.ServiceBrokerConditionFailed, + v1alpha1.ConditionTrue, + errorReconciliationRetryTimeoutReason, + s) + } + return nil + } return err } glog.V(5).Infof("Successfully fetched %v catalog entries for ServiceBroker %v", len(brokerCatalog.Services), broker.Name) + if broker.Status.OperationStartTime != nil { + clone, err := api.Scheme.DeepCopy(broker) + if err != nil { + return err + } + toUpdate := clone.(*v1alpha1.ServiceBroker) + toUpdate.Status.OperationStartTime = nil + if _, err := c.serviceCatalogClient.ServiceBrokers().UpdateStatus(toUpdate); err != nil { + glog.Errorf("Error updating operation start time of ServiceBroker %q: %v", broker.Name, err) + return err + } + } + glog.V(4).Infof("Converting catalog response for ServiceBroker %v into service-catalog API", broker.Name) catalog, err := convertCatalog(brokerCatalog) if err != nil { diff --git a/pkg/controller/controller_broker_test.go b/pkg/controller/controller_broker_test.go index 5723812291c..df2af282dd0 100644 --- a/pkg/controller/controller_broker_test.go +++ b/pkg/controller/controller_broker_test.go @@ -339,11 +339,14 @@ func TestReconcileServiceBrokerErrorFetchingCatalog(t *testing.T) { assertGetCatalog(t, brokerActions[0]) actions := fakeCatalogClient.Actions() - assertNumberOfActions(t, actions, 1) + assertNumberOfActions(t, actions, 2) updatedServiceBroker := assertUpdateStatus(t, actions[0], broker) assertServiceBrokerReadyFalse(t, updatedServiceBroker) + updatedServiceBroker = assertUpdateStatus(t, actions[1], broker) + assertServiceBrokerOperationStartTimeSet(t, updatedServiceBroker, true) + assertNumberOfActions(t, fakeKubeClient.Actions(), 0) events := getRecordedEvents(testController) @@ -599,6 +602,93 @@ func TestReconcileServiceBrokerWithReconcileError(t *testing.T) { } } +// TestReconcileServiceBrokerSuccessOnFinalRetry verifies that reconciliation can +// succeed on the last attempt before timing out of the retry loop +func TestReconcileServiceBrokerSuccessOnFinalRetry(t *testing.T) { + fakeKubeClient, fakeCatalogClient, fakeServiceBrokerClient, testController, sharedInformers := newTestController(t, getTestCatalogConfig()) + + testServiceClass := getTestServiceClass() + sharedInformers.ServiceClasses().Informer().GetStore().Add(testServiceClass) + + broker := getTestServiceBroker() + startTime := metav1.NewTime(time.Now().Add(-7 * 24 * time.Hour)) + broker.Status.OperationStartTime = &startTime + + if err := testController.reconcileServiceBroker(broker); err != nil { + t.Fatalf("This should not fail : %v", err) + } + + brokerActions := fakeServiceBrokerClient.Actions() + assertNumberOfServiceBrokerActions(t, brokerActions, 1) + assertGetCatalog(t, brokerActions[0]) + + actions := fakeCatalogClient.Actions() + assertNumberOfActions(t, actions, 3) + + // first action should be an update action to clear OperationStartTime + updatedServiceBroker := assertUpdateStatus(t, actions[0], getTestServiceBroker()) + assertServiceBrokerOperationStartTimeSet(t, updatedServiceBroker, false) + + // first action should be an update action for a service class + assertUpdate(t, actions[1], testServiceClass) + + // second action should be an update action for broker status subresource + updatedServiceBroker = assertUpdateStatus(t, actions[2], getTestServiceBroker()) + assertServiceBrokerReadyTrue(t, updatedServiceBroker) + + // verify no kube resources created + kubeActions := fakeKubeClient.Actions() + assertNumberOfActions(t, kubeActions, 0) +} + +// TestReconcileServiceBrokerFailureOnFinalRetry verifies that reconciliation +// completes in the event of an error after the retry duration elapses. +func TestReconcileServiceBrokerFailureOnFinalRetry(t *testing.T) { + fakeKubeClient, fakeCatalogClient, fakeServiceBrokerClient, testController, _ := newTestController(t, fakeosb.FakeClientConfiguration{ + CatalogReaction: &fakeosb.CatalogReaction{ + Error: errors.New("ooops"), + }, + }) + + broker := getTestServiceBroker() + startTime := metav1.NewTime(time.Now().Add(-7 * 24 * time.Hour)) + broker.Status.OperationStartTime = &startTime + + if err := testController.reconcileServiceBroker(broker); err != nil { + t.Fatalf("Should have return no error because the retry duration has elapsed: %v", err) + } + + brokerActions := fakeServiceBrokerClient.Actions() + assertNumberOfServiceBrokerActions(t, brokerActions, 1) + assertGetCatalog(t, brokerActions[0]) + + actions := fakeCatalogClient.Actions() + assertNumberOfActions(t, actions, 2) + + updatedServiceBroker := assertUpdateStatus(t, actions[0], broker) + assertServiceBrokerReadyFalse(t, updatedServiceBroker) + + updatedServiceBroker = assertUpdateStatus(t, actions[1], broker) + assertServiceBrokerCondition(t, updatedServiceBroker, v1alpha1.ServiceBrokerConditionFailed, v1alpha1.ConditionTrue) + assertServiceBrokerOperationStartTimeSet(t, updatedServiceBroker, false) + + assertNumberOfActions(t, fakeKubeClient.Actions(), 0) + + expectedEventPrefixes := []string{ + api.EventTypeWarning + " " + errorFetchingCatalogReason, + api.EventTypeWarning + " " + errorReconciliationRetryTimeoutReason, + } + events := getRecordedEvents(testController) + assertNumEvents(t, events, len(expectedEventPrefixes)) + + for i, e := range expectedEventPrefixes { + a := events[i] + if !strings.HasPrefix(a, e) { + t.Fatalf("Received unexpected event:\n expected prefix: %v\n got: %v", e, a) + } + } +} + // TestUpdateServiceBrokerCondition ensures that with specific conditions // the broker correctly reflects the changes during updateServiceBrokerCondition(). // diff --git a/pkg/controller/controller_instance.go b/pkg/controller/controller_instance.go index afe41decf82..facc6a1168d 100644 --- a/pkg/controller/controller_instance.go +++ b/pkg/controller/controller_instance.go @@ -18,6 +18,7 @@ package controller import ( "fmt" + "time" "github.com/golang/glog" osb "github.com/pmorie/go-open-service-broker-client/v2" @@ -265,6 +266,8 @@ func (c *controller) reconcileServiceInstanceDelete(instance *v1alpha1.ServiceIn // it is arguable we should perform an extract-method refactor on this // code block + now := metav1.Now() + glog.V(4).Infof("Deprovisioning ServiceInstance %v/%v of ServiceClass %v at ServiceBroker %v", instance.Namespace, instance.Name, serviceClass.Name, brokerName) response, err := brokerClient.DeprovisionInstance(request) if err != nil { @@ -288,9 +291,18 @@ func (c *controller) reconcileServiceInstanceDelete(instance *v1alpha1.ServiceIn v1alpha1.ConditionUnknown, errorDeprovisionCalledReason, "Deprovision call failed. "+s) + setServiceInstanceCondition( + toUpdate, + v1alpha1.ServiceInstanceConditionFailed, + v1alpha1.ConditionTrue, + errorDeprovisionCalledReason, + s, + ) c.updateServiceInstanceStatus(toUpdate) c.recorder.Event(instance, api.EventTypeWarning, errorDeprovisionCalledReason, s) - return err + + // Return nil so that the reconciler does not retry the deprovision + return nil } s := fmt.Sprintf( @@ -311,8 +323,26 @@ func (c *controller) reconcileServiceInstanceDelete(instance *v1alpha1.ServiceIn v1alpha1.ConditionUnknown, errorDeprovisionCalledReason, "Deprovision call failed. "+s) - c.updateServiceInstanceStatus(toUpdate) + c.recorder.Event(instance, api.EventTypeWarning, errorDeprovisionCalledReason, s) + + if instance.Status.OperationStartTime == nil { + toUpdate.Status.OperationStartTime = &now + } else if !time.Now().Before(instance.Status.OperationStartTime.Time.Add(c.reconciliationRetryDuration)) { + s := fmt.Sprintf(`Stopping reconciliation retries on ServiceInstance "%v/%v" because too much time has elapsed`, instance.Namespace, instance.Name) + glog.Info(s) + c.recorder.Event(instance, api.EventTypeWarning, errorReconciliationRetryTimeoutReason, s) + setServiceInstanceCondition(toUpdate, + v1alpha1.ServiceInstanceConditionFailed, + v1alpha1.ConditionTrue, + errorReconciliationRetryTimeoutReason, + s) + toUpdate.Status.OperationStartTime = nil + c.updateServiceInstanceStatus(toUpdate) + return nil + } + + c.updateServiceInstanceStatus(toUpdate) return err } @@ -323,6 +353,8 @@ func (c *controller) reconcileServiceInstanceDelete(instance *v1alpha1.ServiceIn toUpdate.Status.LastOperation = &key } + toUpdate.Status.OperationStartTime = &now + // Tag this instance as having an ongoing async operation so we can enforce // no other operations against it can start. toUpdate.Status.AsyncOpInProgress = true @@ -351,6 +383,8 @@ func (c *controller) reconcileServiceInstanceDelete(instance *v1alpha1.ServiceIn glog.V(5).Infof("Deprovision call to broker succeeded for ServiceInstance %v/%v, finalizing", instance.Namespace, instance.Name) + toUpdate.Status.OperationStartTime = nil + setServiceInstanceCondition( toUpdate, v1alpha1.ServiceInstanceConditionReady, @@ -541,6 +575,8 @@ func (c *controller) reconcileServiceInstance(instance *v1alpha1.ServiceInstance request.OriginatingIdentity = originatingIdentity } + now := metav1.Now() + glog.V(4).Infof("Provisioning a new ServiceInstance %v/%v of ServiceClass %v at ServiceBroker %v", instance.Namespace, instance.Name, serviceClass.Name, brokerName) response, err := brokerClient.ProvisionInstance(request) if err != nil { @@ -565,6 +601,8 @@ func (c *controller) reconcileServiceInstance(instance *v1alpha1.ServiceInstance v1alpha1.ConditionFalse, errorProvisionCallFailedReason, "ServiceBroker returned a failure for provision call; operation will not be retried: "+s) + toUpdate.Status.OperationStartTime = nil + toUpdate.Status.ReconciledGeneration = toUpdate.Generation err := c.updateServiceInstanceStatus(toUpdate) if err != nil { return err @@ -583,9 +621,27 @@ func (c *controller) reconcileServiceInstance(instance *v1alpha1.ServiceInstance v1alpha1.ConditionFalse, errorErrorCallingProvisionReason, "Provision call failed and will be retried: "+s) + c.recorder.Event(instance, api.EventTypeWarning, errorErrorCallingProvisionReason, s) + + if instance.Status.OperationStartTime == nil { + toUpdate.Status.OperationStartTime = &now + } else if !time.Now().Before(instance.Status.OperationStartTime.Time.Add(c.reconciliationRetryDuration)) { + s := fmt.Sprintf(`Stopping reconciliation retries on ServiceInstance "%v/%v" because too much time has elapsed`, instance.Namespace, instance.Name) + glog.Info(s) + c.recorder.Event(instance, api.EventTypeWarning, errorReconciliationRetryTimeoutReason, s) + setServiceInstanceCondition(toUpdate, + v1alpha1.ServiceInstanceConditionFailed, + v1alpha1.ConditionTrue, + errorReconciliationRetryTimeoutReason, + s) + toUpdate.Status.OperationStartTime = nil + toUpdate.Status.ReconciledGeneration = toUpdate.Generation + c.updateServiceInstanceStatus(toUpdate) + return nil + } + c.updateServiceInstanceStatus(toUpdate) - c.recorder.Event(instance, api.EventTypeWarning, errorErrorCallingProvisionReason, s) return err } @@ -610,6 +666,8 @@ func (c *controller) reconcileServiceInstance(instance *v1alpha1.ServiceInstance // no other operations against it can start. toUpdate.Status.AsyncOpInProgress = true + toUpdate.Status.OperationStartTime = &now + setServiceInstanceCondition( toUpdate, v1alpha1.ServiceInstanceConditionReady, @@ -627,6 +685,8 @@ func (c *controller) reconcileServiceInstance(instance *v1alpha1.ServiceInstance } else { glog.V(5).Infof("Successfully provisioned ServiceInstance %v/%v of ServiceClass %v at ServiceBroker %v: response: %+v", instance.Namespace, instance.Name, serviceClass.Name, brokerName, response) + toUpdate.Status.OperationStartTime = nil + // Create/Update for Instance has completed successfully, so set // Status.ReconciledGeneration to the Generation used. toUpdate.Status.ReconciledGeneration = toUpdate.Generation @@ -665,6 +725,33 @@ func (c *controller) pollServiceInstance(serviceClass *v1alpha1.ServiceClass, se deleting = true } + // OperationStartTime must be set because we are polling an in-progress + // operation. If it is not set, this is a logical error. Let's bail out. + if instance.Status.OperationStartTime == nil { + clone, err := api.Scheme.DeepCopy(instance) + if err != nil { + return err + } + toUpdate := clone.(*v1alpha1.ServiceInstance) + s := fmt.Sprintf(`Stopping reconciliation retries on ServiceInstance "%v/%v" because the operation start time is not set`, instance.Namespace, instance.Name) + glog.Info(s) + c.recorder.Event(instance, api.EventTypeWarning, errorReconciliationRetryTimeoutReason, s) + setServiceInstanceCondition(toUpdate, + v1alpha1.ServiceInstanceConditionFailed, + v1alpha1.ConditionTrue, + errorReconciliationRetryTimeoutReason, + s) + toUpdate.Status.OperationStartTime = nil + toUpdate.Status.ReconciledGeneration = toUpdate.Generation + if err := c.updateServiceInstanceStatus(toUpdate); err != nil { + return err + } + if err := c.finishPollingServiceInstance(instance); err != nil { + return err + } + return nil + } + request := &osb.LastOperationRequest{ InstanceID: instance.Spec.ExternalID, ServiceID: &serviceClass.ExternalID, @@ -702,6 +789,7 @@ func (c *controller) pollServiceInstance(serviceClass *v1alpha1.ServiceClass, se toUpdate := clone.(*v1alpha1.ServiceInstance) toUpdate.Status.AsyncOpInProgress = false + toUpdate.Status.OperationStartTime = nil c.updateServiceInstanceCondition( toUpdate, v1alpha1.ServiceInstanceConditionReady, @@ -745,6 +833,31 @@ func (c *controller) pollServiceInstance(serviceClass *v1alpha1.ServiceClass, se glog.V(4).Info(s) c.recorder.Event(instance, api.EventTypeWarning, errorPollingLastOperationReason, s) + if !time.Now().Before(instance.Status.OperationStartTime.Time.Add(c.reconciliationRetryDuration)) { + clone, err := api.Scheme.DeepCopy(instance) + if err != nil { + return err + } + toUpdate := clone.(*v1alpha1.ServiceInstance) + s := fmt.Sprintf(`Stopping reconciliation retries on ServiceInstance "%v/%v" because too much time has elapsed`, instance.Namespace, instance.Name) + glog.Info(s) + c.recorder.Event(instance, api.EventTypeWarning, errorReconciliationRetryTimeoutReason, s) + setServiceInstanceCondition(toUpdate, + v1alpha1.ServiceInstanceConditionFailed, + v1alpha1.ConditionTrue, + errorReconciliationRetryTimeoutReason, + s) + toUpdate.Status.OperationStartTime = nil + toUpdate.Status.ReconciledGeneration = toUpdate.Generation + if err := c.updateServiceInstanceStatus(toUpdate); err != nil { + return err + } + if err := c.finishPollingServiceInstance(instance); err != nil { + return err + } + return nil + } + err = c.continuePollingServiceInstance(instance) if err != nil { return err @@ -788,6 +901,32 @@ func (c *controller) pollServiceInstance(serviceClass *v1alpha1.ServiceClass, se ) } + if !time.Now().Before(instance.Status.OperationStartTime.Time.Add(c.reconciliationRetryDuration)) { + clone, err := api.Scheme.DeepCopy(instance) + if err != nil { + return err + } + toUpdate := clone.(*v1alpha1.ServiceInstance) + s := fmt.Sprintf(`Stopping reconciliation retries on ServiceInstance "%v/%v" because too much time has elapsed`, instance.Namespace, instance.Name) + glog.Info(s) + c.recorder.Event(instance, api.EventTypeWarning, errorReconciliationRetryTimeoutReason, s) + setServiceInstanceCondition(toUpdate, + v1alpha1.ServiceInstanceConditionFailed, + v1alpha1.ConditionTrue, + errorReconciliationRetryTimeoutReason, + s) + toUpdate.Status.AsyncOpInProgress = false + toUpdate.Status.OperationStartTime = nil + toUpdate.Status.ReconciledGeneration = toUpdate.Generation + if err := c.updateServiceInstanceStatus(toUpdate); err != nil { + return err + } + if err := c.finishPollingServiceInstance(instance); err != nil { + return err + } + return nil + } + err = c.continuePollingServiceInstance(instance) if err != nil { return err @@ -802,6 +941,7 @@ func (c *controller) pollServiceInstance(serviceClass *v1alpha1.ServiceClass, se } toUpdate := clone.(*v1alpha1.ServiceInstance) toUpdate.Status.AsyncOpInProgress = false + toUpdate.Status.OperationStartTime = nil // If we were asynchronously deleting a Service Instance, finish // the finalizers. @@ -855,22 +995,34 @@ func (c *controller) pollServiceInstance(serviceClass *v1alpha1.ServiceClass, se } toUpdate := clone.(*v1alpha1.ServiceInstance) toUpdate.Status.AsyncOpInProgress = false + toUpdate.Status.OperationStartTime = nil + toUpdate.Status.ReconciledGeneration = toUpdate.Generation - cond := v1alpha1.ConditionFalse + readyCond := v1alpha1.ConditionFalse reason := errorProvisionCallFailedReason msg := "Provision call failed: " + s if deleting { - cond = v1alpha1.ConditionUnknown + readyCond = v1alpha1.ConditionUnknown reason = errorDeprovisionCalledReason msg = "Deprovision call failed:" + s } - c.updateServiceInstanceCondition( + setServiceInstanceCondition( toUpdate, v1alpha1.ServiceInstanceConditionReady, - cond, + readyCond, reason, msg, ) + setServiceInstanceCondition( + toUpdate, + v1alpha1.ServiceInstanceConditionFailed, + v1alpha1.ConditionTrue, + reason, + msg, + ) + if err := c.updateServiceInstanceStatus(toUpdate); err != nil { + return err + } c.recorder.Event(instance, api.EventTypeWarning, errorDeprovisionCalledReason, s) err = c.finishPollingServiceInstance(instance) @@ -879,6 +1031,30 @@ func (c *controller) pollServiceInstance(serviceClass *v1alpha1.ServiceClass, se } default: glog.Warningf("Got invalid state in LastOperationResponse: %q", response.State) + if !time.Now().Before(instance.Status.OperationStartTime.Time.Add(c.reconciliationRetryDuration)) { + clone, err := api.Scheme.DeepCopy(instance) + if err != nil { + return err + } + toUpdate := clone.(*v1alpha1.ServiceInstance) + s := fmt.Sprintf(`Stopping reconciliation retries on ServiceInstance "%v/%v" because too much time has elapsed`, instance.Namespace, instance.Name) + glog.Info(s) + c.recorder.Event(instance, api.EventTypeWarning, errorReconciliationRetryTimeoutReason, s) + setServiceInstanceCondition(toUpdate, + v1alpha1.ServiceInstanceConditionFailed, + v1alpha1.ConditionTrue, + errorReconciliationRetryTimeoutReason, + s) + toUpdate.Status.OperationStartTime = nil + toUpdate.Status.ReconciledGeneration = toUpdate.Generation + if err := c.updateServiceInstanceStatus(toUpdate); err != nil { + return err + } + if err := c.finishPollingServiceInstance(instance); err != nil { + return err + } + return nil + } return fmt.Errorf("Got invalid state in LastOperationResponse: %q", response.State) } return nil diff --git a/pkg/controller/controller_instance_test.go b/pkg/controller/controller_instance_test.go index f61ca0e53a4..381748d7270 100644 --- a/pkg/controller/controller_instance_test.go +++ b/pkg/controller/controller_instance_test.go @@ -1269,6 +1269,7 @@ func TestPollServiceInstanceFailureProvisioningWithOperation(t *testing.T) { // ServiceInstance should be not ready and there no longer is an async operation // in place. assertServiceInstanceReadyFalse(t, updatedServiceInstance) + assertServiceInstanceCondition(t, updatedServiceInstance, v1alpha1.ServiceInstanceConditionFailed, v1alpha1.ConditionTrue) assertAsyncOpInProgressFalse(t, updatedServiceInstance) } @@ -1437,6 +1438,7 @@ func TestPollServiceInstanceFailureDeprovisioningWithOperation(t *testing.T) { // ServiceInstance should be set to unknown since the operation on the broker // failed. assertServiceInstanceReadyCondition(t, updatedServiceInstance, v1alpha1.ConditionUnknown, errorDeprovisionCalledReason) + assertServiceInstanceCondition(t, updatedServiceInstance, v1alpha1.ServiceInstanceConditionFailed, v1alpha1.ConditionTrue) assertAsyncOpInProgressFalse(t, updatedServiceInstance) events := getRecordedEvents(testController) @@ -1635,6 +1637,245 @@ func TestPollServiceInstanceSuccessDeprovisioningWithOperationWithFinalizer(t *t } } +// TestReconcileServiceInstanceSuccessOnFinalRetry verifies that reconciliation +// can succeed on the last attempt before timing out of the retry loop +func TestReconcileServiceInstanceSuccessOnFinalRetry(t *testing.T) { + fakeKubeClient, fakeCatalogClient, fakeServiceBrokerClient, testController, sharedInformers := newTestController(t, fakeosb.FakeClientConfiguration{ + ProvisionReaction: &fakeosb.ProvisionReaction{ + Response: &osb.ProvisionResponse{}, + }, + }) + + sharedInformers.ServiceBrokers().Informer().GetStore().Add(getTestServiceBroker()) + sharedInformers.ServiceClasses().Informer().GetStore().Add(getTestServiceClass()) + + instance := getTestServiceInstance() + startTime := metav1.NewTime(time.Now().Add(-7 * 24 * time.Hour)) + instance.Status.OperationStartTime = &startTime + + if err := testController.reconcileServiceInstance(instance); err != nil { + t.Fatalf("This should not fail : %v", err) + } + + brokerActions := fakeServiceBrokerClient.Actions() + assertNumberOfServiceBrokerActions(t, brokerActions, 1) + assertProvision(t, brokerActions[0], &osb.ProvisionRequest{ + AcceptsIncomplete: true, + InstanceID: instanceGUID, + ServiceID: serviceClassGUID, + PlanID: planGUID, + Context: map[string]interface{}{ + "platform": "kubernetes", + "namespace": "test-ns", + }, + }) + + actions := fakeCatalogClient.Actions() + assertNumberOfActions(t, actions, 1) + + updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) + assertServiceInstanceReadyTrue(t, updatedServiceInstance) + assertServiceInstanceOperationStartTimeSet(t, updatedServiceInstance, false) + + // verify no kube resources created + // One single action comes from getting namespace uid + kubeActions := fakeKubeClient.Actions() + if err := checkKubeClientActions(kubeActions, []kubeClientAction{ + {verb: "get", resourceName: "namespaces", checkType: checkGetActionType}, + }); err != nil { + t.Fatal(err) + } + + events := getRecordedEvents(testController) + assertNumEvents(t, events, 1) + + expectedEvent := api.EventTypeNormal + " " + successProvisionReason + " " + "The instance was provisioned successfully" + if e, a := expectedEvent, events[0]; e != a { + t.Fatalf("Received unexpected event: %v", a) + } +} + +// TestReconcileServiceInstanceFailureOnFinalRetry verifies that reconciliation +// completes in the event of an error after the retry duration elapses. +func TestReconcileServiceInstanceFailureOnFinalRetry(t *testing.T) { + fakeKubeClient, fakeCatalogClient, fakeServiceBrokerClient, testController, sharedInformers := newTestController(t, fakeosb.FakeClientConfiguration{ + ProvisionReaction: &fakeosb.ProvisionReaction{ + Error: errors.New("fake creation failure"), + }, + }) + + sharedInformers.ServiceBrokers().Informer().GetStore().Add(getTestServiceBroker()) + sharedInformers.ServiceClasses().Informer().GetStore().Add(getTestServiceClass()) + + instance := getTestServiceInstance() + startTime := metav1.NewTime(time.Now().Add(-7 * 24 * time.Hour)) + instance.Status.OperationStartTime = &startTime + + if err := testController.reconcileServiceInstance(instance); err != nil { + t.Fatalf("Should have return no error because the retry duration has elapsed: %v", err) + } + + brokerActions := fakeServiceBrokerClient.Actions() + assertNumberOfServiceBrokerActions(t, brokerActions, 1) + assertProvision(t, brokerActions[0], &osb.ProvisionRequest{ + AcceptsIncomplete: true, + InstanceID: instanceGUID, + ServiceID: serviceClassGUID, + PlanID: planGUID, + Context: map[string]interface{}{ + "platform": "kubernetes", + "namespace": "test-ns", + }, + }) + + // verify no kube resources created + // One single action comes from getting namespace uid + kubeActions := fakeKubeClient.Actions() + if err := checkKubeClientActions(kubeActions, []kubeClientAction{ + {verb: "get", resourceName: "namespaces", checkType: checkGetActionType}, + }); err != nil { + t.Fatal(err) + } + + actions := fakeCatalogClient.Actions() + assertNumberOfActions(t, actions, 1) + + updatedObject := assertUpdateStatus(t, actions[0], instance) + assertServiceInstanceReadyFalse(t, updatedObject) + assertServiceInstanceCondition(t, updatedObject, v1alpha1.ServiceInstanceConditionFailed, v1alpha1.ConditionTrue, errorReconciliationRetryTimeoutReason) + assertServiceInstanceOperationStartTimeSet(t, updatedObject, false) + + expectedEventPrefixes := []string{ + api.EventTypeWarning + " " + errorErrorCallingProvisionReason, + api.EventTypeWarning + " " + errorReconciliationRetryTimeoutReason, + } + events := getRecordedEvents(testController) + assertNumEvents(t, events, len(expectedEventPrefixes)) + + for i, e := range expectedEventPrefixes { + a := events[i] + if !strings.HasPrefix(a, e) { + t.Fatalf("Received unexpected event:\n expected prefix: %v\n got: %v", e, a) + } + } +} + +// TestPollServiceInstanceSuccessOnFinalRetry verifies that polling +// can succeed on the last attempt before timing out of the retry loop +func TestPollServiceInstanceSuccessOnFinalRetry(t *testing.T) { + fakeKubeClient, fakeCatalogClient, fakeServiceBrokerClient, testController, sharedInformers := newTestController(t, fakeosb.FakeClientConfiguration{ + PollLastOperationReaction: &fakeosb.PollLastOperationReaction{ + Response: &osb.LastOperationResponse{ + State: osb.StateSucceeded, + Description: strPtr(lastOperationDescription), + }, + }, + }) + + sharedInformers.ServiceBrokers().Informer().GetStore().Add(getTestServiceBroker()) + sharedInformers.ServiceClasses().Informer().GetStore().Add(getTestServiceClass()) + + instance := getTestServiceInstanceAsyncProvisioning(testOperation) + instanceKey := testNamespace + "/" + testServiceInstanceName + startTime := metav1.NewTime(time.Now().Add(-7 * 24 * time.Hour)) + instance.Status.OperationStartTime = &startTime + + if testController.pollingQueue.NumRequeues(instanceKey) != 0 { + t.Fatalf("Expected polling queue to not have any record of test instance") + } + + if err := testController.pollServiceInstanceInternal(instance); err != nil { + t.Fatalf("pollServiceInstanceInternal failed: %s", err) + } + + if testController.pollingQueue.NumRequeues(instanceKey) != 0 { + t.Fatalf("Expected polling queue to not have any record of test instance as polling should have completed") + } + + brokerActions := fakeServiceBrokerClient.Actions() + assertNumberOfServiceBrokerActions(t, brokerActions, 1) + assertPollLastOperation(t, brokerActions[0], &osb.LastOperationRequest{ + InstanceID: instanceGUID, + ServiceID: strPtr(serviceClassGUID), + PlanID: strPtr(planGUID), + }) + + // verify no kube resources created. + // No actions + kubeActions := fakeKubeClient.Actions() + assertNumberOfActions(t, kubeActions, 0) + + actions := fakeCatalogClient.Actions() + assertNumberOfActions(t, actions, 1) + + updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) + // ServiceInstance should be ready and there no longer is an async operation + // in place. + assertServiceInstanceReadyTrue(t, updatedServiceInstance) + assertAsyncOpInProgressFalse(t, updatedServiceInstance) + assertServiceInstanceOperationStartTimeSet(t, updatedServiceInstance, false) +} + +// TestPollServiceInstanceFailureOnFinalRetry verifies that polling +// completes in the event of an error after the retry duration elapses. +func TestPollServiceInstanceFailureOnFinalRetry(t *testing.T) { + fakeKubeClient, fakeCatalogClient, fakeServiceBrokerClient, testController, sharedInformers := newTestController(t, fakeosb.FakeClientConfiguration{ + PollLastOperationReaction: &fakeosb.PollLastOperationReaction{ + Response: &osb.LastOperationResponse{ + State: osb.StateInProgress, + Description: strPtr(lastOperationDescription), + }, + }, + }) + + sharedInformers.ServiceBrokers().Informer().GetStore().Add(getTestServiceBroker()) + sharedInformers.ServiceClasses().Informer().GetStore().Add(getTestServiceClass()) + + instance := getTestServiceInstanceAsyncProvisioning(testOperation) + instanceKey := testNamespace + "/" + testServiceInstanceName + startTime := metav1.NewTime(time.Now().Add(-7 * 24 * time.Hour)) + instance.Status.OperationStartTime = &startTime + + if testController.pollingQueue.NumRequeues(instanceKey) != 0 { + t.Fatalf("Expected polling queue to not have any record of test instance") + } + + if err := testController.pollServiceInstanceInternal(instance); err != nil { + t.Fatalf("Should have return no error because the retry duration has elapsed: %v", err) + } + + if testController.pollingQueue.NumRequeues(instanceKey) != 0 { + t.Fatalf("Expected polling queue to not have any record of test instance as polling should have completed") + } + + brokerActions := fakeServiceBrokerClient.Actions() + assertNumberOfServiceBrokerActions(t, brokerActions, 1) + assertPollLastOperation(t, brokerActions[0], &osb.LastOperationRequest{ + InstanceID: instanceGUID, + ServiceID: strPtr(serviceClassGUID), + PlanID: strPtr(planGUID), + }) + + // there should have been 2 actions: + // (1) update the status with the last operation description + // (2) update the status with the Failed condition + actions := fakeCatalogClient.Actions() + assertNumberOfActions(t, actions, 2) + + updatedServiceInstance := assertUpdateStatus(t, actions[0], instance) + assertServiceInstanceReadyFalse(t, updatedServiceInstance) + + updatedServiceInstance = assertUpdateStatus(t, actions[1], instance) + assertServiceInstanceCondition(t, updatedServiceInstance, v1alpha1.ServiceInstanceConditionFailed, v1alpha1.ConditionTrue, errorReconciliationRetryTimeoutReason) + assertAsyncOpInProgressFalse(t, updatedServiceInstance) + assertServiceInstanceOperationStartTimeSet(t, updatedServiceInstance, false) + + // verify no kube resources created. + // No actions + kubeActions := fakeKubeClient.Actions() + assertNumberOfActions(t, kubeActions, 0) +} + // TestSetServiceInstanceCondition ensures that with the expected conditions the // SetServiceInstanceCondition() updates a status properly with the given condition // The test cases are proving: diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go index 6e18ec7079f..83016a9eeb0 100644 --- a/pkg/controller/controller_test.go +++ b/pkg/controller/controller_test.go @@ -513,6 +513,7 @@ func getTestServiceInstanceAsyncProvisioning(operation string) *v1alpha1.Service if operation != "" { instance.Status.LastOperation = &operation } + operationStartTime := metav1.NewTime(time.Now().Add(-1 * time.Hour)) instance.Status = v1alpha1.ServiceInstanceStatus{ Conditions: []v1alpha1.ServiceInstanceCondition{{ Type: v1alpha1.ServiceInstanceConditionReady, @@ -520,7 +521,8 @@ func getTestServiceInstanceAsyncProvisioning(operation string) *v1alpha1.Service Message: "Provisioning", LastTransitionTime: metav1.NewTime(time.Now().Add(-5 * time.Minute)), }}, - AsyncOpInProgress: true, + AsyncOpInProgress: true, + OperationStartTime: &operationStartTime, } return instance @@ -531,6 +533,7 @@ func getTestServiceInstanceAsyncDeprovisioning(operation string) *v1alpha1.Servi if operation != "" { instance.Status.LastOperation = &operation } + operationStartTime := metav1.NewTime(time.Now().Add(-1 * time.Hour)) instance.Status = v1alpha1.ServiceInstanceStatus{ Conditions: []v1alpha1.ServiceInstanceCondition{{ Type: v1alpha1.ServiceInstanceConditionReady, @@ -538,7 +541,8 @@ func getTestServiceInstanceAsyncDeprovisioning(operation string) *v1alpha1.Servi Message: "Deprovisioning", LastTransitionTime: metav1.NewTime(time.Now().Add(-5 * time.Minute)), }}, - AsyncOpInProgress: true, + AsyncOpInProgress: true, + OperationStartTime: &operationStartTime, } // Set the deleted timestamp to simulate deletion @@ -1065,6 +1069,7 @@ func newTestController(t *testing.T, config fakeosb.FakeClientConfiguration) ( 24*time.Hour, osb.LatestAPIVersion().HeaderValue(), fakeRecorder, + 7*24*time.Hour, ) if err != nil { t.Fatal(err) @@ -1116,6 +1121,7 @@ func newTestControllerWithServiceBrokerServer( 24*time.Hour, osb.LatestAPIVersion().HeaderValue(), fakeRecorder, + 7*24*time.Hour, ) if err != nil { return nil, err @@ -1347,22 +1353,37 @@ func testActionFor(t *testing.T, name string, f failfFunc, action clientgotestin } func assertServiceBrokerReadyTrue(t *testing.T, obj runtime.Object) { - assertServiceBrokerReadyCondition(t, obj, v1alpha1.ConditionTrue) + assertServiceBrokerCondition(t, obj, v1alpha1.ServiceBrokerConditionReady, v1alpha1.ConditionTrue) } func assertServiceBrokerReadyFalse(t *testing.T, obj runtime.Object) { - assertServiceBrokerReadyCondition(t, obj, v1alpha1.ConditionFalse) + assertServiceBrokerCondition(t, obj, v1alpha1.ServiceBrokerConditionReady, v1alpha1.ConditionFalse) } -func assertServiceBrokerReadyCondition(t *testing.T, obj runtime.Object, status v1alpha1.ConditionStatus) { +func assertServiceBrokerCondition(t *testing.T, obj runtime.Object, conditionType v1alpha1.ServiceBrokerConditionType, status v1alpha1.ConditionStatus) { broker, ok := obj.(*v1alpha1.ServiceBroker) if !ok { fatalf(t, "Couldn't convert object %+v into a *v1alpha1.ServiceBroker", obj) } for _, condition := range broker.Status.Conditions { - if condition.Type == v1alpha1.ServiceBrokerConditionReady && condition.Status != status { - fatalf(t, "ready condition had unexpected status; expected %v, got %v", status, condition.Status) + if condition.Type == conditionType && condition.Status != status { + fatalf(t, "%v condition had unexpected status; expected %v, got %v", conditionType, status, condition.Status) + } + } +} + +func assertServiceBrokerOperationStartTimeSet(t *testing.T, obj runtime.Object, isOperationStartTimeSet bool) { + broker, ok := obj.(*v1alpha1.ServiceBroker) + if !ok { + fatalf(t, "Couldn't convert object %+v into a *v1alpha1.ServiceBroker", obj) + } + + if e, a := isOperationStartTimeSet, broker.Status.OperationStartTime != nil; e != a { + if e { + fatalf(t, "expected OperationStartTime to not be nil, but was") + } else { + fatalf(t, "expected OperationStartTime to be nil, but was not") } } } @@ -1376,17 +1397,66 @@ func assertServiceInstanceReadyFalse(t *testing.T, obj runtime.Object, reason .. } func assertServiceInstanceReadyCondition(t *testing.T, obj runtime.Object, status v1alpha1.ConditionStatus, reason ...string) { + assertServiceInstanceCondition(t, obj, v1alpha1.ServiceInstanceConditionReady, status, reason...) +} + +func assertServiceInstanceCondition(t *testing.T, obj runtime.Object, conditionType v1alpha1.ServiceInstanceConditionType, status v1alpha1.ConditionStatus, reason ...string) { instance, ok := obj.(*v1alpha1.ServiceInstance) if !ok { fatalf(t, "Couldn't convert object %+v into a *v1alpha1.ServiceInstance", obj) } + foundCondition := false for _, condition := range instance.Status.Conditions { - if condition.Type == v1alpha1.ServiceInstanceConditionReady && condition.Status != status { - fatalf(t, "ready condition had unexpected status; expected %v, got %v", status, condition.Status) + if condition.Type == conditionType { + foundCondition = true + if condition.Status != status { + fatalf(t, "%v condition had unexpected status; expected %v, got %v", conditionType, status, condition.Status) + } + if len(reason) == 1 && condition.Reason != reason[0] { + fatalf(t, "unexpected reason; expected %v, got %v", reason[0], condition.Reason) + } } - if len(reason) == 1 && condition.Reason != reason[0] { - fatalf(t, "unexpected reason; expected %v, got %v", reason[0], condition.Reason) + } + + if !foundCondition { + fatalf(t, "%v condition not found", conditionType) + } +} + +func assertServiceInstanceConditionsCount(t *testing.T, obj runtime.Object, count int) { + instance, ok := obj.(*v1alpha1.ServiceInstance) + if !ok { + fatalf(t, "Couldn't convert object %+v into a *v1alpha1.ServiceInstance", obj) + } + + if e, a := count, len(instance.Status.Conditions); e != a { + t.Fatalf("Expected %v condition, got %v", e, a) + } +} + +func assertServiceInstanceReconciledGeneration(t *testing.T, obj runtime.Object, reconciledGeneration int64) { + instance, ok := obj.(*v1alpha1.ServiceInstance) + if !ok { + fatalf(t, "Couldn't convert object %+v into a *v1alpha1.ServiceInstance", obj) + } + + if e, a := reconciledGeneration, instance.Status.ReconciledGeneration; e != a { + fatalf(t, "unexpected reconciled generation: expected %v, got %v", e, a) + } +} + +func assertServiceInstanceOperationStartTimeSet(t *testing.T, obj runtime.Object, isOperationStartTimeSet bool) { + instance, ok := obj.(*v1alpha1.ServiceInstance) + if !ok { + fatalf(t, "Couldn't convert object %+v into a *v1alpha1.ServiceInstance", obj) + } + + if e, a := isOperationStartTimeSet, instance.Status.OperationStartTime != nil; e != a { + if e { + fatalf(t, "expected OperationStartTime to not be nil, but was") + } else { + fatalf(t, "expected OperationStartTime to be nil, but was not") } } } @@ -1446,18 +1516,56 @@ func assertServiceInstanceCredentialReadyFalse(t *testing.T, obj runtime.Object, } func assertServiceInstanceCredentialReadyCondition(t *testing.T, obj runtime.Object, status v1alpha1.ConditionStatus, reason ...string) { + assertServiceInstanceCredentialCondition(t, obj, v1alpha1.ServiceInstanceCredentialConditionReady, status, reason...) +} + +func assertServiceInstanceCredentialCondition(t *testing.T, obj runtime.Object, conditionType v1alpha1.ServiceInstanceCredentialConditionType, status v1alpha1.ConditionStatus, reason ...string) { binding, ok := obj.(*v1alpha1.ServiceInstanceCredential) if !ok { fatalf(t, "Couldn't convert object %+v into a *v1alpha1.ServiceInstanceCredential", obj) } + conditionFound := false for _, condition := range binding.Status.Conditions { - if condition.Type == v1alpha1.ServiceInstanceCredentialConditionReady && condition.Status != status { - t.Logf("ready condition: %+v", condition) - fatalf(t, "ready condition had unexpected status; expected %v, got %v", status, condition.Status) + if condition.Type == conditionType { + conditionFound = true + if condition.Status != status { + t.Logf("%v condition: %+v", conditionType, condition) + fatalf(t, "%v condition had unexpected status; expected %v, got %v", conditionType, status, condition.Status) + } + if len(reason) == 1 && condition.Reason != reason[0] { + fatalf(t, "unexpected reason; expected %v, got %v", reason[0], condition.Reason) + } } - if len(reason) == 1 && condition.Reason != reason[0] { - fatalf(t, "unexpected reason; expected %v, got %v", reason[0], condition.Reason) + } + + if !conditionFound { + fatalf(t, "unfound %v condition", conditionType) + } +} + +func assertServiceInstanceCredentialReconciledGeneration(t *testing.T, obj runtime.Object, reconciledGeneration int64) { + binding, ok := obj.(*v1alpha1.ServiceInstanceCredential) + if !ok { + fatalf(t, "Couldn't convert object %+v into a *v1alpha1.ServiceInstanceCredential", obj) + } + + if e, a := reconciledGeneration, binding.Status.ReconciledGeneration; e != a { + fatalf(t, "unexpected reconciled generation: expected %v, got %v", e, a) + } +} + +func assertServiceInstanceCredentialOperationStartTimeSet(t *testing.T, obj runtime.Object, isOperationStartTimeSet bool) { + binding, ok := obj.(*v1alpha1.ServiceInstanceCredential) + if !ok { + fatalf(t, "Couldn't convert object %+v into a *v1alpha1.ServiceInstanceCredential", obj) + } + + if e, a := isOperationStartTimeSet, binding.Status.OperationStartTime != nil; e != a { + if e { + fatalf(t, "expected OperationStartTime to not be nil, but was") + } else { + fatalf(t, "expected OperationStartTime to be nil, but was not") } } } diff --git a/pkg/openapi/openapi_generated.go b/pkg/openapi/openapi_generated.go index 6924f79a058..3165a71abbd 100644 --- a/pkg/openapi/openapi_generated.go +++ b/pkg/openapi/openapi_generated.go @@ -327,12 +327,18 @@ func GetOpenAPIDefinitions(ref openapi.ReferenceCallback) map[string]openapi.Ope Format: "int64", }, }, + "operationStartTime": { + SchemaProps: spec.SchemaProps{ + Description: "OperationStartTime is the time at which the current operation began.", + Ref: ref("k8s.io/apimachinery/pkg/apis/meta/v1.Time"), + }, + }, }, Required: []string{"conditions", "reconciledGeneration"}, }, }, Dependencies: []string{ - "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1alpha1.ServiceBrokerCondition"}, + "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1alpha1.ServiceBrokerCondition", "k8s.io/apimachinery/pkg/apis/meta/v1.Time"}, }, "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1alpha1.ServiceClass": { Schema: spec.Schema{ @@ -788,12 +794,18 @@ func GetOpenAPIDefinitions(ref openapi.ReferenceCallback) map[string]openapi.Ope Format: "int64", }, }, + "operationStartTime": { + SchemaProps: spec.SchemaProps{ + Description: "OperationStartTime is the time at which the current operation began.", + Ref: ref("k8s.io/apimachinery/pkg/apis/meta/v1.Time"), + }, + }, }, Required: []string{"conditions", "reconciledGeneration"}, }, }, Dependencies: []string{ - "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1alpha1.ServiceInstanceCredentialCondition"}, + "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1alpha1.ServiceInstanceCredentialCondition", "k8s.io/apimachinery/pkg/apis/meta/v1.Time"}, }, "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1alpha1.ServiceInstanceList": { Schema: spec.Schema{ @@ -942,12 +954,18 @@ func GetOpenAPIDefinitions(ref openapi.ReferenceCallback) map[string]openapi.Ope Format: "int64", }, }, + "operationStartTime": { + SchemaProps: spec.SchemaProps{ + Description: "OperationStartTime is the time at which the current operation began.", + Ref: ref("k8s.io/apimachinery/pkg/apis/meta/v1.Time"), + }, + }, }, Required: []string{"conditions", "asyncOpInProgress", "reconciledGeneration"}, }, }, Dependencies: []string{ - "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1alpha1.ServiceInstanceCondition"}, + "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1alpha1.ServiceInstanceCondition", "k8s.io/apimachinery/pkg/apis/meta/v1.Time"}, }, "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1alpha1.ServicePlan": { Schema: spec.Schema{ diff --git a/test/integration/controller_test.go b/test/integration/controller_test.go index 229ee9acf96..276bc95855e 100644 --- a/test/integration/controller_test.go +++ b/test/integration/controller_test.go @@ -1047,7 +1047,7 @@ func newTestController(t *testing.T, config fakeosb.FakeClientConfiguration) ( informerFactory := scinformers.NewSharedInformerFactory(catalogClient, 10*time.Second) serviceCatalogSharedInformers := informerFactory.Servicecatalog().V1alpha1() - fakeRecorder := record.NewFakeRecorder(5) + fakeRecorder := record.NewFakeRecorder(10) // create a test controller testController, err := controller.NewController( @@ -1061,6 +1061,7 @@ func newTestController(t *testing.T, config fakeosb.FakeClientConfiguration) ( 24*time.Hour, osb.LatestAPIVersion().HeaderValue(), fakeRecorder, + 7*24*time.Hour, ) t.Log("controller start") if err != nil {