Skip to content

CLOUDP-350375: MongoDBMultiCluster: Add annotation to disable reconciliation #511

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+78 −0
Open

CLOUDP-350375: MongoDBMultiCluster: Add annotation to disable reconciliation #511

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
5 changes: 5 additions & 0 deletions controllers/operator/mongodbmultireplicaset_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,11 @@ func (r *ReconcileMongoDbMultiReplicaSet) Reconcile(ctx context.Context, request
return reconcileResult, err
}

if val, ok := mrs.Annotations[util.DisableReconciliation]; ok && val == util.DisableReconciliationValue {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will be something our users will be using during migration, but as part of this PR I"m not going to document the annotation/add a release note. It will be included in the migration guide once we release a version which is ready for migration. Until then - let's leave it undocumented.

log.Info("Reconciliation disabled")
return r.updateStatus(ctx, &mrs, workflow.Disabled(), log)
Copy link
Contributor

Choose a reason for hiding this comment

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

q: It looks like disabled workflow was introduced to notify if a subresource is enabled or disabled. would it be a good idea to use the same to notify that a resource is deprecated?

I was also wondering if we should call it disabled or deprecated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@viveksinghggits I found this:

mongodb-kubernetes/api/v1/status/phase.go

Lines 20 to 21 in 219a3df

// PhaseDisabled means that the resource is not enabled
PhaseDisabled Phase = "Disabled"

I believe these statuses are generic. It just happened so that we only use it currently to report that a backup is disabled. But I think there is no reason not to use it for anything else. I'm happy to update this comment to reflect this:

mongodb-kubernetes/controllers/operator/workflow/disabled.go

Lines 7 to 10 in 219a3df

// disabledStatus indicates that the subresource is not enabled
type disabledStatus struct {
*okStatus
}

I was also wondering if we should call it disabled or deprecated?

We might be adding a deprecation message somewhere on the resource. But it will likely going to be a permanent message.

For the purpose of migration we need a status which can be used to programmatically check that the resource is not being reconciled.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think just updating the comment should be ok. Just wanted to bring the attention to that comment, not a big issue.

}

if !architectures.IsRunningStaticArchitecture(mrs.Annotations) {
agents.UpgradeAllIfNeeded(ctx, agents.ClientSecret{Client: r.client, SecretClient: r.SecretClient}, r.omConnectionFactory, GetWatchedNamespace(), true)
}
Expand Down
70 changes: 70 additions & 0 deletions controllers/operator/mongodbmultireplicaset_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"context"
"encoding/json"
"fmt"
"maps"
"sort"
"testing"

Expand Down Expand Up @@ -1425,6 +1426,75 @@ func TestValidationsRunOnReconcile(t *testing.T) {
assert.Equal(t, fmt.Sprintf("Multiple clusters with the same name (%s) are not allowed", duplicateName), mrs.Status.Message)
}

func TestReconcileDisableReconciliationAnnotation(t *testing.T) {
tests := []struct {
name string
annotations map[string]string
expectedPhase status.Phase
expectedReconcileResult reconcile.Result
}{
{
name: "reconciliation disabled when annotation is true",
annotations: map[string]string{util.DisableReconciliation: util.DisableReconciliationValue},
expectedPhase: status.PhaseDisabled,
expectedReconcileResult: reconcile.Result{Requeue: false, RequeueAfter: 0},
},
{
name: "reconciliation proceeds when annotation is false",
annotations: map[string]string{util.DisableReconciliation: "false"},
expectedPhase: status.PhaseRunning,
expectedReconcileResult: reconcile.Result{RequeueAfter: util.TWENTY_FOUR_HOURS},
},
{
name: "reconciliation proceeds when annotation is empty string",
annotations: map[string]string{util.DisableReconciliation: ""},
expectedPhase: status.PhaseRunning,
expectedReconcileResult: reconcile.Result{RequeueAfter: util.TWENTY_FOUR_HOURS},
},
{
name: "reconciliation proceeds when annotation is arbitrary value",
annotations: map[string]string{util.DisableReconciliation: "some-other-value"},
expectedPhase: status.PhaseRunning,
expectedReconcileResult: reconcile.Result{RequeueAfter: util.TWENTY_FOUR_HOURS},
},
{
name: "reconciliation proceeds when annotation is absent",
expectedPhase: status.PhaseRunning,
expectedReconcileResult: reconcile.Result{RequeueAfter: util.TWENTY_FOUR_HOURS},
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
ctx := context.Background()
mrs := mdbmulti.DefaultMultiReplicaSetBuilder().SetClusterSpecList(clusters).Build()

// Set up test annotations
if mrs.Annotations == nil {
mrs.Annotations = map[string]string{}
}
maps.Copy(mrs.Annotations, tt.annotations)

reconciler, client, _, _ := defaultMultiReplicaSetReconciler(ctx, nil, "", "", mrs)

// Update the resource with the annotation
err := client.Update(ctx, mrs)
assert.NoError(t, err)

// Reconcile should return without error and with expected result
result, err := reconciler.Reconcile(ctx, requestFromObject(mrs))
assert.NoError(t, err)
assert.Equal(t, tt.expectedReconcileResult, result)

// Fetch the updated resource to check the status
err = client.Get(ctx, kube.ObjectKey(mrs.Namespace, mrs.Name), mrs)
assert.NoError(t, err)

assert.Equal(t, tt.expectedPhase, mrs.Status.Phase)
})
}
}

func assertClusterpresent(t *testing.T, m map[string]int, specs mdb.ClusterSpecList, arr []int) {
tmp := make([]int, 0)
for _, s := range specs {
Expand Down
3 changes: 3 additions & 0 deletions pkg/util/constants.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,9 @@ const (
LastAchievedSpec = "mongodb.com/v1.lastSuccessfulConfiguration"
LastAchievedRsMemberIds = "mongodb.com/v1.lastAchievedRsMemberIds"

DisableReconciliation = "mongodb.com/v1.disableReconciliation"
DisableReconciliationValue = "true"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to define a constant because linter was not happy.


// SecretVolumeName is the name of the volume resource.
SecretVolumeName = "secret-certs"

Expand Down