Improve agent starting and simultaneously restarting (common for Agent install) concurrency handling #25282
Conversation
…dition When datadogagent service starts it will start in the end dependent services, including datadog-process-agent and datadog-trace-agent. If at the same time the datadogagent service is restarted by "agent.exe restart-service" command, there are a number of edge cases with dependent services concurrent start and stop which need to be handled explicitly. This PR makes a small change which addresses some of the edge cases (full fix will follow). Specifically, it addresses a case when a dependent service is started soon after restart command starts stopping dependent services. Previously, stopping command collects only running dependent services, this fix changes code to collect all dependent services (because stopped service now may be running after collection finished).
…restarting concurrency
…gent-restart-service
iglendd
changed the title
iglendd
changed the title
|
[Fast Unit Tests Report] On pipeline 33584671 (CI Visibility). The following jobs did not run any unit tests: Jobs:
If you modified Go files and expected unit tests to run in these jobs, please double check the job logs. If you think tests should have been executed reach out to #agent-developer-experience |
Test changes on VMUse this command from test-infra-definitions to manually test this PR changes on a VM: inv create-vm --pipeline-id=33584671 --os-family=ubuntu |
drichards-87
left a comment
drichards-87
left a comment
There was a problem hiding this comment.
Left you a minor suggestion from Docs and approved the PR.
…420923b9ea275.yaml Co-authored-by: DeForest Richards <56796055+drichards-87@users.noreply.github.com>
|
/merge |
|
🚂 MergeQueue This merge request is not mergeable yet, because of pending checks/missing approvals. It will be added to the queue as soon as checks pass and/or get approvals. Use |
Regression DetectorRegression Detector ResultsRun ID: 380f03ad-b66c-40f3-82eb-466b82bbf438 Performance changes are noted in the perf column of each table:
No significant changes in experiment optimization goalsConfidence level: 90.00% There were no significant changes in experiment optimization goals at this confidence level and effect size tolerance.
|
| perf | experiment | goal | Δ mean % | Δ mean % CI |
|---|---|---|---|---|
| ❌ | file_to_blackhole | % cpu utilization | +30.35 | [+25.44, +35.26] |
Fine details of change detection per experiment
| perf | experiment | goal | Δ mean % | Δ mean % CI |
|---|---|---|---|---|
| ❌ | file_to_blackhole | % cpu utilization | +30.35 | [+25.44, +35.26] |
| ➖ | tcp_syslog_to_blackhole | ingress throughput | +4.80 | [-16.97, +26.57] |
| ➖ | uds_dogstatsd_to_api_cpu | % cpu utilization | +1.78 | [-1.07, +4.63] |
| ➖ | process_agent_standard_check_with_stats | memory utilization | +0.39 | [+0.34, +0.44] |
| ➖ | process_agent_standard_check | memory utilization | +0.37 | [+0.32, +0.43] |
| ➖ | pycheck_1000_100byte_tags | % cpu utilization | +0.18 | [-4.43, +4.78] |
| ➖ | otel_to_otel_logs | ingress throughput | +0.10 | [-0.26, +0.45] |
| ➖ | idle | memory utilization | +0.05 | [+0.01, +0.09] |
| ➖ | tcp_dd_logs_filter_exclude | ingress throughput | +0.04 | [+0.01, +0.07] |
| ➖ | trace_agent_json | ingress throughput | +0.00 | [-0.01, +0.01] |
| ➖ | trace_agent_msgpack | ingress throughput | -0.00 | [-0.01, +0.01] |
| ➖ | uds_dogstatsd_to_api | ingress throughput | -0.01 | [-0.22, +0.19] |
| ➖ | process_agent_real_time_mode | memory utilization | -0.27 | [-0.32, -0.22] |
| ➖ | file_tree | memory utilization | -0.37 | [-0.46, -0.28] |
| ➖ | basic_py_check | % cpu utilization | -0.53 | [-2.93, +1.88] |
Explanation
A regression test is an A/B test of target performance in a repeatable rig, where "performance" is measured as "comparison variant minus baseline variant" for an optimization goal (e.g., ingress throughput). Due to intrinsic variability in measuring that goal, we can only estimate its mean value for each experiment; we report uncertainty in that value as a 90.00% confidence interval denoted "Δ mean % CI".
For each experiment, we decide whether a change in performance is a "regression" -- a change worth investigating further -- if all of the following criteria are true:
-
Its estimated |Δ mean %| ≥ 5.00%, indicating the change is big enough to merit a closer look.
-
Its 90.00% confidence interval "Δ mean % CI" does not contain zero, indicating that if our statistical model is accurate, there is at least a 90.00% chance there is a difference in performance between baseline and comparison variants.
-
Its configuration does not mark it "erratic".
|
This merge request was unqueued If you need support, contact us on Slack #devflow! |
|
/merge |
|
🚂 MergeQueue Pull request added to the queue. There are 5 builds ahead! (estimated merge in less than 3h) Use |
It is the second and final part of the solution based on the first part partial solution (#25282). This PR is based on the same testing framework and its major contribution is detecting various parts of the race condition mostly based on the fact that ControlService with SERVICE_CONTROL_STOP code would fail if any dependent service is running. Accordingly, second or more iterations to stop dependent services will be required, unless total timeout is exceeded. This will make Agent restart-service robust enough to handle race conditions when the Agent is just starting, but starting concurrently to the Agent restarting via Agent CLI command. This will not help Powershell restart-service cmdlet which would have non insignificant chances to fail if by the time core agent service is about to stop one of its dependent services is concurrently started just before attempting to stop main service. In addition, a modest refactoring had been done to remove duplicate calls and reuse more recent Go runtime svc.ListDependentServices() function instead of handcrafted version.
…25453) * WINA-747 Complete solution to solve startup/shutdown race con…dition It is the second and final part of the solution based on the first part partial solution (#25282). This PR is based on the same testing framework and its major contribution is detecting various parts of the race condition mostly based on the fact that ControlService with SERVICE_CONTROL_STOP code would fail if any dependent service is running. Accordingly, second or more iterations to stop dependent services will be required, unless total timeout is exceeded. This will make Agent restart-service robust enough to handle race conditions when the Agent is just starting, but starting concurrently to the Agent restarting via Agent CLI command. This will not help Powershell restart-service cmdlet which would have non insignificant chances to fail if by the time core agent service is about to stop one of its dependent services is concurrently started just before attempting to stop main service. In addition, a modest refactoring had been done to remove duplicate calls and reuse more recent Go runtime svc.ListDependentServices() function instead of handcrafted version.
…t install) concurrency handling (#25282) * WINA-747 Partially addressing agent service startup/shutdown race condition When datadogagent service starts it will start in the end dependent services, including datadog-process-agent and datadog-trace-agent. If at the same time the datadogagent service is restarted by "agent.exe restart-service" command, there are a number of edge cases with dependent services concurrent start and stop which need to be handled explicitly. This PR makes a small change which addresses some of the edge cases (full fix will follow). Specifically, it addresses a case when a dependent service is started soon after restart command starts stopping dependent services. Previously, stopping command collects only running dependent services, this fix changes code to collect all dependent services (because stopped service now may be running after collection finished). * Added scaffolding to deterministically test handling of starting and restarting concurrency * Add a release note * Fix lint problems * Fix lint problem #2 * Update releasenotes/notes/improve-agent-start-restart-concurrency-642420923b9ea275.yaml Co-authored-by: DeForest Richards <56796055+drichards-87@users.noreply.github.com> --------- Co-authored-by: DeForest Richards <56796055+drichards-87@users.noreply.github.com>
|
|
||
| var status svc.Status | ||
| status, err = s.mainSvc.Query() | ||
| assert.Nil(s.T(), err, "Main service should restar successfully: %v", err) |
There was a problem hiding this comment.
💬 suggestion
Using the same error message you use at line 307 can end up in harder time knowing what did not work in case of failure, suggest changing the message here to should query main service with no error
There was a problem hiding this comment.
Will do, too bad I did not wait for you to review :), thank you
4 participants
What does this PR do?
Improve agent starting and simultaneously restarting, which is common for Agent install and upgrade, concurrency handling. Specifically it handles the following edge case:
datadogagentservice is starting in the end of Agent installation (no Agent services are running at this moment)agent.exe restart-serviceCLI command is started.agent.exe restart-serviceCLI command will get list of datadogagent dependent services to be stopped before stoppingdatadogagentservice itself.datadog-trace-agentservice (or other dependent services) is started by the still startingdatadogagentservice (from step no.1).agent.exe restart-serviceCLI command should stop just starteddatadog-trace-agentservice before moving on to stopdatadogagentservice itself.Specifically:
datadogagentservice previously collected onlyRunningdependent service. In some cases with concurrent execution stopping dependent services missed just started dependents services if they started AFTER dependent service collection,. The command would miss newly started dependent services and stoppingdatadogagentservice would fail because a dependent service is still running. New code collects all dependent services to avoid this particular edge case. It does not solve all possible race condition but it eliminates a few of them.Both changes above are also similar to the Powershell's Restart-Service cmdlet logic.
Motivation
We suspect that there is a small risk that certain small timing changes in Agent code may increase race conditions outlined above.
Additional Notes
Possible Drawbacks / Trade-offs
Describe how to test/QA your changes
No need for manual test since key logic is tested in the accompanied unit tests