Skip to content

Commit

Permalink
remove unnecessary execution env vars (#4806)
Browse files Browse the repository at this point in the history
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

- **Refactor**
- Removed environment variable tracking for `NODE_ID`, `JOB_NAME`, and
`JOB_NAMESPACE` in job execution.
- Updated test cases to remove node-specific verifications and focus on
partition-specific outputs.
- Adjusted expected outputs in tests to reflect changes in environment
variable priorities.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
  • Loading branch information
wdbaruni authored Jan 12, 2025
1 parent 7bd50cc commit 5003f7d
Show file tree
Hide file tree
Showing 3 changed files with 5 additions and 32 deletions.
3 changes: 0 additions & 3 deletions pkg/compute/envvars.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,10 @@ func GetExecutionEnvVars(execution *models.Execution) map[string]string {
// Build system environment variables
sysEnv := make(map[string]string)
sysEnv[models.EnvVarPrefix+"EXECUTION_ID"] = execution.ID
sysEnv[models.EnvVarPrefix+"NODE_ID"] = envvar.Sanitize(execution.NodeID)

// Add job-related environment variables if job exists
if execution.Job != nil {
sysEnv[models.EnvVarPrefix+"JOB_ID"] = execution.JobID
sysEnv[models.EnvVarPrefix+"JOB_NAME"] = envvar.Sanitize(execution.Job.Name)
sysEnv[models.EnvVarPrefix+"JOB_NAMESPACE"] = envvar.Sanitize(execution.Job.Namespace)
sysEnv[models.EnvVarPrefix+"JOB_TYPE"] = execution.Job.Type

// Add partition-related environment variables
Expand Down
16 changes: 3 additions & 13 deletions pkg/compute/envvars_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ func TestGetExecutionEnvVars(t *testing.T) {
},
want: map[string]string{
"BACALHAU_EXECUTION_ID": "exec-1",
"BACALHAU_NODE_ID": "node-1",
},
},
{
Expand All @@ -53,10 +52,7 @@ func TestGetExecutionEnvVars(t *testing.T) {
},
want: map[string]string{
"BACALHAU_EXECUTION_ID": "exec-1",
"BACALHAU_NODE_ID": "node-1",
"BACALHAU_JOB_ID": "job-1",
"BACALHAU_JOB_NAME": "test-job",
"BACALHAU_JOB_NAMESPACE": "default",
"BACALHAU_JOB_TYPE": "batch",
"BACALHAU_PARTITION_INDEX": "0",
"BACALHAU_PARTITION_COUNT": "3",
Expand All @@ -77,9 +73,9 @@ func TestGetExecutionEnvVars(t *testing.T) {
{
Name: "task-1",
Env: map[string]string{
"MY_VAR": "my-value",
"BACALHAU_NODE_ID": "should-not-override", // Should not override system env
"OTHER_VAR": "other-value",
"MY_VAR": "my-value",
"BACALHAU_JOB_TYPE": "should-not-override", // Should not override system env
"OTHER_VAR": "other-value",
},
},
},
Expand All @@ -88,10 +84,7 @@ func TestGetExecutionEnvVars(t *testing.T) {
},
want: map[string]string{
"BACALHAU_EXECUTION_ID": "exec-1",
"BACALHAU_NODE_ID": "node-1", // System value takes precedence
"BACALHAU_JOB_ID": "job-1",
"BACALHAU_JOB_NAME": "test-job",
"BACALHAU_JOB_NAMESPACE": "default",
"BACALHAU_JOB_TYPE": "batch",
"BACALHAU_PARTITION_INDEX": "0",
"BACALHAU_PARTITION_COUNT": "3",
Expand Down Expand Up @@ -120,10 +113,7 @@ func TestGetExecutionEnvVars(t *testing.T) {
},
want: map[string]string{
"BACALHAU_EXECUTION_ID": "exec-1",
"BACALHAU_NODE_ID": "node-1",
"BACALHAU_JOB_ID": "job-1",
"BACALHAU_JOB_NAME": "test_job_with_spaces", // Sanitized
"BACALHAU_JOB_NAMESPACE": "test_namespace", // Sanitized
"BACALHAU_JOB_TYPE": "batch",
"BACALHAU_PARTITION_INDEX": "0",
"BACALHAU_PARTITION_COUNT": "1",
Expand Down
18 changes: 2 additions & 16 deletions pkg/test/devstack/partitioning_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,10 +62,7 @@ func (s *PartitionSuite) TestSinglePartition() {
[]string{
"BACALHAU_EXECUTION_ID=",
"BACALHAU_JOB_ID=",
"BACALHAU_JOB_NAME=" + s.T().Name(),
"BACALHAU_JOB_NAMESPACE=default",
"BACALHAU_JOB_TYPE=batch",
"BACALHAU_NODE_ID=node-0",
"BACALHAU_PARTITION_COUNT=1",
"BACALHAU_PARTITION_INDEX=0",
},
Expand Down Expand Up @@ -98,7 +95,7 @@ func (s *PartitionSuite) TestMultiplePartitions() {
Name: s.T().Name(),
Engine: dockmodels.NewDockerEngineBuilder("busybox:1.37.0").
WithEntrypoint("sh", "-c",
"echo Node=${BACALHAU_NODE_ID} Partition=${BACALHAU_PARTITION_INDEX} of ${BACALHAU_PARTITION_COUNT}",
"echo Partition=${BACALHAU_PARTITION_INDEX} of ${BACALHAU_PARTITION_COUNT}",
).
MustBuild(),
Publisher: publisher_local.NewSpecConfig(),
Expand All @@ -116,16 +113,6 @@ func (s *PartitionSuite) TestMultiplePartitions() {
},
-1,
),
// Verify they run on the nodes
scenario.FileContains(
"stdout",
[]string{
"Node=node-0",
"Node=node-1",
"Node=node-2",
},
-1,
),
),
JobCheckers: []scenario.StateChecks{
scenario.WaitForSuccessfulCompletion(),
Expand Down Expand Up @@ -155,9 +142,8 @@ func (s *PartitionSuite) TestPartitionRetry() {
ExternalHooks: noop.ExecutorConfigExternalHooks{
JobHandler: func(ctx context.Context, execContext noop.ExecutionContext) (*models.RunCommandResult, error) {
partition := execContext.Env["BACALHAU_PARTITION_INDEX"]
nodeID := execContext.Env["BACALHAU_NODE_ID"]

output := fmt.Sprintf("Running partition %s on node %s\n", partition, nodeID)
output := fmt.Sprintf("Running partition %s\n", partition)

// Only increment attempt counter for partition 1
var currentAttempt int32
Expand Down

0 comments on commit 5003f7d

Please sign in to comment.