Skip to content

Map RUNNER_TEMP for container action #4011

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ericsciple merged 1 commit into from
Sep 3, 2025
Merged

Map RUNNER_TEMP for container action #4011

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions src/Runner.Common/Constants.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,7 @@ public static class Features
public static readonly string AllowRunnerContainerHooks = "DistributedTask.AllowRunnerContainerHooks";
public static readonly string AddCheckRunIdToJobContext = "actions_add_check_run_id_to_job_context";
public static readonly string DisplayHelpfulActionsDownloadErrors = "actions_display_helpful_actions_download_errors";
public static readonly string ContainerActionRunnerTemp = "actions_container_action_runner_temp";
}

// Node version migration related constants
Expand Down
5 changes: 5 additions & 0 deletions src/Runner.Worker/FeatureManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,10 @@ public static bool IsContainerHooksEnabled(Variables variables)
var isContainerHooksPathSet = !string.IsNullOrEmpty(Environment.GetEnvironmentVariable(Constants.Hooks.ContainerHooksPath));
return isContainerHookFeatureFlagSet && isContainerHooksPathSet;
}

public static bool IsContainerActionRunnerTempEnabled(Variables variables)
{
return variables?.GetBoolean(Constants.Runner.Features.ContainerActionRunnerTemp) ?? false;
}
}
}
8 changes: 8 additions & 0 deletions src/Runner.Worker/Handlers/ContainerActionHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,11 +191,19 @@ public async Task RunAsync(ActionRunStage stage)
ArgUtil.Directory(tempWorkflowDirectory, nameof(tempWorkflowDirectory));

container.MountVolumes.Add(new MountVolume("/var/run/docker.sock", "/var/run/docker.sock"));
if (FeatureManager.IsContainerActionRunnerTempEnabled(ExecutionContext.Global.Variables))
Copy link
Contributor

@salmanmkc salmanmkc Sep 3, 2025

Choose a reason for hiding this comment

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

Should we do this also with environment variables? So that we don't have to rely on just feature flags? Same comment for the check below too

Also nit: do we need the helper method since it's a one line helper method?

Copy link
Collaborator Author

@ericsciple ericsciple Sep 3, 2025

Choose a reason for hiding this comment

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

The feature flag is temporary for safe rollout only. It will be cleaned up in the following runner release.

I didn't have a reason to add an environment variable check in this case, so I kept it clean and under server control. Environment variables help when customers are involved to help try out a pre-release behavior and confirm it works for their scenarios.

The only reason I added the helper method is to make this file read consistently wrt feature flag checks.

{
container.MountVolumes.Add(new MountVolume(tempDirectory, "/github/runner_temp"));
}
container.MountVolumes.Add(new MountVolume(tempHomeDirectory, "/github/home"));
container.MountVolumes.Add(new MountVolume(tempWorkflowDirectory, "/github/workflow"));
container.MountVolumes.Add(new MountVolume(tempFileCommandDirectory, "/github/file_commands"));
container.MountVolumes.Add(new MountVolume(defaultWorkingDirectory, "/github/workspace"));

if (FeatureManager.IsContainerActionRunnerTempEnabled(ExecutionContext.Global.Variables))
Copy link
Contributor

@salmanmkc salmanmkc Sep 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

nit : Could we reorder this so that there's just one feature flag check call? The also might help reduce the need for a helper method. Or does the order of operations here matter a lot? E.g. putting the mount of the new temp directory in the feature flag call at the end of the mounting section and then path translate at the beginning of the mapping section?

Sorry quite a picky nit, feel free to ignore this, not sure on whether it makes a difference to the behavior

Edit: also this would then make it easier to do that check plus an env variable check if added, since we'd be able to just check them all once.

Alternatively we can just store the ff value above and then use it.

Copy link
Collaborator Author

@ericsciple ericsciple Sep 3, 2025

Choose a reason for hiding this comment

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

I optimized the code, to make the diff easier to read when the feature flag is removed (future PR).

I prefer checking the feature flag twice rather than introduce a local variable. Rule of thumb: more local variables -> worse readability. Checking the feature flag isn't expensive, it's a dictionary lookup.

{
container.AddPathTranslateMapping(tempDirectory, "/github/runner_temp");
}
container.AddPathTranslateMapping(tempHomeDirectory, "/github/home");
container.AddPathTranslateMapping(tempWorkflowDirectory, "/github/workflow");
container.AddPathTranslateMapping(tempFileCommandDirectory, "/github/file_commands");
Expand Down
Loading