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

Prevent accidental duplicate launching #611

Open
daipom opened this issue Jan 24, 2024 · 12 comments · May be fixed by #622 or #617
Open

Prevent accidental duplicate launching #611

daipom opened this issue Jan 24, 2024 · 12 comments · May be fixed by #622 or #617
Labels
enhancement New feature or request

Comments

@daipom
Copy link
Contributor

daipom commented Jan 24, 2024

fluentd or td-agent command launches Fluentd with the default config path.
This makes it easy to start Fluentd, but it also causes accidental duplicate launching.

For example, it launches Fluentd duplicated while Fluentd is running as a systemd unit.

Processing the same files, such as buffer files, pos files, and output destination files, causes unassumed errors.
At worst, it may cause file corruption or data loss.

The current specification makes it too easy to start Fluentd, so I'm concerned about mishandling.
For example, trying to check the version and launching Fluentd by mistake, etc.

The current specification

The default config path is set in env variables.

ENV["FLUENT_CONF"]="/etc/<%= package_dir %>/<%= service_name %>.conf"

So, we can start Fluentd without any command arguments.

fluent-package:

$ fluentd

td-agent:

$ td-agent

What I want to do

I want to add some prevention.
For example,

  • Add confirmation that you are really trying to start Fluentd

It would change the specifications, so we need to be careful about compatibility.

@kenhys
Copy link
Contributor

kenhys commented Feb 5, 2024

It may be better to implement such a guard feature in fluentd not fluent-package-builder.

@kenhys
Copy link
Contributor

kenhys commented Feb 6, 2024

https://github.com/fluent/fluentd/blob/master/lib/fluent/command/fluentd.rb#L343
It is enough to guard before starting service appropriately. 🤔

@daipom
Copy link
Contributor Author

daipom commented Feb 6, 2024

It may be better to implement such a guard feature in fluentd not fluent-package-builder.

I too think it may end up that way.
However, since the specification of easy duplicate launching is only a problem of the package, it is necessary to consider how to take appropriate countermeasures on either side.

@kenhys
Copy link
Contributor

kenhys commented Feb 8, 2024

Use case on Windows:

  • Already started fluentd as a service (fluentd)
  • Launch Fluent Package Prompt with Administrator privilege
  • Execute another instance via fluentd -v

If fluentdopt is not changed, usually c:/opt/fluent/etc/fluent/fluentd.conf will be used in both cases.
Thus, blocking from c:/opt/fluent/fluentd.bat (msi/assets/fluentd.bat) is effective if it will be fixed in fluent-package side.

c:\opt\fluent>where fluentd
c:\opt\fluent\fluentd.bat
c:\opt\fluent\bin\fluentd
c:\opt\fluent\bin\fluentd.bat

Use case in Linux:

  • Already started fluentd as a service (fluentdwinsvc)
  • Execute another instance via fluentd -v

If fluentd.service is not changed, usually /etc/fluent/fluentd.conf (FLUENT_CONF) will be used.
The default path of fluentd itself is /etc/fluent/fluent.conf, Thus same configuration
is not used by default.

If configuration path is specified explicitly with -C /etc/fluent/fluentd.conf, need to
guard it.

/opt/fluent/bin/fluentd is autogenerated file, so lib/fluent/comman/fluentd may be better to block it.

NOTE: both of cases, finally call lib/fluent/comand/fluentd (/opt/fluent/bin/fluentd => gems.../bin/fluentd => gems../lib/fluent/command/fluentd)

@kenhys
Copy link
Contributor

kenhys commented Feb 9, 2024

PoC PR for this issue.

fluent/fluentd#4399

Not stable yet because the guard condition fires unexpectedly.

@kenhys
Copy link
Contributor

kenhys commented Feb 15, 2024

Reconsider, it is not appropriate to introduce package specific fix to fluentd internals. 🤔

@kenhys
Copy link
Contributor

kenhys commented Feb 15, 2024

  • Linux: guard in template
    fluent-package/templates/usr/sbin/fluentd.erb
  • Windows: guard in assets
    fluent-package/msi/assets/fluentd.bat

@daipom daipom linked a pull request Feb 21, 2024 that will close this issue
@daipom
Copy link
Contributor Author

daipom commented Feb 21, 2024

Thanks!

Use case in Linux:
...
If fluentd.service is not changed, usually /etc/fluent/fluentd.conf (FLUENT_CONF) will be used. The default path of fluentd itself is /etc/fluent/fluent.conf, Thus same configuration is not used by default.

The default path of fluentd itself is /etc/fluent/fluent.conf

This?

https://github.com/fluent/fluentd/blob/2b4ca5d2927b706c3bdc98ffd0a0b66232bc0b65/lib/fluent/env.rb#L21

DEFAULT_CONFIG_PATH = ENV['FLUENT_CONF'] || '/etc/fluent/fluent.conf'

However, /usr/sbin/fluentd sets the FLUENT_CONF environmental var.
So, the same configuration is used by default.

ENV["FLUENT_CONF"]="/etc/<%= package_dir %>/<%= service_name %>.conf"

@daipom
Copy link
Contributor Author

daipom commented Feb 21, 2024

@kenhys
Copy link
Contributor

kenhys commented Feb 22, 2024

However, /usr/sbin/fluentd sets the FLUENT_CONF environmental var.
So, the same configuration is used by default.

Yes, so fixed via #622.

@kenhys
Copy link
Contributor

kenhys commented Feb 29, 2024

both of PR was ready for review.

#617
#622

@kenhys kenhys added this to the 5.0.3 milestone Feb 29, 2024
@daipom
Copy link
Contributor Author

daipom commented Feb 29, 2024

Thanks! Sorry for keeping you waiting. I will see it tomorrow or this weekend.

@kenhys kenhys modified the milestones: 5.0.3, 5.0.4 (T.B.D.) Mar 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment