-
Notifications
You must be signed in to change notification settings - Fork 617
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
Update agentstate to include task network namespace and default interface name to populate task network config #4315
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2358,6 +2358,12 @@ func (engine *DockerTaskEngine) provisionContainerResourcesAwsvpc(task *apitask. | |
field.TaskID: task.GetID(), | ||
"ip": taskIP, | ||
}) | ||
task.SetNetworkNamespace(cniConfig.ContainerNetNS) | ||
// Note: By default, the interface name is set to eth0 within the CNI configs. We can also always assume that the first entry of the CNI network config to be | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this ordering true? Or is it always config for task ENI? Can there be other Network configs returned in the slice here other than the config for task ENI? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the name eth0 assigned by Agent? Do we make any assumption on the string being There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From what I understand, there can only be one ENI that will be used for a task in AWSVPC (and it seems to be true for trunk ENI as well). For reference, this is what I'm referring to on why we can make this assumption -> https://github.com/aws/amazon-ecs-agent/blob/master/agent/api/task/task_linux.go#L285 It does seem like there are other network configs that will be appended/added but the primary task ENI/network config is what's being processed first. FWIW, we're also sort of already making an assumption that the very first entry of the list of task ENI will be the primary network interface here -> https://github.com/aws/amazon-ecs-agent/blob/master/agent/api/task/task.go#L2826 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yep, the name From what I can tell, seems like we might be making an assumption for As for host mode, no there is not and we can't make this assumption as it can vary on platforms and hardware |
||
// the task ENI. Otherwise this means that there weren't any task ENIs passed down to agent from the task payload. | ||
if len(cniConfig.NetworkConfigs) > 0 { | ||
task.SetDefaultIfname(cniConfig.NetworkConfigs[0].IfName) | ||
} | ||
engine.state.AddTaskIPAddress(taskIP, task.Arn) | ||
task.SetLocalIPAddress(taskIP) | ||
engine.saveTaskData(task) | ||
|
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.
nit: would be good to have a comment about what the
defaultIfname
means in different mode.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.
good idea, thanks!
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 can eth0
->it can be eth0
?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.
Where do we populate DefaultIfname for Host mode?
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.
We can also add a comment for expected value for Bridge mode