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.

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

CLOUDP-277700: Reduced reconcile period for externally linked resources #1854

Merged
merged 1 commit into from
Oct 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,10 @@ func (r *AtlasDatabaseUserReconciler) ready(ctx *workflow.Context, atlasDatabase
EnsureStatusOption(status.AtlasDatabaseUserNameOption(atlasDatabaseUser.Spec.Username)).
EnsureStatusOption(status.AtlasDatabaseUserPasswordVersion(passwordVersion))

if atlasDatabaseUser.Spec.ExternalProjectRef != nil {
return workflow.Requeue(workflow.StandaloneResourceRequeuePeriod).ReconcileResult()
}

return workflow.OK().ReconcileResult()
Comment on lines +205 to 209
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the only place we exit OK right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes

}

Expand Down
169 changes: 169 additions & 0 deletions pkg/controller/atlasdatabaseuser/atlasdatabaseuser_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,10 @@ import (
"errors"
"reflect"
"testing"
"time"

"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
"go.mongodb.org/atlas-sdk/v20231115008/admin"
Expand Down Expand Up @@ -283,6 +286,172 @@ func TestTerminate(t *testing.T) {
}
}

func TestReady(t *testing.T) {
tests := map[string]struct {
dbUser *akov2.AtlasDatabaseUser
passwordVersion string
interceptors interceptor.Funcs

expectedResult ctrl.Result
expectedConditions []api.Condition
}{
"fail to set finalizer": {
dbUser: &akov2.AtlasDatabaseUser{
ObjectMeta: metav1.ObjectMeta{
Name: "user1",
Namespace: "default",
},
Spec: akov2.AtlasDatabaseUserSpec{
Project: &common.ResourceRefNamespaced{
Name: "my-project",
Namespace: "default",
},
Username: "user1",
PasswordSecret: &common.ResourceRef{
Name: "user-pass",
},
DatabaseName: "admin",
},
},
passwordVersion: "1",
interceptors: interceptor.Funcs{
Patch: func(ctx context.Context, client client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error {
return errors.New("failed to set finalizer")
},
},
expectedResult: workflow.Terminate(workflow.AtlasFinalizerNotSet, "").ReconcileResult(),
expectedConditions: []api.Condition{
api.FalseCondition(api.DatabaseUserReadyType).
WithReason(string(workflow.AtlasFinalizerNotSet)).
WithMessageRegexp("failed to set finalizer"),
},
},
"fail to set last applied config": {
dbUser: &akov2.AtlasDatabaseUser{
ObjectMeta: metav1.ObjectMeta{
Name: "user1",
Namespace: "default",
},
Spec: akov2.AtlasDatabaseUserSpec{
Project: &common.ResourceRefNamespaced{
Name: "my-project",
Namespace: "default",
},
Username: "user1",
PasswordSecret: &common.ResourceRef{
Name: "user-pass",
},
DatabaseName: "admin",
},
},
passwordVersion: "1",
interceptors: interceptor.Funcs{
Patch: func(ctx context.Context, client client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error {
if patch.Type() == types.JSONPatchType {
return nil
}

return errors.New("failed to set last applied config")
},
},
expectedResult: workflow.Terminate(workflow.Internal, "").ReconcileResult(),
expectedConditions: []api.Condition{
api.FalseCondition(api.DatabaseUserReadyType).
WithReason(string(workflow.Internal)).
WithMessageRegexp("failed to set last applied config"),
},
},
"don't requeue when it's a linked resource": {
dbUser: &akov2.AtlasDatabaseUser{
ObjectMeta: metav1.ObjectMeta{
Name: "user1",
Namespace: "default",
},
Spec: akov2.AtlasDatabaseUserSpec{
Project: &common.ResourceRefNamespaced{
Name: "my-project",
Namespace: "default",
},
Username: "user1",
PasswordSecret: &common.ResourceRef{
Name: "user-pass",
},
DatabaseName: "admin",
},
},
passwordVersion: "1",
expectedResult: workflow.OK().ReconcileResult(),
expectedConditions: []api.Condition{
api.TrueCondition(api.ReadyType),
api.TrueCondition(api.DatabaseUserReadyType),
},
},
"don't requeue when it's a standalone resource": {
dbUser: &akov2.AtlasDatabaseUser{
ObjectMeta: metav1.ObjectMeta{
Name: "user1",
Namespace: "default",
},
Spec: akov2.AtlasDatabaseUserSpec{
ExternalProjectRef: &akov2.ExternalProjectReference{
ID: "project-id",
},
LocalCredentialHolder: api.LocalCredentialHolder{
ConnectionSecret: &api.LocalObjectReference{
Name: "user-creds",
},
},
Username: "user1",
PasswordSecret: &common.ResourceRef{
Name: "user-pass",
},
DatabaseName: "admin",
},
},
passwordVersion: "1",
expectedResult: workflow.Requeue(15 * time.Minute).ReconcileResult(),
expectedConditions: []api.Condition{
api.TrueCondition(api.ReadyType),
api.TrueCondition(api.DatabaseUserReadyType),
},
},
}

for name, tt := range tests {
t.Run(name, func(t *testing.T) {
testScheme := runtime.NewScheme()
assert.NoError(t, akov2.AddToScheme(testScheme))
assert.NoError(t, corev1.AddToScheme(testScheme))
k8sClient := fake.NewClientBuilder().
WithScheme(testScheme).
WithObjects(tt.dbUser).
WithStatusSubresource(tt.dbUser).
WithInterceptorFuncs(tt.interceptors).
Build()

logger := zaptest.NewLogger(t).Sugar()
c := &AtlasDatabaseUserReconciler{
Client: k8sClient,
Log: logger,
}
ctx := &workflow.Context{
Context: context.Background(),
Log: logger,
}

assert.Equal(t, tt.expectedResult, c.ready(ctx, tt.dbUser, tt.passwordVersion))
assert.True(
t,
cmp.Equal(
tt.expectedConditions,
ctx.Conditions(),
cmpopts.IgnoreFields(api.Condition{}, "LastTransitionTime"),
),
)
})
}
}

func TestFindAtlasDatabaseUserForSecret(t *testing.T) {
for _, tc := range []struct {
name string
Expand Down
12 changes: 10 additions & 2 deletions pkg/controller/workflow/result.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,9 @@ import (
)

const (
DefaultRetry = time.Second * 10
DefaultTimeout = time.Minute * 20
DefaultRetry = time.Second * 10
StandaloneResourceRequeuePeriod = time.Minute * 15
Copy link
Collaborator

@josvazg josvazg Oct 9, 2024

Choose a reason for hiding this comment

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

We said we would make this configurable down to 5 mins minimum. We can do it on a follow up.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TD doc says it's 15 minutes. That's why I set it as the value

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the heads up, I have fixed the TD to reiterate what was said in the scope, this should be configurable before release and 5 minutes is the minimum value allowed.

DefaultTimeout = time.Minute * 20
)

type Result struct {
Expand All @@ -30,6 +31,13 @@ func OK() Result {
}
}

func Requeue(period time.Duration) Result {
return Result{
terminated: false,
requeueAfter: period,
}
}

// Terminate indicates that the reconciliation logic cannot proceed and needs to be finished (and possibly requeued).
// This is not an expected termination of the reconciliation process so 'warning' flag is set to 'true'.
// 'reason' and 'message' indicate the error state and are supposed to be reflected in the `conditions` for the
Expand Down
Loading