msi: guard duplicated instance #622
Conversation
|
This is PoC |
kenhys
force-pushed
the
guard-winsvc
branch
5 times, most recently
from
a8a63a5
to
55c7a03
Compare
|
Basically it will work, but there is one concern. If user execute directly c:\opt\fluent\bin\fluentd, it can not be blocked. NOTE: search path is like this: |
|
It should use findstr /I |
kenhys
force-pushed
the
guard-winsvc
branch
2 times, most recently
from
2813851
to
9a3369d
Compare
|
Verified when fluentdwinsvc is running, it will abort like this: |
|
Hmm, it seems that fluentd.bat exit expectedly, but not capture $LASTEXITCODE correctly. |
|
With locally built msi, it work as expected. |
|
It's a problem of test code. |
|
Test code was fixed, waiting CI. |
|
All checks has passed. |
There was a problem hiding this comment.
Thanks for this fix.
My first impression is that it should be a simpler change as a patch version update.
I have concerns about the following points.
- Specification change
- If this is to be included in v5.0.3, the specifications should not be changed as much as possible.
- This PR disables Fluentd from starting if certain conditions are met. This change looks to me too large for a patch version update.
- Maintainability
- I am concerned about the maintainability of parsing arguments on this startup script.
For example, what about the following directions?
- Add a confirmation step when the state of the service is
Running.- In addition, add
forceoption so that the service can be launched without confirmation even if the state isRunning, as the current specification.
- In addition, add
|
I've forgot to eliminate |
|
|
| if "%%p"=="--force" ( | ||
| set /a FLUENT_FORCE_RUN=1 | ||
| ) else ( | ||
| set "FLUENT_ARGS=!FLUENT_ARGS! %%p" | ||
| ) |
There was a problem hiding this comment.
I've forgot to eliminate --force from command line arguments.
It was fixed now.
I see...
I'm sorry for not considering this point.
Is this for the possibility of future option conflicts?
This looks like a very difficult issue to me...
Not passing the option to Fluentd does not solve the conflict completely.
(We can not use the option on the Fluentd side in the future.)
Once an argument is added, it is difficult to change it later, so we should be very careful about it.
We should consider this point carefully after releasing 5.0.3.
There was a problem hiding this comment.
There is a possibility to conflict with fluentd side. (It is not planned yet)
When adding "dare to execute something" option, longer option name is better in conventionally.
There was a problem hiding this comment.
--force-running-fluentd or ...
What is the suitable name? 🤔
Before: Even though fluentdwinsvc is running, you can execute duplicated Fluentd instance. It may cause inconsistency of buffer or pos file. After: Guard multiple Fluentd instance when fluendwinsvc service is running by default. Note that --force-running-multiple-instance option is specified, you can run additional Fluentd instance explicitly. (it is same as previous behavior) Signed-off-by: Kentaro Hayashi <hayashi@clear-code.com>
%FLUENT_PACKAGE_TOPDIR% contains trailing /, so it is redundant to add /. Signed-off-by: Kentaro Hayashi <hayashi@clear-code.com>
msi: guard multiple Fluentd instance
Before:
After: