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

Use non-cached CR on reconciliation #940

Merged
merged 3 commits into from
Mar 10, 2020
+33 −3
Merged

Use non-cached CR on reconciliation #940

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bc16d6a
Use non-cached CR on reconciliation
jpkrohling Mar 2, 2020
f534f5a
Fixed broken tests
jpkrohling Mar 10, 2020
d0c870b
Added new test
jpkrohling Mar 10, 2020
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
2 changes: 1 addition & 1 deletion pkg/controller/jaeger/jaeger_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ func (r *ReconcileJaeger) Reconcile(request reconcile.Request) (reconcile.Result

// Fetch the Jaeger instance
instance := &v1.Jaeger{}
err := r.client.Get(ctx, request.NamespacedName, instance)
err := r.rClient.Get(ctx, request.NamespacedName, instance)
if err != nil {
if k8serrors.IsNotFound(err) {
// Request object not found, could have been deleted after reconcile request.
Expand Down
34 changes: 32 additions & 2 deletions pkg/controller/jaeger/jaeger_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
osv1 "github.com/openshift/api/route/v1"
"github.com/spf13/viper"
"github.com/stretchr/testify/assert"
"k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"
"k8s.io/client-go/kubernetes/scheme"
Expand Down Expand Up @@ -71,7 +72,7 @@ func TestDeletedInstance(t *testing.T) {
// no known objects
cl := fake.NewFakeClient()

r := &ReconcileJaeger{client: cl, scheme: s}
r := &ReconcileJaeger{client: cl, scheme: s, rClient: cl}
jpkrohling marked this conversation as resolved.
Show resolved Hide resolved

req := reconcile.Request{
NamespacedName: types.NamespacedName{
Expand Down Expand Up @@ -132,7 +133,7 @@ func TestSkipOnNonOwnedCR(t *testing.T) {
s := scheme.Scheme
s.AddKnownTypes(v1.SchemeGroupVersion, jaeger)
cl := fake.NewFakeClient(jaeger)
r := &ReconcileJaeger{client: cl, scheme: s}
r := &ReconcileJaeger{client: cl, scheme: s, rClient: cl}
req := reconcile.Request{NamespacedName: nsn}

// test
Expand All @@ -149,6 +150,35 @@ func TestSkipOnNonOwnedCR(t *testing.T) {
assert.Equal(t, v1.JaegerPhase(""), persisted.Status.Phase)
}

func TestGetResourceFromNonCachedClient(t *testing.T) {
// prepare
nsn := types.NamespacedName{Name: "my-instance"}
jaeger := v1.NewJaeger(nsn)

s := scheme.Scheme
s.AddKnownTypes(v1.SchemeGroupVersion, jaeger)

// simulates the case where the cache is stale: the instance has been deleted (client) but the cache hasn't been updated (cachedClient)
// we trigger the reconciliation and expect it to finish without errors, while we expect to not have an instance afterwards
// if the code is using the cached client, we would end up either with an error (trying to update an instance that does not exist)
// or we'd end up with an instance
cachedClient := fake.NewFakeClient(jaeger)
client := fake.NewFakeClient()

r := &ReconcileJaeger{client: cachedClient, scheme: s, rClient: client}
req := reconcile.Request{NamespacedName: nsn}

// test
_, err := r.Reconcile(req)

// verify
assert.NoError(t, err)
persisted := &v1.Jaeger{}
err = client.Get(context.Background(), req.NamespacedName, persisted)
assert.Error(t, err)
assert.True(t, errors.IsNotFound(err))
}

func getReconciler(objs []runtime.Object) (*ReconcileJaeger, client.Client) {
s := scheme.Scheme

Expand Down