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

Configured resource-only container /docker-daemon with 70% of node me… #9961

Merged
merged 2 commits into from
Jun 17, 2015

Conversation

dchen1107
Copy link
Member

…mory capacity. This is a workaround to docker memory leakage issue.

Without this pr, /docker-daemon is configured with unlimited memory:

# cat /sys/fs/cgroup/memory/docker-daemon/memory.limit_in_bytes 
18446744073709551615

With this, it has 70% of node memory capacity:

$ cluster/kubectl.sh describe nodes kubernetes-minion-b113
Name:           kubernetes-minion-b113
Labels:         kubernetes.io/hostname=kubernetes-minion-b113
CreationTimestamp:  Tue, 16 Jun 2015 11:03:35 -0700
Conditions:
  Type      Status  LastHeartbeatTime           LastTransitionTime          Reason                  Message
  Ready     True    Wed, 17 Jun 2015 11:22:52 -0700     Tue, 16 Jun 2015 11:03:45 -0700     kubelet is posting ready status     
Addresses:  10.240.87.219,104.197.16.208
Capacity:
 cpu:       1
 memory:    3800808Ki
 pods:      100
# cat /sys/fs/cgroup/memory/docker-daemon/memory.limit_in_bytes 
2724421632

Fix #9881

cc/ @rjnagal

…mory

capacity. This is a workaround to docker memory leakage issue.
@dchen1107
Copy link
Member Author

ok to test

@rjnagal
Copy link
Contributor

rjnagal commented Jun 17, 2015

Do we have a docker issue # for the leak?

@k8s-bot
Copy link

k8s-bot commented Jun 17, 2015

GCE e2e build/test passed for commit ca95eb7.

@dchen1107
Copy link
Member Author

Docker issue is moby/moby#9139, while filed against 1.3.0, and docker claim it is fixed in 1.7.0.

} else {
capacity = CapacityFromMachineInfo(info)
}
memoryLimit := (int64(capacity.Memory().Value() * DockerMemoryLimitThresholdPercent / 100))
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 sanity check for a lower threshold in case cAdvisor is returning 0 or really low values. Make it at least 100M?

@dchen1107
Copy link
Member Author

If cadvisor return 0 which means failed to get node memory capacity, libcontainer will ignore that value, because kernel cgroup will reject value 0 to write to memory.limit_in_bytes anyway. But you are right, cadvisor might return a wrong value which is too small. I will add a check.

@dchen1107
Copy link
Member Author

Add a sanity check, and make sure it at least 150M for now. PTAL?

@dchen1107
Copy link
Member Author

ok to test

@k8s-bot
Copy link

k8s-bot commented Jun 17, 2015

GCE e2e build/test passed for commit 32d5f46.

@k8s-bot
Copy link

k8s-bot commented Jun 17, 2015

GCE e2e build/test passed for commit 32d5f46.

@rjnagal
Copy link
Contributor

rjnagal commented Jun 17, 2015

LGTM

@rjnagal rjnagal added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 17, 2015
saad-ali added a commit that referenced this pull request Jun 17, 2015
Configured resource-only container /docker-daemon with 70% of node me…
@saad-ali saad-ali merged commit 6f0c484 into kubernetes:master Jun 17, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants