Add resource limits extensions for CPU, memory, and shared memory by roman-y-wu · Pull Request #331 · osrf/rocker

roman-y-wu · 2025-07-25T02:04:26Z

Enhanced ShmSize extension with build-time shm-size support via --shm-size-build
Added CpuLimits extension providing --cpus argument for runtime CPU limits
Added MemoryLimits extension providing --memory/-m argument for runtime memory limits
Extended core infrastructure to support docker build arguments from extensions
Added comprehensive input validation for memory and CPU formats
Updated documentation and added test coverage for all new functionality
Improved .gitignore with comprehensive Python and development file patterns

These extensions expose docker build and runtime resource limit arguments (shm-size, cpus, memory) to rocker users, providing better control over container resource allocation.

- Enhanced ShmSize extension with build-time shm-size support via --shm-size-build - Added CpuLimits extension providing --cpus argument for runtime CPU limits - Added MemoryLimits extension providing --memory/-m argument for runtime memory limits - Extended core infrastructure to support docker build arguments from extensions - Added comprehensive input validation for memory and CPU formats - Updated documentation and added test coverage for all new functionality - Improved .gitignore with comprehensive Python and development file patterns These extensions expose docker build and runtime resource limit arguments (shm-size, cpus, memory) to rocker users, providing better control over container resource allocation.

tfoote

Thanks adding these has been on my wishlist for a while, aka #311 and #239

I have a few requests that I've put in line.

At a high level I think it might make sense to separate out the resource constrant based optoins into a separate constraint_extensions.py file since the extensions.py file is getting quite long. And the resource constraints is a well defined but still decently sized set of possibilities: https://docs.docker.com/engine/containers/resource_constraints/

If you can update this PR with those adjustments I'll be happy to land this.

tfoote · 2025-07-28T18:21:20Z

.gitignore

-dist
-deb_dist
-*.pyc
+# Byte-compiled / optimized / DLL files


Please revert your changes to the .gitignore files. These look like good options for a global git config ignore on your system. We don't want to put ignores for all potential frameworks and tools into every project.

Please revert these changes to the .gitignore

tfoote · 2025-07-28T18:26:57Z

src/rocker/extensions.py

+
+    @staticmethod
+    def _validate_memory_format(value):
+        """Validate memory format (e.g., '1g', '512m', '1024k')."""


This looks incomplete. From https://docs.docker.com/reference/compose-file/build/#shm_size It refers to a formal "byte-values" definition which allows more variations: https://docs.docker.com/reference/compose-file/extension/#specifying-byte-values

Values express a byte value as a string in {amount}{byte unit} format: The supported units are b (bytes), k or kb (kilo bytes), m or mb (mega bytes) and g or gb (giga bytes). 2b 1024kb 2048k 300m 1gb

It would be good to cover that spec as well as link to it for reference.

tfoote · 2025-07-28T18:37:00Z

src/rocker/extensions.py

+        return args
+
+    @staticmethod
+    def _validate_memory_format(value):


The memory argument validation routine should be reused and not duplicated, it's just a helper function that can be validated and tested separately and then just reused in the two different extensions and potentially in more in the future.

tfoote · 2025-07-28T18:38:47Z

src/rocker/extensions.py

+            return value
+        except ValueError:
+            from argparse import ArgumentTypeError
+            raise ArgumentTypeError(f"Invalid CPU format: {value}. Expected a number (e.g., '1.5')")


Suggested change

raise ArgumentTypeError(f"Invalid CPU format: {value}. Expected a number (e.g., '1.5')")

raise ArgumentTypeError(f"Invalid CPU format: {value}. Expected a floating point number (e.g., '1.5')")

tfoote · 2025-07-28T20:18:09Z

The CI looks to have failed, it might be related to the use of EOL images i've updated here: #332

If that's the issue a rebase will help.

- Move ShmSize, CpuLimits, and MemoryLimits to separate constraint_extensions.py file - Create reusable validate_memory_format() helper function to avoid code duplication - Enhance memory validation to support Docker byte-value formats (2b, 1024kb, 300m, 1gb) - Update CPU error message to "Expected a floating point number (e.g., '1.5')" - Update setup.py entry points to reference new constraint_extensions module Addresses all feedback from tfoote in PR osrf#331 review comments. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>

- Move ShmSize, CpuLimits, and MemoryLimits to separate constraint_extensions.py file - Create reusable validate_memory_format() helper function to avoid code duplication - Enhance memory validation to support Docker byte-value formats (2b, 1024kb, 300m, 1gb) - Update CPU error message to "Expected a floating point number (e.g., '1.5')" - Update setup.py entry points to reference new constraint_extensions module Addresses all feedback from tfoote in PR osrf#331 review comments.

tfoote

Thanks for the refactors it looks good with the new file and string validation.

Could you add one quick unittest for the validation function.

And with the source in a separate file it would be great to match that with splitting out the test cases into a separate file with the same name pattern.

And the .gitignore changes still need to be reverted.

With those changes this should be good to go. With the rebase I see that the tests are passing now.

…mits

tfoote

Thanks for moving the content and the tests to the new locations.

There's duplicate copies of the test now in the test_extension.py and please revert the .gitignore changes and this will be good to go.

tfoote · 2025-08-05T21:44:41Z

.gitignore

-dist
-deb_dist
-*.pyc
+# Byte-compiled / optimized / DLL files


Please revert these changes to the .gitignore

tfoote · 2025-08-05T21:47:36Z

test/test_extension.py

+        self.assertEqual(build_args, {'shm_size': '2g'})
+
+
+class CpuLimitsExtensionTest(unittest.TestCase):


This is now in test_constraint_extensions.py please remove this duplicate.

tfoote · 2025-08-05T21:47:44Z

test/test_extension.py

+        self.assertIn('--cpus 2.5', args)
+
+
+class MemoryLimitsExtensionTest(unittest.TestCase):


This is now in test_constraint_extensions.py please remove this duplicate too.

Thanks for the review!

…ion.py

tfoote

Thanks for iterating and adding these extra features.

roman-y-wu requested a review from tfoote as a code owner July 25, 2025 02:04

Update .gitignore

502d46b

tfoote requested changes Jul 28, 2025

View reviewed changes

Merge branch 'osrf:main' into main

b190cdd

This was linked to issues Jul 30, 2025

Enable memory, cpu, and other limits on the container #239

Closed

Add support for shmem, memory, cpu limits for build stage #311

Closed

Merge branch 'main' into main

941c3d5

roman-y-wu force-pushed the main branch from 65b8cb3 to 941c3d5 Compare July 30, 2025 16:19

tfoote requested changes Jul 31, 2025

View reviewed changes

Add tests for constraint extensions: ShmSize, CpuLimits, and MemoryLi…

70ca375

…mits

tfoote requested changes Aug 5, 2025

View reviewed changes

Revert .gitignore changes and remove duplicate tests from test_extens…

2978e90

…ion.py

tfoote approved these changes Aug 8, 2025

View reviewed changes

tfoote merged commit ce67dc6 into osrf:main Aug 8, 2025
4 checks passed

tfoote mentioned this pull request Aug 8, 2025

Restore interactive as the default behavior #333

Merged

	raise ArgumentTypeError(f"Invalid CPU format: {value}. Expected a number (e.g., '1.5')")
	raise ArgumentTypeError(f"Invalid CPU format: {value}. Expected a floating point number (e.g., '1.5')")

		self.assertEqual(build_args, {'shm_size': '2g'})


		class CpuLimitsExtensionTest(unittest.TestCase):

		self.assertIn('--cpus 2.5', args)


		class MemoryLimitsExtensionTest(unittest.TestCase):

Conversation

roman-y-wu commented Jul 25, 2025

Uh oh!

tfoote left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tfoote commented Jul 28, 2025

Uh oh!

tfoote left a comment

Choose a reason for hiding this comment

Uh oh!

tfoote left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tfoote left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants