Skip to content

CLOUDP-80765: Handle Cluster pause/unpause #89

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

Merged
merged 15 commits into from
Feb 10, 2021
Merged

CLOUDP-80765: Handle Cluster pause/unpause #89

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
78cb6db
Add special case for Paused
vasilevp Feb 5, 2021
f432351
Run make generate manifests
vasilevp Feb 5, 2021
6755c28
Add int tests for cluster (un)pause
vasilevp Feb 8, 2021
3b00aab
Support "REPAIRING" state (for pause)
vasilevp Feb 8, 2021
3cd01f8
Add tests between pause/unpause
vasilevp Feb 9, 2021
5686ceb
Fix lastGeneration in tests
vasilevp Feb 9, 2021
ba12fab
Reduce project remove timeout
vasilevp Feb 9, 2021
8f88f60
Merge remote-tracking branch 'origin/main' into CLOUDP-80765-cluster-…
vasilevp Feb 9, 2021
6f60768
Add common checks to post-pause
vasilevp Feb 9, 2021
0c1d6d2
Fix atlas cluster unit test
vasilevp Feb 9, 2021
6146120
Rework tests
vasilevp Feb 9, 2021
f3bf39c
Starting lastGeneration should be 0
vasilevp Feb 10, 2021
fa78e81
Merge remote-tracking branch 'origin/main' into CLOUDP-80765-cluster-…
vasilevp Feb 10, 2021
110483f
More lastGeneration stuff
vasilevp Feb 10, 2021
3a1e0cf
Fix timeout after merge
vasilevp Feb 10, 2021
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
3 changes: 3 additions & 0 deletions config/crd/bases/atlas.mongodb.com_atlasclusters.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,9 @@ spec:
maximum: 50
minimum: 1
type: integer
paused:
description: Flag that indicates whether the cluster should be paused.
type: boolean
pitEnabled:
description: Flag that indicates the cluster uses continuous cloud
backups.
Expand Down
3 changes: 3 additions & 0 deletions pkg/api/v1/atlascluster_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,9 @@ type AtlasClusterSpec struct {
// +optional
NumShards *int `json:"numShards,omitempty"`

// Flag that indicates whether the cluster should be paused.
Paused *bool `json:"paused,omitempty"`

// Flag that indicates the cluster uses continuous cloud backups.
// +optional
PitEnabled *bool `json:"pitEnabled,omitempty"`
Expand Down
9 changes: 4 additions & 5 deletions pkg/api/v1/atlascluster_types_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,10 @@ import (
"go.mongodb.org/atlas/mongodbatlas"
)

var excludedClusterFieldsOurs = map[string]bool{}
var excludedClusterFieldsTheirs = map[string]bool{}
var (
excludedClusterFieldsOurs = map[string]bool{}
excludedClusterFieldsTheirs = map[string]bool{}
)

func init() {
excludedClusterFieldsOurs["projectRef"] = true
Expand All @@ -32,9 +34,6 @@ func init() {
excludedClusterFieldsTheirs["connectionStrings"] = true
excludedClusterFieldsTheirs["srvAddress"] = true
excludedClusterFieldsTheirs["stateName"] = true

// CLOUDP-80765
excludedClusterFieldsTheirs["paused"] = true
}

func TestCompatibility(t *testing.T) {
Expand Down
5 changes: 5 additions & 0 deletions pkg/api/v1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

21 changes: 17 additions & 4 deletions pkg/controller/atlascluster/cluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,15 +48,28 @@ func (r *AtlasClusterReconciler) ensureClusterState(log *zap.SugaredLogger, conn

switch c.StateName {
case "IDLE":
if done, err := clusterMatchesSpec(log, c, cluster.Spec); err != nil {
return c, workflow.Terminate(workflow.Internal, err.Error())
} else if done {
return c, workflow.OK()
}

spec, err := cluster.Spec.Cluster()
if err != nil {
return c, workflow.Terminate(workflow.Internal, err.Error())
}

if done, err := clusterMatchesSpec(log, c, cluster.Spec); err != nil {
return c, workflow.Terminate(workflow.Internal, err.Error())
} else if done {
return c, workflow.OK()
if cluster.Spec.Paused != nil {
if c.Paused == nil || *c.Paused != *cluster.Spec.Paused {
// paused is different from Atlas
// we need to first send a special (un)pause request before reconciling everything else
spec = &mongodbatlas.Cluster{
Paused: cluster.Spec.Paused,
}
} else {
// otherwise, don't send the paused field
spec.Paused = nil
Copy link
Contributor

@antonlisovenko antonlisovenko Feb 8, 2021

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@vasilevp vasilevp Feb 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@antonlisovenko antonlisovenko Feb 8, 2021

Choose a reason for hiding this comment

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

thanks for the information, I think this should be safe 👍

}
}

c, _, err = client.Clusters.Update(ctx, project.Status.ID, cluster.Spec.Name, spec)
Expand Down
46 changes: 43 additions & 3 deletions test/int/cluster_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,6 @@ var _ = Describe("AtlasCluster", func() {
)))
Expect(createdCluster.Status.ObservedGeneration).To(Equal(createdCluster.Generation))
Expect(createdCluster.Status.ObservedGeneration).To(Equal(lastGeneration + 1))
lastGeneration++
})
}

Expand Down Expand Up @@ -116,7 +115,7 @@ var _ = Describe("AtlasCluster", func() {
Eventually(testutil.WaitFor(k8sClient, createdCluster, status.TrueCondition(status.ReadyType), validateClusterUpdatingFunc()),
1200, interval).Should(BeTrue())

doCommonChecks()
lastGeneration++
}

Describe("Create/Update the cluster", func() {
Expand All @@ -139,12 +138,14 @@ var _ = Describe("AtlasCluster", func() {
By("Updating the Cluster labels", func() {
createdCluster.Spec.Labels = []mdbv1.LabelSpec{{Key: "int-test", Value: "true"}}
performUpdate()
doCommonChecks()
checkAtlasState()
})

By("Updating the Cluster backups settings", func() {
createdCluster.Spec.ProviderBackupEnabled = boolptr(true)
performUpdate()
doCommonChecks()
checkAtlasState(func(c *mongodbatlas.Cluster) {
Expect(c.ProviderBackupEnabled).To(Equal(createdCluster.Spec.ProviderBackupEnabled))
})
Expand All @@ -153,13 +154,52 @@ var _ = Describe("AtlasCluster", func() {
By("Decreasing the Cluster disk size", func() {
createdCluster.Spec.DiskSizeGB = intptr(10)
performUpdate()
doCommonChecks()
checkAtlasState(func(c *mongodbatlas.Cluster) {
Expect(*c.DiskSizeGB).To(BeEquivalentTo(*createdCluster.Spec.DiskSizeGB))

// check whether https://github.com/mongodb/go-client-mongodb-atlas/issues/140 is fixed
Expect(c.DiskSizeGB).To(BeAssignableToTypeOf(float64ptr(0)), "DiskSizeGB is no longer a *float64, please check the spec!")
})
})

By("Pausing the cluster", func() {
createdCluster.Spec.Paused = boolptr(true)
performUpdate()
doCommonChecks()
checkAtlasState(func(c *mongodbatlas.Cluster) {
Expect(c.Paused).To(Equal(createdCluster.Spec.Paused))
})
Copy link
Contributor

@antonlisovenko antonlisovenko Feb 8, 2021

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@vasilevp vasilevp Feb 8, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@antonlisovenko antonlisovenko Feb 8, 2021

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@vasilevp vasilevp Feb 8, 2021

Choose a reason for hiding this comment

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

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...

Copy link
Contributor

@antonlisovenko antonlisovenko Feb 8, 2021

Choose a reason for hiding this comment

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

as discussed p2p having the cluster as "not ready" is a correct outcome 👍 This is something we can confirm with the test

})

By("Updating the Cluster configuration while paused (should fail)", func() {
createdCluster.Spec.ProviderBackupEnabled = boolptr(false)

Expect(k8sClient.Update(context.Background(), createdCluster)).To(Succeed())
Eventually(
testutil.WaitFor(k8sClient, createdCluster, status.FalseCondition(status.ClusterReadyType).WithReason(string(workflow.ClusterNotCreatedInAtlas))),
60,
interval,
).Should(BeTrue())

lastGeneration++
})

By("Unpausing the cluster", func() {
createdCluster.Spec.Paused = boolptr(false)
performUpdate()
doCommonChecks()
checkAtlasState(func(c *mongodbatlas.Cluster) {
Expect(c.Paused).To(Equal(createdCluster.Spec.Paused))
})
})

By("Checking that modifications were applied after unpausing", func() {
doCommonChecks()
checkAtlasState(func(c *mongodbatlas.Cluster) {
Copy link
Contributor

@antonlisovenko antonlisovenko Feb 9, 2021

Choose a reason for hiding this comment

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

can we also check that the AtlasCluster has now all status conditions set to true?

Expect(c.ProviderBackupEnabled).To(Equal(createdCluster.Spec.ProviderBackupEnabled))
})
})
})
})
})
Expand Down Expand Up @@ -197,7 +237,7 @@ func validateClusterUpdatingFunc() func(a mdbv1.AtlasCustomResource) {
}
// When the create request has been made to Atlas - we expect the following status
if !isIdle {
Expect(c.Status.StateName).To(Equal("UPDATING"), fmt.Sprintf("Current conditions: %+v", c.Status.Conditions))
Expect(c.Status.StateName).To(Or(Equal("UPDATING"), Equal("REPAIRING")), fmt.Sprintf("Current conditions: %+v", c.Status.Conditions))
expectedConditionsMatchers := testutil.MatchConditions(
status.FalseCondition(status.ClusterReadyType).WithReason(string(workflow.ClusterUpdating)).WithMessageRegexp("cluster is updating"),
status.FalseCondition(status.ReadyType),
Expand Down