feat(userspace): deprecate `-d` daemonize option #2677

incertum · 2023-07-08T06:25:42Z

Symptoms included the message queue filling up without popping any messages even though events were handled normally.

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind bug

/kind cleanup

/kind design

/kind documentation

/kind failing-test

/kind feature

/kind release

Any specific area of the project related to this PR?

Uncomment one (or more) /area <> lines:

/area build

/area engine

/area tests

/area proposals

/area CI

What this PR does / why we need it:

Update : feat(userspace): deprecate -d daemonize option

@falcosecurity/falco-maintainers

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

feat(userspace)!: remove `-d` daemonize option

jasondellaluce · 2023-07-10T08:27:38Z

/milestone 0.36.0

leogr

@incertum Does this PR just correct the comments?

incertum · 2023-08-02T15:49:24Z

This is not wip and rather "desperation in progress" as it's just a dummy commit somehow, I have no glue really other than it's broken no outputs when using -d option. Ideas? At this point it could be anything, some other issues or just the wrong place now to exit out ...

leogr · 2023-08-11T15:08:21Z

Questions:

Have you tried to disable stdout and use just file?
Do we really need the -d option? 🤔

incertum · 2023-08-15T23:12:10Z

Have you tried to disable stdout and use just file?

yes same outcome, that is, no outputs. My debugging journey came to the conclusion that somehow we fail to reach the code flow where we pop from the queue when using -d, something that of course makes no sense as I could confirm we got events and were putting them into the queue ...

Do we really need the -d option?

I actually synced with SREs and the answer would be no. However, we should keep the pidfile option. That was the one appealing aspect of using -d for one environment, but we already changed the deployment and don't need -d anymore anyways.

@leogr depending on how much engineering resources we have, I am of course ok with keeping it if someone finds a fix. At the same time, it would not be good to keep supporting something that is broken. In addition, removing some older features not urgently needed will be better for all of us maintainers, because we reduce maintenance overhead and make space for new features we urgently do need. Therefore, I would be ok to just remove it without a formal deprecation process since it is already broken in the current release. Again, we can keep the pidfile option without needing -d.

If above sounds like a plan I can go ahead and repurpose this PR, else I may just close it as I am not advancing with finding a fix.

leogr · 2023-08-21T09:21:41Z

Do we really need the -d option?

I actually synced with SREs and the answer would be no. However, we should keep the pidfile option. That was the one appealing aspect of using -d for one environment, but we already changed the deployment and don't need -d anymore anyways.

@leogr depending on how much engineering resources we have, I am of course ok with keeping it if someone finds a fix. At the same time, it would not be good to keep supporting something that is broken. In addition, removing some older features not urgently needed will be better for all of us maintainers, because we reduce maintenance overhead and make space for new features we urgently do need. Therefore, I would be ok to just remove it without a formal deprecation process since it is already broken in the current release. Again, we can keep the pidfile option without needing -d.

Since using systemd or Kubernetes is the default way to go, I find the -d flag somewhat redundant and, as you pointed out, potentially confusing. I actually don't see any compelling use case where that option should be used.

So, I'm in favor to remove it just because not having that option would provide a better UX, regardless of the fact that's broken. Ofc, I wouldn't fix it if we decide to remove it.

Also, I totally agree with keeping the --pidfile option without -d. Btw, it looks like we are already using --pidfile this way 👇
https://github.com/falcosecurity/falco/blob/master/scripts/systemd/falco-kmod.service

If above sounds like a plan I can go ahead and repurpose this PR, else I may just close it as I am not advancing with finding a fix.

let's see what others think cc @falcosecurity/falco-maintainers

Andreagit97 · 2023-08-22T11:27:37Z

Sorry, I'm late to the party, agree with removing this!

incertum · 2023-08-23T22:23:40Z

Removed -d flag in case we want to follow through with the deprecation. Something else missing or did I remove it properly?

leogr · 2023-08-24T09:25:41Z

Removed -d flag in case we want to follow through with the deprecation. Something else missing or did I remove it properly?

One file is conflicting. Rebase pls. 🙏

Andreagit97

All these cleanups are really appreciated, thank you!

userspace/falco/app/actions/pidfile.cpp

Andreagit97 · 2023-08-25T12:58:45Z

userspace/falco/app/actions/init_outputs.cpp

@@ -51,7 +51,7 @@ falco::app::run_result falco::app::actions::init_outputs(falco::app::state& s)

 	if (s.options.dry_run)
 	{
-		falco_logger::log(LOG_DEBUG, "Skipping daemonizing in dry-run\n");
+		falco_logger::log(LOG_DEBUG, "Skipping outputs initialization in dry-run\n");


oh cool you have already done it here, ignore my comment in the other PR i don't remember which 😹

Andreagit97

/approve

poiana · 2023-08-25T13:20:16Z

LGTM label has been added.

Git tree hash: 43f32110b12100baf0134e85f0f798f6a78a24f5

Andreagit97 · 2023-08-25T13:21:36Z

we need a rebase now 🙄

Deprecate `-d` option (currently broken). Symptoms included the message queue filling up without popping any messages even though events were handled normally. Maintainers decided to deprecate not needed `-d` option while keeping the useful `pidfile` command args option. Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

Andreagit97

/approve

poiana · 2023-08-25T15:41:16Z

LGTM label has been added.

Git tree hash: 0ee2ac67767609a87e2f7a78b5855ac8dbbc6a97

poiana · 2023-08-25T15:46:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Andreagit97, incertum, leogr

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~~ [Andreagit97,incertum,leogr]

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

poiana added release-note kind/bug dco-signoff: yes area/engine labels Jul 8, 2023

poiana requested review from Kaizhe and LucaGuerra July 8, 2023 06:25

poiana added approved size/XS labels Jul 8, 2023

poiana added this to the 0.36.0 milestone Jul 10, 2023

leogr reviewed Aug 2, 2023

View reviewed changes

incertum force-pushed the fix-daemonize branch from fd7d018 to 7308f8b Compare August 23, 2023 22:20

poiana added size/L and removed size/XS labels Aug 23, 2023

incertum changed the title ~~fix(userspace): fix broken outputs when using -d daemonize option~~ feat(userspace): deprecate -d daemonize option Aug 23, 2023

incertum force-pushed the fix-daemonize branch from 7308f8b to 27445dc Compare August 24, 2023 16:34

Andreagit97 reviewed Aug 25, 2023

View reviewed changes

Andreagit97 previously approved these changes Aug 25, 2023

View reviewed changes

poiana assigned Andreagit97 Aug 25, 2023

poiana added the lgtm label Aug 25, 2023

incertum dismissed Andreagit97’s stale review via e3031c7 August 25, 2023 15:19

incertum force-pushed the fix-daemonize branch from 1fa24f9 to e3031c7 Compare August 25, 2023 15:19

poiana removed the lgtm label Aug 25, 2023

poiana requested a review from Andreagit97 August 25, 2023 15:19

Andreagit97 approved these changes Aug 25, 2023

View reviewed changes

poiana added the lgtm label Aug 25, 2023

leogr approved these changes Aug 25, 2023

View reviewed changes

poiana assigned leogr Aug 25, 2023

poiana merged commit 37ea9b2 into falcosecurity:master Aug 25, 2023
15 checks passed

Andreagit97 mentioned this pull request Aug 28, 2023

cleanup: remove unused --pidfile option from systemd units #2742

Merged

LucaGuerra mentioned this pull request Sep 7, 2023

[TRACKING] Breaking changes in Falco 0.36.0 #2788

Closed

incertum deleted the fix-daemonize branch March 5, 2024 22:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(userspace): deprecate `-d` daemonize option #2677

feat(userspace): deprecate `-d` daemonize option #2677

incertum commented Jul 8, 2023 •

edited by LucaGuerra

Loading

jasondellaluce commented Jul 10, 2023

leogr left a comment

incertum commented Aug 2, 2023

leogr commented Aug 11, 2023

incertum commented Aug 15, 2023

leogr commented Aug 21, 2023

Andreagit97 commented Aug 22, 2023

incertum commented Aug 23, 2023

leogr commented Aug 24, 2023

Andreagit97 left a comment

Andreagit97 Aug 25, 2023

Andreagit97 left a comment

poiana commented Aug 25, 2023

Andreagit97 commented Aug 25, 2023 •

edited

Loading

Andreagit97 left a comment

poiana commented Aug 25, 2023

poiana commented Aug 25, 2023

feat(userspace): deprecate -d daemonize option #2677

feat(userspace): deprecate -d daemonize option #2677

Conversation

incertum commented Jul 8, 2023 • edited by LucaGuerra Loading

jasondellaluce commented Jul 10, 2023

leogr left a comment

Choose a reason for hiding this comment

incertum commented Aug 2, 2023

leogr commented Aug 11, 2023

incertum commented Aug 15, 2023

leogr commented Aug 21, 2023

Andreagit97 commented Aug 22, 2023

incertum commented Aug 23, 2023

leogr commented Aug 24, 2023

Andreagit97 left a comment

Choose a reason for hiding this comment

Andreagit97 Aug 25, 2023

Choose a reason for hiding this comment

Andreagit97 left a comment

Choose a reason for hiding this comment

poiana commented Aug 25, 2023

Andreagit97 commented Aug 25, 2023 • edited Loading

Andreagit97 left a comment

Choose a reason for hiding this comment

poiana commented Aug 25, 2023

poiana commented Aug 25, 2023

feat(userspace): deprecate `-d` daemonize option #2677

feat(userspace): deprecate `-d` daemonize option #2677

incertum commented Jul 8, 2023 •

edited by LucaGuerra

Loading

Andreagit97 commented Aug 25, 2023 •

edited

Loading