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.

Sign up for GitHub

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

Jump to bottom

handlers: Add Volumes to v1 and v2 metadata #1531

Merged
merged 5 commits into from
Aug 22, 2018
Merged

handlers: Add Volumes to v1 and v2 metadata #1531

merged 5 commits into from
Aug 22, 2018

Conversation

sharanyad
Copy link
Contributor

@sharanyad sharanyad commented Aug 20, 2018 •
edited
Loading

Summary

Expose volumes metadata as part of each container in task through v1 and v2 metadata endpoint.
This addresses #1516

Implementation details

new task metadata(from functional tests):

[Info] Received tasks metadata:
{
    "Tasks": [
        {
            "Arn": "arn:aws:ecs:us-west-2:074626758757:task/0353708a-6653-4bc2-9e84-ad2dcc8fe68c",
            "DesiredStatus": "RUNNING",
            "KnownStatus": "RUNNING",
            "Family": "ecsftest-agent-introspection-validator-4c0f2e0d3d9e85fd9e5ca7fdf73a3628",
            "Version": "1",
            "Containers": [
                {
                    "DockerId": "9c562697bcbeab2ce97491080d78dbeb38e64d21203417895edf1b5ce203ba24",
                    "DockerName": "ecs-ecsftest-agent-introspection-validator-4c0f2e0d3d9e85fd9e5ca7fdf73a3628-1-agent-introspection-validator-be84a5fd9680eac74b00",
                    "Name": "agent-introspection-validator",
                    "Volumes": [
                        {
                            "DockerName": "ecs-ecsftest-agent-introspection-validator-4c0f2e0d3d9e85fd9e5ca7fdf73a3628-1-task-local-bcb19ac3cc93fe8f8901",
                            "Source": "/var/lib/docker/volumes/ecs-ecsftest-agent-introspection-validator-4c0f2e0d3d9e85fd9e5ca7fdf73a3628-1-task-local-bcb19ac3cc93fe8f8901/_data",
                            "Destination": "/ecs"
                        }
                    ]
                }
            ]
        }
    ]
}

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

Licensing

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@sharanyad sharanyad requested review from richardpen, adnxn, yhlee-aws and a team August 20, 2018 18:45
adnxn
adnxn reviewed Aug 21, 2018
View reviewed changes
agent/api/container/container.go Outdated
@@ -190,6 +190,9 @@ type Container struct {
// KnownPortBindingsUnsafe is an array of port bindings for the container.
KnownPortBindingsUnsafe []PortBinding `json:"KnownPortBindings"`

// VolumesUnsafe is an array of volume mounts in the container.
VolumesUnsafe []docker.Mount
Copy link
Contributor

Choose a reason for hiding this comment

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

correct me if im wrong: this ends up in the statefile yea? if yes - needs a statefile version bump?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This field is not required in the statefile. Added json ignore tag.

@sharanyad sharanyad added this to the 1.20.2 milestone Aug 21, 2018
@fenxiong
Copy link
Contributor

i think since we add a new field in container response, the functional test for agent introspection endpoint should be updated?

@adnxn
Copy link
Contributor

adnxn commented Aug 21, 2018

agreed with @fenxiong, the functional test for introspection endpoint should be updated as well.

@sharanyad
Copy link
Contributor Author

Updated the functional tests

richardpen
richardpen reviewed Aug 21, 2018
View reviewed changes
agent/api/container/container.go
@@ -190,6 +190,9 @@ type Container struct {
// KnownPortBindingsUnsafe is an array of port bindings for the container.
KnownPortBindingsUnsafe []PortBinding `json:"KnownPortBindings"`

// VolumesUnsafe is an array of volume mounts in the container.
VolumesUnsafe []docker.Mount `json:"-"`
Copy link

Choose a reason for hiding this comment

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

Please update this unit test to cover this:

amazon-ecs-agent/agent/engine/docker_task_engine_test.go

Line 1923 in 02a6a9f

func TestSynchronizeContainerStatus(t *testing.T) {

agent/handlers/v1/response.go Outdated
// NewVolumesResponse creates VolumeResponse for a container
func NewVolumesResponse(dockerContainer *apicontainer.DockerContainer) []VolumeResponse {
container := dockerContainer.Container
resp := []VolumeResponse{}
Copy link

Choose a reason for hiding this comment

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

You may want to use var resp []VolumeResponse instead, as the marshal of resp := []VolumeResponse would be [] instead of null.

misc/taskmetadata-validator/taskmetadata-validator.go
@@ -243,6 +251,11 @@ func main() {
}
}

if containerMetadata.Volumes == nil {
Copy link

Choose a reason for hiding this comment

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

Can you check some of the fields that were specified in the task definition?

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 docker name and source are difficult to be verified, since the volume name is created with some task information and random ID at the end. Hence checking for nil here. Do you have other suggestions?

Copy link

Choose a reason for hiding this comment

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

I think you can test with the shared volume, which we specify the name, and the destination should be also specified in the task definition? We may not able to specify the source though.

richardpen
richardpen approved these changes Aug 21, 2018
View reviewed changes
Copy link

@richardpen richardpen left a comment

Choose a reason for hiding this comment

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

LGTM as long as the tests pass.

adnxn
adnxn approved these changes Aug 21, 2018
View reviewed changes
Copy link
Contributor

@adnxn adnxn left a comment

Choose a reason for hiding this comment

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

thanks for updating the functional test

@sharanyad sharanyad merged commit f23d4de into aws:dev Aug 22, 2018
@fenxiong fenxiong mentioned this pull request Dec 4, 2018
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adnxn adnxn adnxn approved these changes

@richardpen richardpen richardpen approved these changes

@yhlee-aws yhlee-aws yhlee-aws approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.20.2
Development

Successfully merging this pull request may close these issues.
5 participants
@sharanyad @fenxiong @adnxn @richardpen @yhlee-aws