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.

Sign up for GitHub

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

Jump to bottom

fix: do the inspector after forking for daemon mode #731

Merged
merged 1 commit into from
Jul 22, 2019
Merged

fix: do the inspector after forking for daemon mode #731

merged 1 commit into from
Jul 22, 2019

Conversation

fntlnz
Copy link
Contributor

@fntlnz fntlnz commented Jul 18, 2019 •
edited
Loading

What type of PR is this?
/kind bug

Any specific area of the project related to this PR?

/area engine

What this PR does / why we need it:

When falco is started in daemon node, it does not carry the threads related to container metadata and in general all the other threads done before the fork because fork only carries the main thread.

This solves that problem by moving all the thread openings after fork.

Which issue(s) this PR fixes:

Fixes #716

Special notes for your reviewer:

Here are packages to test:
DEB
Download: https://falco-dev-public.s3.amazonaws.com/pr-731/falco-fntlnz-test-x86_64.deb
Sha256: 1586eea70cd750f730341a0dfa83275d7893fa5aca0c0f0929d0b3df5557c000 falco-fntlnz-test-x86_64.deb

RPM
Download: https://falco-dev-public.s3.amazonaws.com/pr-731/falco-fntlnz-test-x86_64.rpm
Sha256: 60c8eda61db76d50e78403c73304c2d77dff6fe62901852bf995222b81e81abc falco-fntlnz-test-x86_64.rpm

TAR.GZ
Download: https://falco-dev-public.s3.amazonaws.com/pr-731/falco-fntlnz-test-x86_64.tar.gz
Sha256: 4b6917d1369b55d757e9eaa6c4e0b9b7e915c279611850c6c6e63a2a4cd095a8 falco-fntlnz-test-x86_64.tar.gz
Does this PR introduce a user-facing change?:

fix: container metadata are now fetched when the falco process is run in daemon mode

@poiana poiana requested review from leodido and mfdii July 18, 2019 22:45
mfdii
mfdii suggested changes Jul 18, 2019
View reviewed changes
userspace/falco/falco.cpp Outdated Show resolved Hide resolved
@mfdii
Copy link
Member

mfdii commented Jul 18, 2019

confirmed this works, however the inspector->start_dropping_mode(1); call needs to be moved to after the innspector->open()

@fntlnz fntlnz force-pushed the fix/716-container-metadata-thread branch from 86d202e to 1de866a Compare July 19, 2019 13:07
@fntlnz
Copy link
Contributor Author

fntlnz commented Jul 19, 2019

Thanks @mfdii forgot that one! Updated PTAL

@fntlnz fntlnz requested a review from mfdii July 19, 2019 13:10
@fntlnz
Copy link
Contributor Author

fntlnz commented Jul 19, 2019 •
edited
Loading

I compiled packages for those who want to test this on x86_64:

DEB
Download: https://falco-dev-public.s3.amazonaws.com/pr-731/falco-fntlnz-test-x86_64.deb
Sha256: 1586eea70cd750f730341a0dfa83275d7893fa5aca0c0f0929d0b3df5557c000 falco-fntlnz-test-x86_64.deb

RPM
Download: https://falco-dev-public.s3.amazonaws.com/pr-731/falco-fntlnz-test-x86_64.rpm
Sha256: 60c8eda61db76d50e78403c73304c2d77dff6fe62901852bf995222b81e81abc falco-fntlnz-test-x86_64.rpm

TAR.GZ
Download: https://falco-dev-public.s3.amazonaws.com/pr-731/falco-fntlnz-test-x86_64.tar.gz
Sha256: 4b6917d1369b55d757e9eaa6c4e0b9b7e915c279611850c6c6e63a2a4cd095a8 falco-fntlnz-test-x86_64.tar.gz

@fntlnz fntlnz mentioned this pull request Jul 19, 2019
Closed
leodido
leodido approved these changes Jul 22, 2019
View reviewed changes
Copy link
Member

@leodido leodido left a comment

Choose a reason for hiding this comment

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

🔥

@poiana poiana added the lgtm label Jul 22, 2019
@poiana
Copy link

poiana commented Jul 22, 2019

LGTM label has been added.

Git tree hash: bf8517c89df1066f4a32520999a54a6c4d0683c3

@mfdii
Copy link
Member

mfdii commented Jul 22, 2019

/approve

@mfdii mfdii requested a review from leodido July 22, 2019 13:47
mfdii
mfdii previously approved these changes Jul 22, 2019
View reviewed changes
leodido
leodido previously approved these changes Jul 22, 2019
View reviewed changes
@leodido
Copy link
Member

leodido commented Jul 22, 2019 •
edited
Loading

Pls rebase and I'll merge this! Thanks @fntlnz :)

Nevermind, done by myself.

@fntlnz @leodido
fix: do the inspector after forking for daemon mode
7ffe5f1 
Signed-off-by: Lorenzo Fontana <lo@linux.com>
@leodido leodido dismissed stale reviews from mfdii and themself via 7ffe5f1 July 22, 2019 21:52
@leodido leodido force-pushed the fix/716-container-metadata-thread branch from 1de866a to 7ffe5f1 Compare July 22, 2019 21:52
@poiana poiana removed the lgtm label Jul 22, 2019
@leodido leodido self-requested a review July 22, 2019 21:52
leodido
leodido approved these changes Jul 22, 2019
View reviewed changes
Copy link
Member

@leodido leodido left a comment

Choose a reason for hiding this comment

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

🔥

@poiana poiana added the lgtm label Jul 22, 2019
@poiana
Copy link

poiana commented Jul 22, 2019

LGTM label has been added.

Git tree hash: 22dad3ec4ea2027e434ca43c6a2dcc7271152c18

@poiana
Copy link

poiana commented Jul 22, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: leodido, mfdii

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@mfdii
Copy link
Member

mfdii commented Jul 22, 2019

/approved

@leodido leodido merged commit 4b2ea32 into dev Jul 22, 2019
@poiana poiana deleted the fix/716-container-metadata-thread branch July 22, 2019 23:13
@fntlnz fntlnz mentioned this pull request Jul 30, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leodido leodido leodido approved these changes

@mfdii mfdii mfdii left review comments

Assignees

@leodido leodido

@mfdii mfdii

Labels
approved area/engine dco-signoff: yes kind/bug lgtm release-note size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Falco is not showing container information. All container events are coming with name "incomplete"
4 participants
@fntlnz @mfdii @poiana @leodido