From: Christoph Heiss <c.heiss@proxmox.com>
To: Aaron Lauterer <a.lauterer@proxmox.com>
Cc: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [RFC installer 2/6] add proxmox-auto-installer
Date: Thu, 21 Sep 2023 13:16:03 +0200 [thread overview]
Message-ID: <dimfclycvcnhz6gesfhg7zvjwcotiikmrwv76srsh5kb2ysb56@grj673rcy4nu> (raw)
In-Reply-To: <20230905132832.3179097-3-a.lauterer@proxmox.com>
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 <a.lauterer@proxmox.com" ]
> +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<Vec<String>>,
> + pub post_command: Option<Vec<String>>,
The additional `Option<..>` can be avoided for both:
#[serde(default)]
pub pre_command: Vec<String>,
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<bool>,
> + pub cidr: Option<String>,
`proxmox_tui_installer::utils::CidrAddress` implements `Deserialize` too
> + pub dns: Option<String>,
> + pub gateway: Option<String>,
.. and `std::net::IpAddr`
> + // use BTreeMap to have keys sorted
> + pub filter: Option<BTreeMap<String, String>>,
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<Filesystem>,
> + pub disk_selection: Option<Vec<String>>,
> + pub filter_match: Option<FilterMatch>,
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<BTreeMap<String, String>>,
> + pub zfs: Option<ZfsOptions>,
> + pub lvm: Option<LvmOptions>,
> + pub btrfs: Option<BtrfsOptions>,
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<usize>,
> + pub checksum: Option<ZfsChecksumOption>,
> + pub compress: Option<ZfsCompressOption>,
#[serde(default)] and avoid the `Option<..>`s again :^)
> + pub copies: Option<usize>,
> + pub hdsize: Option<f64>,
> +}
^ 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<f64>,
> + pub swapsize: Option<f64>,
> + pub maxroot: Option<f64>,
> + pub maxvz: Option<f64>,
> + pub minfree: Option<f64>,
> +}
^ 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<f64>,
> +}
^ 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<InstallConfig, String> {
> + 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<NetworkOptions, String> {
> + 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<String, String>,
> + udev_list: &BTreeMap<String, BTreeMap<String, String>>,
> +) -> Result<String, String> {
> + if filter.is_empty() {
> + return Err(String::from("no filter defined"));
> + }
> + let mut dev_index: Option<String> = 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<String>) -> 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(())
> +}
> +
next prev parent reply other threads:[~2023-09-21 11:16 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-05 13:28 [pve-devel] [RFC installer 0/6] add automated installation Aaron Lauterer
2023-09-05 13:28 ` [pve-devel] [RFC installer 1/6] low level: sys: fetch udev properties Aaron Lauterer
2023-09-05 13:28 ` [pve-devel] [RFC installer 2/6] add proxmox-auto-installer Aaron Lauterer
2023-09-21 11:16 ` Christoph Heiss [this message]
2023-09-21 11:30 ` Thomas Lamprecht
2023-09-21 11:39 ` Christoph Heiss
2023-09-05 13:28 ` [pve-devel] [RFC installer 3/6] add answer file fetch script Aaron Lauterer
2023-09-20 9:52 ` Christoph Heiss
2023-09-05 13:28 ` [pve-devel] [PATCH installer 4/6] makefile: fix handling of multiple usr_bin files Aaron Lauterer
2023-09-05 13:28 ` [pve-devel] [RFC installer 5/6] makefile: add auto installer Aaron Lauterer
2023-09-05 13:28 ` [pve-devel] [RFC docs 6/6] installation: add unattended documentation Aaron Lauterer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=dimfclycvcnhz6gesfhg7zvjwcotiikmrwv76srsh5kb2ysb56@grj673rcy4nu \
--to=c.heiss@proxmox.com \
--cc=a.lauterer@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox