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

Add docker resource constraints for CPU and Memory #28

Merged
merged 5 commits into from
Sep 10, 2015

Conversation

cbednarski
Copy link
Contributor

This encapsulates #23

This PR includes a basic implementation for Docker CPU and Memory constraints.

  • Memory is a hard limit. Memory allocation will fail when this limit is reached, so the app will crash before the OOM killer gets involved. This means a process will get AT MOST the amount of memory configured.
  • CPU is a soft limit and is proportionate to other processes running on the machine. This means that a process should always get AT LEAST the CPU configured, but maybe more if the system is not fully allocated or other processes are not using their allocations.

Also, the start container call is now implemented using a native go docker API client (fsouza/go-dockerclient).

@@ -34,9 +37,10 @@ type dockerHandle struct {
doneCh chan struct{}
}

func NewDockerDriver(logger *log.Logger) Driver {
func NewDockerDriver(logger *log.Logger, config *config.Config) Driver {
Copy link
Member

Choose a reason for hiding this comment

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

Yo dawg I heard you like configs

@ryanuber
Copy link
Member

ryanuber commented Sep 9, 2015

@cbednarski the code looks good. So the reason we are threading the config into the drivers now is so that we can get the image name for the docker containers?

@@ -59,17 +63,81 @@ func (d *DockerDriver) Fingerprint(cfg *config.Config, node *structs.Node) (bool
return true, nil
}

// containerOptionsForTask initializes a strcut needed to call
Copy link
Contributor

Choose a reason for hiding this comment

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

typo "strcut" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, should be "struct".

@catsby
Copy link
Contributor

catsby commented Sep 9, 2015

Small comments, otherwise 👍

@cbednarski
Copy link
Contributor Author

So the reason we are threading the config into the drivers now is so that we can get the image name for the docker containers?

@ryanuber No, the image name is provided through the task configuration. The driver configuration is tells the docker client which socket to use to connect to the docker daemon. By default this is a unix socket at /var/run/docker.sock but it could also be HTTP, or could be located somewhere else. Later, Docker will also need a way to get credentials to pull from private repos.

I think it's inevitable that Docker and other drivers will need additional config options as we go.

- Add validation that Resources.MemoryMB and Resources.CPU have non-zero values
- Change log calls to use logger whenever possible
- Change log format to add colon after driver.docker
cbednarski added a commit that referenced this pull request Sep 10, 2015
Add docker resource constraints for CPU and Memory
@cbednarski cbednarski merged commit b3395a5 into master Sep 10, 2015
@cbednarski cbednarski deleted the f-docker-resource-constraints branch September 10, 2015 18:30
@cbednarski cbednarski restored the f-docker-resource-constraints branch September 10, 2015 18:32
@cbednarski cbednarski deleted the f-docker-resource-constraints branch September 18, 2015 01:05
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants