Arvados

Tom Clegg wrote in #note-10:

21123-scheduling-status @ 35d4eea994c98b4bb65111c9da6e57abbc7e014f -- developer-run-tests: #4068

This implementation looks good to me. My comments are all about documentation or support stuff.

doc/api/dispatch.html.textile.liquid¶

+* a @scheduling_status@ entry: a brief explanation of the container's status in the dispatch queue, or empty if scheduling is not applicable, e.g., the container has already started running.

In order to better match the prevailing style of the existing list elements, this would be better written as:

* a @scheduling_status@ entry with a brief explanation of the container's status in the dispatch queue, or empty if scheduling is not applicable (e.g., the container has already started running).

I also wonder what "empty" means. Empty string? null? Does not appear? (That's what it looks like from the code.) Clarification would be helpful.

doc/api/methods/container_requests.html.textile.liquid¶

Similar comment about "empty" in the scheduling_status table row.

Tests¶

Have you done hundreds of runs of the new/modified tests to check for race conditions in the new code?

lib/dispatchcloud/dispatcher.go¶

I will throw this out only once: instead of sQueue throughout, consider schedQueue, or if that's too long, schedQue or schedQ. Just looking at dispatcher's other fields, s could reference sched, ssh, setup or stop. You really need to know the type of sQueue for the rest to make sense, and I don't think that will always be at hand for different development tasks.

lib/dispatchcloud/scheduler/run_queue.go¶

I think it would be a nice style improvement to have all the possible SchedulingStatus strings together. As a reviewer, it would've been nice to check the text for consistent style. I think it will also be an aid to future developers as we add or edit reasons.

I'm imagining them all in a const block, but the details matter less than the principle of them being all together and not sprinkled within a 350-ish-line function definition.

services/api/config/routes.rb¶

Should the new method be added to the discovery document (with documentation)?

Brett Smith wrote in #note-11:

In order to better match the prevailing style of the existing list elements, this would be better written as:

[...]

Fixed.

I also wonder what "empty" means. Empty string? null? Does not appear? (That's what it looks like from the code.) Clarification would be helpful.

Similar comment about "empty" in the scheduling_status table row.

Fixed with "the empty string".

Then thought about it some more and replaced those (and a few other instances) with "an empty string".

Have you done hundreds of runs of the new/modified tests to check for race conditions in the new code?

Yes.

I will throw this out only once: instead of sQueue throughout, consider schedQueue, or if that's too long, schedQue or schedQ. Just looking at dispatcher's other fields, s could reference sched, ssh, setup or stop. You really need to know the type of sQueue for the rest to make sense, and I don't think that will always be at hand for different development tasks.

Good point. Updated to schedQueue*.

I think it would be a nice style improvement to have all the possible SchedulingStatus strings together. As a reviewer, it would've been nice to check the text for consistent style. I think it will also be an aid to future developers as we add or edit reasons.

Good point. Added consts.

Should the new method be added to the discovery document (with documentation)?

Good point. Added to discovery doc by adding a stub method in rails. The documentation string is "container_status container_request" which is weird but that's what we put in such doc strings and there isn't yet a mechanism for specifying a useful string. Should I add that now? Or change the template to "see doc.arvados.org/appropriate-methods-page"?

21123-scheduling-status @ 536888e31706dfb9db43338e858529a7432a13c1 -- developer-run-tests: #4076

I went yak shaving (sdk/python tests don't run on python3.9 any more → upgrade dev vm to bookworm → python is confused → delete VENV3DIR in temp dir → lib/controller tests fail → use journalctl to find crunch-run logs revealing the problem is arv-mount is not installed) and came back with this fix, to save myself/others the trouble next time:

diff --git a/build/run-tests.sh b/build/run-tests.sh
index 28e9e1cf7b..9ed3d32ae2 100755
--- a/build/run-tests.sh
+++ b/build/run-tests.sh
@@ -1058,6 +1058,7 @@ install_deps() {
     do_install sdk/ruby-google-api-client
     do_install sdk/ruby
     do_install services/api
+    do_install services/fuse pip "${VENV3DIR}/bin/" 
     do_install services/keepproxy go
     do_install services/keep-web go
 }

21123-scheduling-status @ 95dba0428816e5b53cde4f8b15ae283619c51d54 -- developer-run-tests: #4077

Tom Clegg wrote in #note-14:

I went yak shaving (sdk/python tests don't run on python3.9 any more → upgrade dev vm to bookworm → python is confused → delete VENV3DIR in temp dir → lib/controller tests fail → use journalctl to find crunch-run logs revealing the problem is arv-mount is not installed)

The fix seems fine but let's go back to that first part. I expect we'll be supporting Python 3.8 through at least the rest of the year and therefore personally I expect the tests to pass on Python 3.8 as well. And for me they do, except for #20909:

What next? test sdk/python:py3
[…]
Ran 936 tests in 306.861s

FAILED (failures=1, skipped=2)
Test failed: <unittest.runner.TextTestResult run=936 errors=0 failures=1>
error: Test failed: <unittest.runner.TextTestResult run=936 errors=0 failures=1>
======= sdk/python tests -- FAILED
======= test sdk/python -- 308s
Failures (1):
Fail: sdk/python tests (308s)
What next? [^D]
[…]
Leaving behind temp dirs in /home/brett/.cache/arvados-test
% ~/.cache/arvados-test/VENV3DIR/bin/python3 -V
Python 3.8.18

Can you please file a bug with what you were seeing?

The branch LGTM. Everything below is small nits, take what you want and then merge either way:

Tom Clegg wrote in #note-12:

Brett Smith wrote in #note-11:

In order to better match the prevailing style of the existing list elements, this would be better written as:

[...]

Fixed.

This still has the "e.g." example without parentheses. This is not a big deal but literally the previous bullet has it in parentheses. Bouncing styles like this is unfortunate.

Have you done hundreds of runs of the new/modified tests to check for race conditions in the new code?

Yes.

Awesome, thank you.

Should the new method be added to the discovery document (with documentation)?

Good point. Added to discovery doc by adding a stub method in rails. The documentation string is "container_status container_request" which is weird but that's what we put in such doc strings and there isn't yet a mechanism for specifying a useful string. Should I add that now? Or change the template to "see doc.arvados.org/appropriate-methods-page"?

I think my vote is we plan to to add a mechanism for more useful strings as part of #19929. If you have ideas for how to do that, can you please add those in a comment on the ticket?

Re the FUSE fix: The comment in your commit message is great. I think it would be even more valuable as a comment in the code directly. Like I said, please let us know what issue you saw running tests on Py3.9.

Thanks.