Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions components/spider-execution-manager/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ impl Config {
executor_binary_path: self.task_executor.bin_path.clone(),
package_dir: self.task_executor.package_dir.clone(),
log_dir: self.task_executor.log_dir.clone(),
env_keys: self.task_executor.env_keys.clone(),
}
}
}
Expand Down Expand Up @@ -83,4 +84,10 @@ pub struct TaskExecutorConfig {

/// Directory the process pool writes per-executor stderr logs into.
pub log_dir: PathBuf,

/// Names of environment variables forwarded from the execution manager's process into each
/// spawned `spider-task-executor`. Their values are read from this process's environment at
/// spawn time.
#[serde(default)]
pub env_keys: Vec<String>,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, maybe env or environment should be sufficient: these are how k8s and docker-compose specify the env keys (without values), respectively.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}
30 changes: 29 additions & 1 deletion components/spider-execution-manager/src/process_pool.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,11 @@ pub struct ProcessPoolConfig {
/// Per-spawn filenames mean each respawn naturally rotates onto a fresh file; a long-lived
/// healthy executor accumulates into one file.
pub log_dir: PathBuf,

/// Names of environment variables forwarded from the execution manager's process into each
/// spawned executor. For each key, the value is read from this process's environment at spawn
/// time and set on the child; a key that is unset (or non-Unicode) is skipped with a warning.
pub env_keys: Vec<String>,
}

/// Request to execute a task inside the spawned task executor.
Expand Down Expand Up @@ -203,7 +208,8 @@ impl ProcessPool {
/// log file, and wraps the child's stdin/stdout in length-delimited codec frames.
///
/// The child's stderr is redirected to `<log_dir>/<em_id>-<executor_id>.log` in
/// create-or-append mode.
/// create-or-append mode. `RUST_LOG`, if set, is forwarded to the spawned task executor to make
/// the child process' log level match the current execution manager.
///
/// # Returns
///
Expand Down Expand Up @@ -234,6 +240,28 @@ impl ProcessPool {
.stdout(Stdio::piped())
.stderr(Stdio::from(log_file))
.kill_on_drop(true);

if let Ok(rust_log) = std::env::var("RUST_LOG") {
command.env("RUST_LOG", rust_log);
}

for key in &self.config.env_keys {
match std::env::var(key) {
Ok(value) => {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to check and skip RUST_LOG.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to do an extra check since:

  • command.env is an upsert; without passing the explicit values, this upsert should be a no-op.
  • When RUST_LOG is 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 have RUST_LOG unset.

command.env(key, value);
}
Err(e) => {
tracing::warn!(
executor_id,
env_key = % key,
err = % e,
"Configured env key could not be read from the execution manager's \
environment; skipping."
);
}
}
}

let mut child = command.spawn()?;
let stdin = child
.stdin
Expand Down
5 changes: 5 additions & 0 deletions components/spider-execution-manager/src/runtime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,10 @@ pub struct RuntimeConfig {

/// Directory the process pool writes per-executor stderr logs into.
pub log_dir: PathBuf,

/// Names of environment variables forwarded from the execution manager's process into each
/// spawned `spider-task-executor` (values read from this process's environment at spawn time).
pub env_keys: Vec<String>,
}

/// Errors returned by [`Runtime`] during bootstrap or the main loop.
Expand Down Expand Up @@ -150,6 +154,7 @@ impl<
executor_binary_path: config.executor_binary_path,
package_dir: config.package_dir,
log_dir: config.log_dir,
env_keys: config.env_keys,
})?;

let cancellation_token = CancellationToken::new();
Expand Down
1 change: 1 addition & 0 deletions tests/huntsman/em-runtime/tests/test_runtime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ fn runtime_config(heartbeat_interval: Duration) -> RuntimeConfig {
executor_binary_path: task_executor_bin(),
package_dir: tdl_package_dir(),
log_dir,
env_keys: Vec::new(),
}
}

Expand Down
1 change: 1 addition & 0 deletions tests/huntsman/task-executor/tests/test_process_pool.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ fn build_pool() -> ProcessPool {
executor_binary_path: task_executor_bin(),
package_dir: tdl_package_dir(),
log_dir,
env_keys: Vec::new(),
};
ProcessPool::new(config).expect("construct pool")
}
Expand Down
Loading