Skip to content
This repository has been archived by the owner on Jan 13, 2021. It is now read-only.

Commit

Permalink
Remove blocked on error status
Browse files Browse the repository at this point in the history
  • Loading branch information
@ash2k
ash2k committed Feb 10, 2018
1 parent df5d135 commit 017b000
Show file tree
Hide file tree
Showing 5 changed files with 164 additions and 84 deletions.
1 change: 0 additions & 1 deletion pkg/apis/smith/v1/types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ const (
// Blocked condition reasons

ResourceReasonDependenciesNotReady = "DependenciesNotReady"
ResourceReasonOtherResourceError = "OtherResourceError"

// Error condition reasons

Expand Down
7 changes: 0 additions & 7 deletions pkg/controller/bundle_sync_task.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,6 @@ func (st *bundleSyncTask) process() (retriableError bool, e error) {

st.processedResources = make(map[smith_v1.ResourceName]*resourceInfo, len(st.bundle.Spec.Resources))

blockedOnError := false
// Visit vertices in sorted order
for _, resName := range sorted {
// Process the resource
Expand All @@ -75,15 +74,13 @@ func (st *bundleSyncTask) process() (retriableError bool, e error) {
processedResources: st.processedResources,
pluginContainers: st.pluginContainers,
scheme: st.scheme,
blockedOnError: blockedOnError,
}
resInfo := rst.processResource(&res)
if retriable, err := resInfo.fetchError(); err != nil && api_errors.IsConflict(errors.Cause(err)) {
// Short circuit on conflict
return retriable, err
}
_, resErr := resInfo.fetchError()
blockedOnError = blockedOnError || resErr != nil
if resErr != nil {
logger.Error("Done processing resource", zap.Bool("ready", resInfo.isReady()), zap.Error(resErr))
} else {
Expand Down Expand Up @@ -217,10 +214,6 @@ func (st *bundleSyncTask) handleProcessResult(retriable bool, processErr error)
blockedCond.Status = smith_v1.ConditionTrue
blockedCond.Reason = smith_v1.ResourceReasonDependenciesNotReady
blockedCond.Message = fmt.Sprintf("Not ready: %q", resStatus.dependencies)
case resourceStatusBlockedByError:
blockedCond.Status = smith_v1.ConditionTrue
blockedCond.Reason = smith_v1.ResourceReasonOtherResourceError
blockedCond.Message = "Some other resource is in error state"
case resourceStatusInProgress:
inProgressCond.Status = smith_v1.ConditionTrue
case resourceStatusReady:
Expand Down
203 changes: 153 additions & 50 deletions pkg/controller/controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ func TestController(t *testing.T) {
key, err := cache.MetaNamespaceKeyFunc(tc.bundle)
require.NoError(t, err)
retriable, err := cntrlr.processKey(tc.logger, key)
require.EqualError(t, err, `bundle contains two resources with the same name "`+resP1+`"`)
assert.EqualError(t, err, `bundle contains two resources with the same name "`+resP1+`"`)
assert.False(t, retriable)

actions := tc.bundleFake.Actions()
Expand Down Expand Up @@ -217,7 +217,7 @@ func TestController(t *testing.T) {
key, err := cache.MetaNamespaceKeyFunc(tc.bundle)
require.NoError(t, err)
retriable, err := cntrlr.processKey(tc.logger, key)
require.EqualError(t, err, `topological sort of resources failed: vertex "bla" not found`)
assert.EqualError(t, err, `topological sort of resources failed: vertex "bla" not found`)
assert.False(t, retriable)

actions := tc.bundleFake.Actions()
Expand Down Expand Up @@ -330,28 +330,7 @@ func TestController(t *testing.T) {
{
method: "PUT",
path: "/api/v1/namespaces/" + testNamespace + "/configmaps/" + mapNeedsAnUpdate,
}: {
statusCode: http.StatusOK,
content: []byte(`{
"apiVersion": "v1",
"kind": "ConfigMap",
"metadata": {
"name": "` + mapNeedsAnUpdate + `",
"namespace": "` + testNamespace + `",
"uid": "` + mapNeedsAnUpdateUid + `",
"labels": {
"` + smith.BundleNameLabel + `": "` + bundle1 + `"
},
"ownerReferences": [{
"apiVersion": "` + smith_v1.BundleResourceGroupVersion + `",
"kind": "` + smith_v1.BundleResourceKind + `",
"name": "` + bundle1 + `",
"uid": "` + bundle1uid + `",
"controller": true,
"blockOwnerDeletion": true
}] }
}`),
},
}: configMapNeedsUpdateResponse(bundle1, bundle1uid),
},
},
plugins: map[smith_v1.PluginName]func(*testing.T) testingPlugin{
Expand Down Expand Up @@ -605,11 +584,11 @@ func TestController(t *testing.T) {
require.NoError(t, err)
retriable, err := cntrlr.processKey(tc.logger, key)
// Sadly, the actual error is not current propagated
require.EqualError(t, err, `error processing resource(s): ["`+resP1+`"]`)
assert.EqualError(t, err, `error processing resource(s): ["`+resP1+`"]`)
assert.False(t, retriable)
},
},
"no resource creates/updates/deletes done after error is encountered": &testCase{
"no blocked resources creates/updates/deletes done after error is encountered": &testCase{
mainClientObjects: []runtime.Object{
configMapNeedsDelete(),
configMapNeedsUpdate(),
Expand Down Expand Up @@ -640,7 +619,29 @@ func TestController(t *testing.T) {
},
},
{
Name: resMapNeedsAnUpdate,
Name: resSb1,
DependsOn: []smith_v1.ResourceName{resSi1},
Spec: smith_v1.ResourceSpec{
Object: &sc_v1b1.ServiceBinding{
TypeMeta: meta_v1.TypeMeta{
Kind: "ServiceBinding",
APIVersion: sc_v1b1.SchemeGroupVersion.String(),
},
ObjectMeta: meta_v1.ObjectMeta{
Name: sb1,
},
Spec: sc_v1b1.ServiceBindingSpec{
ServiceInstanceRef: sc_v1b1.LocalObjectReference{
Name: si1,
},
SecretName: s1,
},
},
},
},
{
Name: resMapNeedsAnUpdate,
DependsOn: []smith_v1.ResourceName{resSb1},
Spec: smith_v1.ResourceSpec{
Object: &core_v1.ConfigMap{
TypeMeta: meta_v1.TypeMeta{
Expand All @@ -663,7 +664,7 @@ func TestController(t *testing.T) {
key, err := cache.MetaNamespaceKeyFunc(tc.bundle)
require.NoError(t, err)
retriable, err := cntrlr.processKey(tc.logger, key)
require.EqualError(t, err, `error processing resource(s): ["`+resSi1+`"]`)
assert.EqualError(t, err, `error processing resource(s): ["`+resSi1+`"]`)
assert.False(t, retriable)
actions := tc.bundleFake.Actions()
require.Len(t, actions, 3)
Expand All @@ -681,14 +682,108 @@ func TestController(t *testing.T) {
assert.Equal(t, smith_v1.ResourceReasonTerminalError, resCond.Reason)
assert.Equal(t, "readiness check failed: BlaBla: Oh no!", resCond.Message)

resCond = smith_testing.AssertResourceCondition(t, updateBundle, resSb1, smith_v1.ResourceBlocked, smith_v1.ConditionTrue)
assert.Equal(t, smith_v1.ResourceReasonDependenciesNotReady, resCond.Reason)
assert.Equal(t, `Not ready: ["`+resSi1+`"]`, resCond.Message)
smith_testing.AssertResourceCondition(t, updateBundle, resSb1, smith_v1.ResourceInProgress, smith_v1.ConditionFalse)
smith_testing.AssertResourceCondition(t, updateBundle, resSb1, smith_v1.ResourceReady, smith_v1.ConditionFalse)
smith_testing.AssertResourceCondition(t, updateBundle, resSb1, smith_v1.ResourceError, smith_v1.ConditionFalse)

resCond = smith_testing.AssertResourceCondition(t, updateBundle, resMapNeedsAnUpdate, smith_v1.ResourceBlocked, smith_v1.ConditionTrue)
assert.Equal(t, smith_v1.ResourceReasonOtherResourceError, resCond.Reason)
assert.Equal(t, "Some other resource is in error state", resCond.Message)
assert.Equal(t, smith_v1.ResourceReasonDependenciesNotReady, resCond.Reason)
assert.Equal(t, `Not ready: ["`+resSb1+`"]`, resCond.Message)
smith_testing.AssertResourceCondition(t, updateBundle, resMapNeedsAnUpdate, smith_v1.ResourceInProgress, smith_v1.ConditionFalse)
smith_testing.AssertResourceCondition(t, updateBundle, resMapNeedsAnUpdate, smith_v1.ResourceReady, smith_v1.ConditionFalse)
smith_testing.AssertResourceCondition(t, updateBundle, resMapNeedsAnUpdate, smith_v1.ResourceError, smith_v1.ConditionFalse)
},
},
"resources are processed after non-blocking error is encountered": &testCase{
mainClientObjects: []runtime.Object{
configMapNeedsDelete(),
configMapNeedsUpdate(),
},
scClientObjects: []runtime.Object{
serviceInstance(false, false, true),
},
bundle: &smith_v1.Bundle{
ObjectMeta: meta_v1.ObjectMeta{
Name: bundle1,
Namespace: testNamespace,
UID: bundle1uid,
},
Spec: smith_v1.BundleSpec{
Resources: []smith_v1.Resource{
{
Name: resSi1,
Spec: smith_v1.ResourceSpec{
Object: &sc_v1b1.ServiceInstance{
TypeMeta: meta_v1.TypeMeta{
Kind: "ServiceInstance",
APIVersion: sc_v1b1.SchemeGroupVersion.String(),
},
ObjectMeta: meta_v1.ObjectMeta{
Name: si1,
},
},
},
},
{
Name: resMapNeedsAnUpdate,
Spec: smith_v1.ResourceSpec{
Object: &core_v1.ConfigMap{
TypeMeta: meta_v1.TypeMeta{
Kind: "ConfigMap",
APIVersion: core_v1.SchemeGroupVersion.String(),
},
ObjectMeta: meta_v1.ObjectMeta{
Name: mapNeedsAnUpdate,
},
},
},
},
},
},
},
namespace: testNamespace,
expectedActions: sets.NewString("PUT=/api/v1/namespaces/" + testNamespace + "/configmaps/" + mapNeedsAnUpdate),
enableServiceCatalog: true,
testHandler: fakeActionHandler{
response: map[path]fakeResponse{
{
method: "PUT",
path: "/api/v1/namespaces/" + testNamespace + "/configmaps/" + mapNeedsAnUpdate,
}: configMapNeedsUpdateResponse(bundle1, bundle1uid),
},
},
test: func(t *testing.T, ctx context.Context, cntrlr *BundleController, tc *testCase, prepare func(ctx context.Context)) {
prepare(ctx)
key, err := cache.MetaNamespaceKeyFunc(tc.bundle)
require.NoError(t, err)
retriable, err := cntrlr.processKey(tc.logger, key)
assert.EqualError(t, err, `error processing resource(s): ["`+resSi1+`"]`)
assert.False(t, retriable)
actions := tc.bundleFake.Actions()
require.Len(t, actions, 3)
bundleUpdate := actions[2].(kube_testing.UpdateAction)
assert.Equal(t, testNamespace, bundleUpdate.GetNamespace())
updateBundle := bundleUpdate.GetObject().(*smith_v1.Bundle)
smith_testing.AssertCondition(t, updateBundle, smith_v1.BundleReady, smith_v1.ConditionFalse)
smith_testing.AssertCondition(t, updateBundle, smith_v1.BundleInProgress, smith_v1.ConditionFalse)
smith_testing.AssertCondition(t, updateBundle, smith_v1.BundleError, smith_v1.ConditionTrue)

smith_testing.AssertResourceCondition(t, updateBundle, resSi1, smith_v1.ResourceBlocked, smith_v1.ConditionFalse)
smith_testing.AssertResourceCondition(t, updateBundle, resSi1, smith_v1.ResourceInProgress, smith_v1.ConditionFalse)
smith_testing.AssertResourceCondition(t, updateBundle, resSi1, smith_v1.ResourceReady, smith_v1.ConditionFalse)
resCond := smith_testing.AssertResourceCondition(t, updateBundle, resSi1, smith_v1.ResourceError, smith_v1.ConditionTrue)
assert.Equal(t, smith_v1.ResourceReasonTerminalError, resCond.Reason)
assert.Equal(t, "readiness check failed: BlaBla: Oh no!", resCond.Message)

smith_testing.AssertResourceCondition(t, updateBundle, resMapNeedsAnUpdate, smith_v1.ResourceBlocked, smith_v1.ConditionFalse)
smith_testing.AssertResourceCondition(t, updateBundle, resMapNeedsAnUpdate, smith_v1.ResourceInProgress, smith_v1.ConditionFalse)
smith_testing.AssertResourceCondition(t, updateBundle, resMapNeedsAnUpdate, smith_v1.ResourceReady, smith_v1.ConditionTrue)
smith_testing.AssertResourceCondition(t, updateBundle, resMapNeedsAnUpdate, smith_v1.ResourceError, smith_v1.ConditionFalse)
},
},
"resources are not deleted if bundle is in progress": &testCase{
mainClientObjects: []runtime.Object{
configMapNeedsDelete(),
Expand Down Expand Up @@ -760,33 +855,15 @@ func TestController(t *testing.T) {
{
method: "PUT",
path: "/api/v1/namespaces/" + testNamespace + "/configmaps/" + mapNeedsAnUpdate,
}: {
statusCode: http.StatusOK,
content: []byte(`{
"apiVersion": "v1",
"kind": "ConfigMap",
"metadata": {
"name": "` + mapNeedsAnUpdate + `",
"namespace": "` + testNamespace + `",
"uid": "` + mapNeedsAnUpdateUid + `",
"ownerReferences": [{
"apiVersion": "` + smith_v1.BundleResourceGroupVersion + `",
"kind": "` + smith_v1.BundleResourceKind + `",
"name": "` + bundle1 + `",
"uid": "` + bundle1uid + `",
"controller": true,
"blockOwnerDeletion": true
}] }
}`),
},
}: configMapNeedsUpdateResponse("invalidBundle", "invalidUid"),
},
},
test: func(t *testing.T, ctx context.Context, cntrlr *BundleController, tc *testCase, prepare func(ctx context.Context)) {
prepare(ctx)
key, err := cache.MetaNamespaceKeyFunc(tc.bundle)
require.NoError(t, err)
retriable, err := cntrlr.processKey(tc.logger, key)
require.EqualError(t, err, `error processing resource(s): ["map-needs-update"]`)
assert.EqualError(t, err, `error processing resource(s): ["`+mapNeedsAnUpdate+`"]`)
assert.False(t, retriable)
actions := tc.bundleFake.Actions()
require.Len(t, actions, 3)
Expand Down Expand Up @@ -1655,6 +1732,32 @@ func configMapNeedsUpdate() *core_v1.ConfigMap {
},
}
}

func configMapNeedsUpdateResponse(bundleName, bundleUid string) fakeResponse {
return fakeResponse{
statusCode: http.StatusOK,
content: []byte(`{
"apiVersion": "v1",
"kind": "ConfigMap",
"metadata": {
"name": "` + mapNeedsAnUpdate + `",
"namespace": "` + testNamespace + `",
"uid": "` + mapNeedsAnUpdateUid + `",
"labels": {
"` + smith.BundleNameLabel + `": "` + bundleName + `"
},
"ownerReferences": [{
"apiVersion": "` + smith_v1.BundleResourceGroupVersion + `",
"kind": "` + smith_v1.BundleResourceKind + `",
"name": "` + bundleName + `",
"uid": "` + bundleUid + `",
"controller": true,
"blockOwnerDeletion": true
}] }
}`),
}
}

func configMapNeedsDelete() *core_v1.ConfigMap {
tr := true
return &core_v1.ConfigMap{
Expand Down
Loading

0 comments on commit 017b000

Please sign in to comment.