Skip to content
This repository has been archived by the owner on Feb 5, 2020. It is now read-only.

Ignore AMI updates on etcd/master nodes to prevent destroying nodes accidently #367

Merged
merged 1 commit into from
Apr 26, 2017

Conversation

chancez
Copy link
Contributor

@chancez chancez commented Apr 25, 2017

Currently if a new version of CoreOS comes out on a user's configured
channel after they've already deployed, and they re-run terraform apply,
Terraform will detect the new AMI, and when reconciling desired state,
it will attempt to destroy existing nodes to update the AMI.

In order to avoid this, I added a ignore_changes to both the etcd node
resource, and the master launch configuration resource, to avoid
updating masters/etcd nodes if the AMI changes.

Longer term, I would expect the Container Linux Update Operator, or a
terraform operator could resolve this, but this seems like a reasonable
change to prevent accidental destruction of a cluster.

Previously this change also set prevent_destroy, but that cannot be
parameterized with vars, so it was removed.

@chancez
Copy link
Contributor Author

chancez commented Apr 26, 2017

I was going to make changes to parameterize the force_destroy, but ran into issues and found this:

hashicorp/terraform#10730

It seems there's no way to easily prevent destruction of a resource during apply, but allow for it during destroy today. I've updated my PR remove the force_destroy bit, as the ignore_changes should handle the case I was seeing, but it would be good if we could find a way to prevent accidental deletion of etcd nodes on a terraform apply, as I've done so a few times now by accident, even after changing nothing in my vars. Perhaps we can contribute something to upstream similar to what's discussed in hashicorp/terraform#3116.

Currently if a new version of CoreOS comes out on a user's configured
channel after they've already deployed, and they re-run terraform apply,
Terraform will detect the new AMI, and when reconciling desired state,
it will attempt to destroy existing nodes to update the AMI.

In order to avoid this, I added a ignore_changes to both the etcd node
resource, and the master launch configuration resource, to avoid
updating masters/etcd nodes if the AMI changes.

Longer term, I would expect the Container Linux Update Operator, or a
terraform operator could resolve this, but this seems like a reasonable
change to prevent accidental destruction of a cluster.

Previously this change also set prevent_destroy, but that cannot be
parameterized with vars, so it was removed.
@chancez chancez changed the title Ignore AMI updates on etcd/master nodes, and disallow destroying etcd nodes resource Ignore AMI updates on etcd/master nodes to prevent destroying nodes accidently Apr 26, 2017
Copy link
Contributor

@s-urbaniak s-urbaniak left a comment

Choose a reason for hiding this comment

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

LGTM, good catch, but I would like to have @alexsomesan have another look.

@alexsomesan
Copy link
Contributor

I agree with @s-urbaniak. It's a good fix.
Users aren't left out in the dark past anyway if they keep CL updates enabled.
Let keep doing this until we have a more solid story around it based on Container Linux Update Operator or the like.

LGTM.

@s-urbaniak s-urbaniak merged commit e9f6aed into coreos:master Apr 26, 2017
@sym3tri
Copy link
Contributor

sym3tri commented Apr 26, 2017

Why not add to workers as well?

@s-urbaniak
Copy link
Contributor

@sym3tri good catch! @chancez do you mind to submit another PR which includes workers?

@chancez
Copy link
Contributor Author

chancez commented Apr 26, 2017

I figured I'd keep the scope small since it's a no brainer for etcd and masters, but wasn't sure about workers. I'd be glad to make another PR with the same change.

chancez pushed a commit to chancez/tectonic-installer that referenced this pull request Apr 26, 2017
Prevents terraform from recreating the resource when a new CoreOS
release comes out.

This an extension of the same changes done to etcd and masters in coreos#367
chancez pushed a commit to chancez/tectonic-installer that referenced this pull request Apr 27, 2017
Prevents terraform from recreating the resource when a new CoreOS
release comes out.

This an extension of the same changes done to etcd and masters in coreos#367
erjohnso pushed a commit to erjohnso/tectonic-installer that referenced this pull request May 3, 2017
Prevents terraform from recreating the resource when a new CoreOS
release comes out.

This an extension of the same changes done to etcd and masters in coreos#367
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants