Conversation
Owner
tis24dev
commented
Feb 10, 2026
tis24dev
commented
- Clarify PVE ACL and silence domains warning
- Implement atomic writes and preserve ownership
- Update README.md
- tests: configure CheckerConfig with temp dirs
- Inherit parent group for staged restores
- fs_atomic: fsync dir on write; rclone default case
- Add FS operation hooks and update tests
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.
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.
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.
Update atomic restore behavior to propagate ownership/permissions from the nearest existing parent when creating directories or writing files. ensureDirExistsWithInheritedMeta now walks up to find an existing ancestor, creates missing nested dirs, and applies inherited uid/gid and mode reliably (with proper error handling and closes). desiredOwnershipForAtomicWrite was adjusted to repair files that have root:root by inheriting the parent's group, prefer parent gid when appropriate, and handle existing files consistently. Added unit tests to cover repairing root group on existing dest and creating nested dirs with inherited metadata. Docs updated to note that staged restores enforce ownership/permissions for created parent directories and mention the repair of common root:root group regressions.
Add a default branch in discoverRcloneBackups to cancel and skip unsupported switch cases. Enhance writeFileAtomic to ensure durability: call f.Sync() after writing, introduce syncDir to open and fsync the parent directory (handling EINVAL/ENOTSUP as non-fatal for dir fsync), and return proper wrapped errors on failures. This ensures both file data and directory metadata are flushed for atomic writes.
Introduce test hooks (atomicGeteuid, atomicFileChown, atomicFileChmod, atomicFileSync) and refactor fs_atomic to use them instead of direct os/file calls to improve testability. Update tests to use a statOverrideFS and fakeFileInfo to simulate file metadata and capture chown calls, removing the need to run tests as root. Adjust expectations for seeded modes and add a new test to verify chown behavior when simulating root for nested directory creation.
There was a problem hiding this comment.
Pull request overview
This PR synchronizes the development branch to main, implementing atomic file writes with ownership/permission preservation for staged restore operations and adding related improvements.
Changes:
- Implements centralized atomic file writing (
writeFileAtomic) and directory creation (ensureDirExistsWithInheritedMeta) with proper ownership/permission handling to avoid relying on umask - Migrates all staged apply operations (PBS, PVE, network, notifications, firewall, HA) to use the new atomic file operations
- Adds comprehensive tests for atomic operations and updates existing tests with proper temporary directory configuration
- Updates documentation to explain atomic write behavior and adds PBS permission troubleshooting guidance
- Suppresses unnecessary warnings for optional files (domains.cfg) and adds clarifying comments for BACKUP_PVE_ACL
- Adds default case handling for rclone backup discovery switch statement
- Removes accidentally committed
.backup.lockfile and adds release testing acknowledgments section to README
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| internal/orchestrator/fs_atomic.go | New file implementing atomic file writes with ownership/permission preservation and directory creation with inherited metadata |
| internal/orchestrator/fs_atomic_test.go | Comprehensive tests for atomic file operations including umask enforcement, ownership preservation, and group inheritance |
| internal/orchestrator/restore_notifications.go | Migrated to use centralized writeFileAtomic function |
| internal/orchestrator/restore_ha.go | Migrated to use ensureDirExistsWithInheritedMeta for proper directory ownership |
| internal/orchestrator/restore_firewall.go | Migrated to use ensureDirExistsWithInheritedMeta for directory creation |
| internal/orchestrator/restore_access_control.go | Removed old writeFileAtomic implementation (moved to fs_atomic.go) |
| internal/orchestrator/pve_staged_apply.go | Migrated to writeFileAtomic and aligned struct field formatting |
| internal/orchestrator/pbs_staged_apply.go | Migrated to writeFileAtomic for PBS configuration files |
| internal/orchestrator/network_staged_apply.go | Migrated to atomic operations for network file overlay |
| internal/orchestrator/orchestrator_test.go | Updated tests with proper temporary directory configuration for CheckerConfig |
| internal/orchestrator/backup_sources.go | Added default case to rclone discovery switch to properly handle unexpected item kinds |
| internal/orchestrator/.backup.lock | Removed accidentally committed lock file |
| internal/config/templates/backup.env | Added clarifying comment for BACKUP_PVE_ACL explaining it includes realms when configured |
| internal/backup/collector_pve.go | Added suppressNotFoundLog flag and suppressed warning for optional domains.cfg file |
| docs/TROUBLESHOOTING.md | Added comprehensive PBS permission issue troubleshooting section |
| docs/RESTORE_GUIDE.md | Updated to explain atomic write behavior and ownership/permission handling for staged categories |
| docs/CONFIGURATION.md | Updated BACKUP_PVE_ACL comment to clarify scope |
| README.md | Added release testing & feedback section acknowledging community contributors |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.