From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id EB7E5D5BF for ; Thu, 21 Sep 2023 13:16:07 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id CE4401EC27 for ; Thu, 21 Sep 2023 13:16:07 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Thu, 21 Sep 2023 13:16:06 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 0FB5548656 for ; Thu, 21 Sep 2023 13:16:06 +0200 (CEST) Date: Thu, 21 Sep 2023 13:16:03 +0200 From: Christoph Heiss To: Aaron Lauterer Cc: Proxmox VE development discussion Message-ID: References: <20230905132832.3179097-1-a.lauterer@proxmox.com> <20230905132832.3179097-3-a.lauterer@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230905132832.3179097-3-a.lauterer@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL -0.188 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment POISEN_SPAM_PILL 0.1 Meta: its spam POISEN_SPAM_PILL_1 0.1 random spam to be learned in bayes POISEN_SPAM_PILL_3 0.1 random spam to be learned in bayes SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [answer.rs, proxmox.com, main.rs, global.country, utils.rs] Subject: Re: [pve-devel] [RFC installer 2/6] add proxmox-auto-installer X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 21 Sep 2023 11:16:08 -0000 Some general notes: - The overall approach seems fine too me. Did some cursory testing too, worked fine - although I did not really test out the filtering/matching much. - Probably just due to it being still a bit early, but all these `.unwrap()/.expect()` should be replaced with proper error handling in the end. - Continuing on the thought of error handling: We should come up with some way to properly report and (persistently) log errors, such that an user can see them. It's really not helpful if the machine is just boot-looping without any indication what went wrong. Any `.expect()`/`.unwrap()` will just panic the auto-installer, which will ultimately result in a reboot. Especially in this case, where everything runs headless, it's IMO important to have /some/ way to know what went wrong. Sending simple JSON-formatted logs to an HTTP endpoint or even using the rsyslog protocol come to mind and would be a good solution for this, I think. Or, if the answer file is read from an (USB drive) partition, write the log there? At the very least, I would log everything to a tmpfile, such that an administrator can e.g. upload that file somewhere using a post-install command. Maybe even /disable/ auto-reboot in case of an error, so that administrators can investigate the machine directly? - For the most part I did a rather in-depth review, just as things caught my eye. Still trimmed down things as much as possible. On Tue, Sep 05, 2023 at 03:28:28PM +0200, Aaron Lauterer wrote: > [..] > > It supports basic globbing/wildcard is supported at the beginning and > end of the search string. The matching is implemented by us as it isn't > that difficult and we can avoid additional crates. If we ever want more extensive matching in the future, we could use the `glob` crate, which provides a `Pattern` struct for using it directly. It's actively maintained and does not have any non-dev dependencies, so from my side it would be fine. > Technically it is reusing a lot of the TUI installer. All the source > files needed are in the 'tui' subdirectory. The idea is, that we can > factor out common code into a dedicated library crate. To make it > easier, unused parts are removed. > Some changes were made as well, for example changing HashMaps to > BTreeMaps to avoid random ordering. Some structs got their properties > made public, but with a refactor, we can probably rework that and > implement additional From methods. If you want I can prepare some patches too, moving all the things used in this series into a separate, shared crate. Just ping me in that case. > > [..] > diff --git a/proxmox-auto-installer/Cargo.toml b/proxmox-auto-installer/Cargo.toml > new file mode 100644 > index 0000000..fd38d28 > --- /dev/null > +++ b/proxmox-auto-installer/Cargo.toml > @@ -0,0 +1,13 @@ > +[package] > +name = "proxmox-auto-installer" > +version = "0.1.0" > +edition = "2021" > +authors = [ "Aaron Lauerer +license = "AGPL-3" > +exclude = [ "build", "debian" ] > +homepage = "https://www.proxmox.com" > + > +[dependencies] > +serde = { version = "1.0", features = ["derive"] } > +serde_json = "1.0" Should be workspace dependencies, as they are shared between both crates anyway (and possible a third, shared crate). > +toml = "0.5.11" > [..] > diff --git a/proxmox-auto-installer/src/answer.rs b/proxmox-auto-installer/src/answer.rs > new file mode 100644 > index 0000000..566030c > --- /dev/null > +++ b/proxmox-auto-installer/src/answer.rs > @@ -0,0 +1,144 @@ > +use serde::{Deserialize, Serialize}; > +use std::collections::BTreeMap; > + > +#[derive(Clone, Deserialize, Debug)] > +#[serde(rename_all = "lowercase")] > +pub struct Answer { > + pub global: Global, > + pub network: Network, > + pub disks: Disks, > +} > + > +#[derive(Clone, Deserialize, Debug)] > +pub struct Global { > + pub country: String, > + pub fqdn: String, `proxmox_tui_installer::utils::Fqdn` implements `Deserialize`, so can be used here directly. As this is a user-provided value and *must* be valid anyway, it's IMHO okay to let it fail if it doesn't parse. > + pub keyboard: String, > + pub mailto: String, > + pub timezone: String, > + pub password: String, > + pub pre_command: Option>, > + pub post_command: Option>, The additional `Option<..>` can be avoided for both: #[serde(default)] pub pre_command: Vec, Further, a way to run commands in the finished installation chroot could be pretty useful too, e.g. to create some files in /etc comes to mind. > +} > + > +#[derive(Clone, Deserialize, Debug)] > +pub struct Network { > + pub use_dhcp: Option, > + pub cidr: Option, `proxmox_tui_installer::utils::CidrAddress` implements `Deserialize` too > + pub dns: Option, > + pub gateway: Option, .. and `std::net::IpAddr` > + // use BTreeMap to have keys sorted > + pub filter: Option>, The `Option<..>` can be again be removed, as with `Global::pre_command` and `Global::post_command`. Same for other instances below. > +} > + > +#[derive(Clone, Deserialize, Debug)] > +pub struct Disks { > + pub filesystem: Option, > + pub disk_selection: Option>, > + pub filter_match: Option, As `FilterMatch::Any` is the default anyway, it can be declared the default variant below, such that `#[serde(default)]` can be used there too. Samo for `filesystem` above. > + // use BTreeMap to have keys sorted > + pub filter: Option>, > + pub zfs: Option, > + pub lvm: Option, > + pub btrfs: Option, These last three should probably be in a enum, much like `proxmox_tui_installer::options::AdvancedBootdiskOptions`. Only one of them will ever be used at the same time, using an enum this invariant can easily be enforced. OTOH, probably needs a custom deserializer, so a simple check somewhere that only one of them is ever set would be fine too. > +} > + > +#[derive(Clone, Deserialize, Debug, PartialEq)] > +#[serde(rename_all = "lowercase")] > +pub enum FilterMatch { > + Any, > + All, > +} > + > +#[derive(Clone, Deserialize, Serialize, Debug)] > +#[serde(rename_all = "kebab-case")] > +pub enum Filesystem { > + Ext4, > + Xfs, > + ZfsRaid0, > + ZfsRaid1, > + ZfsRaid10, > + ZfsRaidZ1, > + ZfsRaidZ2, > + ZfsRaidZ3, > + BtrfsRaid0, > + BtrfsRaid1, > + BtrfsRaid10, > +} This should probably reuse `proxmox_tui_installer::options::FsType`. Having /even more/ definitions of possible filesystems (e.g. the ZFS stuff is already mostly duplicated in pbs-api-types). Also saves us some converting between these two types, like in parse_answer(). In this case it requires a custom deserializer, but boils down to a rather simple `match`. > + > +#[derive(Clone, Deserialize, Debug)] > +pub struct ZfsOptions { > + pub ashift: Option, > + pub checksum: Option, > + pub compress: Option, #[serde(default)] and avoid the `Option<..>`s again :^) > + pub copies: Option, > + pub hdsize: Option, > +} ^ Should reuse `proxmox_tui_installer::options::ZfsBootdiskOptions` > + [..] > + > +#[derive(Copy, Clone, Debug, Default, Eq, PartialEq, Deserialize)] > +#[serde(rename_all(deserialize = "lowercase"))] > +pub enum ZfsCompressOption { > + #[default] > + On, > + Off, > + Lzjb, > + Lz4, > + Zle, > + Gzip, > + Zstd, > +} ^ Same > + > +#[derive(Copy, Clone, Debug, Default, Eq, PartialEq, Deserialize)] > +#[serde(rename_all = "kebab-case")] > +pub enum ZfsChecksumOption { > + #[default] > + On, > + Off, > + Fletcher2, > + Fletcher4, > + Sha256, > +} ^ Same > + > +#[derive(Clone, Deserialize, Serialize, Debug)] > +pub struct LvmOptions { > + pub hdsize: Option, > + pub swapsize: Option, > + pub maxroot: Option, > + pub maxvz: Option, > + pub minfree: Option, > +} ^ Same > + > +impl LvmOptions { > + pub fn new() -> LvmOptions { > + LvmOptions { > + hdsize: None, > + swapsize: None, > + maxroot: None, > + maxvz: None, > + minfree: None, > + } > + } > +} > + > +#[derive(Clone, Deserialize, Serialize, Debug)] > +pub struct BtrfsOptions { > + pub hdsize: Option, > +} ^ Same > + > +impl BtrfsOptions { > + pub fn new() -> BtrfsOptions { > + BtrfsOptions { hdsize: None } > + } > +} > diff --git a/proxmox-auto-installer/src/main.rs b/proxmox-auto-installer/src/main.rs > new file mode 100644 > index 0000000..d647567 > --- /dev/null > +++ b/proxmox-auto-installer/src/main.rs > @@ -0,0 +1,412 @@ > [..] > + > +fn installer_setup( > + in_test_mode: bool, > +) -> Result<(Answer, LocaleInfo, RuntimeInfo, UdevInfo), String> { > + let base_path = if in_test_mode { "./testdir" } else { "/" }; > + let mut path = PathBuf::from(base_path); > + > [..] > + > + let mut buffer = String::new(); > + let lines = std::io::stdin().lock().lines(); > + for line in lines { > + buffer.push_str(&line.unwrap()); > + buffer.push('\n'); > + } Instead of reading line-by-line: use std::io::Read; let mut buffer = String::new(); std::io::stdin().lock().read_to_string(&mut buffer); > + > + let answer: answer::Answer = > + toml::from_str(&buffer).map_err(|err| format!("Failed parsing answer file: {err}"))?; > + > + runtime_info.disks.sort(); > + if runtime_info.disks.is_empty() { > + Err("The installer could not find any supported hard disks.".to_owned()) > + } else { > + Ok((answer, locale_info, runtime_info, udev_info)) > + } > +} > + > [..] > + > +fn run_installation( > + answer: &Answer, > + locales: &LocaleInfo, > + runtime_info: &RuntimeInfo, > + udevadm_info: &UdevInfo, > +) -> Result<(), String> { > + let config = parse_answer(answer, udevadm_info, runtime_info, locales)?; > [..] > + let mut inner = || { > + let reader = child.stdout.take().map(BufReader::new)?; > + let mut writer = child.stdin.take()?; > + > + serde_json::to_writer(&mut writer, &config).unwrap(); > + writeln!(writer).unwrap(); > + > + for line in reader.lines() { > + match line { > + Ok(line) => print!("{line}"), Needs a `println!()`, otherwise everything is just printed on one, very long line. > + Err(_) => break, > + }; > + } > + Some(()) > + }; > + match inner() { > + Some(_) => Ok(()), > + None => Err("low level installer returned early".to_string()), > + } > +} > + > +fn parse_answer( > + answer: &Answer, > + udev_info: &UdevInfo, > + runtime_info: &RuntimeInfo, > + locales: &LocaleInfo, > +) -> Result { > + let filesystem = match &answer.disks.filesystem { > + Some(answer::Filesystem::Ext4) => FsType::Ext4, > + Some(answer::Filesystem::Xfs) => FsType::Xfs, > + Some(answer::Filesystem::ZfsRaid0) => FsType::Zfs(ZfsRaidLevel::Raid0), > + Some(answer::Filesystem::ZfsRaid1) => FsType::Zfs(ZfsRaidLevel::Raid1), > + Some(answer::Filesystem::ZfsRaid10) => FsType::Zfs(ZfsRaidLevel::Raid10), > + Some(answer::Filesystem::ZfsRaidZ1) => FsType::Zfs(ZfsRaidLevel::RaidZ), > + Some(answer::Filesystem::ZfsRaidZ2) => FsType::Zfs(ZfsRaidLevel::RaidZ2), > + Some(answer::Filesystem::ZfsRaidZ3) => FsType::Zfs(ZfsRaidLevel::RaidZ3), > + Some(answer::Filesystem::BtrfsRaid0) => FsType::Btrfs(BtrfsRaidLevel::Raid0), > + Some(answer::Filesystem::BtrfsRaid1) => FsType::Btrfs(BtrfsRaidLevel::Raid1), > + Some(answer::Filesystem::BtrfsRaid10) => FsType::Btrfs(BtrfsRaidLevel::Raid10), > + None => FsType::Ext4, Definitely would put this in a `From<..>` impl, or better re-use the definitions from the TUI (as mentioned above already). > + }; > + > [..] > + utils::set_disks(answer, udev_info, runtime_info, &mut config)?; > + match &config.filesys { > [..] > + FsType::Zfs(_) => { > + let zfs = match &answer.disks.zfs { > + Some(zfs) => zfs.clone(), > + None => answer::ZfsOptions::new(), > + }; > + let first_selected_disk = utils::get_first_selected_disk(&config); > + > + config.hdsize = zfs > + .hdsize > + .unwrap_or(runtime_info.disks[first_selected_disk].size); > + config.zfs_opts = Some(InstallZfsOption { > + ashift: zfs.ashift.unwrap_or(12), > + compress: match zfs.compress { > + Some(answer::ZfsCompressOption::On) => ZfsCompressOption::On, > + Some(answer::ZfsCompressOption::Off) => ZfsCompressOption::Off, > + Some(answer::ZfsCompressOption::Lzjb) => ZfsCompressOption::Lzjb, > + Some(answer::ZfsCompressOption::Lz4) => ZfsCompressOption::Lz4, > + Some(answer::ZfsCompressOption::Zle) => ZfsCompressOption::Zle, > + Some(answer::ZfsCompressOption::Gzip) => ZfsCompressOption::Gzip, > + Some(answer::ZfsCompressOption::Zstd) => ZfsCompressOption::Zstd, > + None => ZfsCompressOption::On, ^ Same > + }, > + checksum: match zfs.checksum { > + Some(answer::ZfsChecksumOption::On) => ZfsChecksumOption::On, > + Some(answer::ZfsChecksumOption::Off) => ZfsChecksumOption::Off, > + Some(answer::ZfsChecksumOption::Fletcher2) => ZfsChecksumOption::Fletcher2, > + Some(answer::ZfsChecksumOption::Fletcher4) => ZfsChecksumOption::Fletcher4, > + Some(answer::ZfsChecksumOption::Sha256) => ZfsChecksumOption::Sha256, > + None => ZfsChecksumOption::On, ^ Same > + }, > + copies: zfs.copies.unwrap_or(1), > + }); > + } > [..] > diff --git a/proxmox-auto-installer/src/utils.rs b/proxmox-auto-installer/src/utils.rs > new file mode 100644 > index 0000000..6ff78ac > --- /dev/null > +++ b/proxmox-auto-installer/src/utils.rs > @@ -0,0 +1,325 @@ > [..] > + > +pub fn get_network_settings( > + answer: &Answer, > + udev_info: &UdevInfo, > + runtime_info: &RuntimeInfo, > +) -> Result { > + let mut network_options = NetworkOptions::from(&runtime_info.network); > + > + // Always use the FQDN from the answer file > + network_options.fqdn = Fqdn::from(answer.global.fqdn.as_str()).expect("Error parsing FQDN"); > + > + if answer.network.use_dhcp.is_none() || !answer.network.use_dhcp.unwrap() { if !answer.network.use_dhcp.unwrap_or_default() { > + network_options.address = CidrAddress::from_str( > + answer > + .network > + .cidr > + .clone() > + .expect("No CIDR defined") Should rather return an `Err(..)` than panic'ing here > + .as_str(), > + ) > + .expect("Error parsing CIDR"); > + network_options.dns_server = IpAddr::from_str( > + answer > + .network > + .dns > + .clone() > + .expect("No DNS server defined") ^ Same > + .as_str(), > + ) > + .expect("Error parsing DNS server"); > + network_options.gateway = IpAddr::from_str( > + answer > + .network > + .gateway > + .clone() > + .expect("No gateway defined") ^ Same > + .as_str(), > + ) > + .expect("Error parsing gateway"); ^ Same > + network_options.ifname = > + get_single_udev_index(answer.network.filter.clone().unwrap(), &udev_info.nics)? > + } > + > + Ok(network_options) > +} > + > +fn get_single_udev_index( > + filter: BTreeMap, > + udev_list: &BTreeMap>, > +) -> Result { > + if filter.is_empty() { > + return Err(String::from("no filter defined")); > + } > + let mut dev_index: Option = None; > + 'outer: for (dev, dev_values) in udev_list { > + for (filter_key, filter_value) in &filter { > + for (udev_key, udev_value) in dev_values { > + if udev_key == filter_key && find_with_glob(filter_value, udev_value) { > + dev_index = Some(dev.clone()); > + break 'outer; // take first match Just return `Ok(dev.clone())` here directly > + } > + } > + } > + } > + if dev_index.is_none() { > + return Err(String::from("filter did not match any device")); > + } > + > + Ok(dev_index.unwrap()) > +} > + > [..] > + > +fn set_selected_disks( > + answer: &Answer, > + udev_info: &UdevInfo, > + runtime_info: &RuntimeInfo, > + config: &mut InstallConfig, > +) -> Result<(), String> { > + match &answer.disks.disk_selection { > + Some(selection) => { > + for disk_name in selection { > + let disk = runtime_info > + .disks > + .iter() > + .find(|item| item.path.ends_with(disk_name.as_str())); > + if let Some(disk) = disk { > + config > + .disk_selection > + .insert(disk.index.clone(), disk.index.clone()); > + } > + } > + } > + None => { > + let filter_match = answer > + .disks > + .filter_match > + .clone() > + .unwrap_or(FilterMatch::Any); > + let selected_disk_indexes = get_matched_udev_indexes( > + answer.disks.filter.clone().unwrap(), > + &udev_info.disks, > + filter_match == FilterMatch::All, > + )?; > + > + for i in selected_disk_indexes.into_iter() { > + let disk = runtime_info > + .disks > + .iter() > + .find(|item| item.index == i) > + .unwrap(); > + config > + .disk_selection > + .insert(disk.index.clone(), disk.index.clone()); Isn't this just the same as `config.disk_selection.insert(i, i)`? > + } > + } > + } > + if config.disk_selection.is_empty() { > + return Err("No disks found matching selection.".to_string()); > + } > + Ok(()) > +} > + > [..] > + > +pub fn verify_locale_settings(answer: &Answer, locales: &LocaleInfo) -> Result<(), String> { > + if !locales > + .countries > + .keys() > + .any(|i| i == &answer.global.country) Can be replaced with `locales.countries.contains_key(&answer.global.country)` - should be more efficiently too, since it avoids iterating all the keys. > + { > + return Err(format!( > + "country code '{}' is not valid", > + &answer.global.country > + )); > + } > + if !locales.kmap.keys().any(|i| i == &answer.global.keyboard) { ^ Same > + return Err(format!( > + "keyboard layout '{}' is not valid", > + &answer.global.keyboard > + )); > + } > + if !locales > + .cczones > + .iter() > + .any(|(_, zones)| zones.contains(&answer.global.timezone)) In `locales.json`, there is also the `zones` key (which is not deserialized by the TUI as it is not needed). This is basically just a list of all available timezones (minus UTC), thus I would add that to `LocaleInfo` and use that list here to check if it is a valid timezone. > + { > + return Err(format!( > + "timezone '{}' is not valid", > + &answer.global.timezone > + )); > + } > + Ok(()) > +} > + > [..] > + > +fn run_cmd(cmds: &Vec) -> Result<(), String> { Ideally, this would be called run_cmds(), as it processes multiple commands, that should be reflected in the name. Maybe the above could be renamed to e.g. run_cmds_step()? > + for cmd in cmds { > + #[cfg(debug_assertions)] > + println!("Would run command '{}'", cmd); > + > +// #[cfg(not(debug_assertions))] > + { > + println!("Command '{}':", cmd); > + let output = Command::new("/bin/bash") > + .arg("-c") > + .arg(cmd.clone()) > + .output() > + .map_err(|err| format!("error running command {}: {err}", cmd))?; As this is also used to run user-provided commands, it should them in some well-known working directory. (Possibly a temporary directory, or just /). > + print!( > + "{}", > + String::from_utf8(output.stdout).map_err(|err| format!("{err}"))? > + ); > + print!( > + "{}", > + String::from_utf8(output.stderr).map_err(|err| format!("{err}"))? > + ); > + if !output.status.success() { > + return Err("command failed".to_string()); > + } > + } > + } > + > + Ok(()) > +} > +