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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.