hashicorp / terraform

Terraform enables you to safely and predictably create, change, and improve infrastructure. It is a source-available tool that codifies APIs into declarative configuration files that can be shared amongst team members, treated as code, edited, reviewed, and versioned.
https://www.terraform.io/
Other
42.75k stars 9.56k forks source link

Terraform insists on destroying EBS disks when destroying EC2 instances, even when they aren't attached #30614

Open JohnEmhoff opened 2 years ago

JohnEmhoff commented 2 years ago

Terraform Version

Terraform v1.1.7
on linux_amd64
+ provider registry.terraform.io/hashicorp/aws v3.72.0
+ provider registry.terraform.io/hashicorp/local v2.1.0

Terraform Configuration Files

resource "aws_instance" "ec2" {
  ami           = var.ami
  instance_type = "m5.2xlarge"

  subnet_id              = var.subnet_ids[0]
  key_name               = aws_key_pair.ec2.key_name
  iam_instance_profile   = aws_iam_instance_profile.basic-profile.name

  root_block_device {
    volume_size = 64
  }
}

resource "aws_ebs_volume" "data" {
  availability_zone = aws_instance.ec2.availability_zone
  size              = 2048
}

resource "aws_volume_attachment" "attachment" {
  device_name = "/dev/xvdi"
  volume_id   = aws_ebs_volume.data.id
  instance_id = aws_instance.ec2.id
}

Debug Output

Expected Behavior

Running terraform destroy -target aws_instance.ec2 should delete the EC2 instance and the attachment, but not the EBS disk itself. I thought a work around would be to delete the attachment, but terraform still wants to delete the EBS disk. This is after doing a -destroy on the attachment and also even after completely removing the attachment from the configuration. I have no idea how terraform even knows to associate the two resources at that point.

Actual Behavior

The destroy command above always wants to destroy the EBS disk, even when the attachment no longer exists.

Steps to Reproduce

  1. terraform init
  2. terraform apply
  3. terraform destroy -target aws_instance.ec2

Additional Context

References

I think this is a pretty old issue, there's a stack overflow post with someone wrestling with exactly this from a couple years ago: https://devops.stackexchange.com/questions/10900/how-to-terraform-destroy-an-aws-instance-without-also-destroying-its-aws-eb

apparentlymart commented 2 years ago

Hi @JohnEmhoff,

I suspect what's happening here is that Terraform remembers in the state that when it created the EBS volume it was declared to depend on the EC2 instance, and so as far as Terraform is concerned that means that the EBS volume cannot outlive the EC2 instance.

In this case though I can see that the dependency you've created there isn't a "real" one -- you're just using the EC2 instance's availability zone as a convenience to avoid separately looking up the availability zone that var.subnet_ids[0] belongs to. Unfortunately all dependencies are the same as far as Terraform Core is concerned, so it has no way to understand that the reference in availability_zone = aws_instance.ec2.availability_zone doesn't imply that aws_ebs_volume.data cannot exist without aws_instance.ec2; that is what dependencies in Terraform normally imply.

Given that, I think what happens here matches how Terraform is intended to behave, and so this isn't a bug in the sense mean it for our labeling purposes (Terraform not behaving as designed) but you are describing a reasonable use-case and so I'm going to reclassify this as an enhancement and hopefully through discussion we can figure out how to frame your problem in a way that might enable us to design something new to address it.


To start with though, I think there are two pressing questions implied by your comment here. First how can you convince Terraform that what it has recorded in the state isn't correct, despite how the configuration was written at creation time, and second how you might've written that configuration differently to get the desired result with today's Terraform.

There are no commands for directly manipulating dependencies in the state, but it happens that an implementation detail of terraform state mv is to discard dependencies, because direct state manipulations like that don't give any information about how to update the dependencies. Therefore I think you may be able to get the state in a form that would allow you to run the command you intended to run by the following two steps:

The first of these will change the resource address in the state, discarding the stored dependencies as a side-effect. The second will then restore it back to its original address, so everything should be the same as it was except for the dependencies no longer being recorded.

After this, I think you'll be able to run the terraform destroy -target=... command you tried and get the result you wanted, as long as you've updated your configuration to no longer have any relationship between the EBS volume and the EC2 instance.

That then is a nice segue into the question of what configuration would've avoided including this incorrect dependency in the state in the first place. The key requirement here seems to be that your module needs an availability zone name, but it's designed so it can only get that indirectly via the given subnet ID. The usual way I'd recommend achieving that is to change the interface of the module to expect to be passed a subnet object as a whole, rather than just the ID, by specifying the subset of the subnet resource attributes the module needs:

variable "subnet" {
  type = object({
    id                = string
    availability_zone = string
  })
}

resource "aws_instance" "ec2" {
  ami           = var.ami
  instance_type = "m5.2xlarge"

  subnet_id              = var.subnet.id
  key_name               = aws_key_pair.ec2.key_name
  iam_instance_profile   = aws_iam_instance_profile.basic-profile.name

  root_block_device {
    volume_size = 64
  }
}

resource "aws_ebs_volume" "data" {
  availability_zone = var.subnet.availability_zone
  size              = 2048
}

resource "aws_volume_attachment" "attachment" {
  device_name = "/dev/xvdi"
  volume_id   = aws_ebs_volume.data.id
  instance_id = aws_instance.ec2.id
}

Then from the caller's perspective it would look like this:

resource "aws_subnet" "example" {
  # (could alternatively be data "aws_subnet" "example" with the same effect,
  # if this subnet is managed elsewhere)

  # ...
}

module "example" {
  source = "..."

  # Just pass in the entire resource object. The module requires
  # a subset of this object type, and so type conversion will
  # succeed and the module will get both of the required
  # attributes.
  subnet = aws_subnet.example
}

With this design, the volume depends on the subnet rather than the EC2 instance. That isn't totally accurate either -- volumes belong to availability zones rather than subnets, and so technically it should be possible for the volume to outlive the subnet -- but hopefully it's accurate enough for your purposes here. If not, then the most accurate representation of the true dependency graph would be to pass the availability zone name in to the module separately and have the volume depend only on that, thus making its lifecycle independent of the subnet too.

Note also that -target is intended for exceptional use rather than routine use, and so if you're intending to do what you described as part of a routine workflow then that would suggest moving the EBS volume declaration into a separate configuration entirely, and consuming it in this configuration only as an external dependency declared using a data block.

But the above is a correct model of the dependencies sufficient if you're only destroying the EC2 instance for some exceptional, one-off reason. You might also consider terraform apply -replace aws_instance.ec2 if your goal is to just get a new instance with the same volume attached, though that will still rely on you having described the dependency graph accurately so that Terraform can see that the volume can outlive the instance.


I don't yet feel ready to try to speculate about how to generalize what you experienced here into something we could use to start designing a solution, but I'm hoping that if you try the ideas I shared above and let us know how that worked out.

If you're able, it would be most helpful if you could say a little more about why it's important for you to be able to declare this EC2 instance in the way you tried here, so that we can start to think about what missing Terraform feature might've helped you get the result you wanted in a better way -- ideally without using -target, which is always a last resort.

Thanks!

JohnEmhoff commented 2 years ago

Hey @apparentlymart, thanks for the thoughtful and detailed response. That makes a lot of sense and the insight into how the dependencies are handled is very helpful. The solutions you suggest sounds pretty reasonable -- I was expecting to have to do a state rm / import which felt a little too futzy and dangerous.

As for exactly why I'm trying to do this thing with -target...right now I just want to prove that we've somewhat decoupled our state from the rest of the infrastructure, so that we can e.g. swap out the instance or whatever. The exact mechanism of destroy -target is definitely artificial but I'm glad that it forced the issue and triggered some learning.

Now, in regards to a "fix" -- it would have been nice if terraform could have told me what the precise dependency link between the disk and instance was when it went to destroy. I'm not going to claim I have ideas for how to convey that information clearly and succinctly, though :) (and I'm totally ready to be embarrassed when you tell me I could have found it by scrolling up in my logs).

I can only assume that making terraform "smarter" here about the types of dependencies is a total non-starter, and in the end will probably just make issues like this harder to find and more subtle.

jbardin commented 2 years ago

I have a feeling that the state mv workaround is not going to avoid the same problem here for a couple reasons. The dependencies are not immediately discarded during that operation in case they are still relevant during the refactoring, though they can be later dropped only once terraform can see they no longer make sense. The other reason being that the configuration tends to trump all, and since the config explicitly states that the aws_ebs_volume.data depends on aws_instance.ec2, that can always be added back in during the plan (though using destroy might be an edge case here, since it runs a slightly different plan of its own).

I think you will have more luck re-working the configured dependencies as described above, rather than fighting against the behavior of Terraform.

apparentlymart commented 2 years ago

Hi @JohnEmhoff! Thanks for that extra context.

It seems like one particular thing that was challenging here is that -target doesn't really give any feedback about what it's including and excluding, and why. Internally it honestly works as a bit of an exceptional trick -- in light of it being an exceptional-use feature -- where it just takes the graph Terraform would normally have built, with all of the dependency edges that arise from Terraform's normal rules, and then cuts off subgraphs from it in a way that is pretty naive to what exactly the graph is representing. Consequently, it doesn't really have enough context about what it is doing to talk about why it's chopping off what it's chopping: it sees the world only in terms of graph theory, rather than in terms of infrastructure. It only knows that A depends on B and therefore if I target B I must also include A, without understanding what A or B are and why they are connected.

I don't know that we'll be able to do anything remarkable to improve this, but I appreciate you sharing it. One thing we could consider is some way to view the dependencies of a resource instance that are captured in the state, but I'd worry then that it wouldn't be obvious that it's worth looking at that until you already know that it's the state that is saving them, and so you probably would've still been unsure where that dependency was coming from.

JohnEmhoff commented 2 years ago

@jbardin Good to know! I ended up changing the subnet IDs to tuples of (subnet_id, az) and it's working as I expect (I'm okay with the subnet <-> EBS dependency it introduces).

@apparentlymart That's a fair point RE: -target being weird, but I think state/dependency feedback would be possible in the more common case of a config change that triggers a destroy/create. If I changed the instance type, for instance, it still would have tried to delete the EBS disk and it would have been helpful to know why.

apparentlymart commented 2 years ago

If we think specifically about the situation where Terraform is replacing the EC2 instance (rather than just destroying it) I think the game is slightly different because Terraform has additional information in that case: it can see how the attributes of that EC2 instance object are going to change as a result of the replacement.

How availability_zone will change depends on how the provider has implemented that look up. If it's able to look up the AZ for the subnet at planning time then that creates the most ideal case: Terraform will be able to see that the before and after availability zones are the same, and so can conclude that this particular reference can survive in this case and so a downstream action isn't needed.

More likely though the provider will look this up only at apply time, because I expect the provider is just echoing back data from an object in the EC2 API that's describing the instance itself, rather than asking directly about the subnet. In that case there would be a diff on the EC2 instance like this:

   availability_zone = "us-east-1" -> (known after apply)

That would then be echoed in the EBS volume, since its AZ is derived from this one. I assume the provider would propose to replace the EBS volume in this case because the AZ might change, and so Terraform must be conservative and assume that it will. (Even though our intuitions about AWS tell us it won't change in practice; the AWS provider would be giving a slightly inaccurate prediction in this case, because internally it's also being a bit naive about how subnet ids relate to AZs)

Do you think plan that included a diff like the above for both the EC2 instance and the volume would be sufficient to explain to you why Terraform is proposing to replace both of them? It's true that the plan output doesn't explicitly say that the EBS volume's AZ depends on the EC2 instance's AZ (and maybe it could) but that information is at least visible in your configuration.

(My intent here is not to defend Terraform's current handling, but rather to understand if the issue here was primarily because of using -target or whether there's a deeper cause we could look into.)

JohnEmhoff commented 2 years ago

I assume the provider would propose to replace the EBS volume in this case because the AZ might change, and so Terraform must be conservative and assume that it will.

I just tested this by changing the instance config in a way that forces a replacement (and undoing the fix from earlier), and that is in fact what happens.

Do you think plan that included a diff like the above for both the EC2 instance and the volume would be sufficient to explain to you why Terraform is proposing to replace both of them?

The plan does in fact say it's the AZ that causes the EBS deletion:

-/+ resource "aws_ebs_volume" "data" {
      ~ arn                  = "..." -> (known after apply)
      ~ availability_zone    = "us-east-2a" -> (known after apply) # forces replacement
      ~ encrypted            = false -> (known after apply)
...

It doesn't go all the way and say it's because the EC2 instance is being recycled, but I think there are enough breadcrumbs that a more patient version of myself would have noticed this and figured it out. So, what's left? I'm not really sure -- I think the main piece of insight I got from all this is how to think a little more about resource dependencies.

apparentlymart commented 2 years ago

Great, thanks for confirming!

I think then I would take away the following from what we've discussed here:

I think that means that the main root problem here is that destroy mode and targeting don't really blend well together, and target doesn't generate enough explanation to understand why.