Fix Windows Ctrl-c #31857

jack0x2 · 2024-12-06T21:31:56Z

What does this PR do?

Fixes issue on windows where when running agent.exe run you could not ctrl-c. This was caused by pythons usage of the POSIX signal library, which reregisters the Console Handler.

Motivation

https://datadoghq.atlassian.net/browse/WINA-467

Allows ctrl-c of the agent.exe command when running it manually.

Describe how you validated your changes

Linuxs:

Is tested with added e2e test to test that ctrl-c can be exited with SIGINT.

Windows:

Sending Console Control within e2e framework fails due to issues with allocated ssh consoles on windows.
Manual Steps:
- Create windows machine with agent installed
- run Stop-Service datadogagent
- run & 'C:\Program Files\Datadog\Datadog Agent\bin\agent.exe' run
- Wait for agent to start running checks (wanna make sure python checks have started running)
- ctrl-c
- Make sure the agent exits

Possible Drawbacks / Trade-offs

Currently the reregistering of the console handler is done in the GetPlatformModules function, which means it is reregistered for both when agent is run from service interface and manually with run command.

Additional Notes

This currently reregisters the Console Handler on windows, but because it was changed by python all uses of the go signal.notify function will no longer be notified of the console events. Updates to cpython would be needed to prevent signal library on windows from calling POSIX function on import.

agent-platform-auto-pr · 2024-12-06T22:19:35Z

Test changes on VM

Use this command from test-infra-definitions to manually test this PR changes on a VM:

inv aws.create-vm --pipeline-id=50965491 --os-family=ubuntu

Note: This applies to commit 7c940ed

cit-pr-commenter · 2024-12-06T22:46:17Z

Regression Detector

Regression Detector Results

Metrics dashboard
Target profiles
Run ID: a294ae3b-cd4c-4c33-b722-05c959edc589

Baseline: 68f07c6
Comparison: 7c940ed
Diff

Optimization Goals: ✅ No significant changes detected

Fine details of change detection per experiment

perf	experiment	goal	Δ mean %	Δ mean % CI	trials	links
➖	quality_gate_logs	% cpu utilization	+1.92	[-1.06, +4.91]	1	Logs
➖	quality_gate_idle_all_features	memory utilization	+1.86	[+1.74, +1.98]	1	Logs bounds checks dashboard
➖	tcp_syslog_to_blackhole	ingress throughput	+0.78	[+0.71, +0.85]	1	Logs
➖	uds_dogstatsd_to_api_cpu	% cpu utilization	+0.59	[-0.13, +1.31]	1	Logs
➖	file_tree	memory utilization	+0.21	[+0.09, +0.33]	1	Logs
➖	quality_gate_idle	memory utilization	+0.08	[+0.03, +0.12]	1	Logs bounds checks dashboard
➖	file_to_blackhole_0ms_latency_http2	egress throughput	+0.02	[-0.78, +0.82]	1	Logs
➖	file_to_blackhole_1000ms_latency_linear_load	egress throughput	+0.00	[-0.46, +0.47]	1	Logs
➖	tcp_dd_logs_filter_exclude	ingress throughput	+0.00	[-0.01, +0.01]	1	Logs
➖	uds_dogstatsd_to_api	ingress throughput	-0.00	[-0.11, +0.11]	1	Logs
➖	file_to_blackhole_300ms_latency	egress throughput	-0.01	[-0.64, +0.62]	1	Logs
➖	file_to_blackhole_100ms_latency	egress throughput	-0.05	[-0.77, +0.67]	1	Logs
➖	file_to_blackhole_0ms_latency	egress throughput	-0.07	[-0.92, +0.79]	1	Logs
➖	file_to_blackhole_0ms_latency_http1	egress throughput	-0.08	[-0.92, +0.77]	1	Logs
➖	file_to_blackhole_500ms_latency	egress throughput	-0.17	[-0.93, +0.60]	1	Logs
➖	otel_to_otel_logs	ingress throughput	-0.60	[-1.25, +0.04]	1	Logs
➖	file_to_blackhole_1000ms_latency	egress throughput	-0.79	[-1.57, -0.01]	1	Logs

Bounds Checks: ✅ Passed

