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
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.
There was a problem hiding this comment.
Pull request overview
This PR syncs dev to main by clarifying PVE ACL/domains behavior, improving staged restore file application safety, and updating documentation to reflect the new semantics (atomic writes + ownership/permission handling).
Changes:
- Add atomic file-write helper(s) for staged restores that enforce final mode bits and preserve/derive ownership/group.
- Reduce noise around optional PVE
domains.cfgby suppressing “not found” warnings/counting when absent on default installs. - Update docs/README/config templates to clarify access-control scope and restore permission behavior.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal/orchestrator/fs_atomic.go | Introduces atomic write + inherited directory metadata helpers used by staged restore paths. |
| internal/orchestrator/fs_atomic_test.go | Adds unit tests for umask-proof mode enforcement and ownership/group preservation/inheritance behaviors. |
| internal/orchestrator/restore_notifications.go | Switches staged notification config writes to atomic writes. |
| internal/orchestrator/pve_staged_apply.go | Switches staged /etc/vzdump.conf writes to atomic writes; formatting-only struct alignment changes. |
| internal/orchestrator/pbs_staged_apply.go | Switches staged PBS config writes to atomic writes. |
| internal/orchestrator/network_staged_apply.go | Uses inherited-meta directory creation + atomic file writes for staged network overlay application. |
| internal/orchestrator/restore_firewall.go | Uses inherited-meta directory creation during staged firewall directory syncing. |
| internal/orchestrator/restore_ha.go | Uses inherited-meta directory creation for /etc/pve/ha; removes trailing whitespace. |
| internal/orchestrator/restore_access_control.go | Removes the older local writeFileAtomic implementation (now centralized in fs_atomic.go). |
| internal/orchestrator/.backup.lock | Updates pid/time content (appears to be a runtime lock artifact). |
| internal/config/templates/backup.env | Clarifies what BACKUP_PVE_ACL covers (users/roles/groups/ACL; realms when configured). |
| internal/backup/collector_pve.go | Adds suppression for missing domains.cfg warnings/counting on default standalone installs. |
| docs/TROUBLESHOOTING.md | Adds guidance for PBS permission-denied failures after restore. |
| docs/RESTORE_GUIDE.md | Documents atomic staged applies and clarifies domains.cfg/realms optionality and permission semantics. |
| docs/CONFIGURATION.md | Clarifies BACKUP_PVE_ACL meaning in docs. |
| README.md | Adds “Release Testing & Feedback” acknowledgements section and moves repo activity section down. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/orchestrator/.backup.lock
Outdated
Comment on lines
1
to
3
| pid=69074 | ||
| host=pve | ||
| time=2026-02-09T14:30:21+01:00 | ||
| time=2026-02-10T14:08:14+01:00 |
There was a problem hiding this comment.
This looks like a runtime-generated lock file (pid/host/time) rather than source code. Committing it will cause noisy diffs and may break local behavior if tooling expects to manage this file under BACKUP_PATH/LockDir at runtime. Consider removing it from the repo and adding an appropriate .gitignore entry instead.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
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.
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.