Skip to content

Commit

Permalink
[Breaking] Rename entrypoint_cmd->entrypoint and precondition_script (T…
Browse files Browse the repository at this point in the history
…raceMachina#475)

In prep for the release we are renaming a few config variables to
make things more future-proof. `entrypoint_cmd` is now `entrypoint`.

`precondition_script` is now `experimental_precondition_script`.
  • Loading branch information
allada authored Dec 12, 2023
1 parent a01a552 commit dbe61d2
Show file tree
Hide file tree
Showing 5 changed files with 28 additions and 28 deletions.
26 changes: 13 additions & 13 deletions nativelink-config/src/cas_server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -439,7 +439,7 @@ pub struct LocalWorkerConfig {
#[serde(default, deserialize_with = "convert_numeric_with_shellexpand")]
pub max_action_timeout: usize,

/// If timeout is handled in `entrypoint_cmd` or another wrapper script.
/// If timeout is handled in `entrypoint` or another wrapper script.
/// If set to true NativeLink will not honor the timeout the action requested
/// and instead will always force kill the action after max_action_timeout
/// has been reached. If this is set to false, the smaller value of the action's
Expand All @@ -449,7 +449,7 @@ pub struct LocalWorkerConfig {
/// The real timeout can be received via an environment variable set in:
/// `EnvironmentSource::TimeoutMillis`.
///
/// Example on where this is useful: `entrypoint_cmd` launches the action inside
/// Example on where this is useful: `entrypoint` launches the action inside
/// a docker container, but the docker container may need to be downloaded. Thus
/// the timer should not start until the docker container has started executing
/// the action. In this case, action will likely be wrapped in another program,
Expand All @@ -465,7 +465,17 @@ pub struct LocalWorkerConfig {
/// command like: "run.sh sleep 5".
/// Default: {Use the command from the job request}.
#[serde(default, deserialize_with = "convert_string_with_shellexpand")]
pub entrypoint_cmd: String,
pub entrypoint: String,

/// An optional script to run before every action is processed on the worker.
/// The value should be the full path to the script to execute and will pause
/// all actions on the worker if it returns an exit code other than 0.
/// If not set, then the worker will never pause and will continue to accept
/// jobs according to the scheduler configuration.
/// This is useful, for example, if the worker should not take any more
/// actions until there is enough resource available on the machine to
/// handle them.
pub experimental_precondition_script: Option<String>,

/// Underlying CAS store that the worker will use to download CAS artifacts.
/// This store must be a `FastSlowStore`. The `fast` store must be a
Expand Down Expand Up @@ -493,16 +503,6 @@ pub struct LocalWorkerConfig {
/// worker.
pub platform_properties: HashMap<String, WorkerProperty>,

/// An optional script to run before every action is processed on the worker.
/// The value should be the full path to the script to execute and will pause
/// all actions on the worker if it returns an exit code other than 0.
/// If not set, then the worker will never pause and will continue to accept
/// jobs according to the scheduler configuration.
/// This is useful, for example, if the worker should not take any more
/// actions until there is enough resource available on the machine to
/// handle them.
pub precondition_script: Option<String>,

/// An optional mapping of environment names to set for the execution
/// as well as those specified in the action itself. If set, will set each
/// key as an environment variable before executing the job with the value
Expand Down
10 changes: 5 additions & 5 deletions nativelink-worker/src/local_worker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ async fn preconditions_met(precondition_script: Option<String>) -> Result<(), Er
// so that's not currently possible. Perhaps we'll move this in
// future to pass useful information through? Or perhaps we'll
// have a pre-condition and a pre-execute script instead, although
// arguably entrypoint_cmd already gives us that.
// arguably entrypoint already gives us that.
let precondition_process = process::Command::new(precondition_script)
.kill_on_drop(true)
.stdin(Stdio::null())
Expand Down Expand Up @@ -221,7 +221,7 @@ impl<'a, T: WorkerApiClientTrait, U: RunningActionsManager> LocalWorkerImpl<'a,
.err_tip(|| "In LocalWorkerImpl::new()");
let running_actions_manager = self.running_actions_manager.clone();
let worker_id_clone = worker_id.clone();
let precondition_script_cfg = self.config.precondition_script.clone();
let precondition_script_cfg = self.config.experimental_precondition_script.clone();
let actions_in_transit = self.actions_in_transit.clone();
let start_action_fut = self.metrics.clone().wrap(move |metrics| async move {
metrics.preconditions.wrap(preconditions_met(precondition_script_cfg))
Expand Down Expand Up @@ -349,10 +349,10 @@ pub async fn new_local_worker(
fs::create_dir_all(&config.work_directory)
.await
.err_tip(|| format!("Could not make work_directory : {}", config.work_directory))?;
let entrypoint_cmd = if config.entrypoint_cmd.is_empty() {
let entrypoint = if config.entrypoint.is_empty() {
None
} else {
Some(config.entrypoint_cmd.clone())
Some(config.entrypoint.clone())
};
let max_action_timeout = if config.max_action_timeout == 0 {
DEFAULT_MAX_ACTION_TIMEOUT
Expand All @@ -362,7 +362,7 @@ pub async fn new_local_worker(
let running_actions_manager = Arc::new(RunningActionsManagerImpl::new(RunningActionsManagerArgs {
root_work_directory: config.work_directory.clone(),
execution_configuration: ExecutionConfiguration {
entrypoint_cmd,
entrypoint,
additional_environment: config.additional_environment.clone(),
},
cas_store: fast_slow_store,
Expand Down
6 changes: 3 additions & 3 deletions nativelink-worker/src/running_actions_manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -652,8 +652,8 @@ impl RunningActionImpl {
return Err(make_input_err!("No arguments provided in Command proto"));
}
let args: Vec<&OsStr> =
if let Some(entrypoint_cmd) = &self.running_actions_manager.execution_configuration.entrypoint_cmd {
std::iter::once(entrypoint_cmd.as_ref())
if let Some(entrypoint) = &self.running_actions_manager.execution_configuration.entrypoint {
std::iter::once(entrypoint.as_ref())
.chain(command_proto.arguments.iter().map(AsRef::as_ref))
.collect()
} else {
Expand Down Expand Up @@ -1218,7 +1218,7 @@ pub struct ExecutionConfiguration {
/// If set, will be executed instead of the first argument passed in the
/// ActionInfo with all of the arguments in the ActionInfo passed as
/// arguments to this command.
pub entrypoint_cmd: Option<String>,
pub entrypoint: Option<String>,
/// The only environment variables that will be specified when the command
/// executes other than those in the ActionInfo. On Windows, SystemRoot
/// and PATH are also assigned (see inner_execute).
Expand Down
4 changes: 2 additions & 2 deletions nativelink-worker/tests/local_worker_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -470,7 +470,7 @@ mod local_worker_tests {
}

#[tokio::test]
async fn precondition_script_fails() -> Result<(), Box<dyn std::error::Error>> {
async fn experimental_precondition_script_fails() -> Result<(), Box<dyn std::error::Error>> {
let temp_path = make_temp_path("scripts");
fs::create_dir_all(temp_path.clone()).await?;
#[cfg(target_family = "unix")]
Expand All @@ -497,7 +497,7 @@ mod local_worker_tests {
precondition_script
};
let local_worker_config = LocalWorkerConfig {
precondition_script: Some(precondition_script),
experimental_precondition_script: Some(precondition_script),
..Default::default()
};

Expand Down
10 changes: 5 additions & 5 deletions nativelink-worker/tests/running_actions_manager_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1321,7 +1321,7 @@ mod running_actions_manager_tests {
// print to stdout. We then check the results of both to make sure the shell script was
// invoked and the actual command was invoked under the shell script.
#[tokio::test]
async fn entrypoint_cmd_does_invoke_if_set() -> Result<(), Box<dyn std::error::Error>> {
async fn entrypoint_does_invoke_if_set() -> Result<(), Box<dyn std::error::Error>> {
#[cfg(target_family = "unix")]
const TEST_WRAPPER_SCRIPT_CONTENT: &str = "\
#!/bin/bash
Expand Down Expand Up @@ -1377,7 +1377,7 @@ exit 0
let running_actions_manager = Arc::new(RunningActionsManagerImpl::new(RunningActionsManagerArgs {
root_work_directory: root_work_directory.clone(),
execution_configuration: ExecutionConfiguration {
entrypoint_cmd: Some(test_wrapper_script.into_string().unwrap()),
entrypoint: Some(test_wrapper_script.into_string().unwrap()),
additional_environment: None,
},
cas_store: Pin::into_inner(cas_store.clone()),
Expand Down Expand Up @@ -1450,7 +1450,7 @@ exit 0
}

#[tokio::test]
async fn entrypoint_cmd_injects_properties() -> Result<(), Box<dyn std::error::Error>> {
async fn entrypoint_injects_properties() -> Result<(), Box<dyn std::error::Error>> {
#[cfg(target_family = "unix")]
const TEST_WRAPPER_SCRIPT_CONTENT: &str = "\
#!/bin/bash
Expand Down Expand Up @@ -1506,7 +1506,7 @@ exit 0
let running_actions_manager = Arc::new(RunningActionsManagerImpl::new(RunningActionsManagerArgs {
root_work_directory: root_work_directory.clone(),
execution_configuration: ExecutionConfiguration {
entrypoint_cmd: Some(test_wrapper_script.into_string().unwrap()),
entrypoint: Some(test_wrapper_script.into_string().unwrap()),
additional_environment: Some(HashMap::from([
(
"PROPERTY".to_string(),
Expand Down Expand Up @@ -1647,7 +1647,7 @@ exit 1
let running_actions_manager = Arc::new(RunningActionsManagerImpl::new(RunningActionsManagerArgs {
root_work_directory: root_work_directory.clone(),
execution_configuration: ExecutionConfiguration {
entrypoint_cmd: Some(test_wrapper_script.into_string().unwrap()),
entrypoint: Some(test_wrapper_script.into_string().unwrap()),
additional_environment: Some(HashMap::from([(
"SIDE_CHANNEL_FILE".to_string(),
EnvironmentSource::side_channel_file,
Expand Down

0 comments on commit dbe61d2

Please sign in to comment.