Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Log Directories Created After Startup not Tracked #385

Merged
merged 26 commits into from
Oct 7, 2022

Conversation

stsantilena
Copy link
Contributor

@stsantilena stsantilena commented Aug 3, 2022

Enables the Agent to track directories that are listed in the configuration but are initially missing. Once the directory is create its added to the file system cache and tracked.

@stsantilena stsantilena added bug Something isn't working enhancement New feature or request labels Aug 17, 2022
@stsantilena stsantilena removed the enhancement New feature or request label Aug 18, 2022
@stsantilena stsantilena changed the title [WIP] Log Directories Created After Startup not Tracked Log Directories Created After Startup not Tracked Sep 9, 2022
@stsantilena stsantilena marked this pull request as ready for review September 9, 2022 22:24
) -> Result<DirPathBuf, DirPathBufError> {
match path {
Some(p) => {
if p.is_dir() {
Copy link
Contributor

Choose a reason for hiding this comment

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

does it work for:

  • it is a file
  • permission error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The configuration is meant to be set as directory so a single file wouldn't be expected. I followed same pattern as existing directory check. As for permissions, in theory, I think could fail is for some reason it was running in non-root mode and didn't have directory access.

warn!("{:?} is not a directory; moving to parent directory", p);
let mut postfix_pathbuf = PathBuf::new();
let mut pop_pathbuf = PathBuf::new();
if let Some(path) = postfix {
Copy link
Contributor

Choose a reason for hiding this comment

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

shadowing fn arg on purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The postfix function input is only non-None in the event that a given directory doesn't exist, in which case it saves that piece of the path so it can properly construct the missing directory path.

Copy link
Contributor

Choose a reason for hiding this comment

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

i was thinking about var name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I see. Updated var name path -> pop

common/fs/src/cache/dir_path.rs Show resolved Hide resolved
@@ -344,6 +344,7 @@ impl TryFrom<RawConfig> for Config {
// Filter off paths that are not directories and warn about them
.filter_map(|d| {
d.clone()
//TODO: Need to fix this iteration as warning now happens in dir_path.rs
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@c-nixon I don't think there needs to any change here, perhaps the warning might be different message? Also, thinking the iteration structure should stay for Windows.

Copy link
Contributor

@dkhokhlov dkhokhlov Sep 29, 2022

Choose a reason for hiding this comment

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

if we allow to specify non-existing dirs - why do we filter them here out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This iteration just filters out anything that throws an error. Originally, if a directory didn't exist and error was thrown. Now, there is a more complex process happening down in find_valid_path that allows directories that don't yet exist; still propagating errors up but for different reasons. So really just the comment is no longer valid.

Ok(DirPathBuf { inner: path })
Ok(DirPathBuf {
inner: path,
postfix: None,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to add this line to fix the Windows build. This shouldn't affect existing Windows behavior.

@@ -1573,7 +1573,6 @@ async fn test_line_redact() {
test_line_rules(None, None, redact, to_write, expected).await;
}

#[ignore]
#[tokio::test]
async fn test_directory_created_after_initialization() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turned on bug specific integration test from previous PR.

let parent = match level_up(&p) {
Some(p) => {
if p == root_pathbuf {
panic!("configured directory recursed to root!");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Set to panic if function recurses back to the root directory. This will watch the entire root file system which is most likely not desirable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed this so that instead of throwing a panic, it will warn and watch root level for missing dir.

Copy link
Collaborator

@c-nixon c-nixon left a comment

Choose a reason for hiding this comment

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

This all looks good to me!

} else {
warn!("{:?} is not a directory; moving to parent directory", p);
let root_pathbuf = Path::new("/");
let mut postfix_pathbuf = PathBuf::new();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like there might be some way to use the std::path Component types to implement this, but that might just complicate the logic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's probably a few different ways to go about it. The tricky part is flipping the path each integration to make sure it comes out correct, and plays nicely with the path rules.

@c-nixon c-nixon force-pushed the LOG-9806-log-dir-not-tracked-cont branch from 909d8e8 to fb32617 Compare October 7, 2022 10:55
@c-nixon c-nixon merged commit 64ab339 into master Oct 7, 2022
@c-nixon c-nixon deleted the LOG-9806-log-dir-not-tracked-cont branch October 7, 2022 14:09
@dkhokhlov dkhokhlov linked an issue Jan 26, 2023 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Agent monitoring directories that dont existing at start time.
3 participants