Skip to content

Use temp files for Rollback #1285

Closed
aphralG wants to merge 2 commits intomainfrom
use-temp-files-rollback
Closed

Use temp files for Rollback #1285
aphralG wants to merge 2 commits intomainfrom
use-temp-files-rollback

Conversation

@aphralG
Copy link
Contributor

@aphralG aphralG commented Sep 22, 2025

Proposed changes

  • Removed rollbackFileContents map[string][]byte which was used to rollback files during a failed config apply
  • Instead write files that have beed updated or deleted to a temp Dir rollback<some-number> in /var/lib/nginx-agent/
  • Clear all temp files and temp dirs after a config apply is complete in the ClearCache function

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING document
  • I have run make install-tools and have attached any dependency changes to this pull request
  • If applicable, I have added tests that prove my fix is effective or that my feature works
  • If applicable, I have checked that any relevant tests pass after adding my changes
  • If applicable, I have updated any relevant documentation (README.md)
  • If applicable, I have tested my cross-platform changes on Ubuntu 22, Redhat 8, SUSE 15 and FreeBSD 13

@aphralG aphralG self-assigned this Sep 22, 2025
@aphralG aphralG requested a review from a team as a code owner September 22, 2025 13:52
@github-actions github-actions bot added the chore Pull requests for routine tasks label Sep 22, 2025
Comment on lines +201 to +202
outputFile, createErr := os.Create(destPath)
if createErr != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • we're destroying any file permissions it had when it gets moved. Read from sourcepath and apply to destpath?
  • It's often better to create a temp file in the target location and then do a rename once the data is written. This way other processes don't see partially written files at expected locations.

@UnwashedMeme
Copy link
Member

For a while now I've been wanting an NGINX with chroot option:

  1. create a new chroot for NGINX
    • especially if you can take advantage of some underlying COW semantics, e.g. zfs, btrfs, overlayfs
  2. test the new config nginx --chroot=/var/lib/nginx/chroots/$UUID -t /etc/nginx/nginx.conf
  3. tell nginx to pivot and reload nginx --chroot=/var/lib/nginx/chroots/$UUID -- this can no longer be a simple SIGHUP.

But this would make these tasks so easy to do-- we can prepare the new space, and if it doesn't work there's no trying to copy files back into the right location.

I've been thinking maybe we could do this with a eBPF program that intercepts open calls to rewrite the path to the desired chroot and then step 3 becomes 1. replace the eBPF, 2. SIGHUP. Would need to ensure the newly forked workers receive the new eBPF and bonus points if the old workers maintain the chroot/eBPF-rewrite until they die.

@aphralG aphralG closed this Sep 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

chore Pull requests for routine tasks

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants