* [pbs-devel] [PATCH proxmox-backup 1/9] file-restore: add debug mode with serial access
2021-05-06 15:26 [pbs-devel] [PATCH 0/9] Debug mode and smaller fixes for single file restore Stefan Reiter
@ 2021-05-06 15:26 ` Stefan Reiter
2021-05-06 15:26 ` [pbs-devel] [PATCH proxmox-backup 2/9] file-restore: try to kill VM when stale Stefan Reiter
` (8 subsequent siblings)
9 siblings, 0 replies; 17+ messages in thread
From: Stefan Reiter @ 2021-05-06 15:26 UTC (permalink / raw)
To: pbs-devel
Set PBS_QEMU_DEBUG=1 on a command that starts a VM and then connect to
the debug root shell via:
minicom -D \unix#/run/proxmox-backup/file-restore-serial-10.sock
or similar.
Note that this requires 'proxmox-backup-restore-image-debug' to work,
the postinst script is updated to also generate the corresponding image.
Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---
debian/proxmox-backup-file-restore.postinst | 12 ++++-
src/bin/proxmox_file_restore/qemu_helper.rs | 55 ++++++++++++++++++---
src/buildcfg.rs | 4 ++
3 files changed, 62 insertions(+), 9 deletions(-)
diff --git a/debian/proxmox-backup-file-restore.postinst b/debian/proxmox-backup-file-restore.postinst
index 27eb87e9..feb0557b 100755
--- a/debian/proxmox-backup-file-restore.postinst
+++ b/debian/proxmox-backup-file-restore.postinst
@@ -6,6 +6,7 @@ update_initramfs() {
# regenerate initramfs for single file restore VM
INST_PATH="/usr/lib/x86_64-linux-gnu/proxmox-backup/file-restore"
CACHE_PATH="/var/cache/proxmox-backup/file-restore-initramfs.img"
+ CACHE_PATH_DBG="/var/cache/proxmox-backup/file-restore-initramfs-debug.img"
# cleanup first, in case proxmox-file-restore was uninstalled since we do
# not want an unuseable image lying around
@@ -20,7 +21,7 @@ update_initramfs() {
# avoid leftover temp file
cleanup() {
- rm -f "$CACHE_PATH.tmp"
+ rm -f "$CACHE_PATH.tmp" "$CACHE_PATH_DBG.tmp"
}
trap cleanup EXIT
@@ -34,6 +35,15 @@ update_initramfs() {
| cpio -o --format=newc -A -F "$CACHE_PATH.tmp" )
mv -f "$CACHE_PATH.tmp" "$CACHE_PATH"
+ if [ -f "$INST_PATH/initramfs-debug.img" ]; then
+ echo "Updating file-restore debug initramfs..."
+ cp "$INST_PATH/initramfs-debug.img" "$CACHE_PATH_DBG.tmp"
+ ( cd "$INST_PATH"; \
+ printf "./proxmox-restore-daemon" \
+ | cpio -o --format=newc -A -F "$CACHE_PATH_DBG.tmp" )
+ mv -f "$CACHE_PATH_DBG.tmp" "$CACHE_PATH_DBG"
+ fi
+
trap - EXIT
}
diff --git a/src/bin/proxmox_file_restore/qemu_helper.rs b/src/bin/proxmox_file_restore/qemu_helper.rs
index 9328addf..20b4f344 100644
--- a/src/bin/proxmox_file_restore/qemu_helper.rs
+++ b/src/bin/proxmox_file_restore/qemu_helper.rs
@@ -47,9 +47,13 @@ fn create_restore_log_dir() -> Result<String, Error> {
Ok(logpath)
}
-fn validate_img_existance() -> Result<(), Error> {
+fn validate_img_existance(debug: bool) -> Result<(), Error> {
let kernel = PathBuf::from(buildcfg::PROXMOX_BACKUP_KERNEL_FN);
- let initramfs = PathBuf::from(buildcfg::PROXMOX_BACKUP_INITRAMFS_FN);
+ let initramfs = PathBuf::from(if debug {
+ buildcfg::PROXMOX_BACKUP_INITRAMFS_DBG_FN
+ } else {
+ buildcfg::PROXMOX_BACKUP_INITRAMFS_FN
+ });
if !kernel.exists() || !initramfs.exists() {
bail!("cannot run file-restore VM: package 'proxmox-backup-restore-image' is not (correctly) installed");
}
@@ -79,7 +83,7 @@ fn try_kill_vm(pid: i32) -> Result<(), Error> {
Ok(())
}
-async fn create_temp_initramfs(ticket: &str) -> Result<(Fd, String), Error> {
+async fn create_temp_initramfs(ticket: &str, debug: bool) -> Result<(Fd, String), Error> {
use std::ffi::CString;
use tokio::fs::File;
@@ -88,8 +92,14 @@ async fn create_temp_initramfs(ticket: &str) -> Result<(Fd, String), Error> {
nix::unistd::unlink(&tmp_path)?;
tools::fd_change_cloexec(tmp_fd.0, false)?;
+ let initramfs = if debug {
+ buildcfg::PROXMOX_BACKUP_INITRAMFS_DBG_FN
+ } else {
+ buildcfg::PROXMOX_BACKUP_INITRAMFS_FN
+ };
+
let mut f = File::from_std(unsafe { std::fs::File::from_raw_fd(tmp_fd.0) });
- let mut base = File::open(buildcfg::PROXMOX_BACKUP_INITRAMFS_FN).await?;
+ let mut base = File::open(initramfs).await?;
tokio::io::copy(&mut base, &mut f).await?;
@@ -122,18 +132,24 @@ pub async fn start_vm(
files: impl Iterator<Item = String>,
ticket: &str,
) -> Result<(i32, i32), Error> {
- validate_img_existance()?;
-
if let Err(_) = std::env::var("PBS_PASSWORD") {
bail!("environment variable PBS_PASSWORD has to be set for QEMU VM restore");
}
+ let debug = if let Ok(val) = std::env::var("PBS_QEMU_DEBUG") {
+ !val.is_empty()
+ } else {
+ false
+ };
+
+ validate_img_existance(debug)?;
+
let pid;
let (pid_fd, pid_path) = make_tmp_file("/tmp/file-restore-qemu.pid.tmp", CreateOptions::new())?;
nix::unistd::unlink(&pid_path)?;
tools::fd_change_cloexec(pid_fd.0, false)?;
- let (_ramfs_pid, ramfs_path) = create_temp_initramfs(ticket).await?;
+ let (_ramfs_pid, ramfs_path) = create_temp_initramfs(ticket, debug).await?;
let logpath = create_restore_log_dir()?;
let logfile = &format!("{}/qemu.log", logpath);
@@ -174,7 +190,11 @@ pub async fn start_vm(
"-initrd",
&ramfs_path,
"-append",
- "quiet panic=1",
+ if debug {
+ "debug panic=1"
+ } else {
+ "quiet panic=1"
+ },
"-daemonize",
"-pidfile",
&format!("/dev/fd/{}", pid_fd.as_raw_fd()),
@@ -218,6 +238,19 @@ pub async fn start_vm(
cid
));
+ if debug {
+ let debug_args = [
+ "-chardev",
+ &format!(
+ "socket,id=debugser,path=/run/proxmox-backup/file-restore-serial-{}.sock,server,nowait",
+ cid
+ ),
+ "-serial",
+ "chardev:debugser",
+ ];
+ qemu_cmd.args(debug_args.iter());
+ }
+
qemu_cmd.stdout(std::process::Stdio::null());
qemu_cmd.stderr(std::process::Stdio::piped());
@@ -260,6 +293,12 @@ pub async fn start_vm(
if let Ok(Ok(_)) =
time::timeout(Duration::from_secs(2), client.get("api2/json/status", None)).await
{
+ if debug {
+ eprintln!(
+ "Connect to '/run/proxmox-backup/file-restore-serial-{}.sock' for shell access",
+ cid
+ )
+ }
return Ok((pid, cid as i32));
}
if kill(pid_t, None).is_err() {
diff --git a/src/buildcfg.rs b/src/buildcfg.rs
index b0f61efb..c70ab6ea 100644
--- a/src/buildcfg.rs
+++ b/src/buildcfg.rs
@@ -43,6 +43,10 @@ pub const PROXMOX_BACKUP_API_PID_FN: &str = concat!(PROXMOX_BACKUP_RUN_DIR_M!(),
pub const PROXMOX_BACKUP_INITRAMFS_FN: &str =
concat!(PROXMOX_BACKUP_CACHE_DIR_M!(), "/file-restore-initramfs.img");
+/// filename of the cached initramfs to use for debugging single file restore
+pub const PROXMOX_BACKUP_INITRAMFS_DBG_FN: &str =
+ concat!(PROXMOX_BACKUP_CACHE_DIR_M!(), "/file-restore-initramfs-debug.img");
+
/// filename of the kernel to use for booting single file restore VMs
pub const PROXMOX_BACKUP_KERNEL_FN: &str =
concat!(PROXMOX_BACKUP_FILE_RESTORE_BIN_DIR_M!(), "/bzImage");
--
2.20.1
^ permalink raw reply [flat|nested] 17+ messages in thread* [pbs-devel] [PATCH proxmox-backup 2/9] file-restore: try to kill VM when stale
2021-05-06 15:26 [pbs-devel] [PATCH 0/9] Debug mode and smaller fixes for single file restore Stefan Reiter
2021-05-06 15:26 ` [pbs-devel] [PATCH proxmox-backup 1/9] file-restore: add debug mode with serial access Stefan Reiter
@ 2021-05-06 15:26 ` Stefan Reiter
2021-05-07 7:04 ` [pbs-devel] applied: " Thomas Lamprecht
2021-05-06 15:26 ` [pbs-devel] [PATCH proxmox-backup 3/9] file-restore: add more RAM for VMs with many drives or debug Stefan Reiter
` (7 subsequent siblings)
9 siblings, 1 reply; 17+ messages in thread
From: Stefan Reiter @ 2021-05-06 15:26 UTC (permalink / raw)
To: pbs-devel
Helps to clean up a VM that has crashed, is not responding to vsock API
calls, but still has a running QEMU instance.
We always check the process commandline to ensure we don't kill a random
process that took over the PID.
Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---
src/bin/proxmox_file_restore/block_driver_qemu.rs | 2 ++
src/bin/proxmox_file_restore/qemu_helper.rs | 2 +-
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/bin/proxmox_file_restore/block_driver_qemu.rs b/src/bin/proxmox_file_restore/block_driver_qemu.rs
index c95bf80b..24f62796 100644
--- a/src/bin/proxmox_file_restore/block_driver_qemu.rs
+++ b/src/bin/proxmox_file_restore/block_driver_qemu.rs
@@ -98,6 +98,7 @@ async fn cleanup_map(map: &mut HashMap<String, VMState>) -> bool {
"VM '{}' (pid: {}, cid: {}) was not reachable, removing from map",
name, state.pid, state.cid
);
+ let _ = super::qemu_helper::try_kill_vm(state.pid);
}
}
@@ -131,6 +132,7 @@ async fn ensure_running(details: &SnapRestoreDetails) -> Result<VsockClient, Err
Err(err) => {
eprintln!("stale VM detected, restarting ({})", err);
// VM is dead, restart
+ let _ = super::qemu_helper::try_kill_vm(vm.pid);
let vms = start_vm(vm.cid, details).await?;
new_cid = vms.cid;
state.map.insert(name, vms.clone());
diff --git a/src/bin/proxmox_file_restore/qemu_helper.rs b/src/bin/proxmox_file_restore/qemu_helper.rs
index 20b4f344..a2b5b09d 100644
--- a/src/bin/proxmox_file_restore/qemu_helper.rs
+++ b/src/bin/proxmox_file_restore/qemu_helper.rs
@@ -60,7 +60,7 @@ fn validate_img_existance(debug: bool) -> Result<(), Error> {
Ok(())
}
-fn try_kill_vm(pid: i32) -> Result<(), Error> {
+pub fn try_kill_vm(pid: i32) -> Result<(), Error> {
let pid = Pid::from_raw(pid);
if let Ok(()) = kill(pid, None) {
// process is running (and we could kill it), check if it is actually ours
--
2.20.1
^ permalink raw reply [flat|nested] 17+ messages in thread* [pbs-devel] applied: [PATCH proxmox-backup 2/9] file-restore: try to kill VM when stale
2021-05-06 15:26 ` [pbs-devel] [PATCH proxmox-backup 2/9] file-restore: try to kill VM when stale Stefan Reiter
@ 2021-05-07 7:04 ` Thomas Lamprecht
0 siblings, 0 replies; 17+ messages in thread
From: Thomas Lamprecht @ 2021-05-07 7:04 UTC (permalink / raw)
To: Proxmox Backup Server development discussion, Stefan Reiter
On 06.05.21 17:26, Stefan Reiter wrote:
> Helps to clean up a VM that has crashed, is not responding to vsock API
> calls, but still has a running QEMU instance.
>
> We always check the process commandline to ensure we don't kill a random
> process that took over the PID.
>
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
> src/bin/proxmox_file_restore/block_driver_qemu.rs | 2 ++
> src/bin/proxmox_file_restore/qemu_helper.rs | 2 +-
> 2 files changed, 3 insertions(+), 1 deletion(-)
>
>
applied, thanks!
^ permalink raw reply [flat|nested] 17+ messages in thread
* [pbs-devel] [PATCH proxmox-backup 3/9] file-restore: add more RAM for VMs with many drives or debug
2021-05-06 15:26 [pbs-devel] [PATCH 0/9] Debug mode and smaller fixes for single file restore Stefan Reiter
2021-05-06 15:26 ` [pbs-devel] [PATCH proxmox-backup 1/9] file-restore: add debug mode with serial access Stefan Reiter
2021-05-06 15:26 ` [pbs-devel] [PATCH proxmox-backup 2/9] file-restore: try to kill VM when stale Stefan Reiter
@ 2021-05-06 15:26 ` Stefan Reiter
2021-05-07 7:04 ` [pbs-devel] applied: " Thomas Lamprecht
2021-05-06 15:26 ` [pbs-devel] [PATCH proxmox-backup 4/9] file-restore: support more drives Stefan Reiter
` (6 subsequent siblings)
9 siblings, 1 reply; 17+ messages in thread
From: Stefan Reiter @ 2021-05-06 15:26 UTC (permalink / raw)
To: pbs-devel
The guest kernel requires more memory depending on how many disks are
attached. 256 seems to be enough for basically any reasonable and
unreasonable amount of disks though.
For debug instance, make it 1G, as these are never started automatically
anyway, and need at least 512MB since the initramfs (especially when
including a debug build of the daemon) is substantially bigger.
Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---
src/bin/proxmox_file_restore/qemu_helper.rs | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/src/bin/proxmox_file_restore/qemu_helper.rs b/src/bin/proxmox_file_restore/qemu_helper.rs
index a2b5b09d..9b44b55d 100644
--- a/src/bin/proxmox_file_restore/qemu_helper.rs
+++ b/src/bin/proxmox_file_restore/qemu_helper.rs
@@ -183,8 +183,6 @@ pub async fn start_vm(
"-vnc",
"none",
"-enable-kvm",
- "-m",
- "128",
"-kernel",
buildcfg::PROXMOX_BACKUP_KERNEL_FN,
"-initrd",
@@ -226,11 +224,24 @@ pub async fn start_vm(
id += 1;
}
+ let ram = if debug {
+ 1024
+ } else {
+ // add more RAM if many drives are given
+ match id {
+ f if f < 10 => 128,
+ f if f < 20 => 192,
+ _ => 256,
+ }
+ };
+
// Try starting QEMU in a loop to retry if we fail because of a bad 'cid' value
let mut attempts = 0;
loop {
let mut qemu_cmd = std::process::Command::new("qemu-system-x86_64");
qemu_cmd.args(base_args.iter());
+ qemu_cmd.arg("-m");
+ qemu_cmd.arg(ram.to_string());
qemu_cmd.args(&drives);
qemu_cmd.arg("-device");
qemu_cmd.arg(format!(
--
2.20.1
^ permalink raw reply [flat|nested] 17+ messages in thread* [pbs-devel] applied: [PATCH proxmox-backup 3/9] file-restore: add more RAM for VMs with many drives or debug
2021-05-06 15:26 ` [pbs-devel] [PATCH proxmox-backup 3/9] file-restore: add more RAM for VMs with many drives or debug Stefan Reiter
@ 2021-05-07 7:04 ` Thomas Lamprecht
0 siblings, 0 replies; 17+ messages in thread
From: Thomas Lamprecht @ 2021-05-07 7:04 UTC (permalink / raw)
To: Proxmox Backup Server development discussion, Stefan Reiter
On 06.05.21 17:26, Stefan Reiter wrote:
> The guest kernel requires more memory depending on how many disks are
> attached. 256 seems to be enough for basically any reasonable and
> unreasonable amount of disks though.
>
> For debug instance, make it 1G, as these are never started automatically
> anyway, and need at least 512MB since the initramfs (especially when
> including a debug build of the daemon) is substantially bigger.
>
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
> src/bin/proxmox_file_restore/qemu_helper.rs | 15 +++++++++++++--
> 1 file changed, 13 insertions(+), 2 deletions(-)
>
>
applied, thanks!
^ permalink raw reply [flat|nested] 17+ messages in thread
* [pbs-devel] [PATCH proxmox-backup 4/9] file-restore: support more drives
2021-05-06 15:26 [pbs-devel] [PATCH 0/9] Debug mode and smaller fixes for single file restore Stefan Reiter
` (2 preceding siblings ...)
2021-05-06 15:26 ` [pbs-devel] [PATCH proxmox-backup 3/9] file-restore: add more RAM for VMs with many drives or debug Stefan Reiter
@ 2021-05-06 15:26 ` Stefan Reiter
2021-05-07 7:04 ` [pbs-devel] applied: " Thomas Lamprecht
2021-05-06 15:26 ` [pbs-devel] [PATCH proxmox-backup 5/9] file-restore-daemon: work around tokio DuplexStream bug Stefan Reiter
` (5 subsequent siblings)
9 siblings, 1 reply; 17+ messages in thread
From: Stefan Reiter @ 2021-05-06 15:26 UTC (permalink / raw)
To: pbs-devel
A PCI bus can only support up to 32 devices, so excluding built-in
devices that left us with a maximum of about 25 drives. By adding a new
PCI bridge every 32 devices (starting at bridge ID 2 to avoid conflicts
with automatic bridges), we can theoretically support up to 8096 drives.
Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---
Tested with a 41 disk VM, the most I could force into a PVE config.
src/bin/proxmox_file_restore/qemu_helper.rs | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/src/bin/proxmox_file_restore/qemu_helper.rs b/src/bin/proxmox_file_restore/qemu_helper.rs
index 9b44b55d..c9d816ca 100644
--- a/src/bin/proxmox_file_restore/qemu_helper.rs
+++ b/src/bin/proxmox_file_restore/qemu_helper.rs
@@ -217,10 +217,21 @@ pub async fn start_vm(
"file=pbs:repository={},,snapshot={},,archive={}{},read-only=on,if=none,id=drive{}",
details.repo, details.snapshot, file, keyfile, id
));
+
+ // a PCI bus can only support 32 devices, so add a new one every 32
+ let bus = (id / 32) + 2;
+ if id % 32 == 0 {
+ drives.push("-device".to_owned());
+ drives.push(format!("pci-bridge,id=bridge{},chassis_nr={}", bus, bus));
+ }
+
drives.push("-device".to_owned());
// drive serial is used by VM to map .fidx files to /dev paths
let serial = file.strip_suffix(".img.fidx").unwrap_or(&file);
- drives.push(format!("virtio-blk-pci,drive=drive{},serial={}", id, serial));
+ drives.push(format!(
+ "virtio-blk-pci,drive=drive{},serial={},bus=bridge{}",
+ id, serial, bus
+ ));
id += 1;
}
--
2.20.1
^ permalink raw reply [flat|nested] 17+ messages in thread* [pbs-devel] applied: [PATCH proxmox-backup 4/9] file-restore: support more drives
2021-05-06 15:26 ` [pbs-devel] [PATCH proxmox-backup 4/9] file-restore: support more drives Stefan Reiter
@ 2021-05-07 7:04 ` Thomas Lamprecht
0 siblings, 0 replies; 17+ messages in thread
From: Thomas Lamprecht @ 2021-05-07 7:04 UTC (permalink / raw)
To: Proxmox Backup Server development discussion, Stefan Reiter
On 06.05.21 17:26, Stefan Reiter wrote:
> A PCI bus can only support up to 32 devices, so excluding built-in
> devices that left us with a maximum of about 25 drives. By adding a new
> PCI bridge every 32 devices (starting at bridge ID 2 to avoid conflicts
> with automatic bridges), we can theoretically support up to 8096 drives.
>
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
>
> Tested with a 41 disk VM, the most I could force into a PVE config.
>
> src/bin/proxmox_file_restore/qemu_helper.rs | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
>
applied, thanks!
^ permalink raw reply [flat|nested] 17+ messages in thread
* [pbs-devel] [PATCH proxmox-backup 5/9] file-restore-daemon: work around tokio DuplexStream bug
2021-05-06 15:26 [pbs-devel] [PATCH 0/9] Debug mode and smaller fixes for single file restore Stefan Reiter
` (3 preceding siblings ...)
2021-05-06 15:26 ` [pbs-devel] [PATCH proxmox-backup 4/9] file-restore: support more drives Stefan Reiter
@ 2021-05-06 15:26 ` Stefan Reiter
2021-05-06 18:12 ` Thomas Lamprecht
2021-05-06 15:26 ` [pbs-devel] [PATCH proxmox-backup 6/9] file-restore-daemon: watchdog: add inhibit for long downloads Stefan Reiter
` (4 subsequent siblings)
9 siblings, 1 reply; 17+ messages in thread
From: Stefan Reiter @ 2021-05-06 15:26 UTC (permalink / raw)
To: pbs-devel
See this PR for more info: https://github.com/tokio-rs/tokio/pull/3756
As a workaround use a pair of connected unix sockets - this obviously
incurs some overhead, albeit not measureable on my machine. Once tokio
includes the fix we can go back to a DuplexStream for performance and
simplicity.
Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---
Thanks to @Wolfgang for helping debug this!
src/bin/proxmox_restore_daemon/api.rs | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/src/bin/proxmox_restore_daemon/api.rs b/src/bin/proxmox_restore_daemon/api.rs
index f1d601ce..5aeb69f3 100644
--- a/src/bin/proxmox_restore_daemon/api.rs
+++ b/src/bin/proxmox_restore_daemon/api.rs
@@ -275,7 +275,11 @@ fn extract(
bail!("file or directory {:?} does not exist", path);
}
- let (mut writer, reader) = tokio::io::duplex(1024 * 64);
+ // FIXME: DuplexStream is currently broken and doesn't wake pending writers on close, i.e.
+ // this doesn't drop the WatchdogInhibitor if we encounter an error (client aborts, etc...)
+ // see: https://github.com/tokio-rs/tokio/pull/3756
+ // let (mut writer, reader) = tokio::io::duplex(1024 * 64);
+ let (mut writer, reader) = tokio::net::UnixStream::pair()?;
if pxar {
tokio::spawn(async move {
--
2.20.1
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [pbs-devel] [PATCH proxmox-backup 5/9] file-restore-daemon: work around tokio DuplexStream bug
2021-05-06 15:26 ` [pbs-devel] [PATCH proxmox-backup 5/9] file-restore-daemon: work around tokio DuplexStream bug Stefan Reiter
@ 2021-05-06 18:12 ` Thomas Lamprecht
2021-05-07 7:26 ` Fabian Grünbichler
0 siblings, 1 reply; 17+ messages in thread
From: Thomas Lamprecht @ 2021-05-06 18:12 UTC (permalink / raw)
To: Proxmox Backup Server development discussion, Stefan Reiter
On 06.05.21 17:26, Stefan Reiter wrote:
> See this PR for more info: https://github.com/tokio-rs/tokio/pull/3756
>
> As a workaround use a pair of connected unix sockets - this obviously
> incurs some overhead, albeit not measureable on my machine. Once tokio
> includes the fix we can go back to a DuplexStream for performance and
> simplicity.
>
now that it got merged, do we want to cherry-pick this or await inclusion
in stable instead of applying this workaround?
CC'ing Fabian as our tokio (or better said rust) packaging wizard
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
>
> Thanks to @Wolfgang for helping debug this!
>
> src/bin/proxmox_restore_daemon/api.rs | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/src/bin/proxmox_restore_daemon/api.rs b/src/bin/proxmox_restore_daemon/api.rs
> index f1d601ce..5aeb69f3 100644
> --- a/src/bin/proxmox_restore_daemon/api.rs
> +++ b/src/bin/proxmox_restore_daemon/api.rs
> @@ -275,7 +275,11 @@ fn extract(
> bail!("file or directory {:?} does not exist", path);
> }
>
> - let (mut writer, reader) = tokio::io::duplex(1024 * 64);
> + // FIXME: DuplexStream is currently broken and doesn't wake pending writers on close, i.e.
> + // this doesn't drop the WatchdogInhibitor if we encounter an error (client aborts, etc...)
> + // see: https://github.com/tokio-rs/tokio/pull/3756
> + // let (mut writer, reader) = tokio::io::duplex(1024 * 64);
> + let (mut writer, reader) = tokio::net::UnixStream::pair()?;
>
> if pxar {
> tokio::spawn(async move {
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [pbs-devel] [PATCH proxmox-backup 5/9] file-restore-daemon: work around tokio DuplexStream bug
2021-05-06 18:12 ` Thomas Lamprecht
@ 2021-05-07 7:26 ` Fabian Grünbichler
0 siblings, 0 replies; 17+ messages in thread
From: Fabian Grünbichler @ 2021-05-07 7:26 UTC (permalink / raw)
To: Proxmox Backup Server development discussion, Stefan Reiter,
Thomas Lamprecht
On May 6, 2021 8:12 pm, Thomas Lamprecht wrote:
> On 06.05.21 17:26, Stefan Reiter wrote:
>> See this PR for more info: https://github.com/tokio-rs/tokio/pull/3756
>>
>> As a workaround use a pair of connected unix sockets - this obviously
>> incurs some overhead, albeit not measureable on my machine. Once tokio
>> includes the fix we can go back to a DuplexStream for performance and
>> simplicity.
>>
>
> now that it got merged, do we want to cherry-pick this or await inclusion
> in stable instead of applying this workaround?
>
> CC'ing Fabian as our tokio (or better said rust) packaging wizard
we can
A) use this workaround, and switch back once a fixed tokio version is
available
B) cherry-pick the fix into our tokio package, not use this workaround,
package and bump dependency ASAP once a tokio version with the fix is
available
A has the advantage of not (subtly) breaking third-party builds that use
crates from crates.io, but requires us to remember switching back once
the fix is available. B has the advantage that we don't have to
remember anything, since we'll update tokio sooner or later anyway ;)
both is fine for me, cherry-picking is about 10 minutes worth of work
so..
>
>> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
>> ---
>>
>> Thanks to @Wolfgang for helping debug this!
>>
>> src/bin/proxmox_restore_daemon/api.rs | 6 +++++-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/bin/proxmox_restore_daemon/api.rs b/src/bin/proxmox_restore_daemon/api.rs
>> index f1d601ce..5aeb69f3 100644
>> --- a/src/bin/proxmox_restore_daemon/api.rs
>> +++ b/src/bin/proxmox_restore_daemon/api.rs
>> @@ -275,7 +275,11 @@ fn extract(
>> bail!("file or directory {:?} does not exist", path);
>> }
>>
>> - let (mut writer, reader) = tokio::io::duplex(1024 * 64);
>> + // FIXME: DuplexStream is currently broken and doesn't wake pending writers on close, i.e.
>> + // this doesn't drop the WatchdogInhibitor if we encounter an error (client aborts, etc...)
>> + // see: https://github.com/tokio-rs/tokio/pull/3756
>> + // let (mut writer, reader) = tokio::io::duplex(1024 * 64);
>> + let (mut writer, reader) = tokio::net::UnixStream::pair()?;
>>
>> if pxar {
>> tokio::spawn(async move {
>>
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [pbs-devel] [PATCH proxmox-backup 6/9] file-restore-daemon: watchdog: add inhibit for long downloads
2021-05-06 15:26 [pbs-devel] [PATCH 0/9] Debug mode and smaller fixes for single file restore Stefan Reiter
` (4 preceding siblings ...)
2021-05-06 15:26 ` [pbs-devel] [PATCH proxmox-backup 5/9] file-restore-daemon: work around tokio DuplexStream bug Stefan Reiter
@ 2021-05-06 15:26 ` Stefan Reiter
2021-05-06 15:26 ` [pbs-devel] [PATCH proxmox-backup 7/9] file-restore-daemon: limit concurrent download calls Stefan Reiter
` (3 subsequent siblings)
9 siblings, 0 replies; 17+ messages in thread
From: Stefan Reiter @ 2021-05-06 15:26 UTC (permalink / raw)
To: pbs-devel
The extract API call may be active for more than the watchdog timeout,
so a simple ping is not enough.
This adds an "inhibit" API, which will stop the watchdog from completing
as long as at least one WatchdogInhibitor instance is alive. Keep one in
the download task, so it will be dropped once it completes (or errors).
Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---
src/bin/proxmox_restore_daemon/api.rs | 8 ++++++--
src/bin/proxmox_restore_daemon/watchdog.rs | 24 +++++++++++++++++++++-
2 files changed, 29 insertions(+), 3 deletions(-)
diff --git a/src/bin/proxmox_restore_daemon/api.rs b/src/bin/proxmox_restore_daemon/api.rs
index 5aeb69f3..c578b2c0 100644
--- a/src/bin/proxmox_restore_daemon/api.rs
+++ b/src/bin/proxmox_restore_daemon/api.rs
@@ -25,7 +25,7 @@ use proxmox_backup::tools::{self, fs::read_subdir, zip::zip_directory};
use pxar::encoder::aio::TokioWriter;
-use super::{disk::ResolveResult, watchdog_remaining, watchdog_ping};
+use super::{disk::ResolveResult, watchdog_remaining, watchdog_inhibit, watchdog_ping};
// NOTE: All API endpoints must have Permission::Superuser, as the configs for authentication do
// not exist within the restore VM. Safety is guaranteed by checking a ticket via a custom ApiAuth.
@@ -248,8 +248,10 @@ fn extract(
_info: &ApiMethod,
_rpcenv: Box<dyn RpcEnvironment>,
) -> ApiResponseFuture {
- watchdog_ping();
+ // download can take longer than watchdog timeout, inhibit until done
+ let _inhibitor = watchdog_inhibit();
async move {
+ let _inhibitor = _inhibitor;
let path = tools::required_string_param(¶m, "path")?;
let mut path = base64::decode(path)?;
if let Some(b'/') = path.last() {
@@ -283,6 +285,7 @@ fn extract(
if pxar {
tokio::spawn(async move {
+ let _inhibitor = _inhibitor;
let result = async move {
// pxar always expects a directory as it's root, so to accommodate files as
// well we encode the parent dir with a filter only matching the target instead
@@ -340,6 +343,7 @@ fn extract(
});
} else {
tokio::spawn(async move {
+ let _inhibitor = _inhibitor;
let result = async move {
if vm_path.is_dir() {
zip_directory(&mut writer, &vm_path).await?;
diff --git a/src/bin/proxmox_restore_daemon/watchdog.rs b/src/bin/proxmox_restore_daemon/watchdog.rs
index 399f99a7..24997809 100644
--- a/src/bin/proxmox_restore_daemon/watchdog.rs
+++ b/src/bin/proxmox_restore_daemon/watchdog.rs
@@ -4,6 +4,9 @@ use proxmox::tools::time::epoch_i64;
const TIMEOUT: i64 = 600; // seconds
static TRIGGERED: AtomicI64 = AtomicI64::new(0);
+static INHIBITORS: AtomicI64 = AtomicI64::new(0);
+
+pub struct WatchdogInhibitor {}
fn handle_expired() -> ! {
use nix::sys::reboot;
@@ -37,5 +40,24 @@ pub fn watchdog_ping() {
/// Returns the remaining time before watchdog expiry in seconds
pub fn watchdog_remaining() -> i64 {
- TIMEOUT - (epoch_i64() - TRIGGERED.load(Ordering::Acquire))
+ if INHIBITORS.load(Ordering::Acquire) > 0 {
+ TIMEOUT
+ } else {
+ TIMEOUT - (epoch_i64() - TRIGGERED.load(Ordering::Acquire))
+ }
+}
+
+/// Returns an object that inhibts watchdog expiry for its lifetime, it will issue a ping on Drop
+pub fn watchdog_inhibit() -> WatchdogInhibitor {
+ let prev = INHIBITORS.fetch_add(1, Ordering::AcqRel);
+ log::info!("Inhibit added: {}", prev + 1);
+ WatchdogInhibitor {}
+}
+
+impl Drop for WatchdogInhibitor {
+ fn drop(&mut self) {
+ watchdog_ping();
+ let prev = INHIBITORS.fetch_sub(1, Ordering::AcqRel);
+ log::info!("Inhibit dropped: {}", prev - 1);
+ }
}
--
2.20.1
^ permalink raw reply [flat|nested] 17+ messages in thread* [pbs-devel] [PATCH proxmox-backup 7/9] file-restore-daemon: limit concurrent download calls
2021-05-06 15:26 [pbs-devel] [PATCH 0/9] Debug mode and smaller fixes for single file restore Stefan Reiter
` (5 preceding siblings ...)
2021-05-06 15:26 ` [pbs-devel] [PATCH proxmox-backup 6/9] file-restore-daemon: watchdog: add inhibit for long downloads Stefan Reiter
@ 2021-05-06 15:26 ` Stefan Reiter
2021-05-06 15:26 ` [pbs-devel] [PATCH proxmox-backup-restore-image 8/9] kernel: power off on panic Stefan Reiter
` (2 subsequent siblings)
9 siblings, 0 replies; 17+ messages in thread
From: Stefan Reiter @ 2021-05-06 15:26 UTC (permalink / raw)
To: pbs-devel
While the issue with vsock packets starving kernel memory is mostly
worked around by the '64k -> 4k buffer' patch in
'proxmox-backup-restore-image', let's be safe and also limit the number
of concurrent transfers. 8 downloads per VM seems like a fair value.
Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---
Mostly optional, included since I had it lying around for a while, but only now
is it useable, since it suffered from the same tokio issue as the watchdog
inhibit.
src/bin/proxmox_restore_daemon/api.rs | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/src/bin/proxmox_restore_daemon/api.rs b/src/bin/proxmox_restore_daemon/api.rs
index c578b2c0..f2732e64 100644
--- a/src/bin/proxmox_restore_daemon/api.rs
+++ b/src/bin/proxmox_restore_daemon/api.rs
@@ -6,6 +6,7 @@ use hyper::{header, Body, Response, StatusCode};
use log::error;
use pathpatterns::{MatchEntry, MatchPattern, MatchType, Pattern};
use serde_json::Value;
+use tokio::sync::Semaphore;
use std::ffi::OsStr;
use std::fs;
@@ -41,6 +42,8 @@ pub const ROUTER: Router = Router::new()
.get(&list_subdirs_api_method!(SUBDIRS))
.subdirs(SUBDIRS);
+static DOWNLOAD_SEM: Semaphore = Semaphore::const_new(8);
+
fn read_uptime() -> Result<f32, Error> {
let uptime = fs::read_to_string("/proc/uptime")?;
// unwrap the Option, if /proc/uptime is empty we have bigger problems
@@ -252,6 +255,12 @@ fn extract(
let _inhibitor = watchdog_inhibit();
async move {
let _inhibitor = _inhibitor;
+
+ let _permit = match DOWNLOAD_SEM.try_acquire() {
+ Ok(permit) => permit,
+ Err(_) => bail!("maximum concurrent download limit reached, please wait for another restore to finish before attempting a new one"),
+ };
+
let path = tools::required_string_param(¶m, "path")?;
let mut path = base64::decode(path)?;
if let Some(b'/') = path.last() {
@@ -286,6 +295,7 @@ fn extract(
if pxar {
tokio::spawn(async move {
let _inhibitor = _inhibitor;
+ let _permit = _permit;
let result = async move {
// pxar always expects a directory as it's root, so to accommodate files as
// well we encode the parent dir with a filter only matching the target instead
@@ -344,6 +354,7 @@ fn extract(
} else {
tokio::spawn(async move {
let _inhibitor = _inhibitor;
+ let _permit = _permit;
let result = async move {
if vm_path.is_dir() {
zip_directory(&mut writer, &vm_path).await?;
--
2.20.1
^ permalink raw reply [flat|nested] 17+ messages in thread* [pbs-devel] [PATCH proxmox-backup-restore-image 8/9] kernel: power off on panic
2021-05-06 15:26 [pbs-devel] [PATCH 0/9] Debug mode and smaller fixes for single file restore Stefan Reiter
` (6 preceding siblings ...)
2021-05-06 15:26 ` [pbs-devel] [PATCH proxmox-backup 7/9] file-restore-daemon: limit concurrent download calls Stefan Reiter
@ 2021-05-06 15:26 ` Stefan Reiter
2021-05-06 15:26 ` [pbs-devel] [PATCH proxmox-backup-restore-image 9/9] add debug initramfs as seperate package Stefan Reiter
2021-05-25 11:50 ` [pbs-devel] applied-series: [PATCH 0/9] Debug mode and smaller fixes for single file restore Thomas Lamprecht
9 siblings, 0 replies; 17+ messages in thread
From: Stefan Reiter @ 2021-05-06 15:26 UTC (permalink / raw)
To: pbs-devel
Instead of just rebooting, which may lead to an infinite loop, try to
resolve the situation by just powering off the VM - it can be restarted
any time anyway.
Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---
...restore-halt-machine-on-kernel-panic.patch | 32 +++++++++++++++++++
1 file changed, 32 insertions(+)
create mode 100644 src/patches/kernel/0004-PBS-restore-halt-machine-on-kernel-panic.patch
diff --git a/src/patches/kernel/0004-PBS-restore-halt-machine-on-kernel-panic.patch b/src/patches/kernel/0004-PBS-restore-halt-machine-on-kernel-panic.patch
new file mode 100644
index 0000000..d79833f
--- /dev/null
+++ b/src/patches/kernel/0004-PBS-restore-halt-machine-on-kernel-panic.patch
@@ -0,0 +1,32 @@
+From 7222e7424aab957f63b98853ea9fb30eec83666e Mon Sep 17 00:00:00 2001
+From: Stefan Reiter <s.reiter@proxmox.com>
+Date: Mon, 3 May 2021 11:13:10 +0200
+Subject: [PATCH] PBS-restore: halt machine on kernel panic
+
+Otherwise we might get into a loop where the user-space watchdog never
+has time to start, and thus the VM will run forever. Still not an idea
+options, since the kernel might hang and not panic, but better than
+nothing, and at least solves the out-of-memory forever looping.
+
+Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
+---
+ kernel/panic.c | 3 +++
+ 1 file changed, 3 insertions(+)
+
+diff --git a/kernel/panic.c b/kernel/panic.c
+index 332736a72a58..56339ae5165c 100644
+--- a/kernel/panic.c
++++ b/kernel/panic.c
+@@ -325,6 +325,9 @@ void panic(const char *fmt, ...)
+ }
+ }
+ if (panic_timeout != 0) {
++ /* PBS restore: stop machine on panic, let host deal with it */
++ machine_power_off();
++
+ /*
+ * This will not be a clean reboot, with everything
+ * shutting down. But if there is a chance of
+--
+2.20.1
+
--
2.20.1
^ permalink raw reply [flat|nested] 17+ messages in thread* [pbs-devel] [PATCH proxmox-backup-restore-image 9/9] add debug initramfs as seperate package
2021-05-06 15:26 [pbs-devel] [PATCH 0/9] Debug mode and smaller fixes for single file restore Stefan Reiter
` (7 preceding siblings ...)
2021-05-06 15:26 ` [pbs-devel] [PATCH proxmox-backup-restore-image 8/9] kernel: power off on panic Stefan Reiter
@ 2021-05-06 15:26 ` Stefan Reiter
2021-05-06 18:07 ` Thomas Lamprecht
2021-05-25 11:50 ` [pbs-devel] applied-series: [PATCH 0/9] Debug mode and smaller fixes for single file restore Thomas Lamprecht
9 siblings, 1 reply; 17+ messages in thread
From: Stefan Reiter @ 2021-05-06 15:26 UTC (permalink / raw)
To: pbs-devel
"proxmox-backup-restore-image-debug", containing only the debug
initramfs, so depends on the base "proxmox-backup-restore-image" for the
kernel.
Adapt the init-shim to start an agetty on ttyS1, which the host
can use to connect to a root shell for debugging, and use
create_dir_all, since some debug packages seem to create /sys and /proc
as empty dirs already.
The build_initramfs.sh script is modified to include dependency
resolution via apt-rdepends, so debug packages like agetty (util-linux),
busybox and gdb can easily be added. This now builds both the regular
and the debug binary at once, to avoid downloading shared packages
twice.
Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---
The updated build_initramfs script should also make further additions to the
initrd easier, e.g. adding lvm and zfs tooling if necessary.
Makefile | 13 +++--
debian/control | 10 +++-
...proxmox-backup-restore-image-debug.install | 1 +
...ckup-restore-image-debug.lintian-overrides | 2 +
...roxmox-backup-restore-image-debug.triggers | 1 +
src/Makefile | 5 +-
src/build_initramfs.sh | 55 +++++++++++++------
src/init-shim-rs/src/main.rs | 46 +++++++++++++++-
8 files changed, 106 insertions(+), 27 deletions(-)
create mode 100644 debian/proxmox-backup-restore-image-debug.install
create mode 100644 debian/proxmox-backup-restore-image-debug.lintian-overrides
create mode 100644 debian/proxmox-backup-restore-image-debug.triggers
diff --git a/Makefile b/Makefile
index d11ac3e..92aa791 100644
--- a/Makefile
+++ b/Makefile
@@ -2,11 +2,14 @@ include /usr/share/dpkg/pkg-info.mk
include /usr/share/dpkg/architecture.mk
PACKAGE=proxmox-backup-restore-image
+PACKAGE_DBG=proxmox-backup-restore-image-debug
BUILDDIR=${PACKAGE}-${DEB_VERSION_UPSTREAM_REVISION}
DEB=${PACKAGE}_${DEB_VERSION}_${DEB_BUILD_ARCH}.deb
DSC=${PACKAGE}_${DEB_VERSION}.dsc
+DEB_DBG=${PACKAGE_DBG}_${DEB_VERSION}_${DEB_BUILD_ARCH}.deb
+DSC_DBG=${PACKAGE_DBG}_${DEB_VERSION}.dsc
all: deb
@@ -32,21 +35,23 @@ ${BUILDDIR}: submodules.prepared
deb: ${DEB}
${DEB}: ${BUILDDIR}
cd ${BUILDDIR}; dpkg-buildpackage -b -us -uc
- lintian ${DEB}
+ lintian ${DEB} ${DEB_DBG}
+${DEB_DBG}: ${DEB}
.PHONY: dsc
dsc: ${DSC}
${DSC}: ${BUILDDIR}
cd ${BUILDDIR}; dpkg-buildpackage -S -us -uc -d
- lintian ${DSC}
+ lintian ${DSC} ${DSC_DBG}
+${DSC_DBG}: ${DSC}
.PHONY: dinstall
dinstall: deb
- dpkg -i ${DEB}
+ dpkg -i ${DEB} ${DEB_DBG}
.PHONY: upload
upload: ${DEB}
- tar cf - ${DEB} | ssh -X repoman@repo.proxmox.com upload --product pbs,pve --dist buster
+ tar cf - ${DEB} ${DEB_DBG} | ssh -X repoman@repo.proxmox.com upload --product pbs,pve --dist buster
.PHONY: clean
clean:
diff --git a/debian/control b/debian/control
index 5b50392..b87c903 100644
--- a/debian/control
+++ b/debian/control
@@ -2,7 +2,8 @@ Source: proxmox-backup-restore-image
Section: admin
Priority: optional
Maintainer: Proxmox Support Team <support@proxmox.com>
-Build-Depends: asciidoc-base,
+Build-Depends: apt-rdepends,
+ asciidoc-base,
automake,
bc,
bison,
@@ -34,3 +35,10 @@ Description: Kernel/initramfs images for Proxmox Backup single-file restore.
Preconfigured images used as base for single file restore of Proxmox Backup
Server snapshots. Not really useful on their own, so best used together with
the proxmox-backup-file-restore package, which provide the actual tools.
+
+Package: proxmox-backup-restore-image-debug
+Architecture: amd64
+Depends: proxmox-backup-restore-image
+Description: Debug initramfs image for Proxmox Backup single-file restore.
+ Not required for production use, only useful for manual inspection of file
+ restore VMs. Includes busybox and gdb.
diff --git a/debian/proxmox-backup-restore-image-debug.install b/debian/proxmox-backup-restore-image-debug.install
new file mode 100644
index 0000000..02ffb49
--- /dev/null
+++ b/debian/proxmox-backup-restore-image-debug.install
@@ -0,0 +1 @@
+build/initramfs/initramfs-debug.img /usr/lib/x86_64-linux-gnu/proxmox-backup/file-restore/
diff --git a/debian/proxmox-backup-restore-image-debug.lintian-overrides b/debian/proxmox-backup-restore-image-debug.lintian-overrides
new file mode 100644
index 0000000..ad59aad
--- /dev/null
+++ b/debian/proxmox-backup-restore-image-debug.lintian-overrides
@@ -0,0 +1,2 @@
+missing-depends-on-sensible-utils
+uses-dpkg-database-directly
diff --git a/debian/proxmox-backup-restore-image-debug.triggers b/debian/proxmox-backup-restore-image-debug.triggers
new file mode 100644
index 0000000..948d898
--- /dev/null
+++ b/debian/proxmox-backup-restore-image-debug.triggers
@@ -0,0 +1 @@
+activate-noawait proxmox-backup-restore-image-update
diff --git a/src/Makefile b/src/Makefile
index 37f385f..a398ea1 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -9,12 +9,13 @@ SHIM_DIR=init-shim-rs
KERNEL_IMG=${BUILDDIR}/bzImage
INITRAMFS_IMG=${INITRAMFS_BUILDDIR}/initramfs.img
+INITRAMFS_IMG_DBG=${INITRAMFS_BUILDDIR}/initramfs-debug.img
CONFIG=config-base
RUST_SRC=$(wildcard ${SHIM_DIR}/**/*.rs) ${SHIM_DIR}/Cargo.toml
-all: ${KERNEL_IMG} ${INITRAMFS_IMG}
+all: ${KERNEL_IMG} ${INITRAMFS_IMG_DBG}
${BUILDDIR}.prepared: ${CONFIG}
rm -rf ${BUILDDIR}
@@ -54,6 +55,8 @@ ${INITRAMFS_IMG}: ${BUILDDIR}.prepared ${RUST_SRC} build_initramfs.sh
cd ${SHIM_DIR}; cargo build --release
sh build_initramfs.sh
+${INITRAMFS_IMG_DBG}: ${INITRAMFS_IMG}
+
.PHONY: test-run
test-run: ${KERNEL_IMG} ${INITRAMFS_IMG}
# note: this will always fail since /proxmox-restore-daemon is not
diff --git a/src/build_initramfs.sh b/src/build_initramfs.sh
index 4efa29b..c4ee95c 100755
--- a/src/build_initramfs.sh
+++ b/src/build_initramfs.sh
@@ -6,29 +6,36 @@ ROOT="root"
BUILDDIR="build/initramfs"
INIT="../../init-shim-rs/target/release/init-shim-rs"
-PKGS=" \
- libc6:amd64 \
- libgcc1:amd64 \
- libstdc++6:amd64 \
- libssl1.1:amd64 \
- libattr1:amd64 \
- libacl1:amd64
-"
-
echo "Using build dir: $BUILDDIR"
rm -rf "$BUILDDIR"
mkdir -p "$BUILDDIR"
cd "$BUILDDIR"
mkdir "$ROOT"
-# add necessary packages to initramfs
-for pkg in $PKGS; do
- apt-get download "$pkg"
- dpkg-deb -x ./*.deb "$ROOT"
+# adds necessary packages to initramfs build root folder
+add_pkgs() {
+ DEPS=""
+ for pkg in $1; do
+ LOCAL_DEPS=$(apt-rdepends -f Depends -s Depends "$pkg" | grep -v '^ ')
+ DEPS="$DEPS $LOCAL_DEPS"
+ done
+ # debconf and gcc are unnecessary
+ DEPS=$(echo "$DEPS" |\
+ sed -E 's/debconf(-2\.0)?//' |\
+ sed -E 's/gcc-.{1,2}-base//')
+ apt-get download $DEPS
+ for deb in ./*.deb; do
+ dpkg-deb -x "$deb" "$ROOT"
+ done
rm ./*.deb
-done
+}
-rm -rf ${ROOT:?}/usr/share # contains only docs and debian stuff
+make_cpio() {
+ fakeroot -- sh -c "
+ cd '$ROOT';
+ find . -print0 | cpio --null -oV --format=newc -F ../$1
+ "
+}
cp $INIT "$ROOT/init"
chmod a+x "$ROOT/init" # just to be sure
@@ -36,7 +43,19 @@ chmod a+x "$ROOT/init" # just to be sure
# tell daemon it's running in the correct environment
touch "$ROOT/restore-vm-marker"
-fakeroot -- sh -c "
- cd '$ROOT';
- find . -print0 | cpio --null -oV --format=newc -F ../initramfs.img
+add_pkgs "
+ libstdc++6:amd64 \
+ libssl1.1:amd64 \
+ libacl1:amd64 \
"
+rm -rf ${ROOT:?}/usr/share # contains only docs and debian stuff
+make_cpio "initramfs.img"
+
+# add debug helpers for debug initramfs, packages from above are included too
+add_pkgs "
+ util-linux:amd64 \
+ busybox-static:amd64 \
+ gdb:amd64 \
+"
+# leave /usr/share here, it contains necessary stuff for gdb
+make_cpio "initramfs-debug.img"
diff --git a/src/init-shim-rs/src/main.rs b/src/init-shim-rs/src/main.rs
index 89aff7b..641218f 100644
--- a/src/init-shim-rs/src/main.rs
+++ b/src/init-shim-rs/src/main.rs
@@ -1,6 +1,8 @@
-use anyhow::Error;
+use anyhow::{bail, Error};
use std::ffi::CStr;
use std::fs;
+use std::path::PathBuf;
+use std::process::Command;
const URANDOM_MAJ: u64 = 1;
const URANDOM_MIN: u64 = 9;
@@ -21,15 +23,54 @@ fn main() {
do_mknod("/dev/urandom", URANDOM_MAJ, URANDOM_MIN)
});
+ if let Err(err) = run_agetty() {
+ // not fatal
+ println!("[init-shim] debug: agetty start failed: {}", err);
+ }
+
let uptime = read_uptime();
println!("[init-shim] reached daemon start after {:.2}s", uptime);
do_run("/proxmox-restore-daemon");
}
+fn run_agetty() -> Result<(), Error> {
+ use nix::unistd::{fork, ForkResult};
+
+ if !PathBuf::from("/sbin/agetty").exists() {
+ bail!("/sbin/agetty not found, probably not running debug mode and safe to ignore");
+ }
+
+ if !PathBuf::from("/sys/class/tty/ttyS1/device/driver/serial8250").exists() {
+ bail!("ttyS1 device does not exist or is not a 8250");
+ }
+
+ let dev = fs::read_to_string("/sys/class/tty/ttyS1/dev")?;
+ let (tty_maj, tty_min) = dev.trim().split_at(dev.find(':').unwrap_or(1));
+ do_mknod("/dev/ttyS1", tty_maj.parse()?, tty_min[1..].parse()?)?;
+
+ match unsafe { fork() } {
+ Ok(ForkResult::Parent { .. }) => {}
+ Ok(ForkResult::Child) => loop {
+ // continue to restart agetty if it exits, this runs in a forked process
+ println!("[init-shim] Spawning new agetty");
+ let res = Command::new("/sbin/agetty")
+ .args(&["-a", "root", "-l", "/bin/busybox", "-o", "sh", "115200", "ttyS1"])
+ .spawn()
+ .unwrap()
+ .wait()
+ .unwrap();
+ println!("[init-shim] agetty exited: {}", res.code().unwrap_or(-1));
+ },
+ Err(err) => println!("fork failed: {}", err),
+ }
+
+ Ok(())
+}
+
fn do_mount(target: &str, fstype: &str) -> Result<(), Error> {
use nix::mount::{mount, MsFlags};
- fs::create_dir(target)?;
+ fs::create_dir_all(target)?;
let none_type: Option<&CStr> = None;
mount(
none_type,
@@ -63,7 +104,6 @@ fn read_uptime() -> f32 {
fn do_run(cmd: &str) -> ! {
use std::io::ErrorKind;
- use std::process::Command;
let spawn_res = Command::new(cmd).env("RUST_BACKTRACE", "1").spawn();
--
2.20.1
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [pbs-devel] [PATCH proxmox-backup-restore-image 9/9] add debug initramfs as seperate package
2021-05-06 15:26 ` [pbs-devel] [PATCH proxmox-backup-restore-image 9/9] add debug initramfs as seperate package Stefan Reiter
@ 2021-05-06 18:07 ` Thomas Lamprecht
0 siblings, 0 replies; 17+ messages in thread
From: Thomas Lamprecht @ 2021-05-06 18:07 UTC (permalink / raw)
To: Proxmox Backup Server development discussion, Stefan Reiter
On 06.05.21 17:26, Stefan Reiter wrote:
> "proxmox-backup-restore-image-debug", containing only the debug
> initramfs, so depends on the base "proxmox-backup-restore-image" for the
> kernel.
>
> Adapt the init-shim to start an agetty on ttyS1, which the host
> can use to connect to a root shell for debugging, and use
> create_dir_all, since some debug packages seem to create /sys and /proc
> as empty dirs already.
>
> The build_initramfs.sh script is modified to include dependency
> resolution via apt-rdepends, so debug packages like agetty (util-linux),
> busybox and gdb can easily be added. This now builds both the regular
> and the debug binary at once, to avoid downloading shared packages
> twice.
>
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
>
> The updated build_initramfs script should also make further additions to the
> initrd easier, e.g. adding lvm and zfs tooling if necessary.
>
> Makefile | 13 +++--
> debian/control | 10 +++-
> ...proxmox-backup-restore-image-debug.install | 1 +
> ...ckup-restore-image-debug.lintian-overrides | 2 +
> ...roxmox-backup-restore-image-debug.triggers | 1 +
> src/Makefile | 5 +-
> src/build_initramfs.sh | 55 +++++++++++++------
> src/init-shim-rs/src/main.rs | 46 +++++++++++++++-
> 8 files changed, 106 insertions(+), 27 deletions(-)
> create mode 100644 debian/proxmox-backup-restore-image-debug.install
> create mode 100644 debian/proxmox-backup-restore-image-debug.lintian-overrides
> create mode 100644 debian/proxmox-backup-restore-image-debug.triggers
>
> diff --git a/Makefile b/Makefile
> index d11ac3e..92aa791 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2,11 +2,14 @@ include /usr/share/dpkg/pkg-info.mk
> include /usr/share/dpkg/architecture.mk
>
> PACKAGE=proxmox-backup-restore-image
> +PACKAGE_DBG=proxmox-backup-restore-image-debug
>
> BUILDDIR=${PACKAGE}-${DEB_VERSION_UPSTREAM_REVISION}
>
> DEB=${PACKAGE}_${DEB_VERSION}_${DEB_BUILD_ARCH}.deb
> DSC=${PACKAGE}_${DEB_VERSION}.dsc
> +DEB_DBG=${PACKAGE_DBG}_${DEB_VERSION}_${DEB_BUILD_ARCH}.deb
> +DSC_DBG=${PACKAGE_DBG}_${DEB_VERSION}.dsc
>
multiple source control files do not really make sense. There's only one source
per d/control instance (normally per repo) for Debians POV...
> all: deb
>
> @@ -32,21 +35,23 @@ ${BUILDDIR}: submodules.prepared
> deb: ${DEB}
> ${DEB}: ${BUILDDIR}
> cd ${BUILDDIR}; dpkg-buildpackage -b -us -uc
> - lintian ${DEB}
> + lintian ${DEB} ${DEB_DBG}
> +${DEB_DBG}: ${DEB}
>
> .PHONY: dsc
> dsc: ${DSC}
> ${DSC}: ${BUILDDIR}
> cd ${BUILDDIR}; dpkg-buildpackage -S -us -uc -d
> - lintian ${DSC}
> + lintian ${DSC} ${DSC_DBG}
> +${DSC_DBG}: ${DSC}
>
> .PHONY: dinstall
> dinstall: deb
> - dpkg -i ${DEB}
> + dpkg -i ${DEB} ${DEB_DBG}
>
> .PHONY: upload
> upload: ${DEB}
> - tar cf - ${DEB} | ssh -X repoman@repo.proxmox.com upload --product pbs,pve --dist buster
> + tar cf - ${DEB} ${DEB_DBG} | ssh -X repoman@repo.proxmox.com upload --product pbs,pve --dist buster
>
> .PHONY: clean
> clean:
> diff --git a/debian/control b/debian/control
> index 5b50392..b87c903 100644
> --- a/debian/control
> +++ b/debian/control
> @@ -2,7 +2,8 @@ Source: proxmox-backup-restore-image
> Section: admin
> Priority: optional
> Maintainer: Proxmox Support Team <support@proxmox.com>
> -Build-Depends: asciidoc-base,
> +Build-Depends: apt-rdepends,
> + asciidoc-base,
> automake,
> bc,
> bison,
> @@ -34,3 +35,10 @@ Description: Kernel/initramfs images for Proxmox Backup single-file restore.
> Preconfigured images used as base for single file restore of Proxmox Backup
> Server snapshots. Not really useful on their own, so best used together with
> the proxmox-backup-file-restore package, which provide the actual tools.
> +
> +Package: proxmox-backup-restore-image-debug
> +Architecture: amd64
> +Depends: proxmox-backup-restore-image
> +Description: Debug initramfs image for Proxmox Backup single-file restore.
> + Not required for production use, only useful for manual inspection of file
> + restore VMs. Includes busybox and gdb.
> diff --git a/debian/proxmox-backup-restore-image-debug.install b/debian/proxmox-backup-restore-image-debug.install
> new file mode 100644
> index 0000000..02ffb49
> --- /dev/null
> +++ b/debian/proxmox-backup-restore-image-debug.install
> @@ -0,0 +1 @@
> +build/initramfs/initramfs-debug.img /usr/lib/x86_64-linux-gnu/proxmox-backup/file-restore/
> diff --git a/debian/proxmox-backup-restore-image-debug.lintian-overrides b/debian/proxmox-backup-restore-image-debug.lintian-overrides
> new file mode 100644
> index 0000000..ad59aad
> --- /dev/null
> +++ b/debian/proxmox-backup-restore-image-debug.lintian-overrides
> @@ -0,0 +1,2 @@
> +missing-depends-on-sensible-utils
> +uses-dpkg-database-directly
> diff --git a/debian/proxmox-backup-restore-image-debug.triggers b/debian/proxmox-backup-restore-image-debug.triggers
> new file mode 100644
> index 0000000..948d898
> --- /dev/null
> +++ b/debian/proxmox-backup-restore-image-debug.triggers
> @@ -0,0 +1 @@
> +activate-noawait proxmox-backup-restore-image-update
> diff --git a/src/Makefile b/src/Makefile
> index 37f385f..a398ea1 100644
> --- a/src/Makefile
> +++ b/src/Makefile
> @@ -9,12 +9,13 @@ SHIM_DIR=init-shim-rs
>
> KERNEL_IMG=${BUILDDIR}/bzImage
> INITRAMFS_IMG=${INITRAMFS_BUILDDIR}/initramfs.img
> +INITRAMFS_IMG_DBG=${INITRAMFS_BUILDDIR}/initramfs-debug.img
>
> CONFIG=config-base
>
> RUST_SRC=$(wildcard ${SHIM_DIR}/**/*.rs) ${SHIM_DIR}/Cargo.toml
>
> -all: ${KERNEL_IMG} ${INITRAMFS_IMG}
> +all: ${KERNEL_IMG} ${INITRAMFS_IMG_DBG}
>
> ${BUILDDIR}.prepared: ${CONFIG}
> rm -rf ${BUILDDIR}
> @@ -54,6 +55,8 @@ ${INITRAMFS_IMG}: ${BUILDDIR}.prepared ${RUST_SRC} build_initramfs.sh
> cd ${SHIM_DIR}; cargo build --release
> sh build_initramfs.sh
>
> +${INITRAMFS_IMG_DBG}: ${INITRAMFS_IMG}
> +
> .PHONY: test-run
> test-run: ${KERNEL_IMG} ${INITRAMFS_IMG}
> # note: this will always fail since /proxmox-restore-daemon is not
> diff --git a/src/build_initramfs.sh b/src/build_initramfs.sh
> index 4efa29b..c4ee95c 100755
> --- a/src/build_initramfs.sh
> +++ b/src/build_initramfs.sh
> @@ -6,29 +6,36 @@ ROOT="root"
> BUILDDIR="build/initramfs"
> INIT="../../init-shim-rs/target/release/init-shim-rs"
>
> -PKGS=" \
> - libc6:amd64 \
> - libgcc1:amd64 \
> - libstdc++6:amd64 \
> - libssl1.1:amd64 \
> - libattr1:amd64 \
> - libacl1:amd64
> -"
> -
> echo "Using build dir: $BUILDDIR"
> rm -rf "$BUILDDIR"
> mkdir -p "$BUILDDIR"
> cd "$BUILDDIR"
> mkdir "$ROOT"
>
> -# add necessary packages to initramfs
> -for pkg in $PKGS; do
> - apt-get download "$pkg"
> - dpkg-deb -x ./*.deb "$ROOT"
> +# adds necessary packages to initramfs build root folder
> +add_pkgs() {
> + DEPS=""
> + for pkg in $1; do
> + LOCAL_DEPS=$(apt-rdepends -f Depends -s Depends "$pkg" | grep -v '^ ')
> + DEPS="$DEPS $LOCAL_DEPS"
> + done
> + # debconf and gcc are unnecessary
> + DEPS=$(echo "$DEPS" |\
> + sed -E 's/debconf(-2\.0)?//' |\
> + sed -E 's/gcc-.{1,2}-base//')
> + apt-get download $DEPS
> + for deb in ./*.deb; do
> + dpkg-deb -x "$deb" "$ROOT"
> + done
> rm ./*.deb
> -done
> +}
>
> -rm -rf ${ROOT:?}/usr/share # contains only docs and debian stuff
> +make_cpio() {
> + fakeroot -- sh -c "
> + cd '$ROOT';
> + find . -print0 | cpio --null -oV --format=newc -F ../$1
> + "
> +}
>
> cp $INIT "$ROOT/init"
> chmod a+x "$ROOT/init" # just to be sure
> @@ -36,7 +43,19 @@ chmod a+x "$ROOT/init" # just to be sure
> # tell daemon it's running in the correct environment
> touch "$ROOT/restore-vm-marker"
>
> -fakeroot -- sh -c "
> - cd '$ROOT';
> - find . -print0 | cpio --null -oV --format=newc -F ../initramfs.img
> +add_pkgs "
> + libstdc++6:amd64 \
> + libssl1.1:amd64 \
> + libacl1:amd64 \
> "
> +rm -rf ${ROOT:?}/usr/share # contains only docs and debian stuff
> +make_cpio "initramfs.img"
> +
> +# add debug helpers for debug initramfs, packages from above are included too
> +add_pkgs "
> + util-linux:amd64 \
> + busybox-static:amd64 \
> + gdb:amd64 \
> +"
> +# leave /usr/share here, it contains necessary stuff for gdb
> +make_cpio "initramfs-debug.img"
> diff --git a/src/init-shim-rs/src/main.rs b/src/init-shim-rs/src/main.rs
> index 89aff7b..641218f 100644
> --- a/src/init-shim-rs/src/main.rs
> +++ b/src/init-shim-rs/src/main.rs
> @@ -1,6 +1,8 @@
> -use anyhow::Error;
> +use anyhow::{bail, Error};
> use std::ffi::CStr;
> use std::fs;
> +use std::path::PathBuf;
> +use std::process::Command;
>
> const URANDOM_MAJ: u64 = 1;
> const URANDOM_MIN: u64 = 9;
> @@ -21,15 +23,54 @@ fn main() {
> do_mknod("/dev/urandom", URANDOM_MAJ, URANDOM_MIN)
> });
>
> + if let Err(err) = run_agetty() {
> + // not fatal
> + println!("[init-shim] debug: agetty start failed: {}", err);
> + }
> +
> let uptime = read_uptime();
> println!("[init-shim] reached daemon start after {:.2}s", uptime);
>
> do_run("/proxmox-restore-daemon");
> }
>
> +fn run_agetty() -> Result<(), Error> {
> + use nix::unistd::{fork, ForkResult};
> +
> + if !PathBuf::from("/sbin/agetty").exists() {
> + bail!("/sbin/agetty not found, probably not running debug mode and safe to ignore");
> + }
> +
> + if !PathBuf::from("/sys/class/tty/ttyS1/device/driver/serial8250").exists() {
> + bail!("ttyS1 device does not exist or is not a 8250");
> + }
> +
> + let dev = fs::read_to_string("/sys/class/tty/ttyS1/dev")?;
> + let (tty_maj, tty_min) = dev.trim().split_at(dev.find(':').unwrap_or(1));
> + do_mknod("/dev/ttyS1", tty_maj.parse()?, tty_min[1..].parse()?)?;
> +
> + match unsafe { fork() } {
> + Ok(ForkResult::Parent { .. }) => {}
> + Ok(ForkResult::Child) => loop {
> + // continue to restart agetty if it exits, this runs in a forked process
> + println!("[init-shim] Spawning new agetty");
> + let res = Command::new("/sbin/agetty")
> + .args(&["-a", "root", "-l", "/bin/busybox", "-o", "sh", "115200", "ttyS1"])
> + .spawn()
> + .unwrap()
> + .wait()
> + .unwrap();
> + println!("[init-shim] agetty exited: {}", res.code().unwrap_or(-1));
> + },
> + Err(err) => println!("fork failed: {}", err),
> + }
> +
> + Ok(())
> +}
> +
> fn do_mount(target: &str, fstype: &str) -> Result<(), Error> {
> use nix::mount::{mount, MsFlags};
> - fs::create_dir(target)?;
> + fs::create_dir_all(target)?;
> let none_type: Option<&CStr> = None;
> mount(
> none_type,
> @@ -63,7 +104,6 @@ fn read_uptime() -> f32 {
>
> fn do_run(cmd: &str) -> ! {
> use std::io::ErrorKind;
> - use std::process::Command;
>
> let spawn_res = Command::new(cmd).env("RUST_BACKTRACE", "1").spawn();
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [pbs-devel] applied-series: [PATCH 0/9] Debug mode and smaller fixes for single file restore
2021-05-06 15:26 [pbs-devel] [PATCH 0/9] Debug mode and smaller fixes for single file restore Stefan Reiter
` (8 preceding siblings ...)
2021-05-06 15:26 ` [pbs-devel] [PATCH proxmox-backup-restore-image 9/9] add debug initramfs as seperate package Stefan Reiter
@ 2021-05-25 11:50 ` Thomas Lamprecht
9 siblings, 0 replies; 17+ messages in thread
From: Thomas Lamprecht @ 2021-05-25 11:50 UTC (permalink / raw)
To: Proxmox Backup Server development discussion, Stefan Reiter
On 06.05.21 17:26, Stefan Reiter wrote:
> Adds a debug mode for restore VMs, triggerable by setting PBS_QEMU_DEBUG=1 on a
> proxmox-file-restore command that starts a new VM. You can then access a root
> shell in the VM via the socket printed.
>
> Also includes several smaller fixes for file restore:
> * try to better avoid stale VMs
> * support more drives per VM
> * work around a tokio bug leaving suspended tasks behind
> * avoid watchdog expiry during downloads
>
>
> proxmox-backup: Stefan Reiter (7):
> file-restore: add debug mode with serial access
> file-restore: try to kill VM when stale
> file-restore: add more RAM for VMs with many drives or debug
> file-restore: support more drives
> file-restore-daemon: work around tokio DuplexStream bug
> file-restore-daemon: watchdog: add inhibit for long downloads
> file-restore-daemon: limit concurrent download calls
>
> proxmox-backup-restore-image: Stefan Reiter (2):
> kernel: power off on panic
> add debug initramfs as seperate package
>
applied remaining patches, thanks!
^ permalink raw reply [flat|nested] 17+ messages in thread