Repository navigation
storewolf: preserve parent settings when applying defaults - #1073
xiaotianlyu wants to merge 1 commit into
Conversation
Skip child defaults when the datastore already contains their parent setting. This preserves legacy aggregate values during migration. Signed-off-by: Melody Lyu <tianlyu@amazon.com>
5f53328 to
1bf2baf
Compare
xiaotianlyu
changed the title
| Ok(()) | ||
| } | ||
|
|
||
| fn key_or_ancestor_populated(key: &Key, existing_data: &HashSet<Key>) -> bool { |
There was a problem hiding this comment.
So if we have a.b in datastore, will it skip adding a.b.c in the datastore? Skipping all descendants when an ancestor exists will silently drop defaults for genuinely-new child settings (e.g. a future host-containers.control.user-data).
| Ok(()) | ||
| } | ||
|
|
||
| fn key_or_ancestor_populated(key: &Key, existing_data: &HashSet<Key>) -> bool { |
There was a problem hiding this comment.
Use the existing Key::starts_with_segments instead of raw-string prefix matching — it's correct by structure, not by string coincidence, and avoids a second divergent prefix mechanism.
| } | ||
|
|
||
| fn key_or_ancestor_populated(key: &Key, existing_data: &HashSet<Key>) -> bool { | ||
| existing_data.iter().any(|existing_key| { |
There was a problem hiding this comment.
This compares each default key against every existing key on every boot, so the cost is existing keys x default keys. Could we walk the default key's ancestors and look each one up in the HashSet instead?
|
Closing this PR because we agreed to retain the legacy NTP defaults and make the forward migration a no-op. With that approach, this Storewolf change is no longer needed. |
3 participants
Description of changes:
Storewolf currently skips a default only when the exact datastore key already exists. During an upgrade, an existing setting may use a parent key while the new defaults use named child keys.
For example, bottlerocket-os/bottlerocket#4923 migrates the legacy
settings.ntp.time-serverslist to named settings undersettings.ntp.time-servers.<name>.*. Storewolf would otherwise add the new default servers when the legacy parent key already exists. This can cause custom NTP configurations or an empty server list to receivedefault servers during an upgrade.
Update Storewolf to skip child defaults when an ancestor key already exists. Unrelated defaults continue to be populated normally.
Testing done:
cargo test -p storewolf skips_descendants_of_existing_settingcargo clippy -p storewolf --all-targets -- -D warningscargo fmt --manifest-path sources/Cargo.toml --all -- --checkgit diff --checkTerms 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.