[pve-devel] [RFC installer 2/6] add proxmox-auto-installer

[Christoph Heiss]

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 at 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(())
> +}
> +