-
Notifications
You must be signed in to change notification settings - Fork 519
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
bootstrap-containers: migrate to using configuration file #3724
bootstrap-containers: migrate to using configuration file #3724
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't there is systemd unit that needs to change in order to pass the new argument?
sources/api/migration/migrations/v1.18.1/bootstrap-containers-config-file-v0-1-0/src/main.rs
Outdated
Show resolved
Hide resolved
Let's merge this first #3768 then rebase your PR to align on 1.19.2 |
77e9407
to
60c3928
Compare
rebase on latest |
60c3928
to
65bc4fc
Compare
Finished changes and testing run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verified this commit from the testing report 65bc4fc7
. 👍
65bc4fc
to
e5bf382
Compare
Addressed comments |
bootstrap-containers = "v1" | ||
+++ | ||
{{#if settings.bootstrap-containers}} | ||
{{#each settings.bootstrap-containers}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be more readable (and probably more maintainable in the long run) to have this rendered by a handlebars helper? e.g.
{{bootstrap-containers settings.bootstrap-containers}}
Then you can just use Rust and strong typing to generate the toml tables. . I want us to avoid another "generate on-the-flight complex structures" and use the Rust to guarantee the correctness of the serialized format. (@bcressey what do you think?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this approach, this could just be a generic settings-to-toml
helper which we could use for all of these PRs where we substitute reading from the API for reading from a config file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be more readable (and probably more maintainable in the long run) to have this rendered by a handlebars helper?
It doesn't really help in the long run where the goal is to decouple templates and settings extensions so they can be maintained independently.
I like this approach, this could just be a generic settings-to-toml helper which we could use for all of these PRs where we substitute reading from the API for reading from a config file
I wasn't too keen on settings-to-json
when that put in an appearance recently.
I am a little more positively inclined on something like settings-to-toml
if the helper is producing a 1:1 match with a subset of the external system settings API, if only because that API is covered by a strong covenant with the end user that it won't change.
However I'm still not very enthusiastic. I'd like most of these agents to be able to be configured in ways that aren't 100% reflected in the external API - like kubelet
and ecs-agent
are not - since some behaviors may make sense to hard-code in certain variants.
Ultimately an agent whose config is nothing but the external API is in a precarious position: it makes more sense to me to have use a combination of Bottlerocket features and systemd functionality to dispense with that agent entirely (as with ecs-settings-applier
in the linked PR) where we can, than to add helpers which special-case just that agent rather than answering a more general need.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove bear.toml
e5bf382
to
ad83e08
Compare
Still need to remove tokio |
ad83e08
to
f7f12a5
Compare
removed tokio |
4a467fa
to
b40aebb
Compare
Rebased off latest and moved migrations to 1.20.0 will rerun the tests to be safe |
This migrates the bootstrap-containers tool to fetch its settings via a generated toml file. log: * add migrations and package changes * migrate bootstrap-containers to load from config file * add mount with secret_t
b40aebb
to
bcfb715
Compare
Release.toml was wrong resulting in migrations not occuring, fixed this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with one additional testing callout.
apiclient::set::set(socket_path, &settings) | ||
.await | ||
.context(error::SetSnafu)?; | ||
command("apiclient", ["set", formatted.as_str()])?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you verify that this actually works as expected? I.e. a bootstrap container that is set to "once" mode only runs once on first boot and not multiple times - either on the same boot (which can happen when the systemd target changes) or on subsequent boots.
I'd suggest a bootstrap container that runs something like:
#!/bin/bash
TS="$(date +%s)"
apiclient get settings.bootstrap-containers >
/.bottlerocket/rootfs/local/${TS}.txt
And confirm that only one file is created and it shows the expected value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and it only returned one file, logs also show correct behavior
Issue number: #3626
Closes #3626
Description of changes:
This migrates the bootstrap-containers tool to fetch its settings via a generated toml file.
log:
Testing done:
cargo +nightly udeps
to ensure all unused dependencies have been removedTest Report
Initial_Build_ID.Output
Initial_Config.Output
configuration-files.bootstrap-containers-toml
services.bootstrap-containers
Downgrade.Output
Downgrade_Build_ID.Output
Downgrade_Config.Output
configuration-files.bootstrap-containers-toml
services.bootstrap-containers
Upgrade.Output
Upgrade_Build_ID.Output
Upgrade_Config.Output
configuration-files.bootstrap-containers-toml
services.bootstrap-containers
Change_Settings.Output
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.