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(userspace): switch to timer_settime API for stats writer. #2646

Merged
merged 2 commits into from
Jun 22, 2023
Merged

fix(userspace): switch to timer_settime API for stats writer. #2646

merged 2 commits into from
Jun 22, 2023

Conversation

FedeDP
Copy link
Contributor

@FedeDP FedeDP commented Jun 21, 2023 •
edited by jasondellaluce
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:

Switched to use timer_settime API in stats writer, as setitimer seems to have issues when built from our CI (glibc/gcc bugs?)

Which issue(s) this PR fixes:

Fixes #2645

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

fix(userspace): switch to timer_settime API for stats writer.

@poiana poiana requested review from Kaizhe and leogr June 21, 2023 09:07
@FedeDP
Copy link
Contributor Author

FedeDP commented Jun 21, 2023

Refs #2645

@poiana poiana added size/S and removed size/XS labels Jun 21, 2023
@FedeDP
Copy link
Contributor Author

FedeDP commented Jun 21, 2023

Added some more debug prints!

@FedeDP
Copy link
Contributor Author

FedeDP commented Jun 21, 2023

It seems like the timer is ticking just twice (ie: once for the timer.it_value and once for the timer.it_interval). Digging into it.

@poiana poiana added size/M and removed size/S labels Jun 21, 2023
@FedeDP
fix(userspace): switch to timer_settime API in stats writer.
156a070 
It seems like `setitimer` is not correctly working when built from CI; perhaps a gcc/glibc bug?

Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
@FedeDP
Copy link
Contributor Author

FedeDP commented Jun 21, 2023

Why do i always git commit -sam 😠

@FedeDP
Copy link
Contributor Author

FedeDP commented Jun 21, 2023

Dropped debug prints; timer_settime API seems to be reliably working.

@FedeDP
Copy link
Contributor Author

FedeDP commented Jun 21, 2023 •
edited
Loading

/cc @incertum @jasondellaluce

To test, grab latest package produced by this PR CI, and do the same on master. You'll notice that on master, the tick is triggered just twice; on this PR it gets ticked continuously. (enable metrics with 1s timeout to quickly test)

https://github.com/falcosecurity/falco/suites/13762856851/artifacts/762497258

@FedeDP
Copy link
Contributor Author

FedeDP commented Jun 21, 2023

/kind bug

@FedeDP FedeDP changed the title chore(userspace): added some debug prints. fix(userspace): switch to timer_settime API for stats writer. Jun 21, 2023
@FedeDP
chore(userspace): cleanup old code.
c3d23b7 
Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
@poiana poiana added size/S and removed size/M labels Jun 21, 2023
@jasondellaluce
Copy link
Contributor

/milestone 0.35.1

@poiana poiana added this to the 0.35.1 milestone Jun 21, 2023
@jasondellaluce jasondellaluce mentioned this pull request Jun 21, 2023
Closed
43 tasks
@incertum
Copy link
Contributor

@FedeDP our hero !!! Fantastic job 🤩 I could replicate the issues on master and confirming this PR fixes the issues!
Well done, and thanks for planning a 0.35.1 release!

incertum
incertum approved these changes Jun 21, 2023
View reviewed changes
Copy link
Contributor

@incertum incertum left a comment

Choose a reason for hiding this comment

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

/approve

@poiana poiana added the lgtm label Jun 21, 2023
jasondellaluce
jasondellaluce approved these changes Jun 22, 2023
View reviewed changes
Copy link
Contributor

@jasondellaluce jasondellaluce left a comment

Choose a reason for hiding this comment

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

/approve

@poiana
Copy link
Contributor

poiana commented Jun 22, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FedeDP, incertum, jasondellaluce

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:
  • OWNERS [FedeDP,incertum,jasondellaluce]

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

@poiana poiana merged commit f7e15ca into master Jun 22, 2023
@poiana poiana deleted the chore/test branch June 22, 2023 08:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@incertum incertum incertum approved these changes

@jasondellaluce jasondellaluce jasondellaluce approved these changes

@Kaizhe Kaizhe Awaiting requested review from Kaizhe

@leogr leogr Awaiting requested review from leogr

Assignees

@jasondellaluce jasondellaluce

@incertum incertum

Labels
approved dco-signoff: yes kind/bug lgtm release-note size/S
Projects
Falco Roadmap
Archived in project
Milestone
0.35.1
Development

Successfully merging this pull request may close these issues.

Metrics are not created if http_output is enabled
4 participants
@FedeDP @jasondellaluce @incertum @poiana