Add a configuration item to limit parallelism

[vladaionescu]

A value of 0 disables the feature (default).

CC @tonistiigi

Signed-off-by: Vlad A. Ionescu [email protected]

[AkihiroSuda]

Can we have tests?

It would be great if we can also have this as a buildctl build option, but that can be another PR.

[tonistiigi]

I thought something per-build as well but maybe it is better to leave that to a better solution that can monitor resource usage.

I guess for the test we could do 2 parallel sleep runs that print a start/end date and check that they are sequential if this config is set to 1. Maybe there is something better.

[vladaionescu]

Sounds good. Got swamped with a few other things. Will take a look next week.

[vladaionescu]

I've added a draft test to this (WIP). I can't figure out how to set the buildkit configuration as part of an integration test. Do you have any pointers to some test code doing something like that?

[tonistiigi]

@vladaionescu Look at how the secmode/netmode is implemented https://github.com/moby/buildkit/blob/master/client/client_test.go#L3485-L3495

[vladaionescu]

Ok test is done - thanks for bearing with me. Ready for another look!

[tonistiigi]

Bit weird to pass the semaphore object as a opt in here rather than the number. Any reason for it?

[vladaionescu]

Main reason was to keep the interpretation of the number 0 in the same place where the config is interpreted. Not a strong opinion though.

[vladaionescu]

This is now ready for another look.

Maybe both processes create a file and than wait for up to 5 sec for it to get deleted by the other process

I ended up using a touch rather than a rm, but the idea is the same. I noticed that the test isn't stable with 5s so I left it with 10s.

[vladaionescu]

Created follow-up proposal that uses Aquire() method here #2108 in order to keep resource controls ops specific, not scheduler specific. If you agree with that reasoning, can you change this signature already.

Sounds good - I've switched over to the new Acquire() and had to put the semaphore in WorkerOpt as a result - which probably makes more sense as it makes the semaphore per-worker. The semaphore can easily be swapped out for the resource manager in the future.

It's possible to move the max-parallelism setting within the worker config instead of the root. If that's preferrable, I might need some guidance on how to adjust the integration test to edit the worker config rather than the root config.

[tonistiigi]

It's possible to move the max-parallelism setting within the worker config instead of the root.

Didn't think of this but might even make sense to put it under worker. Eg. if you have multiple workers and one is a very big machine and another one is small. You may want to have different configs for them.

[tonistiigi]

Where is the new test running in CI? I think we might be skipping it

[vladaionescu]

Ok ready for another look.

Didn't think of this but might even make sense to put it under worker.

Done.

Where is the new test running in CI? I think we might be skipping it

I just added the ./solver package to GHA. Here's the new test passing: https://github.com/moby/buildkit/runs/2575149561?check_suite_focus=true#step:7:734