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

Deprecate ECS settings applier #3834

Merged

Conversation

arnaldo2792
Copy link
Contributor

@arnaldo2792 arnaldo2792 commented Mar 20, 2024

Issue number:

Closes #3621

Description of changes:

This deprecates the ECS settings applier and migrates all the ECS configurations to use systemd drop-ins.

The ecs.config template file was removed, and replaced with systemd drop-ins for the ecs.service. This will allow downstream builders consume the configurations and build upon them if needed. The new configuration files include:

  • 00-defaults.conf: immutable configuration file with the default hard-coded configurations used for Bottlerocket
  • 10-base.conf: mutable configuration file generated by the API server
  • 20-nvidia.conf: immutable configuration file with the hard-coded configuration to enable NVIDIA support

With this PR, the ecs-agent-base and ecs-agent-nvidia packages were created to pull down the correct configuration while building the variant. Variants were updated accordingly to use the respective file.

In the new ecs-base-conf template file, most of the {{#if}} statements were removed in favor of using helpers like default. There are two ECS boolean configurations in the API system whose values are inverted:

  • ECS_PRIVILEGED_DISABLED -> allow-privileged-containers
  • ECS_DISABLE_IMAGE_CLEANUP -> image-cleanup-enabled

I tried to use the native not helper to generate the correct values for the settings, e.g. {{not (default <setting>)}}. However, the default helper will transform the boolean types to string when passed to the not helper. This results in the not helper always returning true, since not-empty strings are considered "truthy". I added the negate_or_else handlebars helper, which will render the passed configuration or a default value.

Testing done:

For aws-ecs-2:

  • Run internal suite of tests
  • Test ECS_TASK_METADATA_RPS_LIMIT
  • Test that both ECS_PRIVILEGED_DISABLED and ECS_DISABLE_IMAGE_CLEANUP are set as expected
  • Test that the value for ECS_AVAILABLE_LOGGING_DRIVERS is a valid JSON string, and that the ECS agent parses it
  • Test ECS_ENABLE_CONTAINER_METADATA
bash-5.1# docker exec 7b059bad5345 sh -c '[ -f ${ECS_CONTAINER_METADATA_FILE} ] && echo Exists!'                                                                                                                                                                                          
Exists!
  • Migration testing
    • Verified all the configuration files exist on upgrade
    • Verified both ecs.config and ecs.config.json exist on downgrade

For aws-ecs-1:

  • Run internal suite of tests

All the other tests are omitted since the configuration file and API definition are the same for both variants.

For aws-ecs-2-nvidia:

  • Run smoke test

For aws-ecs-1-nvidia:

  • Run smoke test

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.

@arnaldo2792
Copy link
Contributor Author

(Forced push added unit tests)

@arnaldo2792
Copy link
Contributor Author

(forced push adds the migration and fixes the implementation of one of the schnauzer helpers)

@arnaldo2792 arnaldo2792 marked this pull request as ready for review March 21, 2024 22:14
packages/ecs-agent/ecs-agent.spec Outdated Show resolved Hide resolved
packages/ecs-agent/ecs-agent.spec Outdated Show resolved Hide resolved
packages/ecs-agent/ecs.config.in Outdated Show resolved Hide resolved
packages/ecs-agent/ecs.config.in Outdated Show resolved Hide resolved
sources/api/schnauzer/src/helpers.rs Outdated Show resolved Hide resolved
packages/ecs-agent/ecs.config.in 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 Outdated Show resolved Hide resolved
sources/api/schnauzer/src/helpers.rs Outdated Show resolved Hide resolved
@arnaldo2792
Copy link
Contributor Author

Forced push includes:

  • Use drop-ins instead of configuration files
  • Remove unnecessary handlebars helpers
  • Split commits that add handlebars helpers

@arnaldo2792
Copy link
Contributor Author

(forced push includes rebase to resolve conflicts)

@arnaldo2792
Copy link
Contributor Author

Forced push to address one comment about the ecs_metadata_service_limits helper.

packages/ecs-agent/ecs-agent.spec Outdated Show resolved Hide resolved
packages/ecs-agent/ecs-agent.spec Outdated Show resolved Hide resolved
packages/ecs-agent/ecs-agent.spec Outdated Show resolved Hide resolved
packages/ecs-agent/ecs-tmpfiles.conf Outdated Show resolved Hide resolved
packages/systemd/systemd-tmpfiles.conf Outdated Show resolved Hide resolved
Environment=ECS_LOGLEVEL="{{settings.ecs.loglevel}}"
Environment=ECS_NUM_IMAGES_DELETE_PER_CYCLE="{{default 5 settings.ecs.image-cleanup-delete-per-cycle}}"
Environment=ECS_RESERVED_MEMORY="{{default 0 settings.ecs.reserved-memory}}"
Environment=ECS_TASK_METADATA_RPS_LIMIT="{{ecs_metadata_service_limits settings.ecs.metadata-service-rps settings.ecs.metadata-service-burst}}"
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency with the rest of the template, where the defaults are visible, you could pass the defaults here as well, e.g.:

Suggested change
Environment=ECS_TASK_METADATA_RPS_LIMIT="{{ecs_metadata_service_limits settings.ecs.metadata-service-rps settings.ecs.metadata-service-burst}}"
Environment=ECS_TASK_METADATA_RPS_LIMIT="{{ecs_metadata_service_limits 40 60 settings.ecs.metadata-service-rps settings.ecs.metadata-service-burst}}"

sources/api/schnauzer/src/helpers.rs Outdated Show resolved Hide resolved
@arnaldo2792
Copy link
Contributor Author

Forced push includes:

  • Fix description in subpackage for NVIDIA
  • Remove unnecessary lines in tmpfiles.d
  • Renamed base package to config
  • Renamed default variable in helper to fallback

@arnaldo2792
Copy link
Contributor Author

(Forced push includes a missing fix to rename nvidia to nvidia-config)

This adds the new `negate_or_else` helper which takes a boolean value
and applies a boolean not to it.

Signed-off-by: Arnaldo Garcia Rincon <[email protected]>
This adds the `ecs_metadata_service_limits` template helper used to render
properly formatted values for the `ECS_METADATA_SERVICE_LIMITS`
configuration for the ECS agent.

Signed-off-by: Arnaldo Garcia Rincon <[email protected]>
With this, all the API settings to configure the ECS agent are now
created as systemd drop-in files. Two subpackages were created to
modularize the configuration.

Signed-off-by: Arnaldo Garcia Rincon <[email protected]>
Now that all the configurations are included in systemd drop-ins, this
binary isn't needed

Signed-off-by: Arnaldo Garcia Rincon <[email protected]>
@arnaldo2792
Copy link
Contributor Author

(Forced push to rebase onto develop)

@arnaldo2792 arnaldo2792 requested a review from yeazelm April 4, 2024 13:40
@arnaldo2792 arnaldo2792 merged commit aef8fdf into bottlerocket-os:develop Apr 4, 2024
35 checks passed
@ginglis13 ginglis13 mentioned this pull request May 7, 2024
14 tasks
@arnaldo2792 arnaldo2792 deleted the ecs-settings-applier-remove branch September 5, 2024 17:56
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.

OOTB: Remove conditional compilation and model dependency from ecs-settings-applier
4 participants