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

refactor: svc.Run() as main routine blocker #1372

Closed

Conversation

DiniFarb
Copy link
Contributor

@DiniFarb DiniFarb commented Jan 6, 2024

Regarding Issue #1352

I have made some tests with svc.Run() in the main Go routine here: DiniFarb@16888c2#diff-88d237fa0e6dd92a26d0dd0a14ad18246fb0924b68520062e9c9150e2004d7fe
With that change, I could no longer reproduce the stop error.
In that commit, the Windows service interface is initialized after the whole windows_exporter has already started. But the interface should actually be initialized as early as possible, so that the svc.Status{State: svc.StartPending} can be sent back before most of the service is loaded. Thus, I have refactored the startup process while I tried the following order:

  1. Initialize Kingpin flags.
  2. Load args/config file.
  3. Initialize logger.
  4. Initialize Windows service (if not CLI).
  5. Build collectors.
  6. Build HTTP server and start.

I know that this is not a small change, so I am very open to changing things to keep a better understanding of the startup process for everyone. :)

@DiniFarb DiniFarb requested a review from a team as a code owner January 6, 2024 19:39
@DiniFarb DiniFarb changed the title refactor: scv.Run() as main routine blocker refactor: svc.Run() as main routine blocker Jan 6, 2024
@jkroepke
Copy link
Member

jkroepke commented Jan 6, 2024

I have the feeling that it reverts #1047 and re-introduce #551


I guess a better solution would be a dedicate service binary which holds the communication between Windows Service Host and windows_exporter.

The Grafana Agent is using that (grafana/agent#3243) and it worked very well.

@DiniFarb
Copy link
Contributor Author

DiniFarb commented Jan 7, 2024

Yes and in #1047 was also mentioned that this fix could affect the stop process (#1047 (comment))

All in all it is not so easy to keep that in a good way. The solution from grafana/agant looks promising though. But this would be an even bigger change then I did.

I just realized how this stop issue can be fixed even easier without refactoring the main start process. With just moving the stop signal after the svc.Run() it is assured that the window interface can handle the stop process and then send the final stop to the main routine. Here would be the according changes: 9921ba3

For now I could close this PR and add a new one with the small fix in 9921ba3.
But yeah I agree, in the end it would be better to split it up like it is done in the grafana agent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants