feat(spider-execution-manager): Forward RUST_LOG and configured environment variables to spawned task executors. - #389
Conversation
WalkthroughChangesThe execution manager adds an Executor environment forwarding
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Config
participant Runtime
participant ProcessPool
participant TaskExecutor
Config->>Runtime: propagate inherited_env
Runtime->>ProcessPool: initialize ProcessPoolConfig
ProcessPool->>TaskExecutor: spawn with RUST_LOG and configured environment values
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
LinZhihao-723
left a comment
There was a problem hiding this comment.
The current way for passing env var for CLP is confirmed with @junhaoliao: we'd prefer to pass only the key, not the entire kv pair.
| #[serde(default)] | ||
| pub env_keys: Vec<String>, |
There was a problem hiding this comment.
I don't think env_keys is a good name in the config. Since these envs are to be passed into task executor., how about executor_env_keys.
There was a problem hiding this comment.
I don't fully agree. While these keys are indeed passed into the executor, it's also required that the execution manager has these environment variables defined. A more precise name would probably be inherited_env_keys, but that sounds too developer-oriented.
There was a problem hiding this comment.
Actually, maybe env or environment should be sufficient: these are how k8s and docker-compose specify the env keys (without values), respectively.
There was a problem hiding this comment.
Is there any risk a user of Spider is not a developer or a coding agent? lol.
I am ok with no keys. How about inherited_env?
|
|
||
| for key in &self.config.env_keys { | ||
| match std::env::var(key) { | ||
| Ok(value) => { |
There was a problem hiding this comment.
We need to check and skip RUST_LOG.
There was a problem hiding this comment.
I don't think we need to do an extra check since:
command.envis an upsert; without passing the explicit values, this upsert should be a no-op.- When
RUST_LOGis explicitly given in this list and it's not set, we should print a warning to make this absence visible. While in the default path, it is allowed to haveRUST_LOGunset.
Description
This PR lets the execution manager forward environment variables into the
spider-task-executorsubprocesses its process pool spawns.RUST_LOGis always forwarded so an executor's log verbosity matches the execution manager's, and a newenv_keysconfiguration field lets operators forward an arbitrary list of additional variables (for example credentials or feature flags a task package needs). Each variable's value is read from the execution manager's own environment at spawn time. Forwarding is additive: the child already inherits the full parent environment, so this only sets the selected keys explicitly on top.env_keysconfiguration (config.rs)env_keys: Vec<String>toTaskExecutorConfig, deserialized from thetask_executorsection of the YAML config. It names the environment variables to forward to executors, beyond the always-forwardedRUST_LOG. The field is#[serde(default)], so it is optional and defaults to an empty list when omitted.Config::runtime_config()forwards the list into the derivedRuntimeConfig.Threading through the runtime (
runtime.rs)env_keystoRuntimeConfig, and passes it intoProcessPoolConfigwhenRuntime::createbuilds the process pool.Environment forwarding at spawn (
process_pool.rs)env_keystoProcessPoolConfig.spawn_executornow populates the childCommand's environment before spawning:RUST_LOGis always forwarded when it is set in the execution manager's environment, and is silently skipped when unset (it is commonly unset, so a missing value is not worth a warning). Each variable named inenv_keysis then forwarded when set; a key that is unset or holds a non-Unicode value is skipped with awarn-level log that carries the key and the underlyingstd::env::VarError(which distinguishes not-present from non-Unicode).env_clear()is issued, so the child keeps the inherited parent environment and the explicit forwards remain correct even if the child environment is later cleared.Tests (
em-runtime-tests,task-executor-tests)env_keys: Vec::new(), preserving the existing no-forwarding behavior.Notes
RUST_LOGis treated as a built-in and is not part ofenv_keys; the two paths differ deliberately on a missing value (silent forRUST_LOG, warned for a configured key, since a configured key was explicitly requested).env_keysis optional (#[serde(default)]): atask_executorconfig that omits it deserializes with an empty list — no forwarding beyondRUST_LOG— so existing config files keep loading unchanged.Checklist
breaking change.
Validation performed
Summary by CodeRabbit
New Features
RUST_LOGwhen it is set.Tests