-
Notifications
You must be signed in to change notification settings - Fork 155
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
TaskDefinition replacement on every update #4576
Comments
Can you share anything about your code? I cannot reproduce this with a simple test. const nginx = new awsx.ecs.FargateService("nginx", {
taskDefinitionArgs: {
containers: {
nginx: {
image: "nginx",
},
},
}
});
|
Sure. See this example code: https://gist.github.com/kennyjwilli/18920da9c2d880270dc2f92d1f6b1908. Running with these deps.
|
@kennyjwilli I know it's been a while and this might no longer be relevant, but I also ran into this. You can see from the diff you posted that the keys were initially all sorted, and Pulumi is trying to update them to match the unsorted order defined in your code. So... our workaround at the time was to sort the container's environment variables by name, something like
This is definitely not ideal. We didn't dig in enough to figure out exactly what was wrong. Our theory at the time was that Pulumi might be applying some JSON canonicalization during update but not preview. Edit: I remember now that |
I spent some time looking at this this afternoon. I am fairly certain that replacements will only be proposed if there is an actual change. If there is no change at all, the ordering of env vars won't matter. The underlying provider takes care of taking env var ordering into account as part of deciding whether there is a diff or not. And if the provider says there is not a diff (after taking into account canonicalization which includes canonical reordernig of env vars), then a replacement will not be proposed. In the original example above for example, I see that the second container removed the
It's plausible that this addition of All that said, there is a second issue which makes this more confusing. Once the diff (and replacement) are identified, Pulumi then renders what it believes the diff to be back to the user. When it does that, it does not account for the canonicalization that was used to determine whether a diff was required or not. So it renders the diff in terms of the original string inputs - which may have had a variety of differences that don't matter to the canonicalized diff logic - but there will be at least one difference which does matter. This one difference will be hard to find amongst the noise though. Notably, the upstream provider has specific logic to handle the noise of env var reordering in this exact condition: StateFunc: func(v interface{}) string {
// Sort the lists of environment variables as they are serialized to state, so we won't get
// spurious reorderings in plans (diff is suppressed if the environment variables haven't changed,
// but they still show in the plan if some other property changes).
orderedCDs, _ := expandContainerDefinitions(v.(string))
containerDefinitions(orderedCDs).OrderEnvironmentVariables()
unnormalizedJson, _ := flattenContainerDefinitions(orderedCDs)
json, _ := structure.NormalizeJsonString(unnormalizedJson)
return json
}, And Pulumi does invoke that state func - which leads to the following in the Pulumi stat file: {
"urn": "urn:pulumi:dev::ecsreplace::awsx:ecs:FargateService$awsx:ecs:FargateTaskDefinition$aws:ecs/taskDefinition:TaskDefinition::service",
"custom": true,
"id": "service-cf4359e8",
"type": "aws:ecs/taskDefinition:TaskDefinition",
"inputs": {
"__defaults": [
"skipDestroy"
],
"containerDefinitions": "[{\"cpu\":512,\"environment\":[{\"name\":\"FOO\",\"value\":\"BAR\"},{\"name\":\"BAZ\",\"value\":\"BAR2\"},{\"name\":\"ZAB\",\"value\":\"BAR3\"}],\"essential\":true,\"image\":\"153052959999.dkr.ecr.us-west-2.amazonaws.com/repo-c7a2477:0d5bf3701e4847eaa6faff0ff8c8e7c26b34dc32fc8bd50b7aceb04b174d81e6\",\"memory\":128,\"portMappings\":[{\"containerPort\":80,\"hostPort\":80}],\"name\":\"container\",\"logConfiguration\":{\"logDriver\":\"awslogs\",\"options\":{\"awslogs-group\":\"service-81e1d63\",\"awslogs-region\":\"us-west-2\",\"awslogs-stream-prefix\":\"container\"}}}]",
"cpu": "512",
"executionRoleArn": "arn:aws:iam::153052959999:role/service-execution-d3a7c84",
"family": "service-cf4359e8",
"memory": "1024",
"networkMode": "awsvpc",
"requiresCompatibilities": [
"FARGATE"
],
"skipDestroy": false,
"taskRoleArn": "arn:aws:iam::153052959999:role/service-task-4338deb"
},
"outputs": {
"__meta": "{\"schema_version\":\"1\"}",
"arn": "arn:aws:ecs:us-west-2:153052959999:task-definition/service-cf4359e8:1",
"containerDefinitions": "[{\"cpu\":512,\"environment\":[{\"name\":\"BAZ\",\"value\":\"BAR2\"},{\"name\":\"FOO\",\"value\":\"BAR\"},{\"name\":\"ZAB\",\"value\":\"BAR3\"}],\"essential\":true,\"image\":\"153052959999.dkr.ecr.us-west-2.amazonaws.com/repo-c7a2477:0d5bf3701e4847eaa6faff0ff8c8e7c26b34dc32fc8bd50b7aceb04b174d81e6\",\"logConfiguration\":{\"logDriver\":\"awslogs\",\"options\":{\"awslogs-group\":\"service-81e1d63\",\"awslogs-region\":\"us-west-2\",\"awslogs-stream-prefix\":\"container\"}},\"memory\":128,\"mountPoints\":[],\"name\":\"container\",\"portMappings\":[{\"containerPort\":80,\"hostPort\":80,\"protocol\":\"tcp\"}],\"volumesFrom\":[]}]",
"cpu": "512",
"ephemeralStorage": null,
"executionRoleArn": "arn:aws:iam::153052959999:role/service-execution-d3a7c84",
"family": "service-cf4359e8",
"id": "service-cf4359e8",
"inferenceAccelerators": [],
"ipcMode": "",
"memory": "1024",
"networkMode": "awsvpc",
"pidMode": "",
"placementConstraints": [],
"proxyConfiguration": null,
"requiresCompatibilities": [
"FARGATE"
],
"revision": 1,
"runtimePlatform": null,
"skipDestroy": false,
"tags": {},
"tagsAll": {},
"taskRoleArn": "arn:aws:iam::153052959999:role/service-task-4338deb",
"volumes": []
},
...
}, Note that the |
Few years passed but I'm facing the same issue - I tried to sort the
I added logging into my pulumi code for the resulting I'm using
Does anyone have more insight into this or any potential workaround I could try? Is this actually a bug in Pulumi or terraform? aving ECS trigger re-deploy of the whole service with every |
my fault - it was actually caused by a duplicate environment variable 🤦♂️ |
Wanted to share another case we encountered today, in case it helps 🙂 . If environment variable resolves to new aws.ecs.TaskDefinition(
'example-task-definition',
{
family: 'example-service',
containerDefinitions: JSON.stringify([
{
name: 'example-service',
image: '<<REDACTED>>',
environment: [{
name: 'EXAMPLE_VARIABLE',
value: null
}],
},
]),
}
) Ref 1. Not quite working minimal example of |
Thanks for this repro! If this reproduces still this can be fixable by improving diff normalizers for TaskDefinition in pulumi-aws provider (or upstream TF provider if applicable). Moving to pulumi-aws I think this is where the fix needs to happen. |
I was able to repro it. Upstream is not correctly handling environment variables that have a null value. The intended behavior can be copied from the AWS Batch integration for ECS, that one is already filtering out nulls: https://github.com/hashicorp/terraform-provider-aws/blob/465d0690625e474010378042164d42595e617d1a/internal/service/batch/ecs_properties.go#L80-L95 What's notable is that this also happens for environment variables set to an empty string |
I opened an upstream issue to track this: hashicorp/terraform-provider-aws#39533 |
Pulumi thinks it needs to replace my
aws:ecs:TaskDefinition
on every up run even though nothing has changed. Looking at the diff output for thecontainerDefinitions
parameter on the TaskDefinition, I can see the only difference is on both my containers,mountPoints
,portMappings
,user
, andvolumesFrom
currently exist as empty arrays (except user which is"0"
) and do not exist in the desired TaskDefinition. I am not setting these properties. Below are the before and after of thecontainerDefinitions
parameter. The JSON Diff tool is useful for comparing. This happens every time, however, I have not reproduced this issue outside of our codebase.As a workaround, I can manually add each of the missing properties. This results in a correct diff on each update (i.e., no changes when no changes are present).
Before
After
Diff
The text was updated successfully, but these errors were encountered: