Hi @roman-kiselenko. Thanks for your PR.

I"m waiting for a cri-o member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

/ok-to-test

@roman-kiselenko do you mind rebasing on top of the latest main branch?

@roman-kiselenko do you mind rebasing on top of the latest main branch?

@saschagrunert

It"s not ready yet, I"m almost done with the implementation, and after adding some test will perform rebase and request the review. 🙏

Codecov Report

Merging #7832 (9fa92d1) into main (f63de87) will increase coverage by 0.88%.
Report is 68 commits behind head on main.
The diff coverage is 27.08%.

@@            Coverage Diff             @@
##             main    #7832      +/-   ##
==========================================
+ Coverage   48.94%   49.82%   +0.88%     
==========================================
  Files         151      153       +2     
  Lines       16426    16635     +209     
==========================================
+ Hits         8039     8288     +249     
+ Misses       7412     7324      -88     
- Partials      975     1023      +48     

@roman-kiselenko, looks great so far! Thank you! 🚀

Some ideas on how this would be deployed.

• There will be a default hardcoded limit, a minimal memory limit, we will fallback to if nothing else is set

(somewhere within the code base)

const defaultContainerMinimumMemory = 7.5 * 1024 * 1024 // 7.5 MiB

• crio.conf will take the following (for the relevant tables TOML tables):

A default of 7.5 or 12 MiB (something that remains to be seen) is overridden by the following:

(within a crio.conf configuration file)

[crio.runtime]
container_minimum_memory = "7680K"

Which then overrides this setting for a specific runtime, such as crun, for example:

[crio.runtime.runtimes.crun]
container_minimum_memory = "384K"

(somewhere in the terminal on the commnad-line)

Above, irregardless where the value was set, can be then overridden with directly specifying value via a command-line switch such as the --default-container-minimum-memory, or for a specific runtime via the --runtimes command-line switch that has a specific format to follow.

Since both Docker and Podman allow users to supply values using more of a human-friendly way, such as ability to provide units next to values, which makes it easier to say 7.5M (or 7.5m) to denote 7.5 MiB rather than 7864320 (bytes), which is easy to get wrong.

This parsing and conversion to bytes can be done using the RAMInBytes() helper from the go-units package that we already include as dependency.

Granted, this then makes it a little more inconvenient to deal with in therm of the type of the field with a corresponding configuration type (as in, Go type, that is). However, since this value is used once when setting up cgroups into which the container will be place, so at a very beginning, then there is no need to cache this and a calculation of a bytes value on-the-fly via some helper would be fine, I belive.

Thoughts?

@kwilczynski SGTM.

I"ve done changes from int64 to string representation, going to add some tests now.

Do we want to add an integration test for that feature?

Added some integration tests, PTAL

/retest

@cri-o/cri-o-maintainers, please have another look at this! Thank you!

/approve
/lgtm

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: haircommander, kwilczynski, roman-kiselenko, saschagrunert

The full list of commands accepted by this bot can be found here.

The pull request process is described here

/unhold

/cherry-pick release-1.29

@kwilczynski: cannot checkout 1.29: error checking out "1.29": exit status 1 error: pathspec "1.29" did not match any file(s) known to git

@kwilczynski: #7832 failed to apply on top of branch "release-1.29":

Applying: Implement configurable container minimum memory limit per OCI runtime.
Using index info to reconstruct a base tree...
M	completions/fish/crio.fish
M	docs/crio.8.md
M	docs/crio.conf.5.md
M	internal/config/cgmgr/cgmgr_linux.go
M	internal/config/cgmgr/cgmgr_test.go
M	internal/config/cgmgr/cgmgr_unsupported.go
M	internal/config/cgmgr/cgroupfs_linux.go
M	internal/config/cgmgr/systemd_linux.go
M	internal/criocli/criocli.go
M	internal/oci/oci.go
M	pkg/config/config.go
M	pkg/config/config_test.go
M	pkg/config/template.go
M	server/container_create_linux.go
M	server/nri-api.go
M	server/sandbox_run_linux.go
M	test/image.bats
Falling back to patching base and 3-way merge...
Auto-merging test/image.bats
CONFLICT (content): Merge conflict in test/image.bats
Auto-merging server/sandbox_run_linux.go
Auto-merging server/nri-api.go
Auto-merging server/container_create_linux.go
Auto-merging pkg/config/template.go
Auto-merging pkg/config/config_test.go
Auto-merging pkg/config/config.go
Auto-merging internal/oci/oci.go
CONFLICT (content): Merge conflict in internal/oci/oci.go
Auto-merging internal/criocli/criocli.go
Auto-merging internal/config/cgmgr/systemd_linux.go
Auto-merging internal/config/cgmgr/cgroupfs_linux.go
Auto-merging internal/config/cgmgr/cgmgr_unsupported.go
Auto-merging internal/config/cgmgr/cgmgr_test.go
Auto-merging internal/config/cgmgr/cgmgr_linux.go
CONFLICT (content): Merge conflict in internal/config/cgmgr/cgmgr_linux.go
Auto-merging docs/crio.conf.5.md
Auto-merging docs/crio.8.md
Auto-merging completions/fish/crio.fish
error: Failed to merge in the changes.
hint: Use "git am --show-current-patch=diff" to see the failed patch
Patch failed at 0001 Implement configurable container minimum memory limit per OCI runtime.
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

OK. Requires manual cherry-pick.