metadata service continued #981

adnxn · 2017-09-12T15:32:10Z

Summary

carried over work from #913 with following additions:

fix mismatch of generated mocks between metadata service and dev branch
includes dev changes up to 6f11735
updated gomock to latest version and regenerated all mocks
added code to handle agent restarts and unit test
functional testing for unix

Implementation details

n/a

Testing

Builds on Linux (make release)
Builds on Windows (go build -out amazon-ecs-agent.exe ./agent)
Unit tests on Linux (make test) pass
Unit tests on Windows (go test -timeout=25s ./agent/...) pass
Integration tests on Linux (make run-integ-tests) pass
Integration tests on Windows (.\scripts\run-integ-tests.ps1) pass
Functional tests on Linux (make run-functional-tests) pass
Functional tests on Windows (.\scripts\run-functional-tests.ps1) pass

New tests cover the changes:

Description for the changelog

Licensing

This contribution is under the terms of the Apache 2.0 License:

aaithal

First pass of review. I'll take another detailed look.

aaithal · 2017-09-14T16:04:11Z

README.md

@@ -175,10 +175,16 @@ configure them as something other than the defaults.
 | `ECS_IMAGE_MINIMUM_CLEANUP_AGE` | 30m | The minimum time interval between when an image is pulled and when it can be considered for automated image cleanup. | 1h | 1h |
 | `ECS_NUM_IMAGES_DELETE_PER_CYCLE` | 5 | The maximum number of images to delete in a single automated image cleanup cycle. If set to less than 1, the value is ignored. | 5 | 5 |
 | `ECS_INSTANCE_ATTRIBUTES` | `{"stack": "prod"}` | These attributes take effect only during initial registration. After the agent has joined an ECS cluster, use the PutAttributes API action to add additional attributes. For more information, see [Amazon ECS Container Agent Configuration](http://docs.aws.amazon.com/AmazonECS/latest/developerguide/ecs-agent-config.html) in the Amazon ECS Developer Guide.| `{}` | `{}` |
+<<<<<<< 6f11735d92bbf1ce7bbe099721a17384095fdff7


Seems like a remnant of merge resolution. please get rid of this.

aaithal · 2017-09-14T16:08:08Z

agent/config/config.go

@@ -393,6 +395,8 @@ func environmentConfig() (Config, error) {
 		CNIPluginsPath:                   cniPluginsPath,
 		AWSVPCBlockInstanceMetdata:       awsVPCBlockInstanceMetadata,
 		AWSVPCAdditionalLocalRoutes:      additionalLocalRoutes,
+		ContainerMetadataEnabled:         containerMetadataEnabled,


Can you also update DefaultConfig to set ContainerMetadataEnabled to false?

aaithal · 2017-09-14T16:09:21Z

agent/config/config.go

@@ -353,6 +353,8 @@ func environmentConfig() (Config, error) {
 			errs = append(errs, err)
 		}
 	}
+	containerMetadataEnabled := utils.ParseBool(os.Getenv("ECS_ENABLE_CONTAINER_METADATA"), false)
+	dataDirOnHost := os.Getenv("ECS_HOST_DATA_DIR")


What happens when ECS Init/whatever's starting the Agent mounts some other directory as Agent's data directory (-v /tmp:/data) sets ECS_HOST_DATA_DIR to "/var/lib/ecs"

It doesn't seem like a strong enough abstraction to be dependent on Agent configuration options to expect ECS_HOST_DATA_DIR to be the same as whatever's being mounted as /data on the host. My main concern is the late-binding/asynchronous failures this could lead to if the wrong host mount was specified. Wondering if there are better alternatives here. Can we at the very least inspect the Agent container and check if ECS_HOST_DATA_DIR is mounted? If the answer is that we'll revisit the validation here at some latter stage, that's fine too. But, I want us to be aware of potential failures here.

I think we can get rid of ECS_HOST_DATA_DIR if we directly inspect the Agent container for whichever directory is mounted as /data and pass that into the metadata methods wherever appropriate.

@aaithal hm you're right, there's a failure case here. i agree it makes sense to validate ECS_HOST_DATA_DIR is mounted by inspecting the Agent container.

however, i'm not sure if it makes more sense to fail during agent creation with a configuration error since the user will be expecting the metadata feature to be enabled, or if it would make more sense for us to surface a warning log and just disable the metadata feature.

i think failing explicitly during agent creation, newAgent(...), makes more sense since this would be a configuration error. whereas logging, disabling the feature and moving forward seems like a poor experience since the only ways for the user to find out about these errors would be to catch it in the log, or through containers failing because they expect a metadata file.

or do you have a more sensible way to communicate this error to the user?

i think failing explicitly during agent creation, newAgent(...), makes more sense since this would be a configuration error.

I agree

@aaithal so as per our discussion earlier, we will call this out in the README under usage instructions for the metadata feature.

There are a few assumptions here that I think are problematic:

Assuming that ECS_HOST_DATA_DIR is /var/lib/ecs on the host

Assuming that the ECS agent is running inside a container (it runs outside a container on Windows)

Assuming that the ECS agent container has the name ecs-agent so you can inspect the container (true on ECS-optimized AMIs and other places where ecs-init is used, but not elsewhere)

Assuming that you can get the ECS agent container ID through its hostname so you can inspect the container (won't be true if the UTS namespace is shared)

Adding to samuelkarp's second point, I believe the metadata feature does not work when the agent is run outside a container on Linux environments. Initially I was told this is a minor concern, but do we have use cases for the agent being run outside a container on Linux? For Windows, this situation is explicitly handled (As noted in the current documentation).

aaithal · 2017-09-14T16:33:27Z

agent/containermetadata/types.go

+// Network is a struct that keeps track of metadata of a network interface
+type Network struct {
+	NetworkMode string `json:"NetworkMode,omitempty"`
+	IPv4Address string `json:"IPv4Address,omitempty"`


Can you change this to IPv4Addresses instead? For now it can be a list of length 1. It'd be helpful to surface this info when we start containers with multiple IP addresses.

aaithal · 2017-09-14T16:33:57Z

agent/containermetadata/types.go

+type Network struct {
+	NetworkMode string `json:"NetworkMode,omitempty"`
+	IPv4Address string `json:"IPv4Address,omitempty"`
+	IPv6Address string `json:"IPv6Address,omitempty"`


Same as above. Can you change this to IPv6Addresses instead ([]string)?

aaithal · 2017-09-14T16:38:39Z

agent/functional_tests/testdata/taskdefinitions/mdservice-validator-unix/task-definition.json

+    "name": "mdservice-validator-unix",
+    "cpu": 10,
+    "memory": 10,
+    "command": ["sh", "-c", "sleep 1 && if cat $ECS_CONTAINER_METADATA_FILE | grep \"READY\"; then exit 42; else exit 1; fi"]


sleep 1 may not always be sufficient. Can you increase this duration to something like 10s? Or, you can check for the existence in a loop and poll on the file for a maximum of 1m.

richardpen · 2017-09-13T22:36:12Z

README.md

@@ -175,10 +175,16 @@ configure them as something other than the defaults.
 | `ECS_IMAGE_MINIMUM_CLEANUP_AGE` | 30m | The minimum time interval between when an image is pulled and when it can be considered for automated image cleanup. | 1h | 1h |
 | `ECS_NUM_IMAGES_DELETE_PER_CYCLE` | 5 | The maximum number of images to delete in a single automated image cleanup cycle. If set to less than 1, the value is ignored. | 5 | 5 |
 | `ECS_INSTANCE_ATTRIBUTES` | `{"stack": "prod"}` | These attributes take effect only during initial registration. After the agent has joined an ECS cluster, use the PutAttributes API action to add additional attributes. For more information, see [Amazon ECS Container Agent Configuration](http://docs.aws.amazon.com/AmazonECS/latest/developerguide/ecs-agent-config.html) in the Amazon ECS Developer Guide.| `{}` | `{}` |
+<<<<<<< 6f11735d92bbf1ce7bbe099721a17384095fdff7


Is this a merge conflict not fixed?

richardpen · 2017-09-13T22:37:35Z

README.md

 | `ECS_ENABLE_TASK_ENI` | `false` | Whether to enable task networking for task to be launched with its own network interface | `false` | Not applicable |
 | `ECS_CNI_PLUGINS_PATH` | `/ecs/cni` | The path where the cni binary file is located | `/amazon-ecs-cni-plugins` | Not applicable |
 | `ECS_AWSVPC_BLOCK_IMDS` | `true` | Whether to block access to [Instance Metdata](http://docs.aws.amazon.com/AWSEC2/latest/UserGuide/ec2-instance-metadata.html) for Tasks started with `awsvpc` network mode | `false` | Not applicable |
 | `ECS_AWSVPC_ADDITIONAL_LOCAL_ROUTES` | `["10.0.15.0/24"]` | In `awsvpc` network mode, traffic to these prefixes will be routed via the host bridge instead of the task ENI | `[]` | Not applicable |
+| `ECS_ENABLE_TASK_ENI` | `false` | Whether to enable task networking for task to be launched with its own network interface | `false` | `false` |


Why this appeared in this pr?

ugh it's related to your last comment, left over from merge resolution. fixed.

richardpen · 2017-09-13T22:41:56Z

agent/api/container.go

@@ -112,6 +112,10 @@ type Container struct {
 	// handled properly so that the state storage continues to work.
 	SentStatusUnsafe ContainerStatus `json:"SentStatus"`

+	// MetadataUpdated is set to true when we have completed updating the


nit: MetadataFileUpdated

richardpen · 2017-09-13T23:16:00Z

agent/containermetadata/manager.go

+// starts up
+// In the future the metadata may require multiple stages of update and these
+// statuses should amended/appended accordingly.
+type MetadataStatus string


Why not use enum type like the container/task status?

I believe this was a section I wrote and the reason I used a string rather than enum was to display the actual name of the status in the JSON Marshaling. Not quite certain but using an enum would produce numerical codes in the Marshaled output, which I think is less helpful to users checking for this status.

You can still have status names in marshaled JSON output with iotas (Go doesn't actually have enums), but you'd need to implement the TextMarshaler and TextUnmarshaler interfaces.

I would recommend doing this, as you then get stronger assurances about what will be marshaled/unmarshaled.

@adnxn can you address @samuelkarp's comment as well? Also, can this be moved to containermetadata/types.go ?

gotcha, fixed. using iotas and moved to containermetadata/types.go.

richardpen · 2017-09-13T23:27:33Z

agent/containermetadata/manager.go

+
+	"github.com/aws/amazon-ecs-agent/agent/config"
+	"github.com/aws/amazon-ecs-agent/agent/utils/ioutilwrapper"
+


please remove this blank line

richardpen · 2017-09-14T19:54:46Z

agent/containermetadata/manager_test.go

+	}
+}
+
+// TestClean is the mainline case for metadata create


Can you fix this comment, it's not clear to me.

As with the comment below, I wrote this comment intending it to mean "TestClean is the happy path case for metadata file cleanup".

Then we may want these comments to be more specific to the test, as the current comment doesn't have useful and clear meaning for the test.

richardpen · 2017-09-14T19:55:20Z

agent/containermetadata/manager_unix_test.go

+	"github.com/golang/mock/gomock"
+)
+
+// TestCreate is the mainline case for metadata create


why this comment is reused everywhere, can you fix this? Also please use assert or require for this package.

Same as above

richardpen · 2017-09-14T20:03:50Z

agent/containermetadata/utils_test.go

+	_, err := getTaskIDfromARN(mockTaskARN)
+	expectErrorMessage := fmt.Sprintf("get task ARN: cannot find TaskID for TaskARN %s", mockTaskARN)
+
+	if err.Error() != expectErrorMessage {


Please use assert instead, same for other packages.

richardpen · 2017-09-14T20:06:07Z

agent/engine/default.go

@@ -26,6 +27,6 @@ import (
 var log = logger.ForModule("TaskEngine")

 // NewTaskEngine returns a default TaskEngine
-func NewTaskEngine(cfg *config.Config, client DockerClient, credentialsManager credentials.Manager, containerChangeEventStream *eventstream.EventStream, imageManager ImageManager, state dockerstate.TaskEngineState) TaskEngine {
-	return NewDockerTaskEngine(cfg, client, credentialsManager, containerChangeEventStream, imageManager, state)
+func NewTaskEngine(cfg *config.Config, client DockerClient, credentialsManager credentials.Manager, containerChangeEventStream *eventstream.EventStream, imageManager ImageManager, state dockerstate.TaskEngineState, metadataManager containermetadata.Manager) TaskEngine {


Can you break this into multiple lines.

richardpen · 2017-09-14T20:06:28Z

agent/engine/docker_task_engine.go

 }

 // NewDockerTaskEngine returns a created, but uninitialized, DockerTaskEngine.
 // The distinction between created and initialized is that when created it may
 // be serialized/deserialized, but it will not communicate with docker until it
 // is also initialized.
-func NewDockerTaskEngine(cfg *config.Config, client DockerClient, credentialsManager credentials.Manager, containerChangeEventStream *eventstream.EventStream, imageManager ImageManager, state dockerstate.TaskEngineState) *DockerTaskEngine {
+func NewDockerTaskEngine(cfg *config.Config, client DockerClient, credentialsManager credentials.Manager, containerChangeEventStream *eventstream.EventStream, imageManager ImageManager, state dockerstate.TaskEngineState, metadataManager containermetadata.Manager) *DockerTaskEngine {


please break this into multiple lines

richardpen · 2017-09-21T21:19:43Z

agent/api/container.go

@@ -306,3 +310,17 @@ func (c *Container) IsInternal() bool {
 func (c *Container) IsRunning() bool {
 	return c.GetKnownStatus().IsRunning()
 }
+
+func (c *Container) IsMetadataFileUpdated() bool {


Lint: Please add a comment here for public methods, same for below.

richardpen · 2017-09-21T21:24:19Z

agent/config/config_windows.go

@@ -59,7 +60,9 @@ func DefaultConfig() Config {
 			netBIOSPort,
 		},
 		ReservedPortsUDP: []uint16{},
-		DataDir:          filepath.Join(ecsRoot, "data"),
+		DataDir:          dataDir,
+		// DataDirOnHost is identical to DataDir for Windows because we do not run on a container


nit: run agent as a container

richardpen · 2017-09-21T21:55:05Z

agent/containermetadata/write_metadata_windows.go

+	if err != nil {
+		// Retry if file does not exist
+		if osWrap.IsNotExist(err) {
+			file, err = osWrap.Create(metadataFileName)


Do we need to set the file permission after creating?

looking at this again, we can remove the retry block with osWrap.IsNotExist(err) {... by changing the open file line to file, err := osWrap.OpenFile(metadataFileName, os.O_WRONLY | os.O_CREATE, metadataPerm).

actually nvm. from godocs,

"On Windows, the mode must be non-zero but otherwise only the 0200 bit (owner writable) of mode is used; it controls whether the file's read-only attribute is set or cleared. attribute. The other bits are currently unused. Use mode 0400 for a read-only file and 0600 for a readable+writable file."

so we might not be able to have the exact same permission as we do on linux. i'll follow up with someone on the team that knows more about windows file permissions.

richardpen · 2017-09-21T22:05:33Z

agent/functional_tests/tests/functionaltests_unix_test.go

@@ -636,3 +636,27 @@ func fluentdDriverTest(taskDefinition string, t *testing.T) {
 	err = SearchStrInDir(fluentdLogPath, "ecsfts", logTag)
 	assert.NoError(t, err, "failed to find the log tag specified in the task definition")
 }
+
+// TestMetadataServiceValidator Tests that the metadata file can be accessed from the


Can we also add this test for windows?

richardpen · 2017-09-21T22:06:39Z

agent/statemanager/state_manager.go

@@ -48,6 +48,7 @@ const (
 	//   b) Add 'ContainerResourcesProvisioned' as a new 'ContainerStatus' enum
 	//   c) Add 'SteadyStateStatus' field to 'Container' struct
 	//   d) Add 'ENIAttachments' struct
+	//	 e) Add 'MetadataUpdated' field to 'api.Container'


nit: this should be space instead of tab.

aaithal · 2017-09-22T16:38:10Z

agent/containermetadata/manager.go

+// statuses should amended/appended accordingly.
+type MetadataStatus string
+
+const (


This is more of a code structure thing. I like to see consts at the top of the file (you'll find that to be the case in rest of this code base as well). Can you move this block to the top of the file?

aaithal · 2017-09-22T16:44:23Z

agent/engine/docker_task_engine.go

+	if engine.cfg.ContainerMetadataEnabled {
+		err := engine.metadataManager.Clean(task.Arn)
+		if err != nil {
+			seelog.Errorf("Clean task metadata failed for task %s: %v", task, err)


Can you please change this to a Warn? I'd prefer Error to be used if it correlates with a customer facing error, which this is not (Applies to the rest of the file(s) as well).

aaithal · 2017-10-04T17:03:33Z

@adnxn an you investigate why appveyor is failing and resolve merge conflicts as well?

aaithal · 2017-10-04T17:06:55Z

agent/containermetadata/manager.go

+// starts up
+// In the future the metadata may require multiple stages of update and these
+// statuses should amended/appended accordingly.
+type MetadataStatus string


@adnxn can you address @samuelkarp's comment as well? Also, can this be moved to containermetadata/types.go ?

aaithal · 2017-10-04T17:10:02Z

agent/containermetadata/manager.go

+	}
+
+	// Acquire the metadata then write it in JSON format to the file
+	metadata := manager.parseMetadataAtContainerCreate(taskARN, containerName)


The logic of parsing, marshaling and writing the metadata file is duplicated between this and the Update method. Please extract that to a method of its own.

aaithal · 2017-10-04T17:14:09Z

agent/containermetadata/utils.go

+// containing the task-id which we extract by splitting it by "/".
+// This function should be eventually be replaced by a standardized ARN parsing
+// module
+func getTaskIDfromARN(taskARN string) (string, error) {


We should be able to get rid of this if we use the updated SDK and the arn.Parse() method from aws/aws-sdk-go@1fb10cf

fixed. updating sdk here #1006 and added the aws/arn package to this set of changes.

not including this change since godep is doing this.

aaithal · 2017-10-04T17:14:59Z

agent/containermetadata/write_metadata_unix.go

+	}
+	metadataFileName := filepath.Join(metadataFileDir, metadataFile)
+
+	temp, err := ioutilWrap.TempFile(metadataFileDir, "temp_metadata_file")


Can you const "temp_metadata_file"?

aaithal · 2017-10-04T17:16:29Z

agent/engine/docker_task_engine.go

+	if engine.cfg.ContainerMetadataEnabled {
+		err := engine.metadataManager.Clean(task.Arn)
+		if err != nil {
+			seelog.Warnf("Clean task metadata failed for task %s: %v", task, err)


This should be task.Arn. Also, can you add unit tests for sweepTask and exercise this code path?

fixed the task.Arn thing. and the existing happy path test exercises that code path.

@huerdong

all @huerdong commits squashed for manageable commit history in PR

This commit is fix gomock version mismatch and regenerate all mocks to fix breaking builds with metadata service changes

This change adds logic in the agent to update the metadata file after the agent restarts and the related unit test. Note that we added a MetadataFileUpdated flag in the api.Container struct and this is included with state file's ECSDataVersion 6 bump.

aaithal · 2017-10-06T23:24:07Z

agent/containermetadata/parse_metadata.go

+
+	// Get Port bindings from docker configurations
+	var ports []api.PortBinding
+	var err error


minor: if there's no specific reason to declare this here, can you get rid of it?

im going to leave this one as it is, the nested branch needs both, and we return ports even if its empty.

aaithal · 2017-10-06T23:25:38Z

agent/containermetadata/types.go

+}
+
+func (status *MetadataStatus) UnmarshalText(text []byte) error {
+	var err error


You can get rid of this line and return nil at the end of the method.

oh i see why, fixed.

aaithal · 2017-10-06T23:26:13Z

agent/containermetadata/types.go

+// statuses should amended/appended accordingly.
+type MetadataStatus int32
+
+func (status MetadataStatus) MarshalText() (text []byte, err error) {


We have unit test for this and the unmarshal method?

added tests for this.

aaithal · 2017-10-06T23:28:25Z

agent/engine/docker_task_engine.go

+		go func() {
+			err := engine.metadataManager.Update(dockerContainer.DockerID, task.Arn, container.Name)
+			if err != nil {
+				seelog.Warnf("Update metadata file failed for container %s of task %s: %v", container, task, err)


minor: you can return here and get rid of else.

aaithal · 2017-10-06T23:29:02Z

agent/engine/docker_task_engine.go

+				seelog.Warnf("Update metadata file failed for container %s of task %s: %v", container, task, err)
+			} else {
+				container.SetMetadataFileUpdated()
+				seelog.Debugf("Updated metadata file for container %s of task %s", container, task)


Here and in the rest of the file, let's log less: task.Arn, container.Name

gotcha. cleaned up instances of this in the file.

this change also includes minor changes to addresses cr

aaithal

mostly looks good. I have some minor comments.

aaithal · 2017-10-09T18:16:52Z

agent/statemanager/state_manager.go

@@ -49,6 +49,7 @@ const (
 	//   c) Add 'SteadyStateStatus' field to 'Container' struct
 	//   d) Add 'ENIAttachments' struct
 	//   e) Deprecate 'SteadyStateDependencies' in favor of 'TransitionDependencySet'
+	//	 f) Add 'MetadataUpdated' field to 'api.Container'
 	ECSDataVersion = 6


This should be 7 now.

right, fixed.

aaithal · 2017-10-09T18:19:16Z

agent/containermetadata/manager.go

+)
+
+const (
+	metadataEnvironmentVariable = "ECS_CONTAINER_METADATA_FILE"


Please add documentation for this variable stating that this will be set in the container's host config and this is what customers would use to locate/access metadata.

added documentation for ECS_CONTAINER_METADATA_FILE in the readme, also we should discuss if we need to include more detailed documentation wrt format of the file and the specific fields.

not including this change in favor of separate docs pr

Why not? You can still document what this variable does // metadataEnvironmentVariable is ...

i didn't realise you meant in the code earlier. fixed.

didnt realise you meant document in the code earlier. fixed now.

aaithal · 2017-10-09T18:20:33Z

agent/containermetadata/manager.go

+
+	// Acquire the metadata then write it in JSON format to the file
+	metadata := manager.parseMetadata(dockerContainer, taskARN, containerName)
+	err = manager.marshalAndWrite(metadata, taskARN, containerName)


nit: if you aren't doing anything with the error returned, you can just do return manager.marshalAndWrite(metadata, taskARN, containerName) here.

makes sense, fixed.

aaithal · 2017-10-09T18:21:50Z

agent/containermetadata/manager.go

+func (manager *metadataManager) Clean(taskARN string) error {
+	metadataPath, err := getTaskMetadataDir(taskARN, manager.dataDir)
+	if err != nil {
+		return fmt.Errorf("clean task %s: %v", taskARN, err)


please add more context here: "clean task metadata: unable to get metadata directory for task %s: %v"

aaithal · 2017-10-09T18:23:22Z

agent/containermetadata/manager.go

+func (manager *metadataManager) marshalAndWrite(metadata Metadata, taskARN string, containerName string) error {
+	data, err := json.MarshalIndent(metadata, "", "\t")
+	if err != nil {
+		return fmt.Errorf("create metadata for task %s container %s: %v", taskARN, containerName, err)


same here, needs more context: "create metadata for container %s in task %s: failed to marshal metadata"

aaithal · 2017-10-09T18:23:47Z

agent/containermetadata/manager.go

+	}
+
+	// Write the metadata to file
+	err = writeToMetadataFile(manager.osWrap, manager.ioutilWrap, data, taskARN, containerName, manager.dataDir)


here too, you could just do return writeMetadata...

makes sense, fixed.

unit tests no manager_test.go no longer depend on error message format.

aaithal · 2017-10-09T23:18:39Z

README.md

@@ -181,6 +181,9 @@ configure them as something other than the defaults.
 | `ECS_AWSVPC_ADDITIONAL_LOCAL_ROUTES` | `["10.0.15.0/24"]` | In `awsvpc` network mode, traffic to these prefixes will be routed via the host bridge instead of the task ENI | `[]` | Not applicable |
 | `ECS_ENABLE_CONTAINER_METADATA` | `<true|false>` | Whether to enable creation of a metadata file in the containers. If enabled the agent will inject a file with the container's various metadata and the user can access the container's metadata internally by accessing the environment variable $ECS_CONTAINER_METADATA_FILE. | `false` | `false` |
 | `ECS_HOST_DATA_DIR` | `/var/lib/ecs` | The source directory on the host from which ECS_DATADIR is mounted. We use this to determine the source mount path for container metadata files in the case the Agent is run in a container. We do not use this value in Windows because the Agent is not run on a container in Windows. | `/var/lib/ecs` | `Not used` |
+| `ECS_CONTAINER_METADATA_FILE` | set by the agent | This is variable is set in


I don't think we need to document this in README.md. The variables we have here are the Agent config settings used to tune the Agent. It may make sense to add an example at the end of the readme or something demonstrating how to use the metadata feature.

okay, i'll add a separate section for this altogether.

not including this change in favor of separate docs pr

richardpen

LGTM except minor comments

richardpen · 2017-10-12T19:48:51Z

README.md

@@ -179,6 +179,8 @@ configure them as something other than the defaults.
 | `ECS_CNI_PLUGINS_PATH` | `/ecs/cni` | The path where the cni binary file is located | `/amazon-ecs-cni-plugins` | Not applicable |
 | `ECS_AWSVPC_BLOCK_IMDS` | `true` | Whether to block access to [Instance Metdata](http://docs.aws.amazon.com/AWSEC2/latest/UserGuide/ec2-instance-metadata.html) for Tasks started with `awsvpc` network mode | `false` | Not applicable |
 | `ECS_AWSVPC_ADDITIONAL_LOCAL_ROUTES` | `["10.0.15.0/24"]` | In `awsvpc` network mode, traffic to these prefixes will be routed via the host bridge instead of the task ENI | `[]` | Not applicable |
+| `ECS_ENABLE_CONTAINER_METADATA` | `true` | When `true`, the agent will create a file describing the container's metadata and the file can be located and consumed by using the container enviornment variable `$ECS_CONTAINER_METADATA_FILE` | `false` | `false` |
+| `ECS_HOST_DATA_DIR` | `/var/lib/ecs` | The source directory on the host from which ECS_DATADIR is mounted. We use this to determine the source mount path for container metadata files in the case the Agent is run in a container. We do not use this value in Windows because the Agent is not run on a container in Windows. | `/var/lib/ecs` | `Not used` |


nit: in the case the ECS Agent is running as a container xxxxx because the ECS Agent is not running as a container

richardpen · 2017-10-12T19:49:30Z

agent/api/container.go

@@ -317,3 +321,20 @@ func (c *Container) IsInternal() bool {
 func (c *Container) IsRunning() bool {
 	return c.GetKnownStatus().IsRunning()
 }
+
+// IsMetadataFileUpdated() returns true if the metadata file has been once the


can you get rid of the parenthesis in the comment?

vsiddharth

LGTM 📦 🚀

adnxn force-pushed the mdservice-dev branch 3 times, most recently from 2af95e7 to 2feb1fa Compare September 13, 2017 19:47

adnxn changed the title ~~[WIP] metadata service continued~~ metadata service continued Sep 13, 2017

adnxn requested review from aaithal and richardpen September 13, 2017 20:07

adnxn force-pushed the mdservice-dev branch 2 times, most recently from 7ac27ba to 9383f5f Compare September 13, 2017 23:03

aaithal reviewed Sep 14, 2017

View reviewed changes

richardpen reviewed Sep 14, 2017

View reviewed changes

adnxn force-pushed the mdservice-dev branch 4 times, most recently from 558b98a to ff162ba Compare September 15, 2017 21:53

aaithal mentioned this pull request Sep 18, 2017

[WIP] Container Metadata #913

Closed

8 tasks

richardpen reviewed Sep 21, 2017

View reviewed changes

aaithal reviewed Sep 22, 2017

View reviewed changes

aaithal reviewed Oct 4, 2017

View reviewed changes

huerdong and others added 11 commits October 4, 2017 13:43

mdservice: initial commit

6e9aadd

all @huerdong commits squashed for manageable commit history in PR

engine: fixed method call

ed6aaef

agent: regenerate mocks

203ec87

This commit is fix gomock version mismatch and regenerate all mocks to fix breaking builds with metadata service changes

vendor: updating gomock

d04bb67

agent: ran 'make gogenerate'

14274b3

engine: modify tests to include metadata service

9d5d867

agent/functional_tests: added unix functional test

b2ee17c

engine: remove duplicate function

a152504

containermetadata: handle multiple IP addresses

cdf61a5

containermetadata: use asserts for tests

b06c3f3

adnxn added 4 commits October 4, 2017 13:48

functional_tests: added windows functional test

9e0d80f

agent: downgrade gomock to resolve merge conflicts

4640038

containermetadata: extract duplicated parsing code

508d9e5

containermetadata: use iotas for status

de5fa33

adnxn force-pushed the mdservice-dev branch from b208d50 to de5fa33 Compare October 6, 2017 19:09

engine: fix windows test to include metedata mock

d5146c5

aaithal reviewed Oct 6, 2017

View reviewed changes

adnxn added 2 commits October 9, 2017 09:06

engine: reduce logging verbosity

13fb758

this change also includes minor changes to addresses cr

containermetadata: add unit tests for un/marshal

68fa765

aaithal reviewed Oct 9, 2017

View reviewed changes

adnxn added 2 commits October 9, 2017 12:26

statemanger: version bump to 7

b4f68c6

containermetadata: refactored unit tests

e9fd7cf

unit tests no manager_test.go no longer depend on error message format.

aaithal reviewed Oct 9, 2017

View reviewed changes

adnxn force-pushed the mdservice-dev branch 3 times, most recently from ee953a1 to 347977d Compare October 11, 2017 19:51

aaithal approved these changes Oct 11, 2017

View reviewed changes

richardpen approved these changes Oct 12, 2017

View reviewed changes

readme: reword metadata option description

6817a23

adnxn force-pushed the mdservice-dev branch from 347977d to 6817a23 Compare October 12, 2017 22:16

vsiddharth approved these changes Oct 12, 2017

View reviewed changes

adnxn merged commit 6817a23 into aws:dev Oct 12, 2017

jhaynes mentioned this pull request Oct 31, 2017

V1.15.0 release #1018

Merged

jtoberon mentioned this pull request Nov 1, 2017

Provide metadata to containers #710

Closed

adnxn deleted the mdservice-dev branch March 14, 2018 17:46


		"github.com/aws/amazon-ecs-agent/agent/config"
		"github.com/aws/amazon-ecs-agent/agent/utils/ioutilwrapper"

metadata service continued #981

metadata service continued #981

Conversation

adnxn commented Sep 12, 2017 • edited Loading

Summary

Implementation details

Testing

Description for the changelog

Licensing

aaithal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaithal Sep 14, 2017 • edited Loading

Choose a reason for hiding this comment

huerdong Sep 14, 2017 • edited Loading

Choose a reason for hiding this comment

adnxn Sep 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huerdong Sep 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huerdong Sep 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adnxn Sep 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaithal commented Oct 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adnxn Oct 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adnxn Oct 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adnxn commented Sep 12, 2017 •

edited

Loading

aaithal Sep 14, 2017 •

edited

Loading

huerdong Sep 14, 2017 •

edited

Loading

adnxn Sep 14, 2017 •

edited

Loading

huerdong Sep 18, 2017 •

edited

Loading

huerdong Sep 14, 2017 •

edited

Loading

adnxn Sep 22, 2017 •

edited

Loading

adnxn Oct 5, 2017 •

edited

Loading

adnxn Oct 5, 2017 •

edited

Loading

adnxn Oct 9, 2017 •

edited

Loading