From 07d467e22e0b325a1addaab5e74a4fc93e5b0dd6 Mon Sep 17 00:00:00 2001 From: tis24dev Date: Mon, 9 Feb 2026 15:46:57 +0100 Subject: [PATCH 1/4] Clarify PVE ACL and silence domains warning Clarify PVE access-control handling and avoid noisy warnings when domains.cfg is absent. Changes: - docs/CONFIGURATION.md, docs/RESTORE_GUIDE.md: expand BACKUP_PVE_ACL/docs to note users/roles/groups/ACL and that domains/realms are optional when not present. - internal/config/templates/backup.env: add explanatory comment for BACKUP_PVE_ACL. - internal/backup/collector_pve.go: introduce suppressNotFoundLog and mark domains.cfg as optional so it's not counted or warned about on default standalone PVE installs. - internal/orchestrator/.backup.lock: update PID and timestamp. Rationale: domains.cfg may not exist on default standalone installations; suppressing the warning and not marking it as missing reduces false-positive log noise and incorrect missing counts. --- docs/CONFIGURATION.md | 2 +- docs/RESTORE_GUIDE.md | 7 ++++--- internal/backup/collector_pve.go | 17 +++++++++++++---- internal/config/templates/backup.env | 2 +- internal/orchestrator/.backup.lock | 4 ++-- 5 files changed, 21 insertions(+), 11 deletions(-) diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index 57ad64f..33deac6 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -907,7 +907,7 @@ BACKUP_PVE_FIREWALL=true # PVE firewall configuration BACKUP_VZDUMP_CONFIG=true # /etc/vzdump.conf # Access control lists -BACKUP_PVE_ACL=true # User permissions +BACKUP_PVE_ACL=true # Access control (users/roles/groups/ACL; realms when configured) # Scheduled jobs BACKUP_PVE_JOBS=true # Backup jobs configuration diff --git a/docs/RESTORE_GUIDE.md b/docs/RESTORE_GUIDE.md index 7e0bcd9..40af4d7 100644 --- a/docs/RESTORE_GUIDE.md +++ b/docs/RESTORE_GUIDE.md @@ -98,7 +98,7 @@ Each category is handled in one of three ways: | `storage_pve` | PVE Storage Configuration | **Staged** storage definitions (applied via API) + VZDump config | `./etc/pve/storage.cfg`
`./etc/pve/datacenter.cfg`
`./etc/vzdump.conf` | | `pve_jobs` | PVE Backup Jobs | **Staged** scheduled backup jobs (applied via API) | `./etc/pve/jobs.cfg`
`./etc/pve/vzdump.cron` | | `pve_notifications` | PVE Notifications | **Staged** notification targets and matchers (applied via API) | `./etc/pve/notifications.cfg`
`./etc/pve/priv/notifications.cfg` | -| `pve_access_control` | PVE Access Control | **Staged** access control + secrets restored 1:1 via pmxcfs file apply (root@pam safety rail) | `./etc/pve/user.cfg`
`./etc/pve/domains.cfg`
`./etc/pve/priv/shadow.cfg`
`./etc/pve/priv/token.cfg`
`./etc/pve/priv/tfa.cfg` | +| `pve_access_control` | PVE Access Control | **Staged** access control + secrets restored 1:1 via pmxcfs file apply (root@pam safety rail) | `./etc/pve/user.cfg`
`./etc/pve/domains.cfg` (when present)
`./etc/pve/priv/shadow.cfg`
`./etc/pve/priv/token.cfg`
`./etc/pve/priv/tfa.cfg` | | `pve_firewall` | PVE Firewall | **Staged** firewall rules and node host firewall (pmxcfs file apply + rollback timer) | `./etc/pve/firewall/`
`./etc/pve/nodes/*/host.fw` | | `pve_ha` | PVE High Availability (HA) | **Staged** HA resources/groups/rules (pmxcfs file apply + rollback timer) | `./etc/pve/ha/resources.cfg`
`./etc/pve/ha/groups.cfg`
`./etc/pve/ha/rules.cfg` | | `pve_sdn` | PVE SDN | **Staged** SDN definitions (pmxcfs file apply; definitions only) | `./etc/pve/sdn/`
`./etc/pve/sdn.cfg` | @@ -145,8 +145,9 @@ Not all categories are available in every backup. The restore workflow: ### PVE Access Control 1:1 (pve_access_control) On standalone PVE restores (non-cluster backups), ProxSave restores `pve_access_control` **1:1** by applying staged files directly to the pmxcfs mount (`/etc/pve`): -- `/etc/pve/user.cfg` + `/etc/pve/domains.cfg` -- `/etc/pve/priv/{shadow,token,tfa}.cfg` +- `/etc/pve/user.cfg` (includes users/roles/groups/ACL) +- `/etc/pve/domains.cfg` (realms, when present) +- `/etc/pve/priv/{shadow,token,tfa}.cfg` (when present) **Root safety rail**: - `root@pam` is preserved from the fresh install (not imported from the backup). diff --git a/internal/backup/collector_pve.go b/internal/backup/collector_pve.go index ae0109b..756356a 100644 --- a/internal/backup/collector_pve.go +++ b/internal/backup/collector_pve.go @@ -164,6 +164,9 @@ func (c *Collector) populatePVEManifest() { disableHint string log bool countNotFound bool + // suppressNotFoundLog suppresses any log output when a file is not found. + // This is useful for optional files that may not exist on a default installation. + suppressNotFoundLog bool } logEntry := func(opts manifestLogOpts, entry ManifestEntry) { @@ -186,6 +189,9 @@ func (c *Collector) populatePVEManifest() { case StatusSkipped: c.logger.Info(" %s: skipped (excluded)", opts.description) case StatusNotFound: + if opts.suppressNotFoundLog { + return + } if strings.TrimSpace(opts.disableHint) != "" { c.logger.Warning(" %s: not configured. If unused, set %s=false to disable.", opts.description, opts.disableHint) } else { @@ -268,10 +274,13 @@ func (c *Collector) populatePVEManifest() { countNotFound: true, }) record(filepath.Join(pveConfigPath, "domains.cfg"), c.config.BackupPVEACL, manifestLogOpts{ - description: "Domain configuration", - disableHint: "BACKUP_PVE_ACL", - log: true, - countNotFound: true, + description: "Domain configuration", + disableHint: "BACKUP_PVE_ACL", + log: true, + // domains.cfg may not exist on a default standalone PVE install (built-in realms only). + // Don't warn or count as "missing" in that case. + countNotFound: false, + suppressNotFoundLog: true, }) // Scheduled jobs. diff --git a/internal/config/templates/backup.env b/internal/config/templates/backup.env index f579cff..7a7cd71 100644 --- a/internal/config/templates/backup.env +++ b/internal/config/templates/backup.env @@ -276,7 +276,7 @@ METRICS_PATH=${BASE_DIR}/metrics BACKUP_CLUSTER_CONFIG=true BACKUP_PVE_FIREWALL=true BACKUP_VZDUMP_CONFIG=true -BACKUP_PVE_ACL=true +BACKUP_PVE_ACL=true # Access control (users/roles/groups/ACL; realms when configured) BACKUP_PVE_JOBS=true BACKUP_PVE_SCHEDULES=true BACKUP_PVE_REPLICATION=true diff --git a/internal/orchestrator/.backup.lock b/internal/orchestrator/.backup.lock index e38741a..e3dcfb0 100644 --- a/internal/orchestrator/.backup.lock +++ b/internal/orchestrator/.backup.lock @@ -1,3 +1,3 @@ -pid=29780 +pid=47261 host=pve -time=2026-02-09T14:30:21+01:00 +time=2026-02-09T15:38:50+01:00 From c30c30b39bf9a1f5d179fe384cd79ce5f7e709ed Mon Sep 17 00:00:00 2001 From: tis24dev Date: Tue, 10 Feb 2026 15:08:52 +0100 Subject: [PATCH 2/4] Implement atomic writes and preserve ownership Add a centralized atomic write utility and directory metadata helpers (internal/orchestrator/fs_atomic.go) and unit tests (fs_atomic_test.go). Replace ad-hoc MkdirAll/WriteFile calls in staged-apply paths (network, PBS, PVE, firewall, HA, notifications) to use ensureDirExistsWithInheritedMeta and writeFileAtomic so staged restores are applied atomically and final ownership/permissions are enforced (avoiding umask issues). Update docs: README credits release testers and docs/RESTORE_GUIDE.md and TROUBLESHOOTING.md explain atomic/staged behavior and PBS permission recovery steps. Update internal backup lock metadata (.backup.lock). Also remove the previous duplicate writeFileAtomic in restore_access_control.go in favor of the new centralized implementation. --- README.md | 25 +++ docs/RESTORE_GUIDE.md | 11 +- docs/TROUBLESHOOTING.md | 26 +++ internal/orchestrator/.backup.lock | 4 +- internal/orchestrator/fs_atomic.go | 176 ++++++++++++++++++ internal/orchestrator/fs_atomic_test.go | 129 +++++++++++++ internal/orchestrator/network_staged_apply.go | 11 +- internal/orchestrator/pbs_staged_apply.go | 10 +- internal/orchestrator/pve_staged_apply.go | 29 ++- .../orchestrator/restore_access_control.go | 36 ---- internal/orchestrator/restore_firewall.go | 12 +- internal/orchestrator/restore_ha.go | 5 +- .../orchestrator/restore_notifications.go | 5 +- 13 files changed, 390 insertions(+), 89 deletions(-) create mode 100644 internal/orchestrator/fs_atomic.go create mode 100644 internal/orchestrator/fs_atomic_test.go diff --git a/README.md b/README.md index 98ff8ec..2017c99 100644 --- a/README.md +++ b/README.md @@ -75,6 +75,31 @@ Thank you so much! ## Recognitions +## Release Testing & Feedback +A special thanks to the community members who help by testing releases and reporting issues. 💙 + + + + + +
+ + @NukeThemTillTheyGlow + +
+ @NukeThemTillTheyGlow +
+ release testing + 		+ + @marc6901 + +
+ @marc6901 +
+ release testing +
+ ## Repo Activity ![Alt](https://repobeats.axiom.co/api/embed/d9565d6d1ed8222a5da5fedf25c18a9c8beab382.svg "Repobeats analytics image") \ No newline at end of file diff --git a/docs/RESTORE_GUIDE.md b/docs/RESTORE_GUIDE.md index 40af4d7..75d0c85 100644 --- a/docs/RESTORE_GUIDE.md +++ b/docs/RESTORE_GUIDE.md @@ -86,7 +86,7 @@ Restore operations are organized into **20–22 categories** (PBS = 20, PVE = 22 Each category is handled in one of three ways: - **Normal**: extracted directly to `/` (system paths) after safety backup -- **Staged**: extracted to `/tmp/proxsave/restore-stage-*` and then applied in a controlled way (file copy/validation or `pvesh`) +- **Staged**: extracted to `/tmp/proxsave/restore-stage-*` and then applied in a controlled way (file copy/validation or `pvesh`); when staged files are written to system paths, ProxSave applies them **atomically** and enforces the final permissions/ownership (i.e. not left to `umask`) - **Export-only**: extracted to an export directory for manual review (never written to system paths) ### PVE-Specific Categories (11 categories) @@ -2669,11 +2669,10 @@ tar -xzf /path/to/decrypted.tar.gz ./specific/file/path **Q: Does restore preserve file permissions and ownership?** -A: Yes, completely: -- **Ownership**: UID/GID preserved -- **Permissions**: Mode bits preserved -- **Timestamps**: mtime and atime preserved -- **ctime**: Cannot be set (kernel-managed) +A: Yes: +- **Extraction**: ProxSave preserves UID/GID, mode bits and timestamps (mtime/atime) for extracted entries. +- **Staged categories**: files are extracted under `/tmp/proxsave/restore-stage-*` and then applied to system paths using atomic replace; ProxSave explicitly applies mode bits (not left to `umask`) and preserves/derives ownership/group to match expected system defaults (important on PBS, where `proxmox-backup-proxy` runs as `backup`). +- **ctime**: Cannot be set (kernel-managed). --- diff --git a/docs/TROUBLESHOOTING.md b/docs/TROUBLESHOOTING.md index 91cdbdf..90a15d4 100644 --- a/docs/TROUBLESHOOTING.md +++ b/docs/TROUBLESHOOTING.md @@ -623,6 +623,32 @@ MIN_DISK_SPACE_PRIMARY_GB=5 # Lower threshold - If it says **DISARMED/CLEARED**, reconnect using the **post-apply IP** (new config remains active). - Check the rollback log path printed in the footer for details. +#### PBS UI/API fails after restore: `proxmox-backup-proxy` permission denied + +**Symptoms**: +- PBS web UI login fails or API requests return authentication errors +- `systemctl status proxmox-backup-proxy` shows a restart loop +- Journal contains errors like: + - `unable to read "/etc/proxmox-backup/user.cfg" - Permission denied (os error 13)` + - `unable to read "/etc/proxmox-backup/authkey.pub" - Permission denied (os error 13)` + - `configuration directory '/etc/proxmox-backup' permission problem` + +**Cause**: +- PBS services (notably `proxmox-backup-proxy`) run as user `backup` and require specific ownership/permissions under `/etc/proxmox-backup`. +- If staged restore (or manual file copy) rewrites these files with the wrong owner/group/mode, the services cannot read them and may refuse to start. + +**What to do**: +1. Ensure you're running the latest ProxSave build and rerun restore for the staged PBS categories you selected (e.g. `pbs_access_control`, `pbs_jobs`, `pbs_remotes`, `pbs_host`, `pbs_notifications`, `datastore_pbs`). ProxSave applies staged files atomically and enforces final permissions/ownership (not left to `umask`). +2. If PBS is already broken and you need a quick recovery: + - Identify the blocking path component with `namei -l /etc/proxmox-backup/user.cfg`. + - Restore package defaults (recommended): reinstall `proxmox-backup-server` and restart services. Example: + ```bash + apt-get update + apt-get install --reinstall proxmox-backup-server + systemctl restart proxmox-backup proxmox-backup-proxy + ``` + - Or fix ownership/permissions to match a clean install of your PBS version (verify `/etc/proxmox-backup` and the files referenced in the journal are readable by user `backup`). + #### Error during network preflight: `addr_add_dry_run() got an unexpected keyword argument 'nodad'` **Symptoms**: diff --git a/internal/orchestrator/.backup.lock b/internal/orchestrator/.backup.lock index e3dcfb0..bc45580 100644 --- a/internal/orchestrator/.backup.lock +++ b/internal/orchestrator/.backup.lock @@ -1,3 +1,3 @@ -pid=47261 +pid=69074 host=pve -time=2026-02-09T15:38:50+01:00 +time=2026-02-10T14:08:14+01:00 diff --git a/internal/orchestrator/fs_atomic.go b/internal/orchestrator/fs_atomic.go new file mode 100644 index 0000000..d74a5b4 --- /dev/null +++ b/internal/orchestrator/fs_atomic.go @@ -0,0 +1,176 @@ +package orchestrator + +import ( + "errors" + "fmt" + "os" + "path/filepath" + "strings" + "syscall" +) + +type uidGid struct { + uid int + gid int + ok bool +} + +func uidGidFromFileInfo(info os.FileInfo) uidGid { + if info == nil { + return uidGid{} + } + st, ok := info.Sys().(*syscall.Stat_t) + if !ok || st == nil { + return uidGid{} + } + return uidGid{uid: int(st.Uid), gid: int(st.Gid), ok: true} +} + +func modeBits(mode os.FileMode) os.FileMode { + return mode & 0o7777 +} + +func findNearestExistingDirMeta(dir string) (uidGid, os.FileMode) { + dir = filepath.Clean(strings.TrimSpace(dir)) + if dir == "" || dir == "." { + return uidGid{}, 0o755 + } + + candidate := dir + for { + info, err := restoreFS.Stat(candidate) + if err == nil && info != nil && info.IsDir() { + inheritMode := modeBits(info.Mode()) + if inheritMode == 0 { + inheritMode = 0o755 + } + return uidGidFromFileInfo(info), inheritMode + } + + parent := filepath.Dir(candidate) + if parent == candidate || parent == "." || parent == "" { + break + } + candidate = parent + } + + return uidGid{}, 0o755 +} + +func ensureDirExistsWithInheritedMeta(dir string) error { + dir = filepath.Clean(strings.TrimSpace(dir)) + if dir == "" || dir == "." || dir == string(os.PathSeparator) { + return nil + } + + if info, err := restoreFS.Stat(dir); err == nil { + if info != nil && info.IsDir() { + return nil + } + return fmt.Errorf("path exists but is not a directory: %s", dir) + } else if err != nil && !errors.Is(err, os.ErrNotExist) { + return fmt.Errorf("stat %s: %w", dir, err) + } + + owner, perm := findNearestExistingDirMeta(filepath.Dir(dir)) + if err := restoreFS.MkdirAll(dir, perm); err != nil { + return fmt.Errorf("mkdir %s: %w", dir, err) + } + + f, err := restoreFS.Open(dir) + if err != nil { + return fmt.Errorf("open dir %s: %w", dir, err) + } + defer f.Close() + + if os.Geteuid() == 0 && owner.ok { + if err := f.Chown(owner.uid, owner.gid); err != nil { + return fmt.Errorf("chown dir %s: %w", dir, err) + } + } + if err := f.Chmod(perm); err != nil { + return fmt.Errorf("chmod dir %s: %w", dir, err) + } + return nil +} + +func desiredOwnershipForAtomicWrite(destPath string) uidGid { + destPath = filepath.Clean(strings.TrimSpace(destPath)) + if destPath == "" || destPath == "." { + return uidGid{} + } + + if info, err := restoreFS.Stat(destPath); err == nil && info != nil && !info.IsDir() { + return uidGidFromFileInfo(info) + } + + parent := filepath.Dir(destPath) + if info, err := restoreFS.Stat(parent); err == nil && info != nil && info.IsDir() { + parentOwner := uidGidFromFileInfo(info) + if parentOwner.ok { + return uidGid{uid: 0, gid: parentOwner.gid, ok: true} + } + } + + return uidGid{} +} + +func writeFileAtomic(path string, data []byte, perm os.FileMode) error { + path = filepath.Clean(strings.TrimSpace(path)) + if path == "" || path == "." { + return fmt.Errorf("invalid path") + } + perm = modeBits(perm) + if perm == 0 { + perm = 0o644 + } + + dir := filepath.Dir(path) + if err := ensureDirExistsWithInheritedMeta(dir); err != nil { + return err + } + + owner := desiredOwnershipForAtomicWrite(path) + + tmpPath := fmt.Sprintf("%s.proxsave.tmp.%d", path, nowRestore().UnixNano()) + f, err := restoreFS.OpenFile(tmpPath, os.O_CREATE|os.O_WRONLY|os.O_EXCL|os.O_TRUNC, 0o600) + if err != nil { + return err + } + + writeErr := func() error { + if len(data) == 0 { + return nil + } + _, err := f.Write(data) + return err + }() + if writeErr == nil { + if os.Geteuid() == 0 && owner.ok { + if err := f.Chown(owner.uid, owner.gid); err != nil { + writeErr = err + } + } + if writeErr == nil { + if err := f.Chmod(perm); err != nil { + writeErr = err + } + } + } + + closeErr := f.Close() + if writeErr != nil { + _ = restoreFS.Remove(tmpPath) + return writeErr + } + if closeErr != nil { + _ = restoreFS.Remove(tmpPath) + return closeErr + } + + if err := restoreFS.Rename(tmpPath, path); err != nil { + _ = restoreFS.Remove(tmpPath) + return err + } + return nil +} diff --git a/internal/orchestrator/fs_atomic_test.go b/internal/orchestrator/fs_atomic_test.go new file mode 100644 index 0000000..0385501 --- /dev/null +++ b/internal/orchestrator/fs_atomic_test.go @@ -0,0 +1,129 @@ +package orchestrator + +import ( + "os" + "syscall" + "testing" + "time" +) + +func TestWriteFileAtomic_EnforcesModeDespiteUmask(t *testing.T) { + fakeFS := NewFakeFS() + origFS := restoreFS + origTime := restoreTime + t.Cleanup(func() { + restoreFS = origFS + restoreTime = origTime + _ = os.RemoveAll(fakeFS.Root) + }) + + restoreFS = fakeFS + restoreTime = &FakeTime{Current: time.Unix(10, 0)} + + oldUmask := syscall.Umask(0o077) + defer syscall.Umask(oldUmask) + + destPath := "/etc/proxmox-backup/user.cfg" + if err := writeFileAtomic(destPath, []byte("test\n"), 0o640); err != nil { + t.Fatalf("writeFileAtomic: %v", err) + } + + info, err := fakeFS.Stat(destPath) + if err != nil { + t.Fatalf("stat: %v", err) + } + if got := info.Mode().Perm(); got != 0o640 { + t.Fatalf("mode=%#o, want %#o", got, 0o640) + } +} + +func TestWriteFileAtomic_PreservesOwnershipFromExistingDest(t *testing.T) { + if os.Geteuid() != 0 { + t.Skip("requires root") + } + + fakeFS := NewFakeFS() + origFS := restoreFS + origTime := restoreTime + t.Cleanup(func() { + restoreFS = origFS + restoreTime = origTime + _ = os.RemoveAll(fakeFS.Root) + }) + + restoreFS = fakeFS + restoreTime = &FakeTime{Current: time.Unix(11, 0)} + + destPath := "/etc/proxmox-backup/user.cfg" + if err := fakeFS.WriteFile(destPath, []byte("old\n"), 0o640); err != nil { + t.Fatalf("seed dest: %v", err) + } + if err := os.Chown(fakeFS.onDisk(destPath), 123, 456); err != nil { + t.Fatalf("chown seed dest: %v", err) + } + if err := os.Chmod(fakeFS.onDisk(destPath), 0o640); err != nil { + t.Fatalf("chmod seed dest: %v", err) + } + + if err := writeFileAtomic(destPath, []byte("new\n"), 0o640); err != nil { + t.Fatalf("writeFileAtomic: %v", err) + } + + info, err := fakeFS.Stat(destPath) + if err != nil { + t.Fatalf("stat: %v", err) + } + owner := uidGidFromFileInfo(info) + if !owner.ok { + t.Fatalf("uid/gid not available from fileinfo") + } + if owner.uid != 123 || owner.gid != 456 { + t.Fatalf("uid/gid=%d:%d, want %d:%d", owner.uid, owner.gid, 123, 456) + } +} + +func TestWriteFileAtomic_InheritsGroupFromParentWhenDestMissing(t *testing.T) { + if os.Geteuid() != 0 { + t.Skip("requires root") + } + + fakeFS := NewFakeFS() + origFS := restoreFS + origTime := restoreTime + t.Cleanup(func() { + restoreFS = origFS + restoreTime = origTime + _ = os.RemoveAll(fakeFS.Root) + }) + + restoreFS = fakeFS + restoreTime = &FakeTime{Current: time.Unix(12, 0)} + + parentDir := "/etc/proxmox-backup" + if err := fakeFS.MkdirAll(parentDir, 0o700); err != nil { + t.Fatalf("seed parent dir: %v", err) + } + if err := os.Chown(fakeFS.onDisk(parentDir), 777, 888); err != nil { + t.Fatalf("chown seed parent dir: %v", err) + } + if err := os.Chmod(fakeFS.onDisk(parentDir), 0o700); err != nil { + t.Fatalf("chmod seed parent dir: %v", err) + } + + destPath := "/etc/proxmox-backup/remote.cfg" + if err := writeFileAtomic(destPath, []byte("remote: test\n"), 0o640); err != nil { + t.Fatalf("writeFileAtomic: %v", err) + } + + info, err := fakeFS.Stat(destPath) + if err != nil { + t.Fatalf("stat: %v", err) + } + owner := uidGidFromFileInfo(info) + if !owner.ok { + t.Fatalf("uid/gid not available from fileinfo") + } + if owner.uid != 0 || owner.gid != 888 { + t.Fatalf("uid/gid=%d:%d, want %d:%d", owner.uid, owner.gid, 0, 888) + } +} diff --git a/internal/orchestrator/network_staged_apply.go b/internal/orchestrator/network_staged_apply.go index c4bc2f7..3412d15 100644 --- a/internal/orchestrator/network_staged_apply.go +++ b/internal/orchestrator/network_staged_apply.go @@ -70,8 +70,8 @@ func copyDirOverlay(srcDir, destDir string) ([]string, error) { return nil, nil } - if err := restoreFS.MkdirAll(destDir, 0o755); err != nil { - return nil, fmt.Errorf("mkdir %s: %w", destDir, err) + if err := ensureDirExistsWithInheritedMeta(destDir); err != nil { + return nil, fmt.Errorf("ensure %s: %w", destDir, err) } var applied []string @@ -132,17 +132,12 @@ func copyFileOverlay(src, dest string) (bool, error) { return false, fmt.Errorf("read %s: %w", src, err) } - if err := restoreFS.MkdirAll(filepath.Dir(dest), 0o755); err != nil { - return false, fmt.Errorf("mkdir %s: %w", filepath.Dir(dest), err) - } - mode := os.FileMode(0o644) if info != nil { mode = info.Mode().Perm() } - if err := restoreFS.WriteFile(dest, data, mode); err != nil { + if err := writeFileAtomic(dest, data, mode); err != nil { return false, fmt.Errorf("write %s: %w", dest, err) } return true, nil } - diff --git a/internal/orchestrator/pbs_staged_apply.go b/internal/orchestrator/pbs_staged_apply.go index 57124f2..3111daa 100644 --- a/internal/orchestrator/pbs_staged_apply.go +++ b/internal/orchestrator/pbs_staged_apply.go @@ -205,10 +205,7 @@ func applyPBSDatastoreCfgFromStage(ctx context.Context, logger *logging.Logger, } destPath := "/etc/proxmox-backup/datastore.cfg" - if err := restoreFS.MkdirAll(filepath.Dir(destPath), 0o755); err != nil { - return fmt.Errorf("ensure %s: %w", filepath.Dir(destPath), err) - } - if err := restoreFS.WriteFile(destPath, []byte(out.String()), 0o640); err != nil { + if err := writeFileAtomic(destPath, []byte(out.String()), 0o640); err != nil { return fmt.Errorf("write %s: %w", destPath, err) } @@ -380,10 +377,7 @@ func applyPBSConfigFileFromStage(ctx context.Context, logger *logging.Logger, st return nil } - if err := restoreFS.MkdirAll(filepath.Dir(destPath), 0o755); err != nil { - return fmt.Errorf("ensure %s: %w", filepath.Dir(destPath), err) - } - if err := restoreFS.WriteFile(destPath, []byte(trimmed+"\n"), 0o640); err != nil { + if err := writeFileAtomic(destPath, []byte(trimmed+"\n"), 0o640); err != nil { return fmt.Errorf("write %s: %w", destPath, err) } diff --git a/internal/orchestrator/pve_staged_apply.go b/internal/orchestrator/pve_staged_apply.go index 3af6892..a85d816 100644 --- a/internal/orchestrator/pve_staged_apply.go +++ b/internal/orchestrator/pve_staged_apply.go @@ -102,10 +102,7 @@ func applyPVEVzdumpConfFromStage(logger *logging.Logger, stageRoot string) error return removeIfExists(destPath) } - if err := restoreFS.MkdirAll(filepath.Dir(destPath), 0o755); err != nil { - return fmt.Errorf("ensure %s: %w", filepath.Dir(destPath), err) - } - if err := restoreFS.WriteFile(destPath, []byte(trimmed+"\n"), 0o644); err != nil { + if err := writeFileAtomic(destPath, []byte(trimmed+"\n"), 0o644); err != nil { return fmt.Errorf("write %s: %w", destPath, err) } @@ -443,10 +440,10 @@ func pveStorageMountGuardCandidatesFromSections(sections []proxmoxNotificationSe } type pveStorageMountGuardItem struct { - GuardTarget string - StorageID string - StorageType string - IsNetwork bool + GuardTarget string + StorageID string + StorageType string + IsNetwork bool RequiresFstab bool } @@ -462,10 +459,10 @@ func pveStorageMountGuardItems(candidates []pveStorageMountGuardCandidate, mount switch storageType { case "nfs", "cifs", "cephfs", "glusterfs": out = append(out, pveStorageMountGuardItem{ - GuardTarget: filepath.Join("/mnt/pve", storageID), - StorageID: storageID, - StorageType: storageType, - IsNetwork: true, + GuardTarget: filepath.Join("/mnt/pve", storageID), + StorageID: storageID, + StorageType: storageType, + IsNetwork: true, RequiresFstab: false, }) @@ -490,10 +487,10 @@ func pveStorageMountGuardItems(candidates []pveStorageMountGuardCandidate, mount continue } out = append(out, pveStorageMountGuardItem{ - GuardTarget: target, - StorageID: storageID, - StorageType: storageType, - IsNetwork: false, + GuardTarget: target, + StorageID: storageID, + StorageType: storageType, + IsNetwork: false, RequiresFstab: true, }) } diff --git a/internal/orchestrator/restore_access_control.go b/internal/orchestrator/restore_access_control.go index 5556e21..efdda05 100644 --- a/internal/orchestrator/restore_access_control.go +++ b/internal/orchestrator/restore_access_control.go @@ -838,42 +838,6 @@ func readProxmoxConfigSectionsOptional(path string) ([]proxmoxNotificationSectio return parseProxmoxNotificationSections(raw) } -func writeFileAtomic(path string, data []byte, perm os.FileMode) error { - dir := filepath.Dir(path) - if err := restoreFS.MkdirAll(dir, 0o755); err != nil { - return err - } - - tmpPath := fmt.Sprintf("%s.proxsave.tmp.%d", path, nowRestore().UnixNano()) - f, err := restoreFS.OpenFile(tmpPath, os.O_CREATE|os.O_WRONLY|os.O_EXCL|os.O_TRUNC, perm) - if err != nil { - return err - } - - writeErr := func() error { - if len(data) == 0 { - return nil - } - _, err := f.Write(data) - return err - }() - closeErr := f.Close() - if writeErr != nil { - _ = restoreFS.Remove(tmpPath) - return writeErr - } - if closeErr != nil { - _ = restoreFS.Remove(tmpPath) - return closeErr - } - - if err := restoreFS.Rename(tmpPath, path); err != nil { - _ = restoreFS.Remove(tmpPath) - return err - } - return nil -} - func renderProxmoxConfig(sections []proxmoxNotificationSection) string { if len(sections) == 0 { return "" diff --git a/internal/orchestrator/restore_firewall.go b/internal/orchestrator/restore_firewall.go index 91b7f4d..9e655c1 100644 --- a/internal/orchestrator/restore_firewall.go +++ b/internal/orchestrator/restore_firewall.go @@ -613,8 +613,8 @@ func syncDirExact(srcDir, destDir string) ([]string, error) { return nil, nil } - if err := restoreFS.MkdirAll(destDir, 0o755); err != nil { - return nil, fmt.Errorf("mkdir %s: %w", destDir, err) + if err := ensureDirExistsWithInheritedMeta(destDir); err != nil { + return nil, fmt.Errorf("ensure %s: %w", destDir, err) } stageFiles := make(map[string]struct{}) @@ -661,8 +661,8 @@ func syncDirExact(srcDir, destDir string) ([]string, error) { if err != nil { return fmt.Errorf("readlink %s: %w", src, err) } - if err := restoreFS.MkdirAll(filepath.Dir(dest), 0o755); err != nil { - return fmt.Errorf("mkdir %s: %w", filepath.Dir(dest), err) + if err := ensureDirExistsWithInheritedMeta(filepath.Dir(dest)); err != nil { + return fmt.Errorf("ensure %s: %w", filepath.Dir(dest), err) } _ = restoreFS.Remove(dest) if err := restoreFS.Symlink(target, dest); err != nil { @@ -674,8 +674,8 @@ func syncDirExact(srcDir, destDir string) ([]string, error) { if info.IsDir() { stageDirs[rel] = struct{}{} - if err := restoreFS.MkdirAll(dest, 0o755); err != nil { - return fmt.Errorf("mkdir %s: %w", dest, err) + if err := ensureDirExistsWithInheritedMeta(dest); err != nil { + return fmt.Errorf("ensure %s: %w", dest, err) } if err := walkStage(src); err != nil { return err diff --git a/internal/orchestrator/restore_ha.go b/internal/orchestrator/restore_ha.go index a6062a2..5341941 100644 --- a/internal/orchestrator/restore_ha.go +++ b/internal/orchestrator/restore_ha.go @@ -296,8 +296,8 @@ func applyPVEHAFromStage(logger *logging.Logger, stageRoot string) (applied []st stageHA := filepath.Join(stageRoot, "etc", "pve", "ha") destHA := "/etc/pve/ha" - if err := restoreFS.MkdirAll(destHA, 0o755); err != nil { - return nil, fmt.Errorf("mkdir %s: %w", destHA, err) + if err := ensureDirExistsWithInheritedMeta(destHA); err != nil { + return nil, fmt.Errorf("ensure %s: %w", destHA, err) } // Only prune config files if the stage actually contains HA config. @@ -488,4 +488,3 @@ func buildHARollbackScript(markerPath, backupPath, logPath string) string { } return strings.Join(lines, "\n") + "\n" } - diff --git a/internal/orchestrator/restore_notifications.go b/internal/orchestrator/restore_notifications.go index bcc8d88..6181c09 100644 --- a/internal/orchestrator/restore_notifications.go +++ b/internal/orchestrator/restore_notifications.go @@ -326,10 +326,7 @@ func applyConfigFileFromStage(logger *logging.Logger, stageRoot, relPath, destPa return nil } - if err := restoreFS.MkdirAll(filepath.Dir(destPath), 0o755); err != nil { - return fmt.Errorf("ensure %s: %w", filepath.Dir(destPath), err) - } - if err := restoreFS.WriteFile(destPath, []byte(trimmed+"\n"), perm); err != nil { + if err := writeFileAtomic(destPath, []byte(trimmed+"\n"), perm); err != nil { return fmt.Errorf("write %s: %w", destPath, err) } logging.DebugStep(logger, "notifications staged apply file", "Applied %s -> %s", relPath, destPath) From e35aa0a4bc49e8c84224ecc2826f9259442bd95f Mon Sep 17 00:00:00 2001 From: tis24dev Date: Tue, 10 Feb 2026 16:38:05 +0100 Subject: [PATCH 3/4] Update README.md --- README.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 2017c99..3762885 100644 --- a/README.md +++ b/README.md @@ -77,6 +77,7 @@ Thank you so much! ## Release Testing & Feedback A special thanks to the community members who help by testing releases and reporting issues. 💙 +
@@ -100,6 +101,7 @@ A special thanks to the community members who help by testing releases and repor
-## Repo Activity +
+## Repo Activity ![Alt](https://repobeats.axiom.co/api/embed/d9565d6d1ed8222a5da5fedf25c18a9c8beab382.svg "Repobeats analytics image") \ No newline at end of file From f2571c67058f5c749ce3ead3469ad11e2df707c3 Mon Sep 17 00:00:00 2001 From: tis24dev Date: Tue, 10 Feb 2026 17:07:38 +0100 Subject: [PATCH 4/4] tests: configure CheckerConfig with temp dirs Remove stale .backup.lock and update orchestrator tests to create isolated temp directories for backup, logs and lock files. Tests now populate CheckerConfig (BackupPath, LogPath, LockDirPath, LockFilePath, MinDiskPrimaryGB, SafetyFactor, MaxLockAge), call Validate(), and register a cleanup to release the backup lock. This makes the tests self-contained and avoids reliance on a repository lock file, reducing flakiness. --- internal/orchestrator/.backup.lock | 3 -- internal/orchestrator/orchestrator_test.go | 34 ++++++++++++++++++++-- 2 files changed, 32 insertions(+), 5 deletions(-) delete mode 100644 internal/orchestrator/.backup.lock diff --git a/internal/orchestrator/.backup.lock b/internal/orchestrator/.backup.lock deleted file mode 100644 index bc45580..0000000 --- a/internal/orchestrator/.backup.lock +++ /dev/null @@ -1,3 +0,0 @@ -pid=69074 -host=pve -time=2026-02-10T14:08:14+01:00 diff --git a/internal/orchestrator/orchestrator_test.go b/internal/orchestrator/orchestrator_test.go index 68910c5..2b5be72 100644 --- a/internal/orchestrator/orchestrator_test.go +++ b/internal/orchestrator/orchestrator_test.go @@ -413,9 +413,24 @@ func TestOrchestrator_RunPreBackupChecks(t *testing.T) { orch := New(logger, false) - checkerCfg := &checks.CheckerConfig{} + backupDir := t.TempDir() + logDir := t.TempDir() + lockDir := filepath.Join(backupDir, "lock") + checkerCfg := &checks.CheckerConfig{ + BackupPath: backupDir, + LogPath: logDir, + LockDirPath: lockDir, + LockFilePath: filepath.Join(lockDir, ".backup.lock"), + MinDiskPrimaryGB: 0, + SafetyFactor: 1.0, + MaxLockAge: time.Hour, + } + if err := checkerCfg.Validate(); err != nil { + t.Fatalf("checker config validation failed: %v", err) + } checker := checks.NewChecker(logger, checkerCfg) orch.SetChecker(checker) + t.Cleanup(func() { _ = orch.ReleaseBackupLock() }) ctx := context.Background() err := orch.RunPreBackupChecks(ctx) @@ -432,9 +447,24 @@ func TestOrchestrator_RunPreBackupChecks_ContextCancellation(t *testing.T) { orch := New(logger, false) - checkerCfg := &checks.CheckerConfig{} + backupDir := t.TempDir() + logDir := t.TempDir() + lockDir := filepath.Join(backupDir, "lock") + checkerCfg := &checks.CheckerConfig{ + BackupPath: backupDir, + LogPath: logDir, + LockDirPath: lockDir, + LockFilePath: filepath.Join(lockDir, ".backup.lock"), + MinDiskPrimaryGB: 0, + SafetyFactor: 1.0, + MaxLockAge: time.Hour, + } + if err := checkerCfg.Validate(); err != nil { + t.Fatalf("checker config validation failed: %v", err) + } checker := checks.NewChecker(logger, checkerCfg) orch.SetChecker(checker) + t.Cleanup(func() { _ = orch.ReleaseBackupLock() }) ctx, cancel := context.WithCancel(context.Background()) cancel() // Cancel immediately