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 tests for docker resource limits #4611

Closed

Conversation

Projects
None yet
2 participants
@stefwalter
Copy link
Contributor

commented Jun 21, 2016

I've split this out of #4600

@stefwalter stefwalter force-pushed the stefwalter:docker-limits-tests branch from 22f89fc to dfb45f2 Jun 22, 2016

@stefwalter stefwalter removed the needswork label Jun 22, 2016

@stefwalter stefwalter changed the title WIP: Add tests for docker resource limits Add tests for docker resource limits Jun 22, 2016

@dperpeet dperpeet added the needswork label Jun 22, 2016

@dperpeet

This comment has been minimized.

Copy link
Member

commented Jun 22, 2016

tests are failing, sorry

@stefwalter stefwalter removed the needswork label Jun 22, 2016

@@ -406,3 +406,10 @@
border: 1px solid #d7d7d7;
cursor: auto;
}

.size-text {

This comment has been minimized.

Copy link
@dperpeet

dperpeet Jun 23, 2016

Member

shouldn't we add ct? maybe .size-ct-text

This comment has been minimized.

Copy link
@stefwalter

stefwalter Jun 23, 2016

Author Contributor

Done with: .size-text-ct

});
var used;
var total;
var avail;

This comment has been minimized.

Copy link
@dperpeet

dperpeet Jun 23, 2016

Member

I know this is legacy code, but when I read avail, the first two things that pop into my mind are the noun and verb avail, not the abbreviation of available.

This comment has been minimized.

Copy link
@stefwalter

stefwalter Jun 23, 2016

Author Contributor

Done.

limit = min;
return cockpit.format_bytes(limit, 1024);
/* Logarithmic scale */
var minv = Math.log(min);

This comment has been minimized.

Copy link
@dperpeet

dperpeet Jun 23, 2016

Member

Should we do some checking here that min and max are > 0?

This comment has been minimized.

Copy link
@stefwalter

stefwalter Jun 23, 2016

Author Contributor

Added.

@dperpeet

This comment has been minimized.

Copy link
Member

commented Jun 23, 2016

Setting <1024 cpu shares can lead to an error, do we need to catch this?
It has been fixed upstream in 1.11, so maybe not:
screenshot from 2016-06-23 09-24-04
moby/moby#22818

@dperpeet

This comment has been minimized.

Copy link
Member

commented Jun 23, 2016

Changing the resources for a running container on my system didn't work. I opened the dialog to change, unchecked cpu and/or memory, but on closing the changes didn't apply.

Installed Packages
Name        : docker
Arch        : x86_64
Epoch       : 2
Version     : 1.10.3
Release     : 9.git667d6d1.fc24
@stefwalter

This comment has been minimized.

Copy link
Contributor Author

commented Jun 23, 2016

Changing the resources for a running container on my system didn't work. I opened the dialog to change, unchecked cpu and/or memory, but on closing the changes didn't apply.

Cannot reproduce. On Fedora 23 I started a busybox container with limits on both CPU and Memory. I then went and unchecked both of those and they reset to unlimited and to defaults.

Could you blow by blow outline a more clear reproducer using one of the test vms?

@stefwalter stefwalter added the question label Jun 23, 2016

@stefwalter stefwalter force-pushed the stefwalter:docker-limits-tests branch from d9ab661 to da0fdbf Jun 23, 2016

@dperpeet dperpeet added needswork and removed question labels Jun 23, 2016

@dperpeet

This comment has been minimized.

Copy link
Member

commented Jun 23, 2016

On the current f24 testvm:

  • create busybox with cpu/memory limits enabled, at default
  • run with /bin/sleep 100000000
  • go to container, change resource limits
  • uncheck both
  • open "change resource limits" dialog again, CPU priority will still be checked
@stefwalter

This comment has been minimized.

Copy link
Contributor Author

commented Jun 23, 2016

Okay, hopefully this fixes the issue. No more checkbox in the Change dialog for CPU priority. It doesn't make sense there.

@stefwalter stefwalter removed the needswork label Jun 23, 2016

else:
b.wait_not_visible("#containers-run-image-memory")
b.click("#containers-run-image-cpu input[type='checkbox']");
b.set_val("#containers-run-image-cpu input.size-text", "512");

This comment has been minimized.

Copy link
@dperpeet

dperpeet Jun 23, 2016

Member

needs to be size-text-ct

b.click("#containers-run-image-with-terminal");
if memory_supported:
b.click("#containers-run-image-memory input[type='checkbox']");
b.set_val("#containers-run-image-memory input.size-text", "236");

This comment has been minimized.

Copy link
@dperpeet

dperpeet Jun 23, 2016

Member

needs to be size-text-ct

b.click("#container-details-resource-row button")
b.wait_popup("container-resources-dialog")
if memory_supported:
b.set_val(".memory-slider input.size-text", "246");

This comment has been minimized.

Copy link
@dperpeet

dperpeet Jun 23, 2016

Member

needs to be size-text-ct

b.set_val(".memory-slider input.size-text", "246");
else:
b.wait_not_visible(".memory-slider")
b.set_val(".cpu-slider input.size-text", "256");

This comment has been minimized.

Copy link
@dperpeet

dperpeet Jun 23, 2016

Member

needs to be size-text-ct

@dperpeet dperpeet added the needswork label Jun 23, 2016

stefwalter added some commits Jun 20, 2016

docker: Add textual fields to resource sliders
Besides allowing people to enter actual values, this allows
the tests to function appropriately.

For now we just limit memory to MiB.

Add tests for this functionality.
docker: Refactor how the Docker /info is retrieved
This needs to be retrieved regularly anyway for storage, so do
it manually anyway.

stefwalter added some commits Jun 20, 2016

docker: Use the Docker /info to configure memory limit behavior
Use the MemTotal and MemoryLimit fields to configure the memory
resource limit behavior in the UI. On Debian for example the
operating system doesn't support memory limits, or at least Docker
doesn't know how to implement that there.
base1: Add meta property to cockpit.metrics()
This lets callers look at the meta messages that come in.
docker: Track the cgroups that come in properly
Various versions of Docker and systemd have different cgroup
names. We figure out which ones we have, and how to assign them
to different containers rather than assuming they're in a specific
format.
test: Fix workaround for buggy docker logging
Docker sometimes truncates the last line of logging output.
Lets make our tests more immune to this.

@stefwalter stefwalter force-pushed the stefwalter:docker-limits-tests branch from da0fdbf to 7fde461 Jun 24, 2016

@stefwalter

This comment has been minimized.

Copy link
Contributor Author

commented Jun 24, 2016

Fixed the size-text-ct issues.

@stefwalter stefwalter removed the needswork label Jun 24, 2016

@dperpeet

This comment has been minimized.

Copy link
Member

commented Jun 24, 2016

testResources fails on fedora-24

@dperpeet dperpeet added the needswork label Jun 24, 2016

@stefwalter

This comment has been minimized.

Copy link
Contributor Author

commented Jun 24, 2016

testResources fails on fedora-24

I've seen this elsewhere. Holding off on fixing this until the run dialog is refactored.

@stefwalter stefwalter removed the needswork label Jun 24, 2016

@dperpeet dperpeet closed this in 4084a91 Jun 26, 2016

@stefwalter stefwalter deleted the stefwalter:docker-limits-tests branch Sep 8, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.