-
Notifications
You must be signed in to change notification settings - Fork 717
feat(AzBatchService): Include cpu-shares and memory in docker run command #5799
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
feat(AzBatchService): Include cpu-shares and memory in docker run command #5799
Conversation
…mand The Azure Batch task did not include the --cpu-shares or --memory options of the docker run command, meaning resource allocation was only controlled by slots on the node. This PR introduces the additional options to the container run command, including --cpu-shares and --memory. The behaviour will now be more similar to AWS Batch which includes a soft limit on CPUs and hard limit on memory. It also refactors the container options to use a StringBuilder class instead of string concatenation. Signed-off-by: adamrtalbot <[email protected]>
✅ Deploy Preview for nextflow-docs-staging ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
…te test Signed-off-by: adamrtalbot <[email protected]>
Signed-off-by: adamrtalbot <[email protected]>
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.
Nice. I'd prefer readability in this code, seem the befits of string builder is negligible in this scenario.
I quite like the StringBuilder 😢 but I can revert it quickly enough. |
Signed-off-by: adamrtalbot <[email protected]>
Reverted in e0daedb |
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.
Nice @adamrtalbot, well done
Hi, Just fyi. This PR has implications on AZCOPY. If you set memory to e.g. 4GB azcopy will crash when transferring multiple large files (50GB+). For large files we found that a minimum of 16GB is required to copy the files to the VM. Due to code in link it will then try to generate a folder that also crashs. It took a bit of time to find this bug :) |
Is this because azcopy requires more than 4GB? This is now a hard limit, so if azcopy tries to use more than 4GB it will cause issues. In theory, it will cause fewer issues when packing large number of tasks onto a single node. I'm not massively familiar with azcopy memory control but we could see if we could control azcopy memory usage, e.g. take longer but stay under that limit. |
Looks like we should be able to fix this by setting AZCOPY_BUFFER_GB: https://learn.microsoft.com/en-us/azure/storage/common/storage-use-azcopy-optimize#optimize-memory-use |
The Azure Batch task did not include the
--cpu-shares
or--memory
options of the docker run command, meaning resource allocation was only controlled by slots on the node. This PR introduces the additional options to the container run command, including--cpu-shares
and--memory
. The behaviour will now be more similar to AWS Batch which includes a soft limit on CPUs and hard limit on memory.