* [pbs-devel] [PATCH backup stable-2] pbs2to3: add upgrade checker binary
@ 2023-06-28 8:10 Christian Ebner
2023-06-28 8:59 ` Fiona Ebner
2023-06-28 10:13 ` Lukas Wagner
0 siblings, 2 replies; 6+ messages in thread
From: Christian Ebner @ 2023-06-28 8:10 UTC (permalink / raw)
To: pbs-devel
Adds the pbs2to3 upgrade checker with some basic checks.
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
Makefile | 1 +
debian/proxmox-backup-server.install | 2 +
src/bin/pbs2to3.rs | 602 +++++++++++++++++++++++++++
zsh-completions/_pbs2to3 | 13 +
4 files changed, 618 insertions(+)
create mode 100644 src/bin/pbs2to3.rs
create mode 100644 zsh-completions/_pbs2to3
diff --git a/Makefile b/Makefile
index b307009d..7477935d 100644
--- a/Makefile
+++ b/Makefile
@@ -19,6 +19,7 @@ USR_BIN := \
USR_SBIN := \
proxmox-backup-manager \
proxmox-backup-debug \
+ pbs2to3
# Binaries for services:
SERVICE_BIN := \
diff --git a/debian/proxmox-backup-server.install b/debian/proxmox-backup-server.install
index 76f50cd0..ebe51aae 100644
--- a/debian/proxmox-backup-server.install
+++ b/debian/proxmox-backup-server.install
@@ -11,6 +11,7 @@ usr/lib/x86_64-linux-gnu/proxmox-backup/proxmox-daily-update
usr/lib/x86_64-linux-gnu/proxmox-backup/sg-tape-cmd
usr/sbin/proxmox-backup-debug
usr/sbin/proxmox-backup-manager
+usr/sbin/pbs2to3
usr/bin/pmtx
usr/bin/pmt
usr/bin/proxmox-tape
@@ -39,3 +40,4 @@ usr/share/zsh/vendor-completions/_proxmox-backup-manager
usr/share/zsh/vendor-completions/_proxmox-tape
usr/share/zsh/vendor-completions/_pmtx
usr/share/zsh/vendor-completions/_pmt
+usr/share/zsh/vendor-completions/_pbs2to3
diff --git a/src/bin/pbs2to3.rs b/src/bin/pbs2to3.rs
new file mode 100644
index 00000000..97d064cd
--- /dev/null
+++ b/src/bin/pbs2to3.rs
@@ -0,0 +1,602 @@
+use std::io::Write;
+use std::path::Path;
+
+use anyhow::{format_err, Error};
+use regex::Regex;
+use termcolor::{Color, ColorChoice, ColorSpec, StandardStream, WriteColor};
+
+use proxmox_apt::repositories::{self, APTRepositoryFile};
+use proxmox_backup::api2::node::apt;
+
+const OLD_SUITE: &str = "bullseye";
+const NEW_SUITE: &str = "bookworm";
+const PROXMOX_BACKUP_META: &str = "proxmox-backup";
+const MIN_PBS_MAJOR: u8 = 2;
+const MIN_PBS_MINOR: u8 = 4;
+const MIN_PBS_PKGREL: u8 = 1;
+
+fn main() -> Result<(), Error> {
+ let mut checker = Checker::new();
+ checker.check_pbs_packages()?;
+ checker.check_misc()?;
+ checker.summary()?;
+ Ok(())
+}
+
+struct Checker {
+ output: ConsoleOutput,
+ upgraded: bool,
+}
+
+impl Checker {
+ pub fn new() -> Self {
+ Self {
+ output: ConsoleOutput::new(),
+ upgraded: false,
+ }
+ }
+
+ pub fn check_pbs_packages(&mut self) -> Result<(), Error> {
+ self.output.print_header("CHECKING VERSION INFORMATION FOR PBS PACKAGES")?;
+ self.output.log_info("Checking for package updates..")?;
+
+ let result = Self::get_upgradable_packages();
+ match result {
+ Err(err) => {
+ self.output.log_warn(format!("{}", err).as_str())?;
+ self.output.log_fail("unable to retrieve list of package updates!")?;
+ }
+ Ok(cache) => {
+ if cache.package_status.is_empty() {
+ self.output.log_pass("all packages up-to-date")?;
+ } else {
+ let pkgs = cache.package_status.iter()
+ .map(|pkg| format!("{}, ", pkg.package.clone()))
+ .collect::<String>();
+ self.output.log_warn(
+ format!(
+ "updates for the following packages are available:\
+ \n {}",
+ &pkgs[..pkgs.len()-2]
+ ).as_str()
+ )?;
+ }
+ }
+ }
+
+ self.output.log_info("Checking proxmox backup server package version..")?;
+ let pkg_versions = apt::get_versions()?;
+
+ let pbs_meta_pkg = pkg_versions.iter().find(|pkg| {
+ pkg.package.as_str() == PROXMOX_BACKUP_META
+ });
+
+ if let Some(pbs_meta_pkg) = pbs_meta_pkg {
+ let pkg_version = Regex::new(r"^(\d+)\.(\d+)[.-](\d+)")?;
+ let captures = pkg_version.captures(&pbs_meta_pkg.old_version);
+ if let Some(captures) = captures {
+ let maj = Self::extract_version_from_captures(1, &captures)?;
+ let min = Self::extract_version_from_captures(2, &captures)?;
+ let pkgrel = Self::extract_version_from_captures(3, &captures)?;
+
+ if maj > MIN_PBS_MAJOR {
+ self.output.log_pass(
+ format!("Already upgraded to Proxmox Backup Server {}", maj).as_str()
+ )?;
+ self.upgraded = true;
+ } else if maj >= MIN_PBS_MAJOR && min >= MIN_PBS_MINOR && pkgrel >= MIN_PBS_PKGREL {
+ self.output.log_pass(
+ format!(
+ "'{}' has version >= {}.{}-{}",
+ PROXMOX_BACKUP_META,
+ MIN_PBS_MAJOR,
+ MIN_PBS_MINOR,
+ MIN_PBS_PKGREL,
+ ).as_str()
+ )?;
+ } else {
+ self.output.log_fail(
+ format!(
+ "'{}' package is too old, please upgrade to >= {}.{}-{}",
+ PROXMOX_BACKUP_META,
+ MIN_PBS_MAJOR,
+ MIN_PBS_MINOR,
+ MIN_PBS_PKGREL,
+ ).as_str()
+ )?;
+ }
+ } else {
+ self.output.log_fail(
+ format!(
+ "could not match the '{}' package version, is it installed?",
+ PROXMOX_BACKUP_META,
+ ).as_str()
+ )?;
+ }
+
+ self.output.log_info("Check running kernel version..")?;
+ let mut krunning = Regex::new(r"^6\.(?:2\.(?:[2-9]\d+|1[6-8]|1\d\d+)|5)[^~]*$")?;
+ let mut kinstalled = "pve-kernel-6.2";
+ if !self.upgraded {
+ krunning = Regex::new(r"^(?:5\.(?:13|15)|6\.2)")?;
+ kinstalled = "pve-kernel-5.15";
+ }
+
+ let mut command = std::process::Command::new("uname");
+ command.arg("-r");
+ match command.output() {
+ Err(_err) => self.output.log_fail("unable to determine running kernel version.")?,
+ Ok(ret) => {
+ let running_version = std::str::from_utf8(&ret.stdout[..ret.stdout.len()-1])?;
+ if krunning.is_match(running_version) {
+ if self.upgraded {
+ self.output.log_pass(
+ format!(
+ "running new kernel '{}' after upgrade.",
+ running_version,
+ ).as_str()
+ )?;
+ } else {
+ self.output.log_pass(
+ format!(
+ "running kernel '{}' is considered suitable for upgrade.",
+ running_version,
+ ).as_str()
+ )?;
+ }
+ } else {
+ let installed_kernel = pkg_versions.iter().find(|pkg| {
+ pkg.package.as_str() == kinstalled
+ });
+ if installed_kernel.is_some() {
+ self.output.log_warn(
+ format!(
+ "a suitable kernel '{}' is installed, but an unsuitable '{}' \
+ is booted, missing reboot?!",
+ kinstalled,
+ running_version,
+ ).as_str()
+ )?;
+ } else {
+ self.output.log_warn(
+ format!(
+ "unexpected running and installed kernel '{}'.",
+ running_version,
+ ).as_str()
+ )?;
+ }
+ }
+ }
+ }
+
+ } else {
+ self.output.log_fail(format!("'{}' package not found!", PROXMOX_BACKUP_META).as_str())?;
+ }
+
+ Ok(())
+ }
+
+ fn extract_version_from_captures(index: usize, captures: ®ex::Captures) -> Result<u8, Error> {
+ if let Some(capture) = captures.get(index) {
+ let val = capture.as_str().parse::<u8>()?;
+ Ok(val)
+ } else {
+ Ok(0)
+ }
+ }
+
+ fn check_bootloader(&mut self) -> Result<(), Error> {
+ self.output.log_info("Checking bootloader configuration...")?;
+
+ // PBS packages version check needs to be run before
+ if !self.upgraded {
+ self.output.log_skip("not yet upgraded, no need to check the presence of systemd-boot")?;
+ }
+
+ if !Path::is_file(Path::new("/etc/kernel/proxmox-boot-uuids")) {
+ self.output.log_skip("proxmox-boot-tool not used for bootloader configuration")?;
+ return Ok(());
+ }
+
+ if !Path::is_dir(Path::new("/sys/firmware/efi")) {
+ self.output.log_skip("System booted in legacy-mode - no need for systemd-boot")?;
+ return Ok(());
+ }
+
+ if Path::is_file(Path::new("/usr/share/doc/systemd-boot/changelog.Debian.gz")) {
+ self.output.log_pass("systemd-boot is installed")?;
+ } else {
+ self.output.log_warn(
+ "proxmox-boot-tool is used for bootloader configuration in uefi mode \
+ but the separate systemd-boot package, existing in Debian Bookworm \
+ is not installed.\n\
+ initializing new ESPs will not work unitl the package is installed."
+ )?;
+ }
+ Ok(())
+ }
+
+ fn check_apt_repos(&mut self) -> Result<(), Error> {
+ self.output.log_info("Checking for package repository suite mismatches..")?;
+
+ let mut strange = false;
+ let mut mismatches = Vec::new();
+
+ let (repo_files, _repo_errors, _digest) = repositories::repositories()?;
+ for repo_file in repo_files {
+ let (found_strange, mut found_mismatches) = self.check_repo_file(repo_file)?;
+ if found_strange {
+ strange = true;
+ }
+ mismatches.append(&mut found_mismatches);
+ }
+
+ match (mismatches.is_empty(), strange) {
+ (true, false) => self.output.log_pass("found no suite mismatch")?,
+ (true, true) => self.output.log_notice(
+ "found no suite mismatches, but found at least one strange suite"
+ )?,
+ (false, _) => {
+ let mut message = String::from(
+ "Found mixed old and new packages repository suites, fix before upgrading!\
+ \n Mismatches:"
+ );
+ for (suite, location) in mismatches.iter() {
+ message.push_str(
+ format!("\n found suite '{}' at '{}'", suite, location).as_str()
+ );
+ }
+ message.push('\n');
+ self.output.log_fail(message.as_str())?
+ }
+ }
+
+ Ok(())
+ }
+
+ pub fn check_misc(&mut self) -> Result<(), Error> {
+ self.output.print_header("MISCELLANEOUS CHECKS")?;
+ self.check_pbs_services()?;
+ self.check_time_sync()?;
+ self.check_apt_repos()?;
+ self.check_bootloader()?;
+ Ok(())
+ }
+
+ pub fn summary(&mut self) -> Result<(), Error> {
+ self.output.print_summary()
+ }
+
+ fn check_repo_file(
+ &mut self,
+ repo_file: APTRepositoryFile
+ ) -> Result<(bool, Vec<(String, String)>), Error> {
+ let deb_suite = Regex::new(r"(\w+)(-updates|-backports|-security)?$")?;
+ let mut strange_suite = false;
+ let mut found_suite: Option<(String, String)> = None;
+ let mut mismatches = Vec::new();
+
+ for repo in repo_file.repositories {
+ for suite in &repo.suites {
+ if let Some(suite) = deb_suite.captures(suite) {
+ if let Some(suite_match) = suite.get(1) {
+ let suite = suite_match.as_str();
+ let location = repo_file.path.clone().unwrap_or(String::new());
+
+ if suite != OLD_SUITE && suite != NEW_SUITE {
+ self.output.log_notice(format!(
+ "found unusual suite '{}', neighter old '{}' nor new '{}'..\
+ \n Affected file {}\
+ \n Please assure this is shipping compatible packages for \
+ the upgrade!",
+ suite,
+ OLD_SUITE,
+ NEW_SUITE,
+ location,
+ ).as_str())?;
+ strange_suite = true;
+ continue;
+ }
+
+ if let Some((ref current_suite, ref current_location)) = found_suite {
+ if suite != current_suite {
+ if mismatches.is_empty() {
+ mismatches.push(
+ (current_suite.clone(), current_location.clone())
+ );
+ mismatches.push((suite.to_string(), location));
+ } else {
+ mismatches.push((suite.to_string(), location));
+ }
+ }
+ } else {
+ found_suite = Some((suite.to_string(), location));
+ }
+ }
+ }
+ }
+ }
+
+ Ok((strange_suite, mismatches))
+ }
+
+ fn get_systemd_unit_state(
+ &mut self,
+ unit: &str,
+ ) -> Result<(SystemdUnitState, SystemdUnitState), Error> {
+ let mut command = std::process::Command::new("systemctl");
+ command.arg("is-enabled");
+ command.arg(unit);
+ let output = command.output().map_err(|err| {
+ format_err!("failed to execute {:?} - {}", command, err)
+ })?;
+
+ let is_enabled_state = match output.stdout.as_slice() {
+ b"enabled\n" => SystemdUnitState::Enabled,
+ b"disabled\n" => SystemdUnitState::Disabled,
+ _ => SystemdUnitState::Unknown,
+ };
+
+ let mut command = std::process::Command::new("systemctl");
+ command.arg("is-active");
+ command.arg(unit);
+ let output = command.output().map_err(|err| {
+ format_err!("failed to execute {:?} - {}", command, err)
+ })?;
+
+ let is_active_state = match output.stdout.as_slice() {
+ b"active\n" => SystemdUnitState::Active,
+ b"inactive\n" => SystemdUnitState::Inactive,
+ b"failed\n" => SystemdUnitState::Failed,
+ _ => SystemdUnitState::Unknown,
+ };
+ Ok((is_enabled_state, is_active_state))
+ }
+
+ fn check_pbs_services(&mut self) -> Result<(), Error> {
+ self.output.log_info("Checking pbs daemon services..")?;
+
+ for service in ["proxmox-backup.service", "proxmox-backup-proxy.service"] {
+ match self.get_systemd_unit_state(service)? {
+ (_, SystemdUnitState::Active) => {
+ self.output.log_pass(
+ format!("systemd unit '{}' is in state 'active'", service).as_str()
+ )?;
+ }
+ (_, SystemdUnitState::Inactive) => {
+ self.output.log_fail(
+ format!(
+ "systemd unit '{}' is in state 'inactive'\
+ \n Please check the service for errors and start it.",
+ service,
+ ).as_str()
+ )?;
+ }
+ (_, SystemdUnitState::Failed) => {
+ self.output.log_fail(
+ format!(
+ "systemd unit '{}' is in state 'failed'\
+ \n Please check the service for errors and start it.",
+ service,
+ ).as_str()
+ )?;
+ }
+ (_, _) => {
+ self.output.log_fail(
+ format!(
+ "systemd unit '{}' is not in state 'active'\
+ \n Please check the service for errors and start it.",
+ service,
+ ).as_str()
+ )?;
+ }
+ }
+ }
+ Ok(())
+ }
+
+ fn check_time_sync(&mut self) -> Result<(), Error> {
+ self.output.log_info("Checking for supported & active NTP service..")?;
+ if self.get_systemd_unit_state("systemd-timesyncd.service")?.1 == SystemdUnitState::Active {
+ self.output.log_warn(
+ "systemd-timesyncd is not the best choice for time-keeping on servers, due to only \
+ applying updates on boot.\
+ \n While not necessary for the upgrade it's recommended to use one of:\
+ \n * chrony (Default in new Proxmox VE installations)\
+ \n * ntpsec\
+ \n * openntpd"
+ )?;
+ } else if self.get_systemd_unit_state("ntp.service")?.1 == SystemdUnitState::Active {
+ self.output.log_info(
+ "Debian deprecated and removed the ntp package for Bookworm, but the system \
+ will automatically migrate to the 'ntpsec' replacement package on upgrade."
+ )?;
+ } else if self.get_systemd_unit_state("chrony.service")?.1 == SystemdUnitState::Active ||
+ self.get_systemd_unit_state("openntpd.service")?.1 == SystemdUnitState::Active ||
+ self.get_systemd_unit_state("ntpsec.service")?.1 == SystemdUnitState::Active
+ {
+ self.output.log_pass("Detected active time synchronisation unit")?;
+ } else {
+ self.output.log_warn(
+ "No (active) time synchronisation daemon (NTP) detected, but synchronized systems \
+ are important!"
+ )?;
+ }
+ Ok(())
+ }
+
+ fn get_upgradable_packages() -> Result<proxmox_backup::tools::apt::PkgState, Error> {
+ let cache = if let Ok(false) = proxmox_backup::tools::apt::pkg_cache_expired() {
+ if let Ok(Some(cache)) = proxmox_backup::tools::apt::read_pkg_state() {
+ cache
+ } else {
+ proxmox_backup::tools::apt::update_cache()?
+ }
+ } else {
+ proxmox_backup::tools::apt::update_cache()?
+ };
+
+ Ok(cache)
+ }
+}
+
+#[derive(PartialEq)]
+enum SystemdUnitState {
+ Active,
+ Enabled,
+ Disabled,
+ Failed,
+ Inactive,
+ Unknown,
+}
+
+#[derive(Default)]
+struct Counters {
+ pass: u64,
+ skip: u64,
+ notice: u64,
+ warn: u64,
+ fail: u64,
+}
+
+enum LogLevel {
+ Pass,
+ Info,
+ Skip,
+ Notice,
+ Warn,
+ Fail,
+}
+
+struct ConsoleOutput {
+ stream: StandardStream,
+ first_header: bool,
+ counters: Counters,
+}
+
+impl ConsoleOutput {
+ pub fn new() -> Self {
+ Self {
+ stream: StandardStream::stdout(ColorChoice::Always),
+ first_header: true,
+ counters: Counters::default(),
+ }
+ }
+
+ pub fn print_header(&mut self, message: &str) -> Result<(), Error> {
+ if !self.first_header {
+ writeln!(&mut self.stream, "")?;
+ }
+ self.first_header = false;
+ writeln!(&mut self.stream, "= {message} =\n")?;
+ Ok(())
+ }
+
+ pub fn set_color(&mut self, color: Color) -> Result<(), Error> {
+ self.stream.set_color(ColorSpec::new().set_fg(Some(color)))?;
+ Ok(())
+ }
+
+ pub fn log_line(&mut self, level: LogLevel, message: &str) -> Result<(), Error> {
+ match level {
+ LogLevel::Pass => {
+ self.counters.pass += 1;
+ self.set_color(Color::Green)?;
+ writeln!(&mut self.stream, "PASS: {}", message)?;
+ self.set_color(Color::White)?;
+ }
+ LogLevel::Info => {
+ writeln!(&mut self.stream, "INFO: {}", message)?;
+ }
+ LogLevel::Skip => {
+ self.counters.skip += 1;
+ writeln!(&mut self.stream, "SKIP: {}", message)?;
+ }
+ LogLevel::Notice => {
+ self.counters.notice += 1;
+ writeln!(&mut self.stream, "NOTICE: {}", message)?;
+ }
+ LogLevel::Warn=> {
+ self.counters.warn += 1;
+ self.set_color(Color::Yellow)?;
+ writeln!(&mut self.stream, "WARN: {}", message)?;
+ self.set_color(Color::White)?;
+ }
+ LogLevel::Fail => {
+ self.counters.fail += 1;
+ self.set_color(Color::Red)?;
+ writeln!(&mut self.stream, "FAIL: {}", message)?;
+ self.set_color(Color::White)?;
+ }
+ }
+ Ok(())
+ }
+
+ pub fn log_pass(&mut self, message: &str) -> Result<(), Error> {
+ self.log_line(LogLevel::Pass, message)
+ }
+
+ pub fn log_info(&mut self, message: &str) -> Result<(), Error> {
+ self.log_line(LogLevel::Info, message)
+ }
+
+ pub fn log_skip(&mut self, message: &str) -> Result<(), Error> {
+ self.log_line(LogLevel::Skip, message)
+ }
+
+ pub fn log_notice(&mut self, message: &str) -> Result<(), Error> {
+ self.log_line(LogLevel::Notice, message)
+ }
+
+ pub fn log_warn(&mut self, message: &str) -> Result<(), Error> {
+ self.log_line(LogLevel::Warn, message)
+ }
+
+ pub fn log_fail(&mut self, message: &str) -> Result<(), Error> {
+ self.log_line(LogLevel::Fail, message)
+ }
+
+ pub fn print_summary(&mut self) -> Result<(), Error> {
+ self.print_header("SUMMARY")?;
+
+ let total = self.counters.fail + self.counters.pass + self.counters.notice +
+ self.counters.skip + self.counters.warn;
+
+ self.set_color(Color::White)?;
+ writeln!(&mut self.stream, "TOTAL: {total}")?;
+ self.set_color(Color::Green)?;
+ writeln!(&mut self.stream, "PASSED: {}", self.counters.pass)?;
+ self.set_color(Color::White)?;
+ writeln!(&mut self.stream, "SKIPPED: {}", self.counters.skip)?;
+ writeln!(&mut self.stream, "NOTICE: {}", self.counters.notice)?;
+ if self.counters.warn > 0 {
+ self.set_color(Color::Yellow)?;
+ writeln!(&mut self.stream, "WARNINGS: {}", self.counters.warn)?;
+ }
+ if self.counters.fail > 0 {
+ self.set_color(Color::Red)?;
+ writeln!(&mut self.stream, "FAILURES: {}", self.counters.fail)?;
+ }
+ if self.counters.warn > 0 || self.counters.fail > 0 {
+ let mut color = Color::Yellow;
+ if self.counters.fail > 0 {
+ color = Color::Red;
+ }
+
+ self.set_color(color)?;
+ writeln!(
+ &mut self.stream,
+ "\nATTENTION: Please check the output for detailed information!",
+ )?;
+ if self.counters.fail > 0 {
+ writeln!(
+ &mut self.stream,
+ "Try to solve the problems one at a time and rerun this checklist tool again.",
+ )?;
+ }
+ }
+ self.set_color(Color::White)?;
+ Ok(())
+ }
+}
+
diff --git a/zsh-completions/_pbs2to3 b/zsh-completions/_pbs2to3
new file mode 100644
index 00000000..f6dc3d67
--- /dev/null
+++ b/zsh-completions/_pbs2to3
@@ -0,0 +1,13 @@
+#compdef _pbs2to3() pbs2to3
+
+function _pbs2to3() {
+ local cwords line point cmd curr prev
+ cwords=${#words[@]}
+ line=$words
+ point=${#line}
+ cmd=${words[1]}
+ curr=${words[cwords]}
+ prev=${words[cwords-1]}
+ compadd -- $(COMP_CWORD="$cwords" COMP_LINE="$line" COMP_POINT="$point" \
+ pbs2to3 bashcomplete "$cmd" "$curr" "$prev")
+}
--
2.30.2
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pbs-devel] [PATCH backup stable-2] pbs2to3: add upgrade checker binary
2023-06-28 8:10 [pbs-devel] [PATCH backup stable-2] pbs2to3: add upgrade checker binary Christian Ebner
@ 2023-06-28 8:59 ` Fiona Ebner
2023-06-28 9:43 ` Christian Ebner
2023-06-28 10:13 ` Lukas Wagner
1 sibling, 1 reply; 6+ messages in thread
From: Fiona Ebner @ 2023-06-28 8:59 UTC (permalink / raw)
To: Proxmox Backup Server development discussion, Christian Ebner
Am 28.06.23 um 10:10 schrieb Christian Ebner:
> Adds the pbs2to3 upgrade checker with some basic checks.
>
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
Some things I noticed:
> * chrony (Default in new Proxmox VE installations)
Should be "Proxmox Backup Server installations"
For repository checking, likely it's better to only check the suites of
enabled repositories and only the 'deb' ones (not 'deb-src').
My
deb http://deb.debian.org/debian-debug bullseye-debug main contrib
line produces:
NOTICE: found unusual suite 'debug', neighter old 'bullseye' nor new
'bookworm'..
but the suite is 'bullseye-debug'.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pbs-devel] [PATCH backup stable-2] pbs2to3: add upgrade checker binary
2023-06-28 8:59 ` Fiona Ebner
@ 2023-06-28 9:43 ` Christian Ebner
0 siblings, 0 replies; 6+ messages in thread
From: Christian Ebner @ 2023-06-28 9:43 UTC (permalink / raw)
To: Fiona Ebner, Proxmox Backup Server development discussion
Thanks for the review, I will send the v2 with comments taken into consideration.
Cheers,
Chris
> On 28.06.2023 10:59 CEST Fiona Ebner <f.ebner@proxmox.com> wrote:
>
>
> Am 28.06.23 um 10:10 schrieb Christian Ebner:
> > Adds the pbs2to3 upgrade checker with some basic checks.
> >
> > Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
>
> Some things I noticed:
>
> > * chrony (Default in new Proxmox VE installations)
>
> Should be "Proxmox Backup Server installations"
>
> For repository checking, likely it's better to only check the suites of
> enabled repositories and only the 'deb' ones (not 'deb-src').
>
> My
> deb http://deb.debian.org/debian-debug bullseye-debug main contrib
> line produces:
> NOTICE: found unusual suite 'debug', neighter old 'bullseye' nor new
> 'bookworm'..
> but the suite is 'bullseye-debug'.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pbs-devel] [PATCH backup stable-2] pbs2to3: add upgrade checker binary
2023-06-28 8:10 [pbs-devel] [PATCH backup stable-2] pbs2to3: add upgrade checker binary Christian Ebner
2023-06-28 8:59 ` Fiona Ebner
@ 2023-06-28 10:13 ` Lukas Wagner
2023-06-28 10:14 ` Lukas Wagner
1 sibling, 1 reply; 6+ messages in thread
From: Lukas Wagner @ 2023-06-28 10:13 UTC (permalink / raw)
To: Proxmox Backup Server development discussion, Christian Ebner
Hi,
tested on current `stable-2` branch. Seems to work just fine, at least on a clean PBS installation.
Some comments regarding the code are inline (nothing major though).
Tested-by: Lukas Wagner <l.wagner@proxmox.com>
Reviewed-by: Lukas Wagner <l.wagner@proxmox.com>
On 6/28/23 10:10, Christian Ebner wrote:
> Adds the pbs2to3 upgrade checker with some basic checks.
>
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
> Makefile | 1 +
> debian/proxmox-backup-server.install | 2 +
> src/bin/pbs2to3.rs | 602 +++++++++++++++++++++++++++
> zsh-completions/_pbs2to3 | 13 +
> 4 files changed, 618 insertions(+)
> create mode 100644 src/bin/pbs2to3.rs
> create mode 100644 zsh-completions/_pbs2to3
>
> diff --git a/Makefile b/Makefile
> index b307009d..7477935d 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -19,6 +19,7 @@ USR_BIN := \
> USR_SBIN := \
> proxmox-backup-manager \
> proxmox-backup-debug \
> + pbs2to3
>
> # Binaries for services:
> SERVICE_BIN := \
> diff --git a/debian/proxmox-backup-server.install b/debian/proxmox-backup-server.install
> index 76f50cd0..ebe51aae 100644
> --- a/debian/proxmox-backup-server.install
> +++ b/debian/proxmox-backup-server.install
> @@ -11,6 +11,7 @@ usr/lib/x86_64-linux-gnu/proxmox-backup/proxmox-daily-update
> usr/lib/x86_64-linux-gnu/proxmox-backup/sg-tape-cmd
> usr/sbin/proxmox-backup-debug
> usr/sbin/proxmox-backup-manager
> +usr/sbin/pbs2to3
> usr/bin/pmtx
> usr/bin/pmt
> usr/bin/proxmox-tape
> @@ -39,3 +40,4 @@ usr/share/zsh/vendor-completions/_proxmox-backup-manager
> usr/share/zsh/vendor-completions/_proxmox-tape
> usr/share/zsh/vendor-completions/_pmtx
> usr/share/zsh/vendor-completions/_pmt
> +usr/share/zsh/vendor-completions/_pbs2to3
> diff --git a/src/bin/pbs2to3.rs b/src/bin/pbs2to3.rs
> new file mode 100644
> index 00000000..97d064cd
> --- /dev/null
> +++ b/src/bin/pbs2to3.rs
> @@ -0,0 +1,602 @@
> +use std::io::Write;
> +use std::path::Path;
> +
> +use anyhow::{format_err, Error};
> +use regex::Regex;
> +use termcolor::{Color, ColorChoice, ColorSpec, StandardStream, WriteColor};
> +
> +use proxmox_apt::repositories::{self, APTRepositoryFile};
> +use proxmox_backup::api2::node::apt;
> +
> +const OLD_SUITE: &str = "bullseye";
> +const NEW_SUITE: &str = "bookworm";
> +const PROXMOX_BACKUP_META: &str = "proxmox-backup";
> +const MIN_PBS_MAJOR: u8 = 2;
> +const MIN_PBS_MINOR: u8 = 4;
> +const MIN_PBS_PKGREL: u8 = 1;
> +
> +fn main() -> Result<(), Error> {
> + let mut checker = Checker::new();
> + checker.check_pbs_packages()?;
> + checker.check_misc()?;
> + checker.summary()?;
> + Ok(())
> +}
> +
> +struct Checker {
> + output: ConsoleOutput,
> + upgraded: bool,
> +}
> +
> +impl Checker {
> + pub fn new() -> Self {
> + Self {
> + output: ConsoleOutput::new(),
> + upgraded: false,
> + }
> + }
> +
The `check_pbs_packages` function is a bit long and hard to read, maybe break this
into multiple subroutines? Generally I prefer to have functions that fit entirely on screen without
scrolling, unless there is a good reason against it.
> + pub fn check_pbs_packages(&mut self) -> Result<(), Error> {
> + self.output.print_header("CHECKING VERSION INFORMATION FOR PBS PACKAGES")?;
> + self.output.log_info("Checking for package updates..")?;
> +
> + let result = Self::get_upgradable_packages();
> + match result {
> + Err(err) => {
> + self.output.log_warn(format!("{}", err).as_str())?;
^
Nit: You could inline the variable here: `format!("{err}")`
Also applies to many other locations in the code, I'll refrain from highlighting the other occurences ;).
> + self.output.log_fail("unable to retrieve list of package updates!")?;
> + }
> + Ok(cache) => {
> + if cache.package_status.is_empty() {
> + self.output.log_pass("all packages up-to-date")?;
> + } else {
> + let pkgs = cache.package_status.iter()
> + .map(|pkg| format!("{}, ", pkg.package.clone()))
> + .collect::<String>()> + self.output.log_warn(
> + format!(
> + "updates for the following packages are available:\
> + \n {}",
> + &pkgs[..pkgs.len()-2]
> + ).as_str()
You could change the signatures for the `log_*` functions to
pub fn log_pass<T: AsRef<str>>(&mut self, message: T) -> Result<(), Error> {
self.log_line(LogLevel::Pass, message.as_ref())
}
-> then the function can take a `String` as a well as `&str`, allowing you to omit the
`as_str()` calls in various places.
> + )?;
> + }
> + }
> + }
> +
> + self.output.log_info("Checking proxmox backup server package version..")?;
> + let pkg_versions = apt::get_versions()?;
> +
> + let pbs_meta_pkg = pkg_versions.iter().find(|pkg| {
> + pkg.package.as_str() == PROXMOX_BACKUP_META
> + });
> +
> + if let Some(pbs_meta_pkg) = pbs_meta_pkg {
> + let pkg_version = Regex::new(r"^(\d+)\.(\d+)[.-](\d+)")?;
> + let captures = pkg_version.captures(&pbs_meta_pkg.old_version);
> + if let Some(captures) = captures {
> + let maj = Self::extract_version_from_captures(1, &captures)?;
> + let min = Self::extract_version_from_captures(2, &captures)?;
> + let pkgrel = Self::extract_version_from_captures(3, &captures)?;
> +
> + if maj > MIN_PBS_MAJOR {
> + self.output.log_pass(
> + format!("Already upgraded to Proxmox Backup Server {}", maj).as_str()
> + )?;
> + self.upgraded = true;
> + } else if maj >= MIN_PBS_MAJOR && min >= MIN_PBS_MINOR && pkgrel >= MIN_PBS_PKGREL {
> + self.output.log_pass(
> + format!(
> + "'{}' has version >= {}.{}-{}",
> + PROXMOX_BACKUP_META,
> + MIN_PBS_MAJOR,
> + MIN_PBS_MINOR,
> + MIN_PBS_PKGREL,
> + ).as_str()
> + )?;
> + } else {
> + self.output.log_fail(
> + format!(
> + "'{}' package is too old, please upgrade to >= {}.{}-{}",
> + PROXMOX_BACKUP_META,
> + MIN_PBS_MAJOR,
> + MIN_PBS_MINOR,
> + MIN_PBS_PKGREL,
> + ).as_str()
> + )?;
> + }
> + } else {
> + self.output.log_fail(
> + format!(
> + "could not match the '{}' package version, is it installed?",
> + PROXMOX_BACKUP_META,
> + ).as_str()
> + )?;
> + }
> +
> + self.output.log_info("Check running kernel version..")?;
> + let mut krunning = Regex::new(r"^6\.(?:2\.(?:[2-9]\d+|1[6-8]|1\d\d+)|5)[^~]*$")?;
> + let mut kinstalled = "pve-kernel-6.2";
> + if !self.upgraded {
> + krunning = Regex::new(r"^(?:5\.(?:13|15)|6\.2)")?;
> + kinstalled = "pve-kernel-5.15";
> + }
Nit: I'd probably write something like:
let (krunning, kinstalled) = if self.upgraded {
(
Regex::new(...),
"pve-kernel-..."
)
} else {
...
}
That way you can avoid the `mut` and also avoid compiling a regex that is not actually used.
> +
> + let mut command = std::process::Command::new("uname");
> + command.arg("-r");
> + match command.output() {
Nit: Also here you could avoid the `mut` and use the `Command` in a chained fashion:
let output = Command::new(..)
.arg(...)
.output();
match output {
...
}
> + Err(_err) => self.output.log_fail("unable to determine running kernel version.")?,
> + Ok(ret) => {
> + let running_version = std::str::from_utf8(&ret.stdout[..ret.stdout.len()-1])?;
> + if krunning.is_match(running_version) {
> + if self.upgraded {
> + self.output.log_pass(
> + format!(
> + "running new kernel '{}' after upgrade.",
> + running_version,
> + ).as_str()
> + )?;
> + } else {
> + self.output.log_pass(
> + format!(
> + "running kernel '{}' is considered suitable for upgrade.",
> + running_version,
> + ).as_str()
> + )?;
> + }
> + } else {
> + let installed_kernel = pkg_versions.iter().find(|pkg| {
> + pkg.package.as_str() == kinstalled
> + });
> + if installed_kernel.is_some() {
> + self.output.log_warn(
> + format!(
> + "a suitable kernel '{}' is installed, but an unsuitable '{}' \
> + is booted, missing reboot?!",
> + kinstalled,
> + running_version,
> + ).as_str()
> + )?;
> + } else {
> + self.output.log_warn(
> + format!(
> + "unexpected running and installed kernel '{}'.",
> + running_version,
> + ).as_str()
> + )?;
> + }
> + }
> + }
> + }
> +
> + } else {
> + self.output.log_fail(format!("'{}' package not found!", PROXMOX_BACKUP_META).as_str())?;
> + }
> +
> + Ok(())
> + }
> +
> + fn extract_version_from_captures(index: usize, captures: ®ex::Captures) -> Result<u8, Error> {
> + if let Some(capture) = captures.get(index) {
> + let val = capture.as_str().parse::<u8>()?;
> + Ok(val)
> + } else {
> + Ok(0)
> + }
> + }
> +
> + fn check_bootloader(&mut self) -> Result<(), Error> {
> + self.output.log_info("Checking bootloader configuration...")?;
> +
> + // PBS packages version check needs to be run before
> + if !self.upgraded {
> + self.output.log_skip("not yet upgraded, no need to check the presence of systemd-boot")?;
> + }
> +
> + if !Path::is_file(Path::new("/etc/kernel/proxmox-boot-uuids")) {
^
`Path::is_file(&self)` takes `&self` as parameter, so I'd just write:
`if !Path::new("...").is_file() {`
> + self.output.log_skip("proxmox-boot-tool not used for bootloader configuration")?;
> + return Ok(());
> + }
> +
> + if !Path::is_dir(Path::new("/sys/firmware/efi")) {
Here as well.
> + self.output.log_skip("System booted in legacy-mode - no need for systemd-boot")?;
> + return Ok(());
> + }/
> +
> + if Path::is_file(Path::new("/usr/share/doc/systemd-boot/changelog.Debian.gz")) {
Here as well.
> + self.output.log_pass("systemd-boot is installed")?;
> + } else {
> + self.output.log_warn(
> + "proxmox-boot-tool is used for bootloader configuration in uefi mode \
> + but the separate systemd-boot package, existing in Debian Bookworm \
> + is not installed.\n\
> + initializing new ESPs will not work unitl the package is installed."
> + )?;
> + }
> + Ok(())
> + }
> +
> + fn check_apt_repos(&mut self) -> Result<(), Error> {
> + self.output.log_info("Checking for package repository suite mismatches..")?;
> +
> + let mut strange = false;
> + let mut mismatches = Vec::new();
> +
> + let (repo_files, _repo_errors, _digest) = repositories::repositories()?;
> + for repo_file in repo_files {
> + let (found_strange, mut found_mismatches) = self.check_repo_file(repo_file)?;
> + if found_strange {
> + strange = true;
> + }
> + mismatches.append(&mut found_mismatches);
> + }
> +
> + match (mismatches.is_empty(), strange) {
> + (true, false) => self.output.log_pass("found no suite mismatch")?,
> + (true, true) => self.output.log_notice(
> + "found no suite mismatches, but found at least one strange suite"
> + )?,
> + (false, _) => {
> + let mut message = String::from(
> + "Found mixed old and new packages repository suites, fix before upgrading!\
> + \n Mismatches:"
> + );
> + for (suite, location) in mismatches.iter() {
> + message.push_str(
> + format!("\n found suite '{}' at '{}'", suite, location).as_str()
> + );
> + }
> + message.push('\n');
> + self.output.log_fail(message.as_str())?
> + }
> + }
> +
> + Ok(())
> + }
> +
> + pub fn check_misc(&mut self) -> Result<(), Error> {
> + self.output.print_header("MISCELLANEOUS CHECKS")?;
> + self.check_pbs_services()?;
> + self.check_time_sync()?;
> + self.check_apt_repos()?;
> + self.check_bootloader()?;
> + Ok(())
> + }
> +
> + pub fn summary(&mut self) -> Result<(), Error> {
> + self.output.print_summary()
> + }
> +
The function below is deeply nested (up to 7 levels, if I counted correctly), maybe break
stuff out into smaller functions?
> + fn check_repo_file(
> + &mut self,
> + repo_file: APTRepositoryFile
> + ) -> Result<(bool, Vec<(String, String)>), Error> {
> + let deb_suite = Regex::new(r"(\w+)(-updates|-backports|-security)?$")?;
> + let mut strange_suite = false;
> + let mut found_suite: Option<(String, String)> = None;
> + let mut mismatches = Vec::new();
> +
> + for repo in repo_file.repositories {
> + for suite in &repo.suites {
> + if let Some(suite) = deb_suite.captures(suite) {
> + if let Some(suite_match) = suite.get(1) {
> + let suite = suite_match.as_str();
> + let location = repo_file.path.clone().unwrap_or(String::new());
^
clippy: You can use `unwrap_or_default` here.
> +
> + if suite != OLD_SUITE && suite != NEW_SUITE {
> + self.output.log_notice(format!(
> + "found unusual suite '{}', neighter old '{}' nor new '{}'..\
> + \n Affected file {}\
> + \n Please assure this is shipping compatible packages for \
> + the upgrade!",
> + suite,
> + OLD_SUITE,
> + NEW_SUITE,
> + location,
> + ).as_str())?;
> + strange_suite = true;
> + continue;
> + }
> +
> + if let Some((ref current_suite, ref current_location)) = found_suite {
> + if suite != current_suite {
> + if mismatches.is_empty() {
> + mismatches.push(
> + (current_suite.clone(), current_location.clone())
> + );
> + mismatches.push((suite.to_string(), location));
> + } else {
> + mismatches.push((suite.to_string(), location));
> + }
> + }
> + } else {
> + found_suite = Some((suite.to_string(), location));
> + }
> + }
> + }
> + }
> + }
> +
> + Ok((strange_suite, mismatches))
> + }
> +
> + fn get_systemd_unit_state(
> + &mut self,
> + unit: &str,
> + ) -> Result<(SystemdUnitState, SystemdUnitState), Error> {
> + let mut command = std::process::Command::new("systemctl");
> + command.arg("is-enabled");
> + command.arg(unit);
> + let output = command.output().map_err(|err| {
> + format_err!("failed to execute {:?} - {}", command, err)
> + })?;
> +
> + let is_enabled_state = match output.stdout.as_slice() {
> + b"enabled\n" => SystemdUnitState::Enabled,
> + b"disabled\n" => SystemdUnitState::Disabled,
> + _ => SystemdUnitState::Unknown,
> + };
> +
> + let mut command = std::process::Command::new("systemctl");
> + command.arg("is-active");
> + command.arg(unit);
> + let output = command.output().map_err(|err| {
> + format_err!("failed to execute {:?} - {}", command, err)
> + })?;
> +
> + let is_active_state = match output.stdout.as_slice() {
> + b"active\n" => SystemdUnitState::Active,
> + b"inactive\n" => SystemdUnitState::Inactive,
> + b"failed\n" => SystemdUnitState::Failed,
> + _ => SystemdUnitState::Unknown,
> + };
> + Ok((is_enabled_state, is_active_state))
> + }
> +
> + fn check_pbs_services(&mut self) -> Result<(), Error> {
> + self.output.log_info("Checking pbs daemon services..")?;
> +
> + for service in ["proxmox-backup.service", "proxmox-backup-proxy.service"] {
> + match self.get_systemd_unit_state(service)? {
> + (_, SystemdUnitState::Active) => {
> + self.output.log_pass(
> + format!("systemd unit '{}' is in state 'active'", service).as_str()
> + )?;
> + }
> + (_, SystemdUnitState::Inactive) => {
> + self.output.log_fail(
> + format!(
> + "systemd unit '{}' is in state 'inactive'\
> + \n Please check the service for errors and start it.",
> + service,
> + ).as_str()
> + )?;
> + }
> + (_, SystemdUnitState::Failed) => {
> + self.output.log_fail(
> + format!(
> + "systemd unit '{}' is in state 'failed'\
> + \n Please check the service for errors and start it.",
> + service,
> + ).as_str()
> + )?;
> + }
> + (_, _) => {
> + self.output.log_fail(
> + format!(
> + "systemd unit '{}' is not in state 'active'\
> + \n Please check the service for errors and start it.",
> + service,
> + ).as_str()
> + )?;
> + }
> + }
> + }
> + Ok(())
> + }
> +
> + fn check_time_sync(&mut self) -> Result<(), Error> {
> + self.output.log_info("Checking for supported & active NTP service..")?;
> + if self.get_systemd_unit_state("systemd-timesyncd.service")?.1 == SystemdUnitState::Active {
> + self.output.log_warn(
> + "systemd-timesyncd is not the best choice for time-keeping on servers, due to only \
> + applying updates on boot.\
> + \n While not necessary for the upgrade it's recommended to use one of:\
> + \n * chrony (Default in new Proxmox VE installations)\
> + \n * ntpsec\
> + \n * openntpd"
> + )?;
> + } else if self.get_systemd_unit_state("ntp.service")?.1 == SystemdUnitState::Active {
> + self.output.log_info(
> + "Debian deprecated and removed the ntp package for Bookworm, but the system \
> + will automatically migrate to the 'ntpsec' replacement package on upgrade."
> + )?;
> + } else if self.get_systemd_unit_state("chrony.service")?.1 == SystemdUnitState::Active ||
> + self.get_systemd_unit_state("openntpd.service")?.1 == SystemdUnitState::Active ||
> + self.get_systemd_unit_state("ntpsec.service")?.1 == SystemdUnitState::Active
> + {
> + self.output.log_pass("Detected active time synchronisation unit")?;
> + } else {
> + self.output.log_warn(
> + "No (active) time synchronisation daemon (NTP) detected, but synchronized systems \
> + are important!"
> + )?;
> + }
> + Ok(())
> + }
> +
> + fn get_upgradable_packages() -> Result<proxmox_backup::tools::apt::PkgState, Error> {
> + let cache = if let Ok(false) = proxmox_backup::tools::apt::pkg_cache_expired() {
> + if let Ok(Some(cache)) = proxmox_backup::tools::apt::read_pkg_state() {
> + cache
> + } else {
> + proxmox_backup::tools::apt::update_cache()?
> + }
> + } else {
> + proxmox_backup::tools::apt::update_cache()?
> + };
> +
> + Ok(cache)
> + }
> +}
> +
> +#[derive(PartialEq)]
> +enum SystemdUnitState {
> + Active,
> + Enabled,
> + Disabled,
> + Failed,
> + Inactive,
> + Unknown,
> +}
> +
> +#[derive(Default)]
> +struct Counters {
> + pass: u64,
> + skip: u64,
> + notice: u64,
> + warn: u64,
> + fail: u64,
> +}
> +
> +enum LogLevel {
> + Pass,
> + Info,
> + Skip,
> + Notice,
> + Warn,
> + Fail,
> +}
> +
> +struct ConsoleOutput {
> + stream: StandardStream,
> + first_header: bool,
> + counters: Counters,
> +}
> +
> +impl ConsoleOutput {
> + pub fn new() -> Self {
> + Self {
> + stream: StandardStream::stdout(ColorChoice::Always),
> + first_header: true,
> + counters: Counters::default(),
> + }
> + }
> +
> + pub fn print_header(&mut self, message: &str) -> Result<(), Error> {
> + if !self.first_header {
> + writeln!(&mut self.stream, "")?;
^
clippy: You can remove the empty string if you just want to write a new line.
> + }
> + self.first_header = false;
> + writeln!(&mut self.stream, "= {message} =\n")?;
> + Ok(())
> + }
> +
> + pub fn set_color(&mut self, color: Color) -> Result<(), Error> {
> + self.stream.set_color(ColorSpec::new().set_fg(Some(color)))?;
> + Ok(())
> + }
> +
> + pub fn log_line(&mut self, level: LogLevel, message: &str) -> Result<(), Error> {
> + match level {
> + LogLevel::Pass => {
> + self.counters.pass += 1;
> + self.set_color(Color::Green)?;
> + writeln!(&mut self.stream, "PASS: {}", message)?;
> + self.set_color(Color::White)?;
> + }
> + LogLevel::Info => {
> + writeln!(&mut self.stream, "INFO: {}", message)?;
> + }
> + LogLevel::Skip => {
> + self.counters.skip += 1;
> + writeln!(&mut self.stream, "SKIP: {}", message)?;
> + }
> + LogLevel::Notice => {
> + self.counters.notice += 1;
> + writeln!(&mut self.stream, "NOTICE: {}", message)?;
> + }
> + LogLevel::Warn=> {
> + self.counters.warn += 1;
> + self.set_color(Color::Yellow)?;
> + writeln!(&mut self.stream, "WARN: {}", message)?;
> + self.set_color(Color::White)?;
> + }
> + LogLevel::Fail => {
> + self.counters.fail += 1;
> + self.set_color(Color::Red)?;
> + writeln!(&mut self.stream, "FAIL: {}", message)?;
> + self.set_color(Color::White)?;
> + }
> + }
> + Ok(())
> + }
> +
> + pub fn log_pass(&mut self, message: &str) -> Result<(), Error> {
> + self.log_line(LogLevel::Pass, message)
> + }
> +
> + pub fn log_info(&mut self, message: &str) -> Result<(), Error> {
> + self.log_line(LogLevel::Info, message)
> + }
> +
> + pub fn log_skip(&mut self, message: &str) -> Result<(), Error> {
> + self.log_line(LogLevel::Skip, message)
> + }
> +
> + pub fn log_notice(&mut self, message: &str) -> Result<(), Error> {
> + self.log_line(LogLevel::Notice, message)
> + }
> +
> + pub fn log_warn(&mut self, message: &str) -> Result<(), Error> {
> + self.log_line(LogLevel::Warn, message)
> + }
> +
> + pub fn log_fail(&mut self, message: &str) -> Result<(), Error> {
> + self.log_line(LogLevel::Fail, message)
> + }
> +
> + pub fn print_summary(&mut self) -> Result<(), Error> {
> + self.print_header("SUMMARY")?;
> +
> + let total = self.counters.fail + self.counters.pass + self.counters.notice +
> + self.counters.skip + self.counters.warn;
> +
> + self.set_color(Color::White)?;
> + writeln!(&mut self.stream, "TOTAL: {total}")?;
> + self.set_color(Color::Green)?;
> + writeln!(&mut self.stream, "PASSED: {}", self.counters.pass)?;
> + self.set_color(Color::White)?;
> + writeln!(&mut self.stream, "SKIPPED: {}", self.counters.skip)?;
> + writeln!(&mut self.stream, "NOTICE: {}", self.counters.notice)?;
> + if self.counters.warn > 0 {
> + self.set_color(Color::Yellow)?;
> + writeln!(&mut self.stream, "WARNINGS: {}", self.counters.warn)?;
> + }
> + if self.counters.fail > 0 {
> + self.set_color(Color::Red)?;
> + writeln!(&mut self.stream, "FAILURES: {}", self.counters.fail)?;
> + }
> + if self.counters.warn > 0 || self.counters.fail > 0 {
> + let mut color = Color::Yellow;
> + if self.counters.fail > 0 {
> + color = Color::Red;
> + }
> +
> + self.set_color(color)?;
> + writeln!(
> + &mut self.stream,
> + "\nATTENTION: Please check the output for detailed information!",
> + )?;
> + if self.counters.fail > 0 {
> + writeln!(
> + &mut self.stream,
> + "Try to solve the problems one at a time and rerun this checklist tool again.",
> + )?;
> + }
> + }
> + self.set_color(Color::White)?;
> + Ok(())
> + }
> +}
> +
> diff --git a/zsh-completions/_pbs2to3 b/zsh-completions/_pbs2to3
> new file mode 100644
> index 00000000..f6dc3d67
> --- /dev/null
> +++ b/zsh-completions/_pbs2to3
> @@ -0,0 +1,13 @@
> +#compdef _pbs2to3() pbs2to3
> +
> +function _pbs2to3() {
> + local cwords line point cmd curr prev
> + cwords=${#words[@]}
> + line=$words
> + point=${#line}
> + cmd=${words[1]}
> + curr=${words[cwords]}
> + prev=${words[cwords-1]}
> + compadd -- $(COMP_CWORD="$cwords" COMP_LINE="$line" COMP_POINT="$point" \
> + pbs2to3 bashcomplete "$cmd" "$curr" "$prev")
> +}
--
- Lukas
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pbs-devel] [PATCH backup stable-2] pbs2to3: add upgrade checker binary
2023-06-28 10:13 ` Lukas Wagner
@ 2023-06-28 10:14 ` Lukas Wagner
2023-06-28 10:17 ` Christian Ebner
0 siblings, 1 reply; 6+ messages in thread
From: Lukas Wagner @ 2023-06-28 10:14 UTC (permalink / raw)
To: Proxmox Backup Server development discussion, Christian Ebner
Oh, seems that I have missed v2 - but I think many comments still apply.
On 6/28/23 12:13, Lukas Wagner wrote:
> Hi,
>
> tested on current `stable-2` branch. Seems to work just fine, at least on a clean PBS installation.
>
> Some comments regarding the code are inline (nothing major though).
>
> Tested-by: Lukas Wagner <l.wagner@proxmox.com>
> Reviewed-by: Lukas Wagner <l.wagner@proxmox.com>
>
> On 6/28/23 10:10, Christian Ebner wrote:
>> Adds the pbs2to3 upgrade checker with some basic checks.
>>
>> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
>> ---
>> Makefile | 1 +
>> debian/proxmox-backup-server.install | 2 +
>> src/bin/pbs2to3.rs | 602 +++++++++++++++++++++++++++
>> zsh-completions/_pbs2to3 | 13 +
>> 4 files changed, 618 insertions(+)
>> create mode 100644 src/bin/pbs2to3.rs
>> create mode 100644 zsh-completions/_pbs2to3
>>
>> diff --git a/Makefile b/Makefile
>> index b307009d..7477935d 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -19,6 +19,7 @@ USR_BIN := \
>> USR_SBIN := \
>> proxmox-backup-manager \
>> proxmox-backup-debug \
>> + pbs2to3
>> # Binaries for services:
>> SERVICE_BIN := \
>> diff --git a/debian/proxmox-backup-server.install b/debian/proxmox-backup-server.install
>> index 76f50cd0..ebe51aae 100644
>> --- a/debian/proxmox-backup-server.install
>> +++ b/debian/proxmox-backup-server.install
>> @@ -11,6 +11,7 @@ usr/lib/x86_64-linux-gnu/proxmox-backup/proxmox-daily-update
>> usr/lib/x86_64-linux-gnu/proxmox-backup/sg-tape-cmd
>> usr/sbin/proxmox-backup-debug
>> usr/sbin/proxmox-backup-manager
>> +usr/sbin/pbs2to3
>> usr/bin/pmtx
>> usr/bin/pmt
>> usr/bin/proxmox-tape
>> @@ -39,3 +40,4 @@ usr/share/zsh/vendor-completions/_proxmox-backup-manager
>> usr/share/zsh/vendor-completions/_proxmox-tape
>> usr/share/zsh/vendor-completions/_pmtx
>> usr/share/zsh/vendor-completions/_pmt
>> +usr/share/zsh/vendor-completions/_pbs2to3
>> diff --git a/src/bin/pbs2to3.rs b/src/bin/pbs2to3.rs
>> new file mode 100644
>> index 00000000..97d064cd
>> --- /dev/null
>> +++ b/src/bin/pbs2to3.rs
>> @@ -0,0 +1,602 @@
>> +use std::io::Write;
>> +use std::path::Path;
>> +
>> +use anyhow::{format_err, Error};
>> +use regex::Regex;
>> +use termcolor::{Color, ColorChoice, ColorSpec, StandardStream, WriteColor};
>> +
>> +use proxmox_apt::repositories::{self, APTRepositoryFile};
>> +use proxmox_backup::api2::node::apt;
>> +
>> +const OLD_SUITE: &str = "bullseye";
>> +const NEW_SUITE: &str = "bookworm";
>> +const PROXMOX_BACKUP_META: &str = "proxmox-backup";
>> +const MIN_PBS_MAJOR: u8 = 2;
>> +const MIN_PBS_MINOR: u8 = 4;
>> +const MIN_PBS_PKGREL: u8 = 1;
>> +
>> +fn main() -> Result<(), Error> {
>> + let mut checker = Checker::new();
>> + checker.check_pbs_packages()?;
>> + checker.check_misc()?;
>> + checker.summary()?;
>> + Ok(())
>> +}
>> +
>> +struct Checker {
>> + output: ConsoleOutput,
>> + upgraded: bool,
>> +}
>> +
>> +impl Checker {
>> + pub fn new() -> Self {
>> + Self {
>> + output: ConsoleOutput::new(),
>> + upgraded: false,
>> + }
>> + }
>> +
>
> The `check_pbs_packages` function is a bit long and hard to read, maybe break this
> into multiple subroutines? Generally I prefer to have functions that fit entirely on screen without
> scrolling, unless there is a good reason against it.
>> + pub fn check_pbs_packages(&mut self) -> Result<(), Error> {
>> + self.output.print_header("CHECKING VERSION INFORMATION FOR PBS PACKAGES")?;
>> + self.output.log_info("Checking for package updates..")?;
>> +
>> + let result = Self::get_upgradable_packages();
>> + match result {
>> + Err(err) => {
>> + self.output.log_warn(format!("{}", err).as_str())?;
> ^
> Nit: You could inline the variable here: `format!("{err}")`
> Also applies to many other locations in the code, I'll refrain from highlighting the other occurences ;).
>
>
>> + self.output.log_fail("unable to retrieve list of package updates!")?;
>> + }
>> + Ok(cache) => {
>> + if cache.package_status.is_empty() {
>> + self.output.log_pass("all packages up-to-date")?;
>> + } else {
>> + let pkgs = cache.package_status.iter()
>> + .map(|pkg| format!("{}, ", pkg.package.clone()))
>> + .collect::<String>()> + self.output.log_warn(
>> + format!(
>> + "updates for the following packages are available:\
>> + \n {}",
>> + &pkgs[..pkgs.len()-2]
>> + ).as_str()
> You could change the signatures for the `log_*` functions to
>
> pub fn log_pass<T: AsRef<str>>(&mut self, message: T) -> Result<(), Error> {
> self.log_line(LogLevel::Pass, message.as_ref())
> }
>
> -> then the function can take a `String` as a well as `&str`, allowing you to omit the
> `as_str()` calls in various places.
>
>> + )?;
>> + }
>> + }
>> + }
>> +
>> + self.output.log_info("Checking proxmox backup server package version..")?;
>> + let pkg_versions = apt::get_versions()?;
>> +
>> + let pbs_meta_pkg = pkg_versions.iter().find(|pkg| {
>> + pkg.package.as_str() == PROXMOX_BACKUP_META
>> + });
>> +
>> + if let Some(pbs_meta_pkg) = pbs_meta_pkg {
>> + let pkg_version = Regex::new(r"^(\d+)\.(\d+)[.-](\d+)")?;
>> + let captures = pkg_version.captures(&pbs_meta_pkg.old_version);
>> + if let Some(captures) = captures {
>> + let maj = Self::extract_version_from_captures(1, &captures)?;
>> + let min = Self::extract_version_from_captures(2, &captures)?;
>> + let pkgrel = Self::extract_version_from_captures(3, &captures)?;
>> +
>> + if maj > MIN_PBS_MAJOR {
>> + self.output.log_pass(
>> + format!("Already upgraded to Proxmox Backup Server {}", maj).as_str()
>> + )?;
>> + self.upgraded = true;
>> + } else if maj >= MIN_PBS_MAJOR && min >= MIN_PBS_MINOR && pkgrel >= MIN_PBS_PKGREL {
>> + self.output.log_pass(
>> + format!(
>> + "'{}' has version >= {}.{}-{}",
>> + PROXMOX_BACKUP_META,
>> + MIN_PBS_MAJOR,
>> + MIN_PBS_MINOR,
>> + MIN_PBS_PKGREL,
>> + ).as_str()
>> + )?;
>> + } else {
>> + self.output.log_fail(
>> + format!(
>> + "'{}' package is too old, please upgrade to >= {}.{}-{}",
>> + PROXMOX_BACKUP_META,
>> + MIN_PBS_MAJOR,
>> + MIN_PBS_MINOR,
>> + MIN_PBS_PKGREL,
>> + ).as_str()
>> + )?;
>> + }
>> + } else {
>> + self.output.log_fail(
>> + format!(
>> + "could not match the '{}' package version, is it installed?",
>> + PROXMOX_BACKUP_META,
>> + ).as_str()
>> + )?;
>> + }
>> +
>> + self.output.log_info("Check running kernel version..")?;
>> + let mut krunning = Regex::new(r"^6\.(?:2\.(?:[2-9]\d+|1[6-8]|1\d\d+)|5)[^~]*$")?;
>> + let mut kinstalled = "pve-kernel-6.2";
>> + if !self.upgraded {
>> + krunning = Regex::new(r"^(?:5\.(?:13|15)|6\.2)")?;
>> + kinstalled = "pve-kernel-5.15";
>> + }
>
> Nit: I'd probably write something like:
>
> let (krunning, kinstalled) = if self.upgraded {
> (
> Regex::new(...),
> "pve-kernel-..."
> )
> } else {
> ...
> }
>
> That way you can avoid the `mut` and also avoid compiling a regex that is not actually used.
>
>> +
>> + let mut command = std::process::Command::new("uname");
>> + command.arg("-r");
>> + match command.output() {
>
> Nit: Also here you could avoid the `mut` and use the `Command` in a chained fashion:
>
> let output = Command::new(..)
> .arg(...)
> .output();
>
> match output {
> ...
> }
>
>> + Err(_err) => self.output.log_fail("unable to determine running kernel version.")?,
>> + Ok(ret) => {
>> + let running_version = std::str::from_utf8(&ret.stdout[..ret.stdout.len()-1])?;
>> + if krunning.is_match(running_version) {
>> + if self.upgraded {
>> + self.output.log_pass(
>> + format!(
>> + "running new kernel '{}' after upgrade.",
>> + running_version,
>> + ).as_str()
>> + )?;
>> + } else {
>> + self.output.log_pass(
>> + format!(
>> + "running kernel '{}' is considered suitable for upgrade.",
>> + running_version,
>> + ).as_str()
>> + )?;
>> + }
>> + } else {
>> + let installed_kernel = pkg_versions.iter().find(|pkg| {
>> + pkg.package.as_str() == kinstalled
>> + });
>> + if installed_kernel.is_some() {
>> + self.output.log_warn(
>> + format!(
>> + "a suitable kernel '{}' is installed, but an unsuitable '{}' \
>> + is booted, missing reboot?!",
>> + kinstalled,
>> + running_version,
>> + ).as_str()
>> + )?;
>> + } else {
>> + self.output.log_warn(
>> + format!(
>> + "unexpected running and installed kernel '{}'.",
>> + running_version,
>> + ).as_str()
>> + )?;
>> + }
>> + }
>> + }
>> + }
>> +
>> + } else {
>> + self.output.log_fail(format!("'{}' package not found!", PROXMOX_BACKUP_META).as_str())?;
>> + }
>> +
>> + Ok(())
>> + }
>> +
>> + fn extract_version_from_captures(index: usize, captures: ®ex::Captures) -> Result<u8, Error> {
>> + if let Some(capture) = captures.get(index) {
>> + let val = capture.as_str().parse::<u8>()?;
>> + Ok(val)
>> + } else {
>> + Ok(0)
>> + }
>> + }
>> +
>> + fn check_bootloader(&mut self) -> Result<(), Error> {
>> + self.output.log_info("Checking bootloader configuration...")?;
>> +
>> + // PBS packages version check needs to be run before
>> + if !self.upgraded {
>> + self.output.log_skip("not yet upgraded, no need to check the presence of systemd-boot")?;
>> + }
>> +
>> + if !Path::is_file(Path::new("/etc/kernel/proxmox-boot-uuids")) {
> ^
> `Path::is_file(&self)` takes `&self` as parameter, so I'd just write:
>
> `if !Path::new("...").is_file() {`
>
>> + self.output.log_skip("proxmox-boot-tool not used for bootloader configuration")?;
>> + return Ok(());
>> + }
>> +
>> + if !Path::is_dir(Path::new("/sys/firmware/efi")) {
> Here as well.
>> + self.output.log_skip("System booted in legacy-mode - no need for systemd-boot")?;
>> + return Ok(());
>> + }/
>> +
>> + if Path::is_file(Path::new("/usr/share/doc/systemd-boot/changelog.Debian.gz")) {
> Here as well.
>> + self.output.log_pass("systemd-boot is installed")?;
>> + } else {
>> + self.output.log_warn(
>> + "proxmox-boot-tool is used for bootloader configuration in uefi mode \
>> + but the separate systemd-boot package, existing in Debian Bookworm \
>> + is not installed.\n\
>> + initializing new ESPs will not work unitl the package is installed."
>> + )?;
>> + }
>> + Ok(())
>> + }
>> +
>> + fn check_apt_repos(&mut self) -> Result<(), Error> {
>> + self.output.log_info("Checking for package repository suite mismatches..")?;
>> +
>> + let mut strange = false;
>> + let mut mismatches = Vec::new();
>> +
>> + let (repo_files, _repo_errors, _digest) = repositories::repositories()?;
>> + for repo_file in repo_files {
>> + let (found_strange, mut found_mismatches) = self.check_repo_file(repo_file)?;
>> + if found_strange {
>> + strange = true;
>> + }
>> + mismatches.append(&mut found_mismatches);
>> + }
>> +
>> + match (mismatches.is_empty(), strange) {
>> + (true, false) => self.output.log_pass("found no suite mismatch")?,
>> + (true, true) => self.output.log_notice(
>> + "found no suite mismatches, but found at least one strange suite"
>> + )?,
>> + (false, _) => {
>> + let mut message = String::from(
>> + "Found mixed old and new packages repository suites, fix before upgrading!\
>> + \n Mismatches:"
>> + );
>> + for (suite, location) in mismatches.iter() {
>> + message.push_str(
>> + format!("\n found suite '{}' at '{}'", suite, location).as_str()
>> + );
>> + }
>> + message.push('\n');
>> + self.output.log_fail(message.as_str())?
>> + }
>> + }
>> +
>> + Ok(())
>> + }
>> +
>> + pub fn check_misc(&mut self) -> Result<(), Error> {
>> + self.output.print_header("MISCELLANEOUS CHECKS")?;
>> + self.check_pbs_services()?;
>> + self.check_time_sync()?;
>> + self.check_apt_repos()?;
>> + self.check_bootloader()?;
>> + Ok(())
>> + }
>> +
>> + pub fn summary(&mut self) -> Result<(), Error> {
>> + self.output.print_summary()
>> + }
>> +
>
> The function below is deeply nested (up to 7 levels, if I counted correctly), maybe break
> stuff out into smaller functions?
>> + fn check_repo_file(
>> + &mut self,
>> + repo_file: APTRepositoryFile
>> + ) -> Result<(bool, Vec<(String, String)>), Error> {
>> + let deb_suite = Regex::new(r"(\w+)(-updates|-backports|-security)?$")?;
>> + let mut strange_suite = false;
>> + let mut found_suite: Option<(String, String)> = None;
>> + let mut mismatches = Vec::new();
>> +
>> + for repo in repo_file.repositories {
>> + for suite in &repo.suites {
>> + if let Some(suite) = deb_suite.captures(suite) {
>> + if let Some(suite_match) = suite.get(1) {
>> + let suite = suite_match.as_str();
>> + let location = repo_file.path.clone().unwrap_or(String::new());
> ^
> clippy: You can use `unwrap_or_default` here.
>
>> +
>> + if suite != OLD_SUITE && suite != NEW_SUITE {
>> + self.output.log_notice(format!(
>> + "found unusual suite '{}', neighter old '{}' nor new '{}'..\
>> + \n Affected file {}\
>> + \n Please assure this is shipping compatible packages for \
>> + the upgrade!",
>> + suite,
>> + OLD_SUITE,
>> + NEW_SUITE,
>> + location,
>> + ).as_str())?;
>> + strange_suite = true;
>> + continue;
>> + }
>> +
>> + if let Some((ref current_suite, ref current_location)) = found_suite {
>> + if suite != current_suite {
>> + if mismatches.is_empty() {
>> + mismatches.push(
>> + (current_suite.clone(), current_location.clone())
>> + );
>> + mismatches.push((suite.to_string(), location));
>> + } else {
>> + mismatches.push((suite.to_string(), location));
>> + }
>> + }
>> + } else {
>> + found_suite = Some((suite.to_string(), location));
>> + }
>> + }
>> + }
>> + }
>> + }
>> +
>> + Ok((strange_suite, mismatches))
>> + }
>> +
>> + fn get_systemd_unit_state(
>> + &mut self,
>> + unit: &str,
>> + ) -> Result<(SystemdUnitState, SystemdUnitState), Error> {
>> + let mut command = std::process::Command::new("systemctl");
>> + command.arg("is-enabled");
>> + command.arg(unit);
>> + let output = command.output().map_err(|err| {
>> + format_err!("failed to execute {:?} - {}", command, err)
>> + })?;
>> +
>> + let is_enabled_state = match output.stdout.as_slice() {
>> + b"enabled\n" => SystemdUnitState::Enabled,
>> + b"disabled\n" => SystemdUnitState::Disabled,
>> + _ => SystemdUnitState::Unknown,
>> + };
>> +
>> + let mut command = std::process::Command::new("systemctl");
>> + command.arg("is-active");
>> + command.arg(unit);
>> + let output = command.output().map_err(|err| {
>> + format_err!("failed to execute {:?} - {}", command, err)
>> + })?;
>> +
>> + let is_active_state = match output.stdout.as_slice() {
>> + b"active\n" => SystemdUnitState::Active,
>> + b"inactive\n" => SystemdUnitState::Inactive,
>> + b"failed\n" => SystemdUnitState::Failed,
>> + _ => SystemdUnitState::Unknown,
>> + };
>> + Ok((is_enabled_state, is_active_state))
>> + }
>> +
>> + fn check_pbs_services(&mut self) -> Result<(), Error> {
>> + self.output.log_info("Checking pbs daemon services..")?;
>> +
>> + for service in ["proxmox-backup.service", "proxmox-backup-proxy.service"] {
>> + match self.get_systemd_unit_state(service)? {
>> + (_, SystemdUnitState::Active) => {
>> + self.output.log_pass(
>> + format!("systemd unit '{}' is in state 'active'", service).as_str()
>> + )?;
>> + }
>> + (_, SystemdUnitState::Inactive) => {
>> + self.output.log_fail(
>> + format!(
>> + "systemd unit '{}' is in state 'inactive'\
>> + \n Please check the service for errors and start it.",
>> + service,
>> + ).as_str()
>> + )?;
>> + }
>> + (_, SystemdUnitState::Failed) => {
>> + self.output.log_fail(
>> + format!(
>> + "systemd unit '{}' is in state 'failed'\
>> + \n Please check the service for errors and start it.",
>> + service,
>> + ).as_str()
>> + )?;
>> + }
>> + (_, _) => {
>> + self.output.log_fail(
>> + format!(
>> + "systemd unit '{}' is not in state 'active'\
>> + \n Please check the service for errors and start it.",
>> + service,
>> + ).as_str()
>> + )?;
>> + }
>> + }
>> + }
>> + Ok(())
>> + }
>> +
>> + fn check_time_sync(&mut self) -> Result<(), Error> {
>> + self.output.log_info("Checking for supported & active NTP service..")?;
>> + if self.get_systemd_unit_state("systemd-timesyncd.service")?.1 == SystemdUnitState::Active {
>> + self.output.log_warn(
>> + "systemd-timesyncd is not the best choice for time-keeping on servers, due to only \
>> + applying updates on boot.\
>> + \n While not necessary for the upgrade it's recommended to use one of:\
>> + \n * chrony (Default in new Proxmox VE installations)\
>> + \n * ntpsec\
>> + \n * openntpd"
>> + )?;
>> + } else if self.get_systemd_unit_state("ntp.service")?.1 == SystemdUnitState::Active {
>> + self.output.log_info(
>> + "Debian deprecated and removed the ntp package for Bookworm, but the system \
>> + will automatically migrate to the 'ntpsec' replacement package on upgrade."
>> + )?;
>> + } else if self.get_systemd_unit_state("chrony.service")?.1 == SystemdUnitState::Active ||
>> + self.get_systemd_unit_state("openntpd.service")?.1 == SystemdUnitState::Active ||
>> + self.get_systemd_unit_state("ntpsec.service")?.1 == SystemdUnitState::Active
>> + {
>> + self.output.log_pass("Detected active time synchronisation unit")?;
>> + } else {
>> + self.output.log_warn(
>> + "No (active) time synchronisation daemon (NTP) detected, but synchronized systems \
>> + are important!"
>> + )?;
>> + }
>> + Ok(())
>> + }
>> +
>> + fn get_upgradable_packages() -> Result<proxmox_backup::tools::apt::PkgState, Error> {
>> + let cache = if let Ok(false) = proxmox_backup::tools::apt::pkg_cache_expired() {
>> + if let Ok(Some(cache)) = proxmox_backup::tools::apt::read_pkg_state() {
>> + cache
>> + } else {
>> + proxmox_backup::tools::apt::update_cache()?
>> + }
>> + } else {
>> + proxmox_backup::tools::apt::update_cache()?
>> + };
>> +
>> + Ok(cache)
>> + }
>> +}
>> +
>> +#[derive(PartialEq)]
>> +enum SystemdUnitState {
>> + Active,
>> + Enabled,
>> + Disabled,
>> + Failed,
>> + Inactive,
>> + Unknown,
>> +}
>> +
>> +#[derive(Default)]
>> +struct Counters {
>> + pass: u64,
>> + skip: u64,
>> + notice: u64,
>> + warn: u64,
>> + fail: u64,
>> +}
>> +
>> +enum LogLevel {
>> + Pass,
>> + Info,
>> + Skip,
>> + Notice,
>> + Warn,
>> + Fail,
>> +}
>> +
>> +struct ConsoleOutput {
>> + stream: StandardStream,
>> + first_header: bool,
>> + counters: Counters,
>> +}
>> +
>> +impl ConsoleOutput {
>> + pub fn new() -> Self {
>> + Self {
>> + stream: StandardStream::stdout(ColorChoice::Always),
>> + first_header: true,
>> + counters: Counters::default(),
>> + }
>> + }
>> +
>> + pub fn print_header(&mut self, message: &str) -> Result<(), Error> {
>> + if !self.first_header {
>> + writeln!(&mut self.stream, "")?;
> ^
> clippy: You can remove the empty string if you just want to write a new line.
>
>> + }
>> + self.first_header = false;
>> + writeln!(&mut self.stream, "= {message} =\n")?;
>> + Ok(())
>> + }
>> +
>> + pub fn set_color(&mut self, color: Color) -> Result<(), Error> {
>> + self.stream.set_color(ColorSpec::new().set_fg(Some(color)))?;
>> + Ok(())
>> + }
>> +
>> + pub fn log_line(&mut self, level: LogLevel, message: &str) -> Result<(), Error> {
>> + match level {
>> + LogLevel::Pass => {
>> + self.counters.pass += 1;
>> + self.set_color(Color::Green)?;
>> + writeln!(&mut self.stream, "PASS: {}", message)?;
>> + self.set_color(Color::White)?;
>> + }
>> + LogLevel::Info => {
>> + writeln!(&mut self.stream, "INFO: {}", message)?;
>> + }
>> + LogLevel::Skip => {
>> + self.counters.skip += 1;
>> + writeln!(&mut self.stream, "SKIP: {}", message)?;
>> + }
>> + LogLevel::Notice => {
>> + self.counters.notice += 1;
>> + writeln!(&mut self.stream, "NOTICE: {}", message)?;
>> + }
>> + LogLevel::Warn=> {
>> + self.counters.warn += 1;
>> + self.set_color(Color::Yellow)?;
>> + writeln!(&mut self.stream, "WARN: {}", message)?;
>> + self.set_color(Color::White)?;
>> + }
>> + LogLevel::Fail => {
>> + self.counters.fail += 1;
>> + self.set_color(Color::Red)?;
>> + writeln!(&mut self.stream, "FAIL: {}", message)?;
>> + self.set_color(Color::White)?;
>> + }
>> + }
>> + Ok(())
>> + }
>> +
>> + pub fn log_pass(&mut self, message: &str) -> Result<(), Error> {
>> + self.log_line(LogLevel::Pass, message)
>> + }
>> +
>> + pub fn log_info(&mut self, message: &str) -> Result<(), Error> {
>> + self.log_line(LogLevel::Info, message)
>> + }
>> +
>> + pub fn log_skip(&mut self, message: &str) -> Result<(), Error> {
>> + self.log_line(LogLevel::Skip, message)
>> + }
>> +
>> + pub fn log_notice(&mut self, message: &str) -> Result<(), Error> {
>> + self.log_line(LogLevel::Notice, message)
>> + }
>> +
>> + pub fn log_warn(&mut self, message: &str) -> Result<(), Error> {
>> + self.log_line(LogLevel::Warn, message)
>> + }
>> +
>> + pub fn log_fail(&mut self, message: &str) -> Result<(), Error> {
>> + self.log_line(LogLevel::Fail, message)
>> + }
>> +
>> + pub fn print_summary(&mut self) -> Result<(), Error> {
>> + self.print_header("SUMMARY")?;
>> +
>> + let total = self.counters.fail + self.counters.pass + self.counters.notice +
>> + self.counters.skip + self.counters.warn;
>> +
>> + self.set_color(Color::White)?;
>> + writeln!(&mut self.stream, "TOTAL: {total}")?;
>> + self.set_color(Color::Green)?;
>> + writeln!(&mut self.stream, "PASSED: {}", self.counters.pass)?;
>> + self.set_color(Color::White)?;
>> + writeln!(&mut self.stream, "SKIPPED: {}", self.counters.skip)?;
>> + writeln!(&mut self.stream, "NOTICE: {}", self.counters.notice)?;
>> + if self.counters.warn > 0 {
>> + self.set_color(Color::Yellow)?;
>> + writeln!(&mut self.stream, "WARNINGS: {}", self.counters.warn)?;
>> + }
>> + if self.counters.fail > 0 {
>> + self.set_color(Color::Red)?;
>> + writeln!(&mut self.stream, "FAILURES: {}", self.counters.fail)?;
>> + }
>> + if self.counters.warn > 0 || self.counters.fail > 0 {
>> + let mut color = Color::Yellow;
>> + if self.counters.fail > 0 {
>> + color = Color::Red;
>> + }
>> +
>> + self.set_color(color)?;
>> + writeln!(
>> + &mut self.stream,
>> + "\nATTENTION: Please check the output for detailed information!",
>> + )?;
>> + if self.counters.fail > 0 {
>> + writeln!(
>> + &mut self.stream,
>> + "Try to solve the problems one at a time and rerun this checklist tool again.",
>> + )?;
>> + }
>> + }
>> + self.set_color(Color::White)?;
>> + Ok(())
>> + }
>> +}
>> +
>> diff --git a/zsh-completions/_pbs2to3 b/zsh-completions/_pbs2to3
>> new file mode 100644
>> index 00000000..f6dc3d67
>> --- /dev/null
>> +++ b/zsh-completions/_pbs2to3
>> @@ -0,0 +1,13 @@
>> +#compdef _pbs2to3() pbs2to3
>> +
>> +function _pbs2to3() {
>> + local cwords line point cmd curr prev
>> + cwords=${#words[@]}
>> + line=$words
>> + point=${#line}
>> + cmd=${words[1]}
>> + curr=${words[cwords]}
>> + prev=${words[cwords-1]}
>> + compadd -- $(COMP_CWORD="$cwords" COMP_LINE="$line" COMP_POINT="$point" \
>> + pbs2to3 bashcomplete "$cmd" "$curr" "$prev")
>> +}
>
--
- Lukas
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pbs-devel] [PATCH backup stable-2] pbs2to3: add upgrade checker binary
2023-06-28 10:14 ` Lukas Wagner
@ 2023-06-28 10:17 ` Christian Ebner
0 siblings, 0 replies; 6+ messages in thread
From: Christian Ebner @ 2023-06-28 10:17 UTC (permalink / raw)
To: Proxmox Backup Server development discussion, Lukas Wagner
Hi Lukas,
thx for the review!
I will take your comments in consideration for an upcoming v3.
Cheers,
Chris
> On 28.06.2023 12:14 CEST Lukas Wagner <l.wagner@proxmox.com> wrote:
>
>
> Oh, seems that I have missed v2 - but I think many comments still apply.
>
> On 6/28/23 12:13, Lukas Wagner wrote:
> > Hi,
> >
> > tested on current `stable-2` branch. Seems to work just fine, at least on a clean PBS installation.
> >
> > Some comments regarding the code are inline (nothing major though).
> >
> > Tested-by: Lukas Wagner <l.wagner@proxmox.com>
> > Reviewed-by: Lukas Wagner <l.wagner@proxmox.com>
> >
> > On 6/28/23 10:10, Christian Ebner wrote:
> >> Adds the pbs2to3 upgrade checker with some basic checks.
> >>
> >> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> >> ---
> >> Makefile | 1 +
> >> debian/proxmox-backup-server.install | 2 +
> >> src/bin/pbs2to3.rs | 602 +++++++++++++++++++++++++++
> >> zsh-completions/_pbs2to3 | 13 +
> >> 4 files changed, 618 insertions(+)
> >> create mode 100644 src/bin/pbs2to3.rs
> >> create mode 100644 zsh-completions/_pbs2to3
> >>
> >> diff --git a/Makefile b/Makefile
> >> index b307009d..7477935d 100644
> >> --- a/Makefile
> >> +++ b/Makefile
> >> @@ -19,6 +19,7 @@ USR_BIN := \
> >> USR_SBIN := \
> >> proxmox-backup-manager \
> >> proxmox-backup-debug \
> >> + pbs2to3
> >> # Binaries for services:
> >> SERVICE_BIN := \
> >> diff --git a/debian/proxmox-backup-server.install b/debian/proxmox-backup-server.install
> >> index 76f50cd0..ebe51aae 100644
> >> --- a/debian/proxmox-backup-server.install
> >> +++ b/debian/proxmox-backup-server.install
> >> @@ -11,6 +11,7 @@ usr/lib/x86_64-linux-gnu/proxmox-backup/proxmox-daily-update
> >> usr/lib/x86_64-linux-gnu/proxmox-backup/sg-tape-cmd
> >> usr/sbin/proxmox-backup-debug
> >> usr/sbin/proxmox-backup-manager
> >> +usr/sbin/pbs2to3
> >> usr/bin/pmtx
> >> usr/bin/pmt
> >> usr/bin/proxmox-tape
> >> @@ -39,3 +40,4 @@ usr/share/zsh/vendor-completions/_proxmox-backup-manager
> >> usr/share/zsh/vendor-completions/_proxmox-tape
> >> usr/share/zsh/vendor-completions/_pmtx
> >> usr/share/zsh/vendor-completions/_pmt
> >> +usr/share/zsh/vendor-completions/_pbs2to3
> >> diff --git a/src/bin/pbs2to3.rs b/src/bin/pbs2to3.rs
> >> new file mode 100644
> >> index 00000000..97d064cd
> >> --- /dev/null
> >> +++ b/src/bin/pbs2to3.rs
> >> @@ -0,0 +1,602 @@
> >> +use std::io::Write;
> >> +use std::path::Path;
> >> +
> >> +use anyhow::{format_err, Error};
> >> +use regex::Regex;
> >> +use termcolor::{Color, ColorChoice, ColorSpec, StandardStream, WriteColor};
> >> +
> >> +use proxmox_apt::repositories::{self, APTRepositoryFile};
> >> +use proxmox_backup::api2::node::apt;
> >> +
> >> +const OLD_SUITE: &str = "bullseye";
> >> +const NEW_SUITE: &str = "bookworm";
> >> +const PROXMOX_BACKUP_META: &str = "proxmox-backup";
> >> +const MIN_PBS_MAJOR: u8 = 2;
> >> +const MIN_PBS_MINOR: u8 = 4;
> >> +const MIN_PBS_PKGREL: u8 = 1;
> >> +
> >> +fn main() -> Result<(), Error> {
> >> + let mut checker = Checker::new();
> >> + checker.check_pbs_packages()?;
> >> + checker.check_misc()?;
> >> + checker.summary()?;
> >> + Ok(())
> >> +}
> >> +
> >> +struct Checker {
> >> + output: ConsoleOutput,
> >> + upgraded: bool,
> >> +}
> >> +
> >> +impl Checker {
> >> + pub fn new() -> Self {
> >> + Self {
> >> + output: ConsoleOutput::new(),
> >> + upgraded: false,
> >> + }
> >> + }
> >> +
> >
> > The `check_pbs_packages` function is a bit long and hard to read, maybe break this
> > into multiple subroutines? Generally I prefer to have functions that fit entirely on screen without
> > scrolling, unless there is a good reason against it.
> >> + pub fn check_pbs_packages(&mut self) -> Result<(), Error> {
> >> + self.output.print_header("CHECKING VERSION INFORMATION FOR PBS PACKAGES")?;
> >> + self.output.log_info("Checking for package updates..")?;
> >> +
> >> + let result = Self::get_upgradable_packages();
> >> + match result {
> >> + Err(err) => {
> >> + self.output.log_warn(format!("{}", err).as_str())?;
> > ^
> > Nit: You could inline the variable here: `format!("{err}")`
> > Also applies to many other locations in the code, I'll refrain from highlighting the other occurences ;).
> >
> >
> >> + self.output.log_fail("unable to retrieve list of package updates!")?;
> >> + }
> >> + Ok(cache) => {
> >> + if cache.package_status.is_empty() {
> >> + self.output.log_pass("all packages up-to-date")?;
> >> + } else {
> >> + let pkgs = cache.package_status.iter()
> >> + .map(|pkg| format!("{}, ", pkg.package.clone()))
> >> + .collect::<String>()> + self.output.log_warn(
> >> + format!(
> >> + "updates for the following packages are available:\
> >> + \n {}",
> >> + &pkgs[..pkgs.len()-2]
> >> + ).as_str()
> > You could change the signatures for the `log_*` functions to
> >
> > pub fn log_pass<T: AsRef<str>>(&mut self, message: T) -> Result<(), Error> {
> > self.log_line(LogLevel::Pass, message.as_ref())
> > }
> >
> > -> then the function can take a `String` as a well as `&str`, allowing you to omit the
> > `as_str()` calls in various places.
> >
> >> + )?;
> >> + }
> >> + }
> >> + }
> >> +
> >> + self.output.log_info("Checking proxmox backup server package version..")?;
> >> + let pkg_versions = apt::get_versions()?;
> >> +
> >> + let pbs_meta_pkg = pkg_versions.iter().find(|pkg| {
> >> + pkg.package.as_str() == PROXMOX_BACKUP_META
> >> + });
> >> +
> >> + if let Some(pbs_meta_pkg) = pbs_meta_pkg {
> >> + let pkg_version = Regex::new(r"^(\d+)\.(\d+)[.-](\d+)")?;
> >> + let captures = pkg_version.captures(&pbs_meta_pkg.old_version);
> >> + if let Some(captures) = captures {
> >> + let maj = Self::extract_version_from_captures(1, &captures)?;
> >> + let min = Self::extract_version_from_captures(2, &captures)?;
> >> + let pkgrel = Self::extract_version_from_captures(3, &captures)?;
> >> +
> >> + if maj > MIN_PBS_MAJOR {
> >> + self.output.log_pass(
> >> + format!("Already upgraded to Proxmox Backup Server {}", maj).as_str()
> >> + )?;
> >> + self.upgraded = true;
> >> + } else if maj >= MIN_PBS_MAJOR && min >= MIN_PBS_MINOR && pkgrel >= MIN_PBS_PKGREL {
> >> + self.output.log_pass(
> >> + format!(
> >> + "'{}' has version >= {}.{}-{}",
> >> + PROXMOX_BACKUP_META,
> >> + MIN_PBS_MAJOR,
> >> + MIN_PBS_MINOR,
> >> + MIN_PBS_PKGREL,
> >> + ).as_str()
> >> + )?;
> >> + } else {
> >> + self.output.log_fail(
> >> + format!(
> >> + "'{}' package is too old, please upgrade to >= {}.{}-{}",
> >> + PROXMOX_BACKUP_META,
> >> + MIN_PBS_MAJOR,
> >> + MIN_PBS_MINOR,
> >> + MIN_PBS_PKGREL,
> >> + ).as_str()
> >> + )?;
> >> + }
> >> + } else {
> >> + self.output.log_fail(
> >> + format!(
> >> + "could not match the '{}' package version, is it installed?",
> >> + PROXMOX_BACKUP_META,
> >> + ).as_str()
> >> + )?;
> >> + }
> >> +
> >> + self.output.log_info("Check running kernel version..")?;
> >> + let mut krunning = Regex::new(r"^6\.(?:2\.(?:[2-9]\d+|1[6-8]|1\d\d+)|5)[^~]*$")?;
> >> + let mut kinstalled = "pve-kernel-6.2";
> >> + if !self.upgraded {
> >> + krunning = Regex::new(r"^(?:5\.(?:13|15)|6\.2)")?;
> >> + kinstalled = "pve-kernel-5.15";
> >> + }
> >
> > Nit: I'd probably write something like:
> >
> > let (krunning, kinstalled) = if self.upgraded {
> > (
> > Regex::new(...),
> > "pve-kernel-..."
> > )
> > } else {
> > ...
> > }
> >
> > That way you can avoid the `mut` and also avoid compiling a regex that is not actually used.
> >
> >> +
> >> + let mut command = std::process::Command::new("uname");
> >> + command.arg("-r");
> >> + match command.output() {
> >
> > Nit: Also here you could avoid the `mut` and use the `Command` in a chained fashion:
> >
> > let output = Command::new(..)
> > .arg(...)
> > .output();
> >
> > match output {
> > ...
> > }
> >
> >> + Err(_err) => self.output.log_fail("unable to determine running kernel version.")?,
> >> + Ok(ret) => {
> >> + let running_version = std::str::from_utf8(&ret.stdout[..ret.stdout.len()-1])?;
> >> + if krunning.is_match(running_version) {
> >> + if self.upgraded {
> >> + self.output.log_pass(
> >> + format!(
> >> + "running new kernel '{}' after upgrade.",
> >> + running_version,
> >> + ).as_str()
> >> + )?;
> >> + } else {
> >> + self.output.log_pass(
> >> + format!(
> >> + "running kernel '{}' is considered suitable for upgrade.",
> >> + running_version,
> >> + ).as_str()
> >> + )?;
> >> + }
> >> + } else {
> >> + let installed_kernel = pkg_versions.iter().find(|pkg| {
> >> + pkg.package.as_str() == kinstalled
> >> + });
> >> + if installed_kernel.is_some() {
> >> + self.output.log_warn(
> >> + format!(
> >> + "a suitable kernel '{}' is installed, but an unsuitable '{}' \
> >> + is booted, missing reboot?!",
> >> + kinstalled,
> >> + running_version,
> >> + ).as_str()
> >> + )?;
> >> + } else {
> >> + self.output.log_warn(
> >> + format!(
> >> + "unexpected running and installed kernel '{}'.",
> >> + running_version,
> >> + ).as_str()
> >> + )?;
> >> + }
> >> + }
> >> + }
> >> + }
> >> +
> >> + } else {
> >> + self.output.log_fail(format!("'{}' package not found!", PROXMOX_BACKUP_META).as_str())?;
> >> + }
> >> +
> >> + Ok(())
> >> + }
> >> +
> >> + fn extract_version_from_captures(index: usize, captures: ®ex::Captures) -> Result<u8, Error> {
> >> + if let Some(capture) = captures.get(index) {
> >> + let val = capture.as_str().parse::<u8>()?;
> >> + Ok(val)
> >> + } else {
> >> + Ok(0)
> >> + }
> >> + }
> >> +
> >> + fn check_bootloader(&mut self) -> Result<(), Error> {
> >> + self.output.log_info("Checking bootloader configuration...")?;
> >> +
> >> + // PBS packages version check needs to be run before
> >> + if !self.upgraded {
> >> + self.output.log_skip("not yet upgraded, no need to check the presence of systemd-boot")?;
> >> + }
> >> +
> >> + if !Path::is_file(Path::new("/etc/kernel/proxmox-boot-uuids")) {
> > ^
> > `Path::is_file(&self)` takes `&self` as parameter, so I'd just write:
> >
> > `if !Path::new("...").is_file() {`
> >
> >> + self.output.log_skip("proxmox-boot-tool not used for bootloader configuration")?;
> >> + return Ok(());
> >> + }
> >> +
> >> + if !Path::is_dir(Path::new("/sys/firmware/efi")) {
> > Here as well.
> >> + self.output.log_skip("System booted in legacy-mode - no need for systemd-boot")?;
> >> + return Ok(());
> >> + }/
> >> +
> >> + if Path::is_file(Path::new("/usr/share/doc/systemd-boot/changelog.Debian.gz")) {
> > Here as well.
> >> + self.output.log_pass("systemd-boot is installed")?;
> >> + } else {
> >> + self.output.log_warn(
> >> + "proxmox-boot-tool is used for bootloader configuration in uefi mode \
> >> + but the separate systemd-boot package, existing in Debian Bookworm \
> >> + is not installed.\n\
> >> + initializing new ESPs will not work unitl the package is installed."
> >> + )?;
> >> + }
> >> + Ok(())
> >> + }
> >> +
> >> + fn check_apt_repos(&mut self) -> Result<(), Error> {
> >> + self.output.log_info("Checking for package repository suite mismatches..")?;
> >> +
> >> + let mut strange = false;
> >> + let mut mismatches = Vec::new();
> >> +
> >> + let (repo_files, _repo_errors, _digest) = repositories::repositories()?;
> >> + for repo_file in repo_files {
> >> + let (found_strange, mut found_mismatches) = self.check_repo_file(repo_file)?;
> >> + if found_strange {
> >> + strange = true;
> >> + }
> >> + mismatches.append(&mut found_mismatches);
> >> + }
> >> +
> >> + match (mismatches.is_empty(), strange) {
> >> + (true, false) => self.output.log_pass("found no suite mismatch")?,
> >> + (true, true) => self.output.log_notice(
> >> + "found no suite mismatches, but found at least one strange suite"
> >> + )?,
> >> + (false, _) => {
> >> + let mut message = String::from(
> >> + "Found mixed old and new packages repository suites, fix before upgrading!\
> >> + \n Mismatches:"
> >> + );
> >> + for (suite, location) in mismatches.iter() {
> >> + message.push_str(
> >> + format!("\n found suite '{}' at '{}'", suite, location).as_str()
> >> + );
> >> + }
> >> + message.push('\n');
> >> + self.output.log_fail(message.as_str())?
> >> + }
> >> + }
> >> +
> >> + Ok(())
> >> + }
> >> +
> >> + pub fn check_misc(&mut self) -> Result<(), Error> {
> >> + self.output.print_header("MISCELLANEOUS CHECKS")?;
> >> + self.check_pbs_services()?;
> >> + self.check_time_sync()?;
> >> + self.check_apt_repos()?;
> >> + self.check_bootloader()?;
> >> + Ok(())
> >> + }
> >> +
> >> + pub fn summary(&mut self) -> Result<(), Error> {
> >> + self.output.print_summary()
> >> + }
> >> +
> >
> > The function below is deeply nested (up to 7 levels, if I counted correctly), maybe break
> > stuff out into smaller functions?
> >> + fn check_repo_file(
> >> + &mut self,
> >> + repo_file: APTRepositoryFile
> >> + ) -> Result<(bool, Vec<(String, String)>), Error> {
> >> + let deb_suite = Regex::new(r"(\w+)(-updates|-backports|-security)?$")?;
> >> + let mut strange_suite = false;
> >> + let mut found_suite: Option<(String, String)> = None;
> >> + let mut mismatches = Vec::new();
> >> +
> >> + for repo in repo_file.repositories {
> >> + for suite in &repo.suites {
> >> + if let Some(suite) = deb_suite.captures(suite) {
> >> + if let Some(suite_match) = suite.get(1) {
> >> + let suite = suite_match.as_str();
> >> + let location = repo_file.path.clone().unwrap_or(String::new());
> > ^
> > clippy: You can use `unwrap_or_default` here.
> >
> >> +
> >> + if suite != OLD_SUITE && suite != NEW_SUITE {
> >> + self.output.log_notice(format!(
> >> + "found unusual suite '{}', neighter old '{}' nor new '{}'..\
> >> + \n Affected file {}\
> >> + \n Please assure this is shipping compatible packages for \
> >> + the upgrade!",
> >> + suite,
> >> + OLD_SUITE,
> >> + NEW_SUITE,
> >> + location,
> >> + ).as_str())?;
> >> + strange_suite = true;
> >> + continue;
> >> + }
> >> +
> >> + if let Some((ref current_suite, ref current_location)) = found_suite {
> >> + if suite != current_suite {
> >> + if mismatches.is_empty() {
> >> + mismatches.push(
> >> + (current_suite.clone(), current_location.clone())
> >> + );
> >> + mismatches.push((suite.to_string(), location));
> >> + } else {
> >> + mismatches.push((suite.to_string(), location));
> >> + }
> >> + }
> >> + } else {
> >> + found_suite = Some((suite.to_string(), location));
> >> + }
> >> + }
> >> + }
> >> + }
> >> + }
> >> +
> >> + Ok((strange_suite, mismatches))
> >> + }
> >> +
> >> + fn get_systemd_unit_state(
> >> + &mut self,
> >> + unit: &str,
> >> + ) -> Result<(SystemdUnitState, SystemdUnitState), Error> {
> >> + let mut command = std::process::Command::new("systemctl");
> >> + command.arg("is-enabled");
> >> + command.arg(unit);
> >> + let output = command.output().map_err(|err| {
> >> + format_err!("failed to execute {:?} - {}", command, err)
> >> + })?;
> >> +
> >> + let is_enabled_state = match output.stdout.as_slice() {
> >> + b"enabled\n" => SystemdUnitState::Enabled,
> >> + b"disabled\n" => SystemdUnitState::Disabled,
> >> + _ => SystemdUnitState::Unknown,
> >> + };
> >> +
> >> + let mut command = std::process::Command::new("systemctl");
> >> + command.arg("is-active");
> >> + command.arg(unit);
> >> + let output = command.output().map_err(|err| {
> >> + format_err!("failed to execute {:?} - {}", command, err)
> >> + })?;
> >> +
> >> + let is_active_state = match output.stdout.as_slice() {
> >> + b"active\n" => SystemdUnitState::Active,
> >> + b"inactive\n" => SystemdUnitState::Inactive,
> >> + b"failed\n" => SystemdUnitState::Failed,
> >> + _ => SystemdUnitState::Unknown,
> >> + };
> >> + Ok((is_enabled_state, is_active_state))
> >> + }
> >> +
> >> + fn check_pbs_services(&mut self) -> Result<(), Error> {
> >> + self.output.log_info("Checking pbs daemon services..")?;
> >> +
> >> + for service in ["proxmox-backup.service", "proxmox-backup-proxy.service"] {
> >> + match self.get_systemd_unit_state(service)? {
> >> + (_, SystemdUnitState::Active) => {
> >> + self.output.log_pass(
> >> + format!("systemd unit '{}' is in state 'active'", service).as_str()
> >> + )?;
> >> + }
> >> + (_, SystemdUnitState::Inactive) => {
> >> + self.output.log_fail(
> >> + format!(
> >> + "systemd unit '{}' is in state 'inactive'\
> >> + \n Please check the service for errors and start it.",
> >> + service,
> >> + ).as_str()
> >> + )?;
> >> + }
> >> + (_, SystemdUnitState::Failed) => {
> >> + self.output.log_fail(
> >> + format!(
> >> + "systemd unit '{}' is in state 'failed'\
> >> + \n Please check the service for errors and start it.",
> >> + service,
> >> + ).as_str()
> >> + )?;
> >> + }
> >> + (_, _) => {
> >> + self.output.log_fail(
> >> + format!(
> >> + "systemd unit '{}' is not in state 'active'\
> >> + \n Please check the service for errors and start it.",
> >> + service,
> >> + ).as_str()
> >> + )?;
> >> + }
> >> + }
> >> + }
> >> + Ok(())
> >> + }
> >> +
> >> + fn check_time_sync(&mut self) -> Result<(), Error> {
> >> + self.output.log_info("Checking for supported & active NTP service..")?;
> >> + if self.get_systemd_unit_state("systemd-timesyncd.service")?.1 == SystemdUnitState::Active {
> >> + self.output.log_warn(
> >> + "systemd-timesyncd is not the best choice for time-keeping on servers, due to only \
> >> + applying updates on boot.\
> >> + \n While not necessary for the upgrade it's recommended to use one of:\
> >> + \n * chrony (Default in new Proxmox VE installations)\
> >> + \n * ntpsec\
> >> + \n * openntpd"
> >> + )?;
> >> + } else if self.get_systemd_unit_state("ntp.service")?.1 == SystemdUnitState::Active {
> >> + self.output.log_info(
> >> + "Debian deprecated and removed the ntp package for Bookworm, but the system \
> >> + will automatically migrate to the 'ntpsec' replacement package on upgrade."
> >> + )?;
> >> + } else if self.get_systemd_unit_state("chrony.service")?.1 == SystemdUnitState::Active ||
> >> + self.get_systemd_unit_state("openntpd.service")?.1 == SystemdUnitState::Active ||
> >> + self.get_systemd_unit_state("ntpsec.service")?.1 == SystemdUnitState::Active
> >> + {
> >> + self.output.log_pass("Detected active time synchronisation unit")?;
> >> + } else {
> >> + self.output.log_warn(
> >> + "No (active) time synchronisation daemon (NTP) detected, but synchronized systems \
> >> + are important!"
> >> + )?;
> >> + }
> >> + Ok(())
> >> + }
> >> +
> >> + fn get_upgradable_packages() -> Result<proxmox_backup::tools::apt::PkgState, Error> {
> >> + let cache = if let Ok(false) = proxmox_backup::tools::apt::pkg_cache_expired() {
> >> + if let Ok(Some(cache)) = proxmox_backup::tools::apt::read_pkg_state() {
> >> + cache
> >> + } else {
> >> + proxmox_backup::tools::apt::update_cache()?
> >> + }
> >> + } else {
> >> + proxmox_backup::tools::apt::update_cache()?
> >> + };
> >> +
> >> + Ok(cache)
> >> + }
> >> +}
> >> +
> >> +#[derive(PartialEq)]
> >> +enum SystemdUnitState {
> >> + Active,
> >> + Enabled,
> >> + Disabled,
> >> + Failed,
> >> + Inactive,
> >> + Unknown,
> >> +}
> >> +
> >> +#[derive(Default)]
> >> +struct Counters {
> >> + pass: u64,
> >> + skip: u64,
> >> + notice: u64,
> >> + warn: u64,
> >> + fail: u64,
> >> +}
> >> +
> >> +enum LogLevel {
> >> + Pass,
> >> + Info,
> >> + Skip,
> >> + Notice,
> >> + Warn,
> >> + Fail,
> >> +}
> >> +
> >> +struct ConsoleOutput {
> >> + stream: StandardStream,
> >> + first_header: bool,
> >> + counters: Counters,
> >> +}
> >> +
> >> +impl ConsoleOutput {
> >> + pub fn new() -> Self {
> >> + Self {
> >> + stream: StandardStream::stdout(ColorChoice::Always),
> >> + first_header: true,
> >> + counters: Counters::default(),
> >> + }
> >> + }
> >> +
> >> + pub fn print_header(&mut self, message: &str) -> Result<(), Error> {
> >> + if !self.first_header {
> >> + writeln!(&mut self.stream, "")?;
> > ^
> > clippy: You can remove the empty string if you just want to write a new line.
> >
> >> + }
> >> + self.first_header = false;
> >> + writeln!(&mut self.stream, "= {message} =\n")?;
> >> + Ok(())
> >> + }
> >> +
> >> + pub fn set_color(&mut self, color: Color) -> Result<(), Error> {
> >> + self.stream.set_color(ColorSpec::new().set_fg(Some(color)))?;
> >> + Ok(())
> >> + }
> >> +
> >> + pub fn log_line(&mut self, level: LogLevel, message: &str) -> Result<(), Error> {
> >> + match level {
> >> + LogLevel::Pass => {
> >> + self.counters.pass += 1;
> >> + self.set_color(Color::Green)?;
> >> + writeln!(&mut self.stream, "PASS: {}", message)?;
> >> + self.set_color(Color::White)?;
> >> + }
> >> + LogLevel::Info => {
> >> + writeln!(&mut self.stream, "INFO: {}", message)?;
> >> + }
> >> + LogLevel::Skip => {
> >> + self.counters.skip += 1;
> >> + writeln!(&mut self.stream, "SKIP: {}", message)?;
> >> + }
> >> + LogLevel::Notice => {
> >> + self.counters.notice += 1;
> >> + writeln!(&mut self.stream, "NOTICE: {}", message)?;
> >> + }
> >> + LogLevel::Warn=> {
> >> + self.counters.warn += 1;
> >> + self.set_color(Color::Yellow)?;
> >> + writeln!(&mut self.stream, "WARN: {}", message)?;
> >> + self.set_color(Color::White)?;
> >> + }
> >> + LogLevel::Fail => {
> >> + self.counters.fail += 1;
> >> + self.set_color(Color::Red)?;
> >> + writeln!(&mut self.stream, "FAIL: {}", message)?;
> >> + self.set_color(Color::White)?;
> >> + }
> >> + }
> >> + Ok(())
> >> + }
> >> +
> >> + pub fn log_pass(&mut self, message: &str) -> Result<(), Error> {
> >> + self.log_line(LogLevel::Pass, message)
> >> + }
> >> +
> >> + pub fn log_info(&mut self, message: &str) -> Result<(), Error> {
> >> + self.log_line(LogLevel::Info, message)
> >> + }
> >> +
> >> + pub fn log_skip(&mut self, message: &str) -> Result<(), Error> {
> >> + self.log_line(LogLevel::Skip, message)
> >> + }
> >> +
> >> + pub fn log_notice(&mut self, message: &str) -> Result<(), Error> {
> >> + self.log_line(LogLevel::Notice, message)
> >> + }
> >> +
> >> + pub fn log_warn(&mut self, message: &str) -> Result<(), Error> {
> >> + self.log_line(LogLevel::Warn, message)
> >> + }
> >> +
> >> + pub fn log_fail(&mut self, message: &str) -> Result<(), Error> {
> >> + self.log_line(LogLevel::Fail, message)
> >> + }
> >> +
> >> + pub fn print_summary(&mut self) -> Result<(), Error> {
> >> + self.print_header("SUMMARY")?;
> >> +
> >> + let total = self.counters.fail + self.counters.pass + self.counters.notice +
> >> + self.counters.skip + self.counters.warn;
> >> +
> >> + self.set_color(Color::White)?;
> >> + writeln!(&mut self.stream, "TOTAL: {total}")?;
> >> + self.set_color(Color::Green)?;
> >> + writeln!(&mut self.stream, "PASSED: {}", self.counters.pass)?;
> >> + self.set_color(Color::White)?;
> >> + writeln!(&mut self.stream, "SKIPPED: {}", self.counters.skip)?;
> >> + writeln!(&mut self.stream, "NOTICE: {}", self.counters.notice)?;
> >> + if self.counters.warn > 0 {
> >> + self.set_color(Color::Yellow)?;
> >> + writeln!(&mut self.stream, "WARNINGS: {}", self.counters.warn)?;
> >> + }
> >> + if self.counters.fail > 0 {
> >> + self.set_color(Color::Red)?;
> >> + writeln!(&mut self.stream, "FAILURES: {}", self.counters.fail)?;
> >> + }
> >> + if self.counters.warn > 0 || self.counters.fail > 0 {
> >> + let mut color = Color::Yellow;
> >> + if self.counters.fail > 0 {
> >> + color = Color::Red;
> >> + }
> >> +
> >> + self.set_color(color)?;
> >> + writeln!(
> >> + &mut self.stream,
> >> + "\nATTENTION: Please check the output for detailed information!",
> >> + )?;
> >> + if self.counters.fail > 0 {
> >> + writeln!(
> >> + &mut self.stream,
> >> + "Try to solve the problems one at a time and rerun this checklist tool again.",
> >> + )?;
> >> + }
> >> + }
> >> + self.set_color(Color::White)?;
> >> + Ok(())
> >> + }
> >> +}
> >> +
> >> diff --git a/zsh-completions/_pbs2to3 b/zsh-completions/_pbs2to3
> >> new file mode 100644
> >> index 00000000..f6dc3d67
> >> --- /dev/null
> >> +++ b/zsh-completions/_pbs2to3
> >> @@ -0,0 +1,13 @@
> >> +#compdef _pbs2to3() pbs2to3
> >> +
> >> +function _pbs2to3() {
> >> + local cwords line point cmd curr prev
> >> + cwords=${#words[@]}
> >> + line=$words
> >> + point=${#line}
> >> + cmd=${words[1]}
> >> + curr=${words[cwords]}
> >> + prev=${words[cwords-1]}
> >> + compadd -- $(COMP_CWORD="$cwords" COMP_LINE="$line" COMP_POINT="$point" \
> >> + pbs2to3 bashcomplete "$cmd" "$curr" "$prev")
> >> +}
> >
>
> --
> - Lukas
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-06-28 10:17 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-28 8:10 [pbs-devel] [PATCH backup stable-2] pbs2to3: add upgrade checker binary Christian Ebner
2023-06-28 8:59 ` Fiona Ebner
2023-06-28 9:43 ` Christian Ebner
2023-06-28 10:13 ` Lukas Wagner
2023-06-28 10:14 ` Lukas Wagner
2023-06-28 10:17 ` Christian Ebner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox