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.

Sign up for GitHub

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

Jump to bottom

Support settings oci defaults on ecs variants #3259

Merged
merged 3 commits into from
Aug 16, 2023
Merged

Support settings oci defaults on ecs variants #3259

merged 3 commits into from
Aug 16, 2023

Conversation

vyaghras
Copy link
Contributor

@vyaghras vyaghras commented Jul 12, 2023 •
edited
Loading

Issue number:

Closes #2759

Description of changes:
Currently in bottlerocket we have support for setting OCI defaults in orchestrator-launched pods using api. (Refer #1703 #2404 #2697 ). The settings we added in api are:

  • settings.oci-defaults.resource-limits
  • settings.oci-defaults.capabilities

But currently in bottlerocket these setting do not work for ECS variants in same way as EKS variants because ECS variants uses docker.(Moby package)
This PR will enable the support for these setting for ecs variants.

The priority for these settings are daemon.json => Hostconfig, meaning if any configuration change has been done in HostConfig for Capabilities and Resource limits, it should get preference over the setting done by daemon.json.

Testing done:

  • Set a capability true using api and create container using docker, that capability should be added in that container.
  • Change the soft limit and hard limit of already set rlimit and create a container, the limit should be the changed one.
  • Set the defaults limits using daemon.json file, Create the container, it should use the one set by default ulimit.
  • Change something in host config and create a container, that container should have the requested settings from host config.
  • Migration testing to and from 1.15.0
  • If nothing set in host config for rlimit then it should use the set in daemon.json using api client
  • Perform above testing for ecs-1-nvidia instance.
  • Task definition should override the daemon config ulimits

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.

stmcginnis
stmcginnis suggested changes Jul 13, 2023
View reviewed changes
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.

A few minor things, but looks pretty good overall.

packages/docker-engine/0001-read-default-oci-spec-from-file.patch Outdated Show resolved Hide resolved
packages/docker-engine/0001-read-default-oci-spec-from-file.patch Outdated Show resolved Hide resolved
packages/docker-engine/0001-read-default-oci-spec-from-file.patch Outdated Show resolved Hide resolved
Release.toml Outdated Show resolved Hide resolved
packages/docker-engine/daemon-json Outdated Show resolved Hide resolved
packages/docker-engine/daemon-nvidia-json Outdated Show resolved Hide resolved
yeazelm
yeazelm reviewed Jul 13, 2023
View reviewed changes
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.

I'd like to clean up the commit messages and order a bit as well.

packages/docker-engine/0001-read-default-oci-spec-from-file.patch Outdated Show resolved Hide resolved
packages/docker-engine/0001-read-default-oci-spec-from-file.patch Outdated Show resolved Hide resolved
packages/docker-engine/0001-read-default-oci-spec-from-file.patch Outdated Show resolved Hide resolved
packages/docker-engine/0001-read-default-oci-spec-from-file.patch Outdated Show resolved Hide resolved
packages/docker-engine/0001-read-default-oci-spec-from-file.patch Outdated Show resolved Hide resolved
sources/models/shared-defaults/oci-defaults-docker-cri-resource-limits.toml Outdated Show resolved Hide resolved
@vyaghras vyaghras force-pushed the support_settings_oci_defaults_on_ecs_variants branch 3 times, most recently from bddf1b7 to c2684d8 Compare July 13, 2023 21:44
@vyaghras vyaghras force-pushed the support_settings_oci_defaults_on_ecs_variants branch from c2684d8 to a5768cf Compare July 14, 2023 18:46
@vyaghras vyaghras force-pushed the support_settings_oci_defaults_on_ecs_variants branch 5 times, most recently from c6f840b to 50f0e28 Compare July 20, 2023 16:56
@vyaghras vyaghras force-pushed the support_settings_oci_defaults_on_ecs_variants branch 5 times, most recently from 01ae0fa to c6f55b1 Compare July 31, 2023 19:20
@vyaghras
Copy link
Contributor Author

Change patch to add "default-capabilities" in the daemon.json and update these default-capabilities and default-ulimits using apiclient settings.

@vyaghras vyaghras force-pushed the support_settings_oci_defaults_on_ecs_variants branch from c6f55b1 to 8e9dae8 Compare August 3, 2023 16:45
@vyaghras vyaghras force-pushed the support_settings_oci_defaults_on_ecs_variants branch 2 times, most recently from 1881e5a to 1ad752c Compare August 4, 2023 16:15
@vyaghras vyaghras force-pushed the support_settings_oci_defaults_on_ecs_variants branch 2 times, most recently from c8584ab to 63e498f Compare August 4, 2023 18:13
bcressey
bcressey reviewed Aug 4, 2023
View reviewed changes
packages/containerd/containerd-cri-base-json Outdated Show resolved Hide resolved
sources/api/schnauzer/src/helpers.rs Outdated Show resolved Hide resolved
sources/api/schnauzer/src/helpers.rs Outdated Show resolved Hide resolved
sources/models/shared-defaults/oci-defaults-docker-capabilities.toml Outdated Show resolved Hide resolved
sources/models/shared-defaults/oci-defaults-docker.toml Outdated Show resolved Hide resolved
sources/models/src/aws-ecs-1/mod.rs Show resolved Hide resolved
sources/api/schnauzer/src/helpers.rs Outdated Show resolved Hide resolved
sources/api/schnauzer/src/helpers.rs Outdated Show resolved Hide resolved
sources/api/schnauzer/src/helpers.rs Outdated Show resolved Hide resolved
packages/docker-engine/0001-Change-default-capabilities-using-daemon-config.patch Outdated Show resolved Hide resolved
@vyaghras vyaghras force-pushed the support_settings_oci_defaults_on_ecs_variants branch 4 times, most recently from fa74142 to 79f8b93 Compare August 9, 2023 22:03
webern
webern approved these changes Aug 11, 2023
View reviewed changes
Copy link
Contributor

@webern webern left a comment

Choose a reason for hiding this comment

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

Looks good.

sources/api/schnauzer/src/helpers.rs Show resolved Hide resolved
sources/api/schnauzer/src/helpers.rs Outdated Show resolved Hide resolved
sources/api/schnauzer/src/helpers.rs Outdated Show resolved Hide resolved
sources/api/schnauzer/src/helpers.rs Outdated Show resolved Hide resolved
sources/api/schnauzer/src/helpers.rs Show resolved Hide resolved
sources/models/shared-defaults/oci-defaults-docker-resource-limits.toml Show resolved Hide resolved
@vyaghras
migrations: add migrations for 'settings.oci-defaults'
a2d49da 
This migration will handle the added capabilities and resource limits
settings added for ecs variants.

Signed-off-by: Shikha Vyaghra <[email protected]>
@vyaghras
packages: add patch to read default capabilities
e7e2a92 
This patch is to override the capabilities in default runtime
spec(that is embedded in moby package code) by reading a
default-capabilities parameter passed using etc/docker/daemon.json
file. We can update the default capabilities string array using
api client, that in turn will take precedence over the default
capabilities in OCI Spec.

Signed-off-by: Shikha Vyaghra <[email protected]>
@vyaghras vyaghras force-pushed the support_settings_oci_defaults_on_ecs_variants branch 2 times, most recently from 183a85a to 03af410 Compare August 14, 2023 17:07
@bcressey bcressey mentioned this pull request Aug 14, 2023
Merged
12 tasks
@vyaghras vyaghras force-pushed the support_settings_oci_defaults_on_ecs_variants branch 2 times, most recently from b36629d to f6b6d7f Compare August 16, 2023 16:20
@vyaghras
api: enable setting rlimits and capabilities for ecs
bd2619e 
The fields default-capabilities and default-ulimits in etc/daemon.json
holds the OCI default capabilities and resource limits that has been
set using api-client respectively. These settings can be updated/added
using api-client.
@vyaghras vyaghras force-pushed the support_settings_oci_defaults_on_ecs_variants branch from f6b6d7f to bd2619e Compare August 16, 2023 16:28
@vyaghras vyaghras merged commit 947c178 into bottlerocket-os:develop Aug 16, 2023
42 checks passed
@vyaghras vyaghras deleted the support_settings_oci_defaults_on_ecs_variants branch August 16, 2023 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yeazelm yeazelm yeazelm approved these changes

@arnaldo2792 arnaldo2792 arnaldo2792 approved these changes

@webern webern webern approved these changes

@stmcginnis stmcginnis stmcginnis approved these changes

@bcressey bcressey bcressey approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

support settings.oci-defaults on ecs variants
6 participants
@vyaghras @bcressey @stmcginnis @webern @arnaldo2792 @yeazelm