Skip to content
This repository has been archived by the owner on Jul 11, 2023. It is now read-only.

Support multiple EBS volumes attached and persisted to single node ASG. #319

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

Magicloud
Copy link
Contributor

@Magicloud Magicloud commented Apr 3, 2020

This change contains multiple changes to modules, to support having multiple persisted EBS attached to single node ASG.

The problem is that, since this is a breaking change, many variables/outputs are different. I think we should make them new modules to avoid breaking current code.

@Magicloud
Copy link
Contributor Author

@ketzacoatl The change works, but not the final code (filling up description and so) since I'd like to hear your idea. Is it better to make another few modules instead of making breaking changes to original modules?

@Magicloud Magicloud changed the title Update Changelog for new release Support multiple EBS-es attached and persisted to single node ASG. Apr 7, 2020
@ketzacoatl ketzacoatl changed the title Support multiple EBS-es attached and persisted to single node ASG. Support multiple EBS volumes attached and persisted to single node ASG. Apr 7, 2020
Copy link
Contributor

@ketzacoatl ketzacoatl left a comment

Choose a reason for hiding this comment

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

Please add a README and Makefile with the example, and include in the README notes on how to use the example and run the tests.

modules/init-snippet-attach-ebs-volume/snippet.tpl Outdated Show resolved Hide resolved
modules/persistent-ebs/locals.tf Show resolved Hide resolved
modules/persistent-ebs/main.tf Show resolved Hide resolved
modules/persistent-ebs/outputs.tf Show resolved Hide resolved
modules/persistent-ebs/variables.tf Outdated Show resolved Hide resolved
modules/single-node-asg/main.tf Show resolved Hide resolved
modules/single-node-asg/outputs.tf Show resolved Hide resolved
modules/single-node-asg/variables.tf Outdated Show resolved Hide resolved
@ketzacoatl
Copy link
Contributor

@Magicloud do we also want to create a copy of the previous single-node-asg module to allow users access to the previous workflow?

@qrilka
Copy link
Contributor

qrilka commented Apr 27, 2020

@Magicloud what's the status of this? Do you plan to add README as requested above and add a module to use with the previous workflow?

@Magicloud
Copy link
Contributor Author

Please add a README and Makefile with the example, and include in the README notes on how to use the example and run the tests.

Done.

# set -x
apt update
${module.install-awscli.init_snippet}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update header comments as well (about CLI availability), thanks

@qrilka
Copy link
Contributor

qrilka commented May 26, 2020

I tried to create an ASG with no extra volumes (passing []) got an error during apply:


Error: Invalid index

  on .terraform/modules/asg-a/modules/init-snippet-attach-ebs-volume/main.tf line 19, in data "template_file" "init_snippet":
  19:     device_path   = var.device_paths[count.index]
    |----------------
    | count.index is 0
    | var.device_paths is empty list of string

The given key does not identify an element in this collection value.

@Magicloud
Copy link
Contributor Author

I tried to create an ASG with no extra volumes (passing []) got an error during apply:


Error: Invalid index

  on .terraform/modules/asg-a/modules/init-snippet-attach-ebs-volume/main.tf line 19, in data "template_file" "init_snippet":
  19:     device_path   = var.device_paths[count.index]
    |----------------
    | count.index is 0
    | var.device_paths is empty list of string

The given key does not identify an element in this collection value.

@qrilka Does this happen on existing resources? I tried from beginning, no problem.

@qrilka
Copy link
Contributor

qrilka commented May 27, 2020

@Magicloud yes, probably that was the reason - I switched from master version to this branch

@qrilka
Copy link
Contributor

qrilka commented May 27, 2020

So probably it could be a problem when upgrading modules (and when this will get merged)

@Magicloud
Copy link
Contributor Author

@qrilka I see. Since the logic is changed totally, it could break.

@qrilka
Copy link
Contributor

qrilka commented Jun 4, 2020

@Magicloud while using this branch for A/B switching demo I have found that tags cause unnecessary updates like the following:

  # module.asg-magenta.module.service-data.aws_ebs_volume.main[0] will be updated in-place
  ~ resource "aws_ebs_volume" "main" {
        arn                  = "arn:aws:ec2:us-west-2:xxxxx:volume/vol-xxxxx"
        availability_zone    = "us-west-2a"
        encrypted            = true
        id                   = "vol-xxxx"
        iops                 = 100
        kms_key_id           = "arn:aws:kms:us-west-2:xxxx:key/xxxx-ae63-4278-bba4-xxxx"
        multi_attach_enabled = false
        size                 = 20
      ~ tags                 = {
            "Name"             = "switching-demo-magenta-us-west-2a-a"
          - "cldy-instance-id" = "i-0b4f3320df090f7ca" -> null
        }
        type                 = "gp2"
    }

@Magicloud
Copy link
Contributor Author

@qrilka Where does that tag come from?

@qrilka
Copy link
Contributor

qrilka commented Jun 15, 2020

That's a good question but I have not idea and not sure if I will have time to find it out...

@ketzacoatl
Copy link
Contributor

That tag is unrelated and we should not have blocked this PR on that. Unless there's reason to hold back, I think we should merge this PR.

Tested in examples/single-node-asg-tester

The change in all three modules is compatible with old code that defines just one data volume.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants