-
Notifications
You must be signed in to change notification settings - Fork 619
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
Conversation
6d1fd7b
to
37fb05b
Compare
8c59d8d
to
b38e6ce
Compare
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.
LGTM
NetworkNamespace string `json:"NetworkNamespace,omitempty"` | ||
|
||
// TODO: Will need to initialize/set the value in a follow PR | ||
FaultInjectionEnabled bool `json:"FaultInjectionEnabled,omitempty"` | ||
|
||
DefaultIfname string `json:"DefaultIfname,omitempty"` |
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
b38e6ce
to
4656626
Compare
4656626
to
1cb8269
Compare
NetworkNamespace string `json:"NetworkNamespace,omitempty"` | ||
|
||
// TODO: Will need to initialize/set the value in a follow PR | ||
FaultInjectionEnabled bool `json:"FaultInjectionEnabled,omitempty"` | ||
|
||
DefaultIfname string `json:"DefaultIfname,omitempty"` |
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
?
1cb8269
to
0333094
Compare
@@ -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 comment
The 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 comment
The 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 eth0
? Are there any assumptions of host interface name?
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.
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 comment
The 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 eth0? Are there any assumptions of host interface name?
Yep, the name eth0
is the default ENI name on linux and it's set here .
From what I can tell, seems like we might be making an assumption for eth0
? -> https://github.com/search?q=repo%3Aaws%2Famazon-ecs-agent%20eth0&type=code
As for host mode, no there is not and we can't make this assumption as it can vary on platforms and hardware
NetworkNamespace string `json:"NetworkNamespace,omitempty"` | ||
|
||
// TODO: Will need to initialize/set the value in a follow PR | ||
FaultInjectionEnabled bool `json:"FaultInjectionEnabled,omitempty"` | ||
|
||
DefaultIfname string `json:"DefaultIfname,omitempty"` |
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?
NetworkNamespace string `json:"NetworkNamespace,omitempty"` | ||
|
||
// TODO: Will need to initialize/set the value in a follow PR | ||
FaultInjectionEnabled bool `json:"FaultInjectionEnabled,omitempty"` | ||
|
||
DefaultIfname string `json:"DefaultIfname,omitempty"` |
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
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?
This will be done a near future PR. Currently, agent doesn't know the default host network interface.
We can also add a comment for expected value for Bridge mode
It isn't being obtained for the rest of the network modes like bridge in these changes and we don't have plans currently to do so.
0333094
to
5480fb4
Compare
5480fb4
to
a14e211
Compare
a14e211
to
b7f8041
Compare
b7f8041
to
e3bfef2
Compare
e3bfef2
to
3383129
Compare
…network namespace
3383129
to
ebad60f
Compare
Summary
This PR will update the agent state so that it keeps track of the task network namespace path and default interface name of the corresponding task namespace. This is a follow up to the following PRs:
where we will be populating the new
TaskNetworkConfig
struct and its fields for theTaskResponse
.Note: There will be a follow up PR to actually obtain and populate the
DefaultIfname
on host mode.Implementation details
agent/api/task/task.go
DefaultIfname
field to theTask
struct to keep track of the default interface name of the taskNetworkNamespace
andDefaultIfname
agent/engine/docker_task_engine.go
buildCNIConfigFromTaskContainerAwsvpc
and then setting it as the value for the taskNetworkNamespace
NetworkConfigs
of the CNI config that we get frombuildCNIConfigFromTaskContainerAwsvpc
. By default on linux, the interface name iseth0
and the first entry within theNetworkConfigs
list should correspond to the task ENI that was passed in from the task payload. Otherwise, this would mean we're trying to start a task in AWSVPC without a corresponding task ENIagent/handlers/v4/tmdsstate.go
FaultInjectionEnabled
andTaskNetworkConfig
for theTaskResponse
based on the agent state (before, it was empty)FaultInjectionEnabled
or not for future proofinghost
so that it can be used in the concurrent map introduced in Add read/write lock in the fault handler #4309 for the fault handlers It won't be used in the actual fault injection implementationecs-agent/tmds/handlers/v4/state/response.go
NewTaskNetworkConfig
pointer object based on the passed innetworkMode
,networkNamespacePath
, andinterfaceName
. This is called inagent/handlers/v4/tmdsstate.go
Testing
TestV4GetTaskMetadataWithTaskNetworkConfig
inagent/handlers/task_server_setup_test.go
to test whether or not we can create/populate theTaskNetworkConfig
of theTaskResponse
based on the corresponding taskTestProvisionContainerResourcesAwsvpcSetPausePIDInVolumeResources
inagent/engine/docker_task_engine_test.go
so that it can also check that theDefaultIfname
andNetworkNamespace
of the task is being set properlyManual Testing
Tested whether or not the
DefaultIfname
andNetworkNamespace
can be loaded back after an agent restart by first launching a task in AWSVPC mode that will call the TMDS endpoint to get the task metadata.Note: Additional logging statements were added but not pushed as part of the changes in order to better see the expected behavior/results.
Results:
Task start up
After restart agent
Host Mode
After restart
New tests cover the changes: Yes
Description for the changelog
Feature: Update agent state to initialize and populate task default interface name and network namespace
Additional Information
Does this PR include breaking model changes? If so, Have you added transformation functions?
Does this PR include the addition of new environment variables in the README?
Licensing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.