-
Notifications
You must be signed in to change notification settings - Fork 14
Adds option to set memory and cpu constraint #671
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
Conversation
713abdd
to
f7c8735
Compare
Hey @kyteinsky @oleksandr-nc, |
31bee10
to
c78d2a3
Compare
Hello! You can use |
P.S.: But we usually do this after the PR is approved, before merging - the only exceptions are those PRs where the author is confident there won't be any changes to not remove commits of bots from git history. The developers are building JS locally for review. I will try to check this PR - is it ready for review or not? |
I see, that makes sense. Thank you. |
@Hephi2 When I edit the daemon deploy page and fill the Memory limit and then start changing the CPU limit, the Memory limit gets reset. |
cpu-limit.mp4 |
Tests were performed on HaRP, results:Without memory limit:
Result: 🆗 With 8GB memory limit:
Result: 🆗 Without CPU limit: ![]() Result: 🆗 With CPU limit= ![]() Result: 🆗 @Hephi2 Second question, if you don't mind, does |
@oleksandr-nc Regarding your second question: |
Are you using Vue2 and not building this with Vue3? |
I can't reproduce this. Which browser do you use and does any other browser have this issue too? |
@Hephi2 Also, see https://www.corsair.com/us/en/explorer/diy-builder/storage/mebibyte-vs-megabyte-whats-the-difference/ |
Firefox. But reactivity in Vue2 should work the same among all browsers? |
Description what happens on video: On mount, when editing a daemon whose |
@Hephi2 would you be up for giving it one more go to see if you can reproduce it with the additional hints? |
Yes, sure. |
So I was able to reproduce this issue. The steps were:
the fields should be empty now. |
Actually, this was covered if both were not set. However, in the edge case described by @kyteinsky, the corresponding other key was not initialised. |
With latest changes editing Deploy Config works fine for me. @kyteinsky can you check on your side as well? |
confirmed, that is fixed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
/compile |
@Hephi2 please squash three first commits and after that we can merge this |
- Adds fields in admin UI - Sets HostConfig.Memory and HostConfig.NanoCPUs fields for Docker Socket Proxy - Passes memory and nanoCPU values to HaRP Proxy (requires changes in HaRP repo) Signed-off-by: Hephi2 <[email protected]> Update built assets after source changes Signed-off-by: Hephi2 <[email protected]> Fixes resetting memory limit bug, MB -> MiB Signed-off-by: Hephi2 <[email protected]>
8c6c85d
to
40ecdd0
Compare
/compile |
Signed-off-by: nextcloud-command <[email protected]>
Solves #547
- Adds fields in admin UI
- Sets HostConfig.Memory and HostConfig.NanoCPUs fields for Docker Socket Proxy
- Passes memory and nanoCPU values to HaRP Proxy (requires changes in HaRP repo)
Requires changes in HaRP to work. See nextcloud/HaRP#46
To validate the effect of the resource constraints run e.g. the following commands:
docker exec -it <container_id> bash
yes > /dev/null
docker stats <container_id>
that the container stays around the configured CPU cappython3 -c "a = ' ' * (1024 * 1024 * 1024); input()"
docker stats <container_id>
that the limit is equal to to set memory value and if the needed memory is too high, the process will be killed