Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 3 additions & 3 deletions apis/v1alpha1/ack-generate-metadata.yaml
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
ack_generate_info:
build_date: "2021-07-16T03:54:07Z"
build_date: "2021-07-16T19:55:47Z"
build_hash: 7832e9aa4a48565302cd440f4cdf2267f04adfed
go_version: go1.15.2 darwin/amd64
version: v0.5.0
api_directory_checksum: 04701e412e7e4597466c1d56571be2c5de2b1e27
api_version: v1alpha1
aws_sdk_go_version: ""
generator_config_info:
file_checksum: cc29e8be7c6f65ef2f8db75fc3752adbf18590df
file_checksum: 821cc69bbfdafc0bb1302bb609f7b92774448303
original_file_name: generator.yaml
last_modification:
reason: API generation
timestamp: 2021-07-16 03:54:15.20368 +0000 UTC
timestamp: 2021-07-16 19:55:56.542005 +0000 UTC
2 changes: 2 additions & 0 deletions apis/v1alpha1/generator.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,8 @@ resources:
template_path: hooks/sdk_delete_post_request.go.tpl
sdk_update_post_build_request:
template_path: hooks/sdk_update_post_build_request.go.tpl
delta_post_compare:
code: "filterDelta(delta, a, b)"
Snapshot:
update_conditions_custom_method_name: CustomUpdateConditions
exceptions:
Expand Down
2 changes: 2 additions & 0 deletions generator.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,8 @@ resources:
template_path: hooks/sdk_delete_post_request.go.tpl
sdk_update_post_build_request:
template_path: hooks/sdk_update_post_build_request.go.tpl
delta_post_compare:
code: "filterDelta(delta, a, b)"
Snapshot:
update_conditions_custom_method_name: CustomUpdateConditions
exceptions:
Expand Down
42 changes: 42 additions & 0 deletions pkg/common/delta.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License"). You may
// not use this file except in compliance with the License. A copy of the
// License is located at
//
// http://aws.amazon.com/apache2.0/
//
// or in the "license" file accompanying this file. This file is distributed
// on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
// express or implied. See the License for the specific language governing
// permissions and limitations under the License.

package common

import ackcompare "github.com/aws-controllers-k8s/runtime/pkg/compare"

// remove the Difference corresponding to the given subject from the delta struct
//TODO: ideally this would have a common implementation in compare/delta.go
func RemoveFromDelta(
delta *ackcompare.Delta,
subject string,
) {
// copy slice
differences := delta.Differences

// identify index of Difference to remove
//TODO: this could require a stricter Path.Equals down the road
var i *int = nil
for j, diff := range differences {
if diff.Path.Contains(subject) {
i = &j
break
}
}

// if found, create a new slice and replace the original
if i != nil {
differences = append(differences[:*i], differences[*i+1:]...)
delta.Differences = differences
}
}
Comment on lines +40 to +42
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: This method may return boolean indicating if there was a removal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ACK, not sure how we would use that return value but we can add it on later if there is a use for it

37 changes: 8 additions & 29 deletions pkg/resource/replication_group/custom_update_api.go
Original file line number Diff line number Diff line change
Expand Up @@ -84,13 +84,13 @@ func (rm *resourceManager) CustomModifyReplicationGroup(
if desired.ko.Spec.AutomaticFailoverEnabled != nil && *desired.ko.Spec.AutomaticFailoverEnabled == false {
latestAutomaticFailoverEnabled := latest.ko.Status.AutomaticFailover != nil && *latest.ko.Status.AutomaticFailover == "enabled"
if latestAutomaticFailoverEnabled != *desired.ko.Spec.AutomaticFailoverEnabled {
return rm.modifyReplicationGroup(ctx, desired, latest)
return rm.modifyReplicationGroup(ctx, desired, latest, delta)
}
}
if desired.ko.Spec.MultiAZEnabled != nil && *desired.ko.Spec.MultiAZEnabled == false {
latestMultiAZEnabled := latest.ko.Status.MultiAZ != nil && *latest.ko.Status.MultiAZ == "enabled"
if latestMultiAZEnabled != *desired.ko.Spec.MultiAZEnabled {
return rm.modifyReplicationGroup(ctx, desired, latest)
return rm.modifyReplicationGroup(ctx, desired, latest, delta)
}
}

Expand All @@ -107,7 +107,7 @@ func (rm *resourceManager) CustomModifyReplicationGroup(
return rm.updateShardConfiguration(ctx, desired, latest)
}

return rm.modifyReplicationGroup(ctx, desired, latest)
return rm.modifyReplicationGroup(ctx, desired, latest, delta)
}

