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

in_exec: allow for long-running commands #732

Closed
wants to merge 3 commits into from
Closed

in_exec: allow for long-running commands #732

wants to merge 3 commits into from

Conversation

j0057
Copy link

@j0057 j0057 commented Aug 31, 2018

Add a boolean option (follow) to in_exec plugin to read the spawned
process output using non-blocking I/O, this way Fluent-bit can capture
the output of commands that never exit, for example vmstat -S M 10.

Signed-off-by: Joost Molenaar [email protected]

@edsiper
Copy link
Member

edsiper commented Sep 7, 2018

cc: @nokute78

follow = flb_input_get_property("follow", in);
if (follow != NULL) {
if (strcasecmp(follow, "true") == 0) {
exec_config->follow = 1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We defined FLB_TRUE/FLB_FALSE at flb_macros.h. Would you replace it?

Copy link
Author

Choose a reason for hiding this comment

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

I updated the code to make use of the FLB_TRUE/FLB_FALSE macros.

exec_config->argv[argc] = NULL;

/* follow setting */
follow = flb_input_get_property("follow", in);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Refer to in_exec of fluentd, run_interval option means follow mode or not.
https://docs.fluentd.org/v1.0/articles/in_exec#run_interval

I means,
I think we don't need new option "follow".

If Interval_Sec and Interval_NSec are not set, exec_config->follow is FLB_TRUE. (= run_interval is not set)
if Interval_Sec or/and Interval_NSec is/are set, exec_config->follow is FLB_FALSE. (= run_interval is set)
Default value of Interval_Sec and Interval_NSec are 0.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Note: We support fd event based API flb_input_set_collector_event. (e.g. in_kmsg uses this API)

I think the follow mode is not time based mode. How about trying to use it?

flb_input_set_collector_event(in,
                                           in_exec_collect,
                                           fileno(exec_config->cmdp),
                                           config);

@nokute78
Copy link
Collaborator

nokute78 commented Sep 9, 2018

@j0057 I commented on your commit.

Defunct process will be generated if user set such configuraion.
(Follow mode and command which exits immediately)
Do you have any idea? ( exit after execvp?)

[INPUT]
    Name          exec
    Tag           exec_ls
    Command       ls  /var/log
    follow true

[OUTPUT]
    Name   stdout
    Match  *

@edsiper
Copy link
Member

edsiper commented Sep 26, 2018

@j0057 ping

j0057 added 3 commits October 3, 2018 10:03
Add a boolean option (follow) to in_exec plugin to read the spawned
process output using non-blocking I/O, this way Fluent-bit can capture
the output of commands that never exit, for example `vmstat -S M 10`.

Signed-off-by: Joost Molenaar <[email protected]>
Signed-off-by: Joost Molenaar <[email protected]>
@edsiper
Copy link
Member

edsiper commented Nov 21, 2018

@nokute78 is this ready to go after the latest changes or it needs some changes ?

@edsiper edsiper added the waiting-for-user Waiting for more information, tests or requested changes label Nov 21, 2018
@edsiper
Copy link
Member

edsiper commented Nov 23, 2018

@nokute78 ping

@nokute78
Copy link
Collaborator

nokute78 commented Nov 26, 2018

@edsiper Sorry for late reply.

I proposed 3 items and 1 of it was fixed.

  • using FLB_TRUE/FLB_FALSE
  • executing once if Interval_Sec and Interval_NSec are not set.
  • using event based API for long-running command instead of time based API.

@edsiper
Copy link
Member

edsiper commented Nov 26, 2018

thanks @nokute78

@j0057 please advice if you want to include the improvement with the changes suggested.

@j0057
Copy link
Author

j0057 commented Jan 2, 2019

Hi @edsiper, life is currently not allowing me to invest the time needed to see this pull request through to its completion. I think rewriting it to use the event-based API would cost me at least a few days to learn about the concepts involved -- the PR as it is took me about 8 hours, because I'm not very fluent in C. I'm sorry for wasting your time and not closing the PR sooner.

@nokute78, thank you for reviewing my code, my apologies to you as well.

@edsiper
Copy link
Member

edsiper commented Jan 2, 2019

Thanks anyways for the effort on this, no worries, it's just code :) no waste of time, have a great 2019!

@nokute78 thank you for reviewing and help on this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-for-user Waiting for more information, tests or requested changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants