Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[opsrc] Do not delete csc during purge #117

Merged
merged 1 commit into from
Feb 26, 2019
Merged

[opsrc] Do not delete csc during purge #117

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions pkg/operatorsource/configuring.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ func (r *configuringReconciler) Reconcile(ctx context.Context, in *v1alpha1.Oper

if err == nil {
nextPhase = phase.GetNext(phase.Succeeded)
r.logger.Info("The object has been successfully reconciled")
r.logger.Info("CatalogSourceConfig object has been created successfully")

return
}
Expand Down Expand Up @@ -115,8 +115,8 @@ func (r *configuringReconciler) Reconcile(ctx context.Context, in *v1alpha1.Oper
return
}

nextPhase = phase.GetNext(phase.Succeeded)
r.logger.Info("The object has been successfully reconciled")
r.logger.Info("CatalogSourceConfig object has been updated successfully")

nextPhase = phase.GetNext(phase.Succeeded)
return
}
14 changes: 3 additions & 11 deletions pkg/operatorsource/purging.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (
"github.com/operator-framework/operator-marketplace/pkg/datastore"
"github.com/operator-framework/operator-marketplace/pkg/phase"
log "github.com/sirupsen/logrus"
k8s_errors "k8s.io/apimachinery/pkg/api/errors"
"sigs.k8s.io/controller-runtime/pkg/client"
)

Expand Down Expand Up @@ -52,19 +51,12 @@ func (r *purgingReconciler) Reconcile(ctx context.Context, in *v1alpha1.Operator

out = in.DeepCopy()

r.logger.Info("Purging all resource(s)")
// We will purge the datastore and leave the CatalogSourceConfig object
// alone. It will be updated accordingly by the reconciliation loop.

r.datastore.RemoveOperatorSource(in.GetUID())

builder := &CatalogSourceConfigBuilder{}
csc := builder.WithTypeMeta().
WithNamespacedName(in.Namespace, in.Name).
CatalogSourceConfig()

if err = r.client.Delete(ctx, csc); err != nil && !k8s_errors.IsNotFound(err) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Do not delete CatalogSourceConfig object(s) during purge, instead let the CatalogSourceConfig(s) be updated during the reconciliation process.

I don't follow how the CatalogSourceConfig will get updated during the reconciliation process given it's phase will remain at Succeeded. Shouldn't you force an update so that we get the current state from Quay? Or are you saying that it will get updated during the next sync loop?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CatalogSourceConfig object will get updated during the 'Configuring' phase.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right this makes sense. Basically the Purging phase, rather than deleting the object, just resets the status and then sets the next phase to Initial. That way the configuring reconciler just updates the already existing object and it should work.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, sorry I missed the bit of existing code below. Make sense now.

nextPhase = phase.GetNextWithMessage(phase.OperatorSourcePurging, err.Error())
return
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you update the comments in this file to reflect that we are no longer actually "purging" the operator source object itself?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should also be reflected in log statement which says Purging all resource(s)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the log statement to clarify what has been purged.

r.logger.Info("Purged datastore. No change(s) were made to corresponding CatalogSourceConfig")

// Since all observable states stored in the Status resource might already
// be stale, we should Reset everything in Status except for 'Current Phase'
Expand Down
42 changes: 0 additions & 42 deletions pkg/operatorsource/purging_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@ import (
"testing"

"github.com/stretchr/testify/assert"
k8s_errors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/runtime/schema"

gomock "github.com/golang/mock/gomock"
"github.com/operator-framework/operator-marketplace/pkg/apis/marketplace/v1alpha1"
Expand Down Expand Up @@ -37,51 +35,11 @@ func TestReconcileWithPurging(t *testing.T) {
reconciler := operatorsource.NewPurgingReconciler(helperGetContextLogger(), datastore, client)

// We expect the operator source to be removed from the datastore.
csc := helperNewCatalogSourceConfig(opsrcIn.Namespace, opsrcIn.Name)
datastore.EXPECT().RemoveOperatorSource(opsrcIn.GetUID()).Times(1)

// We expect the associated CatalogConfigSource object to be deleted.
client.EXPECT().Delete(ctx, csc)

opsrcGot, nextPhaseGot, errGot := reconciler.Reconcile(ctx, opsrcIn)

assert.NoError(t, errGot)
assert.Equal(t, opsrcWant, opsrcGot)
assert.Equal(t, nextPhaseWant, nextPhaseGot)
}

// In the event the associated CatalogSourceConfig object is not found while
// purging is in progress, we expect NotFound error to be ignored and the next
// phase set to "Initial" so that reconciliation can start anew.
func TestReconcileWithPurgingWithCatalogSourceConfigNotFound(t *testing.T) {
controller := gomock.NewController(t)
defer controller.Finish()

ctx := context.TODO()

opsrcIn := helperNewOperatorSourceWithPhase("marketplace", "foo", phase.OperatorSourcePurging)
opsrcWant := opsrcIn.DeepCopy()

nextPhaseWant := &v1alpha1.Phase{
Name: phase.Initial,
Message: phase.GetMessage(phase.Initial),
}

datastore := mocks.NewDatastoreWriter(controller)
client := mocks.NewKubeClient(controller)
reconciler := operatorsource.NewPurgingReconciler(helperGetContextLogger(), datastore, client)

// We expect the operator source to be removed from the datastore.
csc := helperNewCatalogSourceConfig(opsrcIn.Namespace, opsrcIn.Name)
datastore.EXPECT().RemoveOperatorSource(opsrcIn.GetUID())

// We expect kube client to throw a NotFound error.
notFoundErr := k8s_errors.NewNotFound(schema.GroupResource{}, "CatalogSourceConfig not found")
client.EXPECT().Delete(ctx, csc).Return(notFoundErr)

opsrcGot, nextPhaseGot, errGot := reconciler.Reconcile(ctx, opsrcIn)

assert.Error(t, errGot)
assert.Equal(t, opsrcGot, opsrcWant)
assert.Equal(t, nextPhaseWant, nextPhaseGot)
}