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

[STORM-3271] enable supervisors to launch a worker inside a docker container #3287

Merged
merged 5 commits into from
Oct 1, 2020

Conversation

Ethanlm
Copy link
Contributor

@Ethanlm Ethanlm commented Jun 12, 2020

What is the purpose of the change

With this new functionality, a supervisor can launch a worker process inside a docker container for better security and portability.

How was the change tested

  1. Tested by submitting a topology, and validated the basic functions like launching, killing, cleaning up, profiling works
  2. Validated the worker process is isolated inside a container with mostly read-only filesystem, and its own pid namespace
  3. Validated cpu bindings and cpu throttling work as expected.
  4. Validated the cpu and memory metrics work as expected.

@Ethanlm Ethanlm force-pushed the STORM-3271-docker branch 2 times, most recently from a8b9127 to 1841f58 Compare June 12, 2020 19:59
* @return the full command.
*/
@Override
public String getCommandWithArguments() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to override this method? Looks like the parent DockerCommand.getCommandWithArguments is doing the same thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. I will remove this one.

Copy link
Contributor

@agresch agresch left a comment

Choose a reason for hiding this comment

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

minor documentation comments. Looks good. I confess not to have looked in great detail at all the worker launcher changes.

docs/Docker-support.md Outdated Show resolved Hide resolved
docs/Docker-support.md Outdated Show resolved Hide resolved
docs/Docker-support.md Outdated Show resolved Hide resolved
Copy link
Contributor

@agresch agresch left a comment

Choose a reason for hiding this comment

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

Would be nice to get another review.....

Copy link
Contributor

@kishorvpatil kishorvpatil left a comment

Choose a reason for hiding this comment

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

LGTM

@Ethanlm Ethanlm merged commit 935f74c into apache:master Oct 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants