config/remote/service: clarify locking behaviors - #42061
Conversation
7c5c597 to
4a0d230
Compare
Static quality checks✅ Please find below the results from static quality gates Successful checksInfo
|
Regression DetectorRegression Detector ResultsMetrics dashboard Baseline: 9dc3550 Optimization Goals: ✅ No significant changes detected
|
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | docker_containers_cpu | % cpu utilization | -0.38 | [-2.27, +1.50] | 1 | Logs |
Fine details of change detection per experiment
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | docker_containers_memory | memory utilization | +1.19 | [+0.78, +1.60] | 1 | Logs |
| ➖ | quality_gate_idle_all_features | memory utilization | +0.21 | [+0.17, +0.26] | 1 | Logs bounds checks dashboard |
| ➖ | ddot_metrics_sum_delta | memory utilization | +0.18 | [+0.02, +0.34] | 1 | Logs |
| ➖ | file_to_blackhole_100ms_latency | egress throughput | +0.16 | [-0.45, +0.76] | 1 | Logs |
| ➖ | quality_gate_logs | % cpu utilization | +0.08 | [-2.71, +2.88] | 1 | Logs bounds checks dashboard |
| ➖ | otlp_ingest_metrics | memory utilization | +0.02 | [-0.10, +0.15] | 1 | Logs |
| ➖ | uds_dogstatsd_to_api | ingress throughput | +0.00 | [-0.22, +0.22] | 1 | Logs |
| ➖ | tcp_dd_logs_filter_exclude | ingress throughput | -0.00 | [-0.01, +0.01] | 1 | Logs |
| ➖ | file_to_blackhole_500ms_latency | egress throughput | -0.02 | [-0.63, +0.59] | 1 | Logs |
| ➖ | file_to_blackhole_0ms_latency | egress throughput | -0.04 | [-0.65, +0.56] | 1 | Logs |
| ➖ | file_to_blackhole_1000ms_latency | egress throughput | -0.21 | [-0.82, +0.41] | 1 | Logs |
| ➖ | ddot_logs | memory utilization | -0.25 | [-0.33, -0.17] | 1 | Logs |
| ➖ | ddot_metrics_sum_cumulative | memory utilization | -0.26 | [-0.38, -0.14] | 1 | Logs |
| ➖ | ddot_metrics | memory utilization | -0.31 | [-0.46, -0.15] | 1 | Logs |
| ➖ | quality_gate_idle | memory utilization | -0.34 | [-0.38, -0.30] | 1 | Logs bounds checks dashboard |
| ➖ | uds_dogstatsd_20mb_12k_contexts_20_senders | memory utilization | -0.34 | [-0.39, -0.29] | 1 | Logs |
| ➖ | file_tree | memory utilization | -0.37 | [-0.43, -0.31] | 1 | Logs |
| ➖ | docker_containers_cpu | % cpu utilization | -0.38 | [-2.27, +1.50] | 1 | Logs |
| ➖ | ddot_metrics_sum_cumulativetodelta_exporter | memory utilization | -0.41 | [-0.60, -0.21] | 1 | Logs |
| ➖ | otlp_ingest_logs | memory utilization | -0.43 | [-0.57, -0.30] | 1 | Logs |
| ➖ | quality_gate_metrics_logs | memory utilization | -0.49 | [-0.70, -0.29] | 1 | Logs bounds checks dashboard |
| ➖ | tcp_syslog_to_blackhole | ingress throughput | -0.91 | [-0.98, -0.84] | 1 | Logs |
Bounds Checks: ✅ Passed
| perf | experiment | bounds_check_name | replicates_passed | links |
|---|---|---|---|---|
| ✅ | docker_containers_cpu | simple_check_run | 10/10 | |
| ✅ | docker_containers_memory | memory_usage | 10/10 | |
| ✅ | docker_containers_memory | simple_check_run | 10/10 | |
| ✅ | file_to_blackhole_0ms_latency | lost_bytes | 10/10 | |
| ✅ | file_to_blackhole_0ms_latency | memory_usage | 10/10 | |
| ✅ | file_to_blackhole_1000ms_latency | 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_500ms_latency | lost_bytes | 10/10 | |
| ✅ | file_to_blackhole_500ms_latency | memory_usage | 10/10 | |
| ✅ | quality_gate_idle | intake_connections | 10/10 | bounds checks dashboard |
| ✅ | quality_gate_idle | memory_usage | 10/10 | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | intake_connections | 10/10 | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | memory_usage | 10/10 | bounds checks dashboard |
| ✅ | quality_gate_logs | intake_connections | 10/10 | bounds checks dashboard |
| ✅ | quality_gate_logs | lost_bytes | 10/10 | bounds checks dashboard |
| ✅ | quality_gate_logs | memory_usage | 10/10 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | cpu_usage | 10/10 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | intake_connections | 10/10 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | lost_bytes | 10/10 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | memory_usage | 10/10 | bounds checks dashboard |
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, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check lost_bytes: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check lost_bytes: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.
| response, err := s.api.FetchOrgStatus(context.Background()) | ||
| response, err := s.getAPI().FetchOrgStatus(context.Background()) | ||
|
|
||
| s.mu.Lock() |
There was a problem hiding this comment.
there's no need for this to be protected by a mutex, it's an internal function and is guaranteed by the caller to be called only by one goroutine exclusively, this forces to serialize operations that are not supposed to be.
If you want to change the structure then the various functionality should still be independent as they are now. i.e. separate them in their own struct will show why there was no need for those mutex locks to being with.
As an example: #42072
There was a problem hiding this comment.
Thanks for the suggestion. I incorporated your commit.
| windowDuration time.Duration | ||
| capacity int | ||
| allowance int | ||
| mu struct { |
There was a problem hiding this comment.
this struct is never called by multiple goroutines, while this adds close to no overhead it's not necessary
There was a problem hiding this comment.
I see that now. I read the code more closely, thanks for the push. After reading more closely it became clearer to me that this thing is just a rate limiter and the channel doesn't need to be coupled to it. I renamed it and added commentary clarifying the usage. See if it pleases oyu./
A better refactor might be to pull the state of the refresh loop out of the service struct, but I'll not do that here.
4a0d230 to
3b314d6
Compare
| response, err := s.api.FetchOrgStatus(context.Background()) | ||
| response, err := s.getAPI().FetchOrgStatus(context.Background()) | ||
|
|
||
| s.mu.Lock() |
There was a problem hiding this comment.
Thanks for the suggestion. I incorporated your commit.
| windowDuration time.Duration | ||
| capacity int | ||
| allowance int | ||
| mu struct { |
There was a problem hiding this comment.
I see that now. I read the code more closely, thanks for the push. After reading more closely it became clearer to me that this thing is just a rate limiter and the channel doesn't need to be coupled to it. I renamed it and added commentary clarifying the usage. See if it pleases oyu./
A better refactor might be to pull the state of the refresh loop out of the service struct, but I'll not do that here.
| func (s *CoreAgentService) getAPI() api.API { | ||
| s.mu.Lock() | ||
| defer s.mu.Unlock() | ||
| return s.mu.api |
There was a problem hiding this comment.
this method shows the api shouldn't be in the mutex lock as anyway it removes any protection, which means that also the UpdatePARJWT call doesn't need the mutex
There was a problem hiding this comment.
Yeah, you're right. I confused myself at some point into thinking that api could be reset in the way db and uptane could. Fixed.
| } | ||
|
|
||
| func (s *CoreAgentService) refresh() error { | ||
| s.Lock() |
There was a problem hiding this comment.
this is nice, thanks for removing the exposed mutex!
| if err != nil { | ||
| s.Unlock() | ||
| s.mu.Unlock() |
There was a problem hiding this comment.
shouldn't this be in a defer? we must guaranteee it's unlocked and it's always locked or in the critical points where we don't need it locked it's unlocked and then locked
There was a problem hiding this comment.
Sure. In this specific case I think it was fine, but in general I agree that having defers is better. I added a commit that handles the unlocking and locking around the Fetch more explicitly and delegates the top-level locking to a defer.
| @@ -953,12 +1017,14 @@ func (s *CoreAgentService) ClientGetConfigs(_ context.Context, request *pbgo.Cli | |||
| return nil, err | |||
| } | |||
|
|
|||
| targetFiles, err := s.getTargetFiles(s.uptane, neededFiles) | |||
| // TODO: Do not hold the mutex while calling getTargetFiles -- it may go to | |||
There was a problem hiding this comment.
this is fine, the CDN source has never been used and there's no plan to use it in the future, so this is going to disk exclusively.
There was a problem hiding this comment.
Ack. There's an argument to be made that going to disk while holding the mutex is also bad, but you're right that in this context relative to other things it doesn't really matter.
| rcType string | ||
| db *bbolt.DB |
There was a problem hiding this comment.
it must not have access to the db reference itself, it's owned and contained by uptane
There was a problem hiding this comment.
I tacked on a commit to hide this underneath an io.Closer as that's all it's here for. I also made Close idempotent.
f15fff4 to
541b5c0
Compare
| if err != nil { | ||
| s.Unlock() | ||
| s.mu.Unlock() |
There was a problem hiding this comment.
Sure. In this specific case I think it was fine, but in general I agree that having defers is better. I added a commit that handles the unlocking and locking around the Fetch more explicitly and delegates the top-level locking to a defer.
| rcType string | ||
| db *bbolt.DB |
There was a problem hiding this comment.
I tacked on a commit to hide this underneath an io.Closer as that's all it's here for. I also made Close idempotent.
| func (s *CoreAgentService) getAPI() api.API { | ||
| s.mu.Lock() | ||
| defer s.mu.Unlock() | ||
| return s.mu.api |
There was a problem hiding this comment.
Yeah, you're right. I confused myself at some point into thinking that api could be reset in the way db and uptane could. Fixed.
| @@ -953,12 +1017,14 @@ func (s *CoreAgentService) ClientGetConfigs(_ context.Context, request *pbgo.Cli | |||
| return nil, err | |||
| } | |||
|
|
|||
| targetFiles, err := s.getTargetFiles(s.uptane, neededFiles) | |||
| // TODO: Do not hold the mutex while calling getTargetFiles -- it may go to | |||
There was a problem hiding this comment.
Ack. There's an argument to be made that going to disk while holding the mutex is also bad, but you're right that in this context relative to other things it doesn't really matter.
|
/gitlab trigger-ci |
|
View all feedbacks in Devflow UI.
Started pipeline #79804923 |
541b5c0 to
84cabf7
Compare
|
|
||
| agentVersion string | ||
| db *bbolt.DB |
There was a problem hiding this comment.
this looks great, thank you!
I have a PR that was supposed to get merged in on Friday but it's been blocked by code freezes 😀 it addresses removing db from CoreAgentService - all db interactions will now be done via the uptaneClient - #40065
There was a problem hiding this comment.
Fine by me, I'm happy to rebase your change.
This `Service` structure is exported but with no users and now no methods. Furthermore, it embeds a mutex and two fields, but the two fields that it embeds don't actually use that mutex -- they are never modified. Instead we hoist the fields up to the `CoreAgentService`, the actual user of these fields.
The embedding before made `Lock()` and `Unlock()` part of the public API of the `CoreAgentService`. This was not intentional.
The comment said it should be, but nobody ever set this channel to nil, so subsequent calls would panic.
a603de4 to
dfbd4fe
Compare
Isolate the state of the orgpoller to clarify synchronization.
Rename `cacheBypassClients` to `rateLimiter` and extract the bypass request channel from the rate limiter to CoreAgentService, clarifying the threading model and separation of concerns.
This dependency is never changed.
dfbd4fe to
d9b07d7
Compare
|
/merge |
|
View all feedbacks in Devflow UI.
This merge request is not mergeable according to GitHub. Common reasons include pending required checks, missing approvals, or merge conflicts — but it could also be blocked by other repository rules or settings.
The expected merge time in
|
### What does this PR do? Fixes `TestWithDatabaseFileName` to test what it was intended to. ### Motivation Before #40065, this test was exercising the `NewService` constructor to ensure that the option was respected. In that change, the test was modified to set up a mock that returns the expected value and then to call the mock directly -- it stopped testing anything. ### Additional Notes Stacked on #42061 Co-authored-by: andrew.werner <andrew.werner@datadoghq.com>
### What does this PR do? Properly use the injected clock everywhere. ### Motivation It speeds up the tests, and it makes it possible to write tests involving time: Before: ``` $ go test -tags test ./pkg/config/remote/service --count=1 ok github.com/DataDog/datadog-agent/pkg/config/remote/service 2.213s ``` After ``` $ go test -tags test ./pkg/config/remote/service --count=1 ok github.com/DataDog/datadog-agent/pkg/config/remote/service 0.206s ``` ### Describe how you validated your changes ### Additional Notes Stacked on #42061 Co-authored-by: andrew.werner <andrew.werner@datadoghq.com>
| mu struct { | ||
| sync.Mutex |
There was a problem hiding this comment.
This is a cool pattern! I am curious where did you learn it from?
What does this PR do?
This PR is a bundle of cleanups around the code in
pkg/config/remote/service. It fixes some race conditions, and more generally attempts to tame and document the use of mutexes around this service.Motivation
I'm making changes to this package as part of https://datadoghq.atlassian.net/browse/DEBUG-4590 and was having a hard time getting a handle of the synchronization.
Describe how you validated your changes
All the tests pass, and no behavior should be changed.
Additional Notes
This pattern of using an anonymous struct with an embedded mutex to document what is protected by that mutex is a pattern I've seen widely used in the Go ecosystem and one I've found broadly beneficial.
PLEASE REVIEW COMMIT BY COMMIT