-
Notifications
You must be signed in to change notification settings - Fork 9.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
resource/aws_ecs_service: Change type of placement_strategy to TypeList #1476
Conversation
This PR is compatible with the current master👍 |
Can't wait to get this merged! 👍 |
Tested it on our stack and it works. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe your idea is correct here, this should be a list, however the change presents at least two issues I see:
- Because
placement_strategy
is aForceNew
attribute, users may see a need to re-create their ECS Services because the ordering of your configuration may not match the ordering AWS has. I believe this would only occur if you created the service, then happened to rearrange the strategies in your configuration. It’s not very likely but it’s still there, and we’d need to mark this as a breaking change. - The current implementation normalizes
host
toinstanceId
for the user:
- https://github.com/terraform-providers/terraform-provider-aws/pull/1476/files#diff-51a2b869d60207ffb83c18f0daef501dL136 .
This code does not, so if the user is usinghost
, they’ll see a diff. We need to either normalize, or update the documentation to note that you should useinstanceId
instead.
Let me know what you think about these issues
@catsby Thank you for review.
I agree with you. And from the first, does
I just didn’t consider normalization. |
And should we update the documentation to reflect that the order of |
About normalization, I misunderstood. uhhh.. it's impossible to hold the order depending on the current AWS state. |
It maybe incorrect... So far I couldn't understand the behaviour of |
@catsby It'd be great if you could correct me when I misunderstand or miss any workflows🙇 |
Sorry:bow: |
Is it possible to get eyes on this. We can't deploy our stack with this bug present as the priority is basicly randomized. |
@catsby It'd be great if you could review again 🙇 |
|
@catsby @radeksimko What is the status of this PR? Will be merged? |
This is indeed a breaking change, unfortunately. I'll discuss internally where that leaves the the PR as far as what release it goes into, 1.3.0 or delay to 2.0.0 |
@catsby I got it. Thank you👍 |
Hey @radeksimko since this is potentially a breaking change, should we delay this to a 2.0.0 release? |
I think we could make it non-breaking change with state migration, but at this point it is BC. |
@radeksimko Could you give me advice about how to implement without breaking change? state migration? |
Hey there! We have some way of migrating the state from one version to the next. All states start at 0 unless specified. Here are some example migrations (they have matching test files, too)
You trigger them by adding this info into the schema for the resource: In this example, we tell the schema to invoke Unfortunately I'm not immediately aware of examples of migrating from
I believe those are the basics here. I understand this is asking a lot of you. If you're not up to it I completely understand, you've done so much already. We can likely take it from here and attempt the above, but I can't promise it will be any time week or next. Please let me know if you have any questions! |
AWS ECS Service is partially immutable (see here) so every change related to placement strategy/constraints should result in forcenew. |
@s-maj correct! With this pull request as is, there will likely be users who are confronted with a The changes made in this pull request are changes made to how we store the configuration of placement strategies. It could be that with this change, no actual changes to the ECS Service are needed. For example, and I could be wrong, but I suspect that anyone with a single placement strategy will sees no change, Terraform simply changes how it's stored internally. For people with more than 1, the migration could possibly convert them into their proper order, and there's no difference. It's likely though with increasing number of strategies defined that this likelihood would go down though. If the ordering is different, then we'll see a I would need to tinker to verify 🤔 |
Ok, I see your point now :) I assumed that all folks are using default
"spread(attribute:ecs.availability-zone), spread(instanceId)" or something
evenly complex but some might have simpler strategies.
…On Wed, Nov 8, 2017 at 4:24 PM, Clint ***@***.***> wrote:
@s-maj <https://github.com/s-maj> correct! With this pull request *as is*,
there will likely be users who are confronted with a ForceNew in
subsequent terraform plan invocations. Hopefully a state migration can
lower the number of people who need them.
The changes made in this pull request are changes made to how we store the
configuration of placement strategies. It could be that with this change,
no actual changes to the ECS Service are needed. For example, and I could
be wrong, but I suspect that anyone with a single placement strategy will
sees no change, Terraform simply changes how it's stored internally.
I would need to tinker to verify 🤔
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1476 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AOhaWvw3D1G8JlWYQQm9D5kOb5v81Yt4ks5s0dXYgaJpZM4O_cie>
.
|
@catsby Thank you for your guidance. It seems so fun that I'm eager to try it. But when I have a trouble, I'll ask your help:bow: |
@catsby @radeksimko Have you already decided to merge this PR for v2.0.0? |
We will come back to this PR as well as to other PRs which are labelled as |
@radeksimko @catsby Cloud you review migration? I think it avoid breaking change so this PR can be merged before |
328a758
to
1e6ef87
Compare
@radeksimko, @catsby given that @atsushi-ishibashi has resolved the issue that would make this a breaking change, is there a possibility to get this in before |
@radeksimko, @catsby - can we please look at re-prioritising this one? this bug breaks high availability in a lot of scenarios, and it's seemingly just waiting on a review... we're willing to sponsor this fix to get it out the door. |
@radeksimko @catsby Adding another voice to the chorus asking if there's any update on this. I believe this should be considered a major bug in the provider, as the incorrect (and unexpected) ordering can cause real issues when using ECS. @atsushi-ishibashi Looks like there's a merge conflict now. Do you think you can clear that up so this can be merged once the maintainers response? Thanks so much for providing this fix. |
Hi Everyone! 👋 Sorry this is causing a lot of pain and frustration due to the unexpected behavior of Terraform. Thank you very much for your efforts here. Indeed the implementation details of this PR are very problematic due to how Terraform internally stores state versus the actual configuration. I'm personally not aware of a good way to seamlessly migrate an existing attribute from All is not lost though! We recently needed to do something similar for CloudFront distributions to migrate from an existing unordered attribute to an ordered one: #4117 You'll notice there that we opted to create a brand new attribute (prepended with To that end, I'm going to close this PR in preference for an implementation similar to #4117 for this situation, rather than keep this old and unlikely to be accepted PR open. Again, thank you @atsushi-ishibashi for your work here. For those not familiar, @atsushi-ishibashi does a bunch of great work in this provider. 👍 Please reach out with any questions! |
I'm not aware of anyone at HashiCorp working or planning to work on this at this time, but if I see a pull request come through I'll happily look at it. |
I will 💪 |
Thank you @atsushi-ishibashi - the hero the ECS community needs. |
For those following this issue you'll probably be interested in knowing we have implemented a new argument |
Hi there! We are currently working on fixing our deprecation warnings within our terraform scripts and this issue is one of them. I'm simply wondering if there is a way for us to do a state migration for all of our ecs services as they are ForcedNew by this change. If it helps, all of our services have a single placement strategy. Thanks! |
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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks! |
#1466