Pass clusterARN in task state change struct to account for Fargate FC tasks. #4107
Conversation
| @@ -124,6 +126,9 @@ func (c *ContainerStateChange) String() string { | |||
| // String returns a human readable string representation of a TaskStateChange. | |||
| func (change *TaskStateChange) String() string { | |||
| res := fmt.Sprintf("%s -> %s", change.TaskARN, change.Status.String()) | |||
| if len(change.ClusterARN) != 0 { | |||
| res += fmt.Sprintf(", ClusterARN: %s", change.ClusterARN) | |||
There was a problem hiding this comment.
nit(non-blocking): can we follow the format on L128 "%s -> %s" here?
There was a problem hiding this comment.
Sure. Will fix it if there is any blocking comment.
| @@ -507,8 +507,10 @@ func (client *ecsClient) SubmitTaskStateChange(change ecs.TaskStateChange) error | |||
|
|
|||
| func (client *ecsClient) submitTaskStateChange(change ecs.TaskStateChange) error { | |||
|
|
|||
| clusterARN := convertTaskToClusterARN(change.TaskARN) | |||
|
|
|||
| clusterARN := client.configAccessor.Cluster() | |||
There was a problem hiding this comment.
It seems like the problem here is the cluster arn changing in between tasks, and that does not get updated in the config accessor. Wouldn't this be accessible from the config accessor so long as that were kept up to date? Why do we need an additional field on the TaskStateChange?
There was a problem hiding this comment.
Firecracker can host multiple tasks on the same instance. So, clusterARN for different tasks will be different since we use task account's clusterARN. We cannot update the configaccessor as we would need to keep updating the member variable every time the API is called for the respective task. It's not that the config accessor is kept up to date in the lifecycle of the task, its just that it would keep differing for different tasks.
There was a problem hiding this comment.
I see, thanks for explaining!
| @@ -124,6 +126,9 @@ func (c *ContainerStateChange) String() string { | |||
| // String returns a human readable string representation of a TaskStateChange. | |||
| func (change *TaskStateChange) String() string { | |||
There was a problem hiding this comment.
nit: Can we update task state change string unit testing to account for case where ClusterARN is populated?
There was a problem hiding this comment.
Sure. Will add in a follow-up CR.
| @@ -506,6 +506,11 @@ func (client *ecsClient) SubmitTaskStateChange(change ecs.TaskStateChange) error | |||
| } | |||
|
|
|||
| func (client *ecsClient) submitTaskStateChange(change ecs.TaskStateChange) error { | |||
|
|
|||
| clusterARN := client.configAccessor.Cluster() | |||
| if len(change.ClusterARN) != 0 { | |||
There was a problem hiding this comment.
nit: Prefer to use change.ClusterARN != "" in favor of len(change.ClusterARN) != 0 for readability.
There was a problem hiding this comment.
Sure. Will change in a follow-up CR.
| @@ -506,6 +506,11 @@ func (client *ecsClient) SubmitTaskStateChange(change ecs.TaskStateChange) error | |||
| } | |||
|
|
|||
| func (client *ecsClient) submitTaskStateChange(change ecs.TaskStateChange) error { | |||
|
|
|||
There was a problem hiding this comment.
nit: Unnecessary newline added here - not blocking to this pull request, but please make sure this is removed in a followup.
| @@ -519,7 +524,7 @@ func (client *ecsClient) submitTaskStateChange(change ecs.TaskStateChange) error | |||
| } | |||
|
|
|||
| _, err := client.submitStateChangeClient.SubmitTaskStateChange(&ecsmodel.SubmitTaskStateChangeInput{ | |||
There was a problem hiding this comment.
nit: Let's look into adding a unit test for STSC that has as input a TaskStateChange with ClusterARN field populated. I understand that there is a level of urgency to merging the underlying change of this pull request, so this can be done in a followup pull request if not done in this one.
Summary
This PR fixes an issue in SubmitTaskStateChange API where the clusterARN populated in change request is incorrect. For one specific platform, the clusterARN is different for different tasks.
Implementation details
Testing
New tests cover the changes:
Description for the changelog
Pass clusterARN in task state change struct.
Does this PR include breaking model changes? If so, Have you added transformation functions?
Licensing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.