-
Notifications
You must be signed in to change notification settings - Fork 21
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.
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
base: master
Are you sure you want to change the base?
CLOUDP-350375: MongoDBMultiCluster
: Add annotation to disable reconciliation
#511
Conversation
MongoDBMultiCluster
: Add annotation to disable reconciliationMongoDBMultiCluster
: Add annotation to disable reconciliation
96ca920
to
a1095fa
Compare
return reconcileResult, err | ||
} | ||
|
||
if val, ok := mrs.Annotations[util.DisableReconciliation]; ok && val == util.DisableReconciliationValue { |
There was a problem hiding this comment.
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.
LastAchievedRsMemberIds = "mongodb.com/v1.lastAchievedRsMemberIds" | ||
|
||
DisableReconciliation = "mongodb.com/v1.disableReconciliation" | ||
DisableReconciliationValue = "true" |
There was a problem hiding this comment.
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.
a1095fa
to
24ec913
Compare
The new `mongodb.com/v1.disable-reconciliation` annotation allows disabling reconciliation for `MongoDBMultiCluster` objects. This will be used later when migrating multi-cluster replica sets from `MongoDBMultiCluster` to `MongoDB`.
24ec913
to
72152af
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR looks good to me. Just left a small comment/question.
|
||
if val, ok := mrs.Annotations[util.DisableReconciliation]; ok && val == util.DisableReconciliationValue { | ||
log.Info("Reconciliation disabled") | ||
return r.updateStatus(ctx, &mrs, workflow.Disabled(), log) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Summary
The new
mongodb.com/v1.disable-reconciliation
annotation allows disabling reconciliation forMongoDBMultiCluster
objects.This will be used later when migrating multi-cluster replica sets from
MongoDBMultiCluster
toMongoDB
.Proof of Work
mongodb.com/v1.disableReconciliation
to anyMongoDBMultiCluster
kubectl wait mongodbmulticluster NAME -n NAMESPACE --for=jsonpath='{.status.phase}'=Disabled
.kubectl wait
command printsmongodbmulticluster.mongodb.com/NAME condition met
.Checklist
skip-changelog
label if not needed