perf	experiment	bounds_check_name	replicates_passed	links
✅	file_to_blackhole_0ms_latency	lost_bytes	10/10
✅	file_to_blackhole_0ms_latency	memory_usage	10/10
✅	file_to_blackhole_0ms_latency_http1	lost_bytes	10/10
✅	file_to_blackhole_0ms_latency_http1	memory_usage	10/10
✅	file_to_blackhole_0ms_latency_http2	lost_bytes	10/10
✅	file_to_blackhole_0ms_latency_http2	memory_usage	10/10
✅	file_to_blackhole_1000ms_latency	memory_usage	10/10
✅	file_to_blackhole_1000ms_latency_linear_load	memory_usage	10/10
✅	file_to_blackhole_100ms_latency	lost_bytes	10/10
✅	file_to_blackhole_100ms_latency	memory_usage	10/10
✅	file_to_blackhole_300ms_latency	lost_bytes	10/10
✅	file_to_blackhole_300ms_latency	memory_usage	10/10
✅	file_to_blackhole_500ms_latency	lost_bytes	10/10
✅	file_to_blackhole_500ms_latency	memory_usage	10/10
✅	quality_gate_idle	memory_usage	10/10	bounds checks dashboard
✅	quality_gate_idle_all_features	memory_usage	10/10	bounds checks dashboard
✅	quality_gate_logs	lost_bytes	10/10
✅	quality_gate_logs	memory_usage	10/10

Explanation

Confidence level: 90.00%
Effect size tolerance: |Δ mean %| ≥ 5.00%

Performance changes are noted in the perf column of each table:

✅ = significantly better comparison variant performance
❌ = significantly worse comparison variant performance
➖ = no significant change in performance

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

CI Pass/Fail Decision

✅ Passed. All Quality Gates passed.

quality_gate_idle_all_features, bounds check memory_usage: 10/10 replicas passed. Gate passed.
quality_gate_idle, bounds check memory_usage: 10/10 replicas passed. Gate passed.
quality_gate_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.
quality_gate_logs, bounds check lost_bytes: 10/10 replicas passed. Gate passed.

vickenty · 2024-12-09T16:41:48Z

rtloader/three/three.cpp

+    // force initialize siginterrupt with signal in python so it can be overwritten by the agent
+    // This only effects the windows builds as linux already has the sigint handler initialized
+    // and thus python will ignore it
+    Py_InitializeEx(1);


This doesn't seem to actually force anything, since this behavior is what we have today by the default:

This function works like Py_Initialize() if initsigs is 1.

(src)

I was just making it more explicit, but can go back the the current implantation.

cmd/agent/subcommands/run/command_windows.go

pkg/util/winutil/winctrlhandler.go

agent-platform-auto-pr · 2024-12-10T17:13:48Z

Package size comparison

Comparison with ancestor 68f07c6efd56010f4c23d64da525f3aadc8b3409

Diff per package

package	diff	status	size	ancestor	threshold
datadog-agent-amd64-deb	0.00MB	✅	1265.73MB	1265.73MB	140.00MB
datadog-iot-agent-amd64-deb	0.00MB	✅	113.24MB	113.24MB	10.00MB
datadog-dogstatsd-amd64-deb	0.00MB	✅	78.41MB	78.41MB	10.00MB
datadog-heroku-agent-amd64-deb	0.00MB	✅	526.58MB	526.58MB	70.00MB
datadog-agent-x86_64-rpm	0.00MB	✅	1274.97MB	1274.97MB	140.00MB
datadog-agent-x86_64-suse	0.00MB	✅	1274.97MB	1274.97MB	140.00MB
datadog-iot-agent-x86_64-rpm	0.00MB	✅	113.31MB	113.31MB	10.00MB
datadog-iot-agent-x86_64-suse	0.00MB	✅	113.31MB	113.31MB	10.00MB
datadog-dogstatsd-x86_64-rpm	0.00MB	✅	78.49MB	78.49MB	10.00MB
datadog-dogstatsd-x86_64-suse	0.00MB	✅	78.49MB	78.49MB	10.00MB
datadog-agent-arm64-deb	0.00MB	✅	1000.85MB	1000.85MB	140.00MB
datadog-iot-agent-arm64-deb	0.00MB	✅	108.72MB	108.72MB	10.00MB
datadog-dogstatsd-arm64-deb	0.00MB	✅	55.65MB	55.65MB	10.00MB
datadog-agent-aarch64-rpm	0.00MB	✅	1010.07MB	1010.07MB	140.00MB
datadog-iot-agent-aarch64-rpm	0.00MB	✅	108.79MB	108.79MB	10.00MB

Decision

✅ Passed

dustmop

LGTM, with one suggestion

pkg/util/winutil/winctrlhandler.go

jack0x2 · 2024-12-12T15:22:08Z

/merge

dd-devflow · 2024-12-12T15:22:57Z

Devflow running: `/merge`

View all feedbacks in Devflow UI.

2024-12-12 15:22:57 UTC ℹ️ MergeQueue: waiting for PR to be ready

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.
Note: if you pushed new commits since the last approval, you may need additional approval.
You can remove it from the waiting list with /remove command.

2024-12-12 16:28:15 UTC ℹ️ MergeQueue: merge request added to the queue

The median merge time in main is 26m.

2024-12-12 17:01:46 UTC ℹ️ MergeQueue: This merge request was merged

github-actions bot added team/agent-shared-components team/agent-devx-loops medium review PR review might take time labels Dec 6, 2024

jack0x2 added the qa/rc-required Only for a PR that requires validation on the Release Candidate label Dec 6, 2024

jack0x2 marked this pull request as ready for review December 9, 2024 14:49

jack0x2 requested review from a team as code owners December 9, 2024 14:49

jack0x2 requested a review from dustmop December 9, 2024 14:49

vickenty reviewed Dec 9, 2024

View reviewed changes

rtrieu approved these changes Dec 9, 2024

View reviewed changes

clarkb7 reviewed Dec 10, 2024

View reviewed changes

cmd/agent/subcommands/run/command_windows.go Show resolved Hide resolved

pkg/util/winutil/winctrlhandler.go Outdated Show resolved Hide resolved

pkg/util/winutil/winctrlhandler.go Outdated Show resolved Hide resolved

pkg/util/winutil/winctrlhandler.go Outdated Show resolved Hide resolved

jack0x2 added 4 commits December 10, 2024 10:42

Fix agent.exe run ctrl-c on windows

feb0007

Add linux ctrl-c test

61df0cb

Release Notes

2a34a30

fix comments and callback type

5c31b72

jack0x2 force-pushed the jack.phillips/fix-agent-ctrlc branch from 6633aa7 to 5c31b72 Compare December 10, 2024 16:48

remove extra line

5491d86

KevinFairise2 approved these changes Dec 11, 2024

View reviewed changes

dustmop approved these changes Dec 11, 2024

View reviewed changes

pkg/util/winutil/winctrlhandler.go Outdated Show resolved Hide resolved

jack0x2 requested a review from vickenty December 11, 2024 15:36

clarkb7 approved these changes Dec 11, 2024

View reviewed changes

vickenty approved these changes Dec 12, 2024

View reviewed changes

jack0x2 added 2 commits December 12, 2024 10:21

simplify line

a25bf11

Merge branch 'main' into jack.phillips/fix-agent-ctrlc

7c940ed

dd-mergequeue bot merged commit 2f2c6db into main Dec 12, 2024
348 of 365 checks passed

dd-mergequeue bot deleted the jack.phillips/fix-agent-ctrlc branch December 12, 2024 17:01

github-actions bot added this to the 7.62.0 milestone Dec 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Windows Ctrl-c #31857

Fix Windows Ctrl-c #31857

jack0x2 commented Dec 6, 2024 •

edited

Loading

agent-platform-auto-pr bot commented Dec 6, 2024 •

edited

Loading

cit-pr-commenter bot commented Dec 6, 2024 •

edited

Loading

Fine details of change detection per experiment

Bounds Checks: ✅ Passed

Explanation

vickenty Dec 9, 2024 •

edited

Loading

jack0x2 Dec 9, 2024

agent-platform-auto-pr bot commented Dec 10, 2024 •

edited

Loading

dustmop left a comment

jack0x2 commented Dec 12, 2024

dd-devflow bot commented Dec 12, 2024 •

edited

Loading

Fix Windows Ctrl-c #31857

Fix Windows Ctrl-c #31857

Conversation

jack0x2 commented Dec 6, 2024 • edited Loading

What does this PR do?

Motivation

Describe how you validated your changes

Possible Drawbacks / Trade-offs

Additional Notes

agent-platform-auto-pr bot commented Dec 6, 2024 • edited Loading

Test changes on VM

cit-pr-commenter bot commented Dec 6, 2024 • edited Loading

Regression Detector

Regression Detector Results

Optimization Goals: ✅ No significant changes detected

Fine details of change detection per experiment

Bounds Checks: ✅ Passed

Explanation

CI Pass/Fail Decision

vickenty Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

jack0x2 Dec 9, 2024

Choose a reason for hiding this comment

agent-platform-auto-pr bot commented Dec 10, 2024 • edited Loading

Package size comparison

Decision

dustmop left a comment

Choose a reason for hiding this comment

jack0x2 commented Dec 12, 2024

dd-devflow bot commented Dec 12, 2024 • edited Loading

Devflow running: /merge

jack0x2 commented Dec 6, 2024 •

edited

Loading

agent-platform-auto-pr bot commented Dec 6, 2024 •

edited

Loading

cit-pr-commenter bot commented Dec 6, 2024 •

edited

Loading

vickenty Dec 9, 2024 •

edited

Loading

agent-platform-auto-pr bot commented Dec 10, 2024 •

edited

Loading

dd-devflow bot commented Dec 12, 2024 •

edited

Loading

Devflow running: `/merge`