msi: guard duplicated instance #622

kenhys · 2024-02-19T09:31:33Z

msi: guard multiple Fluentd instance

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 option is specified, you can run additional
  Fluentd instance explicitly. (it is same as previous behavior)

kenhys · 2024-02-19T09:35:58Z

This is PoC

kenhys · 2024-02-20T05:29:19Z

Basically it will work, but there is one concern.

If user execute directly c:\opt\fluent\bin\fluentd, it can not be blocked.
Usually fluent package prompt will be used, so it is a rare case.

NOTE: search path is like this:

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

kenhys · 2024-02-20T07:31:17Z

It should use findstr /I

kenhys · 2024-02-20T07:41:37Z

Verified when fluentdwinsvc is running, it will abort like this:

c:\opt\fluent>fluentd.bat -c c:\opt\fluent\etc\fluent\fluentd.conf
Error: can't start duplicate Fluentd instance with same "c:\opt\fluent\etc\fluent\fluentd.conf"

c:\opt\fluent>fluentd.bat
Error: can't start duplicate Fluentd instance with same c:/opt/fluent/etc/fluent/fluentd.conf

kenhys · 2024-02-20T08:36:04Z

Hmm, it seems that fluentd.bat exit expectedly, but not capture $LASTEXITCODE correctly.

2024-02-20T08:26:44.9840365Z DEBUG:   55+  >>>> Start-Process "C:\\opt\\fluent\\fluentd.bat" -ArgumentList �[m  �[93m
2024-02-20T08:26:44.9842223Z "-c", "C:\\opt\\fluent\\etc\\fluent\\fluentd.conf" -Wait -NoNewWindow�[11X�[11C�[m
2024-02-20T08:26:44.9843565Z �[80X�[80C
2024-02-20T08:26:45.0579514Z �[?25h�[?25lError: can't start duplicate Fluentd instance with same C:/opt/fluent/etc/fluent��[?25h�[?25l
2024-02-20T08:26:45.0580328Z /fluentd.conf�[67X�[67C
2024-02-20T08:26:45.0580632Z �[?25h�[?25l
2024-02-20T08:26:45.0580880Z �[80X�[80C
2024-02-20T08:26:45.9898406Z �[?25h�[93m�[H1�[25;1H�[?25lDEBUG:   56+  >>>> $exitcode = $LASTEXITCODE�[m 
2024-02-20T08:26:45.9929166Z �[?25h�[?25l�[93m�[H1�[25;1H�[?25h�[?25l�[m
2024-02-20T08:26:45.9929643Z �[93mDEBUG:   57+ if ( >>>> $exitcode -ne 2) {�[m�[39X�[39C
2024-02-20T08:26:45.9952814Z �[?25h�[?25l�[93m
2024-02-20T08:26:45.9953365Z DEBUG:   58+      >>>> Write-Host "Failed to abort when it is conflict with �[m    
2024-02-20T08:26:45.9955671Z �[?25h�[?25l
2024-02-20T08:26:45.9956365Z �[93mrunning fluentdwinsvc"�[58X�[58C
2024-02-20T08:26:45.9986556Z �[?25h�[?25l�[m
2024-02-20T08:26:45.9995251Z Failed to abort when it is conflict with running fluentdwinsvc�[18X�[18C�[25;63H�[?25h�[?25l
2024-02-20T08:26:45.9996149Z �[93mDEBUG:   59+      >>>> [Environment]::Exit(1)�[m�[35X�[35C
2024-02-20T08:26:46.0051281Z �[?25h�[?25l
2024-02-20T08:26:46.0051698Z �[80X�[80C
2024-02-20T08:26:50.0433064Z �[?25h�[93m�[HD�[25;1H
2024-02-20T08:26:50.0555867Z ##[error]Process completed with exit code 1.
2024-02-20T08:26:50.0788284Z Post job cleanup.

kenhys · 2024-02-21T01:28:37Z

With locally built msi, it work as expected.

PS C:\opt\fluent> .\fluentd.bat
Error: can't start duplicate Fluentd instance with same C:/opt/fluent/etc/fluent/fluentd.conf
PS C:\opt\fluent> $exitcode = $LASTEXITCODE
PS C:\opt\fluent> $exitcode
2
PS C:\opt\fluent>

kenhys · 2024-02-21T01:47:06Z

It's a problem of test code.
It should use -PassThru and capture with .ExitCode.

kenhys · 2024-02-21T01:49:37Z

Test code was fixed, waiting CI.

kenhys · 2024-02-21T03:09:30Z

All checks has passed.

daipom

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 force option so that the service can be launched without confirmation even if the state is Running, as the current specification.

kenhys · 2024-03-01T03:27:09Z

I've forgot to eliminate --force from command line arguments.
It was fixed now.

kenhys · 2024-03-01T03:30:13Z

daipom · 2024-03-01T08:59:19Z

fluent-package/msi/assets/fluentd.bat

+    if "%%p"=="--force" (
+        set /a FLUENT_FORCE_RUN=1
+    ) else (
+        set "FLUENT_ARGS=!FLUENT_ARGS! %%p"
+    )


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 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.

--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 <[email protected]>

%FLUENT_PACKAGE_TOPDIR% contains trailing /, so it is redundant to add /. Signed-off-by: Kentaro Hayashi <[email protected]>

@kenhys

Before: We can launch Fluentd by fluentd.bat even though fluentdwinsvc is running. Launching multiple Fluentd with the same config may cause inconsistency of the buffers or the pos files. After: We can't launch Fluentd by fluent.bat with the default config path if fluentdwinsvc is running. If `--config` (`-c`) or `--dry-run` are specified, we can execute fluentd.bat as before. Inspired by @kenhys's PR fluent#622. Signed-off-by: Daijiro Fukuda <[email protected]> Co-authored-by: Kentaro Hayashi <[email protected]>

daipom

Thanks for this, and I'm sorry for the delay.
I have made another version #648, based on this fix.

This PR adds an option to force execution for compatibility.
On the other hand, #648 checks -c (--config) option existence to decide whether to prevent launching for compatibility.
(There are other conditions, but they are not notable. dry-run should be considered as well in this PR direction.)

I think both of these directions are possible.

This PR is more strictly preventing launching multiple instances.

#648 doesn't need a new option.
However, we can launch Fluentd with -c (--config).
So, a possibility of unintentional duplicate launching remains.
(I believe it is less important to consider such a case, though).

Let's consider how to solve #611 based on the information so far.

@kenhys

Before: We can launch Fluentd by fluentd.bat even though fluentdwinsvc is running. Launching multiple Fluentd with the same config may cause inconsistency of the buffers or the pos files. After: We can't launch Fluentd by fluent.bat with the default config path if fluentdwinsvc is running. If `--config` (`-c`) or `--dry-run` are specified, we can execute fluentd.bat as before. Inspired by @kenhys's PR fluent#622. Signed-off-by: Daijiro Fukuda <[email protected]> Co-authored-by: Kentaro Hayashi <[email protected]>

@kenhys

Before: We can launch Fluentd by fluentd.bat even though fluentdwinsvc is running. Launching multiple Fluentd with the same config may cause inconsistency of the buffers or the pos files. After: We can't launch Fluentd by fluent.bat with the default config path if fluentdwinsvc is running. If some options are specified, we can execute fluentd.bat as before. Inspired by @kenhys's PR fluent#622. Signed-off-by: Daijiro Fukuda <[email protected]> Co-authored-by: Kentaro Hayashi <[email protected]>

kenhys · 2024-06-27T01:55:39Z

#647 and #648 approaches are more better alternatives.

@kenhys

Before: We can launch Fluentd by fluentd.bat even though fluentdwinsvc is running. Launching multiple Fluentd with the same config may cause inconsistency of the buffers or the pos files. After: We can't launch Fluentd by fluent.bat with the default config path if fluentdwinsvc is running. If some options are specified, we can execute fluentd.bat as before. Inspired by @kenhys's PR fluent#622. Signed-off-by: Daijiro Fukuda <[email protected]> Co-authored-by: Kentaro Hayashi <[email protected]>

daipom · 2024-06-27T02:21:00Z

Thanks for considering.
If we need a force execution option or a strict check whether the config path is the same in the future, let's revive this!

Note: strict check: https://github.com/fluent/fluent-package-builder/blob/f045a795e8f02c9731262f29a6832d95b40c85a1/fluent-package/msi/assets/fluentd.bat

@kenhys

Before: We can launch Fluentd by fluentd.bat even though fluentdwinsvc is running. Launching multiple Fluentd with the same config may cause inconsistency of the buffers or the pos files. After: We can't launch Fluentd by fluent.bat with the default config path if fluentdwinsvc is running. If some options are specified, we can execute fluentd.bat as before. Inspired by @kenhys's PR fluent#622. Signed-off-by: Daijiro Fukuda <[email protected]> Co-authored-by: Kentaro Hayashi <[email protected]>

@kenhys

Before: We can launch Fluentd by fluentd.bat even though fluentdwinsvc is running. Launching multiple Fluentd with the same config may cause inconsistency of the buffers or the pos files. After: We can't launch Fluentd by fluent.bat with the default config path if fluentdwinsvc is running. If some options are specified, we can execute fluentd.bat as before. Inspired by @kenhys's PR fluent#622. Signed-off-by: Daijiro Fukuda <[email protected]> Co-authored-by: Kentaro Hayashi <[email protected]>

@kenhys

Before: We can launch Fluentd by fluentd.bat even though fluentdwinsvc is running. Launching multiple Fluentd with the same config may cause inconsistency of the buffers or the pos files. After: We can't launch Fluentd by fluent.bat with the default config path if fluentdwinsvc is running. If some options are specified, we can execute fluentd.bat as before. Inspired by @kenhys's PR #622. Signed-off-by: Daijiro Fukuda <[email protected]> Co-authored-by: Kentaro Hayashi <[email protected]>

kenhys force-pushed the guard-winsvc branch 5 times, most recently from a8a63a5 to 55c7a03 Compare February 20, 2024 05:26

kenhys added this to the 5.0.3 milestone Feb 20, 2024

kenhys added the Windows label Feb 20, 2024

kenhys marked this pull request as ready for review February 20, 2024 05:45

kenhys force-pushed the guard-winsvc branch 2 times, most recently from 2813851 to 9a3369d Compare February 20, 2024 07:39

kenhys force-pushed the guard-winsvc branch from 9a3369d to f045a79 Compare February 21, 2024 01:46

kenhys requested a review from daipom February 21, 2024 03:09

daipom linked an issue Feb 21, 2024 that may be closed by this pull request

Prevent accidental duplicate launching #611

Closed

kenhys mentioned this pull request Feb 22, 2024

Prevent accidental duplicate launching #611

Closed

daipom added the enhancement New feature or request label Feb 29, 2024

daipom reviewed Feb 29, 2024

View reviewed changes

kenhys force-pushed the guard-winsvc branch from f045a79 to b57d42d Compare February 29, 2024 09:21

kenhys modified the milestones: 5.0.3, 5.0.4 (T.B.D.) Mar 1, 2024

kenhys force-pushed the guard-winsvc branch from b57d42d to f3b7715 Compare March 1, 2024 03:25

daipom reviewed Mar 1, 2024

View reviewed changes

kenhys force-pushed the guard-winsvc branch from f3b7715 to 76b620b Compare March 6, 2024 06:43

kenhys force-pushed the guard-winsvc branch from 76b620b to b560c1c Compare April 4, 2024 07:01

kenhys force-pushed the guard-winsvc branch 2 times, most recently from d54f6e9 to 7c90cde Compare June 24, 2024 05:30

kenhys added 2 commits June 25, 2024 11:04

msi: remove extra trailing /

230c25a

%FLUENT_PACKAGE_TOPDIR% contains trailing /, so it is redundant to add /. Signed-off-by: Kentaro Hayashi <[email protected]>

kenhys force-pushed the guard-winsvc branch from 7c90cde to 230c25a Compare June 25, 2024 02:04

daipom mentioned this pull request Jun 25, 2024

msi: guard duplicated instance (not adding a new option) #648

Merged

daipom reviewed Jun 25, 2024

View reviewed changes

kenhys closed this Jun 27, 2024

kenhys deleted the guard-winsvc branch July 23, 2024 01:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

msi: guard duplicated instance #622

msi: guard duplicated instance #622

kenhys commented Feb 19, 2024 •

edited

Loading

kenhys commented Feb 19, 2024

kenhys commented Feb 20, 2024 •

edited

Loading

kenhys commented Feb 20, 2024

kenhys commented Feb 20, 2024

kenhys commented Feb 20, 2024

kenhys commented Feb 21, 2024

kenhys commented Feb 21, 2024 •

edited

Loading

kenhys commented Feb 21, 2024

kenhys commented Feb 21, 2024

daipom left a comment

kenhys commented Mar 1, 2024

kenhys commented Mar 1, 2024

daipom Mar 1, 2024

kenhys Mar 1, 2024 •

edited

Loading

kenhys Mar 1, 2024

daipom left a comment

kenhys commented Jun 27, 2024

daipom commented Jun 27, 2024 •

edited

Loading

msi: guard duplicated instance #622

msi: guard duplicated instance #622

Conversation

kenhys commented Feb 19, 2024 • edited Loading

kenhys commented Feb 19, 2024

kenhys commented Feb 20, 2024 • edited Loading

kenhys commented Feb 20, 2024

kenhys commented Feb 20, 2024

kenhys commented Feb 20, 2024

kenhys commented Feb 21, 2024

kenhys commented Feb 21, 2024 • edited Loading

kenhys commented Feb 21, 2024

kenhys commented Feb 21, 2024

daipom left a comment

Choose a reason for hiding this comment

kenhys commented Mar 1, 2024

kenhys commented Mar 1, 2024

daipom Mar 1, 2024

Choose a reason for hiding this comment

kenhys Mar 1, 2024 • edited Loading

Choose a reason for hiding this comment

kenhys Mar 1, 2024

Choose a reason for hiding this comment

daipom left a comment

Choose a reason for hiding this comment

kenhys commented Jun 27, 2024

daipom commented Jun 27, 2024 • edited Loading

kenhys commented Feb 19, 2024 •

edited

Loading

kenhys commented Feb 20, 2024 •

edited

Loading

kenhys commented Feb 21, 2024 •

edited

Loading

kenhys Mar 1, 2024 •

edited

Loading

daipom commented Jun 27, 2024 •

edited

Loading