CLOUDP-80765: Handle Cluster pause/unpause #89
Conversation
antonlisovenko
left a comment
antonlisovenko
left a comment
There was a problem hiding this comment.
Interesting logic with nullifying 'paused' and performing a one-time change request.
Looks good, the only suggestion is to add update requests after pause/unpause to make sure that the explicit nullyfing the field hasn't affected anything in atlas
| performUpdate() | ||
| checkAtlasState(func(c *mongodbatlas.Cluster) { | ||
| Expect(c.Paused).To(Equal(createdCluster.Spec.Paused)) | ||
| }) |
There was a problem hiding this comment.
can we check that after the cluster paused/unpaused - the following updates to the other fields can be done?
Also I guess we need to make sure that those updates don't change the cluster in Atlas (paused/unpaused) back to nil
There was a problem hiding this comment.
Atlas does not support updates on paused clusters:
You cannot modify a paused cluster other than to resume the cluster. Nor can you pause a cluster with pending changes.
There was a problem hiding this comment.
ok, thanks for this clarification.
Then I guess the following behavior is expected?
- the user updates "paused: true"
- then the user changes other fields in the spec, controller sends this to Atlas but no changes happen there as the cluster is paused? The AtlasCluster resource is in "ready: true" state though?
- then the user updates "pause: false" and the changes done in the previous step reach Atlas now?
WDYT about testing the above scenario or maybe you'll find a better scenario to check
There was a problem hiding this comment.
then the user changes other fields in the spec, controller sends this to Atlas but no changes happen there as the cluster is paused? The AtlasCluster resource is in "ready: true" state though?
The cluster will be in a non-ready state because such updates will fail on Atlas' side. I'm not sure whether we want to make it "ready" in this scenario though...
There was a problem hiding this comment.
as discussed p2p having the cluster as "not ready" is a correct outcome 👍 This is something we can confirm with the test
| } | ||
| } else { | ||
| // otherwise, don't send the paused field | ||
| spec.Paused = nil |
There was a problem hiding this comment.
interesting! Will this update the paused in Atlas to nil as well? How does Atlas interpret paused as nil - may it start the cluster back?
There was a problem hiding this comment.
omitempty in JSON spec means that nil is not included in the payload, so no changes will be made
Docs:
You do not need to provide all parameters when modifying a cluster, only the parameters you want to change and any parameters required to make that change.
There was a problem hiding this comment.
thanks for the information, I think this should be safe 👍
antonlisovenko
left a comment
antonlisovenko
left a comment
There was a problem hiding this comment.
Thanks for the test, just an additional check in the test and should be ready to go!
| }) | ||
|
|
||
| By("Checking that modifications were applied after unpausing", func() { | ||
| checkAtlasState(func(c *mongodbatlas.Cluster) { |
There was a problem hiding this comment.
can we also check that the AtlasCluster has now all status conditions set to true?
No description provided.