Skip to content
This repository has been archived by the owner on May 6, 2022. It is now read-only.

Commit

Permalink
Use controller's default brokerRelistInterval for namespaced brokers (#…
Browse files Browse the repository at this point in the history 
…2313)

* Use controller's default brokerRelistInterval for namespaced brokers

* Refactor shouldReconcile(Cluster)ServiceBroker into a single function

The two functions only relied on common components.

* Remove duplicated code in getTestClusterServiceBrokerWithStatus

* Run subtests with testing.Run()

* Add test where relistDuration is nil and interval has elapsed
  • Loading branch information
@luksa @k8s-ci-robot
luksa authored and k8s-ci-robot committed Sep 5, 2018
1 parent 4728149 commit 6dfe967
Show file tree
Hide file tree
Showing 6 changed files with 123 additions and 142 deletions.
59 changes: 59 additions & 0 deletions pkg/controller/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1426,3 +1426,62 @@ func (c *controller) getServiceBrokerForServiceBinding(instance *v1beta1.Service
}
return broker, nil
}

// shouldReconcileServiceBroker determines whether a broker should be reconciled; it
// returns true unless the broker has a ready condition with status true and
// the controller's broker relist interval has not elapsed since the broker's
// ready condition became true, or if the broker's RelistBehavior is set to Manual.
func shouldReconcileServiceBrokerCommon(pcb *pretty.ContextBuilder, brokerMeta *metav1.ObjectMeta, brokerSpec *v1beta1.CommonServiceBrokerSpec, brokerStatus *v1beta1.CommonServiceBrokerStatus, now time.Time, defaultRelistInterval time.Duration) bool {
if brokerStatus.ReconciledGeneration != brokerMeta.Generation {
// If the spec has changed, we should reconcile the broker.
return true
}
if brokerMeta.DeletionTimestamp != nil || len(brokerStatus.Conditions) == 0 {
// If the deletion timestamp is set or the broker has no status
// conditions, we should reconcile it.
return true
}

// find the ready condition in the broker's status
for _, condition := range brokerStatus.Conditions {
if condition.Type == v1beta1.ServiceBrokerConditionReady {
// The broker has a ready condition

if condition.Status == v1beta1.ConditionTrue {

// The broker's ready condition has status true, meaning that
// at some point, we successfully listed the broker's catalog.
if brokerSpec.RelistBehavior == v1beta1.ServiceBrokerRelistBehaviorManual {
// If a broker is configured with RelistBehaviorManual, it should
// ignore the Duration and only relist based on spec changes

glog.V(10).Info(pcb.Message("Not processing because RelistBehavior is set to Manual"))
return false
}

// By default, the broker should relist if it has been longer than the
// RelistDuration since the last time we fetched the Catalog
duration := defaultRelistInterval
if brokerSpec.RelistDuration != nil {
duration = brokerSpec.RelistDuration.Duration
}

intervalPassed := true
if brokerStatus.LastCatalogRetrievalTime != nil {
intervalPassed = now.After(brokerStatus.LastCatalogRetrievalTime.Time.Add(duration))
}
if intervalPassed == false {
glog.V(10).Info(pcb.Message("Not processing because RelistDuration has not elapsed since the last relist"))
}
return intervalPassed
}

// The broker's ready condition wasn't true; we should try to re-
// list the broker.
return true
}
}

// The broker didn't have a ready condition; we should reconcile it.
return true
}
62 changes: 8 additions & 54 deletions pkg/controller/controller_clusterservicebroker.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,60 +94,14 @@ func (c *controller) clusterServiceBrokerDelete(obj interface{}) {
// the controller's broker relist interval has not elapsed since the broker's
// ready condition became true, or if the broker's RelistBehavior is set to Manual.
func shouldReconcileClusterServiceBroker(broker *v1beta1.ClusterServiceBroker, now time.Time, defaultRelistInterval time.Duration) bool {
pcb := pretty.NewClusterServiceBrokerContextBuilder(broker)
if broker.Status.ReconciledGeneration != broker.Generation {
// If the spec has changed, we should reconcile the broker.
return true
}
if broker.DeletionTimestamp != nil || len(broker.Status.Conditions) == 0 {
// If the deletion timestamp is set or the broker has no status
// conditions, we should reconcile it.
return true
}

// find the ready condition in the broker's status
for _, condition := range broker.Status.Conditions {
if condition.Type == v1beta1.ServiceBrokerConditionReady {
// The broker has a ready condition

if condition.Status == v1beta1.ConditionTrue {

// The broker's ready condition has status true, meaning that
// at some point, we successfully listed the broker's catalog.
if broker.Spec.RelistBehavior == v1beta1.ServiceBrokerRelistBehaviorManual {
// If a broker is configured with RelistBehaviorManual, it should
// ignore the Duration and only relist based on spec changes

glog.V(10).Info(pcb.Message("Not processing because RelistBehavior is set to Manual"))
return false
}

// By default, the broker should relist if it has been longer than the
// RelistDuration since the last time we fetched the Catalog
duration := defaultRelistInterval

if broker.Spec.RelistDuration != nil {
duration = broker.Spec.RelistDuration.Duration
}

intervalPassed := true
if broker.Status.LastCatalogRetrievalTime != nil {
intervalPassed = now.After(broker.Status.LastCatalogRetrievalTime.Time.Add(duration))
}
if intervalPassed == false {
glog.V(10).Info(pcb.Message("Not processing because RelistDuration has not elapsed since the last relist"))
}
return intervalPassed
}

// The broker's ready condition wasn't true; we should try to re-
// list the broker.
return true
}
}

// The broker didn't have a ready condition; we should reconcile it.
return true
return shouldReconcileServiceBrokerCommon(
pretty.NewClusterServiceBrokerContextBuilder(broker),
&broker.ObjectMeta,
&broker.Spec.CommonServiceBrokerSpec,
&broker.Status.CommonServiceBrokerStatus,
now,
defaultRelistInterval,
)
}

func (c *controller) reconcileClusterServiceBrokerKey(key string) error {
Expand Down
49 changes: 32 additions & 17 deletions pkg/controller/controller_clusterservicebroker_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -173,16 +173,29 @@ func TestShouldReconcileClusterServiceBroker(t *testing.T) {
reconcile: true,
},
{
name: "ready, duration behavior, nil duration",
name: "ready, duration behavior, nil duration, interval not elapsed",
broker: func() *v1beta1.ClusterServiceBroker {
broker := getTestClusterServiceBrokerWithStatus(v1beta1.ConditionTrue)
t := metav1.NewTime(time.Now().Add(-23 * time.Hour))
broker := getTestClusterServiceBrokerWithStatusAndTime(v1beta1.ConditionTrue, t, t)
broker.Spec.RelistBehavior = v1beta1.ServiceBrokerRelistBehaviorDuration
broker.Spec.RelistDuration = nil
return broker
}(),
now: time.Now(),
reconcile: false,
},
{
name: "ready, duration behavior, nil duration, interval elapsed",
broker: func() *v1beta1.ClusterServiceBroker {
t := metav1.NewTime(time.Now().Add(-25 * time.Hour))
broker := getTestClusterServiceBrokerWithStatusAndTime(v1beta1.ConditionTrue, t, t)
broker.Spec.RelistBehavior = v1beta1.ServiceBrokerRelistBehaviorDuration
broker.Spec.RelistDuration = nil
return broker
}(),
now: time.Now(),
reconcile: true,
},
{
name: "ready, manual behavior",
broker: func() *v1beta1.ClusterServiceBroker {
Expand All @@ -196,24 +209,26 @@ func TestShouldReconcileClusterServiceBroker(t *testing.T) {
}

for _, tc := range cases {
var ltt *time.Time
if len(tc.broker.Status.Conditions) != 0 {
ltt = &tc.broker.Status.Conditions[0].LastTransitionTime.Time
}
t.Run(tc.name, func(t *testing.T) {
var ltt *time.Time
if len(tc.broker.Status.Conditions) != 0 {
ltt = &tc.broker.Status.Conditions[0].LastTransitionTime.Time
}

if tc.broker.Spec.RelistDuration != nil {
interval := tc.broker.Spec.RelistDuration.Duration
lastRelistTime := tc.broker.Status.LastCatalogRetrievalTime
t.Logf("%v: now: %v, interval: %v, last transition time: %v, last relist time: %v", tc.name, tc.now, interval, ltt, lastRelistTime)
} else {
t.Logf("broker.Spec.RelistDuration set to nil")
}
if tc.broker.Spec.RelistDuration != nil {
interval := tc.broker.Spec.RelistDuration.Duration
lastRelistTime := tc.broker.Status.LastCatalogRetrievalTime
t.Logf("now: %v, interval: %v, last transition time: %v, last relist time: %v", tc.now, interval, ltt, lastRelistTime)
} else {
t.Logf("broker.Spec.RelistDuration set to nil")
}

actual := shouldReconcileClusterServiceBroker(tc.broker, tc.now, 24*time.Hour)
actual := shouldReconcileClusterServiceBroker(tc.broker, tc.now, 24*time.Hour)

if e, a := tc.reconcile, actual; e != a {
t.Errorf("%v: unexpected result: %s", tc.name, expectedGot(e, a))
}
if e, a := tc.reconcile, actual; e != a {
t.Errorf("unexpected result: %s", expectedGot(e, a))
}
})
}
}

Expand Down
68 changes: 10 additions & 58 deletions pkg/controller/controller_servicebroker.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,63 +82,15 @@ func (c *controller) serviceBrokerDelete(obj interface{}) {
// returns true unless the broker has a ready condition with status true and
// the controller's broker relist interval has not elapsed since the broker's
// ready condition became true, or if the broker's RelistBehavior is set to Manual.
func shouldReconcileServiceBroker(broker *v1beta1.ServiceBroker, now time.Time) bool {
// ERIK TODO: This should get refactored out into a shared method because it
// only relies on Common components.
pcb := pretty.NewServiceBrokerContextBuilder(broker)
if broker.Status.ReconciledGeneration != broker.Generation {
// If the spec has changed, we should reconcile the broker.
return true
}
if broker.DeletionTimestamp != nil || len(broker.Status.Conditions) == 0 {
// If the deletion timestamp is set or the broker has no status
// conditions, we should reconcile it.
return true
}

// find the ready condition in the broker's status
for _, condition := range broker.Status.Conditions {
if condition.Type == v1beta1.ServiceBrokerConditionReady {
// The broker has a ready condition

if condition.Status == v1beta1.ConditionTrue {

// The broker's ready condition has status true, meaning that
// at some point, we successfully listed the broker's catalog.
if broker.Spec.RelistBehavior == v1beta1.ServiceBrokerRelistBehaviorManual {
// If a broker is configured with RelistBehaviorManual, it should
// ignore the Duration and only relist based on spec changes

glog.V(10).Info(pcb.Message("Not processing because RelistBehavior is set to Manual"))
return false
}

if broker.Spec.RelistDuration == nil {
glog.Error(pcb.Message("Unable to process because RelistBehavior is set to Duration with a nil RelistDuration value"))
return false
}

// By default, the broker should relist if it has been longer than the
// RelistDuration since the last time we fetched the Catalog
duration := broker.Spec.RelistDuration.Duration
intervalPassed := true
if broker.Status.LastCatalogRetrievalTime != nil {
intervalPassed = now.After(broker.Status.LastCatalogRetrievalTime.Time.Add(duration))
}
if intervalPassed == false {
glog.V(10).Info(pcb.Message("Not processing because RelistDuration has not elapsed since the last relist"))
}
return intervalPassed
}

// The broker's ready condition wasn't true; we should try to re-
// list the broker.
return true
}
}

// The broker didn't have a ready condition; we should reconcile it.
return true
func shouldReconcileServiceBroker(broker *v1beta1.ServiceBroker, now time.Time, defaultRelistInterval time.Duration) bool {
return shouldReconcileServiceBrokerCommon(
pretty.NewServiceBrokerContextBuilder(broker),
&broker.ObjectMeta,
&broker.Spec.CommonServiceBrokerSpec,
&broker.Status.CommonServiceBrokerStatus,
now,
defaultRelistInterval,
)
}

func (c *controller) reconcileServiceBrokerKey(key string) error {
Expand Down Expand Up @@ -171,7 +123,7 @@ func (c *controller) reconcileServiceBroker(broker *v1beta1.ServiceBroker) error
// set to Manual, do not reconcile it.
// * If the broker's ready condition is true and the relist interval has not
// elapsed, do not reconcile it.
if !shouldReconcileServiceBroker(broker, time.Now()) {
if !shouldReconcileServiceBroker(broker, time.Now(), c.brokerRelistInterval) {
return nil
}

Expand Down
12 changes: 12 additions & 0 deletions pkg/controller/controller_servicebroker_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,27 @@ import (
"errors"
"fmt"
"testing"
"time"

"github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1beta1"
scfeatures "github.com/kubernetes-incubator/service-catalog/pkg/features"
"github.com/kubernetes-incubator/service-catalog/test/fake"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
utilfeature "k8s.io/apiserver/pkg/util/feature"
clientgotesting "k8s.io/client-go/testing"
)

// NOTE: This only tests a single test case. Others are tested in TestShouldReconcileClusterServiceBroker.
func TestShouldReconcileServiceBroker(t *testing.T) {
broker := getTestClusterServiceBroker()
broker.Spec.RelistDuration = &metav1.Duration{Duration: 3 * time.Minute}

if !shouldReconcileClusterServiceBroker(broker, time.Now(), 24*time.Hour) {
t.Error("expected true, bot got false")
}
}

func TestReconcileServiceClassFromServiceBrokerCatalog(t *testing.T) {
updatedClass := func() *v1beta1.ServiceClass {
p := getTestServiceClass()
Expand Down
15 changes: 2 additions & 13 deletions pkg/controller/controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -490,19 +490,8 @@ func getTestClusterServiceBroker() *v1beta1.ClusterServiceBroker {
}

func getTestClusterServiceBrokerWithStatus(status v1beta1.ConditionStatus) *v1beta1.ClusterServiceBroker {
lastTransitionTime := metav1.NewTime(time.Now().Add(-5 * time.Minute))
broker := getTestClusterServiceBroker()
broker.Status = v1beta1.ClusterServiceBrokerStatus{
CommonServiceBrokerStatus: v1beta1.CommonServiceBrokerStatus{
Conditions: []v1beta1.ServiceBrokerCondition{{
Type: v1beta1.ServiceBrokerConditionReady,
Status: status,
LastTransitionTime: lastTransitionTime,
}},
LastCatalogRetrievalTime: &lastTransitionTime,
},
}
return broker
t := metav1.NewTime(time.Now().Add(-5 * time.Minute))
return getTestClusterServiceBrokerWithStatusAndTime(status, t, t)
}

func getTestClusterServiceBrokerWithStatusAndTime(status v1beta1.ConditionStatus, lastTransitionTime, lastRelistTime metav1.Time) *v1beta1.ClusterServiceBroker {
Expand Down

0 comments on commit 6dfe967

Please sign in to comment.