Skip to content

Conversation

aphralG
Copy link
Contributor

@aphralG aphralG commented Sep 26, 2025

Proposed changes

Changes:

  • Update Atomic writes to create the temp directory in the location of the NGINX config or a default of "/etc/nginx" if we can't get the config dir. Instead of the file being moved it is now renamed preserving the file permissions etc

  • Rollback has changed to before a config apply starts create a copy of the file to a temp directory (need to use copy here to not delete original) at the location of the NGINX config. If a rollback occurs these files are renamed to rollback the files

  • File watchers are now enabled by a specific topic sent after we have cleaned up the temp directories, File Watcher now when disabled removes all directories it is watching and then they are added back when it is enabled. This is to prevent an issue where the events that happened while the watcher was disabled were still on the channel when the watcher was enabled causing agent to process those events unnecessarily.

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 26, 2025
@aphralG aphralG requested a review from a team as a code owner September 26, 2025 13:11
@github-actions github-actions bot added bug Something isn't working chore Pull requests for routine tasks labels Sep 26, 2025
@aphralG aphralG changed the title DRAFT: Rollback Using Temp Files & Fix Atomic Writes Rollback Using Temp Files & Fix Atomic Writes Sep 29, 2025
@aphralG aphralG requested a review from dhurley September 30, 2025 11:51
@sean-breen
Copy link
Contributor

sean-breen commented Oct 1, 2025

if we can't get the config dir

What does this mean? We will write into the /etc/nginx directory by default? Shouldn't we stop if we can't find the config dir? What happens when we can't find the directory and someone has set it to a non-default location?

@aphralG
Copy link
Contributor Author

aphralG commented Oct 1, 2025

if we can't get the config dir

What does this mean? We will write into the /etc/nginx directory by default? Shouldn't we stop if we can't find the config dir? What happens when we can't find the directory and someone has set it to a non-default location?

I can change it to fail just after discussing with Donal we were just trying to set it to a default in case something happened where it wouldn't be set. But in reality it will probably always bet set as it is sent with the config context.

@sean-breen
Copy link
Contributor

sean-breen commented Oct 1, 2025

if we can't get the config dir
What does this mean? We will write into the /etc/nginx directory by default? Shouldn't we stop if we can't find the config dir? What happens when we can't find the directory and someone has set it to a non-default location?

I can change it to fail just after discussing with Donal we were just trying to set it to a default in case something happened where it wouldn't be set. But in reality it will probably always bet set as it is sent with the config context.

Fair, I'm more concerned with us writing the intermediary files into the /etc/nginx directory, would it be possible to write the temp files to /etc/nginx-agent or another location that we know will exist and we have access to?

) error
SetIsConnected(isConnected bool)
MoveFilesFromTempDirectory(ctx context.Context, fileAction *model.FileCache, tempDir string) error
renameFile(ctx context.Context, hash, fileName, tempDir string) error
Copy link
Collaborator

Choose a reason for hiding this comment

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

Think you can remove this from the interface?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working chore Pull requests for routine tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants