-
Notifications
You must be signed in to change notification settings - Fork 11
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
Support Task Metadata Endpoint v3 #17
Conversation
Does Fargate supports task metadata endpoint v3? According to AWS doc Fargate seems to support only v2 currently, but I haven't check actual behavior yet. |
platform/ecsv3/ecs.go
Outdated
} | ||
|
||
func isRunning(status string) bool { | ||
return strings.EqualFold("running", status) |
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.
Which value does this "status" take actually?
If it's guaranteed to be exact a constant value like "running", "Running", "RUNNING", etc…, we can simply use status == "SOMEVAL"
instead of strings.EqualFold("SOMEVAL", status)
.
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.
"RUNNING" is assumed.
https://github.com/aws/amazon-ecs-agent/blob/aabe65ee15a013a6bc594da52b94d817419ee74a/agent/api/container/status/containerstatus.go#L62
I will correct that.
agent/platform.go
Outdated
@@ -29,6 +30,11 @@ func NewPlatform(ctx context.Context, ignoreContainer *regexp.Regexp) (platform. | |||
case platform.ECSAwsvpc: | |||
return ecsawsvpc.NewECSAwsvpcPlatform(false, ignoreContainer) | |||
|
|||
case platform.ECSv3: | |||
metadataURI := os.Getenv("ECS_CONTAINER_METADATA_URI") |
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.
What happens if users mistakingly specify ECSv3 at unsupported environments like old container instances (or Fargate)?
Maybe it doesn't cause disaster 💥 with current implementation, but users will be happy if they can get feedback to know the container-agent is in unsupported environments.
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.
In the current implementation, the connection error to the endpoint is output in the log and the agent terminates.
Fix to give the user appropriate feedback.
provider/types.go
Outdated
@@ -0,0 +1,11 @@ | |||
package provider |
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.
Currently provider is used only inside platform/ecsv3, therefore it seems we don't have to place top-level of container-agent.
Is there any future plan to use this provider elsewhere?
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.
It was also planned to be used on the Kubernetes platform, but it seems that just the platform package is enough.
Move to the ecsv3 package.
Co-Authored-By: hayajo <hayato.imai@gmail.com>
Thank you for the review. |
Supported Task Metadata Endpoint v3.