daemon: read-copy-update the daemon config#43980
Conversation
6f1ae80 to
50eaf9f
Compare
556565e to
9f1d591
Compare
9f1d591 to
877a36d
Compare
877a36d to
a355206
Compare
a355206 to
2c41cf3
Compare
tianon
left a comment
There was a problem hiding this comment.
The implementation details look pretty hairy, but it does seem to be pretty comprehensive (including new tests 👀)
daemon/config/registry.go
Outdated
| ) | ||
|
|
||
| // RegistryHosts returns registry configuration in containerd resolvers format. | ||
| func (conf *Config) RegistryHosts() docker.RegistryHosts { |
There was a problem hiding this comment.
Reviewer's note: this is just moved from daemon/daemon.go
There was a problem hiding this comment.
Sadly, I had to move it back. On a positive note, reloaded registry configuration will now be picked up by Buildkit!
| // A non-standard Base32 encoding which lacks vowels to avoid accidentally | ||
| // spelling naughty words. Don't use this to encode any data which requires | ||
| // compatibility with anything outside of the currently-running process. | ||
| var base32Disemvoweled = base32.NewEncoding("0123456789BCDFGHJKLMNPQRSTVWXYZ-") |
There was a problem hiding this comment.
😂 ❤️
(Should this also remove "vowel looking" numbers like 0, 1, 3, 4?)
|
This LGTM, sadly a nasty rebase. |
4be5647 to
9007e62
Compare
|
It was a nasty rebase, though on the bright side, I spotted and fixed some mistakes in the process and made some improvements. |
b0c1a58 to
a8fb1df
Compare
| backend Backend | ||
| daemon experimentalProvider | ||
| routes []router.Route | ||
| features *map[string]bool |
There was a problem hiding this comment.
👀 🤦
I totally missed that this was happening.
|
discussing with @corhere - he wants to look at splitting out one fix from this PR to make it back-portable, so let's wait with merging for that. |
5c45f9e to
2340c39
Compare
|
@thaJeztah all done: the first two commits are now the backport-able fixes. I went through the diffs again to confirm that those are the only changes worth backporting. |
27fc3db to
c1caa19
Compare
Passing around a bare pointer to the map of configured features in order to propagate to consumers changes to the configuration across reloads is dangerous. Map operations are not atomic, so concurrently reading from the map while it is being updated is a data race as there is no synchronization. Use a getter function to retrieve the current features map so the features can be retrieved race-free. Remove the unused features argument from the build router. Signed-off-by: Cory Snider <[email protected]>
Historically, daemon.RegistryHosts() has returned a docker.RegistryHosts callback function which closes over a point-in-time snapshot of the daemon configuration. When constructing the BuildKit builder at daemon startup, the return value of daemon.RegistryHosts() has been used. Therefore the BuildKit builder would use the registry configuration as it was at daemon startup for the life of the process, even if the registry configuration is changed and the configuration reloaded. Provide BuildKit with a RegistryHosts callback which reflects the live daemon configuration after reloads so that registry operations performed by BuildKit always use the same configuration as the rest of the daemon. Signed-off-by: Cory Snider <[email protected]>
Config reloading has interleaved validations and other fallible operations with mutating the live daemon configuration. The daemon configuration could be left in a partially-reloaded state if any of the operations returns an error. Mutating a copy of the configuration and atomically swapping the config struct on success is not currently an option as config values are not copyable due to the presence of sync.Mutex fields. Introduce a two-phase commit protocol to defer any mutations of the daemon state until after all fallible operations have succeeded. Reload transactions are not yet entirely hermetic. The platform reloading logic for custom runtimes on *nix could still leave the directory of generated runtime wrapper scripts in an indeterminate state if an error is encountered. Signed-off-by: Cory Snider <[email protected]>
Ensure data-race-free access to the daemon configuration without locking by mutating a deep copy of the config and atomically storing a pointer to the copy into the daemon-wide configStore value. Any operations which need to read from the daemon config must capture the configStore value only once and pass it around to guarantee a consistent view of the config. Signed-off-by: Cory Snider <[email protected]>
The existing runtimes reload logic went to great lengths to replace the directory containing runtime wrapper scripts as atomically as possible within the limitations of the Linux filesystem ABI. Trouble is, atomically swapping the wrapper scripts directory solves the wrong problem! The runtime configuration is "locked in" when a container is started, including the path to the runC binary. If a container is started with a runtime which requires a daemon-managed wrapper script and then the daemon is reloaded with a config which no longer requires the wrapper script (i.e. some args -> no args, or the runtime is dropped from the config), that container would become unmanageable. Any attempts to stop, exec or otherwise perform lifecycle management operations on the container are likely to fail due to the wrapper script no longer existing at its original path. Atomically swapping the wrapper scripts is also incompatible with the read-copy-update paradigm for reloading configuration. A handler in the daemon could retain a reference to the pre-reload configuration for an indeterminate amount of time after the daemon configuration has been reloaded and updated. It is possible for the daemon to attempt to start a container using a deleted wrapper script if a request to run a container races a reload. Solve the problem of deleting referenced wrapper scripts by ensuring that all wrapper scripts are *immutable* for the lifetime of the daemon process. Any given runtime wrapper script must always exist with the same contents, no matter how many times the daemon config is reloaded, or what changes are made to the config. This is accomplished by using everyone's favourite design pattern: content-addressable storage. Each wrapper script file name is suffixed with the SHA-256 digest of its contents to (probabilistically) guarantee immutability without needing any concurrency control. Stale runtime wrapper scripts are only cleaned up on the next daemon restart. Split the derived runtimes configuration from the user-supplied configuration to have a place to store derived state without mutating the user-supplied configuration or exposing daemon internals in API struct types. Hold the derived state and the user-supplied configuration in a single struct value so that they can be updated as an atomic unit. Signed-off-by: Cory Snider <[email protected]>
The daemon has made a habit of mutating the DefaultRuntime and Runtimes values in the Config struct to merge defaults. This would be fine if it was a part of the regular configuration loading and merging process, as is done with other config options. The trouble is it does so in surprising places, such as in functions with 'verify' or 'validate' in their name. It has been necessary in order to validate that the user has not defined a custom runtime named "runc" which would shadow the built-in runtime of the same name. Other daemon code depends on the runtime named "runc" always being defined in the config, but merging it with the user config at the same time as the other defaults are merged would trip the validation. The root of the issue is that the daemon has used the same config values for both validating the daemon runtime configuration as supplied by the user and for keeping track of which runtimes have been set up by the daemon. Now that a completely separate value is used for the latter purpose, surprising contortions are no longer required to make the validation work as intended. Consolidate the validation of the runtimes config and merging of the built-in runtimes into the daemon.setupRuntimes() function. Set the result of merging the built-in runtimes config and default default runtime on the returned runtimes struct, without back-propagating it onto the config.Config argument. Signed-off-by: Cory Snider <[email protected]>
c1caa19 to
0f6eeec
Compare
- What I did
Made daemon config reloading transactional and almost entirely atomic. Config reloading will either succeed or leave the daemon state untouched. Reloading no longer pauses the world. Daemon operations see a consistent and immutable view of the daemon config, even when racing a reload.
- How I did it
config.Configstruct copyable by removing the embedded mutexesregistryService.ReplaceConfig()are not necessarily atomic w.r.t. the reload transaction as they can take effect before the updated config is applied toconfigStore- How to verify it
CI
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)