// modifyReplicationGroup updates replication group
Expand All @@ -118,6 +118,7 @@ func (rm *resourceManager) modifyReplicationGroup(
ctx context.Context,
desired *resource,
latest *resource,
delta *ackcompare.Delta,
) (*resource, error) {
// Method currently handles SecurityGroupIDs, EngineVersion
// Avoid making unnecessary DescribeCacheCluster API call if both fields are nil in spec.
Expand All @@ -134,8 +135,8 @@ func (rm *resourceManager) modifyReplicationGroup(

// SecurityGroupIds, EngineVersion
if rm.securityGroupIdsDiffer(desired, latest, latestCacheCluster) ||
rm.engineVersionsDiffer(desired, latest) {
input := rm.newModifyReplicationGroupRequestPayload(desired, latest, latestCacheCluster)
delta.DifferentAt("Spec.EngineVersion") {
input := rm.newModifyReplicationGroupRequestPayload(desired, latest, latestCacheCluster, delta)
resp, respErr := rm.sdkapi.ModifyReplicationGroupWithContext(ctx, input)
rm.metrics.RecordAPICall("UPDATE", "ModifyReplicationGroup", respErr)
if respErr != nil {
Expand Down Expand Up @@ -648,6 +649,7 @@ func (rm *resourceManager) newModifyReplicationGroupRequestPayload(
desired *resource,
latest *resource,
latestCacheCluster *svcsdk.CacheCluster,
delta *ackcompare.Delta,
) *svcsdk.ModifyReplicationGroupInput {
input := &svcsdk.ModifyReplicationGroupInput{}

Expand All @@ -667,37 +669,14 @@ func (rm *resourceManager) newModifyReplicationGroupRequestPayload(
input.SetSecurityGroupIds(ids)
}

if rm.engineVersionsDiffer(desired, latest) &&
if delta.DifferentAt("Spec.EngineVersion") &&
desired.ko.Spec.EngineVersion != nil {
input.SetEngineVersion(*desired.ko.Spec.EngineVersion)
}

return input
}

/*
engineVersionsDiffer returns true if the desired engine version is different
from the latest observed engine version, and false if they differ or if
the desired EngineVersion is nil
*/
func (rm *resourceManager) engineVersionsDiffer(
desired *resource,
latest *resource,
) bool {
if desired.ko.Spec.EngineVersion == nil {
return false
}

latestEV := ""
if latest.ko.Spec.EngineVersion != nil {
latestEV = *latest.ko.Spec.EngineVersion
}

return *desired.ko.Spec.EngineVersion != latestEV

//TODO: should Delta be used in this function?
}

// This method copies the data from given replicationGroup by populating it into copy of supplied resource
// and returns it.
func (rm *resourceManager) provideUpdatedResource(
Expand Down
54 changes: 0 additions & 54 deletions pkg/resource/replication_group/custom_update_api_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -942,57 +942,3 @@ func provideCacheClusterSecurityGroups(IDs ...string) []*svcsdk.SecurityGroupMem
}
return securityGroups
}

// TestEngineVersionsDiffer tests scenarios to check if desired, latest (from cache cluster)
// Engine Version configuration differs.
func TestEngineVersionsDiffer(t *testing.T) {
assert := assert.New(t)
require := require.New(t)
// setup
rm := provideResourceManager()
// Tests
t.Run("NoDiff=NoSpec_NoStatus", func(t *testing.T) {
desiredRG := provideResource()
latestRG := provideResource()
require.Nil(desiredRG.ko.Spec.EngineVersion)
require.Nil(latestRG.ko.Spec.EngineVersion)
differ := rm.engineVersionsDiffer(desiredRG, latestRG)
assert.False(differ)
})
t.Run("NoDiff=OnlyDesiredNil", func(t *testing.T) {
desiredRG := provideResource()
latestRG := provideResource()
latestEV := "test-engine-version"
latestRG.ko.Spec.EngineVersion = &latestEV
require.Nil(desiredRG.ko.Spec.EngineVersion)
require.NotNil(latestRG.ko.Spec.EngineVersion)
differ := rm.engineVersionsDiffer(desiredRG, latestRG)
assert.False(differ)
})
t.Run("NoDiff=Desired_Latest_Match", func(t *testing.T) {
desiredRG := provideResource()
latestRG := provideResource()
latestEV := "test-engine-version"
desiredRG.ko.Spec.EngineVersion = &latestEV
latestRG.ko.Spec.EngineVersion = &latestEV
require.NotNil(desiredRG.ko.Spec.EngineVersion)
require.NotNil(latestRG.ko.Spec.EngineVersion)
differ := rm.engineVersionsDiffer(desiredRG, latestRG)
assert.False(differ)
})
t.Run("Diff=Desired_Latest_Mismatch", func(t *testing.T) {
desiredRG := provideResource()
latestRG := provideResource()
desiredEV := "desired-test-engine-version"
latestEV := "latest-test-engine-version"

desiredRG.ko.Spec.EngineVersion = &desiredEV
latestRG.ko.Spec.EngineVersion = &latestEV

require.NotNil(desiredRG.ko.Spec.EngineVersion)
require.NotNil(latestRG.ko.Spec.EngineVersion)
require.NotEqual(*desiredRG.ko.Spec.EngineVersion, *latestRG.ko.Spec.EngineVersion)
differ := rm.engineVersionsDiffer(desiredRG, latestRG)
assert.True(differ)
})
}
1 change: 1 addition & 0 deletions pkg/resource/replication_group/delta.go

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

64 changes: 64 additions & 0 deletions pkg/resource/replication_group/delta_util.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License"). You may
// not use this file except in compliance with the License. A copy of the
// License is located at
//
// http://aws.amazon.com/apache2.0/
//
// or in the "license" file accompanying this file. This file is distributed
// on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
// express or implied. See the License for the specific language governing
// permissions and limitations under the License.

package replication_group

import (
"strings"
"regexp"

ackcompare "github.com/aws-controllers-k8s/runtime/pkg/compare"

"github.com/aws-controllers-k8s/elasticache-controller/pkg/common"
)

// remove non-meaningful differences from delta
func filterDelta(
delta *ackcompare.Delta,
desired *resource,
latest *resource,
) {
Comment on lines +26 to +30
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice.

Not related to this PR but mentioning it here due to its context: I think this is where Server defaults can be handled: Remove differences from delta when corresponding spec field is nil if last applied spec also had that field nil.

  • This will require access to "last applied spec" which needs to be kept somewhere for reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep this should enable us to (at least temporarily) handle defaults before the framework-level solution is ready


if delta.DifferentAt("Spec.EngineVersion") {
if desired.ko.Spec.EngineVersion != nil && latest.ko.Spec.EngineVersion != nil {
if engineVersionsMatch(*desired.ko.Spec.EngineVersion, *latest.ko.Spec.EngineVersion) {
common.RemoveFromDelta(delta, "Spec.EngineVersion")
}
}
// TODO: handle the case of a nil difference (especially when desired EV is nil)
}
}

// returns true if desired and latest engine versions match and false otherwise
// precondition: both desiredEV and latestEV are non-nil
// this handles the case where only the major EV is specified, e.g. "6.x" (or similar), but the latest
// version shows the minor version, e.g. "6.0.5"
func engineVersionsMatch(
desiredEV string,
latestEV string,
) bool {
if desiredEV == latestEV {
return true
}

// if the last character of desiredEV is "x", only check for a major version match
last := len(desiredEV) - 1
if desiredEV[last:] == "x" {
// cut off the "x" and replace all occurrences of '.' with '\.' (as '.' is a special regex character)
desired := strings.Replace(desiredEV[:last], ".", "\\.", -1)
r, _ := regexp.Compile(desired + ".*")
return r.MatchString(latestEV)
}

return false
}
26 changes: 26 additions & 0 deletions pkg/resource/replication_group/delta_util_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License"). You may
// not use this file except in compliance with the License. A copy of the
// License is located at
//
// http://aws.amazon.com/apache2.0/
//
// or in the "license" file accompanying this file. This file is distributed
// on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
// express or implied. See the License for the specific language governing
// permissions and limitations under the License.

package replication_group

import "testing"
import "github.com/stretchr/testify/require"

func TestEngineVersionsMatch(t *testing.T) {
require := require.New(t)

require.True(engineVersionsMatch("6.x", "6.0.5"))
require.False(engineVersionsMatch("13.x", "6.0.6"))
require.True(engineVersionsMatch("5.0.3", "5.0.3"))
require.False(engineVersionsMatch("5.0.3", "5.0.4"))
}
16 changes: 16 additions & 0 deletions pkg/resource/replication_group/post_set_output.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,13 @@ func (rm *resourceManager) updateSpecFields(
latestCacheCluster, err := rm.describeCacheCluster(ctx, resource)
if err == nil && latestCacheCluster != nil {
setEngineVersion(latestCacheCluster, resource)
setMaintenanceWindow(latestCacheCluster, resource)
}
}

//TODO: for all the fields here, reevaluate if the latest observed state should always be populated,
// even if the corresponding field was not specified in desired

Comment on lines +49 to +51
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 it should be fine to populate observed state fields here, the filterDelta() should determine if the diff is to be ignored.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed

// if ReplicasPerNodeGroup was given in desired.Spec, update ko.Spec with the latest observed value
func setReplicasPerNodeGroup(
respRG *svcsdk.ReplicationGroup,
Expand Down Expand Up @@ -72,3 +76,15 @@ func setEngineVersion(
*ko.Spec.EngineVersion = *latestCacheCluster.EngineVersion
}
}

// update maintenance window (if non-nil in API response) regardless of whether it was specified in desired
func setMaintenanceWindow(
latestCacheCluster *svcsdk.CacheCluster,
resource *resource,
) {
ko := resource.ko
if latestCacheCluster.PreferredMaintenanceWindow != nil {
pmw := *latestCacheCluster.PreferredMaintenanceWindow
ko.Spec.PreferredMaintenanceWindow = &pmw
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ spec:
cacheNodeType: cache.t3.micro
engine: redis
numNodeGroups: 1
preferredMaintenanceWindow: "wed:08:00-wed:09:00"
replicasPerNodeGroup: 1
replicationGroupDescription: cluster-mode disabled RG
replicationGroupID: rg-cmd
Expand Down
Loading