Skip to content
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

[WIP] Container Metadata #913

Closed
wants to merge 106 commits into from
Closed

[WIP] Container Metadata #913

wants to merge 106 commits into from

Conversation

huerdong
Copy link
Contributor

@huerdong huerdong commented Aug 4, 2017

Summary

Adds the Container Metadata feature to the agent, exposing the metadata from ECS and from Docker to agent managed containers, such as the container's DockerID and the Container Instance ARN, and then exposes this metadata for users to access from within the container.

Implementation details

See #848

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: Yes

Description for the changelog

Feature - Add metadata service for ECS-created containers.

Licensing

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

@sharanyad
Copy link
Contributor

Shouldn't this be merged into dev instead of master?

@vsiddharth
Copy link
Contributor

@huerdong please change the base branch

@huerdong huerdong closed this Aug 4, 2017
@huerdong
Copy link
Contributor Author

huerdong commented Aug 4, 2017

I fixed the branch problem.

@huerdong huerdong reopened this Aug 4, 2017
@samuelkarp
Copy link
Contributor

You'll need to change the base branch before we can review this. Right now, this pull request shows commits that are already merged into the dev branch from different authors; it's not practical to review that mix of commits.

@huerdong huerdong mentioned this pull request Aug 4, 2017
8 tasks
@huerdong huerdong changed the base branch from master to dev August 4, 2017 18:22
@huerdong huerdong changed the title Container Metadata (Duplicate) Container Metadata Aug 4, 2017
@vsiddharth
Copy link
Contributor

@huerdong Please update the summary block and check the appropriate boxes.
Please double check the license attribution too.

Thanks


