-
Notifications
You must be signed in to change notification settings - Fork 6.2k
Improve installer (Windows) #21090
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
Improve installer (Windows) #21090
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Improves the Windows installer's service management by fixing issues where the Netdata service failed to stop properly during installation and provided no feedback about failures.
- Replaced unreliable
net stopcommand with process termination usingtaskkill.exe - Added service recovery configuration to automatically restart on failures
- Refactored service control to use separate install/uninstall actions with proper timing
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| ResetPeriodInDays="0" | ||
| FirstFailureActionType="restart" | ||
| SecondFailureActionType="restart" | ||
| ThirdFailureActionType="restart" |
Copilot
AI
Oct 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The service recovery configuration has aggressive restart behavior with no reset period. Consider adding a reasonable reset period (e.g., 1 day) and escalating failure actions (e.g., restart -> restart -> none) to prevent infinite restart loops if the service consistently fails.
| ResetPeriodInDays="0" | |
| FirstFailureActionType="restart" | |
| SecondFailureActionType="restart" | |
| ThirdFailureActionType="restart" | |
| ResetPeriodInDays="1" | |
| FirstFailureActionType="restart" | |
| SecondFailureActionType="restart" | |
| ThirdFailureActionType="none" |
Copilot uses AI. Check for mistakes.
| /> | ||
| </ServiceInstall> | ||
|
|
||
| <ServiceControl Id="StartService" Name="Netdata" Start="install" Wait="no" /> |
Copilot
AI
Oct 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting Wait="no" for service start could lead to race conditions where subsequent installation steps might execute before the service is fully started. Consider using Wait="yes" to ensure proper sequencing.
| <ServiceControl Id="StartService" Name="Netdata" Start="install" Wait="no" /> | |
| <ServiceControl Id="StartService" Name="Netdata" Start="install" Wait="yes" /> |
Copilot uses AI. Check for mistakes.
(cherry picked from commit 1d9ca4d)
(cherry picked from commit 1d9ca4d)
Summary
The Netdata installer failed to stop the Netdata service during installation. It also did not raise errors or provide feedback about this failure.
This PR addresses these issues by modifying the service management mechanism.
Test Plan
ServiceConfigfunctions correctly.Additional Information
For users: How does this change affect me?