-
Notifications
You must be signed in to change notification settings - Fork 124
install: Add reset #1389
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
base: main
Are you sure you want to change the base?
install: Add reset #1389
Conversation
Signed-off-by: Colin Walters <[email protected]>
This is a nondestructive variant of `to-existing-root`. Signed-off-by: Colin Walters <[email protected]>
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.
Code Review
The code changes introduce the install reset
command, a valuable addition for managing bootc systems. The refactoring to use MergeState
is a clean way to accommodate this new functionality alongside existing deployment logic. The new stateroot management functions in ostree-ext
are also well-structured and come with thorough tests. I've found a few issues that should be addressed: a leftover debugging macro, a potential panic from an assertion on user input, and a logic issue in how new stateroot names are generated when the year changes. My detailed comments and suggestions below aim to resolve these points to improve the robustness and quality of the code.
}; | ||
|
||
let booted_stateroot = booted_deployment.osname(); | ||
assert!(booted_stateroot.as_str() != target_stateroot); |
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.
Using assert!
here is risky because target_stateroot
can be user-provided. If a user specifies the same name as the booted stateroot, this will cause a panic in release builds. It's better to use anyhow::ensure!
to return a graceful error message.
anyhow::ensure!(
booted_stateroot.as_str() != target_stateroot,
"Target stateroot cannot be the same as the booted one"
);
let (year, serial) = stateroots | ||
.iter() | ||
.filter_map(|v| { | ||
if let StaterootKind::Auto(v) = v.kind { | ||
Some(v) | ||
} else { | ||
None | ||
} | ||
}) | ||
.max() | ||
.map(|(year, serial)| (year, serial + 1)) | ||
.unwrap_or((current_year, 0)); |
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.
The current logic for allocating a new stateroot name can produce incorrect results when the year changes. For example, if the latest stateroot is from 2023 (e.g., state-2023-5
) and the current year is 2024, this logic will generate state-2023-6
instead of state-2024-0
.
The logic should reset the serial to 0 when the year advances. A more robust implementation would also handle cases where the system clock is set to a future time.
Here's a suggested improvement that handles year changes correctly:
let (year, serial) = stateroots | |
.iter() | |
.filter_map(|v| { | |
if let StaterootKind::Auto(v) = v.kind { | |
Some(v) | |
} else { | |
None | |
} | |
}) | |
.max() | |
.map(|(year, serial)| (year, serial + 1)) | |
.unwrap_or((current_year, 0)); | |
let (year, serial) = stateroots | |
.iter() | |
.filter_map(|v| { | |
if let StaterootKind::Auto(v) = v.kind { | |
Some(v) | |
} else { | |
None | |
} | |
}) | |
.max() | |
.map(|(found_year, found_serial)| { | |
if found_year < current_year { | |
(current_year, 0) | |
} else { | |
(found_year, found_serial + 1) | |
} | |
}) | |
.unwrap_or((current_year, 0)); |
crate::status::get_status_require_booted(sysroot)?; | ||
|
||
let stateroots = list_stateroots(sysroot)?; | ||
dbg!(&stateroots); |
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.
This is a nondestructive variant of
to-existing-root
.