docker, core, editoast: add mode single-worker for all infra #9166

bougue-pe · 2024-10-03T08:09:53Z

Also:

editoast: remove http core client
cleanup unavailable envvar OSRD_BACKEND_URL

🔍 please review by commit + one more developper should test it (integration + e2e + regular use)
This work may probably be improved (doc, script, etc.) any suggestion is welcome.

Fix: #8599

codecov-commenter · 2024-10-03T08:12:10Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 3.84615% with 25 lines in your changes missing coverage. Please review.

Project coverage is 39.14%. Comparing base (e4eef48) to head (d60f674).
Report is 9 commits behind head on dev.

Files with missing lines	Patch %	Lines
...re/src/main/java/fr/sncf/osrd/cli/WorkerCommand.kt	0.00%	12 Missing ⚠️
editoast/src/core/mq_client.rs	0.00%	10 Missing ⚠️
editoast/src/client/mod.rs	0.00%	2 Missing ⚠️
editoast/src/core/mod.rs	0.00%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@             Coverage Diff              @@
##                dev    #9166      +/-   ##
============================================
+ Coverage     39.02%   39.14%   +0.11%     
  Complexity     2245     2245              
============================================
  Files          1289     1289              
  Lines         97319    97218     -101     
  Branches       3280     3280              
============================================
+ Hits          37981    38054      +73     
+ Misses        57399    57225     -174     
  Partials       1939     1939

Flag	Coverage Δ
core	`74.92% <0.00%> (-0.02%)`	⬇️
editoast	`72.46% <7.14%> (+0.02%)`	⬆️
front	`10.33% <ø> (+0.20%)`	⬆️
gateway	`2.50% <ø> (+0.30%)`	⬆️
osrdyne	`3.52% <ø> (ø)`
railjson_generator	`87.49% <ø> (ø)`
tests	`86.71% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

flomonster · 2024-10-03T08:44:33Z

I did not reviewed it yet but I have a small question.

This features seems useful for debuging purpose (see #8599). I believe we're talking of core? If so I don't see why we should handle this feature using docker compose.

eckter

LGTM for core with one minor comment.

Not tested yet, I'll probably come back to it later.

eckter · 2024-10-03T08:52:12Z

core/src/main/java/fr/sncf/osrd/cli/WorkerCommand.kt

+        WORKER_ID = System.getenv("WORKER_ID") ?: (if (ALL_INFRA) "all_infra_worker" else null)
+        WORKER_KEY = System.getenv("WORKER_KEY") ?: (if (ALL_INFRA) "all" else null)


This sets the priority as "env variable > all infra > null". I'd prefer if we used "all infra > env variable > null". It's quite minor but that way we could change a single flag to enable the "all infra" mode, instead of having to also unset the worker key/id.

Suggested change

WORKER_ID = System.getenv("WORKER_ID") ?: (if (ALL_INFRA) "all_infra_worker" else null)

WORKER_KEY = System.getenv("WORKER_KEY") ?: (if (ALL_INFRA) "all" else null)

WORKER_ID = if (ALL_INFRA) "all_infra_worker" else System.getenv("WORKER_ID")

WORKER_KEY = if (ALL_INFRA) "all" else System.getenv("WORKER_KEY")

I went for the safety on regular use, but it's fine to change.
Any reaction to that is welcome to make a "definitive" choice: current choice is to go for @eckter's suggestion.

Ah, I remember, I also wanted to let a possible choice of the worker_key name, which is not consistent with the "all infra" naming here.

The namings using "all" are about the targeted usage more than what it actually does/permit: no pre-load + work attached to just a grouping key, no matter the infra(s) being used.
Not sure it is useful for now or in the future to change/rename so I went for your suggestion in 302e4ef after upvotes.

bougue-pe · 2024-10-03T08:57:08Z

I did not reviewed it yet but I have a small question.

This features seems useful for debuging purpose (see #8599). I believe we're talking of core? If so I don't see why we should handle this feature using docker compose.

Compose file here is more of a helper/documentation to plug all together, as editoast also needs some specific param.
And I tried to put the params/envvar at the best place (IMO 😅) to enable mutualization when reusing core in another compose.

younesschrifi

LGTM ! Didn't tested it yet. I'm waiting for your rebase to do so and then I'll approve the PR👍🏽

woshilapin

Looks good although, as discussed, it will make us not really use the full complexity of osrdyne on local, might not help to detect possible bugs in it. That said, all for making the developer experience smoother.

⚠️ not tested since I still didn't find the time to handle osrdyne that depends on a Docker daemon that I don't have (using podman, daemon less).

docker/docker-compose.host.yml

woshilapin · 2024-10-03T12:40:32Z

editoast/src/core/mod.rs

+    pub async fn new_mq(
+        uri: String,
+        worker_pool_identifier: String,
+        timeout: u64,
+        single_worker: bool,
+    ) -> Result<Self> {


Would it make sense that this function takes directly a mq_client::Options as an input argument? I'm just triggered by the increasing number of arguments.

Not convinced by the benefits: on the only caller place, we would just embed the mq_client::Options creation, revealing more of the internals (and adding little more code/import).
Not strictly opposed to it, but not so fond of the idea here.

I'll be helpful to settle the debate: I'm fine with both :p

Is revealing the internal really a problem? It will make calling the function more explicit. Compare these, for example:

new_mq("foobar", "yolo", 42, false);

As a reader of the code, I will therefore need to go consult the function to understand what each of these parameters mean (which in Github might not be trivial, so I may need to pull the code, to benefit from rust-analyzer).

Compared to this:

new_mq(mq_client::Options { uri: "foobar", worker_pool_identifier: "yolo", timeout: 42, single_worker: false });

Sure it's more verbose, but more verbose and more explicit it's not necessarily bad if you think that code is read more often than it's written. In some languages, you can name the parameter when calling but sadly, Rust doesn't allow that (there has been discussion about it).

⚠️ This comment is long, because I wanted to be explicit about why I tend to be averse to a lot of arguments in a function. For functions with only few arguments, usually, the name of the function is enough to understand what the 1 or 2 arguments are, without naming them. That said, 4 is not yet a lot of arguments, just enough to trigger me. So I'm fine to keep it as it is, feel free to resolve whatever you do.

Convinced by the "named params": 52ffad3#diff-09ec5a58d69869bd41dba8612f5c3973f7bbe0147510aa5f2a1a44632ef30903R417-R422

leovalais

Lgtm, not tested

ElysaSrc · 2024-10-07T06:54:54Z

I do not have anything to say about the PR per se, but I'm curious on why we would want to make this change the default behavior ?

What's the rationale behind this ?

bougue-pe · 2024-10-07T13:13:09Z

@ElysaSrc there is a bit more info in the related issue #8599 but you may already have had a look and I'm not sure about your question.

The main purpose is to ease debug of core (mainly without having to spawn a new core and track the id of the infra which can be painful, like for integration tests).
The solution provided is OK for the business logic and hijacks part of the ops/architecture part, so it is limited in that regard (but a bit less than switching back to the web api).

Then the rationale of changing the noopdyne compose file is to consider that using dyne as noop is (almost?) only for that use case, so we might as well add the "changes" to editoast and core conf on the way to ease/document that case.
So the use case (which I didn't test completly, as I realized few minutes ago) would be:

compose up the whole stack
down the component(s) that I want to debug/replace locally
spawn locally those components
<do actual test/use>

Hope that it answers your question, and maybe it's worth adding a bit of doc and a bit of description (will try to after more test, but any suggestion is welcome).

ElysaSrc · 2024-10-07T13:16:16Z

@ElysaSrc there is a bit more info in the related issue #8599 but you may already have had a look and I'm not sure about your question.

The main purpose is to ease debug of core (mainly without having to spawn a new core and track the id of the infra which can be painful, like for integration tests).
The solution provided is OK for the business logic and hijacks part of the ops/architecture part, so it is limited in that regard (but a bit less than switching back to the web api).

Then the rationale of changing the noopdyne compose file is to consider that using dyne as noop is (almost?) only for that use case, so we might as well add the "changes" to editoast and core conf on the way to ease/document that case.
So the use case (which I didn't test completly, as I realized few minutes ago) would be:

compose up the whole stack

down the component(s) that I want to debug/replace locally

spawn locally those components

<do actual test/use>

Hope that it answers your question, and maybe it's worth adding a bit of doc and a bit of description (will try to after more test, but any suggestion is welcome).

Thanks for the detailed response !

Erashin

Tested for core debug purposes. Works as intended. Good job :)

Khoyo

Great work!

Should we also rename noopdyne as single-worker? It wasn't a great name, and I mostly named it that way because it wasn't a single-worker mode ^^'

Khoyo · 2024-10-11T11:21:43Z

core/src/main/java/fr/sncf/osrd/cli/WorkerCommand.kt

-        WORKER_ID = System.getenv("WORKER_ID")
-        WORKER_KEY = System.getenv("WORKER_KEY")
+        ALL_INFRA = !System.getenv("ALL_INFRA").isNullOrEmpty()
+        WORKER_ID = if (ALL_INFRA) "all_infra_worker" else System.getenv("WORKER_ID")


pretty sure this will conflict, could you rebase?

It did, ended up refactoring a bit through a getBooleanEnvvar fun 52ffad3#diff-7f5b2e605ea8cd19fd586f8d299a533f45c5419298dc3b16aca328645dee196aR75

Khoyo · 2024-10-11T11:23:14Z

docker-compose.yml

+    environment:
+      # Should match the reference, see. ./docker/osrdyne.yml
+      CORE_EDITOAST_URL: "http://osrd-editoast"
+      JAVA_TOOL_OPTIONS: "-javaagent:/app/opentelemetry-javaagent.jar"
+      CORE_MONITOR_TYPE: "opentelemetry"
+      OTEL_EXPORTER_OTLP_TRACES_PROTOCOL: "grpc"
+      OTEL_EXPORTER_OTLP_TRACES_ENDPOINT: "http://jaeger:4317"
+      OTEL_METRICS_EXPORTER: "none"
+      OTEL_LOGS_EXPORTER: "none"


I think directly providing the reference in ./docker/osrdyne.yml is a better idea... Or at least make clear that those are just for documentation, and that ./docker/osrdyne.yml is not the reference, but the actually used values.

Hopefully improved in 52ffad3#diff-e45e45baeda1c1e73482975a664062aa56f20c03dd9d64a827aba57775bed0d3R70-R71

Also cleanup unavailable envvar OSRD_BACKEND_URL Signed-off-by: Pierre-Etienne Bougué <[email protected]>

Signed-off-by: Pierre-Etienne Bougué <[email protected]>

…infra

…or all infra

…orker for all infra

bougue-pe · 2024-10-11T19:39:35Z

Renamed in 2329db4 & d60f674:

osrd-compose.sh to host-compose.sh
docker-compose.noopdyne.yml to docker-compose.single-worker.yml

Also added a dedicated single-worker-compose.sh script to wrap this up with a bit more doc.

bougue-pe requested review from Khoyo, woshilapin, leovalais, clarani and ElysaSrc October 3, 2024 08:09

bougue-pe requested review from a team as code owners October 3, 2024 08:09

github-actions bot added area:core Work on Core Service area:editoast Work on Editoast Service labels Oct 3, 2024

bougue-pe requested a review from Erashin October 3, 2024 08:10

eckter reviewed Oct 3, 2024

View reviewed changes

younesschrifi reviewed Oct 3, 2024

View reviewed changes

woshilapin approved these changes Oct 3, 2024

View reviewed changes

bougue-pe force-pushed the peb/osrdyne/one_core_for_all branch from c23a2d7 to e7b8258 Compare October 3, 2024 13:46

bougue-pe requested a review from younesschrifi October 3, 2024 14:02

leovalais approved these changes Oct 3, 2024

View reviewed changes

ElysaSrc approved these changes Oct 7, 2024

View reviewed changes

Erashin approved these changes Oct 9, 2024

View reviewed changes

bougue-pe force-pushed the peb/osrdyne/one_core_for_all branch from 302e4ef to 56cee29 Compare October 11, 2024 07:03

Khoyo reviewed Oct 11, 2024

View reviewed changes

bougue-pe added 2 commits October 11, 2024 16:33

docker, core, editoast: add mode single-worker for all infra

a35260c

Also cleanup unavailable envvar OSRD_BACKEND_URL Signed-off-by: Pierre-Etienne Bougué <[email protected]>

editoast: remove http core client

e36fcd3

Signed-off-by: Pierre-Etienne Bougué <[email protected]>

bougue-pe added 4 commits October 11, 2024 16:41

fixup! docker, core, editoast: add mode single-worker for all infra

b8390e7

fixup! fixup! docker, core, editoast: add mode single-worker for all …

52ffad3

…infra

fixup! fixup! fixup! docker, core, editoast: add mode single-worker f…

2329db4

…or all infra

fixup! fixup! fixup! fixup! docker, core, editoast: add mode single-w…

d60f674

…orker for all infra

bougue-pe force-pushed the peb/osrdyne/one_core_for_all branch from 56cee29 to d60f674 Compare October 11, 2024 19:32

bougue-pe requested a review from a team as a code owner October 11, 2024 19:32

github-actions bot added the area:front Work on Standard OSRD Interface modules label Oct 11, 2024

bougue-pe requested a review from Khoyo October 11, 2024 19:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docker, core, editoast: add mode single-worker for all infra #9166

docker, core, editoast: add mode single-worker for all infra #9166

bougue-pe commented Oct 3, 2024 •

edited

Loading

codecov-commenter commented Oct 3, 2024 •

edited

Loading

flomonster commented Oct 3, 2024

eckter left a comment

eckter Oct 3, 2024

bougue-pe Oct 3, 2024

bougue-pe Oct 3, 2024 •

edited

Loading

bougue-pe commented Oct 3, 2024

younesschrifi left a comment •

edited

Loading

woshilapin left a comment •

edited

Loading

woshilapin Oct 3, 2024

bougue-pe Oct 3, 2024

leovalais Oct 3, 2024

woshilapin Oct 4, 2024

bougue-pe Oct 11, 2024 •

edited

Loading

leovalais left a comment

ElysaSrc commented Oct 7, 2024

bougue-pe commented Oct 7, 2024 •

edited

Loading

ElysaSrc commented Oct 7, 2024

Erashin left a comment •

edited

Loading

Khoyo left a comment •

edited

Loading

Khoyo Oct 11, 2024

bougue-pe Oct 11, 2024

Khoyo Oct 11, 2024

bougue-pe Oct 11, 2024

bougue-pe commented Oct 11, 2024 •

edited

Loading

		WORKER_ID = System.getenv("WORKER_ID") ?: (if (ALL_INFRA) "all_infra_worker" else null)
		WORKER_KEY = System.getenv("WORKER_KEY") ?: (if (ALL_INFRA) "all" else null)

docker, core, editoast: add mode single-worker for all infra #9166

Are you sure you want to change the base?

docker, core, editoast: add mode single-worker for all infra #9166

Conversation

bougue-pe commented Oct 3, 2024 • edited Loading

codecov-commenter commented Oct 3, 2024 • edited Loading

Codecov Report

flomonster commented Oct 3, 2024

eckter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bougue-pe Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

bougue-pe commented Oct 3, 2024

younesschrifi left a comment • edited Loading

Choose a reason for hiding this comment

woshilapin left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bougue-pe Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

leovalais left a comment

Choose a reason for hiding this comment

ElysaSrc commented Oct 7, 2024

bougue-pe commented Oct 7, 2024 • edited Loading

ElysaSrc commented Oct 7, 2024

Erashin left a comment • edited Loading

Choose a reason for hiding this comment

Khoyo left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bougue-pe commented Oct 11, 2024 • edited Loading

bougue-pe commented Oct 3, 2024 •

edited

Loading

codecov-commenter commented Oct 3, 2024 •

edited

Loading

bougue-pe Oct 3, 2024 •

edited

Loading

younesschrifi left a comment •

edited

Loading

woshilapin left a comment •

edited

Loading

bougue-pe Oct 11, 2024 •

edited

Loading

bougue-pe commented Oct 7, 2024 •

edited

Loading

Erashin left a comment •

edited

Loading

Khoyo left a comment •

edited

Loading

bougue-pe commented Oct 11, 2024 •

edited

Loading