-
Notifications
You must be signed in to change notification settings - Fork 172
Bootuuid option #1978
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?
Bootuuid option #1978
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -368,6 +368,11 @@ pub(crate) struct InstallConfigOpts { | |
| /// The stateroot name to use. Defaults to `default`. | ||
| #[clap(long)] | ||
| pub(crate) stateroot: Option<String>, | ||
|
|
||
| /// Don't pass --write-uuid to bootupd during bootloader installation. | ||
| #[clap(long)] | ||
| #[serde(default)] | ||
| pub(crate) skip_boot_uuid_stamp: bool, | ||
| } | ||
|
|
||
| #[derive(Debug, Default, Clone, clap::Parser, Serialize, Deserialize, PartialEq, Eq)] | ||
|
|
@@ -1512,7 +1517,7 @@ async fn verify_target_fetch( | |
|
|
||
| /// Preparation for an install; validates and prepares some (thereafter immutable) global state. | ||
| async fn prepare_install( | ||
| config_opts: InstallConfigOpts, | ||
| mut config_opts: InstallConfigOpts, | ||
| source_opts: InstallSourceOpts, | ||
| target_opts: InstallTargetOpts, | ||
| mut composefs_options: InstallComposefsOpts, | ||
|
|
@@ -1637,8 +1642,15 @@ async fn prepare_install( | |
| } | ||
|
|
||
| let install_config = config::load_config()?; | ||
| if install_config.is_some() { | ||
| if let Some(ref config) = install_config { | ||
| tracing::debug!("Loaded install configuration"); | ||
| // Merge config file values into config_opts (CLI takes precedence) | ||
| // Only apply config file value if CLI didn't explicitly set it | ||
| if !config_opts.skip_boot_uuid_stamp { | ||
| if let Some(skip) = config.skip_boot_uuid_stamp { | ||
| config_opts.skip_boot_uuid_stamp = skip; | ||
| } | ||
| } | ||
|
Comment on lines
+1649
to
+1653
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This logic for merging the if !config_opts.skip_boot_uuid_stamp {
config_opts.skip_boot_uuid_stamp = config.skip_boot_uuid_stamp.unwrap_or(false);
} |
||
| } else { | ||
| tracing::debug!("No install configuration found"); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -85,6 +85,10 @@ pub(crate) struct InstallConfiguration { | |
| pub(crate) root_mount_spec: Option<String>, | ||
| /// Mount specification for the /boot filesystem. | ||
| pub(crate) boot_mount_spec: Option<String>, | ||
| /// Wether to skip writing the boot partition UUID to the bootloader configuration. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| /// When true, uses --with-static-configs instead of --write-uuid for bootupd. | ||
| /// Defaults to false if not specified (the boot UUID is written by default). | ||
| pub(crate) skip_boot_uuid_stamp: Option<bool>, | ||
| } | ||
|
|
||
| fn merge_basic<T>(s: &mut Option<T>, o: Option<T>, _env: &EnvProperties) { | ||
|
|
@@ -160,6 +164,11 @@ impl Mergeable for InstallConfiguration { | |
| merge_basic(&mut self.stateroot, other.stateroot, env); | ||
| merge_basic(&mut self.root_mount_spec, other.root_mount_spec, env); | ||
| merge_basic(&mut self.boot_mount_spec, other.boot_mount_spec, env); | ||
| merge_basic( | ||
| &mut self.skip_boot_uuid_stamp, | ||
| other.skip_boot_uuid_stamp, | ||
| env, | ||
| ); | ||
| if let Some(other_kargs) = other.kargs { | ||
| self.kargs | ||
| .get_or_insert_with(Default::default) | ||
|
|
@@ -731,4 +740,53 @@ boot-mount-spec = "" | |
| assert_eq!(install.root_mount_spec.as_deref().unwrap(), ""); | ||
| assert_eq!(install.boot_mount_spec.as_deref().unwrap(), ""); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_parse_skip_boot_uuid_stamp() { | ||
| // Test parsing true | ||
| let c: InstallConfigurationToplevel = toml::from_str( | ||
| r#"[install] | ||
| skip-boot-uuid-stamp = true | ||
| "#, | ||
| ) | ||
| .unwrap(); | ||
| assert_eq!(c.install.unwrap().skip_boot_uuid_stamp.unwrap(), true); | ||
|
|
||
| // Test parsing false | ||
| let c: InstallConfigurationToplevel = toml::from_str( | ||
| r#"[install] | ||
| skip-boot-uuid-stamp = false | ||
| "#, | ||
| ) | ||
| .unwrap(); | ||
| assert_eq!(c.install.unwrap().skip_boot_uuid_stamp.unwrap(), false); | ||
|
|
||
| // Test default (not specified) is None | ||
| let c: InstallConfigurationToplevel = toml::from_str( | ||
| r#"[install] | ||
| root-fs-type = "xfs" | ||
| "#, | ||
| ) | ||
| .unwrap(); | ||
| assert!(c.install.unwrap().skip_boot_uuid_stamp.is_none()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_merge_skip_boot_uuid_stamp() { | ||
| let env = EnvProperties { | ||
| sys_arch: "x86_64".to_string(), | ||
| }; | ||
| let mut install: InstallConfiguration = toml::from_str( | ||
| r#"skip-boot-uuid-stamp = false | ||
| "#, | ||
| ) | ||
| .unwrap(); | ||
| let other = InstallConfiguration { | ||
| skip_boot_uuid_stamp: Some(true), | ||
| ..Default::default() | ||
| }; | ||
| install.merge(other, &env); | ||
| // skip_boot_uuid_stamp should be overridden to true | ||
| assert_eq!(install.skip_boot_uuid_stamp.unwrap(), true); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,6 +33,9 @@ The `install` section supports these subfields: | |
| - `boot-mount-spec`: A string specifying the /boot filesystem mount specification. | ||
| If not provided and /boot is a separate mount, its UUID will be used. | ||
| An empty string signals to omit boot mount kargs entirely. | ||
| - `skip-boot-uuid-stamp`: A boolean that controls whether to skip writing partition UUIDs | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is actually specific to bootupd and does not apply with e.g. sdboot and DPS in general. I think it's also meaningless with s390x. I wonder if we should have a dedicated Now...then the next question is if we change things so that we error if bootupd isn't used. That would force cases like the FCOS image builds to conditionalize the install config per-arch/layout, but it does feel more right to me instead of silently ignoring it here, which is less compatible if we ever aded any other knobs
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another approach suggested by @jlebon was to hide that logic behind the Now about having it fail when not applicable.. I am not really opposed to it. It make the pipeline slightly more complex, but we already have different manifests for each arch so I guess that isn't too much more work. Final question: you said "bootupd", by that you mean grub ? Isn't bootupd support sd-boot at some point ?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes well the main argument is to support the shim + sdboot case actually but the uuid is really IMO a legacy for GRUB systems and newer things should use the DPS which we enforce with the sealeduki-composefs case. (I have a WIP to allow DPS with ostree+grub but it requires newer Fedora) |
||
| to the bootloader configuration. When `true`, bootupd is invoked with `--with-static-configs` | ||
| instead of `--write-uuid`. Defaults to `false` (UUIDs are written by default). | ||
|
|
||
| # filesystem | ||
|
|
||
|
|
||
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.
Also per below how about
--bootupd-skip-boot-uuid