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

Add docker logs plugin #507

Merged
merged 28 commits into from
Feb 16, 2024
Merged

Add docker logs plugin #507

merged 28 commits into from
Feb 16, 2024

Conversation

JSCU-CNI
Copy link
Contributor

As discussed in #441.

@Horofic Horofic self-requested a review January 17, 2024 12:10
JSCU-CNI added a commit to JSCU-CNI/dissect.target that referenced this pull request Jan 17, 2024
introduces dependency on fox-it#507.
@JSCU-CNI
Copy link
Contributor Author

JSCU-CNI commented Feb 1, 2024

Thanks for the thorough review @Schamper and @Horofic. I've implemented all feedback where applicable and replied to all comments where things are still unclear.

Copy link

codecov bot commented Feb 1, 2024

Codecov Report

Attention: 14 lines in your changes are missing coverage. Please review.

Comparison is base (6a42513) 73.69% compared to head (fd6b1be) 73.79%.

Files Patch % Lines
dissect/target/plugins/apps/container/docker.py 89.74% 12 Missing ⚠️
dissect/target/target.py 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #507      +/-   ##
==========================================
+ Coverage   73.69%   73.79%   +0.10%     
==========================================
  Files         278      279       +1     
  Lines       23153    23272     +119     
==========================================
+ Hits        17063    17174     +111     
- Misses       6090     6098       +8     
Flag Coverage Δ
unittests 73.79% <90.66%> (+0.10%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Horofic Horofic mentioned this pull request Feb 14, 2024
@JSCU-CNI
Copy link
Contributor Author

Thanks @Horofic. There seems to be a race condition in the order in which records are returned. I am unable to reproduce the failing tests locally. Unfortunately sorting on records is not possible, what do you recommend as a fix?

@Schamper
Copy link
Member

Schamper commented Feb 14, 2024

Thanks @Horofic. There seems to be a race condition in the order in which records are returned. I am unable to reproduce the failing tests locally. Unfortunately sorting on records is not possible, what do you recommend as a fix?

Maybe the test log files are iterated/loaded in a different order (based on some filesystem/Python listdir quirk).

@Horofic
Copy link
Contributor

Horofic commented Feb 14, 2024

Thanks @Horofic. There seems to be a race condition in the order in which records are returned. I am unable to reproduce the failing tests locally. Unfortunately sorting on records is not possible, what do you recommend as a fix?

Maybe the test log files are iterated/loaded in a different order (based on some filesystem/Python listdir quirk).

Check. In that case, incorporating the last remarks from @Schamper should do it.

@Horofic Horofic requested review from Horofic and Schamper February 14, 2024 14:07
Horofic
Horofic previously approved these changes Feb 14, 2024
@Schamper
Copy link
Member

Thanks @Horofic. There seems to be a race condition in the order in which records are returned. I am unable to reproduce the failing tests locally. Unfortunately sorting on records is not possible, what do you recommend as a fix?

Maybe the test log files are iterated/loaded in a different order (based on some filesystem/Python listdir quirk).

Check. In that case, incorporating the last remarks from @Schamper should do it.

It was a remark at a possible cause, the suggestion wasn't to fix that 😄

@JSCU-CNI
Copy link
Contributor Author

JSCU-CNI commented Feb 15, 2024

Perhaps fox-it/flow.record#108 fixes this when adding results.sort() in the tests or the following for the meantime:

import operator
results.sort(key=operator.attrgetter('_generated'))

Interesting to see the 3.12 CI environment does not seem to be impacted by this.

@Schamper
Copy link
Member

Perhaps fox-it/flow.record#108 fixes this when adding results.sort() in the tests or the following for the meantime:

import operator
results.sort(key=operator.attrgetter('_generated'))

Interesting to see the 3.12 CI environment does not seem to be impacted by this.

I don't think this will fix the problem, as I think the problem lies in the order at which the log files are mapped into the virtual filesystem. At least on my local system, it gets mapped correctly, but for some reason in the CI it gets mapped the other way around (probably something to do with timestamps).

Sorting on the record content (e.g. ts, message) should result in a consistent test output.

@JSCU-CNI
Copy link
Contributor Author

Seems like the pypy39 pipeline is having a rough Friday :(

Fatal Python error: Segmentation fault

@Schamper
Copy link
Member

Seems like the pypy39 pipeline is having a rough Friday :(

Fatal Python error: Segmentation fault

Yeah unfortunately the PyPy 3.9 CI does that. We have to keep re-running it until it doesn't segfault 😄

@Schamper Schamper requested a review from Horofic February 16, 2024 15:40
@Schamper Schamper merged commit b5a01cd into fox-it:main Feb 16, 2024
16 checks passed
@JSCU-CNI JSCU-CNI deleted the feature/docker-logs branch February 16, 2024 16:04
Poeloe pushed a commit that referenced this pull request Feb 29, 2024
Zawadidone pushed a commit to Zawadidone/dissect.target that referenced this pull request Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants