Skip to content

Commit 7aa9667

Browse files
vasilevpvasilevp
authored
Add long-running test cases & implement slice merge for ReplicationSpecs (#138)
1 parent 150732d commit 7aa9667

File tree

6 files changed

+327
-37
lines changed

6 files changed

+327
-37
lines changed

‎pkg/api/v1/atlascluster_types.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,6 @@ func init() {
3131
}
3232

3333
type ProviderName string
34-
type ClusterType string
3534

3635
const (
3736
ProviderAWS ProviderName = "AWS"
@@ -40,6 +39,8 @@ const (
4039
ProviderTenant ProviderName = "TENANT"
4140
)
4241

42+
type ClusterType string
43+
4344
const (
4445
TypeReplicaSet ClusterType = "REPLICASET"
4546
TypeSharded ClusterType = "SHARDED"

‎pkg/controller/atlascluster/cluster.go‎

Lines changed: 47 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -40,31 +40,31 @@ func (r *AtlasClusterReconciler) ensureClusterState(ctx *workflow.Context, proje
4040

4141
switch c.StateName {
4242
case "IDLE":
43-
if done, err := clusterMatchesSpec(ctx.Log, c, cluster.Spec); err != nil {
43+
resultingCluster, err := mergedCluster(*c, cluster.Spec)
44+
if err != nil {
4445
return c, workflow.Terminate(workflow.Internal, err.Error())
45-
} else if done {
46-
return c, workflow.OK()
4746
}
4847

49-
spec, err := cluster.Spec.Cluster()
50-
if err != nil {
51-
return c, workflow.Terminate(workflow.Internal, err.Error())
48+
if done := clustersEqual(ctx.Log, *c, resultingCluster); done {
49+
return c, workflow.OK()
5250
}
5351

5452
if cluster.Spec.Paused != nil {
5553
if c.Paused == nil || *c.Paused != *cluster.Spec.Paused {
5654
// paused is different from Atlas
5755
// we need to first send a special (un)pause request before reconciling everything else
58-
spec = &mongodbatlas.Cluster{
56+
resultingCluster = mongodbatlas.Cluster{
5957
Paused: cluster.Spec.Paused,
6058
}
6159
} else {
6260
// otherwise, don't send the paused field
63-
spec.Paused = nil
61+
resultingCluster.Paused = nil
6462
}
6563
}
6664

67-
c, _, err = ctx.Client.Clusters.Update(context.Background(), project.Status.ID, cluster.Spec.Name, spec)
65+
resultingCluster = cleanupCluster(resultingCluster)
66+
67+
c, _, err = ctx.Client.Clusters.Update(context.Background(), project.Status.ID, cluster.Spec.Name, &resultingCluster)
6868
if err != nil {
6969
return c, workflow.Terminate(workflow.ClusterNotUpdatedInAtlas, err.Error())
7070
}
@@ -84,22 +84,49 @@ func (r *AtlasClusterReconciler) ensureClusterState(ctx *workflow.Context, proje
8484
}
8585
}
8686

87-
// clusterMatchesSpec will merge everything from the Spec into existing Cluster and use that to detect change.
88-
// Direct comparison is not feasible because Atlas will set a lot of fields to default values, so we need to apply our changes on top of that.
89-
func clusterMatchesSpec(log *zap.SugaredLogger, cluster *mongodbatlas.Cluster, spec mdbv1.AtlasClusterSpec) (bool, error) {
90-
clusterMerged := mongodbatlas.Cluster{}
91-
if err := compat.JSONCopy(&clusterMerged, cluster); err != nil {
92-
return false, err
87+
// cleanupCluster will unset some fields that cannot be changed via API or are deprecated.
88+
func cleanupCluster(cluster mongodbatlas.Cluster) mongodbatlas.Cluster {
89+
cluster.ID = ""
90+
cluster.MongoDBVersion = ""
91+
cluster.MongoURI = ""
92+
cluster.MongoURIUpdated = ""
93+
cluster.MongoURIWithOptions = ""
94+
cluster.SrvAddress = ""
95+
cluster.StateName = ""
96+
cluster.ReplicationFactor = nil
97+
cluster.ReplicationSpec = nil
98+
cluster.ConnectionStrings = nil
99+
return cluster
100+
}
101+
102+
// mergedCluster will return the result of merging AtlasClusterSpec with Atlas Cluster
103+
func mergedCluster(cluster mongodbatlas.Cluster, spec mdbv1.AtlasClusterSpec) (result mongodbatlas.Cluster, err error) {
104+
if err = compat.JSONCopy(&result, cluster); err != nil {
105+
return
106+
}
107+
108+
if err = compat.JSONCopy(&result, spec); err != nil {
109+
return
93110
}
94111

95-
if err := compat.JSONCopy(&clusterMerged, spec); err != nil {
96-
return false, err
112+
// TODO: might need to do this with other slices
113+
if err = compat.JSONSliceMerge(&result.ReplicationSpecs, cluster.ReplicationSpecs); err != nil {
114+
return
97115
}
98116

99-
d := cmp.Diff(*cluster, clusterMerged, cmpopts.EquateEmpty())
117+
if err = compat.JSONSliceMerge(&result.ReplicationSpecs, spec.ReplicationSpecs); err != nil {
118+
return
119+
}
120+
121+
return
122+
}
123+
124+
// clustersEqual compares two Atlas Clusters
125+
func clustersEqual(log *zap.SugaredLogger, clusterA mongodbatlas.Cluster, clusterB mongodbatlas.Cluster) bool {
126+
d := cmp.Diff(clusterA, clusterB, cmpopts.EquateEmpty())
100127
if d != "" {
101-
log.Debugf("Cluster differs from spec: %s", d)
128+
log.Debugf("Clusters are different: %s", d)
102129
}
103130

104-
return d == "", nil
131+
return d == ""
105132
}

‎pkg/controller/atlascluster/cluster_test.go‎

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -12,29 +12,34 @@ import (
1212

1313
func TestClusterMatchesSpec(t *testing.T) {
1414
t.Run("Clusters match (enums)", func(t *testing.T) {
15-
atlasClusterEnum := mongodbatlas.Cluster{
15+
atlasCluster := mongodbatlas.Cluster{
1616
ProviderSettings: &mongodbatlas.ProviderSettings{
1717
ProviderName: "AWS",
1818
},
1919
ClusterType: "GEOSHARDED",
2020
}
21-
operatorClusterEnum := mdbv1.AtlasClusterSpec{
21+
operatorCluster := mdbv1.AtlasClusterSpec{
2222
ProviderSettings: &mdbv1.ProviderSettingsSpec{
2323
ProviderName: mdbv1.ProviderAWS,
2424
},
2525
ClusterType: mdbv1.TypeGeoSharded,
2626
}
2727

28-
match, err := clusterMatchesSpec(zap.S(), &atlasClusterEnum, operatorClusterEnum)
28+
merged, err := mergedCluster(atlasCluster, operatorCluster)
2929
assert.NoError(t, err)
30-
assert.True(t, match)
30+
31+
equal := clustersEqual(zap.S(), atlasCluster, merged)
32+
assert.True(t, equal)
3133
})
34+
3235
t.Run("Clusters don't match (enums)", func(t *testing.T) {
3336
atlasClusterEnum := mongodbatlas.Cluster{ClusterType: "GEOSHARDED"}
3437
operatorClusterEnum := mdbv1.AtlasClusterSpec{ClusterType: mdbv1.TypeReplicaSet}
3538

36-
match, err := clusterMatchesSpec(zap.S(), &atlasClusterEnum, operatorClusterEnum)
39+
merged, err := mergedCluster(atlasClusterEnum, operatorClusterEnum)
3740
assert.NoError(t, err)
38-
assert.False(t, match)
41+
42+
equal := clustersEqual(zap.S(), atlasClusterEnum, merged)
43+
assert.False(t, equal)
3944
})
4045
}

‎pkg/util/compat/json_slice_merge.go‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
package compat
2+
3+
import (
4+
"errors"
5+
"fmt"
6+
"reflect"
7+
)
8+
9+
// JSONSliceMerge will merge two slices using JSONCopy according to these rules:
10+
//
11+
// 1. If `dst` and `src` are the same length, all elements are merged
12+
//
13+
// 2. If `dst` is longer, only the first `len(src)` elements are merged
14+
//
15+
// 3. If `src` is longer, first `len(dst)` elements are merged, then remaining elements are appended to `dst`
16+
func JSONSliceMerge(dst, src interface{}) error {
17+
dstVal := reflect.ValueOf(dst)
18+
srcVal := reflect.ValueOf(src)
19+
20+
if dstVal.Kind() != reflect.Ptr {
21+
return errors.New("dst must be a pointer to slice")
22+
}
23+
24+
dstVal = reflect.Indirect(dstVal)
25+
srcVal = reflect.Indirect(srcVal)
26+
27+
if dstVal.Kind() != reflect.Slice {
28+
return errors.New("dst must be pointing to a slice")
29+
}
30+
31+
if srcVal.Kind() != reflect.Slice {
32+
return errors.New("src must be a slice or a pointer to slice")
33+
}
34+
35+
minLen := dstVal.Len()
36+
if srcVal.Len() < minLen {
37+
minLen = srcVal.Len()
38+
}
39+
40+
// merge common elements
41+
for i := 0; i < minLen; i++ {
42+
dstX := dstVal.Index(i).Addr().Interface()
43+
if err := JSONCopy(dstX, srcVal.Index(i).Interface()); err != nil {
44+
return fmt.Errorf("cannot copy value at index %d: %w", i, err)
45+
}
46+
}
47+
48+
// append extra elements (if any)
49+
dstType := reflect.TypeOf(dst).Elem().Elem()
50+
for i := minLen; i < srcVal.Len(); i++ {
51+
newVal := reflect.New(dstType).Interface()
52+
if err := JSONCopy(&newVal, srcVal.Index(i).Interface()); err != nil {
53+
return fmt.Errorf("cannot copy value at index %d: %w", i, err)
54+
}
55+
dstVal.Set(reflect.Append(dstVal, reflect.ValueOf(newVal).Elem()))
56+
}
57+
58+
return nil
59+
}

‎pkg/util/compat/json_slice_merge_test.go‎

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
package compat_test
2+
3+
import (
4+
"errors"
5+
"testing"
6+
7+
"github.com/stretchr/testify/require"
8+
9+
. "github.com/mongodb/mongodb-atlas-kubernetes/pkg/util/compat"
10+
)
11+
12+
func TestJSONSliceMerge(t *testing.T) {
13+
type Item struct {
14+
ID string `json:"id,omitempty"`
15+
Name string `json:"name,omitempty"`
16+
}
17+
18+
type OtherItem struct {
19+
OtherID string `json:"id,omitempty"`
20+
OtherName string `json:"name,omitempty"`
21+
}
22+
23+
tests := []struct {
24+
name string
25+
dst, src, expected interface{}
26+
expectedError error
27+
}{
28+
{
29+
name: "src is longer",
30+
dst: &[]*Item{
31+
{"00001", "dst1"},
32+
{"00002", "dst2"},
33+
{"00003", "dst3"},
34+
},
35+
src: []OtherItem{ // copying from different element type
36+
{"99999", "src1"}, // different key, different value
37+
{"", "src2"}, // no key, different value
38+
{"", ""}, // no key, no value
39+
{"12345", "extra"}, // extra value
40+
},
41+
expected: &[]*Item{ // kept dst element type
42+
{"99999", "src1"}, // key & value replaced by src
43+
{"00002", "src2"}, // only value replaced by src
44+
{"00003", "dst3"}, // untouched
45+
{"12345", "extra"}, // appended from src
46+
},
47+
},
48+
{
49+
name: "dst is longer",
50+
dst: &[]*Item{
51+
{"00001", "dst1"},
52+
{"00002", "dst2"},
53+
{"00003", "dst3"},
54+
},
55+
src: []OtherItem{
56+
{"99999", "src1"},
57+
},
58+
expected: &[]*Item{
59+
{"99999", "src1"}, // key & value replaced by src
60+
{"00002", "dst2"}, // untouched
61+
{"00003", "dst3"}, // untouched
62+
},
63+
},
64+
{
65+
name: "src is nil",
66+
dst: &[]*Item{
67+
{"00001", "dst1"},
68+
{"00002", "dst2"},
69+
{"00003", "dst3"},
70+
},
71+
src: nil,
72+
expectedError: errors.New("src must be a slice or a pointer to slice"),
73+
expected: &[]*Item{
74+
{"00001", "dst1"}, // untouched
75+
{"00002", "dst2"}, // untouched
76+
{"00003", "dst3"}, // untouched
77+
},
78+
},
79+
{
80+
name: "dst is nil",
81+
dst: nil,
82+
expectedError: errors.New("dst must be a pointer to slice"),
83+
src: []OtherItem{
84+
{"99999", "src1"},
85+
},
86+
expected: nil,
87+
},
88+
}
89+
90+
for _, tt := range tests {
91+
t.Run(tt.name, func(t *testing.T) {
92+
require := require.New(t)
93+
err := JSONSliceMerge(tt.dst, tt.src)
94+
require.Equal(tt.expectedError, err)
95+
require.Equal(tt.expected, tt.dst)
96+
})
97+
}
98+
}

0 commit comments

Comments
 (0)