Skip to content
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.

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

[rush] experimental: Rush Operation Resource Plugin #5094

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

elliot-nelson
Copy link
Collaborator

Summary

Add a new plugin to the rush-plugins folder. The Operation Resource Plugin allows a configuration file to constrain parallelism on specified combinations of tags, projects, and phases, and optionally to require a specific resource from a limited list of resources to be passed down to the underlying operation script.

Details

Check out the provided README for suggested usage and implementation details.

How it was tested

  • No regressions in rushstack repo (builds/tests pass)
  • Installed and configured in local monorepos and tested with various combinations of tag names / operation names.

Impacted documentation

  • N/A

Copy link
Contributor

@dmichon-msft dmichon-msft left a comment

Choose a reason for hiding this comment

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

I like the concept; in practice this is just a generalization of the cobuild machinery (cobuild locks are an example of a resource) so it'd be beneficial to converge it.


For example, you might add a line like this before running `rush` in CI/CD:

```bash
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this can technically alter build cache computations, I think it'd be better to inject an additional command line parameter to specify the resource configuration file. rush-serve-plugin shows an example of how such a capability can be used, i.e. have the plugin configuration file name the command line parameter, and the developer inject the parameter into the command-line.json file as a parameter that is associated with the command but not with the individual phases.


Instead, this plugin makes use of Rush's built-in phased operation hooks to _delay_ the start of a given task if it meets certain criteria. This approach has pros and cons:

- One con is that this approach is not always _optimal_. A given operation may need to be delayed because it requires a resource where none is available, and it will simply wait for another operation to finish so it can start. An optimal implementation would pause this operation and run a _different_ operation, perhaps one with no resource constraints.
Copy link
Contributor

Choose a reason for hiding this comment

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

You could leverage beforeExecuteOperation with early return to let the scheduler schedule something else.

const pool: IResourcePool = this._resourcePools.get(constraint.resourcePool.poolName)!;

// Wait to obtain a resource from the resource pool for this constraint
const resource: string = await this._obtainResource(pool, runnerContext.operation);
Copy link
Contributor

Choose a reason for hiding this comment

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

You can ensure proper prioritization if you separate this into a tryObtainResource and obtainResource. If you fail to obtain the resource immediately, return OperationStatus.Waiting, and when you successfully obtain the resource, set the status back to OperationStatus.Ready. You'll want to do a tiny bit of extra tracking so that when the operation gets requeued it picks up the already reserved resource, but that's straightforward.

Alternatively, you can skip the tryObtainResource and always have an operation initially return OperationStatus.Waiting, unless your synchronous cache of assigned resources says the operation already has one assigned, which is pretty much the same thing.

if (constraint.resourcePool.envVarName) {
const runner: any = runnerContext.operation.runner;
if (!runner.environment) {
runner.environment = {};
Copy link
Contributor

Choose a reason for hiding this comment

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

Is runner.environment already part of the specification for IOperationRunner? If not, I can see a number of uses for it; for example I've been playing a lot with cpu profiling lately and being able to customize NODE_OPTIONS per operation via plugin would be handy.

That said, probably safer to have environment be calculated via a dedicated getEnvironmentForOperation hook that multiple plugins can tap and thereby augment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Makes sense -- as the :any probably gives away, this part is a TOTAL hack, I'm not even checking right now (although I'm assuming) that it is a ScriptOperation haha.

runnerContext.operation
);

if (constraint) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest having your resource assignments implement the [Symbol.disposable](): void contract and just store a WeakMap<Operation, IDisposable[]> of the resources that have been assigned to each operation, then in this step, iterate and dispose all resources, then delete the map entry.

@dmichon-msft
Copy link
Contributor

dmichon-msft commented Feb 7, 2025

The hook I'm adding in #5099 should be able to be used here to provide the relevant variables to the runner.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

2 participants