err = os.MkdirAll(metadataDirectoryPath, os.ModePerm)
if err != nil {
return err
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add more context for this error. At the very least something like fmt.Errorf("creating metadata directory: %v", err).

// Create metadata file
err = createMetadataFile(metadataDirectoryPath)
if err != nil {
return err
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above. This error needs context. You can even use errors.Wrapf() from https://github.com/pkg/errors

// configuration and data then packages it for JSON Marshaling
// Since we accept incomplete metadata fields, we should not return
// errors here and handle them at this or the above stage.
func (manager *metadataManager) parseMetadata(dockerContainer *docker.Container, task *api.Task, container *api.Container) Metadata {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like all you need here are task's arn and container's name. Can you just pass those instead of task and container?

// Since we accept incomplete metadata fields, we should not return
// errors here and handle them at this or the above stage.
func (manager *metadataManager) parseMetadata(dockerContainer *docker.Container, task *api.Task, container *api.Container) Metadata {
taskMD := parseTaskMetadata(task, container)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Along the same lines, I don't see the value of creating a function which just initializes a struct. It would be lot helpful to anyone who reads the code to just do this instead:
return Metadata { taskMetadata: TaskMetadata{...} ...}

type Network struct {
NetworkMode string `json:"NetworkMode,omitempty"`
IPv4Address string `json:"IPv4Address,omitempty"`
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add the IPv6Address field here as well with omitempty. You don't need to do anything for it right now. But, it'll be useful when "Task ENIs" is enabled.

// files of a given task
func getTaskMetadataDir(task *api.Task, dataDir string) (string, error) {
taskID, err := getTaskIDfromARN(task.Arn)
return filepath.Join(dataDir, metadataJoinSuffix, taskID), err
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to deal with err here.

return err
}
defer temp.Close()
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is redundant


const (
metadataJoinSuffix = "metadata"
metadataFile = "metadata.json"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about "ecs-container-metadata.json" ?

if engine.cfg.ContainerMetadataEnabled {
err := engine.metadataManager.CleanTaskMetadata(task)
if err != nil {
seelog.Errorf("clean task metadata failed for task %s: %v", task, err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logs need to start with Upper case letters.

Copy link
Contributor

@vsiddharth vsiddharth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look much better than before

README.md Outdated
@@ -175,6 +175,8 @@ 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.| `{}` | `{}` |
| `ECS_ENABLE_CONTAINER_METADATA` | `<true|false>` | Whether to enable creation of a metadata file in the container | `false` | `false` |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update this line with more information

README.md Outdated
@@ -175,6 +175,8 @@ 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.| `{}` | `{}` |
| `ECS_ENABLE_CONTAINER_METADATA` | `<true|false>` | Whether to enable creation of a metadata file in the container | `false` | `false` |
| `ECS_HOST_DATA_DIR` | `/var/lib/ecs` | The source directory on the host from which ECS_DATADIR is mounted | `/var/lib/ecs` | `Not used` |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update this line too

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add information on why this is not used for Windows?


// metadataManager implements the MetadataManager interface
type metadataManager struct {
client dockerMetadataClient
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to see some comments here for each attribute

cluster: manager.cluster,
taskMetadata: taskMD,
containerInstanceARN: manager.containerInstanceARN,
metadataStatus: "NOT_READY",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we have a new type for metadata status ?
I'd like to have it be one amongst a set of well defined values

if err != nil {
return fmt.Errorf("write to metadata file for task %s container %s: %v", task, container, err)
}
metadataFilePath := filepath.Join(metadataFileDir, metadataFile)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be duplicated code from the createMetadataFile method

ContainerMetadataClientVersion = dockerclient.Version_1_24
)

// createMetadataFile initializes the metadata file
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update this comment

func createMetadataFile(metadataDirectoryPath string) error {
metadataFilePath := filepath.Join(metadataDirectoryPath, metadataFile)
file, err := os.Create(metadataFilePath)
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why didn't you go with the "Tempfile" approach here ?

@@ -108,6 +110,26 @@ func newAgent(
return nil, err
}

var metadataManager containermetadata.MetadataManager
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

containermetadata.Manager stutters less than containermetadata.MetadataManager

Copy link
Contributor Author

@huerdong huerdong Aug 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand your purpose but I am reluctant to make this change because there are so many structs in the agent with the word Manager in them. Having a single struct called Manager, even if preceded by package containermetadata, feels confusing to me.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine. As long as variables are named correctly, it shouldn't matter what the struct name is.

@@ -108,6 +110,26 @@ func newAgent(
return nil, err
}

var metadataManager containermetadata.MetadataManager
if cfg.ContainerMetadataEnabled {
// Get the oldest Docker Client version with up to date inspect API
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move this to a method. Also, this needs a test in agent/app/agent_test.go

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now I am cautious about making changes here because I have encountered in the past import cycles and/or "interface is not implemented" errors when dealing with this section of code. I will see what I can do here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how moving this to a method would result in import cycles. On the other hand, moving this to a method improves readability and makes it more testable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean moving it into a method but still within the app package? Maybe I misconstrued your request.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean moving it into a method but still within the app package?

Yeah.

@@ -315,6 +315,9 @@ func environmentConfig() (Config, error) {
seelog.Debugf("Setting instance attribute %v: %v", attributeKey, attributeValue)
}

containerMetadataEnabled := utils.ParseBool(os.Getenv("ECS_ENABLE_CONTAINER_METADATA"), false)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs a test

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this and the "needs a test" comments below for the config package, I have added tests to check for my new configuration variables.

@@ -28,6 +28,7 @@ func DefaultConfig() Config {
ReservedPorts: []uint16{SSHPort, DockerReservedPort, DockerReservedSSLPort, AgentIntrospectionPort, AgentCredentialsPort},
ReservedPortsUDP: []uint16{},
DataDir: "/data/",
DataDirOnHost: "/var/lib/ecs",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs a test

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see above

@@ -44,6 +44,7 @@ const (
func DefaultConfig() Config {
programData := utils.DefaultIfBlank(os.Getenv("ProgramData"), `C:\ProgramData`)
ecsRoot := filepath.Join(programData, "Amazon", "ECS")
dataDir := filepath.Join(ecsRoot, "data")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs a test

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see above


// MetadataSerializer is an intermediate struct that converts the information
// in Metadata into information to encode into JSOn
type MetadataSerializer struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this not be global scoped (metadataSerializer )?

// getTaskMetadataDir acquires the directory with all of the metadata
// files of a given task
func getTaskMetadataDir(task *api.Task, dataDir string) (string, error) {
taskID, err := getTaskIDfromARN(task.Arn)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need handle this error (points to a lack of unit test).

// operations
type MetadataManager interface {
SetContainerInstanceARN(string)
CreateMetadata(*docker.Config, *docker.HostConfig, *api.Task, *api.Container) error
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can name this as Create instead.

type MetadataManager interface {
SetContainerInstanceARN(string)
CreateMetadata(*docker.Config, *docker.HostConfig, *api.Task, *api.Container) error
UpdateMetadata(string, *api.Task, *api.Container) error
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can name this as Update instead.

SetContainerInstanceARN(string)
CreateMetadata(*docker.Config, *docker.HostConfig, *api.Task, *api.Container) error
UpdateMetadata(string, *api.Task, *api.Container) error
CleanTaskMetadata(*api.Task) error
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can name this as Clean instead.

@aaithal
Copy link
Contributor

aaithal commented Aug 7, 2017

@huerdong can you also paste the output of go test -cover ./agent/containermetadata? You may need to make some modifications with the command. I'm mostly looking for test coverage data.

@vsiddharth
Copy link
Contributor

@huerdong Could you also resolve the conflicts when you update #913 ?

// Get the oldest Docker Client version with up to date inspect API
// as some metadata is unavailable in older API client versions
// If this version is unavailable we use default
// Due to limitations of interfaces and import cycles
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think your only import cycle issue is with the containermetadata.ContainerMetadataClientVersion. I'd be in favor of moving this into the client/factory code directly and just moving the constant over there as well so you don't have an import cycle; that'll keep the impact localized to just the code that needs to be refactored anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not quite sure what you mean here. The problem I am encountering is that adding the WithVersion function to the internal dockerMetadataClient interface in the containermetadata package led to "interface not implemented" errors in the past. I am very uncertain about this section of code as a result, but I will try to see if I can minimize the exposure in this section.

if cfg.ContainerMetadataEnabled {
// Get the oldest Docker Client version with up to date inspect API
// as some metadata is unavailable in older API client versions
// If this version is unavailable we use default
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be reasonable to just disable the feature if Docker is too old.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The feature still works for older versions of docker. The differences in the versions do not actually matter previously because I get the relevant networking metadata through an alternative route for older versions. With the ENI feature added, this difference is significant because older versions of Docker API won't display the additional network modes. Again, I am not very familiar with ENI project except that I was asked to make the Network information into a list to support multiple network modes. I would think if a user wants to use this feature even with an older version of Docker, we should not disable it if it works for that older client.

// configuration and data then packages it for JSON Marshaling
// Since we accept incomplete metadata fields, we should not return
// errors here and handle them at this or the above stage.
func (manager *metadataManager) parseMetadata(dockerContainer *docker.Container, taskARN string, containerName string) Metadata {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally, I'd like to see the functions in a file organized with caller-above-callee so that you start with abstractions at a high level and get deeper into details as you read further in the file. That would mean that the entrypoints (parseMetadata and parseMetadataAtContainerCreate) are at the top, then parseDockerContainerMetadata, and last parseNetworkMetadata.

Copy link
Contributor Author

@huerdong huerdong Aug 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have made this reorganization.


file, err := os.OpenFile(metadataFileName, os.O_WRONLY, 644)
if err != nil {
// Retry if file does not exist
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's worth checking what this err is before blindly retrying.

dockerContainerMD := client.StartContainer(dockerContainer.DockerID, startContainerTimeout)

// Get metadata through container inspection and available task information then write this to the metadata file
// Performs this in the background to avoid delaying container start
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It almost feels like this should be a new container state. As is, if the agent restarts while this goroutine is running (after the container has started and we've recorded that in the state file, but before we've completed updating the metadata), you may never get metadata properly updated (startContainer won't be called again).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I previously discussed making this a separate container state with @aaithal. Given the timeline of my ability to work on this project, I do not think I can feasibly make this change right now.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could definite happen in the agent, to make it simple than adding a new state is to add a new state in the container struct to indicate the metadata state.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added a TODO here for adding a metadata file status in api.Container and for doing metadata updates for containers running after agent restart.

@@ -0,0 +1,65 @@
// Copyright 2017 Amazon.com, Inc. or its affiliates. All Rights Reserved.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this file named 'write_metadata.go'? This does not really write metadata as such.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of my writing operations was originally here and has since been moved to other places. It is probably better to name it utils.go

@sharanyad
Copy link
Contributor

Having some unit tests for metadata manager would be good.

@petderek
Copy link
Contributor

petderek commented Aug 8, 2017

Why do we need INITIAL, READY, NOT_READY, etc? As a user, do I care about the state of the metadata?

@aaithal
Copy link
Contributor

aaithal commented Aug 8, 2017

@petderek it'd have been better if you had just added to the threaded discussion than a comment here. Regardless, as a user, I'd care as it'd help me distinguish between "will i ever get this data" and "i'll eventually get this data if i keep polling"

break
}
}
metadataManager = containermetadata.NewMetadataManager(metadataDockerClient, cfg)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the default client will be used if ContainerMetadataClientVersion wasn't found, should we disable this feature or shut down the agent instead of providing partial information in the metadata file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only missing metadata is in the network settings with the old client. However because we have specific checks in the parsing of the networking metadata to address this issue we will not actually be missing any information unless there are multiple network modes present.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we handle old clients, do we even care about the client version anymore? Should we just use the default client and get rid of this logic entirely?

Copy link
Contributor Author

@huerdong huerdong Aug 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The client version matters because with Task ENI multiple network interfaces are now supported. The default client does not give information on additional networks which is only available in v1.21.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@richardpen and @aaithal can correct me here, but I don't believe that docker inspect will return information about the network interfaces added through a CNI plugin.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but I don't believe that docker inspect will return information about the network interfaces added through a CNI plugin

That's correct. It's also worth nothing that we invoke 2 top level CNI plugins and each of them results in a network interface being added to the container (in effect, being part of multiple "network"s)

Copy link
Contributor Author

@huerdong huerdong Aug 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, I will remove the 1.21 client check and use the default. I will leave the Networks field in the metadata stored as a list in case we use a newer API version as a default later.

dockerContainerMD := client.StartContainer(dockerContainer.DockerID, startContainerTimeout)

// Get metadata through container inspection and available task information then write this to the metadata file
// Performs this in the background to avoid delaying container start

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could definite happen in the agent, to make it simple than adding a new state is to add a new state in the container struct to indicate the metadata state.

@@ -597,6 +610,15 @@ func (engine *DockerTaskEngine) createContainer(task *api.Task, container *api.C
seelog.Infof("Created container name mapping for task %s - %s -> %s", task, container, containerName)
engine.saver.ForceSave()

// Create metadata directory and file then populate it with common metadata of all containers of this task

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the changes in this file are not unit tested.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added unit tests for the docker_task_engine_test.go for the cases where metadata service is enabled.

@@ -303,6 +308,14 @@ func (engine *DockerTaskEngine) sweepTask(task *api.Task) {
seelog.Errorf("Error removing container reference from image state: %v", err)
}
}

// Clean metadata directory for task
if engine.cfg.ContainerMetadataEnabled {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check seems weak, as writing the metadata file could fail. The safer way is to add a new state of this file and only delete this file if it's created.

Also please add a debug log here to indicate this happened.

Copy link
Contributor Author

@huerdong huerdong Aug 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure why an additional check is needed. If the file is not created, the cleanup would simply do nothing (Not even return an error).
https://golang.org/pkg/os/#RemoveAll

}
metadataFileName := filepath.Join(metadataFileDir, metadataFile)

file, err := os.OpenFile(metadataFileName, os.O_WRONLY, 644)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you make 644 a constant, probably move the one defined in write_metadata_unix.go to metadatamanager.go.

@huerdong
Copy link
Contributor Author

huerdong commented Aug 9, 2017

I have added unit tests for the containermetadata package, addressing comments by @sharanyad @aaithal.
In addition as requested by @aaithal here is the output of go test -cover ./agent/containermetadata

ok github.com/aws/amazon-ecs-agent/agent/containermetadata 0.012s coverage: 74.5% of statements

Edit: As noted in one of the more recent commits, I have increased this number to 96.2%

@aaithal
Copy link
Contributor

aaithal commented Aug 9, 2017

@huerdong 74.5% is a relatively low number for a new package that's being written. Please take another look at what code paths are not being covered in tests and add coverage for the same. Here's a blog on how you can generate html reports to visualize the missing test coverage in code: https://blog.golang.org/cover

@opalelement
Copy link

@aaithal Looks like @huerdong mentioned the code coverage has been bumped to 96.2%

@vsiddharth
Copy link
Contributor

@opalelement The code coverage was bumped by @huerdong after we introduced him to using wrappers for mocks.

@mariuszluciow
Copy link

Hi here, any news on this? We are waiting for #198 quite some time now...

@adnxn adnxn changed the title Container Metadata [WIP] Container Metadata Sep 5, 2017
@adnxn adnxn mentioned this pull request Sep 12, 2017
8 tasks
@aaithal
Copy link
Contributor

aaithal commented Sep 18, 2017

closing this in favor of #981

@aaithal aaithal closed this Sep 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants