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

containerd: resource-limit settings for oci default #3206

Conversation

vyaghras
Copy link
Contributor

@vyaghras vyaghras commented Jun 16, 2023

Issue number:

Closes #2814 #2862

Description of changes:
Enable the capability to set the values for following resource limits
MaxAddressSpace,
MaxCoreFileSize,
MaxCpuTime,
MaxDataSize,
MaxFileLocks,
MaxFileSize,
MaxLockedMemory,
MaxMsgqueueSize,
MaxNicePriority,
MaxPendingSignals,
MaxProcesses,
MaxRealtimePriority,
MaxRealtimeTimeout,
MaxResidentSet,
MaxStackSize

Testing done:

  • Try setting a value for all the settings using api client - It should show these values in etc/containerd/cri-base.json
  • Try setting a "unlimited" as hard-limit for max-locked-memory in the settings using api client - It should show u64::MAX as hard limit for RLIMIT_MEMLOCK in etc/containerd/cri-base.json and the ulimit -Hl should set to unlimited.
  • Create a pod on bottlerocket host and check /proc/self/limits - It should list all the settings that has been done
    - Create a sample nginx workload container launched via kubernetes and check /proc/self/limits - This has default value for Max locked memory as 65536.
    - Change Max locked memory setting using api client to 6536.
    - Create another container that runs the nginx image and check the /proc/self/limits - This has default value for Max locked memory as 6536.
  • Migration testing
    - Create an ami with 1.14.3 version and create instance using that.
    - Try to set the max-locked-memory setting- 1.14.3 will not allow to do this setting.
    - Upgrade to 1.15.0 with the image in your custom TUF repo.
    - Try to set the max-locked-memory setting- 1.15.0 will allow to do memlock setting and add this in etc/containerd/cri_base.json.
    - Downgrade back to the older version 1.14.3.
    - Try to set the max-locked-memory setting- 1.14.3 will not allow to do this setting.

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

Copy link
Contributor

@stmcginnis stmcginnis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it's doing what's expected. I ran:

apiclient apply <<EOF
[settings.oci-defaults.resource-limits.max-address-space]
hard-limit = 2000000000
soft-limit = 1000000000

[settings.oci-defaults.resource-limits.max-core-file-size]
hard-limit = 2000000000
soft-limit = 1000000000

[settings.oci-defaults.resource-limits.max-cpu-time]
hard-limit = 10000
soft-limit = 1000

[settings.oci-defaults.resource-limits.max-data-size]
hard-limit = 1000000000
soft-limit = 100000000

[settings.oci-defaults.resource-limits.max-file-locks]
hard-limit = 2048
soft-limit = 1024

[settings.oci-defaults.resource-limits.max-file-size]
hard-limit = 204800
soft-limit = 102400

[settings.oci-defaults.resource-limits.max-locked-memory]
hard-limit = 1000000000
soft-limit = 100000000

[settings.oci-defaults.resource-limits.max-msgqueue-size]
hard-limit = 1000000001
soft-limit = 1000000000

[settings.oci-defaults.resource-limits.max-nice-priority]
hard-limit = 7
soft-limit = 6

[settings.oci-defaults.resource-limits.max-open-files]
hard-limit = 1048576
soft-limit = 65536

[settings.oci-defaults.resource-limits.max-pending-signals]
hard-limit = 1000
soft-limit = 500

[settings.oci-defaults.resource-limits.max-processes]
hard-limit = 10000
soft-limit = 9999

[settings.oci-defaults.resource-limits.max-realtime-priority]
hard-limit = 1000000000
soft-limit = 100000000

[settings.oci-defaults.resource-limits.max-realtime-timeout]
hard-limit = 1000000000
soft-limit = 100000000

[settings.oci-defaults.resource-limits.max-resident-set]
hard-limit = 1000000000
soft-limit = 100000000

[settings.oci-defaults.resource-limits.max-stack-size]
hard-limit = 1000000000
soft-limit = 100000000
EOF

Which results in /etc/containerd/cri-base.json containing:

"rlimits": [{ "type": "RLIMIT_SIGPENDING", "hard": 1000, "soft": 500 },
{ "type": "RLIMIT_RTPRIO", "hard": 1000000000, "soft": 100000000 },
{ "type": "RLIMIT_RSS", "hard": 1000000000, "soft": 100000000 },
{ "type": "RLIMIT_NICE", "hard": 7, "soft": 6 },
{ "type": "RLIMIT_CORE", "hard": 2000000000, "soft": 1000000000 },
{ "type": "RLIMIT_NPROC", "hard": 10000, "soft": 9999 },
{ "type": "RLIMIT_AS", "hard": 2000000000, "soft": 1000000000 },
{ "type": "RLIMIT_RTTIME", "hard": 1000000000, "soft": 100000000 },
{ "type": "RLIMIT_NOFILE", "hard": 1048576, "soft": 65536 },
{ "type": "RLIMIT_STACK", "hard": 1000000000, "soft": 100000000 },
{ "type": "RLIMIT_CPU", "hard": 10000, "soft": 1000 }, 
{ "type": "RLIMIT_MEMLOCK", "hard": 1000000000, "soft": 100000000 },
{ "type": "RLIMIT_FSIZE", "hard": 204800, "soft": 102400 },
{ "type": "RLIMIT_MSGQUEUE", "hard": 1000000001, "soft": 1000000000 },
{ "type": "RLIMIT_LOCKS", "hard": 2048, "soft": 1024 },
{ "type": "RLIMIT_DATA", "hard": 1000000000, "soft": 100000000 }]

I do think you will need an AddPrefixesMigration migration though since these values didn't exist before. Might be good to confirm that with someone else that has a stronger grasp of our migration/datastore handling though.

@cbgbt
Copy link
Contributor

cbgbt commented Jun 16, 2023

I do think you will need an AddPrefixesMigration

That's correct. Consider the following workflow:

  • You are running Bottlerocket X
  • You upgrade to Y, which adds these new RLIMIT attributes to the model
  • You decide to set settings.oci-defaults.resource-limits.max-resident-set
  • You downgrade Bottlerocket from Y back to X. Version X of Bottlerocket doesn't know what max-resident-set is, so it fails to load the model.

An AddPrefixesMigration ensures that any new settings prefixes which won't be understood on older versions of Bottlerocket are removed on downgrade. You'd need to specify one of these migrations for each of the new types added (All in the same binary, AddPrefixesMigration accepts a list.)

Copy link
Contributor

@etungsten etungsten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice! 🚀

@stmcginnis
Copy link
Contributor

This can now be rebased on the latest develop to pick up a pin of the rust toolchain version used on the GitHub worker nodes. That should allow the checks to pass. This is a temporary workaround until the SDK is bumped to use rust 1.70.0.

@vyaghras vyaghras force-pushed the add-oci-defualts-resource-limit-settings branch 2 times, most recently from 42a8442 to 716aeb3 Compare June 21, 2023 14:59
@vyaghras vyaghras requested a review from yeazelm June 21, 2023 15:14
@vyaghras vyaghras force-pushed the add-oci-defualts-resource-limit-settings branch 3 times, most recently from 66ae8f0 to 5f0ce28 Compare June 22, 2023 21:09
Copy link
Contributor

@yeazelm yeazelm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. I do wonder if we do any validation on these values. For example, setting a soft limit above the hard limit is not possible or setting a value for nice outside of -20 - 19. I'm not sure what we would do in those cases, but its worth considering if there is anything we would like to do there.

Copy link
Contributor

@bcressey bcressey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be nice to confirm that -1 (for "infinity" / "unlimited") is accepted everywhere, or else to document what the right value to use is in order to get that behavior:

[settings.oci-defaults.resource-limits.max-address-space]
hard-limit = -1
soft-limit = -1

I'm not confident that -1 will deserialize into u32::MAX, and I could see someone wanting to raise or remove a limit at runtime that they specified at launch time. That could be a problem since we don't allow settings to be deleted.

Release.toml Outdated Show resolved Hide resolved
sources/models/src/modeled_types/oci_defaults.rs Outdated Show resolved Hide resolved
sources/models/src/modeled_types/oci_defaults.rs Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@vyaghras vyaghras force-pushed the add-oci-defualts-resource-limit-settings branch 5 times, most recently from 83f7609 to 38cc70d Compare July 14, 2023 21:31
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@vyaghras vyaghras force-pushed the add-oci-defualts-resource-limit-settings branch 5 times, most recently from 29cf6c5 to 9069667 Compare July 20, 2023 17:19
Copy link
Contributor

@bcressey bcressey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM. One nit on the doc table, and I still have that open question about how to specify "unlimited" via settings, and making sure that works.

README.md Outdated Show resolved Hide resolved
@vyaghras vyaghras force-pushed the add-oci-defualts-resource-limit-settings branch from cbcd50a to 491034b Compare July 26, 2023 20:03
@vyaghras
Copy link
Contributor Author

vyaghras commented Jul 26, 2023

Code LGTM. One nit on the doc table, and I still have that open question about how to specify "unlimited" via settings, and making sure that works.

Now we can use "unlimited" string to set any rlimit as unlimited.

README.md Outdated Show resolved Hide resolved
sources/api/schnauzer/src/helpers.rs Outdated Show resolved Hide resolved
sources/models/src/modeled_types/oci_defaults.rs Outdated Show resolved Hide resolved
sources/models/src/modeled_types/oci_defaults.rs Outdated Show resolved Hide resolved
sources/models/src/modeled_types/oci_defaults.rs Outdated Show resolved Hide resolved
sources/models/src/modeled_types/oci_defaults.rs Outdated Show resolved Hide resolved
@vyaghras vyaghras force-pushed the add-oci-defualts-resource-limit-settings branch 2 times, most recently from c006f7a to 5a6ae24 Compare July 27, 2023 17:56
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
sources/api/schnauzer/src/helpers.rs Outdated Show resolved Hide resolved
sources/models/src/lib.rs Outdated Show resolved Hide resolved
sources/models/src/modeled_types/oci_defaults.rs Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@vyaghras vyaghras force-pushed the add-oci-defualts-resource-limit-settings branch 2 times, most recently from cb3e8b9 to 2da9108 Compare August 2, 2023 23:31
@vyaghras
Copy link
Contributor Author

vyaghras commented Aug 2, 2023

Allow range of 0- i64::Max as limit and -1 and "unlimited" string to mark any limit as unlimited.

@vyaghras vyaghras force-pushed the add-oci-defualts-resource-limit-settings branch 3 times, most recently from 660c0c8 to 53ae3aa Compare August 3, 2023 18:53
Copy link
Contributor

@bcressey bcressey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice. Some of the documentation and code can be tightened up to convey the essential information and avoid repetition, but the logic looks solid.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
sources/models/src/de.rs Outdated Show resolved Hide resolved
sources/models/src/de.rs Outdated Show resolved Hide resolved
sources/models/src/lib.rs Outdated Show resolved Hide resolved
@vyaghras vyaghras force-pushed the add-oci-defualts-resource-limit-settings branch from 53ae3aa to cf092d7 Compare August 4, 2023 19:40
Copy link
Contributor

@bcressey bcressey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a few nits.

README.md Outdated Show resolved Hide resolved
sources/models/src/lib.rs Outdated Show resolved Hide resolved
@vyaghras vyaghras force-pushed the add-oci-defualts-resource-limit-settings branch from cf092d7 to 664b49e Compare August 7, 2023 17:01
Add following rlimits settings for k8s variants, to enable updating
these from api client.
MaxAddressSpace,
MaxCoreFileSize,
MaxCpuTime,
MaxDataSize,
MaxFileLocks,
MaxFileSize,
MaxLockedMemory,
MaxMsgqueueSize,
MaxNicePriority,
MaxPendingSignals,
MaxProcesses,
MaxRealtimePriority,
MaxRealtimeTimeout,
MaxResidentSet,
MaxStackSize
This migration will handle the added resource limits
settings added for k8s variants. The migration
oci-defaults-max-open-files handles the limit values
on upgrade and downgrade. If RLIMIT_NOFILE is unlimited
or more than u32::Max, then it will changed to max possible value in
version lesser than 1.15.0.
@vyaghras vyaghras force-pushed the add-oci-defualts-resource-limit-settings branch from 664b49e to 04e2201 Compare August 7, 2023 17:02
@vyaghras
Copy link
Contributor Author

vyaghras commented Aug 7, 2023

I have tested this change again with all the migrations. The change is working as expected.

@vyaghras vyaghras merged commit 29c6f60 into bottlerocket-os:develop Aug 7, 2023
42 checks passed
@vyaghras vyaghras deleted the add-oci-defualts-resource-limit-settings branch August 7, 2023 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose additional oci-defaults.resource-limits settings
7 participants