provider/aws: ElastiCache (Redis) snapshot window and retention limits #3707

stack72 · 2015-10-31T00:15:20Z

This fixes #3075

make testacc TEST=./builtin/providers/aws TESTARGS='-run=ElasticacheCluster_snapshots' 2>~/tf.log
go generate ./...
TF_ACC=1 go test ./builtin/providers/aws -v -run=ElasticacheCluster_snapshots -timeout 90m
=== RUN   TestAccAWSElasticacheCluster_snapshots
--- PASS: TestAccAWSElasticacheCluster_snapshots (366.93s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    366.946s

Also tested the ValidateFunc:

terraform [aws-elasticache-cluster-snapshoting●] % make testacc TEST=./builtin/providers/aws TESTARGS='-run=ElasticacheCluster_snapshots' 2>~/tf.log
go generate ./...
TF_ACC=1 go test ./builtin/providers/aws -v -run=ElasticacheCluster_snapshots -timeout 90m
=== RUN   TestAccAWSElasticacheCluster_snapshots
--- FAIL: TestAccAWSElasticacheCluster_snapshots (0.11s)
    testing.go:137: Step 0 error: Configuration is invalid.

        Warnings: []string(nil)

        Errors: []string{"aws_elasticache_cluster.bar: snapshot retention limit cannot be more than 35 days"}
FAIL

Creates the values as can be seen in the console:

…or Redis ElastiCache clusters

catsby · 2015-11-02T20:52:28Z

builtin/providers/aws/resource_aws_elasticache_cluster.go

@@ -344,6 +372,15 @@ func resourceAwsElasticacheClusterUpdate(d *schema.ResourceData, meta interface{
 		requestUpdate = true
 	}

+	if d.HasChange("snapshot_window") {
+		req.EngineVersion = aws.String(d.Get("snapshot_window").(string))


Probably not EngineVersion 😄

…'t update the copy/pasted params

stack72 · 2015-11-02T21:02:31Z

@catsby ok, sorry about that. these silly copy paste errors have been fixed now

catsby · 2015-11-02T21:50:21Z

Thanks @stack72 – some where things here... I thought you'd need to specify Computed: true for these attributes, but then I found you did not... so I dug in and found that SnapshotRetentionLimit and SnapshotWindow are nil in the API response... not sure why.

I did try to update the SnapshotWindow attribute and got a weird error:

* aws_elasticache_cluster.test: [WARN] Error updating ElastiCache cluster (test), error: InvalidParameterCombination: Instance type does not support snapshotting. Snapshot window parameter should not be specified.
    status code: 400, request id: 7e2dcf24-81ab-11e5-8466-199fd0904dd9

Not sure what that's about, the debug output of the update didn't mention instance type..

Plan:

~ aws_elasticache_cluster.test
    snapshot_window: "Wed:09:00-Wed:09:30" => "05:00-09:00"

Log:

[DEBUG]: 2015/11/02 15:48:47 [DEBUG] Modifying ElastiCache Cluster (test), opts:
[DEBUG]: {
[DEBUG]:   ApplyImmediately: false,
[DEBUG]:   CacheClusterId: "test",
[DEBUG]:   SnapshotWindow: "05:00-09:00"
[DEBUG]: }

Do you get this behavior too?

stack72 · 2015-11-02T21:53:34Z

@catsby I added an acceptance test to show these in motion and to make sure that they are set as expected and both worked. I was able to control my instance based on this as well

I think you are specifying a Memcached cluster?

If so, it only works for Redis clusters

catsby · 2015-11-02T21:55:35Z

Nah, I'm using Redis:

provider "aws" {
  region = "us-west-2"
}
resource "aws_vpc" "default" {
  cidr_block = "10.250.0.0/16"
  tags {
    Name = "ec-sub-test"
  }
}

resource "aws_subnet" "private" {
  vpc_id = "${aws_vpc.default.id}"
  cidr_block = "10.250.3.0/24"
}

resource "aws_subnet" "private2" {
  vpc_id = "${aws_vpc.default.id}"
  cidr_block = "10.250.2.0/24"
}

resource "aws_elasticache_subnet_group" "test" {
  name        = "test"
  description = "Test Redis"
  subnet_ids = [
    "${aws_subnet.private.*.id}", 
    "${aws_subnet.private2.id}",
  ]
}

resource "aws_elasticache_cluster" "test" {
  cluster_id = "test"
  subnet_group_name = "${aws_elasticache_subnet_group.test.name}"
  num_cache_nodes = 1
  node_type = "cache.t2.micro"
  engine = "redis"
  port = 6379

  snapshot_retention_limit = 1
  snapshot_window = "05:00-09:00"
}

The code seems to work, but adding debug statements in the read shows those attributes are nil from the API... at least for me..

stack72 · 2015-11-02T21:56:34Z

I'll have another look at the debugging of this in the morning and print some info

Thanks for checking this

…work. Also added some debugging to show that the API returns the Elasticache retention period info

stack72 · 2015-11-03T12:37:48Z

@catsby ok, I just added an extra test to make sure that updating works as expected. Also added some debug logic to the file to make sure that it writes INFO messages to the tf.log when running tests

I definitely get results on my side and I am using this logic in my code from this morning as a check

catsby · 2015-11-04T22:24:06Z

builtin/providers/aws/resource_aws_elasticache_cluster_test.go

@@ -149,6 +204,76 @@ resource "aws_elasticache_cluster" "bar" {
    port = 11211
    parameter_group_name = "default.memcached1.4"
    security_group_names = ["${aws_elasticache_security_group.bar.name}"]
+    snapshot_window = "05:00-09:00"
+    snapshot_retention_limit = 3


Engine memcached does not support snapshotting, so the TestAccAWSElasticacheCluster_basic test now fails with this addition :/

catsby · 2015-11-04T22:42:23Z

I definitely get results on my side and I am using this logic in my code from this morning as a check

Do you get results if those attributes are omitted in the config?

catsby · 2015-11-04T22:51:03Z

builtin/providers/aws/resource_aws_elasticache_cluster.go

@@ -344,6 +374,14 @@ func resourceAwsElasticacheClusterUpdate(d *schema.ResourceData, meta interface{
 		requestUpdate = true
 	}

+	if d.HasChange("snapshot_window") {


snapshot_window and snapshot_retention_limit are not toggling the requestUpdate bit, so they're never actually being sent in any update call.

stack72 · 2015-11-05T12:22:33Z

ok, @catsby all is working as expected now :)

I was able to code out the cache.t2 sizing as follows:

if !strings.Contains(d.Get("node_type").(string), "cache.t2") {
        if v, ok := d.GetOk("snapshot_retention_limit"); ok {
            req.SnapshotRetentionLimit = aws.Int64(int64(v.(int)))
        }

        if v, ok := d.GetOk("snapshot_window"); ok {
            req.SnapshotWindow = aws.String(v.(string))
        }
    }

I was also able to get the updates working (was missing requestUpdate=true):

if d.HasChange("snapshot_window") {
            req.SnapshotWindow = aws.String(d.Get("snapshot_window").(string))
            requestUpdate = true
        }

        if d.HasChange("snapshot_retention_limit") {
            req.SnapshotRetentionLimit = aws.Int64(int64(d.Get("snapshot_retention_limit").(int)))
            requestUpdate = true
        }

also made sure not to cause panics in the Read event:

if c.SnapshotWindow != nil {
            d.Set("snapshot_window", c.SnapshotWindow)
        }
        if c.SnapshotRetentionLimit != nil {
            d.Set("snapshot_retention_limit", c.SnapshotRetentionLimit)
        }

The acceptance tests all work as expected now EXCEPT the vpc test which throws a panic. I just tried this on master and get the same error I'm afraid

P.

…catsby

…catsby's findings

catsby · 2015-11-05T20:46:33Z

builtin/providers/aws/resource_aws_elasticache_cluster.go

@@ -187,6 +205,16 @@ func resourceAwsElasticacheClusterCreate(d *schema.ResourceData, meta interface{
 		req.CacheParameterGroupName = aws.String(v.(string))
 	}

+	if !strings.Contains(d.Get("node_type").(string), "cache.t2") {


I don't believe this is the approach we should take. We should document that snapshotting does not work with cache.t2's , and we should bubble up the error if a user uses t2's anyway.

Only applying these params in this block would (especially without any message) would be misleading the user, as without an error, anything in their config that doesn't throw an error in applying will be written to state, so from Terraforms view, they are applied. But in reality, they are not.

So you thibk I should just log that snapshotting isn't available if the user specifies t2 and then just let the values be set? Or should I specifically check for snapahotting in conjunction with t2 (using a check like above) and throw an error?

catsby · 2015-11-05T20:48:49Z

Left a few additional notes, and we still need to document the snapshot limitation regarding t2s, otherwise looking good 😄

catsby · 2015-11-05T21:27:58Z

The acceptance tests all work as expected now EXCEPT the vpc test which throws a panic. I just tried this on master and get the same error I'm afraid

Yup, that's my fault 😦 it gets fixed in #3777

… We now allow the error to bubble up to the userr when the wrong instance type is used. The limitation for t2 instance types now allowing snapshotting is also now documented

stack72 · 2015-11-06T11:19:15Z

ok, @catsby, I have documented the t2 issue and now let the error bubble up to the user. I now get the following:

    * aws_elasticache_cluster.bar: Error creating Elasticache: InvalidParameterCombination: Instance type does not support snapshotting. Snapshot window parameter should not be specified.
            status code: 400, request id: 7b21df4b-8477-11e5-be36-5514cd0a8d29

This goes hand in hand with the NOTE added to the docs

catsby · 2015-11-06T22:37:33Z

builtin/providers/aws/resource_aws_elasticache_cluster_test.go

+resource "aws_elasticache_cluster" "bar" {
+    cluster_id = "tf-test-%03d"
+    engine = "redis"
+    node_type = "cache.t2.small"


this needs to be changed to cache.m1.small, or snapshot_window and snapshot_retention_limit need to be removed. The test that uses this config doesn't pass

Went ahead and made this change myself, thanks!

catsby · 2015-11-06T22:56:07Z

Thanks!

resource/key_pair: drop custom ValidateFunc for name_prefix

ghost · 2020-04-30T02:05:01Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

Adding the ability to specify a snapshot window and retention limit f…

7dd1546

…or Redis ElastiCache clusters

radeksimko added enhancement provider/aws labels Oct 31, 2015

catsby reviewed Nov 2, 2015
View reviewed changes

When I was setting the update parameters for the Snapshotting, I didn…

4f05df6

…'t update the copy/pasted params

catsby added the waiting-response An issue/pull request is waiting for a response from the community label Nov 2, 2015

Added an extra test for the Elasticache Cluster to show that updates …

707bfd7

…work. Also added some debugging to show that the API returns the Elasticache retention period info

catsby reviewed Nov 4, 2015
View reviewed changes

Making the changes to the snapshotting for Elasticache Redis as per @…

ca2ea80

…catsby's findings

catsby reviewed Nov 5, 2015
View reviewed changes

Removing the instance_type check in the ElastiCache cluster creation.…

350f91e

… We now allow the error to bubble up to the userr when the wrong instance type is used. The limitation for t2 instance types now allowing snapshotting is also now documented

catsby reviewed Nov 6, 2015
View reviewed changes

catsby merged commit 350f91e into hashicorp:master Nov 6, 2015

catsby mentioned this pull request Nov 6, 2015

AWS Feature: Automated Backups for elasticache redis cluster #3075

Closed

stack72 deleted the aws-elasticache-cluster-snapshoting branch November 8, 2015 16:24

omeid pushed a commit to omeid/terraform that referenced this pull request Mar 30, 2018

Merge pull request hashicorp#3707 from loivis/validatefunc/key_pair

405fd79

resource/key_pair: drop custom ValidateFunc for name_prefix

ghost locked and limited conversation to collaborators Apr 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

provider/aws: ElastiCache (Redis) snapshot window and retention limits #3707

provider/aws: ElastiCache (Redis) snapshot window and retention limits #3707

stack72 commented Oct 31, 2015

catsby Nov 2, 2015

stack72 Nov 2, 2015

stack72 commented Nov 2, 2015

catsby commented Nov 2, 2015

stack72 commented Nov 2, 2015

catsby commented Nov 2, 2015

stack72 commented Nov 2, 2015

stack72 commented Nov 3, 2015

catsby Nov 4, 2015

catsby commented Nov 4, 2015

catsby Nov 4, 2015

stack72 commented Nov 5, 2015

catsby Nov 5, 2015

stack72 Nov 5, 2015

catsby commented Nov 5, 2015

catsby commented Nov 5, 2015

stack72 commented Nov 6, 2015

catsby Nov 6, 2015

catsby Nov 6, 2015

catsby commented Nov 6, 2015

ghost commented Apr 30, 2020

provider/aws: ElastiCache (Redis) snapshot window and retention limits #3707

provider/aws: ElastiCache (Redis) snapshot window and retention limits #3707

Conversation

stack72 commented Oct 31, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stack72 commented Nov 2, 2015

catsby commented Nov 2, 2015

stack72 commented Nov 2, 2015

catsby commented Nov 2, 2015

stack72 commented Nov 2, 2015

stack72 commented Nov 3, 2015

Choose a reason for hiding this comment

catsby commented Nov 4, 2015

Choose a reason for hiding this comment

stack72 commented Nov 5, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

catsby commented Nov 5, 2015

catsby commented Nov 5, 2015

stack72 commented Nov 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

catsby commented Nov 6, 2015

ghost commented Apr 30, 2020