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

ecs_client/model, functional_tests: updated model, functional tests for devices and init fields #1004

Merged
merged 1 commit into from
Nov 9, 2017
Merged

ecs_client/model, functional_tests: updated model, functional tests for devices and init fields #1004

merged 1 commit into from
Nov 9, 2017

Conversation

sharanyad
Copy link
Contributor

@sharanyad sharanyad commented Oct 5, 2017 •
edited
Loading

Summary

Model changes to Container Definition for enabling devices and init for Linux containers.
Functional tests

Implementation details

Functional tests that verify:

  • If device is added to the container with the right cgroup permissions
  • If init flag is enabled, then init process is present in the container

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

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

@sharanyad
Copy link
Contributor Author

This PR depends on the merge of #996

@sharanyad sharanyad changed the title ecs_client/model, functional_tests: updated model, functional tests for devices and init fields [WIP] ecs_client/model, functional_tests: updated model, functional tests for devices and init fields Oct 5, 2017
aaithal
aaithal requested changes Oct 6, 2017
View reviewed changes
Copy link
Contributor

@aaithal aaithal left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Is this still WIP? Also, can you link the following issues in the commit message?

  1. Add support for --init #852
  2. Allow to input device flag in the task definition #433

Lastly, please rephrase the commit message as per this comment.

@sharanyad
Copy link
Contributor Author

Referenced the issues in the commit message.
I mentioned it as WIP as there will be more changes added to this PR for the documentation of new fields and change in required agent version for the tests. I would like to get the tests reviewed before that.

aaithal
aaithal reviewed Oct 6, 2017
View reviewed changes
agent/functional_tests/testdata/taskdefinitions/devices/task-definition.json
}
]
},
"command": ["sh", "-c", "if ls /dev/sda && ! fdisk /dev/sda; then exit 42; else exit 1; fi"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we not do fdisk here? Especially because you are mounting /dev/xvda into the container? Or at least fdisk with no flags? fdisk -l seems reasonable. Or any of lsblk or blockdev?

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 fdisk call is supposed to fail here, since only read permission is added to the device in container. -l flag can also be used here.

Copy link
Contributor

@aaithal aaithal Oct 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Right, but it'll also fail if the device wasn't mounted. You need a check to ensure that it's mounted as well. Can't you check the permission with something like stat --format=%a /dev/sda?

Alternatively, you can do both. Make sure that disk exists fdisk -l and that you can't write to it. fdisk

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 commands like stat and lsblk take information from sysfs, hence do not show the right permissions for the mounted devices.

I tried the following:

$ ls -l /dev/xvda
brw-rw---- 1 root disk 202, 0 Jul  5 23:37 /dev/xvda

docker run -it --device /dev/xvda:/dev/dummy ubuntu
# ls -l /dev/dummy
brw-rw---- 1 root disk 202, 0 Oct  6 18:54 /dev/dummy
# stat --format=%a /dev/dummy
660
# fdisk /dev/dummy
Welcome to fdisk (util-linux 2.27.1).
Changes will remain in memory only, until you decide to write them.
Be careful before using the write command.
Command (m for help): q

and

docker run -it --device /dev/xvda:/dev/dummy:r ubuntu
# ls -l /dev/dummy
brw-rw---- 1 root disk 202, 0 Oct  6 18:58 /dev/dummy
# stat --format=%a /dev/dummy
660
# fdisk /dev/dummy
Welcome to fdisk (util-linux 2.27.1).
Changes will remain in memory only, until you decide to write them.
Be careful before using the write command.
fdisk: cannot open /dev/dummy: Operation not permitted

As you mentioned, we can do ls to check if the device is mounted and fdisk for permissions.

agent/functional_tests/testdata/taskdefinitions/init-process/task-definition.json Outdated
"linuxParameters": {
"initProcessEnabled":true
},
"command": ["sh", "-c", "ps -x | grep \"/dev/init\" && exit 42 || exit 1"]
Copy link
Contributor

Choose a reason for hiding this comment

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

This will return false positives I think. Can we do pidof "/dev/init" == 1 check instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pidof "/dev/init" == 1 is a better check. will change this.

@jhaynes
Copy link
Contributor

jhaynes commented Oct 10, 2017

Can you add entries to the CHANGELOG.md for enabling init and devices and reference this pr as well as #996?

@sharanyad
Copy link
Contributor Author

These changes cannot go in until support for device and init is there in the backend services. Will add changelog and reference #996.

@sharanyad sharanyad added this to the 1.15.2 milestone Nov 8, 2017
@sharanyad sharanyad changed the title [WIP] ecs_client/model, functional_tests: updated model, functional tests for devices and init fields ecs_client/model, functional_tests: updated model, functional tests for devices and init fields Nov 8, 2017
@aaithal
Copy link
Contributor

aaithal commented Nov 9, 2017

@sharanyad this is tagged for milestone 1.15.2. Can you please address the comments if this needs to go in the next release? Or, please change the milestone if it doesn't need to go in the next release. Thanks!

@sharanyad
Copy link
Contributor Author

@aaithal The comments have been addressed. The functional tests pass now too.
Could you please have a look again? Thanks.

@sharanyad
ecs_client/model, functional_tests: devices and init support
c268ea0 
This commit contains the model changes and functional tests for the new Task Definition fields-devices and initProcessEnabled.

This addresses the following issues:
* aws#433
* aws#852
@sharanyad sharanyad merged commit b13c8e9 into aws:dev Nov 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aaithal aaithal aaithal approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@sharanyad @jhaynes @aaithal