[procmgr] Config gates for processes.d auto-start - #54732
[procmgr] Config gates for processes.d auto-start#54732jose-manuel-almaza wants to merge 16 commits into
Conversation
jose-manuel-almaza
added
the
qa/done
jose-manuel-almaza
added
changelog/no-changelog
There was a problem hiding this comment.
Could this just read the config schema instead of duplicating everything ?
There was a problem hiding this comment.
I'd like that longer term, but afaik procmgr can't use the Go config runtime or the embedded schema. For config gates we only need env resolution for ~13 keys, so this PR keeps an explicit table that mirrors those schema env_vars (with schema comments cross-linking back). Happy to follow up with either schema-driven codegen for env_bindings.rs or a CI check that diffs ENV_BINDINGS against schema env_vars for gated keys. WDYT?
There was a problem hiding this comment.
The schema is a yaml schema with a well known format so I do think it could be used ?
Either read at build/run time, or to generate rust code for those configs
There was a problem hiding this comment.
Yep. That's what I meant by long term. I'll evaluate the effort to implement it now
![datadog-prod-us1-5[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
The reload path now leaves a failed config-managed process stopped even after a subsequent config reload supplies a valid command, removing the recovery behavior covered by the deleted test. The same diff also removes the schema defaults for sketch v3 routing and shadow settings while serializer code still reads them, which can produce an empty v3beta route or disable expected shadowing.
🤖 Datadog Autotest · Commit 8cc535a · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8cc535acbf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
This comment has been minimized.
This comment has been minimized.
2ad9173 to
8fa88c5
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d2e1a30819
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Gate evaluation stays stubbed in the spawn-profiles PR; integration tests that require real YAML gate behavior belong in jose/procmgr-config-gates.
Gate integration tests live in #54732 with the real evaluator after they were removed from the spawn-profiles foundation PR.
feee95a to
b94e8b0
Compare
Gate integration tests live in #54732 with the real evaluator after they were removed from the spawn-profiles foundation PR.
b94e8b0 to
a06eeda
Compare
a06eeda to
df5ab47
Compare
There was a problem hiding this comment.
The added Rust coverage cannot compile: the manager tests lost required imports, and the Windows stdio module defines the same test function twice. These blockers prevent CI from validating the new config-gate and spawn behavior.
🤖 Datadog Autotest · Commit c3919a6 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
| }; | ||
| use crate::test_helpers; | ||
| use crate::uuid_gen::{SequentialUuidGenerator, UuidGenerator, V4UuidGenerator}; | ||
| use std::io::Write; |
There was a problem hiding this comment.
The Rust test target cannot compile, preventing CI from validating the new start and restart gate behavior.
Assertion details
- Input: Compile the crate with
cfg(test)enabled. - Expected: Restore the parent-module import and explicitly import
crate::config_gate::ConditionConfigFileso the Rust test target compiles. This requires updating the test module imports, so no production-code-only inline suggestion is appropriate. - Actual: The PR removes
use super::*, but this test module still uses parent-only names includingProcessManager,RuntimeHandles,mpsc,ExitEvent,PendingRestart, andManagedProcess. Its new gate helpers also useConditionConfigFilewithout importing it.
Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
| } | ||
|
|
||
| #[test] | ||
| fn unopenable_file_path_falls_back_to_inherit() { |
There was a problem hiding this comment.
Windows Rust tests cannot compile, blocking validation of the changed spawn and stdio behavior.
Assertion details
- Input: Compile the Windows Rust test module.
- Expected:
Delete the newly added duplicate block at lines 290–303; the earlier identical coverage should remain. This is a test-code correction, so no production-code-only inline suggestion is provided. - Actual: The added block defines
unopenable_file_path_falls_back_to_inherita second time in the same test module; the first definition remains at line 240. Rust rejects duplicate function names in one module.
Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
There was a problem hiding this comment.
The new gate call references a nonexistent ManagedProcess method, so dd-procmgrd cannot compile; the PR also breaks locked dependency resolution and both general and Windows test compilation.
🤖 Datadog Autotest · Commit b21c86a · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
| #[must_use] | ||
| pub(crate) fn start_conditions_met(&self) -> bool { | ||
| self.condition_path_exists_met() | ||
| self.condition_path_exists_met() && self.config_gate_met() |
There was a problem hiding this comment.
Every dd-procmgrd build fails to compile before the new config gates can run.
Assertion details
- Input: Compile any dd-procmgrd target containing
ManagedProcess::start_conditions_met. - Expected: The start-condition path should call the existing
condition_config_any_metfunction or define an equivalentManagedProcesspredicate. - Actual:
start_conditions_metcallsself.config_gate_met(), butManagedProcessdefines no such method and no trait supplies it, producing a method-resolution compilation error.
Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
| @@ -736,14 +717,23 @@ dependencies = [ | |||
| "dd-procmgr-client", | |||
There was a problem hiding this comment.
Locked Cargo resolution fails before affected workspace packages can build.
Assertion details
- Input: Resolve or build the workspace with Cargo.lock enforced.
- Expected: Regenerate Cargo.lock with a complete
dd-procmgr-clientpackage entry and correct dependency lists. - Actual: The PR removes the
dd-procmgr-clientpackage entry whiledd-procmgrdandpar-controlstill list that workspace package as a dependency.
Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
| }; | ||
| use crate::test_helpers; | ||
| use crate::uuid_gen::{SequentialUuidGenerator, UuidGenerator, V4UuidGenerator}; | ||
| use std::io::Write; |
There was a problem hiding this comment.
The general Rust unit-test target fails during compilation, so none of its tests run.
Assertion details
- Input: Compile the manager module with
cfg(test). - Expected: Restore the parent-module imports and import or fully qualify
ConditionConfigFile. - Actual: Removing
use super::*leaves parent-module symbols such asProcessManager,RuntimeHandles, andExitEventunresolved; the added helpers also useConditionConfigFilewithout importing it.
Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
| } | ||
|
|
||
| #[test] | ||
| fn unopenable_file_path_falls_back_to_inherit() { |
There was a problem hiding this comment.
The Windows Rust test target cannot compile, blocking Windows CI validation.
Assertion details
- Input: Compile the Windows stdio module with
cfg(test). - Expected:
Remove the duplicate test or rename it to cover a distinct behavior. - Actual: The added test duplicates the existing
unopenable_file_path_falls_back_to_inheritfunction in the same module, causing Rust error E0428.
Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
Parse condition_config_any from processes.d and evaluate config gates before auto-start; stub always-open gates are replaced in the next commit.
…R 2/4) Restore core Agent SCM env cache, resolve_executable, tokio baseline env, secret-backend win32 helpers, and Windows to_command_stdio helpers moved out of the spawn-profiles PR so each stacked PR carries only what it uses.
Add Agent-parity config gate evaluation (YAML, env, fleet, system-probe derivations) so processes.d definitions can gate on process_config and related settings. Secret backend resolution lands in PR 3.
Revert accidental sketches-to-series rename in core_schema.yaml from the config-gates split. Serializer shadow settings still read sketches.* keys.
Mirror condition_path_exists behavior so ${DD_CONF_DIR} and other
placeholders resolve before YAML gate evaluation.
serde_yaml leaves yes/on as strings while go.yaml.in/yaml/v2 treats them as true. Apply YAML 1.1 parsing for file lookups only; env stays ParseBool.
Gate integration tests live in #54732 with the real evaluator after they were removed from the spawn-profiles foundation PR.
Restore pkg/config/setup/BUILD.bazel comments from the base branch. The #53249 split had added nonexistent schema.codegen flags; this PR only needs the config_windows_test.go test target wiring.
…g gates Use sp_bool_default for discovery.enabled so system_probe_config.enabled derivation matches Go on Linux (true when unset) and ECS Fargate (false).
Resolve fleet policy directory from the config file being evaluated (system-probe.yaml or datadog.yaml), matching Go system-probe loading.
…g gates Load YAML with saphyr so YAML 1.1 bool spellings apply only to plain scalars; quoted strings use ParseBool only, matching agent config semantics.
PATHEXT defaults use uppercase extensions while created files often use lowercase; match existing files via read_dir when Path::is_file fails.
…ails Warn and reuse the inherit/NUL stdio path when CreateFileW cannot open a configured stdout/stderr file, matching Unix and tokio spawn behavior.
4 participants
What does this PR do?
Adds config gates to dd-procmgr so
processes.ddefinitions can usecondition_config_anyto auto-start only when Agent config says they should.Implementation mirrors the Windows legacy SCM startup checks in
dependent_services_windows.goand Agent config resolution:DD_*) with Agent precedence (ignore empty values, no trim beforeParseBool, legacyprocess_config.enabledtransforms)system_probe_config.enabled(USM/NPM/security knobs, sk-tracer and discovery adjustments)DD_*overrides from the core Agent SCMEnvironmentregistry when not set in the procmgr process envWires gate evaluation into
ManagedProcessstart/reload paths in the manager.Stack context: PR 2. Builds on #54731 (
jose/procmgr-spawn-profiles). List/describe (#53568) is parallel and is not a parent of this PR.Motivation
Moving subservices (starting with process-agent) to dd-procmgr requires the supervisor to apply the same start/stop rules as the Agent today. Without config gates, a
processes.dentry would always spawn when registered, which breaks parity with legacy SCM and fleet policy.Describe how you validated your changes
pkg/procmgr/rust/src/config_gate/(env bindings, YAML load, system-probe derivations, gate evaluation)pkg/config/setup/config_windows_test.go)Additional Notes
ENC[...]/ secret backend resolution in this PR (fleet policyENC[...]values are not resolved here either).pkg/config/schema/yaml/process_config.yamlfor env binding documentation.pkg/system-probe/config/so procmgr gate derivations stay aligned with Goadjust*logic.