Conversation
shioyama
changed the base branch from
main
to
cbruckmayer/check-we-executed-all-tests
shioyama
force-pushed
the
shioyama/parallel_ci_queue
branch
3 times, most recently
from
8a1e3eb to
b1203e0
Compare
shioyama
changed the title
Minitest.__run patch internals to Minitest::Queue.run and remove rate limit on reserved tests
|
|
||
| def initialize(redis, config) | ||
| @reserved_test = nil | ||
| @reserved_tests = Set.new |
There was a problem hiding this comment.
I guess it would be difficult to send the process ID back up and scope by that?
There was a problem hiding this comment.
Hmm, not sure that would be possible 🤔
We have a master process that is reserving tests, which forked processes pull from. The master process fills the queue as soon as it has less than the max (currently 1).
When we are reserving a test, therefore, we don't know which process will end up running it, so we can't check for a "mismatch" at that point.
However, we might be able to improve the check in acknowledge such that when a worker process takes a test (pops it from the server via drb), we track its process ID and associate it with the reserved test. Then in theory when the result comes back, we could check that the reserved test exists, and that the pid we associated with it matches the worker that is trying to record the result.
I'm not sure that we can get the pid though from a request to the drb server. 🤔
There was a problem hiding this comment.
Oh never mind we can just make the worker send its pid.
There was a problem hiding this comment.
Ok so we can indeed check that the worker recording a result for a test matches the pid for the worker that asked for that same test. I don't think it would be easy to do that on the gem side of this, though. And I'm not really convinced that having that check would do much for us.
OTOH I don't see how we can do anything about reserve, given that when a server process reserves a test, it doesn't know which worker will end up taking it. So there's no check to be made at that moment.
shioyama
force-pushed
the
shioyama/parallel_ci_queue
branch
2 times, most recently
from
2521f8c to
65bfaed
Compare
ruby/lib/ci/queue/redis/worker.rb
Outdated
|
|
||
| def initialize(redis, config) | ||
| @reserved_test = nil | ||
| def initialize(redis, config, max_reserved_tests: 1) |
There was a problem hiding this comment.
@ChrisBr I added a parameter we can use to ensure we are not reserving more tests than we allow in the in-memory queue. This is fairly easy and I think would catch the obvious issues? It would be a duplicate check since we already check this in the parallelization server but can't be too careful.
shioyama
force-pushed
the
shioyama/parallel_ci_queue
branch
from
47ff8be to
65bfaed
Compare
|
Ok I extracted the stuff not related to reserving tests to #337, which should be uncontroversial. Then we can deal with the change to reserved tests on its own. |
shioyama
changed the title
Minitest.__run patch internals to Minitest::Queue.run and remove rate limit on reserved tests
shioyama
force-pushed
the
shioyama/parallel_ci_queue
branch
from
65bfaed to
b8cb23b
Compare
|
Updated this PR to only include the part about reserving tests; the rest is in #337. |
ruby/lib/ci/queue/redis/worker.rb
Outdated
| if reserved_tests.include?(test) | ||
| reserved_tests.delete(test) | ||
| else | ||
| raise ReservationError, "Acknowledged #{test.inspect} but #{@reserved_test.inspect} was reserved" | ||
| raise ReservationError, "Acknowledged #{test.inspect} but only #{reserved_tests.map(&:inspect).join(", ")} reserved" |
There was a problem hiding this comment.
unless reserved_tests.delete?(test)
raise ...
end
shioyama
force-pushed
the
shioyama/parallel_ci_queue
branch
from
2f0f332 to
3035c5f
Compare
shioyama
changed the base branch from
cbruckmayer/check-we-executed-all-tests
to
main
|
Rebased this on |
3 participants
Currently, ci-queue only allows one test to be reserved at a time. We are attempting to implement test parallelism within ci-queue jobs, which requires that the server process reserves more than this.
I've made some changes here to support that change:
reserved_testas a set instead of a single test (renamed toreserved_tests)acknowledgeandrequeue, not onreservesince there is nothing to check when reserving a test.I've implemented this for everything, i.e. without some configuration to switch it on and off. This simplifies things a lot, because I don't think ensuring that we're not reserving when a test has already been reserved is actually doing much atm. But maybe I'm missing something.